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 <werner....@siemens.com> 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://www.nxp.com/docs/en/user-guide/UM10204.pdf
> [2]
> https://dokumen.tips/documents/designware-dw-apb-i2c-databook-pudn-designware-dwapbi2c-databook-preface-preface.html
> _______________________________________________
> coreboot mailing list -- coreboot@coreboot.org
> To unsubscribe send an email to coreboot-le...@coreboot.org
>
_______________________________________________
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
