| Message ID | 20250108110620.86900-1-kkartik@nvidia.com |
|---|---|
| Headers | show |
| Series | Add I2C support for Tegra264 | expand |
On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote: > From: Akhil R <akhilrajeev@nvidia.com> > > Add support for SW Mutex register introduced in Tegra264 to provide The spelling is a bit inconsistent. Earlier you referred to this as SW MUTEX register, which makes sense if that's what it's called. But then you call it "SW Mutex" register here. If you don't want to refer to it by the documented name, it should probably be "SW mutex" instead. > an option to share the interface between multiple firmware and/or "firmwares" > Virtual Machines. "virtual machines" 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> > --- > drivers/i2c/busses/i2c-tegra.c | 126 +++++++++++++++++++++++++++++---- > 1 file changed, 111 insertions(+), 15 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index cf05937cb826..a5974af5b1af 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -135,6 +135,11 @@ > #define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16) > #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0) > > +#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 > + > /* configuration load timeout in microseconds */ > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > @@ -202,6 +207,7 @@ enum msg_end_type { > * in HS mode. > * @has_interface_timing_reg: Has interface timing register to program the tuned > * timing settings. > + * @has_mutex: Has Mutex register for mutual exclusion with other firmwares or VM. "mutex" > */ > struct tegra_i2c_hw_feature { > bool has_continue_xfer_support; > @@ -228,6 +234,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; > }; > > /** > @@ -371,6 +378,99 @@ 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); > +} > + > +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev) > +{ > + u32 val, id; > + > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > + if (id != 0) > + return 0; > + > + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID); > + i2c_writel(i2c_dev, val, I2C_SW_MUTEX); > + > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > + > + if (id != I2C_SW_MUTEX_ID) > + return 0; > + > + return 1; > +} > + > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev) > +{ > + /* Poll until mutex is acquired. */ > + while (tegra_i2c_mutex_trylock(i2c_dev)) > + cpu_relax(); > +} Don't we want to use a timeout here? Otherwise we risk blocking the thread that this runs on if some firmware decides not to release the mutex. Also, is the logic not the wrong way around? I.e. trylock returns true if the hardware mutex was successfully locked, in which case it doesn't make sense to keep spinning, right? Or do I misunderstand how this works? Thierry > + > +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev) > +{ > + u32 val, id; > + > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > + > + if (WARN_ON(id != I2C_SW_MUTEX_ID)) > + return; > + > + i2c_writel(i2c_dev, 0, I2C_SW_MUTEX); > +} > + > +static void tegra_i2c_bus_lock(struct i2c_adapter *adapter, > + unsigned int flags) > +{ > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > + > + rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter)); > + tegra_i2c_mutex_lock(i2c_dev); > +} > + > +static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter, > + unsigned int flags) > +{ > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > + int ret; > + > + ret = rt_mutex_trylock(&adapter->bus_lock); > + if (ret) > + ret = tegra_i2c_mutex_trylock(i2c_dev); > + > + return ret; > +} > + > +static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter, > + unsigned int flags) > +{ > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > + > + rt_mutex_unlock(&adapter->bus_lock); > + tegra_i2c_mutex_unlock(i2c_dev); > +} > + > +static const struct i2c_lock_operations tegra_i2c_lock_ops = { > + .lock_bus = tegra_i2c_bus_lock, > + .trylock_bus = tegra_i2c_bus_trylock, > + .unlock_bus = tegra_i2c_bus_unlock, > +}; > + > static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask) > { > u32 int_mask; > @@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev) > i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT); > } > > -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); > -} > - > static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > { > u32 mask, val, offset; > @@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0x0, > .setup_hold_time_hs_mode = 0x0, > .has_interface_timing_reg = false, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { > @@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0x0, > .setup_hold_time_hs_mode = 0x0, > .has_interface_timing_reg = false, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { > @@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0x0, > .setup_hold_time_hs_mode = 0x0, > .has_interface_timing_reg = false, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { > @@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0x0, > .setup_hold_time_hs_mode = 0x0, > .has_interface_timing_reg = true, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { > @@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0, > .setup_hold_time_hs_mode = 0, > .has_interface_timing_reg = true, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { > @@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { > .setup_hold_time_fast_fast_plus_mode = 0, > .setup_hold_time_hs_mode = 0, > .has_interface_timing_reg = true, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { > @@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { > .setup_hold_time_hs_mode = 0x090909, > .has_interface_timing_reg = true, > .has_hs_mode_support = true, > + .has_mutex = false, > }; > > static const struct tegra_i2c_hw_feature tegra264_i2c_hw = { > @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature tegra264_i2c_hw = { > .setup_hold_time_hs_mode = 0x090909, > .has_interface_timing_reg = true, > .has_hs_mode_support = true, > + .has_mutex = true, > }; > > static const struct of_device_id tegra_i2c_of_match[] = { > @@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct platform_device *pdev) > i2c_dev->adapter.nr = pdev->id; > ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); > > + if (i2c_dev->hw->has_mutex) > + i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops; > + > if (i2c_dev->hw->supports_bus_clear) > i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; > > -- > 2.43.0 >
On Thu, Jan 09, 2025 at 10:13:48AM +0530, Mukesh Kumar Savaliya wrote: > Hi Kartik, > > On 1/8/2025 4:36 PM, Kartik Rajput wrote: > > From: Akhil R <akhilrajeev@nvidia.com> > > > > Add support for Tegra264 SoC which supports 17 generic I2C controllers, > > two of which are in the AON (always-on) partition of the SoC. Tegra264 > > I2C supports all the features supported by Tegra194 I2C controllers. > > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> > > --- > > drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > > index 7b97c6d347ee..cf05937cb826 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { > > .has_hs_mode_support = true, > > }; > > +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = { > I could see 7 controllers have been already added. And this may keep > growing. I'm not sure I understand the concern here. This is IP that's been in use ever since the first Tegra chip was released about 15 years ago. It's quite normal that the list of supported hardware will grow over time. At the same time there will be occasional improvements of the hardware that require certain parameterization. > Can we make either default set which is common for most of and change only > sepcific fields ? It's difficult to do. These are const structures on purpose so that they can go into .rodata, so as such there's no good way to reuse defaults. I suppose we could do something like add preprocessor defines, but I doubt that they would make things any better (these are quite fine-grained, so macros would likely only cover one or two fields at a time). > Second option - read these fields from DT and overwrite default if it's > mentioned in DTSI. Some information is already parsed from DT. What's in this structure can all be derived from the compatible string, hence why it's associated with the compatible string via the of_device_id table. Moreover, we cannot move any of this information out into device tree (at least not for existing chips) because it would break DT ABI. > Please review and see if this makes sense. what others say ? I'm always open to suggestions, but I also don't see this as very problematic. It's data that is cleanly structured out, not difficult to maintain and doesn't take up a huge amount of space. Thierry
On Wed, Jan 08, 2025 at 04:36:15PM +0530, Kartik Rajput wrote: > Following series of patches add support for Tegra264 and High Speed (HS) > Mode in i2c-tegra.c driver. > > Akhil R (3): > i2c: tegra: Add HS mode support > i2c: tegra: Add Tegra264 support > i2c: tegra: Add support for SW Mutex register > > Kartik Rajput (2): > dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C > i2c: tegra: Do not configure DMA if not supported It'd probably make sense to restructure this series a little bit. It's customary to have the DT patch first. It would then make more sense to add preparatory patches and then follow those up by the Tegra264 support patches, so something like this: dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C i2c: tegra: Do not configure DMA if not supported i2c: tegra: Add HS mode support i2c: tegra: Add support for SW Mutex register i2c: tegra: Add Tegra264 support Thierry
Thanks Thierry ! On 1/9/2025 3:54 PM, Thierry Reding wrote: > On Thu, Jan 09, 2025 at 10:13:48AM +0530, Mukesh Kumar Savaliya wrote: >> Hi Kartik, >> >> On 1/8/2025 4:36 PM, Kartik Rajput wrote: >>> From: Akhil R <akhilrajeev@nvidia.com> >>> >>> Add support for Tegra264 SoC which supports 17 generic I2C controllers, >>> two of which are in the AON (always-on) partition of the SoC. Tegra264 >>> I2C supports all the features supported by Tegra194 I2C controllers. >>> >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> >>> Signed-off-by: Kartik Rajput <kkartik@nvidia.com> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>> index 7b97c6d347ee..cf05937cb826 100644 >>> --- a/drivers/i2c/busses/i2c-tegra.c >>> +++ b/drivers/i2c/busses/i2c-tegra.c >>> @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { >>> .has_hs_mode_support = true, >>> }; >>> +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = { >> I could see 7 controllers have been already added. And this may keep >> growing. > > I'm not sure I understand the concern here. This is IP that's been in > use ever since the first Tegra chip was released about 15 years ago. > It's quite normal that the list of supported hardware will grow over > time. At the same time there will be occasional improvements of the > hardware that require certain parameterization. > yes, i understand it can grow with new controllers. Was trying to optimize the growing list with common fields. Example: tegra30_i2c_hw and tegra20_i2c_hw has one field changing from 20 fields. So was thinking after seeing this commonality. One suggestion: can one structure be default and then delta can be overridden ? No concern if no other way as you mentioned below. >> Can we make either default set which is common for most of and change only >> sepcific fields ? > > It's difficult to do. These are const structures on purpose so that they > can go into .rodata, so as such there's no good way to reuse defaults. I > suppose we could do something like add preprocessor defines, but I doubt > that they would make things any better (these are quite fine-grained, so > macros would likely only cover one or two fields at a time). > Sure. Let's wait for others opinion. I understand complexity. >> Second option - read these fields from DT and overwrite default if it's >> mentioned in DTSI. > > Some information is already parsed from DT. What's in this structure can > all be derived from the compatible string, hence why it's associated > with the compatible string via the of_device_id table. Moreover, we > cannot move any of this information out into device tree (at least not > for existing chips) because it would break DT ABI. > Got it. >> Please review and see if this makes sense. what others say ? > > I'm always open to suggestions, but I also don't see this as very > problematic. It's data that is cleanly structured out, not difficult to > maintain and doesn't take up a huge amount of space. > I Agree. > Thierry
On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote: > Tegra264 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 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> > --- > .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml > index b57ae6963e62..2a016359328e 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: | Don't need '|' if no formatting. > + 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 > + T194, a MUTEX register is added to support use of the same I2C > + instance across multiple firmware. > + const: nvidia,tegra264-i2c > > reg: > maxItems: 1 > -- > 2.43.0 >
Thanks for reviewing the patch Thierry! On Wed, 2025-01-08 at 18:01 +0100, Thierry Reding wrote: > On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote: > > From: Akhil R <akhilrajeev@nvidia.com> > > > > Add support for SW Mutex register introduced in Tegra264 to provide > > The spelling is a bit inconsistent. Earlier you referred to this as > SW > MUTEX register, which makes sense if that's what it's called. But > then > you call it "SW Mutex" register here. If you don't want to refer to > it > by the documented name, it should probably be "SW mutex" instead. > > > an option to share the interface between multiple firmware and/or > > "firmwares" > > > Virtual Machines. > > "virtual machines" or "VMs" > ACK. I will fix this in the next patchset. > > 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> > > --- > > drivers/i2c/busses/i2c-tegra.c | 126 > > +++++++++++++++++++++++++++++---- > > 1 file changed, 111 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c > > b/drivers/i2c/busses/i2c-tegra.c > > index cf05937cb826..a5974af5b1af 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -135,6 +135,11 @@ > > #define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16) > > #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0) > > > > +#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 > > + > > /* configuration load timeout in microseconds */ > > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > > > @@ -202,6 +207,7 @@ enum msg_end_type { > > * in HS mode. > > * @has_interface_timing_reg: Has interface timing register to > > program the tuned > > * timing settings. > > + * @has_mutex: Has Mutex register for mutual exclusion with other > > firmwares or VM. > > "mutex" > > > */ > > struct tegra_i2c_hw_feature { > > bool has_continue_xfer_support; > > @@ -228,6 +234,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; > > }; > > > > /** > > @@ -371,6 +378,99 @@ 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); > > +} > > + > > +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev) > > +{ > > + u32 val, id; > > + > > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > > + if (id != 0) > > + return 0; > > + > > + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID); > > + i2c_writel(i2c_dev, val, I2C_SW_MUTEX); > > + > > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > > + > > + if (id != I2C_SW_MUTEX_ID) > > + return 0; > > + > > + return 1; > > +} > > + > > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev) > > +{ > > + /* Poll until mutex is acquired. */ > > + while (tegra_i2c_mutex_trylock(i2c_dev)) > > + cpu_relax(); > > +} > > Don't we want to use a timeout here? Otherwise we risk blocking the > thread that this runs on if some firmware decides not to release the > mutex. > We can continue to access the controller with a warning in case the request times out. Something like this? static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev) { unsigned int num_retries = 25; // Move this to a macro. /* Poll until mutex is acquired or timeout. */ while ( --num_retries && !tegra_i2c_mutex_trylock(i2c_dev)) msleep(1); WARN_ON(!num_retries); } > Also, is the logic not the wrong way around? I.e. trylock returns > true > if the hardware mutex was successfully locked, in which case it > doesn't > make sense to keep spinning, right? Or do I misunderstand how this > works? > > Thierry > The logic is indeed wrong here, apologies for the oversight. I will fix this in the next patchset. > > + > > +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev) > > +{ > > + u32 val, id; > > + > > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX); > > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val); > > + > > + if (WARN_ON(id != I2C_SW_MUTEX_ID)) > > + return; > > + > > + i2c_writel(i2c_dev, 0, I2C_SW_MUTEX); > > +} > > + > > +static void tegra_i2c_bus_lock(struct i2c_adapter *adapter, > > + unsigned int flags) > > +{ > > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > > + > > + rt_mutex_lock_nested(&adapter->bus_lock, > > i2c_adapter_depth(adapter)); > > + tegra_i2c_mutex_lock(i2c_dev); > > +} > > + > > +static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter, > > + unsigned int flags) > > +{ > > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > > + int ret; > > + > > + ret = rt_mutex_trylock(&adapter->bus_lock); > > + if (ret) > > + ret = tegra_i2c_mutex_trylock(i2c_dev); > > + > > + return ret; > > +} > > + > > +static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter, > > + unsigned int flags) > > +{ > > + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter); > > + > > + rt_mutex_unlock(&adapter->bus_lock); > > + tegra_i2c_mutex_unlock(i2c_dev); > > +} > > + > > +static const struct i2c_lock_operations tegra_i2c_lock_ops = { > > + .lock_bus = tegra_i2c_bus_lock, > > + .trylock_bus = tegra_i2c_bus_trylock, > > + .unlock_bus = tegra_i2c_bus_unlock, > > +}; > > + > > static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 > > mask) > > { > > u32 int_mask; > > @@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct > > tegra_i2c_dev *i2c_dev) > > i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT); > > } > > > > -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); > > -} > > - > > static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > > { > > u32 mask, val, offset; > > @@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature > > tegra20_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0x0, > > .setup_hold_time_hs_mode = 0x0, > > .has_interface_timing_reg = false, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { > > @@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature > > tegra30_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0x0, > > .setup_hold_time_hs_mode = 0x0, > > .has_interface_timing_reg = false, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { > > @@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature > > tegra114_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0x0, > > .setup_hold_time_hs_mode = 0x0, > > .has_interface_timing_reg = false, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { > > @@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature > > tegra124_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0x0, > > .setup_hold_time_hs_mode = 0x0, > > .has_interface_timing_reg = true, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { > > @@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature > > tegra210_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0, > > .setup_hold_time_hs_mode = 0, > > .has_interface_timing_reg = true, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { > > @@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature > > tegra186_i2c_hw = { > > .setup_hold_time_fast_fast_plus_mode = 0, > > .setup_hold_time_hs_mode = 0, > > .has_interface_timing_reg = true, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { > > @@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature > > tegra194_i2c_hw = { > > .setup_hold_time_hs_mode = 0x090909, > > .has_interface_timing_reg = true, > > .has_hs_mode_support = true, > > + .has_mutex = false, > > }; > > > > static const struct tegra_i2c_hw_feature tegra264_i2c_hw = { > > @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature > > tegra264_i2c_hw = { > > .setup_hold_time_hs_mode = 0x090909, > > .has_interface_timing_reg = true, > > .has_hs_mode_support = true, > > + .has_mutex = true, > > }; > > > > static const struct of_device_id tegra_i2c_of_match[] = { > > @@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct > > platform_device *pdev) > > i2c_dev->adapter.nr = pdev->id; > > ACPI_COMPANION_SET(&i2c_dev->adapter.dev, > > ACPI_COMPANION(&pdev->dev)); > > > > + if (i2c_dev->hw->has_mutex) > > + i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops; > > + > > if (i2c_dev->hw->supports_bus_clear) > > i2c_dev->adapter.bus_recovery_info = > > &tegra_i2c_recovery_info; > > > > -- > > 2.43.0 > >
On Wed, 2025-01-08 at 17:47 +0100, Thierry Reding wrote: > On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote: > > Tegra264 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 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> > > --- > > .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml | 6 > > ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20- > > i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20- > > i2c.yaml > > index b57ae6963e62..2a016359328e 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 > > + T194, a MUTEX register is added to support use of the > > same I2C > > Maybe spell out Tegra194 above for consistency? > > > + instance across multiple firmware. > > I don't know if this last sentence makes much sense in a DT bindings > document, but doesn't hurt either. Maybe s/firmware/firmwares/. > > Thierry ACK. I will update this in the next patch set.
On Thu, 2025-01-09 at 11:37 +0100, Thierry Reding wrote: > On Wed, Jan 08, 2025 at 04:36:15PM +0530, Kartik Rajput wrote: > > Following series of patches add support for Tegra264 and High Speed > > (HS) > > Mode in i2c-tegra.c driver. > > > > Akhil R (3): > > i2c: tegra: Add HS mode support > > i2c: tegra: Add Tegra264 support > > i2c: tegra: Add support for SW Mutex register > > > > Kartik Rajput (2): > > dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C > > i2c: tegra: Do not configure DMA if not supported > > It'd probably make sense to restructure this series a little bit. > It's > customary to have the DT patch first. It would then make more sense > to > add preparatory patches and then follow those up by the Tegra264 > support > patches, so something like this: > > dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C > i2c: tegra: Do not configure DMA if not supported > i2c: tegra: Add HS mode support > i2c: tegra: Add support for SW Mutex register > i2c: tegra: Add Tegra264 support > > Thierry Thanks for reviewing the patches in this series Thierry! I will re-arrange this and post a new patch set for review. Thanks & Regards, Kartik
Thanks Rob! On Fri, 2025-01-10 at 09:55 -0600, Rob Herring wrote: > External email: Use caution opening links or attachments > > > On Wed, Jan 08, 2025 at 04:36:17PM +0530, Kartik Rajput wrote: > > Tegra264 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 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> > > --- > > .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml | 6 > > ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20- > > i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20- > > i2c.yaml > > index b57ae6963e62..2a016359328e 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: | > > Don't need '|' if no formatting. > ACK. Will fix this in v2. Thanks & Regards, Kartik