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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Mar 01, 2022 at 01:56:59AM +0300, Dmitry Osipenko wrote:
> 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,

As said, the clocks given in the power domain are only enabled during
the process of enabling/disabling the power domain and are disabled
again directly afterwards:

> 	if (rockchip_pmu_domain_is_on(pd) != power_on) {

They are enabled here:

> 		ret = clk_bulk_enable(pd->num_clks, pd->clks);
> 		if (ret < 0) {
> 			dev_err(pmu->dev, "failed to enable clocks\n");
> 			mutex_unlock(&pmu->mutex);
> 			return ret;
> 		}
> 
> 		if (!power_on) {
> 			rockchip_pmu_save_qos(pd);
> 
> 			/* if powering down, idle request to NIU first */
> 			rockchip_pmu_set_idle_request(pd, true);
> 		}
>

Then the power domain is switched:

> 		rockchip_do_pmu_set_power_domain(pd, power_on);
> 
> 		if (power_on) {
> 			/* if powering up, leave idle mode */
> 			rockchip_pmu_set_idle_request(pd, false);
> 
> 			rockchip_pmu_restore_qos(pd);
> 		}
> 

And here the clocks are disabled again:

> 		clk_bulk_disable(pd->num_clks, pd->clks);
> 	}

> 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 the power domain driver additionally does is: It does a of_clk_get()
on all the clocks found in the node of a power domains consumer. It then
does a pm_clk_add_clk() on the clocks and sets the GENPD_FLAG_PM_CLK
flag. This has the effect that all clocks of a device in a power domain
are enabled as long as the power domain itself is enabled. This means
when I just add HCLK_VO to the DSI node, then the power domain driver
will enable it, even when the clock is not touched in the DSI driver at
all. To me this looks really fishy because I think a device itself
should have control over its clocks. I don't know how many devices
really depend on the power domain driver controlling their clocks, but
everyone of them will stop working when the power domain driver is not
compiled in.

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

Yes, they depend on that power domain as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |