Re: [PATCH v9 08/12] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF

[neil.armstrong@linaro.org]

Hi,

On 27/11/2023 09:38, Jerome Brunet wrote:
> 
>>>
>>>>
>>>> I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk ratios,
>>>> since it doesn't exist on AXG. Not sure we would ever need it... and none
>>>> of the other upstream DSI drivers supports such setups.
>>>>
>>>> The main reasons I set only mipi_dsi_pxclk in DT is because :
>>>> 1) the DSI controller requires a bitclk to respond, pclk is not enough
>>>> 2) GP0 is disabled with an invalid config at cold boot, thus we cannot
>>>> rely on a default/safe rate on an initial prepare_enable().
>>>> This permits setting initial valid state for the DSI controller, while
>>>> the actual bitclk and vclk are calculated dynamically with panel/bridge
>>>> runtime parameters.
>>> Nothing against setting rate in DT when it is static. Setting it then
>>> overriding it is not easy to follow.
>>
>> Yup, would be simpler to only have parenting set in DT, since it must
>> stay static, I'm fine trying to move rate setup to code.
>>
>>> To work around GP0 not being set, assuming you want to keep rate
>>> propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o
>>> enabling it) to force a setup on gp0 then clk_prepare_enable() on
>>> pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk.
>>> It is a bit hackish. Might be better to claim gp0 in your driver to
>>> manage it directly, cutting rate propagation above it to control each
>>> branch of the subtree as you need. It seems you need to have control over
>>> that anyway and it would be clear GP0 is expected to belong to DSI.
>>
>> Controlling the PLL from the DSI controller seems violating too much layers,
>> DSI controller driver is not feed directly by the PLL so it's a non-sense
>> regarding DT properties.
> 
> Not sure what you mean by this. You have shown in your the commit
> message that the DSI clocks make significant subtree. I don't see a
> problem if you need to manage the root of that subtree. I'd be great if
> you didn't need to, but it is what it is ...

I really think the choice of PLL should not be done by the DSI controller,
but by the Video pipeline driver or statically until we can do this.

My point is that we should only define the clocks that are related to each
hardware, for example the whole VCLK/VCLK2 clocks should be defined for the
VPU HW, then only the few endpoint clocks should be defined for the HDMI
or DSI controllers, PHY clock and ENCI/ENCP for HDMI, DSI and ENCL for DSI.

The big plan is to entirely switch to CCF for VPU, but first I want to have
DSI working, and since DSI needs GP0, we need CCF for that so the intermediate
plan is to have partial CCF handling only for DSI with fixed clock tree in DT,
then in the future the Meson DRM driver would set up the appropriate clock
tree for HDMI, DSI, Composite and perhaps DP for T7 SoCs then the controller
bridge will call the clk_set_rate() in the same design I did for DSI.

Here's the tracked item: https://gitlab.com/amlogic-foss/mainline-linux-issues-tracker/-/issues/9

CCF clock control is a mandatory item to solved dual-head display: https://gitlab.com/amlogic-foss/mainline-linux-issues-tracker/-/issues/6

> 
>>
>> Setting a safe clock from the DSI controller probe is an idea, but again I
>> don't know which value I should use...
> 
> You mentionned that the problem comes DSI bridges that needs to change
> at runtime. I don't know much about those TBH, but is there
> anyway you can come up with a static GP0 rate that would then be able to
> divide to serve all the rates bridge would need in your use case ?

No, there's no such things in the DSI world, MIPI only specifies the electrical
and transport layer, everything else is custom per vendor.

> 
> GP0 can go a lot higher than ~100MHz and there are dividers unused in the
> tree it seems.
> 
> I suppose there is a finite number of required rate for each use case ?
> If there are not too many and there is a common divider that allows a
> common rate GP0 can do, it would solve your problem. It's a lot of if
> but It is worth checking.
> 
> This is how audio works and DT assigned rate is a good match in this case.

Yeah I know, but I would love it but no...

> 
>>
>> I'll review the clk parenting flags and try to hack something.
>>
>> Thanks,
>> Neil
>>
>>

Thanks,
Neil