2017-07-05 09:36:32

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

On 31 May 2017 at 13:33, Marc Gonzalez <[email protected]> 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.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> 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(+)
>
[...]
> 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;
> +

Does this mean multi-function devices are not supported? E.g., on my
system I have

-[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Device 1a00
+-02.0 Advanced Micro Devices, Inc. [AMD] Device 1a01
+-02.1-[01]----00.0 Renesas Technology Corp. uPD720201 USB 3.0
\-02.2-[02]--+-00.0 NVIDIA Corporation GT218 [GeForce 210]
\-00.1 NVIDIA Corporation HD Audio Controller

where the HDMI audio is on devfn 00.1 on bus 2


> + /*
> + * 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


2017-07-05 12:18:42

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

On 05/07/2017 11:36, Ard Biesheuvel wrote:

> On 31 May 2017 at 13:33, 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.
>>
>> + /*
>> + * QUIRK #1
>> + * Reads in configuration space outside devfn 0 return garbage.
>> + */
>> + if (devfn != 0)
>> + return PCIBIOS_FUNC_NOT_SUPPORTED;
>> +
>
> Does this mean multi-function devices are not supported? E.g., on my
> system I have
>
> -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Device 1a00
> +-02.0 Advanced Micro Devices, Inc. [AMD] Device 1a01
> +-02.1-[01]----00.0 Renesas Technology Corp. uPD720201 USB 3.0
> \-02.2-[02]--+-00.0 NVIDIA Corporation GT218 [GeForce 210]
> \-00.1 NVIDIA Corporation HD Audio Controller
>
> where the HDMI audio is on devfn 00.1 on bus 2

Thanks for having a look. Here's the situation.

# lspci -v
00:00.0 PCI bridge: Sigma Designs, Inc. Device 0024 (rev 01) (prog-if 00 [Normal decode])
Flags: bus master, fast devsel, latency 0, IRQ 31
Memory at <ignored> (64-bit, non-prefetchable)
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
I/O behind bridge: 00000000-00000fff
Memory behind bridge: 00400000-004fffff
Prefetchable memory behind bridge: 00000000-000fffff
Capabilities: [50] MSI: Enable+ Count=1/4 Maskable- 64bit+
Capabilities: [78] Power Management version 3
Capabilities: [80] Express Root Port (Slot-), MSI 03
Capabilities: [100] Virtual Channel
Capabilities: [800] Advanced Error Reporting
Kernel driver in use: pcieport

01:00.0 USB controller: Renesas Technology Corp. uPD720201 USB 3.0 Host Controller (rev 03) (prog-if 30 [XHCI])
Flags: bus master, fast devsel, latency 0, IRQ 21
Memory at 50400000 (64-bit, non-prefetchable) [size=8K]
Capabilities: [50] Power Management version 3
Capabilities: [70] MSI: Enable- Count=1/8 Maskable- 64bit+
Capabilities: [90] MSI-X: Enable+ Count=8 Masked-
Capabilities: [a0] Express Endpoint, MSI 00
Capabilities: [100] Advanced Error Reporting
Capabilities: [150] Latency Tolerance Reporting
Kernel driver in use: xhci_hcd


IIUC, bus 0 will always be the PCIe host bridge.
On bus 0, reads outside of devfn 0 return garbage.
I think (?) this is not an issue, because the host bridge
is not multi-function.

There is a /separate/ erratum for bus 1. The HW returns
garbage when enumerating non-existent devfn.
IIUC, there is an OOB SoC-specific error-reporting mechanism,
so it might be possible to check for an error after every read,
and replace the garbage with -1u on error.

The errata list does not mention buses > 1 but I would assume
they are, at a minimum, affected by the "bus 1" errata --
such setups were not tested at all. (It would require some
kind of PCIe switch, I suppose.)

As a first-order approximation, I just conflated the two
errata, and ignored multi-function EPs.

NB: the "typical" use-case for the PCIe slot is adding a validated
WiFi card, because the SoC doesn't support WiFi natively.
Some customers also consider using a USB3 card, instead of the
native USB2 Chipidea controller.

Regards.