Hi Igor,
On Friday, September 28, 2012 9:26:38 AM, Igor Grinberg wrote:
Hi Benoît,
please, see a minor #ifdef comment below
On 09/27/12 22:21, Benoît Thébaudeau wrote:
The i.MX51 has a single USB PHY clock, while the i.MX53 has two. These 3 clocks have different clock gate control bit-fields.
The existing code was correct only for i.MX53, so this patch fixes the i.MX51 use case.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com Cc: Stefano Babic sbabic@denx.de Cc: Marek Vasut marex@denx.de Cc: Jana Rapava fermata7@gmail.com Cc: Wolfgang Grandegger wg@denx.de Cc: Igor Grinberg grinberg@compulab.co.il
This patch supersedes http://patchwork.ozlabs.org/patch/177403/ . Changes for v2:
- Split patch into 3 parts (the 3, 4 and 5 from this v2 series).
- Merge the various set_usb_phy*_clk() functions (they were
identical).
.../arch/arm/cpu/armv7/mx5/clock.c | 20 +++++++++++++------- .../arch/arm/include/asm/arch-mx5/clock.h | 7 ++++++- .../drivers/usb/host/ehci-mx5.c | 6 +++++- 3 files changed, 24 insertions(+), 9 deletions(-)
diff --git u-boot-imx-e1eb75b.orig/arch/arm/cpu/armv7/mx5/clock.c u-boot-imx-e1eb75b/arch/arm/cpu/armv7/mx5/clock.c index df7e5cd..f727cfa 100644 --- u-boot-imx-e1eb75b.orig/arch/arm/cpu/armv7/mx5/clock.c +++ u-boot-imx-e1eb75b/arch/arm/cpu/armv7/mx5/clock.c @@ -126,11 +126,21 @@ int enable_i2c_clk(unsigned char enable, unsigned i2c_num) } #endif
-void set_usb_phy1_clk(void) +void set_usb_phy_clk(void) { clrbits_le32(&mxc_ccm->cscmr1, MXC_CCM_CSCMR1_USB_PHY_CLK_SEL); }
+#if defined(CONFIG_MX51) +void enable_usb_phy_clk(unsigned char enable)
I think the same name as the MX53 case will do better, see explanation below.
+{
- unsigned int cg = enable ? MXC_CCM_CCGR_CG_ON :
MXC_CCM_CCGR_CG_OFF;
- clrsetbits_le32(&mxc_ccm->CCGR2,
MXC_CCM_CCGR2_USB_PHY(MXC_CCM_CCGR_CG_MASK),MXC_CCM_CCGR2_USB_PHY(cg));+}
I would add here (again explanation below): void enable_usb_phy2_clk(unsigned char enable) { /* MX51 has a single USB PHY clock, so do nothing here */ }
+#elif defined(CONFIG_MX53) void enable_usb_phy1_clk(unsigned char enable) { unsigned int cg = enable ? MXC_CCM_CCGR_CG_ON : MXC_CCM_CCGR_CG_OFF; @@ -140,11 +150,6 @@ void enable_usb_phy1_clk(unsigned char enable) MXC_CCM_CCGR4_USB_PHY1(cg)); }
-void set_usb_phy2_clk(void) -{
- clrbits_le32(&mxc_ccm->cscmr1, MXC_CCM_CSCMR1_USB_PHY_CLK_SEL);
-}
void enable_usb_phy2_clk(unsigned char enable) { unsigned int cg = enable ? MXC_CCM_CCGR_CG_ON : MXC_CCM_CCGR_CG_OFF; @@ -153,6 +158,7 @@ void enable_usb_phy2_clk(unsigned char enable) MXC_CCM_CCGR4_USB_PHY2(MXC_CCM_CCGR_CG_MASK), MXC_CCM_CCGR4_USB_PHY2(cg)); } +#endif
/*
- Calculate the frequency of PLLn.
@@ -803,7 +809,7 @@ void mxc_set_sata_internal_clock(void) u32 *tmp_base = (u32 *)(IIM_BASE_ADDR + 0x180c);
- set_usb_phy1_clk();
set_usb_phy_clk();
clrsetbits_le32(tmp_base, 0x6, 0x4);
} diff --git u-boot-imx-e1eb75b.orig/arch/arm/include/asm/arch-mx5/clock.h u-boot-imx-e1eb75b/arch/arm/include/asm/arch-mx5/clock.h index 55e3b51..e4ca417 100644 --- u-boot-imx-e1eb75b.orig/arch/arm/include/asm/arch-mx5/clock.h +++ u-boot-imx-e1eb75b/arch/arm/include/asm/arch-mx5/clock.h @@ -56,8 +56,13 @@ u32 imx_get_uartclk(void); u32 imx_get_fecclk(void); unsigned int mxc_get_clock(enum mxc_clock clk); int mxc_set_clock(u32 ref, u32 freq, u32 clk_type); -void set_usb_phy2_clk(void); +void set_usb_phy_clk(void); +#if defined(CONFIG_MX51) +void enable_usb_phy_clk(unsigned char enable); +#elif defined(CONFIG_MX53) +void enable_usb_phy1_clk(unsigned char enable); void enable_usb_phy2_clk(unsigned char enable); +#endif
If you follow my suggestion above, then here you will only have: -void set_usb_phy2_clk(void); +void set_usb_phy_clk(void); +void enable_usb_phy1_clk(unsigned char enable);
and no complicating with #ifdefs needed.
void set_usboh3_clk(void); void enable_usboh3_clk(unsigned char enable); void mxc_set_sata_internal_clock(void); diff --git u-boot-imx-e1eb75b.orig/drivers/usb/host/ehci-mx5.c u-boot-imx-e1eb75b/drivers/usb/host/ehci-mx5.c index 58cdcbe..5bf89f0 100644 --- u-boot-imx-e1eb75b.orig/drivers/usb/host/ehci-mx5.c +++ u-boot-imx-e1eb75b/drivers/usb/host/ehci-mx5.c @@ -221,8 +221,12 @@ int ehci_hcd_init(void)
set_usboh3_clk(); enable_usboh3_clk(1);
- set_usb_phy2_clk();
- set_usb_phy_clk();
+#if defined(CONFIG_MX51) && CONFIG_MXC_USB_PORT == 0
- enable_usb_phy_clk(1);
+#elif defined(CONFIG_MX53) enable_usb_phy2_clk(1); +#endif
same here, it should be much cleaner.
mdelay(1);
/* Do board specific initialization */
Indeed. In clock.h, the #ifdefs could anyway be removed with the naming from my patch. The only difference would be an error at link time rather than at compile time if these functions were called for the wrong i.MX5. If we change the naming as you suggest, it's not a problem for set_usb_phy1_clk(), but for enable_usb_phy2_clk() I think that it would not really make sense. My point is that ehci-mx5.c should be cleaned even more deeply in order to initialize the USB PHYs corresponding to the selected EHCI ports, and not all (or only some for i.MX53) the PHYs at the beginning of ehci_hcd_init(). Also, calling enable_usb_phy2_clk() for i.MX51 would not make sense, even if it does nothing, because it would be confusing. So this solution has both advantages and drawbacks.
Stefano, Marek, what do you think?
Best regards, Benoît