The fix e5f4ca3fce90 broke Intel Edison host mode

The fix e5f4ca3fce90 broke Intel Edison host mode

[Andy Shevchenko]

Hi!

The fix in the commit
e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
effectively broke host mode on Intel Edison.

When device in a gadget mode and I attach the USB Mass Storage device

Before: automatically switches to host mode and detects storage and
everything okay
After: automatically switches to host mode and that's it.

Revert, as obviously from above, helps.

So, please fix this or I will send revert near to rc5,

Happy to test any proposed patches! (Maybe Ferry also can help with that)

-- 
With Best Regards,
Andy Shevchenko

Re: The fix e5f4ca3fce90 broke Intel Edison host mode

[Serge Semin]

Hello Andy

On Fri, Mar 12, 2021 at 09:33:59PM +0200, Andy Shevchenko wrote:
> Hi!
> 
> The fix in the commit
> e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
> effectively broke host mode on Intel Edison.
> 
> When device in a gadget mode and I attach the USB Mass Storage device
> 
> Before: automatically switches to host mode and detects storage and
> everything okay
> After: automatically switches to host mode and that's it.
> 
> Revert, as obviously from above, helps.
> 
> So, please fix this or I will send revert near to rc5,

As I've explained in the patchlog the commit was required because the
original functionality submitted by Felipe three years ago aside with
questionable fix introduced the suspend mode regression. So an access to
any PHY register over the ULPI interface just disabled "Suspend USB 2.0
HS/FS/LS PHY" mode, which made pointless enabling it in the main part of
the driver and consequently increased the USB bus power consumption. See
the commit e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
for details. It clears the SusPHY flag and never gets it back. Instead it
was enough to wait a little longer in the ULPI RegAccess busy-loop to let
PHY to get out of the suspend mode and restore the link. So to speak
reverting my patch would mean to get the regression back for all
IP-core versions, which isn't what any of us would want.

What I can suggest to you is to investigate the problem more thorough of why
you having the mass storage device undetected after the mode switching.
At least you need to look at the system log and find out whether it's ULPI
Registers commands timeout to blame. If so then I would take a look at the
PHY hardware manual whether it supports the Low-power mode. In any case
most likely setting "snps,dis_u2_susphy_quirk" property in the DWC USB3 DT
node will help you to work around the problem. Declaring that DT-prop will
effectively disable using the LPM for any mode (Host/gadget/DRD), but will
permit PHY suspension on the system suspend procedure (please see
snps,dis_u2_susphy_quirk implementation for details).

-Sergey

> 
> Happy to test any proposed patches! (Maybe Ferry also can help with that)
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: The fix e5f4ca3fce90 broke Intel Edison host mode

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 11:10 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> Hello Andy
>
> On Fri, Mar 12, 2021 at 09:33:59PM +0200, Andy Shevchenko wrote:
> > Hi!
> >
> > The fix in the commit
> > e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
> > effectively broke host mode on Intel Edison.
> >
> > When device in a gadget mode and I attach the USB Mass Storage device
> >
> > Before: automatically switches to host mode and detects storage and
> > everything okay
> > After: automatically switches to host mode and that's it.
> >
> > Revert, as obviously from above, helps.
> >
> > So, please fix this or I will send revert near to rc5,
>
> As I've explained in the patchlog the commit was required because the
> original functionality submitted by Felipe three years ago aside with
> questionable fix introduced the suspend mode regression. So an access to
> any PHY register over the ULPI interface just disabled "Suspend USB 2.0
> HS/FS/LS PHY" mode, which made pointless enabling it in the main part of
> the driver and consequently increased the USB bus power consumption. See
> the commit e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> for details. It clears the SusPHY flag and never gets it back. Instead it
> was enough to wait a little longer in the ULPI RegAccess busy-loop to let
> PHY to get out of the suspend mode and restore the link. So to speak
> reverting my patch would mean to get the regression back for all
> IP-core versions, which isn't what any of us would want.
>
> What I can suggest to you is to investigate the problem more thorough of why
> you having the mass storage device undetected after the mode switching.
> At least you need to look at the system log and find out whether it's ULPI
> Registers commands timeout to blame. If so then I would take a look at the
> PHY hardware manual whether it supports the Low-power mode. In any case
> most likely setting "snps,dis_u2_susphy_quirk" property in the DWC USB3 DT
> node will help you to work around the problem. Declaring that DT-prop will
> effectively disable using the LPM for any mode (Host/gadget/DRD), but will
> permit PHY suspension on the system suspend procedure (please see
> snps,dis_u2_susphy_quirk implementation for details).

Before the patch there was no regression on the board in question.
After the patch
I got a clear regression that I may reproduce 100%. It's the
responsibility of the author of the patch to try to figure out what
broke other's cases. It's as simple as that.

As I told you, I am happy to test any suggestion how to fix, otherwise
I will send a revert and I have a good justification to do that.

> > Happy to test any proposed patches! (Maybe Ferry also can help with that)



-- 
With Best Regards,
Andy Shevchenko

Re: The fix e5f4ca3fce90 broke Intel Edison host mode

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 11:46 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Mar 22, 2021 at 11:10 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > Hello Andy
> >
> > On Fri, Mar 12, 2021 at 09:33:59PM +0200, Andy Shevchenko wrote:
> > > Hi!
> > >
> > > The fix in the commit
> > > e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
> > > effectively broke host mode on Intel Edison.
> > >
> > > When device in a gadget mode and I attach the USB Mass Storage device
> > >
> > > Before: automatically switches to host mode and detects storage and
> > > everything okay
> > > After: automatically switches to host mode and that's it.
> > >
> > > Revert, as obviously from above, helps.
> > >
> > > So, please fix this or I will send revert near to rc5,
> >
> > As I've explained in the patchlog the commit was required because the
> > original functionality submitted by Felipe three years ago aside with
> > questionable fix introduced the suspend mode regression. So an access to
> > any PHY register over the ULPI interface just disabled "Suspend USB 2.0
> > HS/FS/LS PHY" mode, which made pointless enabling it in the main part of
> > the driver and consequently increased the USB bus power consumption. See
> > the commit e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> > for details. It clears the SusPHY flag and never gets it back. Instead it
> > was enough to wait a little longer in the ULPI RegAccess busy-loop to let
> > PHY to get out of the suspend mode and restore the link. So to speak
> > reverting my patch would mean to get the regression back for all
> > IP-core versions, which isn't what any of us would want.
> >
> > What I can suggest to you is to investigate the problem more thorough of why
> > you having the mass storage device undetected after the mode switching.
> > At least you need to look at the system log and find out whether it's ULPI
> > Registers commands timeout to blame. If so then I would take a look at the
> > PHY hardware manual whether it supports the Low-power mode. In any case
> > most likely setting "snps,dis_u2_susphy_quirk" property in the DWC USB3 DT
> > node will help you to work around the problem. Declaring that DT-prop will
> > effectively disable using the LPM for any mode (Host/gadget/DRD), but will
> > permit PHY suspension on the system suspend procedure (please see
> > snps,dis_u2_susphy_quirk implementation for details).
>
> Before the patch there was no regression on the board in question.
> After the patch
> I got a clear regression that I may reproduce 100%. It's the
> responsibility of the author of the patch to try to figure out what
> broke other's cases. It's as simple as that.
>
> As I told you, I am happy to test any suggestion how to fix, otherwise
> I will send a revert and I have a good justification to do that.
>
> > > Happy to test any proposed patches! (Maybe Ferry also can help with that)

With regards to the second part of the second paragraph, I will look
at that quirk if it helps.
Thanks.

-- 
With Best Regards,
Andy Shevchenko

Re: The fix e5f4ca3fce90 broke Intel Edison host mode

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 11:49 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Mar 22, 2021 at 11:46 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Mar 22, 2021 at 11:10 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > >
> > > Hello Andy
> > >
> > > On Fri, Mar 12, 2021 at 09:33:59PM +0200, Andy Shevchenko wrote:
> > > > Hi!
> > > >
> > > > The fix in the commit
> > > > e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
> > > > effectively broke host mode on Intel Edison.
> > > >
> > > > When device in a gadget mode and I attach the USB Mass Storage device
> > > >
> > > > Before: automatically switches to host mode and detects storage and
> > > > everything okay
> > > > After: automatically switches to host mode and that's it.
> > > >
> > > > Revert, as obviously from above, helps.
> > > >
> > > > So, please fix this or I will send revert near to rc5,
> > >
> > > As I've explained in the patchlog the commit was required because the
> > > original functionality submitted by Felipe three years ago aside with
> > > questionable fix introduced the suspend mode regression. So an access to
> > > any PHY register over the ULPI interface just disabled "Suspend USB 2.0
> > > HS/FS/LS PHY" mode, which made pointless enabling it in the main part of
> > > the driver and consequently increased the USB bus power consumption. See
> > > the commit e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> > > for details. It clears the SusPHY flag and never gets it back. Instead it
> > > was enough to wait a little longer in the ULPI RegAccess busy-loop to let
> > > PHY to get out of the suspend mode and restore the link. So to speak
> > > reverting my patch would mean to get the regression back for all
> > > IP-core versions, which isn't what any of us would want.
> > >
> > > What I can suggest to you is to investigate the problem more thorough of why
> > > you having the mass storage device undetected after the mode switching.
> > > At least you need to look at the system log and find out whether it's ULPI
> > > Registers commands timeout to blame. If so then I would take a look at the
> > > PHY hardware manual whether it supports the Low-power mode. In any case
> > > most likely setting "snps,dis_u2_susphy_quirk" property in the DWC USB3 DT
> > > node will help you to work around the problem. Declaring that DT-prop will
> > > effectively disable using the LPM for any mode (Host/gadget/DRD), but will
> > > permit PHY suspension on the system suspend procedure (please see
> > > snps,dis_u2_susphy_quirk implementation for details).
> >
> > Before the patch there was no regression on the board in question.
> > After the patch
> > I got a clear regression that I may reproduce 100%. It's the
> > responsibility of the author of the patch to try to figure out what
> > broke other's cases. It's as simple as that.
> >
> > As I told you, I am happy to test any suggestion how to fix, otherwise
> > I will send a revert and I have a good justification to do that.
> >
> > > > Happy to test any proposed patches! (Maybe Ferry also can help with that)
>
> With regards to the second part of the second paragraph, I will look
> at that quirk if it helps.
> Thanks.

