Patchwork [v10] i2c-designware: make SDA hold time configurable

login
register
mail settings
Submitter Arnd Bergmann
Date July 3, 2013, 11:43 a.m.
Message ID <201307031343.11647.arnd@arndb.de>
Download mbox | patch
Permalink /patch/256603/
State Superseded
Headers show

Comments

Arnd Bergmann - July 3, 2013, 11:43 a.m.
On Wednesday 26 June 2013, Wolfram Sang wrote:
>   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > This patch makes the SDA hold time configurable through device tree.
> > 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> 
> Applied to for-next, thanks for keeping at it and providing lots of
> useful information. Much appreciated!

Sorry, but I got a regression that I didn't find reported elsewhere
so far, even though it breaks a lot of the ARM defconfig builds:

drivers/built-in.o: In function `dw_i2c_probe':
/git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'

I suspect you want something like the change below.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Ruppert - July 3, 2013, 1:29 p.m.
On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> On Wednesday 26 June 2013, Wolfram Sang wrote:
> >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > This patch makes the SDA hold time configurable through device tree.
> > > 
> > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > 
> > Applied to for-next, thanks for keeping at it and providing lots of
> > useful information. Much appreciated!
> 
> Sorry, but I got a regression that I didn't find reported elsewhere
> so far, even though it breaks a lot of the ARM defconfig builds:
> 
> drivers/built-in.o: In function `dw_i2c_probe':
> /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> 
> I suspect you want something like the change below.

This looks similar to a patch Vincent Stehle submitted yesterday, see
https://lkml.org/lkml/2013/7/2/145

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index def79b5..57e3a07 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	if (pdev->dev.of_node) {
>  		u32 ht = 0;
>  		u32 ic_clk = dev->get_clk_rate_khz(dev);
> +		u64 hold_time;
>  
>  		of_property_read_u32(pdev->dev.of_node,
>  					"i2c-sda-hold-time-ns", &ht);
> -		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
> +		hold_time = (u64)ic_clk * ht + 500000;
> +		do_div(hold_time, 1000000);
> +		dev->sda_hold_time = hold_time;
>  	}
>  
>  	dev->functionality =
Arnd Bergmann - July 3, 2013, 2:20 p.m.
On Wednesday 03 July 2013, Christian Ruppert wrote:
> On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > This patch makes the SDA hold time configurable through device tree.
> > > > 
> > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > 
> > > Applied to for-next, thanks for keeping at it and providing lots of
> > > useful information. Much appreciated!
> > 
> > Sorry, but I got a regression that I didn't find reported elsewhere
> > so far, even though it breaks a lot of the ARM defconfig builds:
> > 
> > drivers/built-in.o: In function `dw_i2c_probe':
> > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> > 
> > I suspect you want something like the change below.
> 
> This looks similar to a patch Vincent Stehle submitted yesterday, see
> https://lkml.org/lkml/2013/7/2/145

Thanks for the link. Actually his patch looks wrong to me, because

 dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, 1000000); 

assigns the division remainder to sda_hold_time, not the quotient.


	Arnd

> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index def79b5..57e3a07 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  	if (pdev->dev.of_node) {
> >  		u32 ht = 0;
> >  		u32 ic_clk = dev->get_clk_rate_khz(dev);
> > +		u64 hold_time;
> >  
> >  		of_property_read_u32(pdev->dev.of_node,
> >  					"i2c-sda-hold-time-ns", &ht);
> > -		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
> > +		hold_time = (u64)ic_clk * ht + 500000;
> > +		do_div(hold_time, 1000000);
> > +		dev->sda_hold_time = hold_time;
> >  	}
> >  
> >  	dev->functionality =
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Ruppert - July 3, 2013, 2:38 p.m.
On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote:
> On Wednesday 03 July 2013, Christian Ruppert wrote:
> > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > > This patch makes the SDA hold time configurable through device tree.
> > > > > 
> > > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > > 
> > > > Applied to for-next, thanks for keeping at it and providing lots of
> > > > useful information. Much appreciated!
> > > 
> > > Sorry, but I got a regression that I didn't find reported elsewhere
> > > so far, even though it breaks a lot of the ARM defconfig builds:
> > > 
> > > drivers/built-in.o: In function `dw_i2c_probe':
> > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> > > 
> > > I suspect you want something like the change below.
> > 
> > This looks similar to a patch Vincent Stehle submitted yesterday, see
> > https://lkml.org/lkml/2013/7/2/145
> 
> Thanks for the link. Actually his patch looks wrong to me, because
> 
>  dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, 1000000); 
> 
> assigns the division remainder to sda_hold_time, not the quotient.

