Hi Bjorn, thanks for taking time with this. On Wed, 2020-04-01 at 15:37 -0500, Bjorn Helgaas wrote: > On Tue, Mar 24, 2020 at 07:28:10PM +0100, Nicolas Saenz Julienne wrote: > > On the Raspberry Pi 4, after a PCI reset, VL805's firmware may either be > > loaded directly from an EEPROM or, if not present, by the SoC's > > VideCore. The function informs VideCore that VL805 was just reset, or > > requests for a probe defer. > > Cover letter mentions both "VideCore" and "VideoCore". I dunno which > is correct, but between the commit log and the comment, this patch > mentions "VideCore" four times. Ouch, sorry, it's VideoCore. I have an auto complete thing, wrote it once wrong and polluted the whole patch. > > Based on Tim Gover's downstream implementation. > > Maybe a URL? I was under the impression that adding links in the commit log that are likely to be short-lived was frowned upon. That said I could've added it into the cover letter. For reference here it is: https://github.com/raspberrypi/linux/commit/9935b4c7e360b4494b4cb6e3ce797238a1ab78bd > > Signed-off-by: Nicolas Saenz Julienne > > Reviewed-by: Florian Fainelli > > > > --- > > Changes since v4: > > - Inline function definition when RASPBERRYPI_FIRMWARE is not defined > > > > Changes since v1: > > - Move include into .c file and add forward declaration to .h > > > > drivers/firmware/raspberrypi.c | 38 ++++++++++++++++++++++ > > include/soc/bcm2835/raspberrypi-firmware.h | 7 ++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > > index da26a584dca0..cbb495aff6a0 100644 > > --- a/drivers/firmware/raspberrypi.c > > +++ b/drivers/firmware/raspberrypi.c > > @@ -12,6 +12,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & > > 0xf)) > > @@ -286,6 +287,43 @@ struct rpi_firmware *rpi_firmware_get(struct > > device_node *firmware_node) > > } > > EXPORT_SYMBOL_GPL(rpi_firmware_get); > > > > +/* > > + * On the Raspberry Pi 4, after a PCI reset, VL805's firmware may either be > > + * loaded directly from an EEPROM or, if not present, by the SoC's > > VideCore. > > + * Inform VideCore that VL805 was just reset, or defer xhci's probe if not > > yet > > + * joinable trough the mailbox interface. > > s/trough/through/ Noted. > I don't see anything in this patch that looks like a mailbox > interface, but maybe that's just because I don't know anything about > Raspberry Pi. There are two layers to this. The bcm2835-mailbox interface, that is generic to all SoC users and the Raspberry Pi firmware driver, which interacts with RPi's custom VideoCore firmware trough the bcm2835-mailbox, and provides a light level of abstraction. It's like that to keep a clear separation between what's a SoC feature an what is RPi specific. So with a call to rpi_firmware_get() you're supposed to get a handle to the shared RPi firmware structure. As long as it's ready. To pass messages down the mailbox, you call rpi_firmware_property(), which takes care of contention, formating and DMA issues, before passing it into the actual mailbox interface and beyond. > > + */ > > +int rpi_firmware_init_vl805(struct pci_dev *pdev) > > +{ > > + struct device_node *fw_np; > > + struct rpi_firmware *fw; > > + u32 dev_addr; > > + int ret; > > + > > + fw_np = of_find_compatible_node(NULL, NULL, > > + "raspberrypi,bcm2835-firmware"); > > + if (!fw_np) > > + return 0; > > + > > + fw = rpi_firmware_get(fw_np); > > + of_node_put(fw_np); > > + if (!fw) > > + return -EPROBE_DEFER; > > + > > + dev_addr = pdev->bus->number << 20 | PCI_SLOT(pdev->devfn) << 15 | > > + PCI_FUNC(pdev->devfn) << 12; > > + > > + ret = rpi_firmware_property(fw, RPI_FIRMWARE_NOTIFY_XHCI_RESET, > > + &dev_addr, sizeof(dev_addr)); > > + if (ret) > > + return ret; > > + > > + dev_dbg(&pdev->dev, "loaded Raspberry Pi's VL805 firmware\n"); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(rpi_firmware_init_vl805); > > + > > static const struct of_device_id rpi_firmware_of_match[] = { > > { .compatible = "raspberrypi,bcm2835-firmware", }, > > {}, [...] Regards, Nicolas