Patchwork [U-Boot,v2,07/10] usb: mxs: Disable USB Port 1 for i.MX23

login
register
mail settings
Submitter Otavio Salvador
Date Feb. 11, 2013, 1:35 p.m.
Message ID <1360589756-26355-8-git-send-email-otavio@ossystems.com.br>
Download mbox | patch
Permalink /patch/219605/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Otavio Salvador - Feb. 11, 2013, 1:35 p.m.
The i.MX23 just one USB port so disable the second controller probe
when building for i.MX23.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
Changes in v2:
- Avoid wrong clock setting in MX23

 drivers/usb/host/ehci-mxs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Marek Vasut - Feb. 13, 2013, 9:15 p.m.
Dear Otavio Salvador,

> The i.MX23 just one USB port so disable the second controller probe
> when building for i.MX23.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> Changes in v2:
> - Avoid wrong clock setting in MX23
> 
>  drivers/usb/host/ehci-mxs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
> index 5062af5..b7bf856 100644
> --- a/drivers/usb/host/ehci-mxs.c
> +++ b/drivers/usb/host/ehci-mxs.c
> @@ -50,10 +50,12 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
> port) usb_base = MXS_USBCTRL0_BASE;
>  		phy_base = MXS_USBPHY0_BASE;
>  		break;
> +#ifdef CONFIG_MX28
>  	case 1:
>  		usb_base = MXS_USBCTRL1_BASE;
>  		phy_base = MXS_USBPHY1_BASE;
>  		break;
> +#endif
>  	default:
>  		printf("CONFIG_EHCI_MXS_PORT (port = %d)\n", port);
>  		return -1;
> @@ -67,7 +69,9 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int port)
>  /* This DIGCTL register ungates clock to USB */
>  #define	HW_DIGCTL_CTRL			0x8001c000
>  #define	HW_DIGCTL_CTRL_USB0_CLKGATE	(1 << 2)
> +#ifdef CONFIG_MX28
>  #define	HW_DIGCTL_CTRL_USB1_CLKGATE	(1 << 16)
> +#endif
> 
>  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor
> **hcor) {
> @@ -95,8 +99,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,
> struct ehci_hcor **hcor) writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS |
> CLKCTRL_PLL1CTRL0_POWER,
>  			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
> 
> +#if defined(CONFIG_MX23)
> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE,	&digctl_ctrl->reg_clr);

What is that large space before '&digctl_regs->...' ?

> +#elif defined(CONFIG_MX28)
>  	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
>  		&digctl_ctrl->reg_clr);
> +#endif

I'm sure these can be wrapped in much more elegant way, yes?

> 
>  	/* Start USB PHY */
>  	writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
> @@ -153,8 +161,12 @@ int ehci_hcd_stop(int index)
>  			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
> 
>  	/* Gate off the USB clock */
> +#if defined(CONFIG_MX23)
> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE,	&digctl_ctrl->reg_set);
> +#elif defined(CONFIG_MX28)
>  	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
>  		&digctl_ctrl->reg_set);
> +#endif
> 
>  	return 0;
>  }

Best regards,
Marek Vasut
Otavio Salvador - Feb. 13, 2013, 9:24 p.m.
On Wed, Feb 13, 2013 at 7:15 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Otavio Salvador,
>
>> The i.MX23 just one USB port so disable the second controller probe
>> when building for i.MX23.
>>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>> Changes in v2:
>> - Avoid wrong clock setting in MX23
>>
>>  drivers/usb/host/ehci-mxs.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
>> index 5062af5..b7bf856 100644
>> --- a/drivers/usb/host/ehci-mxs.c
>> +++ b/drivers/usb/host/ehci-mxs.c
>> @@ -50,10 +50,12 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
>> port) usb_base = MXS_USBCTRL0_BASE;
>>               phy_base = MXS_USBPHY0_BASE;
>>               break;
>> +#ifdef CONFIG_MX28
>>       case 1:
>>               usb_base = MXS_USBCTRL1_BASE;
>>               phy_base = MXS_USBPHY1_BASE;
>>               break;
>> +#endif
>>       default:
>>               printf("CONFIG_EHCI_MXS_PORT (port = %d)\n", port);
>>               return -1;
>> @@ -67,7 +69,9 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int port)
>>  /* This DIGCTL register ungates clock to USB */
>>  #define      HW_DIGCTL_CTRL                  0x8001c000
>>  #define      HW_DIGCTL_CTRL_USB0_CLKGATE     (1 << 2)
>> +#ifdef CONFIG_MX28
>>  #define      HW_DIGCTL_CTRL_USB1_CLKGATE     (1 << 16)
>> +#endif
>>
>>  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor
>> **hcor) {
>> @@ -95,8 +99,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,
>> struct ehci_hcor **hcor) writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS |
>> CLKCTRL_PLL1CTRL0_POWER,
>>                       &clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
>>
>> +#if defined(CONFIG_MX23)
>> +     writel(HW_DIGCTL_CTRL_USB0_CLKGATE,     &digctl_ctrl->reg_clr);
>
> What is that large space before '&digctl_regs->...' ?

