[PATCH 1/2] otg/ulpi: bail out on read errors

[PATCH 1/2] otg/ulpi: bail out on read errors

[Wolfram Sang]

otg_read may return errnos, so bail out correctly to prevent bogus
ID-numbers.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/usb/otg/ulpi.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index b1b3469..d331b22 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -59,12 +59,17 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 
 static int ulpi_init(struct otg_transceiver *otg)
 {
-	int i, vid, pid;
-
-	vid = (otg_io_read(otg, ULPI_VENDOR_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_VENDOR_ID_LOW);
-	pid = (otg_io_read(otg, ULPI_PRODUCT_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_PRODUCT_ID_LOW);
+	int i, vid, pid, ret;
+	u32 ulpi_id = 0;
+
+	for (i = 0; i < 4; i++) {
+		ret = otg_io_read(otg, ULPI_PRODUCT_ID_HIGH - i);
+		if (ret < 0)
+			return ret;
+		ulpi_id = (ulpi_id << 8) | ret;
+	}
+	vid = ulpi_id & 0xffff;
+	pid = ulpi_id >> 16;
 
 	pr_info("ULPI transceiver vendor/product ID 0x%04x/0x%04x\n", vid, pid);
 
-- 
1.7.1

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Wolfram Sang]

The old code registered the hcd even if there were no transceivers
detected, leading to oopses like this if we try to probe a non-existant
ULPI:

[    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
[    2.740000] timeout polling for ULPI device
[    2.740000] timeout polling for ULPI device
[    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
[    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host Controller
[    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered, assigned bus number 2
[    2.770000] Unhandled fault: external abort on non-linefetch (0x808) at 0xc4876184
[    2.770000] Internal error: : 808 [#1] PREEMPT
[    2.770000] last sysfs file:
[    2.770000] Modules linked in:
[    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
[    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
[    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
[    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
[    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
[    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
[    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
[    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
[    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM Segment kernel
[    2.770000] Control: 0005317f  Table: a0004000  DAC: 00000017
[    2.770000] Process swapper (pid: 1, stack limit = 0xc3814270)
...

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Greg KH <gregkh@suse.de>
Cc: stable at kernel.org
---
 drivers/usb/host/ehci-mxc.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
index 544ccfd..bd40277 100644
--- a/drivers/usb/host/ehci-mxc.c
+++ b/drivers/usb/host/ehci-mxc.c
@@ -207,10 +207,17 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	/* Initialize the transceiver */
 	if (pdata->otg) {
 		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
-		if (otg_init(pdata->otg) != 0)
-			dev_err(dev, "unable to init transceiver\n");
-		else if (otg_set_vbus(pdata->otg, 1) != 0)
+		ret = otg_init(pdata->otg);
+		if (ret) {
+			dev_err(dev, "unable to init transceiver, probably missing\n");
+			ret = -ENODEV;
+			goto err_add;
+		}
+		ret = otg_set_vbus(pdata->otg, 1);
+		if (ret) {
 			dev_err(dev, "unable to enable vbus on transceiver\n");
+			goto err_add;
+		}
 	}
 
 	priv->hcd = hcd;
-- 
1.7.1

[PATCH 1/2] otg/ulpi: bail out on read errors

[Daniel Mack]

On Tue, Jun 15, 2010 at 12:34:22PM +0200, Wolfram Sang wrote:
> otg_read may return errnos, so bail out correctly to prevent bogus
> ID-numbers.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Daniel Mack <daniel@caiaq.de>

> Cc: Greg KH <gregkh@suse.de>
> ---
>  drivers/usb/otg/ulpi.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..d331b22 100644
> --- a/drivers/usb/otg/ulpi.c
> +++ b/drivers/usb/otg/ulpi.c
> @@ -59,12 +59,17 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
>  
>  static int ulpi_init(struct otg_transceiver *otg)
>  {
> -	int i, vid, pid;
> -
> -	vid = (otg_io_read(otg, ULPI_VENDOR_ID_HIGH) << 8) |
> -	       otg_io_read(otg, ULPI_VENDOR_ID_LOW);
> -	pid = (otg_io_read(otg, ULPI_PRODUCT_ID_HIGH) << 8) |
> -	       otg_io_read(otg, ULPI_PRODUCT_ID_LOW);
> +	int i, vid, pid, ret;
> +	u32 ulpi_id = 0;
> +
> +	for (i = 0; i < 4; i++) {
> +		ret = otg_io_read(otg, ULPI_PRODUCT_ID_HIGH - i);
> +		if (ret < 0)
> +			return ret;
> +		ulpi_id = (ulpi_id << 8) | ret;
> +	}
> +	vid = ulpi_id & 0xffff;
> +	pid = ulpi_id >> 16;
>  
>  	pr_info("ULPI transceiver vendor/product ID 0x%04x/0x%04x\n", vid, pid);
>  
> -- 
> 1.7.1
> 

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Daniel Mack]

On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> The old code registered the hcd even if there were no transceivers
> detected, leading to oopses like this if we try to probe a non-existant
> ULPI:

Hmm. I'm aware that there was a missing bail in this function, but
actually, I had hardware which didn't properly detect the ULPI chip but
still worked fine. There has been quite some discussion here about that,
and eventually I decided to not make this a hard error as it didn't
really harm.

> [    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
> [    2.740000] timeout polling for ULPI device
> [    2.740000] timeout polling for ULPI device
> [    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
> [    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host Controller
> [    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered, assigned bus number 2
> [    2.770000] Unhandled fault: external abort on non-linefetch (0x808) at 0xc4876184
> [    2.770000] Internal error: : 808 [#1] PREEMPT
> [    2.770000] last sysfs file:
> [    2.770000] Modules linked in:
> [    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
> [    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
> [    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
> [    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
> [    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
> [    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
> [    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
> [    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
> [    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM Segment kernel
> [    2.770000] Control: 0005317f  Table: a0004000  DAC: 00000017
> [    2.770000] Process swapper (pid: 1, stack limit = 0xc3814270)

But that, of course, changes everthing. If you happen to see such
Ooopses, your patch should go in, and I will check the boards again and
see if they still fail to probe the ULPI. Eventually, we might need some
kind of quirks field in the platform data :(

Thanks,
Daniel

> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Daniel Mack <daniel@caiaq.de>

> Cc: Greg KH <gregkh@suse.de>
> Cc: stable at kernel.org
> ---
>  drivers/usb/host/ehci-mxc.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
> index 544ccfd..bd40277 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -207,10 +207,17 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  	/* Initialize the transceiver */
>  	if (pdata->otg) {
>  		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
> -		if (otg_init(pdata->otg) != 0)
> -			dev_err(dev, "unable to init transceiver\n");
> -		else if (otg_set_vbus(pdata->otg, 1) != 0)
> +		ret = otg_init(pdata->otg);
> +		if (ret) {
> +			dev_err(dev, "unable to init transceiver, probably missing\n");
> +			ret = -ENODEV;
> +			goto err_add;
> +		}
> +		ret = otg_set_vbus(pdata->otg, 1);
> +		if (ret) {
>  			dev_err(dev, "unable to enable vbus on transceiver\n");
> +			goto err_add;
> +		}
>  	}
>  
>  	priv->hcd = hcd;
> -- 
> 1.7.1
> 

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Wolfram Sang]

On Tue, Jun 15, 2010 at 12:43:42PM +0200, Daniel Mack wrote:
> On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > The old code registered the hcd even if there were no transceivers
> > detected, leading to oopses like this if we try to probe a non-existant
> > ULPI:
> 
> Hmm. I'm aware that there was a missing bail in this function, but
> actually, I had hardware which didn't properly detect the ULPI chip but
> still worked fine. There has been quite some discussion here about that,
> and eventually I decided to not make this a hard error as it didn't
> really harm.

My use case is that we have two very similar boards, one uses just the second
host controller, the other one uses also OTG. Of course, we'd like to have just
one kernel, so we wanted to check if probing for OTG fails gracefully enough,
if the ULPI is not present. It did not, so this is what my patch intends to
fix. I think, checking why your boards fail to do the init, is the cleaner way
to go in the long run.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100615/c48eff7c/attachment.sig>

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Daniel Mack]

On Tue, Jun 15, 2010 at 01:03:33PM +0200, Wolfram Sang wrote:
> On Tue, Jun 15, 2010 at 12:43:42PM +0200, Daniel Mack wrote:
> > On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > > The old code registered the hcd even if there were no transceivers
> > > detected, leading to oopses like this if we try to probe a non-existant
> > > ULPI:
> > 
> > Hmm. I'm aware that there was a missing bail in this function, but
> > actually, I had hardware which didn't properly detect the ULPI chip but
> > still worked fine. There has been quite some discussion here about that,
> > and eventually I decided to not make this a hard error as it didn't
> > really harm.
> 
> My use case is that we have two very similar boards, one uses just the second
> host controller, the other one uses also OTG. Of course, we'd like to have just
> one kernel, so we wanted to check if probing for OTG fails gracefully enough,
> if the ULPI is not present. It did not, so this is what my patch intends to
> fix. I think, checking why your boards fail to do the init, is the cleaner way
> to go in the long run.

There's a design flaw in Freescale's MX31 reference designs which lets
the nCS pin float. And many board designers blindly copied that flaw
over to their own designs.

See

  http://marc.info/?l=linux-usb&m=125985151110906&w=4

So we might need quirks handling here.


Daniel

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Philippe Rétornaz]

Le mardi, 15 juin 2010 12.43:42, Daniel Mack a ?crit :
> On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > The old code registered the hcd even if there were no transceivers
> > detected, leading to oopses like this if we try to probe a non-existant
> > ULPI:
> 
> Hmm. I'm aware that there was a missing bail in this function, but
> actually, I had hardware which didn't properly detect the ULPI chip but
> still worked fine. There has been quite some discussion here about that,
> and eventually I decided to not make this a hard error as it didn't
> really harm.
> 
> > [    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
> > [    2.740000] timeout polling for ULPI device
> > [    2.740000] timeout polling for ULPI device
> > [    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
> > [    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host
> > Controller [    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered,
> > assigned bus number 2 [    2.770000] Unhandled fault: external abort on
> > non-linefetch (0x808) at 0xc4876184 [    2.770000] Internal error: : 808
> > [#1] PREEMPT
> > [    2.770000] last sysfs file:
> > [    2.770000] Modules linked in:
> > [    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
> > [    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
> > [    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
> > [    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
> > [    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
> > [    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
> > [    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
> > [    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
> > [    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> > Segment kernel [    2.770000] Control: 0005317f  Table: a0004000  DAC:
> > 00000017 [    2.770000] Process swapper (pid: 1, stack limit =
> > 0xc3814270)
> 
> But that, of course, changes everthing. If you happen to see such
> Ooopses, your patch should go in, and I will check the boards again and
> see if they still fail to probe the ULPI. Eventually, we might need some
> kind of quirks field in the platform data :(
> 

Have you tried my imx31 EHCI controller init patch applied ? 
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-May/014729.html

Btw, it has still not be applied, anybody knows why ? 

Regards,

Philippe

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Daniel Mack]

On Tue, Jun 15, 2010 at 01:47:50PM +0200, Philippe R?tornaz wrote:
> Le mardi, 15 juin 2010 12.43:42, Daniel Mack a ?crit :
> > On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > > The old code registered the hcd even if there were no transceivers
> > > detected, leading to oopses like this if we try to probe a non-existant
> > > ULPI:
> > 
> > Hmm. I'm aware that there was a missing bail in this function, but
> > actually, I had hardware which didn't properly detect the ULPI chip but
> > still worked fine. There has been quite some discussion here about that,
> > and eventually I decided to not make this a hard error as it didn't
> > really harm.
> > 
> > > [    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
> > > [    2.740000] timeout polling for ULPI device
> > > [    2.740000] timeout polling for ULPI device
> > > [    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
> > > [    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host
> > > Controller [    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered,
> > > assigned bus number 2 [    2.770000] Unhandled fault: external abort on
> > > non-linefetch (0x808) at 0xc4876184 [    2.770000] Internal error: : 808
> > > [#1] PREEMPT
> > > [    2.770000] last sysfs file:
> > > [    2.770000] Modules linked in:
> > > [    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
> > > [    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
> > > [    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
> > > [    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
> > > [    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
> > > [    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
> > > [    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
> > > [    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
> > > [    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> > > Segment kernel [    2.770000] Control: 0005317f  Table: a0004000  DAC:
> > > 00000017 [    2.770000] Process swapper (pid: 1, stack limit =
> > > 0xc3814270)
> > 
> > But that, of course, changes everthing. If you happen to see such
> > Ooopses, your patch should go in, and I will check the boards again and
> > see if they still fail to probe the ULPI. Eventually, we might need some
> > kind of quirks field in the platform data :(
> > 
> 
> Have you tried my imx31 EHCI controller init patch applied ? 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-May/014729.html

No, I haven't yet, as I'm not currently working on such a board. But I
will once I get back to that issue. Thanks for the reminder :)

Daniel

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Wolfram Sang]

On Tue, Jun 15, 2010 at 12:43:42PM +0200, Daniel Mack wrote:
> On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > The old code registered the hcd even if there were no transceivers
> > detected, leading to oopses like this if we try to probe a non-existant
> > ULPI:
> 
> Hmm. I'm aware that there was a missing bail in this function, but
> actually, I had hardware which didn't properly detect the ULPI chip but
> still worked fine. There has been quite some discussion here about that,
> and eventually I decided to not make this a hard error as it didn't
> really harm.

Hmm, so, do you think this patch is stable-material after all?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100616/bd0e517d/attachment.sig>

[PATCH 2/2] ehci-mxc: bail out on transceiver problems

[Daniel Mack]

On Wed, Jun 16, 2010 at 03:44:58AM +0200, Wolfram Sang wrote:
> On Tue, Jun 15, 2010 at 12:43:42PM +0200, Daniel Mack wrote:
> > On Tue, Jun 15, 2010 at 12:34:23PM +0200, Wolfram Sang wrote:
> > > The old code registered the hcd even if there were no transceivers
> > > detected, leading to oopses like this if we try to probe a non-existant
> > > ULPI:
> > 
> > Hmm. I'm aware that there was a missing bail in this function, but
> > actually, I had hardware which didn't properly detect the ULPI chip but
> > still worked fine. There has been quite some discussion here about that,
> > and eventually I decided to not make this a hard error as it didn't
> > really harm.
> 
> Hmm, so, do you think this patch is stable-material after all?

Hard to say. It might break existing board support, which would be a
regression. OTOH, it fixes an Oops. Don't know really ...

Daniel