diff mbox series

[v7,01/34] i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer

Message ID 20200908224006.25636-2-digetx@gmail.com
State Accepted
Commit 900aed24d3e45353e22f7fc00d6826b87c55761a
Headers show
Series Improvements for Tegra I2C driver | expand

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:39 p.m. UTC
The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies
are not updating if interrupts are disabled. Let's switch to use iopoll
API helpers for register-polling. The iopoll API provides helpers for both
atomic and non-atomic cases.

Note that this patch doesn't fix any known problem because normally FIFO
is flushed at the time of starting a new transfer.

Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Thierry Reding Sept. 17, 2020, 11:10 a.m. UTC | #1
On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko wrote:
> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies

> are not updating if interrupts are disabled. Let's switch to use iopoll

> API helpers for register-polling. The iopoll API provides helpers for both

> atomic and non-atomic cases.

> 

> Note that this patch doesn't fix any known problem because normally FIFO

> is flushed at the time of starting a new transfer.

> 

> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

> ---

>  drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------

>  1 file changed, 16 insertions(+), 9 deletions(-)

> 

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

> index 00d3e4d7a01e..ab88cdd70376 100644

> --- a/drivers/i2c/busses/i2c-tegra.c

> +++ b/drivers/i2c/busses/i2c-tegra.c

> @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)

>  

>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)

>  {

> -	unsigned long timeout = jiffies + HZ;

> -	unsigned int offset;

> -	u32 mask, val;

> +	u32 mask, val, offset, reg_offset;


Is there are reason why we need reg_offset? Seems to me like we could
simplify this, see below.

> +	void __iomem *addr;

> +	int err;

>  

>  	if (i2c_dev->hw->has_mst_fifo) {

>  		mask = I2C_MST_FIFO_CONTROL_TX_FLUSH |

> @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)

>  	val |= mask;

>  	i2c_writel(i2c_dev, val, offset);

>  

> -	while (i2c_readl(i2c_dev, offset) & mask) {

> -		if (time_after(jiffies, timeout)) {

> -			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");

> -			return -ETIMEDOUT;

> -		}

> -		usleep_range(1000, 2000);

> +	reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);

> +	addr = i2c_dev->base + reg_offset;


Why not just:

	offset = tegra_i2c_reg_offset(i2c_dev, offset);
	addr = i2c_dev->base + offset;

or even just:

	addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset);

? That makes the patch much smaller because you don't have to rewrite
the whole variable declaration block and just add the "addr" and "err"
variables while removing "timeout".

Thierry
Dmitry Osipenko Sept. 17, 2020, 2:57 p.m. UTC | #2
17.09.2020 14:10, Thierry Reding пишет:
> On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko wrote:
>> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies
>> are not updating if interrupts are disabled. Let's switch to use iopoll
>> API helpers for register-polling. The iopoll API provides helpers for both
>> atomic and non-atomic cases.
>>
>> Note that this patch doesn't fix any known problem because normally FIFO
>> is flushed at the time of starting a new transfer.
>>
>> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 00d3e4d7a01e..ab88cdd70376 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>>  
>>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>  {
>> -	unsigned long timeout = jiffies + HZ;
>> -	unsigned int offset;
>> -	u32 mask, val;
>> +	u32 mask, val, offset, reg_offset;
> 
> Is there are reason why we need reg_offset? Seems to me like we could
> simplify this, see below.

The reason is that this patch addresses potential problem, hence it's
not a cleanup change and it should be kept minimal.

>> +	void __iomem *addr;
>> +	int err;
>>  
>>  	if (i2c_dev->hw->has_mst_fifo) {
>>  		mask = I2C_MST_FIFO_CONTROL_TX_FLUSH |
>> @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>  	val |= mask;
>>  	i2c_writel(i2c_dev, val, offset);
>>  
>> -	while (i2c_readl(i2c_dev, offset) & mask) {
>> -		if (time_after(jiffies, timeout)) {
>> -			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
>> -			return -ETIMEDOUT;
>> -		}
>> -		usleep_range(1000, 2000);
>> +	reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
>> +	addr = i2c_dev->base + reg_offset;
> 
> Why not just:
> 
> 	offset = tegra_i2c_reg_offset(i2c_dev, offset);
> 	addr = i2c_dev->base + offset;
> 
> or even just:
> 
> 	addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset);
> 
> ? That makes the patch much smaller because you don't have to rewrite
> the whole variable declaration block and just add the "addr" and "err"
> variables while removing "timeout".

There is a further "cleanup" patch named "Factor out register polling
into separate function" that already implements yours suggestion.
Thierry Reding Sept. 21, 2020, 10:18 a.m. UTC | #3
On Wed, 09 Sep 2020 01:39:33 +0300, Dmitry Osipenko wrote:
> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies
> are not updating if interrupts are disabled. Let's switch to use iopoll
> API helpers for register-polling. The iopoll API provides helpers for both
> atomic and non-atomic cases.
> 
> Note that this patch doesn't fix any known problem because normally FIFO
> is flushed at the time of starting a new transfer.
> 
> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)

Tested-by: Thierry Reding <treding@nvidia.com>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 00d3e4d7a01e..ab88cdd70376 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -470,9 +470,9 @@  static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
-	unsigned long timeout = jiffies + HZ;
-	unsigned int offset;
-	u32 mask, val;
+	u32 mask, val, offset, reg_offset;
+	void __iomem *addr;
+	int err;
 
 	if (i2c_dev->hw->has_mst_fifo) {
 		mask = I2C_MST_FIFO_CONTROL_TX_FLUSH |
@@ -488,12 +488,19 @@  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 	val |= mask;
 	i2c_writel(i2c_dev, val, offset);
 
-	while (i2c_readl(i2c_dev, offset) & mask) {
-		if (time_after(jiffies, timeout)) {
-			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
-			return -ETIMEDOUT;
-		}
-		usleep_range(1000, 2000);
+	reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
+	addr = i2c_dev->base + reg_offset;
+
+	if (i2c_dev->is_curr_atomic_xfer)
+		err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
+							1000, 1000000);
+	else
+		err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
+						 1000, 1000000);
+
+	if (err) {
+		dev_err(i2c_dev->dev, "failed to flush FIFO\n");
+		return err;
 	}
 	return 0;
 }