[PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[Svenning Sørensen]

According to I2C spec, max SCL rate is 100 kHz, but SCx200/CS5536
controller is currently driving it at 214 kHz according to my math.

SCL is derived from an input clock of 48 MHz, which must be divided
by 480 (240 cycles for each SCL high/low state) to be within spec.

Signed-off-by: Svenning Soerensen <sss-sijd2F9aCfdBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/busses/scx200_acb.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/scx200_acb.c b/drivers/i2c/busses/scx200_acb.c
index 0a7e410..4def02d 100644
--- a/drivers/i2c/busses/scx200_acb.c
+++ b/drivers/i2c/busses/scx200_acb.c
@@ -46,6 +46,7 @@ module_param_array(base, int, NULL, 0);
 MODULE_PARM_DESC(base, "Base addresses for the ACCESS.bus controllers");
 
 #define POLL_TIMEOUT	(HZ/5)
+#define ACBCLK		240	/* 48 MHz / 100 kHz / 2 */
 
 enum scx200_acb_state {
 	state_idle,
@@ -255,14 +256,14 @@ static void scx200_acb_poll(struct scx200_acb_iface *iface)
 static void scx200_acb_reset(struct scx200_acb_iface *iface)
 {
 	/* Disable the ACCESS.bus device and Configure the SCL
-	   frequency: 16 clock cycles */
-	outb(0x70, ACBCTL2);
+	   frequency: 240 clock cycles => 100 kHz */
+	outb(ACBCLK, ACBCTL2);
 	/* Polling mode */
 	outb(0, ACBCTL1);
 	/* Disable slave address */
 	outb(0, ACBADDR);
 	/* Enable the ACCESS.bus device */
-	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
+	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);
 	/* Free STALL after START */
 	outb(inb(ACBCTL1) & ~(ACBCTL1_STASTRE | ACBCTL1_NMINTE), ACBCTL1);
 	/* Send a STOP */
@@ -390,10 +391,10 @@ static int scx200_acb_probe(struct scx200_acb_iface *iface)
 	u8 val;
 
 	/* Disable the ACCESS.bus device and Configure the SCL
-	   frequency: 16 clock cycles */
-	outb(0x70, ACBCTL2);
+	   frequency: 240 clock cycles => 100 kHz*/
+	outb(ACBCLK, ACBCTL2);
 
-	if (inb(ACBCTL2) != 0x70) {
+	if (inb(ACBCTL2) != ACBCLK) {
 		pr_debug("ACBCTL2 readback failed\n");
 		return -ENXIO;
 	}
@@ -406,7 +407,7 @@ static int scx200_acb_probe(struct scx200_acb_iface *iface)
 		return -ENXIO;
 	}
 
-	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
+	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);
 
 	outb(inb(ACBCTL1) | ACBCTL1_NMINTE, ACBCTL1);
 
-- 
2.4.0.rc1

Re: [PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[Wolfram Sang]

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

On Thu, Apr 23, 2015 at 10:27:50PM +0200, Svenning Sørensen wrote:
> According to I2C spec, max SCL rate is 100 kHz, but SCx200/CS5536
> controller is currently driving it at 214 kHz according to my math.
> 
> SCL is derived from an input clock of 48 MHz, which must be divided
> by 480 (240 cycles for each SCL high/low state) to be within spec.
> 
> Signed-off-by: Svenning Soerensen <sss-sijd2F9aCfdBDgjK7y7TUQ@public.gmane.org>

So, did you test/measure the actual frequency? Or is this patch based on
specs only? Thanks for caring about this old driver, though!


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

Re: [PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[Svenning Sørensen]

On 12-05-2015 22:19, Wolfram Sang wrote:
> On Thu, Apr 23, 2015 at 10:27:50PM +0200, Svenning Sørensen wrote:
>> According to I2C spec, max SCL rate is 100 kHz, but SCx200/CS5536
>> controller is currently driving it at 214 kHz according to my math.
>>
>> SCL is derived from an input clock of 48 MHz, which must be divided
>> by 480 (240 cycles for each SCL high/low state) to be within spec.
>>
>> Signed-off-by: Svenning Soerensen <sss-sijd2F9aCfdBDgjK7y7TUQ@public.gmane.org>
> 
> So, did you test/measure the actual frequency? Or is this patch based on
> specs only? Thanks for caring about this old driver, though!
> 

Unfortunately I don't have any scope, so it is based on datasheet only.

I noticed sporadic i2c errors reading the temperature on some of my
PC Engines Alix.2 boards (with LM86 sensor chip) so started digging
the datasheets and driver for an explanation.

I haven't noticed any problems after this change.

Re: [PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[Wolfram Sang]

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

On Thu, Apr 23, 2015 at 10:27:50PM +0200, Svenning Sørensen wrote:
> According to I2C spec, max SCL rate is 100 kHz, but SCx200/CS5536
> controller is currently driving it at 214 kHz according to my math.
> 
> SCL is derived from an input clock of 48 MHz, which must be divided
> by 480 (240 cycles for each SCL high/low state) to be within spec.
> 
> Signed-off-by: Svenning Soerensen <sss-sijd2F9aCfdBDgjK7y7TUQ@public.gmane.org>

Thanks for mentioning that it fixed a problem for you. I needed this
info.

> +#define ACBCLK		240	/* 48 MHz / 100 kHz / 2 */

Can you add a comment where you found the info about 48MHz? Since I
don't know the hardware at all: Can you imagine that there is even older
hardware with a clock where 0x70 was suitable?

> -	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
> +	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);
...
> -	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
> +	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);

For consistency reasons and to be less intrusive, I'd rather keep the
original chunks.


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

Re: [PATCH] i2c: scx200_acb: avoid I2C bus overclocking.

[Wolfram Sang]

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

On Wed, Jun 03, 2015 at 03:21:35AM +0900, Wolfram Sang wrote:
> On Thu, Apr 23, 2015 at 10:27:50PM +0200, Svenning Sørensen wrote:
> > According to I2C spec, max SCL rate is 100 kHz, but SCx200/CS5536
> > controller is currently driving it at 214 kHz according to my math.
> > 
> > SCL is derived from an input clock of 48 MHz, which must be divided
> > by 480 (240 cycles for each SCL high/low state) to be within spec.
> > 
> > Signed-off-by: Svenning Soerensen <sss@secomea.com>
> 
> Thanks for mentioning that it fixed a problem for you. I needed this
> info.
> 
> > +#define ACBCLK		240	/* 48 MHz / 100 kHz / 2 */
> 
> Can you add a comment where you found the info about 48MHz? Since I
> don't know the hardware at all: Can you imagine that there is even older
> hardware with a clock where 0x70 was suitable?
> 
> > -	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
> > +	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);
> ...
> > -	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
> > +	outb(ACBCLK | ACBCTL2_ENABLE, ACBCTL2);
> 
> For consistency reasons and to be less intrusive, I'd rather keep the
> original chunks.

I just remembered that this driver still has a maintainer :) Adding Jim
to CC. Maybe he has some more info? Would be sad to lose this potential
fix, yet I am careful to not introduce regressions to this old hardware.

Jim: The rest of this small thread can be found here:

http://patchwork.ozlabs.org/patch/464046/

Thanks,

   Wolfram



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