Hi Tomi, On Thu, Sep 11, 2025 at 3:26 PM Tomi Valkeinen <[email protected]> wrote: > > Hi, > > On 11/09/2025 11:14, Lad, Prabhakar wrote: > > Hi Tomi, > > > > On Wed, Sep 10, 2025 at 1:30 PM Tomi Valkeinen > > <[email protected]> wrote: > >> > >> Hi, > >> > >> On 03/09/2025 19:17, Prabhakar wrote: > >>> From: Lad Prabhakar <[email protected]> > >>> > >>> 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/tycpr01mb12093a7d99392bc3d6b5e5864c2...@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()? > >From the CPG perspective we could get the info with clk_get_rate() but from the DSI part we can't. So to keep it consistent I will keep this macro as is.
> 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? > Ok, I will move the functions to rzv2h-cpg.c and export the symbols and have the declarations in include/linux/clk/renesas.h. Geert are you OK with the above? > 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. > As discussed on irc we have to live with mHz solution as the HW is picky. Cheers, Prabhakar
