[PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Ruslan Bilovol]

If the clocks are enabled and we want to enable them again
omap4430_phy_set_clk disables them.

Fix this - so now if we try to enable already enabled clocks
it works correctly.

Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
---
 arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
index 4c90477..0196683 100644
--- a/arch/arm/mach-omap2/omap_phy_internal.c
+++ b/arch/arm/mach-omap2/omap_phy_internal.c
@@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
 		clk_enable(clk48m);
 		clk_enable(clk32k);
 		state = 1;
-	} else if (state) {
+	} else if (!on && state) {
 		/* Disable the phy clocks */
 		clk_disable(phyclk);
 		clk_disable(clk48m);
-- 
1.7.1

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Tony Lindgren]

* Ruslan Bilovol <ruslan.bilovol-l0cyMroinI0@public.gmane.org> [120612 10:42]:
> If the clocks are enabled and we want to enable them again
> omap4430_phy_set_clk disables them.
> 
> Fix this - so now if we try to enable already enabled clocks
> it works correctly.

Sounds like Felipe is on vacation. Trying to figure out if this
is something for the fixes branch:

What happens if this not fixed, do you get some error?

Was this caused by some recent commit?

Regards,

Tony


 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol-l0cyMroinI0@public.gmane.org>
> ---
>  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
> index 4c90477..0196683 100644
> --- a/arch/arm/mach-omap2/omap_phy_internal.c
> +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
>  		clk_enable(clk48m);
>  		clk_enable(clk32k);
>  		state = 1;
> -	} else if (state) {
> +	} else if (!on && state) {
>  		/* Disable the phy clocks */
>  		clk_disable(phyclk);
>  		clk_disable(clk48m);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Ruslan Bilovol]

On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren <tony@atomide.com> wrote:
>
> * Ruslan Bilovol <ruslan.bilovol@ti.com> [120612 10:42]:
> > If the clocks are enabled and we want to enable them again
> > omap4430_phy_set_clk disables them.
> >
> > Fix this - so now if we try to enable already enabled clocks
> > it works correctly.
>
> Sounds like Felipe is on vacation. Trying to figure out if this
> is something for the fixes branch:
>
> What happens if this not fixed, do you get some error?

The function is designed for simple clocks on-off:
if zero "on" parameter is passed - switch the clocks off,
if non-zero "on" parameter passed - switch the clocks on.

But due to error in implementation, it works wrong if called twice or more
times with non-zero "on" parameter.

For example, assuming that clocks are disabled:
    omap4430_phy_set_clk(dev, 1);   <=== enables clocks
    omap4430_phy_set_clk(dev, 1);   <=== suddenly disables clocks,
when we expect enabling.
    omap4430_phy_set_clk(dev, 1);   <=== enables clocks again

It seems impact of this wrong behavior is not observed on current code
base, but we see
crashes caused by access to non-clocked registers when heavy use this function
in the charger detection or after implementing otg EYE improvement.

> Was this caused by some recent commit?

No, we have this wrong behavior since the omap_phy_internal.c file
creation (c33fad0c37)

--
Best regards,
Ruslan Bilovol


>
> > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> > ---
> >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > b/arch/arm/mach-omap2/omap_phy_internal.c
> > index 4c90477..0196683 100644
> > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
> >               clk_enable(clk48m);
> >               clk_enable(clk32k);
> >               state = 1;
> > -     } else if (state) {
> > +     } else if (!on && state) {
> >               /* Disable the phy clocks */
> >               clk_disable(phyclk);
> >               clk_disable(clk48m);
> > --
> > 1.7.1
--
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

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Tony Lindgren]

* Ruslan Bilovol <ruslan.bilovol@ti.com> [120629 14:55]:
> On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Ruslan Bilovol <ruslan.bilovol@ti.com> [120612 10:42]:
> > > If the clocks are enabled and we want to enable them again
> > > omap4430_phy_set_clk disables them.
> > >
> > > Fix this - so now if we try to enable already enabled clocks
> > > it works correctly.
> >
> > Sounds like Felipe is on vacation. Trying to figure out if this
> > is something for the fixes branch:
> >
> > What happens if this not fixed, do you get some error?
> 
> The function is designed for simple clocks on-off:
> if zero "on" parameter is passed - switch the clocks off,
> if non-zero "on" parameter passed - switch the clocks on.
> 
> But due to error in implementation, it works wrong if called twice or more
> times with non-zero "on" parameter.
> 
> For example, assuming that clocks are disabled:
>     omap4430_phy_set_clk(dev, 1);   <=== enables clocks
>     omap4430_phy_set_clk(dev, 1);   <=== suddenly disables clocks,
> when we expect enabling.
>     omap4430_phy_set_clk(dev, 1);   <=== enables clocks again
> 
> It seems impact of this wrong behavior is not observed on current code
> base, but we see
> crashes caused by access to non-clocked registers when heavy use this function
> in the charger detection or after implementing otg EYE improvement.

