Default-timings in Designware I2C driver

[Zeh, Werner]

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://www.nxp.com/docs/en/user-guide/UM10204.pdf
[2] https://dokumen.tips/documents/designware-dw-apb-i2c-databook-pudn-designwar...