| Message ID | 20250603153022.39434-2-akhilrajeev@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties | expand |
On Tue, Jun 03, 2025 at 09:00:21PM +0530, Akhil R wrote: > For controllers that has an internal software reset, make the reset > property optional. This is useful in systems that choose to restrict > reset control from Linux. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > v3->v4: No change > v2->v3: No change > v1->v2: > * Call devm_reset_control_get_optional_exclusive() unconditionally. > * Add more delay based on HW recommendation. > > drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 87976e99e6d0..22ddbae9d847 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -134,6 +134,8 @@ > #define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16) > #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0) > > +#define I2C_MASTER_RESET_CNTRL 0x0a8 > + > /* configuration load timeout in microseconds */ > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > @@ -184,6 +186,9 @@ enum msg_end_type { > * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that > * provides additional features and allows for longer messages to > * be transferred in one go. > + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which > + * provides an alternative to controller reset when configured as > + * I2C master > * @quirks: I2C adapter quirks for limiting write/read transfer size and not > * allowing 0 length transfers. > * @supports_bus_clear: Bus Clear support to recover from bus hang during > @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature { > bool has_multi_master_mode; > bool has_slcg_override_reg; > bool has_mst_fifo; > + bool has_mst_reset; > const struct i2c_adapter_quirks *quirks; > bool supports_bus_clear; > bool has_apb_dma; > @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > return 0; > } > > +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (!i2c_dev->hw->has_mst_reset) > + return -EOPNOTSUPP; > + > + i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL); > + udelay(2); > + > + i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL); > + udelay(2); > + > + return 0; > +} > + > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > */ > if (handle) > err = acpi_evaluate_object(handle, "_RST", NULL, NULL); How is the internal reset handled on ACPI? Does the _RST method do the internal reset? Thierry
Hi Andy, ... > @@ -184,6 +186,9 @@ enum msg_end_type { > * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that > * provides additional features and allows for longer messages to > * be transferred in one go. > + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which > + * provides an alternative to controller reset when configured as > + * I2C master > * @quirks: I2C adapter quirks for limiting write/read transfer size and not > * allowing 0 length transfers. > * @supports_bus_clear: Bus Clear support to recover from bus hang during > @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature { > bool has_multi_master_mode; > bool has_slcg_override_reg; > bool has_mst_fifo; > + bool has_mst_reset; > const struct i2c_adapter_quirks *quirks; > bool supports_bus_clear; > bool has_apb_dma; > @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > return 0; > } > > +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (!i2c_dev->hw->has_mst_reset) > + return -EOPNOTSUPP; > + > + i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL); > + udelay(2); > + > + i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL); > + udelay(2); > + > + return 0; > +} > + > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > */ > if (handle) > err = acpi_evaluate_object(handle, "_RST", NULL, NULL); > - else > + else if (i2c_dev->rst) > err = reset_control_reset(i2c_dev->rst); > + else > + err = tegra_i2c_master_reset(i2c_dev); Can you please take a look here? Should the reset happen in ACPI? Thanks, Andi
On Thu, Jun 12, 2025 at 02:57:36AM +0200, Andi Shyti wrote: ... > > +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev) > > +{ > > + if (!i2c_dev->hw->has_mst_reset) > > + return -EOPNOTSUPP; > > + > > + i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL); > > + udelay(2); > > + > > + i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL); > > + udelay(2); > > + > > + return 0; > > +} > > + > > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > > { > > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > > */ > > if (handle) > > err = acpi_evaluate_object(handle, "_RST", NULL, NULL); > > - else > > + else if (i2c_dev->rst) > > err = reset_control_reset(i2c_dev->rst); > > + else > > + err = tegra_i2c_master_reset(i2c_dev); > > Can you please take a look here? Should the reset happen in ACPI? This is a good question. Without seeing all the implementations of _RST method for the platforms based on this SoC it's hard to say. Ideally the _RST (which is called above) must handle it properly, but firmwares have bugs... TL;DR: I think the approach is correct, and if any bug in ACPI will be found, the workaround (quirk) needs to be added here later on.
On Thu, 12 Jun 2025 15:55:21 +0300, Andy Shevchenko wrote: > >> > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) >> > { >> > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; >> > @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) >> > */ >> > if (handle) >> > err = acpi_evaluate_object(handle, "_RST", NULL, NULL); >> > - else >> > + else if (i2c_dev->rst) >> > err = reset_control_reset(i2c_dev->rst); >> > + else >> > + err = tegra_i2c_master_reset(i2c_dev); >> >> Can you please take a look here? Should the reset happen in ACPI? > > This is a good question. Without seeing all the implementations of _RST method > for the platforms based on this SoC it's hard to say. Ideally the _RST (which > is called above) must handle it properly, but firmwares have bugs... > > TL;DR: I think the approach is correct, and if any bug in ACPI will be found, > the workaround (quirk) needs to be added here later on. As in Thierry's comment, I was in thought of updating the code as below. Does it make sense or would it be better keep what it is there now? if (handle && acpi_has_method(handle, "_RST")) err = acpi_evaluate_object(handle, "_RST", NULL, NULL); else if (i2c_dev->rst) err = reset_control_reset(i2c_dev->rst); else err = tegra_i2c_master_reset(i2c_dev); Regards, Akhil
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 87976e99e6d0..22ddbae9d847 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -134,6 +134,8 @@ #define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16) #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0) +#define I2C_MASTER_RESET_CNTRL 0x0a8 + /* configuration load timeout in microseconds */ #define I2C_CONFIG_LOAD_TIMEOUT 1000000 @@ -184,6 +186,9 @@ enum msg_end_type { * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that * provides additional features and allows for longer messages to * be transferred in one go. + * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which + * provides an alternative to controller reset when configured as + * I2C master * @quirks: I2C adapter quirks for limiting write/read transfer size and not * allowing 0 length transfers. * @supports_bus_clear: Bus Clear support to recover from bus hang during @@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature { bool has_multi_master_mode; bool has_slcg_override_reg; bool has_mst_fifo; + bool has_mst_reset; const struct i2c_adapter_quirks *quirks; bool supports_bus_clear; bool has_apb_dma; @@ -604,6 +610,20 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) return 0; } +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev) +{ + if (!i2c_dev->hw->has_mst_reset) + return -EOPNOTSUPP; + + i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL); + udelay(2); + + i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL); + udelay(2); + + return 0; +} + static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) { u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; @@ -621,8 +641,10 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) */ if (handle) err = acpi_evaluate_object(handle, "_RST", NULL, NULL); - else + else if (i2c_dev->rst) err = reset_control_reset(i2c_dev->rst); + else + err = tegra_i2c_master_reset(i2c_dev); WARN_ON_ONCE(err); @@ -1467,6 +1489,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = false, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = false, .has_apb_dma = true, @@ -1491,6 +1514,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = false, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = false, .has_apb_dma = true, @@ -1515,6 +1539,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = false, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = true, .has_apb_dma = true, @@ -1539,6 +1564,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = true, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = true, .has_apb_dma = true, @@ -1563,6 +1589,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = true, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = true, .has_apb_dma = true, @@ -1587,6 +1614,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { .has_multi_master_mode = false, .has_slcg_override_reg = true, .has_mst_fifo = false, + .has_mst_reset = false, .quirks = &tegra_i2c_quirks, .supports_bus_clear = true, .has_apb_dma = false, @@ -1611,6 +1639,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { .has_multi_master_mode = true, .has_slcg_override_reg = true, .has_mst_fifo = true, + .has_mst_reset = true, .quirks = &tegra194_i2c_quirks, .supports_bus_clear = true, .has_apb_dma = false, @@ -1666,7 +1695,7 @@ static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev) if (ACPI_HANDLE(i2c_dev->dev)) return 0; - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); + i2c_dev->rst = devm_reset_control_get_optional_exclusive(i2c_dev->dev, "i2c"); if (IS_ERR(i2c_dev->rst)) return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), "failed to get reset control\n");
For controllers that has an internal software reset, make the reset property optional. This is useful in systems that choose to restrict reset control from Linux. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- v3->v4: No change v2->v3: No change v1->v2: * Call devm_reset_control_get_optional_exclusive() unconditionally. * Add more delay based on HW recommendation. drivers/i2c/busses/i2c-tegra.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)