| Message ID | 20250609093420.3050641-1-kkartik@nvidia.com |
|---|---|
| Headers | show |
| Series | Add I2C support for Tegra264 | expand |
On Mon, Jun 09, 2025 at 03:04:16PM +0530, Kartik Rajput wrote: > iTegra264 has 17 generic I2C controllers, two of which are in always-on > partition of the SoC. In addition to the features supported by Tegra194 > it also supports a SW mutex register to allow sharing the same I2C > instance across multiple firmware. > > Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C. > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> > --- > v2 -> v3: > * Add constraints for "nvidia,tegra264-i2c". > v1 -> v2: > * Fixed typos. > --- > .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml > index 6b6f6762d122..f0693b872cb6 100644 > --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml > +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml > @@ -80,6 +80,12 @@ properties: > support for 64 KiB transactions whereas earlier chips supported no > more than 4 KiB per transactions. > const: nvidia,tegra194-i2c > + - description: > + Tegra264 has 17 generic I2C controllers, two of which are in the AON > + (always-on) partition of the SoC. In addition to the features from > + Tegra194, a SW mutex register is added to support use of the same I2C > + instance across multiple firmwares. > + const: nvidia,tegra264-i2c > > reg: > maxItems: 1 > @@ -186,6 +192,7 @@ allOf: > contains: > enum: > - nvidia,tegra194-i2c > + - nvidia,tegra264-i2c > then: > required: > - resets > -- > 2.43.0 >
On Mon, Jun 09, 2025 at 03:04:19PM +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. > > 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: Akhil R <akhilrajeev@nvidia.com> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> > --- > v2 -> v3: > * Update tegra_i2c_mutex_trylock and tegra_i2c_mutex_unlock to > use readl and writel APIs instead of i2c_readl and i2c_writel > which use relaxed APIs. > * Use dev_warn instead of WARN_ON if mutex lock/unlock fails. > v1 -> v2: > * Fixed typos. > * Fix tegra_i2c_mutex_lock() logic. > * Add a timeout in tegra_i2c_mutex_lock() instead of polling for > mutex indefinitely. > --- > drivers/i2c/busses/i2c-tegra.c | 137 +++++++++++++++++++++++++++++---- > 1 file changed, 122 insertions(+), 15 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index d0b6aa013c96..dae59e9e993b 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -137,6 +137,14 @@ > > #define I2C_MASTER_RESET_CNTRL 0x0a8 > > +#define I2C_SW_MUTEX 0x0ec > +#define I2C_SW_MUTEX_REQUEST GENMASK(3, 0) > +#define I2C_SW_MUTEX_GRANT GENMASK(7, 4) > +#define I2C_SW_MUTEX_ID 9 Maybe this should contain some sort of suffix to denote which ID this is? Maybe I2C_SW_MUTEX_ID_CCPLEX? > + > +/* SW mutex acquire timeout value in milliseconds. */ > +#define I2C_SW_MUTEX_TIMEOUT 25 > + > /* configuration load timeout in microseconds */ > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > @@ -210,6 +218,7 @@ enum msg_end_type { > * @has_interface_timing_reg: Has interface timing register to program the tuned > * timing settings. > * @has_hs_mode_support: Has support for high speed (HS) mode transfers. > + * @has_mutex: Has mutex register for mutual exclusion with other firmwares or VMs. > */ > struct tegra_i2c_hw_feature { > bool has_continue_xfer_support; > @@ -237,6 +246,7 @@ struct tegra_i2c_hw_feature { > u32 setup_hold_time_hs_mode; > bool has_interface_timing_reg; > bool has_hs_mode_support; > + bool has_mutex; > }; > > /** > @@ -380,6 +390,108 @@ 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_poll_register(struct tegra_i2c_dev *i2c_dev, > + u32 reg, u32 mask, u32 delay_us, > + u32 timeout_us) > +{ > + void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg); > + u32 val; > + > + if (!i2c_dev->atomic_mode) > + return readl_relaxed_poll_timeout(addr, val, !(val & mask), > + delay_us, timeout_us); > + > + return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), > + delay_us, timeout_us); > +} The move of this function seems unnecessary. > + > +static int tegra_i2c_mutex_trylock(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 != 0 && id != I2C_SW_MUTEX_ID) > + return 0; > + > + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID); > + writel(val, i2c_dev->base + reg); > + > + val = readl(i2c_dev->base + reg); > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > + > + if (id != I2C_SW_MUTEX_ID) > + return 0; > + > + return 1; > +} Do we need some sort of locking around these? Or is this always guaranteed to be called from a locked region already? > + > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev) > +{ > + unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT; > + > + /* Poll until mutex is acquired or timeout. */ > + while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev)) > + usleep_range(1000, 2000); Maybe this can be rewritten to be easier on the eye? Something like: while (--num_retries) { if (tegra_i2c_mutex_trylock(i2c_dev)) break; usleep_range(1000, 2000); } looks a bit easier to follow. It may also be better to change this to a properly timed loop. As it is, the usleep_range() can take anywhere from 1 to 2 ms, so effectively I2C_SW_MUTEX_TIMEOUT 25 can be 50 ms long. I'm sure that doesn't matter all that much, but it's a bit of an ambiguous specification. So I think we should either be precise with a timed loop if we specify the timeout in milliseconds or we should not pretend that we care about the specific time and rename the variable to something like I2C_SW_MUTEX_RETRIES instead. I prefer the timed loop variant, and I think there's ways you can use helpers from linux/iopoll.h to achieve this (i.e. use the generic read_poll_timeout() with tegra_i2c_mutex_trylock as op parameter and passing i2c_dev as args). > + > + if (!num_retries) > + dev_warn(i2c_dev->dev, "timeout while acquiring mutex, proceeding anyway\n"); > +} I take it there's no way to refuse operations since there's no return value for this function? I wonder if it's the right interface for this, then. If there's no mechanism to enforce the lock in hardware, then we must somehow respect it in software. Proceeding even after failing to acquire the lock seems like a recipe for breaking things. Also, this looks slightly wrong. What if the trylock succeeds on the last retry? num_retries will have been decremented to 0 at that point, so we'll see the warning even if it did succeed. Thierry
On Tue, 2025-06-10 at 09:49 +0200, Thierry Reding wrote: > On Mon, Jun 09, 2025 at 03:04:19PM +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. > > > > 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: Akhil R <akhilrajeev@nvidia.com> > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> > > --- > > v2 -> v3: > > * Update tegra_i2c_mutex_trylock and > > tegra_i2c_mutex_unlock to > > use readl and writel APIs instead of i2c_readl and > > i2c_writel > > which use relaxed APIs. > > * Use dev_warn instead of WARN_ON if mutex lock/unlock > > fails. > > v1 -> v2: > > * Fixed typos. > > * Fix tegra_i2c_mutex_lock() logic. > > * Add a timeout in tegra_i2c_mutex_lock() instead of > > polling for > > mutex indefinitely. > > --- > > drivers/i2c/busses/i2c-tegra.c | 137 > > +++++++++++++++++++++++++++++---- > > 1 file changed, 122 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c > > b/drivers/i2c/busses/i2c-tegra.c > > index d0b6aa013c96..dae59e9e993b 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -137,6 +137,14 @@ > > > > #define I2C_MASTER_RESET_CNTRL 0x0a8 > > > > +#define I2C_SW_MUTEX 0x0ec > > +#define I2C_SW_MUTEX_REQUEST GENMASK(3, 0) > > +#define I2C_SW_MUTEX_GRANT GENMASK(7, 4) > > +#define I2C_SW_MUTEX_ID 9 > > Maybe this should contain some sort of suffix to denote which ID this > is? Maybe I2C_SW_MUTEX_ID_CCPLEX? Ack, I2C_SW_MUTEX_ID_CCPLEX sounds good. I will update this in next revision. > > > + > > +/* SW mutex acquire timeout value in milliseconds. */ > > +#define I2C_SW_MUTEX_TIMEOUT 25 > > + > > /* configuration load timeout in microseconds */ > > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > > > @@ -210,6 +218,7 @@ enum msg_end_type { > > * @has_interface_timing_reg: Has interface timing register to > > program the tuned > > * timing settings. > > * @has_hs_mode_support: Has support for high speed (HS) mode > > transfers. > > + * @has_mutex: Has mutex register for mutual exclusion with other > > firmwares or VMs. > > */ > > struct tegra_i2c_hw_feature { > > bool has_continue_xfer_support; > > @@ -237,6 +246,7 @@ struct tegra_i2c_hw_feature { > > u32 setup_hold_time_hs_mode; > > bool has_interface_timing_reg; > > bool has_hs_mode_support; > > + bool has_mutex; > > }; > > > > /** > > @@ -380,6 +390,108 @@ 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_poll_register(struct tegra_i2c_dev *i2c_dev, > > + u32 reg, u32 mask, u32 > > delay_us, > > + u32 timeout_us) > > +{ > > + void __iomem *addr = i2c_dev->base + > > tegra_i2c_reg_addr(i2c_dev, reg); > > + u32 val; > > + > > + if (!i2c_dev->atomic_mode) > > + return readl_relaxed_poll_timeout(addr, val, !(val > > & mask), > > + delay_us, > > timeout_us); > > + > > + return readl_relaxed_poll_timeout_atomic(addr, val, !(val > > & mask), > > + delay_us, > > timeout_us); > > +} > > The move of this function seems unnecessary. Ack, I will fix this in the next revision. > > > + > > +static int tegra_i2c_mutex_trylock(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 != 0 && id != I2C_SW_MUTEX_ID) > > + return 0; > > + > > + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID); > > + writel(val, i2c_dev->base + reg); > > + > > + val = readl(i2c_dev->base + reg); > > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > > + > > + if (id != I2C_SW_MUTEX_ID) > > + return 0; > > + > > + return 1; > > +} > > Do we need some sort of locking around these? Or is this always > guaranteed to be called from a locked region already? This is currently called from locked region. But, since we plan to move these APIs out of bus lock/unlock operations we should add a lock around these. > > > + > > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev) > > +{ > > + unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT; > > + > > + /* Poll until mutex is acquired or timeout. */ > > + while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev)) > > + usleep_range(1000, 2000); > > Maybe this can be rewritten to be easier on the eye? Something like: > > while (--num_retries) { > if (tegra_i2c_mutex_trylock(i2c_dev)) > break; > > usleep_range(1000, 2000); > } > > looks a bit easier to follow. It may also be better to change this to > a > properly timed loop. As it is, the usleep_range() can take anywhere > from > 1 to 2 ms, so effectively I2C_SW_MUTEX_TIMEOUT 25 can be 50 ms long. > I'm > sure that doesn't matter all that much, but it's a bit of an > ambiguous > specification. So I think we should either be precise with a timed > loop > if we specify the timeout in milliseconds or we should not pretend > that > we care about the specific time and rename the variable to something > like I2C_SW_MUTEX_RETRIES instead. I prefer the timed loop variant, > and > I think there's ways you can use helpers from linux/iopoll.h to > achieve > this (i.e. use the generic read_poll_timeout() with > tegra_i2c_mutex_trylock as op parameter and passing i2c_dev as args). I will update the implementation to use read_poll_timeout() to achieve an accurate timeout here > > > + > > + if (!num_retries) > > + dev_warn(i2c_dev->dev, "timeout while acquiring > > mutex, proceeding anyway\n"); > > +} > > I take it there's no way to refuse operations since there's no return > value for this function? I wonder if it's the right interface for > this, > then. If there's no mechanism to enforce the lock in hardware, then > we > must somehow respect it in software. Proceeding even after failing to > acquire the lock seems like a recipe for breaking things. Should I move the lock/unlock operations to tegra_i2c_runtime_resume/suspend functions? That way we can propagate the error correctly and fail in case we do not acquire the mutex. > > Also, this looks slightly wrong. What if the trylock succeeds on the > last retry? num_retries will have been decremented to 0 at that > point, > so we'll see the warning even if it did succeed. You are correct, ideally we should check I2C_SW_MUTEX register to know if the mutex has been acquired or not. I will fix this in the next revision. > > Thierry Thanks & Regards, Kartik