diff mbox

[PATCHv9,8/8] ARM: OMAP4: USB: power down MUSB PHY if not used

Message ID 1350552010-28760-9-git-send-email-t-kristo@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tero Kristo Oct. 18, 2012, 9:20 a.m. UTC
Commit c9e4412ab8eb8ef82d645d8749c4ce96ad490007 removed all of the USB
PHY functions for OMAP4, but this causes a problem with core retention
as the MUSB module remains enabled if omap-usb2 phy driver is not used.
This keeps the USB DPLL enabled and prevents l3_init pwrdm from idling.

Fixed by adding a minimal function back that disables the USB PHY in
case omap-usb2 driver is not used.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/omap_phy_internal.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

Comments

Felipe Balbi Oct. 18, 2012, 10:33 a.m. UTC | #1
Hi,

On Thu, Oct 18, 2012 at 12:20:10PM +0300, Tero Kristo wrote:
> Commit c9e4412ab8eb8ef82d645d8749c4ce96ad490007 removed all of the USB
> PHY functions for OMAP4, but this causes a problem with core retention
> as the MUSB module remains enabled if omap-usb2 phy driver is not used.
> This keeps the USB DPLL enabled and prevents l3_init pwrdm from idling.
> 
> Fixed by adding a minimal function back that disables the USB PHY in
> case omap-usb2 driver is not used.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/omap_phy_internal.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
> index d992db8..6a4b9cf 100644
> --- a/arch/arm/mach-omap2/omap_phy_internal.c
> +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> @@ -33,6 +33,33 @@
>  #include "soc.h"
>  #include "control.h"
>  
> +#define CONTROL_DEV_CONF		0x300
> +#define PHY_PD				0x1
> +
> +#ifndef CONFIG_OMAP_USB2

this is a tristate, meaning that can be a module.

> +static int __init omap4430_phy_power_down(void)
> +{
> +	void __iomem *ctrl_base;
> +
> +	if (!cpu_is_omap44xx())
> +		return 0;
> +
> +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> +	if (!ctrl_base) {
> +		pr_err("control module ioremap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Power down the phy */
> +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> +
> +	iounmap(ctrl_base);
> +
> +	return 0;
> +}
> +early_initcall(omap4430_phy_power_down);
> +#endif

I think you could do it even if the driver is enabled.

Just to make sure I understand the issue right: is the PHY enabled by
default or did bootloader left this enabled ?
Tero Kristo Oct. 18, 2012, 12:18 p.m. UTC | #2
On Thu, 2012-10-18 at 13:33 +0300, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Oct 18, 2012 at 12:20:10PM +0300, Tero Kristo wrote:
> > Commit c9e4412ab8eb8ef82d645d8749c4ce96ad490007 removed all of the USB
> > PHY functions for OMAP4, but this causes a problem with core retention
> > as the MUSB module remains enabled if omap-usb2 phy driver is not used.
> > This keeps the USB DPLL enabled and prevents l3_init pwrdm from idling.
> > 
> > Fixed by adding a minimal function back that disables the USB PHY in
> > case omap-usb2 driver is not used.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Cc: Felipe Balbi <balbi@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > ---
> >  arch/arm/mach-omap2/omap_phy_internal.c |   27 +++++++++++++++++++++++++++
> >  1 files changed, 27 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
> > index d992db8..6a4b9cf 100644
> > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > @@ -33,6 +33,33 @@
> >  #include "soc.h"
> >  #include "control.h"
> >  
> > +#define CONTROL_DEV_CONF		0x300
> > +#define PHY_PD				0x1
> > +
> > +#ifndef CONFIG_OMAP_USB2
> 
> this is a tristate, meaning that can be a module.

Ok, so extra check for that needed.

> 
> > +static int __init omap4430_phy_power_down(void)
> > +{
> > +	void __iomem *ctrl_base;
> > +
> > +	if (!cpu_is_omap44xx())
> > +		return 0;
> > +
> > +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> > +	if (!ctrl_base) {
> > +		pr_err("control module ioremap failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Power down the phy */
> > +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> > +
> > +	iounmap(ctrl_base);
> > +
> > +	return 0;
> > +}
> > +early_initcall(omap4430_phy_power_down);
> > +#endif
> 
> I think you could do it even if the driver is enabled.

Actually not at least now, it looks like the driver is not controlling
this bit at all, so the driver would fail if we do this.

> 
> Just to make sure I understand the issue right: is the PHY enabled by
> default or did bootloader left this enabled ?

The reset value for the register is zero (at least according to TRM), so
it is enabled from boot. Also, I couldn't find any code from the u-boot
that would touch this bit with a quick look.

-Tero

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Oct. 18, 2012, 1:53 p.m. UTC | #3
hi,

On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
> > > +static int __init omap4430_phy_power_down(void)
> > > +{
> > > +	void __iomem *ctrl_base;
> > > +
> > > +	if (!cpu_is_omap44xx())
> > > +		return 0;
> > > +
> > > +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> > > +	if (!ctrl_base) {
> > > +		pr_err("control module ioremap failed\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	/* Power down the phy */
> > > +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> > > +
> > > +	iounmap(ctrl_base);
> > > +
> > > +	return 0;
> > > +}
> > > +early_initcall(omap4430_phy_power_down);
> > > +#endif
> > 
> > I think you could do it even if the driver is enabled.
> 
> Actually not at least now, it looks like the driver is not controlling
> this bit at all, so the driver would fail if we do this.

then we can consider that a bug in the driver. Kishon, I thought you had
added SCM address space to PHY driver for this particular reason until
we get SCM driver, wasn't it ??

> > Just to make sure I understand the issue right: is the PHY enabled by
> > default or did bootloader left this enabled ?
> 
> The reset value for the register is zero (at least according to TRM), so
> it is enabled from boot. Also, I couldn't find any code from the u-boot
> that would touch this bit with a quick look.

ok, so it looks like we must do this anyway, thanks ;-)
Tero Kristo Oct. 18, 2012, 2:39 p.m. UTC | #4
On Thu, 2012-10-18 at 16:53 +0300, Felipe Balbi wrote:
> hi,
> 
> On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
> > > > +static int __init omap4430_phy_power_down(void)
> > > > +{
> > > > +	void __iomem *ctrl_base;
> > > > +
> > > > +	if (!cpu_is_omap44xx())
> > > > +		return 0;
> > > > +
> > > > +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> > > > +	if (!ctrl_base) {
> > > > +		pr_err("control module ioremap failed\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	/* Power down the phy */
> > > > +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> > > > +
> > > > +	iounmap(ctrl_base);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +early_initcall(omap4430_phy_power_down);
> > > > +#endif
> > > 
> > > I think you could do it even if the driver is enabled.
> > 
> > Actually not at least now, it looks like the driver is not controlling
> > this bit at all, so the driver would fail if we do this.
> 
> then we can consider that a bug in the driver. Kishon, I thought you had
> added SCM address space to PHY driver for this particular reason until
> we get SCM driver, wasn't it ??

Yes, I would say its a bug in the driver. However we need this disable
mechanism for the case where we don't have the driver also (which is the
default config for omap.)

-Tero

> > > Just to make sure I understand the issue right: is the PHY enabled by
> > > default or did bootloader left this enabled ?
> > 
> > The reset value for the register is zero (at least according to TRM), so
> > it is enabled from boot. Also, I couldn't find any code from the u-boot
> > that would touch this bit with a quick look.
> 
> ok, so it looks like we must do this anyway, thanks ;-)
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Oct. 18, 2012, 2:41 p.m. UTC | #5
On Thu, Oct 18, 2012 at 05:39:59PM +0300, Tero Kristo wrote:
> On Thu, 2012-10-18 at 16:53 +0300, Felipe Balbi wrote:
> > hi,
> > 
> > On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
> > > > > +static int __init omap4430_phy_power_down(void)
> > > > > +{
> > > > > +	void __iomem *ctrl_base;
> > > > > +
> > > > > +	if (!cpu_is_omap44xx())
> > > > > +		return 0;
> > > > > +
> > > > > +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> > > > > +	if (!ctrl_base) {
> > > > > +		pr_err("control module ioremap failed\n");
> > > > > +		return -ENOMEM;
> > > > > +	}
> > > > > +
> > > > > +	/* Power down the phy */
> > > > > +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> > > > > +
> > > > > +	iounmap(ctrl_base);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +early_initcall(omap4430_phy_power_down);
> > > > > +#endif
> > > > 
> > > > I think you could do it even if the driver is enabled.
> > > 
> > > Actually not at least now, it looks like the driver is not controlling
> > > this bit at all, so the driver would fail if we do this.
> > 
> > then we can consider that a bug in the driver. Kishon, I thought you had
> > added SCM address space to PHY driver for this particular reason until
> > we get SCM driver, wasn't it ??
> 
> Yes, I would say its a bug in the driver. However we need this disable
> mechanism for the case where we don't have the driver also (which is the
> default config for omap.)

sure, of course. But I'd like to see it unconditionally done, meaning
that there needs to be a counterpart in the driver to make sure you can
run this unconditionally during early phases of boot up ;-)
Kishon Vijay Abraham I Oct. 22, 2012, 8:54 a.m. UTC | #6
Hi,

On Thursday 18 October 2012 08:09 PM, Tero Kristo wrote:
> On Thu, 2012-10-18 at 16:53 +0300, Felipe Balbi wrote:
>> hi,
>>
>> On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
>>>>> +static int __init omap4430_phy_power_down(void)
>>>>> +{
>>>>> +	void __iomem *ctrl_base;
>>>>> +
>>>>> +	if (!cpu_is_omap44xx())
>>>>> +		return 0;
>>>>> +
>>>>> +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
>>>>> +	if (!ctrl_base) {
>>>>> +		pr_err("control module ioremap failed\n");
>>>>> +		return -ENOMEM;
>>>>> +	}
>>>>> +
>>>>> +	/* Power down the phy */
>>>>> +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
>>>>> +
>>>>> +	iounmap(ctrl_base);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +early_initcall(omap4430_phy_power_down);
>>>>> +#endif
>>>>
>>>> I think you could do it even if the driver is enabled.
>>>
>>> Actually not at least now, it looks like the driver is not controlling
>>> this bit at all, so the driver would fail if we do this.
>>
>> then we can consider that a bug in the driver. Kishon, I thought you had
>> added SCM address space to PHY driver for this particular reason until
>> we get SCM driver, wasn't it ??
>
> Yes, I would say its a bug in the driver.

No. It's done in the driver (omap_usb_phy_power() in 
drivers/usb/phy/omap-usb2.c). We explicitly power off the phy during 
probe in the driver.

However we need this disable
> mechanism for the case where we don't have the driver also (which is the
> default config for omap.)

Agree.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Oct. 22, 2012, 9:07 a.m. UTC | #7
On Mon, Oct 22, 2012 at 02:24:30PM +0530, kishon wrote:
> Hi,
> 
> On Thursday 18 October 2012 08:09 PM, Tero Kristo wrote:
> >On Thu, 2012-10-18 at 16:53 +0300, Felipe Balbi wrote:
> >>hi,
> >>
> >>On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
> >>>>>+static int __init omap4430_phy_power_down(void)
> >>>>>+{
> >>>>>+	void __iomem *ctrl_base;
> >>>>>+
> >>>>>+	if (!cpu_is_omap44xx())
> >>>>>+		return 0;
> >>>>>+
> >>>>>+	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
> >>>>>+	if (!ctrl_base) {
> >>>>>+		pr_err("control module ioremap failed\n");
> >>>>>+		return -ENOMEM;
> >>>>>+	}
> >>>>>+
> >>>>>+	/* Power down the phy */
> >>>>>+	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
> >>>>>+
> >>>>>+	iounmap(ctrl_base);
> >>>>>+
> >>>>>+	return 0;
> >>>>>+}
> >>>>>+early_initcall(omap4430_phy_power_down);
> >>>>>+#endif
> >>>>
> >>>>I think you could do it even if the driver is enabled.
> >>>
> >>>Actually not at least now, it looks like the driver is not controlling
> >>>this bit at all, so the driver would fail if we do this.
> >>
> >>then we can consider that a bug in the driver. Kishon, I thought you had
> >>added SCM address space to PHY driver for this particular reason until
> >>we get SCM driver, wasn't it ??
> >
> >Yes, I would say its a bug in the driver.
> 
> No. It's done in the driver (omap_usb_phy_power() in
> drivers/usb/phy/omap-usb2.c). We explicitly power off the phy during
> probe in the driver.

so you also handle enabling the IP later when you need ? That's great,
it means we can do the above unconditionally, driver enabled or not.
Kishon Vijay Abraham I Oct. 22, 2012, 9:25 a.m. UTC | #8
Hi,

On Monday 22 October 2012 02:37 PM, Felipe Balbi wrote:
> On Mon, Oct 22, 2012 at 02:24:30PM +0530, kishon wrote:
>> Hi,
>>
>> On Thursday 18 October 2012 08:09 PM, Tero Kristo wrote:
>>> On Thu, 2012-10-18 at 16:53 +0300, Felipe Balbi wrote:
>>>> hi,
>>>>
>>>> On Thu, Oct 18, 2012 at 03:18:04PM +0300, Tero Kristo wrote:
>>>>>>> +static int __init omap4430_phy_power_down(void)
>>>>>>> +{
>>>>>>> +	void __iomem *ctrl_base;
>>>>>>> +
>>>>>>> +	if (!cpu_is_omap44xx())
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
>>>>>>> +	if (!ctrl_base) {
>>>>>>> +		pr_err("control module ioremap failed\n");
>>>>>>> +		return -ENOMEM;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/* Power down the phy */
>>>>>>> +	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
>>>>>>> +
>>>>>>> +	iounmap(ctrl_base);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +early_initcall(omap4430_phy_power_down);
>>>>>>> +#endif
>>>>>>
>>>>>> I think you could do it even if the driver is enabled.
>>>>>
>>>>> Actually not at least now, it looks like the driver is not controlling
>>>>> this bit at all, so the driver would fail if we do this.
>>>>
>>>> then we can consider that a bug in the driver. Kishon, I thought you had
>>>> added SCM address space to PHY driver for this particular reason until
>>>> we get SCM driver, wasn't it ??
>>>
>>> Yes, I would say its a bug in the driver.
>>
>> No. It's done in the driver (omap_usb_phy_power() in
>> drivers/usb/phy/omap-usb2.c). We explicitly power off the phy during
>> probe in the driver.
>
> so you also handle enabling the IP later when you need ? That's great,
> it means we can do the above unconditionally, driver enabled or not.

yes.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Oct. 27, 2012, 2:52 a.m. UTC | #9
Hi

On Thu, 18 Oct 2012, Tero Kristo wrote:

> Commit c9e4412ab8eb8ef82d645d8749c4ce96ad490007 removed all of the USB
> PHY functions for OMAP4, but this causes a problem with core retention
> as the MUSB module remains enabled if omap-usb2 phy driver is not used.
> This keeps the USB DPLL enabled and prevents l3_init pwrdm from idling.
> 
> Fixed by adding a minimal function back that disables the USB PHY in
> case omap-usb2 driver is not used.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>

Looks like another one for Tony...

- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
index d992db8..6a4b9cf 100644
--- a/arch/arm/mach-omap2/omap_phy_internal.c
+++ b/arch/arm/mach-omap2/omap_phy_internal.c
@@ -33,6 +33,33 @@ 
 #include "soc.h"
 #include "control.h"
 
+#define CONTROL_DEV_CONF		0x300
+#define PHY_PD				0x1
+
+#ifndef CONFIG_OMAP_USB2
+static int __init omap4430_phy_power_down(void)
+{
+	void __iomem *ctrl_base;
+
+	if (!cpu_is_omap44xx())
+		return 0;
+
+	ctrl_base = ioremap(OMAP443X_SCM_BASE, SZ_1K);
+	if (!ctrl_base) {
+		pr_err("control module ioremap failed\n");
+		return -ENOMEM;
+	}
+
+	/* Power down the phy */
+	__raw_writel(PHY_PD, ctrl_base + CONTROL_DEV_CONF);
+
+	iounmap(ctrl_base);
+
+	return 0;
+}
+early_initcall(omap4430_phy_power_down);
+#endif
+
 void am35x_musb_reset(void)
 {
 	u32	regval;