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

[Vinod Koul <vkoul@kernel.org>]

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@kernel.org>
> > ---
> >  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@suse.de
> 
> 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