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
