Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-10 Thread Marek Vasut
On Monday, November 10, 2014 at 02:01:50 AM, Peng Fan wrote:
[...]
  +
  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(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.
  
  Looks OK otherwise.
 
 Sent out v4 patch, please review.

Thanks! Will do as time permits. Sorry for possible delays.

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-09 Thread Peng Fan



On 11/8/2014 7:33 PM, Marek Vasut wrote:

On Saturday, November 08, 2014 at 05:07:21 AM, Peng Fan wrote:

在 11/7/2014 8:17 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:

在 11/7/2014 7:09 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:

[...]


@@ -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:
+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;

+   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.


What need to do is to let board can modify the `type` like following:
+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.


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
+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:
+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?


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.


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.
+int board_usb_phy_mode(int index)
+   __attribute__((weak, alias(usb_phy_mode)));


__weak board_usb_phy_mode(int index) is fine.


+
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(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.


Looks OK otherwise.


Sent out v4 patch, please review.

Thanks,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-08 Thread Marek Vasut
On Saturday, November 08, 2014 at 05:07:21 AM, Peng Fan wrote:
 在 11/7/2014 8:17 PM, Marek Vasut 写道:
  On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:
  在 11/7/2014 7:09 PM, Marek Vasut 写道:
  On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:
  
  [...]
  
  @@ -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:
  +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;
  
  +   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.
  
  What need to do is to let board can modify the `type` like following:
  +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.
  
  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
  +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:
  +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?
  
  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.
 
  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.
 +int board_usb_phy_mode(int index)
 +   __attribute__((weak, alias(usb_phy_mode)));

__weak board_usb_phy_mode(int index) is fine.

 +
 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(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.

Looks OK otherwise.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Marek Vasut
On Friday, November 07, 2014 at 02:08:12 AM, Peng Fan wrote:
 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.

 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 peng@freescale.com
 Signed-off-by: Ye Li b37...@freescale.com
 ---
 
 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(-)
 
 diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
 index 9ec5a0a..e2a247e 100644
 --- a/drivers/usb/host/ehci-mx6.c
 +++ b/drivers/usb/host/ehci-mx6.c
 @@ -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);
 
 - 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.

  }
 
  /* Base address for this IP block is 0x02184800 */
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Peng Fan



在 11/7/2014 4:25 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 02:08:12 AM, Peng Fan wrote:

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.


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 peng@freescale.com
Signed-off-by: Ye Li b37...@freescale.com
---

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(-)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 9ec5a0a..e2a247e 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -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);

-   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:
+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;


+   board_usb_phy_mode(index, type);
+

What need to do is to let board can modify the `type` like following:
+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.

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

Yeah, struct based access PHY register should be done. After this board 
level usb support patch is finished.

  }

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

Regards,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Marek Vasut
On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:

[...]

  @@ -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:
 +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;
 
 +   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.

 What need to do is to let board can modify the `type` like following:
 +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.

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

[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Peng Fan



在 11/7/2014 7:09 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:

[...]


@@ -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:
+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;

+   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.

What need to do is to let board can modify the `type` like following:
+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.


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
+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:
+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?

[...]


Regards,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Marek Vasut
On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:
 在 11/7/2014 7:09 PM, Marek Vasut 写道:
  On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:
  
  [...]
  
  @@ -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:
  +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;
  
  +   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.
 
  What need to do is to let board can modify the `type` like following:
  +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.
  
  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
 +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:
 +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?

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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Peng Fan



在 11/7/2014 8:17 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:

在 11/7/2014 7:09 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:

[...]


@@ -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:
+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;

+   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.


What need to do is to let board can modify the `type` like following:
+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.


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
+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:
+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?


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.

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.
+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(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.


Regards,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

2014-11-07 Thread Peng Fan



在 11/8/2014 12:07 PM, Peng Fan 写道:



在 11/7/2014 8:17 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:

在 11/7/2014 7:09 PM, Marek Vasut 写道:

On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:

[...]


@@ -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:
+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;

+   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.


What need to do is to let board can modify the `type` like following:
+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.


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
+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:
+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?


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.

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.
+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(index, ehci);
+   type = usb_phy_mode(index);

This should be 'type = board_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.

Regards,
Peng.

Regards,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot