NAK/cmnt: [SRU][F:linux-bluefield][PATCH v2 1/1] UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism

[Tim Gardner]

On 7/19/22 15:06, Asmaa Mnebhi wrote:
> Buglink: https://bugs.launchpad.net/bugs/1981105
> 
> Support the I2C lock mechanism, otherwise there could be
> unexpected behavior when an i2c bus is accessed by
> several entities like the linux driver, ATF driver and UEFI driver.
> 
> Make sure to pick up the ATF/UEFI image to accompany this change
> because at boot time ATF will ensure that the lock is released.
> 
> Signed-off-by: Asmaa Mnebhi <asmaa at nvidia.com>
> ---
>   drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
> index 0ac9e70244a7..fcb5bb587df1 100644
> --- a/drivers/i2c/busses/i2c-mlxbf.c
> +++ b/drivers/i2c/busses/i2c-mlxbf.c
> @@ -340,7 +340,8 @@ enum {
>    * Timeout is given in microsends. Note also that timeout handling is not
>    * exact.
>    */
> -#define SMBUS_TIMEOUT   (300 * 1000) /* 300ms */
> +#define SMBUS_TIMEOUT               (300 * 1000) /* 300ms */
> +#define SMBUS_LOCK_POLL_TIMEOUT           (300 * 1000) /* 300ms */
>   
>   /* Encapsulates timing parameters */
>   struct mlx_i2c_timings {
> @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv)
>   	return false;
>   }
>   
> +/*
> + * wait for the lock to be released before acquiring it.
> + */
> +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv)
> +{
> +	if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW,
> +			   1 << MASTER_LOCK_BIT_OFF, true,
> +			   SMBUS_LOCK_POLL_TIMEOUT))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv)
> +{
> +	/* Clear the gw to clear the lock */
> +	writel(0, priv->smbus->io + SMBUS_MASTER_GW);
> +}
> +
>   /*
>    * Poll SMBus master status and return transaction status,
>    * i.e. whether succeeded or failed. I2C and SMBus fault codes
> @@ -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave,
>   	/* Clear status bits  */
>   	smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0);
>   	/* Set the cause data */
> -	smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0);
> +	smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0);

This looks like a bug fix unrelated to the locking changes.

>   	/* Zero PEC byte */
>   	smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0);
>   	/* Zero byte count    */
> @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv,
>   	slave     = request->slave & 0x7f;
>   	addr      = slave << 1;
>   
> -	/* First of all, check whether the HW is idle */
> +	/* Try to acquire the smbus gw lock before any reads of the GW register since
> +	 * a read sets the lock.
> +	 */
> +	if (WARN_ON(!mlx_smbus_master_lock(priv)))
> +		return -EBUSY;
> +

Does this function actually set the master lock bit ? It appears that it 
only polls the master lock bit until it returns 0 or 300 msec elapses. 
Does the HW have some unexplained behavior ? If so, then that is worthy 
of some explanation in the code.

> +	/* Check whether the HW is idle */
>   	if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv)))
>   		return -EBUSY;

If the master lock bit is set, shouldn't it get released before 
returning an error ?

>   
> @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv,
>   	if (write_en) {
>   		ret = mlx_smbus_enable(priv, slave, write_len, block_en,
>   				       pec_en, 0);
> -		if (ret != 0)
> +		if (ret) {
> +			mlx_smbus_master_unlock(priv);
>   			return ret;
> +		}
>   	}
>   
>   	if (read_en) {
> @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv,
>   			    SMBUS_MASTER_FSM_PS_STATE_MASK);
>   	}
>   
> +	mlx_smbus_master_unlock(priv);
> +
>   	return ret;
>   }
>   


-- 
-----------
Tim Gardner
Canonical, Inc