Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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	[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	[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, back to index

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

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git