Patchwork [U-Boot,v3,07/10] usb: mxs: Adapt code for i.MX23 support

login
register
mail settings
Submitter Otavio Salvador
Date Feb. 17, 2013, 7:45 p.m.
Message ID <1361130325-5202-8-git-send-email-otavio@ossystems.com.br>
Download mbox | patch
Permalink /patch/221097/
State Superseded
Headers show

Comments

Otavio Salvador - Feb. 17, 2013, 7:45 p.m.
The i.MX23 just one USB port so we shouldn't mess up with PLL1CTRL and
USB1 port when building for i.MX23.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
Changes in v3:
- Improve commit log
- Move code to enable/disable clock to soc_ehci_hcd_{enable,disable}_clock
- Proper use mx23 clock registers

Changes in v2:
- Avoid wrong clock setting in MX23

 drivers/usb/host/ehci-mxs.c | 78 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 56 insertions(+), 22 deletions(-)
Fabio Estevam - Feb. 17, 2013, 8:03 p.m.
On Sun, Feb 17, 2013 at 4:45 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> The i.MX23 just one USB port so we shouldn't mess up with PLL1CTRL and

I think "MX23 has just one USB port" would be better.

> -int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
> +static void soc_ehci_hcd_enable_clock(void)

Since this is mxs specific a better name would be: mxs_ehci_hcd_enable_clock
Otavio Salvador - Feb. 17, 2013, 8:07 p.m.
On Sun, Feb 17, 2013 at 5:03 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sun, Feb 17, 2013 at 4:45 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>> The i.MX23 just one USB port so we shouldn't mess up with PLL1CTRL and
>
> I think "MX23 has just one USB port" would be better.
>
>> -int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
>> +static void soc_ehci_hcd_enable_clock(void)
>
> Since this is mxs specific a better name would be: mxs_ehci_hcd_enable_clock

I did it but mxs gives the impression it does mostly the same in both
socs; as the code is mostly different between both I choose soc so it
is clear it is for the soc it is being build.

That was my thought... what do you think?
Fabio Estevam - Feb. 17, 2013, 8:11 p.m.
On Sun, Feb 17, 2013 at 5:07 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> I did it but mxs gives the impression it does mostly the same in both
> socs; as the code is mostly different between both I choose soc so it
> is clear it is for the soc it is being build.
>
> That was my thought... what do you think?

When I read  "soc_ehci_hcd_enable_clock" I tend to think that this is
a core USB function.

When I read "mxs_ehci_hcd_enable_clock" it's clear to me that this
relates only to mx23/mx28.

Even if it handles the differences between mx23/mx28, I would prefer
it with a "mxs" prefix.
Otavio Salvador - Feb. 17, 2013, 8:33 p.m.
On Sun, Feb 17, 2013 at 5:11 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sun, Feb 17, 2013 at 5:07 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>
>> I did it but mxs gives the impression it does mostly the same in both
>> socs; as the code is mostly different between both I choose soc so it
>> is clear it is for the soc it is being build.
>>
>> That was my thought... what do you think?
>
> When I read  "soc_ehci_hcd_enable_clock" I tend to think that this is
> a core USB function.
>
> When I read "mxs_ehci_hcd_enable_clock" it's clear to me that this
> relates only to mx23/mx28.
>
> Even if it handles the differences between mx23/mx28, I would prefer
> it with a "mxs" prefix.

Fine; I can prepare it for v4; I will wait for Marek's comments to try
to avoid a v5 ;-)
Marek Vasut - Feb. 17, 2013, 9:21 p.m.
Dear Otavio Salvador,

