Groups > linux.kernel > #1670415 > unrolled thread

[PATCH v9 2/3] PCI: Add tango PCIe host bridge support

Started by	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
First post	2017-06-20 10:30 +0200
Last post	2017-07-04 17:20 +0200
Articles	20 on this page of 27 — 10 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-20 10:30 +0200
    Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-03 01:20 +0200
      Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-03 11:40 +0200
        Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-03 15:30 +0200
          Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Jisheng Zhang <jszhang@marvell.com> - 2017-07-04 09:10 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Jisheng Zhang <jszhang@marvell.com> - 2017-07-04 09:30 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-04 10:10 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Jisheng Zhang <jszhang@marvell.com> - 2017-07-04 10:30 +0200
                Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-04 11:40 +0200
                  Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Joao Pinto <Joao.Pinto@synopsys.com> - 2017-07-05 16:00 +0200
      Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-07-03 12:00 +0200
        Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-03 15:50 +0200
          Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-07-03 16:40 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-04 18:00 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-07-05 01:50 +0200
          Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-07-03 20:20 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-03 20:50 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-04 17:20 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-07-04 20:20 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-07-05 02:10 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-05 07:30 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Mark Brown <broonie@kernel.org> - 2017-07-05 14:40 +0200
        Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-07-03 17:40 +0200
          Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Peter Zijlstra <peterz@infradead.org> - 2017-07-04 09:20 +0200
            Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-07-04 15:10 +0200
              Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Peter Zijlstra <peterz@infradead.org> - 2017-07-04 16:30 +0200
                Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-07-04 17:20 +0200

Page 1 of 2 [1] 2 Next page →

#1670415 — [PATCH v9 2/3] PCI: Add tango PCIe host bridge support

From	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Date	2017-06-20 10:30 +0200
Subject	[PATCH v9 2/3] PCI: Add tango PCIe host bridge support
Message-ID	<tUmHV-6aA-27@gated-at.bofh.it>

This driver is required to work around several hardware bugs
in the PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/pci/host/Kconfig      |   8 +++
 drivers/pci/host/Makefile     |   1 +
 drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h       |   2 +
 4 files changed, 175 insertions(+)
 create mode 100644 drivers/pci/host/pcie-tango.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..5183d9095c3a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
 	  There is 1 internal PCIe port available to support GEN2 with
 	  4 slots.
 
+config PCIE_TANGO
+	bool "Tango PCIe controller"
+	depends on ARCH_TANGO && PCI_MSI && OF
+	select PCI_HOST_COMMON
+	help
+	  Say Y here to enable PCIe controller support for Sigma Designs
+	  Tango systems, such as SMP8759 and later chips.
+
 config VMD
 	depends on PCI_MSI && X86_64
 	tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
new file mode 100644
index 000000000000..67aaadcc1c5e
--- /dev/null
+++ b/drivers/pci/host/pcie-tango.c
@@ -0,0 +1,164 @@
+#include <linux/pci-ecam.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+
+#define MSI_MAX 256
+
+#define SMP8759_MUX		0x48
+#define SMP8759_TEST_OUT	0x74
+
+struct tango_pcie {
+	void __iomem *mux;
+};
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 *val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	/*
+	 * QUIRK #1
+	 * Reads in configuration space outside devfn 0 return garbage.
+	 */
+	if (devfn != 0)
+		return PCIBIOS_FUNC_NOT_SUPPORTED;
+
+	/*
+	 * QUIRK #2
+	 * Unfortunately, config and mem spaces are muxed.
+	 * Linux does not support such a setting, since drivers are free
+	 * to access mem space directly, at any time.
+	 * Therefore, we can only PRAY that config and mem space accesses
+	 * NEVER occur concurrently.
+	 */
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_read(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_write(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= smp8759_config_read,
+		.write		= smp8759_config_write,
+	}
+};
+
+static const struct of_device_id tango_pcie_ids[] = {
+	{ .compatible = "sigma,smp8759-pcie" },
+	{ /* sentinel */ },
+};
+
+static int tango_check_pcie_link(void __iomem *test_out)
+{
+	int i;
+
+	writel_relaxed(16, test_out);
+	for (i = 0; i < 10; ++i) {
+		u32 ltssm_state = readl_relaxed(test_out) >> 8;
+		if ((ltssm_state & 0x1f) == 0xf) /* L0 */
+			return 0;
+		usleep_range(3000, 4000);
+	}
+
+	return -ENODEV;
+}
+
+static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	pcie->mux		= base + SMP8759_MUX;
+
+	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
+}
+
+static int tango_pcie_probe(struct platform_device *pdev)
+{
+	int ret = -EINVAL;
+	void __iomem *base;
+	struct resource *res;
+	struct tango_pcie *pcie;
+	struct device *dev = &pdev->dev;
+
+	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
+	pr_err("Tainting kernel... Use driver at your own risk\n");
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pcie);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
+		ret = smp8759_init(pcie, base);
+
+	if (ret)
+		return ret;
+
+	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
+}
+
+static struct platform_driver tango_pcie_driver = {
+	.probe	= tango_pcie_probe,
+	.driver	= {
+		.name = KBUILD_MODNAME,
+		.of_match_table = tango_pcie_ids,
+	},
+};
+
+builtin_platform_driver(tango_pcie_driver);
+
+/*
+ * QUIRK #3
+ * The root complex advertizes the wrong device class.
+ * Header Type 1 is for PCI-to-PCI bridges.
+ */
+static void tango_fixup_class(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_class);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_class);
+
+/*
+ * QUIRK #4
+ * The root complex exposes a "fake" BAR, which is used to filter
+ * bus-to-system accesses. Only accesses within the range defined
+ * by this BAR are forwarded to the host, others are ignored.
+ *
+ * By default, the DMA framework expects an identity mapping,
+ * and DRAM0 is mapped at 0x80000000.
+ */
+static void tango_fixup_bar(struct pci_dev *dev)
+{
+	dev->non_compliant_bars = true;
+	pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_bar);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index f020ab4079d3..b577dbe46f8f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1369,6 +1369,8 @@
 #define PCI_DEVICE_ID_TTI_HPT374	0x0008
 #define PCI_DEVICE_ID_TTI_HPT372N	0x0009	/* apparently a 372N variant? */
 
+#define PCI_VENDOR_ID_SIGMA		0x1105
+
 #define PCI_VENDOR_ID_VIA		0x1106
 #define PCI_DEVICE_ID_VIA_8763_0	0x0198
 #define PCI_DEVICE_ID_VIA_8380_0	0x0204
-- 
2.11.0

[toc] | [next] | [standalone]

#1679580

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2017-07-03 01:20 +0200
Message-ID	<tYWjL-H8-3@gated-at.bofh.it>
In reply to	#1670415

On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs
> in the PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.

I had to apply these manually because of conflicts in Kconfig and
Makefile.  What are these based on?  Easiest for me is if you base
them on the current -rc1 tag.

> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/pci/host/Kconfig      |   8 +++
>  drivers/pci/host/Makefile     |   1 +
>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h       |   2 +
>  4 files changed, 175 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-tango.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index d7e7c0a827c3..5183d9095c3a 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
>  	  There is 1 internal PCIe port available to support GEN2 with
>  	  4 slots.
>  
> +config PCIE_TANGO
> +	bool "Tango PCIe controller"
> +	depends on ARCH_TANGO && PCI_MSI && OF
> +	select PCI_HOST_COMMON
> +	help
> +	  Say Y here to enable PCIe controller support for Sigma Designs
> +	  Tango systems, such as SMP8759 and later chips.
> +
>  config VMD
>  	depends on PCI_MSI && X86_64
>  	tristate "Intel Volume Management Device Driver"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 084cb4983645..fc7ea90196f3 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> +obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
>  obj-$(CONFIG_VMD) += vmd.o
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> new file mode 100644
> index 000000000000..67aaadcc1c5e
> --- /dev/null
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -0,0 +1,164 @@
> +#include <linux/pci-ecam.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +
> +#define MSI_MAX 256
> +
> +#define SMP8759_MUX		0x48
> +#define SMP8759_TEST_OUT	0x74
> +
> +struct tango_pcie {
> +	void __iomem *mux;
> +};
> +
> +/*** HOST BRIDGE SUPPORT ***/
> +
> +static int smp8759_config_read(struct pci_bus *bus,
> +		unsigned int devfn, int where, int size, u32 *val)

