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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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