[U-Boot] [PATCH v2 0/3] imx:mx6 add usb support

[U-Boot] [PATCH v2 0/3] imx:mx6 add usb support

[Peng Fan]

Changes v2:
 - move pinmux setting into board_init
 - add otg polarity setting in board_ehci_hcd_init
 - Introduce a new weak function to give board a choice to decide the work mode
   of the otg port, since a otg port can also work in host mode without
   touch ID pin.

Peng Fan (3):
  usb:ehci-mx6 add board_ehci_usb_mode function
  imx:mx6sxsabresd add usb support
  imx:mx6slevk add usb support

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h  |  5 +++
 board/freescale/mx6slevk/mx6slevk.c         | 51 +++++++++++++++++++++++++++++
 board/freescale/mx6sxsabresd/mx6sxsabresd.c | 51 +++++++++++++++++++++++++++++
 drivers/usb/host/ehci-mx6.c                 |  7 ++++
 include/configs/mx6slevk.h                  | 14 ++++++++
 include/configs/mx6sxsabresd.h              | 14 ++++++++
 6 files changed, 142 insertions(+)

-- 
1.8.4.5

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

Include a weak function board_ehci_usb_mode to gives board code
a choice. If the board want the otg port work in host mode but not
device mode, this should be handled.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@freescale.com>
---

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 9ec5a0a..3662a80 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -193,6 +193,11 @@ static void usb_oc_config(int index)
 	__raw_writel(val, ctrl);
 }
 
+int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
+{
+	return 0;
+}
+
 int __weak board_ehci_hcd_init(int port)
 {
 	return 0;
@@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init,
 	usb_internal_phy_clock_gate(index, 1);
 	type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
 
+	board_ehci_usb_mode(index, &type);
+
 	*hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
 	*hcor = (struct ehci_hcor *)((uint32_t)*hccr +
 			HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
-- 
1.8.4.5

[U-Boot] [PATCH v2 2/3] imx:mx6sxsabresd add usb support

[Peng Fan]

Add usb support for mx6sxsabresd board.

There are two usb port on mx6sxsabresd board:
1. OTG
2. OTG
The second port is actually an otg port with a USB A type interface on
the board.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@freescale.com>
---

Changes v2:
 Add otg polarity setting
 Move pinmux setting into board_init

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

diff --git a/board/freescale/mx6sxsabresd/mx6sxsabresd.c b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
index 256ea29..81619bd 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,52 @@ 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 UCTRL_PWR_POL		(1 << 9)
+
+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_ehci_usb_mode(int port, enum usb_init_type *type)
+{
+	if (port != 1)
+		return -EINVAL;
+
+	/* port1 works in HOST Mode */
+	*type = USB_INIT_HOST;
+
+	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 +327,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 v2 3/3] imx:mx6slevk add usb support

[Peng Fan]

Add usb support for mx6slevk board.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Signed-off-by: Ye Li <B37916@freescale.com>
---

Changes v2:
 Add otg polarity setting
 move pinmux into board_init

 arch/arm/include/asm/arch-mx6/mx6sl_pins.h |  5 +++
 board/freescale/mx6slevk/mx6slevk.c        | 51 ++++++++++++++++++++++++++++++
 include/configs/mx6slevk.h                 | 14 ++++++++
 3 files changed, 70 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..360599e 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,51 @@ static int setup_fec(void)
 }
 #endif
 
+#ifdef CONFIG_USB_EHCI_MX6
+#define USB_OTHERREGS_OFFSET	0x800
+#define UCTRL_PWR_POL		(1 << 9)
+
+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_ehci_usb_mode(int port, enum usb_init_type *type)
+{
+	if (port != 1)
+		return -EINVAL;
+
+	/* port1 works in HOST Mode */
+	*type = USB_INIT_HOST;
+
+	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 +214,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 v2 0/3] imx:mx6 add usb support

[Marek Vasut]

On Tuesday, November 04, 2014 at 08:49:59 AM, Peng Fan wrote:

So uh, what exactly does this patchset do ? It's not clear at all from the
subject and any description is completely missing.

To my knowledge, i.MX6 already does have USB support in U-Boot, so the subject
is misleading as well.

> Changes v2:
>  - move pinmux setting into board_init
>  - add otg polarity setting in board_ehci_hcd_init
>  - Introduce a new weak function to give board a choice to decide the work
> mode of the otg port, since a otg port can also work in host mode without
> touch ID pin.
> 
> Peng Fan (3):
>   usb:ehci-mx6 add board_ehci_usb_mode function
>   imx:mx6sxsabresd add usb support
>   imx:mx6slevk add usb support
> 
>  arch/arm/include/asm/arch-mx6/mx6sl_pins.h  |  5 +++
>  board/freescale/mx6slevk/mx6slevk.c         | 51
> +++++++++++++++++++++++++++++ board/freescale/mx6sxsabresd/mx6sxsabresd.c
> | 51 +++++++++++++++++++++++++++++ drivers/usb/host/ehci-mx6.c            
>     |  7 ++++
>  include/configs/mx6slevk.h                  | 14 ++++++++
>  include/configs/mx6sxsabresd.h              | 14 ++++++++
>  6 files changed, 142 insertions(+)

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
> Include a weak function board_ehci_usb_mode to gives board code
> a choice.

What choice?

> If the board want the otg port work in host mode but not
> device mode, this should be handled.

How?

Also, isn't usb_phy_enable() supposed to do exactly this kind of selection
between device and host mode ?

> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Signed-off-by: Ye Li <B37916@freescale.com>
> ---
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 9ec5a0a..3662a80 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -193,6 +193,11 @@ static void usb_oc_config(int index)
>  	__raw_writel(val, ctrl);
>  }
> 
> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
> +{
> +	return 0;
> +}
> +
>  int __weak board_ehci_hcd_init(int port)
>  {
>  	return 0;
> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>  	usb_internal_phy_clock_gate(index, 1);
>  	type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
> 
> +	board_ehci_usb_mode(index, &type);
> +
>  	*hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
>  	*hcor = (struct ehci_hcor *)((uint32_t)*hccr +
>  			HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

? 11/4/2014 6:33 PM, Marek Vasut ??:
> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>> Include a weak function board_ehci_usb_mode to gives board code
>> a choice.
>
> What choice?
>
>> If the board want the otg port work in host mode but not
>> device mode, this should be handled.
>
> How?
>
> Also, isn't usb_phy_enable() supposed to do exactly this kind of selection
> between device and host mode ?

In mx6sxsabresd board, there are two usb port, one used for otg, the 
other used for host. However they are connected to SOC USB controller 
otg1 core and otg2 core respectively. Like following:

OTG1 CORE <----> board otg port
OTG2 CORE <----> board host port

However the board do not have ID pin set for board host port. If just 
use usb_phy_enable, the board 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.

Because i did not find way to handle this situation in 
board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let 
board level code handle handle 'type', if board level code want to set 
it's own 'type'.

>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> Signed-off-by: Ye Li <B37916@freescale.com>
>> ---
>>
>> 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 | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>> index 9ec5a0a..3662a80 100644
>> --- a/drivers/usb/host/ehci-mx6.c
>> +++ b/drivers/usb/host/ehci-mx6.c
>> @@ -193,6 +193,11 @@ static void usb_oc_config(int index)
>>   	__raw_writel(val, ctrl);
>>   }
>>
>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
>> +{
>> +	return 0;
>> +}
>> +
>>   int __weak board_ehci_hcd_init(int port)
>>   {
>>   	return 0;
>> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>>   	usb_internal_phy_clock_gate(index, 1);
>>   	type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
>>
>> +	board_ehci_usb_mode(index, &type);
>> +
>>   	*hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
>>   	*hcor = (struct ehci_hcor *)((uint32_t)*hccr +
>>   			HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
>
> Best regards,
> Marek Vasut
>
Regards,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
> ? 11/4/2014 6:33 PM, Marek Vasut ??:
> > On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
> >> Include a weak function board_ehci_usb_mode to gives board code
> >> a choice.
> > 
> > What choice?
> > 
> >> If the board want the otg port work in host mode but not
> >> device mode, this should be handled.
> > 
> > How?
> > 
> > Also, isn't usb_phy_enable() supposed to do exactly this kind of
> > selection between device and host mode ?
> 
> In mx6sxsabresd board, there are two usb port, one used for otg, the
> other used for host. However they are connected to SOC USB controller
> otg1 core and otg2 core respectively. Like following:
> 
> OTG1 CORE <----> board otg port
> OTG2 CORE <----> board host port
> 
> However the board do not have ID pin set for board host port. If just
> use usb_phy_enable, the board 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.
> 
> Because i did not find way to handle this situation in
> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
> board level code handle handle 'type', if board level code want to set
> it's own 'type'.

This part in usb_phy_enable()

163         return val & USBPHY_CTRL_OTG_ID;

should be replaced by some kind of a board-specific callback then, with
default implmentation being the above (reading the phy ctrl register).

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Jeroen Hofstee]

Hello Peng,

On 04-11-14 08:50, Peng Fan wrote:
> Include a weak function board_ehci_usb_mode to gives board code
> a choice. If the board want the otg port work in host mode but not
> device mode, this should be handled.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Signed-off-by: Ye Li <B37916@freescale.com>
> ---
>
> 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 | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 9ec5a0a..3662a80 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -193,6 +193,11 @@ static void usb_oc_config(int index)
>   	__raw_writel(val, ctrl);
>   }
>   
> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
> +{
> +	return 0;
> +}
> +
>   int __weak board_ehci_hcd_init(int port)
>   {
>   	return 0;
> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>   	usb_internal_phy_clock_gate(index, 1);
>   	type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
>   
> +	board_ehci_usb_mode(index, &type);
> +
>   	*hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
>   	*hcor = (struct ehci_hcor *)((uint32_t)*hccr +
>   			HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));

Can you add a prototype type as well and make sure it is included?

Thanks,
Jeroen

[U-Boot] [PATCH v2 0/3] imx:mx6 add usb support

[Peng Fan]

? 11/4/2014 6:31 PM, Marek Vasut ??:
> On Tuesday, November 04, 2014 at 08:49:59 AM, Peng Fan wrote:
>
> So uh, what exactly does this patchset do ? It's not clear at all from the
> subject and any description is completely missing.
>
> To my knowledge, i.MX6 already does have USB support in U-Boot, so the subject
> is misleading as well.
mx6sxsasbresd board and mx6slevk board does not have board level support 
for usb. This patchset is mainly to add pinmux and implement 
board_ehci_hcd_init and board_echi_power and add USB CONFIG in header 
file to make usb work fine.

I'll send another patch for review, if "usb:ehci-mx6 add 
board_ehci_usb_mode function" is fine.
>
>> Changes v2:
>>   - move pinmux setting into board_init
>>   - add otg polarity setting in board_ehci_hcd_init
>>   - Introduce a new weak function to give board a choice to decide the work
>> mode of the otg port, since a otg port can also work in host mode without
>> touch ID pin.
>>
>> Peng Fan (3):
>>    usb:ehci-mx6 add board_ehci_usb_mode function
>>    imx:mx6sxsabresd add usb support
>>    imx:mx6slevk add usb support
>>
>>   arch/arm/include/asm/arch-mx6/mx6sl_pins.h  |  5 +++
>>   board/freescale/mx6slevk/mx6slevk.c         | 51
>> +++++++++++++++++++++++++++++ board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> | 51 +++++++++++++++++++++++++++++ drivers/usb/host/ehci-mx6.c
>>      |  7 ++++
>>   include/configs/mx6slevk.h                  | 14 ++++++++
>>   include/configs/mx6sxsabresd.h              | 14 ++++++++
>>   6 files changed, 142 insertions(+)
>
> Best regards,
> Marek Vasut
>
Regards,
Peng.

[U-Boot] [PATCH v2 0/3] imx:mx6 add usb support

[Marek Vasut]

On Tuesday, November 04, 2014 at 01:05:44 PM, Peng Fan wrote:
> ? 11/4/2014 6:31 PM, Marek Vasut ??:
> > On Tuesday, November 04, 2014 at 08:49:59 AM, Peng Fan wrote:
> > 
> > So uh, what exactly does this patchset do ? It's not clear at all from
> > the subject and any description is completely missing.
> > 
> > To my knowledge, i.MX6 already does have USB support in U-Boot, so the
> > subject is misleading as well.
> 
> mx6sxsasbresd board and mx6slevk board does not have board level support
> for usb. This patchset is mainly to add pinmux and implement
> board_ehci_hcd_init and board_echi_power and add USB CONFIG in header
> file to make usb work fine.
> 
> I'll send another patch for review, if "usb:ehci-mx6 add
> board_ehci_usb_mode function" is fine.

Please see my comment to that patch before you re-submit.

Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

Hi Marek,

? 11/4/2014 7:01 PM, Marek Vasut ??:
> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>>>> Include a weak function board_ehci_usb_mode to gives board code
>>>> a choice.
>>>
>>> What choice?
>>>
>>>> If the board want the otg port work in host mode but not
>>>> device mode, this should be handled.
>>>
>>> How?
>>>
>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
>>> selection between device and host mode ?
>>
>> In mx6sxsabresd board, there are two usb port, one used for otg, the
>> other used for host. However they are connected to SOC USB controller
>> otg1 core and otg2 core respectively. Like following:
>>
>> OTG1 CORE <----> board otg port
>> OTG2 CORE <----> board host port
>>
>> However the board do not have ID pin set for board host port. If just
>> use usb_phy_enable, the board 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.
>>
>> Because i did not find way to handle this situation in
>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
>> board level code handle handle 'type', if board level code want to set
>> it's own 'type'.
>
> This part in usb_phy_enable()
>
> 163         return val & USBPHY_CTRL_OTG_ID;
>
> should be replaced by some kind of a board-specific callback then, with
> default implmentation being the above (reading the phy ctrl register).
>

How about using the following piece of code?
in ehci-mx6.c

unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
{
	return val & USBPHY_CTRL_OTG_ID;
}

replace "return val & USBPHY_CTRL_OTG_ID;" using "
return board_usb_phy_mode(index, val);"

In board file,
unsigned int board_usb_phy_mode(int index, unsigned int val)
{
	if (index == 1)
		return 0; /* HOST */
	else
		return 1; /* DEVICE */
}

> Best regards,
> Marek Vasut
>
Regards,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

Hi Jeroen,

? 11/4/2014 7:40 PM, Jeroen Hofstee ??:
> Hello Peng,
>
> On 04-11-14 08:50, Peng Fan wrote:
>> Include a weak function board_ehci_usb_mode to gives board code
>> a choice. If the board want the otg port work in host mode but not
>> device mode, this should be handled.
>>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> Signed-off-by: Ye Li <B37916@freescale.com>
>> ---
>>
>> 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 | 7 +++++++
>>    1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>> index 9ec5a0a..3662a80 100644
>> --- a/drivers/usb/host/ehci-mx6.c
>> +++ b/drivers/usb/host/ehci-mx6.c
>> @@ -193,6 +193,11 @@ static void usb_oc_config(int index)
>>    	__raw_writel(val, ctrl);
>>    }
>>
>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
>> +{
>> +	return 0;
>> +}
>> +
>>    int __weak board_ehci_hcd_init(int port)
>>    {
>>    	return 0;
>> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>>    	usb_internal_phy_clock_gate(index, 1);
>>    	type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
>>
>> +	board_ehci_usb_mode(index, &type);
>> +
>>    	*hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
>>    	*hcor = (struct ehci_hcor *)((uint32_t)*hccr +
>>    			HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
>
> Can you add a prototype type as well and make sure it is included?
I did not find a good place for the prototype type. I think ehci.h is 
not fine to include this prototype. Any suggestions?
>
> Thanks,
> Jeroen
>
Regards,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
> Hi Marek,
> 
> ? 11/4/2014 7:01 PM, Marek Vasut ??:
> > On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
> >> ? 11/4/2014 6:33 PM, Marek Vasut ??:
> >>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
> >>>> Include a weak function board_ehci_usb_mode to gives board code
> >>>> a choice.
> >>> 
> >>> What choice?
> >>> 
> >>>> If the board want the otg port work in host mode but not
> >>>> device mode, this should be handled.
> >>> 
> >>> How?
> >>> 
> >>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
> >>> selection between device and host mode ?
> >> 
> >> In mx6sxsabresd board, there are two usb port, one used for otg, the
> >> other used for host. However they are connected to SOC USB controller
> >> otg1 core and otg2 core respectively. Like following:
> >> 
> >> OTG1 CORE <----> board otg port
> >> OTG2 CORE <----> board host port
> >> 
> >> However the board do not have ID pin set for board host port. If just
> >> use usb_phy_enable, the board 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.
> >> 
> >> Because i did not find way to handle this situation in
> >> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
> >> board level code handle handle 'type', if board level code want to set
> >> it's own 'type'.
> > 
> > This part in usb_phy_enable()
> > 
> > 163         return val & USBPHY_CTRL_OTG_ID;
> > 
> > should be replaced by some kind of a board-specific callback then, with
> > default implmentation being the above (reading the phy ctrl register).
> 
> How about using the following piece of code?
> in ehci-mx6.c
> 
> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
> {
> 	return val & USBPHY_CTRL_OTG_ID;
> }
> 
> replace "return val & USBPHY_CTRL_OTG_ID;" using "
> return board_usb_phy_mode(index, val);"
> 
> In board file,
> unsigned int board_usb_phy_mode(int index, unsigned int val)

Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val into
the function doesn't seem like a scalable future-proof solution.
[...]
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Jeroen Hofstee]

Hello Peng,

On 04-11-14 14:36, Peng Fan wrote:
>
> Hi Jeroen,
>
> ? 11/4/2014 7:40 PM, Jeroen Hofstee ??:
>> Hello Peng,
>>
>> On 04-11-14 08:50, Peng Fan wrote:
>>> Include a weak function board_ehci_usb_mode to gives board code
>>> a choice. If the board want the otg port work in host mode but not
>>> device mode, this should be handled.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> Signed-off-by: Ye Li <B37916@freescale.com>
>>> ---
>>>
>>> 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 | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>>> index 9ec5a0a..3662a80 100644
>>> --- a/drivers/usb/host/ehci-mx6.c
>>> +++ b/drivers/usb/host/ehci-mx6.c
>>> @@ -193,6 +193,11 @@ static void usb_oc_config(int index)
>>>        __raw_writel(val, ctrl);
>>>    }
>>>
>>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>    int __weak board_ehci_hcd_init(int port)
>>>    {
>>>        return 0;
>>> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type 
>>> init,
>>>        usb_internal_phy_clock_gate(index, 1);
>>>        type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : 
>>> USB_INIT_HOST;
>>>
>>> +    board_ehci_usb_mode(index, &type);
>>> +
>>>        *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
>>>        *hcor = (struct ehci_hcor *)((uint32_t)*hccr +
>>> HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
>>
>> Can you add a prototype type as well and make sure it is included?
> I did not find a good place for the prototype type. I think ehci.h is 
> not fine to include this prototype. Any suggestions?

Ah, good point. I have reserved USB as well for the "not trivial" to
fix warnings. Ideally there should be a header in drivers somewhere
defining what different usb drivers can/should support when dealing with
common usb code and one in include/... for the board interface.

Since this is currently lacking, I am also fine checking it in without a 
valid
prototype and fix it later.

Regards,
Jeroen

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

? 11/5/2014 1:33 AM, Marek Vasut ??:
> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
>> Hi Marek,
>>
>> ? 11/4/2014 7:01 PM, Marek Vasut ??:
>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
>>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>>>>>> Include a weak function board_ehci_usb_mode to gives board code
>>>>>> a choice.
>>>>>
>>>>> What choice?
>>>>>
>>>>>> If the board want the otg port work in host mode but not
>>>>>> device mode, this should be handled.
>>>>>
>>>>> How?
>>>>>
>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
>>>>> selection between device and host mode ?
>>>>
>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
>>>> other used for host. However they are connected to SOC USB controller
>>>> otg1 core and otg2 core respectively. Like following:
>>>>
>>>> OTG1 CORE <----> board otg port
>>>> OTG2 CORE <----> board host port
>>>>
>>>> However the board do not have ID pin set for board host port. If just
>>>> use usb_phy_enable, the board 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.
>>>>
>>>> Because i did not find way to handle this situation in
>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
>>>> board level code handle handle 'type', if board level code want to set
>>>> it's own 'type'.
>>>
>>> This part in usb_phy_enable()
>>>
>>> 163         return val & USBPHY_CTRL_OTG_ID;
>>>
>>> should be replaced by some kind of a board-specific callback then, with
>>> default implmentation being the above (reading the phy ctrl register).
>>
>> How about using the following piece of code?
>> in ehci-mx6.c
>>
>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
>> {
>> 	return val & USBPHY_CTRL_OTG_ID;
>> }
>>
>> replace "return val & USBPHY_CTRL_OTG_ID;" using "
>> return board_usb_phy_mode(index, val);"
>>
>> In board file,
>> unsigned int board_usb_phy_mode(int index, unsigned int val)
>
> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val into
> the function doesn't seem like a scalable future-proof solution.
> [...]

Passing struct usb_ehci * to board code seems exports ehci register 
definition to board layer. How about just use
"int board_usb_phy_mode(int index)" without using 'val' or 'struct 
usb_ehci *ehci'.

> Best regards,
> Marek Vasut
>
Regards,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote:
> ? 11/5/2014 1:33 AM, Marek Vasut ??:
> > On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
> >> Hi Marek,
> >> 
> >> ? 11/4/2014 7:01 PM, Marek Vasut ??:
> >>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
> >>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
> >>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
> >>>>>> Include a weak function board_ehci_usb_mode to gives board code
> >>>>>> a choice.
> >>>>> 
> >>>>> What choice?
> >>>>> 
> >>>>>> If the board want the otg port work in host mode but not
> >>>>>> device mode, this should be handled.
> >>>>> 
> >>>>> How?
> >>>>> 
> >>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
> >>>>> selection between device and host mode ?
> >>>> 
> >>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
> >>>> other used for host. However they are connected to SOC USB controller
> >>>> otg1 core and otg2 core respectively. Like following:
> >>>> 
> >>>> OTG1 CORE <----> board otg port
> >>>> OTG2 CORE <----> board host port
> >>>> 
> >>>> However the board do not have ID pin set for board host port. If just
> >>>> use usb_phy_enable, the board 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.
> >>>> 
> >>>> Because i did not find way to handle this situation in
> >>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
> >>>> board level code handle handle 'type', if board level code want to set
> >>>> it's own 'type'.
> >>> 
> >>> This part in usb_phy_enable()
> >>> 
> >>> 163         return val & USBPHY_CTRL_OTG_ID;
> >>> 
> >>> should be replaced by some kind of a board-specific callback then, with
> >>> default implmentation being the above (reading the phy ctrl register).
> >> 
> >> How about using the following piece of code?
> >> in ehci-mx6.c
> >> 
> >> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
> >> {
> >> 
> >> 	return val & USBPHY_CTRL_OTG_ID;
> >> 
> >> }
> >> 
> >> replace "return val & USBPHY_CTRL_OTG_ID;" using "
> >> return board_usb_phy_mode(index, val);"
> >> 
> >> In board file,
> >> unsigned int board_usb_phy_mode(int index, unsigned int val)
> > 
> > Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val
> > into the function doesn't seem like a scalable future-proof solution.
> > [...]
> 
> Passing struct usb_ehci * to board code seems exports ehci register
> definition to board layer.

Yeah.

> How about just use
> "int board_usb_phy_mode(int index)" without using 'val' or 'struct
> usb_ehci *ehci'.

The board part might need to read the EHCI registers though. How would the
board part be able to do it if you didn't pass the *ehci in ?

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

? 11/5/2014 5:03 PM, Marek Vasut ??:
> On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote:
>> ? 11/5/2014 1:33 AM, Marek Vasut ??:
>>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
>>>> Hi Marek,
>>>>
>>>> ? 11/4/2014 7:01 PM, Marek Vasut ??:
>>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
>>>>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
>>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>>>>>>>> Include a weak function board_ehci_usb_mode to gives board code
>>>>>>>> a choice.
>>>>>>>
>>>>>>> What choice?
>>>>>>>
>>>>>>>> If the board want the otg port work in host mode but not
>>>>>>>> device mode, this should be handled.
>>>>>>>
>>>>>>> How?
>>>>>>>
>>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
>>>>>>> selection between device and host mode ?
>>>>>>
>>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
>>>>>> other used for host. However they are connected to SOC USB controller
>>>>>> otg1 core and otg2 core respectively. Like following:
>>>>>>
>>>>>> OTG1 CORE <----> board otg port
>>>>>> OTG2 CORE <----> board host port
>>>>>>
>>>>>> However the board do not have ID pin set for board host port. If just
>>>>>> use usb_phy_enable, the board 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.
>>>>>>
>>>>>> Because i did not find way to handle this situation in
>>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let
>>>>>> board level code handle handle 'type', if board level code want to set
>>>>>> it's own 'type'.
>>>>>
>>>>> This part in usb_phy_enable()
>>>>>
>>>>> 163         return val & USBPHY_CTRL_OTG_ID;
>>>>>
>>>>> should be replaced by some kind of a board-specific callback then, with
>>>>> default implmentation being the above (reading the phy ctrl register).
>>>>
>>>> How about using the following piece of code?
>>>> in ehci-mx6.c
>>>>
>>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
>>>> {
>>>>
>>>> 	return val & USBPHY_CTRL_OTG_ID;
>>>>
>>>> }
>>>>
>>>> replace "return val & USBPHY_CTRL_OTG_ID;" using "
>>>> return board_usb_phy_mode(index, val);"
>>>>
>>>> In board file,
>>>> unsigned int board_usb_phy_mode(int index, unsigned int val)
>>>
>>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val
>>> into the function doesn't seem like a scalable future-proof solution.
>>> [...]
>>
>> Passing struct usb_ehci * to board code seems exports ehci register
>> definition to board layer.
>
> Yeah.
>
>> How about just use
>> "int board_usb_phy_mode(int index)" without using 'val' or 'struct
>> usb_ehci *ehci'.
>
> The board part might need to read the EHCI registers though. How would the
> board part be able to do it if you didn't pass the *ehci in ?
To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy 
regs definition are not included in "struct usb_ehci". I just think 
expose the ehci register to board layer is not fine and 
board_usb_phy_mode does not need this. I define this just as 
"board_ehci_hcd_init" and "board_ehci_power". Their prototype are
int __weak board_ehci_hcd_init(int port);
int __weak board_ehci_power(int port, int on);

My implementation is the following:

replace "return val & USBPHY_CTRL_OTG_ID;" using "return 
board_usb_phy_mode(index);" in usb_phy_enable

In drivers/usb/host/ehci-mx6.c:
116 int __weak board_usb_phy_mode(int index)
117 {
118         void __iomem *phy_reg;
119         void __iomem *phy_ctrl;
120         u32 val;
121
122         phy_reg = (void __iomem *)phy_bases[index];
123         phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
124
125         val = __raw_readl(phy_ctrl);
126
127         return val & USBPHY_CTRL_OTG_ID;
128 }

In board/freescale/mx6sxsabresd/mx6sxsabresd.c:
295 int board_usb_phy_mode(int port)
296 {
297         void __iomem *phy_reg;
298         void __iomem *phy_ctrl;
299         u32 val;
300
301         switch (port) {
302         case 0:
303                 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
304                 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
305                 val = __raw_readl(phy_ctrl);
306                 return val & USBPHY_CTRL_OTG_ID;
307         case 1:
308                 /* Work in HOST mode. */
309                 return 0;
310         }
311
312         /* suppress warning msg */
313         return 0;
314 }

Is this piece of code fine?
>
Regards,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Wednesday, November 05, 2014 at 10:18:25 AM, Peng Fan wrote:
> ? 11/5/2014 5:03 PM, Marek Vasut ??:
> > On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote:
> >> ? 11/5/2014 1:33 AM, Marek Vasut ??:
> >>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
> >>>> Hi Marek,
> >>>> 
> >>>> ? 11/4/2014 7:01 PM, Marek Vasut ??:
> >>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
> >>>>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
> >>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
> >>>>>>>> Include a weak function board_ehci_usb_mode to gives board code
> >>>>>>>> a choice.
> >>>>>>> 
> >>>>>>> What choice?
> >>>>>>> 
> >>>>>>>> If the board want the otg port work in host mode but not
> >>>>>>>> device mode, this should be handled.
> >>>>>>> 
> >>>>>>> How?
> >>>>>>> 
> >>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
> >>>>>>> selection between device and host mode ?
> >>>>>> 
> >>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
> >>>>>> other used for host. However they are connected to SOC USB
> >>>>>> controller otg1 core and otg2 core respectively. Like following:
> >>>>>> 
> >>>>>> OTG1 CORE <----> board otg port
> >>>>>> OTG2 CORE <----> board host port
> >>>>>> 
> >>>>>> However the board do not have ID pin set for board host port. If
> >>>>>> just use usb_phy_enable, the board 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.
> >>>>>> 
> >>>>>> Because i did not find way to handle this situation in
> >>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to
> >>>>>> let board level code handle handle 'type', if board level code want
> >>>>>> to set it's own 'type'.
> >>>>> 
> >>>>> This part in usb_phy_enable()
> >>>>> 
> >>>>> 163         return val & USBPHY_CTRL_OTG_ID;
> >>>>> 
> >>>>> should be replaced by some kind of a board-specific callback then,
> >>>>> with default implmentation being the above (reading the phy ctrl
> >>>>> register).
> >>>> 
> >>>> How about using the following piece of code?
> >>>> in ehci-mx6.c
> >>>> 
> >>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
> >>>> {
> >>>> 
> >>>> 	return val & USBPHY_CTRL_OTG_ID;
> >>>> 
> >>>> }
> >>>> 
> >>>> replace "return val & USBPHY_CTRL_OTG_ID;" using "
> >>>> return board_usb_phy_mode(index, val);"
> >>>> 
> >>>> In board file,
> >>>> unsigned int board_usb_phy_mode(int index, unsigned int val)
> >>> 
> >>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc
> >>> $val into the function doesn't seem like a scalable future-proof
> >>> solution. [...]
> >> 
> >> Passing struct usb_ehci * to board code seems exports ehci register
> >> definition to board layer.
> > 
> > Yeah.
> > 
> >> How about just use
> >> "int board_usb_phy_mode(int index)" without using 'val' or 'struct
> >> usb_ehci *ehci'.
> > 
> > The board part might need to read the EHCI registers though. How would
> > the board part be able to do it if you didn't pass the *ehci in ?
> 
> To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy
> regs definition are not included in "struct usb_ehci". I just think
> expose the ehci register to board layer is not fine and
> board_usb_phy_mode does not need this. I define this just as
> "board_ehci_hcd_init" and "board_ehci_power". Their prototype are
> int __weak board_ehci_hcd_init(int port);
> int __weak board_ehci_power(int port, int on);
> 
> My implementation is the following:
> 
> replace "return val & USBPHY_CTRL_OTG_ID;" using "return
> board_usb_phy_mode(index);" in usb_phy_enable
> 
> In drivers/usb/host/ehci-mx6.c:
> 116 int __weak board_usb_phy_mode(int index)
> 117 {
> 118         void __iomem *phy_reg;
> 119         void __iomem *phy_ctrl;
> 120         u32 val;
> 121
> 122         phy_reg = (void __iomem *)phy_bases[index];
> 123         phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
> 124
> 125         val = __raw_readl(phy_ctrl);
> 126
> 127         return val & USBPHY_CTRL_OTG_ID;
> 128 }
> 
> In board/freescale/mx6sxsabresd/mx6sxsabresd.c:
> 295 int board_usb_phy_mode(int port)
> 296 {
> 297         void __iomem *phy_reg;
> 298         void __iomem *phy_ctrl;
> 299         u32 val;
> 300
> 301         switch (port) {
> 302         case 0:
> 303                 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
> 304                 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
> 305                 val = __raw_readl(phy_ctrl);
> 306                 return val & USBPHY_CTRL_OTG_ID;
> 307         case 1:
> 308                 /* Work in HOST mode. */
> 309                 return 0;
> 310         }
> 311
> 312         /* suppress warning msg */
> 313         return 0;
> 314 }
> 
> Is this piece of code fine?

These ad-hoc hooks are starting to become absolute horror, but I guess
this one (if properly documented) might just work. Let's see what will
come out of this approach.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan]

? 11/7/2014 4:20 AM, Marek Vasut ??:
> On Wednesday, November 05, 2014 at 10:18:25 AM, Peng Fan wrote:
>> ? 11/5/2014 5:03 PM, Marek Vasut ??:
>>> On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote:
>>>> ? 11/5/2014 1:33 AM, Marek Vasut ??:
>>>>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> ? 11/4/2014 7:01 PM, Marek Vasut ??:
>>>>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
>>>>>>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
>>>>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>>>>>>>>>> Include a weak function board_ehci_usb_mode to gives board code
>>>>>>>>>> a choice.
>>>>>>>>>
>>>>>>>>> What choice?
>>>>>>>>>
>>>>>>>>>> If the board want the otg port work in host mode but not
>>>>>>>>>> device mode, this should be handled.
>>>>>>>>>
>>>>>>>>> How?
>>>>>>>>>
>>>>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
>>>>>>>>> selection between device and host mode ?
>>>>>>>>
>>>>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
>>>>>>>> other used for host. However they are connected to SOC USB
>>>>>>>> controller otg1 core and otg2 core respectively. Like following:
>>>>>>>>
>>>>>>>> OTG1 CORE <----> board otg port
>>>>>>>> OTG2 CORE <----> board host port
>>>>>>>>
>>>>>>>> However the board do not have ID pin set for board host port. If
>>>>>>>> just use usb_phy_enable, the board 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.
>>>>>>>>
>>>>>>>> Because i did not find way to handle this situation in
>>>>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to
>>>>>>>> let board level code handle handle 'type', if board level code want
>>>>>>>> to set it's own 'type'.
>>>>>>>
>>>>>>> This part in usb_phy_enable()
>>>>>>>
>>>>>>> 163         return val & USBPHY_CTRL_OTG_ID;
>>>>>>>
>>>>>>> should be replaced by some kind of a board-specific callback then,
>>>>>>> with default implmentation being the above (reading the phy ctrl
>>>>>>> register).
>>>>>>
>>>>>> How about using the following piece of code?
>>>>>> in ehci-mx6.c
>>>>>>
>>>>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
>>>>>> {
>>>>>>
>>>>>> 	return val & USBPHY_CTRL_OTG_ID;
>>>>>>
>>>>>> }
>>>>>>
>>>>>> replace "return val & USBPHY_CTRL_OTG_ID;" using "
>>>>>> return board_usb_phy_mode(index, val);"
>>>>>>
>>>>>> In board file,
>>>>>> unsigned int board_usb_phy_mode(int index, unsigned int val)
>>>>>
>>>>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc
>>>>> $val into the function doesn't seem like a scalable future-proof
>>>>> solution. [...]
>>>>
>>>> Passing struct usb_ehci * to board code seems exports ehci register
>>>> definition to board layer.
>>>
>>> Yeah.
>>>
>>>> How about just use
>>>> "int board_usb_phy_mode(int index)" without using 'val' or 'struct
>>>> usb_ehci *ehci'.
>>>
>>> The board part might need to read the EHCI registers though. How would
>>> the board part be able to do it if you didn't pass the *ehci in ?
>>
>> To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy
>> regs definition are not included in "struct usb_ehci". I just think
>> expose the ehci register to board layer is not fine and
>> board_usb_phy_mode does not need this. I define this just as
>> "board_ehci_hcd_init" and "board_ehci_power". Their prototype are
>> int __weak board_ehci_hcd_init(int port);
>> int __weak board_ehci_power(int port, int on);
>>
>> My implementation is the following:
>>
>> replace "return val & USBPHY_CTRL_OTG_ID;" using "return
>> board_usb_phy_mode(index);" in usb_phy_enable
>>
>> In drivers/usb/host/ehci-mx6.c:
>> 116 int __weak board_usb_phy_mode(int index)
>> 117 {
>> 118         void __iomem *phy_reg;
>> 119         void __iomem *phy_ctrl;
>> 120         u32 val;
>> 121
>> 122         phy_reg = (void __iomem *)phy_bases[index];
>> 123         phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
>> 124
>> 125         val = __raw_readl(phy_ctrl);
>> 126
>> 127         return val & USBPHY_CTRL_OTG_ID;
>> 128 }
>>
>> In board/freescale/mx6sxsabresd/mx6sxsabresd.c:
>> 295 int board_usb_phy_mode(int port)
>> 296 {
>> 297         void __iomem *phy_reg;
>> 298         void __iomem *phy_ctrl;
>> 299         u32 val;
>> 300
>> 301         switch (port) {
>> 302         case 0:
>> 303                 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
>> 304                 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
>> 305                 val = __raw_readl(phy_ctrl);
>> 306                 return val & USBPHY_CTRL_OTG_ID;
>> 307         case 1:
>> 308                 /* Work in HOST mode. */
>> 309                 return 0;
>> 310         }
>> 311
>> 312         /* suppress warning msg */
>> 313         return 0;
>> 314 }
>>
>> Is this piece of code fine?
>
> These ad-hoc hooks are starting to become absolute horror, but I guess
> this one (if properly documented) might just work. Let's see what will
> come out of this approach.
>
Sent out v3 patch set just now. Please review.

Thanks,
Peng.

[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Marek Vasut]

On Friday, November 07, 2014 at 02:10:28 AM, Peng Fan wrote:
[...]
> >> In board/freescale/mx6sxsabresd/mx6sxsabresd.c:
> >> 295 int board_usb_phy_mode(int port)
> >> 296 {
> >> 297         void __iomem *phy_reg;
> >> 298         void __iomem *phy_ctrl;
> >> 299         u32 val;
> >> 300
> >> 301         switch (port) {
> >> 302         case 0:
> >> 303                 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
> >> 304                 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
> >> 305                 val = __raw_readl(phy_ctrl);
> >> 306                 return val & USBPHY_CTRL_OTG_ID;
> >> 307         case 1:
> >> 308                 /* Work in HOST mode. */
> >> 309                 return 0;
> >> 310         }
> >> 311
> >> 312         /* suppress warning msg */
> >> 313         return 0;
> >> 314 }
> >> 
> >> Is this piece of code fine?
> > 
> > These ad-hoc hooks are starting to become absolute horror, but I guess
> > this one (if properly documented) might just work. Let's see what will
> > come out of this approach.
> 
> Sent out v3 patch set just now. Please review.

Done, there're only minor comments.

Thanks!

Best regards,
Marek Vasut