[PATCH 1/2] i2c-designware: make *CNT values configurable

[PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
registers for each of the I2C speed modes (standard and fast). These
registers are programmed based on the input clock speed in the driver.

However, that is not always the most accurate way. For example on Intel
BayTrail we have 100MHz input clock and calculated *CNT values result as
measured by one of our team:

 Standard mode: 100.25kHz
 Fast mode    : 315.41kHZ

The fast mode speed is over 20% lower than what is expected. It might be
that there are things like strenght of the pull-up resistors, bus
capacitance etc. that are very platform specific and have an effect on the
clock signal.

With this patch we let the platform code to specify more accurate, optimal
*CNT values if they are known beforehand.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 46 +++++++++++++++++++-------------
 drivers/i2c/busses/i2c-designware-core.h | 12 +++++++++
 2 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index ad46616..f31469e 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -308,29 +308,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* set standard and fast speed deviders for high/low periods */
 
 	/* Standard-mode */
-	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				40,	/* tHD;STA = tHIGH = 4.0 us */
-				3,	/* tf = 0.3 us */
-				0,	/* 0: DW default, 1: Ideal */
-				0);	/* No offset */
-	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				47,	/* tLOW = 4.7 us */
-				3,	/* tf = 0.3 us */
-				0);	/* No offset */
+	if (dev->ss_hcnt && dev->ss_lcnt) {
+		hcnt = dev->ss_hcnt;
+		lcnt = dev->ss_lcnt;
+	} else {
+		hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+					40,	/* tHD;STA = tHIGH = 4.0 us */
+					3,	/* tf = 0.3 us */
+					0,	/* 0: DW default, 1: Ideal */
+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+					47,	/* tLOW = 4.7 us */
+					3,	/* tf = 0.3 us */
+					0);	/* No offset */
+	}
 	dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
 	dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
 	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
 	/* Fast-mode */
-	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				6,	/* tHD;STA = tHIGH = 0.6 us */
-				3,	/* tf = 0.3 us */
-				0,	/* 0: DW default, 1: Ideal */
-				0);	/* No offset */
-	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				13,	/* tLOW = 1.3 us */
-				3,	/* tf = 0.3 us */
-				0);	/* No offset */
+	if (dev->fs_hcnt && dev->fs_lcnt) {
+		hcnt = dev->fs_hcnt;
+		lcnt = dev->fs_lcnt;
+	} else {
+		hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+					6,	/* tHD;STA = tHIGH = 0.6 us */
+					3,	/* tf = 0.3 us */
+					0,	/* 0: DW default, 1: Ideal */
+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+					13,	/* tLOW = 1.3 us */
+					3,	/* tf = 0.3 us */
+					0);	/* No offset */
+	}
 	dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 912aa22..e8a7565 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -61,6 +61,14 @@
  * @tx_fifo_depth: depth of the hardware tx fifo
  * @rx_fifo_depth: depth of the hardware rx fifo
  * @rx_outstanding: current master-rx elements in tx fifo
+ * @ss_hcnt: standard speed HCNT value
+ * @ss_lcnt: standard speed LCNT value
+ * @fs_hcnt: fast speed HCNT value
+ * @fs_lcnt: fast speed LCNT value
+ *
+ * HCNT and LCNT parameters can be used if the platform knows more accurate
+ * values than the one computed based only on the input clock frequency.
+ * Leave them to be %0 if not used.
  */
 struct dw_i2c_dev {
 	struct device		*dev;
@@ -91,6 +99,10 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
 	u32			sda_hold_time;
+	u16			ss_hcnt;
+	u16			ss_lcnt;
+	u16			fs_hcnt;
+	u16			fs_lcnt;
 };
 
 #define ACCESS_SWAP		0x00000001
-- 
1.8.3.2

[PATCH 2/2] i2c-designware: configure *CNT values from ACPI

[Mika Westerberg]

Some Intel LPSS I2C devices make the *CNT values available from ACPI
namespace in similar way than the SDA hold value. These values allow
platform/BIOS to specify the most accurate *CNT values for the device
driver to use.

So change the ACPI part of the driver to use these values if they exists
and otherwise use the default method based solely on the input clock speed.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
This applies on top of following patch which is not yet in i2c tree:

http://permalink.gmane.org/gmane.linux.drivers.i2c/15754

 drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c7cfdac..a1488df 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -59,7 +59,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
 	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
-	acpi_string name;
 
 	if (!handle)
 		return -ENODEV;
@@ -68,14 +67,32 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	dev->tx_fifo_depth = 32;
 	dev->rx_fifo_depth = 32;
 
-	name = dev->master_cfg & DW_IC_CON_SPEED_FAST ? "FMCN" : "SSCN";
-	if (ACPI_SUCCESS(acpi_evaluate_object(handle, name, NULL, &buf))) {
+	/* Standard speed configuration */
+	if (ACPI_SUCCESS(acpi_evaluate_object(handle, "SSCN", NULL, &buf))) {
 		union acpi_object *obj = (union acpi_object *)buf.pointer;
 
 		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
 			const union acpi_object *objs = obj->package.elements;
 
-			dev->sda_hold_time = (u32)objs[2].integer.value;
+			dev->ss_hcnt = (u16)objs[0].integer.value;
+			dev->ss_lcnt = (u16)objs[1].integer.value;
+			if (!(dev->master_cfg & DW_IC_CON_SPEED_FAST))
+				dev->sda_hold_time = (u32)objs[2].integer.value;
+		}
+
+		kfree(buf.pointer);
+	}
+	/* Fast speed configuration */
+	if (ACPI_SUCCESS(acpi_evaluate_object(handle, "FMCN", NULL, &buf))) {
+		union acpi_object *obj = (union acpi_object *)buf.pointer;
+
+		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
+			const union acpi_object *objs = obj->package.elements;
+
+			dev->fs_hcnt = (u16)objs[0].integer.value;
+			dev->fs_lcnt = (u16)objs[1].integer.value;
+			if (dev->master_cfg & DW_IC_CON_SPEED_FAST)
+				dev->sda_hold_time = (u32)objs[2].integer.value;
 		}
 
 		kfree(buf.pointer);
-- 
1.8.3.2

Re: [PATCH 2/2] i2c-designware: configure *CNT values from ACPI

[Mika Westerberg]

On Mon, Jul 08, 2013 at 02:45:27PM +0300, Mika Westerberg wrote:
> Some Intel LPSS I2C devices make the *CNT values available from ACPI
> namespace in similar way than the SDA hold value. These values allow
> platform/BIOS to specify the most accurate *CNT values for the device
> driver to use.
> 
> So change the ACPI part of the driver to use these values if they exists
> and otherwise use the default method based solely on the input clock speed.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> This applies on top of following patch which is not yet in i2c tree:
> 
> http://permalink.gmane.org/gmane.linux.drivers.i2c/15754
> 
>  drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c7cfdac..a1488df 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -59,7 +59,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>  	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>  	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>  	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> -	acpi_string name;
>  
>  	if (!handle)
>  		return -ENODEV;
> @@ -68,14 +67,32 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>  	dev->tx_fifo_depth = 32;
>  	dev->rx_fifo_depth = 32;
>  
> -	name = dev->master_cfg & DW_IC_CON_SPEED_FAST ? "FMCN" : "SSCN";
> -	if (ACPI_SUCCESS(acpi_evaluate_object(handle, name, NULL, &buf))) {
> +	/* Standard speed configuration */
> +	if (ACPI_SUCCESS(acpi_evaluate_object(handle, "SSCN", NULL, &buf))) {
>  		union acpi_object *obj = (union acpi_object *)buf.pointer;
>  
>  		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
>  			const union acpi_object *objs = obj->package.elements;
>  
> -			dev->sda_hold_time = (u32)objs[2].integer.value;
> +			dev->ss_hcnt = (u16)objs[0].integer.value;
> +			dev->ss_lcnt = (u16)objs[1].integer.value;
> +			if (!(dev->master_cfg & DW_IC_CON_SPEED_FAST))
> +				dev->sda_hold_time = (u32)objs[2].integer.value;
> +		}
> +
> +		kfree(buf.pointer);
> +	}
> +	/* Fast speed configuration */
> +	if (ACPI_SUCCESS(acpi_evaluate_object(handle, "FMCN", NULL, &buf))) {

There is a problem with the above as it reuses buf from the previous call.
I'll post a new revision of the patches with this fixed.

> +		union acpi_object *obj = (union acpi_object *)buf.pointer;
> +
> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
> +			const union acpi_object *objs = obj->package.elements;
> +
> +			dev->fs_hcnt = (u16)objs[0].integer.value;
> +			dev->fs_lcnt = (u16)objs[1].integer.value;
> +			if (dev->master_cfg & DW_IC_CON_SPEED_FAST)
> +				dev->sda_hold_time = (u32)objs[2].integer.value;
>  		}
>  
>  		kfree(buf.pointer);
> -- 
> 1.8.3.2

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote:
> The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
> registers for each of the I2C speed modes (standard and fast). These
> registers are programmed based on the input clock speed in the driver.
> 
> However, that is not always the most accurate way. For example on Intel
> BayTrail we have 100MHz input clock and calculated *CNT values result as
> measured by one of our team:
> 
>  Standard mode: 100.25kHz
>  Fast mode    : 315.41kHZ
> 
> The fast mode speed is over 20% lower than what is expected.

We have observed similar issues and I am wondering if this effect is due
to the overly-pessimistic formulas in i2c_dw_scl_lcnt and
i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and
I don't pretend having fully understood what the intention is. I'm
having the impression that defining the arguments tf in function of the
hardware would be the "correct" way to achieve better clock accuracy.

> It might be
> that there are things like strenght of the pull-up resistors, bus
> capacitance etc. that are very platform specific and have an effect on the
> clock signal.

I believe tf is supposed to model these things. I just haven't clearly
understood how. In my understanding, transition times should be
subtracted rather than added to cycle times or am I getting something
fundamentally wrong here?

> With this patch we let the platform code to specify more accurate, optimal
> *CNT values if they are known beforehand.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 46 +++++++++++++++++++-------------
>  drivers/i2c/busses/i2c-designware-core.h | 12 +++++++++
>  2 files changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index ad46616..f31469e 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -308,29 +308,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	/* set standard and fast speed deviders for high/low periods */
>  
>  	/* Standard-mode */
> -	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
> -				40,	/* tHD;STA = tHIGH = 4.0 us */
> -				3,	/* tf = 0.3 us */
> -				0,	/* 0: DW default, 1: Ideal */
> -				0);	/* No offset */
> -	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> -				47,	/* tLOW = 4.7 us */
> -				3,	/* tf = 0.3 us */
> -				0);	/* No offset */
> +	if (dev->ss_hcnt && dev->ss_lcnt) {
> +		hcnt = dev->ss_hcnt;
> +		lcnt = dev->ss_lcnt;
> +	} else {
> +		hcnt = i2c_dw_scl_hcnt(input_clock_khz,
> +					40,	/* tHD;STA = tHIGH = 4.0 us */
> +					3,	/* tf = 0.3 us */
> +					0,	/* 0: DW default, 1: Ideal */
> +					0);	/* No offset */
> +		lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> +					47,	/* tLOW = 4.7 us */
> +					3,	/* tf = 0.3 us */
> +					0);	/* No offset */
> +	}
>  	dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
>  	dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
>  	/* Fast-mode */
> -	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
> -				6,	/* tHD;STA = tHIGH = 0.6 us */
> -				3,	/* tf = 0.3 us */
> -				0,	/* 0: DW default, 1: Ideal */
> -				0);	/* No offset */
> -	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> -				13,	/* tLOW = 1.3 us */
> -				3,	/* tf = 0.3 us */
> -				0);	/* No offset */
> +	if (dev->fs_hcnt && dev->fs_lcnt) {
> +		hcnt = dev->fs_hcnt;
> +		lcnt = dev->fs_lcnt;
> +	} else {
> +		hcnt = i2c_dw_scl_hcnt(input_clock_khz,
> +					6,	/* tHD;STA = tHIGH = 0.6 us */
> +					3,	/* tf = 0.3 us */
> +					0,	/* 0: DW default, 1: Ideal */
> +					0);	/* No offset */
> +		lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> +					13,	/* tLOW = 1.3 us */
> +					3,	/* tf = 0.3 us */
> +					0);	/* No offset */
> +	}
>  	dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 912aa22..e8a7565 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -61,6 +61,14 @@
>   * @tx_fifo_depth: depth of the hardware tx fifo
>   * @rx_fifo_depth: depth of the hardware rx fifo
>   * @rx_outstanding: current master-rx elements in tx fifo
> + * @ss_hcnt: standard speed HCNT value
> + * @ss_lcnt: standard speed LCNT value
> + * @fs_hcnt: fast speed HCNT value
> + * @fs_lcnt: fast speed LCNT value
> + *
> + * HCNT and LCNT parameters can be used if the platform knows more accurate
> + * values than the one computed based only on the input clock frequency.
> + * Leave them to be %0 if not used.
>   */
>  struct dw_i2c_dev {
>  	struct device		*dev;
> @@ -91,6 +99,10 @@ struct dw_i2c_dev {
>  	unsigned int		rx_fifo_depth;
>  	int			rx_outstanding;
>  	u32			sda_hold_time;
> +	u16			ss_hcnt;
> +	u16			ss_lcnt;
> +	u16			fs_hcnt;
> +	u16			fs_lcnt;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> -- 
> 1.8.3.2
> 

-- 
  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 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Mon, Jul 08, 2013 at 03:42:17PM +0200, Christian Ruppert wrote:
> On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote:
> > The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
> > registers for each of the I2C speed modes (standard and fast). These
> > registers are programmed based on the input clock speed in the driver.
> > 
> > However, that is not always the most accurate way. For example on Intel
> > BayTrail we have 100MHz input clock and calculated *CNT values result as
> > measured by one of our team:
> > 
> >  Standard mode: 100.25kHz
> >  Fast mode    : 315.41kHZ
> > 
> > The fast mode speed is over 20% lower than what is expected.
> 
> We have observed similar issues and I am wondering if this effect is due
> to the overly-pessimistic formulas in i2c_dw_scl_lcnt and
> i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and
> I don't pretend having fully understood what the intention is. I'm
> having the impression that defining the arguments tf in function of the
> hardware would be the "correct" way to achieve better clock accuracy.

Well, that's not the only timing parameter specified in the I2C spec. We
also have tr among other things (even though the dw driver doesn't use it).

> > It might be
> > that there are things like strenght of the pull-up resistors, bus
> > capacitance etc. that are very platform specific and have an effect on the
> > clock signal.
> 
> I believe tf is supposed to model these things. I just haven't clearly
> understood how. In my understanding, transition times should be
> subtracted rather than added to cycle times or am I getting something
> fundamentally wrong here?

I'm not sure, honestly :-) I believe tf is the same tf that is in the I2C
spec, meaning fall time of both SCL and SDA signals and the spec measures
one clock cycle to be from end of the first tf to the end of the second
(fig. 27 in the I2C spec). If I'm reading it right then it means adding
instead of substracting.

Do you have any suggestions how we could use tf here? At least on Intel
hardware we have an ACPI method that returns directly the optimum *CNT
values.

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Tue, Jul 09, 2013 at 11:44:02AM +0300, Mika Westerberg wrote:
> On Mon, Jul 08, 2013 at 03:42:17PM +0200, Christian Ruppert wrote:
> > On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote:
> > > The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
> > > registers for each of the I2C speed modes (standard and fast). These
> > > registers are programmed based on the input clock speed in the driver.
> > > 
> > > However, that is not always the most accurate way. For example on Intel
> > > BayTrail we have 100MHz input clock and calculated *CNT values result as
> > > measured by one of our team:
> > > 
> > >  Standard mode: 100.25kHz
> > >  Fast mode    : 315.41kHZ
> > > 
> > > The fast mode speed is over 20% lower than what is expected.
> > 
> > We have observed similar issues and I am wondering if this effect is due
> > to the overly-pessimistic formulas in i2c_dw_scl_lcnt and
> > i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and
> > I don't pretend having fully understood what the intention is. I'm
> > having the impression that defining the arguments tf in function of the
> > hardware would be the "correct" way to achieve better clock accuracy.
> 
> Well, that's not the only timing parameter specified in the I2C spec. We
> also have tr among other things (even though the dw driver doesn't use it).

Exactly. Ant T_HD;STA which is mentioned somewhere in the comment and
one of the reasons why I never dared touching these functions. The fact
that the designware IP doesn't seem to manage all timings makes this
function quite difficult to understand.

> > > It might be
> > > that there are things like strenght of the pull-up resistors, bus
> > > capacitance etc. that are very platform specific and have an effect on the
> > > clock signal.
> > 
> > I believe tf is supposed to model these things. I just haven't clearly
> > understood how. In my understanding, transition times should be
> > subtracted rather than added to cycle times or am I getting something
> > fundamentally wrong here?
> 
> I'm not sure, honestly :-) I believe tf is the same tf that is in the I2C
> spec, meaning fall time of both SCL and SDA signals and the spec measures
> one clock cycle to be from end of the first tf to the end of the second
> (fig. 27 in the I2C spec). If I'm reading it right then it means adding
> instead of substracting.
> Do you have any suggestions how we could use tf here?

What I meant is the following: The clock cycle time Tc is composed of
the four components

  Tc = Th + Tf + Tl + Tr

where
  Th: Time during which the signal is high
  Tf: Falling edge transition time
  Tl: Time during which the signal is low
  Tr: Rising edge transition time

The I2C specification specifies a minimum for Tl and Th and a range (or
maximum) for Tr and Tf. A maximum frequency is specified as the
frequency obtained by adding the minima for Th and Tl to the maxima of
Tr ant Tf.
Since as you said, transition times are very much PCB dependent, one way
to guarantee the max. frequency constraint (and to achieve a constant
frequency at its max) is to define the constants
Th' = Th + Tf := Th_min + Tf_max
Tl' = Tl + Tr := Tl_min + Tr_max

and to calculate the variables
Th = Th' - Tf
Tl = Tl' - Tr
in function of Tf and Tr of the given PCB.

> At least on Intel
> hardware we have an ACPI method that returns directly the optimum *CNT
> values.

I don't know ACPI very well and if it deals with register values
directly your patch is probably the right thing. The timing calculation
in the driver seems a bit strange to me, however (see above), but I
never dared touching it because I never had time to dive into the code
deep enough and I was just wondering if it was possible to fix it at the
same time. If ACPI bypasses the function alltogether this is probably
not applicable.

-- 
  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 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> What I meant is the following: The clock cycle time Tc is composed of
> the four components
> 
>   Tc = Th + Tf + Tl + Tr
> 
> where
>   Th: Time during which the signal is high
>   Tf: Falling edge transition time
>   Tl: Time during which the signal is low
>   Tr: Rising edge transition time
> 
> The I2C specification specifies a minimum for Tl and Th and a range (or
> maximum) for Tr and Tf. A maximum frequency is specified as the
> frequency obtained by adding the minima for Th and Tl to the maxima of
> Tr ant Tf.
> Since as you said, transition times are very much PCB dependent, one way
> to guarantee the max. frequency constraint (and to achieve a constant
> frequency at its max) is to define the constants
> Th' = Th + Tf := Th_min + Tf_max
> Tl' = Tl + Tr := Tl_min + Tr_max
> 
> and to calculate the variables
> Th = Th' - Tf
> Tl = Tl' - Tr
> in function of Tf and Tr of the given PCB.

If I understand the above, it leaves Tf and Tr to be PCB specific and then
these values are passed to the core driver from platform data, right?

I'm thinking that passing directly the "measured" *CNT values from the
platform data makes this even more accurate (given that information is
available). And if you only have the Tf and Tr for your board, you can have
custom calculation done in the platform part of the driver that takes them
into account, and then passes these custom *CNT values to the core.

> > At least on Intel
> > hardware we have an ACPI method that returns directly the optimum *CNT
> > values.
> 
> I don't know ACPI very well and if it deals with register values
> directly your patch is probably the right thing.

Our FW people decided to have a custom ACPI method that returns the correct
values to be used in the Windows I2C driver. It returns direct *CNT
register values that have been measured to be good for a given PCB.

> The timing calculation in the driver seems a bit strange to me, however
> (see above), but I never dared touching it because I never had time to
> dive into the code deep enough and I was just wondering if it was
> possible to fix it at the same time.

It would be good if we can fix this for non-ACPI devices as well. Is it
hard to add similar properties to the driver specific Device Tree bindings?

Wolfram, do you have any input on this?

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> > What I meant is the following: The clock cycle time Tc is composed of
> > the four components
> > 
> >   Tc = Th + Tf + Tl + Tr
> > 
> > where
> >   Th: Time during which the signal is high
> >   Tf: Falling edge transition time
> >   Tl: Time during which the signal is low
> >   Tr: Rising edge transition time
> > 
> > The I2C specification specifies a minimum for Tl and Th and a range (or
> > maximum) for Tr and Tf. A maximum frequency is specified as the
> > frequency obtained by adding the minima for Th and Tl to the maxima of
> > Tr ant Tf.
> > Since as you said, transition times are very much PCB dependent, one way
> > to guarantee the max. frequency constraint (and to achieve a constant
> > frequency at its max) is to define the constants
> > Th' = Th + Tf := Th_min + Tf_max
> > Tl' = Tl + Tr := Tl_min + Tr_max
> > 
> > and to calculate the variables
> > Th = Th' - Tf
> > Tl = Tl' - Tr
> > in function of Tf and Tr of the given PCB.
> 
> If I understand the above, it leaves Tf and Tr to be PCB specific and then
> these values are passed to the core driver from platform data, right?

That would be the idea: Calculate Th' and Tl' in function of the desired
clock frequency and duty cycle and then adapt these values using
measured transition times. What prevented me from implementing this
rather academic approach are the following comments in
i2c-designware-core.c:

/*
 * DesignWare I2C core doesn't seem to have solid strategy to meet
 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
 * will result in violation of the tHD;STA spec.
 */

/* ...
 * This is just experimental rule; the tHD;STA period
 * turned out to be proportinal to (_HCNT + 3).  With this setting,
 * we could meet both tHIGH and tHD;STA timing specs.
 * ...
 */

If I interpret this right, the slow down of the clock is intentional to
meet tHD;STA timing constraints.

> I'm thinking that passing directly the "measured" *CNT values from the
> platform data makes this even more accurate (given that information is
> available). And if you only have the Tf and Tr for your board, you can have
> custom calculation done in the platform part of the driver that takes them
> into account, and then passes these custom *CNT values to the core.

Well, as in the previous discussion on SDA hold time, Tf and Tr are
physical values whereas the register values are clock dependent and
probably not appropriate at least for device tree usage (or cases where
the clock speed might change). If I understand you correctly, ACPI is
more register oriented and able to cope with this outside of the OS?

> > > At least on Intel
> > > hardware we have an ACPI method that returns directly the optimum *CNT
> > > values.
> > 
> > I don't know ACPI very well and if it deals with register values
> > directly your patch is probably the right thing.
> 
> Our FW people decided to have a custom ACPI method that returns the correct
> values to be used in the Windows I2C driver. It returns direct *CNT
> register values that have been measured to be good for a given PCB.
> 
> > The timing calculation in the driver seems a bit strange to me, however
> > (see above), but I never dared touching it because I never had time to
> > dive into the code deep enough and I was just wondering if it was
> > possible to fix it at the same time.
> 
> It would be good if we can fix this for non-ACPI devices as well. Is it
> hard to add similar properties to the driver specific Device Tree bindings?

I think it would be quite trivial to add properties for either the
register values or the transition time values. For SDA hold time we
concluded that time values would be more adequate which brings us to the
question of better understanding the hack for tHD;SDA. Otherwise we
might break something. Do you think your team which determines the
tHD;SDA values could give us some guidance on that?

> Wolfram, do you have any input on this?

-- 
  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 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> > > What I meant is the following: The clock cycle time Tc is composed of
> > > the four components
> > > 
> > >   Tc = Th + Tf + Tl + Tr
> > > 
> > > where
> > >   Th: Time during which the signal is high
> > >   Tf: Falling edge transition time
> > >   Tl: Time during which the signal is low
> > >   Tr: Rising edge transition time
> > > 
> > > The I2C specification specifies a minimum for Tl and Th and a range (or
> > > maximum) for Tr and Tf. A maximum frequency is specified as the
> > > frequency obtained by adding the minima for Th and Tl to the maxima of
> > > Tr ant Tf.
> > > Since as you said, transition times are very much PCB dependent, one way
> > > to guarantee the max. frequency constraint (and to achieve a constant
> > > frequency at its max) is to define the constants
> > > Th' = Th + Tf := Th_min + Tf_max
> > > Tl' = Tl + Tr := Tl_min + Tr_max
> > > 
> > > and to calculate the variables
> > > Th = Th' - Tf
> > > Tl = Tl' - Tr
> > > in function of Tf and Tr of the given PCB.
> > 
> > If I understand the above, it leaves Tf and Tr to be PCB specific and then
> > these values are passed to the core driver from platform data, right?
> 
> That would be the idea: Calculate Th' and Tl' in function of the desired
> clock frequency and duty cycle and then adapt these values using
> measured transition times. What prevented me from implementing this
> rather academic approach are the following comments in
> i2c-designware-core.c:
> 
> /*
>  * DesignWare I2C core doesn't seem to have solid strategy to meet
>  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
>  * will result in violation of the tHD;STA spec.
>  */
> 
> /* ...
>  * This is just experimental rule; the tHD;STA period
>  * turned out to be proportinal to (_HCNT + 3).  With this setting,
>  * we could meet both tHIGH and tHD;STA timing specs.
>  * ...
>  */
> 
> If I interpret this right, the slow down of the clock is intentional to
> meet tHD;STA timing constraints.

Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
comments apply to some earlier version of the IP that didn't have the SDA
hold register?

> > I'm thinking that passing directly the "measured" *CNT values from the
> > platform data makes this even more accurate (given that information is
> > available). And if you only have the Tf and Tr for your board, you can have
> > custom calculation done in the platform part of the driver that takes them
> > into account, and then passes these custom *CNT values to the core.
> 
> Well, as in the previous discussion on SDA hold time, Tf and Tr are
> physical values whereas the register values are clock dependent and
> probably not appropriate at least for device tree usage (or cases where
> the clock speed might change). If I understand you correctly, ACPI is
> more register oriented and able to cope with this outside of the OS?

Well, ACPI doesn't care what its methods return (unless the methods are
specfied in the ACPI spec). In this case it just returns a package of three
values: HCNT, LCNT and SDA hold time.

I guess it is supposed to work like this:

 * If there is no vendor specific SSCN/FMCN methods for the device we can
   just use the default and calculate *CNT from the clock speed.
 * If there exists a method, we use the values there instead.

I think the Windows driver does something like above. IMHO we should to the
same in Linux driver to be able to take advantage of the measured *CNT
values.

> > > > At least on Intel
> > > > hardware we have an ACPI method that returns directly the optimum *CNT
> > > > values.
> > > 
> > > I don't know ACPI very well and if it deals with register values
> > > directly your patch is probably the right thing.
> > 
> > Our FW people decided to have a custom ACPI method that returns the correct
> > values to be used in the Windows I2C driver. It returns direct *CNT
> > register values that have been measured to be good for a given PCB.
> > 
> > > The timing calculation in the driver seems a bit strange to me, however
> > > (see above), but I never dared touching it because I never had time to
> > > dive into the code deep enough and I was just wondering if it was
> > > possible to fix it at the same time.
> > 
> > It would be good if we can fix this for non-ACPI devices as well. Is it
> > hard to add similar properties to the driver specific Device Tree bindings?
> 
> I think it would be quite trivial to add properties for either the
> register values or the transition time values. For SDA hold time we
> concluded that time values would be more adequate which brings us to the
> question of better understanding the hack for tHD;SDA. Otherwise we
> might break something. Do you think your team which determines the
> tHD;SDA values could give us some guidance on that?

I2C spec says following about the tHD;STA:

 Standard mode	: min 4.0 us
 Fast mode	: min 0.6 us

But I can try to ask from our HW guys for an answer.

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote:
> On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> > On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> > > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> > > > What I meant is the following: The clock cycle time Tc is composed of
> > > > the four components
> > > > 
> > > >   Tc = Th + Tf + Tl + Tr
> > > > 
> > > > where
> > > >   Th: Time during which the signal is high
> > > >   Tf: Falling edge transition time
> > > >   Tl: Time during which the signal is low
> > > >   Tr: Rising edge transition time
> > > > 
> > > > The I2C specification specifies a minimum for Tl and Th and a range (or
> > > > maximum) for Tr and Tf. A maximum frequency is specified as the
> > > > frequency obtained by adding the minima for Th and Tl to the maxima of
> > > > Tr ant Tf.
> > > > Since as you said, transition times are very much PCB dependent, one way
> > > > to guarantee the max. frequency constraint (and to achieve a constant
> > > > frequency at its max) is to define the constants
> > > > Th' = Th + Tf := Th_min + Tf_max
> > > > Tl' = Tl + Tr := Tl_min + Tr_max
> > > > 
> > > > and to calculate the variables
> > > > Th = Th' - Tf
> > > > Tl = Tl' - Tr
> > > > in function of Tf and Tr of the given PCB.
> > > 
> > > If I understand the above, it leaves Tf and Tr to be PCB specific and then
> > > these values are passed to the core driver from platform data, right?
> > 
> > That would be the idea: Calculate Th' and Tl' in function of the desired
> > clock frequency and duty cycle and then adapt these values using
> > measured transition times. What prevented me from implementing this
> > rather academic approach are the following comments in
> > i2c-designware-core.c:
> > 
> > /*
> >  * DesignWare I2C core doesn't seem to have solid strategy to meet
> >  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
> >  * will result in violation of the tHD;STA spec.
> >  */
> > 
> > /* ...
> >  * This is just experimental rule; the tHD;STA period
> >  * turned out to be proportinal to (_HCNT + 3).  With this setting,
> >  * we could meet both tHIGH and tHD;STA timing specs.
> >  * ...
> >  */
> > 
> > If I interpret this right, the slow down of the clock is intentional to
> > meet tHD;STA timing constraints.
> 
> Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
> comments apply to some earlier version of the IP that didn't have the SDA
> hold register?

Scratch that.

I re-read the spec and tHD;STA is hold time for (repeated) start. There is
a constraint that says that the device must internally provide a hold time
of at least 300ns for the SDA signal. Maybe that's the constraint the
comment above is referring to?

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

On 7/11/13 7:13 PM, Mika Westerberg wrote:
> On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote:
>> On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
>>> On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
>>>> On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
>>>>> What I meant is the following: The clock cycle time Tc is composed of
>>>>> the four components
>>>>>
>>>>>    Tc = Th + Tf + Tl + Tr
>>>>>
>>>>> where
>>>>>    Th: Time during which the signal is high
>>>>>    Tf: Falling edge transition time
>>>>>    Tl: Time during which the signal is low
>>>>>    Tr: Rising edge transition time
>>>>>
>>>>> The I2C specification specifies a minimum for Tl and Th and a range (or
>>>>> maximum) for Tr and Tf. A maximum frequency is specified as the
>>>>> frequency obtained by adding the minima for Th and Tl to the maxima of
>>>>> Tr ant Tf.
>>>>> Since as you said, transition times are very much PCB dependent, one way
>>>>> to guarantee the max. frequency constraint (and to achieve a constant
>>>>> frequency at its max) is to define the constants
>>>>> Th' = Th + Tf := Th_min + Tf_max
>>>>> Tl' = Tl + Tr := Tl_min + Tr_max
>>>>>
>>>>> and to calculate the variables
>>>>> Th = Th' - Tf
>>>>> Tl = Tl' - Tr
>>>>> in function of Tf and Tr of the given PCB.
>>>>
>>>> If I understand the above, it leaves Tf and Tr to be PCB specific and then
>>>> these values are passed to the core driver from platform data, right?
>>>
>>> That would be the idea: Calculate Th' and Tl' in function of the desired
>>> clock frequency and duty cycle and then adapt these values using
>>> measured transition times. What prevented me from implementing this
>>> rather academic approach are the following comments in
>>> i2c-designware-core.c:

When we talk about I2C timing specs, we should not bring up "clock speed"
things.  All we have to do is to strictly meet timing constraints of
tHIGH, tLOW, tHD;SATA, tr, tf, etc.  The resulting "clock speed" is not
a goal.

>>> /*
>>>   * DesignWare I2C core doesn't seem to have solid strategy to meet
>>>   * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
>>>   * will result in violation of the tHD;STA spec.
>>>   */
>>>
>>> /* ...
>>>   * This is just experimental rule; the tHD;STA period
>>>   * turned out to be proportinal to (_HCNT + 3).  With this setting,
>>>   * we could meet both tHIGH and tHD;STA timing specs.
>>>   * ...
>>>   */
>>>
>>> If I interpret this right, the slow down of the clock is intentional to
>>> meet tHD;STA timing constraints.

Correct.

>> Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
>> comments apply to some earlier version of the IP that didn't have the SDA
>> hold register?

If I remember DesignWare APB I2C spec correctly, SDA hold time register
doesn't help to meet tHD;STA spec.  Could someone confirm it really so
with a real hardware, please?

   Shinya

> Scratch that.
>
> I re-read the spec and tHD;STA is hold time for (repeated) start. There is
> a constraint that says that the device must internally provide a hold time
> of at least 300ns for the SDA signal. Maybe that's the constraint the
> comment above is referring to?
>

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Fri, Jul 12, 2013 at 04:56:49PM +0900, Shinya Kuribayashi wrote:
> On 7/11/13 7:13 PM, Mika Westerberg wrote:
> >On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote:
> >>On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> >>>On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> >>>>On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> >>>>>What I meant is the following: The clock cycle time Tc is composed of
> >>>>>the four components
> >>>>>
> >>>>>   Tc = Th + Tf + Tl + Tr
> >>>>>
> >>>>>where
> >>>>>   Th: Time during which the signal is high
> >>>>>   Tf: Falling edge transition time
> >>>>>   Tl: Time during which the signal is low
> >>>>>   Tr: Rising edge transition time
> >>>>>
> >>>>>The I2C specification specifies a minimum for Tl and Th and a range (or
> >>>>>maximum) for Tr and Tf. A maximum frequency is specified as the
> >>>>>frequency obtained by adding the minima for Th and Tl to the maxima of
> >>>>>Tr ant Tf.
> >>>>>Since as you said, transition times are very much PCB dependent, one way
> >>>>>to guarantee the max. frequency constraint (and to achieve a constant
> >>>>>frequency at its max) is to define the constants
> >>>>>Th' = Th + Tf := Th_min + Tf_max
> >>>>>Tl' = Tl + Tr := Tl_min + Tr_max
> >>>>>
> >>>>>and to calculate the variables
> >>>>>Th = Th' - Tf
> >>>>>Tl = Tl' - Tr
> >>>>>in function of Tf and Tr of the given PCB.
> >>>>
> >>>>If I understand the above, it leaves Tf and Tr to be PCB specific and then
> >>>>these values are passed to the core driver from platform data, right?
> >>>
> >>>That would be the idea: Calculate Th' and Tl' in function of the desired
> >>>clock frequency and duty cycle and then adapt these values using
> >>>measured transition times. What prevented me from implementing this
> >>>rather academic approach are the following comments in
> >>>i2c-designware-core.c:
> 
> When we talk about I2C timing specs, we should not bring up "clock speed"
> things.  All we have to do is to strictly meet timing constraints of
> tHIGH, tLOW, tHD;SATA, tr, tf, etc.  The resulting "clock speed" is not
> a goal.

OK, thanks for the explanation.

I'm relatively new with I2C bus even though I've adapted the DW I2C driver
to work on our HW.

> >>>/*
> >>>  * DesignWare I2C core doesn't seem to have solid strategy to meet
> >>>  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
> >>>  * will result in violation of the tHD;STA spec.
> >>>  */
> >>>
> >>>/* ...
> >>>  * This is just experimental rule; the tHD;STA period
> >>>  * turned out to be proportinal to (_HCNT + 3).  With this setting,
> >>>  * we could meet both tHIGH and tHD;STA timing specs.
> >>>  * ...
> >>>  */
> >>>
> >>>If I interpret this right, the slow down of the clock is intentional to
> >>>meet tHD;STA timing constraints.
> 
> Correct.
> 
> >>Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
> >>comments apply to some earlier version of the IP that didn't have the SDA
> >>hold register?
> 
> If I remember DesignWare APB I2C spec correctly, SDA hold time register
> doesn't help to meet tHD;STA spec.  Could someone confirm it really so
> with a real hardware, please?

Indeed, SDA hold in the DesignWare I2C is not the same as tHD;STA according
the databook. Unfortunately I don't have means to actually measure that
here.

Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
methods are measured by our HW guys on a certain board and they have
verified that using those we meet all the I2C timing specs.

In order to take advantage of those we need some way to pass those values
to the i2c designware core. I have two suggestions:
  
  - Use the method outlined in this patch and let the interface driver
    override *CNT values.

  - Allow interface drivers to override the function that does calculations
    for these values like:

	if (dev->std_scl_cnt)
		dev->std_scl_cnt(dev, &hcnt, &lcnt);
	else
		/* Fallback to the calculation based solely on iclk */

Any comments on these?

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

Hi,

Now I've had a look at the whole discussion.

Basically, DW I2C core provides a good enough (and quite direct) way
to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.

But from my experience (with a slightly old version of DW I2C core
around 2005, version 1.06a or so), DW I2C core was apparently lacking
in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
then found that we could meet tHD;STA by increasing *HCNT values, so
that's what currently we have in i2c-designware.c  I know with that
workaround applied, tHIGH is to be configured with a larger value
than necessary, but we have no choice.  For I2C bus systems, we must
meet every timing constraint strictly as required.  If DW I2C core
cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
it a hardware bug.

On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
>> If I understand the above, it leaves Tf and Tr to be PCB specific and then
>> these values are passed to the core driver from platform data, right?
>
> That would be the idea: Calculate Th' and Tl' in function of the desired
> clock frequency and duty cycle and then adapt these values using
> measured transition times.

I think this would be a good solution.

On 7/8/13 10:42 PM, Christian Ruppert wrote:
> Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
> methods are measured by our HW guys on a certain board and they have
> verified that using those we meet all the I2C timing specs.
>
> In order to take advantage of those we need some way to pass those values
> to the i2c designware core. I have two suggestions:
>
>    - Use the method outlined in this patch and let the interface driver
>      override *CNT values.

With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA
quite accurately.  On the other hand, what we can't control with DW
I2C core is tr and tf.  I assumed that it could never be longer than
300nsec (it's defined as a max. in the I2C specification), so I used
it for calculation.

I agree that tr and tf are PCB specific, and it would a good first
step toward timing optimization to make them configurable through
platform data.

Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
calculations don't suit with later DW I2C cores, then it would be
nice for someone who can access to the data book to update formulas,
or provide alternative formulas and make them selectable depending
on DW core versions.

It always helps us to have a way to calculate *HCNT and *LCNT values
automatically.  As said above, DW I2C core can control tHIGH, tLOW,
tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
with proper formulas.  It also helps hardware people as well to
provide reference HCNT/LCNT values.

And as a third step, if we want to use optimized HCNT/LCNT values
which can not be obtained from proper formulas + user-requested
tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
or provided from hardware team, then I'm fine with having a way to
_override_ HCNT/LCNT values.  Such direct way might be useful.

>    - Allow interface drivers to override the function that does calculations
>      for these values like:
>
> 	if (dev->std_scl_cnt)
> 		dev->std_scl_cnt(dev, &hcnt, &lcnt);
> 	else
> 		/* Fallback to the calculation based solely on iclk */

To make the code having less indentations and be more clear that *CNT
values are being overridden, something like this would be nice (leave
more good comments if necessary I'll leave it to you):

  	/* set standard and fast speed deviders for high/low periods */
  
  	/* Standard-mode */
  	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
  				40,	/* tHD;STA = tHIGH = 4.0 us */
  				3,	/* tf = 0.3 us */
  				0,	/* 0: DW default, 1: Ideal */
  				0);	/* No offset */
  	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
  				47,	/* tLOW = 4.7 us */
  				3,	/* tf = 0.3 us */
  				0);	/* No offset */
+	if (dev->ss_hcnt && dev->ss_lcnt) {
+		/* give preference to immediate values over optimal ones */
+		hcnt = dev->ss_hcnt;
+		lcnt = dev->ss_lcnt;
+	}
  	dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
  	dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
  	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);

   Shinya

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote:
> Hi,
> 
> Now I've had a look at the whole discussion.
> 
> Basically, DW I2C core provides a good enough (and quite direct) way
> to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
> 
> But from my experience (with a slightly old version of DW I2C core
> around 2005, version 1.06a or so), DW I2C core was apparently lacking
> in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
> then found that we could meet tHD;STA by increasing *HCNT values, so
> that's what currently we have in i2c-designware.c  I know with that
> workaround applied, tHIGH is to be configured with a larger value
> than necessary, but we have no choice.  For I2C bus systems, we must
> meet every timing constraint strictly as required.  If DW I2C core
> cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
> it a hardware bug.

I agree. However, tHD;STA [min] is defined to the same value as tHIGH
[min] for all modes in the I2C specification. Do I understand you
correctly that the SCL_HCNT parameters control at the same time tHIGH
and tHD;STA?

Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and
it looks like the delay between the falling edge clock of the start
condition and the rising edge of the first data bit of the start byte is
controlled by this parameter. I conclude that in the drawing below (1)
is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME.

     _________
scl           \__________ ...
     ___           ______
sda     \_________/       ...

        |-----|---|
          (1)  (2)

> On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> >>If I understand the above, it leaves Tf and Tr to be PCB specific and then
> >>these values are passed to the core driver from platform data, right?
> >
> >That would be the idea: Calculate Th' and Tl' in function of the desired
> >clock frequency and duty cycle and then adapt these values using
> >measured transition times.
> 
> I think this would be a good solution.
> 
> On 7/8/13 10:42 PM, Christian Ruppert wrote:
> >Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
> >methods are measured by our HW guys on a certain board and they have
> >verified that using those we meet all the I2C timing specs.
> >
> >In order to take advantage of those we need some way to pass those values
> >to the i2c designware core. I have two suggestions:
> >
> >   - Use the method outlined in this patch and let the interface driver
> >     override *CNT values.
> 
> With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA
> quite accurately.  On the other hand, what we can't control with DW
> I2C core is tr and tf.  I assumed that it could never be longer than
> 300nsec (it's defined as a max. in the I2C specification), so I used
> it for calculation.
> 
> I agree that tr and tf are PCB specific, and it would a good first
> step toward timing optimization to make them configurable through
> platform data.
> 
> Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
> calculations don't suit with later DW I2C cores, then it would be
> nice for someone who can access to the data book to update formulas,
> or provide alternative formulas and make them selectable depending
> on DW core versions.

I'm not having the impression there is a huge difference between the
different generations of DW blocks. Probably we can find one formula
that suits all blocks. We just have to be careful (in doubt rather
conservative) with the transition times. This seems to be currently
the case and if I understand Mika correctly, his objective is to remove
some of this conservatism.

> It always helps us to have a way to calculate *HCNT and *LCNT values
> automatically.  As said above, DW I2C core can control tHIGH, tLOW,
> tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
> with proper formulas.  It also helps hardware people as well to
> provide reference HCNT/LCNT values.
> 
> And as a third step, if we want to use optimized HCNT/LCNT values
> which can not be obtained from proper formulas + user-requested
> tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
> or provided from hardware team, then I'm fine with having a way to
> _override_ HCNT/LCNT values.  Such direct way might be useful.

I agree. Probably it is best to have both, a default method based on
formulas and timing parameters (the formulas are quite simple anyway)
which works with device tree and such and a second method based on
register values which works with mechanisms like ACPI.

Christian

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote:
>> Basically, DW I2C core provides a good enough (and quite direct) way
>> to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
>>
>> But from my experience (with a slightly old version of DW I2C core
>> around 2005, version 1.06a or so), DW I2C core was apparently lacking
>> in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
>> then found that we could meet tHD;STA by increasing *HCNT values, so
>> that's what currently we have in i2c-designware.c  I know with that
>> workaround applied, tHIGH is to be configured with a larger value
>> than necessary, but we have no choice.  For I2C bus systems, we must
>> meet every timing constraint strictly as required.  If DW I2C core
>> cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
>> it a hardware bug.
>
> I agree. However, tHD;STA [min] is defined to the same value as tHIGH
> [min] for all modes in the I2C specification. Do I understand you
> correctly that the SCL_HCNT parameters control at the same time tHIGH
> and tHD;STA?

*HCNT value does affect both tHIGH and tHD;STA at the same time.
But we have to make sure that composition of tHIGH is different
from the one of tHD;STA.

For tHIGH
----------

DW I2C core is aware of the SCL rising transition (tr) and can
ignore it.  It starts counting the HIGH period of the SCL signal
(tHIGH) after the SCL input voltage increases at VIH.

This is described in the data book as an ideal configuration,
and the resulting formula would be:

	HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a)

please refer to the data book for details about (1+4+3) part.

For tLOW
--------

DW I2C core starts counting the SCL CNTs for the LOW period of
the SCL signal (tLOW) as soon as it pulls the SCL line _without_
_confirming_ the SCL input voltage decreases at VIL.

In order to meet the tLOW timing spec, we need to take into
account the fall time of SCL signal (tf), so the resulting
formula should be:

	LCNT + 1 >= IC_CLK * (tLOW + tf)

please refer to the data book for details about '+1' part.

For tHD;STA
-----------

There is no explanation about tHD;STA in the data book.  We
only have (my) experimental result; tHD;STA turned out to be
proportional to 'HCNT + 3'.  The formula is:

	HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b)

DW I2C core seems to start counting the SCL CNTs for the HIGH
period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_
line without confirming the SDA input voltage decreases at VIL,
so we have to take into account the SDA falling transition (tf)
here.

As can be expected from (a) and (b), if tHD;STA [min] is defined
to the same value as tHIGH [min], we need to have larger HCNT
value than necessary for tHIGH, to meet tHD;STA constraint.

> Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and
> it looks like the delay between the falling edge clock of the start
> condition and the rising edge of the first data bit of the start byte is
> controlled by this parameter. I conclude that in the drawing below (1)
> is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME.
>
>       _________
> scl           \__________ ...
>       ___           ______
> sda     \_________/       ...
>
>          |-----|---|
>            (1)  (2)

Thanks for the clarification, understood.

   Shinya

>>> In order to take advantage of those we need some way to pass those values
>>> to the i2c designware core. I have two suggestions:
>>>
>>>    - Use the method outlined in this patch and let the interface driver
>>>      override *CNT values.
>>
>> With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA
>> quite accurately.  On the other hand, what we can't control with DW
>> I2C core is tr and tf.  I assumed that it could never be longer than
>> 300nsec (it's defined as a max. in the I2C specification), so I used
>> it for calculation.
>>
>> I agree that tr and tf are PCB specific, and it would a good first
>> step toward timing optimization to make them configurable through
>> platform data.
>>
>> Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
>> calculations don't suit with later DW I2C cores, then it would be
>> nice for someone who can access to the data book to update formulas,
>> or provide alternative formulas and make them selectable depending
>> on DW core versions.
>
> I'm not having the impression there is a huge difference between the
> different generations of DW blocks. Probably we can find one formula
> that suits all blocks. We just have to be careful (in doubt rather
> conservative) with the transition times. This seems to be currently
> the case and if I understand Mika correctly, his objective is to remove
> some of this conservatism.
>
>> It always helps us to have a way to calculate *HCNT and *LCNT values
>> automatically.  As said above, DW I2C core can control tHIGH, tLOW,
>> tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
>> with proper formulas.  It also helps hardware people as well to
>> provide reference HCNT/LCNT values.
>>
>> And as a third step, if we want to use optimized HCNT/LCNT values
>> which can not be obtained from proper formulas + user-requested
>> tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
>> or provided from hardware team, then I'm fine with having a way to
>> _override_ HCNT/LCNT values.  Such direct way might be useful.
>
> I agree. Probably it is best to have both, a default method based on
> formulas and timing parameters (the formulas are quite simple anyway)
> which works with device tree and such and a second method based on
> register values which works with mechanisms like ACPI.

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote:
> On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote:
> >>Basically, DW I2C core provides a good enough (and quite direct) way
> >>to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
> >>
> >>But from my experience (with a slightly old version of DW I2C core
> >>around 2005, version 1.06a or so), DW I2C core was apparently lacking
> >>in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
> >>then found that we could meet tHD;STA by increasing *HCNT values, so
> >>that's what currently we have in i2c-designware.c  I know with that
> >>workaround applied, tHIGH is to be configured with a larger value
> >>than necessary, but we have no choice.  For I2C bus systems, we must
> >>meet every timing constraint strictly as required.  If DW I2C core
> >>cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
> >>it a hardware bug.
> >
> >I agree. However, tHD;STA [min] is defined to the same value as tHIGH
> >[min] for all modes in the I2C specification. Do I understand you
> >correctly that the SCL_HCNT parameters control at the same time tHIGH
> >and tHD;STA?
> 
> *HCNT value does affect both tHIGH and tHD;STA at the same time.
> But we have to make sure that composition of tHIGH is different
> from the one of tHD;STA.
> 
> For tHIGH
> ----------
> 
> DW I2C core is aware of the SCL rising transition (tr) and can
> ignore it.  It starts counting the HIGH period of the SCL signal
> (tHIGH) after the SCL input voltage increases at VIH.
> 
> This is described in the data book as an ideal configuration,
> and the resulting formula would be:
> 
> 	HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a)
> 
> please refer to the data book for details about (1+4+3) part.
> 
> For tLOW
> --------
> 
> DW I2C core starts counting the SCL CNTs for the LOW period of
> the SCL signal (tLOW) as soon as it pulls the SCL line _without_
> _confirming_ the SCL input voltage decreases at VIL.
> 
> In order to meet the tLOW timing spec, we need to take into
> account the fall time of SCL signal (tf), so the resulting
> formula should be:
> 
> 	LCNT + 1 >= IC_CLK * (tLOW + tf)
> 
> please refer to the data book for details about '+1' part.
> 
> For tHD;STA
> -----------
> 
> There is no explanation about tHD;STA in the data book.  We
> only have (my) experimental result; tHD;STA turned out to be
> proportional to 'HCNT + 3'.  The formula is:
> 
> 	HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b)
> 
> DW I2C core seems to start counting the SCL CNTs for the HIGH
> period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_
> line without confirming the SDA input voltage decreases at VIL,
> so we have to take into account the SDA falling transition (tf)
> here.
> 
> As can be expected from (a) and (b), if tHD;STA [min] is defined
> to the same value as tHIGH [min], we need to have larger HCNT
> value than necessary for tHIGH, to meet tHD;STA constraint.
> 
> [...]

Hrmmm... This makes my head spin. Do you think the following code
snippet represents an accurate method to calculate the register values?
If you agree I'll prepare a patch based on that for the DW I2C. The
question will be, however, how to obtain the transition times.

Would it make sense to add generic I2C device tree properties for those
parameters? These parameters are independent of the actual bus driver,
rather a PCB property... And as such the correct place would be device
tree or ACPI or similar.

Greetings,
  Christian

--- SNIP ---
/*
 *        t_f;SDA
 *          |-|
 *     _____              _____ . . .
 *          \            /
 * SDA       \          /
 *            \________/       t_r;SCL    t_f;SCL
 *                               |-|        |-|
 *     ______________               ________
 *                   \             /        \
 * SCL                \           /          \
 *                     \_________/            \_______ . . .
 *            |------| |---------| |--------|
 *            t_HD;STA    t_LOW      t_HIGH
 *
 *          |--------|-----------| |--------|
 *         (   HCNT       LCNT        HCNT   ) * 1/f_SYS
 *
 *
 * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH)
 *      = f_SYS * (t_HIGH + t_f;SDA)      because T_HD;STA == T_HIGH
 * LCNT = f_SYS * (t_f;SCL + t_LOW)
 * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL
 */

#define I2C_STD_MODE       (0)
#define I2C_FAST_MODE      (1)
#define I2C_FAST_MODE_PLUS (2)

static const struct i2c_timing_params {
	unsigned int t_SCL;   /* t_SCL in (ms * 65536) */
	unsigned int t_LOW;   /* t_LOW in (ms * 65536) */
	unsigned int t_HIGH;  /* t_HIGH = t_HD;STA in (ms * 65536) */
} i2c_timing_params[] = {
	{ /* I2C_STD_MODE */
		.t_SCL   = 10.0e-3 * 65536 + 0.5,
		.t_LOW   =  4.7e-3 * 65536 + 0.5,
		.t_HIGH  =  4.0e-3 * 65536 + 0.5,
	},
	{ /* I2C_FAST_MODE */
		.t_SCL   =  2.5e-3 * 65536 + 0.5,
		.t_LOW   =  1.3e-3 * 65536 + 0.5,
		.t_HIGH  =  0.6e-3 * 65536 + 0.5,
	},
	{ /* I2C_FAST_MODE_PLUS */
		.t_SCL   =  1.0e-3 * 65536 + 0.5,
		.t_LOW   =  0.5e-3 * 65536 + 0.5,
		.t_HIGH  =  0.26e-3 * 65536 + 0.5,
	},
};

#define DW_CNT_OFFS    (1)
#define DW_FILT_OFFS   (2)
#define DW_SCL_LATENCY (3)

/*
 * all timings in ns f_SYS in kHz
 * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns
 */
void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS
			unsigned int tf_SCL, unsigned int tr_SCL,
			unsigned int tf_SDA, unsigned int mode,
			unsigned int IC_SPKLEN)
{
	unsigned int H, L;
	int D;
	/*
	 * The worst case High count timing includes the SDA falling
	 * transition of start condition
	 */
	H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH);

	/* The Low count timing always includes the preceding falling edge. */
	L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW);

	/*
	 * The clock timings must not exceed the maximum frequencies
	 * from the I2C specification, slow down if required.
	 */
	D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2;
	if (D > 0) {
		H += D;
		L += D;
	}

	/*
	 * Round the HIGH count and subtract
	 * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN)
	 * which are added to the register value inside the DW IP.
	 */
	H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS
				- DW_SCL_LATENCY - IC_SPKLEN;

	/*
	 * Round the LOW count and subtract DW_CNT_OFFS which is added to
	 * the register value inside the DW IP.
	 */
	L = ((L + 32768) >> 16) - DW_CNT_OFFS;

	/* Respect the DW IP minimum timings */
	if (H < (IC_SPKLEN + 5))
		H = IC_SPKLEN + 5;
	if (L < (IC_SPKLEN + 7))
		L = IC_SPKLEN + 7;
	*HCNT = H;
	*LCNT = L;
}

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

On 7/22/13 10:17 PM, Christian Ruppert wrote:> On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote:
>> On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote:
>>>> Basically, DW I2C core provides a good enough (and quite direct) way
>>>> to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
>>>>
>>>> But from my experience (with a slightly old version of DW I2C core
>>>> around 2005, version 1.06a or so), DW I2C core was apparently lacking
>>>> in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
>>>> then found that we could meet tHD;STA by increasing *HCNT values, so
>>>> that's what currently we have in i2c-designware.c  I know with that
>>>> workaround applied, tHIGH is to be configured with a larger value
>>>> than necessary, but we have no choice.  For I2C bus systems, we must
>>>> meet every timing constraint strictly as required.  If DW I2C core
>>>> cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
>>>> it a hardware bug.
>>>
>>> I agree. However, tHD;STA [min] is defined to the same value as tHIGH
>>> [min] for all modes in the I2C specification. Do I understand you
>>> correctly that the SCL_HCNT parameters control at the same time tHIGH
>>> and tHD;STA?
>>
>> *HCNT value does affect both tHIGH and tHD;STA at the same time.
>> But we have to make sure that composition of tHIGH is different
>> from the one of tHD;STA.
>>
>> For tHIGH
>> ----------
>>
>> DW I2C core is aware of the SCL rising transition (tr) and can
>> ignore it.  It starts counting the HIGH period of the SCL signal
>> (tHIGH) after the SCL input voltage increases at VIH.
>>
>> This is described in the data book as an ideal configuration,
>> and the resulting formula would be:
>>
>> 	HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a)
>>
>> please refer to the data book for details about (1+4+3) part.
>>
>> For tLOW
>> --------
>>
>> DW I2C core starts counting the SCL CNTs for the LOW period of
>> the SCL signal (tLOW) as soon as it pulls the SCL line _without_
>> _confirming_ the SCL input voltage decreases at VIL.
>>
>> In order to meet the tLOW timing spec, we need to take into
>> account the fall time of SCL signal (tf), so the resulting
>> formula should be:
>>
>> 	LCNT + 1 >= IC_CLK * (tLOW + tf)
>>
>> please refer to the data book for details about '+1' part.
>>
>> For tHD;STA
>> -----------
>>
>> There is no explanation about tHD;STA in the data book.  We
>> only have (my) experimental result; tHD;STA turned out to be
>> proportional to 'HCNT + 3'.  The formula is:
>>
>> 	HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b)
>>
>> DW I2C core seems to start counting the SCL CNTs for the HIGH
>> period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_
>> line without confirming the SDA input voltage decreases at VIL,
>> so we have to take into account the SDA falling transition (tf)
>> here.
>>
>> As can be expected from (a) and (b), if tHD;STA [min] is defined
>> to the same value as tHIGH [min], we need to have larger HCNT
>> value than necessary for tHIGH, to meet tHD;STA constraint.
>>
>> [...]
>
> Hrmmm... This makes my head spin. Do you think the following code
> snippet represents an accurate method to calculate the register values?
> If you agree I'll prepare a patch based on that for the DW I2C. The
> question will be, however, how to obtain the transition times.

Please find my comments below.

> /*
>  *        t_f;SDA
>  *          |-|
>  *     _____              _____ . . .
>  *          \            /
>  * SDA       \          /
>  *            \________/       t_r;SCL    t_f;SCL
>  *                               |-|        |-|
>  *     ______________               ________
>  *                   \             /        \
>  * SCL                \           /          \
>  *                     \_________/            \_______ . . .
>  *            |------| |---------| |--------|
>  *            t_HD;STA    t_LOW      t_HIGH
>  *
>  *          |--------|-----------| |--------|
>  *         (   HCNT       LCNT        HCNT   ) * 1/f_SYS

Composition is:  HCNT+3     LCNT+1      HCNT+(1+4+3)

The offsets for the HCNT are different in the cases of tHD;STA and
t_HIGH.  It's based on my experimental result and we can't know how
DW I2C core actually counts those period, but it can be easily
imagined that for DW I2C core, the way to count t_HD;STA is similar
to the way to count t_LOW; it starts counting CNTs as soon as it
pulls the SCL/SDA line, then waits for given CNTs.

I think your equations and snippet code are based on the assumption
that DW I2C core must be counting the number of CNTs for the HIGH
period of the SCL signal (that's t_HD;STA and t_HIGH) in the same
way, but I don't think so.  From my observation and experimental
results, it must be different.  We couldn't know what actually is,
though.

>  *
>  * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH)
>  *      = f_SYS * (t_HIGH + t_f;SDA)      because T_HD;STA == T_HIGH
>  * LCNT = f_SYS * (t_f;SCL + t_LOW)
>  * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL

As said before, all t_SCL things should go away.  Please forget
about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
pointless concept for the I2C bus systems.  For example, as long
as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
certain I2C bus, it doesn't matter that the resulting clock speed
is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
Nobody in the I2C bus doesn't care about actual bus/clock speed.

We don't have to care about the resulting bus speed, or rather
we should/must not check to see if it's within the proper range.

> #define I2C_STD_MODE       (0)
> #define I2C_FAST_MODE      (1)
> #define I2C_FAST_MODE_PLUS (2)
>
> static const struct i2c_timing_params {
> 	unsigned int t_SCL;   /* t_SCL in (ms * 65536) */
> 	unsigned int t_LOW;   /* t_LOW in (ms * 65536) */
> 	unsigned int t_HIGH;  /* t_HIGH = t_HD;STA in (ms * 65536) */
> } i2c_timing_params[] = {
> 	{ /* I2C_STD_MODE */
> 		.t_SCL   = 10.0e-3 * 65536 + 0.5,
> 		.t_LOW   =  4.7e-3 * 65536 + 0.5,
> 		.t_HIGH  =  4.0e-3 * 65536 + 0.5,
> 	},
> 	{ /* I2C_FAST_MODE */
> 		.t_SCL   =  2.5e-3 * 65536 + 0.5,
> 		.t_LOW   =  1.3e-3 * 65536 + 0.5,
> 		.t_HIGH  =  0.6e-3 * 65536 + 0.5,
> 	},
> 	{ /* I2C_FAST_MODE_PLUS */
> 		.t_SCL   =  1.0e-3 * 65536 + 0.5,
> 		.t_LOW   =  0.5e-3 * 65536 + 0.5,
> 		.t_HIGH  =  0.26e-3 * 65536 + 0.5,
> 	},
> };

I just can't take to these 65536s, 0.5s, and {(x + 32768) >> 16},
as it's not intuitive.  Would be nice to come up with another way!

> #define DW_CNT_OFFS    (1)
> #define DW_FILT_OFFS   (2)
> #define DW_SCL_LATENCY (3)
>
> /*
>   * all timings in ns f_SYS in kHz
>   * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns
>   */
> void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS
> 			unsigned int tf_SCL, unsigned int tr_SCL,
> 			unsigned int tf_SDA, unsigned int mode,
> 			unsigned int IC_SPKLEN)
> {
> 	unsigned int H, L;
> 	int D;
> 	/*
> 	 * The worst case High count timing includes the SDA falling
> 	 * transition of start condition
> 	 */
> 	H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH);
>
> 	/* The Low count timing always includes the preceding falling edge. */
> 	L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW);
>
> 	/*
> 	 * The clock timings must not exceed the maximum frequencies
> 	 * from the I2C specification, slow down if required.
> 	 */
> 	D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2;
> 	if (D > 0) {
> 		H += D;
> 		L += D;
> 	}

These H/L adjustments based on clock timings are not necessary.

> 	/*
> 	 * Round the HIGH count and subtract
> 	 * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN)
> 	 * which are added to the register value inside the DW IP.
> 	 */
> 	H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS
> 				- DW_SCL_LATENCY - IC_SPKLEN;

And this adjustment for HCNT has to be altered to suit t_HIGH or
t_HD;STA.  It can not be processed in a single form like this.

> 	/*
> 	 * Round the LOW count and subtract DW_CNT_OFFS which is added to
> 	 * the register value inside the DW IP.
> 	 */
> 	L = ((L + 32768) >> 16) - DW_CNT_OFFS;

This is Ok.

> 	/* Respect the DW IP minimum timings */
> 	if (H < (IC_SPKLEN + 5))
> 		H = IC_SPKLEN + 5;
> 	if (L < (IC_SPKLEN + 7))
> 		L = IC_SPKLEN + 7;
> 	*HCNT = H;
> 	*LCNT = L;
> }

I'm not familiar with this SPKLEN (spike length?) as it didn't
appear in the data book around 2005, but it's fine to try to make
sure H/L values are within the range supported.

> Would it make sense to add generic I2C device tree properties for those
> parameters? These parameters are independent of the actual bus driver,
> rather a PCB property... And as such the correct place would be device
> tree or ACPI or similar.

If there are other bus drivers that make use of tr/tf transition
times, I think it makes sense.

Hope this helps,

   Shinya

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote:
> On 7/22/13 10:17 PM, Christian Ruppert wrote:> On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote:
> >>On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote:
> >>>>Basically, DW I2C core provides a good enough (and quite direct) way
> >>>>to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
> >>>>
> >>>>But from my experience (with a slightly old version of DW I2C core
> >>>>around 2005, version 1.06a or so), DW I2C core was apparently lacking
> >>>>in an appropriate hardware mechanism to meet tHD;STA timing spec.  We
> >>>>then found that we could meet tHD;STA by increasing *HCNT values, so
> >>>>that's what currently we have in i2c-designware.c  I know with that
> >>>>workaround applied, tHIGH is to be configured with a larger value
> >>>>than necessary, but we have no choice.  For I2C bus systems, we must
> >>>>meet every timing constraint strictly as required.  If DW I2C core
> >>>>cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
> >>>>it a hardware bug.
> >>>
> >>>I agree. However, tHD;STA [min] is defined to the same value as tHIGH
> >>>[min] for all modes in the I2C specification. Do I understand you
> >>>correctly that the SCL_HCNT parameters control at the same time tHIGH
> >>>and tHD;STA?
> >>
> >>*HCNT value does affect both tHIGH and tHD;STA at the same time.
> >>But we have to make sure that composition of tHIGH is different
> >>from the one of tHD;STA.
> >>
> >>For tHIGH
> >>----------
> >>
> >>DW I2C core is aware of the SCL rising transition (tr) and can
> >>ignore it.  It starts counting the HIGH period of the SCL signal
> >>(tHIGH) after the SCL input voltage increases at VIH.
> >>
> >>This is described in the data book as an ideal configuration,
> >>and the resulting formula would be:
> >>
> >>	HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a)
> >>
> >>please refer to the data book for details about (1+4+3) part.
> >>
> >>For tLOW
> >>--------
> >>
> >>DW I2C core starts counting the SCL CNTs for the LOW period of
> >>the SCL signal (tLOW) as soon as it pulls the SCL line _without_
> >>_confirming_ the SCL input voltage decreases at VIL.
> >>
> >>In order to meet the tLOW timing spec, we need to take into
> >>account the fall time of SCL signal (tf), so the resulting
> >>formula should be:
> >>
> >>	LCNT + 1 >= IC_CLK * (tLOW + tf)
> >>
> >>please refer to the data book for details about '+1' part.
> >>
> >>For tHD;STA
> >>-----------
> >>
> >>There is no explanation about tHD;STA in the data book.  We
> >>only have (my) experimental result; tHD;STA turned out to be
> >>proportional to 'HCNT + 3'.  The formula is:
> >>
> >>	HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b)
> >>
> >>DW I2C core seems to start counting the SCL CNTs for the HIGH
> >>period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_
> >>line without confirming the SDA input voltage decreases at VIL,
> >>so we have to take into account the SDA falling transition (tf)
> >>here.
> >>
> >>As can be expected from (a) and (b), if tHD;STA [min] is defined
> >>to the same value as tHIGH [min], we need to have larger HCNT
> >>value than necessary for tHIGH, to meet tHD;STA constraint.
> >>
> >>[...]
> >
> >Hrmmm... This makes my head spin. Do you think the following code
> >snippet represents an accurate method to calculate the register values?
> >If you agree I'll prepare a patch based on that for the DW I2C. The
> >question will be, however, how to obtain the transition times.
> 
> Please find my comments below.
> 
> >/*
> > *        t_f;SDA
> > *          |-|
> > *     _____              _____ . . .
> > *          \            /
> > * SDA       \          /
> > *            \________/       t_r;SCL    t_f;SCL
> > *                               |-|        |-|
> > *     ______________               ________
> > *                   \             /        \
> > * SCL                \           /          \
> > *                     \_________/            \_______ . . .
> > *            |------| |---------| |--------|
> > *            t_HD;STA    t_LOW      t_HIGH
> > *
> > *          |--------|-----------| |--------|
> > *         (   HCNT       LCNT        HCNT   ) * 1/f_SYS
> 
> Composition is:  HCNT+3     LCNT+1      HCNT+(1+4+3)

OK

> The offsets for the HCNT are different in the cases of tHD;STA and
> t_HIGH.  It's based on my experimental result and we can't know how
> DW I2C core actually counts those period, but it can be easily
> imagined that for DW I2C core, the way to count t_HD;STA is similar
> to the way to count t_LOW; it starts counting CNTs as soon as it
> pulls the SCL/SDA line, then waits for given CNTs.
> 
> I think your equations and snippet code are based on the assumption
> that DW I2C core must be counting the number of CNTs for the HIGH
> period of the SCL signal (that's t_HD;STA and t_HIGH) in the same
> way, but I don't think so.  From my observation and experimental
> results, it must be different.  We couldn't know what actually is,
> though.
> 
> > *
> > * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH)
> > *      = f_SYS * (t_HIGH + t_f;SDA)      because T_HD;STA == T_HIGH
> > * LCNT = f_SYS * (t_f;SCL + t_LOW)
> > * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL

So the corrected HCNT would be:
HCNT = max(f_SYS * (t_HD;STA + t_f;SDA) - 3, f_SYS * t_HIGH - 7)
     = f_SYS * (t_HD;STA + t_f;SDA) - 3

> As said before, all t_SCL things should go away.  Please forget
> about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
> pointless concept for the I2C bus systems.  For example, as long
> as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
> certain I2C bus, it doesn't matter that the resulting clock speed
> is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
> Nobody in the I2C bus doesn't care about actual bus/clock speed.
> 
> We don't have to care about the resulting bus speed, or rather
> we should/must not check to see if it's within the proper range.

Actually, the I2C specification clearly defines f_SCL;max (and thus
implies t_SCL;min), both in the tables and the timing diagrams. Why can
we ignore this constraint while having to meet all the others?

> >#define I2C_STD_MODE       (0)
> >#define I2C_FAST_MODE      (1)
> >#define I2C_FAST_MODE_PLUS (2)
> >
> >static const struct i2c_timing_params {
> >	unsigned int t_SCL;   /* t_SCL in (ms * 65536) */
> >	unsigned int t_LOW;   /* t_LOW in (ms * 65536) */
> >	unsigned int t_HIGH;  /* t_HIGH = t_HD;STA in (ms * 65536) */
> >} i2c_timing_params[] = {
> >	{ /* I2C_STD_MODE */
> >		.t_SCL   = 10.0e-3 * 65536 + 0.5,
> >		.t_LOW   =  4.7e-3 * 65536 + 0.5,
> >		.t_HIGH  =  4.0e-3 * 65536 + 0.5,
> >	},
> >	{ /* I2C_FAST_MODE */
> >		.t_SCL   =  2.5e-3 * 65536 + 0.5,
> >		.t_LOW   =  1.3e-3 * 65536 + 0.5,
> >		.t_HIGH  =  0.6e-3 * 65536 + 0.5,
> >	},
> >	{ /* I2C_FAST_MODE_PLUS */
> >		.t_SCL   =  1.0e-3 * 65536 + 0.5,
> >		.t_LOW   =  0.5e-3 * 65536 + 0.5,
> >		.t_HIGH  =  0.26e-3 * 65536 + 0.5,
> >	},
> >};
> 
> I just can't take to these 65536s, 0.5s, and {(x + 32768) >> 16},
> as it's not intuitive.  Would be nice to come up with another way!

OK, I'll make this more readable in the next version.

