| Message ID | 20200908224006.25636-13-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
| Context | Check | Description |
|---|---|---|
| tagr/GVS | success | 1124251 |
| tagr/GVS-1122491 | pending | None |
| tagr/GVS-1122491 | fail | None |
| tagr/GVS | pending | 1122533 |
| tagr/GVS | success | 1122533 |
| tagr/GVS | pending | 1124251 |
On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > Use clk-bulk helpers and factor out clocks initialization into separate > function in order to make code cleaner. > > The clocks initialization now performed after reset-control initialization > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > helper which doesn't silence this error code. Hence reset_control_get() > now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP > driver that provides reset controls and BPMP doesn't come up early during > boot. Previously rst was protected by the clocks retrieval and now this > patch makes dev_err_probe() to be used for the rst error handling. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > 1 file changed, 67 insertions(+), 120 deletions(-) This is tempting from a diffstat point of view, but the downside is that we can now no longer validate that all of the necessary clocks are given in device tree. Previously the driver would fail to probe the I2C controller if any of the expected clocks were not defined in device tree, but now it's just going to continue without it and not give any indication as to what's wrong. Thierry
On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > Use clk-bulk helpers and factor out clocks initialization into separate > > function in order to make code cleaner. > > > > The clocks initialization now performed after reset-control initialization > > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > > helper which doesn't silence this error code. Hence reset_control_get() > > now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP > > driver that provides reset controls and BPMP doesn't come up early during > > boot. Previously rst was protected by the clocks retrieval and now this > > patch makes dev_err_probe() to be used for the rst error handling. > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > > 1 file changed, 67 insertions(+), 120 deletions(-) > > This is tempting from a diffstat point of view, but the downside is that > we can now no longer validate that all of the necessary clocks are given > in device tree. > > Previously the driver would fail to probe the I2C controller if any of > the expected clocks were not defined in device tree, but now it's just > going to continue without it and not give any indication as to what's > wrong. You may print an error in the error path as previously. Since both clocks are mandatory (as far as I understood the code) user will need to check DT in any case.
17.09.2020 14:38, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: >> Use clk-bulk helpers and factor out clocks initialization into separate >> function in order to make code cleaner. >> >> The clocks initialization now performed after reset-control initialization >> in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk >> helper which doesn't silence this error code. Hence reset_control_get() >> now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP >> driver that provides reset controls and BPMP doesn't come up early during >> boot. Previously rst was protected by the clocks retrieval and now this >> patch makes dev_err_probe() to be used for the rst error handling. >> >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- >> 1 file changed, 67 insertions(+), 120 deletions(-) > > This is tempting from a diffstat point of view, but the downside is that > we can now no longer validate that all of the necessary clocks are given > in device tree. > > Previously the driver would fail to probe the I2C controller if any of > the expected clocks were not defined in device tree, but now it's just > going to continue without it and not give any indication as to what's > wrong. The clocks can't be missed randomly in a device-tree because they are a part of the core tegra.dtsi The tegra-i2c DT binding isn't converted to YAML, but once it will be, then the dtbs_check will tell you about such obvious problems like a missing mandatory property. Even if clock is missing, then you won't miss this problem since I2C shouldn't work in that case. There is a Qualcomm I2C driver that already uses clk_bulk_get_all() and doesn't worry about "accidentally" missing clocks. It's still possible to add the clk-num checking, but it should be unpractical. We could always add it later on if there will be a real incident. Do you agree?
On Wed, 09 Sep 2020 01:39:44 +0300, Dmitry Osipenko wrote: > Use clk-bulk helpers and factor out clocks initialization into separate > function in order to make code cleaner. > > The clocks initialization now performed after reset-control initialization > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > helper which doesn't silence this error code. Hence reset_control_get() > now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP > driver that provides reset controls and BPMP doesn't come up early during > boot. Previously rst was protected by the clocks retrieval and now this > patch makes dev_err_probe() to be used for the rst error handling. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > 1 file changed, 67 insertions(+), 120 deletions(-) Tested-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > > Use clk-bulk helpers and factor out clocks initialization into separate > > > function in order to make code cleaner. > > > > > > The clocks initialization now performed after reset-control initialization > > > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > > > helper which doesn't silence this error code. Hence reset_control_get() > > > now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP > > > driver that provides reset controls and BPMP doesn't come up early during > > > boot. Previously rst was protected by the clocks retrieval and now this > > > patch makes dev_err_probe() to be used for the rst error handling. > > > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > --- > > > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > > > 1 file changed, 67 insertions(+), 120 deletions(-) > > > > This is tempting from a diffstat point of view, but the downside is that > > we can now no longer validate that all of the necessary clocks are given > > in device tree. > > > > Previously the driver would fail to probe the I2C controller if any of > > the expected clocks were not defined in device tree, but now it's just > > going to continue without it and not give any indication as to what's > > wrong. > > You may print an error in the error path as previously. Since both > clocks are mandatory (as far as I understood the code) user will need > to check DT in any case. The problem is that the number of required clocks depends on the variant of the IP block that's implemented. Some require just one clock and others require two or three. With this patch the driver is just going to pick whatever clocks are given in device tree, but it removes any possibility of detecting whether the device trees contain the correct clocks. So we may very well run into a situation where the driver now successfully probes but then malfunctions because one or more of the clocks were not specified in device tree. Thierry
On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:38, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > >> Use clk-bulk helpers and factor out clocks initialization into separate > >> function in order to make code cleaner. > >> > >> The clocks initialization now performed after reset-control initialization > >> in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > >> helper which doesn't silence this error code. Hence reset_control_get() > >> now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP > >> driver that provides reset controls and BPMP doesn't come up early during > >> boot. Previously rst was protected by the clocks retrieval and now this > >> patch makes dev_err_probe() to be used for the rst error handling. > >> > >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > >> 1 file changed, 67 insertions(+), 120 deletions(-) > > > > This is tempting from a diffstat point of view, but the downside is that > > we can now no longer validate that all of the necessary clocks are given > > in device tree. > > > > Previously the driver would fail to probe the I2C controller if any of > > the expected clocks were not defined in device tree, but now it's just > > going to continue without it and not give any indication as to what's > > wrong. > > The clocks can't be missed randomly in a device-tree because they are a > part of the core tegra.dtsi You can easily generate a device-tree without using the core DTS includes. > The tegra-i2c DT binding isn't converted to YAML, but once it will be, > then the dtbs_check will tell you about such obvious problems like a > missing mandatory property. Once that has happened, yes, I think we may be able to simplify the driver. But before that happens I don't think we should throw away our only line of defense against broken device trees. > Even if clock is missing, then you won't miss this problem since I2C > shouldn't work in that case. But the whole point of this error handling here is so that we can make it easier to find the cause of an error. I2C malfunctioning can be subtle. You could have some EEPROM on I2C that you normally don't touch, but then at some point you try to access it from userspace and you read garbage and then you need to start looking why this is happening. The whole point of error messages is so that you can easily find the root cause. > There is a Qualcomm I2C driver that already uses clk_bulk_get_all() and > doesn't worry about "accidentally" missing clocks. Just because one particular driver doesn't care doesn't mean everybody else should stop caring. > It's still possible to add the clk-num checking, but it should be > unpractical. We could always add it later on if there will be a real > incident. Do you agree? If there's an incident it's already too late. The whole point of error checking here is to avoid any accidental breakage. Thierry
On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: [...] > It's still possible to add the clk-num checking, but it should be > unpractical. We could always add it later on if there will be a real > incident. Do you agree? There's also clk_bulk_get(), which allows you to specify the number of clocks and their consumer IDs that you want to request. That seems like it would allow us to both avoid the repetitive calls to clk APIs and at the same time allows us to specify exactly which clocks we need. Would that not work as a compromise? Thierry
On Mon, Sep 21, 2020 at 2:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: ... > > > This is tempting from a diffstat point of view, but the downside is that > > > we can now no longer validate that all of the necessary clocks are given > > > in device tree. > > > > > > Previously the driver would fail to probe the I2C controller if any of > > > the expected clocks were not defined in device tree, but now it's just > > > going to continue without it and not give any indication as to what's > > > wrong. > > > > You may print an error in the error path as previously. Since both > > clocks are mandatory (as far as I understood the code) user will need > > to check DT in any case. > > The problem is that the number of required clocks depends on the variant > of the IP block that's implemented. Some require just one clock and > others require two or three. With this patch the driver is just going to > pick whatever clocks are given in device tree, but it removes any > possibility of detecting whether the device trees contain the correct > clocks. So we may very well run into a situation where the driver now > successfully probes but then malfunctions because one or more of the > clocks were not specified in device tree. > > Thierry I still failed to get this. Are you suggesting that CCF bulk operations are fundamentally broken? In the above case one may add more checks. AFAICS is_vi won't be removed, so can be easily checked. Basically that for-loop for div_clk is questionable. I agree on that.
On Mon, Sep 21, 2020 at 02:15:09PM +0300, Andy Shevchenko wrote: > On Mon, Sep 21, 2020 at 2:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > > > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > ... > > > > > This is tempting from a diffstat point of view, but the downside is that > > > > we can now no longer validate that all of the necessary clocks are given > > > > in device tree. > > > > > > > > Previously the driver would fail to probe the I2C controller if any of > > > > the expected clocks were not defined in device tree, but now it's just > > > > going to continue without it and not give any indication as to what's > > > > wrong. > > > > > > You may print an error in the error path as previously. Since both > > > clocks are mandatory (as far as I understood the code) user will need > > > to check DT in any case. > > > > The problem is that the number of required clocks depends on the variant > > of the IP block that's implemented. Some require just one clock and > > others require two or three. With this patch the driver is just going to > > pick whatever clocks are given in device tree, but it removes any > > possibility of detecting whether the device trees contain the correct > > clocks. So we may very well run into a situation where the driver now > > successfully probes but then malfunctions because one or more of the > > clocks were not specified in device tree. > > > > Thierry > > I still failed to get this. Are you suggesting that CCF bulk > operations are fundamentally broken? No, I'm not suggesting that. All I'm saying is the way that they are used here is causing the driver to behave differently that it was before. Taking for example the VI I2C controller instantiation. That requires the "slow" clock to be specified. Previously if the VI I2C device tree node didn't have that "slow" clock specified, the I2C driver probe would exit with an error code. After this change it will simply not see the "slow" clock and then just continue without it as if it was optional. In other words, after this patch we have no way of saying which clocks are required and which are optional. They all become optional, basically and the driver would attempt to continue (and most likely hang) if no clocks at all had been specified in device tree. > In the above case one may add more checks. AFAICS is_vi won't be > removed, so can be easily checked. > Basically that for-loop for div_clk is questionable. I agree on that. But we need that one to find which of the clocks is the divider clock so that we can call clk_set_rate() on it later on. It's a bit odd that we'd just continue even if we didn't find the divider clock. I think the CCF does handle this transparently and will just no-op all the calls for NULL clocks, but it still means that we won't be running the I2C bus at the right frequency if the divider clock was not specified in device tree. Thierry
21.09.2020 14:12, Thierry Reding пишет: > On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: > [...] >> It's still possible to add the clk-num checking, but it should be >> unpractical. We could always add it later on if there will be a real >> incident. Do you agree? > > There's also clk_bulk_get(), which allows you to specify the number of > clocks and their consumer IDs that you want to request. That seems like > it would allow us to both avoid the repetitive calls to clk APIs and at > the same time allows us to specify exactly which clocks we need. Would > that not work as a compromise? I'll change to use clk_bulk_get(), thanks.
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 505b5d37077d..c6a29a8069d9 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -165,9 +165,6 @@ enum msg_end_type { * @has_continue_xfer_support: Continue transfer supports. * @has_per_pkt_xfer_complete_irq: Has enable/disable capability for transfer * complete interrupt per packet basis. - * @has_single_clk_source: The I2C controller has single clock source. Tegra30 - * and earlier SoCs have two clock sources i.e. div-clk and - * fast-clk. * @has_config_load_reg: Has the config load register to load the new * configuration. * @clk_divisor_hs_mode: Clock divisor in HS mode. @@ -208,7 +205,6 @@ enum msg_end_type { struct tegra_i2c_hw_feature { bool has_continue_xfer_support; bool has_per_pkt_xfer_complete_irq; - bool has_single_clk_source; bool has_config_load_reg; int clk_divisor_hs_mode; int clk_divisor_std_mode; @@ -236,7 +232,8 @@ struct tegra_i2c_hw_feature { * @hw: Tegra I2C HW feature * @adapter: core I2C layer adapter information * @div_clk: clock reference for div clock of I2C controller - * @fast_clk: clock reference for fast clock of I2C controller + * @clocks: array of I2C controller clocks + * @nclocks: number of clocks in the array * @rst: reset control for the I2C controller * @base: ioremapped registers cookie * @base_phys: physical base address of the I2C controller @@ -265,8 +262,8 @@ struct tegra_i2c_dev { const struct tegra_i2c_hw_feature *hw; struct i2c_adapter adapter; struct clk *div_clk; - struct clk *fast_clk; - struct clk *slow_clk; + struct clk_bulk_data *clocks; + unsigned int nclocks; struct reset_control *rst; void __iomem *base; phys_addr_t base_phys; @@ -662,25 +659,9 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev) if (ret) return ret; - ret = clk_enable(i2c_dev->fast_clk); - if (ret < 0) { - dev_err(i2c_dev->dev, - "Enabling fast clk failed, err %d\n", ret); + ret = clk_bulk_enable(i2c_dev->nclocks, i2c_dev->clocks); + if (ret) return ret; - } - - ret = clk_enable(i2c_dev->slow_clk); - if (ret < 0) { - dev_err(dev, "failed to enable slow clock: %d\n", ret); - goto disable_fast_clk; - } - - ret = clk_enable(i2c_dev->div_clk); - if (ret < 0) { - dev_err(i2c_dev->dev, - "Enabling div clk failed, err %d\n", ret); - goto disable_slow_clk; - } /* * VI I2C device is attached to VE power domain which goes through @@ -691,17 +672,14 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev) if (i2c_dev->is_vi) { ret = tegra_i2c_init(i2c_dev); if (ret) - goto disable_div_clk; + goto disable_clocks; } return 0; -disable_div_clk: - clk_disable(i2c_dev->div_clk); -disable_slow_clk: - clk_disable(i2c_dev->slow_clk); -disable_fast_clk: - clk_disable(i2c_dev->fast_clk); +disable_clocks: + clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); + return ret; } @@ -709,9 +687,7 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) { struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); - clk_disable(i2c_dev->div_clk); - clk_disable(i2c_dev->slow_clk); - clk_disable(i2c_dev->fast_clk); + clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); return pinctrl_pm_select_idle_state(i2c_dev->dev); } @@ -1479,7 +1455,6 @@ static struct i2c_bus_recovery_info tegra_i2c_recovery_info = { static const struct tegra_i2c_hw_feature tegra20_i2c_hw = { .has_continue_xfer_support = false, .has_per_pkt_xfer_complete_irq = false, - .has_single_clk_source = false, .clk_divisor_hs_mode = 3, .clk_divisor_std_mode = 0, .clk_divisor_fast_mode = 0, @@ -1504,7 +1479,6 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = { static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = false, - .has_single_clk_source = false, .clk_divisor_hs_mode = 3, .clk_divisor_std_mode = 0, .clk_divisor_fast_mode = 0, @@ -1529,7 +1503,6 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = { static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = true, - .has_single_clk_source = true, .clk_divisor_hs_mode = 1, .clk_divisor_std_mode = 0x19, .clk_divisor_fast_mode = 0x19, @@ -1554,7 +1527,6 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = { static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = true, - .has_single_clk_source = true, .clk_divisor_hs_mode = 1, .clk_divisor_std_mode = 0x19, .clk_divisor_fast_mode = 0x19, @@ -1579,7 +1551,6 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = { static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = true, - .has_single_clk_source = true, .clk_divisor_hs_mode = 1, .clk_divisor_std_mode = 0x19, .clk_divisor_fast_mode = 0x19, @@ -1604,7 +1575,6 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = { static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = true, - .has_single_clk_source = true, .clk_divisor_hs_mode = 1, .clk_divisor_std_mode = 0x16, .clk_divisor_fast_mode = 0x19, @@ -1629,7 +1599,6 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = { static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { .has_continue_xfer_support = true, .has_per_pkt_xfer_complete_irq = true, - .has_single_clk_source = true, .clk_divisor_hs_mode = 1, .clk_divisor_std_mode = 0x4f, .clk_divisor_fast_mode = 0x3c, @@ -1666,13 +1635,58 @@ static const struct of_device_id tegra_i2c_of_match[] = { }; MODULE_DEVICE_TABLE(of, tegra_i2c_of_match); +static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev) +{ + unsigned int i; + int err; + + err = devm_clk_bulk_get_all(i2c_dev->dev, &i2c_dev->clocks); + if (err < 0) + return err; + + i2c_dev->nclocks = err; + + err = clk_bulk_prepare(i2c_dev->nclocks, i2c_dev->clocks); + if (err) + return err; + + for (i = 0; i < i2c_dev->nclocks; i++) { + if (!strcmp(i2c_dev->clocks[i].id, "div-clk")) { + i2c_dev->div_clk = i2c_dev->clocks[i].clk; + break; + } + } + + if (!i2c_dev->is_multimaster_mode) + return 0; + + err = clk_enable(i2c_dev->div_clk); + if (err) { + dev_err(i2c_dev->dev, "failed to enable div-clk: %d\n", err); + goto unprepare_clocks; + } + + return 0; + +unprepare_clocks: + clk_bulk_unprepare(i2c_dev->nclocks, i2c_dev->clocks); + + return err; +} + +static void tegra_i2c_release_clocks(struct tegra_i2c_dev *i2c_dev) +{ + if (i2c_dev->is_multimaster_mode) + clk_disable(i2c_dev->div_clk); + + clk_bulk_unprepare(i2c_dev->nclocks, i2c_dev->clocks); +} + static int tegra_i2c_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct tegra_i2c_dev *i2c_dev; struct resource *res; - struct clk *div_clk; - struct clk *fast_clk; void __iomem *base; phys_addr_t base_phys; int irq; @@ -1688,21 +1702,12 @@ static int tegra_i2c_probe(struct platform_device *pdev) if (irq < 0) return irq; - div_clk = devm_clk_get(&pdev->dev, "div-clk"); - if (IS_ERR(div_clk)) { - if (PTR_ERR(div_clk) != -EPROBE_DEFER) - dev_err(&pdev->dev, "missing controller clock\n"); - - return PTR_ERR(div_clk); - } - i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); if (!i2c_dev) return -ENOMEM; i2c_dev->base = base; i2c_dev->base_phys = base_phys; - i2c_dev->div_clk = div_clk; i2c_dev->adapter.algo = &tegra_i2c_algo; i2c_dev->adapter.retries = 1; i2c_dev->adapter.timeout = 6 * HZ; @@ -1712,12 +1717,17 @@ static int tegra_i2c_probe(struct platform_device *pdev) i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c"); if (IS_ERR(i2c_dev->rst)) { - dev_err(&pdev->dev, "missing controller reset\n"); + dev_err_probe(&pdev->dev, PTR_ERR(i2c_dev->rst), + "failed to get reset control\n"); return PTR_ERR(i2c_dev->rst); } tegra_i2c_parse_dt(i2c_dev); + ret = tegra_i2c_init_clocks(i2c_dev); + if (ret) + return ret; + i2c_dev->hw = of_device_get_match_data(&pdev->dev); i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node, "nvidia,tegra20-i2c-dvc"); @@ -1729,46 +1739,8 @@ static int tegra_i2c_probe(struct platform_device *pdev) init_completion(&i2c_dev->msg_complete); init_completion(&i2c_dev->dma_complete); - if (!i2c_dev->hw->has_single_clk_source) { - fast_clk = devm_clk_get(&pdev->dev, "fast-clk"); - if (IS_ERR(fast_clk)) { - dev_err(&pdev->dev, "missing fast clock\n"); - return PTR_ERR(fast_clk); - } - i2c_dev->fast_clk = fast_clk; - } - - if (i2c_dev->is_vi) { - i2c_dev->slow_clk = devm_clk_get(dev, "slow"); - if (IS_ERR(i2c_dev->slow_clk)) { - if (PTR_ERR(i2c_dev->slow_clk) != -EPROBE_DEFER) - dev_err(dev, "failed to get slow clock: %ld\n", - PTR_ERR(i2c_dev->slow_clk)); - - return PTR_ERR(i2c_dev->slow_clk); - } - } - platform_set_drvdata(pdev, i2c_dev); - ret = clk_prepare(i2c_dev->fast_clk); - if (ret < 0) { - dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret); - return ret; - } - - ret = clk_prepare(i2c_dev->slow_clk); - if (ret < 0) { - dev_err(dev, "failed to prepare slow clock: %d\n", ret); - goto unprepare_fast_clk; - } - - ret = clk_prepare(i2c_dev->div_clk); - if (ret < 0) { - dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret); - goto unprepare_slow_clk; - } - /* * VI I2C is in VE power domain which is not always on and not * an IRQ safe. So, IRQ safe device can't be attached to a non-IRQ @@ -1785,21 +1757,12 @@ static int tegra_i2c_probe(struct platform_device *pdev) goto put_rpm; } - if (i2c_dev->is_multimaster_mode) { - ret = clk_enable(i2c_dev->div_clk); - if (ret < 0) { - dev_err(i2c_dev->dev, "div_clk enable failed %d\n", - ret); - goto put_rpm; - } - } - if (i2c_dev->hw->supports_bus_clear) i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; ret = tegra_i2c_init_dma(i2c_dev); if (ret < 0) - goto disable_div_clk; + goto put_rpm; ret = tegra_i2c_init(i2c_dev); if (ret) { @@ -1834,20 +1797,10 @@ static int tegra_i2c_probe(struct platform_device *pdev) release_dma: tegra_i2c_release_dma(i2c_dev); -disable_div_clk: - if (i2c_dev->is_multimaster_mode) - clk_disable(i2c_dev->div_clk); - put_rpm: pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); - clk_unprepare(i2c_dev->div_clk); - -unprepare_slow_clk: - clk_unprepare(i2c_dev->slow_clk); - -unprepare_fast_clk: - clk_unprepare(i2c_dev->fast_clk); + tegra_i2c_release_clocks(i2c_dev); return ret; } @@ -1858,16 +1811,10 @@ static int tegra_i2c_remove(struct platform_device *pdev) i2c_del_adapter(&i2c_dev->adapter); - if (i2c_dev->is_multimaster_mode) - clk_disable(i2c_dev->div_clk); - pm_runtime_disable(&pdev->dev); - clk_unprepare(i2c_dev->div_clk); - clk_unprepare(i2c_dev->slow_clk); - clk_unprepare(i2c_dev->fast_clk); - tegra_i2c_release_dma(i2c_dev); + tegra_i2c_release_clocks(i2c_dev); return 0; }
Use clk-bulk helpers and factor out clocks initialization into separate function in order to make code cleaner. The clocks initialization now performed after reset-control initialization in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk helper which doesn't silence this error code. Hence reset_control_get() now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP driver that provides reset controls and BPMP doesn't come up early during boot. Previously rst was protected by the clocks retrieval and now this patch makes dev_err_probe() to be used for the rst error handling. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- 1 file changed, 67 insertions(+), 120 deletions(-)