[U-Boot] [PATCH V2 2/2] imx: mx6 add i2c4 clock support for i.MX6SX

[Peng Fan]

Hi Stefano,

On Sun, Jun 28, 2015 at 10:52:50AM +0200, Stefano Babic wrote:
>Hi Peng,
>
>On 25/06/2015 03:33, Peng Fan wrote:
>> Add I2C4 clock support for i.MX6SX. Since we use runtime check,
>> but not macro, we need to remove `#ifdef ..` in crm_regs.h, or
>> we will get compliation failure for other platforms.
>
>               ^----- compliation ?
I mean gcc will complains about failing to compile the code.
>
>> 
>> Making the macros only for i.MX6SX open to other i.MX6x maybe not
>> a good choice, but we have runtime check.
>> 
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>> ---
>>  arch/arm/cpu/armv7/mx6/clock.c           | 14 ++++++++++----
>>  arch/arm/include/asm/arch-mx6/crm_regs.h |  9 ++++-----
>>  2 files changed, 14 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/arm/cpu/armv7/mx6/clock.c b/arch/arm/cpu/armv7/mx6/clock.c
>> index 1134770..b461898 100644
>> --- a/arch/arm/cpu/armv7/mx6/clock.c
>> +++ b/arch/arm/cpu/armv7/mx6/clock.c
>> @@ -126,6 +126,7 @@ int enable_i2c_clk(unsigned char enable, unsigned i2c_num)
>>  {
>>  	u32 reg;
>>  	u32 mask;
>> +	u32 *addr;
>>  
>>  	if (i2c_num > 3)
>>  		return -EINVAL;
>> @@ -140,14 +141,19 @@ int enable_i2c_clk(unsigned char enable, unsigned i2c_num)
>>  			reg &= ~mask;
>>  		__raw_writel(reg, &imx_ccm->CCGR2);
>>  	} else {
>> -		mask = MXC_CCM_CCGR_CG_MASK
>> -			<< (MXC_CCM_CCGR1_I2C4_SERIAL_OFFSET);
>> -		reg = __raw_readl(&imx_ccm->CCGR1);
>> +		if (is_cpu_type(MXC_CPU_MX6SX)) {
>> +			mask = MXC_CCM_CCGR6_I2C4_MASK;
>> +			addr = &imx_ccm->CCGR6;
>> +		} else {
>> +			mask = MXC_CCM_CCGR1_I2C4_SERIAL_MASK;
>> +			addr = &imx_ccm->CCGR1;
>> +		}
>> +		reg = __raw_readl(addr);
>>  		if (enable)
>>  			reg |= mask;
>>  		else
>>  			reg &= ~mask;
>> -		__raw_writel(reg, &imx_ccm->CCGR1);
>> +		__raw_writel(reg, addr);
>>  	}
>>  	return 0;
>>  }
>> diff --git a/arch/arm/include/asm/arch-mx6/crm_regs.h b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> index 887d048..4c743b6 100644
>> --- a/arch/arm/include/asm/arch-mx6/crm_regs.h
>> +++ b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> @@ -740,7 +740,10 @@ struct mxc_ccm_reg {
>>  #define MXC_CCM_CCGR6_USDHC4_MASK		(3 << MXC_CCM_CCGR6_USDHC4_OFFSET)
>>  #define MXC_CCM_CCGR6_EMI_SLOW_OFFSET		10
>>  #define MXC_CCM_CCGR6_EMI_SLOW_MASK		(3 << MXC_CCM_CCGR6_EMI_SLOW_OFFSET)
>> -#ifdef CONFIG_MX6SX
>> +/* The two does not exist on i.MX6SX */
>> +#define MXC_CCM_CCGR6_VDOAXICLK_OFFSET		12
>> +#define MXC_CCM_CCGR6_VDOAXICLK_MASK		(3 << MXC_CCM_CCGR6_VDOAXICLK_OFFSET)
>> +/* The following *CCGR6* exist only i.MX6SX */
>
>Agree to group together macros that belong only to a SOC variant, else
>it is difficult to find them. But what has
>MXC_CCM_CCGR6_VDOAXICLK_OFFSET to do with addings I2C4 clock to i.MX6SX
>? It should be posted in a separate patch.

I just move this piece of code from the back to the front, see the end
of the patch.
I removed the #ifdef in header file, because it is really not a good idea
to see so many #ifdefs in header file. The bad point here is some CCM macros
are exposed to other platforms, so we should take care to use the CCM macros.

Take the following as example:
In crm_regs.h
#ifdef CONFIG_MX6SX
#define MXC_CCM_CCGR6_PWM7_MASK 	(3 << 30)
#else
#define MXC_CCM_CCGR6_VDOAXICLK_MASK	(3 << 12)
#endif

u32 test()
{
	if (is_cpu_type(MXC_CPU_MX6SX))
		return MXC_CCM_CCGR6_PWM7_MASK;		
	else
		return MXC_CCM_CCGR6_VDOAXICLK_OFFSET;
}

Here gcc can not successfully compile the code if we want to build for i.mx6sx,
becuase MXC_CCM_CCGR6_VDOAXICLK_OFFSET is not defined for i.mx6sx.

So:
#define MXC_CCM_CCGR6_PWM7_MASK     (3 << 30)
/* This CCM Definition does not exist on i.MX6SX */
#define MXC_CCM_CCGR6_VDOAXICLK_MASK    (3 << 12)

>
>>  #define MXC_CCM_CCGR6_PWM8_OFFSET		16
>>  #define MXC_CCM_CCGR6_PWM8_MASK			(3 << MXC_CCM_CCGR6_PWM8_OFFSET)
>>  #define MXC_CCM_CCGR6_VADC_OFFSET		20
>> @@ -755,10 +758,6 @@ struct mxc_ccm_reg {
>>  #define MXC_CCM_CCGR6_PWM6_MASK			(3 << MXC_CCM_CCGR6_PWM6_OFFSET)
>>  #define MXC_CCM_CCGR6_PWM7_OFFSET		30
>>  #define MXC_CCM_CCGR6_PWM7_MASK			(3 << MXC_CCM_CCGR6_PWM7_OFFSET)
>> -#else
>> -#define MXC_CCM_CCGR6_VDOAXICLK_OFFSET		12
>> -#define MXC_CCM_CCGR6_VDOAXICLK_MASK		(3 << MXC_CCM_CCGR6_VDOAXICLK_OFFSET)
>> -#endif
>>  
>>  #define BM_ANADIG_PLL_SYS_LOCK 0x80000000
>>  #define BP_ANADIG_PLL_SYS_RSVD0      20
>> 
>
>Best regards,
>Stefano Babic
>
>-- 
>=====================================================================
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>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
>=====================================================================

Regards,
Peng.
--