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

[PATCH 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, this 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 noted through the device tree.

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

 .../bindings/i2c/snps,designware-i2c.yaml     | 19 +++++++++++++++
 drivers/i2c/busses/i2c-designware-common.c    | 16 +++++++++++++
 drivers/i2c/busses/i2c-designware-core.h      |  8 +++++++
 drivers/i2c/busses/i2c-designware-master.c    | 24 +++++++++++++++++--
 drivers/i2c/busses/i2c-designware-platdrv.c   |  2 ++
 5 files changed, 67 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Michael Wu]

In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing
parameter for High Speed Mode") hs_hcnt and hs_hcnt are computed based on
fixed tHIGH = 160 and tLOW = 320. However, this fixed values only applies
to the set of conditions of IC_CAP_LOADING = 400pF and
IC_FREQ_OPTIMIZATION = 1. Outside of this conditions set, if this fixed
values are still used, the calculated HCNT and LCNT will make the SCL
frequency unabled to reach 3.4 MHz.

If hs_hcnt and hs_lcnt are calculated based on fixed tHIGH = 160 and
tLOW = 320, SCL frequency may not reach 3.4 MHz when IC_CAP_LOADING is not
400pF or IC_FREQ_OPTIMIZATION is not 1.

Section 3.15.4.5 in DesignWare DW_apb_i2c Databook v2.03 says 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 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 and 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    |  8 +++++++
 drivers/i2c/busses/i2c-designware-master.c  | 24 +++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-platdrv.c |  2 ++
 4 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..f0a7d0ce6fd6 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -332,6 +332,22 @@ void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_adjust_bus_speed);
 
+void i2c_dw_parse_of(struct dw_i2c_dev *dev)
+{
+	int ret;
+
+	ret = device_property_read_u32(dev->dev, "bus-loading",
+				       &dev->bus_loading);
+	if (ret || dev->bus_loading < 400)
+		dev->bus_loading = 100;
+	else
+		dev->bus_loading = 400;
+
+	dev->clk_freq_optimized =
+		device_property_read_bool(dev->dev, "clk-freq-optimized");
+
+}
+
 u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
 	/*
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e9606c00b8d1..064ba3426499 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -242,6 +242,11 @@ 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_loading: for high speed mode, the bus loading affects the high and low
+ *	pulse width of SCL
+ * @clk_freq_optimized: indicate whether the system 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.
@@ -300,6 +305,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_loading;
+	bool			clk_freq_optimized;
 };
 
 #define ACCESS_INTR_MASK			BIT(0)
@@ -329,6 +336,7 @@ struct i2c_dw_semaphore_callbacks {
 };
 
 int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
+void i2c_dw_parse_of(struct dw_i2c_dev *dev);
 u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
 u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
 int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index c7e56002809a..7b187f68394e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -140,16 +140,36 @@ 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->clk_freq_optimized) {
+				if (dev->bus_loading == 400) {
+					t_high = 160;
+					t_low = 320;
+				} else {
+					t_high = 60;
+					t_low = 120;
+				}
+			} else {
+				if (dev->bus_loading == 400) {
+					t_high = 120;
+					t_low = 320;
+				} else {
+					t_high = 60;
+					t_low = 160;
+				}
+			}
+
 			ic_clk = i2c_dw_clk_rate(dev);
 			dev->hs_hcnt =
 				i2c_dw_scl_hcnt(ic_clk,
-						160,	/* tHIGH = 160 ns */
+						t_high,
 						sda_falling_time,
 						0,	/* DW default */
 						0);	/* No offset */
 			dev->hs_lcnt =
 				i2c_dw_scl_lcnt(ic_clk,
-						320,	/* tLOW = 320 ns */
+						t_low,
 						scl_falling_time,
 						0);	/* No offset */
 		}
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index df3dc1e8093e..9fdcf1068a70 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -307,6 +307,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 
 	reset_control_deassert(dev->rst);
 
+	i2c_dw_parse_of(dev);
+
 	t = &dev->timings;
 	i2c_parse_fw_timings(&pdev->dev, t, false);
 
-- 
2.43.0

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Andy Shevchenko]

