Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
On Fri, Dec 13, 2013 at 6:27 PM, Andreas Naumann wrote: > On 13.12.2013 13:34, Grazvydas Ignotas wrote: >> >> Hmm I don't know about that, this would be inconsistent with what all >> other OMAP drivers do. Maybe we should do what musb_core.c does just > > Ok, thats cool. > >> to be consistent and add a similar comment. Only the static variable >> could be avoided in favor of struct omap2430_glue member. > > Whats wrong with the static? Well besides minor memory wastage when the driver is compiled in but not used, you'd cause problems if TI makes a SoC with multiple musb instances. Yes this driver already contains global variables, but still we can avoid adding more easily. Gražvydas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
On 13.12.2013 13:34, Grazvydas Ignotas wrote:
On Thu, Dec 12, 2013 at 11:29 PM, Andreas Naumann wrote:
Hi Grazvydas,
Von: Grazvydas Ignotas [mailto:nota...@gmail.com]
Gesendet: Donnerstag, 12. Dezember 2013 01:21
An: linux-usb@vger.kernel.org
Cc: Felipe Balbi; linux-o...@vger.kernel.org; Naumann Andreas; Grazvydas
Ignotas; sta...@vger.kernel.org
Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
This is a hard to reproduce problem which leads to non-functional
USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
which introduces save/restore of OTG_INTERFSEL over suspend.
Since the resume function is also called early in driver init, it uses a
non-initialized value (which is 0 and a non-supported setting in DM37xx
for INTERFSEL). Shortly after the correct value is set. Apparently this
works most time, but not always.
Fix it by not writing the value on runtime resume if it is 0
(0 should never be saved in the context as it's invalid value,
so we use it as an indicator that context hasn't been saved yet).
This issue was originally found by Andreas Naumann:
http://marc.info/?l=linux-usb&m=138562574719654&w=2
Reported-and-bisected-by: Andreas Naumann
Signed-off-by: Grazvydas Ignotas
Cc:
---
This is a regression from 3.2, so should go to -rc and stable, IMO.
It's really annoying issue if you want to have a stable OTG behavior,
I've burned quite a lot of time on it myself over a year ago and gave up
eventually. Good thing Andreas finally found it, many thanks to him!
drivers/usb/musb/omap2430.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 2a408cd..737b3da 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
if (musb) {
omap2430_low_level_init(musb);
- musb_writel(musb->mregs, OTG_INTERFSEL,
+ if (musb->context.otg_interfsel != 0)
+ musb_writel(musb->mregs, OTG_INTERFSEL,
musb->context.otg_interfsel);
phy_power_on(musb->phy);
}
Oh, easy way out. I like it but I've also been thinking about your comment
on my original post, which was that initializing otg_interfsel to the PHYSEL
bits only might be dangerous because we cant be sure that there are other
bits in the register.
However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
Well I was trying to do a minimal fix so that it could be suitable for
merging to stable kernels. But yes you're right, I've just checked
OMAP4 TRM and 0 is actually valid value there..
After thinking about my patch again, I would propose to change otg_interfsel
into otg_physel and read-modify-write only those bits in resume() as you
suggested in your first answer. That way I could discard the problematic
first read in probe() while leaving other bits untouched. If you agree I
post a patch for this tomorrow.
Hmm I don't know about that, this would be inconsistent with what all
other OMAP drivers do. Maybe we should do what musb_core.c does just
Ok, thats cool.
to be consistent and add a similar comment. Only the static variable
could be avoided in favor of struct omap2430_glue member.
Whats wrong with the static?
cheers,
Andreas
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
On Thu, Dec 12, 2013 at 11:29 PM, Andreas Naumann wrote:
> Hi Grazvydas,
>
>> Von: Grazvydas Ignotas [mailto:nota...@gmail.com]
>> Gesendet: Donnerstag, 12. Dezember 2013 01:21
>> An: linux-usb@vger.kernel.org
>> Cc: Felipe Balbi; linux-o...@vger.kernel.org; Naumann Andreas; Grazvydas
>> Ignotas; sta...@vger.kernel.org
>> Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
>>
>>
>> This is a hard to reproduce problem which leads to non-functional
>> USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
>> e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
>> which introduces save/restore of OTG_INTERFSEL over suspend.
>>
>> Since the resume function is also called early in driver init, it uses a
>> non-initialized value (which is 0 and a non-supported setting in DM37xx
>> for INTERFSEL). Shortly after the correct value is set. Apparently this
>> works most time, but not always.
>>
>> Fix it by not writing the value on runtime resume if it is 0
>> (0 should never be saved in the context as it's invalid value,
>> so we use it as an indicator that context hasn't been saved yet).
>>
>> This issue was originally found by Andreas Naumann:
>> http://marc.info/?l=linux-usb&m=138562574719654&w=2
>>
>> Reported-and-bisected-by: Andreas Naumann
>> Signed-off-by: Grazvydas Ignotas
>> Cc:
>> ---
>> This is a regression from 3.2, so should go to -rc and stable, IMO.
>> It's really annoying issue if you want to have a stable OTG behavior,
>> I've burned quite a lot of time on it myself over a year ago and gave up
>> eventually. Good thing Andreas finally found it, many thanks to him!
>>
>> drivers/usb/musb/omap2430.c |3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index 2a408cd..737b3da 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
>>
>> if (musb) {
>> omap2430_low_level_init(musb);
>> - musb_writel(musb->mregs, OTG_INTERFSEL,
>> + if (musb->context.otg_interfsel != 0)
>> + musb_writel(musb->mregs, OTG_INTERFSEL,
>> musb->context.otg_interfsel);
>> phy_power_on(musb->phy);
>> }
>>
>
> Oh, easy way out. I like it but I've also been thinking about your comment
> on my original post, which was that initializing otg_interfsel to the PHYSEL
> bits only might be dangerous because we cant be sure that there are other
> bits in the register.
> However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
Well I was trying to do a minimal fix so that it could be suitable for
merging to stable kernels. But yes you're right, I've just checked
OMAP4 TRM and 0 is actually valid value there..
> After thinking about my patch again, I would propose to change otg_interfsel
> into otg_physel and read-modify-write only those bits in resume() as you
> suggested in your first answer. That way I could discard the problematic
> first read in probe() while leaving other bits untouched. If you agree I
> post a patch for this tomorrow.
Hmm I don't know about that, this would be inconsistent with what all
other OMAP drivers do. Maybe we should do what musb_core.c does just
to be consistent and add a similar comment. Only the static variable
could be avoided in favor of struct omap2430_glue member.
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
Hi Grazvydas,
Von: Grazvydas Ignotas [mailto:nota...@gmail.com]
Gesendet: Donnerstag, 12. Dezember 2013 01:21
An: linux-usb@vger.kernel.org
Cc: Felipe Balbi; linux-o...@vger.kernel.org; Naumann Andreas; Grazvydas
Ignotas; sta...@vger.kernel.org
Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
This is a hard to reproduce problem which leads to non-functional
USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
which introduces save/restore of OTG_INTERFSEL over suspend.
Since the resume function is also called early in driver init, it uses a
non-initialized value (which is 0 and a non-supported setting in DM37xx
for INTERFSEL). Shortly after the correct value is set. Apparently this
works most time, but not always.
Fix it by not writing the value on runtime resume if it is 0
(0 should never be saved in the context as it's invalid value,
so we use it as an indicator that context hasn't been saved yet).
This issue was originally found by Andreas Naumann:
http://marc.info/?l=linux-usb&m=138562574719654&w=2
Reported-and-bisected-by: Andreas Naumann
Signed-off-by: Grazvydas Ignotas
Cc:
---
This is a regression from 3.2, so should go to -rc and stable, IMO.
It's really annoying issue if you want to have a stable OTG behavior,
I've burned quite a lot of time on it myself over a year ago and gave up
eventually. Good thing Andreas finally found it, many thanks to him!
drivers/usb/musb/omap2430.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 2a408cd..737b3da 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
if (musb) {
omap2430_low_level_init(musb);
- musb_writel(musb->mregs, OTG_INTERFSEL,
+ if (musb->context.otg_interfsel != 0)
+ musb_writel(musb->mregs, OTG_INTERFSEL,
musb->context.otg_interfsel);
phy_power_on(musb->phy);
}
Oh, easy way out. I like it but I've also been thinking about your
comment on my original post, which was that initializing otg_interfsel
to the PHYSEL bits only might be dangerous because we cant be sure that
there are other bits in the register.
However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
After thinking about my patch again, I would propose to change
otg_interfsel into otg_physel and read-modify-write only those bits in
resume() as you suggested in your first answer. That way I could discard
the problematic first read in probe() while leaving other bits
untouched. If you agree I post a patch for this tomorrow.
cheers,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
On Thu, Dec 12, 2013 at 02:20:59AM +0200, Grazvydas Ignotas wrote:
> This is a hard to reproduce problem which leads to non-functional
> USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
> e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
> which introduces save/restore of OTG_INTERFSEL over suspend.
>
> Since the resume function is also called early in driver init, it uses a
> non-initialized value (which is 0 and a non-supported setting in DM37xx
> for INTERFSEL). Shortly after the correct value is set. Apparently this
> works most time, but not always.
>
> Fix it by not writing the value on runtime resume if it is 0
> (0 should never be saved in the context as it's invalid value,
> so we use it as an indicator that context hasn't been saved yet).
>
> This issue was originally found by Andreas Naumann:
> http://marc.info/?l=linux-usb&m=138562574719654&w=2
>
> Reported-and-bisected-by: Andreas Naumann
> Signed-off-by: Grazvydas Ignotas
> Cc:
> ---
> This is a regression from 3.2, so should go to -rc and stable, IMO.
> It's really annoying issue if you want to have a stable OTG behavior,
> I've burned quite a lot of time on it myself over a year ago and gave up
> eventually. Good thing Andreas finally found it, many thanks to him!
>
> drivers/usb/musb/omap2430.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 2a408cd..737b3da 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
>
> if (musb) {
> omap2430_low_level_init(musb);
> - musb_writel(musb->mregs, OTG_INTERFSEL,
> + if (musb->context.otg_interfsel != 0)
You might want to write an inline comment explaining why you're checking
otg_interfsel for 0 here (as you explained 0 is not a valid value to be
found).
Br, David
> + musb_writel(musb->mregs, OTG_INTERFSEL,
> musb->context.otg_interfsel);
> phy_power_on(musb->phy);
> }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" 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-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
This is a hard to reproduce problem which leads to non-functional
USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
which introduces save/restore of OTG_INTERFSEL over suspend.
Since the resume function is also called early in driver init, it uses a
non-initialized value (which is 0 and a non-supported setting in DM37xx
for INTERFSEL). Shortly after the correct value is set. Apparently this
works most time, but not always.
Fix it by not writing the value on runtime resume if it is 0
(0 should never be saved in the context as it's invalid value,
so we use it as an indicator that context hasn't been saved yet).
This issue was originally found by Andreas Naumann:
http://marc.info/?l=linux-usb&m=138562574719654&w=2
Reported-and-bisected-by: Andreas Naumann
Signed-off-by: Grazvydas Ignotas
Cc:
---
This is a regression from 3.2, so should go to -rc and stable, IMO.
It's really annoying issue if you want to have a stable OTG behavior,
I've burned quite a lot of time on it myself over a year ago and gave up
eventually. Good thing Andreas finally found it, many thanks to him!
drivers/usb/musb/omap2430.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 2a408cd..737b3da 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
if (musb) {
omap2430_low_level_init(musb);
- musb_writel(musb->mregs, OTG_INTERFSEL,
+ if (musb->context.otg_interfsel != 0)
+ musb_writel(musb->mregs, OTG_INTERFSEL,
musb->context.otg_interfsel);
phy_power_on(musb->phy);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
