| Message ID | 20170929171637.121262-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Friday, September 29, 2017, Chris Brandt wrote: > Most systems with this i2c are going to have a clock of either 33.3MHz or > 32MHz. That 4% difference is not reason enough to warrant that the driver > to completely fail. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > v2: > * simplified error message. > --- What ever happened to this patch? Was there any objection? Thanks, Chris
On Wed, Oct 11, 2017 at 03:20:29PM +0000, Chris Brandt wrote: > On Friday, September 29, 2017, Chris Brandt wrote: > > Most systems with this i2c are going to have a clock of either 33.3MHz or > > 32MHz. That 4% difference is not reason enough to warrant that the driver > > to completely fail. > > > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > > --- > > v2: > > * simplified error message. > > --- > > What ever happened to this patch? > Was there any objection? In deed, I'd prefer to limit the settings to the two known frequencies. We shouldn't use the settings if someone is using it with e.g. 16MHz? Of course, my most favorite option would be implementing the formula instead of fixed settings, but I am not forcing that on you.
On Friday, October 13, 2017, Wolfram Sang wrote: > On Wed, Oct 11, 2017 at 03:20:29PM +0000, Chris Brandt wrote: > > On Friday, September 29, 2017, Chris Brandt wrote: > > > Most systems with this i2c are going to have a clock of either 33.3MHz > or > > > 32MHz. That 4% difference is not reason enough to warrant that the > driver > > > to completely fail. > > > > > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > > > --- > > > v2: > > > * simplified error message. > > > --- > > > > What ever happened to this patch? > > Was there any objection? > > In deed, I'd prefer to limit the settings to the two known frequencies. > We shouldn't use the settings if someone is using it with e.g. 16MHz? How about a range of -4% to +2% ? /* * TODO: Implement formula to calculate the timing values depending on * variable parent clock rate and arbitrary bus speed. * For now, just use calculations based on a 33325000Hz clock. */ rate = clk_get_rate(riic->clk); if ((rate < 32000000) || (rate > 34000000)) { dev_err(&riic->adapter.dev, "invalid parent clk (%lu). Must be 32MHz-34MHz\n", rate); clk_disable_unprepare(riic->clk); return -EINVAL; } > Of course, my most favorite option would be implementing the formula > instead of fixed settings, but I am not forcing that on you. So I looked at that. However... Technically, to do it right, to calculate the ACTUAL I2C baud rate, you have to take into effect the load resistance and capacitance of the lines in order to factor in the rise and fall times correctly. Of course that varies board to board. And then, 100kHz (standard-mode) and 400kHz (fast-mode) have different minimum HIGH/LOW times and duty cycle restrictions, so that has to be taken into account when coming up with a formula. --or-- Just live with the fact that the speed might be off by 4%. (I decided on option #2) Chris
Hi Chris, > How about a range of -4% to +2% ? I am fine with a range in general, but I don't like +x% because we should never be faster than requested. Clients may have problems with that. > Technically, to do it right, to calculate the ACTUAL I2C baud rate, you > have to take into effect the load resistance and capacitance of the > lines in order to factor in the rise and fall times correctly. Of course We have those generic bindings upstream: - i2c-scl-falling-time-ns Number of nanoseconds the SCL signal takes to fall; t(f) in the I2C specification. - i2c-scl-internal-delay-ns Number of nanoseconds the IP core additionally needs to setup SCL. - i2c-scl-rising-time-ns Number of nanoseconds the SCL signal takes to rise; t(r) in the I2C specification. - i2c-sda-falling-time-ns Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C specification. So far, that was all that is needed, however... > Just live with the fact that the speed might be off by 4%. ... as I said before, I won't force it on you for the frequencies you want to support. Regards, Wolfram
Hi Wolfram, On Friday, October 13, 2017, Wolfram Sang wrote: > > Technically, to do it right, to calculate the ACTUAL I2C baud rate, you > > have to take into effect the load resistance and capacitance of the > > lines in order to factor in the rise and fall times correctly. Of course > > We have those generic bindings upstream: > > - i2c-scl-falling-time-ns > Number of nanoseconds the SCL signal takes to fall; t(f) in the > I2C > specification. > > - i2c-scl-internal-delay-ns > Number of nanoseconds the IP core additionally needs to setup SCL. > > - i2c-scl-rising-time-ns > Number of nanoseconds the SCL signal takes to rise; t(r) in the > I2C > specification. > > - i2c-sda-falling-time-ns > Number of nanoseconds the SDA signal takes to fall; t(f) in the > I2C > specification. > > So far, that was all that is needed, however... So for now, I'm going to take the easy way out and just put in the range as you suggested. But, I think later I will revisit this and come up with an algorithm for calculating the rate/duty at run-time. I want to do that when I have more time to verify things with a scope and such. Thanks, Chris
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index c811af4c8d81..b16b54c88e65 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -299,12 +299,12 @@ static int riic_init_hw(struct riic_dev *riic, u32 spd) /* * TODO: Implement formula to calculate the timing values depending on - * variable parent clock rate and arbitrary bus speed + * variable parent clock rate and arbitrary bus speed. + * For now, just use calculations based on a 33325000Hz clock. */ rate = clk_get_rate(riic->clk); - if (rate != 33325000) { - dev_err(&riic->adapter.dev, - "invalid parent clk (%lu). Must be 33325000Hz\n", rate); + if (!rate) { + dev_err(&riic->adapter.dev, "invalid parent clock rate of 0\n"); clk_disable_unprepare(riic->clk); return -EINVAL; }
Most systems with this i2c are going to have a clock of either 33.3MHz or 32MHz. That 4% difference is not reason enough to warrant that the driver to completely fail. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- v2: * simplified error message. --- drivers/i2c/busses/i2c-riic.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)