[PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Tin Huynh]

ACPI always sets txfifo and rxfifo to 32. This configuration will
cause problem if the IP core supports a fifo size of less than 32.
The driver should read the fifo size from the IP and select the 
smaller one of the two.

Signed-off-by: Tin Huynh <tnhuynh@apm.com>

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)

Change from V2:
 -Add a helper function to set fifo size.

Change from V1:
 -Revert the default 32 for fifo, read parameter from IP core
 and pick the smaller one of the two.
 -Correct the title to describe new approach.

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..665f491 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -150,6 +150,24 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
 	return 0;
 }
 
+static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev)
+{
+	u32 param1, tx_fifo_depth, rx_fifo_depth;
+
+	param1 = i2c_dw_read_comp_param(dev);
+	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
+	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
+	if (!dev->tx_fifo_depth) {
+		dev->tx_fifo_depth = tx_fifo_depth;
+		dev->rx_fifo_depth = rx_fifo_depth;
+	} else if (tx_fifo_depth) {
+		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
+				tx_fifo_depth);
+		dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
+				rx_fifo_depth);
+	}
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -246,13 +264,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 				1000000);
 	}
 
-	if (!dev->tx_fifo_depth) {
-		u32 param1 = i2c_dw_read_comp_param(dev);
-
-		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
-		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
+	if (!dev->tx_fifo_depth)
 		dev->adapter.nr = pdev->id;
-	}
+	dw_i2c_set_fifo_size(dev);
 
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
-- 
1.7.1

Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Andy Shevchenko]

Thanks for an update! My comments below.

On Mon, 2016-12-12 at 15:36 +0700, Tin Huynh wrote:
> ACPI always sets txfifo and rxfifo to 32. This configuration will
> cause problem if the IP core supports a fifo size of less than 32.
> The driver should read the fifo size from the IP and select the 
> smaller one of the two.

I would use FIFO in capital to be consistent with what you refer to
(apparently not a variable name), so

Tx FIFO, Rx FIFO, FIFO, and so on.


> 
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
> 
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c |   26
> ++++++++++++++++++++------
>  1 files changed, 20 insertions(+), 6 deletions(-)
> 
> Change from V2:
>  -Add a helper function to set fifo size.
> 
> Change from V1:
>  -Revert the default 32 for fifo, read parameter from IP core
>  and pick the smaller one of the two.
>  -Correct the title to describe new approach.
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..665f491 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -150,6 +150,24 @@ static int i2c_dw_plat_prepare_clk(struct
> dw_i2c_dev *i_dev, bool prepare)
>  	return 0;
>  }
>  
> +static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev)
> +{
> +	u32 param1, tx_fifo_depth, rx_fifo_depth;
> +
> +	param1 = i2c_dw_read_comp_param(dev);

You name it as param1 because you read *_PARAM1? For me it's not clear
from the name of helper function.

u32 param would work otherwise.

> +	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> +	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> +	if (!dev->tx_fifo_depth) {
> +		dev->tx_fifo_depth = tx_fifo_depth;
> +		dev->rx_fifo_depth = rx_fifo_depth;
> +	} else if (tx_fifo_depth) {
> +		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> +				tx_fifo_depth);
> +		dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> +				rx_fifo_depth);
> +	}

So, let's clarify here:
Is it possible to have an IP without parameter block enabled? I mean to
read something arbitrary (or zeroes, or all-ones) from param1.

If not, just remove second condition at all.

> +}
> +
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> @@ -246,13 +264,9 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  				1000000);
>  	}
>  
> -	if (!dev->tx_fifo_depth) {
> -		u32 param1 = i2c_dw_read_comp_param(dev);
> -
> -		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> -		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> 

> +	if (!dev->tx_fifo_depth)
>  		dev->adapter.nr = pdev->id;

Now you spread condition to two locations and it's hard to remember
ordering without looking closer to the code.

That's why I suggested to pass an ID parameter in the first place.

> -	}
> +	dw_i2c_set_fifo_size(dev);
>  
>  	adap = &dev->adapter;
>  	adap->owner = THIS_MODULE;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Mika Westerberg]

