mbox series

[0/9] Raspberry Pi 4 USB firmware initialization rework

Message ID 20200608192701.18355-1-nsaenzjulienne@suse.de
Headers show
Series Raspberry Pi 4 USB firmware initialization rework | expand

Message

Nicolas Saenz Julienne June 8, 2020, 7:26 p.m. UTC 
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 reworks how we handle this.

The previous solution makes use of PCI quirks and exporting platform
specific functions. Albeit functional it feels pretty shoehorned. This
proposes an alternative way of handling the triggering of the xHCI chip
initialization trough means of a reset controller.

The benefits are pretty evident: less platform churn in core xHCI code,
and no explicit device dependency management in pcie-brcmstb.

Note that patch #1 depend on another series[1].

The series is based on next-20200605.

[1] https://lwn.net/ml/linux-kernel/cover.662a8d401787ef33780d91252a352de91dc4be10.1590594293.git-series.maxime@cerno.tech/

---

Nicolas Saenz Julienne (9):
  dt-bindings: reset: Add a binding for the RPi Firmware USB reset
  reset: Add Raspberry Pi 4 firmware USB reset controller
  ARM: dts: bcm2711: Add firmware usb reset node
  ARM: dts: bcm2711: Add reset controller to xHCI node
  usb: xhci-pci: Add support for reset controllers
  Revert "USB: pci-quirks: Add Raspberry Pi 4 quirk"
  usb: host: pci-quirks: Bypass xHCI quirks for Raspberry Pi 4
  Revert "firmware: raspberrypi: Introduce vl805 init routine"
  Revert "PCI: brcmstb: Wait for Raspberry Pi's firmware when present"

 .../arm/bcm/raspberrypi,bcm2835-firmware.yaml |  21 +++
 arch/arm/boot/dts/bcm2711-rpi-4-b.dts         |  12 ++
 drivers/firmware/Kconfig                      |   3 +-
 drivers/firmware/raspberrypi.c                |  61 ---------
 drivers/pci/controller/pcie-brcmstb.c         |  17 ---
 drivers/reset/Kconfig                         |   9 ++
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-raspberrypi-usb.c         | 122 ++++++++++++++++++
 drivers/usb/host/pci-quirks.c                 |  22 ++--
 drivers/usb/host/xhci-pci.c                   |   9 ++
 include/soc/bcm2835/raspberrypi-firmware.h    |   7 -
 11 files changed, 184 insertions(+), 100 deletions(-)
 create mode 100644 drivers/reset/reset-raspberrypi-usb.c

Comments

Florian Fainelli June 8, 2020, 7:43 p.m. UTC | #1 
On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> Some atypical users of xhci-pci might need to manually reset their xHCI
> controller before starting the HCD setup. Check if a reset controller
> device is available to the PCI bus and trigger a reset.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/xhci-pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index ef513c2fb843..45f70facdfcd 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/acpi.h>
> +#include <linux/reset.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -339,6 +340,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	struct xhci_hcd *xhci;
>  	struct usb_hcd *hcd;
>  	struct xhci_driver_data *driver_data;
> +	struct reset_control *reset;
>  
>  	driver_data = (struct xhci_driver_data *)id->driver_data;
>  	if (driver_data && driver_data->quirks & XHCI_RENESAS_FW_QUIRK) {
> @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  			return retval;
>  	}
>  
> +	reset = devm_reset_control_get(&dev->bus->dev, NULL);

Should not this be devm_reset_control_get_optional()?

> +	if (IS_ERR(reset)) {
> +		retval = PTR_ERR(reset);
> +		return retval;
> +	}
> +	reset_control_reset(reset);
> +
>  	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
>  	pm_runtime_get_noresume(&dev->dev);
>  
>
Florian Fainelli June 8, 2020, 7:50 p.m. UTC | #2 
On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> The board doesn't need the quirks to be run, and takes case of its own

(if you have to resubmit) takes care

> initialization trough a reset controller device. So let's bypass it

its quirk.