Wrap these with as much as possible on the first line, e.g.,

  static int smp8759_config_read(struct pci_bus *bus, unsigned int devfn,
                                 int where, int size, u32 *val)


> +{
> +	int ret;
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);

Reorder as:

        struct pci_config_window *cfg = bus->sysdata;
        struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
        int ret;

> +
> +	/*
> +	 * QUIRK #1

Omit "QUIRK #1", "QUIRK #2", etc unless they're actual references to an errata
document.

> +	 * Reads in configuration space outside devfn 0 return garbage.
> +	 */
> +	if (devfn != 0)
> +		return PCIBIOS_FUNC_NOT_SUPPORTED;
> +
> +	/*
> +	 * QUIRK #2
> +	 * Unfortunately, config and mem spaces are muxed.
> +	 * Linux does not support such a setting, since drivers are free
> +	 * to access mem space directly, at any time.
> +	 * Therefore, we can only PRAY that config and mem space accesses
> +	 * NEVER occur concurrently.
> +	 */
> +	writel_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
> +	writel_relaxed(0, pcie->mux);

I'm very hesitant about this.  When people stress this, we're going to
get reports of data corruption.  Even with the disclaimer below, I
don't feel good about this.  Adding the driver is an implicit claim
that we support the device, but we know it can't be made reliable.

What is the benefit of adding this driver?  How many units are in the
field?  Are you hoping to have support in distros like RHEL?  Are
these running self-built kernels straight from kernel.org?  Is it
feasible for you to distribute this driver separately from the
upstream kernel?

> +	return ret;
> +}
> +
> +static int smp8759_config_write(struct pci_bus *bus,
> +		unsigned int devfn, int where, int size, u32 val)
> +{
> +	int ret;
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> +	writel_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_write(bus, devfn, where, size, val);
> +	writel_relaxed(0, pcie->mux);
> +
> +	return ret;
> +}
> +
> +static struct pci_ecam_ops smp8759_ecam_ops = {
> +	.bus_shift	= 20,
> +	.pci_ops	= {
> +		.map_bus	= pci_ecam_map_bus,
> +		.read		= smp8759_config_read,
> +		.write		= smp8759_config_write,
> +	}
> +};
> +
> +static const struct of_device_id tango_pcie_ids[] = {
> +	{ .compatible = "sigma,smp8759-pcie" },
> +	{ /* sentinel */ },
> +};

Move table down to point where it's needed.

> +static int tango_check_pcie_link(void __iomem *test_out)

I think this is checking for link up.  Rename to tango_pcie_link_up()
to follow the convention of other drivers.  Take a struct tango_pcie *
instead of an address, if possible.

> +{
> +	int i;
> +
> +	writel_relaxed(16, test_out);
> +	for (i = 0; i < 10; ++i) {
> +		u32 ltssm_state = readl_relaxed(test_out) >> 8;
> +		if ((ltssm_state & 0x1f) == 0xf) /* L0 */
> +			return 0;
> +		usleep_range(3000, 4000);
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> +	pcie->mux		= base + SMP8759_MUX;
> +
> +	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
> +}
> +
> +static int tango_pcie_probe(struct platform_device *pdev)
> +{
> +	int ret = -EINVAL;
> +	void __iomem *base;
> +	struct resource *res;
> +	struct tango_pcie *pcie;
> +	struct device *dev = &pdev->dev;

Reverse declaration order.  Then they'll be declared in order of use.

> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
> +	pr_err("Tainting kernel... Use driver at your own risk\n");

These should be dev_err().

> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> +
> +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pcie);

Minor style: move this down to the end, so we're not associating an
uninitialized drvdata structure with the pdev.  Better to wait until
it's fully initialized.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	base = devm_ioremap_resource(&pdev->dev, res);

Use "dev", not "&pdev->dev".

> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))

Not necessary, since tango_pcie_ids[] only contains
"sigma,smp8759-pcie".  If/when you need different init for different
versions, you can add something like this back, and it will be obvious
why it's needed.  Right now, there's no need for this, so it looks out
of place.

> +		ret = smp8759_init(pcie, base);
> +
> +	if (ret)
> +		return ret;
> +
> +	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
> +}
> +
> +static struct platform_driver tango_pcie_driver = {
> +	.probe	= tango_pcie_probe,
> +	.driver	= {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = tango_pcie_ids,

I think you need ".suppress_bind_attrs = true" here to prevent issues
when unbinding driver.  See
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5f40e8098fe

> +	},
> +};
> +

Remove blank line.

> +builtin_platform_driver(tango_pcie_driver);
> +
> +/*
> + * QUIRK #3
> + * The root complex advertizes the wrong device class.

s/advertizes/advertises/

> + * Header Type 1 is for PCI-to-PCI bridges.
> + */
> +static void tango_fixup_class(struct pci_dev *dev)
> +{
> +	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_class);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_class);
> +
> +/*
> + * QUIRK #4
> + * The root complex exposes a "fake" BAR, which is used to filter
> + * bus-to-system accesses. Only accesses within the range defined
> + * by this BAR are forwarded to the host, others are ignored.
> + *
> + * By default, the DMA framework expects an identity mapping,
> + * and DRAM0 is mapped at 0x80000000.
> + */
> +static void tango_fixup_bar(struct pci_dev *dev)
> +{
> +	dev->non_compliant_bars = true;
> +	pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_bar);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_bar);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index f020ab4079d3..b577dbe46f8f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1369,6 +1369,8 @@
>  #define PCI_DEVICE_ID_TTI_HPT374	0x0008
>  #define PCI_DEVICE_ID_TTI_HPT372N	0x0009	/* apparently a 372N variant? */
>  
> +#define PCI_VENDOR_ID_SIGMA		0x1105
> +
>  #define PCI_VENDOR_ID_VIA		0x1106
>  #define PCI_DEVICE_ID_VIA_8763_0	0x0198
>  #define PCI_DEVICE_ID_VIA_8380_0	0x0204
> -- 
> 2.11.0
>

[toc] | [prev] | [next] | [standalone]

#1679837

From	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date	2017-07-03 11:40 +0200
Message-ID	<tZ5ZN-7Eb-47@gated-at.bofh.it>
In reply to	#1679580

On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
>> This driver is required to work around several hardware bugs
>> in the PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>
> I had to apply these manually because of conflicts in Kconfig and
> Makefile.  What are these based on?  Easiest for me is if you base
> them on the current -rc1 tag.
>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  drivers/pci/host/Kconfig      |   8 +++
>>  drivers/pci/host/Makefile     |   1 +
>>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci_ids.h       |   2 +
>>  4 files changed, 175 insertions(+)
>>  create mode 100644 drivers/pci/host/pcie-tango.c
>>
[..]
>> +     /*
>> +      * QUIRK #2
>> +      * Unfortunately, config and mem spaces are muxed.
>> +      * Linux does not support such a setting, since drivers are free
>> +      * to access mem space directly, at any time.
>> +      * Therefore, we can only PRAY that config and mem space accesses
>> +      * NEVER occur concurrently.
>> +      */
>> +     writel_relaxed(1, pcie->mux);
>> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
>> +     writel_relaxed(0, pcie->mux);
>
> I'm very hesitant about this.  When people stress this, we're going to
> get reports of data corruption.  Even with the disclaimer below, I
> don't feel good about this.  Adding the driver is an implicit claim
> that we support the device, but we know it can't be made reliable.
>

I noticed that the Synopsys driver suffers from a similar issue: in
dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
to perform a config space access, and switches it back to I/O space
afterwards (unless it has more than 2 viewports, in which case it uses
dedicated windows for I/O space and config space)

[toc] | [prev] | [next] | [standalone]

#1680003

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2017-07-03 15:30 +0200
Message-ID	<tZ9Am-22A-25@gated-at.bofh.it>
In reply to	#1679837

[+cc Jingoo, Joao]

On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:
> On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
> >> This driver is required to work around several hardware bugs
> >> in the PCIe controller.
> >>
> >> NB: Revision 1 does not support legacy interrupts, or IO space.
> >
> > I had to apply these manually because of conflicts in Kconfig and
> > Makefile.  What are these based on?  Easiest for me is if you base
> > them on the current -rc1 tag.
> >
> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> >> ---
> >>  drivers/pci/host/Kconfig      |   8 +++
> >>  drivers/pci/host/Makefile     |   1 +
> >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/pci_ids.h       |   2 +
> >>  4 files changed, 175 insertions(+)
> >>  create mode 100644 drivers/pci/host/pcie-tango.c
> >>
> [..]
> >> +     /*
> >> +      * QUIRK #2
> >> +      * Unfortunately, config and mem spaces are muxed.
> >> +      * Linux does not support such a setting, since drivers are free
> >> +      * to access mem space directly, at any time.
> >> +      * Therefore, we can only PRAY that config and mem space accesses
> >> +      * NEVER occur concurrently.
> >> +      */
> >> +     writel_relaxed(1, pcie->mux);
> >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> +     writel_relaxed(0, pcie->mux);
> >
> > I'm very hesitant about this.  When people stress this, we're going to
> > get reports of data corruption.  Even with the disclaimer below, I
> > don't feel good about this.  Adding the driver is an implicit claim
> > that we support the device, but we know it can't be made reliable.
> 
> I noticed that the Synopsys driver suffers from a similar issue: in
> dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
> to perform a config space access, and switches it back to I/O space
> afterwards (unless it has more than 2 viewports, in which case it uses
> dedicated windows for I/O space and config space)

That doesn't sound good.  Jingoo, Joao?  I remember some discussion
about this, but not the details.

I/O accesses use wrappers (inb(), etc), so there's at least the
possibility of a mutex to serialize them with respect to config
accesses.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1680701

From	Jisheng Zhang <jszhang@marvell.com>
Date	2017-07-04 09:10 +0200
Message-ID	<tZq89-51q-3@gated-at.bofh.it>
In reply to	#1680003

On Mon, 3 Jul 2017 08:27:04 -0500 wrote:

> [+cc Jingoo, Joao]
> 
> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:
> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:  
> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:  
> > >> This driver is required to work around several hardware bugs
> > >> in the PCIe controller.
> > >>
> > >> NB: Revision 1 does not support legacy interrupts, or IO space.  
> > >
> > > I had to apply these manually because of conflicts in Kconfig and
> > > Makefile.  What are these based on?  Easiest for me is if you base
> > > them on the current -rc1 tag.
> > >  
> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> > >> ---
> > >>  drivers/pci/host/Kconfig      |   8 +++
> > >>  drivers/pci/host/Makefile     |   1 +
> > >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
> > >>  include/linux/pci_ids.h       |   2 +
> > >>  4 files changed, 175 insertions(+)
> > >>  create mode 100644 drivers/pci/host/pcie-tango.c
> > >>  
> > [..]  
> > >> +     /*
> > >> +      * QUIRK #2
> > >> +      * Unfortunately, config and mem spaces are muxed.
> > >> +      * Linux does not support such a setting, since drivers are free
> > >> +      * to access mem space directly, at any time.
> > >> +      * Therefore, we can only PRAY that config and mem space accesses
> > >> +      * NEVER occur concurrently.
> > >> +      */
> > >> +     writel_relaxed(1, pcie->mux);
> > >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
> > >> +     writel_relaxed(0, pcie->mux);  
> > >
> > > I'm very hesitant about this.  When people stress this, we're going to
> > > get reports of data corruption.  Even with the disclaimer below, I
> > > don't feel good about this.  Adding the driver is an implicit claim
> > > that we support the device, but we know it can't be made reliable.  
> > 
> > I noticed that the Synopsys driver suffers from a similar issue: in
> > dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
> > to perform a config space access, and switches it back to I/O space
> > afterwards (unless it has more than 2 viewports, in which case it uses
> > dedicated windows for I/O space and config space)  
> 
> That doesn't sound good.  Jingoo, Joao?  I remember some discussion
> about this, but not the details.
> 
> I/O accesses use wrappers (inb(), etc), so there's at least the
> possibility of a mutex to serialize them with respect to config
> accesses.
> 

IIRC, for 2 viewports, we don't need to worry about the config space
access, because config space access is serialized by pci_lock; We
do have race between config space and io space. But the accessing config
space and io space at the same time is rare. And the PCIe EPs which
has io space are rare too, supporting these EPs are not the potential
target of those platforms with 2 viewports.

Thanks,
Jisheng

[toc] | [prev] | [next] | [standalone]

#1680717

From	Jisheng Zhang <jszhang@marvell.com>
Date	2017-07-04 09:30 +0200
Message-ID	<tZqrw-5a4-25@gated-at.bofh.it>
In reply to	#1680701

On Tue, 4 Jul 2017 14:58:40 +0800 Jisheng Zhang wrote:

> On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
> 
> > [+cc Jingoo, Joao]
> > 
> > On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:  
> > > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:    
> > > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:    
> > > >> This driver is required to work around several hardware bugs
> > > >> in the PCIe controller.
> > > >>
> > > >> NB: Revision 1 does not support legacy interrupts, or IO space.    
> > > >
> > > > I had to apply these manually because of conflicts in Kconfig and
> > > > Makefile.  What are these based on?  Easiest for me is if you base
> > > > them on the current -rc1 tag.
> > > >    
> > > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> > > >> ---
> > > >>  drivers/pci/host/Kconfig      |   8 +++
> > > >>  drivers/pci/host/Makefile     |   1 +
> > > >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
> > > >>  include/linux/pci_ids.h       |   2 +
> > > >>  4 files changed, 175 insertions(+)
> > > >>  create mode 100644 drivers/pci/host/pcie-tango.c
> > > >>    
> > > [..]    
> > > >> +     /*
> > > >> +      * QUIRK #2
> > > >> +      * Unfortunately, config and mem spaces are muxed.
> > > >> +      * Linux does not support such a setting, since drivers are free
> > > >> +      * to access mem space directly, at any time.
> > > >> +      * Therefore, we can only PRAY that config and mem space accesses
> > > >> +      * NEVER occur concurrently.
> > > >> +      */
> > > >> +     writel_relaxed(1, pcie->mux);
> > > >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
> > > >> +     writel_relaxed(0, pcie->mux);    
> > > >
> > > > I'm very hesitant about this.  When people stress this, we're going to
> > > > get reports of data corruption.  Even with the disclaimer below, I
> > > > don't feel good about this.  Adding the driver is an implicit claim
> > > > that we support the device, but we know it can't be made reliable.    
> > > 
> > > I noticed that the Synopsys driver suffers from a similar issue: in
> > > dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
> > > to perform a config space access, and switches it back to I/O space
> > > afterwards (unless it has more than 2 viewports, in which case it uses
> > > dedicated windows for I/O space and config space)    
> > 
> > That doesn't sound good.  Jingoo, Joao?  I remember some discussion
> > about this, but not the details.
> > 
> > I/O accesses use wrappers (inb(), etc), so there's at least the
> > possibility of a mutex to serialize them with respect to config
> > accesses.
> >   
> 
> IIRC, for 2 viewports, we don't need to worry about the config space
> access, because config space access is serialized by pci_lock; We
> do have race between config space and io space. But the accessing config
> space and io space at the same time is rare. And the PCIe EPs which
> has io space are rare too, supporting these EPs are not the potential
> target of those platforms with 2 viewports.
> 

PS: I think most platforms choose 2 pcie designware viewports just because
it's the default setting. And I have send a feature request to ASIC people
to increase the viewports to 3 for future marvell berlin SoCs.

[toc] | [prev] | [next] | [standalone]

#1680739

From	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date	2017-07-04 10:10 +0200
Message-ID	<tZr4f-5Hh-13@gated-at.bofh.it>
In reply to	#1680701

On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote:
> On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
>
>> [+cc Jingoo, Joao]
>>
>> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:
>> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
>> > >> This driver is required to work around several hardware bugs
>> > >> in the PCIe controller.
>> > >>
>> > >> NB: Revision 1 does not support legacy interrupts, or IO space.
>> > >
>> > > I had to apply these manually because of conflicts in Kconfig and
>> > > Makefile.  What are these based on?  Easiest for me is if you base
>> > > them on the current -rc1 tag.
>> > >
>> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> > >> ---
>> > >>  drivers/pci/host/Kconfig      |   8 +++
>> > >>  drivers/pci/host/Makefile     |   1 +
>> > >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>> > >>  include/linux/pci_ids.h       |   2 +
>> > >>  4 files changed, 175 insertions(+)
>> > >>  create mode 100644 drivers/pci/host/pcie-tango.c
>> > >>
>> > [..]
>> > >> +     /*
>> > >> +      * QUIRK #2
>> > >> +      * Unfortunately, config and mem spaces are muxed.
>> > >> +      * Linux does not support such a setting, since drivers are free
>> > >> +      * to access mem space directly, at any time.
>> > >> +      * Therefore, we can only PRAY that config and mem space accesses
>> > >> +      * NEVER occur concurrently.
>> > >> +      */
>> > >> +     writel_relaxed(1, pcie->mux);
>> > >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
>> > >> +     writel_relaxed(0, pcie->mux);
>> > >
>> > > I'm very hesitant about this.  When people stress this, we're going to
>> > > get reports of data corruption.  Even with the disclaimer below, I
>> > > don't feel good about this.  Adding the driver is an implicit claim
>> > > that we support the device, but we know it can't be made reliable.
>> >
>> > I noticed that the Synopsys driver suffers from a similar issue: in
>> > dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
>> > to perform a config space access, and switches it back to I/O space
>> > afterwards (unless it has more than 2 viewports, in which case it uses
>> > dedicated windows for I/O space and config space)
>>
>> That doesn't sound good.  Jingoo, Joao?  I remember some discussion
>> about this, but not the details.
>>
>> I/O accesses use wrappers (inb(), etc), so there's at least the
>> possibility of a mutex to serialize them with respect to config
>> accesses.
>>
>
> IIRC, for 2 viewports, we don't need to worry about the config space
> access, because config space access is serialized by pci_lock; We
> do have race between config space and io space. But the accessing config
> space and io space at the same time is rare.

Being 'rare' is not sufficient, unfortunately. In the current
situation, I/O space accesses may occur when the outbound window is
directed to the config space of a potentially completely unrelated
device. This is bad.

> And the PCIe EPs which
> has io space are rare too, supporting these EPs are not the potential
> target of those platforms with 2 viewports.
>

I am not sure EP mode is relevant here. What I do know is that boards
like the Marvell 8040 based MacchiatoBin uses this IP in RC mode, and
exposes config, MMIO and IO space windows using only 2 viewports. Note
that this is essentially a bug in the DT description, given that its
version of the IP supports 8 viewports. But the driver needs to be
fixed as well.

[toc] | [prev] | [next] | [standalone]

#1680757

From	Jisheng Zhang <jszhang@marvell.com>
Date	2017-07-04 10:30 +0200
Message-ID	<tZrnz-5Ps-7@gated-at.bofh.it>
In reply to	#1680739

On Tue, 4 Jul 2017 09:02:06 +0100 Ard Biesheuvel wrote:

> On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote:
> > On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
> >  
> >> [+cc Jingoo, Joao]
> >>
> >> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:  
> >> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:  
> >> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:  
> >> > >> This driver is required to work around several hardware bugs
> >> > >> in the PCIe controller.
> >> > >>
> >> > >> NB: Revision 1 does not support legacy interrupts, or IO space.  
> >> > >
> >> > > I had to apply these manually because of conflicts in Kconfig and
> >> > > Makefile.  What are these based on?  Easiest for me is if you base
> >> > > them on the current -rc1 tag.
> >> > >  
> >> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> >> > >> ---
> >> > >>  drivers/pci/host/Kconfig      |   8 +++
> >> > >>  drivers/pci/host/Makefile     |   1 +
> >> > >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
> >> > >>  include/linux/pci_ids.h       |   2 +
> >> > >>  4 files changed, 175 insertions(+)
> >> > >>  create mode 100644 drivers/pci/host/pcie-tango.c
> >> > >>  
> >> > [..]  
> >> > >> +     /*
> >> > >> +      * QUIRK #2
> >> > >> +      * Unfortunately, config and mem spaces are muxed.
> >> > >> +      * Linux does not support such a setting, since drivers are free
> >> > >> +      * to access mem space directly, at any time.
> >> > >> +      * Therefore, we can only PRAY that config and mem space accesses
> >> > >> +      * NEVER occur concurrently.
> >> > >> +      */
> >> > >> +     writel_relaxed(1, pcie->mux);
> >> > >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> > >> +     writel_relaxed(0, pcie->mux);  
> >> > >
> >> > > I'm very hesitant about this.  When people stress this, we're going to
> >> > > get reports of data corruption.  Even with the disclaimer below, I
> >> > > don't feel good about this.  Adding the driver is an implicit claim
> >> > > that we support the device, but we know it can't be made reliable.  
> >> >
> >> > I noticed that the Synopsys driver suffers from a similar issue: in
> >> > dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
> >> > to perform a config space access, and switches it back to I/O space
> >> > afterwards (unless it has more than 2 viewports, in which case it uses
> >> > dedicated windows for I/O space and config space)  
> >>
> >> That doesn't sound good.  Jingoo, Joao?  I remember some discussion
> >> about this, but not the details.
> >>
> >> I/O accesses use wrappers (inb(), etc), so there's at least the
> >> possibility of a mutex to serialize them with respect to config
> >> accesses.
> >>  
> >
> > IIRC, for 2 viewports, we don't need to worry about the config space
> > access, because config space access is serialized by pci_lock; We
> > do have race between config space and io space. But the accessing config
> > space and io space at the same time is rare.  
> 
> Being 'rare' is not sufficient, unfortunately. In the current
> situation, I/O space accesses may occur when the outbound window is
> directed to the config space of a potentially completely unrelated
> device. This is bad.

Yep, I agree with you.

> 
> > And the PCIe EPs which
> > has io space are rare too, supporting these EPs are not the potential
> > target of those platforms with 2 viewports.
> >  
> 
> I am not sure EP mode is relevant here. What I do know is that boards

I mean those PCIe EP cards which have IO space, but that doesn't matter.

> like the Marvell 8040 based MacchiatoBin uses this IP in RC mode, and
> exposes config, MMIO and IO space windows using only 2 viewports. Note
> that this is essentially a bug in the DT description, given that its
> version of the IP supports 8 viewports. But the driver needs to be
> fixed as well.

To fix for 2 viewports situation, we need to serialize access of the io
and config space. In internal repo, we can achieve it by modifying the
io access helper functions such as inl/outl, but this won't be accepted
by the mainline IMHO. Except fixing the HW, any elegant solution?

Suggestions are appreciated.

Thanks,
Jisheng

[toc] | [prev] | [next] | [standalone]

#1680811

From	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date	2017-07-04 11:40 +0200
Message-ID	<tZstk-6qV-25@gated-at.bofh.it>
In reply to	#1680757

