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

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 2/28/22 17: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?

The RK3568_PD_VO already has HCLK_VO and the domain should be
auto-enabled before HDMI registers are accessed, hence you should do the
opposite and remove the HCLK_VO/P clock from the HDMI DT, not add it. If
the HCLK_VO clock isn't enabled by the domain driver, then you need to
check why. Or am I missing something?

What about DSI and DP? Don't they depend on RK3568_PD_VO as well?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel