| Message ID | 20210917101416.20760-7-kewei.xu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introducing an attribute to select the time setting | expand |
On Fri, Sep 17, 2021 at 06:14:15PM +0800, Kewei Xu wrote: > In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust > support"), the I2C timing calculation has been revised to support > ac-timing adjustment, however that will break on some I2C components. > As a result we want to introduce a new setting "default-adjust-timing" > so those components can choose to use the old (default) timing algorithm. Why can't the new calculation be updated in a way that it works for all I2C components?
On Sat, 2021-10-02 at 08:40 +0200, Wolfram Sang wrote: > On Fri, Sep 17, 2021 at 06:14:15PM +0800, Kewei Xu wrote: > > In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing > > adjust > > support"), the I2C timing calculation has been revised to support > > ac-timing adjustment, however that will break on some I2C > > components. > > As a result we want to introduce a new setting "default-adjust- > > timing" > > so those components can choose to use the old (default) timing > > algorithm. > > Why can't the new calculation be updated in a way that it works for > all > I2C components? > Hi, In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), the I2C timing calculation has been revised to support ac-timing adjustment.But in our design, it will make tSU,STA/tHD,STA/tSU,STO shorter when the slave device have clock- stretching feature. Then we upload the commit a80f24945fcf ("i2c: mediatek: Use scl_int_delay_ns to compensate clock-stretching") to support adjusting tSU,STA/tHD,STA/tSU,STO when the slave device clock- stretching. But if the slave device stretch the SCL line for too long time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec. However in the old (default) timing algorithm before the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new setting "default-adjust-timing" for using the old (default) timing algorithm. Thanks~
Hi, > stretching. But if the slave device stretch the SCL line for too long > time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec. Isn't the new algorithm broken if it cannot support clock stretching? What was the problem of the old algorithm not meeting the spec? > However in the old (default) timing algorithm before the commit > be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), > tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new > setting "default-adjust-timing" for using the old (default) timing > algorithm." What I still do not get: the old algorithm was able to handle clock stretching. Why can't you update the new one to handle clock stretching as well. I might be missing something, but what is it? Happy hacking, Wolfram
> > stretching. But if the slave device stretch the SCL line for too long > > time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec. > > Isn't the new algorithm broken if it cannot support clock stretching? > What was the problem of the old algorithm not meeting the spec? > > > However in the old (default) timing algorithm before the commit > > be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), > > tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new > > setting "default-adjust-timing" for using the old (default) timing > > algorithm." > > What I still do not get: the old algorithm was able to handle clock > stretching. Why can't you update the new one to handle clock stretching > as well. I might be missing something, but what is it? I am still interested. Especially in the last question. Is the last question clear to you? I can explain some more otherwise.
On Mon, 2021-11-29 at 13:49 +0100, Wolfram Sang wrote: > > > stretching. But if the slave device stretch the SCL line for too > > > long > > > time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet > > > spec. > > > > Isn't the new algorithm broken if it cannot support clock > > stretching? > > What was the problem of the old algorithm not meeting the spec? > > > > > However in the old (default) timing algorithm before the commit > > > be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), > > > tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new > > > setting "default-adjust-timing" for using the old (default) > > > timing > > > algorithm." > > > > What I still do not get: the old algorithm was able to handle clock > > stretching. Why can't you update the new one to handle clock > > stretching > > as well. I might be missing something, but what is it? > > I am still interested. Especially in the last question. Is the last > question clear to you? I can explain some more otherwise. > Hi,Wolfram, I'm very sorry that I didn't reply to your information in time due to my many personal affairs. The Old algorithm was designed to focus only on normal functions, and need to add additional custom code to adjust ac-timing when the communication timing did not meet the specifications. so when there is no clock stretch, ac-timing does not meet the spec, but the function is always normal. The new algorithm(The commit patch: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support") is based on the requirements of i2c spec to calculate the hardware-related settings so that the function and ac- timing are normal When there is no clock stretch or the clock stretch time is short. When the stretching time is very long (>60us), i2c ac- timing does not meet the specifications and causes function abnormal. In order to make the i2c function normal, this patch was submitted, that is, when the stretch is long, the old algorithm is used to ensure the function is normal, and when the stretch is short, the new algorithm is used to ensure that the ac-timing and function are normal. We found that when the ac-timing calculation formula is updated, the new algorithm can make i2c ac-timing meet the spec and function normally. So we plan to replace this patch with a patch that updates the calculation formula. Thanks~ Kewei
Hi Kewei! > I'm very sorry that I didn't reply to your information in time due > to my many personal affairs. No worries, there is no pressure from my side. I hope you are all well now! > We found that when the ac-timing calculation formula is updated, the > new algorithm can make i2c ac-timing meet the spec and function > normally. So we plan to replace this patch with a patch that updates > the calculation formula. Cool! I'm glad this is possible. Looking forward to the new patch, Wolfram
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c index 0c611f7bf384..6a07adaa6422 100644 --- a/drivers/i2c/busses/i2c-mt65xx.c +++ b/drivers/i2c/busses/i2c-mt65xx.c @@ -66,6 +66,7 @@ #define I2C_DMA_HARD_RST 0x0002 #define I2C_DMA_HANDSHAKE_RST 0x0004 +#define I2C_DEFAULT_CLK_DIV 5 #define MAX_SAMPLE_CNT_DIV 8 #define MAX_STEP_CNT_DIV 64 #define MAX_CLOCK_DIV 256 @@ -250,6 +251,7 @@ struct mtk_i2c { struct clk *clk_arb; /* Arbitrator clock for i2c */ bool have_pmic; /* can use i2c pins from PMIC */ bool use_push_pull; /* IO config push-pull mode */ + bool use_default_timing; /* no timing adjust mode */ u16 irq_stat; /* interrupt status */ unsigned int clk_src_div; @@ -532,7 +534,11 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c) else ext_conf_val = I2C_FS_START_CON; - if (i2c->dev_comp->timing_adjust) { + if (i2c->use_default_timing) { + if (i2c->dev_comp->timing_adjust) + mtk_i2c_writew(i2c, I2C_DEFAULT_CLK_DIV - 1, + OFFSET_CLOCK_DIV); + } else if (i2c->dev_comp->timing_adjust) { ext_conf_val = i2c->ac_timing.ext; mtk_i2c_writew(i2c, i2c->ac_timing.inter_clk_div, OFFSET_CLOCK_DIV); @@ -615,7 +621,7 @@ static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c, unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1), clk_src); - if (!i2c->dev_comp->timing_adjust) + if (i2c->use_default_timing || !i2c->dev_comp->timing_adjust) return 0; if (i2c->dev_comp->ltiming_adjust) @@ -775,7 +781,65 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src, return 0; } -static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk) +static int mtk_i2c_set_speed_default_timing(struct mtk_i2c *i2c, + unsigned int parent_clk) +{ + unsigned int clk_src; + unsigned int step_cnt; + unsigned int sample_cnt; + unsigned int l_step_cnt; + unsigned int l_sample_cnt; + unsigned int target_speed; + int ret; + + if (i2c->dev_comp->timing_adjust) + i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV; + + clk_src = parent_clk / i2c->clk_src_div; + target_speed = i2c->speed_hz; + + if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ) { + /* Set master code speed register */ + ret = mtk_i2c_calculate_speed(i2c, clk_src, + I2C_MAX_FAST_MODE_FREQ, + &l_step_cnt, &l_sample_cnt); + if (ret < 0) + return ret; + + i2c->timing_reg = (l_sample_cnt << 8) | l_step_cnt; + + /* Set the high speed mode register */ + ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed, + &step_cnt, &sample_cnt); + if (ret < 0) + return ret; + + i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE | + (sample_cnt << 12) | (step_cnt << 8); + + if (i2c->dev_comp->ltiming_adjust) + i2c->ltiming_reg = (l_sample_cnt << 6) | l_step_cnt | + (sample_cnt << 12) | (step_cnt << 9); + } else { + ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed, + &step_cnt, &sample_cnt); + if (ret < 0) + return ret; + + i2c->timing_reg = (sample_cnt << 8) | step_cnt; + + /* Disable the high speed transaction */ + i2c->high_speed_reg = I2C_TIME_CLR_VALUE; + + if (i2c->dev_comp->ltiming_adjust) + i2c->ltiming_reg = (sample_cnt << 6) | step_cnt; + } + + return 0; +} + +static int mtk_i2c_set_speed_adjust_timing(struct mtk_i2c *i2c, + unsigned int parent_clk) { unsigned int clk_src; unsigned int step_cnt; @@ -1271,6 +1335,8 @@ static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c) i2c->have_pmic = of_property_read_bool(np, "mediatek,have-pmic"); i2c->use_push_pull = of_property_read_bool(np, "mediatek,use-push-pull"); + i2c->use_default_timing = + of_property_read_bool(np, "mediatek,use-default-timing"); i2c_parse_fw_timings(i2c->dev, &i2c->timing_info, true); @@ -1357,7 +1423,10 @@ static int mtk_i2c_probe(struct platform_device *pdev) strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name)); - ret = mtk_i2c_set_speed(i2c, clk_get_rate(clk)); + if (i2c->use_default_timing) + ret = mtk_i2c_set_speed_default_timing(i2c, clk_get_rate(clk)); + else + ret = mtk_i2c_set_speed_adjust_timing(i2c, clk_get_rate(clk)); if (ret) { dev_err(&pdev->dev, "Failed to set the speed.\n"); return -EINVAL;
In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"), the I2C timing calculation has been revised to support ac-timing adjustment, however that will break on some I2C components. As a result we want to introduce a new setting "default-adjust-timing" so those components can choose to use the old (default) timing algorithm. Fixes: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support") Signed-off-by: Kewei Xu <kewei.xu@mediatek.com> Reviewed-by: Qii Wang <qii.wang@mediatek.com> --- drivers/i2c/busses/i2c-mt65xx.c | 77 +++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-)