On 4 July 2017 at 09:19, Jisheng Zhang <jszhang@marvell.com> wrote:
> On Tue, 4 Jul 2017 09:02:06 +0100 Ard Biesheuvel wrote:
>
>> On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote:
>> > On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
>> >
>> >> [+cc Jingoo, Joao]
>> >>
>> >> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:
>> >> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
>> >> > >> This driver is required to work around several hardware bugs
>> >> > >> in the PCIe controller.
>> >> > >>
>> >> > >> NB: Revision 1 does not support legacy interrupts, or IO space.
>> >> > >
>> >> > > I had to apply these manually because of conflicts in Kconfig and
>> >> > > Makefile.  What are these based on?  Easiest for me is if you base
>> >> > > them on the current -rc1 tag.
>> >> > >
>> >> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> >> > >> ---
>> >> > >>  drivers/pci/host/Kconfig      |   8 +++
>> >> > >>  drivers/pci/host/Makefile     |   1 +
>> >> > >>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>> >> > >>  include/linux/pci_ids.h       |   2 +
>> >> > >>  4 files changed, 175 insertions(+)
>> >> > >>  create mode 100644 drivers/pci/host/pcie-tango.c
>> >> > >>
>> >> > [..]
>> >> > >> +     /*
>> >> > >> +      * QUIRK #2
>> >> > >> +      * Unfortunately, config and mem spaces are muxed.
>> >> > >> +      * Linux does not support such a setting, since drivers are free
>> >> > >> +      * to access mem space directly, at any time.
>> >> > >> +      * Therefore, we can only PRAY that config and mem space accesses
>> >> > >> +      * NEVER occur concurrently.
>> >> > >> +      */
>> >> > >> +     writel_relaxed(1, pcie->mux);
>> >> > >> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
>> >> > >> +     writel_relaxed(0, pcie->mux);
>> >> > >
>> >> > > I'm very hesitant about this.  When people stress this, we're going to
>> >> > > get reports of data corruption.  Even with the disclaimer below, I
>> >> > > don't feel good about this.  Adding the driver is an implicit claim
>> >> > > that we support the device, but we know it can't be made reliable.
>> >> >
>> >> > I noticed that the Synopsys driver suffers from a similar issue: in
>> >> > dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
>> >> > to perform a config space access, and switches it back to I/O space
>> >> > afterwards (unless it has more than 2 viewports, in which case it uses
>> >> > dedicated windows for I/O space and config space)
>> >>
>> >> That doesn't sound good.  Jingoo, Joao?  I remember some discussion
>> >> about this, but not the details.
>> >>
>> >> I/O accesses use wrappers (inb(), etc), so there's at least the
>> >> possibility of a mutex to serialize them with respect to config
>> >> accesses.
>> >>
>> >
>> > IIRC, for 2 viewports, we don't need to worry about the config space
>> > access, because config space access is serialized by pci_lock; We
>> > do have race between config space and io space. But the accessing config
>> > space and io space at the same time is rare.
>>
>> Being 'rare' is not sufficient, unfortunately. In the current
>> situation, I/O space accesses may occur when the outbound window is
>> directed to the config space of a potentially completely unrelated
>> device. This is bad.
>
> Yep, I agree with you.
>
>>
>> > And the PCIe EPs which
>> > has io space are rare too, supporting these EPs are not the potential
>> > target of those platforms with 2 viewports.
>> >
>>
>> I am not sure EP mode is relevant here. What I do know is that boards
>
> I mean those PCIe EP cards which have IO space, but that doesn't matter.
>

Ah, ok. But if such EP cards are so rare, why expose I/O space at all
if we cannot do it safely?

>> like the Marvell 8040 based MacchiatoBin uses this IP in RC mode, and
>> exposes config, MMIO and IO space windows using only 2 viewports. Note
>> that this is essentially a bug in the DT description, given that its
>> version of the IP supports 8 viewports. But the driver needs to be
>> fixed as well.
>
> To fix for 2 viewports situation, we need to serialize access of the io
> and config space. In internal repo, we can achieve it by modifying the
> io access helper functions such as inl/outl, but this won't be accepted
> by the mainline IMHO. Except fixing the HW, any elegant solution?
>
> Suggestions are appreciated.
>

I think the safe and upstreamable approach is to disable the I/O
window completely if num-viewports <= 2.

[toc] | [prev] | [next] | [standalone]

#1681546

From	Joao Pinto <Joao.Pinto@synopsys.com>
Date	2017-07-05 16:00 +0200
Message-ID	<tZT0u-6W8-15@gated-at.bofh.it>
In reply to	#1680811

Hi to all,

Às 10:38 AM de 7/4/2017, Ard Biesheuvel escreveu:
> On 4 July 2017 at 09:19, Jisheng Zhang <jszhang@marvell.com> wrote:
>> On Tue, 4 Jul 2017 09:02:06 +0100 Ard Biesheuvel wrote:
>>
>>> On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote:
>>>> On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
>>>>
>>>>> [+cc Jingoo, Joao]
>>>>>
>>>>> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:
>>>>>> On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
>>>>>>>> This driver is required to work around several hardware bugs
>>>>>>>> in the PCIe controller.
>>>>>>>>
>>>>>>>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>>>>>>
>>>>>>> I had to apply these manually because of conflicts in Kconfig and
>>>>>>> Makefile.  What are these based on?  Easiest for me is if you base
>>>>>>> them on the current -rc1 tag.
>>>>>>>
>>>>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>>>>> ---
>>>>>>>>  drivers/pci/host/Kconfig      |   8 +++
>>>>>>>>  drivers/pci/host/Makefile     |   1 +
>>>>>>>>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  include/linux/pci_ids.h       |   2 +
>>>>>>>>  4 files changed, 175 insertions(+)
>>>>>>>>  create mode 100644 drivers/pci/host/pcie-tango.c
>>>>>>>>
>>>>>> [..]
>>>>>>>> +     /*
>>>>>>>> +      * QUIRK #2
>>>>>>>> +      * Unfortunately, config and mem spaces are muxed.
>>>>>>>> +      * Linux does not support such a setting, since drivers are free
>>>>>>>> +      * to access mem space directly, at any time.
>>>>>>>> +      * Therefore, we can only PRAY that config and mem space accesses
>>>>>>>> +      * NEVER occur concurrently.
>>>>>>>> +      */
>>>>>>>> +     writel_relaxed(1, pcie->mux);
>>>>>>>> +     ret = pci_generic_config_read(bus, devfn, where, size, val);
>>>>>>>> +     writel_relaxed(0, pcie->mux);
>>>>>>>
>>>>>>> I'm very hesitant about this.  When people stress this, we're going to
>>>>>>> get reports of data corruption.  Even with the disclaimer below, I
>>>>>>> don't feel good about this.  Adding the driver is an implicit claim
>>>>>>> that we support the device, but we know it can't be made reliable.
>>>>>>
>>>>>> I noticed that the Synopsys driver suffers from a similar issue: in
>>>>>> dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
>>>>>> to perform a config space access, and switches it back to I/O space
>>>>>> afterwards (unless it has more than 2 viewports, in which case it uses
>>>>>> dedicated windows for I/O space and config space)
>>>>>
>>>>> That doesn't sound good.  Jingoo, Joao?  I remember some discussion
>>>>> about this, but not the details.
>>>>>
>>>>> I/O accesses use wrappers (inb(), etc), so there's at least the
>>>>> possibility of a mutex to serialize them with respect to config
>>>>> accesses.
>>>>>
>>>>
>>>> IIRC, for 2 viewports, we don't need to worry about the config space
>>>> access, because config space access is serialized by pci_lock; We
>>>> do have race between config space and io space. But the accessing config
>>>> space and io space at the same time is rare.
>>>
>>> Being 'rare' is not sufficient, unfortunately. In the current
>>> situation, I/O space accesses may occur when the outbound window is
>>> directed to the config space of a potentially completely unrelated
>>> device. This is bad.
>>
>> Yep, I agree with you.
>>
>>>
>>>> And the PCIe EPs which
>>>> has io space are rare too, supporting these EPs are not the potential
>>>> target of those platforms with 2 viewports.
>>>>
>>>
>>> I am not sure EP mode is relevant here. What I do know is that boards
>>
>> I mean those PCIe EP cards which have IO space, but that doesn't matter.
>>
> 
> Ah, ok. But if such EP cards are so rare, why expose I/O space at all
> if we cannot do it safely?
> 
>>> like the Marvell 8040 based MacchiatoBin uses this IP in RC mode, and
>>> exposes config, MMIO and IO space windows using only 2 viewports. Note
>>> that this is essentially a bug in the DT description, given that its
>>> version of the IP supports 8 viewports. But the driver needs to be
>>> fixed as well.
>>
>> To fix for 2 viewports situation, we need to serialize access of the io
>> and config space. In internal repo, we can achieve it by modifying the
>> io access helper functions such as inl/outl, but this won't be accepted
>> by the mainline IMHO. Except fixing the HW, any elegant solution?
>>
>> Suggestions are appreciated.
>>
> 
> I think the safe and upstreamable approach is to disable the I/O
> window completely if num-viewports <= 2.
> 

I think that the critical case is when we are using a single ATU region, and
that has to managed by software. The hardware is not capable of managing racing
conditions, like programming the ATU while still transfering data, so problems
can ocurre for sure. In this case we should be able to add to the driver a
simple lock to signal that the ATU is being used. If the ATU was being used we
could make a spinlock for example. Would this acceptable in your opinion? The
problem is when we are transfering data. Are we able to know that data is being
processed?

When using a single region, when we access the config mem, we have to stop all
data transfers, reprogram the ATU to handle the config access, do the config
read/write, put the region "back" and restart the data transfer.

When using 2 or more regions, we can separate the scopes and the data transfers
does not need to be stopped. The race problem is harder to ocurre.

Thanks,
Joao

[toc] | [prev] | [next] | [standalone]

#1679849

From	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Date	2017-07-03 12:00 +0200
Message-ID	<tZ6j7-7MN-11@gated-at.bofh.it>
In reply to	#1679580

Hello Bjorn,

On 03/07/2017 01:18, Bjorn Helgaas wrote:

> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
> 
>> This driver is required to work around several hardware bugs
>> in the PCIe controller.
>
> [ Snip style issues ]

I wish you had pointed these out in your May 23 review, or in
my March 29 version (or even just alluded to them). I would
have fixed them in time for 2.13.

>> +	/*
>> +	 * QUIRK #2
>> +	 * Unfortunately, config and mem spaces are muxed.
>> +	 * Linux does not support such a setting, since drivers are free
>> +	 * to access mem space directly, at any time.
>> +	 * Therefore, we can only PRAY that config and mem space accesses
>> +	 * NEVER occur concurrently.
>> +	 */
>> +	writel_relaxed(1, pcie->mux);
>> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
>> +	writel_relaxed(0, pcie->mux);
> 
> I'm very hesitant about this.  When people stress this, we're going to
> get reports of data corruption.  Even with the disclaimer below, I
> don't feel good about this.  Adding the driver is an implicit claim
> that we support the device, but we know it can't be made reliable.

I can't say I didn't see this coming (I had taken your long silence
as a sign of your reluctance) but back in May, I thought you implied
that a warning + tainting the kernel would be sufficient.

Mark Rutland points out stop_machine. I will test this solution.
Would you find that acceptable?

> What is the benefit of adding this driver?  How many units are in the
> field?  Are you hoping to have support in distros like RHEL?  Are
> these running self-built kernels straight from kernel.org?  Is it
> feasible for you to distribute this driver separately from the
> upstream kernel?

The benefit of upstreaming (for me) is making kernel maintainers
aware that one is using specific internal APIs. Then one may be
notified when these APIs are about to change.

I'm told we have sold ~100k units. Though I don't know how many
are in the field and using PCIe.

There are no plans to use "full-blown" distros, we use embedded
oriented distros, such as buildroot.

Maintaining out-of-tree drivers is what we've been doing for
~15 years, and there are many pain-points involved. Ask Greg
what he thinks of OOT drivers.

>> +static int tango_check_pcie_link(void __iomem *test_out)
> 
> I think this is checking for link up.  Rename to tango_pcie_link_up()
> to follow the convention of other drivers.  Take a struct tango_pcie *
> instead of an address, if possible.

Anything's possible. NB: if I pass the struct, then I have to store
the address in the struct, which isn't the case now, since I never
need the address later. If you don't mind adding an unnecessary
field to the struct, I can do it. What do you say?

>> +static struct platform_driver tango_pcie_driver = {
>> +	.probe	= tango_pcie_probe,
>> +	.driver	= {
>> +		.name = KBUILD_MODNAME,
>> +		.of_match_table = tango_pcie_ids,
> 
> I think you need ".suppress_bind_attrs = true" here to prevent issues
> when unbinding driver.  See
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5f40e8098fe

Will do. In that case, I can drop tango_pcie_remove() right?

Regards.

[toc] | [prev] | [next] | [standalone]

#1680116

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2017-07-03 15:50 +0200
Message-ID	<tZ9TM-2c1-129@gated-at.bofh.it>
In reply to	#1679849

On Mon, Jul 03, 2017 at 11:54:20AM +0200, Marc Gonzalez wrote:
> Hello Bjorn,
> 
> On 03/07/2017 01:18, Bjorn Helgaas wrote:
> 
> > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
> > 
> >> This driver is required to work around several hardware bugs
> >> in the PCIe controller.
> >
> > [ Snip style issues ]
> 
> I wish you had pointed these out in your May 23 review, or in
> my March 29 version (or even just alluded to them). I would
> have fixed them in time for 2.13.

They're trivial, and if they were the only issues I would have just
done them myself while merging.

> >> +	/*
> >> +	 * QUIRK #2
> >> +	 * Unfortunately, config and mem spaces are muxed.
> >> +	 * Linux does not support such a setting, since drivers are free
> >> +	 * to access mem space directly, at any time.
> >> +	 * Therefore, we can only PRAY that config and mem space accesses
> >> +	 * NEVER occur concurrently.
> >> +	 */
> >> +	writel_relaxed(1, pcie->mux);
> >> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> +	writel_relaxed(0, pcie->mux);
> > 
> > I'm very hesitant about this.  When people stress this, we're going to
> > get reports of data corruption.  Even with the disclaimer below, I
> > don't feel good about this.  Adding the driver is an implicit claim
> > that we support the device, but we know it can't be made reliable.
> 
> I can't say I didn't see this coming (I had taken your long silence
> as a sign of your reluctance) but back in May, I thought you implied
> that a warning + tainting the kernel would be sufficient.
> 
> Mark Rutland points out stop_machine. I will test this solution.
> Would you find that acceptable?

Sounds possible, though I can't say for sure without seeing the code.

The problem is serializing vs. memory accesses, since they don't use
any wrappers.  However, they are ioremapped(), so it's at least
conceivable that another solution would be to use VM to trap those
accesses.  I'm not a VM person, so I don't know whether that's
feasible in Linux.

> > What is the benefit of adding this driver?  How many units are in the
> > field?  Are you hoping to have support in distros like RHEL?  Are
> > these running self-built kernels straight from kernel.org?  Is it
> > feasible for you to distribute this driver separately from the
> > upstream kernel?
> 
> The benefit of upstreaming (for me) is making kernel maintainers
> aware that one is using specific internal APIs. Then one may be
> notified when these APIs are about to change.
> 
> I'm told we have sold ~100k units. Though I don't know how many
> are in the field and using PCIe.
> 
> There are no plans to use "full-blown" distros, we use embedded
> oriented distros, such as buildroot.
> 
> Maintaining out-of-tree drivers is what we've been doing for
> ~15 years, and there are many pain-points involved. Ask Greg
> what he thinks of OOT drivers.
> 
> 
> >> +static int tango_check_pcie_link(void __iomem *test_out)
> > 
> > I think this is checking for link up.  Rename to tango_pcie_link_up()
> > to follow the convention of other drivers.  Take a struct tango_pcie *
> > instead of an address, if possible.
> 
> Anything's possible. NB: if I pass the struct, then I have to store
> the address in the struct, which isn't the case now, since I never
> need the address later. If you don't mind adding an unnecessary
> field to the struct, I can do it. What do you say?

The benefit of following the same formula as other drivers is pretty
large.  Most drivers save the equivalent of "base" in the struct.  If
you did that, you wouldn't need an extra pointer; you would just use
"base + SMP8759_MUX" in the config accessors and "base +
SMP8759_TEST_OUT" in tango_pcie_link_up().

> >> +static struct platform_driver tango_pcie_driver = {
> >> +	.probe	= tango_pcie_probe,
> >> +	.driver	= {
> >> +		.name = KBUILD_MODNAME,
> >> +		.of_match_table = tango_pcie_ids,
> > 
> > I think you need ".suppress_bind_attrs = true" here to prevent issues
> > when unbinding driver.  See
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5f40e8098fe
> 
> Will do. In that case, I can drop tango_pcie_remove() right?

I think so; I don't think there would be any way to remove the driver.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1680303

From	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Date	2017-07-03 16:40 +0200
Message-ID	<tZaG9-2L5-89@gated-at.bofh.it>
In reply to	#1680116

Bjorn Helgaas wrote:

> Marc Gonzalez wrote:
>
>> On 03/07/2017 01:18, Bjorn Helgaas wrote:
>>
>>> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
>>>
>>>> +static int tango_check_pcie_link(void __iomem *test_out)
>>>
>>> I think this is checking for link up.  Rename to tango_pcie_link_up()
>>> to follow the convention of other drivers.  Take a struct tango_pcie *
>>> instead of an address, if possible.
>>
>> Anything's possible. NB: if I pass the struct, then I have to store
>> the address in the struct, which isn't the case now, since I never
>> need the address later. If you don't mind adding an unnecessary
>> field to the struct, I can do it. What do you say?
> 
> The benefit of following the same formula as other drivers is pretty
> large.  Most drivers save the equivalent of "base" in the struct.  If
> you did that, you wouldn't need an extra pointer; you would just use
> "base + SMP8759_MUX" in the config accessors and "base + SMP8759_TEST_OUT"
> in tango_pcie_link_up().

The problem is that TEST_OUT is at 0x74 on SMP8759, but at 0x138
on my other chip. In fact, all registers have been "reshuffled",
and none have the same offsets on the two chips.

My solution was to define specific registers in the struct.

In my [RFC PATCH v0.2] posted March 23, I tried illustrating
the issue:

+static const struct of_device_id tango_pcie_ids[] = {
+	{ .compatible = "sigma,smp8759-pcie" },
+	{ .compatible = "sigma,rev2-pcie" },
+	{ /* sentinel */ },
+};
+
+static void smp8759_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	pcie->mux		= base + 0x48;
+	pcie->msi_status	= base + 0x80;
+	pcie->msi_mask		= base + 0xa0;
+	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
+}
+
+static void rev2_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	void __iomem *misc_irq	= base + 0x40;
+	void __iomem *doorbell	= base + 0x8c;
+
+	pcie->mux		= base + 0x2c;
+	pcie->msi_status	= base + 0x4c;
+	pcie->msi_mask		= base + 0x6c;
+	pcie->msi_doorbell	= 0x80000000;
+
+	writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0);
+	writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4);
+
+	/* Enable legacy PCI interrupts */
+	writel(BIT(15), misc_irq);
+	writel(0xf << 4, misc_irq + 4);
+}


Do you agree that the 'base + OFFSET' idiom does not work in
this specific situation? Would you handle it differently?

Regards.

[toc] | [prev] | [next] | [standalone]

#1681072

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2017-07-04 18:00 +0200
Message-ID	<tZyp3-1UF-15@gated-at.bofh.it>
In reply to	#1680303

On Mon, Jul 03, 2017 at 04:34:29PM +0200, Marc Gonzalez wrote:
> Bjorn Helgaas wrote:
> 
> > Marc Gonzalez wrote:
> >
> >> On 03/07/2017 01:18, Bjorn Helgaas wrote:
> >>
> >>> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:
> >>>
> >>>> +static int tango_check_pcie_link(void __iomem *test_out)
> >>>
> >>> I think this is checking for link up.  Rename to tango_pcie_link_up()
> >>> to follow the convention of other drivers.  Take a struct tango_pcie *
> >>> instead of an address, if possible.
> >>
> >> Anything's possible. NB: if I pass the struct, then I have to store
> >> the address in the struct, which isn't the case now, since I never
> >> need the address later. If you don't mind adding an unnecessary
> >> field to the struct, I can do it. What do you say?
> > 
> > The benefit of following the same formula as other drivers is pretty
> > large.  Most drivers save the equivalent of "base" in the struct.  If
> > you did that, you wouldn't need an extra pointer; you would just use
> > "base + SMP8759_MUX" in the config accessors and "base + SMP8759_TEST_OUT"
> > in tango_pcie_link_up().
> 
> The problem is that TEST_OUT is at 0x74 on SMP8759, but at 0x138
> on my other chip. In fact, all registers have been "reshuffled",
> and none have the same offsets on the two chips.
> 
> My solution was to define specific registers in the struct.
> 
> In my [RFC PATCH v0.2] posted March 23, I tried illustrating
> the issue:
> 
> +static const struct of_device_id tango_pcie_ids[] = {
> +	{ .compatible = "sigma,smp8759-pcie" },
> +	{ .compatible = "sigma,rev2-pcie" },
> +	{ /* sentinel */ },
> +};
> +
> +static void smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> +	pcie->mux		= base + 0x48;
> +	pcie->msi_status	= base + 0x80;
> +	pcie->msi_mask		= base + 0xa0;
> +	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
> +}
> +
> +static void rev2_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> +	void __iomem *misc_irq	= base + 0x40;
> +	void __iomem *doorbell	= base + 0x8c;
> +
> +	pcie->mux		= base + 0x2c;
> +	pcie->msi_status	= base + 0x4c;
> +	pcie->msi_mask		= base + 0x6c;
> +	pcie->msi_doorbell	= 0x80000000;
> +
> +	writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0);
> +	writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4);
> +
> +	/* Enable legacy PCI interrupts */
> +	writel(BIT(15), misc_irq);
> +	writel(0xf << 4, misc_irq + 4);
> +}
> 
> 
> Do you agree that the 'base + OFFSET' idiom does not work in
> this specific situation? Would you handle it differently?

It's definitely a hassle to support chips with different register
layouts.  Your hardware guys are really making your life hard :)

drivers/pci/host/pcie-iproc.c is one strategy.  It has
iproc_pcie_reg_paxb[] and iproc_pcie_reg_paxb_v2[] with register
offsets for different chip versions.

It saves a table of register offsets per controller, which is similar
to saving a set of pointers per controller.  Saving the pointers as
you suggest above is marginally more storage but probably easier to
read.

If the chips are fundamentally different, i.e., if they *operate*
differently in addition to having a different register layout, you
could make two separate drivers.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1681227

From	Mason <slash.tmp@free.fr>
Date	2017-07-05 01:50 +0200
Message-ID	<tZFJV-6Hr-27@gated-at.bofh.it>
In reply to	#1681072

On 04/07/2017 17:58, Bjorn Helgaas wrote:

> It's definitely a hassle to support chips with different register
> layouts.  Your hardware guys are really making your life hard :)

Now where did I put my foam bat...

> If the chips are fundamentally different, i.e., if they *operate*
> differently in addition to having a different register layout, you
> could make two separate drivers.

It's the exact same underlying IP. Revision 2 is only a
bug fix rev. IIUC, some of the fixes lead to adding a
register here, removing a register there... and I don't
think the HW dev ever considered the pain of supporting
both revs within a single driver.

This dual support explains some of the peculiarities
you noted in my submission.

Regards.

[toc] | [prev] | [next] | [standalone]

#1680520

From	Russell King - ARM Linux <linux@armlinux.org.uk>
Date	2017-07-03 20:20 +0200
Message-ID	<tZe6Z-54E-7@gated-at.bofh.it>
In reply to	#1680116

On Mon, Jul 03, 2017 at 08:40:31AM -0500, Bjorn Helgaas wrote:
> The problem is serializing vs. memory accesses, since they don't use
> any wrappers.  However, they are ioremapped(), so it's at least
> conceivable that another solution would be to use VM to trap those
> accesses.  I'm not a VM person, so I don't know whether that's
> feasible in Linux.

Bjorn,

You're forgetting that MMIO (iow, memory returned by ioremap()) must
be accessed through the appropriate accessors, and must not be
directly dereferenced in C.  (We do have buggy drivers that do that
but they are buggy, and in many cases are getting attention to fix
that.)

However, adding a spinlock into them is really not nice, because it
adds extra overhead that's only necessary for rare cases like Sigma
Designs - especially when you consider that these accessors are used
for all MMIO accesses, not just PCI.  It would effectively mean that
we end up serialising all MMIO accesses throughout the kernel when
Sigma Designs SoCs are enabled, destroying some of the SMP benefit.

I don't think we can sanely use the MMU to trap those accesses either,
that would mean sending IPIs to tell other CPUs to do something, and
waiting for them to respond - which can deadlock if we're already in
an IRQ-protected region (iirc, config accesses are made with IRQs
off.)

I don't think there's an easy solution to this problem - and I'm not
sure that stop_machine() can be made to work in this path (which
needs a process context).  I have a suspicion that the Sigma Designs
PCI implementation is just soo insane that it's never going to work
reliably in a multi-SoC kernel without introducing severe performance
issues for everyone else.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1680535

