[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <l4po62bw6672xpaabkbvu6snyg4hrgcdxaijpt6evizortwjok@jwg2asezj3cb>
Date: Fri, 5 Sep 2025 01:28:00 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Kartik Rajput <kkartik@...dia.com>
Cc: akhilrajeev@...dia.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, thierry.reding@...il.com, jonathanh@...dia.com,
ldewangan@...dia.com, digetx@...il.com, linux-i2c@...r.kernel.org,
devicetree@...r.kernel.org, linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/5] i2c: tegra: Add support for SW mutex register
Hi Kartik,
On Thu, Aug 28, 2025 at 11:29:32AM +0530, Kartik Rajput wrote:
> Add support for SW mutex register introduced in Tegra264 to provide
> an option to share the interface between multiple firmwares and/or
> VMs.
You could add a short description on how to use the mutex
register here.
> However, the hardware does not ensure any protection based on the
> values. The driver/firmware should honor the peer who already holds
> the mutex.
>
> Signed-off-by: Kartik Rajput <kkartik@...dia.com>
> Signed-off-by: Akhil R <akhilrajeev@...dia.com>
...
> @@ -381,6 +391,73 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
> readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> }
>
> +static int tegra_i2c_mutex_acquired(struct tegra_i2c_dev *i2c_dev)
this is a bool function.
> +{
> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
> + u32 val, id;
> +
> + val = readl(i2c_dev->base + reg);
> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> + if (id != I2C_SW_MUTEX_ID_CCPLEX)
> + return 0;
> +
> + return 1;
return id != I2C_SW_MUTEX_ID_CCPLEX;
> +}
> +
> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
I think this can be bool.
> +{
> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
> + u32 val, id;
> +
> + val = readl(i2c_dev->base + reg);
> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> + if (id != 0 && id != I2C_SW_MUTEX_ID_CCPLEX)
> + return 0;
> +
> + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID_CCPLEX);
> + writel(val, i2c_dev->base + reg);
> +
> + return tegra_i2c_mutex_acquired(i2c_dev);
> +}
> +
> +static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> +{
> + int locked;
I guess this can be bool.
> + int ret;
> +
> + if (i2c_dev->atomic_mode)
> + ret = read_poll_timeout_atomic(tegra_i2c_mutex_trylock, locked, locked,
> + USEC_PER_MSEC, I2C_SW_MUTEX_TIMEOUT_US,
> + false, i2c_dev);
> + else
> + ret = read_poll_timeout(tegra_i2c_mutex_trylock, locked, locked, USEC_PER_MSEC,
> + I2C_SW_MUTEX_TIMEOUT_US, false, i2c_dev);
> +
> + if (!tegra_i2c_mutex_acquired(i2c_dev))
> + dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
I would try a few times before giving up.
Besides, is there a chance where ret is '0' and the mutex is not
acquired? If so, we are not signalling error if the mutex is not
acquired, but I think we should.
I would do:
if (...)
ret = ...
else
ret = ...
if (ret)
return ret;
if (!tegra_i2c_mutex_acquired(i2c_dev)) {
dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
return -ESOMETHING;
}
return 0;
Makes sense?
> +
> + return ret;
> +}
> +
> +static int tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
> +{
> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
> + u32 val, id;
> +
> + val = readl(i2c_dev->base + reg);
> +
> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> + if (id && id != I2C_SW_MUTEX_ID_CCPLEX) {
> + dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
> + return -EPERM;
I would try a few times before giving up.
> + }
> +
> + writel(0, i2c_dev->base + reg);
> +
> + return 0;
> +}
> +
> static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
> {
> u32 int_mask;
> @@ -1422,6 +1499,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> return ret;
> }
>
> +
no need for this extra blank line.
> + if (i2c_dev->hw->has_mutex) {
I would put this check in tegra_i2c_mutex_lock() and _unlock() in
order to avoid two level indentation here.
> + ret = tegra_i2c_mutex_lock(i2c_dev);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < num; i++) {
> enum msg_end_type end_type = MSG_END_STOP;
>
> @@ -1451,6 +1535,12 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> break;
> }
>
> + if (i2c_dev->hw->has_mutex) {
> + ret = tegra_i2c_mutex_unlock(i2c_dev);
> + if (ret)
> + return ret;
We are skipping pm_runtime_put(), though.
Thanks,
Andi
> + }
> +
> pm_runtime_put(i2c_dev->dev);
>
> return ret ?: i;
Powered by blists - more mailing lists