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

[Heiko Schocher]

Hello Heinrich,

On 13.10.23 10:32, Heinrich Schuchardt wrote:
> 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?

No.

>> 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.

Yep, I looked at the exact same code, and you are correct, with

"All invocations of i2c_dw_scl_hcnt() have cond = 0, offset = 0."

> 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.

Can  you try this out? And if it works may I ask if you send a v2
with some explanation added in U-Boot code, that we sync here with
that what you found in kernel driver?

Many Thanks!

bye,
Heiko
> 
> Best regards
> 
> Heinrich

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de