usb: ehci-orion: add more constants for register values

PATCH Superseded Thomas Petazzoni 2 participants 5 messages Mar 19, 2015

Subsystems (2)

USB USB ehci driver

lore

Share & tools

lore

Thomas Petazzoni

author

2015-03-19 14:01 UTC #0 lore raw

PATCH v2 1/1 usb: ehci-orion: add more constants for register values

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 <[email protected]>

---
Changes since v1: use tabs instead of spaces for alignment.

drivers/usb/host/ehci-orion.c | 18 ++++++++++++------

 1 file changed, 12 insertions(+), 6 deletions(-)

drivers/usb/host/ehci-orion.c +12 -6

index f6eafec..bfcbb9a 100644
@@ -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

Signature · 1 line

2.1.0

Alan Stern

maintainer

2015-03-19 14:27 UTC #1 lore raw

PATCH v2 1/1 usb: ehci-orion: add more constants for register values

Quoted 5 lines · 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 <[email protected]>

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

drivers/usb/host/ehci-orion.c static void orion_usb_phy_v1_setup(struct usb_hcd *hcd) · line 79

Quoted 8 lines

drivers/usb/host/ehci-orion.c +2 -2

@@ -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

Thomas Petazzoni

author

2015-03-19 14:45 UTC #2 lore raw

PATCH v2 1/1 usb: ehci-orion: add more constants for register values

Dear Alan Stern,

Quoted 15 lines · Alan Stern

drivers/usb/host/ehci-orion.c static void orion_usb_phy_v1_setup(struct usb_hcd *hcd) · line 79
Quoted 8 lines
drivers/usb/host/ehci-orion.c +2 -2
@@ -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)
		;

Quoted 6 lines

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

Signature · 3 lines

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

Alan Stern

maintainer

2015-03-19 20:30 UTC #3 lore raw

PATCH v2 1/1 usb: ehci-orion: add more constants for register values

Quoted 3 lines · Thomas Petazzoni

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 <[email protected]>

to the v2 patch.

Quoted 6 lines

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

Thomas Petazzoni

author

2015-03-19 21:06 UTC #4 lore raw

PATCH v2 1/1 usb: ehci-orion: add more constants for register values

Alan,

Quoted 9 lines · Alan Stern

Quoted 3 lines · Thomas Petazzoni
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 <[email protected]>

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

Quoted 9 lines

Quoted 6 lines
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

Signature · 3 lines

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