[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Stefan Roese]

The current implementation for baudrate calculation is incorrect.
This part from the formula:

"2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!

This patch fixes this and moves this calculation to a function instead of using a macro.
This new function is taken from the Linux kernel.

This was detected and tested on the Marvell Armada A38x DB-88F6820-GP eval board.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Luka Perkov <luka.perkov@sartura.hr>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Ian Campbell <ijc@hellion.org.uk>
Cc: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/mvtwsi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
index 9b2ca1e..320235c 100644
--- a/drivers/i2c/mvtwsi.c
+++ b/drivers/i2c/mvtwsi.c
@@ -228,13 +228,10 @@ static int twsi_stop(int status)
 	return status;
 }
 
-/*
- * Ugly formula to convert m and n values to a frequency comes from
- * TWSI specifications
- */
-
-#define TWSI_FREQUENCY(m, n) \
-	(CONFIG_SYS_TCLK / (10 * (m + 1) * (1 << n)))
+static unsigned int twsi_calc_freq(const int n, const int m)
+{
+	return CONFIG_SYS_TCLK / (10 * (m + 1) * (2 << n));
+}
 
 /*
  * Reset controller.
@@ -266,7 +263,7 @@ static unsigned int twsi_i2c_set_bus_speed(struct i2c_adapter *adap,
 	/* compute m, n setting for highest speed not above requested speed */
 	for (n = 0; n < 8; n++) {
 		for (m = 0; m < 16; m++) {
-			tmp_speed = TWSI_FREQUENCY(m, n);
+			tmp_speed = twsi_calc_freq(n, m);
 			if ((tmp_speed <= requested_speed)
 			 && (tmp_speed > highest_speed)) {
 				highest_speed = tmp_speed;
-- 
2.3.3

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Hans de Goede]

Hi,

On 17-03-15 11:08, Stefan Roese wrote:
> The current implementation for baudrate calculation is incorrect.
> This part from the formula:
>
> "2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!
>
> This patch fixes this and moves this calculation to a function instead of using a macro.

Hmm, this does not match with what the Allwinner datasheets say:
https://github.com/allwinner-zh/documents/blob/master/A20/A20%20user%20manual%20v1.3%2020141010.pdf

They say:

Fsamp = F 0 = Fin / 2^CLK_N
F1 = F0 / (CLK_M + 1)
Foscl = F1 / 10 = Fin / (2^CLK_N * (CLK_M + 1)*10)

With Foscl being the ultimate i2c speed. Notice that they are talking about
2 ^ CLK_N not 2 ^ (CLK_N + 1)

And they have a few examples which match this. Now it could be that a
register value of 0 means CLK_N = 1, reg 1 CLK_N = 2, etc. this is not clearly
specified ...

> This new function is taken from the Linux kernel.

Interesting, because on Allwinnner / sunxi devices we are using the kernel
driver formula unmodified, and things seem to work fine despite this. Could
be tolerances allowing this, could be the Allwinner datasheet being unclear.

I've send allwinner a mail asking them to clarify this.

> This was detected and tested on the Marvell Armada A38x DB-88F6820-GP eval board.

So I take it you connected a memory oscilloscope to the i2c wires ?

Regards,

Hans



>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Ian Campbell <ijc@hellion.org.uk>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>   drivers/i2c/mvtwsi.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> index 9b2ca1e..320235c 100644
> --- a/drivers/i2c/mvtwsi.c
> +++ b/drivers/i2c/mvtwsi.c
> @@ -228,13 +228,10 @@ static int twsi_stop(int status)
>   	return status;
>   }
>
> -/*
> - * Ugly formula to convert m and n values to a frequency comes from
> - * TWSI specifications
> - */
> -
> -#define TWSI_FREQUENCY(m, n) \
> -	(CONFIG_SYS_TCLK / (10 * (m + 1) * (1 << n)))
> +static unsigned int twsi_calc_freq(const int n, const int m)
> +{
> +	return CONFIG_SYS_TCLK / (10 * (m + 1) * (2 << n));
> +}
>
>   /*
>    * Reset controller.
> @@ -266,7 +263,7 @@ static unsigned int twsi_i2c_set_bus_speed(struct i2c_adapter *adap,
>   	/* compute m, n setting for highest speed not above requested speed */
>   	for (n = 0; n < 8; n++) {
>   		for (m = 0; m < 16; m++) {
> -			tmp_speed = TWSI_FREQUENCY(m, n);
> +			tmp_speed = twsi_calc_freq(n, m);
>   			if ((tmp_speed <= requested_speed)
>   			 && (tmp_speed > highest_speed)) {
>   				highest_speed = tmp_speed;
>

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Stefan Roese]

Hi Hans,

On 17.03.2015 12:15, Hans de Goede wrote:
> On 17-03-15 11:08, Stefan Roese wrote:
>> The current implementation for baudrate calculation is incorrect.
>> This part from the formula:
>>
>> "2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!
>>
>> This patch fixes this and moves this calculation to a function instead
>> of using a macro.
>
> Hmm, this does not match with what the Allwinner datasheets say:
> https://github.com/allwinner-zh/documents/blob/master/A20/A20%20user%20manual%20v1.3%2020141010.pdf
>
>
> They say:
>
> Fsamp = F 0 = Fin / 2^CLK_N
> F1 = F0 / (CLK_M + 1)
> Foscl = F1 / 10 = Fin / (2^CLK_N * (CLK_M + 1)*10)
>
> With Foscl being the ultimate i2c speed. Notice that they are talking about
> 2 ^ CLK_N not 2 ^ (CLK_N + 1)
>
> And they have a few examples which match this. Now it could be that a
> register value of 0 means CLK_N = 1, reg 1 CLK_N = 2, etc. this is not
> clearly
> specified ...
>
>> This new function is taken from the Linux kernel.
>
> Interesting, because on Allwinnner / sunxi devices we are using the kernel
> driver formula unmodified, and things seem to work fine despite this. Could
> be tolerances allowing this, could be the Allwinner datasheet being
> unclear.
>
> I've send allwinner a mail asking them to clarify this.

Good.

Here the complete formula from the A38x manual:

F_SCL = F_TCLK / (10 * (M + 1) * 2 ^ (N + 1))

>> This was detected and tested on the Marvell Armada A38x DB-88F6820-GP
>> eval board.
>
> So I take it you connected a memory oscilloscope to the i2c wires ?

No. I noticed that I2C doesn't work on this board with the current code. 
And checked for differences to the Linux code. And with this change, I2C 
does work on this board.

Thanks,
Stefan

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Hans de Goede]

Hi,

On 17-03-15 12:25, Stefan Roese wrote:
> Hi Hans,
>
> On 17.03.2015 12:15, Hans de Goede wrote:
>> On 17-03-15 11:08, Stefan Roese wrote:
>>> The current implementation for baudrate calculation is incorrect.
>>> This part from the formula:
>>>
>>> "2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!
>>>
>>> This patch fixes this and moves this calculation to a function instead
>>> of using a macro.
>>
>> Hmm, this does not match with what the Allwinner datasheets say:
>> https://github.com/allwinner-zh/documents/blob/master/A20/A20%20user%20manual%20v1.3%2020141010.pdf
>>
>>
>> They say:
>>
>> Fsamp = F 0 = Fin / 2^CLK_N
>> F1 = F0 / (CLK_M + 1)
>> Foscl = F1 / 10 = Fin / (2^CLK_N * (CLK_M + 1)*10)
>>
>> With Foscl being the ultimate i2c speed. Notice that they are talking about
>> 2 ^ CLK_N not 2 ^ (CLK_N + 1)
>>
>> And they have a few examples which match this. Now it could be that a
>> register value of 0 means CLK_N = 1, reg 1 CLK_N = 2, etc. this is not
>> clearly
>> specified ...
>>
>>> This new function is taken from the Linux kernel.
>>
>> Interesting, because on Allwinnner / sunxi devices we are using the kernel
>> driver formula unmodified, and things seem to work fine despite this. Could
>> be tolerances allowing this, could be the Allwinner datasheet being
>> unclear.
>>
>> I've send allwinner a mail asking them to clarify this.
>
> Good.
>
> Here the complete formula from the A38x manual:
>
> F_SCL = F_TCLK / (10 * (M + 1) * 2 ^ (N + 1))
>
>>> This was detected and tested on the Marvell Armada A38x DB-88F6820-GP
>>> eval board.
>>
>> So I take it you connected a memory oscilloscope to the i2c wires ?
>
> No. I noticed that I2C doesn't work on this board with the current code. And checked for differences to the Linux code. And with this change, I2C does work on this board.

Ok (note neither have I measured the actual speed on Allwinner devices),
lets wait a bit to see what Allwinner has to say.

Regards,

Hans

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Hans de Goede]

Hi,

On 17-03-15 12:15, Hans de Goede wrote:
> Hi,
>
> On 17-03-15 11:08, Stefan Roese wrote:
>> The current implementation for baudrate calculation is incorrect.
>> This part from the formula:
>>
>> "2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!
>>
>> This patch fixes this and moves this calculation to a function instead of using a macro.
>
> Hmm, this does not match with what the Allwinner datasheets say:
> https://github.com/allwinner-zh/documents/blob/master/A20/A20%20user%20manual%20v1.3%2020141010.pdf
>
> They say:
>
> Fsamp = F 0 = Fin / 2^CLK_N
> F1 = F0 / (CLK_M + 1)
> Foscl = F1 / 10 = Fin / (2^CLK_N * (CLK_M + 1)*10)
>
> With Foscl being the ultimate i2c speed. Notice that they are talking about
> 2 ^ CLK_N not 2 ^ (CLK_N + 1)
>
> And they have a few examples which match this. Now it could be that a
> register value of 0 means CLK_N = 1, reg 1 CLK_N = 2, etc. this is not clearly
> specified ...
>
>> This new function is taken from the Linux kernel.
>
> Interesting, because on Allwinnner / sunxi devices we are using the kernel
> driver formula unmodified, and things seem to work fine despite this. Could
> be tolerances allowing this, could be the Allwinner datasheet being unclear.
>
> I've send allwinner a mail asking them to clarify this.

I've just gotten an answer from allwinner, and the formula in their datasheet
is correct, with the raw register value == CLK_N so the current (before this patch)
u-boot code is correct for Allwinner devices and we need a #ifdef here to keep the
old formula for sunxi devices, you can use:

#ifdef CONFIG_SUNXI

For this.

Regards,

Hans

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Hans de Goede]

Hi,

On 18-03-15 09:20, Hans de Goede wrote:
> Hi,
>
> On 17-03-15 12:15, Hans de Goede wrote:
>> Hi,
>>
>> On 17-03-15 11:08, Stefan Roese wrote:
>>> The current implementation for baudrate calculation is incorrect.
>>> This part from the formula:
>>>
>>> "2 ^ (n + 1)" is not equivalent to (1 << n) but to (2 << n)!
>>>
>>> This patch fixes this and moves this calculation to a function instead of using a macro.
>>
>> Hmm, this does not match with what the Allwinner datasheets say:
>> https://github.com/allwinner-zh/documents/blob/master/A20/A20%20user%20manual%20v1.3%2020141010.pdf
>>
>> They say:
>>
>> Fsamp = F 0 = Fin / 2^CLK_N
>> F1 = F0 / (CLK_M + 1)
>> Foscl = F1 / 10 = Fin / (2^CLK_N * (CLK_M + 1)*10)
>>
>> With Foscl being the ultimate i2c speed. Notice that they are talking about
>> 2 ^ CLK_N not 2 ^ (CLK_N + 1)
>>
>> And they have a few examples which match this. Now it could be that a
>> register value of 0 means CLK_N = 1, reg 1 CLK_N = 2, etc. this is not clearly
>> specified ...
>>
>>> This new function is taken from the Linux kernel.
>>
>> Interesting, because on Allwinnner / sunxi devices we are using the kernel
>> driver formula unmodified, and things seem to work fine despite this. Could
>> be tolerances allowing this, could be the Allwinner datasheet being unclear.
>>
>> I've send allwinner a mail asking them to clarify this.
>
> I've just gotten an answer from allwinner, and the formula in their datasheet
> is correct, with the raw register value == CLK_N so the current (before this patch)
> u-boot code is correct for Allwinner devices and we need a #ifdef here to keep the
> old formula for sunxi devices, you can use:

p.s.

This means the kernel currently does the wrong thing on Allwinner, I've put fixing
this on my todo list.

Regards,

Hans

[U-Boot] [PATCH] i2c: mvtwsi: Fix problem with baud rate calculation

[Stefan Roese]

Hi Hans,

On 18.03.2015 09:20, Hans de Goede wrote:

<snip>

>> I've send allwinner a mail asking them to clarify this.
>
> I've just gotten an answer from allwinner, and the formula in their
> datasheet
> is correct, with the raw register value == CLK_N so the current (before
> this patch)
> u-boot code is correct for Allwinner devices and we need a #ifdef here
> to keep the
> old formula for sunxi devices, you can use:
>
> #ifdef CONFIG_SUNXI
>
> For this.

Okay, will send v2 right away...

Thanks,
Stefan