powerpc/usb: fix bug of CPU hang when missing USB PHY clock

PATCH Superseded Shengzhou Liu 5 participants 10 messages Oct 18, 2012 – Oct 22, 2012

Subsystems (2)

USB USB ehci driver

lore

Share & tools

lore

Shengzhou Liu

author

2012-10-18 02:40 UTC #0 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

when missing USB PHY clock, u-boot will hang during USB initialization when issuing "usb start". We should check USBGP[PHY_CLK_VALID] bit to avoid CPU hanging in this case.

Signed-off-by Shengzhou Liu <[email protected]>

---
against master branch of upstream u-boot tree

 drivers/usb/host/ehci-fsl.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

drivers/usb/host/ehci-fsl.c +16 -6

index 7b8f033..68abf11 100644
@@ -30,6 +30,18 @@
 
 #include "ehci.h"
 
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci)
+{
+	if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+			(!in_be32(&ehci->prictrl))) {
+		printf("USB PHY clock invalid!\n");
+		return 0;
+	} else {
+		return 1;
+	}
+}
+
 /*
  * Create the appropriate control structures to manage
  * a new EHCI host controller.

@@ -82,18 +94,16 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 		udelay(1000); /* delay required for PHY Clk to appear */
 #endif
 		out_le32(&(*hcor)->or_portsc[0], PORT_PTS_UTMI);
+		setbits_be32(&ehci->control, USB_EN);
 	} else {
-#if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
-		clrbits_be32(&ehci->control, UTMI_PHY_EN);
 		setbits_be32(&ehci->control, PHY_CLK_SEL_ULPI);
+		clrsetbits_be32(&ehci->control, UTMI_PHY_EN, USB_EN);
 		udelay(1000); /* delay required for PHY Clk to appear */
-#endif
+		if (!usb_phy_clk_valid(ehci))
+			return -EINVAL;
 		out_le32(&(*hcor)->or_portsc[0], PORT_PTS_ULPI);
 	}
 
-	/* Enable interface. */
-	setbits_be32(&ehci->control, USB_EN);
-
 	out_be32(&ehci->prictrl, 0x0000000c);
 	out_be32(&ehci->age_cnt_limit, 0x00000040);
 	out_be32(&ehci->sictrl, 0x00000001);

Signature · 1 line

1.6.4

Liu Shengzhou-B36685

2012-10-18 06:28 UTC #2 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 9 lines

-----Original Message-----
drivers/usb/host/ehci-fsl.c line 37
Quoted 5 lines
drivers/usb/host/ehci-fsl.c +4 -0
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci) {
+	if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+			(!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)

[Shengzhou] Yes, using !(A||B) is also okay:)

Quoted 5 lines

drivers/usb/host/ehci-fsl.c line 38
Quoted 2 lines
drivers/usb/host/ehci-fsl.c +1 -0
+		printf("USB PHY clock invalid!\n");
debug() ?

[Shengzhou] No, it's not for debug purpose, it should be printf() as a necessary info.

Marek Vasut

2012-10-18 07:15 UTC #3 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Dear Liu Shengzhou-B36685,

Quoted 11 lines

Quoted 9 lines
-----Original Message-----
drivers/usb/host/ehci-fsl.c line 37
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +4 -0
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci) {
+	if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+			(!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)
[Shengzhou] Yes, using !(A||B) is also okay:)

Good, you did your logic homework well. Now go one step further:

if (a || b)
return 1;

printf()
return 0;

How will that work?

Quoted 7 lines

Quoted 4 lines
drivers/usb/host/ehci-fsl.c line 38
Quoted 2 lines
drivers/usb/host/ehci-fsl.c +1 -0
+		printf("USB PHY clock invalid!\n");
debug() ?
[Shengzhou] No, it's not for debug purpose, it should be printf() as a necessary info.

OK.

Best regards,
Marek Vasut

Liu Shengzhou-B36685

2012-10-18 08:54 UTC #4 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 21 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, October 18, 2012 3:16 PM
To: Liu Shengzhou-B36685
Cc: u-boot at lists.denx.de; Stefan Roese; agust at denx.de
Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
Dear Liu Shengzhou-B36685,
Quoted 9 lines
Quoted 8 lines
-----Original Message-----
drivers/usb/host/ehci-fsl.c line 37
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +4 -0
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci) {
+	if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+			(!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)

Quoted 7 lines

Quoted 1 line
[Shengzhou] Yes, using !(A||B) is also okay:)
Good, you did your logic homework well. Now go one step further:
if (a || b)
return 1;

[Shengzhou] No, this doesn't work, b is 0 at initial time, but b is 1 at the second time, a is depend on the register PHY_CLK_VALID bit, We just want to check it at the first time and then think it is always valid after that, it's using a trick:)

Quoted 4 lines

printf()
return 0;
How will that work?

Marek Vasut

2012-10-18 09:04 UTC #5 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Dear Liu Shengzhou-B36685,

Quoted 24 lines

Quoted 24 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, October 18, 2012 3:16 PM
To: Liu Shengzhou-B36685
Cc: u-boot at lists.denx.de; Stefan Roese; agust at denx.de
Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
Dear Liu Shengzhou-B36685,
Quoted 11 lines
Quoted 9 lines
-----Original Message-----
drivers/usb/host/ehci-fsl.c line 37
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +4 -0
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci) {
+	if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+			(!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)
[Shengzhou] Yes, using !(A||B) is also okay:)
Good, you did your logic homework well. Now go one step further:

Quoted 8 lines

Quoted 3 lines
if (a || b)
return 1;
[Shengzhou] No, this doesn't work, b is 0 at initial time, but b is 1 at the second time, a is depend on the register PHY_CLK_VALID bit, We just want to check it at the first time and then think it is always valid after that, it's using a trick:)

Good point, I was just testing you of course ;-)

Best regards,
Marek Vasut

Andy Fleming

2012-10-19 21:21 UTC #6 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 17 lines · Marek Vasut

Dear Liu Shengzhou-B36685,
Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
Quoted 8 lines
drivers/usb/host/ehci-fsl.c line 37
Quoted 5 lines
drivers/usb/host/ehci-fsl.c +4 -0
+/* Check USB PHY clock valid */
+static int usb_phy_clk_valid(struct usb_ehci *ehci) {
+     if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+                     (!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)
[Shengzhou] Yes, using !(A||B) is also okay:)
Good, you did your logic homework well. Now go one step further:
if (a || b)

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
return 1;
[Shengzhou] No, this doesn't work, b is 0 at initial time, but b is 1 at the second time, a is depend on the register PHY_CLK_VALID bit, We just want to check it at the first time and then think it is always valid after that, it's using a trick:)
Good point, I was just testing you of course ;-)

I may just be dim. Why is this a good point? If in_be32(&ehci->prictrl) is non-zero, then this function will return '1'. If (in_be32(&ehci->control) & PHY_CLK_VALID) is non-zero, this function will return 1.

What am I missing?

Andy

Liu Shengzhou-B36685

2012-10-22 04:51 UTC #7 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 38 lines

-----Original Message-----
From: Andy Fleming [mailto:afleming at gmail.com]
Sent: Saturday, October 20, 2012 5:22 AM
To: Marek Vasut
Cc: Liu Shengzhou-B36685; u-boot at lists.denx.de; Stefan Roese
Subject: Re: [U-Boot] [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
Quoted 17 lines · Marek Vasut
Dear Liu Shengzhou-B36685,
Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +4 -0
+/* Check USB PHY clock valid */ static int
+usb_phy_clk_valid(struct usb_ehci *ehci) {
+     if ((!(in_be32(&ehci->control) & PHY_CLK_VALID)) &&
+                     (!in_be32(&ehci->prictrl))) {
(!A && !B) condition can certainly be done without the double negation ;-)
[Shengzhou] Yes, using !(A||B) is also okay:)
Good, you did your logic homework well. Now go one step further:
if (a || b)
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
return 1;
[Shengzhou] No, this doesn't work, b is 0 at initial time, but b is 1 at the second time, a is depend on the register PHY_CLK_VALID bit, We just want to check it at the first time and then think it is always valid after that, it's using a trick:)
Good point, I was just testing you of course ;-)

Quoted 8 lines

I may just be dim. Why is this a good point? If in_be32(&ehci->prictrl) is non-zero, then this function will return '1'. If (in_be32(&ehci->control) & PHY_CLK_VALID) is non-zero, this function will return 1.
What am I missing?
Andy

[Shengzhou] Because the indication of PHY_CLK_VALID is time-sensitive, we just think the value read at the first time is reliable, then PHY_CLK_VALID will be zero after that though actually PHY clock is still valid.

Anatolij Gustschin

2012-10-19 20:13 UTC #8 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

In reply to Marek Vasut

Hi,

Quoted 9 lines · On Thu, 18 Oct 2012 06:21:56 +0200 Marek Vasut <[email protected]> wrote:

Dear Shengzhou Liu,
Quoted 5 lines
when missing USB PHY clock, u-boot will hang during USB initialization when issuing "usb start". We should check USBGP[PHY_CLK_VALID] bit to avoid CPU hanging in this case.
Signed-off-by Shengzhou Liu <[email protected]>
CCing PPC experts.

...

Quoted 18 lines

(quoted diff) +0 -0

@@ -82,18 +94,16 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,

struct ehci_hcor **hcor) udelay(1000); /* delay required for PHY Clk to appear */
#endif

drivers/usb/host/ehci-fsl.c +4 -3

 		out_le32(&(*hcor)->or_portsc[0], PORT_PTS_UTMI);
+		setbits_be32(&ehci->control, USB_EN);
 	} else {
-#if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
-		clrbits_be32(&ehci->control, UTMI_PHY_EN);
 		setbits_be32(&ehci->control, PHY_CLK_SEL_ULPI);
+		clrsetbits_be32(&ehci->control, UTMI_PHY_EN, USB_EN);
 		udelay(1000); /* delay required for PHY Clk to appear */
-#endif
+		if (!usb_phy_clk_valid(ehci))
+			return -EINVAL;
 		out_le32(&(*hcor)->or_portsc[0], PORT_PTS_ULPI);
 	}

Quoted 2 lines

drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) · line 95
Quoted 2 lines
drivers/usb/host/ehci-fsl.c +0 -2
-	/* Enable interface. */
-	setbits_be32(&ehci->control, USB_EN);

you moved the USB interface enabling before the PHY CLK check but the commit description doesn't mention why it is needed. It would be good to mention the reason in the commit log.

Thanks,
Anatolij

Liu Shengzhou-B36685

2012-10-22 03:47 UTC #9 lore raw

PATCH 1/1 powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 9 lines

-----Original Message-----
From: Anatolij Gustschin [mailto:agust at denx.de]
Sent: Saturday, October 20, 2012 4:14 AM
To: Liu Shengzhou-B36685
Cc: Marek Vasut; u-boot at lists.denx.de; Stefan Roese
Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock

Quoted 6 lines

you moved the USB interface enabling before the PHY CLK check but the commit description doesn't mention why it is needed. It would be good to mention the reason in the commit log.
Thanks,
Anatolij

[Shengzhou] Ok, will add it in the commit log, thanks.