[PATCH] i2c: davinci: Increase module clock frequency

[PATCH] i2c: davinci: Increase module clock frequency

[Alexander Sverdlin]

I2C controller used in Keystone SoC has an undocumented peculiarity which
results in SDA-SCL margins being dependent on module clock. Driving high
capacity bus near its limits can result in STOP condition sometimes being
understood as REPEATED-START by slaves (or NACK instead of ACK, etc...).
Driving the module with higher clocks increases the margin between SDA and SCL
transitions, making the operations with higher bus rates more robust. Therefore,
target the module clock to 12MHz instead of 7MHz, still staying within
the specification limits.

Before the change STOP timing looked like this on 400kHz:

SDA   ----------+          +----
                 \        /
                  \      /
                   +----+
                       (1)
SCL   --+          +------------
         \        /
          \      /
           +----+
               (2)

While only point (1) signals STOP, point (2) could be incorrectly recognized as
repeated-START (almost no margin between SDA and SCL transitions).

After the change there is at least 600ns margin measured between SCL fall and
SDA fall during STOP generation:

SDA   ------+          +----
             \        /
              \      /
               +----+

SCL   --+          +--------
         \        /
          \      /
           +----+
           ->|    |<- 600ns
                ->|   |<- tSUSTO

So called tSUSTO (setup time for STOP condition) is still slightly higher than
600ns, so no problem here.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/i2c/busses/i2c-davinci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c5628a4..87c46a4 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -203,7 +203,7 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 	 */
 
 	/* get minimum of 7 MHz clock, but max of 12 MHz */
-	psc = (input_clock / 7000000) - 1;
+	psc = (input_clock / 12000000) - 1;
 	if ((input_clock / (psc + 1)) > 12000000)
 		psc++;	/* better to run under spec than over */
 	d = (psc >= 2) ? 5 : 7 - psc;

Re: [PATCH] i2c: davinci: Increase module clock frequency

[santosh shilimkar]

On 11/19/2015 4:21 AM, Alexander Sverdlin wrote:
> I2C controller used in Keystone SoC has an undocumented peculiarity which
> results in SDA-SCL margins being dependent on module clock. Driving high
> capacity bus near its limits can result in STOP condition sometimes being
> understood as REPEATED-START by slaves (or NACK instead of ACK, etc...).
> Driving the module with higher clocks increases the margin between SDA and SCL
> transitions, making the operations with higher bus rates more robust. Therefore,
> target the module clock to 12MHz instead of 7MHz, still staying within
> the specification limits.
>
> Before the change STOP timing looked like this on 400kHz:
>
> SDA   ----------+          +----
>                   \        /
>                    \      /
>                     +----+
>                         (1)
> SCL   --+          +------------
>           \        /
>            \      /
>             +----+
>                 (2)
>
> While only point (1) signals STOP, point (2) could be incorrectly recognized as
> repeated-START (almost no margin between SDA and SCL transitions).
>
> After the change there is at least 600ns margin measured between SCL fall and
> SDA fall during STOP generation:
>
> SDA   ------+          +----
>               \        /
>                \      /
>                 +----+
>
> SCL   --+          +--------
>           \        /
>            \      /
>             +----+
>             ->|    |<- 600ns
>                  ->|   |<- tSUSTO
>
> So called tSUSTO (setup time for STOP condition) is still slightly higher than
> 600ns, so no problem here.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
Nice text artwork.
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

Re: [PATCH] i2c: davinci: Increase module clock frequency

[Wolfram Sang]

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

>  	/* get minimum of 7 MHz clock, but max of 12 MHz */
> -	psc = (input_clock / 7000000) - 1;
> +	psc = (input_clock / 12000000) - 1;

Doesn't make this the above comment invalid?


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

Re: [PATCH] i2c: davinci: Increase module clock frequency

[Alexander Sverdlin]

Hi!

On 30.11.2015 14:55, EXT Wolfram Sang wrote:
>>  	/* get minimum of 7 MHz clock, but max of 12 MHz */
>> > -	psc = (input_clock / 7000000) - 1;
>> > +	psc = (input_clock / 12000000) - 1;
> Doesn't make this the above comment invalid?

The comment refers to datasheet, not really to the code. And eventual changes to the datasheet
that's what can make it invalid (though I don't know TI's plans on it). Nevertheless, yes, I
think, it's better to drop the comment. Should I re-spin the patch with comment removal in it? 

-- 
Best regards,
Alexander Sverdlin.
Sent from my pdp-11

Re: [PATCH] i2c: davinci: Increase module clock frequency

[Wolfram Sang]

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

> The comment refers to datasheet, not really to the code. And eventual changes to the datasheet
> that's what can make it invalid (though I don't know TI's plans on it). Nevertheless, yes, I
> think, it's better to drop the comment. Should I re-spin the patch with comment removal in it? 

Thanks. Even better would be replacing the comment: can you give a short
explanation why there is now 12MHz and what needs to be considered if
the value is to be changed again?


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

[PATCH v2] i2c: davinci: Increase module clock frequency

[Alexander Sverdlin]

I2C controller used in Keystone SoC has an undocumented peculiarity which
results in SDA-SCL margins being dependent on module clock. Driving high
capacity bus near its limits can result in STOP condition sometimes being
understood as REPEATED-START by slaves (or NACK instead of ACK, etc...).
Driving the module with higher clocks increases the margin between SDA and SCL
transitions, making the operations with higher bus rates more robust. Therefore,
target the module clock to 12MHz instead of 7MHz, still staying within
the specification limits.

Before the change STOP timing looked like this on 400kHz:

SDA   ----------+          +----
                 \        /
                  \      /
                   +----+
                       (1)
SCL   --+          +------------
         \        /
          \      /
           +----+
               (2)

While only point (1) signals STOP, point (2) could be incorrectly recognized as
repeated-START (almost no margin between SDA and SCL transitions).

After the change there is at least 600ns margin measured between SCL fall and
SDA fall during STOP generation:

SDA   ------+          +----
             \        /
              \      /
               +----+

SCL   --+          +--------
         \        /
          \      /
           +----+
           ->|    |<- 600ns
                ->|   |<- tSUSTO

So called tSUSTO (setup time for STOP condition) is still slightly higher than
600ns, so no problem here.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
---

Changes in v2:
- Replaced old comment referencing to datasheet.

 drivers/i2c/busses/i2c-davinci.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c5628a4..a8bdcb5 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -202,8 +202,15 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 	 * d is always 6 on Keystone I2C controller
 	 */
 
-	/* get minimum of 7 MHz clock, but max of 12 MHz */
-	psc = (input_clock / 7000000) - 1;
+	/*
+	 * Both Davinci and current Keystone User Guides recommend a value
+	 * between 7MHz and 12MHz. In reality 7MHz module clock doesn't
+	 * always produce enough margin between SDA and SCL transitions.
+	 * Measurements show that the higher the module clock is, the
+	 * bigger is the margin, providing more reliable communication.
+	 * So we better target for 12MHz.
+	 */
+	psc = (input_clock / 12000000) - 1;
 	if ((input_clock / (psc + 1)) > 12000000)
 		psc++;	/* better to run under spec than over */
 	d = (psc >= 2) ? 5 : 7 - psc;

Re: [PATCH v2] i2c: davinci: Increase module clock frequency

[Wolfram Sang]

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

On Mon, Nov 30, 2015 at 03:51:00PM +0100, Alexander Sverdlin wrote:
> I2C controller used in Keystone SoC has an undocumented peculiarity which
> results in SDA-SCL margins being dependent on module clock. Driving high
> capacity bus near its limits can result in STOP condition sometimes being
> understood as REPEATED-START by slaves (or NACK instead of ACK, etc...).
> Driving the module with higher clocks increases the margin between SDA and SCL
> transitions, making the operations with higher bus rates more robust. Therefore,
> target the module clock to 12MHz instead of 7MHz, still staying within
> the specification limits.
> 
> Before the change STOP timing looked like this on 400kHz:
> 

Applied to for-current, thanks!


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

Re: [PATCH v2] i2c: davinci: Increase module clock frequency

[Wolfram Sang]

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

On Mon, Nov 30, 2015 at 03:51:00PM +0100, Alexander Sverdlin wrote:
> I2C controller used in Keystone SoC has an undocumented peculiarity which
> results in SDA-SCL margins being dependent on module clock. Driving high
> capacity bus near its limits can result in STOP condition sometimes being
> understood as REPEATED-START by slaves (or NACK instead of ACK, etc...).
> Driving the module with higher clocks increases the margin between SDA and SCL
> transitions, making the operations with higher bus rates more robust. Therefore,
> target the module clock to 12MHz instead of 7MHz, still staying within
> the specification limits.
> 
> Before the change STOP timing looked like this on 400kHz:
> 

Applied to for-current and added stable, thanks!


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