[PATCH 0/3] Ensure Low period of SCL is correct

[PATCH 0/3] Ensure Low period of SCL is correct

[Lucas Tanure]

The default duty cycle of 33% is less than the required
by the I2C specs for the LOW period of the SCL clock.

So, for 100Khz or less, use 50%H/50%L duty cycle, and
for the clock above 100Khz, use 40%H/60%L duty cycle.
That ensures the low period of SCL is always more than
the minimum required by the specs at any given frequency.

Lucas Tanure (3):
  i2c: meson: Use _SHIFT and _MASK for register definitions
  i2c: meson: Use 50% duty cycle for I2C clock
  i2c: meson: Remove meson_i2c_data

 drivers/i2c/busses/i2c-meson.c | 104 ++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 48 deletions(-)

-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] i2c: meson: Use _SHIFT and _MASK for register definitions

[Lucas Tanure]

Differentiate between masks and shifts

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 drivers/i2c/busses/i2c-meson.c | 41 ++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 07eb819072c4..4b4a5b2d77ab 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -30,18 +30,21 @@
 #define REG_TOK_RDATA1		0x1c
 
 /* Control register fields */
-#define REG_CTRL_START		BIT(0)
-#define REG_CTRL_ACK_IGNORE	BIT(1)
-#define REG_CTRL_STATUS		BIT(2)
-#define REG_CTRL_ERROR		BIT(3)
-#define REG_CTRL_CLKDIV		GENMASK(21, 12)
-#define REG_CTRL_CLKDIVEXT	GENMASK(29, 28)
-
-#define REG_SLV_ADDR		GENMASK(7, 0)
-#define REG_SLV_SDA_FILTER	GENMASK(10, 8)
-#define REG_SLV_SCL_FILTER	GENMASK(13, 11)
-#define REG_SLV_SCL_LOW		GENMASK(27, 16)
-#define REG_SLV_SCL_LOW_EN	BIT(28)
+#define REG_CTRL_START			BIT(0)
+#define REG_CTRL_ACK_IGNORE		BIT(1)
+#define REG_CTRL_STATUS			BIT(2)
+#define REG_CTRL_ERROR			BIT(3)
+#define REG_CTRL_CLKDIV_SHIFT		12
+#define REG_CTRL_CLKDIV_MASK		GENMASK(21, REG_CTRL_CLKDIV_SHIFT)
+#define REG_CTRL_CLKDIVEXT_SHIFT	28
+#define REG_CTRL_CLKDIVEXT_MASK		GENMASK(29, REG_CTRL_CLKDIVEXT_SHIFT)
+
+#define REG_SLV_ADDR_MASK		GENMASK(7, 0)
+#define REG_SLV_SDA_FILTER_MASK		GENMASK(10, 8)
+#define REG_SLV_SCL_FILTER_MASK		GENMASK(13, 11)
+#define REG_SLV_SCL_LOW_SHIFT		16
+#define REG_SLV_SCL_LOW_MASK		GENMASK(27, REG_SLV_SCL_LOW_SHIFT)
+#define REG_SLV_SCL_LOW_EN		BIT(28)
 
 #define I2C_TIMEOUT_MS		500
 #define FILTER_DELAY		15
@@ -149,11 +152,11 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
 		div = GENMASK(11, 0);
 	}
 
-	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV,
-			   FIELD_PREP(REG_CTRL_CLKDIV, div & GENMASK(9, 0)));
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
+			   FIELD_PREP(REG_CTRL_CLKDIV_MASK, div & GENMASK(9, 0)));
 
-	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT,
-			   FIELD_PREP(REG_CTRL_CLKDIVEXT, div >> 10));
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
+			   FIELD_PREP(REG_CTRL_CLKDIVEXT_MASK, div >> 10));
 
 	/* Disable HIGH/LOW mode */
 	meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0);
@@ -292,8 +295,8 @@ static void meson_i2c_do_start(struct meson_i2c *i2c, struct i2c_msg *msg)
 		TOKEN_SLAVE_ADDR_WRITE;
 
 
-	meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_ADDR,
-			   FIELD_PREP(REG_SLV_ADDR, msg->addr << 1));
+	meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_ADDR_MASK,
+			   FIELD_PREP(REG_SLV_ADDR_MASK, msg->addr << 1));
 
 	meson_i2c_add_token(i2c, TOKEN_START);
 	meson_i2c_add_token(i2c, token);
@@ -467,7 +470,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 
 	/* Disable filtering */
 	meson_i2c_set_mask(i2c, REG_SLAVE_ADDR,
-			   REG_SLV_SDA_FILTER | REG_SLV_SCL_FILTER, 0);
+			   REG_SLV_SDA_FILTER_MASK | REG_SLV_SCL_FILTER_MASK, 0);
 
 	meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
 
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] i2c: meson: Use 50% duty cycle for I2C clock

[Lucas Tanure]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] i2c: meson: Use 50% duty cycle for I2C clock

[Neil Armstrong]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] i2c: meson: Use 50% duty cycle for I2C clock

[Neil Armstrong]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] i2c: meson: Use 50% duty cycle for I2C clock

[Lucas Tanure]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] i2c: meson: Remove meson_i2c_data

[Lucas Tanure]

The div_factor is not used anymore and can be removed.
With 50% duty cycle the div factor should 4 for all
devices.

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 drivers/i2c/busses/i2c-meson.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index b913ba20f06e..c35fd4122bfc 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -65,10 +65,6 @@ enum {
 	STATE_WRITE,
 };
 
-struct meson_i2c_data {
-	unsigned char div_factor;
-};
-
 /**
  * struct meson_i2c - Meson I2C device private data
  *
@@ -514,22 +510,10 @@ static int meson_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct meson_i2c_data i2c_meson6_data = {
-	.div_factor = 4,
-};
-
-static const struct meson_i2c_data i2c_gxbb_data = {
-	.div_factor = 4,
-};
-
-static const struct meson_i2c_data i2c_axg_data = {
-	.div_factor = 3,
-};
-
 static const struct of_device_id meson_i2c_match[] = {
-	{ .compatible = "amlogic,meson6-i2c", .data = &i2c_meson6_data },
-	{ .compatible = "amlogic,meson-gxbb-i2c", .data = &i2c_gxbb_data },
-	{ .compatible = "amlogic,meson-axg-i2c", .data = &i2c_axg_data },
+	{ .compatible = "amlogic,meson6-i2c", },
+	{ .compatible = "amlogic,meson-gxbb-i2c", },
+	{ .compatible = "amlogic,meson-axg-i2c", },
 	{},
 };
 
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Kevin Hilman]

Hi Lucas,

Lucas Tanure <tanure@linux.com> writes:

> The default duty cycle of 33% is less than the required
> by the I2C specs for the LOW period of the SCL clock.
>
> So, for 100Khz or less, use 50%H/50%L duty cycle, and
> for the clock above 100Khz, use 40%H/60%L duty cycle.
> That ensures the low period of SCL is always more than
> the minimum required by the specs at any given frequency.

Thanks for the fixes!

This is going to affect all SoCs, so ould you please summarize how your
changes were tested, and on which SoCs & boards?

Thanks,

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Lucas Tanure]

On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <khilman@baylibre.com> wrote:
>
> Hi Lucas,
>
> Lucas Tanure <tanure@linux.com> writes:
>
> > The default duty cycle of 33% is less than the required
> > by the I2C specs for the LOW period of the SCL clock.
> >
> > So, for 100Khz or less, use 50%H/50%L duty cycle, and
> > for the clock above 100Khz, use 40%H/60%L duty cycle.
> > That ensures the low period of SCL is always more than
> > the minimum required by the specs at any given frequency.
>
> Thanks for the fixes!
>
> This is going to affect all SoCs, so ould you please summarize how your
> changes were tested, and on which SoCs & boards?
>
> Thanks,
>
> Kevin

Hi,

I only tested against the vim3 board, measured the bus with a Saleae
logic pro 16.
The measurements were with 100k, 400k, and a few in-between frequencies.

Is that enough?

Thanks
Lucas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Neil Armstrong]

Hi,

On 29/03/2022 00:31, Lucas Tanure wrote:
> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <khilman@baylibre.com> wrote:
>>
>> Hi Lucas,
>>
>> Lucas Tanure <tanure@linux.com> writes:
>>
>>> The default duty cycle of 33% is less than the required
>>> by the I2C specs for the LOW period of the SCL clock.
>>>
>>> So, for 100Khz or less, use 50%H/50%L duty cycle, and
>>> for the clock above 100Khz, use 40%H/60%L duty cycle.
>>> That ensures the low period of SCL is always more than
>>> the minimum required by the specs at any given frequency.
>>
>> Thanks for the fixes!
>>
>> This is going to affect all SoCs, so ould you please summarize how your
>> changes were tested, and on which SoCs & boards?
>>
>> Thanks,
>>
>> Kevin
> 
> Hi,
> 
> I only tested against the vim3 board, measured the bus with a Saleae
> logic pro 16.
> The measurements were with 100k, 400k, and a few in-between frequencies.

Thanks, it's a great addition to have !

> 
> Is that enough?

A test on GXL/GXM (VIM1 or VIM2) & GXBB (Odroid-C2) devices is lacking before we
can merge this.

If I find some time, I'll have a try, but everyone is welcome testing this serie
and report if it still works fine for them.

Vyacheslav, do you think you can test on your JetHub devices ? it would validate GXL & AXG.

Neil

> 
> Thanks
> Lucas


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Vyacheslav]

04.04.2022 11:01, Neil Armstrong wrote:
> Hi,
> 
> On 29/03/2022 00:31, Lucas Tanure wrote:
>> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <khilman@baylibre.com> wrote:
>>>
>>> Hi Lucas,
>>>
>>> Lucas Tanure <tanure@linux.com> writes:
>>>
>>>> The default duty cycle of 33% is less than the required
>>>> by the I2C specs for the LOW period of the SCL clock.
>>>>
>>>> So, for 100Khz or less, use 50%H/50%L duty cycle, and
>>>> for the clock above 100Khz, use 40%H/60%L duty cycle.
>>>> That ensures the low period of SCL is always more than
>>>> the minimum required by the specs at any given frequency.
>>>
>>> Thanks for the fixes!
>>>
>>> This is going to affect all SoCs, so ould you please summarize how your
>>> changes were tested, and on which SoCs & boards?
>>>
>>> Thanks,
>>>
>>> Kevin
>>
>> Hi,
>>
>> I only tested against the vim3 board, measured the bus with a Saleae
>> logic pro 16.
>> The measurements were with 100k, 400k, and a few in-between frequencies.
> 
> Thanks, it's a great addition to have !
> 
>>
>> Is that enough?
> 
> A test on GXL/GXM (VIM1 or VIM2) & GXBB (Odroid-C2) devices is lacking 
> before we
> can merge this.
> 
> If I find some time, I'll have a try, but everyone is welcome testing 
> this serie
> and report if it still works fine for them.
> 
> Vyacheslav, do you think you can test on your JetHub devices ? it would 
> validate GXL & AXG.

It builds ok on 5.17. JetHub H1/D1 has only rtc clock (pcf8563) and 
1-wire controller (ds2482) on i2c bus. I did't see any difference with 
or without patches. all works at first look.

Vyacheslav


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Neil Armstrong]

Hi,

On 28/03/2022 23:51, Lucas Tanure wrote:
> 
> 
> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <khilman@baylibre.com <mailto:khilman@baylibre.com>> wrote:
> 
>     Hi Lucas,
> 
>     Lucas Tanure <tanure@linux.com <mailto:tanure@linux.com>> writes:
> 
>      > The default duty cycle of 33% is less than the required
>      > by the I2C specs for the LOW period of the SCL clock.
>      >
>      > So, for 100Khz or less, use 50%H/50%L duty cycle, and
>      > for the clock above 100Khz, use 40%H/60%L duty cycle.
>      > That ensures the low period of SCL is always more than
>      > the minimum required by the specs at any given frequency.
> 
>     Thanks for the fixes!
> 
>     This is going to affect all SoCs, so ould you please summarize how your
>     changes were tested, and on which SoCs & boards?
> 
>     Thanks,
> 
>     Kevin
> 
> 
> Hi,
> 
> I only tested against vim3 board, measured the bus with an saleae logic pro 16.
> The measurements were with 100k, 400k and a few in between frequencies.
> 
> Is that enough?

I did a few measures on the Libre Computer Le Potato S905X board:

i2c_AO:

Before the patchset, I got:
- 100KHz: 1.66uS HIGH, 6.75uS LOW, 20%/80%, Freq 118KHz /!\
- 400KHz: Unable to decode, clock line is invalid, Data line is also invalid

With the patchset
- 100KHz: 4.25uS HIGH, 6.58uS LOW, 40%/60%, Freq 92KHz
- 400KHz: 0.33uS HIGH, 3.00uS LOW, 10%/90%, Freq 300KHz

i2c_B:

Before the patchset, I got:
- 100KHz: 2.25uS HIGH, 5.41uS LOW, 29%/71%, Freq 130KHz /!\
- 400KHz: 0.42uS HIGH, 1.66uS LOW, 20%/80%, Freq 480KHz /!\

With the patchset
- 100KHz: 4.75uS HIGH, 5.42uS LOW, 46%/54%, Freq 98KHz
- 400KHz: 0.66uS HIGH, 2.00uS LOW, 24%/75%, Freq 375KHz


So this fixes the frequency, before they were invalid.
And it fixes 400KHz on i2c_AO...

I do not understand why behavior is different between i2c_AO & i2c_B, they
are feed with the same clock so it should be the same.

Did you check on both i2c interfaces ? can you share your results ?

Neil

> 
> Thanks
> Lucas
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Ensure Low period of SCL is correct

[Lucas Tanure]

On Tue, Apr 5, 2022 at 4:11 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi,
>
> On 28/03/2022 23:51, Lucas Tanure wrote:
> >
> >
> > On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <khilman@baylibre.com <mailto:khilman@baylibre.com>> wrote:
> >
> >     Hi Lucas,
> >
> >     Lucas Tanure <tanure@linux.com <mailto:tanure@linux.com>> writes:
> >
> >      > The default duty cycle of 33% is less than the required
> >      > by the I2C specs for the LOW period of the SCL clock.
> >      >
> >      > So, for 100Khz or less, use 50%H/50%L duty cycle, and
> >      > for the clock above 100Khz, use 40%H/60%L duty cycle.
> >      > That ensures the low period of SCL is always more than
> >      > the minimum required by the specs at any given frequency.
> >
> >     Thanks for the fixes!
> >
> >     This is going to affect all SoCs, so ould you please summarize how your
> >     changes were tested, and on which SoCs & boards?
> >
> >     Thanks,
> >
> >     Kevin
> >
> >
> > Hi,
> >
> > I only tested against vim3 board, measured the bus with an saleae logic pro 16.
> > The measurements were with 100k, 400k and a few in between frequencies.
> >
> > Is that enough?
>
> I did a few measures on the Libre Computer Le Potato S905X board:
>
> i2c_AO:
>
> Before the patchset, I got:
> - 100KHz: 1.66uS HIGH, 6.75uS LOW, 20%/80%, Freq 118KHz /!\
> - 400KHz: Unable to decode, clock line is invalid, Data line is also invalid
>
> With the patchset
> - 100KHz: 4.25uS HIGH, 6.58uS LOW, 40%/60%, Freq 92KHz
> - 400KHz: 0.33uS HIGH, 3.00uS LOW, 10%/90%, Freq 300KHz
>
> i2c_B:
>
> Before the patchset, I got:
> - 100KHz: 2.25uS HIGH, 5.41uS LOW, 29%/71%, Freq 130KHz /!\
> - 400KHz: 0.42uS HIGH, 1.66uS LOW, 20%/80%, Freq 480KHz /!\
>
> With the patchset
> - 100KHz: 4.75uS HIGH, 5.42uS LOW, 46%/54%, Freq 98KHz
> - 400KHz: 0.66uS HIGH, 2.00uS LOW, 24%/75%, Freq 375KHz
>
>
> So this fixes the frequency, before they were invalid.
> And it fixes 400KHz on i2c_AO...
>
> I do not understand why behavior is different between i2c_AO & i2c_B, they
> are feed with the same clock so it should be the same.
>
> Did you check on both i2c interfaces ? can you share your results ?

I only checked I2C interfaces i2c3 and i2c_ao.
I will submit a new patch chain with more results.

>
> Neil
>
> >
> > Thanks
> > Lucas
> >
> >
> >
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel