[PATCH] i2c: i2c_mxs: Set ACK_MODE bit

[PATCH] i2c: i2c_mxs: Set ACK_MODE bit

[Fabio Estevam]

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>
---
 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);
 }
 
 static void mxs_i2c_dma_finish(struct mxs_i2c_dev *i2c)
-- 
1.8.1.2

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

[Uwe Kleine-König]

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? 

> ---
>  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
find out the motivation to set RETAIN_CLOCK at all in the select
command.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Fabio Estevam]

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

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

[Marek Vasut]

Dear Fabio Estevam,

> 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.

How/when does this error manifest on the scope/LA?

Best regards,
Marek Vasut

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

[Christoph G. Baumann]

Hello Marek,

> > 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.
>
> How/when does this error manifest on the scope/LA?

the problem turned up when Uwe Kleine-König worked on implementing SMBus and
I2C_M_RECV_LEN support for i.MX28 (my employer commissioned Pengutronix in that
case). The receiving of such messages stops after the first (length information)
byte.
Maybe Uwe can comment on that.



Regards
Christoph

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

[Uwe Kleine-König]

Hello,

On Wed, Jul 03, 2013 at 03:34:12PM +0200, Christoph G. Baumann wrote:
> > > 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.
> >
> > How/when does this error manifest on the scope/LA?
> 
> the problem turned up when Uwe Kleine-König worked on implementing SMBus and
> I2C_M_RECV_LEN support for i.MX28 (my employer commissioned Pengutronix in that
> case). The receiving of such messages stops after the first (length information)
> byte.
> Maybe Uwe can comment on that.
OK, I'm trying to sum up: To support I2C_M_RECV_LEN in the mxs driver I
did:

     1          // prepare sending SAD+R
     2          CTRL0 = RUN | RETAIN_CLOCK | PRE_SEND_START | MASTER_MODE | DIRECTION | XFER_COUNT(1);
     3          poll DEBUG0 until DMAREQ is set;
     4          // send SAD+R
     5          DATA = addr_data
     6          // prepare reading length byte
     7          CTRL0 = RUN | RETAIN_CLOCK | MASTER_MODE | XFER_COUNT(1);
     8          poll DEBUG0 until DMAREQ is set;
     9          // read first data indicating length
    10          data = DATA & 0xff
    11          // send an ack, i.e. clean CTRL0_CLOCK_HELD
    12          CTRL0 = RUN | ACKNOWLEDGE | MASTER_MODE
    13          sleep 1ms
    14          // trigger reading rest of the message
    15          CTRL0 = RUN | SEND_NAK_ON_LAST | MASTER_MODE | MXS_I2C_CTRL0_POST_SEND_STOP
    16          for (i = 0; i < (data + 3) / 4; ++i)
    17                  read from DATA

In line 10 the length bit is read out successfully, but sending the ACK
in line 12 doesn't work, the i.MX28 pulls SDA low, but doesn't generate
a clock pulse on SCL and instead releases SDA and starts reading the
following byte.

Dropping RETAIN_CLOCK in line 7 and/or dropping RUN from line 12 didn't
help.

Freescale's support suggested to set CTRL1.ACK_MODE and some other
things that didn't help and pointed out the imx23 i2c errata. (I.e.
"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.")

The suggested workaround was to either use a Big buffer, read Many bytes
until the slave sends a NACK and interpret the result as smaller read.
Or use gpio bit banging.

I didn't understand the suggested workaround in the errata document, and
the support guy didn't succeed to explain it to me. "The suggested
workaround in this errata is to set the ACK_MODE bit in HW_I2C_CTRL1
register. In i.MX233, this bit is default zero and requires software to
explicitly set it to 1. In i.MX28, this bit is '1' by default already.
Unfortunately, this does not solve the 9th clock pulse issue if
RETAIN_CLOCK is set in the receiving data phase."

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Marek Vasut]

Dear Uwe Kleine-König,

> Hello,
> 
> On Wed, Jul 03, 2013 at 03:34:12PM +0200, Christoph G. Baumann wrote:
> > > > 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.
> > > 
> > > How/when does this error manifest on the scope/LA?
> > 
> > the problem turned up when Uwe Kleine-König worked on implementing SMBus
> > and I2C_M_RECV_LEN support for i.MX28 (my employer commissioned
> > Pengutronix in that case). The receiving of such messages stops after
> > the first (length information) byte.
> > Maybe Uwe can comment on that.
> 
> OK, I'm trying to sum up: To support I2C_M_RECV_LEN in the mxs driver I
> did:

See the other patch I sent , esp. the write PIO command [1], then the order 
would be:

      1          // prepare sending SAD+R
      2          CTRL0 = RETAIN_CLOCK | PRE_SEND_START | MASTER_MODE |
                         DIRECTION | XFER_COUNT(1);
      3          DATA = addr_data
      4          CTRL0 |= RUN

^ this stuff is explained in MX23 RM, see the comment above 
mxs_i2c_pio_trigger_write_cmd() in the patch.

>      6          // prepare reading length byte
>      7          CTRL0 = RUN | RETAIN_CLOCK | MASTER_MODE | XFER_COUNT(1);

I think you can force the controller to send ACK here automatically.

>      8          poll DEBUG0 until DMAREQ is set;

DMAREQ doesn't work in READ xfer :-(

>      9          // read first data indicating length
>     10          data = DATA & 0xff
>     11          // send an ack, i.e. clean CTRL0_CLOCK_HELD
>     12          CTRL0 = RUN | ACKNOWLEDGE | MASTER_MODE
>     13          sleep 1ms

See above, also don't use RETAIN_CLOCK above then.

>     14          // trigger reading rest of the message
>     15          CTRL0 = RUN | SEND_NAK_ON_LAST | MASTER_MODE |
> MXS_I2C_CTRL0_POST_SEND_STOP 16          for (i = 0; i < (data + 3) / 4;
> ++i)
>     17                  read from DATA

Use DMA here, you can't PIO READ more than 4 bytes.

> In line 10 the length bit is read out successfully, but sending the ACK
> in line 12 doesn't work, the i.MX28 pulls SDA low, but doesn't generate
> a clock pulse on SCL and instead releases SDA and starts reading the
> following byte.
> 
> Dropping RETAIN_CLOCK in line 7 and/or dropping RUN from line 12 didn't
> help.
> 
> Freescale's support suggested to set CTRL1.ACK_MODE and some other
> things that didn't help and pointed out the imx23 i2c errata. (I.e.
> "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.")
> 
> The suggested workaround was to either use a Big buffer, read Many bytes
> until the slave sends a NACK and interpret the result as smaller read.
> Or use gpio bit banging.

I suspect is likely doable, but it'd need non-trivial amount of fiddling. Unless 
I get motivated enough to implement this, I'm not plumbing in it. Rather than 
that, I'd like to find out what's wrong with the PIO on MX23.

> I didn't understand the suggested workaround in the errata document, and
> the support guy didn't succeed to explain it to me. "The suggested
> workaround in this errata is to set the ACK_MODE bit in HW_I2C_CTRL1
> register. In i.MX233, this bit is default zero and requires software to
> explicitly set it to 1. In i.MX28, this bit is '1' by default already.
> Unfortunately, this does not solve the 9th clock pulse issue if
> RETAIN_CLOCK is set in the receiving data phase."
> 
> Best regards
> Uwe

[1] http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12699.html

Best regards,
Marek Vasut

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

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>

What's with this one? Bogus? Needed? Still needed after PIO rework?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Marek Vasut]

Dear Wolfram Sang,

> 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>
> 
> What's with this one? Bogus? Needed? Still needed after PIO rework?

I dont think this is needed on MX28 as this is set by default. On MX23, I dont 
see any difference with this enabled/disabled.

Best regards,
Marek Vasut

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

[Fabio Estevam]

On 08/15/2013 07:08 AM, Wolfram Sang 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>
>
> What's with this one? Bogus? Needed? Still needed after PIO rework?

According to the mx23 erratum 2727 this patch is needed.

Regards,

Fabio Estevam

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

[Uwe Kleine-König]

Hello,

On Mon, Aug 19, 2013 at 09:19:01AM -0300, Fabio Estevam wrote:
> On 08/15/2013 07:08 AM, Wolfram Sang 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>
> >
> >What's with this one? Bogus? Needed? Still needed after PIO rework?
> 
> According to the mx23 erratum 2727 this patch is needed.
Last time I worked with i2c stuff on mxs my impression was the mx23
erratum 2727 is non-sense. Having HW_I2C_CTRL1[ACK_MODE] set or not
didn't made any difference in my tests.

I suspect that the description isn't complete. So the driver does
already use RETAIN_CLOCK (for the select phase) which up to know didn't
made any problems on mx23 even without having ACK_MODE=1.

The statement about the HW_I2C_VERSION register appears just misplaced
there.

So I'd vote for not taking this patch until there is a better
understanding of the eventual problem.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 93 bytes --]


> According to the mx23 erratum 2727 this patch is needed.

Have you measured it yourself?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Fabio Estevam]

On Tue, Aug 20, 2013 at 4:04 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> According to the mx23 erratum 2727 this patch is needed.
>
> Have you measured it yourself?

No, only relying on the mx23 errata doc here.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]


> > Have you measured it yourself?
> 
> No, only relying on the mx23 errata doc here.

Only send patches when you KNOW what you are changing, i.e. you verified
to the best of your knowledge. Really! Docs have so many bugs...

Honestly, I hate dealing with patches which are more or less guesses.
They offload the Q&A to me and I surely have enough own tasks to do...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Fabio Estevam]

On Tue, Aug 20, 2013 at 4:20 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> > Have you measured it yourself?
>>
>> No, only relying on the mx23 errata doc here.
>
> Only send patches when you KNOW what you are changing, i.e. you verified
> to the best of your knowledge. Really! Docs have so many bugs...
>
> Honestly, I hate dealing with patches which are more or less guesses.
> They offload the Q&A to me and I surely have enough own tasks to do...

Ok. sir. Will investigate more about this erratum entry.

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

[Marek Vasut]

Dear Uwe Kleine-König,

> Hello,
> 
> On Mon, Aug 19, 2013 at 09:19:01AM -0300, Fabio Estevam wrote:
> > On 08/15/2013 07:08 AM, Wolfram Sang 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>
> > >
> > >What's with this one? Bogus? Needed? Still needed after PIO rework?
> > 
> > According to the mx23 erratum 2727 this patch is needed.
> 
> Last time I worked with i2c stuff on mxs my impression was the mx23
> erratum 2727 is non-sense. Having HW_I2C_CTRL1[ACK_MODE] set or not
> didn't made any difference in my tests.

Yeah, I can confirm this one.

> I suspect that the description isn't complete. So the driver does
> already use RETAIN_CLOCK (for the select phase) which up to know didn't
> made any problems on mx23 even without having ACK_MODE=1.
> 
> The statement about the HW_I2C_VERSION register appears just misplaced
> there.
> 
> So I'd vote for not taking this patch until there is a better
> understanding of the eventual problem.

I was doing some pretty thorough test when doing the PIO rework and watched the 
bus with LA, but didn't notice any difference whether the bit was or wasnt set. 
So either way WFM. On MX28 though, the bit is set after the I2C block is reset, 
on MX23 it is unset, that's the only thing I observed.

Best regards,
Marek Vasut