[V3,1/4] i2c: tegra: make sure register writes completes

The Tegra PPSB (an peripheral bus) queues writes transactions.
In order to guarantee that writes have completed before a
certain time, a read transaction to a register on the same
bus must be executed.
This is necessary in situations such as when clearing an
interrupt status or enable, so that when returning from an
interrupt handler, the HW has already de-asserted its
interrupt status output, which will avoid spurious interrupts.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- Using the readback for mask/unmask of irq also.

Changes from V2:
- Read back is done whenever write happen in i2c register other than TX FIFO.

 drivers/i2c/busses/i2c-tegra.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
> The Tegra PPSB (an peripheral bus) queues writes transactions.
> In order to guarantee that writes have completed before a
> certain time, a read transaction to a register on the same
> bus must be executed.
> This is necessary in situations such as when clearing an
> interrupt status or enable, so that when returning from an
> interrupt handler, the HW has already de-asserted its
> interrupt status output, which will avoid spurious interrupts.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c

> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>  	unsigned long reg)
>  {
>  	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +
> +	/* Read back register to make sure that register writes completed */
> +	if (reg != I2C_TX_FIFO)
> +		readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));

I guess that's fine, but it sure does seem rather heavy-weight. Don't
you only need to do the readback if you just wrote to the IRQ status or
mask registers, rather than if you wrote to /any/ register other than
the FIFO?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
> On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev 
> *i2c_dev, u32 val,
>>   	unsigned long reg)
>>   {
>>   	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> +
>> +	/* Read back register to make sure that register writes completed */
>> +	if (reg != I2C_TX_FIFO)
>> +		readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> I guess that's fine, but it sure does seem rather heavy-weight. Don't
> you only need to do the readback if you just wrote to the IRQ status or
> mask registers, rather than if you wrote to /any/ register other than
> the FIFO?

That's what my second patch but based on your earlier review comment, I 
did for every register.

I think it will not matter much as we dont write all register with every 
transaction, only during initialization.
Then for each transfer we write manly on Tx fifo and interrupt 
mask/status register and hence not too much overweight.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/14/2012 06:35 AM, Laxman Dewangan wrote:
> On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
>> On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
>> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev
>> *i2c_dev, u32 val,
>>>       unsigned long reg)
>>>   {
>>>       writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>> +
>>> +    /* Read back register to make sure that register writes
>>> completed */
>>> +    if (reg != I2C_TX_FIFO)
>>> +        readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> I guess that's fine, but it sure does seem rather heavy-weight. Don't
>> you only need to do the readback if you just wrote to the IRQ status or
>> mask registers, rather than if you wrote to /any/ register other than
>> the FIFO?
> 
> That's what my second patch but based on your earlier review comment, I
> did for every register.

Well, just for the record, in that comment you refer to, I was talking
about the location in the code where the readback should be implemented,
not the set of registers that the readback should happen for. But, it
probably doesn't impact performance. It'd be more self-documenting if
this readback was limited to the specific registers where it was needed
though.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> probably doesn't impact performance. It'd be more self-documenting if
> this readback was limited to the specific registers where it was needed
> though.

I'd think this is easier to maintain in case code gets
rewritten/shuffled around.