[PATCH v2] i2c: riic: remove fixed clock restriction

[PATCH v2] i2c: riic: remove fixed clock restriction

[Chris Brandt]

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(-)

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;
 	}
-- 
2.14.1

RE: [PATCH v2] i2c: riic: remove fixed clock restriction

[Chris Brandt]

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

Re: [PATCH v2] i2c: riic: remove fixed clock restriction

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v2] i2c: riic: remove fixed clock restriction

[Chris Brandt]

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

Re: [PATCH v2] i2c: riic: remove fixed clock restriction

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v2] i2c: riic: remove fixed clock restriction

[Chris Brandt]

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