diff mbox series

[v2,1/3] dt-bindings: usb: ci-hdrc-usb2: add property disable-runtime-pm

Message ID 20200714151822.250783-1-philippe.schenker@toradex.com
State Changes Requested
Headers show
Series [v2,1/3] dt-bindings: usb: ci-hdrc-usb2: add property disable-runtime-pm | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Philippe Schenker July 14, 2020, 3:18 p.m. UTC
Chipidea depends on some hardware signals to be there in order
for runtime-pm to work well. Add the possibility to disable runtime
power management that is necessary for certain boards.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

Changes in v2: None

 Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 +
 1 file changed, 1 insertion(+)

Comments

Peter Chen July 15, 2020, 12:51 a.m. UTC | #1
> The Toradex Colibri iMX6ULL board has a special USB hardware design.
> With runtime-pm enabled USB reset itself continuously. Furthermore the OTG port
> is also not enumerating devices if the Chipidea IP is in runtime sleep mode and a
> device or host gets plugged in.
> 

Hi Philippe,

You may describe the detail what's the special USB hardware design for your board,
and why it causes the problem, and why disable runtime pm could fix this issue, then,
the other users could know if it could apply to their platforms or not in future.

Peter

> This patch adds the opportunity to disable Runtime Power Management from
> devicetree
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> 
> ---
> 
> Changes in v2:
> - Change commit message to tell the use case for Colibri iMX6ULL
> 
>  drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 5ae16368a0c7..5078d0695eb7 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -434,6 +434,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  		usb_phy_init(pdata.usb_phy);
>  	}
> 
> +	if (of_property_read_bool(np, "disable-runtime-pm"))
> +		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
> +
>  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
>  		data->supports_runtime_pm = true;
> 
> --
> 2.27.0
Philippe Schenker July 15, 2020, 10:23 a.m. UTC | #2
On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
>  
> > The Toradex Colibri iMX6ULL board has a special USB hardware design.
> > With runtime-pm enabled USB reset itself continuously. Furthermore
> > the OTG port
> > is also not enumerating devices if the Chipidea IP is in runtime
> > sleep mode and a
> > device or host gets plugged in.
> > 
> 
> Hi Philippe,
> 
> You may describe the detail what's the special USB hardware design for
> your board,

If I only knew the root-cause of that problem - unfortunately I don't.
That's also why I have such a hard time to describe it.

> and why it causes the problem, and why disable runtime pm could fix
> this issue, then,

I cannot provide the 'why' part yet. I'll try something more and hope I
can provide you guys with the exact description.

> the other users could know if it could apply to their platforms or not
> in future.

I only found out about it because you were pointing me in that
direction. I debugged for hours now and didn't came to the root-cause of
the issue. I think to really understand it I would need to know much
more about the Chipidea IP.

I'll get back to you guys with a proposal for a new description.

Philippe

> 
> Peter
> 
> > This patch adds the opportunity to disable Runtime Power Management
> > from
> > devicetree
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Change commit message to tell the use case for Colibri iMX6ULL
> > 
> >  drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 5ae16368a0c7..5078d0695eb7 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -434,6 +434,9 @@ static int ci_hdrc_imx_probe(struct
> > platform_device *pdev)
> >  		usb_phy_init(pdata.usb_phy);
> >  	}
> > 
> > +	if (of_property_read_bool(np, "disable-runtime-pm"))
> > +		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
> > +
> >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> >  		data->supports_runtime_pm = true;
> > 
> > --
> > 2.27.0
Rob Herring July 16, 2020, 7:24 p.m. UTC | #3
On Tue, Jul 14, 2020 at 05:18:20PM +0200, Philippe Schenker wrote:
> Chipidea depends on some hardware signals to be there in order
> for runtime-pm to work well. Add the possibility to disable runtime
> power management that is necessary for certain boards.

This is why we have SoC specific compatible strings. Use that.

> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> ---
> 
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 51376cbe5f3d..67a31df13e69 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -90,6 +90,7 @@ Optional properties:
>    case, the "idle" state needs to pull down the data and strobe pin
>    and the "active" state needs to pull up the strobe pin.
>  - pinctrl-n: alternate pin modes
> +- disable-runtime-pm: This disables the runtime power management.

This is a Linux feature, not h/w description or config.

>  
>  i.mx specific properties
>  - fsl,usbmisc: phandler of non-core register device, with one
> -- 
> 2.27.0
>
Peter Chen July 17, 2020, 2:31 a.m. UTC | #4
On 20-07-16 13:24:52, Rob Herring wrote:
> On Tue, Jul 14, 2020 at 05:18:20PM +0200, Philippe Schenker wrote:
> > Chipidea depends on some hardware signals to be there in order
> > for runtime-pm to work well. Add the possibility to disable runtime
> > power management that is necessary for certain boards.
> 
> This is why we have SoC specific compatible strings. Use that.

It is a board design limitation, not SoC's. To support USB low power
mode for device mode, either VBUS connects to SoC, or VBUS connect to
GPIO, or VBUS connect to Type-C IC, but none of the design is used
at that board. So the USB can't enter low power mode for that board,
otherwise, the USB controller can't be woken up since no any interrupts
will occur if USB cable (host at other side) connects to the connector.

Peter
> 
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> > 
> > Changes in v2: None
> > 
> >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 51376cbe5f3d..67a31df13e69 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -90,6 +90,7 @@ Optional properties:
> >    case, the "idle" state needs to pull down the data and strobe pin
> >    and the "active" state needs to pull up the strobe pin.
> >  - pinctrl-n: alternate pin modes
> > +- disable-runtime-pm: This disables the runtime power management.
> 
> This is a Linux feature, not h/w description or config.
> 
> >  
> >  i.mx specific properties
> >  - fsl,usbmisc: phandler of non-core register device, with one
> > -- 
> > 2.27.0
> >
Rob Herring July 17, 2020, 2:14 p.m. UTC | #5
On Thu, Jul 16, 2020 at 8:31 PM Peter Chen <peter.chen@nxp.com> wrote:
>
> On 20-07-16 13:24:52, Rob Herring wrote:
> > On Tue, Jul 14, 2020 at 05:18:20PM +0200, Philippe Schenker wrote:
> > > Chipidea depends on some hardware signals to be there in order
> > > for runtime-pm to work well. Add the possibility to disable runtime
> > > power management that is necessary for certain boards.
> >
> > This is why we have SoC specific compatible strings. Use that.
>
> It is a board design limitation, not SoC's. To support USB low power
> mode for device mode, either VBUS connects to SoC, or VBUS connect to
> GPIO, or VBUS connect to Type-C IC, but none of the design is used
> at that board. So the USB can't enter low power mode for that board,
> otherwise, the USB controller can't be woken up since no any interrupts
> will occur if USB cable (host at other side) connects to the connector.

That's certainly a better explanation than the original.

Don't you need to describe the Vbus connection in the above cases?
Then you could handle this case based on the lack of a Vbus
description?

Rob
Peter Chen July 20, 2020, 3:44 a.m. UTC | #6
> On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> >
> > > The Toradex Colibri iMX6ULL board has a special USB hardware design.
> > > With runtime-pm enabled USB reset itself continuously. Furthermore
> > > the OTG port is also not enumerating devices if the Chipidea IP is
> > > in runtime sleep mode and a device or host gets plugged in.
> > >
> >
> > Hi Philippe,
> >
> > You may describe the detail what's the special USB hardware design for
> > your board,
> 
> If I only knew the root-cause of that problem - unfortunately I don't.
> That's also why I have such a hard time to describe it.
> 
> > and why it causes the problem, and why disable runtime pm could fix
> > this issue, then,
> 
> I cannot provide the 'why' part yet. I'll try something more and hope I can provide
> you guys with the exact description.
> 
> > the other users could know if it could apply to their platforms or not
> > in future.
> 
> I only found out about it because you were pointing me in that direction. I debugged
> for hours now and didn't came to the root-cause of the issue. I think to really
> understand it I would need to know much more about the Chipidea IP.
> 
> I'll get back to you guys with a proposal for a new description.
> 

Philippe, is it possible to share your USB hardware design at 6ULL?
And how ci_hdrc_gadget_connect is called when the runtime pm is disabled?

Thanks,
Peter
Philippe Schenker July 20, 2020, 7:51 a.m. UTC | #7
On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
>  
> > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > design.
> > > > With runtime-pm enabled USB reset itself continuously.
> > > > Furthermore
> > > > the OTG port is also not enumerating devices if the Chipidea IP
> > > > is
> > > > in runtime sleep mode and a device or host gets plugged in.
> > > > 
> > > 
> > > Hi Philippe,
> > > 
> > > You may describe the detail what's the special USB hardware design
> > > for
> > > your board,
> > 
> > If I only knew the root-cause of that problem - unfortunately I
> > don't.
> > That's also why I have such a hard time to describe it.
> > 
> > > and why it causes the problem, and why disable runtime pm could
> > > fix
> > > this issue, then,
> > 
> > I cannot provide the 'why' part yet. I'll try something more and
> > hope I can provide
> > you guys with the exact description.
> > 
> > > the other users could know if it could apply to their platforms or
> > > not
> > > in future.
> > 
> > I only found out about it because you were pointing me in that
> > direction. I debugged
> > for hours now and didn't came to the root-cause of the issue. I
> > think to really
> > understand it I would need to know much more about the Chipidea IP.
> > 
> > I'll get back to you guys with a proposal for a new description.
> > 
> 
> Philippe, is it possible to share your USB hardware design at 6ULL?

It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF capacitor
and +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the
big problem is that the driver can not detect the 5V on VBUS signal.

I tried to 'inject' 5V to that pin last week and things got really
better with runtime-pm. But I still thinks disabling it for our board
would make sense.

I'll send a new description today where I try to point to VBUS signal
not connected.

Philippe

> And how ci_hdrc_gadget_connect is called when the runtime pm is
> disabled?
> 
> Thanks,
> Peter
>
Peter Chen July 20, 2020, 8:06 a.m. UTC | #8
> On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
> >
> > > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > > design.
> > > > > With runtime-pm enabled USB reset itself continuously.
> > > > > Furthermore
> > > > > the OTG port is also not enumerating devices if the Chipidea IP
> > > > > is in runtime sleep mode and a device or host gets plugged in.
> > > > >
> > > >
> > > > Hi Philippe,
> > > >
> > > > You may describe the detail what's the special USB hardware design
> > > > for your board,
> > >
> > > If I only knew the root-cause of that problem - unfortunately I
> > > don't.
> > > That's also why I have such a hard time to describe it.
> > >
> > > > and why it causes the problem, and why disable runtime pm could
> > > > fix this issue, then,
> > >
> > > I cannot provide the 'why' part yet. I'll try something more and
> > > hope I can provide you guys with the exact description.
> > >
> > > > the other users could know if it could apply to their platforms or
> > > > not in future.
> > >
> > > I only found out about it because you were pointing me in that
> > > direction. I debugged for hours now and didn't came to the
> > > root-cause of the issue. I think to really understand it I would
> > > need to know much more about the Chipidea IP.
> > >
> > > I'll get back to you guys with a proposal for a new description.
> > >
> >
> > Philippe, is it possible to share your USB hardware design at 6ULL?
> 
> It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF capacitor and
> +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the big
> problem is that the driver can not detect the 5V on VBUS signal.
> 

Could you confirm it does not see VBUS at register OTGSC? If it is, how can it work with runtime
disabled, the USBCMD.RS setting (ci_hdrc_gadget_connect is called) depends on VBUS.

Peter

> I tried to 'inject' 5V to that pin last week and things got really better with runtime-pm.
> But I still thinks disabling it for our board would make sense.
> 
> I'll send a new description today where I try to point to VBUS signal not connected.
> 
> Philippe
> 
> > And how ci_hdrc_gadget_connect is called when the runtime pm is
> > disabled?
> >
> > Thanks,
> > Peter
> >
Philippe Schenker July 20, 2020, 10:10 a.m. UTC | #9
On Mon, 2020-07-20 at 08:06 +0000, Peter Chen wrote:
>  
> > On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
> > > > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > > > design.
> > > > > > With runtime-pm enabled USB reset itself continuously.
> > > > > > Furthermore
> > > > > > the OTG port is also not enumerating devices if the Chipidea
> > > > > > IP
> > > > > > is in runtime sleep mode and a device or host gets plugged
> > > > > > in.
> > > > > > 
> > > > > 
> > > > > Hi Philippe,
> > > > > 
> > > > > You may describe the detail what's the special USB hardware
> > > > > design
> > > > > for your board,
> > > > 
> > > > If I only knew the root-cause of that problem - unfortunately I
> > > > don't.
> > > > That's also why I have such a hard time to describe it.
> > > > 
> > > > > and why it causes the problem, and why disable runtime pm
> > > > > could
> > > > > fix this issue, then,
> > > > 
> > > > I cannot provide the 'why' part yet. I'll try something more and
> > > > hope I can provide you guys with the exact description.
> > > > 
> > > > > the other users could know if it could apply to their
> > > > > platforms or
> > > > > not in future.
> > > > 
> > > > I only found out about it because you were pointing me in that
> > > > direction. I debugged for hours now and didn't came to the
> > > > root-cause of the issue. I think to really understand it I would
> > > > need to know much more about the Chipidea IP.
> > > > 
> > > > I'll get back to you guys with a proposal for a new description.
> > > > 
> > > 
> > > Philippe, is it possible to share your USB hardware design at
> > > 6ULL?
> > 
> > It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF
> > capacitor and
> > +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the
> > big
> > problem is that the driver can not detect the 5V on VBUS signal.
> > 
> 
> Could you confirm it does not see VBUS at register OTGSC? If it is,
> how can it work with runtime
> disabled, the USBCMD.RS setting (ci_hdrc_gadget_connect is called)
> depends on VBUS.
> 
> Peter

For this reason I'm using a workaround in extcon like this:

extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;

I know that this is undocumented and wrong, but it works for our
hardware. With this and enabled runtime-pm devices do not get
enumerated.

But with runtime-pm disabled, devices get enumerated.

Further with this workaround the VBUS signal gets 'simulated'
in hw_read_otgsc.

Another problem with runtime-pm enabled is that with no devices plugged
into USB it resets itself every ~1 second.

Philippe.
> 
> > I tried to 'inject' 5V to that pin last week and things got really
> > better with runtime-pm.
> > But I still thinks disabling it for our board would make sense.
> > 
> > I'll send a new description today where I try to point to VBUS
> > signal not connected.
> > 
> > Philippe
> > 
> > > And how ci_hdrc_gadget_connect is called when the runtime pm is
> > > disabled?
> > > 
> > > Thanks,
> > > Peter
> > >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 51376cbe5f3d..67a31df13e69 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -90,6 +90,7 @@  Optional properties:
   case, the "idle" state needs to pull down the data and strobe pin
   and the "active" state needs to pull up the strobe pin.
 - pinctrl-n: alternate pin modes
+- disable-runtime-pm: This disables the runtime power management.
 
 i.mx specific properties
 - fsl,usbmisc: phandler of non-core register device, with one