[U-Boot] [PATCH v4 07/10] usb: mxs: Adapt code for i.MX23 support

[Marek Vasut]

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 at ossystems.com.br>
> ---
> Changes in v4:
> - Rework soc_ehci_hcd_{enable,disable}_clock to mxs_ehci_hcd_clock (Fabio /
> Marek)
> 
> 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 | 58
> +++++++++++++++++++++++++++------------------ 1 file changed, 35
> insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxs.c b/drivers/usb/host/ehci-mxs.c
> index 5062af5..45333ce 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

This should be handled automatically in imx-regs.h no ? I believe all this regs-
xxx.h crap should just be part of imx-regs.h and none of that should be here at 
all.

>  #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,17 +73,40 @@ 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 mxs_ehci_hcd_clock(bool enable)

I wonder if this shall not go to arch/arm.../mxs/clock.c ?

I'd say, pass also "int index" argument and call it twice for mx28 and once for 
mx23. That'd align well with the multi-controller stuff.

>  {
> -
> -	int ret;
> -	uint32_t usb_base, cap_base;
>  	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;
> +	uint32_t reg = HW_DIGCTL_CTRL_USB0_CLKGATE;
> +
> +	writel(CLKCTRL_PLL0CTRL0_EN_USB_CLKS | CLKCTRL_PLL0CTRL0_POWER,
> +		   (enable ? &clkctrl_regs->hw_clkctrl_pll0ctrl0_set : \
> +			&clkctrl_regs->hw_clkctrl_pll0ctrl0_clr));
> +
> +#ifdef CONFIG_MX28
> +	/* i.MX28 has two USB controllers */
> +	reg |= HW_DIGCTL_CTRL_USB1_CLKGATE;
> +
> +	writel(CLKCTRL_PLL1CTRL0_EN_USB_CLKS | CLKCTRL_PLL1CTRL0_POWER,
> +		   (enable ? &clkctrl_regs->hw_clkctrl_pll1ctrl0_set : \
> +			&clkctrl_regs->hw_clkctrl_pll1ctrl0_clr));
> +#endif
> +
> +	/* Gate/gateoff the USB clock */
> +	writel(reg, (enable ? &digctl_ctrl->reg_clr : \
> +				 &digctl_ctrl->reg_set));

Uh ... uh ... uh ...

No, kill the ternary operators. Good old if (enable) .... else .... please.

Something like (imprecise code warning):

if (enable)
   reg_offset = offsetof(mxs_register32, set);
else
   reg_offset = offsetof(mxs_register32, clk);

do_all_the_stuff here, all writel(val, mxs_register32 + offset); like this 
example.

> +}
> +
> +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)
> @@ -90,13 +119,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);
> +	mxs_ehci_hcd_clock(true);
> 
>  	/* Start USB PHY */
>  	writel(0, &ehci_mxs.phy_regs->hw_usbphy_pwd);
> @@ -118,10 +141,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 +166,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);
> +	mxs_ehci_hcd_clock(false);
> 
>  	return 0;
>  }

You might want to separate out the USB patches, they might need some work.

Best regards,
Marek Vasut