Message ID | 20200324182812.20420-4-nsaenzjulienne@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | USB: pci-quirks: Add Raspberry Pi 4 quirk | expand |
On Tue, Mar 24, 2020 at 07:28:11PM +0100, Nicolas Saenz Julienne wrote: > xHCI's PCI fixup, run at the end of pcie-brcmstb's probe, depends on Is there a function name for this fixup that you can mention? > RPi4's VideoCore firmware interface to be up and running. It's possible > for both initializations to race, so make sure it's available prior to > starting. I guess "both initializations" means brcm_pcie_probe() and something else? It'd be nice to include that function name here, too. > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 3a10e678c7f4..a3d3070a5832 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -28,6 +28,8 @@ > #include <linux/string.h> > #include <linux/types.h> > > +#include <soc/bcm2835/raspberrypi-firmware.h> > + > #include "../pci.h" > > /* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ > @@ -917,11 +919,24 @@ static int brcm_pcie_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node, *msi_np; > struct pci_host_bridge *bridge; > + struct device_node *fw_np; > struct brcm_pcie *pcie; > struct pci_bus *child; > struct resource *res; > int ret; > > + /* > + * We have to wait for the Raspberry Pi's firmware interface to be up > + * as some PCI fixups depend on it. It'd be nice to know the nature of this dependency between the firmware interface and the fixups. This may be useful for future maintenance. E.g., if PCI config access doesn't work until the firmware interface is up, that would affect almost everything. But you say "some PCI fixups", so I suppose the actual dependency is probably something else. > + */ > + fw_np = of_find_compatible_node(NULL, NULL, > + "raspberrypi,bcm2835-firmware"); > + if (fw_np && !rpi_firmware_get(fw_np)) { > + of_node_put(fw_np); > + return -EPROBE_DEFER; > + } > + of_node_put(fw_np); > + > bridge = devm_pci_alloc_host_bridge(&pdev->dev, sizeof(*pcie)); > if (!bridge) > return -ENOMEM; > -- > 2.25.1 >
Hi Bjorn, On Wed, 2020-04-01 at 15:41 -0500, Bjorn Helgaas wrote: > On Tue, Mar 24, 2020 at 07:28:11PM +0100, Nicolas Saenz Julienne wrote: > > xHCI's PCI fixup, run at the end of pcie-brcmstb's probe, depends on > > Is there a function name for this fixup that you can mention? Yes, rpi_firmware_init_vl805(), I'll update the description. > > RPi4's VideoCore firmware interface to be up and running. It's possible > > for both initializations to race, so make sure it's available prior to > > starting. > > I guess "both initializations" means brcm_pcie_probe() and something > else? It'd be nice to include that function name here, too. Noted, I'll be more explicit on the next version of the series. More in depth explanation below. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c > > b/drivers/pci/controller/pcie-brcmstb.c > > index 3a10e678c7f4..a3d3070a5832 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -28,6 +28,8 @@ > > #include <linux/string.h> > > #include <linux/types.h> > > > > +#include <soc/bcm2835/raspberrypi-firmware.h> > > + > > #include "../pci.h" > > > > /* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ > > @@ -917,11 +919,24 @@ static int brcm_pcie_probe(struct platform_device > > *pdev) > > { > > struct device_node *np = pdev->dev.of_node, *msi_np; > > struct pci_host_bridge *bridge; > > + struct device_node *fw_np; > > struct brcm_pcie *pcie; > > struct pci_bus *child; > > struct resource *res; > > int ret; > > > > + /* > > + * We have to wait for the Raspberry Pi's firmware interface to be up > > + * as some PCI fixups depend on it. > > It'd be nice to know the nature of this dependency between the > firmware interface and the fixups. This may be useful for future > maintenance. E.g., if PCI config access doesn't work until the > firmware interface is up, that would affect almost everything. But > you say "some PCI fixups", so I suppose the actual dependency is > probably something else. Sorry it wasn't clear enough, I'll redo this comment. Also note that the PCIe bus and the XHCI chip are hardwired, so that's the only device that'll ever be available on the bus. VIA805's XHCI firmware has to be loaded trough RPi's firmware mailbox in between the PCIe bus probe and the subsequent USB probe. Note that a PCI reset clears the firmware. The only mechanism available in between the two operations are PCI Fixups. These are limited in their own way, as I can't return -EPROBE_DEFER if the firmware interface isn't available yet. Hence the need for an explicit dependency between pcie-brcmstb and raspberrypi's firmware mailbox device. Your concern here showcases this series' limitations. From a high level perspective it's not clear to me who should be responsible for downloading the firmware. And I get the feeling I'm abusing PCI fixups. I haven't found any smart way to deal with this three way dependency of platform/non-platform devices. I even looked into adding -EPROBE_DEFER support to fixups, but I fear that would entail moving them into the core device definition. Regards, Nicolas
[+cc Rob for DT platform device dependency question] On Thu, Apr 02, 2020 at 04:27:23PM +0200, Nicolas Saenz Julienne wrote: > On Wed, 2020-04-01 at 15:41 -0500, Bjorn Helgaas wrote: > > On Tue, Mar 24, 2020 at 07:28:11PM +0100, Nicolas Saenz Julienne wrote: > > > xHCI's PCI fixup, run at the end of pcie-brcmstb's probe, depends on > > > RPi4's VideoCore firmware interface to be up and running. It's possible > > > for both initializations to race, so make sure it's available prior to > > > starting. > > > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > > drivers/pci/controller/pcie-brcmstb.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c > > > b/drivers/pci/controller/pcie-brcmstb.c > > > index 3a10e678c7f4..a3d3070a5832 100644 > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > @@ -28,6 +28,8 @@ > > > #include <linux/string.h> > > > #include <linux/types.h> > > > > > > +#include <soc/bcm2835/raspberrypi-firmware.h> > > > + > > > #include "../pci.h" > > > > > > /* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ > > > @@ -917,11 +919,24 @@ static int brcm_pcie_probe(struct platform_device > > > *pdev) > > > { > > > struct device_node *np = pdev->dev.of_node, *msi_np; > > > struct pci_host_bridge *bridge; > > > + struct device_node *fw_np; > > > struct brcm_pcie *pcie; > > > struct pci_bus *child; > > > struct resource *res; > > > int ret; > > > > > > + /* > > > + * We have to wait for the Raspberry Pi's firmware interface to be up > > > + * as some PCI fixups depend on it. > > > > It'd be nice to know the nature of this dependency between the > > firmware interface and the fixups. This may be useful for future > > maintenance. E.g., if PCI config access doesn't work until the > > firmware interface is up, that would affect almost everything. But > > you say "some PCI fixups", so I suppose the actual dependency is > > probably something else. > > Sorry it wasn't clear enough, I'll redo this comment. Also note that > the PCIe bus and the XHCI chip are hardwired, so that's the only > device that'll ever be available on the bus. > > VIA805's XHCI firmware has to be loaded trough RPi's firmware > mailbox in between the PCIe bus probe and the subsequent USB probe. > Note that a PCI reset clears the firmware. The only mechanism > available in between the two operations are PCI Fixups. These are > limited in their own way, as I can't return -EPROBE_DEFER if the > firmware interface isn't available yet. Hence the need for an > explicit dependency between pcie-brcmstb and raspberrypi's firmware > mailbox device. > > Your concern here showcases this series' limitations. From a high > level perspective it's not clear to me who should be responsible for > downloading the firmware. I think it's fairly common for drivers to download firmware to their devices. I guess there's not really any need to download the firmware until a driver wants to use the device, right? > And I get the feeling I'm abusing PCI fixups. I haven't found any > smart way to deal with this three way dependency of > platform/non-platform devices. So IIUC, the three-way dependency involves: 1) brcm_pcie_probe(), which initialize the PCI host controller platform device, enumerates PCI devices, and makes them available for driver binding, 2) the firmware mailbox initialization (maybe rpi_firmware_probe()?), 3) quirk_usb_early_handoff(), which downloads firmware to the VL805 PCI USB adapter via rpi_firmware_init_vl805(), which uses the firmware mailbox? Is there some way to express a dependency between "raspberrypi,bcm2835-firmware" (the platform device claimed by rpi_firmware_probe() and "brcm,bcm2711-pcie"? If we could ensure that rpi_firmware_probe() runs before brcm_pcie_probe(), would that solve part of this? Bjorn
On Thu, 2020-04-02 at 14:38 -0500, Bjorn Helgaas wrote: > [+cc Rob for DT platform device dependency question] > > On Thu, Apr 02, 2020 at 04:27:23PM +0200, Nicolas Saenz Julienne wrote: [...] > > Sorry it wasn't clear enough, I'll redo this comment. Also note that > > the PCIe bus and the XHCI chip are hardwired, so that's the only > > device that'll ever be available on the bus. > > > > VIA805's XHCI firmware has to be loaded trough RPi's firmware > > mailbox in between the PCIe bus probe and the subsequent USB probe. > > Note that a PCI reset clears the firmware. The only mechanism > > available in between the two operations are PCI Fixups. These are > > limited in their own way, as I can't return -EPROBE_DEFER if the > > firmware interface isn't available yet. Hence the need for an > > explicit dependency between pcie-brcmstb and raspberrypi's firmware > > mailbox device. > > > > Your concern here showcases this series' limitations. From a high > > level perspective it's not clear to me who should be responsible for > > downloading the firmware. > > I think it's fairly common for drivers to download firmware to their > devices. I guess there's not really any need to download the firmware > until a driver wants to use the device, right? > > > And I get the feeling I'm abusing PCI fixups. I haven't found any > > smart way to deal with this three way dependency of > > platform/non-platform devices. > > So IIUC, the three-way dependency involves: > > 1) brcm_pcie_probe(), which initialize the PCI host controller > platform device, enumerates PCI devices, and makes them available > for driver binding, Yes, and also resets the PCI bus, which will clear VL805's firmware (the XHCI chip). > 2) the firmware mailbox initialization (maybe > rpi_firmware_probe()?), > > 3) quirk_usb_early_handoff(), which downloads firmware to the VL805 > PCI USB adapter via rpi_firmware_init_vl805(), which uses the > firmware mailbox? And yes, that's the general idea. > Is there some way to express a dependency between > "raspberrypi,bcm2835-firmware" (the platform device claimed by > rpi_firmware_probe() and "brcm,bcm2711-pcie"? If we could ensure that > rpi_firmware_probe() runs before brcm_pcie_probe(), would that solve > part of this? That's ultimately what this patch tries to achieve. If there was a way to offload it to DT it would be way nicer. Regards, Nicolas
On 4/4/2020 12:20 PM, Nicolas Saenz Julienne wrote: > On Thu, 2020-04-02 at 14:38 -0500, Bjorn Helgaas wrote: >> [+cc Rob for DT platform device dependency question] >> >> On Thu, Apr 02, 2020 at 04:27:23PM +0200, Nicolas Saenz Julienne wrote: > > [...] > >>> Sorry it wasn't clear enough, I'll redo this comment. Also note that >>> the PCIe bus and the XHCI chip are hardwired, so that's the only >>> device that'll ever be available on the bus. >>> >>> VIA805's XHCI firmware has to be loaded trough RPi's firmware >>> mailbox in between the PCIe bus probe and the subsequent USB probe. >>> Note that a PCI reset clears the firmware. The only mechanism >>> available in between the two operations are PCI Fixups. These are >>> limited in their own way, as I can't return -EPROBE_DEFER if the >>> firmware interface isn't available yet. Hence the need for an >>> explicit dependency between pcie-brcmstb and raspberrypi's firmware >>> mailbox device. >>> >>> Your concern here showcases this series' limitations. From a high >>> level perspective it's not clear to me who should be responsible for >>> downloading the firmware. >> >> I think it's fairly common for drivers to download firmware to their >> devices. I guess there's not really any need to download the firmware >> until a driver wants to use the device, right? >> >>> And I get the feeling I'm abusing PCI fixups. I haven't found any >>> smart way to deal with this three way dependency of >>> platform/non-platform devices. >> >> So IIUC, the three-way dependency involves: >> >> 1) brcm_pcie_probe(), which initialize the PCI host controller >> platform device, enumerates PCI devices, and makes them available >> for driver binding, > > Yes, and also resets the PCI bus, which will clear VL805's firmware (the XHCI > chip). > >> 2) the firmware mailbox initialization (maybe >> rpi_firmware_probe()?), >> >> 3) quirk_usb_early_handoff(), which downloads firmware to the VL805 >> PCI USB adapter via rpi_firmware_init_vl805(), which uses the >> firmware mailbox? > > And yes, that's the general idea. > >> Is there some way to express a dependency between >> "raspberrypi,bcm2835-firmware" (the platform device claimed by >> rpi_firmware_probe() and "brcm,bcm2711-pcie"? If we could ensure that >> rpi_firmware_probe() runs before brcm_pcie_probe(), would that solve >> part of this? > > That's ultimately what this patch tries to achieve. If there was a way to > offload it to DT it would be way nicer. Have you looked whether device links between producer/consumer could help here? AFAICT there is not usually a way to express an ordering dependency other than by referencing symbols from a different module, which falls apart when everything gets built into the kernel obviously and then you are at the mercy of the linking order and initicall levels. https://www.kernel.org/doc/html/v4.13/driver-api/device_link.html
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 3a10e678c7f4..a3d3070a5832 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -28,6 +28,8 @@ #include <linux/string.h> #include <linux/types.h> +#include <soc/bcm2835/raspberrypi-firmware.h> + #include "../pci.h" /* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ @@ -917,11 +919,24 @@ static int brcm_pcie_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node, *msi_np; struct pci_host_bridge *bridge; + struct device_node *fw_np; struct brcm_pcie *pcie; struct pci_bus *child; struct resource *res; int ret; + /* + * We have to wait for the Raspberry Pi's firmware interface to be up + * as some PCI fixups depend on it. + */ + fw_np = of_find_compatible_node(NULL, NULL, + "raspberrypi,bcm2835-firmware"); + if (fw_np && !rpi_firmware_get(fw_np)) { + of_node_put(fw_np); + return -EPROBE_DEFER; + } + of_node_put(fw_np); + bridge = devm_pci_alloc_host_bridge(&pdev->dev, sizeof(*pcie)); if (!bridge) return -ENOMEM;