> quirk.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/pci-quirks.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 92150ecdb036..4b3be05d1290 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -16,6 +16,8 @@
>  #include <linux/export.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/of.h>
> +
>  #include "pci-quirks.h"
>  #include "xhci-ext-caps.h"
>  
> @@ -1248,6 +1250,16 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
>  	 */
>  	if (pdev->vendor == 0x184e)	/* vendor Netlogic */
>  		return;
> +
> +	/*
> +	 * Bypass the Raspberry Pi 4 controller xHCI controller, things are
> +	 * taken care by the board's co-processor.

taken care of by.

With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> +	 */
> +	if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483 &&
> +	    of_device_is_compatible(of_get_parent(pdev->bus->dev.of_node),
> +				    "brcm,bcm2711-pcie"))
> +		return;
> +
>  	if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
>  			pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
>  			pdev->class != PCI_CLASS_SERIAL_USB_EHCI &&
>
Florian Fainelli June 8, 2020, 8:13 p.m. UTC | #3 
On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> The Raspberry Pi 4 gets its USB functionality from VL805, a PCIe chip
> that implements the xHCI. After a PCI fundamental 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.
> 
> We control this trough a reset controller device that's able to trigger
> the aforementioned process when relevant.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---

[snip]

> +static int rpi_usb_reset_reset(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	struct rpi_usb_reset *priv = to_rpi_usb(rcdev);
> +	u32 dev_addr;
> +	int ret;
> +
> +	/*
> +	 * The pci device address is expected like this:
> +	 *
> +	 * PCI_BUS << 20 | PCI_SLOT << 15 | PCI_FUNC << 12
> +	 *
> +	 * But since rpi's PCIe setup is hardwired, we know the address in
> +	 * advance.
> +	 */
> +	dev_addr = 0x100000;

You could encode the device address as part of the reset identifier,
such that if we ever have more devices to reset, then we only need to
define new identifiers for them, and internally within your reset
controller provide you can resolve that reset identifier 0 is PCI_BUS <<
20 | PCI_SLOT << 15 | PCI_FUN << 12 for instance.

This would make your reset controller define a "#reset-cells" property
to 1 now, such that no further DT ABI breakage would occur if you were
to extend it later on.
Nicolas Saenz Julienne June 9, 2020, 11:18 a.m. UTC | #4 
Hi Florian, thanks for the reviews!

On Mon, 2020-06-08 at 12:43 -0700, Florian Fainelli wrote:
> 
> On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> > Some atypical users of xhci-pci might need to manually reset their xHCI
> > controller before starting the HCD setup. Check if a reset controller
> > device is available to the PCI bus and trigger a reset.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index ef513c2fb843..45f70facdfcd 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> >  #include <linux/acpi.h>
> > +#include <linux/reset.h>
> >  
> >  #include "xhci.h"
> >  #include "xhci-trace.h"
> > @@ -339,6 +340,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > struct pci_device_id *id)
> >  	struct xhci_hcd *xhci;
> >  	struct usb_hcd *hcd;
> >  	struct xhci_driver_data *driver_data;
> > +	struct reset_control *reset;
> >  
> >  	driver_data = (struct xhci_driver_data *)id->driver_data;
> >  	if (driver_data && driver_data->quirks & XHCI_RENESAS_FW_QUIRK) {
> > @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > struct pci_device_id *id)
> >  			return retval;
> >  	}
> >  
> > +	reset = devm_reset_control_get(&dev->bus->dev, NULL);
> 
> Should not this be devm_reset_control_get_optional()?

Yes, you're right.

Regards,
Nicolas

