| Message ID | 20220326102229.421718-3-tanure@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ensure Low period of SCL is correct | expand |
On 26/03/2022 11:22, Lucas Tanure wrote: > The duty cycle of 33% is less than the required > by the I2C specs for the LOW period of the SCL > clock. > > Move the duty cyle to 50% for 100Khz or lower > clocks, and (40% High SCL / 60% Low SCL) duty > cycle for clocks above 100Khz. > > Signed-off-by: Lucas Tanure <tanure@linux.com> > --- > drivers/i2c/busses/i2c-meson.c | 45 +++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c > index 4b4a5b2d77ab..b913ba20f06e 100644 > --- a/drivers/i2c/busses/i2c-meson.c > +++ b/drivers/i2c/busses/i2c-meson.c > @@ -140,29 +140,50 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token) > static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq) > { > unsigned long clk_rate = clk_get_rate(i2c->clk); > - unsigned int div; > + unsigned int div_h, div_l; > > - div = DIV_ROUND_UP(clk_rate, freq); > - div -= FILTER_DELAY; > - div = DIV_ROUND_UP(div, i2c->data->div_factor); > + if (freq <= 100000) { > + div_h = DIV_ROUND_UP(clk_rate, freq); > + div_l = DIV_ROUND_UP(div_h, 4); > + div_h = DIV_ROUND_UP(div_h, 2) - FILTER_DELAY; > + } else { > + /* According to I2C-BUS Spec 2.1, in FAST-MODE, the minimum LOW period is 1.3uS, and > + * minimum HIGH is least 0.6us. > + * For 400000 freq, the period is 2.5us. To keep within the specs, give 40% of period to > + * HIGH and 60% to LOW. This means HIGH at 1.0us and LOW 1.5us. > + * The same applies for Fast-mode plus, where LOW is 0.5us and HIGH is 0.26us. > + * Duty = H/(H + L) = 2/5 > + */ > + div_h = DIV_ROUND_UP(clk_rate * 2, freq * 5) - FILTER_DELAY; > + div_l = DIV_ROUND_UP(clk_rate * 3, freq * 5 * 2); > + } > > /* clock divider has 12 bits */ > - if (div > GENMASK(11, 0)) { > + if (div_h > GENMASK(11, 0)) { > dev_err(i2c->dev, "requested bus frequency too low\n"); > - div = GENMASK(11, 0); > + div_h = GENMASK(11, 0); > + } > + if (div_l > GENMASK(11, 0)) { > + dev_err(i2c->dev, "requested bus frequency too low\n"); > + div_l = GENMASK(11, 0); > } > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK, > - FIELD_PREP(REG_CTRL_CLKDIV_MASK, div & GENMASK(9, 0))); > + FIELD_PREP(REG_CTRL_CLKDIV_MASK, div_h & GENMASK(9, 0))); > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK, > - FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div >> 10)); > + FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div_h >> 10)); > + > + > + /* set SCL low delay */ > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_MASK, > + (div_l << REG_SLV_SCL_LOW_SHIFT) & REG_SLV_SCL_LOW_MASK); > > - /* Disable HIGH/LOW mode */ > - meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0); > + /* Enable HIGH/LOW mode */ > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, REG_SLV_SCL_LOW_EN); While reading registers documentation, it's written: ``` SCL Low delay. This is a new feature in M8baby. In the previous M8baby design, the SCL low time was controlled by bits[21:12] of the register above. In this design, the SCL delay is controlled independently by these bits. ``` Could you keep the previous calculation for Meson6, with DIV_FACTOR fixed to 4, and use the new calculation only for GXBB & AXG ? To ease this, replace the div_factor with a clock calculation function pointer in meson_i2c_data, and pass the old one for Meson6 and the new one for GXBB & AXG. This only slightly changes patch 2 and changes patch 3 from a removal to a variable replace. > > - dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__, > - clk_rate, freq, div); > + dev_dbg(i2c->dev, "%s: clk %lu, freq %u, divh %u, divl %u\n", __func__, > + clk_rate, freq, div_h, div_l); > } > > static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len) Thanks, Neil
Hi, On 26/03/2022 11:22, Lucas Tanure wrote: > The duty cycle of 33% is less than the required > by the I2C specs for the LOW period of the SCL > clock. > > Move the duty cyle to 50% for 100Khz or lower > clocks, and (40% High SCL / 60% Low SCL) duty > cycle for clocks above 100Khz. > > Signed-off-by: Lucas Tanure <tanure@linux.com> > --- > drivers/i2c/busses/i2c-meson.c | 45 +++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c > index 4b4a5b2d77ab..b913ba20f06e 100644 > --- a/drivers/i2c/busses/i2c-meson.c > +++ b/drivers/i2c/busses/i2c-meson.c > @@ -140,29 +140,50 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token) > static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq) > { > unsigned long clk_rate = clk_get_rate(i2c->clk); > - unsigned int div; > + unsigned int div_h, div_l; > > - div = DIV_ROUND_UP(clk_rate, freq); > - div -= FILTER_DELAY; > - div = DIV_ROUND_UP(div, i2c->data->div_factor); > + if (freq <= 100000) { You should use I2C_MAX_STANDARD_MODE_FREQ instead here > + div_h = DIV_ROUND_UP(clk_rate, freq); > + div_l = DIV_ROUND_UP(div_h, 4); > + div_h = DIV_ROUND_UP(div_h, 2) - FILTER_DELAY; > + } else { > + /* According to I2C-BUS Spec 2.1, in FAST-MODE, the minimum LOW period is 1.3uS, and > + * minimum HIGH is least 0.6us. > + * For 400000 freq, the period is 2.5us. To keep within the specs, give 40% of period to > + * HIGH and 60% to LOW. This means HIGH at 1.0us and LOW 1.5us. > + * The same applies for Fast-mode plus, where LOW is 0.5us and HIGH is 0.26us. > + * Duty = H/(H + L) = 2/5 > + */ Please move the comment before the if() > + div_h = DIV_ROUND_UP(clk_rate * 2, freq * 5) - FILTER_DELAY; > + div_l = DIV_ROUND_UP(clk_rate * 3, freq * 5 * 2); > + } > > /* clock divider has 12 bits */ > - if (div > GENMASK(11, 0)) { > + if (div_h > GENMASK(11, 0)) { > dev_err(i2c->dev, "requested bus frequency too low\n"); > - div = GENMASK(11, 0); > + div_h = GENMASK(11, 0); > + } > + if (div_l > GENMASK(11, 0)) { > + dev_err(i2c->dev, "requested bus frequency too low\n"); > + div_l = GENMASK(11, 0); > } > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK, > - FIELD_PREP(REG_CTRL_CLKDIV_MASK, div & GENMASK(9, 0))); > + FIELD_PREP(REG_CTRL_CLKDIV_MASK, div_h & GENMASK(9, 0))); > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK, > - FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div >> 10)); > + FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div_h >> 10)); > + > + > + /* set SCL low delay */ > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_MASK, > + (div_l << REG_SLV_SCL_LOW_SHIFT) & REG_SLV_SCL_LOW_MASK); You could use FIELD_PREP() here > > - /* Disable HIGH/LOW mode */ > - meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0); > + /* Enable HIGH/LOW mode */ > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, REG_SLV_SCL_LOW_EN); > > - dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__, > - clk_rate, freq, div); > + dev_dbg(i2c->dev, "%s: clk %lu, freq %u, divh %u, divl %u\n", __func__, > + clk_rate, freq, div_h, div_l); > } > > static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len) I looked at different amlogic downstream sources, and those match the recommended calculations. So with the legacy back for Meson6, it will be OK. Neil
On Wed, Apr 6, 2022 at 12:31 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi, > > On 26/03/2022 11:22, Lucas Tanure wrote: > > The duty cycle of 33% is less than the required > > by the I2C specs for the LOW period of the SCL > > clock. > > > > Move the duty cyle to 50% for 100Khz or lower > > clocks, and (40% High SCL / 60% Low SCL) duty > > cycle for clocks above 100Khz. > > > > Signed-off-by: Lucas Tanure <tanure@linux.com> > > --- > > drivers/i2c/busses/i2c-meson.c | 45 +++++++++++++++++++++++++--------- > > 1 file changed, 33 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c > > index 4b4a5b2d77ab..b913ba20f06e 100644 > > --- a/drivers/i2c/busses/i2c-meson.c > > +++ b/drivers/i2c/busses/i2c-meson.c > > @@ -140,29 +140,50 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token) > > static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq) > > { > > unsigned long clk_rate = clk_get_rate(i2c->clk); > > - unsigned int div; > > + unsigned int div_h, div_l; > > > > - div = DIV_ROUND_UP(clk_rate, freq); > > - div -= FILTER_DELAY; > > - div = DIV_ROUND_UP(div, i2c->data->div_factor); > > + if (freq <= 100000) { > > You should use I2C_MAX_STANDARD_MODE_FREQ instead here > > > + div_h = DIV_ROUND_UP(clk_rate, freq); > > + div_l = DIV_ROUND_UP(div_h, 4); > > + div_h = DIV_ROUND_UP(div_h, 2) - FILTER_DELAY; > > + } else { > > + /* According to I2C-BUS Spec 2.1, in FAST-MODE, the minimum LOW period is 1.3uS, and > > + * minimum HIGH is least 0.6us. > > + * For 400000 freq, the period is 2.5us. To keep within the specs, give 40% of period to > > + * HIGH and 60% to LOW. This means HIGH at 1.0us and LOW 1.5us. > > + * The same applies for Fast-mode plus, where LOW is 0.5us and HIGH is 0.26us. > > + * Duty = H/(H + L) = 2/5 > > + */ > > Please move the comment before the if() > > > + div_h = DIV_ROUND_UP(clk_rate * 2, freq * 5) - FILTER_DELAY; > > + div_l = DIV_ROUND_UP(clk_rate * 3, freq * 5 * 2); > > + } > > > > /* clock divider has 12 bits */ > > - if (div > GENMASK(11, 0)) { > > + if (div_h > GENMASK(11, 0)) { > > dev_err(i2c->dev, "requested bus frequency too low\n"); > > - div = GENMASK(11, 0); > > + div_h = GENMASK(11, 0); > > + } > > + if (div_l > GENMASK(11, 0)) { > > + dev_err(i2c->dev, "requested bus frequency too low\n"); > > + div_l = GENMASK(11, 0); > > } > > > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK, > > - FIELD_PREP(REG_CTRL_CLKDIV_MASK, div & GENMASK(9, 0))); > > + FIELD_PREP(REG_CTRL_CLKDIV_MASK, div_h & GENMASK(9, 0))); > > > > meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK, > > - FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div >> 10)); > > + FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div_h >> 10)); > > + > > + > > + /* set SCL low delay */ > > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_MASK, > > + (div_l << REG_SLV_SCL_LOW_SHIFT) & REG_SLV_SCL_LOW_MASK); > > You could use FIELD_PREP() here > > > > > - /* Disable HIGH/LOW mode */ > > - meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0); > > + /* Enable HIGH/LOW mode */ > > + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, REG_SLV_SCL_LOW_EN); > > > > - dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__, > > - clk_rate, freq, div); > > + dev_dbg(i2c->dev, "%s: clk %lu, freq %u, divh %u, divl %u\n", __func__, > > + clk_rate, freq, div_h, div_l); > > } > > > > static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len) > > I looked at different amlogic downstream sources, and those match the recommended > calculations. > > So with the legacy back for Meson6, it will be OK. > > Neil Ok, I will do the modifications this weekend. I only tested i2c3 and i2c_ao and will execute more measures and show the results. Thanks Lucas
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c index 4b4a5b2d77ab..b913ba20f06e 100644 --- a/drivers/i2c/busses/i2c-meson.c +++ b/drivers/i2c/busses/i2c-meson.c @@ -140,29 +140,50 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token) static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq) { unsigned long clk_rate = clk_get_rate(i2c->clk); - unsigned int div; + unsigned int div_h, div_l; - div = DIV_ROUND_UP(clk_rate, freq); - div -= FILTER_DELAY; - div = DIV_ROUND_UP(div, i2c->data->div_factor); + if (freq <= 100000) { + div_h = DIV_ROUND_UP(clk_rate, freq); + div_l = DIV_ROUND_UP(div_h, 4); + div_h = DIV_ROUND_UP(div_h, 2) - FILTER_DELAY; + } else { + /* According to I2C-BUS Spec 2.1, in FAST-MODE, the minimum LOW period is 1.3uS, and + * minimum HIGH is least 0.6us. + * For 400000 freq, the period is 2.5us. To keep within the specs, give 40% of period to + * HIGH and 60% to LOW. This means HIGH at 1.0us and LOW 1.5us. + * The same applies for Fast-mode plus, where LOW is 0.5us and HIGH is 0.26us. + * Duty = H/(H + L) = 2/5 + */ + div_h = DIV_ROUND_UP(clk_rate * 2, freq * 5) - FILTER_DELAY; + div_l = DIV_ROUND_UP(clk_rate * 3, freq * 5 * 2); + } /* clock divider has 12 bits */ - if (div > GENMASK(11, 0)) { + if (div_h > GENMASK(11, 0)) { dev_err(i2c->dev, "requested bus frequency too low\n"); - div = GENMASK(11, 0); + div_h = GENMASK(11, 0); + } + if (div_l > GENMASK(11, 0)) { + dev_err(i2c->dev, "requested bus frequency too low\n"); + div_l = GENMASK(11, 0); } meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK, - FIELD_PREP(REG_CTRL_CLKDIV_MASK, div & GENMASK(9, 0))); + FIELD_PREP(REG_CTRL_CLKDIV_MASK, div_h & GENMASK(9, 0))); meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK, - FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div >> 10)); + FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div_h >> 10)); + + + /* set SCL low delay */ + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_MASK, + (div_l << REG_SLV_SCL_LOW_SHIFT) & REG_SLV_SCL_LOW_MASK); - /* Disable HIGH/LOW mode */ - meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0); + /* Enable HIGH/LOW mode */ + meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, REG_SLV_SCL_LOW_EN); - dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__, - clk_rate, freq, div); + dev_dbg(i2c->dev, "%s: clk %lu, freq %u, divh %u, divl %u\n", __func__, + clk_rate, freq, div_h, div_l); } static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)
The duty cycle of 33% is less than the required by the I2C specs for the LOW period of the SCL clock. Move the duty cyle to 50% for 100Khz or lower clocks, and (40% High SCL / 60% Low SCL) duty cycle for clocks above 100Khz. Signed-off-by: Lucas Tanure <tanure@linux.com> --- drivers/i2c/busses/i2c-meson.c | 45 +++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-)