[coreboot] Re: Default-timings in Designware I2C driver

[Zeh, Werner]

In the meantime Kyösti provided a patch [1] where the clock timing is computed based on rise and fall times in the default case.
This results in a much more accurate timings which I was able to measure on mb/siemens/mc_apl6

Nevertheless, looking closer to the driver implementation, a few odds are in there.
First, the values 7 and 1 [2] (which actually should be 8 and 1 according to the Synopsis specification) are completely uncommented and therefore hard to understand from somebody without the specification in mind.
In addition, I still have not fully understood why the spike rejection counter value is subtracted from the count value for the high period as it is not described that way in the specification.

Further on, we easily can run into issues if the OS decides to switch the bus frequency from <= FULL_SPEED_PLUS to HIGH_SPEED on some platforms as e.g. on Apollo Lake there is a stronger output driver enabled internally for HIGH_SPEED mode which significantly changes the rise and fall times. So maybe the earlier implementation, where we had a set of parameters reported in ACPI for every implemented bus speed (removed with [3]), was not so bad in this regard.

Werner

[1] https://review.coreboot.org/c/coreboot/+/52723
[2] https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...
[3] https://review.coreboot.org/c/coreboot/+/34385

Von: Tim Wawrzynczak  
Gesendet: Dienstag, 27. April 2021 23:01
An: [e-mail]
Cc: coreboot 
Betreff: Re: [coreboot] Default-timings in Designware I2C driver

Hi Werner,

I agree we could probably do a little better than we do today (feel free to submit a patch ;)),
but speaking for what I have seen on bringing up Chrome OS devices, the ODMs ultimately 
end up having to tweak these values depending on the board design (bus load, etc.); e.g.

```
$
 git grep 'scl_lcnt' -- src/mainboard/google/*.cb | wc -l
72
```

One idea we've had is to implement a sysfs interface in the Linux kernel to expose
a way to change these registers at runtime, so they can be tuned, or add a custom
module to our payload (depthcharge) to allow tuning these values in the CLI.

-Tim

On Tue, Apr 27, 2021 at 3:01 AM Zeh, Werner  wrote:
Hi everybody.

We do have a driver for a widely used I2C-controller from Designware in src/drivers/i2c/designware.
It is used across multiple platforms and has a quite good configurability from mainboard side. One can tweak everything needed and make it work with the needed timing easily.
Though I stumbled lately across a thing I wouldn't expect the way it is.

It is possible to define the speed one needs to operate the I2C bus on in devicetree by setting the .speed-value to one of the supported defines (I2C_SPEED_STANDARD, I2C_SPEED_FAST, I2C_SPEED_FAST_PLUS, I2C_SPEED_HIGH or I2C_SPEED_FAST_ULTRA), e.g.:

register "common_soc_config" = "{
        .i2c[0] = {
                .speed = I2C_SPEED_STANDARD
                 }
        }"

If one just sets the .speed value, the driver will take care about the needed SCL clock rate and set it up to a default value. Yes, these values might be not the best for the current bus load but should at least provide a clock that matches the requested speed. 
This default values are defined in src/drivers/i2c/designware/dw_i2c.c as follows:
/* High and low times in different speed modes (in ns) */
enum {
        /* SDA Hold Time */
        DEFAULT_SDA_HOLD_TIME   = 300,
        /* Standard Speed */
        MIN_SS_SCL_HIGHTIME     = 4000,
        MIN_SS_SCL_LOWTIME              = 4700,
        /* Fast Speed */
        MIN_FS_SCL_HIGHTIME     = 600,
        MIN_FS_SCL_LOWTIME              = 1300,
        /* Fast Plus Speed */
        MIN_FP_SCL_HIGHTIME     = 260,
        MIN_FP_SCL_LOWTIME      = 500,
        /* High Speed */
        MIN_HS_SCL_HIGHTIME     = 60,
        MIN_HS_SCL_LOWTIME      = 160,
};

While these values do match the needed _minimum_ durations for the high and low period of the SCL signal according to the I2C-specification [1], they are not enough for the overall clock cycle to match the requested speed. Let's take standard speed for example:

MIN_SS_SCL_HIGHTIME = 4,0 us, MIN_SS_SCL_LOWTIME = 4,7 µs 
        ==> MIN_SS_SCL_HIGHTIME + MIN_SS_SCL_LOWTIME = 8,7 µs which equals to 114,943 kHz instead of 100 kHz
The same applies to the other speeds as well.

As the driver is aware of the clock the IP is clocked with inside the Chipset/SoC (CONFIG_DRIVERS_I2C_DESIGNWARE_CLOCK_MHZ), it can be done right. There is code already which computes the needed counter values for high and low period from the provided IP-clock, *LOWTIME and *HIGHTIME in dw_i2c_gen_speed_config():
...
if (speed >= I2C_SPEED_HIGH) {
        /* High speed */
        hcnt_min = MIN_HS_SCL_HIGHTIME;
        lcnt_min = MIN_HS_SCL_LOWTIME;
} else if (speed >= I2C_SPEED_FAST_PLUS) {
        /* Fast-Plus speed */
        hcnt_min = MIN_FP_SCL_HIGHTIME;
        lcnt_min = MIN_FP_SCL_LOWTIME;
} else if (speed >= I2C_SPEED_FAST) {
        /* Fast speed */
        hcnt_min = MIN_FS_SCL_HIGHTIME;
        lcnt_min = MIN_FS_SCL_LOWTIME;
} else {
        /* Standard speed */
        hcnt_min = MIN_SS_SCL_HIGHTIME;
        lcnt_min = MIN_SS_SCL_LOWTIME;
}
config->speed = speed;
config->scl_hcnt = ic_clk * hcnt_min / KHz;
config->scl_lcnt = ic_clk * lcnt_min / KHz;
config->sda_hold = ic_clk * DEFAULT_SDA_HOLD_TIME / KHz;
...

I tend to change the high and low values (MIN_{SS,FS,FPHS}_SCL_{HIGH,LOW}TIME) to match the needed minimum constraints while still fulfilling the overall clock cycle, e.g. by increasing the HIGHTIME.
But I would like to hear other opinions on that. Surely I am not aware of all the use cases out there.

And again: These default values can easily be overridden in the devicetree with something like this:
register "common_soc_config" = "{
        .i2c[0] = {
                .speed = I2C_SPEED_STANDARD,
                .speed_config[0] = {
                        .speed = I2C_SPEED_STANDARD,
                        .scl_lcnt = 664,
                        .scl_hcnt = 657, /*This value is smaller due to the IP adding a count of 8 internally, see [2]*/
                        .sda_hold = 39
                }
        },
}"

But I would expect the driver to do it right in the default configuration.

Any input is highly welcome.

Werner

[1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.co...
[2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdokumen.ti...
_______________________________________________
coreboot mailing list -- mailto:[e-mail]
To unsubscribe send an email to mailto:[e-mail]