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

[PATCH v6 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
VideCore. 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.

---

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: Sync xHCI reset firmware property with downstream
  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             | 38 ++++++++++++++++++++++
 drivers/pci/controller/pcie-brcmstb.c      | 15 +++++++++
 drivers/usb/host/pci-quirks.c              | 16 +++++++++
 include/soc/bcm2835/raspberrypi-firmware.h |  9 ++++-
 5 files changed, 79 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v6 1/4] soc: bcm2835: Sync xHCI reset firmware property with downstream

[Nicolas Saenz Julienne]

The property is needed in order to trigger VL805's firmware load. Note
that there is a gap between the property introduced and the previous
one. This is also the case downstream.

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.25.1

Re: [PATCH v6 1/4] soc: bcm2835: Sync xHCI reset firmware property with downstream

[Bjorn Helgaas]

On Tue, Mar 24, 2020 at 07:28:09PM +0100, Nicolas Saenz Julienne wrote:
> The property is needed in order to trigger VL805's firmware load. Note
> that there is a gap between the property introduced and the previous
> one. This is also the case downstream.

I don't know what "downstream" means, so I don't know what we're
syncing *with*.  If there's another branch or project we need to
coordinate with, is there a name or URL that would help facilitate
that?  If not, I'm not sure what value that sentence adds.

Bjorn

Re: [PATCH v6 1/4] soc: bcm2835: Sync xHCI reset firmware property with downstream

[Florian Fainelli]

On 4/2/2020 11:01 AM, Bjorn Helgaas wrote:
> On Tue, Mar 24, 2020 at 07:28:09PM +0100, Nicolas Saenz Julienne wrote:
>> The property is needed in order to trigger VL805's firmware load. Note
>> that there is a gap between the property introduced and the previous
>> one. This is also the case downstream.
> 
> I don't know what "downstream" means, so I don't know what we're
> syncing *with*.  If there's another branch or project we need to
> coordinate with, is there a name or URL that would help facilitate
> that?  If not, I'm not sure what value that sentence adds.

Downstream here means the Raspberry Pi maintained kernel whose tree can
be found here:

https://github.com/raspberrypi/linux/

Changes appear there first, and people like Nicolas and Stefan try to
get them upstream from that tree into Linus' tree.
-- 
Florian

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

[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. The function informs VideCore that VL805 was just reset, or
requests for a probe defer.

Based on Tim Gover's downstream implementation.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

---
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 <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/pci.h>
 #include <soc/bcm2835/raspberrypi-firmware.h>
 
 #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.
+ */
+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", },
 	{},
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.25.1

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

[Bjorn Helgaas]

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.

> Based on Tim Gover's downstream implementation.

Maybe a URL?

> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> ---
> 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 <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/pci.h>
>  #include <soc/bcm2835/raspberrypi-firmware.h>
>  
>  #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/

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.

> + */
> +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", },
>  	{},
> 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.25.1
> 

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

[Nicolas Saenz Julienne]

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

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 <nsaenzjulienne@suse.de>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > ---
> > 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 <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > +#include <linux/pci.h>
> >  #include <soc/bcm2835/raspberrypi-firmware.h>
> >  
> >  #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


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

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

[Bjorn Helgaas]

On Thu, Apr 02, 2020 at 01:32:35PM +0200, Nicolas Saenz Julienne wrote:
> 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.

Is VL805 the XHCI USB device?  A hint here would help non-RPi experts
know how this fits into the topology.

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

I think your impression is correct.  If this was posted to a mailing
list archived on lore.kernel.org, a link to the cover letter would be
ideal.

> 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.

OK.  The "rpi_firmware_property" name doesn't give much of a hint that
it is sending messages.  It sounds like it might be a lookup function.
But that's an existing thing, not something you're changing here.

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

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

[Nicolas Saenz Julienne]

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

On Thu, 2020-04-02 at 14:40 -0500, Bjorn Helgaas wrote:
> On Thu, Apr 02, 2020 at 01:32:35PM +0200, Nicolas Saenz Julienne wrote:
> > 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.
> 
> Is VL805 the XHCI USB device?  A hint here would help non-RPi experts
> know how this fits into the topology.

Yes, VL805 is the XHCI USB device. I'll keep it in mind for the next series.

Regards,
Nicolas


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

[PATCH v6 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>
---
 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.
+	 */
+	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.25.1

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

[Bjorn Helgaas]

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

Is there a function name for this fixup that you can mention?

> 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.

I guess "both initializations" means brcm_pcie_probe() and something
else?  It'd be nice to include that function name here, too.

> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  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.

> +	 */
> +	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.25.1
> 

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

[Nicolas Saenz Julienne]

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

Hi Bjorn,

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
> 
> Is there a function name for this fixup that you can mention?

Yes, rpi_firmware_init_vl805(), I'll update the description.

> > 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.
> 
> I guess "both initializations" means brcm_pcie_probe() and something
> else?  It'd be nice to include that function name here, too.

Noted, I'll be more explicit on the next version of the series. More in depth
explanation below.

> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  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. 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. I even looked into adding -EPROBE_DEFER support to fixups, but I fear
that would entail moving them into the core device definition.

Regards,
Nicolas


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

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@suse.de>
> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > >  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

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

[Nicolas Saenz Julienne]

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

On Thu, 2020-04-02 at 14:38 -0500, Bjorn Helgaas wrote:
> [+cc Rob for DT platform device dependency question]
> 
> On Thu, Apr 02, 2020 at 04:27:23PM +0200, Nicolas Saenz Julienne wrote:

[...]

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

Yes, and also resets the PCI bus, which will clear VL805's firmware (the XHCI
chip).

>   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?

And yes, that's the general idea.

> 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?

That's ultimately what this patch tries to achieve. If there was a way to
offload it to DT it would be way nicer.

Regards,
Nicolas


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

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

[Florian Fainelli]

On 4/4/2020 12:20 PM, Nicolas Saenz Julienne wrote:
> On Thu, 2020-04-02 at 14:38 -0500, Bjorn Helgaas wrote:
>> [+cc Rob for DT platform device dependency question]
>>
>> On Thu, Apr 02, 2020 at 04:27:23PM +0200, Nicolas Saenz Julienne wrote:
> 
> [...]
> 
>>> 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,
> 
> Yes, and also resets the PCI bus, which will clear VL805's firmware (the XHCI
> chip).
> 
>>   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?
> 
> And yes, that's the general idea.
> 
>> 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?
> 
> That's ultimately what this patch tries to achieve. If there was a way to
> offload it to DT it would be way nicer.

Have you looked whether device links between producer/consumer could
help here? AFAICT there is not usually a way to express an ordering
dependency other than by referencing symbols from a different module,
which falls apart when everything gets built into the kernel obviously
and then you are at the mercy of the linking order and initicall levels.

https://www.kernel.org/doc/html/v4.13/driver-api/device_link.html
-- 
Florian

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

Also, as this creates a dependency between USB_PCI and VideoCore's
firmware interface. Since USB_PCI can't be set as a module neither this
should.

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 ea869addc89b..78ab2ad6d3f0 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.25.1