> >#define DW_CNT_OFFS    (1)
> >#define DW_FILT_OFFS   (2)
> >#define DW_SCL_LATENCY (3)
> >
> >/*
> >  * all timings in ns f_SYS in kHz
> >  * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns
> >  */
> >void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS
> >			unsigned int tf_SCL, unsigned int tr_SCL,
> >			unsigned int tf_SDA, unsigned int mode,
> >			unsigned int IC_SPKLEN)
> >{
> >	unsigned int H, L;
> >	int D;
> >	/*
> >	 * The worst case High count timing includes the SDA falling
> >	 * transition of start condition
> >	 */
> >	H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH);
> >
> >	/* The Low count timing always includes the preceding falling edge. */
> >	L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW);
> >
> >	/*
> >	 * The clock timings must not exceed the maximum frequencies
> >	 * from the I2C specification, slow down if required.
> >	 */
> >	D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2;
> >	if (D > 0) {
> >		H += D;
> >		L += D;
> >	}
> 
> These H/L adjustments based on clock timings are not necessary.
> 
> >	/*
> >	 * Round the HIGH count and subtract
> >	 * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN)
> >	 * which are added to the register value inside the DW IP.
> >	 */
> >	H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS
> >				- DW_SCL_LATENCY - IC_SPKLEN;
> 
> And this adjustment for HCNT has to be altered to suit t_HIGH or
> t_HD;STA.  It can not be processed in a single form like this.

I agree.

> >	/*
> >	 * Round the LOW count and subtract DW_CNT_OFFS which is added to
> >	 * the register value inside the DW IP.
> >	 */
> >	L = ((L + 32768) >> 16) - DW_CNT_OFFS;
> 
> This is Ok.
> 
> >	/* Respect the DW IP minimum timings */
> >	if (H < (IC_SPKLEN + 5))
> >		H = IC_SPKLEN + 5;
> >	if (L < (IC_SPKLEN + 7))
> >		L = IC_SPKLEN + 7;
> >	*HCNT = H;
> >	*LCNT = L;
> >}
> 
> I'm not familiar with this SPKLEN (spike length?) as it didn't
> appear in the data book around 2005, but it's fine to try to make
> sure H/L values are within the range supported.

Yes, SPKLEN is a register defining the number of cycles to filter out in
spike filtering. If this has appeared sometime between 2005 and today we
must also check the version for this. This is really starting to become
complicated.

> >Would it make sense to add generic I2C device tree properties for those
> >parameters? These parameters are independent of the actual bus driver,
> >rather a PCB property... And as such the correct place would be device
> >tree or ACPI or similar.
> 
> If there are other bus drivers that make use of tr/tf transition
> times, I think it makes sense.

Wolfram, what's your opinion on this?

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 1/2] i2c-designware: make *CNT values configurable

[Wolfram Sang]

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


> > >Would it make sense to add generic I2C device tree properties for those
> > >parameters? These parameters are independent of the actual bus driver,
> > >rather a PCB property... And as such the correct place would be device
> > >tree or ACPI or similar.
> > 
> > If there are other bus drivers that make use of tr/tf transition
> > times, I think it makes sense.
> 
> Wolfram, what's your opinion on this?

If it is a PCB property, it makes sense to have generic bindings for
it. Can they have very-safe defaults and thus be optional?


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

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

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

On Mon, Aug 05, 2013 at 12:02:26PM +0200, Wolfram Sang wrote:
> 
> > > >Would it make sense to add generic I2C device tree properties for those
> > > >parameters? These parameters are independent of the actual bus driver,
> > > >rather a PCB property... And as such the correct place would be device
> > > >tree or ACPI or similar.
> > > 
> > > If there are other bus drivers that make use of tr/tf transition
> > > times, I think it makes sense.
> > 
> > Wolfram, what's your opinion on this?
> 
> If it is a PCB property, it makes sense to have generic bindings for
> it. Can they have very-safe defaults and thus be optional?

We can definitely have safe defaults that work for a given
driver/hardware. I don't think the same defaults would be safe for all
drivers/hardware: The timing strategies of different I2C hardware seems
to vary widely (which edges of the clock are sampled, how does different
hardware deal with hold times etc) and depending on which parameters are
available to the driver to control these timings, the safe values would
either have to be the minimum or the maximum in the range allowed by the
I2C specification.
Every driver would thus have to implement its own defaults in case the
properties are not defined.

  Christian

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

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Wolfram Sang]

> Every driver would thus have to implement its own defaults in case the
> properties are not defined.

Placing the defaults at driver level sounds fine to me.

Thanks!

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

Hi,

On 8/5/13 6:31 PM, Christian Ruppert wrote:> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote:
>> As said before, all t_SCL things should go away.  Please forget
>> about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
>> pointless concept for the I2C bus systems.  For example, as long
>> as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
>> certain I2C bus, it doesn't matter that the resulting clock speed
>> is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
>> Nobody in the I2C bus doesn't care about actual bus/clock speed.
>>
>> We don't have to care about the resulting bus speed, or rather
>> we should/must not check to see if it's within the proper range.
>
> Actually, the I2C specification clearly defines f_SCL;max (and thus
> implies t_SCL;min), both in the tables and the timing diagrams. Why can
> we ignore this constraint while having to meet all the others?

If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
f_SCL;max will be met by itself.  And again, all I2C master and
slave devices in the bus don't care about f_SCL; what they do care
are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
f_SCL is pointless and has no value for HCNT/LCNT calculations.

Is that clear?  What is the point to make sure whether f_SCL
constraint is met or not?  Is there any combination where t_f,
t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range?
I don't think there is.

I'd make a compromise proposal; it's fine to make sure whether the
resulting f_SCL is within a supported range, but should not make a
correction of HCNT/LCNT values.  Just report warning messages that
some parameters/calculations might be mis-configured an/or wrong.

   Shinya

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote:
> >Actually, the I2C specification clearly defines f_SCL;max (and thus
> >implies t_SCL;min), both in the tables and the timing diagrams. Why can
> >we ignore this constraint while having to meet all the others?
> 
> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
> f_SCL;max will be met by itself.  And again, all I2C master and
> slave devices in the bus don't care about f_SCL; what they do care
> are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
> f_SCL is pointless and has no value for HCNT/LCNT calculations.

One thing that comes to mind regarding the bus speed is that even if we
have all the minimal timing requirements met we still prefer resulting bus
speeds closer to 400kHz than 315.41kHz for the reasons that we get more
data transferred that way, no?

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

Hi,

On 8/19/13 8:36 PM, Mika Westerberg wrote:
> On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote:
>>> Actually, the I2C specification clearly defines f_SCL;max (and thus
>>> implies t_SCL;min), both in the tables and the timing diagrams. Why can
>>> we ignore this constraint while having to meet all the others?
>>
>> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
>> f_SCL;max will be met by itself.  And again, all I2C master and
>> slave devices in the bus don't care about f_SCL; what they do care
>> are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
>> f_SCL is pointless and has no value for HCNT/LCNT calculations.
>
> One thing that comes to mind regarding the bus speed is that even if we
> have all the minimal timing requirements met we still prefer resulting bus
> speeds closer to 400kHz than 315.41kHz for the reasons that we get more
> data transferred that way, no?

That depends I2C slave devices in the bus in your target systems.
As long as your slave devices can detect START/STOP conditions and
recognize SDA/SCL transitions properly, that should be Ok (you can
use HCNT/LCNT settings for 400 kHz without having all the minimal
timing requirements met).

My comments above was a reply to Christian's snippet code and how to
treat f_SCL;mas constraints, and unrelated to your case in question.
I'm for having a way to override HCNT/LCNT values as said before, and
that should nicely work for you.

   Shinya

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote:
> Hi,
> 
> On 8/5/13 6:31 PM, Christian Ruppert wrote:> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote:
> >>As said before, all t_SCL things should go away.  Please forget
> >>about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
> >>pointless concept for the I2C bus systems.  For example, as long
> >>as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
> >>certain I2C bus, it doesn't matter that the resulting clock speed
> >>is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
> >>Nobody in the I2C bus doesn't care about actual bus/clock speed.
> >>
> >>We don't have to care about the resulting bus speed, or rather
> >>we should/must not check to see if it's within the proper range.
> >
> >Actually, the I2C specification clearly defines f_SCL;max (and thus
> >implies t_SCL;min), both in the tables and the timing diagrams. Why can
> >we ignore this constraint while having to meet all the others?
> 
> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
> f_SCL;max will be met by itself.

I'm not sure if I agree with this:

Standard mode:
       t_r;min          0ns
       t_f;min     +    0ns
       t_HIGH;min  + 4000ns
       t_LOW;min   + 4700ns
       1/f_SCL     = 8700ns
   ==> f_SCL       = 115kHz    ==>    violation of f_SCL;max=100kHz

Fast mode (let's assume V_DD = 5.5V):
       t_r;min         20ns
       t_f;min     +   20ns
       t_HIGH;min  +  600ns
       t_LIW;min   + 1300ns
       1/f_SCL     = 1940ns
   ==> f_SCL       = 515kHz    ==>    violation of f_SCL;max=400kHz

In my understanding, f_SCL;max condition is only met
a) either if t_HIGH = t_HIGH;min and t_LOW = t_LOW;min
          then t_r must be t_r;max and t_f must be t_f;max
b) or if t_r < t_r;max and t_f < t_f;max 
      then t_HIGH must be > t_HIGH;min and T_LOW must be T_LOW;min

Given that we cannot easily influence t_r and t_f we must adjust t_HIGH
and t_LOW. What am I missing here?

> And again, all I2C master and
> slave devices in the bus don't care about f_SCL; what they do care
> are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
> f_SCL is pointless and has no value for HCNT/LCNT calculations.

I partially agree: If I2C devices don't care about f_SCL but only about
t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max
constraint. If this is the case, I'm wondering why it is part of the
specification, though.

> Is that clear?  What is the point to make sure whether f_SCL
> constraint is met or not?  Is there any combination where t_f,
> t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range?
> I don't think there is.

See above.

> I'd make a compromise proposal; it's fine to make sure whether the
> resulting f_SCL is within a supported range, but should not make a
> correction of HCNT/LCNT values.  Just report warning messages that
> some parameters/calculations might be mis-configured an/or wrong.

Not sure if this is a good idea: If f_SCL is met by design I'm perfectly
happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat
the kernel with confusing warnings. If f_SCL is not automatically met we
must either make sure t_HIGH/t_LOW are adjusted or we must take the
decision to ignore that constraint and document the reasons behind that
decision accordingly.

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 1/2] i2c-designware: make *CNT values configurable

[Shinya Kuribayashi]

On 8/21/13 11:39 PM, Christian Ruppert wrote:
> On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote:
>> On 8/5/13 6:31 PM, Christian Ruppert wrote:
>>> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote:
>>>> As said before, all t_SCL things should go away.  Please forget
>>>> about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
>>>> pointless concept for the I2C bus systems.  For example, as long
>>>> as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
>>>> certain I2C bus, it doesn't matter that the resulting clock speed
>>>> is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
>>>> Nobody in the I2C bus doesn't care about actual bus/clock speed.
>>>>
>>>> We don't have to care about the resulting bus speed, or rather
>>>> we should/must not check to see if it's within the proper range.
>>>
>>> Actually, the I2C specification clearly defines f_SCL;max (and thus
>>> implies t_SCL;min), both in the tables and the timing diagrams. Why can
>>> we ignore this constraint while having to meet all the others?
>>
>> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
>> f_SCL;max will be met by itself.
>
> I'm not sure if I agree with this:
>
> Standard mode:
>         t_r;min          0ns
>         t_f;min     +    0ns
>         t_HIGH;min  + 4000ns
>         t_LOW;min   + 4700ns
>         1/f_SCL     = 8700ns
>     ==> f_SCL       = 115kHz    ==>    violation of f_SCL;max=100kHz
>
> Fast mode (let's assume V_DD = 5.5V):
>         t_r;min         20ns
>         t_f;min     +   20ns
>         t_HIGH;min  +  600ns
>         t_LIW;min   + 1300ns
>         1/f_SCL     = 1940ns
>     ==> f_SCL       = 515kHz    ==>    violation of f_SCL;max=400kHz

It's more realistic to calculate with say 50ns < tr,tf < 300ns,
than with tt = tf = 0ns or <20ns.  Even if with such real tf/tr
times, there is cases where f_SCL can be greater than 100/400Hz.

I understand what you mean, but that was not my point.  See below.

>> And again, all I2C master and
>> slave devices in the bus don't care about f_SCL; what they do care
>> are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
>> f_SCL is pointless and has no value for HCNT/LCNT calculations.
>
> I partially agree: If I2C devices don't care about f_SCL but only about
> t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max
> constraint. If this is the case, I'm wondering why it is part of the
> specification, though.

With t_r;max and t_f;max,

Standard mode:
         t_r;max       1000ns (max time applied)
         t_f;max     +  300ns (max time applied)
         t_HIGH;min  + 4000ns
         t_LOW;min   + 4700ns
         1/f_SCL     =10000ns
     ==> f_SCL       = 100kHz    ==> f_SCL;max for Standard-mode

Fast mode:
         t_r;max        300ns (max time applied)
         t_f;max     +  300ns (max time applied)
         t_HIGH;min  +  600ns
         t_LIW;min   + 1300ns
         1/f_SCL     = 2500ns
     ==> f_SCL       = 400kHz    ==> f_SCL;max for Fast-mode

f_SCL;max is defined as a resulting clock frequency with the
combination of:

(1) the max. conditions of t_r and t_f
(2) the min. conditions of t_HIGH and t_LOW

We can try to meet t_HIGH;min and t_LOW;min, but we can't meet
t_r;min nor t_f;min in the actual systems.  The t_r and t_f are
_minimum requisites_ for the I/O buffer characteristic of the
master and the board designs, hence necessarily contain some
time margin.

f_SCL is anything more than the resulting speed of (1) + (2),
so I don't think we need to adjust HCNT/LCNT values at all.
If with t_r < t_r;max and t_f < t_f;max, and you've got a faster
clock speed than f_SCL;max, then it's a bonus and we can accept
it gratefully.

>> I'd make a compromise proposal; it's fine to make sure whether the
>> resulting f_SCL is within a supported range, but should not make a
>> correction of HCNT/LCNT values.  Just report warning messages that
>> some parameters/calculations might be mis-configured an/or wrong.
>
> Not sure if this is a good idea: If f_SCL is met by design I'm perfectly
> happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat
> the kernel with confusing warnings. If f_SCL is not automatically met we
> must either make sure t_HIGH/t_LOW are adjusted or we must take the
> decision to ignore that constraint and document the reasons behind that
> decision accordingly.

I tried to write my thought down, not sure well-explained, though.

Notes:

* As long as tHD;SDA issue remains in the DW I2C core, we need to
   have t_HIGH with a relatively lager value than necessary.  In
   such a case, the resulting f_SCL can never exceed f_SCL;max.

* I also wonder which values do you think should be adjusted to meet
   f_SCL;max, HCNT or LCNT, and why is that?  I think it's hard to
   explain, isn't it?

   Shinya

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Christian Ruppert]

