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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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?


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

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

-Mathias