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@xxxxxxxxxxxxxx> > > > > > > --- > > > > > > > > > > > > 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? 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 |