On Sat, Aug 24, 2013 at 01:58:47PM +0900, Shinya Kuribayashi wrote:
> On 8/21/13 11:39 PM, Christian Ruppert wrote:
> >On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote:
> >>On 8/5/13 6:31 PM, Christian Ruppert wrote:
> >>>On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote:
> >>>>As said before, all t_SCL things should go away.  Please forget
> >>>>about 100kbps, 400kbps, and so on.  Bus/clock speed is totally
> >>>>pointless concept for the I2C bus systems.  For example, as long
> >>>>as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a
> >>>>certain I2C bus, it doesn't matter that the resulting clock speed
> >>>>is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode.
> >>>>Nobody in the I2C bus doesn't care about actual bus/clock speed.
> >>>>
> >>>>We don't have to care about the resulting bus speed, or rather
> >>>>we should/must not check to see if it's within the proper range.
> >>>
> >>>Actually, the I2C specification clearly defines f_SCL;max (and thus
> >>>implies t_SCL;min), both in the tables and the timing diagrams. Why can
> >>>we ignore this constraint while having to meet all the others?
> >>
> >>If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case),
> >>f_SCL;max will be met by itself.
> >
> >I'm not sure if I agree with this:
> >
> >Standard mode:
> >        t_r;min          0ns
> >        t_f;min     +    0ns
> >        t_HIGH;min  + 4000ns
> >        t_LOW;min   + 4700ns
> >        1/f_SCL     = 8700ns
> >    ==> f_SCL       = 115kHz    ==>    violation of f_SCL;max=100kHz
> >
> >Fast mode (let's assume V_DD = 5.5V):
> >        t_r;min         20ns
> >        t_f;min     +   20ns
> >        t_HIGH;min  +  600ns
> >        t_LIW;min   + 1300ns
> >        1/f_SCL     = 1940ns
> >    ==> f_SCL       = 515kHz    ==>    violation of f_SCL;max=400kHz
> 
> It's more realistic to calculate with say 50ns < tr,tf < 300ns,
> than with tt = tf = 0ns or <20ns.  Even if with such real tf/tr
> times, there is cases where f_SCL can be greater than 100/400Hz.

I agree. I just used these values to illustrate my point.

> I understand what you mean, but that was not my point.  See below.
> 
> >>And again, all I2C master and
> >>slave devices in the bus don't care about f_SCL; what they do care
> >>are t_f, t_r, t_HIGH, t_LOW, and so on.  That's why I'm saying
> >>f_SCL is pointless and has no value for HCNT/LCNT calculations.
> >
> >I partially agree: If I2C devices don't care about f_SCL but only about
> >t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max
> >constraint. If this is the case, I'm wondering why it is part of the
> >specification, though.
> 
> With t_r;max and t_f;max,
> 
> Standard mode:
>         t_r;max       1000ns (max time applied)
>         t_f;max     +  300ns (max time applied)
>         t_HIGH;min  + 4000ns
>         t_LOW;min   + 4700ns
>         1/f_SCL     =10000ns
>     ==> f_SCL       = 100kHz    ==> f_SCL;max for Standard-mode
> 
> Fast mode:
>         t_r;max        300ns (max time applied)
>         t_f;max     +  300ns (max time applied)
>         t_HIGH;min  +  600ns
>         t_LIW;min   + 1300ns
>         1/f_SCL     = 2500ns
>     ==> f_SCL       = 400kHz    ==> f_SCL;max for Fast-mode
> 
> f_SCL;max is defined as a resulting clock frequency with the
> combination of:
> 
> (1) the max. conditions of t_r and t_f
> (2) the min. conditions of t_HIGH and t_LOW
> 
> We can try to meet t_HIGH;min and t_LOW;min, but we can't meet
> t_r;min nor t_f;min in the actual systems.  The t_r and t_f are
> _minimum requisites_ for the I/O buffer characteristic of the
> master and the board designs, hence necessarily contain some
> time margin.

I agree.

> f_SCL is anything more than the resulting speed of (1) + (2),
> so I don't think we need to adjust HCNT/LCNT values at all.
> If with t_r < t_r;max and t_f < t_f;max, and you've got a faster
> clock speed than f_SCL;max, then it's a bonus and we can accept
> it gratefully.

Here I'm not sure: The I2C specification clearly states f_SCL min/max
requirements in table 10. I'm feeling a bit uneasy patching a driver
with (to date) rather pessimistic bus timings to something which might
be over-optimistic. If I submit a patch for this I would like to be sure
not to break existing systems. How do other drivers manage this?

> >>I'd make a compromise proposal; it's fine to make sure whether the
> >>resulting f_SCL is within a supported range, but should not make a
> >>correction of HCNT/LCNT values.  Just report warning messages that
> >>some parameters/calculations might be mis-configured an/or wrong.
> >
> >Not sure if this is a good idea: If f_SCL is met by design I'm perfectly
> >happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat
> >the kernel with confusing warnings. If f_SCL is not automatically met we
> >must either make sure t_HIGH/t_LOW are adjusted or we must take the
> >decision to ignore that constraint and document the reasons behind that
> >decision accordingly.
> 
> I tried to write my thought down, not sure well-explained, though.
> 
> Notes:
> 
> * As long as tHD;SDA issue remains in the DW I2C core, we need to
>   have t_HIGH with a relatively lager value than necessary.  In
>   such a case, the resulting f_SCL can never exceed f_SCL;max.

Not in my calculations (Let's assume fast mode with all rise and fall
times being 50ns as you say above and f_sys = 50MHz):
HCNT = f_SYS * (t_HD;STA + t_f;SDA) - 3
-> t_HIGH = t_HD_STA + t_f;SDA + 4 / f_sys
          = 0.6us + 0.05 us + 0.08 us = 0.73us
LCNT = f_SYS * (t_f;SCL + t_LOW)
-> t_LOW  = 0.05us + 1.3us = 1.35us

-> f_SCL = 1 / (t_HIGH + t_LOW + t_r;SCL + t_f;SCL)
         = 1 / (0.73us + 1.35 us + 0.05 us + 0.05us) = 458MHz  > 400MHz

> * I also wonder which values do you think should be adjusted to meet
>   f_SCL;max, HCNT or LCNT, and why is that?  I think it's hard to
>   explain, isn't it?

Originally I thought it would be best to adjust both in the same way.
Thinking about your question I guess it might be more in the spirit of
I2C to only adjust LCNT. The specification (table 10) allows for both,
there are no max values specified for either.

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

[PATCH 1/2] i2c designware make SCL and SDA falling time configurable

[Romain Baeriswyl]

This patch allows to set independantly SCL and SDA falling times. 
The tLOW period is computed by taking into account the SCL falling time.
The tHIGH period is computed by taking into account the SDA falling time.

For instance in case the margin on tLOW is considered too small, it can 
be increased by increasing the SCL falling time which is by default set 
at 300ns.

The same applies for tHIGH period with the help of SDA falling time.

Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |    8 +++++
 drivers/i2c/busses/i2c-designware-core.c           |   34 +++++++++++++------
 drivers/i2c/busses/i2c-designware-core.h           |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    7 ++++
 4 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index 7fd7fa2..5199b0c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -14,6 +14,12 @@ 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.
 
+ - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds.
+   This value which is by default 300ns is used to compute the tLOW period.
+
+ - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds.
+   This value which is by default 300ns is used to compute the tHIGH period.
+
 Example :
 
 	i2c@f0000 {
@@ -34,4 +40,6 @@ Example :
 		interrupts = <12 1>;
 		clock-frequency = <400000>;
 		i2c-sda-hold-time-ns = <300>;
+		i2c-sda-falling-time-ns = <300>;
+		i2c-scl-falling-time-ns = <300>;
 	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 3525fc7..50c35f9 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -217,7 +217,7 @@ i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 		 *
 		 * If your hardware is free from tHD;STA issue, try this one.
 		 */
-		return (ic_clk * tSYMBOL + 5000) / 10000 - 8 + offset;
+		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
 	else
 		/*
 		 * Conditional expression:
@@ -233,7 +233,8 @@ i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 		 * The reason why we need to take into account "tf" here,
 		 * is the same as described in i2c_dw_scl_lcnt().
 		 */
-		return (ic_clk * (tSYMBOL + tf) + 5000) / 10000 - 3 + offset;
+		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
+			- 3 + offset;
 }
 
 static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
@@ -249,7 +250,7 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 	 * account the fall time of SCL signal (tf).  Default tf value
 	 * should be 0.3 us, for safety.
 	 */
-	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
+	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
 }
 
 static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
@@ -286,6 +287,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	u32 input_clock_khz;
 	u32 hcnt, lcnt;
 	u32 reg;
+	u32 sda_falling_time, scl_falling_time;
 
 	input_clock_khz = dev->get_clk_rate_khz(dev);
 
@@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	/* set standard and fast speed deviders for high/low periods */
 
+	if (dev->sda_falling_time)
+		sda_falling_time = dev->sda_falling_time;
+	else
+		sda_falling_time = 300; /* ns */
+
+	if (dev->scl_falling_time)
+		scl_falling_time = dev->scl_falling_time;
+	else
+		scl_falling_time = 300; /* ns */
+
 	/* Standard-mode */
 	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				40,	/* tHD;STA = tHIGH = 4.0 us */
-				3,	/* tf = 0.3 us */
+				4000,	/* tHD;STA = tHIGH = 4.0 us */
+				sda_falling_time,
 				0,	/* 0: DW default, 1: Ideal */
 				0);	/* No offset */
 	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				47,	/* tLOW = 4.7 us */
-				3,	/* tf = 0.3 us */
+				4700,	/* tLOW = 4.7 us */
+				scl_falling_time,
 				0);	/* No offset */
 
 	/* Allow platforms to specify the ideal HCNT and LCNT values */
@@ -323,13 +335,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	/* Fast-mode */
 	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				6,	/* tHD;STA = tHIGH = 0.6 us */
-				3,	/* tf = 0.3 us */
+				600,	/* tHD;STA = tHIGH = 0.6 us */
+				sda_falling_time,
 				0,	/* 0: DW default, 1: Ideal */
 				0);	/* No offset */
 	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				13,	/* tLOW = 1.3 us */
-				3,	/* tf = 0.3 us */
+				1300,	/* tLOW = 1.3 us */
+				scl_falling_time,
 				0);	/* No offset */
 
 	if (dev->fs_hcnt && dev->fs_lcnt) {
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 912aa22..8b9397e 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -91,6 +91,8 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
 	u32			sda_hold_time;
+	u32			sda_falling_time;
+	u32			scl_falling_time;
 	u16			ss_hcnt;
 	u16			ss_lcnt;
 	u16			fs_hcnt;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 8b9d3f1..0d6fe47 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -129,6 +129,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
 					"i2c-sda-hold-time-ns", &ht);
 		dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000,
 					     1000000);
+
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-sda-falling-time-ns",
+				     &dev->sda_falling_time);
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-scl-falling-time-ns",
+				     &dev->scl_falling_time);
 	}
 
 	dev->functionality =
-- 
1.7.1

Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable

[Mika Westerberg]

On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote:
>  static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> @@ -286,6 +287,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	u32 input_clock_khz;
>  	u32 hcnt, lcnt;
>  	u32 reg;
> +	u32 sda_falling_time, scl_falling_time;
>  
>  	input_clock_khz = dev->get_clk_rate_khz(dev);
>  
> @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  
>  	/* set standard and fast speed deviders for high/low periods */
>  
> +	if (dev->sda_falling_time)
> +		sda_falling_time = dev->sda_falling_time;
> +	else
> +		sda_falling_time = 300; /* ns */

I think this looks better:

	sda_falling_time = dev->sda_falling_time ? dev->sda_falling_time : 300;

> +
> +	if (dev->scl_falling_time)
> +		scl_falling_time = dev->scl_falling_time;
> +	else
> +		scl_falling_time = 300; /* ns */

Ditto.

> +
>  	/* Standard-mode */
>  	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
> -				40,	/* tHD;STA = tHIGH = 4.0 us */
> -				3,	/* tf = 0.3 us */
> +				4000,	/* tHD;STA = tHIGH = 4.0 us */
> +				sda_falling_time,
>  				0,	/* 0: DW default, 1: Ideal */
>  				0);	/* No offset */
>  	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> -				47,	/* tLOW = 4.7 us */
> -				3,	/* tf = 0.3 us */
> +				4700,	/* tLOW = 4.7 us */
> +				scl_falling_time,
>  				0);	/* No offset */
>  
>  	/* Allow platforms to specify the ideal HCNT and LCNT values */

Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable

[Ryan Mallon]

On 09/10/13 18:55, Mika Westerberg wrote:
> On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote:
>>  static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
>> @@ -286,6 +287,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>>  	u32 input_clock_khz;
>>  	u32 hcnt, lcnt;
>>  	u32 reg;
>> +	u32 sda_falling_time, scl_falling_time;
>>  
>>  	input_clock_khz = dev->get_clk_rate_khz(dev);
>>  
>> @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>>  
>>  	/* set standard and fast speed deviders for high/low periods */
>>  
>> +	if (dev->sda_falling_time)
>> +		sda_falling_time = dev->sda_falling_time;
>> +	else
>> +		sda_falling_time = 300; /* ns */
> 
> I think this looks better:
> 
> 	sda_falling_time = dev->sda_falling_time ? dev->sda_falling_time : 300;

You can also use the gcc-ism, which is a bit more concise:

	sda_falling_time = dev->sda_falling_time ?: 300;

~Ryan

Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable

[Shinya Kuribayashi]

On 10/10/13 9:54 AM, Ryan Mallon wrote:
> On 09/10/13 18:55, Mika Westerberg wrote:
>> On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote:
>>> @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>>>
>>>   	/* set standard and fast speed deviders for high/low periods */
>>>
>>> +	if (dev->sda_falling_time)
>>> +		sda_falling_time = dev->sda_falling_time;
>>> +	else
>>> +		sda_falling_time = 300; /* ns */
>>
>> I think this looks better:
>>
>> 	sda_falling_time = dev->sda_falling_time ? dev->sda_falling_time : 300;
>
> You can also use the gcc-ism, which is a bit more concise:
>
> 	sda_falling_time = dev->sda_falling_time ?: 300;

+1

Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable

[Wolfram Sang]

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


> >You can also use the gcc-ism, which is a bit more concise:
> >
> >	sda_falling_time = dev->sda_falling_time ?: 300;
> 
> +1

ditto


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

[PATCH v2 1/2] i2c designware make SCL and SDA falling time configurable

[Romain Baeriswyl]

This patch allows to set independantly SCL and SDA falling times.
The tLOW period is computed by taking into account the SCL falling time.
The tHIGH period is computed by taking into account the SDA falling time.

For instance in case the margin on tLOW is considered too small, it can
be increased by increasing the SCL falling time which is by default set
at 300ns.

The same applies for tHIGH period with the help of SDA falling time.

Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |    8 ++++++
 drivers/i2c/busses/i2c-designware-core.c           |   27 +++++++++++--------
 drivers/i2c/busses/i2c-designware-core.h           |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    7 +++++
 4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index 7fd7fa2..5199b0c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -14,6 +14,12 @@ 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.
 
+ - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds.
+   This value which is by default 300ns is used to compute the tLOW period.
+
+ - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds.
+   This value which is by default 300ns is used to compute the tHIGH period.
+
 Example :
 
 	i2c@f0000 {
@@ -34,4 +40,6 @@ Example :
 		interrupts = <12 1>;
 		clock-frequency = <400000>;
 		i2c-sda-hold-time-ns = <300>;
+		i2c-sda-falling-time-ns = <300>;
+		i2c-scl-falling-time-ns = <300>;
 	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index e89e3e2..3bcdebf 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -219,7 +219,7 @@ i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 		 *
 		 * If your hardware is free from tHD;STA issue, try this one.
 		 */
-		return (ic_clk * tSYMBOL + 5000) / 10000 - 8 + offset;
+		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
 	else
 		/*
 		 * Conditional expression:
@@ -235,7 +235,8 @@ i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 		 * The reason why we need to take into account "tf" here,
 		 * is the same as described in i2c_dw_scl_lcnt().
 		 */
-		return (ic_clk * (tSYMBOL + tf) + 5000) / 10000 - 3 + offset;
+		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
+			- 3 + offset;
 }
 
 static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
@@ -251,7 +252,7 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 	 * account the fall time of SCL signal (tf).  Default tf value
 	 * should be 0.3 us, for safety.
 	 */
-	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
+	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
 }
 
 static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
@@ -288,6 +289,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	u32 input_clock_khz;
 	u32 hcnt, lcnt;
 	u32 reg;
+	u32 sda_falling_time, scl_falling_time;
 
 	input_clock_khz = dev->get_clk_rate_khz(dev);
 
@@ -309,15 +311,18 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	/* set standard and fast speed deviders for high/low periods */
 
+	sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
+	scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
+
 	/* Standard-mode */
 	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				40,	/* tHD;STA = tHIGH = 4.0 us */
-				3,	/* tf = 0.3 us */
+				4000,	/* tHD;STA = tHIGH = 4.0 us */
+				sda_falling_time,
 				0,	/* 0: DW default, 1: Ideal */
 				0);	/* No offset */
 	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				47,	/* tLOW = 4.7 us */
-				3,	/* tf = 0.3 us */
+				4700,	/* tLOW = 4.7 us */
+				scl_falling_time,
 				0);	/* No offset */
 
 	/* Allow platforms to specify the ideal HCNT and LCNT values */
@@ -331,13 +336,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	/* Fast-mode */
 	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-				6,	/* tHD;STA = tHIGH = 0.6 us */
-				3,	/* tf = 0.3 us */
+				600,	/* tHD;STA = tHIGH = 0.6 us */
+				sda_falling_time,
 				0,	/* 0: DW default, 1: Ideal */
 				0);	/* No offset */
 	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-				13,	/* tLOW = 1.3 us */
-				3,	/* tf = 0.3 us */
+				1300,	/* tLOW = 1.3 us */
+				scl_falling_time,
 				0);	/* No offset */
 
 	if (dev->fs_hcnt && dev->fs_lcnt) {
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e8a7565..d66b6cb 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -99,6 +99,8 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
 	u32			sda_hold_time;
+	u32			sda_falling_time;
+	u32			scl_falling_time;
 	u16			ss_hcnt;
 	u16			ss_lcnt;
 	u16			fs_hcnt;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d0bdac0..fc24399 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -159,6 +159,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
 					"i2c-sda-hold-time-ns", &ht);
 		dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000,
 					     1000000);
+
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-sda-falling-time-ns",
+				     &dev->sda_falling_time);
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-scl-falling-time-ns",
+				     &dev->scl_falling_time);
 	}
 
 	dev->functionality =
-- 
1.7.1

Re: [PATCH v2 1/2] i2c designware make SCL and SDA falling time configurable

[Wolfram Sang]

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

On Mon, Jan 20, 2014 at 05:43:43PM +0100, Romain Baeriswyl wrote:
> This patch allows to set independantly SCL and SDA falling times.
> The tLOW period is computed by taking into account the SCL falling time.
> The tHIGH period is computed by taking into account the SDA falling time.
> 
> For instance in case the margin on tLOW is considered too small, it can
> be increased by increasing the SCL falling time which is by default set
> at 300ns.
> 
> The same applies for tHIGH period with the help of SDA falling time.
> 
> Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
> Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>

Applied to for-next, thanks!


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

[PATCH 2/2] i2c designware add support of I2C standard mode

[Romain Baeriswyl]

Some legacy devices support ony I2C standard mode at 100kHz. 
This patch allows to select the standard mode through the DTS 
with the use of the existing clock-frequency parameter.

When clock-frequency parameter is not set, the fast mode is selected.
Only when the parameter is set at 100000, the standard mode is selected.

Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |   17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index 5199b0c..16d0bef 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -9,6 +9,7 @@ Required properties :
 Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
+   Only standard mode at 100kHz and fast mode at 400kHz are supported.
 
 Optional properties :
  - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0d6fe47..5f06848 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -87,6 +87,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem;
 	int irq, r;
+	u32 clk_freq;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -122,6 +123,9 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	/* fast mode by default */
+	clk_freq = 400000;
+
 	if (pdev->dev.of_node) {
 		u32 ht = 0;
 		u32 ic_clk = dev->get_clk_rate_khz(dev);
@@ -136,6 +140,11 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		of_property_read_u32(pdev->dev.of_node,
 				     "i2c-scl-falling-time-ns",
 				     &dev->scl_falling_time);
+
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				     &clk_freq);
+		if (clk_freq != 100000 && clk_freq != 400000)
+			return -EINVAL;
 	}
 
 	dev->functionality =
@@ -145,8 +154,12 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
-	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
+	if (clk_freq == 100000)
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_STD;
+	else
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
 	/* Try first if we can configure the device from ACPI */
 	r = dw_i2c_acpi_configure(pdev);
-- 
1.7.1

Re: [PATCH 2/2] i2c designware add support of I2C standard mode

[Mika Westerberg]

On Tue, Oct 08, 2013 at 05:00:55PM +0200, Romain Baeriswyl wrote:
> Some legacy devices support ony I2C standard mode at 100kHz. 
> This patch allows to select the standard mode through the DTS 
> with the use of the existing clock-frequency parameter.
> 
> When clock-frequency parameter is not set, the fast mode is selected.
> Only when the parameter is set at 100000, the standard mode is selected.
> 
> Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
> Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] i2c designware add support of I2C standard mode

[Shinya Kuribayashi]

On 10/9/13 4:56 PM, Mika Westerberg wrote:> On Tue, Oct 08, 2013 at 05:00:55PM +0200, Romain Baeriswyl wrote:
>> Some legacy devices support ony I2C standard mode at 100kHz.
>> This patch allows to select the standard mode through the DTS
>> with the use of the existing clock-frequency parameter.
>>
>> When clock-frequency parameter is not set, the fast mode is selected.
>> Only when the parameter is set at 100000, the standard mode is selected.
>>
>> Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
>> Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I don't have build environment for these patches right now, but both
changes 1/2 and 2/2 look good.

It's quite reasonable and makes the code readable to convert
{3 => 300} into nsec based one.  I should have done so from the
beginning, thanks for doing this.

   Shinya

Re: [PATCH 2/2] i2c designware add support of I2C standard mode

[Wolfram Sang]

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

On Tue, Oct 08, 2013 at 05:00:55PM +0200, Romain Baeriswyl wrote:
> Some legacy devices support ony I2C standard mode at 100kHz. 
> This patch allows to select the standard mode through the DTS 
> with the use of the existing clock-frequency parameter.
> 
> When clock-frequency parameter is not set, the fast mode is selected.
> Only when the parameter is set at 100000, the standard mode is selected.
> 
> Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
> Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>

Pretty much OK. One change needed, though:

> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |   17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index 5199b0c..16d0bef 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -9,6 +9,7 @@ Required properties :
>  Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
> +   Only standard mode at 100kHz and fast mode at 400kHz are supported.

This should not go the bindings doc, since this is a linux specific
detail. It should go...

> +		if (clk_freq != 100000 && clk_freq != 400000)
> +			return -EINVAL;

... here as an dev_err message "Only x and y supported".

Rest looks good, thanks!


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

[PATCH v2 2/2] i2c designware add support of I2C standard mode

[Romain Baeriswyl]

Some legacy devices support ony I2C standard mode at 100kHz.
This patch allows to select the standard mode through the DTS
with the use of the existing clock-frequency parameter.

When clock-frequency parameter is not set, the fast mode is selected.
Only when the parameter is set at 100000, the standard mode is selected.

Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index fc24399..ae0a256 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -122,6 +122,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem;
 	int irq, r;
+	u32 clk_freq;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -151,6 +152,9 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	/* fast mode by default */
+	clk_freq = 400000;
+
 	if (pdev->dev.of_node) {
 		u32 ht = 0;
 		u32 ic_clk = dev->get_clk_rate_khz(dev);
@@ -166,6 +170,15 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		of_property_read_u32(pdev->dev.of_node,
 				     "i2c-scl-falling-time-ns",
 				     &dev->scl_falling_time);
+
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				     &clk_freq);
+
+		/* Only standard mode at 100kHz and fast mode at 400kHz
+		 * are supported.
+		 */
+		if (clk_freq != 100000 && clk_freq != 400000)
+			return -EINVAL;
 	}
 
 	dev->functionality =
@@ -175,8 +188,12 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
-	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
+	if (clk_freq == 100000)
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_STD;
+	else
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
 	/* Try first if we can configure the device from ACPI */
 	r = dw_i2c_acpi_configure(pdev);
-- 
1.7.1

Re: [PATCH v2 2/2] i2c designware add support of I2C standard mode

[Wolfram Sang]

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


> +	/* fast mode by default */

Please add "because of legacy reasons" since the usual default is
100kHz.

> +	clk_freq = 400000;

> +		/* Only standard mode at 100kHz and fast mode at 400kHz
> +		 * are supported.
> +		 */
> +		if (clk_freq != 100000 && clk_freq != 400000)
> +			return -EINVAL;

You didn't add the error message I requested last time.


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

[PATCH V3 2/2] i2c designware add support of I2C standard mode

[Romain Baeriswyl]

From: Romain Baeriswyl <Romain.Baeriswyl@abilis.com>

Some legacy devices support ony I2C standard mode at 100kHz.
This patch allows to select the standard mode through the DTS
with the use of the existing clock-frequency parameter.

When clock-frequency parameter is not set, the fast mode is selected.
Only when the parameter is set at 100000, the standard mode is selected.

Signed-off-by: Romain Baeriswyl <romainba@abilis.com>
Reviewed-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index fc24399..3685893 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -122,6 +122,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem;
 	int irq, r;
+	u32 clk_freq;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -151,6 +152,9 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
+	/* fast mode by default because of legacy reasons */
+	clk_freq = 400000;
+
 	if (pdev->dev.of_node) {
 		u32 ht = 0;
 		u32 ic_clk = dev->get_clk_rate_khz(dev);
@@ -166,6 +170,17 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		of_property_read_u32(pdev->dev.of_node,
 				     "i2c-scl-falling-time-ns",
 				     &dev->scl_falling_time);
+
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				     &clk_freq);
+
+		/* Only standard mode at 100kHz and fast mode at 400kHz
+		 * are supported.
+		 */
+		if (clk_freq != 100000 && clk_freq != 400000) {
+			dev_err(&pdev->dev, "Only 100kHz and 400kHz supported");
+			return -EINVAL;
+		}
 	}
 
 	dev->functionality =
@@ -175,8 +190,12 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
-	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
+	if (clk_freq == 100000)
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_STD;
+	else
+		dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
 	/* Try first if we can configure the device from ACPI */
 	r = dw_i2c_acpi_configure(pdev);
-- 
1.7.1

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

Sorry for the slooow response, I've been on vacation.

On Tue, Jul 16, 2013 at 01:16:18PM +0200, Christian Ruppert wrote:
> > Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
> > calculations don't suit with later DW I2C cores, then it would be
> > nice for someone who can access to the data book to update formulas,
> > or provide alternative formulas and make them selectable depending
> > on DW core versions.
> 
> I'm not having the impression there is a huge difference between the
> different generations of DW blocks. Probably we can find one formula
> that suits all blocks. We just have to be careful (in doubt rather
> conservative) with the transition times. This seems to be currently
> the case and if I understand Mika correctly, his objective is to remove
> some of this conservatism.

What I had originally in mind was that we could just pass whatever
HCNT/LCNT values we get from system FW (with the help of ACPI or DT).

However, if we can make the HCNT/LCNT calculation more accurate using tf
and tr, that are passed from DT or platform data, we should implement that
as well (as a separate patch).

> > It always helps us to have a way to calculate *HCNT and *LCNT values
> > automatically.  As said above, DW I2C core can control tHIGH, tLOW,
> > tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
> > with proper formulas.  It also helps hardware people as well to
> > provide reference HCNT/LCNT values.
> > 
> > And as a third step, if we want to use optimized HCNT/LCNT values
> > which can not be obtained from proper formulas + user-requested
> > tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
> > or provided from hardware team, then I'm fine with having a way to
> > _override_ HCNT/LCNT values.  Such direct way might be useful.
> 
> I agree. Probably it is best to have both, a default method based on
> formulas and timing parameters (the formulas are quite simple anyway)
> which works with device tree and such and a second method based on
> register values which works with mechanisms like ACPI.

I agree.

I'm going to post a new version of this patch (and the SDA hold patch) that
takes care of the ACPI case if there are no objections.