Yes thanks for explaining. Can you please update the commit message a bit
with the explanation above?
 
> > Was this caused by some recent commit?
> 
> No, we have this wrong behavior since the omap_phy_internal.c file
> creation (c33fad0c37)

OK. It's probably safest to wait for Felipe to take a look at this
patch then and queue it for v3.6 -rc cycle.

Regards,

Tony

> > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> > > ---
> > >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > > b/arch/arm/mach-omap2/omap_phy_internal.c
> > > index 4c90477..0196683 100644
> > > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
> > >               clk_enable(clk48m);
> > >               clk_enable(clk32k);
> > >               state = 1;
> > > -     } else if (state) {
> > > +     } else if (!on && state) {
> > >               /* Disable the phy clocks */
> > >               clk_disable(phyclk);
> > >               clk_disable(clk48m);
> > > --
> > > 1.7.1
--
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

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Hi,

On Tue, Jun 12, 2012 at 08:36:21PM +0300, Ruslan Bilovol wrote:
> If the clocks are enabled and we want to enable them again
> omap4430_phy_set_clk disables them.
> 
> Fix this - so now if we try to enable already enabled clocks
> it works correctly.
> 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> ---
>  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_phy_internal.c b/arch/arm/mach-omap2/omap_phy_internal.c
> index 4c90477..0196683 100644
> --- a/arch/arm/mach-omap2/omap_phy_internal.c
> +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
>  		clk_enable(clk48m);
>  		clk_enable(clk32k);
>  		state = 1;
> -	} else if (state) {
> +	} else if (!on && state) {

why don't you let clocks be enabled twice then ? That would cut down the
churn.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Ruslan Bilovol]

Hi,


On Mon, Jul 2, 2012 at 11:13 AM, Felipe Balbi <balbi@ti.com> wrote:
>
> Hi,
>
> On Tue, Jun 12, 2012 at 08:36:21PM +0300, Ruslan Bilovol wrote:
> > If the clocks are enabled and we want to enable them again
> > omap4430_phy_set_clk disables them.
> >
> > Fix this - so now if we try to enable already enabled clocks
> > it works correctly.
> >
> > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> > ---
> >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > b/arch/arm/mach-omap2/omap_phy_internal.c
> > index 4c90477..0196683 100644
> > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
> >               clk_enable(clk48m);
> >               clk_enable(clk32k);
> >               state = 1;
> > -     } else if (state) {
> > +     } else if (!on && state) {
>
> why don't you let clocks be enabled twice then ? That would cut down the
> churn.

Currently we have unbalanced call of this function.
I meet first during musb initialization - it tries to disable the phy
that leads to disabling already disabled clocks.
Next goal is to use internal clocks counter and to throw static
variable 'state'.
But since this is not investigated fully and work is not finished -
simple and obvious fix is pushed.

--
Best regards,
Ruslan Bilovol

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

Hi,

On Mon, Jul 02, 2012 at 08:10:49PM +0300, Ruslan Bilovol wrote:
> Hi,
> 
> 
> On Mon, Jul 2, 2012 at 11:13 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 12, 2012 at 08:36:21PM +0300, Ruslan Bilovol wrote:
> > > If the clocks are enabled and we want to enable them again
> > > omap4430_phy_set_clk disables them.
> > >
> > > Fix this - so now if we try to enable already enabled clocks
> > > it works correctly.
> > >
> > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol-l0cyMroinI0@public.gmane.org>
> > > ---
> > >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > > b/arch/arm/mach-omap2/omap_phy_internal.c
> > > index 4c90477..0196683 100644
> > > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int on)
> > >               clk_enable(clk48m);
> > >               clk_enable(clk32k);
> > >               state = 1;
> > > -     } else if (state) {
> > > +     } else if (!on && state) {
> >
> > why don't you let clocks be enabled twice then ? That would cut down the
> > churn.
> 
> Currently we have unbalanced call of this function.
> I meet first during musb initialization - it tries to disable the phy
> that leads to disabling already disabled clocks.
> Next goal is to use internal clocks counter and to throw static
> variable 'state'.

don't even go that way... what you need is to fix the unbalanced calls
instead of hacking around some generic API.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Ruslan Bilovol]

Hi,

On Tue, Jul 3, 2012 at 7:13 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
>
> Hi,
>
> On Mon, Jul 02, 2012 at 08:10:49PM +0300, Ruslan Bilovol wrote:
> > Hi,
> >
> >
> > On Mon, Jul 2, 2012 at 11:13 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 12, 2012 at 08:36:21PM +0300, Ruslan Bilovol wrote:
> > > > If the clocks are enabled and we want to enable them again
> > > > omap4430_phy_set_clk disables them.
> > > >
> > > > Fix this - so now if we try to enable already enabled clocks
> > > > it works correctly.
> > > >
> > > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol-l0cyMroinI0@public.gmane.org>
> > > > ---
> > > >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > > > b/arch/arm/mach-omap2/omap_phy_internal.c
> > > > index 4c90477..0196683 100644
> > > > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > > > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > > > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int
> > > > on)
> > > >               clk_enable(clk48m);
> > > >               clk_enable(clk32k);
> > > >               state = 1;
> > > > -     } else if (state) {
> > > > +     } else if (!on && state) {
> > >
> > > why don't you let clocks be enabled twice then ? That would cut down
> > > the
> > > churn.
> >
> > Currently we have unbalanced call of this function.
> > I meet first during musb initialization - it tries to disable the phy
> > that leads to disabling already disabled clocks.
> > Next goal is to use internal clocks counter and to throw static
> > variable 'state'.
>
> don't even go that way... what you need is to fix the unbalanced calls
> instead of hacking around some generic API.

Okay Felipe, I understand your position and agree with you. However,
right now the 'hack' that I'm fixing works incorrectly.
So while we do not have replacement of the 'hack', it will be good to
have at least fixed version of this 'hack'.

--
Best regards,
Ruslan Bilovol
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

Hi,

On Thu, Jul 05, 2012 at 08:16:54PM +0300, Ruslan Bilovol wrote:
> Hi,
> 
> On Tue, Jul 3, 2012 at 7:13 PM, Felipe Balbi <balbi@ti.com> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 02, 2012 at 08:10:49PM +0300, Ruslan Bilovol wrote:
> > > Hi,
> > >
> > >
> > > On Mon, Jul 2, 2012 at 11:13 AM, Felipe Balbi <balbi@ti.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 12, 2012 at 08:36:21PM +0300, Ruslan Bilovol wrote:
> > > > > If the clocks are enabled and we want to enable them again
> > > > > omap4430_phy_set_clk disables them.
> > > > >
> > > > > Fix this - so now if we try to enable already enabled clocks
> > > > > it works correctly.
> > > > >
> > > > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> > > > > ---
> > > > >  arch/arm/mach-omap2/omap_phy_internal.c |    2 +-
> > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > > > > b/arch/arm/mach-omap2/omap_phy_internal.c
> > > > > index 4c90477..0196683 100644
> > > > > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > > > > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > > > > @@ -97,7 +97,7 @@ int omap4430_phy_set_clk(struct device *dev, int
> > > > > on)
> > > > >               clk_enable(clk48m);
> > > > >               clk_enable(clk32k);
> > > > >               state = 1;
> > > > > -     } else if (state) {
> > > > > +     } else if (!on && state) {
> > > >
> > > > why don't you let clocks be enabled twice then ? That would cut down
> > > > the
> > > > churn.
> > >
> > > Currently we have unbalanced call of this function.
> > > I meet first during musb initialization - it tries to disable the phy
> > > that leads to disabling already disabled clocks.
> > > Next goal is to use internal clocks counter and to throw static
> > > variable 'state'.
> >
> > don't even go that way... what you need is to fix the unbalanced calls
> > instead of hacking around some generic API.
> 
> Okay Felipe, I understand your position and agree with you. However,
> right now the 'hack' that I'm fixing works incorrectly.
> So while we do not have replacement of the 'hack', it will be good to
> have at least fixed version of this 'hack'.

the problem is that once it "works" nobody ever looks into this again.
So, sorry but I can't accept anything other than a real fix

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]