[U-Boot] [PATCH] 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 Shengzhou.Liu@freescale.com --- against master branch of upstream u-boot tree
drivers/usb/host/ehci-fsl.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index 7b8f033..68abf11 100644 --- a/drivers/usb/host/ehci-fsl.c +++ b/drivers/usb/host/ehci-fsl.c @@ -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);
Dear Shengzhou Liu,
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 Shengzhou.Liu@freescale.com
CCing PPC experts.
against master branch of upstream u-boot tree
drivers/usb/host/ehci-fsl.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index 7b8f033..68abf11 100644 --- a/drivers/usb/host/ehci-fsl.c +++ b/drivers/usb/host/ehci-fsl.c @@ -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))) {
(!A && !B) condition can certainly be done without the double negation ;-)
printf("USB PHY clock invalid!\n");
debug() ?
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);-#if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
clrbits_be32(&ehci->control, UTMI_PHY_EN);
clrsetbits_be32(&ehci->control, UTMI_PHY_EN, USB_EN);-#endif
if (!usb_phy_clk_valid(ehci))return -EINVAL;
- /* 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);
Best regards, Marek Vasut
-----Original Message-----
+/* 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:)
printf("USB PHY clock invalid!\n");debug() ?
[Shengzhou] No, it's not for debug purpose, it should be printf() as a necessary info.
Dear Liu Shengzhou-B36685,
-----Original Message-----
+/* 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?
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
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, October 18, 2012 3:16 PM To: Liu Shengzhou-B36685 Cc: u-boot@lists.denx.de; Stefan Roese; agust@denx.de Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
Dear Liu Shengzhou-B36685,
-----Original Message-----
+/* 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;
[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:)
printf() return 0;
How will that work?
Dear Liu Shengzhou-B36685,
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, October 18, 2012 3:16 PM To: Liu Shengzhou-B36685 Cc: u-boot@lists.denx.de; Stefan Roese; agust@denx.de Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
Dear Liu Shengzhou-B36685,
-----Original Message-----
+/* 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;
[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
On Thu, Oct 18, 2012 at 4:04 AM, Marek Vasut marex@denx.de wrote:
Dear Liu Shengzhou-B36685,
+/* 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;
[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
-----Original Message----- From: Andy Fleming [mailto:afleming@gmail.com] Sent: Saturday, October 20, 2012 5:22 AM To: Marek Vasut Cc: Liu Shengzhou-B36685; u-boot@lists.denx.de; Stefan Roese Subject: Re: [U-Boot] [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
On Thu, Oct 18, 2012 at 4:04 AM, Marek Vasut marex@denx.de wrote:
Dear Liu Shengzhou-B36685,
> +/* 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;
[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
[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.
Hi,
On Thu, 18 Oct 2012 06:21:56 +0200 Marek Vasut marex@denx.de wrote:
Dear Shengzhou Liu,
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 Shengzhou.Liu@freescale.com
CCing PPC experts.
...
@@ -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);-#if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
clrbits_be32(&ehci->control, UTMI_PHY_EN);
clrsetbits_be32(&ehci->control, UTMI_PHY_EN, USB_EN);-#endif
if (!usb_phy_clk_valid(ehci))return -EINVAL;
- /* 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
-----Original Message----- From: Anatolij Gustschin [mailto:agust@denx.de] Sent: Saturday, October 20, 2012 4:14 AM To: Liu Shengzhou-B36685 Cc: Marek Vasut; u-boot@lists.denx.de; Stefan Roese Subject: Re: [PATCH] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
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.
participants (5)
-
Anatolij Gustschin -
Andy Fleming -
Liu Shengzhou-B36685 -
Marek Vasut -
Shengzhou Liu