[U-Boot] [PATCH v2 05/14] mx51: Fix USB PHY clocks
Benoît Thébaudeau
benoit.thebaudeau at advansee.com
Fri Sep 28 15:00:36 CEST 2012
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 <benoit.thebaudeau at advansee.com>
> >>> Cc: Stefano Babic <sbabic at denx.de>
> >>> Cc: Marek Vasut <marex at denx.de>
> >>> Cc: Jana Rapava <fermata7 at gmail.com>
> >>> Cc: Wolfgang Grandegger <wg at denx.de>
> >>> Cc: Igor Grinberg <grinberg at 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?
>
> 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
More information about the U-Boot
mailing list