Re: Re: [PATCH v7 10/24] drm/rockchip: dw_hdmi: Add support for hclk

["zhangqing@rock-chips.com" <zhangqing@rock-chips.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9165 bytes --]

hi，all：

Let me explain the clock dependency：
From the clock tree, pclk_vo0 and hclk_vo0 are completely independent clocks with different parent clocks and different clock frequencies。



But the niu path is :
pclk_vo is dependent on hclk_vo, and the pclk_vo niu goes through  hclk_vo niu.

The clock tree and NIU bus paths are designed independently
So there are three solutions to this problem:
1. DTS adds a reference to Hclk while referencing Pclk.
2, The dependent clock is always on, such as HCLK_VO0, but this is not friendly for the system power.
3. Create a non-clock-tree reference. Clk-link, for example, we have an implementation in our internal branch, but Upstream is not sure how to push it.





张晴
瑞芯微电子股份有限公司
Rockchip Electronics Co.,Ltd
地址：福建省福州市铜盘路软件大道89号软件园A区21号楼
Add:No.21 Building, A District, No.89 Software Boulevard Fuzhou, Fujian 350003, P.R.China
Tel:+86-0591-83991906-8601
邮编：350003
E-mail:elaine.zhang@rock-chips.com
****************************************************************************
保密提示：本邮件及其附件含有机密信息，仅发送给本邮件所指特定收件人。若非该特定收件人，请勿复制、使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件或其他方式即刻告知发件人。福州瑞芯微电子有限公司拥有本邮件信息的著作权及解释权，禁止任何未经授权许可的侵权行为。
IMPORTANT NOTICE: This email is from Fuzhou Rockchip Electronics Co., Ltd .The contents of this email and any attachments may contain information that is privileged, confidential and/or exempt from disclosure under applicable law and relevant NDA. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information is STRICTLY PROHIBITED. Please immediately contact the sender as soon as possible and destroy the material in its entirety in any format. Thank you.
****************************************************************************
 
From: Andy Yan
Date: 2022-03-08 11:31
To: Dmitry Osipenko; Sascha Hauer; Robin Murphy; elaine.zhang
CC: devicetree; Benjamin Gaignard; kernel; Sandy Huang; dri-devel; linux-rockchip; Michael Riesch; Peter Geis; Dmitry Osipenko; linux-arm-kernel; Kever Yang; algea.cao; huangtao
Subject: Re: [PATCH v7 10/24] drm/rockchip: dw_hdmi: Add support for hclk
Hi :
 
