diff mbox

[v2] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

Message ID SG2PR06MB091997C36965C629800302B0D8620@SG2PR06MB0919.apcprd06.prod.outlook.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Yoshihiro Shimoda April 25, 2016, 2:10 a.m. UTC
Hi,

> From: Rob Herring

> Sent: Friday, April 22, 2016 10:29 PM

> 

> On Fri, Apr 22, 2016 at 4:36 AM, Felipe Balbi

> <felipe.balbi@linux.intel.com> wrote:

> >

> > Hi,

> >

> > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes:

> >> The firmware of R-Car USB 3.0 host controller will control the reset.

> >> So, if the xhci driver doesn't do firmware downloading (e.g. kernel

> >> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR

> >> is not set), the reset of USB 3.0 host controller doesn't work

> >> correctly. Then, the host controller will cause long wait in

> >> xhci_reset() because the CMD_RESET bit of op_regs->command is not

> >> cleared for 10 seconds.

> >>

> >> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h

> >> to exit the probe function immediately.

> >>

> >> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 xHCI controllers)

> >> Cc: <stable@vger.kernel.org> # v3.17+

> >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

> >> ---

> >>  Changes from v1:

> >>   - Revise the commit log.

> >>     (http://www.spinics.net/lists/stable/msg130007.html)

> >>

> >>  drivers/usb/host/xhci-rcar.h | 6 +++++-

> >>  1 file changed, 5 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h

> >> index 2941a25..2afed68 100644

> >> --- a/drivers/usb/host/xhci-rcar.h

> >> +++ b/drivers/usb/host/xhci-rcar.h

> >> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)

> >>

> >>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)

> >>  {

> >> -     return 0;

> >> +     /*

> >> +      * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is

> >> +      * disabled, this function fails.

> >> +      */

> >> +     return -ENODEV;

> >

> > okay, if I understood correctly the thing is that you have a kernel

> > built with XHCI platform support but without XHCI RCAR support. Then, if

> > you run this kernel on RCAR board, you see this CMD_RESET problem,

> > right?

> >

> > Isn't this pointing to the fact that xhci-plat.ko built without RCAR

> > isn't exactly compatible with RCAR ?

> >

> > IOW:

> >

> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c

> > index 676ea458148b..3e39320564ce 100644

> > --- a/drivers/usb/host/xhci-plat.c

> > +++ b/drivers/usb/host/xhci-plat.c

> > @@ -112,6 +112,7 @@ static const struct of_device_id usb_xhci_of_match[] = {

> >         }, {

> >                 .compatible = "marvell,armada-380-xhci",

> >                 .data = &xhci_plat_marvell_armada,

> > +#if IS_ENABLED(CONFIG_USB_XHCI_RCAR)

> >         }, {

> >                 .compatible = "renesas,xhci-r8a7790",

> >                 .data = &xhci_plat_renesas_rcar_gen2,

> > @@ -130,6 +131,7 @@ static const struct of_device_id usb_xhci_of_match[] = {

> >         }, {

> >                 .compatible = "renesas,rcar-gen3-xhci",

> >                 .data = &xhci_plat_renesas_rcar_gen3,

> > +#endif

> >         },

> >         {},

> >  };

> >

> > Rob, should we limit compatible flags like this ? Without

> > CONFIG_USB_XHCI_RCAR, this driver won't work on RCAR but, as you can

> > see, this driver might still work on other non-RCAR platforms.

> >

> > What's your take on this ?

> 

> We should fix this in kconfig to always enable the option when RCAR is

> enabled IMO.


I could fix this in kconfig like the followings:

If this is acceptable, I will send each patch to arm / arm64.

Best regards,
Yoshihiro Shimoda

> Rob

Comments

Felipe Balbi April 25, 2016, 6:34 a.m. UTC | #1
Hi,

Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes:
>> > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes:
>> >> The firmware of R-Car USB 3.0 host controller will control the reset.
>> >> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
>> >> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
>> >> is not set), the reset of USB 3.0 host controller doesn't work
>> >> correctly. Then, the host controller will cause long wait in
>> >> xhci_reset() because the CMD_RESET bit of op_regs->command is not
>> >> cleared for 10 seconds.
>> >>
>> >> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h
>> >> to exit the probe function immediately.
>> >>
>> >> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 xHCI controllers)
>> >> Cc: <stable@vger.kernel.org> # v3.17+
>> >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> >> ---
>> >>  Changes from v1:
>> >>   - Revise the commit log.
>> >>     (http://www.spinics.net/lists/stable/msg130007.html)
>> >>
>> >>  drivers/usb/host/xhci-rcar.h | 6 +++++-
>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
>> >> index 2941a25..2afed68 100644
>> >> --- a/drivers/usb/host/xhci-rcar.h
>> >> +++ b/drivers/usb/host/xhci-rcar.h
>> >> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)
>> >>
>> >>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)
>> >>  {
>> >> -     return 0;
>> >> +     /*
>> >> +      * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is
>> >> +      * disabled, this function fails.
>> >> +      */
>> >> +     return -ENODEV;
>> >
>> > okay, if I understood correctly the thing is that you have a kernel
>> > built with XHCI platform support but without XHCI RCAR support. Then, if
>> > you run this kernel on RCAR board, you see this CMD_RESET problem,
>> > right?
>> >
>> > Isn't this pointing to the fact that xhci-plat.ko built without RCAR
>> > isn't exactly compatible with RCAR ?
>> >
>> > IOW:
>> >
>> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> > index 676ea458148b..3e39320564ce 100644
>> > --- a/drivers/usb/host/xhci-plat.c
>> > +++ b/drivers/usb/host/xhci-plat.c
>> > @@ -112,6 +112,7 @@ static const struct of_device_id usb_xhci_of_match[] = {
>> >         }, {
>> >                 .compatible = "marvell,armada-380-xhci",
>> >                 .data = &xhci_plat_marvell_armada,
>> > +#if IS_ENABLED(CONFIG_USB_XHCI_RCAR)
>> >         }, {
>> >                 .compatible = "renesas,xhci-r8a7790",
>> >                 .data = &xhci_plat_renesas_rcar_gen2,
>> > @@ -130,6 +131,7 @@ static const struct of_device_id usb_xhci_of_match[] = {
>> >         }, {
>> >                 .compatible = "renesas,rcar-gen3-xhci",
>> >                 .data = &xhci_plat_renesas_rcar_gen3,
>> > +#endif
>> >         },
>> >         {},
>> >  };
>> >
>> > Rob, should we limit compatible flags like this ? Without
>> > CONFIG_USB_XHCI_RCAR, this driver won't work on RCAR but, as you can
>> > see, this driver might still work on other non-RCAR platforms.
>> >
>> > What's your take on this ?
>> 
>> We should fix this in kconfig to always enable the option when RCAR is
>> enabled IMO.
>
> I could fix this in kconfig like the followings:
>
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index f2bc5c3..905d1d2 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -46,6 +46,7 @@ menuconfig ARCH_RENESAS
>  	select PINCTRL
>  	select ARCH_REQUIRE_GPIOLIB
>  	select ZONE_DMA if ARM_LPAE
> +	select USB_XHCI_RCAR if USB_XHCI_HCD
>  
>  if ARCH_RENESAS
>  
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index efa77c1..010d2b7 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -105,6 +105,7 @@ config ARCH_RENESAS
>  	select PM
>  	select PM_GENERIC_DOMAINS
>  	select RENESAS_IRQC
> +	select USB_XHCI_RCAR if USB_XHCI_HCD
>  	help
>  	  This enables support for the ARMv8 based Renesas SoCs.
>
> If this is acceptable, I will send each patch to arm / arm64.

I'm okay with that, not sure what the ARM folks will think ;-)
Geert Uytterhoeven April 25, 2016, 6:57 a.m. UTC | #2
Hi Shimoda-san,

On Mon, Apr 25, 2016 at 4:10 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Rob Herring
>> Sent: Friday, April 22, 2016 10:29 PM
>> On Fri, Apr 22, 2016 at 4:36 AM, Felipe Balbi
>> <felipe.balbi@linux.intel.com> wrote:
>> > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes:
>> >> The firmware of R-Car USB 3.0 host controller will control the reset.
>> >> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
>> >> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
>> >> is not set), the reset of USB 3.0 host controller doesn't work
>> >> correctly. Then, the host controller will cause long wait in
>> >> xhci_reset() because the CMD_RESET bit of op_regs->command is not
>> >> cleared for 10 seconds.
>> >>
>> >> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h
>> >> to exit the probe function immediately.
>> >>
>> >> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 xHCI controllers)
>> >> Cc: <stable@vger.kernel.org> # v3.17+
>> >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> >> ---
>> >>  Changes from v1:
>> >>   - Revise the commit log.
>> >>     (http://www.spinics.net/lists/stable/msg130007.html)
>> >>
>> >>  drivers/usb/host/xhci-rcar.h | 6 +++++-
>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
>> >> index 2941a25..2afed68 100644
>> >> --- a/drivers/usb/host/xhci-rcar.h
>> >> +++ b/drivers/usb/host/xhci-rcar.h
>> >> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)
>> >>
>> >>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)
>> >>  {
>> >> -     return 0;
>> >> +     /*
>> >> +      * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is
>> >> +      * disabled, this function fails.
>> >> +      */
>> >> +     return -ENODEV;
>> >
>> > okay, if I understood correctly the thing is that you have a kernel
>> > built with XHCI platform support but without XHCI RCAR support. Then, if
>> > you run this kernel on RCAR board, you see this CMD_RESET problem,
>> > right?

[...]

>> We should fix this in kconfig to always enable the option when RCAR is
>> enabled IMO.
>
> I could fix this in kconfig like the followings:
>
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index f2bc5c3..905d1d2 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -46,6 +46,7 @@ menuconfig ARCH_RENESAS
>         select PINCTRL
>         select ARCH_REQUIRE_GPIOLIB
>         select ZONE_DMA if ARM_LPAE
> +       select USB_XHCI_RCAR if USB_XHCI_HCD
>
>  if ARCH_RENESAS
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index efa77c1..010d2b7 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -105,6 +105,7 @@ config ARCH_RENESAS
>         select PM
>         select PM_GENERIC_DOMAINS
>         select RENESAS_IRQC
> +       select USB_XHCI_RCAR if USB_XHCI_HCD
>         help
>           This enables support for the ARMv8 based Renesas SoCs.
>
> If this is acceptable, I will send each patch to arm / arm64.

What about enforcing this in a single place instead, i.e. in
drivers/usb/host/Kconfig?

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
diff mbox

Patch

diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
index f2bc5c3..905d1d2 100644
--- a/arch/arm/mach-shmobile/Kconfig
+++ b/arch/arm/mach-shmobile/Kconfig
@@ -46,6 +46,7 @@  menuconfig ARCH_RENESAS
 	select PINCTRL
 	select ARCH_REQUIRE_GPIOLIB
 	select ZONE_DMA if ARM_LPAE
+	select USB_XHCI_RCAR if USB_XHCI_HCD
 
 if ARCH_RENESAS
 
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index efa77c1..010d2b7 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -105,6 +105,7 @@  config ARCH_RENESAS
 	select PM
 	select PM_GENERIC_DOMAINS
 	select RENESAS_IRQC
+	select USB_XHCI_RCAR if USB_XHCI_HCD
 	help
 	  This enables support for the ARMv8 based Renesas SoCs.