| Message ID | 20200217100701.19949-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: xhci-pci: Raspberry Pi FW loader for VIA VL805 | expand |
On Mon, Feb 17, 2020 at 11:07:00AM +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. Inform VideCore that VL805 was just reset, or defer xhci's > probe if not yet joinable trough the mailbox interface. > > Based on Tim Gover's downstream implementation. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/usb/host/xhci-pci.c | 50 ++++++++++++++++++++++ > include/soc/bcm2835/raspberrypi-firmware.h | 2 +- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 4917c5b033fa..eadace4a9339 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -13,6 +13,8 @@ > #include <linux/module.h> > #include <linux/acpi.h> > > +#include <soc/bcm2835/raspberrypi-firmware.h> That feels really wrong :( > + > #include "xhci.h" > #include "xhci-trace.h" > > @@ -308,6 +310,44 @@ static int xhci_pci_setup(struct usb_hcd *hcd) > return xhci_pci_reinit(xhci, pdev); > } > > +/* > + * 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. > + */ > +static int raspberrypi_load_vl805_fw(struct pci_dev *pdev) > +{ > +#ifdef CONFIG_RASPBERRYPI_FIRMWARE Can you just put #ifdefs in a .h file instead please? > + 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; So for non-rpi systems, this will work just fine, no need to #ifdef out the whole function, right? > + > + 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"); > + > +#endif > + return 0; > +} Why not put this whole function in some rpi-platform code? > + > /* > * We need to register our own PCI probe function (instead of the USB core's > * function) in order to create a second roothub under xHCI. > @@ -321,6 +361,16 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) > > driver = (struct hc_driver *)id->driver_data; > > + if (dev->vendor == PCI_VENDOR_ID_VIA && dev->device == 0x3483) { > + retval = raspberrypi_load_vl805_fw(dev); > + if (retval) { > + if (retval != -EPROBE_DEFER) > + dev_err(&dev->dev, > + "Failed to load VL805's firmware"); Shouldn't the function print an error if there is one? thanks, greg k-h
Hi Greg, thanks for having a look at this. On Mon, 2020-02-17 at 11:36 +0100, Greg Kroah-Hartman wrote: [...] > > +#include <soc/bcm2835/raspberrypi-firmware.h> > > That feels really wrong :( I know, not that happy about it either, but I had to start with something :) > > + > > #include "xhci.h" > > #include "xhci-trace.h" > > > > @@ -308,6 +310,44 @@ static int xhci_pci_setup(struct usb_hcd *hcd) > > return xhci_pci_reinit(xhci, pdev); > > } > > > > +/* > > + * 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. > > + */ > > +static int raspberrypi_load_vl805_fw(struct pci_dev *pdev) > > +{ > > +#ifdef CONFIG_RASPBERRYPI_FIRMWARE > > Can you just put #ifdefs in a .h file instead please? Yes, although... > > + 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; > > So for non-rpi systems, this will work just fine, no need to #ifdef out > the whole function, right? ...you're right here, on non-rpi systems this will just return without errors. On top of that, most VL805 users don't use device-tree, so it'll be fast. > > + > > + 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"); > > + > > +#endif > > + return 0; > > +} > Why not put this whole function in some rpi-platform code? I think moving it into firmware/raspberrypi.c should be acceptable. That said I'd still be on the loose regarding the include file, and the platform specific function call in xhci_pci_probe(). Any suggestions? Note that it's important to us to be able to defer the probe as the firmware inteface bringup might race with xHC's > > + > > /* > > * We need to register our own PCI probe function (instead of the USB > > core's > > * function) in order to create a second roothub under xHCI. > > @@ -321,6 +361,16 @@ static int xhci_pci_probe(struct pci_dev *dev, const > > struct pci_device_id *id) > > > > driver = (struct hc_driver *)id->driver_data; > > > > + if (dev->vendor == PCI_VENDOR_ID_VIA && dev->device == 0x3483) { > > + retval = raspberrypi_load_vl805_fw(dev); > > + if (retval) { > > + if (retval != -EPROBE_DEFER) > > + dev_err(&dev->dev, > > + "Failed to load VL805's firmware"); > > Shouldn't the function print an error if there is one? Noted, I'll update that. Regards, Nicolas
Hi Nicolas, Am 17.02.20 um 11:07 schrieb Nicolas Saenz Julienne: > 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. > > Based on Tim Gover's downstream implementation. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/usb/host/xhci-pci.c | 50 ++++++++++++++++++++++ > include/soc/bcm2835/raspberrypi-firmware.h | 2 +- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 4917c5b033fa..eadace4a9339 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -13,6 +13,8 @@ > #include <linux/module.h> > #include <linux/acpi.h> > > +#include <soc/bcm2835/raspberrypi-firmware.h> > + > #include "xhci.h" > #include "xhci-trace.h" > > @@ -308,6 +310,44 @@ static int xhci_pci_setup(struct usb_hcd *hcd) > return xhci_pci_reinit(xhci, pdev); > } > > +/* > + * 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. > + */ > +static int raspberrypi_load_vl805_fw(struct pci_dev *pdev) > +{ > +#ifdef CONFIG_RASPBERRYPI_FIRMWARE > + 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; not sure, but there could be a corner case which should be handled differently. In case the Raspberry Pi 4 has an EEPROM for the VL805 firmware but the firmware is too old to handle RPI_FIRMWARE_NOTIFY_XHCI_RESET, we should return with 0. I don't know there is a change to detect this reliable. I just want to mention this. > + > + dev_dbg(&pdev->dev, "loaded Raspberry Pi's VL805 firmware\n"); > + > +#endif > + return 0; > +} > + > /* > * We need to register our own PCI probe function (instead of the USB core's > * function) in order to create a second roothub under xHCI. > @@ -321,6 +361,16 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) > > driver = (struct hc_driver *)id->driver_data; > > + if (dev->vendor == PCI_VENDOR_ID_VIA && dev->device == 0x3483) { > + retval = raspberrypi_load_vl805_fw(dev); > + if (retval) { > + if (retval != -EPROBE_DEFER) > + dev_err(&dev->dev, > + "Failed to load VL805's firmware"); > + return retval; > + } > + } > + > /* Prevent runtime suspending between USB-2 and USB-3 initialization */ > pm_runtime_get_noresume(&dev->dev); > > diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h > index 7800e12ee042..cc9cdbc66403 100644 > --- a/include/soc/bcm2835/raspberrypi-firmware.h > +++ b/include/soc/bcm2835/raspberrypi-firmware.h > @@ -90,7 +90,7 @@ enum rpi_firmware_property_tag { > RPI_FIRMWARE_SET_PERIPH_REG = 0x00038045, > RPI_FIRMWARE_GET_POE_HAT_VAL = 0x00030049, > RPI_FIRMWARE_SET_POE_HAT_VAL = 0x00030050, > - > + RPI_FIRMWARE_NOTIFY_XHCI_RESET = 0x00030058, In past we updated the firmware mailbox property interface in a bunch as a separate patch. So we can avoid those gaps. Best regards Stefan > > /* Dispmanx TAGS */ > RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE = 0x00040001,
On Mon, 2020-02-17 at 18:47 +0100, Stefan Wahren wrote: > > + > > + 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; > not sure, but there could be a corner case which should be handled > differently. In case the Raspberry Pi 4 has an EEPROM for the VL805 > firmware but the firmware is too old to handle > RPI_FIRMWARE_NOTIFY_XHCI_RESET, we should return with 0. I don't know > there is a change to detect this reliable. I just want to mention this. Ouch, good point, that can be problematic. I'll test that scenario tomorrow. > > diff --git a/include/soc/bcm2835/raspberrypi-firmware.h > > b/include/soc/bcm2835/raspberrypi-firmware.h > > index 7800e12ee042..cc9cdbc66403 100644 > > --- a/include/soc/bcm2835/raspberrypi-firmware.h > > +++ b/include/soc/bcm2835/raspberrypi-firmware.h > > @@ -90,7 +90,7 @@ enum rpi_firmware_property_tag { > > RPI_FIRMWARE_SET_PERIPH_REG = 0x00038045, > > RPI_FIRMWARE_GET_POE_HAT_VAL = 0x00030049, > > RPI_FIRMWARE_SET_POE_HAT_VAL = 0x00030050, > > - > > + RPI_FIRMWARE_NOTIFY_XHCI_RESET = 0x00030058, > > In past we updated the firmware mailbox property interface in a bunch as > a separate patch. So we can avoid those gaps. Ok, I'll change that. Regards, Nicolas
On 2/17/2020 2:07 AM, 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. Inform VideCore that VL805 was just reset, or defer xhci's > probe if not yet joinable trough the mailbox interface. > > Based on Tim Gover's downstream implementation. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Would it work if you registered the firmware loading as pci fixup such that you would not even have to mangle xhci-pci.c at all and all the logic could be contained within drivers/firmware/raspberrypi.c?
On Mon, 2020-02-17 at 12:52 -0800, Florian Fainelli wrote: > > On 2/17/2020 2:07 AM, 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. Inform VideCore that VL805 was just reset, or defer xhci's > > probe if not yet joinable trough the mailbox interface. > > > > Based on Tim Gover's downstream implementation. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > Would it work if you registered the firmware loading as pci fixup such > that you would not even have to mangle xhci-pci.c at all and all the > logic could be contained within drivers/firmware/raspberrypi.c? Not that simple, PCI fix-ups don't allow for probe deferring. We depend on the firmware and mailbox drivers to be up prior running this, so it's essential. We could cheat and do the deferring first thing during pcie-brcmstb's probe. Actually this might be a workable solution (as in upstreamable): - Wait for firmware to be up in pcie-brcmstb.c - Add firmware code in firmware/raspberrypi.c - Perform call in usb's quirk_usb_early_handoff() (usb/host/pci-quirks.c) Regards, Nicolas
On 2/17/2020 1:19 PM, Nicolas Saenz Julienne wrote: > On Mon, 2020-02-17 at 12:52 -0800, Florian Fainelli wrote: >> >> On 2/17/2020 2:07 AM, 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. Inform VideCore that VL805 was just reset, or defer xhci's >>> probe if not yet joinable trough the mailbox interface. >>> >>> Based on Tim Gover's downstream implementation. >>> >>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >> >> Would it work if you registered the firmware loading as pci fixup such >> that you would not even have to mangle xhci-pci.c at all and all the >> logic could be contained within drivers/firmware/raspberrypi.c? > > Not that simple, PCI fix-ups don't allow for probe deferring. We depend on the > firmware and mailbox drivers to be up prior running this, so it's essential. We > could cheat and do the deferring first thing during pcie-brcmstb's probe. > > Actually this might be a workable solution (as in upstreamable): > - Wait for firmware to be up in pcie-brcmstb.c > - Add firmware code in firmware/raspberrypi.c > - Perform call in usb's quirk_usb_early_handoff() (usb/host/pci-quirks.c) Humm, not a big fan of having to have pcie-brcmstb.c become aware of a firmware loader, because this does not scale great over PCIe root complex controller drivers for one and this is such a Pi4 (not even 2711) specialism that it feels a bit too much... might as well go with your existing patch, just address Greg's feedback? Or how about introducing a type of fixup that could trigger a probe deferral?
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 4917c5b033fa..eadace4a9339 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -13,6 +13,8 @@ #include <linux/module.h> #include <linux/acpi.h> +#include <soc/bcm2835/raspberrypi-firmware.h> + #include "xhci.h" #include "xhci-trace.h" @@ -308,6 +310,44 @@ static int xhci_pci_setup(struct usb_hcd *hcd) return xhci_pci_reinit(xhci, pdev); } +/* + * 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. + */ +static int raspberrypi_load_vl805_fw(struct pci_dev *pdev) +{ +#ifdef CONFIG_RASPBERRYPI_FIRMWARE + 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"); + +#endif + return 0; +} + /* * We need to register our own PCI probe function (instead of the USB core's * function) in order to create a second roothub under xHCI. @@ -321,6 +361,16 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) driver = (struct hc_driver *)id->driver_data; + if (dev->vendor == PCI_VENDOR_ID_VIA && dev->device == 0x3483) { + retval = raspberrypi_load_vl805_fw(dev); + if (retval) { + if (retval != -EPROBE_DEFER) + dev_err(&dev->dev, + "Failed to load VL805's firmware"); + return retval; + } + } + /* Prevent runtime suspending between USB-2 and USB-3 initialization */ pm_runtime_get_noresume(&dev->dev); diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h index 7800e12ee042..cc9cdbc66403 100644 --- a/include/soc/bcm2835/raspberrypi-firmware.h +++ b/include/soc/bcm2835/raspberrypi-firmware.h @@ -90,7 +90,7 @@ enum rpi_firmware_property_tag { RPI_FIRMWARE_SET_PERIPH_REG = 0x00038045, RPI_FIRMWARE_GET_POE_HAT_VAL = 0x00030049, RPI_FIRMWARE_SET_POE_HAT_VAL = 0x00030050, - + RPI_FIRMWARE_NOTIFY_XHCI_RESET = 0x00030058, /* Dispmanx TAGS */ RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE = 0x00040001,
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. Based on Tim Gover's downstream implementation. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/usb/host/xhci-pci.c | 50 ++++++++++++++++++++++ include/soc/bcm2835/raspberrypi-firmware.h | 2 +- 2 files changed, 51 insertions(+), 1 deletion(-)