Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode

[Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 28 September 2017 at 10:48, Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html