Re: [PATCH 0/5] clk: Use v2 rate calculation on i.MX8MP

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Jerome,

>>> For rate negotiation, my suggestion would be to reuse what the
>>> framework already has: clock ranges, and re-running .determine_rate()
>>> on the children being dragged along, with the common ancestor's rate
>>> pinned. When that rate does not suit a sibling's range, other ancestor
>>> rates can be tried within the constraints, rather than giving up.
>>>
>>> clk_set_rate() is a best effort function. It tries to get as close to
>>> the request as possible, honoring the ranges of every clock along the
>>> path, and it is up to the consumer to decide whether the result is
>>> good enough. That decision is lost if the framework recomputes the
>>> rates of a sibling branch on its own. Ranges could help it decide:
>>> either "I'm fine with the new rate", or -EBUSY.
>>
>> I see an upside with this approach: we could keep one single rate
>> negotiation logic. But it still requires modifications to be done on a
>> per user basis to be enabled (but that is needed from your perspective).
>>
>> While I fully understand all the concerns expressed above, there has
>> been one minor point that arose while working on your series: it's
>> very hard to determine the tolerance beyond which the clock rate shall
>> be refused. Expect bikeshedding on a per driver basis.
>
> This is my point exactly. Each consumer is going to need to do its own
> thing and it is why I would prefer if the decision was not a core or
> a provider thing.
>
> A decision must be made somewhere. I don't really see where else it
> could be made than with the consumer. It is the only one which has
> any idea what it is trying acheive.

I agree, the consumer must make that choice. But I just wanted to warn
against the risk of endless discussions about what this tolerance should
be in each and every user. Myself, I didn't know what tolerance would be
fine for my own case. 1% seems ok, it leaves enough room for the CCF to
find a working set of dividers without risking a breaking choice, but
maybe some day someone will come with a panel that cannot be supported
with this limit, and maybe 1.5 or 2% would have been okay for
them. Since that's a very empirical limit, expect some back and forth on
those. "Back and forth" also means "more load on clk maintainers".

> If the range is about HW capability (from the clock side I mean),
> a range can be set from the provider. Some already do that and I
> think it is fine for that purpose. In the experiment I've provided,
> I did not enable the propagation logic in that case, on purpose.
> It could constrain the clock tree for clock that are actually
> unused. It still participate in the decision logic if a consumer
> enable it however.
>
>>
>>> What I'm suggesting is to treat any clock with a range as one that
>>> needs updating. Its consumer cared enough to tell us what it can and
>>> cannot cope with. That removes the need for a specific flag, and puts
>>> the rate policy in the consumer, which is where I'd prefer use case
>>> choices to live, rather than in the provider or the core. It would
>>> also fix clocks being dragged out of their range during an update.
>>> Presumably, a clock without a range does not care enough to give us a
>>> decider, so the framework has no basis to act on its behalf and keeps
>>> today's behavior. I think this behavior will keep on being good enough
>>> for the vast majority clocks out there and it is a lot lighter
>>> on the math side (which is already quite severe as it is)
>>
>> Well, yeah, I see we share conclusions :-)
>>
>>> I've been checking whether the idea is remotely feasible. It involves
>>> multiple planning stages and multiple tops. It is still very rough,
>>> but there is the beginning of something:
>>>
>>> https://github.com/jbrun3t/linux/commits/clk/clk-rate-range-update/
>>>
>>> Miquel, if you fancy trying it, please let me know whether it works
>>> for you. All you should need is to set a range on the clocks you care
>>> about.
>>
>> I managed to pull most of the clock patches in your branch and apply
>> them to my 7.2 branch (with a couple of tweaks). I then made the changes
>> to request a tolerance as you hinted (available below) in the two
>> relevant drivers: it worked as expected!
>>
>> While doing so, I realized the whole operation (set rate, set rate
>> range, get rate and verify) needs to be repeated almost identically in
>> every user of the "tolerance composition" logic, which probably
>> indicates that a core helper would be relevant instead. I had in mind
>> something like ~clk_set_rate_and_tolerance() which would do the
>> equivalent of my fsl_ldb_link_rate_set() and lcdif_pixel_rate_set()
>> helpers.
>
> Indeed, I suppose an helper could be introduced for that if we don't
> already have one. It does not even depend on the experiment presented
> here. As long as there is a user for it, it would be fine IMO.
>
> Maybe something that takes a rate and a ratio, like the duty cycle API
> ?

Definitely.

>>> Sorry for this rather long explanation.
>>>
>>> Brian, your series is what got this discussion going, I know a lot of
>>> efforts have gone into it already. I'd be glad to work on this together.
>>
>> In case that is relevant, here is below the code snippet which I used to
>> get my panels working after removing the assigned rates from the DTS on
>> i.MX8MP.

I'll let you two discuss about the direction you prefer taking. From my
point of view there are good and bad sides on both, but they also both
work in my case.

Let me know once settled and I will post an adapted patch series on top
of it to adopt the selected solution in the LDB and LCDIF cases (+ the
core helper discussed above, if relevant).

Thanks,
Miquèl