My fault. I can fix it.

>> +#elif defined(CONFIG_MX28)
>>       writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
>>               &digctl_ctrl->reg_clr);
>> +#endif
>
> I'm sure these can be wrapped in much more elegant way, yes?

I wanted to keep the changes at minimum; so seems the right route.
What you'd like me to do? Use a temporary variable?

>>       /* Start USB PHY */
>>       writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
>> @@ -153,8 +161,12 @@ int ehci_hcd_stop(int index)
>>                       &clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
>>
>>       /* Gate off the USB clock */
>> +#if defined(CONFIG_MX23)
>> +     writel(HW_DIGCTL_CTRL_USB0_CLKGATE,     &digctl_ctrl->reg_set);
>> +#elif defined(CONFIG_MX28)
>>       writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
>>               &digctl_ctrl->reg_set);
>> +#endif
>>
>>       return 0;
>>  }
>
> Best regards,
> Marek Vasut
Marek Vasut - Feb. 13, 2013, 9:36 p.m.
Dear Otavio Salvador,

> On Wed, Feb 13, 2013 at 7:15 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Otavio Salvador,
> > 
> >> The i.MX23 just one USB port so disable the second controller probe
> >> when building for i.MX23.
> >> 
> >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> >> ---
> >> Changes in v2:
> >> - Avoid wrong clock setting in MX23
> >> 
> >>  drivers/usb/host/ehci-mxs.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
> >> index 5062af5..b7bf856 100644
> >> --- a/drivers/usb/host/ehci-mxs.c
> >> +++ b/drivers/usb/host/ehci-mxs.c
> >> @@ -50,10 +50,12 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
> >> port) usb_base = MXS_USBCTRL0_BASE;
> >> 
> >>               phy_base = MXS_USBPHY0_BASE;
> >>               break;
> >> 
> >> +#ifdef CONFIG_MX28
> >> 
> >>       case 1:
> >>               usb_base = MXS_USBCTRL1_BASE;
> >>               phy_base = MXS_USBPHY1_BASE;
> >>               break;
> >> 
> >> +#endif
> >> 
> >>       default:
> >>               printf("CONFIG_EHCI_MXS_PORT (port = %d)\n", port);
> >>               return -1;
> >> 
> >> @@ -67,7 +69,9 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
> >> port)
> >> 
> >>  /* This DIGCTL register ungates clock to USB */
> >>  #define      HW_DIGCTL_CTRL                  0x8001c000
> >>  #define      HW_DIGCTL_CTRL_USB0_CLKGATE     (1 << 2)
> >> 
> >> +#ifdef CONFIG_MX28
> >> 
> >>  #define      HW_DIGCTL_CTRL_USB1_CLKGATE     (1 << 16)
> >> 
> >> +#endif
> >> 
> >>  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor
> >> 
> >> **hcor) {
> >> @@ -95,8 +99,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,
> >> struct ehci_hcor **hcor) writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS |
> >> CLKCTRL_PLL1CTRL0_POWER,
> >> 
> >>                       &clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
> >> 
> >> +#if defined(CONFIG_MX23)
> >> +     writel(HW_DIGCTL_CTRL_USB0_CLKGATE,     &digctl_ctrl->reg_clr);
> > 
> > What is that large space before '&digctl_regs->...' ?
> 
> My fault. I can fix it.
> 
> >> +#elif defined(CONFIG_MX28)
> >> 
> >>       writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
> >>       
> >>               &digctl_ctrl->reg_clr);
> >> 
> >> +#endif
> > 
> > I'm sure these can be wrapped in much more elegant way, yes?
> 
> I wanted to keep the changes at minimum; so seems the right route.
> What you'd like me to do? Use a temporary variable?

