[U-Boot] [PATCH] arm, imx6, i2c: add I2C4 for MX6DL

[Heiko Schocher <hs@denx.de>]

Hello Stefano,

Am 08.05.2015 09:05, schrieb Stefano Babic:
> Hi Heiko,
>
> On 12/04/2015 10:14, Heiko Schocher wrote:
>> add I2C4 modul for MX6DL based boards.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>
>> ---
>> checkpatch shows:
>> WARNING: line over 80 characters
>> +#define MXC_CCM_CCGR1_I2C4_SERIAL_MASK                 (3 << MXC_CCM_CCGR1_I2C4_SERIAL_OFFSET)
>>
>> but the hole file is full of lines longer than80 characters, so
>> I let this line in the old style ...
>>
>>   arch/arm/cpu/armv7/mx6/clock.c           | 33 +++++++++++++++++++++-----------
>>   arch/arm/imx-common/i2c-mxv7.c           |  5 ++++-
>>   arch/arm/include/asm/arch-mx6/crm_regs.h |  2 ++
>>   arch/arm/include/asm/arch-mx6/imx-regs.h |  1 +
>>   drivers/i2c/mxc_i2c.c                    | 18 ++++++++++++++++-
>
> You patch conflicts with the already applied York's:

Uh.. seems a "git rebase" did not poped up some conflicts ...

> commit f8cb101e1e3f5ee2007b78b6b12e24120385aeac
> Author: York Sun <yorksun@freescale.com>
> Date:   Fri Mar 20 10:20:40 2015 -0700
>
>      driver/i2c/mxc: Enable I2C bus 3 and 4
>
>      Some SoCs have more than two I2C busses. Instead of adding ifdef
>      to the driver, macros are put into board header file where
>      CONFIG_SYS_I2C_MXC is defined.
>
>      Signed-off-by: York Sun <yorksun@freescale.com>
>      CC: Heiko Schocher <hs@denx.de>
>
> On York's patch, activating I2C4 is done according to CONFIG_FSL_LSCH3 -
> this should be rework to set it for MX6DL.

Done.

> As far as I can see, you need only to set I2C4_BASE_ADDR and add your
> U_BOOT_I2C_ADAP_COMPLETE. An item for a follow-up patch can be to clean
> up i2c_bases[].

Yes, I need only to set I2C4_BASE_ADDR !

> There is:
> 	 IMX_I2C_BASE for mx25
> 	 I2Cx_BASE_ADDR for most i.MX
> 	 I2C0_BASE_ADDR	for VF610 (it starts from zero here)
> 	
>
> The #ifdef is quite confusing. Can we maybe set to Null if a controller
> is not available, maybe checking when we return the base address ?

This is not needed, as this fields are not used, if CONFIG_SYS_I2C_MXC_I2C3
and/or CONFIG_SYS_I2C_MXC_I2C4 are not set... but to be save, I added such
a check.

> My proposal could be:
>
> #if !defined (I2C1_BASE_ADDR)
> #define I2C1_BASE_ADDR
> #endif

reworked to:

#if !defined (I2C3_BASE_ADDR)
#define I2C3_BASE_ADDR  NULL
#endif

#if !defined (I2C4_BASE_ADDR)
#define I2C4_BASE_ADDR  NULL
#endif

static void * const i2c_bases[] = {
         (void *)I2C1_BASE_ADDR,
         (void *)I2C2_BASE_ADDR,
         (void *)I2C3_BASE_ADDR,
         (void *)I2C4_BASE_ADDR
};

> // The same for the other 3 controller, adjusting names to have the same
>
> And then:
>
> static void * const i2c_bases[] = {
>          (void *)I2C1_BASE_ADDR,
>          (void *)I2C2_BASE_ADDR,
>          (void *)I2C3_BASE_ADDR,
>          (void *)I2C4_BASE_ADDR,
> }
>
> Can you take a look at it ? Thanks !

Thanks for your review! I had to adapt the aristainetos2  [1] board patch ...
Could you review it too? Thanks!

bye,
Heiko

[1] Patchwork [U-Boot] arm, imx6: add support for aristainetos2 board
http://patchwork.ozlabs.org/patch/460467/

>
>>   5 files changed, 46 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/mx6/clock.c b/arch/arm/cpu/armv7/mx6/clock.c
>> index 055f44e..ae99945 100644
>> --- a/arch/arm/cpu/armv7/mx6/clock.c
>> +++ b/arch/arm/cpu/armv7/mx6/clock.c
>> @@ -140,23 +140,34 @@ int enable_usdhc_clk(unsigned char enable, unsigned bus_num)
>>   #endif
>>
>>   #ifdef CONFIG_SYS_I2C_MXC
>> -/* i2c_num can be from 0 - 2 */
>> +/* i2c_num can be from 0 - 3 */
>>   int enable_i2c_clk(unsigned char enable, unsigned i2c_num)
>>   {
>>   	u32 reg;
>>   	u32 mask;
>>
>> -	if (i2c_num > 2)
>> +	if (i2c_num > 3)
>>   		return -EINVAL;
>> -
>> -	mask = MXC_CCM_CCGR_CG_MASK
>> -		<< (MXC_CCM_CCGR2_I2C1_SERIAL_OFFSET + (i2c_num << 1));
>> -	reg = __raw_readl(&imx_ccm->CCGR2);
>> -	if (enable)
>> -		reg |= mask;
>> -	else
>> -		reg &= ~mask;
>> -	__raw_writel(reg, &imx_ccm->CCGR2);
>> +	if (i2c_num < 3) {
>> +		mask = MXC_CCM_CCGR_CG_MASK
>> +			<< (MXC_CCM_CCGR2_I2C1_SERIAL_OFFSET
>> +			+ (i2c_num << 1));
>> +		reg = __raw_readl(&imx_ccm->CCGR2);
>> +		if (enable)
>> +			reg |= mask;
>> +		else
>> +			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 (enable)
>> +			reg |= mask;
>> +		else
>> +			reg &= ~mask;
>> +		__raw_writel(reg, &imx_ccm->CCGR1);
>> +	}
>>   	return 0;
>>   }
>>   #endif
>> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
>> index 1a632e7..33205fb 100644
>> --- a/arch/arm/imx-common/i2c-mxv7.c
>> +++ b/arch/arm/imx-common/i2c-mxv7.c
>> @@ -67,9 +67,12 @@ static void * const i2c_bases[] = {
>>   #ifdef I2C3_BASE_ADDR
>>   	(void *)I2C3_BASE_ADDR,
>>   #endif
>> +#ifdef I2C4_BASE_ADDR
>> +	(void *)I2C4_BASE_ADDR,
>> +#endif
>>   };
>>
>> -/* i2c_index can be from 0 - 2 */
>> +/* i2c_index can be from 0 - 3 */
>>   int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>>   	      struct i2c_pads_info *p)
>>   {
>> diff --git a/arch/arm/include/asm/arch-mx6/crm_regs.h b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> index 0592ce0..887d048 100644
>> --- a/arch/arm/include/asm/arch-mx6/crm_regs.h
>> +++ b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> @@ -592,6 +592,8 @@ struct mxc_ccm_reg {
>>   #define MXC_CCM_CCGR2_I2C2_SERIAL_MASK			(3 << MXC_CCM_CCGR2_I2C2_SERIAL_OFFSET)
>>   #define MXC_CCM_CCGR2_I2C3_SERIAL_OFFSET		10
>>   #define MXC_CCM_CCGR2_I2C3_SERIAL_MASK			(3 << MXC_CCM_CCGR2_I2C3_SERIAL_OFFSET)
>> +#define MXC_CCM_CCGR1_I2C4_SERIAL_OFFSET		8
>> +#define MXC_CCM_CCGR1_I2C4_SERIAL_MASK			(3 << MXC_CCM_CCGR1_I2C4_SERIAL_OFFSET)
>>   #define MXC_CCM_CCGR2_OCOTP_CTRL_OFFSET			12
>>   #define MXC_CCM_CCGR2_OCOTP_CTRL_MASK			(3 << MXC_CCM_CCGR2_OCOTP_CTRL_OFFSET)
>>   #define MXC_CCM_CCGR2_IOMUX_IPT_CLK_IO_OFFSET		14
>> diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h
>> index 9a4ad8b..3d3d137 100644
>> --- a/arch/arm/include/asm/arch-mx6/imx-regs.h
>> +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h
>> @@ -277,6 +277,7 @@
>>   #define UART3_BASE                  (AIPS2_OFF_BASE_ADDR + 0x6C000)
>>   #define UART4_BASE                  (AIPS2_OFF_BASE_ADDR + 0x70000)
>>   #define UART5_BASE                  (AIPS2_OFF_BASE_ADDR + 0x74000)
>> +#define I2C4_BASE_ADDR              (AIPS2_OFF_BASE_ADDR + 0x78000)
>>   #define IP2APB_USBPHY1_BASE_ADDR    (AIPS2_OFF_BASE_ADDR + 0x78000)
>>   #define IP2APB_USBPHY2_BASE_ADDR    (AIPS2_OFF_BASE_ADDR + 0x7C000)
>>
>> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
>> index fc5ee35..c7d9c94 100644
>> --- a/drivers/i2c/mxc_i2c.c
>> +++ b/drivers/i2c/mxc_i2c.c
>> @@ -114,6 +114,9 @@ static u16 i2c_clk_div[50][2] = {
>>   #ifndef CONFIG_SYS_MXC_I2C3_SPEED
>>   #define CONFIG_SYS_MXC_I2C3_SPEED 100000
>>   #endif
>> +#ifndef CONFIG_SYS_MXC_I2C4_SPEED
>> +#define CONFIG_SYS_MXC_I2C4_SPEED 100000
>> +#endif
>>
>>   #ifndef CONFIG_SYS_MXC_I2C1_SLAVE
>>   #define CONFIG_SYS_MXC_I2C1_SLAVE 0
>> @@ -124,6 +127,9 @@ static u16 i2c_clk_div[50][2] = {
>>   #ifndef CONFIG_SYS_MXC_I2C3_SLAVE
>>   #define CONFIG_SYS_MXC_I2C3_SLAVE 0
>>   #endif
>> +#ifndef CONFIG_SYS_MXC_I2C4_SLAVE
>> +#define CONFIG_SYS_MXC_I2C4_SLAVE 0
>> +#endif
>>
>>
>>   /*
>> @@ -415,7 +421,10 @@ static void * const i2c_bases[] = {
>>   	defined(CONFIG_MX6) || defined(CONFIG_LS102XA)
>>   	(void *)I2C1_BASE_ADDR,
>>   	(void *)I2C2_BASE_ADDR,
>> -	(void *)I2C3_BASE_ADDR
>> +	(void *)I2C3_BASE_ADDR,
>> +#if defined(CONFIG_MX6DL)
>> +	(void *)I2C4_BASE_ADDR
>> +#endif
>>   #elif defined(CONFIG_VF610)
>>   	(void *)I2C0_BASE_ADDR
>>   #elif defined(CONFIG_FSL_LSCH3)
>> @@ -551,4 +560,11 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe,
>>   			 mxc_i2c_set_bus_speed,
>>   			 CONFIG_SYS_MXC_I2C3_SPEED,
>>   			 CONFIG_SYS_MXC_I2C3_SLAVE, 2)
>> +#if defined(CONFIG_MX6DL)
>> +U_BOOT_I2C_ADAP_COMPLETE(mxc3, mxc_i2c_init, mxc_i2c_probe,
>> +			 mxc_i2c_read, mxc_i2c_write,
>> +			 mxc_i2c_set_bus_speed,
>> +			 CONFIG_SYS_MXC_I2C4_SPEED,
>> +			 CONFIG_SYS_MXC_I2C4_SLAVE, 3)
>> +#endif
>>   #endif
>>
>
> Best regards,
> Stefano Babic
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany