* [PATCH v3 0/2] pci: add support for firmware initialized designware RCs @ 2017-08-28 18:04 Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Ard Biesheuvel @ 2017-08-28 18:04 UTC (permalink / raw) To: linux-pci Cc: devicetree, mw, Ard Biesheuvel, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring UEFI based systems incorporating a Synopsys Designware PCIe controller in RC mode will typically configure it before entering the OS. If this configuration is fully static and ECAM compliant, there is no need to expose particulars of the device to the OS, and we can simply describe it as "pci-host-ecam-generic". However, the Synopsys IP may be synthesized in a way where a quirk is needed for config space accesses to the first bus. It makes little sense to instantiate yet another pcie-designware driver that contains all the low level setup code, but it is also not justified to add quirks handling to the generic ECAM driver. So instead, create a variant of the generic ECAM driver that filters config space accesses directed at device #1 and up on the first bus. v3: - use SoC specific compatible strings - drop MSI patch [for now], since it turns out we may not need it v2: - use dev->fwnode directly - replace an instance of pr_err with dev_err, and clarify the error message - fix Kconfig/Makefile dependency errors reported by kbuild Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Graeme Gregory <graeme.gregory@linaro.org> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Jingoo Han <jingoohan1@gmail.com> Cc: Joao Pinto <Joao.Pinto@synopsys.com> Cc: Rob Herring <robh@kernel.org> Ard Biesheuvel (2): pci: designware: add driver for DWC controller in ECAM shift mode dt-bindings: designware: add binding for Designware PCIe in ECAM mode Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 +++++++++++ drivers/pci/dwc/Kconfig | 11 +++ drivers/pci/dwc/Makefile | 1 + drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ 4 files changed, 131 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt create mode 100644 drivers/pci/dwc/pcie-designware-ecam.c -- 2.11.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-08-28 18:04 [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Ard Biesheuvel @ 2017-08-28 18:04 ` Ard Biesheuvel 2017-09-26 17:32 ` Bjorn Helgaas 2017-08-28 18:04 ` [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel 2017-08-29 15:40 ` [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Marcin Wojtas 2 siblings, 1 reply; 17+ messages in thread From: Ard Biesheuvel @ 2017-08-28 18:04 UTC (permalink / raw) To: linux-pci Cc: devicetree, mw, Ard Biesheuvel, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring Some implementations of the Synopsys Designware PCIe controller implement a so-called ECAM shift mode, which allows a static memory window to be configured that covers the configuration space of the entire bus range. If the firmware performs all the low level configuration that is required to expose this controller in a fully ECAM compatible manner, we can simply describe it as "pci-host-ecam-generic" and be done with it. However, it appears that in some cases (one of which is the Armada 80x0), the IP is synthesized with an ATU window size that does not allow the first bus to be mapped in a way that prevents the device on the downstream port from appearing more than once. So implement a driver that relies on the firmware to perform all low level initialization, and drives the controller in ECAM mode, but overrides the config space accessors to take the above quirk into account. Note that, unlike most drivers for this IP, this driver does not expose a fake bridge device at B/D/F 00:00.0. There is no point in doing so, given that this is not a true bridge, and does not require any windows to be configured in order for the downstream device to operate correctly. Omitting it also prevents the PCI resource allocation routines from handing out BAR space to it unnecessarily. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Jingoo Han <jingoohan1@gmail.com> Cc: Joao Pinto <Joao.Pinto@synopsys.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/pci/dwc/Kconfig | 11 +++ drivers/pci/dwc/Makefile | 1 + drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig index d275aadc47ee..477576d07911 100644 --- a/drivers/pci/dwc/Kconfig +++ b/drivers/pci/dwc/Kconfig @@ -169,4 +169,15 @@ config PCIE_KIRIN Say Y here if you want PCIe controller support on HiSilicon Kirin series SoCs. +config PCIE_DW_HOST_ECAM + bool "Synopsys DesignWare PCIe controller in ECAM mode" + depends on OF && PCI + select PCI_HOST_COMMON + select IRQ_DOMAIN + help + Add support for Synopsys DesignWare PCIe controllers configured + by the firmware into ECAM shift mode. In some cases, these are + fully ECAM compliant, in which case the pci-host-generic driver + may be used instead. + endmenu diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile index c61be9738cce..7d5a23e5b767 100644 --- a/drivers/pci/dwc/Makefile +++ b/drivers/pci/dwc/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o +obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),) diff --git a/drivers/pci/dwc/pcie-designware-ecam.c b/drivers/pci/dwc/pcie-designware-ecam.c new file mode 100644 index 000000000000..ede627d7d08b --- /dev/null +++ b/drivers/pci/dwc/pcie-designware-ecam.c @@ -0,0 +1,77 @@ +/* + * Driver for mostly ECAM compatible Synopsys dw PCIe controllers + * configured by the firmware into RC mode + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Copyright (C) 2014 ARM Limited + * Copyright (C) 2017 Linaro Limited + * + * Authors: Will Deacon <will.deacon@arm.com> + * Ard Biesheuvel <ard.biesheuvel@linaro.org> + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/of_address.h> +#include <linux/of_pci.h> +#include <linux/pci-ecam.h> +#include <linux/platform_device.h> + +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, + int size, u32 *val) +{ + struct pci_config_window *cfg = bus->sysdata; + + /* + * The Synopsys dw PCIe controller in RC mode will not filter type 0 + * config TLPs sent to devices 1 and up on its downstream port, + * resulting in devices appearing multiple times on bus 0 unless we + * filter them here. + */ + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { + *val = 0xffffffff; + return PCIBIOS_DEVICE_NOT_FOUND; + } + return pci_generic_config_read(bus, devfn, where, size, val); +} + +static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where, + int size, u32 val) +{ + struct pci_config_window *cfg = bus->sysdata; + + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) + return PCIBIOS_DEVICE_NOT_FOUND; + + return pci_generic_config_write(bus, devfn, where, size, val); +} + +static struct pci_ecam_ops pci_dw_ecam_bus_ops = { + .pci_ops.map_bus = pci_ecam_map_bus, + .pci_ops.read = pci_dw_ecam_config_read, + .pci_ops.write = pci_dw_ecam_config_write, + .bus_shift = 20, +}; + +static const struct of_device_id pci_dw_ecam_of_match[] = { + { .compatible = "marvell,armada8k-pcie-ecam" }, + { .compatible = "socionext,synquacer-pcie-ecam" }, + { .compatible = "snps,dw-pcie-ecam" }, + { }, +}; + +static int pci_dw_ecam_probe(struct platform_device *pdev) +{ + return pci_host_common_probe(pdev, &pci_dw_ecam_bus_ops); +} + +static struct platform_driver pci_dw_ecam_driver = { + .driver.name = "pcie-designware-ecam", + .driver.of_match_table = pci_dw_ecam_of_match, + .driver.suppress_bind_attrs = true, + .probe = pci_dw_ecam_probe, +}; +builtin_platform_driver(pci_dw_ecam_driver); -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel @ 2017-09-26 17:32 ` Bjorn Helgaas 2017-09-28 9:03 ` Will Deacon 2017-09-28 15:51 ` Ard Biesheuvel 0 siblings, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2017-09-26 17:32 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-pci, devicetree, mw, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon [+cc Will] On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > Some implementations of the Synopsys Designware PCIe controller implement > a so-called ECAM shift mode, which allows a static memory window to be > configured that covers the configuration space of the entire bus range. > > If the firmware performs all the low level configuration that is required > to expose this controller in a fully ECAM compatible manner, we can > simply describe it as "pci-host-ecam-generic" and be done with it. > However, it appears that in some cases (one of which is the Armada 80x0), > the IP is synthesized with an ATU window size that does not allow the > first bus to be mapped in a way that prevents the device on the > downstream port from appearing more than once. > > So implement a driver that relies on the firmware to perform all low > level initialization, and drives the controller in ECAM mode, but > overrides the config space accessors to take the above quirk into > account. > > Note that, unlike most drivers for this IP, this driver does not expose > a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > given that this is not a true bridge, and does not require any windows > to be configured in order for the downstream device to operate correctly. > Omitting it also prevents the PCI resource allocation routines from > handing out BAR space to it unnecessarily. This is a tangent, but does this mean the other drivers do not need to expose a fake 00:00.0 device either? s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/pci/dwc/Kconfig | 11 +++ > drivers/pci/dwc/Makefile | 1 + > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ This really doesn't have any DesignWare specifics in it, and it seems more related to drivers/pci/host/pci-host-generic.c than to anything in drivers/pci/dwc. Maybe it should be drivers/pci/host/pci-host-generic-quirks.c or something? That's unwieldy, I admit. Putting it in pci/dwc would make Jingoo and Joao the default maintainers; I don't know how they feel about that. We would probably have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. Any thoughts on this, Will? > 3 files changed, 89 insertions(+) > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig > index d275aadc47ee..477576d07911 100644 > --- a/drivers/pci/dwc/Kconfig > +++ b/drivers/pci/dwc/Kconfig > @@ -169,4 +169,15 @@ config PCIE_KIRIN > Say Y here if you want PCIe controller support > on HiSilicon Kirin series SoCs. > > +config PCIE_DW_HOST_ECAM > + bool "Synopsys DesignWare PCIe controller in ECAM mode" > + depends on OF && PCI > + select PCI_HOST_COMMON > + select IRQ_DOMAIN > + help > + Add support for Synopsys DesignWare PCIe controllers configured > + by the firmware into ECAM shift mode. In some cases, these are > + fully ECAM compliant, in which case the pci-host-generic driver > + may be used instead. This doesn't quite read right. It sounds like a controller in ECAM shift mode might be fully ECAM compliant, but I don't think that's what you intended. IIUC, the controller can be in either "ECAM shift mode" (where we need this new driver) or in a "fully ECAM compliant mode" (where we can use pci-host-generic). > endmenu > diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile > index c61be9738cce..7d5a23e5b767 100644 > --- a/drivers/pci/dwc/Makefile > +++ b/drivers/pci/dwc/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_PCIE_DW) += pcie-designware.o > obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o > +obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o > ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),) > diff --git a/drivers/pci/dwc/pcie-designware-ecam.c b/drivers/pci/dwc/pcie-designware-ecam.c > new file mode 100644 > index 000000000000..ede627d7d08b > --- /dev/null > +++ b/drivers/pci/dwc/pcie-designware-ecam.c > @@ -0,0 +1,77 @@ > +/* > + * Driver for mostly ECAM compatible Synopsys dw PCIe controllers > + * configured by the firmware into RC mode > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * Copyright (C) 2014 ARM Limited > + * Copyright (C) 2017 Linaro Limited > + * > + * Authors: Will Deacon <will.deacon@arm.com> > + * Ard Biesheuvel <ard.biesheuvel@linaro.org> > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/of_address.h> > +#include <linux/of_pci.h> > +#include <linux/pci-ecam.h> > +#include <linux/platform_device.h> > + > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, > + int size, u32 *val) > +{ > + struct pci_config_window *cfg = bus->sysdata; > + > + /* > + * The Synopsys dw PCIe controller in RC mode will not filter type 0 > + * config TLPs sent to devices 1 and up on its downstream port, > + * resulting in devices appearing multiple times on bus 0 unless we > + * filter them here. > + */ > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { Trivial, but maybe you could factor out this test? We already have these functions that do basically the same thing and it'd be nice to use a similar pattern (altera and dw also check for the link being up, which seems racy and possibly bogus to me): altera_pcie_valid_device() dw_pcie_valid_device() rockchip_pcie_valid_device() The fact that altera and rockchip do essentially the same thing as dw here suggests that this pattern is not limited to DesignWare. These other functions also do something similar, though not structured the same way: hisi_pcie_rd_conf() advk_pcie_rd_conf() thunder_pem_bridge_read() rcar_pcie_config_access() gapspci_config_access() > + *val = 0xffffffff; > + return PCIBIOS_DEVICE_NOT_FOUND; > + } > + return pci_generic_config_read(bus, devfn, where, size, val); > +} > + > +static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where, > + int size, u32 val) > +{ > + struct pci_config_window *cfg = bus->sysdata; > + > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) > + return PCIBIOS_DEVICE_NOT_FOUND; > + > + return pci_generic_config_write(bus, devfn, where, size, val); > +} > + > +static struct pci_ecam_ops pci_dw_ecam_bus_ops = { > + .pci_ops.map_bus = pci_ecam_map_bus, > + .pci_ops.read = pci_dw_ecam_config_read, > + .pci_ops.write = pci_dw_ecam_config_write, > + .bus_shift = 20, > +}; > + > +static const struct of_device_id pci_dw_ecam_of_match[] = { > + { .compatible = "marvell,armada8k-pcie-ecam" }, > + { .compatible = "socionext,synquacer-pcie-ecam" }, > + { .compatible = "snps,dw-pcie-ecam" }, > + { }, > +}; > + > +static int pci_dw_ecam_probe(struct platform_device *pdev) > +{ > + return pci_host_common_probe(pdev, &pci_dw_ecam_bus_ops); > +} > + > +static struct platform_driver pci_dw_ecam_driver = { > + .driver.name = "pcie-designware-ecam", > + .driver.of_match_table = pci_dw_ecam_of_match, > + .driver.suppress_bind_attrs = true, > + .probe = pci_dw_ecam_probe, > +}; > +builtin_platform_driver(pci_dw_ecam_driver); > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-26 17:32 ` Bjorn Helgaas @ 2017-09-28 9:03 ` Will Deacon 2017-09-28 15:57 ` Ard Biesheuvel 2017-09-28 15:51 ` Ard Biesheuvel 1 sibling, 1 reply; 17+ messages in thread From: Will Deacon @ 2017-09-28 9:03 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ard Biesheuvel, linux-pci, devicetree, mw, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring On Tue, Sep 26, 2017 at 12:32:00PM -0500, Bjorn Helgaas wrote: > [+cc Will] > > On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > > Some implementations of the Synopsys Designware PCIe controller implement > > a so-called ECAM shift mode, which allows a static memory window to be > > configured that covers the configuration space of the entire bus range. > > > > If the firmware performs all the low level configuration that is required > > to expose this controller in a fully ECAM compatible manner, we can > > simply describe it as "pci-host-ecam-generic" and be done with it. > > However, it appears that in some cases (one of which is the Armada 80x0), > > the IP is synthesized with an ATU window size that does not allow the > > first bus to be mapped in a way that prevents the device on the > > downstream port from appearing more than once. > > > > So implement a driver that relies on the firmware to perform all low > > level initialization, and drives the controller in ECAM mode, but > > overrides the config space accessors to take the above quirk into > > account. > > > > Note that, unlike most drivers for this IP, this driver does not expose > > a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > > given that this is not a true bridge, and does not require any windows > > to be configured in order for the downstream device to operate correctly. > > Omitting it also prevents the PCI resource allocation routines from > > handing out BAR space to it unnecessarily. > > This is a tangent, but does this mean the other drivers do not need to > expose a fake 00:00.0 device either? > > s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > drivers/pci/dwc/Kconfig | 11 +++ > > drivers/pci/dwc/Makefile | 1 + > > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ > > This really doesn't have any DesignWare specifics in it, and it seems > more related to drivers/pci/host/pci-host-generic.c than to anything > in drivers/pci/dwc. Maybe it should be > drivers/pci/host/pci-host-generic-quirks.c or something? That's > unwieldy, I admit. > > Putting it in pci/dwc would make Jingoo and Joao the default > maintainers; I don't know how they feel about that. We would probably > have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. > > Any thoughts on this, Will? The idea of a "generic quirk" makes me smile, I must admit :) I think there are two options: 1. Use the full DWC driver, and don't rely on firmware -or- 2. Rely on firmware, but teach pci-host-generic to deal with the funny config space For (2), we probably want to describe this as generically as possible in case some other SoCs run into the same problem. Will ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 9:03 ` Will Deacon @ 2017-09-28 15:57 ` Ard Biesheuvel 2017-09-28 16:00 ` Will Deacon 0 siblings, 1 reply; 17+ messages in thread From: Ard Biesheuvel @ 2017-09-28 15:57 UTC (permalink / raw) To: Will Deacon Cc: Bjorn Helgaas, linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring On 28 September 2017 at 02:03, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Sep 26, 2017 at 12:32:00PM -0500, Bjorn Helgaas wrote: >> [+cc Will] >> >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >> > Some implementations of the Synopsys Designware PCIe controller implement >> > a so-called ECAM shift mode, which allows a static memory window to be >> > configured that covers the configuration space of the entire bus range. >> > >> > If the firmware performs all the low level configuration that is required >> > to expose this controller in a fully ECAM compatible manner, we can >> > simply describe it as "pci-host-ecam-generic" and be done with it. >> > However, it appears that in some cases (one of which is the Armada 80x0), >> > the IP is synthesized with an ATU window size that does not allow the >> > first bus to be mapped in a way that prevents the device on the >> > downstream port from appearing more than once. >> > >> > So implement a driver that relies on the firmware to perform all low >> > level initialization, and drives the controller in ECAM mode, but >> > overrides the config space accessors to take the above quirk into >> > account. >> > >> > Note that, unlike most drivers for this IP, this driver does not expose >> > a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >> > given that this is not a true bridge, and does not require any windows >> > to be configured in order for the downstream device to operate correctly. >> > Omitting it also prevents the PCI resource allocation routines from >> > handing out BAR space to it unnecessarily. >> >> This is a tangent, but does this mean the other drivers do not need to >> expose a fake 00:00.0 device either? >> >> s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. >> >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: Jingoo Han <jingoohan1@gmail.com> >> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > --- >> > drivers/pci/dwc/Kconfig | 11 +++ >> > drivers/pci/dwc/Makefile | 1 + >> > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ >> >> This really doesn't have any DesignWare specifics in it, and it seems >> more related to drivers/pci/host/pci-host-generic.c than to anything >> in drivers/pci/dwc. Maybe it should be >> drivers/pci/host/pci-host-generic-quirks.c or something? That's >> unwieldy, I admit. >> >> Putting it in pci/dwc would make Jingoo and Joao the default >> maintainers; I don't know how they feel about that. We would probably >> have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. >> >> Any thoughts on this, Will? > > The idea of a "generic quirk" makes me smile, I must admit :) > > I think there are two options: > > 1. Use the full DWC driver, and don't rely on firmware > -or- > 2. Rely on firmware, but teach pci-host-generic to deal with the funny > config space > > For (2), we probably want to describe this as generically as possible > in case some other SoCs run into the same problem. > I take it this implies a DT property. I could add one that consists of an array of val/mask tuples or base/size tuples that allow us to disable arbitrary subregions of the config space. I could also add a simple boolean property that implements this exact quirk. Do you have any preference? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 15:57 ` Ard Biesheuvel @ 2017-09-28 16:00 ` Will Deacon 2017-09-28 16:04 ` Ard Biesheuvel 0 siblings, 1 reply; 17+ messages in thread From: Will Deacon @ 2017-09-28 16:00 UTC (permalink / raw) To: Ard Biesheuvel Cc: Bjorn Helgaas, linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring On Thu, Sep 28, 2017 at 08:57:28AM -0700, Ard Biesheuvel wrote: > On 28 September 2017 at 02:03, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Sep 26, 2017 at 12:32:00PM -0500, Bjorn Helgaas wrote: > >> [+cc Will] > >> > >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > >> > Some implementations of the Synopsys Designware PCIe controller implement > >> > a so-called ECAM shift mode, which allows a static memory window to be > >> > configured that covers the configuration space of the entire bus range. > >> > > >> > If the firmware performs all the low level configuration that is required > >> > to expose this controller in a fully ECAM compatible manner, we can > >> > simply describe it as "pci-host-ecam-generic" and be done with it. > >> > However, it appears that in some cases (one of which is the Armada 80x0), > >> > the IP is synthesized with an ATU window size that does not allow the > >> > first bus to be mapped in a way that prevents the device on the > >> > downstream port from appearing more than once. > >> > > >> > So implement a driver that relies on the firmware to perform all low > >> > level initialization, and drives the controller in ECAM mode, but > >> > overrides the config space accessors to take the above quirk into > >> > account. > >> > > >> > Note that, unlike most drivers for this IP, this driver does not expose > >> > a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > >> > given that this is not a true bridge, and does not require any windows > >> > to be configured in order for the downstream device to operate correctly. > >> > Omitting it also prevents the PCI resource allocation routines from > >> > handing out BAR space to it unnecessarily. > >> > >> This is a tangent, but does this mean the other drivers do not need to > >> expose a fake 00:00.0 device either? > >> > >> s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. > >> > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> > >> > Cc: Jingoo Han <jingoohan1@gmail.com> > >> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > --- > >> > drivers/pci/dwc/Kconfig | 11 +++ > >> > drivers/pci/dwc/Makefile | 1 + > >> > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ > >> > >> This really doesn't have any DesignWare specifics in it, and it seems > >> more related to drivers/pci/host/pci-host-generic.c than to anything > >> in drivers/pci/dwc. Maybe it should be > >> drivers/pci/host/pci-host-generic-quirks.c or something? That's > >> unwieldy, I admit. > >> > >> Putting it in pci/dwc would make Jingoo and Joao the default > >> maintainers; I don't know how they feel about that. We would probably > >> have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. > >> > >> Any thoughts on this, Will? > > > > The idea of a "generic quirk" makes me smile, I must admit :) > > > > I think there are two options: > > > > 1. Use the full DWC driver, and don't rely on firmware > > -or- > > 2. Rely on firmware, but teach pci-host-generic to deal with the funny > > config space > > > > For (2), we probably want to describe this as generically as possible > > in case some other SoCs run into the same problem. > > > > I take it this implies a DT property. I could add one that consists of > an array of val/mask tuples or base/size tuples that allow us to > disable arbitrary subregions of the config space. I could also add a > simple boolean property that implements this exact quirk. Do you have > any preference? I'd say either a boolean property or a new compatible string. I think Rob prefers the latter, from what he said recently on an SMMU thread. Will ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 16:00 ` Will Deacon @ 2017-09-28 16:04 ` Ard Biesheuvel 0 siblings, 0 replies; 17+ messages in thread From: Ard Biesheuvel @ 2017-09-28 16:04 UTC (permalink / raw) To: Will Deacon Cc: Bjorn Helgaas, linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring On 28 September 2017 at 09:00, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Sep 28, 2017 at 08:57:28AM -0700, Ard Biesheuvel wrote: >> On 28 September 2017 at 02:03, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Sep 26, 2017 at 12:32:00PM -0500, Bjorn Helgaas wrote: >> >> [+cc Will] >> >> >> >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >> >> > Some implementations of the Synopsys Designware PCIe controller implement >> >> > a so-called ECAM shift mode, which allows a static memory window to be >> >> > configured that covers the configuration space of the entire bus range. >> >> > >> >> > If the firmware performs all the low level configuration that is required >> >> > to expose this controller in a fully ECAM compatible manner, we can >> >> > simply describe it as "pci-host-ecam-generic" and be done with it. >> >> > However, it appears that in some cases (one of which is the Armada 80x0), >> >> > the IP is synthesized with an ATU window size that does not allow the >> >> > first bus to be mapped in a way that prevents the device on the >> >> > downstream port from appearing more than once. >> >> > >> >> > So implement a driver that relies on the firmware to perform all low >> >> > level initialization, and drives the controller in ECAM mode, but >> >> > overrides the config space accessors to take the above quirk into >> >> > account. >> >> > >> >> > Note that, unlike most drivers for this IP, this driver does not expose >> >> > a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >> >> > given that this is not a true bridge, and does not require any windows >> >> > to be configured in order for the downstream device to operate correctly. >> >> > Omitting it also prevents the PCI resource allocation routines from >> >> > handing out BAR space to it unnecessarily. >> >> >> >> This is a tangent, but does this mean the other drivers do not need to >> >> expose a fake 00:00.0 device either? >> >> >> >> s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. >> >> >> >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> >> > Cc: Jingoo Han <jingoohan1@gmail.com> >> >> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> >> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> > --- >> >> > drivers/pci/dwc/Kconfig | 11 +++ >> >> > drivers/pci/dwc/Makefile | 1 + >> >> > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ >> >> >> >> This really doesn't have any DesignWare specifics in it, and it seems >> >> more related to drivers/pci/host/pci-host-generic.c than to anything >> >> in drivers/pci/dwc. Maybe it should be >> >> drivers/pci/host/pci-host-generic-quirks.c or something? That's >> >> unwieldy, I admit. >> >> >> >> Putting it in pci/dwc would make Jingoo and Joao the default >> >> maintainers; I don't know how they feel about that. We would probably >> >> have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. >> >> >> >> Any thoughts on this, Will? >> > >> > The idea of a "generic quirk" makes me smile, I must admit :) >> > >> > I think there are two options: >> > >> > 1. Use the full DWC driver, and don't rely on firmware >> > -or- >> > 2. Rely on firmware, but teach pci-host-generic to deal with the funny >> > config space >> > >> > For (2), we probably want to describe this as generically as possible >> > in case some other SoCs run into the same problem. >> > >> >> I take it this implies a DT property. I could add one that consists of >> an array of val/mask tuples or base/size tuples that allow us to >> disable arbitrary subregions of the config space. I could also add a >> simple boolean property that implements this exact quirk. Do you have >> any preference? > > I'd say either a boolean property or a new compatible string. I think Rob > prefers the latter, from what he said recently on an SMMU thread. > OK. Given that Rob already acked the binding for this driver, I'll go ahead and rework the patch to add + { .compatible = "marvell,armada8k-pcie-ecam" }, + { .compatible = "socionext,synquacer-pcie-ecam" }, + { .compatible = "snps,dw-pcie-ecam" }, to the pci-host-ecam-generic driver instead. Thanks, Ard. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-26 17:32 ` Bjorn Helgaas 2017-09-28 9:03 ` Will Deacon @ 2017-09-28 15:51 ` Ard Biesheuvel 2017-09-28 17:48 ` Bjorn Helgaas 2017-10-06 14:52 ` Ard Biesheuvel 1 sibling, 2 replies; 17+ messages in thread From: Ard Biesheuvel @ 2017-09-28 15:51 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Will] > > On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >> Some implementations of the Synopsys Designware PCIe controller implement >> a so-called ECAM shift mode, which allows a static memory window to be >> configured that covers the configuration space of the entire bus range. >> >> If the firmware performs all the low level configuration that is required >> to expose this controller in a fully ECAM compatible manner, we can >> simply describe it as "pci-host-ecam-generic" and be done with it. >> However, it appears that in some cases (one of which is the Armada 80x0), >> the IP is synthesized with an ATU window size that does not allow the >> first bus to be mapped in a way that prevents the device on the >> downstream port from appearing more than once. >> >> So implement a driver that relies on the firmware to perform all low >> level initialization, and drives the controller in ECAM mode, but >> overrides the config space accessors to take the above quirk into >> account. >> >> Note that, unlike most drivers for this IP, this driver does not expose >> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >> given that this is not a true bridge, and does not require any windows >> to be configured in order for the downstream device to operate correctly. >> Omitting it also prevents the PCI resource allocation routines from >> handing out BAR space to it unnecessarily. > > This is a tangent, but does this mean the other drivers do not need to > expose a fake 00:00.0 device either? > To be honest, I am not so sure anymore. I am seeing some issues in ASPM code making the assumption that any device which is not a root port has a parent. If this is mandated by the spec, I guess there isn't a whole lot we can do except expose a fake root port on b/d/f 0/0/0. This used to work fine, though, and I have to confirm whether the issues I am seeing currently are due to different hardware or changes in the software. > s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. > OK >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: Jingoo Han <jingoohan1@gmail.com> >> Cc: Joao Pinto <Joao.Pinto@synopsys.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/pci/dwc/Kconfig | 11 +++ >> drivers/pci/dwc/Makefile | 1 + >> drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ > > This really doesn't have any DesignWare specifics in it, and it seems > more related to drivers/pci/host/pci-host-generic.c than to anything > in drivers/pci/dwc. Maybe it should be > drivers/pci/host/pci-host-generic-quirks.c or something? That's > unwieldy, I admit. > I don't care where we put it, and I am fine with owning it if you prefer. > Putting it in pci/dwc would make Jingoo and Joao the default > maintainers; I don't know how they feel about that. We would probably > have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. > > Any thoughts on this, Will? > >> 3 files changed, 89 insertions(+) >> >> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig >> index d275aadc47ee..477576d07911 100644 >> --- a/drivers/pci/dwc/Kconfig >> +++ b/drivers/pci/dwc/Kconfig >> @@ -169,4 +169,15 @@ config PCIE_KIRIN >> Say Y here if you want PCIe controller support >> on HiSilicon Kirin series SoCs. >> >> +config PCIE_DW_HOST_ECAM >> + bool "Synopsys DesignWare PCIe controller in ECAM mode" >> + depends on OF && PCI >> + select PCI_HOST_COMMON >> + select IRQ_DOMAIN >> + help >> + Add support for Synopsys DesignWare PCIe controllers configured >> + by the firmware into ECAM shift mode. In some cases, these are >> + fully ECAM compliant, in which case the pci-host-generic driver >> + may be used instead. > > This doesn't quite read right. It sounds like a controller in ECAM > shift mode might be fully ECAM compliant, but I don't think that's > what you intended. > Yes, that is what I mean. ECAM shift mode results in a fully compliant ECAM config space iff the IP was synthesized with a 32 KB granularity for the iATU windows. The default is 64 KB, though, in which case you need this driver. > IIUC, the controller can be in either "ECAM shift mode" (where we need > this new driver) or in a "fully ECAM compliant mode" (where we can use > pci-host-generic). > No, this is not the case >> endmenu >> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile >> index c61be9738cce..7d5a23e5b767 100644 >> --- a/drivers/pci/dwc/Makefile >> +++ b/drivers/pci/dwc/Makefile >> @@ -1,5 +1,6 @@ >> obj-$(CONFIG_PCIE_DW) += pcie-designware.o >> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o >> +obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o >> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o >> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o >> ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),) >> diff --git a/drivers/pci/dwc/pcie-designware-ecam.c b/drivers/pci/dwc/pcie-designware-ecam.c >> new file mode 100644 >> index 000000000000..ede627d7d08b >> --- /dev/null >> +++ b/drivers/pci/dwc/pcie-designware-ecam.c >> @@ -0,0 +1,77 @@ >> +/* >> + * Driver for mostly ECAM compatible Synopsys dw PCIe controllers >> + * configured by the firmware into RC mode >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * Copyright (C) 2014 ARM Limited >> + * Copyright (C) 2017 Linaro Limited >> + * >> + * Authors: Will Deacon <will.deacon@arm.com> >> + * Ard Biesheuvel <ard.biesheuvel@linaro.org> >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/init.h> >> +#include <linux/of_address.h> >> +#include <linux/of_pci.h> >> +#include <linux/pci-ecam.h> >> +#include <linux/platform_device.h> >> + >> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, >> + int size, u32 *val) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + >> + /* >> + * The Synopsys dw PCIe controller in RC mode will not filter type 0 >> + * config TLPs sent to devices 1 and up on its downstream port, >> + * resulting in devices appearing multiple times on bus 0 unless we >> + * filter them here. >> + */ >> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { > > Trivial, but maybe you could factor out this test? We already have > these functions that do basically the same thing and it'd be nice to > use a similar pattern (altera and dw also check for the link being up, > which seems racy and possibly bogus to me): > > altera_pcie_valid_device() > dw_pcie_valid_device() > rockchip_pcie_valid_device() > > The fact that altera and rockchip do essentially the same thing as dw > here suggests that this pattern is not limited to DesignWare. > > These other functions also do something similar, though not structured > the same way: > > hisi_pcie_rd_conf() > advk_pcie_rd_conf() > thunder_pem_bridge_read() > rcar_pcie_config_access() > gapspci_config_access() > I can look into that. >> + *val = 0xffffffff; >> + return PCIBIOS_DEVICE_NOT_FOUND; >> + } >> + return pci_generic_config_read(bus, devfn, where, size, val); >> +} >> + >> +static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where, >> + int size, u32 val) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + >> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) >> + return PCIBIOS_DEVICE_NOT_FOUND; >> + >> + return pci_generic_config_write(bus, devfn, where, size, val); >> +} >> + >> +static struct pci_ecam_ops pci_dw_ecam_bus_ops = { >> + .pci_ops.map_bus = pci_ecam_map_bus, >> + .pci_ops.read = pci_dw_ecam_config_read, >> + .pci_ops.write = pci_dw_ecam_config_write, >> + .bus_shift = 20, >> +}; >> + >> +static const struct of_device_id pci_dw_ecam_of_match[] = { >> + { .compatible = "marvell,armada8k-pcie-ecam" }, >> + { .compatible = "socionext,synquacer-pcie-ecam" }, >> + { .compatible = "snps,dw-pcie-ecam" }, >> + { }, >> +}; >> + >> +static int pci_dw_ecam_probe(struct platform_device *pdev) >> +{ >> + return pci_host_common_probe(pdev, &pci_dw_ecam_bus_ops); >> +} >> + >> +static struct platform_driver pci_dw_ecam_driver = { >> + .driver.name = "pcie-designware-ecam", >> + .driver.of_match_table = pci_dw_ecam_of_match, >> + .driver.suppress_bind_attrs = true, >> + .probe = pci_dw_ecam_probe, >> +}; >> +builtin_platform_driver(pci_dw_ecam_driver); >> -- >> 2.11.0 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 15:51 ` Ard Biesheuvel @ 2017-09-28 17:48 ` Bjorn Helgaas 2017-09-28 18:33 ` Ard Biesheuvel 2017-09-29 3:29 ` Jingoo Han 2017-10-06 14:52 ` Ard Biesheuvel 1 sibling, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2017-09-28 17:48 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On Thu, Sep 28, 2017 at 08:51:43AM -0700, Ard Biesheuvel wrote: > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > >> Some implementations of the Synopsys Designware PCIe controller implement > >> a so-called ECAM shift mode, which allows a static memory window to be > >> configured that covers the configuration space of the entire bus range. > >> Note that, unlike most drivers for this IP, this driver does not expose > >> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > >> given that this is not a true bridge, and does not require any windows > >> to be configured in order for the downstream device to operate correctly. > >> Omitting it also prevents the PCI resource allocation routines from > >> handing out BAR space to it unnecessarily. > > > > This is a tangent, but does this mean the other drivers do not need to > > expose a fake 00:00.0 device either? > > To be honest, I am not so sure anymore. I am seeing some issues in > ASPM code making the assumption that any device which is not a root > port has a parent. If this is mandated by the spec, I guess there > isn't a whole lot we can do except expose a fake root port on b/d/f > 0/0/0. This used to work fine, though, and I have to confirm whether > the issues I am seeing currently are due to different hardware or > changes in the software. I agree that our ASPM code had some assumptions that any non-root port device should have a parent. I think the spec also makes that assumption, but I haven't found an explicit mandate. And there *are* systems lacking root ports: http://lkml.kernel.org/r/1439808478-23253-1-git-send-email-wangyijing@huawei.com We fixed one such assumption in that thread, but I wouldn't be surprised if more remain. If there are, I think we should fix the code to remove the assumption. > > This really doesn't have any DesignWare specifics in it, and it seems > > more related to drivers/pci/host/pci-host-generic.c than to anything > > in drivers/pci/dwc. Maybe it should be > > drivers/pci/host/pci-host-generic-quirks.c or something? That's > > unwieldy, I admit. > > I don't care where we put it, and I am fine with owning it if you prefer. I think Will's idea of teaching pci-host-generic to deal with this is perfect. > >> +config PCIE_DW_HOST_ECAM > >> + bool "Synopsys DesignWare PCIe controller in ECAM mode" > >> + depends on OF && PCI > >> + select PCI_HOST_COMMON > >> + select IRQ_DOMAIN > >> + help > >> + Add support for Synopsys DesignWare PCIe controllers configured > >> + by the firmware into ECAM shift mode. In some cases, these are > >> + fully ECAM compliant, in which case the pci-host-generic driver > >> + may be used instead. > > > > This doesn't quite read right. It sounds like a controller in ECAM > > shift mode might be fully ECAM compliant, but I don't think that's > > what you intended. > > Yes, that is what I mean. ECAM shift mode results in a fully compliant > ECAM config space iff the IP was synthesized with a 32 KB granularity > for the iATU windows. The default is 64 KB, though, in which case you > need this driver. OK. I'm trying to figure out how I as a user would know whether to select this option. Maybe the config option will go away if you add the smarts to pci-host-generic? Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 17:48 ` Bjorn Helgaas @ 2017-09-28 18:33 ` Ard Biesheuvel 2017-09-29 3:29 ` Jingoo Han 1 sibling, 0 replies; 17+ messages in thread From: Ard Biesheuvel @ 2017-09-28 18:33 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On 28 September 2017 at 10:48, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Sep 28, 2017 at 08:51:43AM -0700, Ard Biesheuvel wrote: >> On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >> >> Some implementations of the Synopsys Designware PCIe controller implement >> >> a so-called ECAM shift mode, which allows a static memory window to be >> >> configured that covers the configuration space of the entire bus range. > >> >> Note that, unlike most drivers for this IP, this driver does not expose >> >> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >> >> given that this is not a true bridge, and does not require any windows >> >> to be configured in order for the downstream device to operate correctly. >> >> Omitting it also prevents the PCI resource allocation routines from >> >> handing out BAR space to it unnecessarily. >> > >> > This is a tangent, but does this mean the other drivers do not need to >> > expose a fake 00:00.0 device either? >> >> To be honest, I am not so sure anymore. I am seeing some issues in >> ASPM code making the assumption that any device which is not a root >> port has a parent. If this is mandated by the spec, I guess there >> isn't a whole lot we can do except expose a fake root port on b/d/f >> 0/0/0. This used to work fine, though, and I have to confirm whether >> the issues I am seeing currently are due to different hardware or >> changes in the software. > > I agree that our ASPM code had some assumptions that any non-root port > device should have a parent. I think the spec also makes that > assumption, but I haven't found an explicit mandate. And there *are* > systems lacking root ports: > > http://lkml.kernel.org/r/1439808478-23253-1-git-send-email-wangyijing@huawei.com > > We fixed one such assumption in that thread, but I wouldn't be > surprised if more remain. If there are, I think we should fix the > code to remove the assumption. > OK, that is good to know. What I am seeing on my board is the following crash Unable to handle kernel NULL pointer dereference at virtual address 00000090 [0000000000000090] user address but active_mm is swapper Internal error: Oops: 96000004 [#1] SMP Modules linked in: CPU: 23 PID: 1 Comm: swapper/0 Not tainted 4.13.0+ #1 Hardware name: Synquacer Evaluation Board (DT) task: ffff800f5c574080 task.stack: ffff800f5c578000 PC is at pcie_aspm_init_link_state+0x204/0xa38 LR is at pcie_aspm_init_link_state+0x184/0xa38 ... [<ffff0000084fda0c>] pcie_aspm_init_link_state+0x204/0xa38 [<ffff0000084e2924>] pci_scan_slot+0x10c/0x150 [<ffff0000084e3a9c>] pci_scan_child_bus+0x3c/0x1b0 [<ffff0000084e37b4>] pci_scan_bridge+0x30c/0x5b8 [<ffff0000084e3b1c>] pci_scan_child_bus+0xbc/0x1b0 [<ffff0000084e37b4>] pci_scan_bridge+0x30c/0x5b8 [<ffff0000084e3b1c>] pci_scan_child_bus+0xbc/0x1b0 [<ffff0000084e3e0c>] pci_scan_root_bus_bridge+0xdc/0xf8 [<ffff000008509c28>] pci_host_common_probe+0x148/0x400 [<ffff00000850ee4c>] pci_dw_ecam_probe+0x2c/0x38 [<ffff0000086348c8>] platform_drv_probe+0x60/0xc8 [<ffff000008631b3c>] driver_probe_device+0x2e4/0x460 [<ffff000008631de4>] __driver_attach+0x12c/0x130 [<ffff00000862f1e0>] bus_for_each_dev+0x88/0xe8 [<ffff000008631218>] driver_attach+0x30/0x40 [<ffff000008630b90>] bus_add_driver+0x200/0x2b8 [<ffff000008632fd8>] driver_register+0x68/0x100 [<ffff0000086347ec>] __platform_driver_register+0x54/0x60 [<ffff000008c44520>] pci_dw_ecam_driver_init+0x20/0x28 [<ffff00000808399c>] do_one_initcall+0x5c/0x168 [<ffff000008c10f98>] kernel_init_freeable+0x1e8/0x288 [<ffff0000088ae9b8>] kernel_init+0x18/0x108 [<ffff0000080836d0>] ret_from_fork+0x10/0x40 Code: 54000f20 f9400aa0 f9400800 f9401c00 (f9404816) which is essentially caused by this code [in alloc_pcie_link_state()] if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || pci_pcie_type(pdev) == PCI_EXP_TYPE_PCIE_BRIDGE) { link->root = link; } else { struct pcie_link_state *parent; parent = pdev->bus->parent->self->link_state; ... so I guess we should fix this instance as well. >> > This really doesn't have any DesignWare specifics in it, and it seems >> > more related to drivers/pci/host/pci-host-generic.c than to anything >> > in drivers/pci/dwc. Maybe it should be >> > drivers/pci/host/pci-host-generic-quirks.c or something? That's >> > unwieldy, I admit. >> >> I don't care where we put it, and I am fine with owning it if you prefer. > > I think Will's idea of teaching pci-host-generic to deal with this is > perfect. > I agree. I originally thought people would prefer the DesignWare quirks to live under dwc/, but it does fit more naturally into the generic driver. >> >> +config PCIE_DW_HOST_ECAM >> >> + bool "Synopsys DesignWare PCIe controller in ECAM mode" >> >> + depends on OF && PCI >> >> + select PCI_HOST_COMMON >> >> + select IRQ_DOMAIN >> >> + help >> >> + Add support for Synopsys DesignWare PCIe controllers configured >> >> + by the firmware into ECAM shift mode. In some cases, these are >> >> + fully ECAM compliant, in which case the pci-host-generic driver >> >> + may be used instead. >> > >> > This doesn't quite read right. It sounds like a controller in ECAM >> > shift mode might be fully ECAM compliant, but I don't think that's >> > what you intended. >> >> Yes, that is what I mean. ECAM shift mode results in a fully compliant >> ECAM config space iff the IP was synthesized with a 32 KB granularity >> for the iATU windows. The default is 64 KB, though, in which case you >> need this driver. > > OK. I'm trying to figure out how I as a user would know whether to > select this option. Maybe the config option will go away if you add > the smarts to pci-host-generic? > Yes. The firmware knows, and so the firmware should expose the correct compatible string in this case, either pci-host-ecam-generic or one of the quirked ones. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 17:48 ` Bjorn Helgaas 2017-09-28 18:33 ` Ard Biesheuvel @ 2017-09-29 3:29 ` Jingoo Han 1 sibling, 0 replies; 17+ messages in thread From: Jingoo Han @ 2017-09-29 3:29 UTC (permalink / raw) To: 'Bjorn Helgaas', 'Ard Biesheuvel' Cc: 'linux-pci', devicetree, 'Marcin Wojtas', 'Leif Lindholm', 'Graeme Gregory', 'Bjorn Helgaas', 'Joao Pinto', 'Rob Herring', 'Will Deacon' On Thursday, September 28, 2017 1:49 PM, Bjorn Helgaas wrote: > On Thu, Sep 28, 2017 at 08:51:43AM -0700, Ard Biesheuvel wrote: > > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > > >> Some implementations of the Synopsys Designware PCIe controller > implement > > >> a so-called ECAM shift mode, which allows a static memory window to > be > > >> configured that covers the configuration space of the entire bus > range. > > > >> Note that, unlike most drivers for this IP, this driver does not > expose > > >> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > > >> given that this is not a true bridge, and does not require any > windows > > >> to be configured in order for the downstream device to operate > correctly. > > >> Omitting it also prevents the PCI resource allocation routines from > > >> handing out BAR space to it unnecessarily. > > > > > > This is a tangent, but does this mean the other drivers do not need to > > > expose a fake 00:00.0 device either? > > > > To be honest, I am not so sure anymore. I am seeing some issues in > > ASPM code making the assumption that any device which is not a root > > port has a parent. If this is mandated by the spec, I guess there > > isn't a whole lot we can do except expose a fake root port on b/d/f > > 0/0/0. This used to work fine, though, and I have to confirm whether > > the issues I am seeing currently are due to different hardware or > > changes in the software. > > I agree that our ASPM code had some assumptions that any non-root port > device should have a parent. I think the spec also makes that > assumption, but I haven't found an explicit mandate. And there *are* > systems lacking root ports: > > http://lkml.kernel.org/r/1439808478-23253-1-git-send-email- > wangyijing@huawei.com > > We fixed one such assumption in that thread, but I wouldn't be > surprised if more remain. If there are, I think we should fix the > code to remove the assumption. > > > > This really doesn't have any DesignWare specifics in it, and it seems > > > more related to drivers/pci/host/pci-host-generic.c than to anything > > > in drivers/pci/dwc. Maybe it should be > > > drivers/pci/host/pci-host-generic-quirks.c or something? That's > > > unwieldy, I admit. > > > > I don't care where we put it, and I am fine with owning it if you prefer. > > I think Will's idea of teaching pci-host-generic to deal with this is > perfect. I agree. I cannot find any reason to create new dwc-specific file. Reusing 'pci-host-generic.c' looks better. Maybe 'pci-host-generic.c with quirks' will be good. Best regards, Jingoo Han > > > >> +config PCIE_DW_HOST_ECAM > > >> + bool "Synopsys DesignWare PCIe controller in ECAM mode" > > >> + depends on OF && PCI > > >> + select PCI_HOST_COMMON > > >> + select IRQ_DOMAIN > > >> + help > > >> + Add support for Synopsys DesignWare PCIe controllers > configured > > >> + by the firmware into ECAM shift mode. In some cases, these are > > >> + fully ECAM compliant, in which case the pci-host-generic > driver > > >> + may be used instead. > > > > > > This doesn't quite read right. It sounds like a controller in ECAM > > > shift mode might be fully ECAM compliant, but I don't think that's > > > what you intended. > > > > Yes, that is what I mean. ECAM shift mode results in a fully compliant > > ECAM config space iff the IP was synthesized with a 32 KB granularity > > for the iATU windows. The default is 64 KB, though, in which case you > > need this driver. > > OK. I'm trying to figure out how I as a user would know whether to > select this option. Maybe the config option will go away if you add > the smarts to pci-host-generic? > > Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-09-28 15:51 ` Ard Biesheuvel 2017-09-28 17:48 ` Bjorn Helgaas @ 2017-10-06 14:52 ` Ard Biesheuvel 2017-10-06 22:45 ` Bjorn Helgaas 1 sibling, 1 reply; 17+ messages in thread From: Ard Biesheuvel @ 2017-10-06 14:52 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On 28 September 2017 at 16:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: >> [+cc Will] >> >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >>> Some implementations of the Synopsys Designware PCIe controller implement >>> a so-called ECAM shift mode, which allows a static memory window to be >>> configured that covers the configuration space of the entire bus range. >>> >>> If the firmware performs all the low level configuration that is required >>> to expose this controller in a fully ECAM compatible manner, we can >>> simply describe it as "pci-host-ecam-generic" and be done with it. >>> However, it appears that in some cases (one of which is the Armada 80x0), >>> the IP is synthesized with an ATU window size that does not allow the >>> first bus to be mapped in a way that prevents the device on the >>> downstream port from appearing more than once. >>> >>> So implement a driver that relies on the firmware to perform all low >>> level initialization, and drives the controller in ECAM mode, but >>> overrides the config space accessors to take the above quirk into >>> account. >>> >>> Note that, unlike most drivers for this IP, this driver does not expose >>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >>> given that this is not a true bridge, and does not require any windows >>> to be configured in order for the downstream device to operate correctly. >>> Omitting it also prevents the PCI resource allocation routines from >>> handing out BAR space to it unnecessarily. >> >> This is a tangent, but does this mean the other drivers do not need to >> expose a fake 00:00.0 device either? >> > > To be honest, I am not so sure anymore. I am seeing some issues in > ASPM code making the assumption that any device which is not a root > port has a parent. If this is mandated by the spec, I guess there > isn't a whole lot we can do except expose a fake root port on b/d/f > 0/0/0. This used to work fine, though, and I have to confirm whether > the issues I am seeing currently are due to different hardware or > changes in the software. > OK, so the issue was new because I hadn't tried using a PCIe switch before, and you have already queued the fix to make the ASPM code deal with that. So I think it /would/ be better for the other drivers to not bother mocking up the root port, and simply expose the downstream device as B/D/F 0/0/0 (assuming the bus range starts at 0). It really looks like Altera, Aardvark, Sigma, etc are all in the same boat here, and need to a) filter type 0 config TLPs to avoid the downstream device to appear 32 times, and b) mangle config space accesses to the 'root port' to hide BARs that have different meanings in this context (the size of the inbound window), in order to prevent the PCI resource allocation routines to waste huge amounts of BAR space on them. >> s/Designware/DesignWare/ in comments, changelogs, Kconfig text, etc. >> > > OK > >>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: Jingoo Han <jingoohan1@gmail.com> >>> Cc: Joao Pinto <Joao.Pinto@synopsys.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> drivers/pci/dwc/Kconfig | 11 +++ >>> drivers/pci/dwc/Makefile | 1 + >>> drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ >> >> This really doesn't have any DesignWare specifics in it, and it seems >> more related to drivers/pci/host/pci-host-generic.c than to anything >> in drivers/pci/dwc. Maybe it should be >> drivers/pci/host/pci-host-generic-quirks.c or something? That's >> unwieldy, I admit. >> > > I don't care where we put it, and I am fine with owning it if you prefer. > >> Putting it in pci/dwc would make Jingoo and Joao the default >> maintainers; I don't know how they feel about that. We would probably >> have to tweak MAINTAINERS if we *didn't* put it in pci/dwc. >> >> Any thoughts on this, Will? >> >>> 3 files changed, 89 insertions(+) >>> >>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig >>> index d275aadc47ee..477576d07911 100644 >>> --- a/drivers/pci/dwc/Kconfig >>> +++ b/drivers/pci/dwc/Kconfig >>> @@ -169,4 +169,15 @@ config PCIE_KIRIN >>> Say Y here if you want PCIe controller support >>> on HiSilicon Kirin series SoCs. >>> >>> +config PCIE_DW_HOST_ECAM >>> + bool "Synopsys DesignWare PCIe controller in ECAM mode" >>> + depends on OF && PCI >>> + select PCI_HOST_COMMON >>> + select IRQ_DOMAIN >>> + help >>> + Add support for Synopsys DesignWare PCIe controllers configured >>> + by the firmware into ECAM shift mode. In some cases, these are >>> + fully ECAM compliant, in which case the pci-host-generic driver >>> + may be used instead. >> >> This doesn't quite read right. It sounds like a controller in ECAM >> shift mode might be fully ECAM compliant, but I don't think that's >> what you intended. >> > > Yes, that is what I mean. ECAM shift mode results in a fully compliant > ECAM config space iff the IP was synthesized with a 32 KB granularity > for the iATU windows. The default is 64 KB, though, in which case you > need this driver. > >> IIUC, the controller can be in either "ECAM shift mode" (where we need >> this new driver) or in a "fully ECAM compliant mode" (where we can use >> pci-host-generic). >> > > No, this is not the case > >>> endmenu >>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile >>> index c61be9738cce..7d5a23e5b767 100644 >>> --- a/drivers/pci/dwc/Makefile >>> +++ b/drivers/pci/dwc/Makefile >>> @@ -1,5 +1,6 @@ >>> obj-$(CONFIG_PCIE_DW) += pcie-designware.o >>> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o >>> +obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o >>> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o >>> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o >>> ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),) >>> diff --git a/drivers/pci/dwc/pcie-designware-ecam.c b/drivers/pci/dwc/pcie-designware-ecam.c >>> new file mode 100644 >>> index 000000000000..ede627d7d08b >>> --- /dev/null >>> +++ b/drivers/pci/dwc/pcie-designware-ecam.c >>> @@ -0,0 +1,77 @@ >>> +/* >>> + * Driver for mostly ECAM compatible Synopsys dw PCIe controllers >>> + * configured by the firmware into RC mode >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + * Copyright (C) 2014 ARM Limited >>> + * Copyright (C) 2017 Linaro Limited >>> + * >>> + * Authors: Will Deacon <will.deacon@arm.com> >>> + * Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> + */ >>> + >>> +#include <linux/kernel.h> >>> +#include <linux/init.h> >>> +#include <linux/of_address.h> >>> +#include <linux/of_pci.h> >>> +#include <linux/pci-ecam.h> >>> +#include <linux/platform_device.h> >>> + >>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, >>> + int size, u32 *val) >>> +{ >>> + struct pci_config_window *cfg = bus->sysdata; >>> + >>> + /* >>> + * The Synopsys dw PCIe controller in RC mode will not filter type 0 >>> + * config TLPs sent to devices 1 and up on its downstream port, >>> + * resulting in devices appearing multiple times on bus 0 unless we >>> + * filter them here. >>> + */ >>> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { >> >> Trivial, but maybe you could factor out this test? We already have >> these functions that do basically the same thing and it'd be nice to >> use a similar pattern (altera and dw also check for the link being up, >> which seems racy and possibly bogus to me): >> >> altera_pcie_valid_device() >> dw_pcie_valid_device() >> rockchip_pcie_valid_device() >> This is rather difficult to factor out, I'm afraid: static bool altera_pcie_valid_device(struct altera_pcie *pcie, struct pci_bus *bus, int dev) static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus, int dev) static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip, struct pci_bus *bus, int dev) They all use different struct types to describe the RC, and the fact that they model a root port means the type0 TLP filter should be applied to bus 1 not bus 0. So I agree there is some similarity between these, but not as much with the driver I am proposing. >> The fact that altera and rockchip do essentially the same thing as dw >> here suggests that this pattern is not limited to DesignWare. >> No. >> These other functions also do something similar, though not structured >> the same way: >> >> hisi_pcie_rd_conf() >> advk_pcie_rd_conf() >> thunder_pem_bridge_read() >> rcar_pcie_config_access() >> gapspci_config_access() >> > > I can look into that. > I think only hisi_pcie_rd_conf() comes close to what I need to do in this driver, and this is not surprising given that it uses Synopsys IP as well. I would like to replace that entirely with this driver at some point, but for now I'd like to proceed with Marvell Armada 8k and Socionext Synquacer only, given that those are the only ones I can actually test myself. So what I will do is respin the patch as an extension to pci-host-generic, so we'll have something to poke at, and perhaps you could give some more detailed guidance as to how to refactor these existing routines. Thanks, Ard. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-10-06 14:52 ` Ard Biesheuvel @ 2017-10-06 22:45 ` Bjorn Helgaas 2017-10-06 23:10 ` Ard Biesheuvel 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2017-10-06 22:45 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On Fri, Oct 06, 2017 at 03:52:03PM +0100, Ard Biesheuvel wrote: > On 28 September 2017 at 16:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> [+cc Will] > >> > >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: > >>> Some implementations of the Synopsys Designware PCIe controller implement > >>> a so-called ECAM shift mode, which allows a static memory window to be > >>> configured that covers the configuration space of the entire bus range. > >>> > >>> If the firmware performs all the low level configuration that is required > >>> to expose this controller in a fully ECAM compatible manner, we can > >>> simply describe it as "pci-host-ecam-generic" and be done with it. > >>> However, it appears that in some cases (one of which is the Armada 80x0), > >>> the IP is synthesized with an ATU window size that does not allow the > >>> first bus to be mapped in a way that prevents the device on the > >>> downstream port from appearing more than once. > >>> > >>> So implement a driver that relies on the firmware to perform all low > >>> level initialization, and drives the controller in ECAM mode, but > >>> overrides the config space accessors to take the above quirk into > >>> account. > >>> > >>> Note that, unlike most drivers for this IP, this driver does not expose > >>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, > >>> given that this is not a true bridge, and does not require any windows > >>> to be configured in order for the downstream device to operate correctly. > >>> Omitting it also prevents the PCI resource allocation routines from > >>> handing out BAR space to it unnecessarily. > >> > >> This is a tangent, but does this mean the other drivers do not need to > >> expose a fake 00:00.0 device either? > >> > > > > To be honest, I am not so sure anymore. I am seeing some issues in > > ASPM code making the assumption that any device which is not a root > > port has a parent. If this is mandated by the spec, I guess there > > isn't a whole lot we can do except expose a fake root port on b/d/f > > 0/0/0. This used to work fine, though, and I have to confirm whether > > the issues I am seeing currently are due to different hardware or > > changes in the software. > > > > OK, so the issue was new because I hadn't tried using a PCIe switch > before, and you have already queued the fix to make the ASPM code deal > with that. > > So I think it /would/ be better for the other drivers to not bother > mocking up the root port, and simply expose the downstream device as > B/D/F 0/0/0 (assuming the bus range starts at 0). > > It really looks like Altera, Aardvark, Sigma, etc are all in the same > boat here, and need to > a) filter type 0 config TLPs to avoid the downstream device to appear > 32 times, and > b) mangle config space accesses to the 'root port' to hide BARs that > have different meanings in this context (the size of the inbound > window), in order to prevent the PCI resource allocation routines to > waste huge amounts of BAR space on them. I don't know what to do about this. Obviously it's not your problem to clean this up. I don't know anything about the topology of those systems. If the ASPM thing was the only issue, we can probably fix that. Of course, that means no device connected to the link from those RCs could use ASPM (maybe that's the case anyway with the mocked-up root ports). > >>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, > >>> + int size, u32 *val) > >>> +{ > >>> + struct pci_config_window *cfg = bus->sysdata; > >>> + > >>> + /* > >>> + * The Synopsys dw PCIe controller in RC mode will not filter type 0 > >>> + * config TLPs sent to devices 1 and up on its downstream port, > >>> + * resulting in devices appearing multiple times on bus 0 unless we > >>> + * filter them here. > >>> + */ > >>> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { > >> > >> Trivial, but maybe you could factor out this test? We already have > >> these functions that do basically the same thing and it'd be nice to > >> use a similar pattern (altera and dw also check for the link being up, > >> which seems racy and possibly bogus to me): > >> > >> altera_pcie_valid_device() > >> dw_pcie_valid_device() > >> rockchip_pcie_valid_device() > >> > > This is rather difficult to factor out, I'm afraid: > > static bool altera_pcie_valid_device(struct altera_pcie *pcie, > struct pci_bus *bus, int dev) > > static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus, > int dev) > > static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip, > struct pci_bus *bus, int dev) > > They all use different struct types to describe the RC, and the fact > that they model a root port means the type0 TLP filter should be > applied to bus 1 not bus 0. > So I agree there is some similarity between these, but not as much > with the driver I am proposing. Sorry, I didn't mean to factor all these out into a single routine; I just meant maybe we could add a pci_dw_valid_device() with a structure similar to those I mentioned. > >> The fact that altera and rockchip do essentially the same thing as dw > >> here suggests that this pattern is not limited to DesignWare. > >> > > No. > >> These other functions also do something similar, though not structured > >> the same way: > >> > >> hisi_pcie_rd_conf() > >> advk_pcie_rd_conf() > >> thunder_pem_bridge_read() > >> rcar_pcie_config_access() > >> gapspci_config_access() > >> > > > > I can look into that. > > > > I think only hisi_pcie_rd_conf() comes close to what I need to do in > this driver, and this is not surprising given that it uses Synopsys IP > as well. I would like to replace that entirely with this driver at > some point, but for now I'd like to proceed with Marvell Armada 8k and > Socionext Synquacer only, given that those are the only ones I can > actually test myself. Sorry again, more unclear communication on my part. I don't think we can easily factor these things out; I was really just making the observation that these all look pretty similar and it would be good to make this new driver look as similar to the existing ones as possible. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode 2017-10-06 22:45 ` Bjorn Helgaas @ 2017-10-06 23:10 ` Ard Biesheuvel 0 siblings, 0 replies; 17+ messages in thread From: Ard Biesheuvel @ 2017-10-06 23:10 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring, Will Deacon On 6 October 2017 at 23:45, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, Oct 06, 2017 at 03:52:03PM +0100, Ard Biesheuvel wrote: >> On 28 September 2017 at 16:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote: >> >> [+cc Will] >> >> >> >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote: >> >>> Some implementations of the Synopsys Designware PCIe controller implement >> >>> a so-called ECAM shift mode, which allows a static memory window to be >> >>> configured that covers the configuration space of the entire bus range. >> >>> >> >>> If the firmware performs all the low level configuration that is required >> >>> to expose this controller in a fully ECAM compatible manner, we can >> >>> simply describe it as "pci-host-ecam-generic" and be done with it. >> >>> However, it appears that in some cases (one of which is the Armada 80x0), >> >>> the IP is synthesized with an ATU window size that does not allow the >> >>> first bus to be mapped in a way that prevents the device on the >> >>> downstream port from appearing more than once. >> >>> >> >>> So implement a driver that relies on the firmware to perform all low >> >>> level initialization, and drives the controller in ECAM mode, but >> >>> overrides the config space accessors to take the above quirk into >> >>> account. >> >>> >> >>> Note that, unlike most drivers for this IP, this driver does not expose >> >>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so, >> >>> given that this is not a true bridge, and does not require any windows >> >>> to be configured in order for the downstream device to operate correctly. >> >>> Omitting it also prevents the PCI resource allocation routines from >> >>> handing out BAR space to it unnecessarily. >> >> >> >> This is a tangent, but does this mean the other drivers do not need to >> >> expose a fake 00:00.0 device either? >> >> >> > >> > To be honest, I am not so sure anymore. I am seeing some issues in >> > ASPM code making the assumption that any device which is not a root >> > port has a parent. If this is mandated by the spec, I guess there >> > isn't a whole lot we can do except expose a fake root port on b/d/f >> > 0/0/0. This used to work fine, though, and I have to confirm whether >> > the issues I am seeing currently are due to different hardware or >> > changes in the software. >> > >> >> OK, so the issue was new because I hadn't tried using a PCIe switch >> before, and you have already queued the fix to make the ASPM code deal >> with that. >> >> So I think it /would/ be better for the other drivers to not bother >> mocking up the root port, and simply expose the downstream device as >> B/D/F 0/0/0 (assuming the bus range starts at 0). >> >> It really looks like Altera, Aardvark, Sigma, etc are all in the same >> boat here, and need to >> a) filter type 0 config TLPs to avoid the downstream device to appear >> 32 times, and >> b) mangle config space accesses to the 'root port' to hide BARs that >> have different meanings in this context (the size of the inbound >> window), in order to prevent the PCI resource allocation routines to >> waste huge amounts of BAR space on them. > > I don't know what to do about this. Obviously it's not your problem > to clean this up. > > I don't know anything about the topology of those systems. If the > ASPM thing was the only issue, we can probably fix that. Of course, > that means no device connected to the link from those RCs could use > ASPM (maybe that's the case anyway with the mocked-up root ports). > That may be the answer, although I am not sure. The limited Synopsys documentation I have access to does list 'L0s and L1 ASPM support; software L1 and L2 support' as a feature, and what they expose as the root port is actually a set of PCIe config registers that are in the MMIO region of the controller (but cannot be remapped statically in the ECAM space). But the same MMIO region has a BAR to configure the inbound window, and has bridge BARs that can be programmed but don't actually affect what gets passed onto the link. The almost-ECAM mode is much more appealing for the use cases I am involved with, and losing ASPM is no big deal, so I'd rather not use the dwc/ drivers if I can avoid it. >> >>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where, >> >>> + int size, u32 *val) >> >>> +{ >> >>> + struct pci_config_window *cfg = bus->sysdata; >> >>> + >> >>> + /* >> >>> + * The Synopsys dw PCIe controller in RC mode will not filter type 0 >> >>> + * config TLPs sent to devices 1 and up on its downstream port, >> >>> + * resulting in devices appearing multiple times on bus 0 unless we >> >>> + * filter them here. >> >>> + */ >> >>> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) { >> >> >> >> Trivial, but maybe you could factor out this test? We already have >> >> these functions that do basically the same thing and it'd be nice to >> >> use a similar pattern (altera and dw also check for the link being up, >> >> which seems racy and possibly bogus to me): >> >> >> >> altera_pcie_valid_device() >> >> dw_pcie_valid_device() >> >> rockchip_pcie_valid_device() >> >> >> >> This is rather difficult to factor out, I'm afraid: >> >> static bool altera_pcie_valid_device(struct altera_pcie *pcie, >> struct pci_bus *bus, int dev) >> >> static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus, >> int dev) >> >> static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip, >> struct pci_bus *bus, int dev) >> >> They all use different struct types to describe the RC, and the fact >> that they model a root port means the type0 TLP filter should be >> applied to bus 1 not bus 0. >> So I agree there is some similarity between these, but not as much >> with the driver I am proposing. > > Sorry, I didn't mean to factor all these out into a single routine; I > just meant maybe we could add a pci_dw_valid_device() with a structure > similar to those I mentioned. > >> >> The fact that altera and rockchip do essentially the same thing as dw >> >> here suggests that this pattern is not limited to DesignWare. >> >> >> >> No. >> >> These other functions also do something similar, though not structured >> >> the same way: >> >> >> >> hisi_pcie_rd_conf() >> >> advk_pcie_rd_conf() >> >> thunder_pem_bridge_read() >> >> rcar_pcie_config_access() >> >> gapspci_config_access() >> >> >> > >> > I can look into that. >> > >> >> I think only hisi_pcie_rd_conf() comes close to what I need to do in >> this driver, and this is not surprising given that it uses Synopsys IP >> as well. I would like to replace that entirely with this driver at >> some point, but for now I'd like to proceed with Marvell Armada 8k and >> Socionext Synquacer only, given that those are the only ones I can >> actually test myself. > > Sorry again, more unclear communication on my part. I don't think we > can easily factor these things out; I was really just making the > observation that these all look pretty similar and it would be good to > make this new driver look as similar to the existing ones as possible. > OK, fair enough. I took Will's advice and extended the pci-host-generic driver instead, but I'm happy to do another round if necessary. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode 2017-08-28 18:04 [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel @ 2017-08-28 18:04 ` Ard Biesheuvel 2017-08-31 14:23 ` Rob Herring 2017-08-29 15:40 ` [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Marcin Wojtas 2 siblings, 1 reply; 17+ messages in thread From: Ard Biesheuvel @ 2017-08-28 18:04 UTC (permalink / raw) To: linux-pci Cc: devicetree, mw, Ard Biesheuvel, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring Describe the binding for firmware-configured instances of the Synopsys Designware PCIe controller in RC mode, that are almost but not quite ECAM compliant. Cc: Rob Herring <robh@kernel.org> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt new file mode 100644 index 000000000000..29bad1337c87 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt @@ -0,0 +1,42 @@ +* Synopsys Designware PCIe root complex in ECAM mode + +In some cases, firmware may already have configured the Synopsys Designware +PCIe controller in RC mode with static ATU window mappings that cover all +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. +In this case, there is no need for the OS to perform any low level setup +of clocks, PHYs or device registers, nor is there any reason for the driver +to reconfigure ATU windows for config and/or IO space accesses at runtime. + +In cases where the IP was synthesized with a minimum ATU window size of +64 KB, it cannot be supported by the generic ECAM driver, because it +requires special config space accessors that filter accesses to device #1 +and beyond on the first bus. + +Required properties: +- compatible: "marvell,armada8k-pcie-ecam" or + "socionext,synquacer-pcie-ecam" or + "snps,dw-pcie-ecam" (must be preceded by a more specific match) + +Please refer to the binding document of "pci-host-ecam-generic" in the +file host-generic-pci.txt for a description of the remaining required +and optional properties. + +Example: + + pcie1: pcie@7f000000 { + compatible = "socionext,synquacer-pcie-ecam", "snps,dw-pcie-ecam"; + device_type = "pci"; + reg = <0x0 0x7f000000 0x0 0xf00000>; + bus-range = <0x0 0xe>; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x1000000 0x00 0x00010000 0x00 0x7ff00000 0x0 0x00010000>, + <0x2000000 0x00 0x70000000 0x00 0x70000000 0x0 0x0f000000>, + <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>; + + #interrupt-cells = <0x1>; + interrupt-map-mask = <0x0 0x0 0x0 0x0>; + interrupt-map = <0x0 0x0 0x0 0x0 &gic 0x0 0x0 0x0 182 0x4>; + msi-map = <0x0 &its 0x0 0x10000>; + dma-coherent; + }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode 2017-08-28 18:04 ` [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel @ 2017-08-31 14:23 ` Rob Herring 0 siblings, 0 replies; 17+ messages in thread From: Rob Herring @ 2017-08-31 14:23 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-pci, devicetree, Marcin Wojtas, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto On Mon, Aug 28, 2017 at 1:04 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Describe the binding for firmware-configured instances of the Synopsys > Designware PCIe controller in RC mode, that are almost but not quite > ECAM compliant. > > Cc: Rob Herring <robh@kernel.org> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++++ > 1 file changed, 42 insertions(+) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 0/2] pci: add support for firmware initialized designware RCs 2017-08-28 18:04 [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel @ 2017-08-29 15:40 ` Marcin Wojtas 2 siblings, 0 replies; 17+ messages in thread From: Marcin Wojtas @ 2017-08-29 15:40 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-pci, devicetree, Leif Lindholm, Graeme Gregory, Bjorn Helgaas, Jingoo Han, Joao Pinto, Rob Herring Hi Ard, It works on Armada 8040 MacchiatoBin board with e1000 card. You can add my: Tested-by: Marcin Wojtas <mw@semihalf.com> Best regards, Marcin 2017-08-28 20:04 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>: > UEFI based systems incorporating a Synopsys Designware PCIe controller > in RC mode will typically configure it before entering the OS. If this > configuration is fully static and ECAM compliant, there is no need to > expose particulars of the device to the OS, and we can simply describe > it as "pci-host-ecam-generic". > > However, the Synopsys IP may be synthesized in a way where a quirk is > needed for config space accesses to the first bus. It makes little sense > to instantiate yet another pcie-designware driver that contains all the > low level setup code, but it is also not justified to add quirks handling > to the generic ECAM driver. > > So instead, create a variant of the generic ECAM driver that filters config > space accesses directed at device #1 and up on the first bus. > > v3: - use SoC specific compatible strings > - drop MSI patch [for now], since it turns out we may not need it > > v2: - use dev->fwnode directly > - replace an instance of pr_err with dev_err, and clarify the error message > - fix Kconfig/Makefile dependency errors reported by kbuild > > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Graeme Gregory <graeme.gregory@linaro.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > Cc: Rob Herring <robh@kernel.org> > > Ard Biesheuvel (2): > pci: designware: add driver for DWC controller in ECAM shift mode > dt-bindings: designware: add binding for Designware PCIe in ECAM mode > > Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 +++++++++++ > drivers/pci/dwc/Kconfig | 11 +++ > drivers/pci/dwc/Makefile | 1 + > drivers/pci/dwc/pcie-designware-ecam.c | 77 ++++++++++++++++++++ > 4 files changed, 131 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt > create mode 100644 drivers/pci/dwc/pcie-designware-ecam.c > > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-10-06 23:10 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-28 18:04 [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel 2017-09-26 17:32 ` Bjorn Helgaas 2017-09-28 9:03 ` Will Deacon 2017-09-28 15:57 ` Ard Biesheuvel 2017-09-28 16:00 ` Will Deacon 2017-09-28 16:04 ` Ard Biesheuvel 2017-09-28 15:51 ` Ard Biesheuvel 2017-09-28 17:48 ` Bjorn Helgaas 2017-09-28 18:33 ` Ard Biesheuvel 2017-09-29 3:29 ` Jingoo Han 2017-10-06 14:52 ` Ard Biesheuvel 2017-10-06 22:45 ` Bjorn Helgaas 2017-10-06 23:10 ` Ard Biesheuvel 2017-08-28 18:04 ` [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel 2017-08-31 14:23 ` Rob Herring 2017-08-29 15:40 ` [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Marcin Wojtas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).