usb: chipidea: Fix ULPI on imx51

Commit Message

Fabio Estevam June 20, 2018, 10:22 p.m. UTC

On Wed, Jun 20, 2018 at 7:07 PM, Fabio Estevam <[e-mail]> wrote:

> This patches causes a regression on a imx51-babbage running 4.18-rc1:
> I get a kernel hang.
>
> If I revert it on top of 4.18-rc1, then it boots fine and USB host is
> functional.
>
> I understand this patch fixes a kernel hang for you, so which commit
> is responsible for the hang you observe?
>
> It seems this commit fixes a hang for you and causes another hang for me :-)
>
> Any ideas?

I am able to boot again if I skip passing the CI_HDRC_OVERRIDE_PHY_CONTROL flag:

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andrey Smirnov June 21, 2018, 12:47 p.m. UTC | #1

On Wed, Jun 20, 2018 at 3:22 PM Fabio Estevam <[e-mail]> wrote:
>
> On Wed, Jun 20, 2018 at 7:07 PM, Fabio Estevam <[e-mail]> wrote:
>
> > This patches causes a regression on a imx51-babbage running 4.18-rc1:
> > I get a kernel hang.
> >
> > If I revert it on top of 4.18-rc1, then it boots fine and USB host is
> > functional.
> >
> > I understand this patch fixes a kernel hang for you, so which commit
> > is responsible for the hang you observe?
> >

I never assumed it was a regression and that USB worked on RDU1 board
before, so I never tried to see if this was a regression. I can only
tell you that it hangs as soon as any PORTSC registers are accessed.

> > It seems this commit fixes a hang for you and causes another hang for me :-)
> >
> > Any ideas?
>

RDU1 design is based heavily on Babbage board, moreso USB1/ULPI
portion of it is an exact copy (it does use different GPIO for PHY
reset, but that's irrelevant), so I am surprised that it breaks in
your case.

However looking at imx51-babbage.dts, I am suspicious of it's USB1
setup. There we have usbh1phy node that references <&gpio2 5
GPIO_ACTIVE_LOW> as reset, but corresponding pinmux, pinctrl_usbh1reg,
is not being used anywhere. Cold that be that the problem you are
seeing is due to USB PHY not being properly reset?

> I am able to boot again if I skip passing the CI_HDRC_OVERRIDE_PHY_CONTROL flag:
>

Yeah, IMHO if you are dropping that flag, you may as well revert the
whole patch :-). The path that make the kernel hang in my case would
be taken if that flag is dropped.

Thanks,
Andrey Smirnov
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fabio Estevam June 21, 2018, 2:08 p.m. UTC | #2

Hi Andrey,

On Thu, Jun 21, 2018 at 9:47 AM, Andrey Smirnov
<[e-mail]> wrote:

> I never assumed it was a regression and that USB worked on RDU1 board
> before, so I never tried to see if this was a regression. I can only
> tell you that it hangs as soon as any PORTSC registers are accessed.

I think we need to investigate this portsc register hang further.

> RDU1 design is based heavily on Babbage board, moreso USB1/ULPI
> portion of it is an exact copy (it does use different GPIO for PHY
> reset, but that's irrelevant), so I am surprised that it breaks in
> your case.
>
> However looking at imx51-babbage.dts, I am suspicious of it's USB1
> setup. There we have usbh1phy node that references <&gpio2 5
> GPIO_ACTIVE_LOW> as reset, but corresponding pinmux, pinctrl_usbh1reg,
> is not being used anywhere. Cold that be that the problem you are
> seeing is due to USB PHY not being properly reset?

Yes, this can be improved, but this is not the cause of the issue .

gpio2 5 is passed as the USB PHY reset:

reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;

The default IOMUX setting after power on reset  is GPIO, so not an issue.

> Yeah, IMHO if you are dropping that flag, you may as well revert the
> whole patch :-). The path that make the kernel hang in my case would
> be taken if that flag is dropped.

Yes, it seems we need to revert the full patch and have a proper fix
in place that works for imx51 RDU, imx53-ppd and imx51 babbage.