Hrmmm... At least when I tested it this morning on an ARC architecture
it worked as intended and returned the quotient. Does that mean we have
an issue with this function on ARC? Can anyone who knows these functions
better than I comment?

Greetings,
  Christian
Arnd Bergmann - July 3, 2013, 2:42 p.m.
On Wednesday 03 July 2013, Christian Ruppert wrote:
> On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote:
> > On Wednesday 03 July 2013, Christian Ruppert wrote:
> > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > > > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > > >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > > > This patch makes the SDA hold time configurable through device tree.
> > > > > > 
> > > > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > > > 
> > > > > Applied to for-next, thanks for keeping at it and providing lots of
> > > > > useful information. Much appreciated!
> > > > 
> > > > Sorry, but I got a regression that I didn't find reported elsewhere
> > > > so far, even though it breaks a lot of the ARM defconfig builds:
> > > > 
> > > > drivers/built-in.o: In function `dw_i2c_probe':
> > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> > > > 
> > > > I suspect you want something like the change below.
> > > 
> > > This looks similar to a patch Vincent Stehle submitted yesterday, see
> > > https://lkml.org/lkml/2013/7/2/145
> > 
> > Thanks for the link. Actually his patch looks wrong to me, because
> > 
> >  dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, 1000000); 
> > 
> > assigns the division remainder to sda_hold_time, not the quotient.
> 
> Hrmmm... At least when I tested it this morning on an ARC architecture
> it worked as intended and returned the quotient. Does that mean we have
> an issue with this function on ARC? Can anyone who knows these functions
> better than I comment?

ARC just uses the generic version of div_u64, which is defined in lib/div64.c.

I suspect that the division remainder just happens to work well enough for
you to not cause any run-time error. You could try adding a printk
in that function to show the values you get on ARC.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - July 3, 2013, 2:44 p.m.
On Wednesday 03 July 2013, Christian Ruppert wrote:
> On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote:
> > On Wednesday 03 July 2013, Christian Ruppert wrote:
> > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > > > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > > >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > > > This patch makes the SDA hold time configurable through device tree.
> > > > > > 
> > > > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > > > 
> > > > > Applied to for-next, thanks for keeping at it and providing lots of
> > > > > useful information. Much appreciated!
> > > > 
> > > > Sorry, but I got a regression that I didn't find reported elsewhere
> > > > so far, even though it breaks a lot of the ARM defconfig builds:
> > > > 
> > > > drivers/built-in.o: In function `dw_i2c_probe':
> > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> > > > 
> > > > I suspect you want something like the change below.
> > > 
> > > This looks similar to a patch Vincent Stehle submitted yesterday, see
> > > https://lkml.org/lkml/2013/7/2/145
> > 
> > Thanks for the link. Actually his patch looks wrong to me, because
> > 
> >  dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, 1000000); 
> > 
> > assigns the division remainder to sda_hold_time, not the quotient.
> 
> Hrmmm... At least when I tested it this morning on an ARC architecture
> it worked as intended and returned the quotient. Does that mean we have
> an issue with this function on ARC? Can anyone who knows these functions
> better than I comment?

ARC just uses the generic version of div_u64, which is defined in lib/div64.c.

I suspect that the division remainder just happens to work well enough for
you to not cause any run-time error. You could try adding a printk
in that function to show the values you get on ARC.

	Arnd
 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Ruppert - July 3, 2013, 2:59 p.m.
On Wed, Jul 03, 2013 at 04:44:09PM +0200, Arnd Bergmann wrote:
> On Wednesday 03 July 2013, Christian Ruppert wrote:
> > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 03 July 2013, Christian Ruppert wrote:
> > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > > > > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > > > >   On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > > > > This patch makes the SDA hold time configurable through device tree.
> > > > > > > 
> > > > > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > > > > 
> > > > > > Applied to for-next, thanks for keeping at it and providing lots of
> > > > > > useful information. Much appreciated!
> > > > > 
> > > > > Sorry, but I got a regression that I didn't find reported elsewhere
> > > > > so far, even though it breaks a lot of the ARM defconfig builds:
> > > > > 
> > > > > drivers/built-in.o: In function `dw_i2c_probe':
> > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3'
> > > > > 
> > > > > I suspect you want something like the change below.
> > > > 
> > > > This looks similar to a patch Vincent Stehle submitted yesterday, see
> > > > https://lkml.org/lkml/2013/7/2/145
> > > 
> > > Thanks for the link. Actually his patch looks wrong to me, because
> > > 
> > >  dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, 1000000); 
> > > 
> > > assigns the division remainder to sda_hold_time, not the quotient.
> > 
> > Hrmmm... At least when I tested it this morning on an ARC architecture
> > it worked as intended and returned the quotient. Does that mean we have
> > an issue with this function on ARC? Can anyone who knows these functions
> > better than I comment?
> 
> ARC just uses the generic version of div_u64, which is defined in lib/div64.c.
> 
> I suspect that the division remainder just happens to work well enough for
> you to not cause any run-time error. You could try adding a printk
> in that function to show the values you get on ARC.

That's what I did and they were identical to the original values
calculated with /. I just looked at include/linux/math64.h and found the
following comment:

/**
 * div_u64 - unsigned 64bit divide with 32bit divisor
 *
 * This is the most common 64bit divide and should be used if possible,
 * as many 32bit archs can optimize this variant better than a full 64bit
 * divide.
 */

Although this doesn't explicitly state what the function returns to me
this sounds more like the quotient is returned rather than the remainder?
Stehle Vincent-B46079 - July 3, 2013, 3:07 p.m.
> From: Christian Ruppert [mailto:christian.ruppert@abilis.com]
(..)
> Although this doesn't explicitly state what the function returns to me
> this sounds more like the quotient is returned rather than the remainder?

Hi,

Yes div_u64() returns the quotient.

It is defined in linux/math64.h as:

  static inline u64 div_u64(u64 dividend, u32 divisor)
  {
          u32 remainder;
          return div_u64_rem(dividend, divisor, &remainder);
  }

The remainder is probably fully optimized out.

Best regards,

V.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - July 3, 2013, 3:26 p.m.
On Wednesday 03 July 2013, Stehle Vincent-B46079 wrote:
> > From: Christian Ruppert [mailto:christian.ruppert@abilis.com]
> (..)
> > Although this doesn't explicitly state what the function returns to me
> > this sounds more like the quotient is returned rather than the remainder?
> 
> Hi,
> 
> Yes div_u64() returns the quotient.
> 
> It is defined in linux/math64.h as:
> 
>   static inline u64 div_u64(u64 dividend, u32 divisor)
>   {
>           u32 remainder;
>           return div_u64_rem(dividend, divisor, &remainder);
>   }
> 
> The remainder is probably fully optimized out.

Ah, you are right, sorry for the confusion on my part.

I thought you had used do_div() rather than div_u64().
Everything's fine then, feel free to add my

Acked-by: Arnd Bergmann <arnd@arndb.de>

to your patch.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index def79b5..57e3a07 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -119,10 +119,13 @@  static int dw_i2c_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node) {
 		u32 ht = 0;
 		u32 ic_clk = dev->get_clk_rate_khz(dev);
+		u64 hold_time;
 
 		of_property_read_u32(pdev->dev.of_node,
 					"i2c-sda-hold-time-ns", &ht);
-		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+		hold_time = (u64)ic_clk * ht + 500000;
+		do_div(hold_time, 1000000);
+		dev->sda_hold_time = hold_time;
 	}
 
 	dev->functionality =