| Message ID | 1417574237-4328-1-git-send-email-addy.ke@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Addy, On Tue, Dec 2, 2014 at 6:37 PM, Addy Ke <addy.ke@rock-chips.com> wrote: > high_ns calculated from the low division of CLKDIV register is the sum > of actual measured high_ns and rise_ns. The rise time which related to > external pull-up resistor can be up to the maximum rise time in I2C spec. > > In my test, if external pull-up resistor is 4.7K, rise_ns is about > 700ns. So the actual measured high_ns is about 3900ns, which is less > than 4000ns(the minimum high_ns in I2C spec). > > To fix this bug, min_low_ns should include fall time and min_high_ns > should include rise time too. > > This patch merged the patch that Doug submitted to chromium, which > can get the rise and fall times for signals from the device tree. > This allows us to more accurately calculate timings. see: > https://chromium-review.googlesource.com/#/c/232774/ > > Signed-off-by: Addy Ke <addy.ke@rock-chips.com> > --- > Changes in v2: > - merged the patch that Doug submitted to chromium > > Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 10 ++++ > drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++------- > 2 files changed, 47 insertions(+), 18 deletions(-) This looks good to me. Thank you for spinning. Reviewed-by: Doug Anderson <dianders@chromium.org>
> + - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec). > + If not specified this is assumed to be the max the spec allows > + (1000 ns for standard mode, 300 ns for fast mode) which might > + cause slightly slower communication. > + - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0 > + spec). If not specified this is assumed to be the max the spec > + allows (300 ns) which might cause slightly slower communication. We already have those bindings from the designware driver: - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds. - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds. - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds. Can you reuse them? Or do you really need a specific rise-time property? If so, please matche the style of the bindings above.
Wolfram, On Wed, Dec 3, 2014 at 3:15 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> + - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec). >> + If not specified this is assumed to be the max the spec allows >> + (1000 ns for standard mode, 300 ns for fast mode) which might >> + cause slightly slower communication. >> + - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0 >> + spec). If not specified this is assumed to be the max the spec >> + allows (300 ns) which might cause slightly slower communication. > > We already have those bindings from the designware driver: > > - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds. > - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds. > - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds. > > Can you reuse them? Or do you really need a specific rise-time property? > If so, please matche the style of the bindings above. Ah, doh! I should have thought to look for existing bindings. Sorry about that. :( If you don't read all the below, my belief is that we should simply rename the strings in Addy's patch. We should change "rise-ns" to "i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time". Wolfram: can you confirm this is OK? I'm voting to leave the "-ns" off the end of both to avoid asymmetry. --- Details: Hrm, we seem to need different parameters than designware i2c. The designware bus is specifying "i2c-sda-hold-time-ns". On Rockchip i2c we specify just the number of cycles that the clk line should be high and the number of cycles that it should be low. The adapter does the rest of the work. As I understand it, the data hold time on Rockchip i2c is equal to half the low time, for instance. That was indicated by Addy who talked to the IC engineer. Because of the above, I _think_ that means that specifying "i2c-sda-hold-time-ns" is not appropriate for us, or at least not easy to convert in a sane way. We could add a "i2c-scl-hold-time-ns", but if I understand correctly I think that the "rise-time" describes the hardware in a cleaner way. If you specify the hold time then (I think) that it requires the user to tweak it whenever he/she adjusts the bus speed. In other words if you have a bus and you decide to move it from running at 400kHz to running at 300kHz (signal integrity issues?) or 100kHz, you need to manually modify the hold time. ...on the other hand the rise time is a property of the hardware I think (size of resistor, etc). For the falling times I guess we should use the "i2c-scl-falling-time" and not list the "i2c-sda-falling-time" for now? As per above the controller takes in the high/low period of the clocks and figures out the rest itself. Possibly we will need to account for i2c-sda-falling-time eventually, but maybe that can come later? -Doug
> If you don't read all the below, my belief is that we should simply > rename the strings in Addy's patch. We should change "rise-ns" to > "i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time". > Wolfram: can you confirm this is OK? I'm voting to leave the "-ns" > off the end of both to avoid asymmetry. New binding should have the "-ns" suffix, right? So, I'd vote to add the suffix to the new bindings and deprecate the ones used in the designware driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns" It might be a little more work now, but it will help us in the future, because it is the correct way to do it.
Wolfram, On Thu, Dec 4, 2014 at 10:40 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> If you don't read all the below, my belief is that we should simply >> rename the strings in Addy's patch. We should change "rise-ns" to >> "i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time". >> Wolfram: can you confirm this is OK? I'm voting to leave the "-ns" >> off the end of both to avoid asymmetry. > > New binding should have the "-ns" suffix, right? So, I'd vote to add the > suffix to the new bindings and deprecate the ones used in the designware > driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns" > > It might be a little more work now, but it will help us in the future, > because it is the correct way to do it. OK, that sounds fair. You're OK with my proposal otherwise? Are you looking for Addy or me to submit a patch supporting the "-ns" suffix on the Designware i2c driver, or should we just use the "-ns" suffix in the Rockchip driver and assume someone will later add support for the new binding in the Designware i2c driver? -Doug
> > New binding should have the "-ns" suffix, right? So, I'd vote to add the > > suffix to the new bindings and deprecate the ones used in the designware > > driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns" > > > > It might be a little more work now, but it will help us in the future, > > because it is the correct way to do it. > > OK, that sounds fair. You're OK with my proposal otherwise? Yes. > Are you looking for Addy or me to submit a patch supporting the "-ns" > suffix on the Designware i2c driver, or should we just use the "-ns" > suffix in the Rockchip driver and assume someone will later add > support for the new binding in the Designware i2c driver? I'd be really happy if you can do it. I'd love to have consistency right away.
Hi, On Thu, Dec 4, 2014 at 11:03 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> > New binding should have the "-ns" suffix, right? So, I'd vote to add the >> > suffix to the new bindings and deprecate the ones used in the designware >> > driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns" >> > >> > It might be a little more work now, but it will help us in the future, >> > because it is the correct way to do it. >> >> OK, that sounds fair. You're OK with my proposal otherwise? > > Yes. OK, for simplicity I put my requested changes in <https://chromium-review.googlesource.com/#/c/232774/3> >> Are you looking for Addy or me to submit a patch supporting the "-ns" >> suffix on the Designware i2c driver, or should we just use the "-ns" >> suffix in the Rockchip driver and assume someone will later add >> support for the new binding in the Designware i2c driver? > > I'd be really happy if you can do it. I'd love to have consistency right > away. Well that was easy. https://patchwork.kernel.org/patch/5445231/ -Doug
diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt index dde6c22..faf5997 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt @@ -21,6 +21,13 @@ Required on RK3066, RK3188 : Optional properties : - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used. + - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec). + If not specified this is assumed to be the max the spec allows + (1000 ns for standard mode, 300 ns for fast mode) which might + cause slightly slower communication. + - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0 + spec). If not specified this is assumed to be the max the spec + allows (300 ns) which might cause slightly slower communication. Example: @@ -39,4 +46,7 @@ i2c0: i2c@2002d000 { clock-names = "i2c"; clocks = <&cru PCLK_I2C0>; + + rise-ns = <800>; + fall-ns = <100>; }; diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 0ee5802..025d4b5 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -102,6 +102,8 @@ struct rk3x_i2c { /* Settings */ unsigned int scl_frequency; + unsigned int rise_ns; + unsigned int fall_ns; /* Synchronization & notification */ spinlock_t lock; @@ -435,6 +437,8 @@ out: * * @clk_rate: I2C input clock rate * @scl_rate: Desired SCL rate + * @rise_ns: How many ns it takes for signals to rise. + * @fall_ns: How many ns it takes for signals to fall. * @div_low: Divider output for low * @div_high: Divider output for high * @@ -443,11 +447,14 @@ out: * too high, we silently use the highest possible rate. */ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate, + unsigned long rise_ns, unsigned long fall_ns, unsigned long *div_low, unsigned long *div_high) { + unsigned long spec_min_low_ns, spec_min_high_ns; + unsigned long spec_max_data_hold_ns; + unsigned long spec_data_hold_buffer_ns; + unsigned long min_low_ns, min_high_ns; - unsigned long max_data_hold_ns; - unsigned long data_hold_buffer_ns; unsigned long max_low_ns, min_total_ns; unsigned long clk_rate_khz, scl_rate_khz; @@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate, /* * min_low_ns: The minimum number of ns we need to hold low - * to meet i2c spec + * to meet i2c spec, should include fall time. * min_high_ns: The minimum number of ns we need to hold high - * to meet i2c spec + * to meet i2c spec, should include rise time. * max_low_ns: The maximum number of ns we can hold low - * to meet i2c spec + * to meet i2c spec. * * Note: max_low_ns should be (max data hold time * 2 - buffer) * This is because the i2c host on Rockchip holds the data line * for half the low time. */ if (scl_rate <= 100000) { - min_low_ns = 4700; - min_high_ns = 4000; - max_data_hold_ns = 3450; - data_hold_buffer_ns = 50; + spec_min_low_ns = 4700; + spec_min_high_ns = 4000; + spec_max_data_hold_ns = 3450; + spec_data_hold_buffer_ns = 50; } else { - min_low_ns = 1300; - min_high_ns = 600; - max_data_hold_ns = 900; - data_hold_buffer_ns = 50; + spec_min_low_ns = 1300; + spec_min_high_ns = 600; + spec_max_data_hold_ns = 900; + spec_data_hold_buffer_ns = 50; } - max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns; + min_low_ns = spec_min_low_ns + fall_ns; + min_high_ns = spec_min_high_ns + rise_ns; + max_low_ns = spec_max_data_hold_ns * 2 - spec_data_hold_buffer_ns; min_total_ns = min_low_ns + min_high_ns; /* Adjust to avoid overflow */ @@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate) u64 t_low_ns, t_high_ns; int ret; - ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low, - &div_high); + ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns, + i2c->fall_ns, &div_low, &div_high); WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency); @@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long switch (event) { case PRE_RATE_CHANGE: if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency, - &div_low, &div_high) != 0) { + i2c->rise_ns, i2c->fall_ns, &div_low, + &div_high) != 0) return NOTIFY_STOP; - } /* scale up */ if (ndata->new_rate > ndata->old_rate) @@ -859,6 +868,16 @@ static int rk3x_i2c_probe(struct platform_device *pdev) i2c->scl_frequency = DEFAULT_SCL_RATE; } + /* Read rise and fall ns; if not there use the default max from spec */ + if (of_property_read_u32(pdev->dev.of_node, "rise-ns", &i2c->rise_ns)) { + if (i2c->scl_frequency <= 100000) + i2c->rise_ns = 1000; + else + i2c->rise_ns = 300; + } + if (of_property_read_u32(pdev->dev.of_node, "fall-ns", &i2c->fall_ns)) + i2c->fall_ns = 300; + strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name)); i2c->adap.owner = THIS_MODULE; i2c->adap.algo = &rk3x_i2c_algorithm;
high_ns calculated from the low division of CLKDIV register is the sum of actual measured high_ns and rise_ns. The rise time which related to external pull-up resistor can be up to the maximum rise time in I2C spec. In my test, if external pull-up resistor is 4.7K, rise_ns is about 700ns. So the actual measured high_ns is about 3900ns, which is less than 4000ns(the minimum high_ns in I2C spec). To fix this bug, min_low_ns should include fall time and min_high_ns should include rise time too. This patch merged the patch that Doug submitted to chromium, which can get the rise and fall times for signals from the device tree. This allows us to more accurately calculate timings. see: https://chromium-review.googlesource.com/#/c/232774/ Signed-off-by: Addy Ke <addy.ke@rock-chips.com> --- Changes in v2: - merged the patch that Doug submitted to chromium Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 10 ++++ drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++------- 2 files changed, 47 insertions(+), 18 deletions(-)