[PATCH v7 0/4] USB: pci-quirks: Add Raspberry Pi 4 quirk

[PATCH v7 0/4] USB: pci-quirks: Add Raspberry Pi 4 quirk

[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
co-processor, VideoCore. This series adds support for the later.

Note that there are a set of constraints we have to consider (some of
them I missed on v1):
 - We need to make sure the VideoCore firmware interface is up and
   running before running the VL805 firmware load call.

 - There is no way to discern RPi4's VL805 chip from other platforms',
   so we need the firmware load to happen *before* running
   quirk_usb_handoff_xhci(). Failure to do so results in an unwarranted
   5 second wait while the fixup code polls xHC's unexisting state.

By Florian's suggestion I've been spending some time exploring the device
link[1] API in order to see if that could save us from explicitly creating
probe dependencies between pcie-brcmstb and firmware/raspberrypi (patch #3).
Technically these dependencies could be inferred from DT. It turns out Saravana
Kannan has been looking at this already. A new boot mechanism, activated with
fw_devlink=on takes care of the device probe ordering on devices with
consumer/supplier relationships. For now this relationship is created based on
the usage of generic DT properties, but has no support for vendor-specifc DT
properties, which we'd be forced to use in order to create a relationship
between our two devices since our setup is highly non generic. There will
probably be at some point support for such properties, and we will then be able
to revisit some of this code.

All this is based on the work by Tim Gover in RPi's downstream
kernel[2].

[1] https://www.kernel.org/doc/html/v4.13/driver-api/device_link.html
[2] https://github.com/raspberrypi/linux/commit/9935b4c7e360b4494b4cb6e3ce797238a1ab78bd

---

Changes since v6:
 - Make rpi_firmware_init_vl805() more robust
 - Rewrite comments and patch descriptions to be more accessible to non RPi
   fluent people
 - Removed Florian's Reviewed-by in patch #2 as function changed
   substantially
 - Tested with/witout u-boot

Changes since v5:
 - Fix issues reported by Kbuild test robot

Changes since v4:
 - Addressed Sergei's comments
 - Fix potential warning in patch #2

Changes since v3:
 - Addressed Greg's comments

There was no v2, my bad.

Changes since v1:
 - Addressed Floarians comments

Nicolas Saenz Julienne (4):
  soc: bcm2835: Add notify xHCI reset property
  firmware: raspberrypi: Introduce vl805 init routine
  PCI: brcmstb: Wait for Raspberry Pi's firmware when present
  USB: pci-quirks: Add Raspberry Pi 4 quirk

 drivers/firmware/Kconfig                   |  3 +-
 drivers/firmware/raspberrypi.c             | 52 ++++++++++++++++++++++
 drivers/pci/controller/pcie-brcmstb.c      | 17 +++++++
 drivers/usb/host/pci-quirks.c              | 16 +++++++
 include/soc/bcm2835/raspberrypi-firmware.h |  9 +++-
 5 files changed, 95 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v7 1/4] soc: bcm2835: Add notify xHCI reset property

[Nicolas Saenz Julienne]

The property is needed in order to trigger VL805's firmware load. Note
that gap between the property introduced and the previous one is due to
the properties not being defined.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/soc/bcm2835/raspberrypi-firmware.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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,
-- 
2.26.2

[PATCH v7 2/4] firmware: raspberrypi: Introduce vl805 init routine

[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 */
+	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__ */
-- 
2.26.2

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

[Nicolas Saenz Julienne]

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>

---

Changes since v6:
- Add more complete comment

 drivers/pci/controller/pcie-brcmstb.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 6d79d14527a6..0b97b94c4a9a 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,26 @@ 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 Raspberry Pi's firmware interface to be up as a
+	 * PCI fixup, rpi_firmware_init_vl805(), depends on it. This driver's
+	 * probe can race with the firmware interface's (see
+	 * drivers/firmware/raspberrypi.c) and potentially break the PCI fixup.
+	 */
+	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.26.2

[PATCH v7 4/4] USB: pci-quirks: Add Raspberry Pi 4 quirk

[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
VideoCore. Inform VideoCore that VL805 was just reset.

Also, as this creates a dependency between USB_PCI and VideoCore's
firmware interface, and since USB_PCI can't be set as a module neither
this can. Reflect that on the firmware interface Kconfg.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---

Changes since v5:
 - Fix Kconfig issue with allmodconfig

Changes since v4:
 - Do not split up error message

Changes since v3:
 - Add more complete error message

Changes since v1:
 - Make RASPBERRYPI_FIRMWARE dependent on this quirk to make sure it
   gets compiled when needed.

 drivers/firmware/Kconfig      |  3 ++-
 drivers/usb/host/pci-quirks.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 8007d4aa76dc..b42140cff8ac 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -178,8 +178,9 @@ config ISCSI_IBFT
 	  Otherwise, say N.
 
 config RASPBERRYPI_FIRMWARE
-	tristate "Raspberry Pi Firmware Driver"
+	bool "Raspberry Pi Firmware Driver"
 	depends on BCM2835_MBOX
+	default USB_PCI
 	help
 	  This option enables support for communicating with the firmware on the
 	  Raspberry Pi.
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index beb2efa71341..0dc34668bb2a 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -16,6 +16,9 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
@@ -1243,11 +1246,24 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 
 static void quirk_usb_early_handoff(struct pci_dev *pdev)
 {
+	int ret;
+
 	/* Skip Netlogic mips SoC's internal PCI USB controller.
 	 * This device does not need/support EHCI/OHCI handoff
 	 */
 	if (pdev->vendor == 0x184e)	/* vendor Netlogic */
 		return;
+
+	if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) {
+		ret = rpi_firmware_init_vl805(pdev);
+		if (ret) {
+			/* Firmware might be outdated, or something failed */
+			dev_warn(&pdev->dev,
+				 "Failed to load VL805's firmware: %d. Will continue to attempt to work, but bad things might happen. You should fix this...\n",
+				 ret);
+		}
+	}
+
 	if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
 			pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
 			pdev->class != PCI_CLASS_SERIAL_USB_EHCI &&
-- 
2.26.2

Re: [PATCH v7 2/4] firmware: raspberrypi: Introduce vl805 init routine

[Stefan Wahren]

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__ */

Re: [PATCH v7 2/4] firmware: raspberrypi: Introduce vl805 init routine

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 2/4] firmware: raspberrypi: Introduce vl805 init routine

[Stefan Wahren]

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

Re: [PATCH v7 2/4] firmware: raspberrypi: Introduce vl805 init routine

[Bjorn Helgaas]

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