Re: [PATCH RFC 0/4] clk: re-set required rates during clk_set_rate()

[Maxime Ripard]

Hi,

On Mon, Oct 02, 2023 at 11:23:31AM +0200, Benjamin Bara wrote:
> This is a spin-off of my initial series[1] with the core-related parts
> picked out. Without the core part, the rest of the series only partly
> makes sense, therefore I wanted to clarify the state of this first.
> That's also the reason why it is marked as RFC for now.
> 
> Background:
> The CCF is currently very rigid in terms of dealing with multiple rate
> changes in a single clk_set_rate() call. However, with the
> CLK_SET_RATE_PARENT property, it is very likely that a shared clock has
> a couple of children which are changed "by accident" when the common
> parent is changed. These children might be clock inputs of hardware
> modules, which might have set a required rate previously. These required
> rates are most likely still expected after the parent has been changed
> by another clock (e.g. a sibling). Currently, it is not very trivial to
> get these required rates inside of a clock driver's
> {determine,round}_rate() op. Therefore, I think the core should also
> participate in the process of ensuring that consumer-set requirements
> are still fulfilled after a rate has changed.
> 
> Idea:
> The idea is to have three rates set per clock, which need to be
> considered during the whole process:
> 
> 1. req_rate: This is the rate required by a consumer. It is set during a
>    clk_set_rate() call.
> 2. new_rate: This is the "new planned rate" for the clock, which it will
>    set, once the "finding new rates" process is finished.
> 3. req_rate_tmp: This rate is set if the clock is "required to change"
>    during the process. It basically means that the clock is an ancestor
>    of a rate-change trigger.
> 
> With these available, the core is able to validate the changed subtree
> in a more simple way. It also allows us to re-enter calc_new_rates(),
> which is one of the key components of clk_set_rate().

There's a couple of things you didn't reply on the first version and you
didn't really expand it here:

  - Most clocks don't care, and only the clocks that have used
    clk_set_rate_exclusive actually care. clk_set_rate never provided
    that guarantee, you're effectively merging clk_set_rate and
    clk_set_rate_exclusive. This might or might not be a good idea (it's
    probably not unless you want to track down regressions forever), but
    we should really tie this to clk_set_rate_exclusive or merge both.

  - Why do we need a new req_rate, and why req_rate can't be changed to
    accomodate your changes.

  - Why do you even need the core to be involved in the first place? You say you
    think it does, but you haven't answered any of my mails to provide more
    details and figure out if we can do it without it.

Maxime

Attachment: signature.asc
Description: PGP signature