[PATCH v2 0/2] Compute HS HCNT and LCNT based on HW parameters

[PATCH v2 0/2] Compute HS HCNT and LCNT based on HW parameters

[Michael Wu]

In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing parameters for
High Speed Mode") hs_hcnt and hs_lcnt are computed based on fixed tHIGH = 160
and tLOW = 320. However, the set of these fixed values only applies to the
combination of hardware parameters "IC_CAP_LOADING = 400pF" and
"IC_FREQ_OPTIMIZATION = 1". Outside of this combination, SCL frequency may not
reach 3.4 MHz if hs_hcnt and hs_lcnt are both computed using these two fixed
values.

Since there are no any registers controlling these two hardware parameters,
their values can only be declared through the device tree.

v2:
- provide more hardware information in dt-bindings
- rename "bus-loading" to "bus-capacitance-pf"
- call new i2c_dw_fw_parse_hw_params() in i2c_dw_fw_parse_and_configure() to
  parse hardware parameters from the device tree.

Michael Wu (2):
  dt-bindings: i2c: snps,designware-i2c: add bus-capacitance-pf and
    clk-freq-optimized
  i2c: dwsignware: determine HS tHIGH and tLOW based on HW parameters

 .../bindings/i2c/snps,designware-i2c.yaml        | 14 ++++++++++++++
 drivers/i2c/busses/i2c-designware-common.c       | 16 ++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h         |  6 ++++++
 drivers/i2c/busses/i2c-designware-master.c       | 14 ++++++++++++--
 4 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] dt-bindings: i2c: snps,designware-i2c: add bus-capacitance-pf and clk-freq-optimized

[Michael Wu]

Since there are no registers controlling the hardware parameters
IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION, their values can only be
declared in the device tree.

bus-capacitance-pf indicates the bus capacitance in picofarad (pF). It
affects the high and low pulse width of SCL line in high speed mode. The
only legal values for this property are 100 and 400, which are used to
calculate the tHIGH and tLOW periods for high speed mode. This property
corresponds to IC_CAP_LOADING.

clk-freq-optimized indicates that the hardware input clock frequency is
reduced by reducing the internal latency. The property affects the high
period and low period of the SCL line. The property conrresponds to
IC_CLK_FREQ_OPTIMIZATION.

Signed-off-by: Michael Wu <michael.wu@kneron.us>
---
 .../bindings/i2c/snps,designware-i2c.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
index 60035a787e5c..fc19e6a8b306 100644
--- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -87,6 +87,20 @@ properties:
       This value is used to compute the tHIGH period.
     default: 300
 
+  bus-capacitance-pf:
+    description: |
+      This property represents the bus capacitance in picofarad (pF). It
+      affects the high and low pulse width of SCL line in high speed mode.
+      The only legal values for this property are 100 and 400, which are used
+      to calculate the tHIGH and tLOW periods for high speed mode.
+    default: 100
+
+  clk-freq-optimized:
+    description: |
+      If the hardware input clock frequency is reduced by reducing the
+      internal latency, this property must be declared in the device tree. It
+      affects the high period and low period of SCL line.
+
   dmas:
     items:
       - description: TX DMA Channel
-- 
2.43.0

Re: [PATCH v2 1/2] dt-bindings: i2c: snps,designware-i2c: add bus-capacitance-pf and clk-freq-optimized

[Krzysztof Kozlowski]

On Fri, Sep 27, 2024 at 12:22:16PM +0800, Michael Wu wrote:
> Since there are no registers controlling the hardware parameters
> IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION, their values can only be
> declared in the device tree.
> 
> bus-capacitance-pf indicates the bus capacitance in picofarad (pF). It
> affects the high and low pulse width of SCL line in high speed mode. The
> only legal values for this property are 100 and 400, which are used to
> calculate the tHIGH and tLOW periods for high speed mode. This property
> corresponds to IC_CAP_LOADING.
> 
> clk-freq-optimized indicates that the hardware input clock frequency is
> reduced by reducing the internal latency. The property affects the high
> period and low period of the SCL line. The property conrresponds to
> IC_CLK_FREQ_OPTIMIZATION.
> 
> Signed-off-by: Michael Wu <michael.wu@kneron.us>
> ---
>  .../bindings/i2c/snps,designware-i2c.yaml          | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> index 60035a787e5c..fc19e6a8b306 100644
> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> @@ -87,6 +87,20 @@ properties:
>        This value is used to compute the tHIGH period.
>      default: 300
>  
> +  bus-capacitance-pf:

Why is this generic property? Is this going to be applied to all I2C
controllers? If not, you miss vendors prefix.

