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 ba...@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 ba...@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.bilo...@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
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 ba...@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 ba...@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.bilo...@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'.
--
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 ba...@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.bilo...@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.
--
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 ba...@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.bilo...@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
--
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 ruslan.bilo...@ti.com [120629 14:55]:
On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren t...@atomide.com wrote:
* Ruslan Bilovol ruslan.bilo...@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.bilo...@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 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 ruslan.bilo...@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
signature.asc
Description: Digital signature
Re: [PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
On Wed, Jun 20, 2012 at 4:31 PM, Tony Lindgren t...@atomide.com wrote:
* Ruslan Bilovol ruslan.bilo...@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.bilo...@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 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 ruslan.bilo...@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?
Was this caused by some recent commit?
Regards,
Tony
Signed-off-by: Ruslan Bilovol ruslan.bilo...@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 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
[PATCH RESEND] usb: otg: OMAP4: Fix the omap4430_phy_set_clk function
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.bilo...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
