Re: [PATCH v8 2/6] clk: renesas: rzv2h-cpg: Add support for DSI clocks

[Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>]

Hi,

On 11/09/2025 11:14, Lad, Prabhakar wrote:
> Hi Tomi,
> 
> On Wed, Sep 10, 2025 at 1:30 PM Tomi Valkeinen
> <tomi.valkeinen+renesas@ideasonboard.com> wrote:
>>
>> Hi,
>>
>> On 03/09/2025 19:17, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>
>>> Add support for PLLDSI and PLLDSI divider clocks.
>>>
>>> Introduce the `renesas-rzv2h-cpg-pll.h` header to centralize and share
>>> PLLDSI related data structures, limits, and algorithms between the
>>> RZ/V2H(P) CPG and DSI drivers.
>>>
>>> The DSI PLL is functionally similar to the CPG's PLLDSI, but has slightly
>>> different parameter limits and omits the programmable divider present in
>>> CPG. To ensure precise frequency calculations, especially for milliHz-level
>>> accuracy needed by the DSI driver, the shared algorithm allows both drivers
>>> to compute PLL parameters consistently using the same logic and input
>>> clock.
>>
>> Can you elaborate a bit more why a new clock APIs are needed for the DSI
>> PLL? This is the first time I have heard a DSI TX (well, any IP) require
>> more precision than Hz. Is that really the case? Are there other reasons?
>>
> Im pasting the same reply from Fab
> (https://lore.kernel.org/all/TYCPR01MB12093A7D99392BC3D6B5E5864C2BC2@TYCPR01MB12093.jpnprd01.prod.outlook.com/#t)
> for the similar concern.
> 
> The PLL found inside the DSI IP is very similar to the PLLDSI found in
> the CPG IP block, although the limits for some of the parameters are

Thanks. As discussed on chat, this confused me: There's a PLLDSI on CPG,
which doesn't provide a DSI clock, but a pixel clock. And then there's a
PLL in the DSI D-PHY which provides the DSI clock.

A few comments overall some for this driver but also the dsi driver:

This hardcodes the refclk rate to 24 MHz with RZ_V2H_OSC_CLK_IN_MEGA in
the header file. That doesn't feel right, shouldn't the refclk rate come
from the clock framework with clk_get_rate()?

While not v2h related, I think it would be good to have a comment in the
dsi driver about how the g2l hs clock rate is derived directly from the
pixel clock.

I still don't see why all the code here has to be in a header file.
Usually headers contain only a few lines of inline code. Is there a
reason why it's not in a .c file?

And last, we discussed the milliHz a bit on chat, you said you'll come
back to that. I don't think it's a blocker, but I'm very curious why
exactly it is needed in the DSI. +/- 12 Hz with, say 3.6GHz clock does
not sound very much to me, and there should be enough time every line
during blanking period to empty any fifos and "catch up".

In fact, if the DSI is so picky about the rate, I find the HW design
odd: in g2l the pixel clock and the DSI clock come from a single source,
which keeps them neatly in sync. If that is required, why change the
design here so that the DSI PLL is independent of the pixel clock, yet
still the DSI PLL must be programmed to be exactly matched to the pixel
clock.

The docs say v2h supports burst mode. Isn't the idea with DSI burst mode
that you run the DSI much faster than the display controller (i.e. with
much higher clock than HSFREQ = (VCLK * bpp) / num_lanes), so that the
DSI can burst the data out from its fifos and then go to sleep? The
separate PLL in v2h allows independent DSI clock config, allowing burst
mode. If the HW can support that, then there shouldn't be a strict
requirement for HSFREQ = (VCLK * bpp) / num_lanes, or hitting the
pclk-hsfreq ratio exactly to milliHz precision.

 Tomi