Re: [PATCH] i2c: i2c_mxs: Set ACK_MODE bit

[Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

Hi Uwe,

On 07/02/2013 03:11 PM, Uwe Kleine-König wrote:
> On Tue, Jul 02, 2013 at 01:01:00PM -0300, Fabio Estevam wrote:
>> According to mx23 erratum 2727:
>>
>> "2727 : I2C 9th Clock Pulse (ACK) not generated when RETAIN_CLOCK set.
>>
>> Description:
>>
>> When RETAIN_CLOCK is set, the ninth clock pulse (ACK) is not generated.
>> However, the SDA line is read at the proper timing interval. If
>> RETAIN_CLOCK is cleared, the ninth clock pulse is generated.
>> Also, the HW_I2C_VERSION register incorrectly states the version is 1.2.
>> It should be 1.3.
>>
>> Workaround:
>> HW_I2C_CTRL1[ACK_MODE] has default value of 0. It should be set to 1 to
>> enable the fix for this issue."
>>
>> It has also been noticed that mx28 needs to implement this fix in order to have
>> SMBus to work properly.
>>
>> Reported-by: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
>> Signed-off-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Did you see this making the driver handle some situations that caused
> failure before?

No, I haven't. I saw the report from Christoph in the linux-arm-kernel
mailing list:
http://marc.info/?l=linux-arm-kernel&m=137277422127826&w=2

And thought it could be nice if we could get it fixed for mx23 and mx28.

>
>> ---
>>   drivers/i2c/busses/i2c-mxs.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
>> index 6d8094d..ce7ac86 100644
>> --- a/drivers/i2c/busses/i2c-mxs.c
>> +++ b/drivers/i2c/busses/i2c-mxs.c
>> @@ -56,6 +56,7 @@
>>   #define MXS_I2C_CTRL1_CLR	(0x48)
>>
>>   #define MXS_I2C_CTRL1_CLR_GOT_A_NAK		0x10000000
>> +#define MXS_I2C_CTRL1_ACK_MODE			0x08000000
>>   #define MXS_I2C_CTRL1_BUS_FREE_IRQ		0x80
>>   #define MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ	0x40
>>   #define MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ		0x20
>> @@ -140,6 +141,14 @@ static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
>>   	writel(0x00300030, i2c->regs + MXS_I2C_TIMING2);
>>
>>   	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
>> +
>> +	/*
>> +	 * According to mx23 erratum 2727:
>> +	 * "I2C 9th Clock Pulse (ACK) not generated when RETAIN_CLOCK set"
>> +	 *
>> +	 * HW_I2C_CTRL1[ACK_MODE] needs to be set when RETAIN_CLOCK is set.
>> +	 */
>> +	writel(MXS_I2C_CTRL1_ACK_MODE, i2c->regs + MXS_I2C_CTRL1_SET);
> So you set this bis in the reset routine. The first thing in
> mxs_i2c_pio_setup_xfer is however:
>
> 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR);
>
> which unsets the ACK_MODE bit. Also when reading the docs I couldn't

Not really. This write is via MXS_I2C_CTRL1_CLR register and does not 
touch the ACK_MODE bit. Keep in mind that mxs has separate registers 
from write and read to same register.

> find out the motivation to set RETAIN_CLOCK at all in the select
> command.

I tried to not set RETAIN_CLOCK bit and the sgtl5000 codec failed to probe.

Regards,

Fabio Estevam