linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/7] Introducing an attribute to select the time setting
@ 2021-09-17 10:14 Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset Kewei Xu
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

v6:
Add the judgment condition, clear the handshake signal between dma and
i2c when multiple msgs are transmitted.

v5:
1. Replace the previous variable name "default_timing_adjust" with "use-default-timing"
2. Added waiting for dma reset mechanism
3. Remove received patch(dt-bindings: i2c: update bindings for MT8195 SOC)

v4:
1. Remove the repeated assignment of the inter_clk_div parameter
2. Modify the wrong assignment of OFFSET_MULTI_DMA
3. Unify the log print format of the i2c_dump_register() and drop the extra outer parentheses
4. Place the fixes at the very least
5. Add fixed tags 25708278f810 ("i2c: mediatek: Add i2c support for MediaTek MT8183")
6. Add "i2c: mediatek: modify bus speed calculation formula"
7. Fix single line characters exceeding 80 characters
8. Combine two different series of patches.

v3:
1. Fix code errors caused by v2 modification

v2:
1. Add "dt-bindings: i2c: add attribute default-timing-adjust"
2. Split the fix into sepatate patch.

Kewei Xu (7):
  i2c: mediatek: fixing the incorrect register offset
  i2c: mediatek: Reset the handshake signal between i2c and dma
  i2c: mediatek: Dump i2c/dma register when a timeout occurs
  dt-bindings: i2c: add attribute use-default-timing
  i2c: mediatek: Add OFFSET_EXT_CONF setting back
  i2c: mediatek: Isolate speed setting via dts for special devices
  i2c: mediatek: modify bus speed calculation formula

 .../devicetree/bindings/i2c/i2c-mt65xx.txt    |   2 +
 drivers/i2c/busses/i2c-mt65xx.c               | 207 ++++++++++++++++--
 2 files changed, 192 insertions(+), 17 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-10-02  6:28   ` Wolfram Sang
  2021-09-17 10:14 ` [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma Kewei Xu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu, Chen-Yu Tsai

The reason for the modification here is that the previous
offset information is incorrect, OFFSET_DEBUGSTAT = 0xE4 is
the correct value.

Fixes: 25708278f810 ("i2c: mediatek: Add i2c support for MediaTek MT8183")
Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 477480d1de6b..32518081b5a4 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -193,7 +193,7 @@ static const u16 mt_i2c_regs_v2[] = {
 	[OFFSET_CLOCK_DIV] = 0x48,
 	[OFFSET_SOFTRESET] = 0x50,
 	[OFFSET_SCL_MIS_COMP_POINT] = 0x90,
-	[OFFSET_DEBUGSTAT] = 0xe0,
+	[OFFSET_DEBUGSTAT] = 0xe4,
 	[OFFSET_DEBUGCTRL] = 0xe8,
 	[OFFSET_FIFO_STAT] = 0xf4,
 	[OFFSET_FIFO_THRESH] = 0xf8,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-10-02  6:30   ` Wolfram Sang
  2021-09-17 10:14 ` [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs Kewei Xu
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

Due to changes in the hardware design of the handshaking signal
between i2c and dma, it is necessary to reset the handshaking
signal before each transfer to ensure that the multi-msgs can
be transferred correctly.

Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 32518081b5a4..08ce8a417bed 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
@@ -47,6 +48,9 @@
 #define I2C_RD_TRANAC_VALUE		0x0001
 #define I2C_SCL_MIS_COMP_VALUE		0x0000
 #define I2C_CHN_CLR_FLAG		0x0000
+#define I2C_CLR_DEBUGCTR		0x0000
+#define I2C_RELIABILITY			0x0010
+#define I2C_DMAACK_ENABLE		0x0008
 
 #define I2C_DMA_CON_TX			0x0000
 #define I2C_DMA_CON_RX			0x0001
@@ -842,6 +846,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	u16 restart_flag = 0;
 	u16 dma_sync = 0;
 	u32 reg_4g_mode;
+	u32 reg_dma_reset;
 	u8 *dma_rd_buf = NULL;
 	u8 *dma_wr_buf = NULL;
 	dma_addr_t rpaddr = 0;
@@ -855,6 +860,27 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 
 	reinit_completion(&i2c->msg_complete);
 
+	if (i2c->dev_comp->apdma_sync && i2c->op != I2C_MASTER_WRRD && num > 1) {
+		mtk_i2c_writew(i2c, I2C_CLR_DEBUGCTR, OFFSET_DEBUGCTRL);
+		writel(I2C_DMA_HANDSHAKE_RST | I2C_DMA_WARM_RST,
+		       i2c->pdmabase + OFFSET_RST);
+
+		ret = readw_poll_timeout(i2c->pdmabase + OFFSET_RST,
+					 reg_dma_reset,
+					 !(reg_dma_reset & I2C_DMA_WARM_RST),
+					 0, 100);
+		if (ret) {
+			dev_err(i2c->dev, "DMA warm reset timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_RST);
+		mtk_i2c_writew(i2c, I2C_HANDSHAKE_RST, OFFSET_SOFTRESET);
+		mtk_i2c_writew(i2c, I2C_CHN_CLR_FLAG, OFFSET_SOFTRESET);
+		mtk_i2c_writew(i2c, I2C_RELIABILITY | I2C_DMAACK_ENABLE,
+			       OFFSET_DEBUGCTRL);
+	}
+
 	control_reg = mtk_i2c_readw(i2c, OFFSET_CONTROL) &
 			~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
 	if ((i2c->speed_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) || (left_num >= 1))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-10-02  6:37   ` Wolfram Sang
  2021-09-17 10:14 ` [PATCH v7 4/7] dt-bindings: i2c: add attribute use-default-timing Kewei Xu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

When a timeout error occurs in i2c transter, it is usually related
to the i2c/dma IP hardware configuration. Therefore, the purpose of
this patch is to dump the key register values of i2c/dma when a
timeout occurs in i2c for debugging.

Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 56 ++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 08ce8a417bed..8a3898a38d8e 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -129,6 +129,7 @@ enum I2C_REGS_OFFSET {
 	OFFSET_HS,
 	OFFSET_SOFTRESET,
 	OFFSET_DCM_EN,
+	OFFSET_MULTI_DMA,
 	OFFSET_PATH_DIR,
 	OFFSET_DEBUGSTAT,
 	OFFSET_DEBUGCTRL,
@@ -196,6 +197,7 @@ static const u16 mt_i2c_regs_v2[] = {
 	[OFFSET_TRANSFER_LEN_AUX] = 0x44,
 	[OFFSET_CLOCK_DIV] = 0x48,
 	[OFFSET_SOFTRESET] = 0x50,
+	[OFFSET_MULTI_DMA] = 0x8c,
 	[OFFSET_SCL_MIS_COMP_POINT] = 0x90,
 	[OFFSET_DEBUGSTAT] = 0xe4,
 	[OFFSET_DEBUGCTRL] = 0xe8,
@@ -837,6 +839,57 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk)
 	return 0;
 }
 
+static void i2c_dump_register(struct mtk_i2c *i2c)
+{
+	dev_err(i2c->dev, "SLAVE_ADDR: 0x%x, INTR_MASK: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_SLAVE_ADDR),
+		mtk_i2c_readw(i2c, OFFSET_INTR_MASK));
+	dev_err(i2c->dev, "INTR_STAT: 0x%x, CONTROL: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_INTR_STAT),
+		mtk_i2c_readw(i2c, OFFSET_CONTROL));
+	dev_err(i2c->dev, "TRANSFER_LEN: 0x%x, TRANSAC_LEN: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_TRANSFER_LEN),
+		mtk_i2c_readw(i2c, OFFSET_TRANSAC_LEN));
+	dev_err(i2c->dev, "DELAY_LEN: 0x%x, HTIMING: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_DELAY_LEN),
+		mtk_i2c_readw(i2c, OFFSET_TIMING));
+	dev_err(i2c->dev, "START: 0x%x, EXT_CONF: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_START),
+		mtk_i2c_readw(i2c, OFFSET_EXT_CONF));
+	dev_err(i2c->dev, "HS: 0x%x, IO_CONFIG: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_HS),
+		mtk_i2c_readw(i2c, OFFSET_IO_CONFIG));
+	dev_err(i2c->dev, "DCM_EN: 0x%x, TRANSFER_LEN_AUX: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_DCM_EN),
+		mtk_i2c_readw(i2c, OFFSET_TRANSFER_LEN_AUX));
+	dev_err(i2c->dev, "CLOCK_DIV: 0x%x, FIFO_STAT: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_CLOCK_DIV),
+		mtk_i2c_readw(i2c, OFFSET_FIFO_STAT));
+	dev_err(i2c->dev, "DEBUGCTRL : 0x%x, DEBUGSTAT: 0x%x\n",
+		mtk_i2c_readw(i2c, OFFSET_DEBUGCTRL),
+		mtk_i2c_readw(i2c, OFFSET_DEBUGSTAT));
+	if (i2c->dev_comp->regs == mt_i2c_regs_v2) {
+		dev_err(i2c->dev, "LTIMING: 0x%x, MULTI_DMA: 0x%x\n",
+			mtk_i2c_readw(i2c, OFFSET_LTIMING),
+			mtk_i2c_readw(i2c, OFFSET_MULTI_DMA));
+	}
+	dev_err(i2c->dev, "\nDMA_INT_FLAG: 0x%x, DMA_INT_EN: 0x%x\n",
+		readl(i2c->pdmabase + OFFSET_INT_FLAG),
+		readl(i2c->pdmabase + OFFSET_INT_EN));
+	dev_err(i2c->dev, "DMA_EN: 0x%x, DMA_CON: 0x%x\n",
+		readl(i2c->pdmabase + OFFSET_EN),
+		readl(i2c->pdmabase + OFFSET_CON));
+	dev_err(i2c->dev, "DMA_TX_MEM_ADDR: 0x%x, DMA_RX_MEM_ADDR: 0x%x\n",
+		readl(i2c->pdmabase + OFFSET_TX_MEM_ADDR),
+		readl(i2c->pdmabase + OFFSET_RX_MEM_ADDR));
+	dev_err(i2c->dev, "DMA_TX_LEN: 0x%x, DMA_RX_LEN: 0x%x\n",
+		readl(i2c->pdmabase + OFFSET_TX_LEN),
+		readl(i2c->pdmabase + OFFSET_RX_LEN));
+	dev_err(i2c->dev, "DMA_TX_4G_MODE: 0x%x, DMA_RX_4G_MODE: 0x%x",
+		readl(i2c->pdmabase + OFFSET_TX_4G_MODE),
+		readl(i2c->pdmabase + OFFSET_RX_4G_MODE));
+}
+
 static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 			       int num, int left_num)
 {
@@ -1065,7 +1118,8 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	}
 
 	if (ret == 0) {
-		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr);
+		dev_err(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr);
+		i2c_dump_register(i2c);
 		mtk_i2c_init_hw(i2c);
 		return -ETIMEDOUT;
 	}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 4/7] dt-bindings: i2c: add attribute use-default-timing
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
                   ` (2 preceding siblings ...)
  2021-09-17 10:14 ` [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back Kewei Xu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu, Rob Herring

Add attribute use-default-timing for DT-binding document.

Fixes: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support")
Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt b/Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt
index 5ea216ae7084..4dbb69745ff3 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt
@@ -34,6 +34,8 @@ Optional properties:
     Only mt6589 and mt8135 support this feature.
   - mediatek,use-push-pull: IO config use push-pull mode.
   - vbus-supply: phandle to the regulator that provides power to SCL/SDA.
+  - mediatek,use-default-timing: use default timing calculation, no timing
+    adjustment.
 
 Example:
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
                   ` (3 preceding siblings ...)
  2021-09-17 10:14 ` [PATCH v7 4/7] dt-bindings: i2c: add attribute use-default-timing Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-10-02  6:40   ` Wolfram Sang
  2021-09-17 10:14 ` [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices Kewei Xu
  2021-09-17 10:14 ` [PATCH v7 7/7] i2c: mediatek: modify bus speed calculation formula Kewei Xu
  6 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust
support"), we miss setting OFFSET_EXT_CONF register if
i2c->dev_comp->timing_adjust is false, now add it back.

Fixes: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support")
Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 8a3898a38d8e..0c611f7bf384 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -42,6 +42,8 @@
 #define I2C_HANDSHAKE_RST		0x0020
 #define I2C_FIFO_ADDR_CLR		0x0001
 #define I2C_DELAY_LEN			0x0002
+#define I2C_ST_START_CON		0x8001
+#define I2C_FS_START_CON		0x1800
 #define I2C_TIME_CLR_VALUE		0x0000
 #define I2C_TIME_DEFAULT_VALUE		0x0003
 #define I2C_WRRD_TRANAC_VALUE		0x0002
@@ -486,6 +488,7 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 {
 	u16 control_reg;
 	u16 intr_stat_reg;
+	u16 ext_conf_val;
 
 	mtk_i2c_writew(i2c, I2C_CHN_CLR_FLAG, OFFSET_START);
 	intr_stat_reg = mtk_i2c_readw(i2c, OFFSET_INTR_STAT);
@@ -524,8 +527,13 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 	if (i2c->dev_comp->ltiming_adjust)
 		mtk_i2c_writew(i2c, i2c->ltiming_reg, OFFSET_LTIMING);
 
+	if (i2c->speed_hz <= I2C_MAX_STANDARD_MODE_FREQ)
+		ext_conf_val = I2C_ST_START_CON;
+	else
+		ext_conf_val = I2C_FS_START_CON;
+
 	if (i2c->dev_comp->timing_adjust) {
-		mtk_i2c_writew(i2c, i2c->ac_timing.ext, OFFSET_EXT_CONF);
+		ext_conf_val = i2c->ac_timing.ext;
 		mtk_i2c_writew(i2c, i2c->ac_timing.inter_clk_div,
 			       OFFSET_CLOCK_DIV);
 		mtk_i2c_writew(i2c, I2C_SCL_MIS_COMP_VALUE,
@@ -550,6 +558,7 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 				       OFFSET_HS_STA_STO_AC_TIMING);
 		}
 	}
+	mtk_i2c_writew(i2c, ext_conf_val, OFFSET_EXT_CONF);
 
 	/* If use i2c pin from PMIC mt6397 side, need set PATH_DIR first */
 	if (i2c->have_pmic)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
                   ` (4 preceding siblings ...)
  2021-09-17 10:14 ` [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  2021-10-02  6:40   ` Wolfram Sang
  2021-09-17 10:14 ` [PATCH v7 7/7] i2c: mediatek: modify bus speed calculation formula Kewei Xu
  6 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust
support"), the I2C timing calculation has been revised to support
ac-timing adjustment, however that will break on some I2C components.
As a result we want to introduce a new setting "default-adjust-timing"
so those components can choose to use the old (default) timing algorithm.

Fixes: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support")
Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 77 +++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 0c611f7bf384..6a07adaa6422 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -66,6 +66,7 @@
 #define I2C_DMA_HARD_RST		0x0002
 #define I2C_DMA_HANDSHAKE_RST		0x0004
 
+#define I2C_DEFAULT_CLK_DIV		5
 #define MAX_SAMPLE_CNT_DIV		8
 #define MAX_STEP_CNT_DIV		64
 #define MAX_CLOCK_DIV			256
@@ -250,6 +251,7 @@ struct mtk_i2c {
 	struct clk *clk_arb;		/* Arbitrator clock for i2c */
 	bool have_pmic;			/* can use i2c pins from PMIC */
 	bool use_push_pull;		/* IO config push-pull mode */
+	bool use_default_timing;	/* no timing adjust mode */
 
 	u16 irq_stat;			/* interrupt status */
 	unsigned int clk_src_div;
@@ -532,7 +534,11 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 	else
 		ext_conf_val = I2C_FS_START_CON;
 
-	if (i2c->dev_comp->timing_adjust) {
+	if (i2c->use_default_timing) {
+		if (i2c->dev_comp->timing_adjust)
+			mtk_i2c_writew(i2c, I2C_DEFAULT_CLK_DIV - 1,
+				       OFFSET_CLOCK_DIV);
+	} else if (i2c->dev_comp->timing_adjust) {
 		ext_conf_val = i2c->ac_timing.ext;
 		mtk_i2c_writew(i2c, i2c->ac_timing.inter_clk_div,
 			       OFFSET_CLOCK_DIV);
@@ -615,7 +621,7 @@ static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
 	unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1),
 					 clk_src);
 
-	if (!i2c->dev_comp->timing_adjust)
+	if (i2c->use_default_timing || !i2c->dev_comp->timing_adjust)
 		return 0;
 
 	if (i2c->dev_comp->ltiming_adjust)
@@ -775,7 +781,65 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 	return 0;
 }
 
-static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk)
+static int mtk_i2c_set_speed_default_timing(struct mtk_i2c *i2c,
+					    unsigned int parent_clk)
+{
+	unsigned int clk_src;
+	unsigned int step_cnt;
+	unsigned int sample_cnt;
+	unsigned int l_step_cnt;
+	unsigned int l_sample_cnt;
+	unsigned int target_speed;
+	int ret;
+
+	if (i2c->dev_comp->timing_adjust)
+		i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV;
+
+	clk_src = parent_clk / i2c->clk_src_div;
+	target_speed = i2c->speed_hz;
+
+	if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ) {
+		/* Set master code speed register */
+		ret = mtk_i2c_calculate_speed(i2c, clk_src,
+					      I2C_MAX_FAST_MODE_FREQ,
+					      &l_step_cnt, &l_sample_cnt);
+		if (ret < 0)
+			return ret;
+
+		i2c->timing_reg = (l_sample_cnt << 8) | l_step_cnt;
+
+		/* Set the high speed mode register */
+		ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed,
+					      &step_cnt, &sample_cnt);
+		if (ret < 0)
+			return ret;
+
+		i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE |
+			(sample_cnt << 12) | (step_cnt << 8);
+
+		if (i2c->dev_comp->ltiming_adjust)
+			i2c->ltiming_reg = (l_sample_cnt << 6) | l_step_cnt |
+					   (sample_cnt << 12) | (step_cnt << 9);
+	} else {
+		ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed,
+					      &step_cnt, &sample_cnt);
+		if (ret < 0)
+			return ret;
+
+		i2c->timing_reg = (sample_cnt << 8) | step_cnt;
+
+		/* Disable the high speed transaction */
+		i2c->high_speed_reg = I2C_TIME_CLR_VALUE;
+
+		if (i2c->dev_comp->ltiming_adjust)
+			i2c->ltiming_reg = (sample_cnt << 6) | step_cnt;
+	}
+
+	return 0;
+}
+
+static int mtk_i2c_set_speed_adjust_timing(struct mtk_i2c *i2c,
+					   unsigned int parent_clk)
 {
 	unsigned int clk_src;
 	unsigned int step_cnt;
@@ -1271,6 +1335,8 @@ static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c)
 	i2c->have_pmic = of_property_read_bool(np, "mediatek,have-pmic");
 	i2c->use_push_pull =
 		of_property_read_bool(np, "mediatek,use-push-pull");
+	i2c->use_default_timing =
+		of_property_read_bool(np, "mediatek,use-default-timing");
 
 	i2c_parse_fw_timings(i2c->dev, &i2c->timing_info, true);
 
@@ -1357,7 +1423,10 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 
 	strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name));
 
-	ret = mtk_i2c_set_speed(i2c, clk_get_rate(clk));
+	if (i2c->use_default_timing)
+		ret = mtk_i2c_set_speed_default_timing(i2c, clk_get_rate(clk));
+	else
+		ret = mtk_i2c_set_speed_adjust_timing(i2c, clk_get_rate(clk));
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to set the speed.\n");
 		return -EINVAL;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 7/7] i2c: mediatek: modify bus speed calculation formula
  2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
                   ` (5 preceding siblings ...)
  2021-09-17 10:14 ` [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices Kewei Xu
@ 2021-09-17 10:14 ` Kewei Xu
  6 siblings, 0 replies; 20+ messages in thread
From: Kewei Xu @ 2021-09-17 10:14 UTC (permalink / raw)
  To: wsa
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	kewei.xu

When clock-div is 0 or greater than 1, the bus speed
calculated by the old speed calculation formula will be
larger than the target speed. So we update the formula.

Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
Reviewed-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 35 +++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 6a07adaa6422..5b379a552228 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -69,11 +69,12 @@
 #define I2C_DEFAULT_CLK_DIV		5
 #define MAX_SAMPLE_CNT_DIV		8
 #define MAX_STEP_CNT_DIV		64
-#define MAX_CLOCK_DIV			256
+#define MAX_CLOCK_DIV_8BITS		256
+#define MAX_CLOCK_DIV_5BITS		32
 #define MAX_HS_STEP_CNT_DIV		8
-#define I2C_STANDARD_MODE_BUFFER	(1000 / 2)
-#define I2C_FAST_MODE_BUFFER		(300 / 2)
-#define I2C_FAST_MODE_PLUS_BUFFER	(20 / 2)
+#define I2C_STANDARD_MODE_BUFFER	(1000 / 3)
+#define I2C_FAST_MODE_BUFFER		(300 / 3)
+#define I2C_FAST_MODE_PLUS_BUFFER	(20 / 3)
 
 #define I2C_CONTROL_RS                  (0x1 << 1)
 #define I2C_CONTROL_DMA_EN              (0x1 << 2)
@@ -725,14 +726,26 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 	unsigned int best_mul;
 	unsigned int cnt_mul;
 	int ret = -EINVAL;
+	int clock_div_constraint = 0;
 
 	if (target_speed > I2C_MAX_HIGH_SPEED_MODE_FREQ)
 		target_speed = I2C_MAX_HIGH_SPEED_MODE_FREQ;
 
+	if (i2c->use_default_timing) {
+		clock_div_constraint = 0;
+	} else if (i2c->dev_comp->ltiming_adjust &&
+		   i2c->ac_timing.inter_clk_div > 1) {
+		clock_div_constraint = 1;
+	} else if (i2c->dev_comp->ltiming_adjust &&
+		   i2c->ac_timing.inter_clk_div == 0) {
+		clock_div_constraint = -1;
+	}
+
 	max_step_cnt = mtk_i2c_max_step_cnt(target_speed);
 	base_step_cnt = max_step_cnt;
 	/* Find the best combination */
-	opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
+	opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed) +
+		  clock_div_constraint;
 	best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
 
 	/* Search for the best pair (sample_cnt, step_cnt) with
@@ -767,7 +780,8 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 	sample_cnt = base_sample_cnt;
 	step_cnt = base_step_cnt;
 
-	if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
+	if ((clk_src / (2 * (sample_cnt * step_cnt - clock_div_constraint))) >
+		 target_speed) {
 		/* In this case, hardware can't support such
 		 * low i2c_bus_freq
 		 */
@@ -854,13 +868,16 @@ static int mtk_i2c_set_speed_adjust_timing(struct mtk_i2c *i2c,
 	target_speed = i2c->speed_hz;
 	parent_clk /= i2c->clk_src_div;
 
-	if (i2c->dev_comp->timing_adjust)
-		max_clk_div = MAX_CLOCK_DIV;
+	if (i2c->dev_comp->timing_adjust && i2c->dev_comp->ltiming_adjust)
+		max_clk_div = MAX_CLOCK_DIV_5BITS;
+	else if (i2c->dev_comp->timing_adjust)
+		max_clk_div = MAX_CLOCK_DIV_8BITS;
 	else
 		max_clk_div = 1;
 
 	for (clk_div = 1; clk_div <= max_clk_div; clk_div++) {
 		clk_src = parent_clk / clk_div;
+		i2c->ac_timing.inter_clk_div = clk_div - 1;
 
 		if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ) {
 			/* Set master code speed register */
@@ -907,8 +924,6 @@ static int mtk_i2c_set_speed_adjust_timing(struct mtk_i2c *i2c,
 		break;
 	}
 
-	i2c->ac_timing.inter_clk_div = clk_div - 1;
-
 	return 0;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset
  2021-09-17 10:14 ` [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset Kewei Xu
@ 2021-10-02  6:28   ` Wolfram Sang
  0 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2021-10-02  6:28 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei,
	Chen-Yu Tsai

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Fri, Sep 17, 2021 at 06:14:10PM +0800, Kewei Xu wrote:
> The reason for the modification here is that the previous
> offset information is incorrect, OFFSET_DEBUGSTAT = 0xE4 is
> the correct value.
> 
> Fixes: 25708278f810 ("i2c: mediatek: Add i2c support for MediaTek MT8183")
> Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: Qii Wang <qii.wang@mediatek.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma
  2021-09-17 10:14 ` [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma Kewei Xu
@ 2021-10-02  6:30   ` Wolfram Sang
  2021-10-08  6:19     ` Kewei Xu
  0 siblings, 1 reply; 20+ messages in thread
From: Wolfram Sang @ 2021-10-02  6:30 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]


> +#define I2C_CLR_DEBUGCTR		0x0000

Hmm, I don't think that a macro to clear a register helps readability...

> +		mtk_i2c_writew(i2c, I2C_CLR_DEBUGCTR, OFFSET_DEBUGCTRL);

...

 +		mtk_i2c_writew(i2c, 0, OFFSET_DEBUGCTRL);

looks good to me. Anyhow, it is not a big issue. Let me know if you want
to change it or keep it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs
  2021-09-17 10:14 ` [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs Kewei Xu
@ 2021-10-02  6:37   ` Wolfram Sang
  2021-10-08  7:13     ` Kewei Xu
  0 siblings, 1 reply; 20+ messages in thread
From: Wolfram Sang @ 2021-10-02  6:37 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]


> @@ -837,6 +839,57 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk)
>  	return 0;
>  }

> +static void i2c_dump_register(struct mtk_i2c *i2c)
> +{
> +	dev_err(i2c->dev, "SLAVE_ADDR: 0x%x, INTR_MASK: 0x%x\n",
> +		mtk_i2c_readw(i2c, OFFSET_SLAVE_ADDR),
> +		mtk_i2c_readw(i2c, OFFSET_INTR_MASK));

I think this is too verbose and should be a debugging only patch not
really suited for upstream. But if you like it this way, then keep
the verbosity. However, dev_err is too strong, this really needs to be
dev_dbg. Timeouts can happen on an I2C bus, think about an EEPROM in a
long erase cycle while you want to read it. Perfectly normal.


>  	if (ret == 0) {
> -		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr);
> +		dev_err(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr);
> +		i2c_dump_register(i2c);

Needs to stay dev_dbg as well.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back
  2021-09-17 10:14 ` [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back Kewei Xu
@ 2021-10-02  6:40   ` Wolfram Sang
  0 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2021-10-02  6:40 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Fri, Sep 17, 2021 at 06:14:14PM +0800, Kewei Xu wrote:
> In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust
> support"), we miss setting OFFSET_EXT_CONF register if
> i2c->dev_comp->timing_adjust is false, now add it back.
> 
> Fixes: be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support")
> Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
> Reviewed-by: Qii Wang <qii.wang@mediatek.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-09-17 10:14 ` [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices Kewei Xu
@ 2021-10-02  6:40   ` Wolfram Sang
  2021-10-08  8:47     ` Kewei Xu
  0 siblings, 1 reply; 20+ messages in thread
From: Wolfram Sang @ 2021-10-02  6:40 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Fri, Sep 17, 2021 at 06:14:15PM +0800, Kewei Xu wrote:
> In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust
> support"), the I2C timing calculation has been revised to support
> ac-timing adjustment, however that will break on some I2C components.
> As a result we want to introduce a new setting "default-adjust-timing"
> so those components can choose to use the old (default) timing algorithm.

Why can't the new calculation be updated in a way that it works for all
I2C components?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma
  2021-10-02  6:30   ` Wolfram Sang
@ 2021-10-08  6:19     ` Kewei Xu
  0 siblings, 0 replies; 20+ messages in thread
From: Kewei Xu @ 2021-10-08  6:19 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

On Sat, 2021-10-02 at 08:30 +0200, Wolfram Sang wrote:
> > +#define I2C_CLR_DEBUGCTR		0x0000
> 
> Hmm, I don't think that a macro to clear a register helps
> readability...
> 
> > +		mtk_i2c_writew(i2c, I2C_CLR_DEBUGCTR,
> > OFFSET_DEBUGCTRL);
> 
> ..
> 
>  +		mtk_i2c_writew(i2c, 0, OFFSET_DEBUGCTRL);
> 
> looks good to me. Anyhow, it is not a big issue. Let me know if you
> want
> to change it or keep it.
> 
OK, I will use 0x00 instead of macro on V8 version,Thanks.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs
  2021-10-02  6:37   ` Wolfram Sang
@ 2021-10-08  7:13     ` Kewei Xu
  0 siblings, 0 replies; 20+ messages in thread
From: Kewei Xu @ 2021-10-08  7:13 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

On Sat, 2021-10-02 at 08:37 +0200, Wolfram Sang wrote:
> > @@ -837,6 +839,57 @@ static int mtk_i2c_set_speed(struct mtk_i2c
> > *i2c, unsigned int parent_clk)
> >  	return 0;
> >  }
> > +static void i2c_dump_register(struct mtk_i2c *i2c)
> > +{
> > +	dev_err(i2c->dev, "SLAVE_ADDR: 0x%x, INTR_MASK: 0x%x\n",
> > +		mtk_i2c_readw(i2c, OFFSET_SLAVE_ADDR),
> > +		mtk_i2c_readw(i2c, OFFSET_INTR_MASK));
> 
> I think this is too verbose and should be a debugging only patch not
> really suited for upstream. But if you like it this way, then keep
> the verbosity. However, dev_err is too strong, this really needs to
> be
> dev_dbg. Timeouts can happen on an I2C bus, think about an EEPROM in
> a
> long erase cycle while you want to read it. Perfectly normal.
> 
> 
> >  	if (ret == 0) {
> > -		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs-
> > >addr);
> > +		dev_err(i2c->dev, "addr: %x, transfer timeout\n", msgs-
> > >addr);
> > +		i2c_dump_register(i2c);
> 
> Needs to stay dev_dbg as well.
> 
> Yes, It is used for debugging,but dump the value of value of the
> register is very important for debugging,so we think it is
> necessary. We will use dev_dbg to replace dev_err in V8. Thanks~

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-10-02  6:40   ` Wolfram Sang
@ 2021-10-08  8:47     ` Kewei Xu
  2021-10-11 10:56       ` Wolfram Sang
  0 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-10-08  8:47 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

On Sat, 2021-10-02 at 08:40 +0200, Wolfram Sang wrote:
> On Fri, Sep 17, 2021 at 06:14:15PM +0800, Kewei Xu wrote:
> > In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing
> > adjust
> > support"), the I2C timing calculation has been revised to support
> > ac-timing adjustment, however that will break on some I2C
> > components.
> > As a result we want to introduce a new setting "default-adjust-
> > timing"
> > so those components can choose to use the old (default) timing
> > algorithm.
> 
> Why can't the new calculation be updated in a way that it works for
> all
> I2C components?
> 
Hi, In the commit be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing
adjust support"), the I2C timing calculation has been revised to
support ac-timing adjustment.But in our design, it will make
tSU,STA/tHD,STA/tSU,STO shorter when the slave device have clock-
stretching feature. Then we upload the commit a80f24945fcf ("i2c:
mediatek: Use scl_int_delay_ns to compensate clock-stretching") to
support adjusting tSU,STA/tHD,STA/tSU,STO when the slave device clock-
stretching. But if the slave device stretch the SCL line for too long
time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec.
However in the old (default) timing algorithm before the commit
be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"),
tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new
setting "default-adjust-timing" for using the old (default) timing
algorithm. Thanks~

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-10-08  8:47     ` Kewei Xu
@ 2021-10-11 10:56       ` Wolfram Sang
  2021-11-29 12:49         ` Wolfram Sang
  0 siblings, 1 reply; 20+ messages in thread
From: Wolfram Sang @ 2021-10-11 10:56 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

Hi,

> stretching. But if the slave device stretch the SCL line for too long
> time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec.

Isn't the new algorithm broken if it cannot support clock stretching?
What was the problem of the old algorithm not meeting the spec?

> However in the old (default) timing algorithm before the commit
> be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"),
> tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new
> setting "default-adjust-timing" for using the old (default) timing
> algorithm."

What I still do not get: the old algorithm was able to handle clock
stretching. Why can't you update the new one to handle clock stretching
as well. I might be missing something, but what is it?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-10-11 10:56       ` Wolfram Sang
@ 2021-11-29 12:49         ` Wolfram Sang
  2021-12-18  9:44           ` Kewei Xu
  0 siblings, 1 reply; 20+ messages in thread
From: Wolfram Sang @ 2021-11-29 12:49 UTC (permalink / raw)
  To: Kewei Xu, matthias.bgg, robh+dt, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel, linux-mediatek, srv_heupstream,
	leilk.liu, qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu,
	yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]


> > stretching. But if the slave device stretch the SCL line for too long
> > time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet spec.
> 
> Isn't the new algorithm broken if it cannot support clock stretching?
> What was the problem of the old algorithm not meeting the spec?
> 
> > However in the old (default) timing algorithm before the commit
> > be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"),
> > tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new
> > setting "default-adjust-timing" for using the old (default) timing
> > algorithm."
> 
> What I still do not get: the old algorithm was able to handle clock
> stretching. Why can't you update the new one to handle clock stretching
> as well. I might be missing something, but what is it?

I am still interested. Especially in the last question. Is the last
question clear to you? I can explain some more otherwise.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-11-29 12:49         ` Wolfram Sang
@ 2021-12-18  9:44           ` Kewei Xu
  2021-12-18 10:17             ` Wolfram Sang
  0 siblings, 1 reply; 20+ messages in thread
From: Kewei Xu @ 2021-12-18  9:44 UTC (permalink / raw)
  To: Wolfram Sang, matthias.bgg, robh+dt, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel, linux-mediatek, srv_heupstream,
	leilk.liu, qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu,
	yuhan.wei

On Mon, 2021-11-29 at 13:49 +0100, Wolfram Sang wrote:
> > > stretching. But if the slave device stretch the SCL line for too
> > > long
> > > time, our design still cannot make tSU,STA/tHD,STA/tSU,STO meet
> > > spec.
> > 
> > Isn't the new algorithm broken if it cannot support clock
> > stretching?
> > What was the problem of the old algorithm not meeting the spec?
> > 
> > > However in the old (default) timing algorithm before the commit
> > > be5ce0e97cc7 ("i2c: mediatek: Add i2c ac-timing adjust support"),
> > > tSU,STA/tHD,STA/tSU,STO can meet spec. So we want to define a new
> > > setting "default-adjust-timing" for using the old (default)
> > > timing
> > > algorithm."
> > 
> > What I still do not get: the old algorithm was able to handle clock
> > stretching. Why can't you update the new one to handle clock
> > stretching
> > as well. I might be missing something, but what is it?
> 
> I am still interested. Especially in the last question. Is the last
> question clear to you? I can explain some more otherwise.
> 
Hi,Wolfram,

I'm very sorry that I didn't reply to your information in time due
to my many personal affairs.

The Old algorithm was designed to focus only on normal functions, and
need to add additional custom code to adjust ac-timing when the
communication timing did not meet the specifications. so when there is
no clock stretch, ac-timing does not meet the spec, but the function is
always normal.

The new algorithm(The commit patch: be5ce0e97cc7 ("i2c: mediatek: Add
i2c ac-timing adjust support") is based on the requirements of i2c spec
to calculate the hardware-related settings so that the function and ac-
timing are normal When there is no clock stretch or the clock stretch
time is short. When the stretching time is very long (>60us), i2c ac-
timing does not meet the specifications and causes function abnormal.

In order to make the i2c function normal, this patch was submitted,
that is, when the stretch is long, the old algorithm is used to ensure
the function is normal, and when the stretch is short, the new
algorithm is used to ensure that the ac-timing and function are normal.

We found that when the ac-timing calculation formula is updated, the
new algorithm can make i2c ac-timing meet the spec and function
normally. So we plan to replace this patch with a patch that updates
the calculation formula.

Thanks~
Kewei

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices
  2021-12-18  9:44           ` Kewei Xu
@ 2021-12-18 10:17             ` Wolfram Sang
  0 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2021-12-18 10:17 UTC (permalink / raw)
  To: Kewei Xu
  Cc: matthias.bgg, robh+dt, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel, linux-mediatek, srv_heupstream, leilk.liu,
	qii.wang, liguo.zhang, caiyu.chen, ot_daolong.zhu, yuhan.wei

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

Hi Kewei!

> I'm very sorry that I didn't reply to your information in time due
> to my many personal affairs.

No worries, there is no pressure from my side. I hope you are all well
now!

> We found that when the ac-timing calculation formula is updated, the
> new algorithm can make i2c ac-timing meet the spec and function
> normally. So we plan to replace this patch with a patch that updates
> the calculation formula.

Cool! I'm glad this is possible.

Looking forward to the new patch,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-12-18 10:17 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 10:14 [PATCH v7 0/7] Introducing an attribute to select the time setting Kewei Xu
2021-09-17 10:14 ` [PATCH v7 1/7] i2c: mediatek: fixing the incorrect register offset Kewei Xu
2021-10-02  6:28   ` Wolfram Sang
2021-09-17 10:14 ` [PATCH v7 2/7] i2c: mediatek: Reset the handshake signal between i2c and dma Kewei Xu
2021-10-02  6:30   ` Wolfram Sang
2021-10-08  6:19     ` Kewei Xu
2021-09-17 10:14 ` [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs Kewei Xu
2021-10-02  6:37   ` Wolfram Sang
2021-10-08  7:13     ` Kewei Xu
2021-09-17 10:14 ` [PATCH v7 4/7] dt-bindings: i2c: add attribute use-default-timing Kewei Xu
2021-09-17 10:14 ` [PATCH v7 5/7] i2c: mediatek: Add OFFSET_EXT_CONF setting back Kewei Xu
2021-10-02  6:40   ` Wolfram Sang
2021-09-17 10:14 ` [PATCH v7 6/7] i2c: mediatek: Isolate speed setting via dts for special devices Kewei Xu
2021-10-02  6:40   ` Wolfram Sang
2021-10-08  8:47     ` Kewei Xu
2021-10-11 10:56       ` Wolfram Sang
2021-11-29 12:49         ` Wolfram Sang
2021-12-18  9:44           ` Kewei Xu
2021-12-18 10:17             ` Wolfram Sang
2021-09-17 10:14 ` [PATCH v7 7/7] i2c: mediatek: modify bus speed calculation formula Kewei Xu

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).