| Message ID | 20241001082937.680372-3-michael.wu@kneron.us |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | Compute HS HCNT and LCNT based on HW parameters | expand |
Hi Michael, On Tue, Oct 01, 2024 at 04:29:34PM GMT, 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. If someone is not familiar with the terms you are using this paragraph is completely meaningless. Can you please describe or at least expandi in parenthesis: hs_hcnt hs_lcnt tHIGH/tLOW (this is easy, but redundancy in commit log is never enough) IC_CAP_LOADING IC_CLK_FREQ_OPTIMIZATION > 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> > --- ... > + * @bus_capacitance_pf: bus capacitance in picofarads > + * @clk_freq_optimized: indicate whether hardware input clock frequency is /indicate/indicates/ /hardware/the hardware/ > + * reduced by reducing the internal latency The sentence above is not really meaningful and it's not describing what "clk_freq_optimized" is. > * > * 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. ... > + u32 t_high, t_low; > + > + /* > + * The legal values stated in the databook for bus > + * capacitance are only 100pF and 400pF. > + * If dev->bus_capacitance_pf is greater than or equals > + * to 400, t_high and t_low are assumed to be > + * appropriate values for 400pF, otherwise 100pF. > + */ > + if (dev->bus_capacitance_pf >= 400) { > + /* assume bus capacitance is 400pF */ > + t_high = dev->clk_freq_optimized ? 160 : 120; > + t_low = 320; > + } else { > + /* assume bus capacitance is 100pF */ > + 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 +186,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, This looks fine to me. Thanks, Andi > scl_falling_time, > 0); /* No offset */ > } > -- > 2.43.0 >
On Tue, Oct 01, 2024 at 04:29:34PM +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. ... > + * @bus_capacitance_pf: bus capacitance in picofarads Since it seems a new version of the series is warranted, and looking into the current kernel source (no other users of this unit were observed), I think we may do correct capitalisation here for the sake of physics and unit system, i.e. * @bus_capacitance_pF: bus capacitance in picofarads
Hi, > On Tue, Oct 01, 2024 at 04:29:34PM +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. > > ... > > > + * @bus_capacitance_pf: bus capacitance in picofarads > > Since it seems a new version of the series is warranted, and looking into > the current kernel source (no other users of this unit were observed), > I think we may do correct capitalisation here for the sake of physics > and unit system, i.e. > > * @bus_capacitance_pF: bus capacitance in picofarads Are you serious? I have never seen capital letters used to declare a variable name. Although checkpatch.pl does not consider this as an issue, is this against everyone's custom or unspoken rules? Sincerely, Michael
On Wed, Oct 02, 2024 at 09:31:00AM +0000, Michael Wu wrote: > > On Tue, Oct 01, 2024 at 04:29:34PM +0800, Michael Wu wrote: ... > > > + * @bus_capacitance_pf: bus capacitance in picofarads > > > > Since it seems a new version of the series is warranted, and looking into > > the current kernel source (no other users of this unit were observed), > > I think we may do correct capitalisation here for the sake of physics > > and unit system, i.e. > > > > * @bus_capacitance_pF: bus capacitance in picofarads > > Are you serious? Are we in the circus? > I have never seen capital letters used to declare a > variable name. Although checkpatch.pl does not consider this as an issue, > is this against everyone's custom or unspoken rules? I do not really care about checkpatch or any rules as any good rule will have a few exceptions anyway. The rationale here is to follow the science with much longer history than anything related to the computer programming. Now, since you haven't looked for the existing examples, try this git grep -n _u[AV]\;
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 080204182bb5..9599be1de775 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -380,6 +380,11 @@ int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev) i2c_parse_fw_timings(device, t, false); + if (device_property_read_u32(device, "snps,bus-capacitance-pf", &dev->bus_capacitance_pf)) + dev->bus_capacitance_pf = 100; + + dev->clk_freq_optimized = device_property_read_bool(device, "snps,clk-freq-optimized"); + 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..44c97bf15843 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -240,6 +240,9 @@ 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 picofarads + * @clk_freq_optimized: indicate whether hardware input clock frequency is + * reduced by reducing the internal latency * * 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 +300,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..3dbc94db94a4 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -154,12 +154,31 @@ 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; + + /* + * The legal values stated in the databook for bus + * capacitance are only 100pF and 400pF. + * If dev->bus_capacitance_pf is greater than or equals + * to 400, t_high and t_low are assumed to be + * appropriate values for 400pF, otherwise 100pF. + */ + if (dev->bus_capacitance_pf >= 400) { + /* assume bus capacitance is 400pF */ + t_high = dev->clk_freq_optimized ? 160 : 120; + t_low = 320; + } else { + /* assume bus capacitance is 100pF */ + 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 +186,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 */ }
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 | 5 +++++ drivers/i2c/busses/i2c-designware-core.h | 5 +++++ drivers/i2c/busses/i2c-designware-master.c | 23 ++++++++++++++++++++-- 3 files changed, 31 insertions(+), 2 deletions(-)