> The i.MX23 just one USB port so we shouldn't mess up with PLL1CTRL and
> USB1 port when building for i.MX23.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
> Changes in v3:
> - Improve commit log
> - Move code to enable/disable clock to soc_ehci_hcd_{enable,disable}_clock
> - Proper use mx23 clock registers
> 
> Changes in v2:
> - Avoid wrong clock setting in MX23
> 
>  drivers/usb/host/ehci-mxs.c | 78
> ++++++++++++++++++++++++++++++++------------- 1 file changed, 56
> insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
> index 5062af5..5e41a38 100644
> --- a/drivers/usb/host/ehci-mxs.c
> +++ b/drivers/usb/host/ehci-mxs.c
> @@ -23,7 +23,11 @@
>  #include <asm/io.h>
>  #include <asm/arch/regs-common.h>
>  #include <asm/arch/regs-base.h>
> +#if defined(CONFIG_MX23)
> +#include <asm/arch/regs-clkctrl-mx23.h>
> +#elif defined(CONFIG_MX28)
>  #include <asm/arch/regs-clkctrl-mx28.h>
> +#endif
>  #include <asm/arch/regs-usb.h>
>  #include <asm/arch/regs-usbphy.h>
> 
> @@ -50,10 +54,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,18 +73,63 @@ 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) +static void soc_ehci_hcd_enable_clock(void)
>  {
> +	struct mxs_register_32 *digctl_ctrl =
> +		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
> +	struct mxs_clkctrl_regs *clkctrl_regs =
> +		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
> 
> -	int ret;
> -	uint32_t usb_base, cap_base;
> +#if defined(CONFIG_MX23)
> +	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
> +			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);

This stuff above ^

> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE, &digctl_ctrl->reg_clr);
> +#elif defined(CONFIG_MX28)
> +	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
> +			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);

And here ^ looks like the same code, no?

btw. can this not be nicely factored away one simple ifdef CONFIG_MX28 ?

> +	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
> +			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
> +
> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
> +		&digctl_ctrl->reg_clr);
> +#endif
> +}
> +
> +static void soc_ehci_hcd_disable_clock(void)
> +{
>  	struct mxs_register_32 *digctl_ctrl =
>  		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
>  	struct mxs_clkctrl_regs *clkctrl_regs =
>  		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
> 
> +#if defined(CONFIG_MX23)
> +	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
> +			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
> +
> +	/* Gate off the USB clock */
> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE, &digctl_ctrl->reg_set);
> +#elif defined(CONFIG_MX28)
> +	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
> +			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
> +	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS,
> +			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
> +
> +	/* Gate off the USB clock */
> +	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
> +		&digctl_ctrl->reg_set);
> +#endif
> +}
> +
> +int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor
> **hcor) +{
> +	int ret;
> +	uint32_t usb_base, cap_base;
> +
>  	ret = mxs_ehci_get_port(&ehci_mxs, CONFIG_EHCI_MXS_PORT);
>  	if (ret)
>  		return ret;
> @@ -90,13 +141,7 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr,
> struct ehci_hcor **hcor) &ehci_mxs.phy_regs->hw_usbphy_ctrl_clr);
> 
>  	/* Enable USB clock */
> -	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
> -			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);
> -	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
> -			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
> -
> -	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
> -		&digctl_ctrl->reg_clr);
> +	soc_ehci_hcd_enable_clock();
> 
>  	/* Start USB PHY */
>  	writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
> @@ -118,10 +163,6 @@ int ehci_hcd_stop(int index)
>  {
>  	int ret;
>  	uint32_t usb_base, cap_base, tmp;
> -	struct mxs_register_32 *digctl_ctrl =
> -		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
> -	struct mxs_clkctrl_regs *clkctrl_regs =
> -		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
>  	struct ehci_hccr *hccr;
>  	struct ehci_hcor *hcor;
> 
> @@ -147,14 +188,7 @@ int ehci_hcd_stop(int index)
>  	writel(tmp, &ehci_mxs.phy_regs->hw_usbphy_pwd);
> 
>  	/* Disable USB clock */
> -	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
> -			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
> -	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS,
> -			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
> -
> -	/* Gate off the USB clock */
> -	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
> -		&digctl_ctrl->reg_set);
> +	soc_ehci_hcd_disable_clock();
> 
>  	return 0;
>  }

