Skip to contentSkip to conversation

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

PATCH New Hans de Goede 6 participants 14 messages Sep 27, 2015 – Nov 30, 2015

Subsystems (2)
I2C subsystem host drivers I2C MV64XXX Marvell and Allwinner driver
lore
Share & tools
lore

Hans de Goede

author
#0 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

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 <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +18 -9
index 30059c1..e75cf6d 100644
@@ -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;
 	}
Signature · 1 line
2.5.0
#1 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Hey Hans,

Quoted 3 lines · 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.

Ah!

Quoted 6 lines

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

Quoted 70 lines
Signed-off-by Hans de Goede <[email protected]>
---
  drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++---------
  1 file changed, 18 insertions(+), 9 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +18 -9
index 30059c1..e75cf6d 100644
@@ -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 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Quoted 7 lines · Olliver Schinagl

Hey Hans,

Quoted 3 lines · 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.

Ah!

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

Quoted 2 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 825

Quoted 2 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -0
+	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:

Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	if (of_device_is_compatible_vendor(np, "allwinner")

Andrew

author
#3 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Hi,

Quoted 11 lines · Andrew Lunn
Quoted 7 lines · Olliver Schinagl

Hey Hans,

Quoted 3 lines · 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.

Ah!

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

Thanks.

Quoted 7 lines
Quoted 2 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 825

Quoted 2 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -0
+	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:

Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	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

#4 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Andrew Lunn
Quoted 5 lines · Andrew Lunn
Quoted 2 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 825

Quoted 2 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -0
+	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

Signature · 10 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150929/0e430dea/attachment.sig>
#5 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Olliver Schinagl
Quoted 8 lines · Olliver Schinagl

Hey Hans,

Quoted 3 lines · 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.

Ah!

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

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

Signature · 10 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150929/97e805ec/attachment.sig>
#6 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Hey Maxime,

Quoted 11 lines · Maxime Ripard
Quoted 10 lines · Olliver Schinagl

Hey Hans,

Quoted 3 lines · 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.

Ah!

Quoted 3 lines

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

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines

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

Quoted 2 lines

Maxime

#7 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Quoted 3 lines

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 :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151020/19029a61/attachment.sig>

#8 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

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!

Quoted 4 lines · Wolfram Sang
Quoted 2 lines

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 :)

#9 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Wolfram Sang

Hey Wolfram,

Quoted 3 lines · Wolfram Sang
Quoted 2 lines

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 <[email protected]>

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 200k.png
Type: image/png
Size: 6542 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151025/5d211822/attachment.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 400k.png
Type: image/png
Size: 6200 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151025/5d211822/attachment-0001.png>

#10 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Hans de Goede
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Hans" == Hans de Goede <[email protected]> writes:

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

s/to/the/

Signature · 1 line
Bye, Peter Korsgaard
#11 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Hans de Goede
Quoted 11 lines · 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 <[email protected]>
Acked-by Maxime Ripard <[email protected]>

Thanks!
Maxime

Signature · 10 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150929/bca0915a/attachment.sig>
Applied
#12 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

In reply to Hans de Goede
Quoted 11 lines · 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 <[email protected]>

Applied to for-current, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151130/ed500ee0/attachment-0001.sig>

#13 lore raw

PATCH 1/1 i2c: mv64xxx: The n clockdiv factor is 0 based on sunxi SoCs

Quoted 8 lines · Wolfram Sang
Quoted 7 lines · 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.

Quoted 6 lines
Quoted 4 lines

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

Signed-off-by Hans de Goede <[email protected]>

Applied to for-current, thanks!

And added stable...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151130/b2c41b4d/attachment.sig>