> +    description: |
> +      This property represents the bus capacitance in picofarad (pF). It
> +      affects the high and low pulse width of SCL line in high speed mode.
> +      The only legal values for this property are 100 and 400, which are used
> +      to calculate the tHIGH and tLOW periods for high speed mode.
> +    default: 100
> +
> +  clk-freq-optimized:

This was never tested.

You got this comment already and not much improved.

It does not look like you tested the bindings, at least after quick
look. Please run  (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +    description: |

Do not need '|' unless you need to preserve formatting.

Missing vendor prefix. Missing tests.

Also, extend the example with this.


> +      If the hardware input clock frequency is reduced by reducing the
> +      internal latency, this property must be declared in the device tree. It
> +      affects the high period and low period of SCL line.

I assume this is hardware choice, not driver?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: snps,designware-i2c: add bus-capacitance-pf and clk-freq-optimized

[Michael Wu]

On Fri, Sep 27 2024 at 10:35:09AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Sep 27, 2024 at 12:22:16PM +0800, Michael Wu wrote:
...
> > index 60035a787e5c..fc19e6a8b306 100644
> > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > @@ -87,6 +87,20 @@ properties:
> >        This value is used to compute the tHIGH period.
> >      default: 300
> >
> > +  bus-capacitance-pf:
> 
> Why is this generic property? Is this going to be applied to all I2C
> controllers? If not, you miss vendors prefix.
>
> > +    description: |
> > +      This property represents the bus capacitance in picofarad (pF). It
> > +      affects the high and low pulse width of SCL line in high speed mode.
> > +      The only legal values for this property are 100 and 400, which are
> used
> > +      to calculate the tHIGH and tLOW periods for high speed mode.
> > +    default: 100
> > +
> > +  clk-freq-optimized:
> 
> This was never tested.
> 
> You got this comment already and not much improved.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run  (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> Missing vendor prefix. Missing tests.
> 
> Also, extend the example with this.
> 

Both are snps,designware-i2c properties, and should be renamed to
snps,bus-capacitance-pf and snps,clk-freq-optimized.

I apologize for my lack of understanding of binding.

> 
> > +      If the hardware input clock frequency is reduced by reducing the
> > +      internal latency, this property must be declared in the device tree. It
> > +      affects the high period and low period of SCL line.
> 
> I assume this is hardware choice, not driver?

Yes, they are hardware properties. The driver must use this information
from the device tree to calculate SCL high and low periods appropriate
for the hardware.

Thanks & Regards,
Michael

[PATCH v2 2/2] i2c: dwsignware: determine HS tHIGH and tLOW based on HW parameters

[Michael Wu]

In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing parameter
for High Speed Mode") hs_hcnt and hs_lcnt are calculated based on fixed
tHIGH = 160 and tLOW = 120. However, the set of these fixed values only
applies to the combination of hardware parameters IC_CAP_LOADING = 400pF
and IC_CLK_FREQ_OPTIMIZATION = 1. Outside of this combination, if these
fixed tHIGH = 160 and tLOW = 120 are still used, the calculated hs_hcnt
and hs_lcnt may not be small enough, making it impossible for the SCL
frequency to reach 3.4 MHz.

Section 3.15.4.5 in DesignWare DW_apb_i2b Databook v2.03 says that when
IC_CLK_FREQ_OPTIMIZATION = 0,

    MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
		     = 120 ns for 3.4 Mbps, bus loading = 400pF
    MIN_SCL_LOWtime = 160 ns for 3.4 Mbps, bus loading = 100pF
		    = 320 ns for 3.4 Mbps, bus loading = 400pF

and section 3.15.4.6 says that when IC_CLK_FREQ_OPTIMIZATION = 1,

    MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
		     = 160 ns for 3.4 Mbps, bus loading = 400pF
    MIN_SCL_LOWtime = 120 ns for 3.4 Mbps, bus loading = 100pF
		    = 320 ns for 3.4 Mbps, bus loading = 400pF

In order to calculate more accurate hs_hcnt amd hs_lcnt, two hardware
parameters IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION must be
considered together.

Signed-off-by: Michael Wu <michael.wu@kneron.us>
---
 drivers/i2c/busses/i2c-designware-common.c | 16 ++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h   |  6 ++++++
 drivers/i2c/busses/i2c-designware-master.c | 14 ++++++++++++--
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 080204182bb5..907f049f09f8 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -372,6 +372,20 @@ static void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 		t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
 }
 
+static void i2c_dw_fw_parse_hw_params(struct dw_i2c_dev *dev)
+{
+	struct device *device = dev->dev;
+	int ret;
+
+	ret = device_property_read_u32(device, "bus-capacitance-pf", &dev->bus_capacitance_pf);
+	if (ret || dev->bus_capacitance_pf < 400)
+		dev->bus_capacitance_pf = 100;
+	else
+		dev->bus_capacitance_pf = 400;
+
+	dev->clk_freq_optimized = device_property_read_bool(device, "clk-freq-optimized");
+}
+
 int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
 {
 	struct i2c_timings *t = &dev->timings;
@@ -380,6 +394,8 @@ int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
 
 	i2c_parse_fw_timings(device, t, false);
 
+	i2c_dw_fw_parse_hw_params(dev);
+
 	i2c_dw_adjust_bus_speed(dev);
 
 	if (is_of_node(fwnode))
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 1ac2afd03a0a..893c2c53648a 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -240,6 +240,10 @@ struct reset_control;
  * @set_sda_hold_time: callback to retrieve IP specific SDA hold timing
  * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE
  * @rinfo: I²C GPIO recovery information
+ * @bus_capacitance_pf: bus capacitance in picofarad
+ * @clk_freq_optimized: indicate whether the hardware input clock frequency is
+ *	reduced by reducing the internal latency required to generate the high
+ *	period and low period of the SCL line
  *
  * 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.
@@ -297,6 +301,8 @@ struct dw_i2c_dev {
 	int			(*set_sda_hold_time)(struct dw_i2c_dev *dev);
 	int			mode;
 	struct i2c_bus_recovery_info rinfo;
+	u32			bus_capacitance_pf;
+	bool			clk_freq_optimized;
 };
 
 #define ACCESS_INTR_MASK			BIT(0)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index e46f1b22c360..b18da66da6a8 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -154,12 +154,22 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
 			dev->hs_hcnt = 0;
 			dev->hs_lcnt = 0;
 		} else if (!dev->hs_hcnt || !dev->hs_lcnt) {
+			u32 t_high, t_low;
+
+			if (dev->bus_capacitance_pf == 400) {
+				t_high = dev->clk_freq_optimized ? 160 : 120;
+				t_low = 320;
+			} else {
+				t_high = 60;
+				t_low = dev->clk_freq_optimized ? 120 : 160;
+			}
+
 			ic_clk = i2c_dw_clk_rate(dev);
 			dev->hs_hcnt =
 				i2c_dw_scl_hcnt(dev,
 						DW_IC_HS_SCL_HCNT,
 						ic_clk,
-						160,	/* tHIGH = 160 ns */
+						t_high,
 						sda_falling_time,
 						0,	/* DW default */
 						0);	/* No offset */
@@ -167,7 +177,7 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
 				i2c_dw_scl_lcnt(dev,
 						DW_IC_HS_SCL_LCNT,
 						ic_clk,
-						320,	/* tLOW = 320 ns */
+						t_low,
 						scl_falling_time,
 						0);	/* No offset */
 		}
-- 
2.43.0

Re: [PATCH v2 2/2] i2c: dwsignware: determine HS tHIGH and tLOW based on HW parameters

[Krzysztof Kozlowski]

On Fri, Sep 27, 2024 at 12:22:17PM +0800, Michael Wu wrote:
> In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing parameter
> for High Speed Mode") hs_hcnt and hs_lcnt are calculated based on fixed
> tHIGH = 160 and tLOW = 120. However, the set of these fixed values only
> applies to the combination of hardware parameters IC_CAP_LOADING = 400pF
> and IC_CLK_FREQ_OPTIMIZATION = 1. Outside of this combination, if these
> fixed tHIGH = 160 and tLOW = 120 are still used, the calculated hs_hcnt
> and hs_lcnt may not be small enough, making it impossible for the SCL
> frequency to reach 3.4 MHz.
> 
> Section 3.15.4.5 in DesignWare DW_apb_i2b Databook v2.03 says that when
> IC_CLK_FREQ_OPTIMIZATION = 0,
> 
>     MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
> 		     = 120 ns for 3.4 Mbps, bus loading = 400pF
>     MIN_SCL_LOWtime = 160 ns for 3.4 Mbps, bus loading = 100pF
> 		    = 320 ns for 3.4 Mbps, bus loading = 400pF
> 
> and section 3.15.4.6 says that when IC_CLK_FREQ_OPTIMIZATION = 1,
> 
>     MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
> 		     = 160 ns for 3.4 Mbps, bus loading = 400pF
>     MIN_SCL_LOWtime = 120 ns for 3.4 Mbps, bus loading = 100pF
> 		    = 320 ns for 3.4 Mbps, bus loading = 400pF
> 
> In order to calculate more accurate hs_hcnt amd hs_lcnt, two hardware
> parameters IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION must be
> considered together.
> 
> Signed-off-by: Michael Wu <michael.wu@kneron.us>
> ---
>  drivers/i2c/busses/i2c-designware-common.c | 16 ++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h   |  6 ++++++
>  drivers/i2c/busses/i2c-designware-master.c | 14 ++++++++++++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 080204182bb5..907f049f09f8 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -372,6 +372,20 @@ static void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
>  		t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
>  }
>  
> +static void i2c_dw_fw_parse_hw_params(struct dw_i2c_dev *dev)
> +{
> +	struct device *device = dev->dev;
> +	int ret;
> +
> +	ret = device_property_read_u32(device, "bus-capacitance-pf", &dev->bus_capacitance_pf);
> +	if (ret || dev->bus_capacitance_pf < 400)

Your binding said nothing about some limits here. Something is not
complete. You might be missing constraints in the binding.


> +		dev->bus_capacitance_pf = 100;
> +	else
> +		dev->bus_capacitance_pf = 400;
> +
> +	dev->clk_freq_optimized = device_property_read_bool(device, "clk-freq-optimized");

Best regards,
Krzysztof

Re: [PATCH v2 2/2] i2c: dwsignware: determine HS tHIGH and tLOW based on HW parameters

[Andy Shevchenko]

On Fri, Sep 27, 2024 at 12:22:17PM +0800, Michael Wu wrote:
> In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing parameter
> for High Speed Mode") hs_hcnt and hs_lcnt are calculated based on fixed
> tHIGH = 160 and tLOW = 120. However, the set of these fixed values only
> applies to the combination of hardware parameters IC_CAP_LOADING = 400pF
> and IC_CLK_FREQ_OPTIMIZATION = 1. Outside of this combination, if these
> fixed tHIGH = 160 and tLOW = 120 are still used, the calculated hs_hcnt
> and hs_lcnt may not be small enough, making it impossible for the SCL
> frequency to reach 3.4 MHz.
> 
> Section 3.15.4.5 in DesignWare DW_apb_i2b Databook v2.03 says that when
> IC_CLK_FREQ_OPTIMIZATION = 0,
> 
>     MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
> 		     = 120 ns for 3.4 Mbps, bus loading = 400pF
>     MIN_SCL_LOWtime = 160 ns for 3.4 Mbps, bus loading = 100pF
> 		    = 320 ns for 3.4 Mbps, bus loading = 400pF
> 
> and section 3.15.4.6 says that when IC_CLK_FREQ_OPTIMIZATION = 1,
> 
>     MIN_SCL_HIGHtime = 60 ns for 3.4 Mbps, bus loading = 100pF
> 		     = 160 ns for 3.4 Mbps, bus loading = 400pF
>     MIN_SCL_LOWtime = 120 ns for 3.4 Mbps, bus loading = 100pF
> 		    = 320 ns for 3.4 Mbps, bus loading = 400pF
> 
> In order to calculate more accurate hs_hcnt amd hs_lcnt, two hardware
> parameters IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION must be
> considered together.

...

> +static void i2c_dw_fw_parse_hw_params(struct dw_i2c_dev *dev)

Separate function is an overkill. Just inline its contents...

...

> +	ret = device_property_read_u32(device, "bus-capacitance-pf", &dev->bus_capacitance_pf);
> +	if (ret || dev->bus_capacitance_pf < 400)
> +		dev->bus_capacitance_pf = 100;
> +	else
> +		dev->bus_capacitance_pf = 400;
> +
> +	dev->clk_freq_optimized = device_property_read_bool(device, "clk-freq-optimized");

...

>  	i2c_parse_fw_timings(device, t, false);
>  
> +	i2c_dw_fw_parse_hw_params(dev);

...here.

>  	i2c_dw_adjust_bus_speed(dev);

...

> + * @bus_capacitance_pf: bus capacitance in picofarad

picofarads

...

> +			u32 t_high, t_low;
> +
> +			if (dev->bus_capacitance_pf == 400) {
> +				t_high = dev->clk_freq_optimized ? 160 : 120;
> +				t_low = 320;
> +			} else {

Yeah, this has no protection against wrong values in the DT/ACPI.
So, perhaps

			} else if (...) {

and assign defaults or bail out with an error?

> +				t_high = 60;
> +				t_low = dev->clk_freq_optimized ? 120 : 160;
> +			}

-- 
With Best Regards,
Andy Shevchenko