Patch

diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
index 5062af5..5e41a38 100644
--- a/drivers/usb/host/ehci-mxs.c
+++ b/drivers/usb/host/ehci-mxs.c
@@ -23,7 +23,11 @@ 
 #include <asm/io.h>
 #include <asm/arch/regs-common.h>
 #include <asm/arch/regs-base.h>
+#if defined(CONFIG_MX23)
+#include <asm/arch/regs-clkctrl-mx23.h>
+#elif defined(CONFIG_MX28)
 #include <asm/arch/regs-clkctrl-mx28.h>
+#endif
 #include <asm/arch/regs-usb.h>
 #include <asm/arch/regs-usbphy.h>
 
@@ -50,10 +54,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,18 +73,63 @@  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)
+static void soc_ehci_hcd_enable_clock(void)
 {
+	struct mxs_register_32 *digctl_ctrl =
+		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
+	struct mxs_clkctrl_regs *clkctrl_regs =
+		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
 
-	int ret;
-	uint32_t usb_base, cap_base;
+#if defined(CONFIG_MX23)
+	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
+			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);
+
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE, &digctl_ctrl->reg_clr);
+#elif defined(CONFIG_MX28)
+	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
+			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);
+	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
+			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
+
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
+		&digctl_ctrl->reg_clr);
+#endif
+}
+
+static void soc_ehci_hcd_disable_clock(void)
+{
 	struct mxs_register_32 *digctl_ctrl =
 		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
 	struct mxs_clkctrl_regs *clkctrl_regs =
 		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
 
+#if defined(CONFIG_MX23)
+	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
+			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
+
+	/* Gate off the USB clock */
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE, &digctl_ctrl->reg_set);
+#elif defined(CONFIG_MX28)
+	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
+			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
+	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS,
+			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
+
+	/* Gate off the USB clock */
+	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
+		&digctl_ctrl->reg_set);
+#endif
+}
+
+int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
+{
+	int ret;
+	uint32_t usb_base, cap_base;
+
 	ret = mxs_ehci_get_port(&ehci_mxs, CONFIG_EHCI_MXS_PORT);
 	if (ret)
 		return ret;
@@ -90,13 +141,7 @@  int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 		&ehci_mxs.phy_regs->hw_usbphy_ctrl_clr);
 
 	/* Enable USB clock */
-	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
-			&clkctrl_regs->hw_clkctrl_pll0ctrl0_set);
-	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
-			&clkctrl_regs->hw_clkctrl_pll1ctrl0_set);
-
-	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
-		&digctl_ctrl->reg_clr);
+	soc_ehci_hcd_enable_clock();
 
 	/* Start USB PHY */
 	writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
@@ -118,10 +163,6 @@  int ehci_hcd_stop(int index)
 {
 	int ret;
 	uint32_t usb_base, cap_base, tmp;
-	struct mxs_register_32 *digctl_ctrl =
-		(struct mxs_register_32 *)HW_DIGCTL_CTRL;
-	struct mxs_clkctrl_regs *clkctrl_regs =
-		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
 	struct ehci_hccr *hccr;
 	struct ehci_hcor *hcor;
 
@@ -147,14 +188,7 @@  int ehci_hcd_stop(int index)
 	writel(tmp, &ehci_mxs.phy_regs->hw_usbphy_pwd);
 
 	/* Disable USB clock */
-	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS,
-			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr);
-	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS,
-			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr);
-
-	/* Gate off the USB clock */
-	writel(HW_DIGCTL_CTRL_USB0_CLKGATE | HW_DIGCTL_CTRL_USB1_CLKGATE,
-		&digctl_ctrl->reg_set);
+	soc_ehci_hcd_disable_clock();
 
 	return 0;
 }