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