diff mbox series

[3/5] i2c: tegra: Add Tegra264 support

Message ID 20250108110620.86900-4-kkartik@nvidia.com
State New
Headers show
Series Add I2C support for Tegra264 | expand

Commit Message

Kartik Rajput Jan. 8, 2025, 11:06 a.m. UTC
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(+)

Comments

Mukesh Kumar Savaliya Jan. 9, 2025, 4:43 a.m. UTC | #1
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.
Can we make either default set which is common for most of and change 
only sepcific fields ?

Second option - read these fields from DT and overwrite default if it's 
mentioned in DTSI.

Please review and see if this makes sense. what others say ?
> +	.has_continue_xfer_support = true,
> +	.has_per_pkt_xfer_complete_irq = true,
> +	.clk_divisor_hs_mode = 1,
> +	.clk_divisor_std_mode = 0x1d,
> +	.clk_divisor_fast_mode = 0x15,
> +	.clk_divisor_fast_plus_mode = 0x8,
> +	.has_config_load_reg = true,
> +	.has_multi_master_mode = true,
> +	.has_slcg_override_reg = true,
> +	.has_mst_fifo = true,
> +	.quirks = &tegra194_i2c_quirks,
> +	.supports_bus_clear = true,
> +	.has_apb_dma = false,
> +	.tlow_std_mode = 0x8,
> +	.thigh_std_mode = 0x7,
> +	.tlow_fast_fastplus_mode = 0x2,
> +	.thigh_fast_fastplus_mode = 0x2,
> +	.tlow_hs_mode = 0x4,
> +	.thigh_hs_mode = 0x2,
> +	.setup_hold_time_std_mode = 0x08080808,
> +	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
> +	.setup_hold_time_hs_mode = 0x090909,
> +	.has_interface_timing_reg = true,
> +	.has_hs_mode_support = true,
> +};
> +
>   static const struct of_device_id tegra_i2c_of_match[] = {
> +	{ .compatible = "nvidia,tegra264-i2c", .data = &tegra264_i2c_hw, },
>   	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
>   	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
>   #if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)
Thierry Reding Jan. 9, 2025, 10:24 a.m. UTC | #2
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
Mukesh Kumar Savaliya Jan. 9, 2025, 12:37 p.m. UTC | #3
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
diff mbox series

Patch

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 = {
+	.has_continue_xfer_support = true,
+	.has_per_pkt_xfer_complete_irq = true,
+	.clk_divisor_hs_mode = 1,
+	.clk_divisor_std_mode = 0x1d,
+	.clk_divisor_fast_mode = 0x15,
+	.clk_divisor_fast_plus_mode = 0x8,
+	.has_config_load_reg = true,
+	.has_multi_master_mode = true,
+	.has_slcg_override_reg = true,
+	.has_mst_fifo = true,
+	.quirks = &tegra194_i2c_quirks,
+	.supports_bus_clear = true,
+	.has_apb_dma = false,
+	.tlow_std_mode = 0x8,
+	.thigh_std_mode = 0x7,
+	.tlow_fast_fastplus_mode = 0x2,
+	.thigh_fast_fastplus_mode = 0x2,
+	.tlow_hs_mode = 0x4,
+	.thigh_hs_mode = 0x2,
+	.setup_hold_time_std_mode = 0x08080808,
+	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
+	.setup_hold_time_hs_mode = 0x090909,
+	.has_interface_timing_reg = true,
+	.has_hs_mode_support = true,
+};
+
 static const struct of_device_id tegra_i2c_of_match[] = {
+	{ .compatible = "nvidia,tegra264-i2c", .data = &tegra264_i2c_hw, },
 	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
 	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)