[U-Boot] [PATCH v3 0/3] Add board level usb supporrt for mxsxsabresd and mx6slevk

[U-Boot] [PATCH v3 0/3] Add board level usb supporrt for mxsxsabresd and mx6slevk

[Peng Fan]

This patch set is mainly to add board level usb support for mx6sxsabresd and
mx6slevk. Add pin mux settings and implement board_ehci_hcd_init and board_usb
_phy_mode.

Also in order to make the host port work for these two boards. A new weak
function is introduced. Detailed info about this is in the "usb:ehci-mx6
add board_usb_phy_mode function" patch commit log.

This patch set has already been tested on mx6sxsabresd and mx6slevk board.

Changes v3:
 Take Marek's suggestions, replace 'return val & USBPHY_CTRL_OTG_ID' with
 this new function like 'return board_usb_phy_mode(index);'
 Detailed discussion here:
 http://lists.denx.de/pipermail/u-boot/2014-November/194131.html

Changes v2:
 Introduce a new weak function to let board have a choice to decide which mode
 to work at. 
 move pinmux setting into board_init
 add otg polarity setting in board_ehci_hcd_init
 set pinmux struct static

Peng Fan (3):
  usb:ehci-mx6 add board_usb_phy_mode function
  imx:mx6sxsabresd add board level support for usb
  imx:mx6slevk add board level support for usb

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h  |  5 +++
 board/freescale/mx6slevk/mx6slevk.c         | 63 +++++++++++++++++++++++++++++
 board/freescale/mx6sxsabresd/mx6sxsabresd.c | 63 +++++++++++++++++++++++++++++
 drivers/usb/host/ehci-mx6.c                 | 16 +++++++-
 include/configs/mx6slevk.h                  | 14 +++++++
 include/configs/mx6sxsabresd.h              | 14 +++++++
 6 files changed, 174 insertions(+), 1 deletion(-)

-- 
1.8.4.5

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

[Peng Fan]

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 <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@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);
 }
 
 /* Base address for this IP block is 0x02184800 */
-- 
1.8.4.5

[U-Boot] [PATCH v3 2/3] imx:mx6sxsabresd add board level support for usb

[Peng Fan]

Add pinmux settings, implement board_ehci_hcd_init, board_usb_phy_mode

There are two usb port on mx6sxsabresd 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 <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@freescale.com>
---

Changes v3:
 implement board_usb_phy_mode
Changes v2:
 Add otg polarity setting
 Move pinmux setting into board_init
 set pinmux setting struct static

 board/freescale/mx6sxsabresd/mx6sxsabresd.c | 63 +++++++++++++++++++++++++++++
 include/configs/mx6sxsabresd.h              | 14 +++++++
 2 files changed, 77 insertions(+)

diff --git a/board/freescale/mx6sxsabresd/mx6sxsabresd.c b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
index 256ea29..5fe58f6 100644
--- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c
+++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
@@ -25,6 +25,7 @@
 #include <netdev.h>
 #include <power/pmic.h>
 #include <power/pfuze100_pmic.h>
+#include <usb.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -271,6 +272,64 @@ int board_mmc_init(bd_t *bis)
 	return fsl_esdhc_initialize(bis, &usdhc_cfg[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
+
+static iomux_v3_cfg_t const usb_otg_pads[] = {
+	/* OGT1 */
+	MX6_PAD_GPIO1_IO09__USB_OTG1_PWR | MUX_PAD_CTRL(NO_PAD_CTRL),
+	MX6_PAD_GPIO1_IO10__ANATOP_OTG1_ID | MUX_PAD_CTRL(NO_PAD_CTRL),
+	/* OTG2 */
+	MX6_PAD_GPIO1_IO12__USB_OTG2_PWR | MUX_PAD_CTRL(NO_PAD_CTRL)
+};
+
+static void setup_usb(void)
+{
+	imx_iomux_v3_setup_multiple_pads(usb_otg_pads,
+					 ARRAY_SIZE(usb_otg_pads));
+}
+
+int board_usb_phy_mode(int port)
+{
+	void __iomem *phy_reg;
+	void __iomem *phy_ctrl;
+	u32 val;
+
+	switch (port) {
+	case 0:
+		phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
+		phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+		val = __raw_readl(phy_ctrl);
+		return val & USBPHY_CTRL_OTG_ID;
+	case 1:
+		/* Work in HOST mode. */
+		return 0;
+	}
+
+	/* suppress warning msg */
+	return 0;
+}
+
+int board_ehci_hcd_init(int port)
+{
+	u32 *usbnc_usb_ctrl;
+
+	if (port > 1)
+		return -EINVAL;
+
+	usbnc_usb_ctrl = (u32 *)(USB_BASE_ADDR + USB_OTHERREGS_OFFSET +
+				 port * 4);
+
+	/* Set Power polarity */
+	setbits_le32(usbnc_usb_ctrl, UCTRL_PWR_POL);
+
+	return 0;
+}
+#endif
+
 int board_init(void)
 {
 	/* Address of boot parameters */
@@ -280,6 +339,10 @@ int board_init(void)
 	setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info1);
 #endif
 
+#ifdef CONFIG_USB_EHCI_MX6
+	setup_usb();
+#endif
+
 	return 0;
 }
 
diff --git a/include/configs/mx6sxsabresd.h b/include/configs/mx6sxsabresd.h
index e02ea18..8edf187 100644
--- a/include/configs/mx6sxsabresd.h
+++ b/include/configs/mx6sxsabresd.h
@@ -198,6 +198,20 @@
 #define CONFIG_PHYLIB
 #define CONFIG_PHY_ATHEROS
 
+
+#define CONFIG_CMD_USB
+#ifdef CONFIG_CMD_USB
+#define CONFIG_USB_EHCI
+#define CONFIG_USB_EHCI_MX6
+#define CONFIG_USB_STORAGE
+#define CONFIG_EHCI_HCD_INIT_AFTER_RESET
+#define CONFIG_USB_HOST_ETHER
+#define CONFIG_USB_ETHER_ASIX
+#define CONFIG_MXC_USB_PORTSC  (PORT_PTS_UTMI | PORT_PTS_PTW)
+#define CONFIG_MXC_USB_FLAGS   0
+#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
+#endif
+
 #define CONFIG_CMD_PCI
 #ifdef CONFIG_CMD_PCI
 #define CONFIG_PCI
-- 
1.8.4.5

[U-Boot] [PATCH v3 3/3] imx:mx6slevk add board level support for usb

[Peng Fan]

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 <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@freescale.com>
---

Changes v3:
 implement board_usb_phy_mode
Changes v2:
 Add otg polarity setting
 move pinmux into board_init
 set pinmux setting static

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h |  5 +++
 board/freescale/mx6slevk/mx6slevk.c        | 63 ++++++++++++++++++++++++++++++
 include/configs/mx6slevk.h                 | 14 +++++++
 3 files changed, 82 insertions(+)

diff --git a/arch/arm/include/asm/arch-mx6/mx6sl_pins.h b/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
index 045ccc4..17b4798 100644
--- a/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
+++ b/arch/arm/include/asm/arch-mx6/mx6sl_pins.h
@@ -34,5 +34,10 @@ enum {
 	MX6_PAD_FEC_REF_CLK__FEC_REF_OUT			= IOMUX_PAD(0x424, 0x134, 0x10, 0x000, 0, 0),
 	MX6_PAD_FEC_RX_ER__GPIO_4_19				= IOMUX_PAD(0x0428, 0x0138, 5, 0x0000, 0, 0),
 	MX6_PAD_FEC_TX_CLK__GPIO_4_21				= IOMUX_PAD(0x0434, 0x0144, 5, 0x0000, 0, 0),
+
+	MX6_PAD_EPDC_PWRCOM__ANATOP_USBOTG1_ID			= IOMUX_PAD(0x03D0, 0x00E0, 4, 0x05DC, 0, 0),
+
+	MX6_PAD_KEY_COL4__USB_USBOTG1_PWR			= IOMUX_PAD(0x0484, 0x017C, 6, 0x0000, 0, 0),
+	MX6_PAD_KEY_COL5__USB_USBOTG2_PWR			= IOMUX_PAD(0x0488, 0x0180, 6, 0x0000, 0, 0),
 };
 #endif	/* __ASM_ARCH_MX6_MX6SL_PINS_H__ */
diff --git a/board/freescale/mx6slevk/mx6slevk.c b/board/freescale/mx6slevk/mx6slevk.c
index a500133..e9ec77e 100644
--- a/board/freescale/mx6slevk/mx6slevk.c
+++ b/board/freescale/mx6slevk/mx6slevk.c
@@ -20,6 +20,7 @@
 #include <fsl_esdhc.h>
 #include <mmc.h>
 #include <netdev.h>
+#include <usb.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -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
+
+static iomux_v3_cfg_t const usb_otg_pads[] = {
+	/* OTG1 */
+	MX6_PAD_KEY_COL4__USB_USBOTG1_PWR | MUX_PAD_CTRL(NO_PAD_CTRL),
+	MX6_PAD_EPDC_PWRCOM__ANATOP_USBOTG1_ID | MUX_PAD_CTRL(NO_PAD_CTRL),
+	/* OTG2 */
+	MX6_PAD_KEY_COL5__USB_USBOTG2_PWR | MUX_PAD_CTRL(NO_PAD_CTRL)
+};
+
+static void setup_usb(void)
+{
+	imx_iomux_v3_setup_multiple_pads(usb_otg_pads,
+					 ARRAY_SIZE(usb_otg_pads));
+}
+
+int board_usb_phy_mode(int port)
+{
+	void __iomem *phy_reg;
+	void __iomem *phy_ctrl;
+	u32 val;
+
+	switch (port) {
+	case 0:
+		phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
+		phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+		val = __raw_readl(phy_ctrl);
+		return val & USBPHY_CTRL_OTG_ID;
+	case 1:
+		/* Work in HOST mode. */
+		return 0;
+	}
+
+	/* suppress warning msg */
+	return 0;
+}
+
+int board_ehci_hcd_init(int port)
+{
+	u32 *usbnc_usb_ctrl;
+
+	if (port > 1)
+		return -EINVAL;
+
+	usbnc_usb_ctrl = (u32 *)(USB_BASE_ADDR + USB_OTHERREGS_OFFSET +
+				 port * 4);
+
+	/* Set Power polarity */
+	setbits_le32(usbnc_usb_ctrl, UCTRL_PWR_POL);
+
+	return 0;
+}
+#endif
 
 int board_early_init_f(void)
 {
@@ -168,6 +226,11 @@ int board_init(void)
 #ifdef	CONFIG_FEC_MXC
 	setup_fec();
 #endif
+
+#ifdef CONFIG_USB_EHCI_MX6
+	setup_usb();
+#endif
+
 	return 0;
 }
 
diff --git a/include/configs/mx6slevk.h b/include/configs/mx6slevk.h
index fddedf1..021dc0e 100644
--- a/include/configs/mx6slevk.h
+++ b/include/configs/mx6slevk.h
@@ -210,4 +210,18 @@
 #define CONFIG_SF_DEFAULT_MODE		SPI_MODE_0
 #endif
 
+/* USB Configs */
+#define CONFIG_CMD_USB
+#ifdef CONFIG_CMD_USB
+#define CONFIG_USB_EHCI
+#define CONFIG_USB_EHCI_MX6
+#define CONFIG_USB_STORAGE
+#define CONFIG_EHCI_HCD_INIT_AFTER_RESET
+#define CONFIG_USB_HOST_ETHER
+#define CONFIG_USB_ETHER_ASIX
+#define CONFIG_MXC_USB_PORTSC		(PORT_PTS_UTMI | PORT_PTS_PTW)
+#define CONFIG_MXC_USB_FLAGS		0
+#define CONFIG_USB_MAX_CONTROLLER_COUNT	2
+#endif
+
 #endif				/* __CONFIG_H */
-- 
1.8.4.5

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

[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.Fan@freescale.com>
> Signed-off-by: Ye Li <B37916@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] [PATCH v3 3/3] imx:mx6slevk add board level support for usb

[Marek Vasut]

On Friday, November 07, 2014 at 02:08:14 AM, Peng Fan wrote:
> 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 <Peng.Fan@freescale.com>
> Signed-off-by: Ye Li <B37916@freescale.com>
> ---

[...]

> @@ -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 ?
[...]
Best regards,
Marek Vasut

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

[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.Fan@freescale.com>
>> Signed-off-by: Ye Li <B37916@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] [PATCH v3 3/3] imx:mx6slevk add board level support for usb

[Peng Fan]

? 11/7/2014 4:26 PM, Marek Vasut ??:
> On Friday, November 07, 2014 at 02:08:14 AM, Peng Fan wrote:
>> 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 <Peng.Fan@freescale.com>
>> Signed-off-by: Ye Li <B37916@freescale.com>
>> ---
>
> [...]
>
>> @@ -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 ?
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.
> [...]
> Best regards,
> Marek Vasut
>
Regards,
Peng.

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

[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] [PATCH v3 3/3] imx:mx6slevk add board level support for usb

[Marek Vasut]

On Friday, November 07, 2014 at 12:08:03 PM, Peng Fan wrote:
> ? 11/7/2014 4:26 PM, Marek Vasut ??:
> > On Friday, November 07, 2014 at 02:08:14 AM, Peng Fan wrote:
> >> 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 <Peng.Fan@freescale.com>
> >> Signed-off-by: Ye Li <B37916@freescale.com>
> >> ---
> > 
> > [...]
> > 
> >> @@ -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 ?
> 
> 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

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

[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] [PATCH v3 3/3] imx:mx6slevk add board level support for usb

[Peng Fan]

? 11/7/2014 7:10 PM, Marek Vasut ??:
> On Friday, November 07, 2014 at 12:08:03 PM, Peng Fan wrote:
>> ? 11/7/2014 4:26 PM, Marek Vasut ??:
>>> On Friday, November 07, 2014 at 02:08:14 AM, Peng Fan wrote:
>>>> 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 <Peng.Fan@freescale.com>
>>>> Signed-off-by: Ye Li <B37916@freescale.com>
>>>> ---
>>>
>>> [...]
>>>
>>>> @@ -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 ?
>>
>> 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.
>
struct phy register is good, but I prefer not to include this in board 
level code, see my reply in this patch "usb:ehci-mx6 add 
board_usb_phy_mode function" just as "board_ehci_power" and 
"board_ehci_hcd_init" do. I think it is good to make it a seperate patch.
> Best regards,
> Marek Vasut
>
Regards,
Peng.

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

[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] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[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] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[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] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[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] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[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] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[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