diff mbox series

[RESEND] i2c: mediatek: Get device clock-stretch time via dts

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

Commit Message

Qii Wang March 13, 2021, 8:04 a.m. UTC
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>
---
 drivers/i2c/busses/i2c-mt65xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Wolfram Sang March 18, 2021, 11:23 a.m. UTC | #1
> 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.
Wolfram Sang April 6, 2021, 7:48 p.m. UTC | #2
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.
Qii Wang April 7, 2021, 12:15 p.m. UTC | #3
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.
Wolfram Sang April 7, 2021, 6:19 p.m. UTC | #4
> 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.
Qii Wang April 12, 2021, 12:03 p.m. UTC | #5
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.
Wolfram Sang April 13, 2021, 8:17 p.m. UTC | #6
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"?
Qii Wang April 14, 2021, 1:37 a.m. UTC | #7
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 mbox series

Patch

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");