[PATCH 0/4] usb: host: xhci-{plat,rcar}: clean up and add a workaround

[PATCH 0/4] usb: host: xhci-{plat,rcar}: clean up and add a workaround

[Yoshihiro Shimoda]

This patch series is based on the latest Greg's linux-usb.git /
usb-next branch.

From patch 1 to 3 are clean-up patches and patch 4 is adding
a workaround code for a specific condition. So, I don't think
the patch 4 is not a fixed patch.

Yoshihiro Shimoda (4):
  usb: host: xhci-plat: add quirks member into struct xhci_plat_priv
  usb: host: xhci-rcar: Add a helper macro to set xhci_plat_priv
  usb: host: xhci-rcar: Use xhci_plat_priv.quirks instead of code
    settings
  usb: host: xhci-rcar: avoid 60s wait by request_firmware() in system
    booting

 drivers/usb/host/xhci-plat.c | 14 +++++---------
 drivers/usb/host/xhci-plat.h |  2 ++
 drivers/usb/host/xhci-rcar.c | 34 +++++-----------------------------
 drivers/usb/host/xhci-rcar.h | 21 +++++++++++++++++++++
 4 files changed, 33 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH 1/4] usb: host: xhci-plat: add quirks member into struct xhci_plat_priv

[Yoshihiro Shimoda]

To simplify adding xhci->quirks instead of the .init_quirk()
function, this patch adds a new parameter "quirks" into
the struct xhci_plat_priv.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/host/xhci-plat.c | 4 +++-
 drivers/usb/host/xhci-plat.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index a1e5ce4..1843b69 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -66,12 +66,14 @@ static int xhci_priv_resume_quirk(struct usb_hcd *hcd)
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {
+	struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+
 	/*
 	 * As of now platform drivers don't provide MSI support so we ensure
 	 * here that the generic code does not try to make a pci_dev from our
 	 * dev struct in order to setup MSI
 	 */
-	xhci->quirks |= XHCI_PLAT;
+	xhci->quirks |= XHCI_PLAT | priv->quirks;
 }
 
 /* called during probe() after chip reset completes */
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index ae29f22..5681723 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -12,10 +12,12 @@
 
 struct xhci_plat_priv {
 	const char *firmware_name;
+	unsigned long long quirks;
 	void (*plat_start)(struct usb_hcd *);
 	int (*init_quirk)(struct usb_hcd *);
 	int (*resume_quirk)(struct usb_hcd *);
 };
 
 #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
+#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
 #endif	/* _XHCI_PLAT_H */
-- 
2.7.4

[PATCH 2/4] usb: host: xhci-rcar: Add a helper macro to set xhci_plat_priv

[Yoshihiro Shimoda]

To avoid copy-and-paste setting of xhci_plat_priv for R-Car SoCs,
this patch add a helper macro SET_XHCI_PLAT_PRIV_FOR_RCAR.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/host/xhci-plat.c | 10 ++--------
 drivers/usb/host/xhci-rcar.h |  7 +++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1843b69..d90cd5e 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -105,17 +105,11 @@ static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = {
 };
 
 static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen2 = {
-	.firmware_name = XHCI_RCAR_FIRMWARE_NAME_V1,
-	.init_quirk = xhci_rcar_init_quirk,
-	.plat_start = xhci_rcar_start,
-	.resume_quirk = xhci_rcar_resume_quirk,
+	SET_XHCI_PLAT_PRIV_FOR_RCAR(XHCI_RCAR_FIRMWARE_NAME_V1)
 };
 
 static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen3 = {
-	.firmware_name = XHCI_RCAR_FIRMWARE_NAME_V3,
-	.init_quirk = xhci_rcar_init_quirk,
-	.plat_start = xhci_rcar_start,
-	.resume_quirk = xhci_rcar_resume_quirk,
+	SET_XHCI_PLAT_PRIV_FOR_RCAR(XHCI_RCAR_FIRMWARE_NAME_V3)
 };
 
 static const struct of_device_id usb_xhci_of_match[] = {
diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
index 804b6ab..1f8c225 100644
--- a/drivers/usb/host/xhci-rcar.h
+++ b/drivers/usb/host/xhci-rcar.h
@@ -31,4 +31,11 @@ static inline int xhci_rcar_resume_quirk(struct usb_hcd *hcd)
 	return 0;
 }
 #endif
+
+#define SET_XHCI_PLAT_PRIV_FOR_RCAR(firmware)				\
+	.firmware_name = firmware,					\
+	.init_quirk = xhci_rcar_init_quirk,				\
+	.plat_start = xhci_rcar_start,					\
+	.resume_quirk = xhci_rcar_resume_quirk,
+
 #endif /* _XHCI_RCAR_H */
-- 
2.7.4

[PATCH 3/4] usb: host: xhci-rcar: Use xhci_plat_priv.quirks instead of code settings

[Yoshihiro Shimoda]

This patch uses xhci_plat_priv.quirks to simplify. The previous
code had conditions to set some quirks in xhci_rcar_init_quirk().
But, the xhci_rcar_init_quirk() is called at the same conditions.
So, no behavior change.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/host/xhci-rcar.c | 28 ----------------------------
 drivers/usb/host/xhci-rcar.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
index 8616c52..34761be 100644
--- a/drivers/usb/host/xhci-rcar.c
+++ b/drivers/usb/host/xhci-rcar.c
@@ -107,15 +107,6 @@ static int xhci_rcar_is_gen2(struct device *dev)
 		of_device_is_compatible(node, "renensas,rcar-gen2-xhci");
 }
 
-static int xhci_rcar_is_gen3(struct device *dev)
-{
-	struct device_node *node = dev->of_node;
-
-	return of_device_is_compatible(node, "renesas,xhci-r8a7795") ||
-		of_device_is_compatible(node, "renesas,xhci-r8a7796") ||
-		of_device_is_compatible(node, "renesas,rcar-gen3-xhci");
-}
-
 void xhci_rcar_start(struct usb_hcd *hcd)
 {
 	u32 temp;
@@ -226,32 +217,13 @@ static bool xhci_rcar_wait_for_pll_active(struct usb_hcd *hcd)
 /* This function needs to initialize a "phy" of usb before */
 int xhci_rcar_init_quirk(struct usb_hcd *hcd)
 {
-	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-
 	/* If hcd->regs is NULL, we don't just call the following function */
 	if (!hcd->regs)
 		return 0;
 
-	/*
-	 * On R-Car Gen2 and Gen3, the AC64 bit (bit 0) of HCCPARAMS1 is set
-	 * to 1. However, these SoCs don't support 64-bit address memory
-	 * pointers. So, this driver clears the AC64 bit of xhci->hcc_params
-	 * to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in
-	 * xhci_gen_setup().
-	 *
-	 * And, since the firmware/internal CPU control the USBSTS.STS_HALT
-	 * and the process speed is down when the roothub port enters U3,
-	 * long delay for the handshake of STS_HALT is neeed in xhci_suspend().
-	 */
-	if (xhci_rcar_is_gen2(hcd->self.controller) ||
-			xhci_rcar_is_gen3(hcd->self.controller)) {
-		xhci->quirks |= XHCI_NO_64BIT_SUPPORT | XHCI_SLOW_SUSPEND;
-	}
-
 	if (!xhci_rcar_wait_for_pll_active(hcd))
 		return -ETIMEDOUT;
 
-	xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 	return xhci_rcar_download_firmware(hcd);
 }
 
diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
index 1f8c225..012744a 100644
--- a/drivers/usb/host/xhci-rcar.h
+++ b/drivers/usb/host/xhci-rcar.h
@@ -32,8 +32,22 @@ static inline int xhci_rcar_resume_quirk(struct usb_hcd *hcd)
 }
 #endif
 
+/*
+ * On R-Car Gen2 and Gen3, the AC64 bit (bit 0) of HCCPARAMS1 is set
+ * to 1. However, these SoCs don't support 64-bit address memory
+ * pointers. So, this driver clears the AC64 bit of xhci->hcc_params
+ * to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in
+ * xhci_gen_setup() by using the XHCI_NO_64BIT_SUPPORT quirk.
+ *
+ * And, since the firmware/internal CPU control the USBSTS.STS_HALT
+ * and the process speed is down when the roothub port enters U3,
+ * long delay for the handshake of STS_HALT is neeed in xhci_suspend()
+ * by using the XHCI_SLOW_SUSPEND quirk.
+ */
 #define SET_XHCI_PLAT_PRIV_FOR_RCAR(firmware)				\
 	.firmware_name = firmware,					\
+	.quirks = XHCI_NO_64BIT_SUPPORT | XHCI_TRUST_TX_LENGTH |	\
+		  XHCI_SLOW_SUSPEND,					\
 	.init_quirk = xhci_rcar_init_quirk,				\
 	.plat_start = xhci_rcar_start,					\
 	.resume_quirk = xhci_rcar_resume_quirk,
