diff mbox series

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

Message ID 20200323170601.419809-4-vkoul@kernel.org (mailing list archive)
State Superseded
Headers show
Series usb: xhci: Add support for Renesas USB controllers | expand

Commit Message

Vinod Koul March 23, 2020, 5:05 p.m. UTC
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(-)

Comments

Mathias Nyman March 26, 2020, 11:29 a.m. UTC | #1
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
Vinod Koul March 26, 2020, 11:51 a.m. UTC | #2
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 Koul April 1, 2020, 12:57 p.m. UTC | #3
On 26-03-20, 17:21, Vinod Koul wrote:
> 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

Sorry to report back that it doesn't work as planned :(

I modified the code to invoke the request_firmware_nowait() which will load
the firmware and provide the firmware in callback. Meanwhile return -EPROBE_DEFER.

After a bit, the core invokes the driver probe again and we hit the
roadblock. The request_firmware uses devres and allocates resources for
loading the firmware. The problem is that device core checks for this:

bus: 'pci': really_probe: probing driver xhci_hcd_pci with device 0000:01:00.0
pci 0000:01:00.0: Resources present before probing

And here the probe fails. In some cases the firmware_callback finishes
before this and we can probe again, but that is not very reliable.

I tested another way to use request_firmware() (sync version) and then
load the firmware in probe and load. The request is done only for
renesas devices if they dont have firmware already running.
So rest of the devices wont have any impact.

Now should we continue this way in the patchset or move to sync version.
Am okay either way.
Christian Lamparter April 1, 2020, 3:39 p.m. UTC | #4
Hello,

On Wednesday, 1 April 2020 14:57:48 CEST Vinod Koul wrote:
> On 26-03-20, 17:21, Vinod Koul wrote:
> > On 26-03-20, 13:29, Mathias Nyman wrote:
> > > 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
> 
> Sorry to report back that it doesn't work as planned :(
> 
> I modified the code to invoke the request_firmware_nowait() which will load
> the firmware and provide the firmware in callback. Meanwhile return -EPROBE_DEFER.
> 
> After a bit, the core invokes the driver probe again and we hit the
> roadblock. The request_firmware uses devres and allocates resources for
> loading the firmware. The problem is that device core checks for this:
> 
> bus: 'pci': really_probe: probing driver xhci_hcd_pci with device 0000:01:00.0
> pci 0000:01:00.0: Resources present before probing
> 
> And here the probe fails. In some cases the firmware_callback finishes
> before this and we can probe again, but that is not very reliable.
> 
> I tested another way to use request_firmware() (sync version) and then
> load the firmware in probe and load. The request is done only for
> renesas devices if they dont have firmware already running.
> So rest of the devices wont have any impact.
> 
> Now should we continue this way in the patchset or move to sync version.
> Am okay either way.

Just a word of caution.

The problem with the usage of "sync" request_firmware in drivers is that if the
code is built into the kernel the request_firmware() could be called before the
(root) filesystem on which the firmware resides is ready.... So this will get
weird during boot because what is the sync request_firmware() going to do? From what
I know, this is why the funny _async firmware request APIs are even a thing...

(I took a quick peek into the RPI 4 code, but unlike this code it seems to fetch
from nvmem/eeprom, is this right? I had a tons-of-fun dealing with caldata and
firmware on UBIFS in UBI Volumes. So I'm prepared to test this cases. :D )

(Another possibility would be to use request_firmware_direct() here.
Though, I don't know if it would be considered API Abuse to -EPROBE_DEFER
on errors of request_firmware_direct() in order to wait for FSes to appear )

Regards,
Christian
Vinod Koul April 1, 2020, 4:18 p.m. UTC | #5
Hi Christian,

On 01-04-20, 17:39, Christian Lamparter wrote:
> On Wednesday, 1 April 2020 14:57:48 CEST Vinod Koul wrote:
> > On 26-03-20, 17:21, Vinod Koul wrote:
> > > On 26-03-20, 13:29, Mathias Nyman wrote:
> > > > 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
> > 
> > Sorry to report back that it doesn't work as planned :(
> > 
> > I modified the code to invoke the request_firmware_nowait() which will load
> > the firmware and provide the firmware in callback. Meanwhile return -EPROBE_DEFER.
> > 
> > After a bit, the core invokes the driver probe again and we hit the
> > roadblock. The request_firmware uses devres and allocates resources for
> > loading the firmware. The problem is that device core checks for this:
> > 
> > bus: 'pci': really_probe: probing driver xhci_hcd_pci with device 0000:01:00.0
> > pci 0000:01:00.0: Resources present before probing
> > 
> > And here the probe fails. In some cases the firmware_callback finishes
> > before this and we can probe again, but that is not very reliable.
> > 
> > I tested another way to use request_firmware() (sync version) and then
> > load the firmware in probe and load. The request is done only for
> > renesas devices if they dont have firmware already running.
> > So rest of the devices wont have any impact.
> > 
> > Now should we continue this way in the patchset or move to sync version.
> > Am okay either way.
> 
> Just a word of caution.
> 
> The problem with the usage of "sync" request_firmware in drivers is that if the
> code is built into the kernel the request_firmware() could be called before the
> (root) filesystem on which the firmware resides is ready.... So this will get
> weird during boot because what is the sync request_firmware() going to do? From what
> I know, this is why the funny _async firmware request APIs are even a thing...

So is your usage a module or inbuilt. I am using it as a module, so
seems okay. For inbuilt, someone needs to do make it in kernel or make
sure the initramfs has this :)

