Hi Werner, Sorry if you think https://review.coreboot.org/c/coreboot/+/34385 could break things for you. Feel free to push somewhat of a revert (if you can think of a better message than "bad counts" :) )
-Tim On Thu, Apr 29, 2021 at 8:55 AM Zeh, Werner <werner....@siemens.com> wrote: > 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/drivers/i2c/designware/dw_i2c.c#579 > [3] https://review.coreboot.org/c/coreboot/+/34385 > > > Von: Tim Wawrzynczak <twawrzync...@google.com> > Gesendet: Dienstag, 27. April 2021 23:01 > An: werner....@siemens.com > Cc: coreboot <coreboot@coreboot.org> > 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 <mailto: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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fdocs%2Fen%2Fuser-guide%2FUM10204.pdf&data=04%7C01%7Cwerner.zeh%40siemens.com%7C826e4a3f0e4648bc46e008d909bf86b6%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637551541076454574%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vM3723zODxZlpcifKoNrOhepB5lFgbFb8RI6GoXkj4w%3D&reserved=0 > [2] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdokumen.tips%2Fdocuments%2Fdesignware-dw-apb-i2c-databook-pudn-designware-dwapbi2c-databook-preface-preface.html&data=04%7C01%7Cwerner.zeh%40siemens.com%7C826e4a3f0e4648bc46e008d909bf86b6%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637551541076464568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=qOQuy%2BUJ%2FAckoG2l4Ysn5lg4em2jt4Wsy5vTdo6moJg%3D&reserved=0 > _______________________________________________ > coreboot mailing list -- mailto:coreboot@coreboot.org > To unsubscribe send an email to mailto:coreboot-le...@coreboot.org >
_______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
