| Message ID | 20200429164734.21506-3-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: pci-quirks: Add Raspberry Pi 4 quirk | expand |
Hi Nicolas, Am 29.04.20 um 18:47 schrieb Nicolas Saenz Julienne: > The Raspberry Pi 4 gets its USB functionality from VL805, a PCIe chip > that implements xHCI. After a PCI reset, VL805's firmware may either be > loaded directly from an EEPROM or, if not present, by the SoC's > co-processor, VideoCore. RPi4's VideoCore OS contains both the non public > firmware load logic and the VL805 firmware blob. The function this patch > introduces triggers the aforementioned process. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > Change since v6: > - Add test to avoid loading the firmware when not needed > - Since we have it around, print VL805's firmware version, it'll make > debugging easier in the future > - Correct typos > - Add a clearer view of HW topology in patch description > > 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 | 52 ++++++++++++++++++++++ > include/soc/bcm2835/raspberrypi-firmware.h | 7 +++ > 2 files changed, 59 insertions(+) > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > index da26a584dca0..230c05e53403 100644 > --- a/drivers/firmware/raspberrypi.c > +++ b/drivers/firmware/raspberrypi.c > @@ -12,6 +12,8 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > +#include <linux/pci.h> > +#include <linux/delay.h> > #include <soc/bcm2835/raspberrypi-firmware.h> > > #define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf)) > @@ -19,6 +21,8 @@ > #define MBOX_DATA28(msg) ((msg) & ~0xf) > #define MBOX_CHAN_PROPERTY 8 > > +#define VL805_PCI_CONFIG_VERSION_OFFSET 0x50 > + > static struct platform_device *rpi_hwmon; > static struct platform_device *rpi_clk; > > @@ -286,6 +290,54 @@ struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > } > EXPORT_SYMBOL_GPL(rpi_firmware_get); > > +/* > + * The Raspberry Pi 4 gets its USB functionality from VL805, a PCIe chip that > + * implements xHCI. After a PCI reset, VL805's firmware may either be loaded > + * directly from an EEPROM or, if not present, by the SoC's co-processor, > + * VideoCore. RPi4's VideoCore OS contains both the non public firmware load > + * logic and the VL805 firmware blob. This function triggers the aforementioned > + * process. > + */ > +int rpi_firmware_init_vl805(struct pci_dev *pdev) > +{ > + struct device_node *fw_np; > + struct rpi_firmware *fw; > + u32 dev_addr, version; > + int ret = 0; > + > + 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 -ENODEV; > + > + /* Make sure we don't trigger a firmware load unnecesarely * s/unnecesarely/unnecessarily/ > + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, &version); pci_read_config_dword() can fail, we might want to store the return value? > + if (version) > + goto exit; > + > + 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)); > + /* Wait for vl805 to startup */ > + udelay(200); I know, it makes it harder to read but do we really want to wait unnecessarily if rpi_firmware_property failed? Btw i assume we are in non-atomic context, so maybe it's worth to use usleep_range() here? > + > +exit: > + if (!version) > + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, > + &version); > + pci_info(pdev, "VL805 firmware version %08x\n", version); In case pci_read_config_dword() fails the return code would be more helpful. Best regards > + return ret; > + > +} > +EXPORT_SYMBOL_GPL(rpi_firmware_init_vl805); > + > static const struct of_device_id rpi_firmware_of_match[] = { > { .compatible = "raspberrypi,bcm2835-firmware", }, > {}, > diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h > index cc9cdbc66403..3025aca3c358 100644 > --- a/include/soc/bcm2835/raspberrypi-firmware.h > +++ b/include/soc/bcm2835/raspberrypi-firmware.h > @@ -10,6 +10,7 @@ > #include <linux/of_device.h> > > struct rpi_firmware; > +struct pci_dev; > > enum rpi_firmware_property_status { > RPI_FIRMWARE_STATUS_REQUEST = 0, > @@ -141,6 +142,7 @@ int rpi_firmware_property(struct rpi_firmware *fw, > int rpi_firmware_property_list(struct rpi_firmware *fw, > void *data, size_t tag_size); > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node); > +int rpi_firmware_init_vl805(struct pci_dev *pdev); > #else > static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, > void *data, size_t len) > @@ -158,6 +160,11 @@ static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware > { > return NULL; > } > + > +static inline int rpi_firmware_init_vl805(struct pci_dev *pdev) > +{ > + return 0; > +} > #endif > > #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
Hi Stefan, thanks for the review! On Sat, 2020-05-02 at 11:05 +0200, Stefan Wahren wrote: > > + /* Make sure we don't trigger a firmware load unnecesarely * > s/unnecesarely/unnecessarily/ Noted > > + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, &version); > pci_read_config_dword() can fail, we might want to store the return value? I planned on doing that, but realised that the vast majority of pci_read_config_*() users pass on checking for errors. Bjorn, any rule of thumb on when to check for errors here? > > + if (version) > > + goto exit; > > + > > + 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)); > > + /* Wait for vl805 to startup */ > > + udelay(200); > > I know, it makes it harder to read but do we really want to wait > unnecessarily if rpi_firmware_property failed? Yes, I figured that it wouldn't hurt much at that faulty state, and you'll be waiting some extra 5s further down the line in quirk_usb_handoff_xhci(). But if you feel it's more correct I'll be happy to change it. > Btw i assume we are in non-atomic context, so maybe it's worth to use > usleep_range() here? Of course, I'll fix that. Regards, Nicolas
Hi Nicolas, Am 04.05.20 um 10:59 schrieb Nicolas Saenz Julienne: > Hi Stefan, thanks for the review! > > On Sat, 2020-05-02 at 11:05 +0200, Stefan Wahren wrote: >>> + if (version) >>> + goto exit; >>> + >>> + 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)); >>> + /* Wait for vl805 to startup */ >>> + udelay(200); >> I know, it makes it harder to read but do we really want to wait >> unnecessarily if rpi_firmware_property failed? > Yes, I figured that it wouldn't hurt much at that faulty state, and you'll be > waiting some extra 5s further down the line in quirk_usb_handoff_xhci(). > > But if you feel it's more correct I'll be happy to change it. no, i don't insist on that. Best regards Stefan
On Mon, May 04, 2020 at 10:59:29AM +0200, Nicolas Saenz Julienne wrote: > On Sat, 2020-05-02 at 11:05 +0200, Stefan Wahren wrote: > > > + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, &version); > > pci_read_config_dword() can fail, we might want to store the return value? > > I planned on doing that, but realised that the vast majority of > pci_read_config_*() users pass on checking for errors. > > Bjorn, any rule of thumb on when to check for errors here? Not really. It *can* fail, for sure. If it does fail, you normally get ~0 data, which means you would skip the firmware load, do another config read (which probably also returns ~0) and print firmware version ffffffff, and the device probably won't work. But checking doesn't get you much other than a better error message. Personally I probably wouldn't bother because it clutters the code so much for so little benefit. Bjorn
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c index da26a584dca0..230c05e53403 100644 --- a/drivers/firmware/raspberrypi.c +++ b/drivers/firmware/raspberrypi.c @@ -12,6 +12,8 @@ #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/pci.h> +#include <linux/delay.h> #include <soc/bcm2835/raspberrypi-firmware.h> #define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf)) @@ -19,6 +21,8 @@ #define MBOX_DATA28(msg) ((msg) & ~0xf) #define MBOX_CHAN_PROPERTY 8 +#define VL805_PCI_CONFIG_VERSION_OFFSET 0x50 + static struct platform_device *rpi_hwmon; static struct platform_device *rpi_clk; @@ -286,6 +290,54 @@ struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) } EXPORT_SYMBOL_GPL(rpi_firmware_get); +/* + * The Raspberry Pi 4 gets its USB functionality from VL805, a PCIe chip that + * implements xHCI. After a PCI reset, VL805's firmware may either be loaded + * directly from an EEPROM or, if not present, by the SoC's co-processor, + * VideoCore. RPi4's VideoCore OS contains both the non public firmware load + * logic and the VL805 firmware blob. This function triggers the aforementioned + * process. + */ +int rpi_firmware_init_vl805(struct pci_dev *pdev) +{ + struct device_node *fw_np; + struct rpi_firmware *fw; + u32 dev_addr, version; + int ret = 0; + + 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 -ENODEV; + + /* Make sure we don't trigger a firmware load unnecesarely */ + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, &version); + if (version) + goto exit; + + 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)); + /* Wait for vl805 to startup */ + udelay(200); + +exit: + if (!version) + pci_read_config_dword(pdev, VL805_PCI_CONFIG_VERSION_OFFSET, + &version); + pci_info(pdev, "VL805 firmware version %08x\n", version); + return ret; + +} +EXPORT_SYMBOL_GPL(rpi_firmware_init_vl805); + static const struct of_device_id rpi_firmware_of_match[] = { { .compatible = "raspberrypi,bcm2835-firmware", }, {}, diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h index cc9cdbc66403..3025aca3c358 100644 --- a/include/soc/bcm2835/raspberrypi-firmware.h +++ b/include/soc/bcm2835/raspberrypi-firmware.h @@ -10,6 +10,7 @@ #include <linux/of_device.h> struct rpi_firmware; +struct pci_dev; enum rpi_firmware_property_status { RPI_FIRMWARE_STATUS_REQUEST = 0, @@ -141,6 +142,7 @@ int rpi_firmware_property(struct rpi_firmware *fw, int rpi_firmware_property_list(struct rpi_firmware *fw, void *data, size_t tag_size); struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node); +int rpi_firmware_init_vl805(struct pci_dev *pdev); #else static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, void *data, size_t len) @@ -158,6 +160,11 @@ static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware { return NULL; } + +static inline int rpi_firmware_init_vl805(struct pci_dev *pdev) +{ + return 0; +} #endif #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
The Raspberry Pi 4 gets its USB functionality from VL805, a PCIe chip that implements xHCI. After a PCI reset, VL805's firmware may either be loaded directly from an EEPROM or, if not present, by the SoC's co-processor, VideoCore. RPi4's VideoCore OS contains both the non public firmware load logic and the VL805 firmware blob. The function this patch introduces triggers the aforementioned process. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- Change since v6: - Add test to avoid loading the firmware when not needed - Since we have it around, print VL805's firmware version, it'll make debugging easier in the future - Correct typos - Add a clearer view of HW topology in patch description 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 | 52 ++++++++++++++++++++++ include/soc/bcm2835/raspberrypi-firmware.h | 7 +++ 2 files changed, 59 insertions(+)