From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= Date: Fri, 28 Sep 2012 15:00:36 +0200 (CEST) Subject: [U-Boot] [PATCH v2 05/14] mx51: Fix USB PHY clocks In-Reply-To: <50657F69.30200@denx.de> Message-ID: <1278391539.5418946.1348837236122.JavaMail.root@advansee.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stefano, On Friday, September 28, 2012 12:43:53 PM, Stefano Babic wrote: > On 28/09/2012 12:27, Beno?t Th?baudeau wrote: > > 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 > >>> Cc: Stefano Babic > >>> Cc: Marek Vasut > >>> Cc: Jana Rapava > >>> Cc: Wolfgang Grandegger > >>> Cc: Igor Grinberg > >>> --- > >>> 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? > > I am much sensible regarding #ifdef. I admit, I hate them. Igor's > suggestion drop some of them, and IMHO the code is more readable, > independently if and when the ehci-mx5.c will be cleaned up. However, > I > understand it is a personal taste ;-) OK, so should I eventually send for that an update only of this specific patch (not the full series), or will it just be fine with the current v2? Best regards, Beno?t