Re: [PATCH v7] i2c-designware: make SDA hold time configurable

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Tue, Apr 09, 2013 at 12:59:54PM +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>

Hi Wolfram,

What happened to this patch? I don't see it merged for 3.10.

The reason I'm asking is that I would like to add ACPI support for the SDA
hold time parameter analogous to the DT version.

Thanks.

> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |    9 +++++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..21fabe7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,9 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - sda-hold-time : should contain the SDA hold time in nanoseconds.
> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +23,14 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		clock-frequency = <400000>;
> +		sda-hold-time = <300>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 94fd818..ba40e14 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -67,6 +67,7 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_COMP_PARAM_1	0xf4
>  #define DW_IC_COMP_TYPE		0xfc
> @@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	if (dev->sda_hold_time)
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> +
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 9c1840e..33dfec3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -88,6 +88,7 @@ struct dw_i2c_dev {
>  	u32			master_cfg;
>  	unsigned int		tx_fifo_depth;
>  	unsigned int		rx_fifo_depth;
> +	u32			sda_hold_time;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0ceb6e1..0a6e29b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,6 +34,7 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -136,6 +137,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	}
>  	clk_prepare_enable(dev->clk);
>  
> +	if (pdev->dev.of_node) {
> +		u32 ht;
> +		u32 ic_clk = dev->get_clk_rate_khz(dev);
> +
> +		of_property_read_u32(pdev->dev.of_node, "sda-hold-time", &ht);
> +		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
> +	}
> +
>  	dev->functionality =
>  		I2C_FUNC_I2C |
>  		I2C_FUNC_10BIT_ADDR |
> -- 
> 1.7.1
> 

[PATCH REBASE] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

This patch makes the SDA hold time configurable through device tree.

[rebased to i2c-current/i2c-next/mainline-3.10-rc1]

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    9 +++++++++
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..21fabe7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - sda-hold-time : should contain the SDA hold time in nanoseconds.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +23,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		sda-hold-time = <300>;
+	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 21fbb34..91d422c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
@@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
 	u32			master_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 8ec9133..589f414 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -120,6 +121,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		u32 ht;
+		u32 ic_clk = dev->get_clk_rate_khz(dev);
+
+		of_property_read_u32(pdev->dev.of_node, "sda-hold-time", &ht);
+		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+	}
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
> This patch makes the SDA hold time configurable through device tree.
> 
> [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>

Hmm, I really have problems adding a generic property. I need to better
understand why this is needed? What is the usecase? Can't a safe value
be calculated depending on the bus-speed? Is there a public datasheet
available somewhere?

Thanks,

   Wolfram


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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

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

On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
> On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
> > This patch makes the SDA hold time configurable through device tree.
> > 
> > [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> > 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> 
> Hmm, I really have problems adding a generic property. I need to better
> understand why this is needed? What is the usecase? Can't a safe value
> be calculated depending on the bus-speed? Is there a public datasheet
> available somewhere?

I checked with our PCB/Applications team and the data sheets for the
peripherals in question (DVB demodulator front ends) are under NDA.
Mika, you seem to be interested in this patch as well. Do you know of
any publicly available data sheets for hardware requiring this
adjustment?

In the case of the Designware block, the parameter both changes SDA and
START hold times, however, and you'll find lots of data sheets for
hardware with START hold time requirements on the net, e.g.
http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf

The empirical solution in the function i2c_dw_scl_hcnt does not seem to
work in all cases: Our lab guys confirmed that we have several PCB
designs which do not work without adjusting the sda-hold-time parameter
to an appropriate value. The value seems to be different for different
PCBs.

I suspect that this kind of configurability is not the same for all i2c
bus master hardware. If you don't want to add a generic property, would
you accept the patch with the property renamed to dwi2c,sda-hold-time?

Greetings,
  Christian


-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
> On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
> > On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
> > > This patch makes the SDA hold time configurable through device tree.
> > > 
> > > [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> > > 
> > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > 
> > Hmm, I really have problems adding a generic property. I need to better
> > understand why this is needed? What is the usecase? Can't a safe value
> > be calculated depending on the bus-speed? Is there a public datasheet
> > available somewhere?
> 
> I checked with our PCB/Applications team and the data sheets for the
> peripherals in question (DVB demodulator front ends) are under NDA.
> Mika, you seem to be interested in this patch as well. Do you know of
> any publicly available data sheets for hardware requiring this
> adjustment?

Sorry for the delay, I was on vacation.

I'm not aware of any publicly available datasheets for this block,
unfortunately.

> In the case of the Designware block, the parameter both changes SDA and
> START hold times, however, and you'll find lots of data sheets for
> hardware with START hold time requirements on the net, e.g.
> http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
> 
> The empirical solution in the function i2c_dw_scl_hcnt does not seem to
> work in all cases: Our lab guys confirmed that we have several PCB
> designs which do not work without adjusting the sda-hold-time parameter
> to an appropriate value. The value seems to be different for different
> PCBs.

Yeah, we see the same problem. Our platform guys have solved this so that
they made the SDA hold time available from ACPI to the device driver.

> I suspect that this kind of configurability is not the same for all i2c
> bus master hardware. If you don't want to add a generic property, would
> you accept the patch with the property renamed to dwi2c,sda-hold-time?
> 
> Greetings,
>   Christian
> 
> 
> -- 
>   Christian Ruppert              ,          <christian.ruppert@abilis.com>
>                                 /|
>   Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
>                              _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

Hi,

On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
> On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
> > On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
> > > This patch makes the SDA hold time configurable through device tree.
> > > 
> > > [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> > > 
> > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > 
> > Hmm, I really have problems adding a generic property. I need to better
> > understand why this is needed? What is the usecase? Can't a safe value
> > be calculated depending on the bus-speed? Is there a public datasheet
> > available somewhere?
> 
> I checked with our PCB/Applications team and the data sheets for the
> peripherals in question (DVB demodulator front ends) are under NDA.
> Mika, you seem to be interested in this patch as well. Do you know of
> any publicly available data sheets for hardware requiring this
> adjustment?

So, I looked around and found:
http://www.maximintegrated.com/app-notes/index.mvp/id/3268

which after thinking further about it gives me the following
conclusions:

- sda-hold-time is a property/requirement of a device not following
  the I2C spec. It is not a property of the master!

- It should not be encoded in the devicetree, since the flaw is implicit
  to the device, so only the driver needs to know about it. I wonder
  about something like this in the i2c slave driver:

	ret = i2c_request_sda_hold_time(client);

  The core then can collect the requests and forward them to the host
  driver. This driver then can set up the hardware or return -EOPNOTSUPP
  and we can even warn the user that there might be problems ahead.

- I wonder if we really need to have a parameter time-in-ns? The
  specs cleary say 300ns, so I'd think this is the value we should
  always use. This is from a theorhetical pov though, maybe your
  practical experience is different. What values do you need?


> In the case of the Designware block, the parameter both changes SDA and
> START hold times, however, and you'll find lots of data sheets for
> hardware with START hold time requirements on the net, e.g.
> http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf

What I couldn't find is a reference manual for a designware IP that
supports sda hold time? I found some spear SoC which do not have that
register, so that should surely be reflected in the patchset, too.

> The empirical solution in the function i2c_dw_scl_hcnt does not seem to
> work in all cases: Our lab guys confirmed that we have several PCB
> designs which do not work without adjusting the sda-hold-time parameter
> to an appropriate value. The value seems to be different for different
> PCBs.

I'd hope that 300ns is a safe value for all PCBs?

> I suspect that this kind of configurability is not the same for all i2c
> bus master hardware.

Yeah, maybe some do sda-holding by default? Dunno, never checked for
that detail.

Regards,

   Wolfram


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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

On Wed, Jun 19, 2013 at 11:45:40AM +0200, Wolfram Sang wrote:
> Hi,
> 
> On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
> > On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
> > > On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
> > > > This patch makes the SDA hold time configurable through device tree.
> > > > 
> > > > [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> > > > 
> > > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > 
> > > Hmm, I really have problems adding a generic property. I need to better
> > > understand why this is needed? What is the usecase? Can't a safe value
> > > be calculated depending on the bus-speed? Is there a public datasheet
> > > available somewhere?
> > 
> > I checked with our PCB/Applications team and the data sheets for the
> > peripherals in question (DVB demodulator front ends) are under NDA.
> > Mika, you seem to be interested in this patch as well. Do you know of
> > any publicly available data sheets for hardware requiring this
> > adjustment?
> 
> So, I looked around and found:
> http://www.maximintegrated.com/app-notes/index.mvp/id/3268
> 
> which after thinking further about it gives me the following
> conclusions:
> 
> - sda-hold-time is a property/requirement of a device not following
>   the I2C spec. It is not a property of the master!

Actually, in a protocol like I2C, every device on the bus must respect
timing constraints like hold time etc. These parameters apply at the
same time to the master and to all slaves.

> - It should not be encoded in the devicetree, since the flaw is implicit
>   to the device, so only the driver needs to know about it. I wonder
>   about something like this in the i2c slave driver:
> 
> 	ret = i2c_request_sda_hold_time(client);
> 
>   The core then can collect the requests and forward them to the host
>   driver. This driver then can set up the hardware or return -EOPNOTSUPP
>   and we can even warn the user that there might be problems ahead.

This might be a solution but given that many I2C drivers are written as
an afterthought by device manufacturers and are released under more or
less open terms of licensing into the wild I doubt this would work very
well in practise.

> - I wonder if we really need to have a parameter time-in-ns? The
>   specs cleary say 300ns, so I'd think this is the value we should
>   always use. This is from a theorhetical pov though, maybe your
>   practical experience is different. What values do you need?

In reality, the I2C specification is more subtle than that: The "data
hold time" is specified as 0ns with a footnote [3] stating that devices
"must internally provide a hold time of at least 300ns for the SDA
signal...".

Revision 5 contains a relatively understandable explanation about how to
interpret this but earlier versions are less helpful. I think this
confusion is at the root of many timing issues encountered with I2C (and
the reason why Synopsys made this configurable). In fact, especially
earlier specs are _all but_ clear in this point and we cannot assume
that all peripherals were designed after Revision 5 was released in
October 2012.

> > In the case of the Designware block, the parameter both changes SDA and
> > START hold times, however, and you'll find lots of data sheets for
> > hardware with START hold time requirements on the net, e.g.
> > http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
> 
> What I couldn't find is a reference manual for a designware IP that
> supports sda hold time? I found some spear SoC which do not have that
> register, so that should surely be reflected in the patchset, too.

If you have access to DesignWare documentation, check out the
"DesignWare DW_apb_i2c Databook" Version 1.17a from March 2012.
Unluckily, I clearly don't have the right to share this document with
you. Do you know the version of the blocks in the spear SoC which do not
support this register?

> > The empirical solution in the function i2c_dw_scl_hcnt does not seem to
> > work in all cases: Our lab guys confirmed that we have several PCB
> > designs which do not work without adjusting the sda-hold-time parameter
> > to an appropriate value. The value seems to be different for different
> > PCBs.
> 
> I'd hope that 300ns is a safe value for all PCBs?

Not according to our PCB guys. The highest value I have found in a quick
check of our device trees is 650ns with others being just slightly above
300ns.

> > I suspect that this kind of configurability is not the same for all i2c
> > bus master hardware.
> 
> Yeah, maybe some do sda-holding by default? Dunno, never checked for
> that detail.
> 
> Regards,
> 
>    Wolfram
> 



-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

Hi Christian,

> > So, I looked around and found:
> > http://www.maximintegrated.com/app-notes/index.mvp/id/3268
> > 
> > which after thinking further about it gives me the following
> > conclusions:
> > 
> > - sda-hold-time is a property/requirement of a device not following
> >   the I2C spec. It is not a property of the master!
> 
> Actually, in a protocol like I2C, every device on the bus must respect
> timing constraints like hold time etc. These parameters apply at the
> same time to the master and to all slaves.

Yes. What I meant is: the flaw of one a specific device on the bus
imposes the timing on the whole bus.

> 
> > - It should not be encoded in the devicetree, since the flaw is implicit
> >   to the device, so only the driver needs to know about it. I wonder
> >   about something like this in the i2c slave driver:
> > 
> > 	ret = i2c_request_sda_hold_time(client);
> > 
> >   The core then can collect the requests and forward them to the host
> >   driver. This driver then can set up the hardware or return -EOPNOTSUPP
> >   and we can even warn the user that there might be problems ahead.
> 
> This might be a solution but given that many I2C drivers are written as
> an afterthought by device manufacturers and are released under more or
> less open terms of licensing into the wild I doubt this would work very
> well in practise.

Hrmgl, the design looks much better to me, though. Once a driver is
identified to need this, the core is able to report this requirement to
a user who might even be unaware of the issue. The dt property has a bit
of "try this if things don't work, you may be lucky" taste to it. Need
to think about it. If PCB design also has an influence, then my idea
won't work, though.

> > - I wonder if we really need to have a parameter time-in-ns? The
> >   specs cleary say 300ns, so I'd think this is the value we should
> >   always use. This is from a theorhetical pov though, maybe your
> >   practical experience is different. What values do you need?
> 
> In reality, the I2C specification is more subtle than that: The "data
> hold time" is specified as 0ns with a footnote [3] stating that devices
> "must internally provide a hold time of at least 300ns for the SDA
> signal...".
> 
> Revision 5 contains a relatively understandable explanation about how to
> interpret this but earlier versions are less helpful. I think this
> confusion is at the root of many timing issues encountered with I2C (and
> the reason why Synopsys made this configurable). In fact, especially
> earlier specs are _all but_ clear in this point and we cannot assume
> that all peripherals were designed after Revision 5 was released in
> October 2012.

OK, agreed. Still surprised that I never encountered such a device,
probably I was just lucky.

> 
> > > In the case of the Designware block, the parameter both changes SDA and
> > > START hold times, however, and you'll find lots of data sheets for
> > > hardware with START hold time requirements on the net, e.g.
> > > http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
> > 
> > What I couldn't find is a reference manual for a designware IP that
> > supports sda hold time? I found some spear SoC which do not have that
> > register, so that should surely be reflected in the patchset, too.
> 
> If you have access to DesignWare documentation, check out the
> "DesignWare DW_apb_i2c Databook" Version 1.17a from March 2012.

I don't have, and I do have a hard time finding information about it
otherwise :(

> Unluckily, I clearly don't have the right to share this document with
> you. Do you know the version of the blocks in the spear SoC which do not
> support this register?

ST Spear300 and 600 have 0x3130352a in the version reg according to the
refman.

> > > The empirical solution in the function i2c_dw_scl_hcnt does not seem to
> > > work in all cases: Our lab guys confirmed that we have several PCB
> > > designs which do not work without adjusting the sda-hold-time parameter
> > > to an appropriate value. The value seems to be different for different
> > > PCBs.
> > 
> > I'd hope that 300ns is a safe value for all PCBs?
> 
> Not according to our PCB guys. The highest value I have found in a quick
> check of our device trees is 650ns with others being just slightly above
> 300ns.

Thanks for sharing your results \o/ This helps me a lot. Can you find
out/guess if this value is solely dependend on a slave or is it also
dependend on PCB layout?

Thanks,

   Wolfram


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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

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

Hello Wolfram,

On Wed, Jun 19, 2013 at 05:20:59PM +0200, Wolfram Sang wrote:
> [...]
> > 
> > > - It should not be encoded in the devicetree, since the flaw is implicit
> > >   to the device, so only the driver needs to know about it. I wonder
> > >   about something like this in the i2c slave driver:
> > > 
> > > 	ret = i2c_request_sda_hold_time(client);
> > > 
> > >   The core then can collect the requests and forward them to the host
> > >   driver. This driver then can set up the hardware or return -EOPNOTSUPP
> > >   and we can even warn the user that there might be problems ahead.
> > 
> > This might be a solution but given that many I2C drivers are written as
> > an afterthought by device manufacturers and are released under more or
> > less open terms of licensing into the wild I doubt this would work very
> > well in practise.
> 
> Hrmgl, the design looks much better to me, though. Once a driver is
> identified to need this, the core is able to report this requirement to
> a user who might even be unaware of the issue. The dt property has a bit
> of "try this if things don't work, you may be lucky" taste to it. Need
> to think about it. If PCB design also has an influence, then my idea
> won't work, though.

I agree with that in theory. We would need to define more constraints in
this case, however: Both min and max setup and hold times would be
required for data and start/stop conditions. We have seen cases in which
the hold time we configured was too big for some devices on the bus
which in turn seemed to get confused between data and start/stop
conditions. I don't remember the exact values causing this issue but it
was definitely less than the max of 650ns I have grepped out yesterday.

Addressing this issue properly would mean implementing some simple form
of STA. Not sure if an operating system kernel is the right place for
this, though (and pretty sure that individual device drivers aren't).
These questions are probably better addressed at the PCB design stage.

> [...]
> > 
> > > > In the case of the Designware block, the parameter both changes SDA and
> > > > START hold times, however, and you'll find lots of data sheets for
> > > > hardware with START hold time requirements on the net, e.g.
> > > > http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
> > > 
> > > What I couldn't find is a reference manual for a designware IP that
> > > supports sda hold time? I found some spear SoC which do not have that
> > > register, so that should surely be reflected in the patchset, too.
> > 
> > If you have access to DesignWare documentation, check out the
> > "DesignWare DW_apb_i2c Databook" Version 1.17a from March 2012.
> 
> I don't have, and I do have a hard time finding information about it
> otherwise :(
> 
> > Unluckily, I clearly don't have the right to share this document with
> > you. Do you know the version of the blocks in the spear SoC which do not
> > support this register?
> 
> ST Spear300 and 600 have 0x3130352a in the version reg according to the
> refman.

Hrmmm... Going through the release notes it looks like this feature was
added with module version 1.11a, released in 2010 (the initial version
of the IP was released in 2003).

> > > > The empirical solution in the function i2c_dw_scl_hcnt does not seem to
> > > > work in all cases: Our lab guys confirmed that we have several PCB
> > > > designs which do not work without adjusting the sda-hold-time parameter
> > > > to an appropriate value. The value seems to be different for different
> > > > PCBs.
> > > 
> > > I'd hope that 300ns is a safe value for all PCBs?
> > 
> > Not according to our PCB guys. The highest value I have found in a quick
> > check of our device trees is 650ns with others being just slightly above
> > 300ns.
> 
> Thanks for sharing your results \o/ This helps me a lot. Can you find
> out/guess if this value is solely dependend on a slave or is it also
> dependend on PCB layout?

In the simplest form of STA, timings are generally considered a function
of three factors:
  . Drive strength
  . Bus load (Fan out)
  . Internal timings of all devices on the bus (setup/hold times etc.)

This applies to ASIC internal timing. My PCB experience is quite limited
and I'm not sure if this can be applied as such. That said, the I2C
specification does define all three of these factors and I deduce they
all have their importance. We know that the specified internal device
timings are sometimes buggy, I don't know about the other two.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Rob Herring]

On 05/14/2013 08:04 AM, Christian Ruppert wrote:
> This patch makes the SDA hold time configurable through device tree.
> 
> [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |    9 +++++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..21fabe7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,9 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - sda-hold-time : should contain the SDA hold time in nanoseconds.

Please specify time units in the property name. Perhaps
i2c-sda-hold-time-ns.

Based on reading the discussion, there is one similar property I have
found: "samsung,i2c-sda-delay = <100>". I wouldn't copy it as it doesn't
tell the units or what the delay is.

Otherwise, looks fine to me.

Rob

> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +23,14 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		clock-frequency = <400000>;
> +		sda-hold-time = <300>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 21fbb34..91d422c 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -67,6 +67,7 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_ENABLE_STATUS	0x9c
>  #define DW_IC_COMP_PARAM_1	0xf4
> @@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	if (dev->sda_hold_time)
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> +
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 9c1840e..33dfec3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -88,6 +88,7 @@ struct dw_i2c_dev {
>  	u32			master_cfg;
>  	unsigned int		tx_fifo_depth;
>  	unsigned int		rx_fifo_depth;
> +	u32			sda_hold_time;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 8ec9133..589f414 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,6 +34,7 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -120,6 +121,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		return PTR_ERR(dev->clk);
>  	clk_prepare_enable(dev->clk);
>  
> +	if (pdev->dev.of_node) {
> +		u32 ht;
> +		u32 ic_clk = dev->get_clk_rate_khz(dev);
> +
> +		of_property_read_u32(pdev->dev.of_node, "sda-hold-time", &ht);
> +		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
> +	}
> +
>  	dev->functionality =
>  		I2C_FUNC_I2C |
>  		I2C_FUNC_10BIT_ADDR |
> 

[PATCH v8] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

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>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
 arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
 arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----
 drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |   10 ++++++++++
 6 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..3266cc7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +23,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		i2c-sda-hold-time-ns = <300>;
+	};
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..6c0e776 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
@@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e761ad1..912aa22 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -90,6 +90,7 @@ struct dw_i2c_dev {
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 35b70a1..81362e5 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -121,6 +122,15 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		u32 ht;
+		u32 ic_clk = dev->get_clk_rate_khz(dev);
+
+		of_property_read_u32(pdev->dev.of_node,
+					"i2c-sda-hold-time-ns", &ht);
+		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+	}
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH v8] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

On Tue, Jun 18, 2013 at 09:44:29AM +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>

Okay, I am convinced of the property now...

> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 35b70a1..81362e5 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,6 +34,7 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -121,6 +122,15 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		return PTR_ERR(dev->clk);
>  	clk_prepare_enable(dev->clk);
>  
> +	if (pdev->dev.of_node) {
> +		u32 ht;
> +		u32 ic_clk = dev->get_clk_rate_khz(dev);
> +
> +		of_property_read_u32(pdev->dev.of_node,
> +					"i2c-sda-hold-time-ns", &ht);
> +		dev->sda_holdO_time = ((u64)ic_clk * ht + 500000) / 1000000;

... but is this correct even if the property is not defined?

> +	}
> +

Thanks for all the input so far!


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

Re: [PATCH v8] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

On Thu, Jun 20, 2013 at 10:04:54PM +0200, Wolfram Sang wrote:
> On Tue, Jun 18, 2013 at 09:44:29AM +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>
> 
> Okay, I am convinced of the property now...

and the version check is missing since older revisions do not support
sda-hold-time.



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

[PATCH v9] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

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>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
 arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
 arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----
 drivers/i2c/busses/i2c-designware-core.c           |   13 +++++++++++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |   10 ++++++++++
 6 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..3266cc7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +23,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		i2c-sda-hold-time-ns = <300>;
+	};
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..66119e2 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,9 +67,12 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_COMP_VERSION	0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
 #define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_COMP_TYPE_VALUE	0x44570140
 
@@ -332,6 +335,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	reg = dw_readl(dev, DW_IC_COMP_VERSION);
+	if (dev->sda_hold_time) {
+		if (reg >= DW_IC_SDA_HOLD_MIN_VERS)
+			dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+		else
+			dev_warn(dev->dev,
+				"Hardware too old to adjust SDA hold time.");
+	}
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e761ad1..912aa22 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -90,6 +90,7 @@ struct dw_i2c_dev {
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 35b70a1..8b9d3f1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -121,6 +122,15 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		u32 ht = 0;
+		u32 ic_clk = dev->get_clk_rate_khz(dev);
+
+		of_property_read_u32(pdev->dev.of_node,
+					"i2c-sda-hold-time-ns", &ht);
+		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+	}
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH v9] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

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

Hi,

On Fri, Jun 21, 2013 at 11:53:42AM +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>
> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
>  arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
>  arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----

Vineet, an ack for those would be nice since I think it is better if I
take all this via I2C.

>  drivers/i2c/busses/i2c-designware-core.c           |   13 +++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |   10 ++++++++++
>  6 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..3266cc7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,9 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.

Not on all versions, should be mentioned here.

> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +23,14 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		clock-frequency = <400000>;
> +		i2c-sda-hold-time-ns = <300>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index c41ca63..66119e2 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -67,9 +67,12 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_ENABLE_STATUS	0x9c
>  #define DW_IC_COMP_PARAM_1	0xf4
> +#define DW_IC_COMP_VERSION	0xf8
> +#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
>  #define DW_IC_COMP_TYPE		0xfc
>  #define DW_IC_COMP_TYPE_VALUE	0x44570140
>  
> @@ -332,6 +335,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	reg = dw_readl(dev, DW_IC_COMP_VERSION);

Why not reading reg inside the if-block?

Thanks,

   Wolfram


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

Re: [PATCH v9] i2c-designware: make SDA hold time configurable

[Vineet Gupta]

On 06/25/2013 10:09 PM, Wolfram Sang wrote:
> Hi,
>
> On Fri, Jun 21, 2013 at 11:53:42AM +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>
>> > ---
>> >  .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
>> >  arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
>> >  arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----
> Vineet, an ack for those would be nice since I think it is better if I
> take all this via I2C.
>

Certainly, I was waiting for you guys to converge :-)

Acked-by: Vineet Gupta <vgupta@synopsys.com>   # for arch/arc/boot/dts/*

-Vineet

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

[Christian Ruppert]

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>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   15 +++++++++++++++
 arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
 arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----
 drivers/i2c/busses/i2c-designware-core.c           |   13 +++++++++++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |   10 ++++++++++
 6 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..7fd7fa2 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+   This option is only supported in hardware blocks version 1.11a or newer.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +24,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		i2c-sda-hold-time-ns = <300>;
+	};
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
 		};
 
 		i2c0: i2c@FF120000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c1: i2c@FF121000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c2: i2c@FF122000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c3: i2c@FF123000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 		i2c4: i2c@FF124000 {
-			sda-hold-time = <432>;
+			i2c-sda-hold-time-ns = <432>;
 		};
 
 		leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..d18d4b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,9 +67,12 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_COMP_VERSION	0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
 #define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_COMP_TYPE_VALUE	0x44570140
 
@@ -332,6 +335,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time) {
+		reg = dw_readl(dev, DW_IC_COMP_VERSION);
+		if (reg >= DW_IC_SDA_HOLD_MIN_VERS)
+			dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+		else
+			dev_warn(dev->dev,
+				"Hardware too old to adjust SDA hold time.");
+	}
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e761ad1..912aa22 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -90,6 +90,7 @@ struct dw_i2c_dev {
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 35b70a1..8b9d3f1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -121,6 +122,15 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		u32 ht = 0;
+		u32 ic_clk = dev->get_clk_rate_khz(dev);
+
+		of_property_read_u32(pdev->dev.of_node,
+					"i2c-sda-hold-time-ns", &ht);
+		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+	}
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

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

[Vineet Gupta]

On 06/26/2013 02:25 PM, 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>
> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   15 +++++++++++++++
>  arch/arc/boot/dts/abilis_tb100_dvk.dts             |   10 +++++-----
>  arch/arc/boot/dts/abilis_tb101_dvk.dts             |   10 +++++-----
>  drivers/i2c/busses/i2c-designware-core.c           |   13 +++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |   10 ++++++++++
>  6 files changed, 49 insertions(+), 10 deletions(-)

Acked-by: Vineet Gupta <vgupta@synopsys.com> for arch/arc bits

-Vineet

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

[Wolfram Sang]

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

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!

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

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

[Arnd Bergmann]

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>

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 =

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

[Christian Ruppert]

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 =

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

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

[Arnd Bergmann]

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 =

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

[Christian Ruppert]

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

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

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

[Arnd Bergmann]

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

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

[Arnd Bergmann]

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
 

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

[Christian Ruppert]

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?

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

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

[Stehle Vincent-B46079]

> 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.

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

[Arnd Bergmann]

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

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

[Andy Shevchenko]

On Mon, Jun 17, 2013 at 11:55 PM, Rob Herring <robherring2@gmail.com> wrote:

>> +Optional properties :
>> + - sda-hold-time : should contain the SDA hold time in nanoseconds.
>
> Please specify time units in the property name. Perhaps
> i2c-sda-hold-time-ns.
>
> Based on reading the discussion, there is one similar property I have
> found: "samsung,i2c-sda-delay = <100>". I wouldn't copy it as it doesn't
> tell the units or what the delay is.

Often on of the painful issues with DT models is the absence of the properties.
Should someone create the property definition first under
Documentation and _then_ patch everything around?
(Though I think better to think and create documents first and after
program accordingly)

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

On Tue, May 14, 2013 at 02:07:45PM +0300, Mika Westerberg wrote:
> On Tue, Apr 09, 2013 at 12:59:54PM +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>
> 
> Hi Wolfram,
> 
> What happened to this patch? I don't see it merged for 3.10.
> 
> The reason I'm asking is that I would like to add ACPI support for the SDA
> hold time parameter analogous to the DT version.

New bindings are complicated. I need to check if there is an
existing/similar one, check if it is generic enough for drivers still to
come, etc. I didn't have the time to do this for 3.10, so it is
3.11 material.

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Fri, May 17, 2013 at 10:29:28AM +0200, Wolfram Sang wrote:
> On Tue, May 14, 2013 at 02:07:45PM +0300, Mika Westerberg wrote:
> > On Tue, Apr 09, 2013 at 12:59:54PM +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>
> > 
> > Hi Wolfram,
> > 
> > What happened to this patch? I don't see it merged for 3.10.
> > 
> > The reason I'm asking is that I would like to add ACPI support for the SDA
> > hold time parameter analogous to the DT version.
> 
> New bindings are complicated. I need to check if there is an
> existing/similar one, check if it is generic enough for drivers still to
> come, etc. I didn't have the time to do this for 3.10, so it is
> 3.11 material.

OK, thanks. I just wanted to be sure this patch wasn't forgotten as it is
useful for Haswell/Lynxpoint as well.

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

> > New bindings are complicated. I need to check if there is an
> > existing/similar one, check if it is generic enough for drivers still to
> > come, etc. I didn't have the time to do this for 3.10, so it is
> > 3.11 material.
> 
> OK, thanks. I just wanted to be sure this patch wasn't forgotten as it is
> useful for Haswell/Lynxpoint as well.

i2c list is monitored by patchwork which is rigorously not forgetting ;)