| Message ID | 1361130325-5202-8-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | Superseded |
| Headers | show |
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
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?
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.
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 ;-)
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; > }
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; }
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(-)