On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:
> In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing
> parameter for High Speed Mode") hs_hcnt and hs_hcnt are computed based on
> fixed tHIGH = 160 and tLOW = 320. However, this fixed values only applies
> to the set of conditions of IC_CAP_LOADING = 400pF and
> IC_FREQ_OPTIMIZATION = 1. Outside of this conditions set, if this fixed
> values are still used, the calculated HCNT and LCNT will make the SCL
> frequency unabled to reach 3.4 MHz.
> 
> If hs_hcnt and hs_lcnt are calculated based on fixed tHIGH = 160 and
> tLOW = 320, SCL frequency may not reach 3.4 MHz when IC_CAP_LOADING is not
> 400pF or IC_FREQ_OPTIMIZATION is not 1.
> 
> Section 3.15.4.5 in DesignWare DW_apb_i2c Databook v2.03 says 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 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 and hs_lcnt, two hardware
> parameters IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION must be
> considered together.

...

> +void i2c_dw_parse_of(struct dw_i2c_dev *dev)
> +{

+	struct device *device = dev->dev;

> +	int ret;
> +
> +	ret = device_property_read_u32(dev->dev, "bus-loading",
> +				       &dev->bus_loading);

	ret = device_property_read_u32(device, "bus-loading", &dev->bus_loading);

(now one line)

> +	if (ret || dev->bus_loading < 400)
> +		dev->bus_loading = 100;
> +	else
> +		dev->bus_loading = 400;

> +	dev->clk_freq_optimized =
> +		device_property_read_bool(dev->dev, "clk-freq-optimized");

	dev->clk_freq_optimized = device_property_read_bool(device, "clk-freq-optimized");

(now one line)

> +

Redundant blank line.

> +}

> + * @bus_loading: for high speed mode, the bus loading affects the high and low
> + *	pulse width of SCL

This is bad naming, better is bus_capacitance.

> + * @clk_freq_optimized: indicate whether the system clock frequency is reduced

...

> +void i2c_dw_parse_of(struct dw_i2c_dev *dev);

Should be part of i2c_dw_fw_parse_and_configure().

...

> +			if (dev->clk_freq_optimized) {
> +				if (dev->bus_loading == 400) {
> +					t_high = 160;
> +					t_low = 320;
> +				} else {
> +					t_high = 60;
> +					t_low = 120;
> +				}
> +			} else {
> +				if (dev->bus_loading == 400) {
> +					t_high = 120;
> +					t_low = 320;
> +				} else {
> +					t_high = 60;
> +					t_low = 160;
> +				}
> +			}

Can be as simple as

			if (dev->bus_loading == 400) {
				t_high = dev->clk_freq_optimized ? 160 : 120;
				t_low = 320;
			} else {
				t_high = 60;
				t_low = dev->clk_freq_optimized ? 120 : 160;
			}

It also makes it much more visible to get how this flag affects the values.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Andy Shevchenko]

On Wed, Sep 25, 2024 at 12:16:10PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:

...

> > + * @bus_loading: for high speed mode, the bus loading affects the high and low
> > + *	pulse width of SCL
> 
> This is bad naming, better is bus_capacitance.

Even more specific bus_capacitance_pf as we usually add physical units to the
variable names, so we immediately understand from the code the order of
numbers and their physical meanings.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Michael Wu]

> On Wed, Sep 25, 2024 at 12:16:10PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:
> 
> ...
> 
> > > + * @bus_loading: for high speed mode, the bus loading affects the high
> and low
> > > + *	pulse width of SCL
> >
> > This is bad naming, better is bus_capacitance.
> 
> Even more specific bus_capacitance_pf as we usually add physical units to the
> variable names, so we immediately understand from the code the order of
> numbers and their physical meanings. 

Sounds good. However, I think the length of "bus_capacitance_pf" is a bit
long, we may often encounter the limit of more than 80 characters in a
line when coding. I'll rename it to "bus_cap_pf".

Thanks & Regards,
Michael

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Andy Shevchenko]

On Thu, Sep 26, 2024 at 08:45:47AM +0000, Michael Wu wrote:
> > On Wed, Sep 25, 2024 at 12:16:10PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:

...

> > > > + * @bus_loading: for high speed mode, the bus loading affects the high
> > and low
> > > > + *	pulse width of SCL
> > >
> > > This is bad naming, better is bus_capacitance.
> > 
> > Even more specific bus_capacitance_pf as we usually add physical units to the
> > variable names, so we immediately understand from the code the order of
> > numbers and their physical meanings. 
> 
> Sounds good. However, I think the length of "bus_capacitance_pf" is a bit
> long, we may often encounter the limit of more than 80 characters in a
> line when coding. I'll rename it to "bus_cap_pf".

Limit had been relaxed to 100. I still think we may use temporary variables,
if needed, in order to make code neater. That said, I slightly prefer
bus_capacitance_pf over the shortened variant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Krzysztof Kozlowski]

On 26/09/2024 14:18, Andy Shevchenko wrote:
> On Thu, Sep 26, 2024 at 08:45:47AM +0000, Michael Wu wrote:
>>> On Wed, Sep 25, 2024 at 12:16:10PM +0300, Andy Shevchenko wrote:
>>>> On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:
> 
> ...
> 
>>>>> + * @bus_loading: for high speed mode, the bus loading affects the high
>>> and low
>>>>> + *	pulse width of SCL
>>>>
>>>> This is bad naming, better is bus_capacitance.
>>>
>>> Even more specific bus_capacitance_pf as we usually add physical units to the
>>> variable names, so we immediately understand from the code the order of
>>> numbers and their physical meanings. 
>>
>> Sounds good. However, I think the length of "bus_capacitance_pf" is a bit
>> long, we may often encounter the limit of more than 80 characters in a
>> line when coding. I'll rename it to "bus_cap_pf".
> 
> Limit had been relaxed to 100. I still think we may use temporary variables,

Just to be clear, because you encourage reformatting it to 100:

You mix coding style with checkpatch. Checkpatch does not define coding
style. Coding style doc defines it. Limit is 80, unless growing to 100
improves readability.

> if needed, in order to make code neater. That said, I slightly prefer
> bus_capacitance_pf over the shortened variant.
> 

Best regards,
Krzysztof

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Andy Shevchenko]

On Thu, Sep 26, 2024 at 03:50:07PM +0200, Krzysztof Kozlowski wrote:
> On 26/09/2024 14:18, Andy Shevchenko wrote:
> > On Thu, Sep 26, 2024 at 08:45:47AM +0000, Michael Wu wrote:
> >>> On Wed, Sep 25, 2024 at 12:16:10PM +0300, Andy Shevchenko wrote:
> >>>> On Wed, Sep 25, 2024 at 04:04:30PM +0800, Michael Wu wrote:

...

> >>>>> + * @bus_loading: for high speed mode, the bus loading affects the high
> >>> and low
> >>>>> + *	pulse width of SCL
> >>>>
> >>>> This is bad naming, better is bus_capacitance.
> >>>
> >>> Even more specific bus_capacitance_pf as we usually add physical units to the
> >>> variable names, so we immediately understand from the code the order of
> >>> numbers and their physical meanings. 
> >>
> >> Sounds good. However, I think the length of "bus_capacitance_pf" is a bit
> >> long, we may often encounter the limit of more than 80 characters in a
> >> line when coding. I'll rename it to "bus_cap_pf".
> > 
> > Limit had been relaxed to 100. I still think we may use temporary variables,
> 
> Just to be clear, because you encourage reformatting it to 100:
> 
> You mix coding style with checkpatch. Checkpatch does not define coding
> style. Coding style doc defines it. Limit is 80, unless growing to 100
> improves readability.

Somebody can still use land line rotary phones, while others are
on mobile ones, indeed. :-)

Jokes aside, the second part of my remark was in regard to how to make
the lines shorter in case somebody is so picky about 80 limit.

> > if needed, in order to make code neater. That said, I slightly prefer
> > bus_capacitance_pf over the shortened variant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Jarkko Nikula]

Hi

On 9/25/24 11:04 AM, Michael Wu wrote:
> In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing
> parameter for High Speed Mode") hs_hcnt and hs_hcnt are computed based on
> fixed tHIGH = 160 and tLOW = 320. However, this fixed values only applies
> to the set of conditions of IC_CAP_LOADING = 400pF and
> IC_FREQ_OPTIMIZATION = 1. Outside of this conditions set, if this fixed
> values are still used, the calculated HCNT and LCNT will make the SCL
> frequency unabled to reach 3.4 MHz.
> 
> If hs_hcnt and hs_lcnt are calculated based on fixed tHIGH = 160 and
> tLOW = 320, SCL frequency may not reach 3.4 MHz when IC_CAP_LOADING is not
> 400pF or IC_FREQ_OPTIMIZATION is not 1.
> 
> Section 3.15.4.5 in DesignWare DW_apb_i2c Databook v2.03 says 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 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 and 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    |  8 +++++++
>   drivers/i2c/busses/i2c-designware-master.c  | 24 +++++++++++++++++++--
>   drivers/i2c/busses/i2c-designware-platdrv.c |  2 ++
>   4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..f0a7d0ce6fd6 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -332,6 +332,22 @@ void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
>   }
>   EXPORT_SYMBOL_GPL(i2c_dw_adjust_bus_speed);
>   
> +void i2c_dw_parse_of(struct dw_i2c_dev *dev)
> +{
> +	int ret;
> +
> +	ret = device_property_read_u32(dev->dev, "bus-loading",
> +				       &dev->bus_loading);

Like Andy said better name would be bus_capacitance_pf.

Also i2c_dw_parse_of() sounds too generic and may lead to think all and 
only device tree related parameters are parsed here.

> +	if (ret || dev->bus_loading < 400)
> +		dev->bus_loading = 100;
> +	else
> +		dev->bus_loading = 400;
> +

I think these are more understandable and robust if no parameter 
adjustments are not done here but used straight in the if statements in 
the i2c_dw_set_timings_master(). Less if statements that way and all 
checked in one place.

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Andy Shevchenko]

On Wed, Sep 25, 2024 at 01:58:27PM +0300, Jarkko Nikula wrote:
> On 9/25/24 11:04 AM, Michael Wu wrote:

...

> Also i2c_dw_parse_of() sounds too generic and may lead to think all and only
> device tree related parameters are parsed here.

We already have a common (designware specific) function for this. the parsing
should be done as the part of that existing function. I.o.w. the existing just
needs to be extended for these two new properties.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Michael Wu]

> On 9/25/24 11:04 AM, Michael Wu wrote:
> > In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing
> > parameter for High Speed Mode") hs_hcnt and hs_hcnt are computed based
> > on
> > fixed tHIGH = 160 and tLOW = 320. However, this fixed values only applies
> > to the set of conditions of IC_CAP_LOADING = 400pF and
> > IC_FREQ_OPTIMIZATION = 1. Outside of this conditions set, if this fixed
> > values are still used, the calculated HCNT and LCNT will make the SCL
> > frequency unabled to reach 3.4 MHz.
> >
> > If hs_hcnt and hs_lcnt are calculated based on fixed tHIGH = 160 and
> > tLOW = 320, SCL frequency may not reach 3.4 MHz when IC_CAP_LOADING is
> > not
> > 400pF or IC_FREQ_OPTIMIZATION is not 1.
> >
> > Section 3.15.4.5 in DesignWare DW_apb_i2c Databook v2.03 says 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 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 and 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    |  8 +++++++
> >   drivers/i2c/busses/i2c-designware-master.c  | 24
> +++++++++++++++++++--
> >   drivers/i2c/busses/i2c-designware-platdrv.c |  2 ++
> >   4 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-common.c
> b/drivers/i2c/busses/i2c-designware-common.c
> > index e8a688d04aee..f0a7d0ce6fd6 100644
> > --- a/drivers/i2c/busses/i2c-designware-common.c
> > +++ b/drivers/i2c/busses/i2c-designware-common.c
> > @@ -332,6 +332,22 @@ void i2c_dw_adjust_bus_speed(struct dw_i2c_dev
> *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(i2c_dw_adjust_bus_speed);
> >
> > +void i2c_dw_parse_of(struct dw_i2c_dev *dev)
> > +{
> > +	int ret;
> > +
> > +	ret = device_property_read_u32(dev->dev, "bus-loading",
> > +				       &dev->bus_loading);
> 
> Like Andy said better name would be bus_capacitance_pf.
I'll rename it to bus_cap_pf due to the limit of not exceed 80 characters a line.
> 
> Also i2c_dw_parse_of() sounds too generic and may lead to think all and
> only device tree related parameters are parsed here.
> 
> > +	if (ret || dev->bus_loading < 400)
> > +		dev->bus_loading = 100;
> > +	else
> > +		dev->bus_loading = 400;
> > +
> 
> I think these are more understandable and robust if no parameter
> adjustments are not done here but used straight in the if statements in
> the i2c_dw_set_timings_master(). Less if statements that way and all
> checked in one place.

bus_loading is a hardware parameter, indicating the capacitance value.
Literally, it has no direct relationship with time, so I may create a new
function to parse it and other hardware parameters. Because the same
reason, it is not suitable to be adjusted in i2c_dw_set_timings_master().
Like i2c_parse_timing() declared in drivers/i2c/i2c-core-base.c,
I recommend changing its value after device_property_read_u32() if
necessary.

Thanks & Regards,
Michael

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[Krzysztof Kozlowski]

On 25/09/2024 10:04, Michael Wu wrote:
> In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing
> parameter for High Speed Mode") hs_hcnt and hs_hcnt are computed based on
> fixed tHIGH = 160 and tLOW = 320. However, this fixed values only applies
> to the set of conditions of IC_CAP_LOADING = 400pF and
> IC_FREQ_OPTIMIZATION = 1. Outside of this conditions set, if this fixed
> values are still used, the calculated HCNT and LCNT will make the SCL
> frequency unabled to reach 3.4 MHz.
> 
> If hs_hcnt and hs_lcnt are calculated based on fixed tHIGH = 160 and
> tLOW = 320, SCL frequency may not reach 3.4 MHz when IC_CAP_LOADING is not
> 400pF or IC_FREQ_OPTIMIZATION is not 1.
> 
> Section 3.15.4.5 in DesignWare DW_apb_i2c Databook v2.03 says 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 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 and 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    |  8 +++++++
>  drivers/i2c/busses/i2c-designware-master.c  | 24 +++++++++++++++++++--
>  drivers/i2c/busses/i2c-designware-platdrv.c |  2 ++
>  4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..f0a7d0ce6fd6 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -332,6 +332,22 @@ void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_adjust_bus_speed);
>  
> +void i2c_dw_parse_of(struct dw_i2c_dev *dev)
> +{
> +	int ret;
> +
> +	ret = device_property_read_u32(dev->dev, "bus-loading",

Generic properties should be described in generic schema. Where is this
one defined?

Also, bindings come before users.

Best regards,
Krzysztof

Re: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters

[kernel test robot]

Hi Michael,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.11]
[cannot apply to andi-shyti/i2c/i2c-host linus/master next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Wu/i2c-designware-determine-HS-tHIGH-and-tLOW-based-on-HW-paramters/20240925-160823
base:   v6.11
patch link:    https://lore.kernel.org/r/20240925080432.186408-2-michael.wu%40kneron.us
patch subject: [PATCH 1/2] i2c: designware: determine HS tHIGH and tLOW based on HW paramters
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240927/202409272122.Lw9sP2il-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409272122.Lw9sP2il-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409272122.Lw9sP2il-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "i2c_dw_parse_of" [drivers/i2c/busses/i2c-designware-platform.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/2] dt-bindings: i2c: snps,designware-i2c: add bus-loading 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
noted in the device tree.

"bus-loading" is used to declare the value of IC_CAP_LOADING, and
"clk-freq-optimized" is used to declare IC_CLK_FREQ_OPTIMIZATION = 1.

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

diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
index 60035a787e5c..f954f5014a00 100644
--- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -87,6 +87,16 @@ properties:
       This value is used to compute the tHIGH period.
     default: 300
 
+  bus-loading:
+    description: |
+      This property should be 100 or 400 for high speed mode.
+      This value is used to compute the tHIGH period and the tLOW period.
+    default: 100
+
+  clk_freq_optimized:
+    description: |
+      Thie property is used to declare whether the internal latency is reduced.
+
   dmas:
     items:
       - description: TX DMA Channel
@@ -146,4 +156,13 @@ examples:
       interrupts = <8>;
       clocks = <&ahb_clk>;
     };
+  - |
+    i2c@ce500000 {
+      compatible = "snps,designware-i2c";
+      reg = <0xce500000 0x1000>;
+      interrupts = <37 1>;
+      clock-frequency = <3400000>;
+      bus-loading = <400>;
+      clk-freq-optimized;
+    };
 ...
-- 
2.43.0

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

[Krzysztof Kozlowski]

On 25/09/2024 10:04, 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
> noted in the device tree.
> 
> "bus-loading" is used to declare the value of IC_CAP_LOADING, and
> "clk-freq-optimized" is used to declare IC_CLK_FREQ_OPTIMIZATION = 1.
> 
> Signed-off-by: Michael Wu <michael.wu@kneron.us>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

> ---
>  .../bindings/i2c/snps,designware-i2c.yaml     | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> index 60035a787e5c..f954f5014a00 100644
> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> @@ -87,6 +87,16 @@ properties:
>        This value is used to compute the tHIGH period.
>      default: 300
>  
> +  bus-loading:
> +    description: |
> +      This property should be 100 or 400 for high speed mode.
> +      This value is used to compute the tHIGH period and the tLOW period.

How? This wasn't ever tested obviously... Missing units in property name
(or ref but units are preferred).

> +    default: 100
> +
> +  clk_freq_optimized:

No underscores.

> +    description: |
> +      Thie property is used to declare whether the internal latency is reduced.

Your descriptions don't tell me much. Drop redundant parts "This
property" and instead describe the hardware aspect of this property.


> +
>    dmas:
>      items:
>        - description: TX DMA Channel
> @@ -146,4 +156,13 @@ examples:
>        interrupts = <8>;
>        clocks = <&ahb_clk>;
>      };
> +  - |
> +    i2c@ce500000 {

No, no need.



Best regards,
Krzysztof