> > +	if (IS_ERR(reset)) {
> > +		retval = PTR_ERR(reset);
> > +		return retval;
> > +	}
> > +	reset_control_reset(reset);
> > +
> >  	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
> >  	pm_runtime_get_noresume(&dev->dev);
> >  
> >
Nicolas Saenz Julienne June 9, 2020, 11:19 a.m. UTC | #5 
On Mon, 2020-06-08 at 22:44 +0300, Andy Shevchenko wrote:
> 
> 
> On Monday, June 8, 2020, Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> wrote:
> > Some atypical users of xhci-pci might need to manually reset their xHCI
> > controller before starting the HCD setup. Check if a reset controller
> > device is available to the PCI bus and trigger a reset.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index ef513c2fb843..45f70facdfcd 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> >  #include <linux/acpi.h>
> > +#include <linux/reset.h>
> > 
> >  #include "xhci.h"
> >  #include "xhci-trace.h"
> > @@ -339,6 +340,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > struct pci_device_id *id)
> >         struct xhci_hcd *xhci;
> >         struct usb_hcd *hcd;
> >         struct xhci_driver_data *driver_data;
> > +       struct reset_control *reset;
> > 
> >         driver_data = (struct xhci_driver_data *)id->driver_data;
> >         if (driver_data && driver_data->quirks & XHCI_RENESAS_FW_QUIRK) {
> > @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > struct pci_device_id *id)
> >                         return retval;
> >         }
> > 
> > +       reset = devm_reset_control_get(&dev->bus->dev, NULL);
> 
>  
> > +       if (IS_ERR(reset)) {
> > +               retval = PTR_ERR(reset);
> > +               return retval;
> > +       }
> 
> These four can be two, we have too many LOCs in the kernel for no reason.

Noted

>  
> > +       reset_control_reset(reset);
> > +
> >         /* Prevent runtime suspending between USB-2 and USB-3 initialization
> > */
> >         pm_runtime_get_noresume(&dev->dev);
> >  
> > -- 
> > 2.26.2
> > 
> > 
> 
>
Philipp Zabel June 9, 2020, 11:59 a.m. UTC | #6 
Hi Nicolas,

On Tue, 2020-06-09 at 13:18 +0200, Nicolas Saenz Julienne wrote:
> Hi Florian, thanks for the reviews!
> 
> On Mon, 2020-06-08 at 12:43 -0700, Florian Fainelli wrote:
> > On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> > > Some atypical users of xhci-pci might need to manually reset their xHCI
> > > controller before starting the HCD setup. Check if a reset controller
> > > device is available to the PCI bus and trigger a reset.
> > > 
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > ---
> > >  drivers/usb/host/xhci-pci.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > > index ef513c2fb843..45f70facdfcd 100644
> > > --- a/drivers/usb/host/xhci-pci.c
> > > +++ b/drivers/usb/host/xhci-pci.c
[...]
> > > @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > > struct pci_device_id *id)
> > >  			return retval;
> > >  	}
> > >  
> > > +	reset = devm_reset_control_get(&dev->bus->dev, NULL);
> > 
> > Should not this be devm_reset_control_get_optional()?
> 
> Yes, you're right.

Please use devm_reset_control_get_optional_exclusive() while you're at
it.

regards
Philipp
Nicolas Saenz Julienne June 9, 2020, 1:08 p.m. UTC | #7 
On Tue, 2020-06-09 at 13:59 +0200, Philipp Zabel wrote:
> Hi Nicolas,
> 
> 
> 
> On Tue, 2020-06-09 at 13:18 +0200, Nicolas Saenz Julienne wrote:
> 
> > Hi Florian, thanks for the reviews!
> > On Mon, 2020-06-08 at 12:43 -0700, Florian Fainelli wrote:
> > > On 6/8/2020 12:26 PM, Nicolas Saenz Julienne wrote:
> > > > Some atypical users of xhci-pci might need to manually reset their xHCI
> > > > controller before starting the HCD setup. Check if a reset controller
> > > > device is available to the PCI bus and trigger a reset.
> > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > > ---
> > > >   drivers/usb/host/xhci-pci.c | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > > > index ef513c2fb843..45f70facdfcd 100644
> > > > --- a/drivers/usb/host/xhci-pci.c
> > > > +++ b/drivers/usb/host/xhci-pci.c
> 
> [...]
> 
> > > > @@ -347,6 +349,13 @@ static int xhci_pci_probe(struct pci_dev *dev,
> > > > const
> > > > struct pci_device_id *id)
> > > >                    return retval;
> > > >    }
> > > >   
> > > > + reset = devm_reset_control_get(&dev->bus->dev, NULL);
> > > Should not this be devm_reset_control_get_optional()?
> > Yes, you're right.
> 
> 
> Please use devm_reset_control_get_optional_exclusive() while you're at
> 
> it.
> 

Will do!

Regards,
Nicolas