Add board level usb supporrt for mxsxsabresd and mx6slevk

Include a weak function board_usb_phy_mode.

usb_phy_enable decide whether the controller in device mode or in host mode by '*phy_ctrl & USBPHY_CTRL_OTG_ID'.

There are two usb port on mx6sxsabresd and also mx6slevk.
1. OTG port
2. HOST port
There are connected to SOC USB controller OTG1 core and OTG2 core as
following: OTG1 core <----> board OTG port
OTG2 core <----> board HOST port

However to these two board, no ID pin for the board host port. If only use '*phy_ctrl & USBPHY_CTRL_OTG_ID' to decide the work mode, the host port will not work, because "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" will always set 'type' with USB_INIT_DEVICE.

So introduce this weak function to let board level code can decide to work in host or device mode, if board level code want to implement this function.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

Changes v3:
Take Marek's suggestions, replace 'return val & USBPHY_CTRL_OTG_ID' with this new function like 'return board_usb_phy_mode(index);' Here board_usb_phy_mode only has one parameter 'index' as board_ehci_power
and board_echi_hcd_init do.
"http://lists.denx.de/pipermail/u-boot/2014-November/194183.html" has
detailed discussion.

Changes v2:
Introduce a new weak function to let board have a choice to decide which
mode to work at.

 drivers/usb/host/ehci-mx6.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

drivers/usb/host/ehci-mx6.c +14 -0

index 9ec5a0a..e2a247e 100644
@@ -113,6 +113,20 @@ static void usb_power_config(int index)
 		     pll_480_ctrl_set);
 }
 
+int __weak board_usb_phy_mode(int port)
+{
+	void __iomem *phy_reg;
+	void __iomem *phy_ctrl;
+	u32 val;
+
+	phy_reg = (void __iomem *)phy_bases[port];
+	phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+	val = __raw_readl(phy_ctrl);
+
+	return val & USBPHY_CTRL_OTG_ID;
+}
+
 /* Return 0 : host node, <>0 : device mode */
 static int usb_phy_enable(int index, struct usb_ehci *ehci)
 {

drivers/usb/host/ehci-mx6.c +0 -0

 }
 
 /* Base address for this IP block is 0x02184800 */

Quoted 11 lines

Include a weak function board_usb_phy_mode.

usb_phy_enable decide whether the controller in device mode or in host mode by '*phy_ctrl & USBPHY_CTRL_OTG_ID'.

There are two usb port on mx6sxsabresd and also mx6slevk.
1. OTG port
2. HOST port
There are connected to SOC USB controller OTG1 core and OTG2 core as
following: OTG1 core <----> board OTG port
OTG2 core <----> board HOST port

This patch has nothing to do with any board, so this part should not be in the commit message.

Quoted 58 lines

However to these two board, no ID pin for the board host port. If only use '*phy_ctrl & USBPHY_CTRL_OTG_ID' to decide the work mode, the host port will not work, because "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" will always set 'type' with USB_INIT_DEVICE.

So introduce this weak function to let board level code can decide to work in host or device mode, if board level code want to implement this function.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

Changes v3:
Take Marek's suggestions, replace 'return val & USBPHY_CTRL_OTG_ID' with this new function like 'return board_usb_phy_mode(index);' Here board_usb_phy_mode only has one parameter 'index' as board_ehci_power
and board_echi_hcd_init do.
"http://lists.denx.de/pipermail/u-boot/2014-November/194183.html" has
detailed discussion.

Changes v2:
Introduce a new weak function to let board have a choice to decide which
mode to work at.

  drivers/usb/host/ehci-mx6.c | 16 +++++++++++++++-
  1 file changed, 15 insertions(+), 1 deletion(-)

drivers/usb/host/ehci-mx6.c +14 -0

index 9ec5a0a..e2a247e 100644
@@ -113,6 +113,20 @@ static void usb_power_config(int index)
  		     pll_480_ctrl_set);
  }
 
+int __weak board_usb_phy_mode(int port)
+{
+	void __iomem *phy_reg;
+	void __iomem *phy_ctrl;
+	u32 val;
+
+	phy_reg = (void __iomem *)phy_bases[port];
+	phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+	val = __raw_readl(phy_ctrl);
+
+	return val & USBPHY_CTRL_OTG_ID;
+}
+
  /* Return 0 : host node, <>0 : device mode */
  static int usb_phy_enable(int index, struct usb_ehci *ehci)
  {

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct usb_ehci
 *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);
  	__raw_writel(val, phy_ctrl);
 

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci *ehci) · line 177

Quoted 2 lines

drivers/usb/host/ehci-mx6.c +1 -1

-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

btw. an idea for a separate patch(set) -- the PHY registers should be converted to struct-based access.

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci *ehci) · line 180

Quoted 3 lines

drivers/usb/host/ehci-mx6.c +0 -0

  }
 
  /* Base address for this IP block is 0x02184800 */

Quoted 10 lines

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci · line 178

Quoted 7 lines

drivers/usb/host/ehci-mx6.c +1 -1

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct usb_ehci
 *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);
 
  	__raw_writel(val, phy_ctrl);
 
-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
         type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

What need to do is to let board can modify the type like following:

(quoted diff) +9 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;
+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

[...]

Quoted 24 lines

Quoted 10 lines

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci · line 178

Quoted 7 lines

drivers/usb/host/ehci-mx6.c +1 -1

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct usb_ehci
 *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);
 
   	__raw_writel(val, phy_ctrl);
 
-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
          type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;

Quoted 6 lines

drivers/usb/host/ehci-mx6.c +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

[...]

? 11/7/2014 7:09 PM, Marek Vasut ??:

Quoted 30 lines · Peng Fan

[...]

Quoted 26 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct

usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);

Quoted 8 lines

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci *ehci) · line 177

Quoted 4 lines

   	__raw_writel(val, phy_ctrl);

drivers/usb/host/ehci-mx6.c +1 -1

-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
          type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

ok. got it.

Quoted 6 lines

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;

Quoted 8 lines

Quoted 6 lines

drivers/usb/host/ehci-mx6.c +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy *)PHY_ADDRESS)". Here I also think pass phy register definition to board level code is not fine just as what we talked about passing ehci struct to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or DEVICE. If the board code want to implement this function, just return what the board want.

After all, this patch may looks like this:
In ehci-mx6.c

drivers/usb/host/ehci-mx6.c +16 -1

+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+
 
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:

(quoted diff) +7 -0

+int board_usb_phy_mode(int port)
+{
+	if (port == 1)
+		return USB_INIT_HOST;
+	else
+		return USB_INIT_DEVICE;
+}

I just want to keep it simple and do not want to touch usb phy register in board code.

Any ideas?

Quoted 102 lines

? 11/7/2014 7:09 PM, Marek Vasut ??:

Quoted 30 lines · Peng Fan

[...]

Quoted 26 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct

usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);

Quoted 8 lines

drivers/usb/host/ehci-mx6.c static int usb_phy_enable(int index, struct usb_ehci *ehci) · line 177

Quoted 4 lines

    	__raw_writel(val, phy_ctrl);

drivers/usb/host/ehci-mx6.c +1 -1

-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
           type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

ok. got it.

Quoted 6 lines

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;

Quoted 8 lines

Quoted 6 lines

drivers/usb/host/ehci-mx6.c +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy *)PHY_ADDRESS)". Here I also think pass phy register definition to board level code is not fine just as what we talked about passing ehci struct to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or DEVICE. If the board code want to implement this function, just return what the board want.

After all, this patch may looks like this:
In ehci-mx6.c

drivers/usb/host/ehci-mx6.c +16 -1

+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+
 
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:

(quoted diff) +7 -0

+int board_usb_phy_mode(int port)
+{
+	if (port == 1)
+		return USB_INIT_HOST;
+	else
+		return USB_INIT_DEVICE;
+}

I just want to keep it simple and do not want to touch usb phy register in board code.

Quoted 1 line

Any ideas?

This seems OKish for all but the part where usb_phy_enable() shouldn't be touched. The return value of usb_phy_enable() should really be a regular return code, not the PHY mode.

You can also still implement a function to query a PHY for it's mode, so you don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board code.

? 11/7/2014 8:17 PM, Marek Vasut ??:

Quoted 104 lines · Peng Fan

Quoted 103 lines

? 11/7/2014 7:09 PM, Marek Vasut ??:

Quoted 31 lines · Peng Fan

[...]

Quoted 27 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct

usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);

Quoted 8 lines

Review

Quoted 4 lines

        __raw_writel(val, phy_ctrl);

(quoted diff) +1 -1

-    return val & USBPHY_CTRL_OTG_ID;
+    return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just
paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
           type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

ok. got it.

Quoted 6 lines

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+    if (port == 1)
+       /* port1 works in HOST Mode */
+           *type = USB_INIT_HOST;

Quoted 8 lines

Quoted 6 lines

(quoted diff) +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy *)PHY_ADDRESS)". Here I also think pass phy register definition to board level code is not fine just as what we talked about passing ehci struct to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or DEVICE. If the board code want to implement this function, just return what the board want.

After all, this patch may looks like this:
In ehci-mx6.c

(quoted diff) +16 -1

+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+
 
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:

(quoted diff) +7 -0

+int board_usb_phy_mode(int port)
+{
+    if (port == 1)
+        return USB_INIT_HOST;
+    else
+        return USB_INIT_DEVICE;
+}

I just want to keep it simple and do not want to touch usb phy register in board code.

Quoted 6 lines

Quoted 1 line

Any ideas?

This seems OKish for all but the part where usb_phy_enable() shouldn't be touched. The return value of usb_phy_enable() should really be a regular return code, not the PHY mode.

ok. I'll fix this.

Quoted 4 lines

You can also still implement a function to query a PHY for it's mode, so you
don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board code.

I am not sure whether this following way is fine or not.

(quoted diff) +3 -0

+int board_usb_phy_mode(int index)
+               __attribute__((weak, alias("usb_phy_mode")));
+

in usb_phy_mode, query a PHY for it's mode.

And righter after usb_phy_enable in ehci-mx6.c.
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;

usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There is no status bit for query enabled or not, so just return 0.

In board file:
int board_usb_phy_mode(int port)

{
         if (port == 1)
                 return USB_INIT_HOST;
         else
                 return usb_phy_mode(port);
}

I think this is better way then previous patch, but i did not find where to put the usb_phy_mode prototype type, since board file will use it.

Regards,
Peng.

? 11/7/2014 8:17 PM, Marek Vasut ??:

Quoted 103 lines · Peng Fan

Quoted 102 lines

? 11/7/2014 7:09 PM, Marek Vasut ??:

Quoted 30 lines · Peng Fan

[...]

Quoted 26 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct

usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);

Quoted 8 lines

Review

Quoted 4 lines

    	__raw_writel(val, phy_ctrl);

(quoted diff) +1 -1

-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
           type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

ok. got it.

Quoted 6 lines

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;

Quoted 8 lines

Quoted 6 lines

(quoted diff) +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy *)PHY_ADDRESS)". Here I also think pass phy register definition to board level code is not fine just as what we talked about passing ehci struct to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or DEVICE. If the board code want to implement this function, just return what the board want.

After all, this patch may looks like this:
In ehci-mx6.c

(quoted diff) +16 -1

+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+
 
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:

(quoted diff) +7 -0

+int board_usb_phy_mode(int port)
+{
+	if (port == 1)
+		return USB_INIT_HOST;
+	else
+		return USB_INIT_DEVICE;
+}

I just want to keep it simple and do not want to touch usb phy register in board code.

Quoted 5 lines

Quoted 1 line

Any ideas?

This seems OKish for all but the part where usb_phy_enable() shouldn't be touched. The return value of usb_phy_enable() should really be a regular return code, not the PHY mode.

ok. I'll fix this.

Quoted 3 lines

You can also still implement a function to query a PHY for it's mode, so you don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board code.

I am not sure whether this following way is fine or not.

+
in usb_phy_mode, query a PHY for it's mode.

And righter after usb_phy_enable in ehci-mx6.c.
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;

(quoted diff) +2 -0

+       usb_phy_enable(index, ehci);
+       type = usb_phy_mode(index);

usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There is no status bit for query enabled or not, so just return 0.

In board file:
int board_usb_phy_mode(int port)

{
         if (port == 1)
                 return USB_INIT_HOST;
         else
                 return usb_phy_mode(port);
}

I think this is better way then previous patch, but i did not find where to put the usb_phy_mode prototype type, since board file will use it.

Quoted 117 lines · Peng Fan

? 11/7/2014 8:17 PM, Marek Vasut ??:

Quoted 103 lines · Peng Fan

Quoted 102 lines

? 11/7/2014 7:09 PM, Marek Vasut ??:

Quoted 30 lines · Peng Fan

[...]

Quoted 26 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct

usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);

Quoted 8 lines

Review

Quoted 4 lines

     	__raw_writel(val, phy_ctrl);

(quoted diff) +1 -1

-	return val & USBPHY_CTRL_OTG_ID;
+	return board_usb_phy_mode(index);

This should be called from ehci_hcd_init() right after usb_phy_enable(). Afterall, the mode detection has nothing to do with the PHY enabling.

This back to what I did in patch v2. right after usb_phy_enable(), just paste that piece of code here:

The weak function:

(quoted diff) +5 -0

+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+       return 0;
+}
+
 
            type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :

USB_INIT_HOST;

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       board_usb_phy_mode(index, &type);
+

The usb_phy_enable() should not return the PHY mode at all though. It should be the board_usb_phy_mode() which adjusts the PHY type. The usb_phy_enable() should return just a success/failure return value.

ok. got it.

Quoted 6 lines

Quoted 6 lines

What need to do is to let board can modify the type like following:

(quoted diff) +5 -0

+int board_usb_phy_mode(int port, enum usb_init_type *type)
+{
+	if (port == 1)
+       /* port1 works in HOST Mode */
+       	*type = USB_INIT_HOST;

Quoted 8 lines

Quoted 6 lines

(quoted diff) +4 -0

+
+       return 0;
+}
+

This is the way that I did in patch v2. If this is fine, I'll resent this patch set.

It should really explicitly set it, not modify it, see above.

I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy *)PHY_ADDRESS)". Here I also think pass phy register definition to board level code is not fine just as what we talked about passing ehci struct to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or DEVICE. If the board code want to implement this function, just return what the board want.

After all, this patch may looks like this:
In ehci-mx6.c

(quoted diff) +16 -1

+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+
 
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:

(quoted diff) +7 -0

+int board_usb_phy_mode(int port)
+{
+	if (port == 1)
+		return USB_INIT_HOST;
+	else
+		return USB_INIT_DEVICE;
+}

I just want to keep it simple and do not want to touch usb phy register in board code.

Quoted 5 lines

Quoted 1 line

Any ideas?

This seems OKish for all but the part where usb_phy_enable() shouldn't be touched. The return value of usb_phy_enable() should really be a regular return code, not the PHY mode.

ok. I'll fix this.

Quoted 3 lines

You can also still implement a function to query a PHY for it's mode, so you don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board code.

I am not sure whether this following way is fine or not.

Review

Quoted 2 lines

(quoted diff) +2 -0

+int board_usb_phy_mode(int index)
+               __attribute__((weak, alias("usb_phy_mode")));

__weak board_usb_phy_mode(int index) is fine.

Quoted 21 lines

+
in usb_phy_mode, query a PHY for it's mode.

And righter after usb_phy_enable in ehci-mx6.c.
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;

(quoted diff) +2 -0

+       usb_phy_enable(index, ehci);
+       type = usb_phy_mode(index);

usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There is no status bit for query enabled or not, so just return 0.

In board file:
int board_usb_phy_mode(int port)

{
          if (port == 1)
                  return USB_INIT_HOST;
          else
                  return usb_phy_mode(port);
}

Quoted 2 lines

I think this is better way then previous patch, but i did not find where to put the usb_phy_mode prototype type, since board file will use it.

Looks OK otherwise.

Quoted 24 lines

Quoted 24 lines

+
in usb_phy_mode, query a PHY for it's mode.

And righter after usb_phy_enable in ehci-mx6.c.
- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
USB_INIT_HOST;

(quoted diff) +2 -0

+       usb_phy_enable(index, ehci);
+       type = usb_phy_mode(index);

usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There is no status bit for query enabled or not, so just return 0.

In board file:
int board_usb_phy_mode(int port)
{

          if (port == 1)
          
                  return USB_INIT_HOST;
          
          else
          
                  return usb_phy_mode(port);

Quoted 6 lines

Quoted 4 lines

}

I think this is better way then previous patch, but i did not find where to put the usb_phy_mode prototype type, since board file will use it.

Looks OK otherwise.

Sent out v4 patch, please review.

Add pinmux settings, implement board_ehci_hcd_init, board_usb_phy_mode

There are two usb port on mx6slevk board:
1. otg port
2. host port
The following are the connection between usb controller and board usb interface, host port has not ID pin set:
otg1 core <---> board otg port
otg2 core <---> board host port
In order to make host port work, board_usb_phy_mode return 0 to let ehci-mx6.c driver decide otg2 core to works in host mode.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

Quoted 15 lines

Add pinmux settings, implement board_ehci_hcd_init, board_usb_phy_mode

