Message ID | 20200608192701.18355-1-nsaenzjulienne@suse.de |
---|---|
Headers | show |
Series | Raspberry Pi 4 USB firmware initialization rework | expand |
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); > >
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 && >
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.
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); > > > >
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 > > > > > >
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
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