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@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?
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).
Thanks,
Robin.
