[PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Shardar Shariff Md]

After CONFIG_LOAD register programing instead of explicitly waiting for
timeout, use readx_poll_timeout() to check for register value to get
updated or wait till timeout.

Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>

---
Changes in v4:
- Split timeout calculation to separate patch

Changes in v5:
- Move disabling of clock to separate patch

Changes in v8:
- 1st change of [PATCH v7] series is merged, v8 is rebased on top of
  merged change, here patch series is changed accordingly.
- Updated the commit message as per review to properly reflect change.
- calculate the register offset seperately to make code more readable.
---
 drivers/i2c/busses/i2c-tegra.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 445398c3..7cbeb4e 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -28,6 +28,7 @@
 #include <linux/of_device.h>
 #include <linux/module.h>
 #include <linux/reset.h>
+#include <linux/iopoll.h>
 
 #include <asm/unaligned.h>
 
@@ -110,6 +111,8 @@
 #define I2C_CLKEN_OVERRIDE			0x090
 #define I2C_MST_CORE_CLKEN_OVR			(1 << 0)
 
+#define I2C_CONFIG_LOAD_TIMEOUT			1000000
+
 /*
  * msg_end_type: The bus control which need to be send at end of transfer.
  * @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -428,7 +431,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	u32 val;
 	int err = 0;
 	u32 clk_divisor;
-	unsigned long timeout = jiffies + HZ;
 
 	err = tegra_i2c_clock_enable(i2c_dev);
 	if (err < 0) {
@@ -478,15 +480,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
 
 	if (i2c_dev->hw->has_config_load_reg) {
+		u32 val;
+		unsigned long reg_offset;
+
+		reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
 		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
-		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
-			if (time_after(jiffies, timeout)) {
-				dev_warn(i2c_dev->dev,
-					"timeout waiting for config load\n");
-				err = -ETIMEDOUT;
-				goto err;
-			}
-			msleep(1);
+		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
+					 val, val == 0, 1000,
+					 I2C_CONFIG_LOAD_TIMEOUT);
+		if (err) {
+			dev_warn(i2c_dev->dev,
+				 "timeout waiting for config load\n");
+			goto err;
 		}
 	}
 
-- 
1.8.1.5

[PATCH v8 2/3] i2c: tegra: add separate function for config_load programing

[Shardar Shariff Md]

Define separate function for configuration load register handling
to make it use by different functions later.

Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>

---
Changes in v2:
- Remove unnecessary paranthesis and align to 80 characters per line

Changes in v3:
- Add separate function for config load handling

Changes in v4:
- Move timeout calculation to separate patch
---
 drivers/i2c/busses/i2c-tegra.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7cbeb4e..4e30f86 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -426,6 +426,28 @@ static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev)
 		clk_disable(i2c_dev->fast_clk);
 }
 
+static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned long reg_offset;
+	u32 val;
+	int err;
+
+	if (i2c_dev->hw->has_config_load_reg) {
+		reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
+		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
+					 val, val == 0, 1000,
+					 I2C_CONFIG_LOAD_TIMEOUT);
+		if (err) {
+			dev_warn(i2c_dev->dev,
+				 "timeout waiting for config load\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val;
@@ -479,21 +501,9 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
 		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
 
-	if (i2c_dev->hw->has_config_load_reg) {
-		u32 val;
-		unsigned long reg_offset;
-
-		reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
-		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
-		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
-					 val, val == 0, 1000,
-					 I2C_CONFIG_LOAD_TIMEOUT);
-		if (err) {
-			dev_warn(i2c_dev->dev,
-				 "timeout waiting for config load\n");
-			goto err;
-		}
-	}
+	err = tegra_i2c_wait_for_config_load(i2c_dev);
+	if (err)
+		goto err;
 
 	if (i2c_dev->irq_disabled) {
 		i2c_dev->irq_disabled = 0;
-- 
1.8.1.5

[PATCH v8 3/3] i2c: tegra: proper handling of error cases

[Shardar Shariff Md]

To summarize the issue observed in error cases:

SW Flow: For i2c message transfer, packet header and data payload is
posted and then required error/packet completion interrupts are enabled
later.

HW flow: HW process the packet just after packet header is posted, if
ARB lost/NACK error occurs (SW will not handle immediately when error
happens as error interrupts are not enabled at this point). HW assumes
error is acknowledged and clears current data in FIFO, But SW here posts
the remaining data payload which still stays in FIFO as stale data
(data without packet header).

Now once the interrupts are enabled, SW handles ARB lost/NACK error by
clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
the error and will parse/process stale data (data without packet header)
present in FIFO which causes invalid NACK errors.

Fix: Enable the error interrupts before posting the packet into FIFO
which make sure HW to not clear the fifo. Also disable the packet mode
before acknowledging errors (ARB lost/NACK error) to not process any
stale data. As error interrupts are enabled before posting the packet
header use spinlock to avoid preempting.

Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>

---
Changes in v2:
- Align the commit message to 72 characters per line.
- Removing unnecessary paranthesis.
- Handle error in isr

Changes in v3:
- Printing error if tegra_i2c_disable_packet_mode() fails
  is already present and handling error is not taken cared
  in ISR which was done in v2 but keeping return error in
  *wait_for_config_load() as its used in tegra_i2c_init()
---
 drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 4e30f86..3e7d2b7 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -194,6 +194,7 @@ struct tegra_i2c_dev {
 	u16 clk_divisor_non_hs_mode;
 	bool is_suspended;
 	bool is_multimaster_mode;
+	spinlock_t xfer_lock;
 };
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -515,14 +516,27 @@ err:
 	return err;
 }
 
+static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 cnfg;
+
+	cnfg = i2c_readl(i2c_dev, I2C_CNFG);
+	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
+		i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG);
+
+	return tegra_i2c_wait_for_config_load(i2c_dev);
+}
+
 static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 {
 	u32 status;
 	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	struct tegra_i2c_dev *i2c_dev = dev_id;
+	unsigned long flags;
 
 	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
+	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
 	if (status == 0) {
 		dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
 			 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
@@ -538,6 +552,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	}
 
 	if (unlikely(status & status_err)) {
+		tegra_i2c_disable_packet_mode(i2c_dev);
 		if (status & I2C_INT_NO_ACK)
 			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
 		if (status & I2C_INT_ARBITRATION_LOST)
@@ -567,7 +582,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 		BUG_ON(i2c_dev->msg_buf_remaining);
 		complete(&i2c_dev->msg_complete);
 	}
-	return IRQ_HANDLED;
+	goto done;
 err:
 	/* An error occurred, mask all interrupts */
 	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
@@ -578,6 +593,8 @@ err:
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
 	complete(&i2c_dev->msg_complete);
+done:
+	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
 	return IRQ_HANDLED;
 }
 
@@ -587,6 +604,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	u32 packet_header;
 	u32 int_mask;
 	unsigned long time_left;
+	unsigned long flags;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -599,6 +617,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
 	reinit_completion(&i2c_dev->msg_complete);
 
+	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
+
+	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	tegra_i2c_unmask_irq(i2c_dev, int_mask);
+
 	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
 			PACKET_HEADER0_PROTOCOL_I2C |
 			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
@@ -628,14 +651,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (!(msg->flags & I2C_M_RD))
 		tegra_i2c_fill_tx_fifo(i2c_dev);
 
-	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
 		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
 	if (msg->flags & I2C_M_RD)
 		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
 	else if (i2c_dev->msg_buf_remaining)
 		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
+	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
 	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
 		i2c_readl(i2c_dev, I2C_INT_MASK));
 
-- 
1.8.1.5

Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Wolfram Sang]

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


> +		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
> +					 val, val == 0, 1000,
> +					 I2C_CONFIG_LOAD_TIMEOUT);

Any reason we can't use readl_poll_timeout() here?


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

RE: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Shardar Mohammed]

> Subject: Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after
> config_load reg programmed
> 
> * PGP Signed by an unknown key
> 
> 
> > +		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
> > +					 val, val == 0, 1000,
> > +					 I2C_CONFIG_LOAD_TIMEOUT);
> 
> Any reason we can't use readl_poll_timeout() here?
[Shardar] Can be used, will change and push the new patchset.

> 
> 
> * Unknown Key
> * 0x14A029B6

Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Wolfram Sang]

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


> > > +		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
> > > +					 val, val == 0, 1000,
> > > +					 I2C_CONFIG_LOAD_TIMEOUT);
> > 
> > Any reason we can't use readl_poll_timeout() here?
> [Shardar] Can be used, will change and push the new patchset.

Still interested? Should be an easy change, but I am reluctant to fix it
here with no testing.


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

RE: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Shardar Mohammed]

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Friday, August 26, 2016 1:10 AM
> To: Shardar Mohammed
> Cc: Laxman Dewangan; swarren@wwwdotorg.org;
> thierry.reding@gmail.com; gnurou@gmail.com; linux-i2c@vger.kernel.org;
> linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after
> config_load reg programmed
> 
> * PGP Signed by an unknown key
> 
> 
> > > > +		err = readx_poll_timeout(readl, i2c_dev->base + reg_offset,
> > > > +					 val, val == 0, 1000,
> > > > +					 I2C_CONFIG_LOAD_TIMEOUT);
> > >
> > > Any reason we can't use readl_poll_timeout() here?
> > [Shardar] Can be used, will change and push the new patchset.
> 
> Still interested? Should be an easy change, but I am reluctant to fix it here
> with no testing.
[Shardar] Will push the change by tomorrow.
> 
> 
> * Unknown Key
> * 0x14A029B6

Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Wolfram Sang]

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

> > Still interested? Should be an easy change, but I am reluctant to fix it here
> > with no testing.
> [Shardar] Will push the change by tomorrow.

Thanks. If possibe, could you rebase it on top of this series?

[PATCH V2 0/9] Some Tegra I2C Updates

Or is it easier to rebase Jon's series on top of yours?

CCing Jon, too...


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

RE: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed

[Shardar Mohammed]

Hi Wolfram Sang,

I have pushed below changes:
PATCH v9 series with only taking care of review comment.

PATCH v10 series with rebasing on top of Jon's patch series  [PATCH V2 0/9] Some Tegra I2C Updates.

Please review.

Thanks,
Shardar

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Friday, August 26, 2016 9:24 PM
> To: Shardar Mohammed
> Cc: Laxman Dewangan; swarren@wwwdotorg.org;
> thierry.reding@gmail.com; gnurou@gmail.com; linux-i2c@vger.kernel.org;
> linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan Hunter
> Subject: Re: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after
> config_load reg programmed
> 
> * PGP Signed by an unknown key
> 
> > > Still interested? Should be an easy change, but I am reluctant to
> > > fix it here with no testing.
> > [Shardar] Will push the change by tomorrow.
> 
> Thanks. If possibe, could you rebase it on top of this series?
> 
> [PATCH V2 0/9] Some Tegra I2C Updates
> 
> Or is it easier to rebase Jon's series on top of yours?
> 
> CCing Jon, too...
> 
> 
> * Unknown Key
> * 0x14A029B6