| Message ID | 1615622664-15032-1-git-send-email-qii.wang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] i2c: mediatek: Get device clock-stretch time via dts | expand |
> tSU,STA/tHD,STA/tSU,STOP maybe out of spec due to device > clock-stretching or circuit loss, we could get device > clock-stretch time from dts to adjust these parameters > to meet the spec via EXT_CONF register. > > Signed-off-by: Qii Wang <qii.wang@mediatek.com> I will look at this next and think about it. New bindings are always a bit more time consuming.
On Sat, Mar 13, 2021 at 04:04:24PM +0800, qii.wang@mediatek.com wrote: > From: Qii Wang <qii.wang@mediatek.com> > > tSU,STA/tHD,STA/tSU,STOP maybe out of spec due to device > clock-stretching or circuit loss, we could get device > clock-stretch time from dts to adjust these parameters > to meet the spec via EXT_CONF register. > > Signed-off-by: Qii Wang <qii.wang@mediatek.com> I tried to understand from the code what the new binding expresses, but I don't fully understand it. Is it the maximum clock stretch time? Because I cannot recall a device which always uses the same delay for clock stretching.
On Tue, 2021-04-06 at 21:48 +0200, Wolfram Sang wrote: > On Sat, Mar 13, 2021 at 04:04:24PM +0800, qii.wang@mediatek.com wrote: > > From: Qii Wang <qii.wang@mediatek.com> > > > > tSU,STA/tHD,STA/tSU,STOP maybe out of spec due to device > > clock-stretching or circuit loss, we could get device > > clock-stretch time from dts to adjust these parameters > > to meet the spec via EXT_CONF register. > > > > Signed-off-by: Qii Wang <qii.wang@mediatek.com> > > I tried to understand from the code what the new binding expresses, but > I don't fully understand it. Is it the maximum clock stretch time? > Because I cannot recall a device which always uses the same delay for > clock stretching. > Due to clock stretch, our HW IP cannot meet the ac-timing spec(tSU;STA,tSU;STO). There isn't a same delay for clock stretching, so we need pass a parameter which can be found through measurement to meet most conditions.
> Due to clock stretch, our HW IP cannot meet the ac-timing > spec(tSU;STA,tSU;STO). > There isn't a same delay for clock stretching, so we need pass a > parameter which can be found through measurement to meet most > conditions. What about using this existing binding? - i2c-scl-internal-delay-ns Number of nanoseconds the IP core additionally needs to setup SCL.
On Wed, 2021-04-07 at 20:19 +0200, Wolfram Sang wrote: > > Due to clock stretch, our HW IP cannot meet the ac-timing > > spec(tSU;STA,tSU;STO). > > There isn't a same delay for clock stretching, so we need pass a > > parameter which can be found through measurement to meet most > > conditions. > > What about using this existing binding? > > - i2c-scl-internal-delay-ns > Number of nanoseconds the IP core additionally needs to setup SCL. > I can't see the relationship between "i2c-scl-falling-time-ns" and clock stretching, is there a parameter related to clock stretching? If you think both of them will affect the ac-timing of SCL, at this point, "i2c-scl-falling-time-ns" maybe a good choice.
On Mon, Apr 12, 2021 at 08:03:14PM +0800, Qii Wang wrote: > On Wed, 2021-04-07 at 20:19 +0200, Wolfram Sang wrote: > > > Due to clock stretch, our HW IP cannot meet the ac-timing > > > spec(tSU;STA,tSU;STO). > > > There isn't a same delay for clock stretching, so we need pass a > > > parameter which can be found through measurement to meet most > > > conditions. > > > > What about using this existing binding? > > > > - i2c-scl-internal-delay-ns > > Number of nanoseconds the IP core additionally needs to setup SCL. > > > > I can't see the relationship between "i2c-scl-falling-time-ns" and clock > stretching, is there a parameter related to clock stretching? ( you wrote "i2c-scl-falling-time-ns" above, didn't you mean "i2c-scl-internal-delay-ns" instead? ) Not yet, and I wonder if there can be one. In I2C (not SMBus), devices are allowed to stretch the clock as long as they want, so what should be specified here? I suggesteed "internal-delay" because AFAIU your hardware needs this delay to be able to cope with clock stretching. > If you think both of them will affect the ac-timing of SCL, at this > point, "i2c-scl-falling-time-ns" maybe a good choice. Do you mean "i2c-scl-falling-time-ns" or "i2c-scl-internal-delay-ns"?
On Tue, 2021-04-13 at 22:17 +0200, Wolfram Sang wrote: > On Mon, Apr 12, 2021 at 08:03:14PM +0800, Qii Wang wrote: > > I can't see the relationship between "i2c-scl-falling-time-ns" and clock > > stretching, is there a parameter related to clock stretching? > > ( you wrote "i2c-scl-falling-time-ns" above, didn't you mean > "i2c-scl-internal-delay-ns" instead? ) > I am sorry, I have confused your comment with lkjoon's comment in the last mail. what I actually want to say is "i2c-scl-internal-delay-ns". > Not yet, and I wonder if there can be one. In I2C (not SMBus), devices > are allowed to stretch the clock as long as they want, so what should be > specified here? > > I suggesteed "internal-delay" because AFAIU your hardware needs this > delay to be able to cope with clock stretching. > If there is not a maximum value for clock stretching, "i2c-scl-internal-delay-ns" should be a good choice for our hardware, although it maybe not for clock stretching. > > If you think both of them will affect the ac-timing of SCL, at this > > point, "i2c-scl-falling-time-ns" maybe a good choice. > > Do you mean "i2c-scl-falling-time-ns" or "i2c-scl-internal-delay-ns"? > "i2c-scl-internal-delay-ns" is better. Thanks for your review. Qii
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c index 2ffd2f3..47c7255 100644 --- a/drivers/i2c/busses/i2c-mt65xx.c +++ b/drivers/i2c/busses/i2c-mt65xx.c @@ -245,6 +245,7 @@ struct mtk_i2c { u16 irq_stat; /* interrupt status */ unsigned int clk_src_div; unsigned int speed_hz; /* The speed in transfer */ + unsigned int clock_stretch_ns; enum mtk_trans_op op; u16 timing_reg; u16 high_speed_reg; @@ -607,7 +608,8 @@ static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c, else clk_ns = sample_ns / 2; - su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns); + su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns + i2c->clock_stretch_ns, + clk_ns); if (su_sta_cnt > max_sta_cnt) return -1; @@ -1171,6 +1173,8 @@ static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c) if (i2c->clk_src_div == 0) return -EINVAL; + of_property_read_u32(np, "clock-stretch-ns", &i2c->clock_stretch_ns); + i2c->have_pmic = of_property_read_bool(np, "mediatek,have-pmic"); i2c->use_push_pull = of_property_read_bool(np, "mediatek,use-push-pull");