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.
On Wed, Dec 18, 2013 at 9:41 AM, Andreas Naumann d...@andin.de wrote:
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?
No, the problem is that omap2430_runtime_resume() is called _during_
omap2430_must_init(), when pm_runtime_get_sync() is called. We can't
read the initial register value before pm_runtime_get_sync() returns
because the hardware is not powered up, and from pm_runtime_get_sync()
omap2430_runtime_resume() is called, where the cached register value
is needed.
You defined 'initialized' as u8 type, then read/write operations won't
be atomic in ARM.
We would only have problems if runtime_suspend() and runtime_resume()
are called at the same time, can this really happed?
Grazvydas
--
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.
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. -- balbi signature.asc Description: Digital signature
Re: [PATCH V3] usb: musb: Fix unstable init of OTG_INTERFSEL.
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. 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
[PATCH V3] usb: musb: Fix unstable init of OTG_INTERFSEL.
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;
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)
+ 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-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.
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().
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.
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-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
