| Message ID | 201307031343.11647.arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
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 =
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
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
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
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
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?
> 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
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
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 =