* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-08-24 9:09 [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Viresh Kumar
@ 2020-08-24 9:17 ` Krzysztof Kozlowski
2020-08-24 11:08 ` Ulf Hansson
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-24 9:17 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
nks, Kukjin Kim, Pavel Machek, Viresh Kumar, georgi.djakov,
linux-arm-kernel
On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <stephan@gerhold.net>
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
>
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
>
> cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
> ... later ...
> of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
> cpu cpu0: _opp_add_static_v2: opp key field not found
> cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
>
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
>
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
>
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
>
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
> EPROBE_DEFER in domain.c, fix NULL return value and reorder
> code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
>
> drivers/base/power/domain.c | 14 +++++----
> drivers/opp/core.c | 53 +++++++++++++++++++-------------
> drivers/opp/of.c | 8 ++---
> drivers/soc/samsung/exynos-asv.c | 2 +-
> 4 files changed, 44 insertions(+), 33 deletions(-)
For Samsung:
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-08-24 9:09 [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Viresh Kumar
2020-08-24 9:17 ` Krzysztof Kozlowski
@ 2020-08-24 11:08 ` Ulf Hansson
2020-08-24 11:39 ` Stephan Gerhold
` (2 subsequent siblings)
4 siblings, 0 replies; 24+ messages in thread
From: Ulf Hansson @ 2020-08-24 11:08 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, linux-samsung-soc, Vincent Guittot,
Stephan Gerhold, Kevin Hilman, Stephen Boyd, Viresh Kumar,
Linux PM, Rafael J. Wysocki, Linux Kernel Mailing List,
Krzysztof Kozlowski, Niklas Cassel, Kukjin Kim, Pavel Machek,
Greg Kroah-Hartman, Georgi Djakov, Linux ARM
On Mon, 24 Aug 2020 at 11:09, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> From: Stephan Gerhold <stephan@gerhold.net>
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
>
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
>
> cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
> ... later ...
> of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
> cpu cpu0: _opp_add_static_v2: opp key field not found
> cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
>
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
>
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
>
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
>
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
> EPROBE_DEFER in domain.c, fix NULL return value and reorder
> code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
>
> drivers/base/power/domain.c | 14 +++++----
> drivers/opp/core.c | 53 +++++++++++++++++++-------------
> drivers/opp/of.c | 8 ++---
> drivers/soc/samsung/exynos-asv.c | 2 +-
> 4 files changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86c..b92bb61550d3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
> if (genpd->set_performance_state) {
> ret = dev_pm_opp_of_add_table(&genpd->dev);
> if (ret) {
> - dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> - ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> + ret);
> goto unlock;
> }
>
> @@ -2054,7 +2055,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
> * state.
> */
> genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
> - WARN_ON(!genpd->opp_table);
> + WARN_ON(IS_ERR(genpd->opp_table));
> }
>
> ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> @@ -2111,8 +2112,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> if (genpd->set_performance_state) {
> ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
> if (ret) {
> - dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> - i, ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> + i, ret);
> goto error;
> }
>
> @@ -2121,7 +2123,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> * performance state.
> */
> genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> - WARN_ON(!genpd->opp_table);
> + WARN_ON(IS_ERR(genpd->opp_table));
> }
>
> genpd->provider = &np->fwnode;
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6978b9218c6e..8c69a764d0a4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1068,7 +1068,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> */
> opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
> if (!opp_table)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> mutex_init(&opp_table->lock);
> mutex_init(&opp_table->genpd_virt_dev_lock);
> @@ -1079,8 +1079,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>
> opp_dev = _add_opp_dev(dev, opp_table);
> if (!opp_dev) {
> - kfree(opp_table);
> - return NULL;
> + ret = -ENOMEM;
> + goto err;
> }
>
> _of_init_opp_table(opp_table, dev, index);
> @@ -1089,16 +1089,21 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> opp_table->clk = clk_get(dev, NULL);
> if (IS_ERR(opp_table->clk)) {
> ret = PTR_ERR(opp_table->clk);
> - if (ret != -EPROBE_DEFER)
> - dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
> - ret);
> + if (ret == -EPROBE_DEFER)
> + goto err;
> +
> + dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> }
>
> /* Find interconnect path(s) for the device */
> ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
> - if (ret)
> + if (ret) {
> + if (ret == -EPROBE_DEFER)
> + goto err;
> +
> dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
> __func__, ret);
> + }
>
> BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
> INIT_LIST_HEAD(&opp_table->opp_list);
> @@ -1107,6 +1112,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> /* Secure the device table modification */
> list_add(&opp_table->node, &opp_tables);
> return opp_table;
> +
> +err:
> + kfree(opp_table);
> + return ERR_PTR(ret);
> }
>
> void _get_opp_table_kref(struct opp_table *opp_table)
> @@ -1129,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
> if (opp_table) {
> if (!_add_opp_dev_unlocked(dev, opp_table)) {
> dev_pm_opp_put_opp_table(opp_table);
> - opp_table = NULL;
> + opp_table = ERR_PTR(-ENOMEM);
> }
> goto unlock;
> }
> @@ -1573,8 +1582,8 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
> struct opp_table *opp_table;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1632,8 +1641,8 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
> struct opp_table *opp_table;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1725,8 +1734,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> int ret, i;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1833,8 +1842,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1901,8 +1910,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
> return ERR_PTR(-EINVAL);
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (!IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1982,8 +1991,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> const char **name = names;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /*
> * If the genpd's OPP table isn't already initialized, parsing of the
> @@ -2153,8 +2162,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> /* Fix regulator count for dynamic OPPs */
> opp_table->regulator_count = 1;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7d9d4455a59e..e39ddcc779af 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> /*
> * OPPs have two version of bindings now. Also try the old (v1)
> @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
> }
>
> opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> ret = _of_add_opp_table_v2(dev, opp_table);
> if (ret)
> diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c
> index 30bb7b7cc769..8abf4dfaa5c5 100644
> --- a/drivers/soc/samsung/exynos-asv.c
> +++ b/drivers/soc/samsung/exynos-asv.c
> @@ -93,7 +93,7 @@ static int exynos_asv_update_opps(struct exynos_asv *asv)
> continue;
>
> opp_table = dev_pm_opp_get_opp_table(cpu);
> - if (IS_ERR_OR_NULL(opp_table))
> + if (IS_ERR(opp_table))
> continue;
>
> if (!last_opp_table || opp_table != last_opp_table) {
> --
> 2.25.0.rc1.19.g042ed3e048af
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-08-24 9:09 [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Viresh Kumar
2020-08-24 9:17 ` Krzysztof Kozlowski
2020-08-24 11:08 ` Ulf Hansson
@ 2020-08-24 11:39 ` Stephan Gerhold
2020-10-15 18:05 ` Sudeep Holla
2020-10-27 22:24 ` Guenter Roeck
4 siblings, 0 replies; 24+ messages in thread
From: Stephan Gerhold @ 2020-08-24 11:39 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Kevin Hilman, Stephen Boyd, Greg Kroah-Hartman,
linux-pm, Rafael J. Wysocki, linux-kernel, Krzysztof Kozlowski,
nks, Kukjin Kim, Pavel Machek, Viresh Kumar, georgi.djakov,
linux-arm-kernel
On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <stephan@gerhold.net>
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
>
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
>
> cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
> ... later ...
> of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
> cpu cpu0: _opp_add_static_v2: opp key field not found
> cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
>
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
>
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
>
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
>
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for
> EPROBE_DEFER in domain.c, fix NULL return value and reorder
> code a bit in core.c, and update exynos-asv.c ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Stephan, I have made some changes to the code. Please try it again and
> lemme know if it works fine.
>
I tested the problem I described in the commit message above
(qcom-cpufreq-nvmem with interconnects) and it works fine
with this patch.
I also tested probe deferal with interconnects with just cpufreq-dt
(second patch), and that works fine as well.
And the changes look good to me too. :)
Thank you!
Stephan
> drivers/base/power/domain.c | 14 +++++----
> drivers/opp/core.c | 53 +++++++++++++++++++-------------
> drivers/opp/of.c | 8 ++---
> drivers/soc/samsung/exynos-asv.c | 2 +-
> 4 files changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86c..b92bb61550d3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
> if (genpd->set_performance_state) {
> ret = dev_pm_opp_of_add_table(&genpd->dev);
> if (ret) {
> - dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> - ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> + ret);
> goto unlock;
> }
>
> @@ -2054,7 +2055,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
> * state.
> */
> genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
> - WARN_ON(!genpd->opp_table);
> + WARN_ON(IS_ERR(genpd->opp_table));
> }
>
> ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> @@ -2111,8 +2112,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> if (genpd->set_performance_state) {
> ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
> if (ret) {
> - dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> - i, ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> + i, ret);
> goto error;
> }
>
> @@ -2121,7 +2123,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> * performance state.
> */
> genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> - WARN_ON(!genpd->opp_table);
> + WARN_ON(IS_ERR(genpd->opp_table));
> }
>
> genpd->provider = &np->fwnode;
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6978b9218c6e..8c69a764d0a4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1068,7 +1068,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> */
> opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
> if (!opp_table)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> mutex_init(&opp_table->lock);
> mutex_init(&opp_table->genpd_virt_dev_lock);
> @@ -1079,8 +1079,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>
> opp_dev = _add_opp_dev(dev, opp_table);
> if (!opp_dev) {
> - kfree(opp_table);
> - return NULL;
> + ret = -ENOMEM;
> + goto err;
> }
>
> _of_init_opp_table(opp_table, dev, index);
> @@ -1089,16 +1089,21 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> opp_table->clk = clk_get(dev, NULL);
> if (IS_ERR(opp_table->clk)) {
> ret = PTR_ERR(opp_table->clk);
> - if (ret != -EPROBE_DEFER)
> - dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
> - ret);
> + if (ret == -EPROBE_DEFER)
> + goto err;
> +
> + dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> }
>
> /* Find interconnect path(s) for the device */
> ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
> - if (ret)
> + if (ret) {
> + if (ret == -EPROBE_DEFER)
> + goto err;
> +
> dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
> __func__, ret);
> + }
>
> BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
> INIT_LIST_HEAD(&opp_table->opp_list);
> @@ -1107,6 +1112,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> /* Secure the device table modification */
> list_add(&opp_table->node, &opp_tables);
> return opp_table;
> +
> +err:
> + kfree(opp_table);
> + return ERR_PTR(ret);
> }
>
> void _get_opp_table_kref(struct opp_table *opp_table)
> @@ -1129,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index)
> if (opp_table) {
> if (!_add_opp_dev_unlocked(dev, opp_table)) {
> dev_pm_opp_put_opp_table(opp_table);
> - opp_table = NULL;
> + opp_table = ERR_PTR(-ENOMEM);
> }
> goto unlock;
> }
> @@ -1573,8 +1582,8 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
> struct opp_table *opp_table;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1632,8 +1641,8 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
> struct opp_table *opp_table;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1725,8 +1734,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> int ret, i;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1833,8 +1842,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1901,8 +1910,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
> return ERR_PTR(-EINVAL);
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (!IS_ERR(opp_table))
> + return opp_table;
>
> /* This should be called before OPPs are initialized */
> if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1982,8 +1991,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> const char **name = names;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (IS_ERR(opp_table))
> + return opp_table;
>
> /*
> * If the genpd's OPP table isn't already initialized, parsing of the
> @@ -2153,8 +2162,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> /* Fix regulator count for dynamic OPPs */
> opp_table->regulator_count = 1;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7d9d4455a59e..e39ddcc779af 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
> int ret;
>
> opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> /*
> * OPPs have two version of bindings now. Also try the old (v1)
> @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
> }
>
> opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> - if (!opp_table)
> - return -ENOMEM;
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
>
> ret = _of_add_opp_table_v2(dev, opp_table);
> if (ret)
> diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c
> index 30bb7b7cc769..8abf4dfaa5c5 100644
> --- a/drivers/soc/samsung/exynos-asv.c
> +++ b/drivers/soc/samsung/exynos-asv.c
> @@ -93,7 +93,7 @@ static int exynos_asv_update_opps(struct exynos_asv *asv)
> continue;
>
> opp_table = dev_pm_opp_get_opp_table(cpu);
> - if (IS_ERR_OR_NULL(opp_table))
> + if (IS_ERR(opp_table))
> continue;
>
> if (!last_opp_table || opp_table != last_opp_table) {
> --
> 2.25.0.rc1.19.g042ed3e048af
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-08-24 9:09 [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Viresh Kumar
` (2 preceding siblings ...)
2020-08-24 11:39 ` Stephan Gerhold
@ 2020-10-15 18:05 ` Sudeep Holla
2020-10-16 4:24 ` Viresh Kumar
2020-10-27 22:24 ` Guenter Roeck
4 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-15 18:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Sudeep Holla,
georgi.djakov, linux-arm-kernel, Viresh Kumar
On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <stephan@gerhold.net>
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
directly. Before this change clk_get(dev..) was allowed to fail and
--EPROBE_DEFER was not an error. We use dev_pm_opp_add to add OPPs
read from the firmware and this change is preventing that.
Sorry for checking this so late, but noticed only when this hit mainline.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-15 18:05 ` Sudeep Holla
@ 2020-10-16 4:24 ` Viresh Kumar
2020-10-16 6:00 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-16 4:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 15-10-20, 19:05, Sudeep Holla wrote:
> OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> directly. Before this change clk_get(dev..) was allowed to fail and
> --EPROBE_DEFER was not an error.
I think the change in itself is fine. We should be returning from
there if we get EPROBE_DEFER. The question is rather why are you
getting EPROBE_DEFER here ?
> We use dev_pm_opp_add to add OPPs
> read from the firmware and this change is preventing that.
>
> Sorry for checking this so late, but noticed only when this hit mainline.
>
> --
> Regards,
> Sudeep
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-16 4:24 ` Viresh Kumar
@ 2020-10-16 6:00 ` Sudeep Holla
2020-10-16 11:12 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-16 6:00 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> On 15-10-20, 19:05, Sudeep Holla wrote:
> > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > directly. Before this change clk_get(dev..) was allowed to fail and
> > --EPROBE_DEFER was not an error.
>
> I think the change in itself is fine. We should be returning from
> there if we get EPROBE_DEFER. The question is rather why are you
> getting EPROBE_DEFER here ?
>
Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
this patch and it worked. I need to check it in detail yet.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-16 6:00 ` Sudeep Holla
@ 2020-10-16 11:12 ` Sudeep Holla
2020-10-16 15:28 ` Stephan Gerhold
2020-10-19 4:58 ` Viresh Kumar
0 siblings, 2 replies; 24+ messages in thread
From: Sudeep Holla @ 2020-10-16 11:12 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Sudeep Holla,
georgi.djakov, linux-arm-kernel, Viresh Kumar
On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > --EPROBE_DEFER was not an error.
> >
> > I think the change in itself is fine. We should be returning from
> > there if we get EPROBE_DEFER. The question is rather why are you
> > getting EPROBE_DEFER here ?
> >
>
> Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> this patch and it worked. I need to check it in detail yet.
>
You confused me earlier. As I said there will be no clock provider
registered for SCMI CPU/Dev DVFS.
opp_table->clk = clk_get(dev, NULL);
will always return -EPROBE_DEFER as there is no clock provider for dev.
But this change now propagates that error to caller of dev_pm_opp_add
which means we can't add opp to a device if there are no clock providers.
This breaks for DVFS which don't operate separately with clocks and
regulators.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-16 11:12 ` Sudeep Holla
@ 2020-10-16 15:28 ` Stephan Gerhold
2020-10-19 4:58 ` Viresh Kumar
1 sibling, 0 replies; 24+ messages in thread
From: Stephan Gerhold @ 2020-10-16 15:28 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Kevin Hilman, Stephen Boyd, Viresh Kumar,
linux-pm, Rafael J. Wysocki, linux-kernel, Krzysztof Kozlowski,
nks, Kukjin Kim, Pavel Machek, Greg Kroah-Hartman, georgi.djakov,
linux-arm-kernel, Viresh Kumar
On Fri, Oct 16, 2020 at 12:12:22PM +0100, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > --EPROBE_DEFER was not an error.
> > >
> > > I think the change in itself is fine. We should be returning from
> > > there if we get EPROBE_DEFER. The question is rather why are you
> > > getting EPROBE_DEFER here ?
> > >
> >
> > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > this patch and it worked. I need to check it in detail yet.
> >
>
> You confused me earlier. As I said there will be no clock provider
> registered for SCMI CPU/Dev DVFS.
> opp_table->clk = clk_get(dev, NULL);
> will always return -EPROBE_DEFER as there is no clock provider for dev.
Shouldn't it return -ENOENT if there is no clock for the SCMI case?
With -EPROBE_DEFER I would expect that it shows up at some point.
I tried to avoid a situation like this by ignoring all errors
!= -EPROBE_DEFER. So if clk_get(dev, NULL) would return -ENOENT
everything should work as expected...
Thanks,
Stephan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-16 11:12 ` Sudeep Holla
2020-10-16 15:28 ` Stephan Gerhold
@ 2020-10-19 4:58 ` Viresh Kumar
2020-10-19 9:17 ` Sudeep Holla
1 sibling, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-19 4:58 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 16-10-20, 12:12, Sudeep Holla wrote:
> On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > --EPROBE_DEFER was not an error.
> > >
> > > I think the change in itself is fine. We should be returning from
> > > there if we get EPROBE_DEFER. The question is rather why are you
> > > getting EPROBE_DEFER here ?
> > >
> >
> > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > this patch and it worked. I need to check it in detail yet.
> >
>
> You confused me earlier. As I said there will be no clock provider
> registered for SCMI CPU/Dev DVFS.
> opp_table->clk = clk_get(dev, NULL);
> will always return -EPROBE_DEFER as there is no clock provider for dev.
> But this change now propagates that error to caller of dev_pm_opp_add
> which means we can't add opp to a device if there are no clock providers.
> This breaks for DVFS which don't operate separately with clocks and
> regulators.
The CPUs DT node shouldn't have a clock property in such a case and I
would expect an error instead of EPROBE_DEFER then. Isn't it ?
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 4:58 ` Viresh Kumar
@ 2020-10-19 9:17 ` Sudeep Holla
2020-10-19 9:24 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-19 9:17 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Sudeep Holla,
georgi.djakov, linux-arm-kernel, Viresh Kumar
On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> On 16-10-20, 12:12, Sudeep Holla wrote:
> > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > --EPROBE_DEFER was not an error.
> > > >
> > > > I think the change in itself is fine. We should be returning from
> > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > getting EPROBE_DEFER here ?
> > > >
> > >
> > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > this patch and it worked. I need to check it in detail yet.
> > >
> >
> > You confused me earlier. As I said there will be no clock provider
> > registered for SCMI CPU/Dev DVFS.
> > opp_table->clk = clk_get(dev, NULL);
> > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > But this change now propagates that error to caller of dev_pm_opp_add
> > which means we can't add opp to a device if there are no clock providers.
> > This breaks for DVFS which don't operate separately with clocks and
> > regulators.
>
> The CPUs DT node shouldn't have a clock property in such a case and I
> would expect an error instead of EPROBE_DEFER then. Isn't it ?
Ideally yes, but for legacy reasons clocks property has been used for
providing OPP/DVFS handle too. While we can change and add new property
for that, it will still break old bindings.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 9:17 ` Sudeep Holla
@ 2020-10-19 9:24 ` Viresh Kumar
2020-10-19 10:12 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-19 9:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 19-10-20, 10:17, Sudeep Holla wrote:
> On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> > On 16-10-20, 12:12, Sudeep Holla wrote:
> > > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > > --EPROBE_DEFER was not an error.
> > > > >
> > > > > I think the change in itself is fine. We should be returning from
> > > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > > getting EPROBE_DEFER here ?
> > > > >
> > > >
> > > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > > this patch and it worked. I need to check it in detail yet.
> > > >
> > >
> > > You confused me earlier. As I said there will be no clock provider
> > > registered for SCMI CPU/Dev DVFS.
> > > opp_table->clk = clk_get(dev, NULL);
> > > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > > But this change now propagates that error to caller of dev_pm_opp_add
> > > which means we can't add opp to a device if there are no clock providers.
> > > This breaks for DVFS which don't operate separately with clocks and
> > > regulators.
> >
> > The CPUs DT node shouldn't have a clock property in such a case and I
> > would expect an error instead of EPROBE_DEFER then. Isn't it ?
>
> Ideally yes, but for legacy reasons clocks property has been used for
> providing OPP/DVFS handle too. While we can change and add new property
> for that, it will still break old bindings.
I am not sure I understood it all. So does your platform have the
clock-names property or not for the CPUs ? And how will something
break here ?
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 9:24 ` Viresh Kumar
@ 2020-10-19 10:12 ` Sudeep Holla
2020-10-19 10:35 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-19 10:12 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On Mon, Oct 19, 2020 at 02:54:11PM +0530, Viresh Kumar wrote:
> On 19-10-20, 10:17, Sudeep Holla wrote:
> > On Mon, Oct 19, 2020 at 10:28:27AM +0530, Viresh Kumar wrote:
> > > On 16-10-20, 12:12, Sudeep Holla wrote:
> > > > On Fri, Oct 16, 2020 at 07:00:21AM +0100, Sudeep Holla wrote:
> > > > > On Fri, Oct 16, 2020 at 09:54:34AM +0530, Viresh Kumar wrote:
> > > > > > On 15-10-20, 19:05, Sudeep Holla wrote:
> > > > > > > OK, this breaks with SCMI which doesn't provide clocks but manage OPPs
> > > > > > > directly. Before this change clk_get(dev..) was allowed to fail and
> > > > > > > --EPROBE_DEFER was not an error.
> > > > > >
> > > > > > I think the change in itself is fine. We should be returning from
> > > > > > there if we get EPROBE_DEFER. The question is rather why are you
> > > > > > getting EPROBE_DEFER here ?
> > > > > >
> > > > >
> > > > > Ah OK, I didn't spend too much time, saw -EPROBE_DEFER, just reverted
> > > > > this patch and it worked. I need to check it in detail yet.
> > > > >
> > > >
> > > > You confused me earlier. As I said there will be no clock provider
> > > > registered for SCMI CPU/Dev DVFS.
> > > > opp_table->clk = clk_get(dev, NULL);
> > > > will always return -EPROBE_DEFER as there is no clock provider for dev.
> > > > But this change now propagates that error to caller of dev_pm_opp_add
> > > > which means we can't add opp to a device if there are no clock providers.
> > > > This breaks for DVFS which don't operate separately with clocks and
> > > > regulators.
> > >
> > > The CPUs DT node shouldn't have a clock property in such a case and I
> > > would expect an error instead of EPROBE_DEFER then. Isn't it ?
> >
> > Ideally yes, but for legacy reasons clocks property has been used for
> > providing OPP/DVFS handle too. While we can change and add new property
> > for that, it will still break old bindings.
>
> I am not sure I understood it all. So does your platform have the
> clock-names property or not for the CPUs ? And how will something
> break here ?
>
Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
by any clock provider driver. E.g. the issue you will see if "clocks"
property is used instead of "qcom,freq-domain" on Qcom parts.
On SCMI, we have used clocks property to represent perf domain which I
understand is not ideal but it is there 🙁.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 10:12 ` Sudeep Holla
@ 2020-10-19 10:35 ` Viresh Kumar
2020-10-19 14:10 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-19 10:35 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 19-10-20, 11:12, Sudeep Holla wrote:
> Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> by any clock provider driver. E.g. the issue you will see if "clocks"
> property is used instead of "qcom,freq-domain" on Qcom parts.
Okay, I understand. But what I still don't understand is why it fails
for you. You have a clocks property in DT for the CPU, the OPP core
tries to get it and will get deferred-probed, which will try probing
at a later point of time and it shall work then. Isn't it ?
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 10:35 ` Viresh Kumar
@ 2020-10-19 14:10 ` Sudeep Holla
2020-10-20 5:05 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-19 14:10 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> On 19-10-20, 11:12, Sudeep Holla wrote:
> > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > by any clock provider driver. E.g. the issue you will see if "clocks"
> > property is used instead of "qcom,freq-domain" on Qcom parts.
>
> Okay, I understand. But what I still don't understand is why it fails
> for you. You have a clocks property in DT for the CPU, the OPP core
> tries to get it and will get deferred-probed, which will try probing
> at a later point of time and it shall work then. Isn't it ?
>
Nope unfortunately. We don't have clock provider, so clk_get will
never succeed and always return -EPROBE_DEFER.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-19 14:10 ` Sudeep Holla
@ 2020-10-20 5:05 ` Viresh Kumar
2020-10-20 5:54 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-20 5:05 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 19-10-20, 15:10, Sudeep Holla wrote:
> On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > property is used instead of "qcom,freq-domain" on Qcom parts.
> >
> > Okay, I understand. But what I still don't understand is why it fails
> > for you. You have a clocks property in DT for the CPU, the OPP core
> > tries to get it and will get deferred-probed, which will try probing
> > at a later point of time and it shall work then. Isn't it ?
> >
>
> Nope unfortunately. We don't have clock provider, so clk_get will
> never succeed and always return -EPROBE_DEFER.
Now this is really bad, you have a fake clocks property, how is the
OPP core supposed to know it ? Damn.
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-20 5:05 ` Viresh Kumar
@ 2020-10-20 5:54 ` Viresh Kumar
2020-10-20 9:37 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-20 5:54 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 20-10-20, 10:35, Viresh Kumar wrote:
> On 19-10-20, 15:10, Sudeep Holla wrote:
> > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > >
> > > Okay, I understand. But what I still don't understand is why it fails
> > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > tries to get it and will get deferred-probed, which will try probing
> > > at a later point of time and it shall work then. Isn't it ?
> > >
> >
> > Nope unfortunately. We don't have clock provider, so clk_get will
> > never succeed and always return -EPROBE_DEFER.
>
> Now this is really bad, you have a fake clocks property, how is the
> OPP core supposed to know it ? Damn.
What about instead of fixing the OPP core, which really is doing the
right thing, we fix your driver (as you can't fix the DT) and add a
dummy CPU clk to make it all work ?
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-20 5:54 ` Viresh Kumar
@ 2020-10-20 9:37 ` Sudeep Holla
2020-10-20 9:41 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-20 9:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Sudeep Holla,
georgi.djakov, linux-arm-kernel, Viresh Kumar
On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> On 20-10-20, 10:35, Viresh Kumar wrote:
> > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > >
> > > > Okay, I understand. But what I still don't understand is why it fails
> > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > tries to get it and will get deferred-probed, which will try probing
> > > > at a later point of time and it shall work then. Isn't it ?
> > > >
> > >
> > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > never succeed and always return -EPROBE_DEFER.
> >
> > Now this is really bad, you have a fake clocks property, how is the
> > OPP core supposed to know it ? Damn.
>
> What about instead of fixing the OPP core, which really is doing the
> right thing, we fix your driver (as you can't fix the DT) and add a
> dummy CPU clk to make it all work ?
>
I really would avoid that. I would rather change the binding as there is
no single official users of that binding in the upstream tree.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-20 9:37 ` Sudeep Holla
@ 2020-10-20 9:41 ` Viresh Kumar
2020-10-20 9:52 ` Sudeep Holla
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-20 9:41 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 20-10-20, 10:37, Sudeep Holla wrote:
> On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > >
> > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > at a later point of time and it shall work then. Isn't it ?
> > > > >
> > > >
> > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > never succeed and always return -EPROBE_DEFER.
> > >
> > > Now this is really bad, you have a fake clocks property, how is the
> > > OPP core supposed to know it ? Damn.
> >
> > What about instead of fixing the OPP core, which really is doing the
> > right thing, we fix your driver (as you can't fix the DT) and add a
> > dummy CPU clk to make it all work ?
> >
>
> I really would avoid that. I would rather change the binding as there is
> no single official users of that binding in the upstream tree.
But how will you solve backward compatibility thing then ?
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-20 9:41 ` Viresh Kumar
@ 2020-10-20 9:52 ` Sudeep Holla
2020-10-20 9:59 ` Viresh Kumar
0 siblings, 1 reply; 24+ messages in thread
From: Sudeep Holla @ 2020-10-20 9:52 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Sudeep Holla,
georgi.djakov, linux-arm-kernel, Viresh Kumar
On Tue, Oct 20, 2020 at 03:11:34PM +0530, Viresh Kumar wrote:
> On 20-10-20, 10:37, Sudeep Holla wrote:
> > On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > > >
> > > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > > at a later point of time and it shall work then. Isn't it ?
> > > > > >
> > > > >
> > > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > > never succeed and always return -EPROBE_DEFER.
> > > >
> > > > Now this is really bad, you have a fake clocks property, how is the
> > > > OPP core supposed to know it ? Damn.
> > >
> > > What about instead of fixing the OPP core, which really is doing the
> > > right thing, we fix your driver (as you can't fix the DT) and add a
> > > dummy CPU clk to make it all work ?
> > >
> >
> > I really would avoid that. I would rather change the binding as there is
> > no single official users of that binding in the upstream tree.
>
> But how will you solve backward compatibility thing then ?
>
I am just betting on the fact that no users upstream means no backward
compatibility needed. If someone raises issue we need to add backward
compatibility with dummy clk as you suggested.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-20 9:52 ` Sudeep Holla
@ 2020-10-20 9:59 ` Viresh Kumar
0 siblings, 0 replies; 24+ messages in thread
From: Viresh Kumar @ 2020-10-20 9:59 UTC (permalink / raw)
To: Sudeep Holla
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 20-10-20, 10:52, Sudeep Holla wrote:
> On Tue, Oct 20, 2020 at 03:11:34PM +0530, Viresh Kumar wrote:
> > On 20-10-20, 10:37, Sudeep Holla wrote:
> > > On Tue, Oct 20, 2020 at 11:24:32AM +0530, Viresh Kumar wrote:
> > > > On 20-10-20, 10:35, Viresh Kumar wrote:
> > > > > On 19-10-20, 15:10, Sudeep Holla wrote:
> > > > > > On Mon, Oct 19, 2020 at 04:05:35PM +0530, Viresh Kumar wrote:
> > > > > > > On 19-10-20, 11:12, Sudeep Holla wrote:
> > > > > > > > Yes it has clocks property but used by SCMI(for CPUFreq/DevFreq) and not
> > > > > > > > by any clock provider driver. E.g. the issue you will see if "clocks"
> > > > > > > > property is used instead of "qcom,freq-domain" on Qcom parts.
> > > > > > >
> > > > > > > Okay, I understand. But what I still don't understand is why it fails
> > > > > > > for you. You have a clocks property in DT for the CPU, the OPP core
> > > > > > > tries to get it and will get deferred-probed, which will try probing
> > > > > > > at a later point of time and it shall work then. Isn't it ?
> > > > > > >
> > > > > >
> > > > > > Nope unfortunately. We don't have clock provider, so clk_get will
> > > > > > never succeed and always return -EPROBE_DEFER.
> > > > >
> > > > > Now this is really bad, you have a fake clocks property, how is the
> > > > > OPP core supposed to know it ? Damn.
> > > >
> > > > What about instead of fixing the OPP core, which really is doing the
> > > > right thing, we fix your driver (as you can't fix the DT) and add a
> > > > dummy CPU clk to make it all work ?
> > > >
> > >
> > > I really would avoid that. I would rather change the binding as there is
> > > no single official users of that binding in the upstream tree.
> >
> > But how will you solve backward compatibility thing then ?
> >
>
> I am just betting on the fact that no users upstream means no backward
> compatibility needed. If someone raises issue we need to add backward
> compatibility with dummy clk as you suggested.
Okay. I would have done a change in the OPP core to fix the issue, but
the current code looks correct and we shouldn't change it to satisfy
buggy users. I hope that makes sense.
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-08-24 9:09 [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Viresh Kumar
` (3 preceding siblings ...)
2020-10-15 18:05 ` Sudeep Holla
@ 2020-10-27 22:24 ` Guenter Roeck
2020-10-28 4:06 ` Viresh Kumar
4 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2020-10-27 22:24 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
Trying again through different e-mail provider. My previous e-mail
got stuck in spam filters. Apologies if this is received multiple
times.
On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> From: Stephan Gerhold <stephan@gerhold.net>
>
[ ... ]
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> - if (!opp_table)
> - return ERR_PTR(-ENOMEM);
> + if (!IS_ERR(opp_table))
> + return opp_table;
This should have been
if (IS_ERR(opp_table))
return opp_table;
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-27 22:24 ` Guenter Roeck
@ 2020-10-28 4:06 ` Viresh Kumar
2020-10-28 17:29 ` Guenter Roeck
0 siblings, 1 reply; 24+ messages in thread
From: Viresh Kumar @ 2020-10-28 4:06 UTC (permalink / raw)
To: Guenter Roeck
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On 27-10-20, 15:24, Guenter Roeck wrote:
> Trying again through different e-mail provider. My previous e-mail
> got stuck in spam filters. Apologies if this is received multiple
> times.
I received only once :)
> On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> > From: Stephan Gerhold <stephan@gerhold.net>
> >
> [ ... ]
You removed way too much and this lost the context. Keeping the
routine's prototype line would have been useful.
> >
> > opp_table = dev_pm_opp_get_opp_table(dev);
> > - if (!opp_table)
> > - return ERR_PTR(-ENOMEM);
> > + if (!IS_ERR(opp_table))
> > + return opp_table;
>
> This should have been
>
> if (IS_ERR(opp_table))
> return opp_table;
I believe this is the delta from dev_pm_opp_register_set_opp_helper()
?
A proper fix is already pushed in linux-next for this yesterday.
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER
2020-10-28 4:06 ` Viresh Kumar
@ 2020-10-28 17:29 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2020-10-28 17:29 UTC (permalink / raw)
To: Viresh Kumar
Cc: Nishanth Menon, Len Brown, ulf.hansson, linux-samsung-soc,
Vincent Guittot, Stephan Gerhold, Kevin Hilman, Stephen Boyd,
Greg Kroah-Hartman, linux-pm, Rafael J. Wysocki, linux-kernel,
Krzysztof Kozlowski, nks, Kukjin Kim, Pavel Machek, Viresh Kumar,
georgi.djakov, linux-arm-kernel
On Wed, Oct 28, 2020 at 09:36:37AM +0530, Viresh Kumar wrote:
> On 27-10-20, 15:24, Guenter Roeck wrote:
> > Trying again through different e-mail provider. My previous e-mail
> > got stuck in spam filters. Apologies if this is received multiple
> > times.
>
> I received only once :)
>
I think the original was tagged by gmail as spam because it had too many
recipients. It came back with lots of "rejected by recipient", but I think
they just do that to confuse spammers. So likely no one received it.
> > On Mon, Aug 24, 2020 at 02:39:32PM +0530, Viresh Kumar wrote:
> > > From: Stephan Gerhold <stephan@gerhold.net>
> > >
> > [ ... ]
>
> You removed way too much and this lost the context. Keeping the
> routine's prototype line would have been useful.
>
Sorry, the original was a bit more complete.
> > >
> > > opp_table = dev_pm_opp_get_opp_table(dev);
> > > - if (!opp_table)
> > > - return ERR_PTR(-ENOMEM);
> > > + if (!IS_ERR(opp_table))
> > > + return opp_table;
> >
> > This should have been
> >
> > if (IS_ERR(opp_table))
> > return opp_table;
>
> I believe this is the delta from dev_pm_opp_register_set_opp_helper()
> ?
>
Correct.
> A proper fix is already pushed in linux-next for this yesterday.
>
Great, thanks. I didn't notice because yesterday's build didn't include
the fix, and starting today I dropped the one qemu boot test that reported
it (beaglexm) because that doesn't work anymore for other reasons (due to
commit fb2c599f056, more specifically).
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread