Re: [PATCH V3] usb: musb: Fix unstable init of OTG_INTERFSEL.
Am 19.12.2013 00:40, schrieb Grazvydas Ignotas: On Wed, Dec 18, 2013 at 5:35 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaum...@ultratronik.de wrote: From: Andreas Naumann anaum...@ultratronik.de 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 e25bec160158abe86c276d7d206264afc3646281, 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. yeah, but the problem is not on the glue layer. The bug is omap_device and pm_runtime not agreeing on device's state. I suppose there was a fix for that recently in linux-omap@vger mailing list. You mean this: http://marc.info/?t=13844488263r=1w=2 ? This looks like a different issue during suspend, this problem is at startup. Both musb_core and omap2430.c expect hardware to be disabled on startup, and that works as expected. The problem is on first pm_runtime_get_sync(), which results in first runtime_resume() call, musb_core checks for first resume and doesn't load yet-unset context in that case, however glue does and breaks things. We have this problem since 3.2. I'm a bit short on time at the moment, but shall I have a go on another version of the patch with the init-variable as atomic_t? Andreas Gražvydas -- 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
Re: [PATCH V3] usb: musb: Fix unstable init of OTG_INTERFSEL.
Am 17.12.2013 18:22, schrieb David Cohen:
On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaum...@ultratronik.de wrote:
From: Andreas Naumann anaum...@ultratronik.de
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
e25bec160158abe86c276d7d206264afc3646281, 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 has not been
initialized yet.
Signed-off-by: Andreas Naumann anaum...@ultratronik.de
---
Even though I find the implementation a bit awkward this should fix
the issue without breaking anything else. Hope everyone is happy
with this.
drivers/usb/musb/omap2430.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 4315d35..fbe2c08 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -48,6 +48,7 @@ struct omap2430_glue {
enum omap_musb_vbus_id_status status;
struct work_struct omap_musb_mailbox_work;
struct device *control_otghs;
+ u8 initialized;
};
#define glue_to_musb(g) platform_get_drvdata(g-musb)
@@ -383,6 +384,7 @@ static int omap2430_musb_init(struct musb *musb)
}
musb_writel(musb-mregs, OTG_INTERFSEL, l);
+ glue-initialized = 1;
pr_debug(HS USB OTG: revision 0x%x, sysconfig 0x%02x,
sysstatus 0x%x, intrfsel 0x%x, simenable 0x%x\n,
@@ -509,6 +511,7 @@ static int omap2430_probe(struct platform_device *pdev)
glue-dev= pdev-dev;
glue-musb = musb;
glue-status = OMAP_MUSB_UNKNOWN;
+ glue-initialized= 0;
You don't need to do this. 'glue' was already allocated with kzalloc().
ok
if (np) {
pdata = devm_kzalloc(pdev-dev, sizeof(*pdata), GFP_KERNEL);
@@ -646,7 +649,8 @@ static int omap2430_runtime_resume(struct device *dev)
if (musb) {
omap2430_low_level_init(musb);
- musb_writel(musb-mregs, OTG_INTERFSEL,
+ if(glue-initialized)
Are you sure this is thread safe?
If you're sending this patch it means runtime_resume can be called
before omap2430_must_init(), but how about at the same time?
You defined 'initialized' as u8 type, then read/write operations won't
be atomic in ARM.
You're right, wasnt thinking of that. Shall I use atomic_t and helpers?
Br, David Cohen
+ musb_writel(musb-mregs, OTG_INTERFSEL,
musb-context.otg_interfsel);
usb_phy_set_suspend(musb-xceiv, 0);
--
1.8.4.1
--
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
--
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: 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 d...@andin.de wrote:
Hi Grazvydas,
Von: Grazvydas Ignotas [mailto:nota...@gmail.com]
Gesendet: Donnerstag, 12. Dezember 2013 01:21
An: linux-...@vger.kernel.org
Cc: Felipe Balbi; linux-omap@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-usbm=138562574719654w=2
Reported-and-bisected-by: Andreas Naumann anaum...@ultratronik.de
Signed-off-by: Grazvydas Ignotas nota...@gmail.com
Cc: sta...@vger.kernel.org
---
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-omap 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-...@vger.kernel.org
Cc: Felipe Balbi; linux-omap@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-usbm=138562574719654w=2
Reported-and-bisected-by: Andreas Naumann anaum...@ultratronik.de
Signed-off-by: Grazvydas Ignotas nota...@gmail.com
Cc: sta...@vger.kernel.org
---
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-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 V2] usb: musb: Fix unstable init of OTG_INTERFSEL.
Nice find, I think I'm also affected by this. However this crashes on OMAP3530 pandora because you now access INTERFSEL before interface clock is enabled.. This is not a problem on DM37xx because it uses different interconnects. You should probably use context_valid variable in omap2430_glue or similar to decide to write to register or not. Grazvydas You mean introducing a context_value? I'm not sure if i want to add more code. Do you think simple removing the first read is an option? So far we read back 0 anyway because the first write to INTERFSEL in resume() set it to uninitialized / 0 anyway. -- 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] omap2 dss: omap_display_init: Dont allow more than the maximum number of displays.
Currently the maximum number of display is hardcoded in the array
omapfb2_device. Made the number a #define and check it in init routine.
Signed-off-by: Andreas Naumann anaum...@ultratronik.de
---
Our board supports a lot of panels and we could probably solve this more
effectively, but arrays shouldnt silently overflow when using more than 10
displays. Created the patch on 3.1 and tested it there. This one is rebased on
todays linux-omap.git
arch/arm/mach-omap2/display.c | 5 +
drivers/video/omap2/omapfb/omapfb.h | 10 +-
include/video/omapdss.h | 2 ++
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index ff37be1..6f1a147 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -338,6 +338,11 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return -ENODEV;
}
+ if( board_data-num_devices OMAPFB_MAX_DISPLAY_NUM ){
+ pr_err(Trying to init more displays(%d) than possible.(%d)\n,
board_data-num_devices, OMAPFB_MAX_DISPLAY_NUM);
+ return -ENODEV;
+ }
+
board_data-version = ver;
board_data-dsi_enable_pads = omap_dsi_enable_pads;
board_data-dsi_disable_pads = omap_dsi_disable_pads;
diff --git a/drivers/video/omap2/omapfb/omapfb.h
b/drivers/video/omap2/omapfb/omapfb.h
index 623cd87..00d3fbc 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -96,15 +96,15 @@ struct omapfb2_device {
int state;
unsigned num_fbs;
- struct fb_info *fbs[10];
- struct omapfb2_mem_region regions[10];
+ struct fb_info *fbs[OMAPFB_MAX_DISPLAY_NUM];
+ struct omapfb2_mem_region regions[OMAPFB_MAX_DISPLAY_NUM];
unsigned num_displays;
- struct omapfb_display_data displays[10];
+ struct omapfb_display_data displays[OMAPFB_MAX_DISPLAY_NUM];
unsigned num_overlays;
- struct omap_overlay *overlays[10];
+ struct omap_overlay *overlays[OMAPFB_MAX_DISPLAY_NUM];
unsigned num_managers;
- struct omap_overlay_manager *managers[10];
+ struct omap_overlay_manager *managers[OMAPFB_MAX_DISPLAY_NUM];
struct workqueue_struct *auto_update_wq;
};
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index aeb4e9a..dfb054f 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -54,6 +54,8 @@
#define DISPC_IRQ_ACBIAS_COUNT_STAT3 (1 29)
#define DISPC_IRQ_FRAMEDONE3 (1 30)
+#define OMAPFB_MAX_DISPLAY_NUM 10
+
struct omap_dss_device;
struct omap_overlay_manager;
struct dss_lcd_mgr_config;
--
1.8.2.3
--
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
