[PATCH v7 10/24] drm/rockchip: dw_hdmi: Add support for hclk
Andy Yan
andy.yan at rock-chips.com
Mon Mar 7 19:31:22 PST 2022
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 at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
More information about the Linux-rockchip
mailing list