The USB host 1 on imx51-babbage has been working since kernel 3.19 :-)

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nikita Yushchenko June 21, 2018, 2:12 p.m. UTC | #3

>>> This patches causes a regression on a imx51-babbage running 4.18-rc1:
>>> I get a kernel hang.
>>>
>>> If I revert it on top of 4.18-rc1, then it boots fine and USB host is
>>> functional.
>>>
>>> I understand this patch fixes a kernel hang for you, so which commit
>>> is responsible for the hang you observe?
>>>
> 
> I never assumed it was a regression and that USB worked on RDU1 board
> before, so I never tried to see if this was a regression. I can only
> tell you that it hangs as soon as any PORTSC registers are accessed.

Hang at register access usually means that module that owns the register
is not clocked.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nikita Yushchenko June 21, 2018, 2:25 p.m. UTC | #4

21.06.2018 17:12, Nikita Yushchenko wrote:
>>>> This patches causes a regression on a imx51-babbage running 4.18-rc1:
>>>> I get a kernel hang.
>>>>
>>>> If I revert it on top of 4.18-rc1, then it boots fine and USB host is
>>>> functional.
>>>>
>>>> I understand this patch fixes a kernel hang for you, so which commit
>>>> is responsible for the hang you observe?
>>>>
>>
>> I never assumed it was a regression and that USB worked on RDU1 board
>> before, so I never tried to see if this was a regression. I can only
>> tell you that it hangs as soon as any PORTSC registers are accessed.
> 
> Hang at register access usually means that module that owns the register
> is not clocked.

On RDU1, call to usb_phy_init() actually calls usb_gen_phy_init() that
does regulator_enable() for &vusb_reg and clk_prepare_enable() for
&clk_26M_usb.  I thing some of these two is actually needed to avoid
hang on register access.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Smirnov June 21, 2018, 9:44 p.m. UTC | #5

On Thu, Jun 21, 2018 at 7:08 AM Fabio Estevam <[e-mail]> wrote:
>
> Hi Andrey,
>
> On Thu, Jun 21, 2018 at 9:47 AM, Andrey Smirnov
> <[e-mail]> wrote:
>
> > I never assumed it was a regression and that USB worked on RDU1 board
> > before, so I never tried to see if this was a regression. I can only
> > tell you that it hangs as soon as any PORTSC registers are accessed.
>
> I think we need to investigate this portsc register hang further.
>

I just finished experimenting with it on RDU1 and Babbage boards and I
can't reproduce the hang that you describe against 4.18-rc1. At this
point I wonder if it's the bootloader that is a variable that plays
into this. I was booting both boards with 2018.06.0 version of barebox
+ the custom patches that can be found here
https://github.com/ndreys/barebox/commits/rdu1-netboot. Note that the
last patch in that branch/stack was added as a part of this
investigation, but even before it, I was able to boot Linux just fine,
albeit without working USB.

Any chance you can try it on your end and see if that makes a difference?

Thanks,
Andrey Smirnov
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen June 22, 2018, 12:49 a.m. UTC | #6

> >>>>

> >>

> >> I never assumed it was a regression and that USB worked on RDU1 board

> >> before, so I never tried to see if this was a regression. I can only

> >> tell you that it hangs as soon as any PORTSC registers are accessed.

> >

> > Hang at register access usually means that module that owns the

> > register is not clocked.

> 

> On RDU1, call to usb_phy_init() actually calls usb_gen_phy_init() that does

> regulator_enable() for &vusb_reg and clk_prepare_enable() for &clk_26M_usb.  I

> thing some of these two is actually needed to avoid hang on register access.


Fabio, would you please check both regulator and clock for ULPI PHY are on at imx51bbg?

Peter

Fabio Estevam June 22, 2018, 4:27 p.m. UTC | #7

Hi Andrey,

On Thu, Jun 21, 2018 at 6:44 PM, Andrey Smirnov
<[e-mail]> wrote:

> I just finished experimenting with it on RDU1 and Babbage boards and I
> can't reproduce the hang that you describe against 4.18-rc1. At this
> point I wonder if it's the bootloader that is a variable that plays
> into this. I was booting both boards with 2018.06.0 version of barebox
> + the custom patches that can be found here
> https://github.com/ndreys/barebox/commits/rdu1-netboot. Note that the
> last patch in that branch/stack was added as a part of this
> investigation, but even before it, I was able to boot Linux just fine,
> albeit without working USB.

Thanks for investigating this issue.

Yes, t is possible that the difference in behaviour that we see could
be related to the fact we use different bootloaders.

In order to make things easier, I am sharing a Buildroot image for
imx51-babbage that contains U-Boot 2018.05 + kernel 4.17.2:
https://www.dropbox.com/s/yevnu4y1zdchnbt/sdcard.img?dl=0

Please flash it directly to the SD card via dd and boot it.

You will notice that it will boot normally. Then please copy a
4.18-rc1 zImage into the first SD card partition and you will notice
the hang.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Smirnov June 24, 2018, 10:40 p.m. UTC | #8

On Fri, Jun 22, 2018 at 9:27 AM Fabio Estevam <[e-mail]> wrote:
>
> Hi Andrey,
>
> On Thu, Jun 21, 2018 at 6:44 PM, Andrey Smirnov
> <[e-mail]> wrote:
>
> > I just finished experimenting with it on RDU1 and Babbage boards and I
> > can't reproduce the hang that you describe against 4.18-rc1. At this
> > point I wonder if it's the bootloader that is a variable that plays
> > into this. I was booting both boards with 2018.06.0 version of barebox
> > + the custom patches that can be found here
> > https://github.com/ndreys/barebox/commits/rdu1-netboot. Note that the
> > last patch in that branch/stack was added as a part of this
> > investigation, but even before it, I was able to boot Linux just fine,
> > albeit without working USB.
>
> Thanks for investigating this issue.
>
> Yes, t is possible that the difference in behaviour that we see could
> be related to the fact we use different bootloaders.
>
> In order to make things easier, I am sharing a Buildroot image for
> imx51-babbage that contains U-Boot 2018.05 + kernel 4.17.2:
> https://www.dropbox.com/s/yevnu4y1zdchnbt/sdcard.img?dl=0
>
> Please flash it directly to the SD card via dd and boot it.
>
> You will notice that it will boot normally. Then please copy a
> 4.18-rc1 zImage into the first SD card partition and you will notice
> the hang.
>

It's definitely the bootloader, or more specifically whether or not it
initialized/used USB before booting the kernel. Some interesting
highlights:

 - On your "good" 4.17.2 based image, if you interrupt U-Boot, do "usb
start" (optionally "usb stop") and then "boot", you'll get the hang
that I was trying to fix with my patch.

 - Things are exact opposite with 4.18-rc1 and doing the above would
_prevent_ the hang and the image would boot fine.

 - Disabling USB in Barebox based boot "stack" gets it to behave the
same way as U-boot from your image (hanging when booting 4.18-rc1)

Digging more into code it seems that the reason for 4.18-rc1's
behavior is the fact that it's missing a call hw_phymode_configure()
after usb_phy_init() and, AFAICT, the only reason it is not happening
is because default image is being built without
CONFIG_USB_CHIPIDEA_ULPI and CONFIG_USB_ULPI_BUS. Enabling those two
options on 4.18-rc1, seem to fix the problem on both your U-Boot based
image and my "special" Barebox setup.

So AFAICT, this patch still fixes a valid issue (my use-case was net
booting via USB-Ethernet dongle), but an additional patch enabling
those two configuration options might be needed. Thoughts?

Also, I don't have any i.MX53 HW, so I wasn't able to test the effects
of enabling those two options there.

Thanks,
Andrey Smirnov
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -294,7 +294,6 @@  static int ci_hdrc_imx_probe(struct platform_device *pdev)
        if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
             of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy &&
            of_usb_get_phy_mode(np) == USBPHY_INTERFACE_MODE_ULPI) {
-               pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
                data->override_phy_control = true;
                usb_phy_init(pdata.usb_phy);
        }