Re: [PATCH v6 3/4] PCI: brcmstb: Wait for Raspberry Pi's firmware when present

[Bjorn Helgaas]

[+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@xxxxxxx>
> > > Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> > > ---
> > >  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