[PATCH] i2c: hix5hd2: Add some debug enhancement for register access

[PATCH] i2c: hix5hd2: Add some debug enhancement for register access

[Tao Lan]

From: taolan <taolan@huawei.com>

Sometimes, to locate a fault, we need to know how the register is
configured and whether the configuration is incorrect. Currently, no
better method is available for analysis.

This patch tries to solve the problem by naming the registers and
printing the accessed values.

Signed-off-by: taolan <taolan@huawei.com>
---
 drivers/i2c/busses/i2c-hix5hd2.c | 80 +++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-hix5hd2.c b/drivers/i2c/busses/i2c-hix5hd2.c
index 61ae58f57047..9ebbad8701f4 100644
--- a/drivers/i2c/busses/i2c-hix5hd2.c
+++ b/drivers/i2c/busses/i2c-hix5hd2.c
@@ -92,28 +92,72 @@ struct hix5hd2_i2c_priv {
 	enum hix5hd2_i2c_state state;
 };
 
+#define i2c_readl readl_relaxed
+#define i2c_writel writel_relaxed
+
+#ifdef I2C_REG_DUMP
+
+#undef i2c_readl
+#undef i2c_writel
+
+struct i2c_reg_name {
+	u32 reg;
+	char *name;
+} g_i2c_reg_name[] = {
+	{0x00, "CTRL"},
+	{0x04, "COM"},
+	{0x08, "ICR"},
+	{0x0c, "SR"},
+	{0x10, "SCL_H"},
+	{0x14, "SCL_L"},
+	{0x18, "TXR"},
+	{0x1c, "RXR"},
+};
+static void i2c_writel(u32 val, void *reg)
+{
+	unsigned long idx = (unsigned long)reg & 0x00FF;
+
+	if (idx <= 0x1C)
+		pr_notice("write 0x%04x to %s\n", val, g_i2c_reg_name[idx >> 0x2].name);
+
+	i2c_writel(val, reg);
+}
+
+static u32 i2c_readl(void *reg)
+{
+	u32 val = i2c_readl(reg);
+	unsigned long idx = (unsigned long)reg & 0x00FF;
+
+	if (idx <= 0x1C)
+		pr_notice("read 0x%04x from %s\n", val, g_i2c_reg_name[idx >> 0x2].name);
+
+	return val;
+}
+#endif
+
+
 static u32 hix5hd2_i2c_clr_pend_irq(struct hix5hd2_i2c_priv *priv)
 {
-	u32 val = readl_relaxed(priv->regs + HIX5I2C_SR);
+	u32 val = i2c_readl(priv->regs + HIX5I2C_SR);
 
-	writel_relaxed(val, priv->regs + HIX5I2C_ICR);
+	i2c_writel(val, priv->regs + HIX5I2C_ICR);
 
 	return val;
 }
 
 static void hix5hd2_i2c_clr_all_irq(struct hix5hd2_i2c_priv *priv)
 {
-	writel_relaxed(I2C_CLEAR_ALL, priv->regs + HIX5I2C_ICR);
+	i2c_writel(I2C_CLEAR_ALL, priv->regs + HIX5I2C_ICR);
 }
 
 static void hix5hd2_i2c_disable_irq(struct hix5hd2_i2c_priv *priv)
 {
-	writel_relaxed(0, priv->regs + HIX5I2C_CTRL);
+	i2c_writel(0, priv->regs + HIX5I2C_CTRL);
 }
 
 static void hix5hd2_i2c_enable_irq(struct hix5hd2_i2c_priv *priv)
 {
-	writel_relaxed(I2C_ENABLE | I2C_UNMASK_TOTAL | I2C_UNMASK_ALL,
+	i2c_writel(I2C_ENABLE | I2C_UNMASK_TOTAL | I2C_UNMASK_ALL,
 		       priv->regs + HIX5I2C_CTRL);
 }
 
@@ -123,17 +167,17 @@ static void hix5hd2_i2c_drv_setrate(struct hix5hd2_i2c_priv *priv)
 	u32 scl, sysclock;
 
 	/* close all i2c interrupt */
-	val = readl_relaxed(priv->regs + HIX5I2C_CTRL);
-	writel_relaxed(val & (~I2C_UNMASK_TOTAL), priv->regs + HIX5I2C_CTRL);
+	val = i2c_readl(priv->regs + HIX5I2C_CTRL);
+	i2c_writel(val & (~I2C_UNMASK_TOTAL), priv->regs + HIX5I2C_CTRL);
 
 	rate = priv->freq;
 	sysclock = clk_get_rate(priv->clk);
 	scl = (sysclock / (rate * 2)) / 2 - 1;
-	writel_relaxed(scl, priv->regs + HIX5I2C_SCL_H);
-	writel_relaxed(scl, priv->regs + HIX5I2C_SCL_L);
+	i2c_writel(scl, priv->regs + HIX5I2C_SCL_H);
+	i2c_writel(scl, priv->regs + HIX5I2C_SCL_L);
 
 	/* restore original interrupt*/
-	writel_relaxed(val, priv->regs + HIX5I2C_CTRL);
+	i2c_writel(val, priv->regs + HIX5I2C_CTRL);
 
 	dev_dbg(priv->dev, "%s: sysclock=%d, rate=%d, scl=%d\n",
 		__func__, sysclock, rate, scl);
@@ -188,7 +232,7 @@ static void hix5hd2_rw_handle_stop(struct hix5hd2_i2c_priv *priv)
 {
 	if (priv->stop) {
 		priv->state = HIX5I2C_STAT_SND_STOP;
-		writel_relaxed(I2C_STOP, priv->regs + HIX5I2C_COM);
+		i2c_writel(I2C_STOP, priv->regs + HIX5I2C_COM);
 	} else {
 		hix5hd2_rw_over(priv);
 	}
@@ -198,10 +242,10 @@ static void hix5hd2_read_handle(struct hix5hd2_i2c_priv *priv)
 {
 	if (priv->msg_len == 1) {
 		/* the last byte don't need send ACK */
-		writel_relaxed(I2C_READ | I2C_NO_ACK, priv->regs + HIX5I2C_COM);
+		i2c_writel(I2C_READ | I2C_NO_ACK, priv->regs + HIX5I2C_COM);
 	} else if (priv->msg_len > 1) {
 		/* if i2c master receive data will send ACK */
-		writel_relaxed(I2C_READ, priv->regs + HIX5I2C_COM);
+		i2c_writel(I2C_READ, priv->regs + HIX5I2C_COM);
 	} else {
 		hix5hd2_rw_handle_stop(priv);
 	}
@@ -213,8 +257,8 @@ static void hix5hd2_write_handle(struct hix5hd2_i2c_priv *priv)
 
 	if (priv->msg_len > 0) {
 		data = priv->msg->buf[priv->msg_idx++];
-		writel_relaxed(data, priv->regs + HIX5I2C_TXR);
-		writel_relaxed(I2C_WRITE, priv->regs + HIX5I2C_COM);
+		i2c_writel(data, priv->regs + HIX5I2C_TXR);
+		i2c_writel(I2C_WRITE, priv->regs + HIX5I2C_COM);
 	} else {
 		hix5hd2_rw_handle_stop(priv);
 	}
@@ -228,7 +272,7 @@ static int hix5hd2_rw_preprocess(struct hix5hd2_i2c_priv *priv)
 		priv->state = HIX5I2C_STAT_RW;
 	} else if (priv->state == HIX5I2C_STAT_RW) {
 		if (priv->msg->flags & I2C_M_RD) {
-			data = readl_relaxed(priv->regs + HIX5I2C_RXR);
+			data = i2c_readl(priv->regs + HIX5I2C_RXR);
 			priv->msg->buf[priv->msg_idx++] = data;
 		}
 		priv->msg_len--;
@@ -304,10 +348,10 @@ static void hix5hd2_i2c_message_start(struct hix5hd2_i2c_priv *priv, int stop)
 	hix5hd2_i2c_clr_all_irq(priv);
 	hix5hd2_i2c_enable_irq(priv);
 
-	writel_relaxed(i2c_8bit_addr_from_msg(priv->msg),
+	i2c_writel(i2c_8bit_addr_from_msg(priv->msg),
 		       priv->regs + HIX5I2C_TXR);
 
-	writel_relaxed(I2C_WRITE | I2C_START, priv->regs + HIX5I2C_COM);
+	i2c_writel(I2C_WRITE | I2C_START, priv->regs + HIX5I2C_COM);
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
-- 
2.17.1

Re: [PATCH] i2c: hix5hd2: Add some debug enhancement for register access

[Wolfram Sang]

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

On Tue, Sep 13, 2022 at 12:48:20PM +0000, Tao Lan wrote:
> From: taolan <taolan@huawei.com>
> 
> Sometimes, to locate a fault, we need to know how the register is
> configured and whether the configuration is incorrect. Currently, no
> better method is available for analysis.
> 
> This patch tries to solve the problem by naming the registers and
> printing the accessed values.
> 
> Signed-off-by: taolan <taolan@huawei.com>

Such debug helpers are not suitable for upstream because they need quite
some resources for rare cases...

> +		pr_notice("write 0x%04x to %s\n", val, g_i2c_reg_name[idx >> 0x2].name);

... and using this loglevel you will flood logs of users.



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

Re: [PATCH] i2c: hix5hd2: Add some debug enhancement for register access

[taolan]

 

-------- Original Message --------
Title: Re: [PATCH] i2c: hix5hd2: Add some debug enhancement for register access
From: wsa@kernel.org
To: taolan@huawei.com; Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Date: 2022/9/18 04:29
> On Tue, Sep 13, 2022 at 12:48:20PM +0000, Tao Lan wrote:
>> From: taolan <taolan@huawei.com>
>>
>> Sometimes, to locate a fault, we need to know how the register is
>> configured and whether the configuration is incorrect. Currently, no
>> better method is available for analysis.
>>
>> This patch tries to solve the problem by naming the registers and
>> printing the accessed values.
>>
>> Signed-off-by: taolan <taolan@huawei.com>
> 
> Such debug helpers are not suitable for upstream because they need quite
> some resources for rare cases...
Yeah, how about writing to proc as an event, or any suggestions？
> 
>> +		pr_notice("write 0x%04x to %s\n", val, g_i2c_reg_name[idx >> 0x2].name);
> 
> ... and using this loglevel you will flood logs of users.
The loglevel can be adjusted to DEBUG level,and this debug can be off by default. is this ok?
> 
>