On 7 October 2017 at 00:21, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +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.
>>
>> Usually, when 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, in some cases (e.g., the Marvell Armada 80x0 as well as the
>> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>> granularity 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,
>> and so we still need special handling in software to drive this static
>> almost-ECAM configuration.
>>
>> So extend the pci-host-generic driver so it can support these controllers
>> as well, by adding special config space accessors that 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.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>> drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>> 1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 7d709a7e0aa8..01e81a30e303 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> }
>> };
>>
>> +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 DesignWare PCIe controller in ECAM 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 out those accesses here.
>> + */
>> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> + return PCIBIOS_DEVICE_NOT_FOUND;
>
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
>
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices. Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case.
>
> What do you think of the following? I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).
>
Thanks Bjorn, that does look better, and it works fine on my system.
Regards,
Ard.
>
>
> commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
> Author: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Date: Fri Oct 6 17:39:18 2017 +0100
>
> PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
>
> 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.
>
> Usually, when 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, in some cases (e.g., the Marvell Armada 80x0 as well as the
> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
> granularity 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,
> and so we still need special handling in software to drive this static
> almost-ECAM configuration.
>
> So extend the pci-host-generic driver so it can support these controllers
> as well, by adding special config space accessors that 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.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
> use generic read/write functions]
> Signed-off-by: Bjorn Helgaas <helgaas@xxxxxxxxxx>
>
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..2f05511ce718 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> }
> };
>
> +static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> +
> + /*
> + * The Synopsys DesignWare PCIe controller in ECAM 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 out those accesses here.
> + */
> + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> + return false;
> +
> + return true;
> +}
> +
> +static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
> + unsigned int devfn, int where)
> +{
> + if (!pci_dw_valid_device(bus, devfn))
> + return NULL;
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
> + .bus_shift = 20,
> + .pci_ops = {
> + .map_bus = pci_dw_ecam_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> static const struct of_device_id gen_pci_of_match[] = {
> { .compatible = "pci-host-cam-generic",
> .data = &gen_pci_cfg_cam_bus_ops },
> @@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
> { .compatible = "pci-host-ecam-generic",
> .data = &pci_generic_ecam_ops },
>
> + { .compatible = "marvell,armada8k-pcie-ecam",
> + .data = &pci_dw_ecam_bus_ops },
> +
> + { .compatible = "socionext,synquacer-pcie-ecam",
> + .data = &pci_dw_ecam_bus_ops },
> +
> + { .compatible = "snps,dw-pcie-ecam",
> + .data = &pci_dw_ecam_bus_ops },
> +
> { },
> };
>