Quirk helped, I will send a patch. Thanks for a hint.

-- 
With Best Regards,
Andy Shevchenko

Re: The fix e5f4ca3fce90 broke Intel Edison host mode

[Serge Semin]

On Mon, Mar 22, 2021 at 02:50:48PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 11:49 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Mar 22, 2021 at 11:46 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Mon, Mar 22, 2021 at 11:10 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > >
> > > > Hello Andy
> > > >
> > > > On Fri, Mar 12, 2021 at 09:33:59PM +0200, Andy Shevchenko wrote:
> > > > > Hi!
> > > > >
> > > > > The fix in the commit
> > > > > e5f4ca3fce90 ("usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression")
> > > > > effectively broke host mode on Intel Edison.
> > > > >
> > > > > When device in a gadget mode and I attach the USB Mass Storage device
> > > > >
> > > > > Before: automatically switches to host mode and detects storage and
> > > > > everything okay
> > > > > After: automatically switches to host mode and that's it.
> > > > >
> > > > > Revert, as obviously from above, helps.
> > > > >
> > > > > So, please fix this or I will send revert near to rc5,
> > > >
> > > > As I've explained in the patchlog the commit was required because the
> > > > original functionality submitted by Felipe three years ago aside with
> > > > questionable fix introduced the suspend mode regression. So an access to
> > > > any PHY register over the ULPI interface just disabled "Suspend USB 2.0
> > > > HS/FS/LS PHY" mode, which made pointless enabling it in the main part of
> > > > the driver and consequently increased the USB bus power consumption. See
> > > > the commit e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> > > > for details. It clears the SusPHY flag and never gets it back. Instead it
> > > > was enough to wait a little longer in the ULPI RegAccess busy-loop to let
> > > > PHY to get out of the suspend mode and restore the link. So to speak
> > > > reverting my patch would mean to get the regression back for all
> > > > IP-core versions, which isn't what any of us would want.
> > > >
> > > > What I can suggest to you is to investigate the problem more thorough of why
> > > > you having the mass storage device undetected after the mode switching.
> > > > At least you need to look at the system log and find out whether it's ULPI
> > > > Registers commands timeout to blame. If so then I would take a look at the
> > > > PHY hardware manual whether it supports the Low-power mode. In any case
> > > > most likely setting "snps,dis_u2_susphy_quirk" property in the DWC USB3 DT
> > > > node will help you to work around the problem. Declaring that DT-prop will
> > > > effectively disable using the LPM for any mode (Host/gadget/DRD), but will
> > > > permit PHY suspension on the system suspend procedure (please see
> > > > snps,dis_u2_susphy_quirk implementation for details).
> > >
> > > Before the patch there was no regression on the board in question.
> > > After the patch
> > > I got a clear regression that I may reproduce 100%. It's the
> > > responsibility of the author of the patch to try to figure out what
> > > broke other's cases. It's as simple as that.
> > >
> > > As I told you, I am happy to test any suggestion how to fix, otherwise
> > > I will send a revert and I have a good justification to do that.
> > >
> > > > > Happy to test any proposed patches! (Maybe Ferry also can help with that)
> >
> > With regards to the second part of the second paragraph, I will look
> > at that quirk if it helps.
> > Thanks.
> 
> Quirk helped, I will send a patch. Thanks for a hint.

Glad I could help.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko