On 3/23/22 11:51, Maxime Ripard wrote:
> Hi,
>
> On Tue, Mar 22, 2022 at 10:05:56PM +0300, Dmitry Osipenko wrote:
>> On 2/25/22 17:35, Maxime Ripard wrote:
>>> When we change a clock minimum or maximum using clk_set_rate_range(),
>>> clk_set_min_rate() or clk_set_max_rate(), the current code will only
>>> trigger a new rate change if the rate is outside of the new boundaries.
>>>
>>> However, a clock driver might want to always keep the clock rate to
>>> one of its boundary, for example the minimum to keep the power
>>> consumption as low as possible.
>>>
>>> Since they don't always get called though, clock providers don't have the
>>> opportunity to implement this behaviour.
>>>
>>> Let's trigger a clk_set_rate() on the previous requested rate every time
>>> clk_set_rate_range() is called. That way, providers that care about the
>>> new boundaries have a chance to adjust the rate, while providers that
>>> don't care about those new boundaries will return the same rate than
>>> before, which will be ignored by clk_set_rate() and won't result in a
>>> new rate change.
>>>
>>> Suggested-by: Stephen Boyd <sboyd@xxxxxxxxxx>
>>> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
>>> ---
>>> drivers/clk/clk.c | 45 ++++++++++++++++----------------
>>> drivers/clk/clk_test.c | 58 +++++++++++++++++++-----------------------
>>> 2 files changed, 49 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index c15ee5070f52..9bc8bf434b94 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -2373,28 +2373,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>>> goto out;
>>> }
>>>
>>> - rate = clk_core_get_rate_nolock(clk->core);
>>> - if (rate < min || rate > max) {
>>> - /*
>>> - * FIXME:
>>> - * We are in bit of trouble here, current rate is outside the
>>> - * the requested range. We are going try to request appropriate
>>> - * range boundary but there is a catch. It may fail for the
>>> - * usual reason (clock broken, clock protected, etc) but also
>>> - * because:
>>> - * - round_rate() was not favorable and fell on the wrong
>>> - * side of the boundary
>>> - * - the determine_rate() callback does not really check for
>>> - * this corner case when determining the rate
>>> - */
>>> -
>>> - rate = clamp(clk->core->req_rate, min, max);
>>> - ret = clk_core_set_rate_nolock(clk->core, rate);
>>> - if (ret) {
>>> - /* rollback the changes */
>>> - clk->min_rate = old_min;
>>> - clk->max_rate = old_max;
>>> - }
>>> + /*
>>> + * Since the boundaries have been changed, let's give the
>>> + * opportunity to the provider to adjust the clock rate based on
>>> + * the new boundaries.
>>> + *
>>> + * We also need to handle the case where the clock is currently
>>> + * outside of the boundaries. Clamping the last requested rate
>>> + * to the current minimum and maximum will also handle this.
>>> + *
>>> + * FIXME:
>>> + * There is a catch. It may fail for the usual reason (clock
>>> + * broken, clock protected, etc) but also because:
>>> + * - round_rate() was not favorable and fell on the wrong
>>> + * side of the boundary
>>> + * - the determine_rate() callback does not really check for
>>> + * this corner case when determining the rate
>>> + */
>>> + rate = clamp(clk->core->req_rate, min, max);
>>> + ret = clk_core_set_rate_nolock(clk->core, rate);
>>> + if (ret) {
>>> + /* rollback the changes */
>>> + clk->min_rate = old_min;
>>> + clk->max_rate = old_max;
>>> }
>>>
>>> out:
>>
>> NVIDIA Tegra30 no longer boots with this change.
>>
>> You can't assume that rate was requested by clk_set_rate() before
>> clk_set_rate_range() is called, see what [1] does.
>
> We don't, and it would be bad indeed.
>
> We even have (multiple) tests to cover that case:
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/tree/drivers/clk/clk_test.c?h=clk-range&id=a9b269310ad9abb2f206fe814fd3afcadddce3aa#n242
>
>> T30 memory rate now drops to min on boot when clk debug range is
>> inited innocuously and CPU no longer can make any progress because
>> display controller takes out whole memory bandwidth.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/memory/tegra/tegra30-emc.c#n1437
>>
>> If clk_set_rate() wasn't ever invoked and req_rate=0, then you must not
>> change the clk rate if it's within the new range. Please revert this
>> patch, thanks.
>
> The whole point of this patch is to give an opportunity to every driver
> to change the rate whenever the boundaries have changed, so we very much
> want to have the option to change it if clk_set_rate() has never been
> called.
>
> However, I think the issue is why req_rate would be 0 in the first
> place?
>
> req_rate is initialized to what recalc_rate returns:
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L3607
>
> So the case where req_rate is 0 shouldn't occur unless you had an
> explicit clk_set_rate to 0, or if your clock was orphaned at some point.
>
> Judging from the code, it seems like the latter is the most plausible.
> Indeed, __clk_core_init() will set req_rate to 0 if the clock is
> orphaned (just like rate and accuracy), and
> clk_core_reparent_orphans_nolock will be in charge of updating them when
> the clock is no longer an orphan.
>
> However, clk_core_reparent_orphans_nolock() will update rate by calling
> __clk_recalc_rate and accuracy by calling __clk_recalc_accuracies, but
> it never sets req_rate.
>
> I'm not sure if this is the right patch, Stephen will tell, but could
> you test:
>
> ------------------------ >8 ------------------------
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9bc8bf434b94..c43340afedee 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3479,6 +3479,7 @@ static void clk_core_reparent_orphans_nolock(void)
> __clk_set_parent_after(orphan, parent, NULL);
> __clk_recalc_accuracies(orphan);
> __clk_recalc_rates(orphan, 0);
> + orphan->req_rate = orphan->rate;
> }
> }
> }
>
> ------------------------ >8 ------------------------
It works, thank you!
Tested-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> # T30 Nexus7
