Message-ID: <bf679eb0-91ff-4701-a43f-891a75db10dc@collabora.com>
Date: Thu, 28 Nov 2024 11:34:53 +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>,
 "simona@...ll.ch" <simona@...ll.ch>,
 "kernel@...labora.com" <kernel@...labora.com>,
 "mripard@...nel.org" <mripard@...nel.org>,
 Jitao Shi (石记涛) <jitao.shi@...iatek.com>,
 "tzimmermann@...e.de" <tzimmermann@...e.de>,
 "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 28/11/24 04:08, CK Hu (胡俊光) ha scritto:
> On Wed, 2024-11-27 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.
>>
>>
>> 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?
> 
> I think each part would use a different config variable to show which code is related to which part.
> So #4 could keep in this patch.
> The commit message should describe all these parts.
> 
> If breakdown, it would be:
> 
> Patch 1: Add config variable 1
> Patch 2: Add config variable 2
> ...
> Patch n: Add support for MT8195 DPI support (add MT8195 config table)
> 
> Both combine into one patch or breakdown to multiple patch is OK for me.
> 

Okay, since both ways are ok for you, I will keep the commits layout as it is,
which is something that I do prefer because it simply makes more sense in my
opinion.

Thank you for the feedback

Cheers!

> Regards,
> CK
> 
>>
>>> 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