On Mon, Apr 18, 2016 at 06:45:55PM +0530, Shardar Shariff Md wrote: > 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 since v1: > - Align the commit message to 72 characters per line. > - Removing unnecessary paranthesis. > - 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.
I think I didn't express myself very clearly, or gave insufficient
information. The changelog reads easier if you have one entry per
revision. The above would then read:
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.
Changes in v2:
- Align the commit message to 72 characters per line.
- Removing unnecessary paranthesis.
And it should go below the --- separator.
One more nitpick below.
> ---
> 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 6235f16..7922892 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -191,6 +191,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)
> @@ -510,14 +511,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);
> +}
Since you no longer use the return value you might as well just make the
function return void. I don't feel very strongly about it, though, so
I'll defer to Wolfram.
Thierry
signature.asc
Description: PGP signature