-- 
2.7.4

[PATCH 4/4] usb: host: xhci-rcar: avoid 60s wait by request_firmware() in system booting

[Yoshihiro Shimoda]

If CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y and CONFIG_USB_XHCI_RCAR=y,
request_firmware() in xhci_rcar_download_firmware() waits for 60s to
sysfs fallback for the firmware like below.

[    1.599701] xhci-hcd ee000000.usb: xHCI Host Controller
[    1.604948] xhci-hcd ee000000.usb: new USB bus registered, assigned bus number 3
[    1.612403] xhci-hcd ee000000.usb: Direct firmware load for r8a779x_usb3_v3.dlmem failed with error -2
[    1.621726] xhci-hcd ee000000.usb: Falling back to sysfs fallback for: r8a779x_usb3_v3.dlmem
[    1.707953] ata1: link resume succeeded after 1 retries
[    1.819379] ata1: SATA link down (SStatus 0 SControl 300)
[   62.436012] xhci-hcd ee000000.usb: can't setup: -11
[   62.440901] xhci-hcd ee000000.usb: USB bus 3 deregistered
[   62.446361] xhci-hcd: probe of ee000000.usb failed with error -11

To avoid this 60s wait, this patch adds to check the system_state
condition and if the system is not running,
xhci_rcar_download_firmware() calls request_firmware_direct()
instead of request_firmware() as a workaround.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/host/xhci-rcar.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
index 34761be..c90cf46 100644
--- a/drivers/usb/host/xhci-rcar.c
+++ b/drivers/usb/host/xhci-rcar.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/firmware.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
@@ -146,7 +147,10 @@ static int xhci_rcar_download_firmware(struct usb_hcd *hcd)
 		firmware_name = priv->firmware_name;
 
 	/* request R-Car USB3.0 firmware */
-	retval = request_firmware(&fw, firmware_name, dev);
+	if (system_state < SYSTEM_RUNNING)
+		retval = request_firmware_direct(&fw, firmware_name, dev);
+	else
+		retval = request_firmware(&fw, firmware_name, dev);
 	if (retval)
 		return retval;
 
-- 
2.7.4

Re: [PATCH 1/4] usb: host: xhci-plat: add quirks member into struct xhci_plat_priv

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> To simplify adding xhci->quirks instead of the .init_quirk()
> function, this patch adds a new parameter "quirks" into
> the struct xhci_plat_priv.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/usb/host/xhci-plat.h
> +++ b/drivers/usb/host/xhci-plat.h
> @@ -12,10 +12,12 @@
>
>  struct xhci_plat_priv {
>         const char *firmware_name;
> +       unsigned long long quirks;
>         void (*plat_start)(struct usb_hcd *);
>         int (*init_quirk)(struct usb_hcd *);
>         int (*resume_quirk)(struct usb_hcd *);
>  };
>
>  #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)

Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *,
also in the future?

    struct xhci_hcd {
            ...
            unsigned long           priv[0] __aligned(sizeof(s64));


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] usb: host: xhci-rcar: Add a helper macro to set xhci_plat_priv

[Geert Uytterhoeven]

On Tue, Aug 27, 2019 at 12:06 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> To avoid copy-and-paste setting of xhci_plat_priv for R-Car SoCs,
> this patch add a helper macro SET_XHCI_PLAT_PRIV_FOR_RCAR.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] usb: host: xhci-rcar: Use xhci_plat_priv.quirks instead of code settings

[Geert Uytterhoeven]

On Tue, Aug 27, 2019 at 12:04 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> This patch uses xhci_plat_priv.quirks to simplify. The previous
> code had conditions to set some quirks in xhci_rcar_init_quirk().
> But, the xhci_rcar_init_quirk() is called at the same conditions.
> So, no behavior change.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/4] usb: host: xhci-plat: add quirks member into struct xhci_plat_priv

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, August 27, 2019 9:36 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > To simplify adding xhci->quirks instead of the .init_quirk()
> > function, this patch adds a new parameter "quirks" into
> > the struct xhci_plat_priv.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your review!

