| Message ID | 1350552010-28760-9-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ?
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
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 ;-)
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 ;-) >
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 ;-)
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
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.
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
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
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;
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(-)