Disable clock before returning error in tegra_i2c_init()
Signed-off-by: Shardar Shariff Md <[email protected]>
---
Changes in v5:
- Move current patch as separate patch
Changes in v6:
- Remove unnecessary line
---
drivers/i2c/busses/i2c-tegra.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d764d64..445398c3 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -483,19 +483,20 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (time_after(jiffies, timeout)) {
dev_warn(i2c_dev->dev,
"timeout waiting for config load\n");
- return -ETIMEDOUT;
+ err = -ETIMEDOUT;
+ goto err;
}
msleep(1);
}
}
- tegra_i2c_clock_disable(i2c_dev);
-
if (i2c_dev->irq_disabled) {
i2c_dev->irq_disabled = 0;
enable_irq(i2c_dev->irq);
}
+err:
+ tegra_i2c_clock_disable(i2c_dev);
return err;
}
--
1.8.1.5
Instead of calculating timeout for the config load during init,
calculate it after config load register is written by using
readx_poll_timeout().
Signed-off-by: Shardar Shariff Md <[email protected]>
---
Changes in v4:
- Split timeout calculation to separate patch
Changes in v5:
- Move disabling of clock to separate patch
---
drivers/i2c/busses/i2c-tegra.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 445398c3..c1b02c7 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,17 @@ 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;
+
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 +
+ tegra_i2c_reg_addr(i2c_dev,
+ I2C_CONFIG_LOAD), 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
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 <[email protected]>
---
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 8d49995..1181bbf 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)
@@ -514,14 +515,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),
@@ -537,6 +551,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)
@@ -566,7 +581,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 |
@@ -577,6 +592,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;
}
@@ -586,6 +603,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);
@@ -598,6 +616,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) |
@@ -627,14 +650,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
Define separate function for configuration load register handling
to make it use by different functions later.
Signed-off-by: Shardar Shariff Md <[email protected]>
---
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 | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c1b02c7..8d49995 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -426,6 +426,27 @@ 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)
+{
+ if (i2c_dev->hw->has_config_load_reg) {
+ u32 val;
+ int err;
+
+ i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
+ err = readx_poll_timeout(readl, i2c_dev->base +
+ tegra_i2c_reg_addr(i2c_dev,
+ I2C_CONFIG_LOAD), 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,20 +500,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;
-
- i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- err = readx_poll_timeout(readl, i2c_dev->base +
- tegra_i2c_reg_addr(i2c_dev,
- I2C_CONFIG_LOAD), 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
On 25/04/16 12:57, Shardar Shariff Md wrote:
> Disable clock before returning error in tegra_i2c_init()
May be explain why we should do this? It looks like currently we will
leave the clock on in the case of an error and so we may never turn it
off again as now we have an unbalanced enable/disable, etc.
> Signed-off-by: Shardar Shariff Md <[email protected]>
>
> ---
> Changes in v5:
> - Move current patch as separate patch
>
> Changes in v6:
> - Remove unnecessary line
> ---
> drivers/i2c/busses/i2c-tegra.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d764d64..445398c3 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -483,19 +483,20 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> if (time_after(jiffies, timeout)) {
> dev_warn(i2c_dev->dev,
> "timeout waiting for config load\n");
> - return -ETIMEDOUT;
> + err = -ETIMEDOUT;
> + goto err;
> }
> msleep(1);
> }
> }
>
> - tegra_i2c_clock_disable(i2c_dev);
> -
> if (i2c_dev->irq_disabled) {
> i2c_dev->irq_disabled = 0;
> enable_irq(i2c_dev->irq);
> }
>
> +err:
> + tegra_i2c_clock_disable(i2c_dev);
Thierry, Stephen, thinking about this some more I wonder if we should
not change the order here, in case of a potential race between an
interrupt and disabling the clock. May be it is safer to disable the
clock first? However, this is in the init and so may be that is not likely?
Cheers
Jon