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

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

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>;
        };

The HDMI controller is part of that domain, so I think this should work,
but it doesn't. That's where I am now, I'll have a closer look.

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 |