There are two usb port on mx6slevk board:
1. otg port
2. host port
The following are the connection between usb controller and board usb interface, host port has not ID pin set:
otg1 core <---> board otg port
otg2 core <---> board host port
In order to make host port work, board_usb_phy_mode return 0 to let ehci-mx6.c driver decide otg2 core to works in host mode.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

[...]

Quoted 4 lines

(quoted diff) +0 -0

@@ -150,6 +151,63 @@ static int setup_fec(void)
  }
  #endif
 

board/freescale/mx6slevk/mx6slevk.c static int setup_fec(void) · line 158

Quoted 5 lines

board/freescale/mx6slevk/mx6slevk.c +5 -0

+#ifdef CONFIG_USB_EHCI_MX6
+#define USB_OTHERREGS_OFFSET	0x800
+#define USBPHY_CTRL		0x30
+#define UCTRL_PWR_POL		(1 << 9)
+#define USBPHY_CTRL_OTG_ID	0x08000000

This looks like an duplication. Aren't those bits defined somewhere in generic code already ?

[...]
Best regards,
Marek Vasut

? 11/7/2014 4:26 PM, Marek Vasut ??:

Quoted 24 lines · Peng Fan

Quoted 15 lines

Add pinmux settings, implement board_ehci_hcd_init, board_usb_phy_mode

There are two usb port on mx6slevk board:
1. otg port
2. host port
The following are the connection between usb controller and board usb interface, host port has not ID pin set:
otg1 core <---> board otg port
otg2 core <---> board host port
In order to make host port work, board_usb_phy_mode return 0 to let ehci-mx6.c driver decide otg2 core to works in host mode.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

[...]

Quoted 5 lines

(quoted diff) +0 -0

@@ -150,6 +151,63 @@ static int setup_fec(void)
 
  }
  #endif
 

Quoted 8 lines

board/freescale/mx6slevk/mx6slevk.c static int setup_fec(void) · line 158

Quoted 5 lines

board/freescale/mx6slevk/mx6slevk.c +5 -0

+#ifdef CONFIG_USB_EHCI_MX6
+#define USB_OTHERREGS_OFFSET	0x800
+#define USBPHY_CTRL		0x30
+#define UCTRL_PWR_POL		(1 << 9)
+#define USBPHY_CTRL_OTG_ID	0x08000000

This looks like an duplication. Aren't those bits defined somewhere in generic code already ?

If this way 'int board_usb_phy_mode(int port, enum usb_init_type *type)' can be accpeted, these bits are not needed and I'll move these bits in the seperate PHY register struct access patch. Anyway, after the board level usb support patch.

Quoted 34 lines

? 11/7/2014 4:26 PM, Marek Vasut ??:

Quoted 32 lines · Peng Fan

Quoted 15 lines

Add pinmux settings, implement board_ehci_hcd_init, board_usb_phy_mode

There are two usb port on mx6slevk board:
1. otg port
2. host port
The following are the connection between usb controller and board usb interface, host port has not ID pin set:
otg1 core <---> board otg port
otg2 core <---> board host port
In order to make host port work, board_usb_phy_mode return 0 to let ehci-mx6.c driver decide otg2 core to works in host mode.

Signed-off-by Peng Fan <[email protected]>

Signed-off-by Ye Li <[email protected]>

---

[...]

board/freescale/mx6slevk/mx6slevk.c static int setup_fec(void) · line 159

Quoted 10 lines

board/freescale/mx6slevk/mx6slevk.c +5 -0

@@ -150,6 +151,63 @@ static int setup_fec(void)
 
   }
   #endif
 
+#ifdef CONFIG_USB_EHCI_MX6
+#define USB_OTHERREGS_OFFSET	0x800
+#define USBPHY_CTRL		0x30
+#define UCTRL_PWR_POL		(1 << 9)
+#define USBPHY_CTRL_OTG_ID	0x08000000

This looks like an duplication. Aren't those bits defined somewhere in generic code already ?

Quoted 4 lines

If this way 'int board_usb_phy_mode(int port, enum usb_init_type *type)' can be accpeted, these bits are not needed and I'll move these bits in the seperate PHY register struct access patch. Anyway, after the board level usb support patch.

What about abstracting that stuff into a function which returns the PHY's idea of the current mode instead. That way, you can determine the PHY's idea of the mode from both board code and the driver code.

Best regards,
Marek Vasut