[PATCH 1/1] i2c: designware_i2c: adjust timing calculation

[Heinrich Schuchardt]

On 10/13/23 06:37, Heiko Schocher wrote:
> Hello Heinrich,
> 
> On 11.10.23 06:48, Heinrich Schuchardt wrote:
>> In SPL probing of the designware_i2c device on the StarFive VisionFive 2
>> board fails with
>>
>>      dw_i2c: mode 0, ic_clk 1000000, speed 100000,
>>      period 10 rise 1 fall 1 tlow 5 thigh 4 spk 0
>>      dw_i2c: bad counts. hcnt = -4 lcnt = 4
>>      device_probe: i2c at 12050000 failed to probe -22
>>
>> When changing the offset for the high phase from 7 to 1 the device is
>> probed correctly.
>>
>> Without this fix the memory size of the StarFive VisionFive 2 board cannot
>> be read from EEPROM.
>>
>> Fixes: e71b6f6622d6 ("i2c: designware_i2c: Rewrite timing calculation")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   drivers/i2c/designware_i2c.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
>> index e54de42abc..55e582091c 100644
>> --- a/drivers/i2c/designware_i2c.c
>> +++ b/drivers/i2c/designware_i2c.c
>> @@ -155,10 +155,10 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum i2c_speed_mode mode,
>>   
>>   	/*
>>   	 * Back-solve for hcnt and lcnt according to the following equations:
>> -	 * SCL_High_time = [(HCNT + IC_*_SPKLEN + 7) * ic_clk] + SCL_Fall_time
>> +	 * SCL_High_time = [(HCNT + IC_*_SPKLEN + 1) * ic_clk] + SCL_Fall_time
>>   	 * SCL_Low_time = [(LCNT + 1) * ic_clk] - SCL_Fall_time + SCL_Rise_time
>>   	 */
>> -	hcnt = min_thigh_cnt - fall_cnt - 7 - spk_cnt;
>> +	hcnt = min_thigh_cnt - fall_cnt - 1 - spk_cnt;
>>   	lcnt = min_tlow_cnt - rise_cnt + fall_cnt - 1;
>>   
>>   	if (hcnt < 0 || lcnt < 0) {
>> @@ -170,13 +170,13 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum i2c_speed_mode mode,
>>   	 * Now add things back up to ensure the period is hit. If it is off,
>>   	 * split the difference and bias to lcnt for remainder
>>   	 */
>> -	tot = hcnt + lcnt + 7 + spk_cnt + rise_cnt + 1;
>> +	tot = hcnt + lcnt + 1 + spk_cnt + rise_cnt + 1;
>>   
>>   	if (tot < period_cnt) {
>>   		diff = (period_cnt - tot) / 2;
>>   		hcnt += diff;
>>   		lcnt += diff;
>> -		tot = hcnt + lcnt + 7 + spk_cnt + rise_cnt + 1;
>> +		tot = hcnt + lcnt + 1 + spk_cnt + rise_cnt + 1;
>>   		lcnt += period_cnt - tot;
>>   	}
> 
> What are this magic constants? Are they somewhere documented in the RM?

I have no access to Designware reference manuals. Do you have?

> 
> Hmm... and does this may break other boards? Should we have this in
> someway configurable?
> 
> Tried to look fast in the linux driver... and it seems to me, this
> constants depend at least on i2c_speed_mode?
> 
> bye,
> Heiko

https://www.nxp.com/docs/en/user-guide/UM10204.pdf has I2C timing 
diagrams in Figure 38 and Figure 39.

Linux has a function i2c_dw_scl_hcnt() where depending on parameter cond 
either a value of 8 or 3 is used. All invocations of i2c_dw_scl_hcnt() 
have cond = 0, offset = 0. Linux' i2c_dw_scl_lcnt() always uses 1.

The Linux comments are:

cond==True
     IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH

cond==False
     IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)

The U-Boot comment has:

     SCL_High_time = [(HCNT + IC_*_SPKLEN + 7) * ic_clk] + SCL_Fall_time

This lets me assume that these are the matching pieces of code.

While my patch had %s/ 7 / 1 / we would need %s/ 7 / 3 / to match the 
Linux formula. This should be enough to avoid the observed "bad count" 
error on the VisionFive 2.

Best regards

Heinrich