[PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Hans de Goede]

According to the datasheets to n factor for dividing the tclk is
2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
on other mv64xxx implementations.

I've contacted Allwinner about this and they have confirmed that the
datasheet is correct.

This commit fixes the clk-divider calculations for Allwinner SoCs
accordingly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 30059c1..e75cf6d 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -146,6 +146,8 @@ struct mv64xxx_i2c_data {
 	bool			errata_delay;
 	struct reset_control	*rstc;
 	bool			irq_clear_inverted;
+	/* Clk div is 2 to the power n, not 2 to the power n + 1 */
+	bool			clk_n_base_0;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -759,25 +761,29 @@ MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
 #ifdef CONFIG_OF
 #ifdef CONFIG_HAVE_CLK
 static int
-mv64xxx_calc_freq(const int tclk, const int n, const int m)
+mv64xxx_calc_freq(struct mv64xxx_i2c_data *drv_data,
+		  const int tclk, const int n, const int m)
 {
-	return tclk / (10 * (m + 1) * (2 << n));
+	if (drv_data->clk_n_base_0)
+		return tclk / (10 * (m + 1) * (1 << n));
+	else
+		return tclk / (10 * (m + 1) * (2 << n));
 }
 
 static bool
-mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n,
-			  u32 *best_m)
+mv64xxx_find_baud_factors(struct mv64xxx_i2c_data *drv_data,
+			  const u32 req_freq, const u32 tclk)
 {
 	int freq, delta, best_delta = INT_MAX;
 	int m, n;
 
 	for (n = 0; n <= 7; n++)
 		for (m = 0; m <= 15; m++) {
-			freq = mv64xxx_calc_freq(tclk, n, m);
+			freq = mv64xxx_calc_freq(drv_data, tclk, n, m);
 			delta = req_freq - freq;
 			if (delta >= 0 && delta < best_delta) {
-				*best_m = m;
-				*best_n = n;
+				drv_data->freq_m = m;
+				drv_data->freq_n = n;
 				best_delta = delta;
 			}
 			if (best_delta == 0)
@@ -815,8 +821,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	if (of_property_read_u32(np, "clock-frequency", &bus_freq))
 		bus_freq = 100000; /* 100kHz by default */
 
-	if (!mv64xxx_find_baud_factors(bus_freq, tclk,
-				       &drv_data->freq_n, &drv_data->freq_m)) {
+	if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
+	    of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
+		drv_data->clk_n_base_0 = true;
+
+	if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) {
 		rc = -EINVAL;
 		goto out;
 	}
-- 
2.5.0

Re: [linux-sunxi] [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Olliver Schinagl]

Hey Hans,

On 27-09-15 16:57, Hans de Goede wrote:
> According to the datasheets to n factor for dividing the tclk is
> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> on other mv64xxx implementations.
Ah!
>
> I've contacted Allwinner about this and they have confirmed that the
> datasheet is correct.
>
> This commit fixes the clk-divider calculations for Allwinner SoCs
> accordingly.
So this explains why all my i2c frequenties are double of what I setup. 
Thanks for taking the time of figuring it out! I'll give it a test 
hopefully soon.

Olliver
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 30059c1..e75cf6d 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -146,6 +146,8 @@ struct mv64xxx_i2c_data {
>   	bool			errata_delay;
>   	struct reset_control	*rstc;
>   	bool			irq_clear_inverted;
> +	/* Clk div is 2 to the power n, not 2 to the power n + 1 */
> +	bool			clk_n_base_0;
>   };
>   
>   static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> @@ -759,25 +761,29 @@ MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
>   #ifdef CONFIG_OF
>   #ifdef CONFIG_HAVE_CLK
>   static int
> -mv64xxx_calc_freq(const int tclk, const int n, const int m)
> +mv64xxx_calc_freq(struct mv64xxx_i2c_data *drv_data,
> +		  const int tclk, const int n, const int m)
>   {
> -	return tclk / (10 * (m + 1) * (2 << n));
> +	if (drv_data->clk_n_base_0)
> +		return tclk / (10 * (m + 1) * (1 << n));
> +	else
> +		return tclk / (10 * (m + 1) * (2 << n));
>   }
>   
>   static bool
> -mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n,
> -			  u32 *best_m)
> +mv64xxx_find_baud_factors(struct mv64xxx_i2c_data *drv_data,
> +			  const u32 req_freq, const u32 tclk)
>   {
>   	int freq, delta, best_delta = INT_MAX;
>   	int m, n;
>   
>   	for (n = 0; n <= 7; n++)
>   		for (m = 0; m <= 15; m++) {
> -			freq = mv64xxx_calc_freq(tclk, n, m);
> +			freq = mv64xxx_calc_freq(drv_data, tclk, n, m);
>   			delta = req_freq - freq;
>   			if (delta >= 0 && delta < best_delta) {
> -				*best_m = m;
> -				*best_n = n;
> +				drv_data->freq_m = m;
> +				drv_data->freq_n = n;
>   				best_delta = delta;
>   			}
>   			if (best_delta == 0)
> @@ -815,8 +821,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>   	if (of_property_read_u32(np, "clock-frequency", &bus_freq))
>   		bus_freq = 100000; /* 100kHz by default */
>   
> -	if (!mv64xxx_find_baud_factors(bus_freq, tclk,
> -				       &drv_data->freq_n, &drv_data->freq_m)) {
> +	if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
> +	    of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
> +		drv_data->clk_n_base_0 = true;
> +
> +	if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) {
>   		rc = -EINVAL;
>   		goto out;
>   	}

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Andrew Lunn]

On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote:
> Hey Hans,
> 
> On 27-09-15 16:57, Hans de Goede wrote:
> >According to the datasheets to n factor for dividing the tclk is
> >2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> >on other mv64xxx implementations.
> Ah!

Just to be sure, i checked Kirkwood, Armada XP and Armada 370
datasheets. They all say n+1.

> >+	if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
> >+	    of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))

Rather than have to extend this list every so often, how about adding
a helper of_device_is_compatible_vendor(), so you can just have:

> >+	if (of_device_is_compatible_vendor(np, "allwinner")

  Andrew

Re: [linux-sunxi] [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Hans de Goede]

Hi,

On 27-09-15 18:53, Andrew Lunn wrote:
> On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote:
>> Hey Hans,
>>
>> On 27-09-15 16:57, Hans de Goede wrote:
>>> According to the datasheets to n factor for dividing the tclk is
>>> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
>>> on other mv64xxx implementations.
>> Ah!
>
> Just to be sure, i checked Kirkwood, Armada XP and Armada 370
> datasheets. They all say n+1.

Thanks.

>>> +	if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
>>> +	    of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
>
> Rather than have to extend this list every so often, how about adding
> a helper of_device_is_compatible_vendor(), so you can just have:
>
>>> +	if (of_device_is_compatible_vendor(np, "allwinner")

I agree that if such a helper would already exist it would be a good
idea to use it, but it seems overkill to just at it for this.

Regards,

Hans

Re: [linux-sunxi] [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Maxime Ripard]

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

On Sun, Sep 27, 2015 at 06:53:03PM +0200, Andrew Lunn wrote:
> > >+	if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
> > >+	    of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
> 
> Rather than have to extend this list every so often, how about adding
> a helper of_device_is_compatible_vendor(), so you can just have:

I don't know, I kind of like the fact that it's explicit. If we ever
have another SoC coming in with a different behaviour, we won't have
to expand it back.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Maxime Ripard]

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

On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote:
> Hey Hans,
> 
> On 27-09-15 16:57, Hans de Goede wrote:
> >According to the datasheets to n factor for dividing the tclk is
> >2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> >on other mv64xxx implementations.
> Ah!
> >
> >I've contacted Allwinner about this and they have confirmed that the
> >datasheet is correct.
> >
> >This commit fixes the clk-divider calculations for Allwinner SoCs
> >accordingly.
> So this explains why all my i2c frequenties are double of what I setup.
> Thanks for taking the time of figuring it out! I'll give it a test hopefully
> soon.

It would have been great to let us know...

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Olliver Schinagl]

Hey Maxime,

On 29-09-15 14:09, Maxime Ripard wrote:
> On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote:
>> Hey Hans,
>>
>> On 27-09-15 16:57, Hans de Goede wrote:
>>> According to the datasheets to n factor for dividing the tclk is
>>> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
>>> on other mv64xxx implementations.
>> Ah!
>>> I've contacted Allwinner about this and they have confirmed that the
>>> datasheet is correct.
>>>
>>> This commit fixes the clk-divider calculations for Allwinner SoCs
>>> accordingly.
>> So this explains why all my i2c frequenties are double of what I setup.
>> Thanks for taking the time of figuring it out! I'll give it a test hopefully
>> soon.
> It would have been great to let us know...
If your talking about past tence, I actually did ;) [0][1] and I 
actually had a tree made ready just 3 weeks ago with 3.15 to start my 
bisection with. It seemed logical to see if it worked there as that was 
the first iteration (based on the sunxi/allwinner based tree before the 
mv migration.

Anyway, I even built tried to build the kernel! but my gcc failed to 
build it so i put it on the back-burner for a while.

WIth Hans having figured it out and fixing it, I'll absolutly will take 
a nother look and check with a scope if it all works out now.

Olliver

[0] http://irclog.whitequark.org/linux-sunxi/2015-01-16#11522114;
[1] http://s24.postimg.org/yiykh6kkl/DS1_Z_Quick_Print2.png


> Maxime
>

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Wolfram Sang]

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


> WIth Hans having figured it out and fixing it, I'll absolutly will take a
> nother look and check with a scope if it all works out now.

Have you done this already? /me is always looking for Tested-by: tags :)

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Olliver Schinagl]

I shamefully admit I have not. My plate is very full at the moment, but 
I will make room for this upcoming weekend as I have a few other patches 
to test aswell. Sorry for the delay!

On 20-10-15 18:58, Wolfram Sang wrote:
>> WIth Hans having figured it out and fixing it, I'll absolutly will take a
>> nother look and check with a scope if it all works out now.
> Have you done this already? /me is always looking for Tested-by: tags :)
>

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Olliver Schinagl]

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

Hey Wolfram,

On 20-10-15 17:58, Wolfram Sang wrote:
>> WIth Hans having figured it out and fixing it, I'll absolutly will take a
>> nother look and check with a scope if it all works out now.
> Have you done this already? /me is always looking for Tested-by: tags :)
And here you have it.

Tested-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

I attached the scope traces for a 200.000 Hz i2c0 and a 400.000 Hz i2c1 
(without anything connected to the bus) on a cubieboard2.

Olliver

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: 200k.png --]
[-- Type: image/png, Size: 6542 bytes --]

[-- Attachment #3: 400k.png --]
[-- Type: image/png, Size: 6200 bytes --]

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Peter Korsgaard]

>>>>> "Hans" == Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

 > According to the datasheets to n factor for dividing the tclk is

s/to/the/

-- 
Bye, Peter Korsgaard

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Maxime Ripard]

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

On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote:
> According to the datasheets to n factor for dividing the tclk is
> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> on other mv64xxx implementations.
> 
> I've contacted Allwinner about this and they have confirmed that the
> datasheet is correct.
> 
> This commit fixes the clk-divider calculations for Allwinner SoCs
> accordingly.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Wolfram Sang]

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

On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote:
> According to the datasheets to n factor for dividing the tclk is
> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> on other mv64xxx implementations.
> 
> I've contacted Allwinner about this and they have confirmed that the
> datasheet is correct.
> 
> This commit fixes the clk-divider calculations for Allwinner SoCs
> accordingly.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-current, thanks!


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

Re: [PATCH] i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

[Wolfram Sang]

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

On Mon, Nov 30, 2015 at 02:51:41PM +0100, Wolfram Sang wrote:
> On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote:
> > According to the datasheets to n factor for dividing the tclk is
> > 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is
> > on other mv64xxx implementations.
> > 
> > I've contacted Allwinner about this and they have confirmed that the
> > datasheet is correct.
> > 
> > This commit fixes the clk-divider calculations for Allwinner SoCs
> > accordingly.
> > 
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Applied to for-current, thanks!

And added stable...

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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