Re: [PATCH v2 1/2] clk: sunxi-ng: nkm: consider alternative parent rates when finding rate

[Frank Oltmanns]

Hi Maxime,

On 2023-06-12 at 14:31:21 +0200, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> [[PGP Signed Part:Undecided]]
> On Mon, Jun 12, 2023 at 10:51:52AM +0200, Frank Oltmanns wrote:
>> > @@ -28,12 +68,17 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
>> >  			for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
>>
>> According to the manual M/N has to be <= 3. Therefore we need a
>> different maximum value for the _m-for-loop:
>>
>>         unsigned long max_m = min(3 * _n, nkm->max_m);
>>         for (_m = nkm->min_m; _m <= max_m; _m++) {
>>
>> I suggest that I add an optional member max_mn_ratio to the structs
>> ccu_nkm and _ccu_nkm. Optional meaning: Ignore if 0.
>
> Which workload is affected by this restriction?
>

Firstly, the restriction increases the minimum rate that pll-mipi of the
A64 SoC can use. The rate off pll-mipi is
        pll-video0 * K * N / M

The Allwinner's user manual ([1], p.94) states that the maximum ratio of
M/N (note how numerator and denominator changed) is 3. So, looking back
to the original formula, the N / M part can be at most 1/3. That
effectively limits the minimum rate that pll-mipi can provide to
        min(pll-video0) * 2 * 1 / 3

The minimum rate of pll-video0 is 192 MHz, i.e., the minimum rate for
pll-mipi becomes 128 MHz. Without the restriction, the minimum rate
currently is 24 MHz. It is my (albeit limited) understanding, that is no
real limitation because no panel would request such low rates. I should
also mention that Allwinner states in the user manual ([1], p. 94) that
the rate must be in the 500 MHz - 1.4 GHz range.

Secondly, it decreases the number of options for M for all N <= 6.
Therefore it reduces the number of meaningful NKM combinations from 275
(without the restriction) to 238. (With meaningful combinations, I mean
the combinations that result in a different rate for pll-mipi, e.g.,
K=2, M=1, N=2 is the same as K=4, M=1, N=1). The consequence is that the
precision of pll-mipi is slightly reduced. Note, however, that this loss
of precision is more than offset by the option that pll-mipi can now
"freely" choose its parent rate.

In conclusion, I don't see any real world limitation that this
restriction introduces.

>> >  				unsigned long tmp_rate;
>> >
>> > -				tmp_rate = parent * _n * _k / _m;
>> > -
>> > +				if (parent_hw) {
>> > +					tmp_parent = optimal_parent_rate(rate, _n, _k, _m);
>> > +					tmp_parent = clk_hw_round_rate(parent_hw, tmp_parent);
>> > +				}
>>
>> Another constraint is PLL-VIDEO0 rate / M >= 24 MHz. Therefore we also need:
>>         if (tmp_parent < 24000000 * _m)
>>                 continue;
>>
>> So, we need another optional member min_m_times_parent or, for
>> shortness, maybe min_m_parent. I could use help finding a good name for
>> this.
>
> Again, if it's not causing any harm I'd rather keep the code as simple
> and maintainable as possible.

Unfortunately, in Allwinner's User Manual, I could not find any
consequences of driving the SoC outside its specifications. Maybe
someone with more experience in that area could weigh in here.

I want to highlight, though, that currently (i.e., without this patch
series), the rate of pll-video0 is not set by the kernel. So in most
installations, it probably runs at 294 (u-boot) or 297 MHz (default
rate). That allows for M to be smaller than or equal to 12. But with
this patch series, we set pll-video0 to any rate that pll-mipi requests.
Where previously, people could have hand-crafted rates that were within
the SoC's specification - based on a well-known pll-video0 rate - this
no longer applies.

Therefore, I think we should be careful not to request rates from the
parent that drives pll-mipi outside its specification. What do you think
about implementing the min_m_parent functionality in a separate patch?
That would probably facilitate the assessment of the patch from a
simplicity and maintenance perspective. If we agree on a way forward, I
can still squash it with other patches in a later version of this patch
series. But if you are not convinced, I can simply drop it at any point
- including now ;).

In short, I don't know if violating the restriction causes any harm in a
real-world application. However, Allwinner may have reasons for listing
such limitations, even if they don't tell us the details. Those are my
two cents.

Thanks for your feedback,
  Frank

[1] https://github.com/OLIMEX/OLINUXINO/blob/027087da32d69651a58a4e6193bedadef9c036ec/DOCUMENTS/A64-PDFs/Allwinner%20A64%20User%20Manual%20v1.0.pdf

>
> Maxime
>
> [[End of PGP Signed Part]]