On Mon, Dec 12, 2016 at 09:02:53PM +0200, Andy Shevchenko wrote:
> > +	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > +	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > +	if (!dev->tx_fifo_depth) {
> > +		dev->tx_fifo_depth = tx_fifo_depth;
> > +		dev->rx_fifo_depth = rx_fifo_depth;
> > +	} else if (tx_fifo_depth) {
> > +		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> > +				tx_fifo_depth);
> > +		dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> > +				rx_fifo_depth);
> > +	}
> 
> So, let's clarify here:
> Is it possible to have an IP without parameter block enabled? I mean to
> read something arbitrary (or zeroes, or all-ones) from param1.

Yes and it is Intel IP. Haswell IIRC and it returned zeroes.

Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Joe Perches]

On Mon, 2016-12-12 at 21:21 +0200, Mika Westerberg wrote:
> On Mon, Dec 12, 2016 at 09:02:53PM +0200, Andy Shevchenko wrote:
> > > +	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > > +	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > > +	if (!dev->tx_fifo_depth) {
> > > +		dev->tx_fifo_depth = tx_fifo_depth;
> > > +		dev->rx_fifo_depth = rx_fifo_depth;
> > > +	} else if (tx_fifo_depth) {
> > > +		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> > > +				tx_fifo_depth);
> > > +		dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> > > +				rx_fifo_depth);
> > > +	}
> > 
> > So, let's clarify here:
> > Is it possible to have an IP without parameter block enabled? I mean to
> > read something arbitrary (or zeroes, or all-ones) from param1.
> 
> Yes and it is Intel IP. Haswell IIRC and it returned zeroes.

The "+ 1"  in the first set of tx_fifo_depth
makes the "else if" check unnecessary.

Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Mika Westerberg]

On Mon, Dec 12, 2016 at 11:35:19AM -0800, Joe Perches wrote:
> On Mon, 2016-12-12 at 21:21 +0200, Mika Westerberg wrote:
> > On Mon, Dec 12, 2016 at 09:02:53PM +0200, Andy Shevchenko wrote:
> > > > +	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > > > +	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > > > +	if (!dev->tx_fifo_depth) {
> > > > +		dev->tx_fifo_depth = tx_fifo_depth;
> > > > +		dev->rx_fifo_depth = rx_fifo_depth;
> > > > +	} else if (tx_fifo_depth) {
> > > > +		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> > > > +				tx_fifo_depth);
> > > > +		dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> > > > +				rx_fifo_depth);
> > > > +	}
> > > 
> > > So, let's clarify here:
> > > Is it possible to have an IP without parameter block enabled? I mean to
> > > read something arbitrary (or zeroes, or all-ones) from param1.
> > 
> > Yes and it is Intel IP. Haswell IIRC and it returned zeroes.
> 
> The "+ 1"  in the first set of tx_fifo_depth
> makes the "else if" check unnecessary.

Good point. I did not notice that change at all.

The designware I2C databook I have here says that 0 is reserved value
and FIFO sizes start from 2 so the above is wrong either way.

Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI

[Andy Shevchenko]

On Mon, 2016-12-12 at 21:21 +0200, Mika Westerberg wrote:
> On Mon, Dec 12, 2016 at 09:02:53PM +0200, Andy Shevchenko wrote:
> > > +	tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > > +	rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > > +	if (!dev->tx_fifo_depth) {
> > > +		dev->tx_fifo_depth = tx_fifo_depth;
> > > +		dev->rx_fifo_depth = rx_fifo_depth;
> > > +	} else if (tx_fifo_depth) {
> > > +		dev->tx_fifo_depth = min_t(u32, dev-
> > > >tx_fifo_depth,
> > > +				tx_fifo_depth);
> > > +		dev->rx_fifo_depth = min_t(u32, dev-
> > > >rx_fifo_depth,
> > > +				rx_fifo_depth);
> > > +	}
> > 
> > So, let's clarify here:
> > Is it possible to have an IP without parameter block enabled? I mean
> > to
> > read something arbitrary (or zeroes, or all-ones) from param1.
> 
> Yes and it is Intel IP. Haswell IIRC and it returned zeroes.

Wow! Missed that.

In case of zeroes returned the above code might break that (if we are
using FIFO size >1byte). tx_fifo_depth will be 1 AFAIU and second
condition will be the case.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy