Message-ID: <aba80c42-1b48-426f-b29d-88bf18473602@nxp.com>
Date: Wed, 18 Dec 2024 14:02:18 +0800
From: Liu Ying <victor.liu@....com>
To: Maxime Ripard <mripard@...nel.org>
Cc: imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
 festevam@...il.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, catalin.marinas@....com, will@...nel.org,
 abelvesa@...nel.org, peng.fan@....com, mturquette@...libre.com,
 sboyd@...nel.org, andrzej.hajda@...el.com, neil.armstrong@...aro.org,
 rfoss@...nel.org, Laurent.pinchart@...asonboard.com, jonas@...boo.se,
 jernej.skrabec@...il.com, maarten.lankhorst@...ux.intel.com,
 tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
 quic_bjorande@...cinc.com, geert+renesas@...der.be,
 dmitry.baryshkov@...aro.org, arnd@...db.de, nfraprado@...labora.com,
 marex@...x.de
Subject: Re: [PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

On 12/17/2024, Maxime Ripard wrote:
> Hi,

Hi,

> 
> Thanks for the description, I have several questions here.

Thanks for the feedback!

> 
> On Thu, Nov 14, 2024 at 02:57:52PM +0800, Liu Ying wrote:
>> This patch series aims to add ITE IT6263 LVDS to HDMI converter on
>> i.MX8MP EVK.
>>
>> Since IT6263 DT binding and driver were picked up from v5 and landed
>> in drm-misc, this patch series contains patches almost all i.MX8MP
>> SoC/platform specific.
>>
>> Patch 1 is a preparation patch to allow display mode of an existing
>> panel to pass the added mode validation logic in patch 3.
>>
>> Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
>> media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.
>>
>> Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
>> the next non-panel bridge, that is the IT6263 in this case.
>>
>> Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
>> "ldb" clock so that it can filter out unsupported display modes read
>> from EDID.
>>
>> Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
>> "pix" clock so that it can filter out display modes which are not
>> supported by pixel clock tree.
>>
>> Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
>> populated.
>>
>> Patch 7 enables the IT6263 bridge driver in defconfig.
>>
>> Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.
>>
>> Since this patch series is related to another one[4] authored by Marek,
>> Maxime asked for a proper description[5] about the exact problem.
>>
>> Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
>> a bit more.
>>
>> [ Description ]
>> It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
>> LVDS display pipelines.  The pipelines are driven by separate DRM driver
>> instances, hence there is no way to negotiate a dynamically changeable
>> PLL rate to satisfy both of them.  The only solution is to assign a
>> sensible/unchangeable clock rate for the PLL in DT.
>>
>> Admittedly, sys_pll3_out can be another clock source to derive pixel clock
>> for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
>> doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
>> at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
>> the shared video PLL case has to be handled and it determines that the above
>> solution(unchangeable PLL rate assigned in DT) has to be used no matter
>> sys_pll3_out is for display or audio, because the separate DRM driver
>> instances really don't know if they are sharing the video PLL or not.
>>
>> [[ i.MX8MP Display Hardware ]]
>> i.MX8MP SoC supports three display pipelines:
>>
>>  -----------------------------           ------------------------
>> | imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
>>  -----------------------------           ------------------------
>> |  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
>> |*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
>> |  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
>> |  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
>> | -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
>> | -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
>> |  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
>> |  clk_ext4 (external)        |         |*-video_pll1_out (video)|
>>  -----------------------------           ------------------------
>>              ||                                     |
>>  -----------------------------           ------------------------
>> |   media_disp{1,2}_pix       |         |        media_ldb       |
>>  ----------------------------- mux+div   ------------------------ mux+div
>>              ||                                     |
>>  -----------------------------           ------------------------
>> | media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
>>  ----------------------------- gate      ------------------------ gate
>>              ||                                     | (LVDS serial clock)
>>              ||                                     V
>> 	     || (Disp2 Pclk)  --------      ------------------
>> 	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
>> 	     |                --------      ------------------
>> 	     |  (Disp1 Pclk)  --------      ------------------
>> 	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
>> 	                      --------      ------------------
>>                               --------      ------------------      ----------
>>                              | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
>>                               --------      ------------------     |     +    |
>>                                  ^                                 |    PLL   |
>>                                  |                                  ----------
>>                                  | (Disp3 pclk)                         | |
>>                                   --------------------------------------  |
>>                                                                           V
>>                                                                     panel/bridge
>>
>> * video_pll1_out is supposed to be used by video outputs.
>>
>> - LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
>>   sys_pll2_1000m.
>>
>> [[ i.MX8MP Display Drivers ]]
>> LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
>> Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
>> separately.
>>
>> LDB: drivers/gpu/drm/bridge/fsl-ldb.c
>>
>> Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c
>>
>> Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>
>> [[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
>> osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
>> because:
>> a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
>> b. Audio subsystem may consume all audio_pll*.
>> c. sys_pll* are system clocks which are supposed to run at fixed typical
>>    rates, e.g., sys_pll2_1000m runs at 1000MHz.
>> d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
>>    600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
>>    https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx8mp-evk-ndm.dts#L19
>>    https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx8mp-ddr4-evk.dts#L25
> 
> Is there any downside to using any of these clocks, aside from the fact
> that their rate must not change?

osc_24m and sys_pll* don't support spread spectrum while audio_pll* and
video_pll1_out support it. Other than that, I don't see any downside to
use any of these clocks, if their clock rates happen to meet the clock
rate requirements of the MIPI DSI and LDB display pipelines.

> 
> Also, if they can't change their rate, why do they have
> CLK_SET_RATE_PARENT (sys_pll* in particular) ?

If media_disp{1,2}_pix and media_ldb clocks have no CLK_SET_RATE_PARENT,
it doesn't mattter whether their parent clocks(these clocks) have it or not.
Note that patch 2 drops CLK_SET_RATE_PARENT for media_disp{1,2}_pix clocks.

Anyway, why have sys_pll* clocks got CLK_SET_RATE_PARENT? The reason I can
think of is that it makes some potential i.MX8MP platforms possible to set
the sys_pll* rates with the parent clock rates(sys_pll1/2/3) updated
accordingly, e.g., if sys_pll2_1000m is the only active child of sys_pll2_out,
sys_pll2_1000m's rate can be assigned to 800MHz(not typical 1000MHz) in DT.
Or, maybe, the sys_pll* rates can be assigned to some particular values to
support nominal/overdrive modes of various i.MX8MP clock roots(some are
derived from the sys_pll* clocks).

> 
>> e. clk_ext* are external clocks without known capabilities.
>>
>> So, the only eligible video_pll1_out is supposed to be shared between LDB
>> and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
>> is already used to derive the audio AXI clock.
>>
>> With the shared video_pll1_out, drivers for the two display pipelines cannot
>> change the PLL clock rate in runtime, since the pipelines are driven by two
>> DRM driver instances.
> 
> What is the typicall frequency on those pipelines? Could setting the PLL

For MIPI DSI to HDMI(ADV7535) and LVDS to HDMI(IT6263), the typical pixel
rates are 148.5MHz(1080p60Hz) and 74.25MHz(720p60Hz) and the typical LDB
clock rate is 519.75MHz.

For MIPI DSI panel and LVDS panel, there no typical pixel rates. It depends
on individual panels. However, it's possible to override panel's pixel clock
rate in DT to use a fixed PLL clock rate if the pixel clock rate deviation
is still acceptable by the panel.

> high enough that any frequency required by any of these pipelines can be
> accomodated through a divider work?

Yes, that's why media_blk_ctrl node in imx8mp.dtsi assigns video_pll1 clock
rate to 1.0395GHz. That rate supports the typical 148.5MHz and 74.25MHz
pixel clock rates. With this patch series applied, i.MX8MP EVK would use
this fixed 1.0395GHz video_pll1 clock to drive both MIPI DSI to HDMI and
LVDS to HDMI simultaneously.

> 
> Something like you run the PLL at 594MHz, and then most HDMI frequencies
> can be reached by a 1, 2 or 4 divider.

PLL running at 594MHz does support the typical pixel clock rates for MIPI
DSI to HDMI, but not for LVDS to HDMI due to the 7x(single-LVDS link) or
3.5x(dual-LVDS link) folder between LDB clock rate and pixel clock rate.
PLL running at 1.0395GHz is the one chosen to support both MIPI DSI to
HDMI and LVDS to HDMI, e.g., with dual-LVDS link, 148.5MHz pixel clock rate
= 1.0395GHz / 7 and 519.75MHz LDB clock rate(148.5MHz * 3.5) = 1.0395GHz / 2.

> 
>> [[ Solution ]]
>> Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
>> display drivers to change the PLL clock source(s) or rate(s) in runtime
>> including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
>> typical display modes like 1920x1080@60 can be supported if external HDMI
>> bridges are connected, and panel display modes can be too.  Also the unneeded
>> CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
>> If needed, bridge drivers just call clk_round_rate() to validate clocks so
>> that unsupported display modes can be filtered out.  Although the
>> unchangeable PLL clock rate disallows more potential display modes, the
>> solution works for single/dual/triple display pipelines(OFC, hardware designers
>> should pick panel/bridge display devices carefully first by considering clock
>> resources).
> 
> I think it's a reasonable idea, if not for the hardcode-it it DT stuff.
> If we can manage to have a fixed setup work ok for all display use
> cases, why would it be in DT? The clock driver seems like a much better
> choice to me.

Different i.MX8MP platforms may have different display devices(panel(s)
and/or bridge(s)). It's flexible to allow each platform to assign the PLL
rate(s) in DT. It doesn't look doable with clock driver, does it?

> 
> Maxime

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists