Re: [PATCH v8 3/5] usb: xhci: Add support for Renesas controller with memory

From: Vinod Koul
Date: Thu Mar 26 2020 - 07:51:24 EST


On 26-03-20, 13:29, Mathias Nyman wrote:
> Hi Vinod
>
> On 23.3.2020 19.05, Vinod Koul wrote:
> > Some rensas controller like uPD720201 and uPD720202 need firmware to be
> > loaded. Add these devices in table and invoke renesas firmware loader
> > functions to check and load the firmware into device memory when
> > required.
> >
> > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > ---
> > drivers/usb/host/xhci-pci-renesas.c | 1 +
> > drivers/usb/host/xhci-pci.c | 29 ++++++++++++++++++++++++++++-
> > drivers/usb/host/xhci-pci.h | 3 +++
> > 3 files changed, 32 insertions(+), 1 deletion(-)
> >
>
> It's unfortunate if firmware loading couldn't be initiated in a PCI fixup hook
> for this Renesas controller. What was the reason it failed?
>
> Nicolas Saenz Julienne just submitted a solution like that for Raspberry Pi 4
> where firmware loading is initiated in pci-quirks.c quirk_usb_early_handoff()
>
> https://lore.kernel.org/lkml/20200324182812.20420-1-nsaenzjulienne@xxxxxxx
>
> Is he doing something different than what was done for the Renesas controller?

I tried and everytime ended up not getting firmware. Though I did not
investigate a lot. Christian seemed to have tested sometime back as
well.

Another problem is that we dont get driver_data in the quirk and there
didnt seem a way to find the firmware name.

> > diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> > index c588277ac9b8..d413d53df94b 100644
> > --- a/drivers/usb/host/xhci-pci-renesas.c
> > +++ b/drivers/usb/host/xhci-pci-renesas.c
> > @@ -336,6 +336,7 @@ static void renesas_fw_callback(const struct firmware *fw,
> > goto cleanup;
> > }
> >
> > + xhci_pci_probe(pdev, ctx->id);
> > return;
>
> I haven't looked into this but instead of calling xhci_pci_probe() here in the async fw
> loading callback could we just return -EPROBE_DEFER until firmware is loaded when
> xhci_pci_probe() is originally called?

Hmm, initially my thinking was how to tell device core to probe again,
and then digging up I saw wait_for_device_probe() which can be used, let
me try that

> > cleanup:
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index a19752178216..7e63658542ac 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -15,6 +15,7 @@
> >
> > #include "xhci.h"
> > #include "xhci-trace.h"
> > +#include "xhci-pci.h"
> >
> > #define SSIC_PORT_NUM 2
> > #define SSIC_PORT_CFG2 0x880c
> > @@ -312,11 +313,25 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
> > * We need to register our own PCI probe function (instead of the USB core's
> > * function) in order to create a second roothub under xHCI.
> > */
> > -static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > +int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > {
> > int retval;
> > struct xhci_hcd *xhci;
> > struct usb_hcd *hcd;
> > + char *renesas_fw;
> > +
> > + renesas_fw = (char *)id->driver_data;
>
> driver_data is useful for other things than just renesas firmware loading.
> Heikki suggested a long time ago to use it for passing the quirk flags as well, which
> makes sense.
>
> We probably need a structure, something like
>
> struct xhci_driver_data = {
> u64 quirks;
> const char *firmware;
> };
>
> > + if (renesas_fw) {
> > + retval = renesas_xhci_pci_probe(dev, id);
> > + switch (retval) {
> > + case 0: /* fw check success, continue */
> > + break;
> > + case 1: /* fw will be loaded by async load */
> > + return 0;
> > + default: /* error */
> > + return retval;
> > + }
> > + }
> >
>
> If returning -EPROBE_DEFER until firmware is loaded is an option then we would prevent probe
> from returning success while the renesas controller is still loading firmware.
>
> So we would end up with something like this:
> (we can add a quirk flag for renesas firmware loading)
>
> int xhci_pci_probe(..)
> {
> ...
> struct xhci_driver_data *data = id->driver_data;
> if (data && data->quirks & XHCI_RENESAS_FW_QUIRK) {
> if (!xhci_renesas_fw_ready(...))
> return -EPROBE_DEFER
> }
> }
>
> xhci_renesas_fw_ready() would need to initiate firmware loading unless
> firmware is already running or loading.
>
> Would that work for you?

I think yes that should work, let me try that..

--
~Vinod