> > --- a/drivers/usb/host/xhci-plat.h
> > +++ b/drivers/usb/host/xhci-plat.h
> > @@ -12,10 +12,12 @@
> >
> >  struct xhci_plat_priv {
> >         const char *firmware_name;
> > +       unsigned long long quirks;
> >         void (*plat_start)(struct usb_hcd *);
> >         int (*init_quirk)(struct usb_hcd *);
> >         int (*resume_quirk)(struct usb_hcd *);
> >  };
> >
> >  #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
> 
> Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *,
> also in the future?
> 
>     struct xhci_hcd {
>             ...
>             unsigned long           priv[0] __aligned(sizeof(s64));

It seems so. But, I'm not sure that we can change type of the priv[0] to
struct xhci_plat_priv *, because this implementation is related to all usb_hcd drivers:

< Details if you are interested in :) >
--- include/linux/usb/hcd.h ---
struct usb_hcd {
	...
	/* The HC driver's private data is stored at the end of
	 * this structure.
	 */
	unsigned long hcd_priv[0]
			__attribute__ ((aligned(sizeof(s64))));
};

--- drivers/usb/host/xhci.c ---
static const struct hc_driver xhci_hc_driver = {
	...
	.hcd_priv_size =	sizeof(struct xhci_hcd),

and

void xhci_init_driver(struct hc_driver *drv,
		      const struct xhci_driver_overrides *over)
{
	BUG_ON(!over);

	/* Copy the generic table to drv then apply the overrides */
	*drv = xhci_hc_driver;

	if (over) {
		drv->hcd_priv_size += over->extra_priv_size;

--- drivers/usb/host/xhci-plat.c ---
static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
	.extra_priv_size = sizeof(struct xhci_plat_priv),

void xhci_init_driver(struct hc_driver *drv,
		      const struct xhci_driver_overrides *over)
{
	BUG_ON(!over);

	/* Copy the generic table to drv then apply the overrides */
	*drv = xhci_hc_driver;

	if (over) {
		drv->hcd_priv_size += over->extra_priv_size;

--- drivers/usb/core/hcd.c ---
struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
		struct device *sysdev, struct device *dev, const char *bus_name,
		struct usb_hcd *primary_hcd)
{
	struct usb_hcd *hcd;

	hcd = kzalloc(sizeof(*hcd) + driver->hcd_priv_size, GFP_KERNEL);
------------------------------

Best regards,
Yoshihiro Shimoda

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH 1/4] usb: host: xhci-plat: add quirks member into struct xhci_plat_priv

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, Aug 28, 2019 at 6:05 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, August 27, 2019 9:36 PM
> > On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > To simplify adding xhci->quirks instead of the .init_quirk()
> > > function, this patch adds a new parameter "quirks" into
> > > the struct xhci_plat_priv.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Thank you for your review!
>
> > > --- a/drivers/usb/host/xhci-plat.h
> > > +++ b/drivers/usb/host/xhci-plat.h
> > > @@ -12,10 +12,12 @@
> > >
> > >  struct xhci_plat_priv {
> > >         const char *firmware_name;
> > > +       unsigned long long quirks;
> > >         void (*plat_start)(struct usb_hcd *);
> > >         int (*init_quirk)(struct usb_hcd *);
> > >         int (*resume_quirk)(struct usb_hcd *);
> > >  };
> > >
> > >  #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> > > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
> >
> > Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *,
> > also in the future?
> >
> >     struct xhci_hcd {
> >             ...
> >             unsigned long           priv[0] __aligned(sizeof(s64));
>
> It seems so. But, I'm not sure that we can change type of the priv[0] to
> struct xhci_plat_priv *, because this implementation is related to all usb_hcd drivers:
>
> < Details if you are interested in :) >

Thank you! Not being familiar with the usb and xhci subsystem, I couldn't
find where it was used/allocated, hence my question.

This confirms it's always of type struct xhci_plat_priv * iff when dealing with
the xhci-plat driver.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] usb: host: xhci-rcar: avoid 60s wait by request_firmware() in system booting

[Simon Horman]

On Tue, Aug 27, 2019 at 07:02:07PM +0900, Yoshihiro Shimoda wrote:
> If CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y and CONFIG_USB_XHCI_RCAR=y,
> request_firmware() in xhci_rcar_download_firmware() waits for 60s to
> sysfs fallback for the firmware like below.
> 
> [    1.599701] xhci-hcd ee000000.usb: xHCI Host Controller
> [    1.604948] xhci-hcd ee000000.usb: new USB bus registered, assigned bus number 3
> [    1.612403] xhci-hcd ee000000.usb: Direct firmware load for r8a779x_usb3_v3.dlmem failed with error -2
> [    1.621726] xhci-hcd ee000000.usb: Falling back to sysfs fallback for: r8a779x_usb3_v3.dlmem
> [    1.707953] ata1: link resume succeeded after 1 retries
> [    1.819379] ata1: SATA link down (SStatus 0 SControl 300)
> [   62.436012] xhci-hcd ee000000.usb: can't setup: -11
> [   62.440901] xhci-hcd ee000000.usb: USB bus 3 deregistered
> [   62.446361] xhci-hcd: probe of ee000000.usb failed with error -11
> 
> To avoid this 60s wait, this patch adds to check the system_state
> condition and if the system is not running,
> xhci_rcar_download_firmware() calls request_firmware_direct()
> instead of request_firmware() as a workaround.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

It seems to me that request_firmware() is working as expected.
And that this patch introduces an alternate behaviour for xhci-rcar
where it will fall back to the user-space helper in some cases but not
others. This inconsistency isn't obviously correct to me. Perhaps
xhci-rcar should always call request_firmware_direct() ?

> ---
>  drivers/usb/host/xhci-rcar.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
> index 34761be..c90cf46 100644
> --- a/drivers/usb/host/xhci-rcar.c
> +++ b/drivers/usb/host/xhci-rcar.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/firmware.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> @@ -146,7 +147,10 @@ static int xhci_rcar_download_firmware(struct usb_hcd *hcd)
>  		firmware_name = priv->firmware_name;
>  
>  	/* request R-Car USB3.0 firmware */
> -	retval = request_firmware(&fw, firmware_name, dev);
> +	if (system_state < SYSTEM_RUNNING)
> +		retval = request_firmware_direct(&fw, firmware_name, dev);
> +	else
> +		retval = request_firmware(&fw, firmware_name, dev);
>  	if (retval)
>  		return retval;
>  
> -- 
> 2.7.4
> 

RE: [PATCH 4/4] usb: host: xhci-rcar: avoid 60s wait by request_firmware() in system booting

[Yoshihiro Shimoda]

Hi Simon-san,

Thank you for your comment!

> From: Simon Horman, Sent: Saturday, August 31, 2019 5:43 PM
> 
> On Tue, Aug 27, 2019 at 07:02:07PM +0900, Yoshihiro Shimoda wrote:
> > If CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y and CONFIG_USB_XHCI_RCAR=y,
> > request_firmware() in xhci_rcar_download_firmware() waits for 60s to
> > sysfs fallback for the firmware like below.
> >
> > [    1.599701] xhci-hcd ee000000.usb: xHCI Host Controller
> > [    1.604948] xhci-hcd ee000000.usb: new USB bus registered, assigned bus number 3
> > [    1.612403] xhci-hcd ee000000.usb: Direct firmware load for r8a779x_usb3_v3.dlmem failed with error -2
> > [    1.621726] xhci-hcd ee000000.usb: Falling back to sysfs fallback for: r8a779x_usb3_v3.dlmem
> > [    1.707953] ata1: link resume succeeded after 1 retries
> > [    1.819379] ata1: SATA link down (SStatus 0 SControl 300)
> > [   62.436012] xhci-hcd ee000000.usb: can't setup: -11
> > [   62.440901] xhci-hcd ee000000.usb: USB bus 3 deregistered
> > [   62.446361] xhci-hcd: probe of ee000000.usb failed with error -11
> >
> > To avoid this 60s wait, this patch adds to check the system_state
> > condition and if the system is not running,
> > xhci_rcar_download_firmware() calls request_firmware_direct()
> > instead of request_firmware() as a workaround.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> It seems to me that request_firmware() is working as expected.
> And that this patch introduces an alternate behaviour for xhci-rcar
> where it will fall back to the user-space helper in some cases but not
> others. This inconsistency isn't obviously correct to me. Perhaps
> xhci-rcar should always call request_firmware_direct() ?

If xhci-rcar always call request_firmware_direct() but end user
uses the user-space helper on the driver, it's a regression because
request_firmware_direct() always disables the user-space helper. So,
I'd like to avoid using request_firmware_direct(). JFYI, I checked
the git history and I found such a situation:
---
commit c0cc00f250e19c717fc9cdbdb7f55aaa569c7498
Author: Hauke Mehrtens <hauke@hauke-m.de>
Date:   Thu Aug 24 23:06:41 2017 +0200

    ath10k: activate user space firmware loading again
---

It seems we need more time to investigate how to fix (or avoid) this issue.
So, I'll resend v2 patch series without this patch.

Best regards,
Yoshihiro Shimoda