* [PATCH 0/3] i2c: meson: scl rate fixups
@ 2020-10-07 8:07 Jerome Brunet
2020-10-07 8:07 ` [PATCH 1/3] i2c: meson: fix clock setting overwrite Jerome Brunet
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jerome Brunet @ 2020-10-07 8:07 UTC (permalink / raw)
To: Wolfram Sang, Kevin Hilman
Cc: Jerome Brunet, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
This patchset fixes various issues related to SCL rate on AML SoCs.
We retain the method which was used so far to set the SCL rate.
This method does not provide manual control of the clock duty cycle
but so far it does seems to be a problem for anyone.
Amlogic vendor kernel source uses "HIGH/LOW" method which allows to set
the rate and the duty cycle of the clock. However the documentation
around this method could be better and the result on actual HW is not
perfectly aligned with the comments in AML code. In case the current
method ever becomes a problem, we might consider switching to this
HIGH/LOW method.
Jerome Brunet (2):
i2c: meson: fix clock setting overwrite
i2c: meson: keep peripheral clock enabled
Nicolas Belin (1):
i2c: meson: fixup rate calculation with filter delay
drivers/i2c/busses/i2c-meson.c | 52 +++++++++++++++++++++-------------
1 file changed, 33 insertions(+), 19 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] i2c: meson: fix clock setting overwrite
2020-10-07 8:07 [PATCH 0/3] i2c: meson: scl rate fixups Jerome Brunet
@ 2020-10-07 8:07 ` Jerome Brunet
2020-10-08 9:59 ` Wolfram Sang
2020-10-07 8:07 ` [PATCH 2/3] i2c: meson: keep peripheral clock enabled Jerome Brunet
2020-10-07 8:07 ` [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay Jerome Brunet
2 siblings, 1 reply; 7+ messages in thread
From: Jerome Brunet @ 2020-10-07 8:07 UTC (permalink / raw)
To: Wolfram Sang, Kevin Hilman
Cc: Jerome Brunet, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
When the slave address is written in do_start(), SLAVE_ADDR is written
completely. This may overwrite some setting related to the clock rate
or signal filtering.
Fix this by writing only the bits related to slave address. To avoid
causing unexpected changed, explicitly disable filtering or high/low
clock mode which may have been left over by the bootloader.
Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/i2c/busses/i2c-meson.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index c5dec572fc48..dac0d2a00cec 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -5,6 +5,7 @@
* Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com>
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/i2c.h>
@@ -38,6 +39,12 @@
#define REG_CTRL_CLKDIVEXT_SHIFT 28
#define REG_CTRL_CLKDIVEXT_MASK 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 I2C_TIMEOUT_MS 500
enum {
@@ -147,6 +154,9 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
(div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
+ /* Disable HIGH/LOW mode */
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0);
+
dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
clk_rate, freq, div);
}
@@ -280,7 +290,10 @@ static void meson_i2c_do_start(struct meson_i2c *i2c, struct i2c_msg *msg)
token = (msg->flags & I2C_M_RD) ? TOKEN_SLAVE_ADDR_READ :
TOKEN_SLAVE_ADDR_WRITE;
- writel(msg->addr << 1, i2c->regs + REG_SLAVE_ADDR);
+
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_ADDR,
+ FIELD_PREP(REG_SLV_ADDR, msg->addr << 1));
+
meson_i2c_add_token(i2c, TOKEN_START);
meson_i2c_add_token(i2c, token);
}
@@ -461,6 +474,10 @@ static int meson_i2c_probe(struct platform_device *pdev)
return ret;
}
+ /* Disable filtering */
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR,
+ REG_SLV_SDA_FILTER | REG_SLV_SCL_FILTER, 0);
+
meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
return 0;
--
2.25.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] i2c: meson: keep peripheral clock enabled
2020-10-07 8:07 [PATCH 0/3] i2c: meson: scl rate fixups Jerome Brunet
2020-10-07 8:07 ` [PATCH 1/3] i2c: meson: fix clock setting overwrite Jerome Brunet
@ 2020-10-07 8:07 ` Jerome Brunet
2020-10-08 10:00 ` Wolfram Sang
2020-10-07 8:07 ` [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay Jerome Brunet
2 siblings, 1 reply; 7+ messages in thread
From: Jerome Brunet @ 2020-10-07 8:07 UTC (permalink / raw)
To: Wolfram Sang, Kevin Hilman
Cc: Jerome Brunet, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
SCL rate appears to be different than what is expected. For example,
We get 164kHz on i2c3 of the vim3 when 400kHz is expected. This is
partially due to the peripheral clock being disabled when the clock is
set.
Let's keep the peripheral clock on after probe to fix the problem. This
does not affect the SCL output which is still gated when i2c is idle.
Fixes: 09af1c2fa490 ("i2c: meson: set clock divider in probe instead of setting it for each transfer")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/i2c/busses/i2c-meson.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index dac0d2a00cec..e7ec2ab2a220 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -370,16 +370,12 @@ static int meson_i2c_xfer_messages(struct i2c_adapter *adap,
struct meson_i2c *i2c = adap->algo_data;
int i, ret = 0;
- clk_enable(i2c->clk);
-
for (i = 0; i < num; i++) {
ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1, atomic);
if (ret)
break;
}
- clk_disable(i2c->clk);
-
return ret ?: i;
}
@@ -448,7 +444,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
return ret;
}
- ret = clk_prepare(i2c->clk);
+ ret = clk_prepare_enable(i2c->clk);
if (ret < 0) {
dev_err(&pdev->dev, "can't prepare clock\n");
return ret;
@@ -470,7 +466,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
ret = i2c_add_adapter(&i2c->adap);
if (ret < 0) {
- clk_unprepare(i2c->clk);
+ clk_disable_unprepare(i2c->clk);
return ret;
}
@@ -488,7 +484,7 @@ static int meson_i2c_remove(struct platform_device *pdev)
struct meson_i2c *i2c = platform_get_drvdata(pdev);
i2c_del_adapter(&i2c->adap);
- clk_unprepare(i2c->clk);
+ clk_disable_unprepare(i2c->clk);
return 0;
}
--
2.25.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay
2020-10-07 8:07 [PATCH 0/3] i2c: meson: scl rate fixups Jerome Brunet
2020-10-07 8:07 ` [PATCH 1/3] i2c: meson: fix clock setting overwrite Jerome Brunet
2020-10-07 8:07 ` [PATCH 2/3] i2c: meson: keep peripheral clock enabled Jerome Brunet
@ 2020-10-07 8:07 ` Jerome Brunet
2020-10-08 10:00 ` Wolfram Sang
2 siblings, 1 reply; 7+ messages in thread
From: Jerome Brunet @ 2020-10-07 8:07 UTC (permalink / raw)
To: Wolfram Sang, Kevin Hilman
Cc: Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel, Jerome Brunet
From: Nicolas Belin <nbelin@baylibre.com>
Apparently, 15 cycles of the peripheral clock are used by the controller
for sampling and filtering. Because this was not known before, the rate
calculation is slightly off.
Clean up and fix the calculation taking this filtering delay into account.
Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/i2c/busses/i2c-meson.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index e7ec2ab2a220..ef73a42577cc 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -34,10 +34,8 @@
#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, 12)
-#define REG_CTRL_CLKDIVEXT_SHIFT 28
-#define REG_CTRL_CLKDIVEXT_MASK GENMASK(29, 28)
+#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)
@@ -46,6 +44,7 @@
#define REG_SLV_SCL_LOW_EN BIT(28)
#define I2C_TIMEOUT_MS 500
+#define FILTER_DELAY 15
enum {
TOKEN_END = 0,
@@ -140,19 +139,21 @@ 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;
- div = DIV_ROUND_UP(clk_rate, freq * i2c->data->div_factor);
+ div = DIV_ROUND_UP(clk_rate, freq);
+ div -= FILTER_DELAY;
+ div = DIV_ROUND_UP(div, i2c->data->div_factor);
/* clock divider has 12 bits */
- if (div >= (1 << 12)) {
+ if (div > GENMASK(11, 0)) {
dev_err(i2c->dev, "requested bus frequency too low\n");
- div = (1 << 12) - 1;
+ div = GENMASK(11, 0);
}
- meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
- (div & GENMASK(9, 0)) << REG_CTRL_CLKDIV_SHIFT);
+ 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_CLKDIVEXT_MASK,
- (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
+ meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT,
+ FIELD_PREP(REG_CTRL_CLKDIVEXT, div >> 10));
/* Disable HIGH/LOW mode */
meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0);
--
2.25.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] i2c: meson: fix clock setting overwrite
2020-10-07 8:07 ` [PATCH 1/3] i2c: meson: fix clock setting overwrite Jerome Brunet
@ 2020-10-08 9:59 ` Wolfram Sang
0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2020-10-08 9:59 UTC (permalink / raw)
To: Jerome Brunet
Cc: Kevin Hilman, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
On Wed, Oct 07, 2020 at 10:07:49AM +0200, Jerome Brunet wrote:
> When the slave address is written in do_start(), SLAVE_ADDR is written
> completely. This may overwrite some setting related to the clock rate
> or signal filtering.
>
> Fix this by writing only the bits related to slave address. To avoid
> causing unexpected changed, explicitly disable filtering or high/low
> clock mode which may have been left over by the bootloader.
>
> Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] i2c: meson: keep peripheral clock enabled
2020-10-07 8:07 ` [PATCH 2/3] i2c: meson: keep peripheral clock enabled Jerome Brunet
@ 2020-10-08 10:00 ` Wolfram Sang
0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2020-10-08 10:00 UTC (permalink / raw)
To: Jerome Brunet
Cc: Kevin Hilman, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]
On Wed, Oct 07, 2020 at 10:07:50AM +0200, Jerome Brunet wrote:
> SCL rate appears to be different than what is expected. For example,
> We get 164kHz on i2c3 of the vim3 when 400kHz is expected. This is
> partially due to the peripheral clock being disabled when the clock is
> set.
>
> Let's keep the peripheral clock on after probe to fix the problem. This
> does not affect the SCL output which is still gated when i2c is idle.
>
> Fixes: 09af1c2fa490 ("i2c: meson: set clock divider in probe instead of setting it for each transfer")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay
2020-10-07 8:07 ` [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay Jerome Brunet
@ 2020-10-08 10:00 ` Wolfram Sang
0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2020-10-08 10:00 UTC (permalink / raw)
To: Jerome Brunet
Cc: Kevin Hilman, Nicolas Belin, linux-i2c, linux-amlogic, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 605 bytes --]
On Wed, Oct 07, 2020 at 10:07:51AM +0200, Jerome Brunet wrote:
> From: Nicolas Belin <nbelin@baylibre.com>
>
> Apparently, 15 cycles of the peripheral clock are used by the controller
> for sampling and filtering. Because this was not known before, the rate
> calculation is slightly off.
>
> Clean up and fix the calculation taking this filtering delay into account.
>
> Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-08 10:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 8:07 [PATCH 0/3] i2c: meson: scl rate fixups Jerome Brunet
2020-10-07 8:07 ` [PATCH 1/3] i2c: meson: fix clock setting overwrite Jerome Brunet
2020-10-08 9:59 ` Wolfram Sang
2020-10-07 8:07 ` [PATCH 2/3] i2c: meson: keep peripheral clock enabled Jerome Brunet
2020-10-08 10:00 ` Wolfram Sang
2020-10-07 8:07 ` [PATCH 3/3] i2c: meson: fixup rate calculation with filter delay Jerome Brunet
2020-10-08 10:00 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).