Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
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 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 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
> > > > > ---
> > > > > 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
signature.asc
Description: Digital signature
Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
Hi,
On Tue, Jul 3, 2012 at 7:13 PM, Felipe Balbi 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 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
> > > > ---
> > > > 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-omap" in
the body of a message to majord...@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
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 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
> > > ---
> > > 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
signature.asc
Description: Digital signature
Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
Hi,
On Mon, Jul 2, 2012 at 11:13 AM, Felipe Balbi 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
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@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
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
> ---
> 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
signature.asc
Description: Digital signature
Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
* Ruslan Bilovol [120629 14:55]:
> On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren wrote:
> >
> > * Ruslan Bilovol [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
> > > ---
> > > 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 majord...@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
On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren wrote:
>
> * Ruslan Bilovol [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
> > ---
> > 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 majord...@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
* Ruslan Bilovol [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
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