> (I took a quick peek into the RPI 4 code, but unlike this code it seems to fetch
> from nvmem/eeprom, is this right? I had a tons-of-fun dealing with caldata and
> firmware on UBIFS in UBI Volumes. So I'm prepared to test this cases. :D )

Okay I will send you this version to play around with.

> (Another possibility would be to use request_firmware_direct() here.
> Though, I don't know if it would be considered API Abuse to -EPROBE_DEFER
> on errors of request_firmware_direct() in order to wait for FSes to appear )

As I said we need to either accept the current change of having async
version and have firmware callback invoke probe to complete the load
action or move to sync. Which of the lesser devils are we going to be
happy with :)
Christian Lamparter April 4, 2020, 10:08 a.m. UTC | #6
Hi,

On Wednesday, 1 April 2020 18:18:52 CEST Vinod Koul wrote:
> On 01-04-20, 17:39, Christian Lamparter wrote:
> > On Wednesday, 1 April 2020 14:57:48 CEST Vinod Koul wrote:
> > > On 26-03-20, 17:21, Vinod Koul wrote:
> > > > On 26-03-20, 13:29, Mathias Nyman wrote:
> > > > > 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
> > > 
> > > Sorry to report back that it doesn't work as planned :(
> > > 
> > > I modified the code to invoke the request_firmware_nowait() which will load
> > > the firmware and provide the firmware in callback. Meanwhile return -EPROBE_DEFER.
> > > 
> > > After a bit, the core invokes the driver probe again and we hit the
> > > roadblock. The request_firmware uses devres and allocates resources for
> > > loading the firmware. The problem is that device core checks for this:
> > > 
> > > bus: 'pci': really_probe: probing driver xhci_hcd_pci with device 0000:01:00.0
> > > pci 0000:01:00.0: Resources present before probing
> > > 
> > > And here the probe fails. In some cases the firmware_callback finishes
> > > before this and we can probe again, but that is not very reliable.
> > > 
> > > I tested another way to use request_firmware() (sync version) and then
> > > load the firmware in probe and load. The request is done only for
> > > renesas devices if they dont have firmware already running.
> > > So rest of the devices wont have any impact.
> > > 
> > > Now should we continue this way in the patchset or move to sync version.
> > > Am okay either way.
> > 
> > Just a word of caution.
> > 
> > The problem with the usage of "sync" request_firmware in drivers is that if the
> > code is built into the kernel the request_firmware() could be called before the
> > (root) filesystem on which the firmware resides is ready.... So this will get
> > weird during boot because what is the sync request_firmware() going to do? From what
> > I know, this is why the funny _async firmware request APIs are even a thing...
> 
> So is your usage a module or inbuilt. I am using it as a module, so
> seems okay. For inbuilt, someone needs to do make it in kernel or make
> sure the initramfs has this :)

Yes, after testing on the device, I can state that everything went as intended :-).

Cheers,
Christian
diff mbox series

Patch

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;
 
 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;
+	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;
+		}
+	}
 
 	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
 	pm_runtime_get_noresume(&dev->dev);
@@ -379,6 +394,11 @@  static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 static void xhci_pci_remove(struct pci_dev *dev)
 {
 	struct xhci_hcd *xhci;
+	int err;
+
+	err = renesas_xhci_pci_remove(dev);
+	if (err)
+		return;
 
 	xhci = hcd_to_xhci(pci_get_drvdata(dev));
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
@@ -534,12 +554,19 @@  static void xhci_pci_shutdown(struct usb_hcd *hcd)
 
 /* PCI driver selection metadata; PCI hotplugging uses this */
 static const struct pci_device_id pci_ids[] = {
+	{ PCI_DEVICE(0x1912, 0x0014),
+		.driver_data =  (unsigned long)"renesas_usb_fw.mem",
+	},
+	{ PCI_DEVICE(0x1912, 0x0015),
+		.driver_data =  (unsigned long)"renesas_usb_fw.mem",
+	},
 	/* handle any USB 3.0 xHCI controller */
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_XHCI, ~0),
 	},
 	{ /* end: all zeroes */ }
 };
 MODULE_DEVICE_TABLE(pci, pci_ids);
+MODULE_FIRMWARE("renesas_usb_fw.mem");
 
 /* pci driver glue; this is a "new style" PCI driver module */
 static struct pci_driver xhci_pci_driver = {
diff --git a/drivers/usb/host/xhci-pci.h b/drivers/usb/host/xhci-pci.h
index 092909dd85a0..1b4330f893fa 100644
--- a/drivers/usb/host/xhci-pci.h
+++ b/drivers/usb/host/xhci-pci.h
@@ -7,5 +7,8 @@ 
 int renesas_xhci_pci_probe(struct pci_dev *dev,
 			   const struct pci_device_id *id);
 int renesas_xhci_pci_remove(struct pci_dev *dev);
+
+int xhci_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id);
+
 #endif