Message-ID: <f1d16db0-a7e1-4cfd-85c6-8beef4385701@collabora.com>
Date: Tue, 26 Nov 2024 10:25:24 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: CK Hu (胡俊光) <ck.hu@...iatek.com>,
 "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "tzimmermann@...e.de" <tzimmermann@...e.de>,
 "kernel@...labora.com" <kernel@...labora.com>,
 Jitao Shi (石记涛) <jitao.shi@...iatek.com>,
 "mripard@...nel.org" <mripard@...nel.org>, "simona@...ll.ch"
 <simona@...ll.ch>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
 "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "robh@...nel.org" <robh@...nel.org>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "airlied@...il.com" <airlied@...il.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>
Subject: Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital
 Parallel Interface

Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
> On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>
>>>>
>>>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>>>> Inside of the SoC, this block is directly connected to the HDMI IP.
>>>
>>> In MT8173, DPI0 is directly connected to HDMI.
>>> The first version of this driver is just for MT8173 DPI0.
>>> Does MT8173 DPI0 need this modification?
>>> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
>>>
>>
>> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
>>
>> Please, what would you like to see in the description of this commit?
> 
> This patch does four jobs.
> 
> 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
> 2. Do not set pixel clock for MT8195/MT8188 DPI.
> 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
> 4. Do not power on/off for MT8195/MT8188 DPI.
> 
> Maybe you should break into 4 patches and each one has different reason.

Yeah I thought about that as well, but there's a fundamental issue with splitting
the thing in multiple patches...

For enabling the tvd_clk in a separate patch, there's no problem - however, for the
others....

1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
    not make sense (nor apply, anyway); then
2. We stop setting pixel clock with another patch; then
3. we don't power on/off, etc etc

The problem with doing it like so is that the patch #1 that I described would be
introducing *faulty code*, because the support for that really depends on all of
the others being present (otherwise the block won't work correctly).

So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
the rest wouldn't be clean.

Besides, keep in mind that... actually... for anything else that is not MT8195/88
DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
nothing apart from incrementing (enable) or decrementing (disable) the refcount for
this clock by 1.

This means that the enablement/disablement of tvd_clk is actually important only
for the MT8195/88 DPI and has literally no effect on anything else that is
currently supported by the mtk_dpi driver anyway.

Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
will split that one out...

> 
> For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.

It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
but eventually it is the HDMI driver that tells to the PHY driver what clock it
actually wants.

For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
PHY's PLL output to the HDMI block.

> If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
> If it is hardware reason. just describe the hardware reason.

Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
analyzed that yet - that HDMI-RX should have the same gating technique, but that's
definitely out of scope for this submission).

> 
> For #4, I don't know why DPI do not control power by its self?
> Even though other driver may control the same power, power manager has reference count,
> so each driver could control the same power by its self.

#4 is there both for a SW and for a HW reason.

The HW reason is that the DPI shall be powered on in a specific sequence in regard
to HDMI-TX, due to the setup that is required by both (and ungating clocks before
full configuration happens would lock up the hw block).

The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
in its .atomic_enable() callback, which happens in the wrong sequence in regard to
HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
because for the others it either is the inverse or it does not matter - so for
performance it's okay for it to be like that both on older SoCs and on DPINTF for
95/88) and this means that we *must not* call dpi_power_on() at that time but
we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
*also* for the clock en/dis calls in DPI.

Cheers,
Angelo

> 
> Regards,
> CK
> 
> 
>>
>> Cheers,
>> Angelo
>>
>>> Regards,
>>> CK
>>>
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>>>> ---
>>
>>

Powered by blists - more mailing lists