Message ID | 20250108110620.86900-4-kkartik@nvidia.com |
---|---|
State | New |
Headers | show |
Series | Add I2C support for Tegra264 | expand |
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)
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
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 --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)