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

From: Andy Shevchenko
Date: Wed Sep 25 2024 - 05:16:30 EST


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