From	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date	2017-07-03 20:50 +0200
Message-ID	<tZeA2-5go-21@gated-at.bofh.it>
In reply to	#1680520

On 3 July 2017 at 19:11, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> On Mon, Jul 03, 2017 at 08:40:31AM -0500, Bjorn Helgaas wrote:
>> The problem is serializing vs. memory accesses, since they don't use
>> any wrappers.  However, they are ioremapped(), so it's at least
>> conceivable that another solution would be to use VM to trap those
>> accesses.  I'm not a VM person, so I don't know whether that's
>> feasible in Linux.
>
> Bjorn,
>
> You're forgetting that MMIO (iow, memory returned by ioremap()) must
> be accessed through the appropriate accessors, and must not be
> directly dereferenced in C.  (We do have buggy drivers that do that
> but they are buggy, and in many cases are getting attention to fix
> that.)
>
> However, adding a spinlock into them is really not nice, because it
> adds extra overhead that's only necessary for rare cases like Sigma
> Designs - especially when you consider that these accessors are used
> for all MMIO accesses, not just PCI.  It would effectively mean that
> we end up serialising all MMIO accesses throughout the kernel when
> Sigma Designs SoCs are enabled, destroying some of the SMP benefit.
>
> I don't think we can sanely use the MMU to trap those accesses either,
> that would mean sending IPIs to tell other CPUs to do something, and
> waiting for them to respond - which can deadlock if we're already in
> an IRQ-protected region (iirc, config accesses are made with IRQs
> off.)
>
> I don't think there's an easy solution to this problem - and I'm not
> sure that stop_machine() can be made to work in this path (which
> needs a process context).  I have a suspicion that the Sigma Designs
> PCI implementation is just soo insane that it's never going to work
> reliably in a multi-SoC kernel without introducing severe performance
> issues for everyone else.
>

I suppose we could perhaps use per-cpu spinlocks? That would put the
complexity in the Sigma config space accessors, i.e., to take each
lock before proceeding with reprogramming the outbound window, and
other implementations wouldn't have to care. However, I do agree with
Russell that having this complexity in the first place is hard to
justify if the only implementation that requires it is a wacky design
that needs lots of other quirks to operate somewhat sanely to begin
with.

[toc] | [prev] | [next] | [standalone]

#1681038

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2017-07-04 17:20 +0200
Message-ID	<tZxMl-1ET-13@gated-at.bofh.it>
In reply to	#1680520

On Mon, Jul 03, 2017 at 07:11:28PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 03, 2017 at 08:40:31AM -0500, Bjorn Helgaas wrote:
> > The problem is serializing vs. memory accesses, since they don't use
> > any wrappers.  However, they are ioremapped(), so it's at least
> > conceivable that another solution would be to use VM to trap those
> > accesses.  I'm not a VM person, so I don't know whether that's
> > feasible in Linux.
> 
> Bjorn,
> 
> You're forgetting that MMIO (iow, memory returned by ioremap()) must
> be accessed through the appropriate accessors, and must not be
> directly dereferenced in C.  (We do have buggy drivers that do that
> but they are buggy, and in many cases are getting attention to fix
> that.)

Oh, you're right, thank you!  I guess you're referring to readb()
and friends.  I haven't found an actual prohibition on directly
dereferencing addresses returned from ioremap(), but
Documentation/driver-api/device-io.rst is clear that they're
suitable for passing to readb(), etc.

I recently told someone else my mistaken idea that ioremap() must
return a valid virtual address.  I wish I remembered who it was, so I
could correct that.  Documentation/DMA-API-HOWTO.txt also suggests
that ioremap() returns a virtual address -- I think I wrote that, and
maybe that virtual address reference should be tweaked a bit.

Another wrinkle is that the pci_mmap_resource() interface is exposed
via sysfs and allows direct userspace mmap of PCI MMIO resources.  In
that case, there is no accessor available.  I wonder if we need some
way to disable this mmap when readb() is non-trivial.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1681116

From	Russell King - ARM Linux <linux@armlinux.org.uk>
Date	2017-07-04 20:20 +0200
Message-ID	<tZAAx-3tZ-7@gated-at.bofh.it>
In reply to	#1681038

On Tue, Jul 04, 2017 at 10:15:02AM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 03, 2017 at 07:11:28PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 03, 2017 at 08:40:31AM -0500, Bjorn Helgaas wrote:
> > > The problem is serializing vs. memory accesses, since they don't use
> > > any wrappers.  However, they are ioremapped(), so it's at least
> > > conceivable that another solution would be to use VM to trap those
> > > accesses.  I'm not a VM person, so I don't know whether that's
> > > feasible in Linux.
> > 
> > Bjorn,
> > 
> > You're forgetting that MMIO (iow, memory returned by ioremap()) must
> > be accessed through the appropriate accessors, and must not be
> > directly dereferenced in C.  (We do have buggy drivers that do that
> > but they are buggy, and in many cases are getting attention to fix
> > that.)
> 
> Oh, you're right, thank you!  I guess you're referring to readb()
> and friends.  I haven't found an actual prohibition on directly
> dereferencing addresses returned from ioremap(), but
> Documentation/driver-api/device-io.rst is clear that they're
> suitable for passing to readb(), etc.

There was a strong suggestion years ago that what is returned from
ioremap() is a cookie that must not be dereferenced by drivers, and
that there was a suggestion that having ioremap() return the virtual
address with an offset (which read*() and friends would undo) would
be a good idea.  However, even back then, we had some cases where
drivers would directly dereference the pointer.  We have sparse today
which helps point these places out (provided drivers stay away from
__force, but unfortunately, I think we've ended up with people who
think that silencing sparse warnings with __force is more preferable
than leaving them there to point out where things are actually wrong.)

So, imho, unfortunately sparse has lost its usefulness in this regard.

> I recently told someone else my mistaken idea that ioremap() must
> return a valid virtual address.  I wish I remembered who it was, so I
> could correct that.  Documentation/DMA-API-HOWTO.txt also suggests
> that ioremap() returns a virtual address -- I think I wrote that, and
> maybe that virtual address reference should be tweaked a bit.

For most implementations, ioremap() does indeed return a virtual address,
but that was never how the API was defined in the first place - it was
always referred to as returning a cookie.

> Another wrinkle is that the pci_mmap_resource() interface is exposed
> via sysfs and allows direct userspace mmap of PCI MMIO resources.  In
> that case, there is no accessor available.  I wonder if we need some
> way to disable this mmap when readb() is non-trivial.

Hmm, no comment, except that while the PCI MMIO space is available to
userspace, and userspace is capable of running that thread on any CPU,
PCI MMIO space can't be switched to config space.

That's another nail in this coffin...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1681231

From	Mason <slash.tmp@free.fr>
Date	2017-07-05 02:10 +0200
Message-ID	<tZG3f-74v-7@gated-at.bofh.it>
In reply to	#1680520

On 03/07/2017 20:11, Russell King - ARM Linux wrote:

> I don't think there's an easy solution to this problem - and I'm not
> sure that stop_machine() can be made to work in this path (which
> needs a process context).  I have a suspicion that the Sigma Designs
> PCI implementation is just soo insane that it's never going to work
> reliably in a multi-SoC kernel without introducing severe performance
> issues for everyone else.

If I remember correctly, this is the second HW block from
tango that has been deemed "too insane for Linux".

The first one was the DMA engine, which doesn't interrupt
when a transfer is done, but when a new transfer may be
programmed. (Though there is a simple work-around for
this one, if we give up command pipelining.)

Do larger SoC vendors have HW devs working closely with
Linux devs, to avoid these design bloopers?

Regards.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2 [1] 2 Next page →

csiph-web

[PATCH v9 2/3] PCI: Add tango PCIe host bridge support

Contents

#1670415 — [PATCH v9 2/3] PCI: Add tango PCIe host bridge support

#1679580

#1679837

#1680003

#1680701

#1680717

#1680739

#1680757

#1680811

#1681546

#1679849

#1680116

#1680303

#1681072

#1681227

#1680520

#1680535

#1681038

#1681116

#1681231