[PATCH v2] usb: ehci-orion: add more constants for register values

[PATCH v2] usb: ehci-orion: add more constants for register values

[Thomas Petazzoni]

This commit adds new register values for the USB_CMD and USB_MODE
registers, which allows to avoid the usage of a number of magic values
in orion_usb_phy_v1_setup().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Changes since v1: use tabs instead of spaces for alignment.

drivers/usb/host/ehci-orion.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index f6eafec..bfcbb9a 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -29,7 +29,13 @@
 #define wrl(off, val)	writel_relaxed((val), hcd->regs + (off))
 
 #define USB_CMD			0x140
+#define   USB_CMD_RUN		BIT(0)
+#define   USB_CMD_RESET		BIT(1)
 #define USB_MODE		0x1a8
+#define   USB_MODE_MASK		GENMASK(1, 0)
+#define   USB_MODE_DEVICE	0x2
+#define   USB_MODE_HOST		0x3
+#define   USB_MODE_SDIS		BIT(4)
 #define USB_CAUSE		0x310
 #define USB_MASK		0x314
 #define USB_WINDOW_CTRL(i)	(0x320 + ((i) << 4))
@@ -69,8 +75,8 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
 	/*
 	 * Reset controller
 	 */
-	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
-	while (rdl(USB_CMD) & 0x2);
+	wrl(USB_CMD, rdl(USB_CMD) | USB_CMD_RESET);
+	while (rdl(USB_CMD) & USB_CMD_RESET);
 
 	/*
 	 * GL# USB-10: Set IPG for non start of frame packets
@@ -112,16 +118,16 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
 	/*
 	 * Stop and reset controller
 	 */
-	wrl(USB_CMD, rdl(USB_CMD) & ~0x1);
-	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
-	while (rdl(USB_CMD) & 0x2);
+	wrl(USB_CMD, rdl(USB_CMD) & ~USB_CMD_RUN);
+	wrl(USB_CMD, rdl(USB_CMD) | USB_CMD_RESET);
+	while (rdl(USB_CMD) & USB_CMD_RESET);
 
 	/*
 	 * GL# USB-5 Streaming disable REG_USB_MODE[4]=1
 	 * TBD: This need to be done after each reset!
 	 * GL# USB-4 Setup USB Host mode
 	 */
-	wrl(USB_MODE, 0x13);
+	wrl(USB_MODE, USB_MODE_SDIS | USB_MODE_HOST);
 }
 
 static void
-- 
2.1.0

[PATCH v2] usb: ehci-orion: add more constants for register values

[Alan Stern]

On Thu, 19 Mar 2015, Thomas Petazzoni wrote:

> This commit adds new register values for the USB_CMD and USB_MODE
> registers, which allows to avoid the usage of a number of magic values
> in orion_usb_phy_v1_setup().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

The new symbol values are fine.  But (side issue) ...

> @@ -69,8 +75,8 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
>  	/*
>  	 * Reset controller
>  	 */
> -	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
> -	while (rdl(USB_CMD) & 0x2);
> +	wrl(USB_CMD, rdl(USB_CMD) | USB_CMD_RESET);
> +	while (rdl(USB_CMD) & USB_CMD_RESET);

For one thing, this use of whitespace does not make the syntax clear.  
At the very least, it should be written as:

	while (rdl(USB_CMD) & USB_CMD_RESET)
		;

so that the reader can see this is a loop with an empty body.  Right 
now it looks like an ordinary, non-looping statement.

For another, have you considered what will happen if the hardware is 
defective and never turns off the USB_CMD_RESET bit?  This sort of 
thing should always be implemented with some sort of timeout.

Alan Stern

[PATCH v2] usb: ehci-orion: add more constants for register values

[Thomas Petazzoni]

Dear Alan Stern,

On Thu, 19 Mar 2015 10:27:18 -0400 (EDT), Alan Stern wrote:

> > @@ -69,8 +75,8 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
> >  	/*
> >  	 * Reset controller
> >  	 */
> > -	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
> > -	while (rdl(USB_CMD) & 0x2);
> > +	wrl(USB_CMD, rdl(USB_CMD) | USB_CMD_RESET);
> > +	while (rdl(USB_CMD) & USB_CMD_RESET);
> 
> For one thing, this use of whitespace does not make the syntax clear.  
> At the very least, it should be written as:
> 
> 	while (rdl(USB_CMD) & USB_CMD_RESET)
> 		;
> 
> so that the reader can see this is a loop with an empty body.  Right 
> now it looks like an ordinary, non-looping statement.
> 
> For another, have you considered what will happen if the hardware is 
> defective and never turns off the USB_CMD_RESET bit?  This sort of 
> thing should always be implemented with some sort of timeout.

These are indeed all valid concerns. However, as you can see, those
concerns are completely orthogonal to the patch: the original code
already has those issues.

Regarding the addition of a timeout, I unfortunately have absolutely no
idea what would be the a proper timeout value at this place. I quickly
glanced through
http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/ehci-specification-for-usb.pdf
for the documentation of this reset bit, but couldn't spot a maximum
accepted duration for the operation.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2] usb: ehci-orion: add more constants for register values

[Alan Stern]

On Thu, 19 Mar 2015, Thomas Petazzoni wrote:

> These are indeed all valid concerns. However, as you can see, those
> concerns are completely orthogonal to the patch: the original code
> already has those issues.

Quite true.  You may add

Acked-by: Alan Stern <stern@rowland.harvard.edu>

to the v2 patch.

> Regarding the addition of a timeout, I unfortunately have absolutely no
> idea what would be the a proper timeout value at this place. I quickly
> glanced through
> http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/ehci-specification-for-usb.pdf
> for the documentation of this reset bit, but couldn't spot a maximum
> accepted duration for the operation.

ehci-hcd uses 250 ms.  This seems to be an arbitrary value, but at 
least it's better than hanging the system.

Alan Stern

[PATCH v2] usb: ehci-orion: add more constants for register values

[Thomas Petazzoni]

Alan,

On Thu, 19 Mar 2015 16:30:09 -0400 (EDT), Alan Stern wrote:
> On Thu, 19 Mar 2015, Thomas Petazzoni wrote:
> 
> > These are indeed all valid concerns. However, as you can see, those
> > concerns are completely orthogonal to the patch: the original code
> > already has those issues.
> 
> Quite true.  You may add
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks. So I presume this means Greg will take the patch?

> > Regarding the addition of a timeout, I unfortunately have absolutely no
> > idea what would be the a proper timeout value at this place. I quickly
> > glanced through
> > http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/ehci-specification-for-usb.pdf
> > for the documentation of this reset bit, but couldn't spot a maximum
> > accepted duration for the operation.
> 
> ehci-hcd uses 250 ms.  This seems to be an arbitrary value, but at 
> least it's better than hanging the system.

Ok, then I'll cook a patch to adjust this aspect of the driver.

Thanks for the suggestion,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com