Either that or wrap it all in struct ehci_mxs and do these ungatings etc on a 
per-port basis.
Otavio Salvador - Feb. 16, 2013, 9:59 p.m.
On Wed, Feb 13, 2013 at 7:36 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Otavio Salvador,
>
>> On Wed, Feb 13, 2013 at 7:15 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Otavio Salvador,
>> >
>> >> The i.MX23 just one USB port so disable the second controller probe
>> >> when building for i.MX23.
>> >>
>> >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> >> ---
>> >> Changes in v2:
>> >> - Avoid wrong clock setting in MX23
>> >>
>> >>  drivers/usb/host/ehci-mxs.c | 12 ++++++++++++
>> >>  1 file changed, 12 insertions(+)
>> >>
>> >> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
>> >> index 5062af5..b7bf856 100644
>> >> --- a/drivers/usb/host/ehci-mxs.c
>> >> +++ b/drivers/usb/host/ehci-mxs.c
>> >> @@ -50,10 +50,12 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
>> >> port) usb_base = MXS_USBCTRL0_BASE;
>> >>
>> >>               phy_base = MXS_USBPHY0_BASE;
>> >>               break;
>> >>
>> >> +#ifdef CONFIG_MX28
>> >>
>> >>       case 1:
>> >>               usb_base = MXS_USBCTRL1_BASE;
>> >>               phy_base = MXS_USBPHY1_BASE;
>> >>               break;
>> >>
>> >> +#endif
>> >>
>> >>       default:
>> >>               printf("CONFIG_EHCI_MXS_PORT (port = %d)\n", port);
>> >>               return -1;
>> >>
>> >> @@ -67,7 +69,9 @@ int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int
>> >> port)
>> >>
>> >>  /* This DIGCTL register ungates clock to USB */
>> >>  #define      HW_DIGCTL_CTRL                  0x8001c000
>> >>  #define      HW_DIGCTL_CTRL_USB0_CLKGATE     (1 << 2)
>> >>
>> >> +#ifdef CONFIG_MX28
>> >>
>> >>  #define      HW_DIGCTL_CTRL_USB1_CLKGATE     (1 << 16)
>> >>
>> >> +#endif
>> >>
>> >>  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor
>> >>
>> >> **hcor) {
>> >> @@ -95,8 +99,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,
>> >> struct ehci_hcor **hcor) writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS |
>> >> CLKCTRL_PLL1CTRL0_POWER,
>> >>
>> >>                       &clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
>> >>
>> >> +#if defined(CONFIG_MX23)
>> >> +     writel(HW_DIGCTL_CTRL_USB0_CLKGATE,     &digctl_ctrl->reg_clr);
>> >
>> > What is that large space before '&digctl_regs->...' ?
>>
>> My fault. I can fix it.
>>
>> >> +#elif defined(CONFIG_MX28)
>> >>
>> >>       writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
>> >>
>> >>               &digctl_ctrl->reg_clr);
>> >>
>> >> +#endif
>> >
>> > I'm sure these can be wrapped in much more elegant way, yes?
>>
>> I wanted to keep the changes at minimum; so seems the right route.
>> What you'd like me to do? Use a temporary variable?
>
> Either that or wrap it all in struct ehci_mxs and do these ungatings etc on a
> per-port basis.

I've prepared the v3 of the patch; I will give it a runtime test
tomorrow and send.

Patch

diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
index 5062af5..b7bf856 100644
--- a/drivers/usb/host/ehci-mxs.c
+++ b/drivers/usb/host/ehci-mxs.c
@@ -50,10 +50,12 @@  int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int port)
 		usb_base = MXS_USBCTRL0_BASE;
 		phy_base = MXS_USBPHY0_BASE;
 		break;
+#ifdef CONFIG_MX28
 	case 1:
 		usb_base = MXS_USBCTRL1_BASE;
 		phy_base = MXS_USBPHY1_BASE;
 		break;
+#endif
 	default:
 		printf("CONFIG_EHCI_MXS_PORT (port = %d)\n", port);
 		return -1;
@@ -67,7 +69,9 @@  int mxs_ehci_get_port(struct ehci_mxs *mxs_usb, int port)
 /* This DIGCTL register ungates clock to USB */
 #define	HW_DIGCTL_CTRL			0x8001c000
 #define	HW_DIGCTL_CTRL_USB0_CLKGATE	(1 << 2)
+#ifdef CONFIG_MX28
 #define	HW_DIGCTL_CTRL_USB1_CLKGATE	(1 << 16)
+#endif
 
 int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 {
@@ -95,8 +99,12 @@  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
 			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
 
+#if defined(CONFIG_MX23)
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE,	&digctl_ctrl->reg_clr);
+#elif defined(CONFIG_MX28)
 	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
 		&digctl_ctrl->reg_clr);
+#endif
 
 	/* Start USB PHY */
 	writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
@@ -153,8 +161,12 @@  int ehci_hcd_stop(int index)
 			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
 
 	/* Gate off the USB clock */
+#if defined(CONFIG_MX23)
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE,	&digctl_ctrl->reg_set);
+#elif defined(CONFIG_MX28)
 	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
 		&digctl_ctrl->reg_set);
+#endif
 
 	return 0;
 }