Re: [PATCH 1/4] i2c: tegra: make sure register writes completes

2012-06-12 Thread Wolfram Sang 
On Tue, Jun 05, 2012 at 06:39:57PM +0530, 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.
 
 Signed-off-by: Laxman Dewangan ldewan...@nvidia.com
 ---
  drivers/i2c/busses/i2c-tegra.c |   13 +
  1 files changed, 13 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
 index 8b2e555..fa92396 100644
 --- a/drivers/i2c/busses/i2c-tegra.c
 +++ b/drivers/i2c/busses/i2c-tegra.c
 @@ -430,6 +430,13 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
   if (i2c_dev-is_dvc)
   dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
  
 + /*
 +  * Register write get queued in the PPSB bus and write can
 +  * happen later. Read back register to make sure that register
 +  * write is completed.
 +  */
 + i2c_readl(i2c_dev, I2C_INT_STATUS);

Does it make sense to put the read into i2c_writel?

 +
   if (status  I2C_INT_PACKET_XFER_COMPLETE) {
   BUG_ON(i2c_dev-msg_buf_remaining);
   complete(i2c_dev-msg_complete);
 @@ -444,6 +451,9 @@ err:
   if (i2c_dev-is_dvc)
   dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
  
 + /* Read back register to make sure that register writes completed */
 + i2c_readl(i2c_dev, I2C_INT_STATUS);
 +
   complete(i2c_dev-msg_complete);
   return IRQ_HANDLED;
  }
 @@ -505,6 +515,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev 
 *i2c_dev,
   ret = wait_for_completion_timeout(i2c_dev-msg_complete, 
 TEGRA_I2C_TIMEOUT);
   tegra_i2c_mask_irq(i2c_dev, int_mask);
  
 + /* Read back register to make sure that register writes completed */
 + i2c_readl(i2c_dev, I2C_INT_MASK);
 +

It definately makes sense to put this read into tegra_i2c_mask_irq()?

   if (WARN_ON(ret == 0)) {
   dev_err(i2c_dev-dev, i2c transfer timed out\n);

Regards,

   Wolfram

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 1/4] i2c: tegra: make sure register writes completes

2012-06-12 Thread Laxman Dewangan 

On Tuesday 12 June 2012 01:24 PM, Wolfram Sang wrote:

* PGP Signed by an unknown key

On Tue, Jun 05, 2012 at 06:39:57PM +0530, Laxman Dewangan wrote:

@@ -430,6 +430,13 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
if (i2c_dev-is_dvc)
dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);

+   /*
+* Register write get queued in the PPSB bus and write can
+* happen later. Read back register to make sure that register
+* write is completed.
+*/
+   i2c_readl(i2c_dev, I2C_INT_STATUS);

Does it make sense to put the read into i2c_writel?

We can not put in i2c_writel() as we also do fifo write using this and 
writing and reading back fifo can drainout the fifo.

hence putting this here seems more appropriate.

+
if (status  I2C_INT_PACKET_XFER_COMPLETE) {
BUG_ON(i2c_dev-msg_buf_remaining);
complete(i2c_dev-msg_complete);
@@ -444,6 +451,9 @@ err:
if (i2c_dev-is_dvc)
dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);

+   /* Read back register to make sure that register writes completed */
+   i2c_readl(i2c_dev, I2C_INT_STATUS);
+
complete(i2c_dev-msg_complete);
return IRQ_HANDLED;
  }
@@ -505,6 +515,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
ret = wait_for_completion_timeout(i2c_dev-msg_complete, 
TEGRA_I2C_TIMEOUT);
tegra_i2c_mask_irq(i2c_dev, int_mask);

+   /* Read back register to make sure that register writes completed */
+   i2c_readl(i2c_dev, I2C_INT_MASK);
+

It definately makes sense to put this read into tegra_i2c_mask_irq()?

Ok, fine with me. I put the read back logic inside mask_irq() and 
unmask_irq().

Will send the fix in next patch.

if (WARN_ON(ret == 0)) {
dev_err(i2c_dev-dev, i2c transfer timed out\n);

Regards,

Wolfram



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

Re: [PATCH 1/4] i2c: tegra: make sure register writes completes

2012-06-12 Thread Stephen Warren 
On 06/12/2012 04:16 AM, Laxman Dewangan wrote:
 On Tuesday 12 June 2012 01:24 PM, Wolfram Sang wrote:
 * PGP Signed by an unknown key

 On Tue, Jun 05, 2012 at 06:39:57PM +0530, Laxman Dewangan wrote:
 @@ -430,6 +430,13 @@ static irqreturn_t tegra_i2c_isr(int irq, void
 *dev_id)
   if (i2c_dev-is_dvc)
   dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);

 +/*
 + * Register write get queued in the PPSB bus and write can
 + * happen later. Read back register to make sure that register
 + * write is completed.
 + */
 +i2c_readl(i2c_dev, I2C_INT_STATUS);

 Does it make sense to put the read into i2c_writel?

 We can not put in i2c_writel() as we also do fifo write using this and
 writing and reading back fifo can drainout the fifo.
 hence putting this here seems more appropriate.

You can put it inside i2c_writel(), but make the read-back conditional
depending on which register was just written. The Tegra SD driver has
similar conditional code in readl/writel to WAR some HW quirks.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html