* [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions
2022-04-09 16:43 [PATCH v2 0/2] Ensure High and Low periods of SCL are correct Lucas Tanure
@ 2022-04-09 16:43 ` Lucas Tanure
2022-04-11 7:27 ` Neil Armstrong
2022-04-15 21:45 ` Wolfram Sang
2022-04-09 16:43 ` [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock Lucas Tanure
2022-04-11 7:30 ` [PATCH v2 0/2] Ensure High and Low periods of SCL are correct Neil Armstrong
2 siblings, 2 replies; 8+ messages in thread
From: Lucas Tanure @ 2022-04-09 16:43 UTC (permalink / raw)
To: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions
2022-04-09 16:43 ` [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions Lucas Tanure
@ 2022-04-11 7:27 ` Neil Armstrong
2022-04-15 21:45 ` Wolfram Sang
1 sibling, 0 replies; 8+ messages in thread
From: Neil Armstrong @ 2022-04-11 7:27 UTC (permalink / raw)
To: Lucas Tanure, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel
On 09/04/2022 18:43, Lucas Tanure wrote:
> 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);
>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions
2022-04-09 16:43 ` [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions Lucas Tanure
2022-04-11 7:27 ` Neil Armstrong
@ 2022-04-15 21:45 ` Wolfram Sang
1 sibling, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2022-04-15 21:45 UTC (permalink / raw)
To: Lucas Tanure
Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 193 bytes --]
On Sat, Apr 09, 2022 at 05:43:33PM +0100, Lucas Tanure wrote:
> Differentiate between masks and shifts
>
> Signed-off-by: Lucas Tanure <tanure@linux.com>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock
2022-04-09 16:43 [PATCH v2 0/2] Ensure High and Low periods of SCL are correct Lucas Tanure
2022-04-09 16:43 ` [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions Lucas Tanure
@ 2022-04-09 16:43 ` Lucas Tanure
2022-04-11 7:29 ` Neil Armstrong
2022-04-15 21:45 ` Wolfram Sang
2022-04-11 7:30 ` [PATCH v2 0/2] Ensure High and Low periods of SCL are correct Neil Armstrong
2 siblings, 2 replies; 8+ messages in thread
From: Lucas Tanure @ 2022-04-09 16:43 UTC (permalink / raw)
To: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel, 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 | 70 +++++++++++++++++++++++++++++-----
1 file changed, 60 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 4b4a5b2d77ab..50dab123380a 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
*
@@ -109,6 +105,10 @@ struct meson_i2c {
const struct meson_i2c_data *data;
};
+struct meson_i2c_data {
+ void (*set_clk_div)(struct meson_i2c *i2c, unsigned int freq);
+};
+
static void meson_i2c_set_mask(struct meson_i2c *i2c, int reg, u32 mask,
u32 val)
{
@@ -137,14 +137,62 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token)
i2c->num_tokens++;
}
-static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
+static void meson_gxbb_axg_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
+{
+ unsigned long clk_rate = clk_get_rate(i2c->clk);
+ unsigned int div_h, div_l;
+
+ /* 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
+ */
+ if (freq <= I2C_MAX_STANDARD_MODE_FREQ) {
+ 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 {
+ 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_h > GENMASK(11, 0)) {
+ dev_err(i2c->dev, "requested bus frequency too low\n");
+ 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_h & GENMASK(9, 0)));
+
+ meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
+ 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,
+ FIELD_PREP(REG_SLV_SCL_LOW_MASK, div_l));
+
+ /* 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, divh %u, divl %u\n", __func__,
+ clk_rate, freq, div_h, div_l);
+}
+
+static void meson6_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
{
unsigned long clk_rate = clk_get_rate(i2c->clk);
unsigned int div;
div = DIV_ROUND_UP(clk_rate, freq);
div -= FILTER_DELAY;
- div = DIV_ROUND_UP(div, i2c->data->div_factor);
+ div = DIV_ROUND_UP(div, 4);
/* clock divider has 12 bits */
if (div > GENMASK(11, 0)) {
@@ -472,7 +520,9 @@ static int meson_i2c_probe(struct platform_device *pdev)
meson_i2c_set_mask(i2c, REG_SLAVE_ADDR,
REG_SLV_SDA_FILTER_MASK | REG_SLV_SCL_FILTER_MASK, 0);
- meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
+ if (!i2c->data->set_clk_div)
+ return -EINVAL;
+ i2c->data->set_clk_div(i2c, timings.bus_freq_hz);
ret = i2c_add_adapter(&i2c->adap);
if (ret < 0) {
@@ -494,15 +544,15 @@ static int meson_i2c_remove(struct platform_device *pdev)
}
static const struct meson_i2c_data i2c_meson6_data = {
- .div_factor = 4,
+ .set_clk_div = meson6_i2c_set_clk_div,
};
static const struct meson_i2c_data i2c_gxbb_data = {
- .div_factor = 4,
+ .set_clk_div = meson_gxbb_axg_i2c_set_clk_div,
};
static const struct meson_i2c_data i2c_axg_data = {
- .div_factor = 3,
+ .set_clk_div = meson_gxbb_axg_i2c_set_clk_div,
};
static const struct of_device_id meson_i2c_match[] = {
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock
2022-04-09 16:43 ` [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock Lucas Tanure
@ 2022-04-11 7:29 ` Neil Armstrong
2022-04-15 21:45 ` Wolfram Sang
1 sibling, 0 replies; 8+ messages in thread
From: Neil Armstrong @ 2022-04-11 7:29 UTC (permalink / raw)
To: Lucas Tanure, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel
On 09/04/2022 18:43, 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 | 70 +++++++++++++++++++++++++++++-----
> 1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 4b4a5b2d77ab..50dab123380a 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
> *
> @@ -109,6 +105,10 @@ struct meson_i2c {
> const struct meson_i2c_data *data;
> };
>
> +struct meson_i2c_data {
> + void (*set_clk_div)(struct meson_i2c *i2c, unsigned int freq);
> +};
> +
> static void meson_i2c_set_mask(struct meson_i2c *i2c, int reg, u32 mask,
> u32 val)
> {
> @@ -137,14 +137,62 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token)
> i2c->num_tokens++;
> }
>
> -static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
> +static void meson_gxbb_axg_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
> +{
> + unsigned long clk_rate = clk_get_rate(i2c->clk);
> + unsigned int div_h, div_l;
> +
> + /* 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
> + */
> + if (freq <= I2C_MAX_STANDARD_MODE_FREQ) {
> + 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 {
> + 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_h > GENMASK(11, 0)) {
> + dev_err(i2c->dev, "requested bus frequency too low\n");
> + 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_h & GENMASK(9, 0)));
> +
> + meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
> + 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,
> + FIELD_PREP(REG_SLV_SCL_LOW_MASK, div_l));
> +
> + /* 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, divh %u, divl %u\n", __func__,
> + clk_rate, freq, div_h, div_l);
> +}
> +
> +static void meson6_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
> {
> unsigned long clk_rate = clk_get_rate(i2c->clk);
> unsigned int div;
>
> div = DIV_ROUND_UP(clk_rate, freq);
> div -= FILTER_DELAY;
> - div = DIV_ROUND_UP(div, i2c->data->div_factor);
> + div = DIV_ROUND_UP(div, 4);
>
> /* clock divider has 12 bits */
> if (div > GENMASK(11, 0)) {
> @@ -472,7 +520,9 @@ static int meson_i2c_probe(struct platform_device *pdev)
> meson_i2c_set_mask(i2c, REG_SLAVE_ADDR,
> REG_SLV_SDA_FILTER_MASK | REG_SLV_SCL_FILTER_MASK, 0);
>
> - meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
> + if (!i2c->data->set_clk_div)
> + return -EINVAL;
> + i2c->data->set_clk_div(i2c, timings.bus_freq_hz);
>
> ret = i2c_add_adapter(&i2c->adap);
> if (ret < 0) {
> @@ -494,15 +544,15 @@ static int meson_i2c_remove(struct platform_device *pdev)
> }
>
> static const struct meson_i2c_data i2c_meson6_data = {
> - .div_factor = 4,
> + .set_clk_div = meson6_i2c_set_clk_div,
> };
>
> static const struct meson_i2c_data i2c_gxbb_data = {
> - .div_factor = 4,
> + .set_clk_div = meson_gxbb_axg_i2c_set_clk_div,
> };
>
> static const struct meson_i2c_data i2c_axg_data = {
> - .div_factor = 3,
> + .set_clk_div = meson_gxbb_axg_i2c_set_clk_div,
> };
>
> static const struct of_device_id meson_i2c_match[] = {
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock
2022-04-09 16:43 ` [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock Lucas Tanure
2022-04-11 7:29 ` Neil Armstrong
@ 2022-04-15 21:45 ` Wolfram Sang
1 sibling, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2022-04-15 21:45 UTC (permalink / raw)
To: Lucas Tanure
Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 398 bytes --]
On Sat, Apr 09, 2022 at 05:43:34PM +0100, 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>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/2] Ensure High and Low periods of SCL are correct
2022-04-09 16:43 [PATCH v2 0/2] Ensure High and Low periods of SCL are correct Lucas Tanure
2022-04-09 16:43 ` [PATCH v2 1/2] i2c: meson: Use _SHIFT and _MASK for register definitions Lucas Tanure
2022-04-09 16:43 ` [PATCH v2 2/2] i2c: meson: Use 50% duty cycle for I2C clock Lucas Tanure
@ 2022-04-11 7:30 ` Neil Armstrong
2 siblings, 0 replies; 8+ messages in thread
From: Neil Armstrong @ 2022-04-11 7:30 UTC (permalink / raw)
To: Lucas Tanure, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-i2c, linux-arm-kernel, linux-amlogic, linux-kernel
Hi,
On 09/04/2022 18:43, Lucas Tanure wrote:
> The default duty cycle of 33% is less than the required
> by the I2C specs for the LOW period of the SCL clock at
> 100KHz bus speed.
>
> 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.
>
> I did a few measures on the Khadas Vim3 board:
>
> i2c_AO (i2c@5000):
>
> Before the patchset, I got:
> - 100KHz: 3.338us HIGH, 6.746us LOW, 33%/67%, Freq 99KHz (Not Valid tHIGH < 4.0us)
> - 400KHz: 860ns HIGH, 1.734us LOW, 33.15%/62.85%, Freq 385.505KHz (Valid)
> - 1000KHz: 362ns HIGH, 732ns LOW, 33.09%/66.91%, Freq 914.077KHz (Valid)
>
> With the patchset
> - 100KHz: 4.952us HIGH, 5.134us LOW, 49%/51%, Freq 99KHz (Valid)
> - 400KHz: 966ns HIGH, 1.628us LOW, 37.24%/62.76%, Freq 385.505KHz (Valid)
> - 1000KHz: 372ns HIGH, 720ns LOW, 34.07%/65.93%, Freq 915.741KHz (Valid)
>
> i2c3 (i2c@1c000):
>
> Before the patchset, I got:
> - 100KHz: 3.348us HIGH, 6.704us LOW, 33%/67%, Freq 99.5KHz (Not Valid tHIGH < 4.0us)
> - 400KHz: 864ns HIGH, 1.69us LOW, 33.83%/62.17%, Freq 391.543KHz (Valid)
> - 1000KHz: 360ns HIGH, 690ns LOW, 34.29%/65.71%, Freq 952.381KHz (Valid)
>
> With the patchset
> - 100KHz: 4.958us HIGH, 5.092us LOW, 49%/51%, Freq 99KHz (Valid)
> - 400KHz: 970ns HIGH, 1.582us LOW, 38%/62%, Freq 391.85KHz (Valid)
> - 1000KHz: 370ns HIGH, 680ns LOW, 35.24%/64.76%, Freq 952.57KHz (Valid)
>
> v2 changelog:
> - Keep the previous calculation for Meson6
> - Use I2C_MAX_STANDARD_MODE_FREQ
> - move the comment before the if()
> - use FIELD_PREP for setting div_l
> - Drop removal of meson_i2c_data
>
> Previous versions:
> V1: https://lkml.org/lkml/2022/3/26/109
>
> Lucas Tanure (2):
> i2c: meson: Use _SHIFT and _MASK for register definitions
> i2c: meson: Use 50% duty cycle for I2C clock
>
> drivers/i2c/busses/i2c-meson.c | 111 ++++++++++++++++++++++++---------
> 1 file changed, 82 insertions(+), 29 deletions(-)
>
Thanks a lot for the timings !
I think it's ok to be applied,
Neil
^ permalink raw reply [flat|nested] 8+ messages in thread