Message-ID: <23563d73-7de1-4316-9dd8-25ae6d66a8de@collabora.com>
Date: Wed, 27 Nov 2024 13:44:43 +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 27/11/24 10:04, CK Hu (胡俊光) ha scritto:
> On Wed, 2024-11-27 at 09:41 +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 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
>>> On Tue, 2024-11-26 at 10:25 +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 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).
>>>
>>> I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
>>> That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
>>> In MT8173. software choose DPI driver to control tvd_clk.
>>> In MT8195, software choose HDMI-PHY driver to control tvd_clk.
>>
>> Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
>> driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
>> that to the HDMITX block (with clk_prepare_enable(tvd_clk)).
>>
>>>
>>> I would like to have the same control flow.
>>> If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
>>> So, is "HDMI-PHY driver to control tvd_clk" better?
>>>
>>
>> I'm not sure I understand this last part, can you please rephrase?
> 
> I would like MT8173 and MT8195 has the same control flow, so keep DPI driver to control tvd_clk in MT8195.
> If it's better to control tvd_clk by HDMI-PHY driver, both MT8173 and MT8195 control tvd_clk by HDMI-PHY driver.
> But we are not able to test MT8173. So MT8173 keep control tvd_clk by DPI driver.
> So control tvd_clk by HDMI-PHY driver is better?
> 

Oh. Okay now I understand what you mean.

Unfortunately, we cannot control the tvd->hdmi gate from the PHY driver... this is
because we do really rely on a specific ungate sequence, and the DPI driver really
does need to control the gating of that CG on its own: when we want to bring up the
DPI+HDMI, we need to:
  - Start with *gated* clocks, so HW is OFF;
  - Call mtk_dpi_bridge_enable() (done by drm framework);
    - There, we now *ungate* the clocks
    - DPI HW is ON -> we reset and write config to DPI registers
      with function mtk_dpi_set_display_mode()
    - We enable the DPI output (set EN in DPI_EN register)

If we move the CG to HDMI PHY, then we have to phy_configure() and phy_enable()
inside of the DPI driver, which is also not really possible and can only be done
in the HDMI driver - and that's because the HDMI driver reads EDID from DDC,
which gives us the wanted pixel clock, and feeds it to the PHY.

In short, there's no way around that, the gating cannot be moved out of DPI driver.

>>
>>>>
>>>>>
>>>>> 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.
>>>
>>> It looks like the #4 could be a separate patch.
>>> The commit message is what you describe here.
>>> And
>>>
>>> if (!dpi->conf->support_hdmi_power_sequence)
>>>        mtk_dpi_power_on();
>>>
>>
>> This means that I'd have to introduce the "hdmi power sequence" before actually
>> introducing the real support for MT8195 HDMI....
>> I honestly don't like that "too much", but it's fine, I don't have *too strong*
>> opinions about that, so I will separate #4 as you suggested for v2.
> 
> This DPI series modification is all about HDMI.
> Maybe merge this series with HDMI series and let the HDMI part in front of DPI part and it's more reasonable.
> 

I have sent the two separately only because I thought it'd be easier for you to
review them .. well, separately.
But .. yes, this series is 99% about HDMI - the only thing that's not related to
HDMI is patch [3/6] which is just a cleanup...

So if you want I can merge the two series into one, that's not a problem at all;
in that case, do you want me to keep the patches as they are, or do you want me
to still split #4 away from this patch?

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

Powered by blists - more mailing lists