On 3/5/22 07:55, Dmitry Osipenko wrote:
> On 3/4/22 17:22, Sascha Hauer wrote:
>> On Wed, Mar 02, 2022 at 12:25:28PM +0100, Sascha Hauer wrote:
>>> On Tue, Mar 01, 2022 at 01:39:31PM +0000, Robin Murphy wrote:
>>>> On 2022-02-28 14:19, Sascha Hauer wrote:
>>>>> On Fri, Feb 25, 2022 at 02:11:54PM +0100, Sascha Hauer wrote:
>>>>>> On Fri, Feb 25, 2022 at 12:41:23PM +0000, Robin Murphy wrote:
>>>>>>> On 2022-02-25 11:10, Dmitry Osipenko wrote:
>>>>>>>> 25.02.2022 13:49, Sascha Hauer пишет:
>>>>>>>>> On Fri, Feb 25, 2022 at 01:26:14PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>> 25.02.2022 10:51, Sascha Hauer пишет:
>>>>>>>>>>> The rk3568 HDMI has an additional clock that needs to be enabled for the
>>>>>>>>>>> HDMI controller to work. The purpose of that clock is not clear. It is
>>>>>>>>>>> named "hclk" in the downstream driver, so use the same name.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Notes:
>>>>>>>>>>>        Changes since v5:
>>>>>>>>>>>        - Use devm_clk_get_optional rather than devm_clk_get
>>>>>>>>>>>
>>>>>>>>>>>     drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++++++++++
>>>>>>>>>>>     1 file changed, 16 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>>>>>>>> index fe4f9556239ac..c6c00e8779ab5 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>>>>>>>> @@ -76,6 +76,7 @@ struct rockchip_hdmi {
>>>>>>>>>>>     const struct rockchip_hdmi_chip_data *chip_data;
>>>>>>>>>>>     struct clk *ref_clk;
>>>>>>>>>>>     struct clk *grf_clk;
>>>>>>>>>>> + struct clk *hclk_clk;
>>>>>>>>>>>     struct dw_hdmi *hdmi;
>>>>>>>>>>>     struct regulator *avdd_0v9;
>>>>>>>>>>>     struct regulator *avdd_1v8;
>>>>>>>>>>> @@ -229,6 +230,14 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>>>>>>>>>     return PTR_ERR(hdmi->grf_clk);
>>>>>>>>>>>     }
>>>>>>>>>>> + hdmi->hclk_clk = devm_clk_get_optional(hdmi->dev, "hclk");
>>>>>>>>>>> + if (PTR_ERR(hdmi->hclk_clk) == -EPROBE_DEFER) {
>>>>>>>>>> Have you tried to investigate the hclk? I'm still thinking that's not
>>>>>>>>>> only HDMI that needs this clock and then the hardware description
>>>>>>>>>> doesn't look correct.
>>>>>>>>> I am still not sure what you mean. Yes, it's not only the HDMI that
>>>>>>>>> needs this clock. The VOP2 needs it as well and the driver handles that.
>>>>>>>> I'm curious whether DSI/DP also need that clock to be enabled. If they
>>>>>>>> do, then you aren't modeling h/w properly AFAICS.
>>>>>>> Assuming nobody at Rockchip decided to make things needlessly inconsistent
>>>>>>> with previous SoCs, HCLK_VOP should be the clock for the VOP's AHB slave
>>>>>>> interface. Usually, if that affected anything other than accessing VOP
>>>>>>> registers, indeed it would smell of something being wrong in the clock tree,
>>>>>>> but in this case I'd also be suspicious of whether it might have ended up
>>>>>>> clocking related GRF registers as well (either directly, or indirectly via
>>>>>>> some gate that the clock driver hasn't modelled yet).
>>>>>> Ok, I am beginning to understand. I verified that hdmi, mipi and dp are
>>>>>> hanging when HCLK_VOP is disabled by disabling that clock via sysfs
>>>>>> using CLOCK_ALLOW_WRITE_DEBUGFS. When it's disabled then the registers
>>>>>> of that units can't be accessed. However, when I disable HCLK_VOP by
>>>>>> directly writing to the gate bit RK3568_CLKGATE_CON(20) then only
>>>>>> accessing VOP registers hangs, the other units stay functional.
>>>>>> So it seems it must be the parent clock which must be enabled. The
>>>>>> parent clock is hclk_vo. This clock should be handled as part of the
>>>>>> RK3568_PD_VO power domain:
>>>>>>
>>>>>> power-domain@RK3568_PD_VO {
>>>>>>                   reg = <RK3568_PD_VO>;
>>>>>>                   clocks = <&cru HCLK_VO>,
>>>>>>                            <&cru PCLK_VO>,
>>>>>>                            <&cru ACLK_VOP_PRE>;
>>>>>>                    pm_qos = <&qos_hdcp>,
>>>>>>                             <&qos_vop_m0>,
>>>>>>                             <&qos_vop_m1>;
>>>>>>                    #power-domain-cells = <0>;
>>>>>>           };
>>>>> Forget this. The clocks in this node are only enabled during enabling or
>>>>> disabling the power domain, they are disabled again immediately afterwards.
>>>>>
>>>>> OK, I need HCLK_VO to access the HDMI registers. I verified that by
>>>>> disabling HCLK_VO at register level (CRU_GATE_CON(20) BIT(1)). The
>>>>> HDMI registers become inaccessible then. This means I'll replace
>>>>> HCLK_VOP in the HDMI node with HCLK_VO. Does this sound sane?
>>>> Well, it's still a mystery hack overall, and in some ways it seems even more
>>>> suspect to be claiming a whole branch of the clock tree rather than a leaf
>>>> gate with a specific purpose. I'm really starting to think that the
>>>> underlying issue here is a bug in the clock driver, or a hardware mishap
>>>> that should logically be worked around by the clock driver, rather than
>>>> individual the consumers.
>>>>
>>>> Does it work if you hack the clock driver to think that PCLK_VO is a child
>>>> of HCLK_VO? Even if that's not technically true, it would seem to
>>>> effectively match the observed behaviour (i.e. all 3 things whose register
>>>> access apparently *should* be enabled by a gate off PCLK_VO, seem to also
>>>> require HCLK_VO).
>>> Yes, that works as expected. I am not sure though if we really want to
>>> go that path. The pclk rates will become completely bogus with this and
>>> should we have to play with the rates in the future we might regret this
>>> step.
>> How do we proceed here? I can include a patch which makes PCLK_VO a
>> child of HCLK_VO if that's what we agree upon.
> Couldn't Andy clarify the actual clock tree structure of the h/w for us?
>
> This will be the best option because datasheet doesn't give the clear
> answer, or at least I couldn't find it. Technically, PCLK indeed should
> be a child of the HCLK in general, so Robin could be right.
 
 
Add our clk expert Elaine, she will share some information about the 
actual clock structure.
 
 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[-- Attachment #1.2: Type: text/html, Size: 19673 bytes --]

[-- Attachment #2: Catch18BF.jpg --]
[-- Type: image/jpeg, Size: 44655 bytes --]

[-- Attachment #3: Catch638B.jpg --]
[-- Type: image/jpeg, Size: 79300 bytes --]

[-- Attachment #4: Catch2D7F.jpg --]
[-- Type: image/jpeg, Size: 55112 bytes --]

[-- Attachment #5: CatchA609.jpg --]
[-- Type: image/jpeg, Size: 82959 bytes --]

[-- Attachment #6: Catch6F90.jpg --]
[-- Type: image/jpeg, Size: 176180 bytes --]