[U-Boot] [PATCH v2 05/14] mx51: Fix USB PHY clocks

[Stefano Babic <sbabic@denx.de>]

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@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?

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 ;-)

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================