Re: [PATCH 4/6] i2c: designware: Detect the FIFO size in the common code

[Andy Shevchenko]

On Fri, Mar 06, 2020 at 04:19:54PM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> 
> The problem with detecting the FIFO depth in the platform driver
> is that in order to implement this we have to access the controller
> IC_COMP_PARAM_1 register. Currently it's done before the
> i2c_dw_set_reg_access() method execution, which is errors prone since
> the method determines the registers endianness and access mode and we
> can't use dw_readl/dw_writel accessors before this information is
> retrieved. We also can't move the i2c_dw_set_reg_access() function
> invocation to after the master/slave probe functions call (when endianness
> and access mode are determined), since the FIFO depth information is used
> by them for initializations. So in order to fix the problem we have no
> choice but to move the FIFO size detection methods to the common code and
> call it at the probe stage.

Sounds reasonable.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> Cc: Paul Burton <paulburton@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: linux-i2c@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  drivers/i2c/busses/i2c-designware-common.c  | 22 +++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h    |  1 +
>  drivers/i2c/busses/i2c-designware-master.c  |  2 ++
>  drivers/i2c/busses/i2c-designware-platdrv.c | 24 ---------------------
>  drivers/i2c/busses/i2c-designware-slave.c   |  2 ++
>  5 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 2de7452fcd6d..4291ff6246d8 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -344,6 +344,28 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
>  		return -EIO;
>  }
>  
> +void i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
> +{
> +	u32 param, tx_fifo_depth, rx_fifo_depth;
> +
> +	/*
> +	 * Try to detect the FIFO depth if not set by interface driver,
> +	 * the depth could be from 2 to 256 from HW spec.
> +	 */
> +	param = dw_readl(dev, DW_IC_COMP_PARAM_1);
> +	tx_fifo_depth = ((param >> 16) & 0xff) + 1;
> +	rx_fifo_depth = ((param >> 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 >= 2) {
> +		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);
> +	}
> +}
> +
>  u32 i2c_dw_func(struct i2c_adapter *adap)
>  {
>  	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 67edbbde1070..3fbc9f22fcf1 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -297,6 +297,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
>  void i2c_dw_release_lock(struct dw_i2c_dev *dev);
>  int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
>  int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
> +void i2c_dw_set_fifo_size(struct dw_i2c_dev *dev);
>  u32 i2c_dw_func(struct i2c_adapter *adap);
>  void i2c_dw_disable(struct dw_i2c_dev *dev);
>  void i2c_dw_disable_int(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index e8b328242256..05da900cf375 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -698,6 +698,8 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  	if (ret)
>  		return ret;
>  
> +	i2c_dw_set_fifo_size(dev);
> +
>  	ret = dev->init(dev);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 3b7d58c2fe85..cb494273bb60 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -219,28 +219,6 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev)
>  	dev->mode = DW_IC_SLAVE;
>  }
>  
> -static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev)
> -{
> -	u32 param, tx_fifo_depth, rx_fifo_depth;
> -
> -	/*
> -	 * Try to detect the FIFO depth if not set by interface driver,
> -	 * the depth could be from 2 to 256 from HW spec.
> -	 */
> -	param = i2c_dw_read_comp_param(dev);
> -	tx_fifo_depth = ((param >> 16) & 0xff) + 1;
> -	rx_fifo_depth = ((param >> 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 >= 2) {
> -		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 void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
>  {
>  	pm_runtime_disable(dev->dev);
> @@ -362,8 +340,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  				div_u64(clk_khz * t->sda_hold_ns + 500000, 1000000);
>  	}
>  
> -	dw_i2c_set_fifo_size(dev);
> -
>  	adap = &dev->adapter;
>  	adap->owner = THIS_MODULE;
>  	adap->class = I2C_CLASS_DEPRECATED;
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
> index f5f001738df5..0fc3aa31d46a 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -260,6 +260,8 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
>  	if (ret)
>  		return ret;
>  
> +	i2c_dw_set_fifo_size(dev);
> +
>  	ret = dev->init(dev);
>  	if (ret)
>  		return ret;
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko