| Message ID | 20220328151116.2034635-2-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
On 3/28/22 18:10, Sascha Hauer wrote: > Whenever pclk_vo is enabled hclk_vo must be enabled as well. This is > described in the Reference Manual as: > > | 2.8.6 NIU Clock gating reliance > | > | A part of niu clocks have a dependence on another niu clock in order to > | sharing the internal bus. When these clocks are in use, another niu > | clock must be opened, and cannot be gated. These clocks and the special > | clock on which they are relied are as following: > | > | Clocks which have dependency The clock which can not be gated > | ----------------------------------------------------------------- > | ... > | pclk_vo_niu, hclk_vo_s_niu hclk_vo_niu > | ... > > The clock framework doesn't offer a way to enable clock B whenever clock A is > enabled, at least not when B is not an ancestor of A. Workaround this by > marking hclk_vo as critical so it is never disabled. This is suboptimal in > terms of power consumption, but a stop gap solution until the clock framework > has a way to deal with this. > > We have this clock tree: > > | aclk_vo 2 2 0 300000000 0 0 50000 Y > | aclk_hdcp 0 0 0 300000000 0 0 50000 N > | pclk_vo 2 3 0 75000000 0 0 50000 Y > | pclk_edp_ctrl 0 0 0 75000000 0 0 50000 N > | pclk_dsitx_1 0 0 0 75000000 0 0 50000 N > | pclk_dsitx_0 1 2 0 75000000 0 0 50000 Y > | pclk_hdmi_host 1 2 0 75000000 0 0 50000 Y > | pclk_hdcp 0 0 0 75000000 0 0 50000 N > | hclk_vo 2 5 0 150000000 0 0 50000 Y > | hclk_hdcp 0 0 0 150000000 0 0 50000 N > | hclk_vop 0 2 0 150000000 0 0 50000 N > > Without this patch the edp, dsitx, hdmi and hdcp driver would enable their > clocks which then enables pclk_vo, but hclk_vo stays disabled and register > accesses just hang. hclk_vo is enabled by the VOP2 driver, so reproducibility > of this issue depends on the probe order. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Notes: > Changes since v8: > - new patch > > drivers/clk/rockchip/clk-rk3568.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/rockchip/clk-rk3568.c b/drivers/clk/rockchip/clk-rk3568.c > index 63dfbeeeb06d9..62694d95173ab 100644 > --- a/drivers/clk/rockchip/clk-rk3568.c > +++ b/drivers/clk/rockchip/clk-rk3568.c > @@ -1591,6 +1591,7 @@ static const char *const rk3568_cru_critical_clocks[] __initconst = { > "hclk_php", > "pclk_php", > "hclk_usb", > + "hclk_vo", > }; > > static const char *const rk3568_pmucru_critical_clocks[] __initconst = { Nice! That's much better than the DT hacks, IMO, thank you. Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
On 2022-03-28 16:10, Sascha Hauer wrote: > Whenever pclk_vo is enabled hclk_vo must be enabled as well. This is > described in the Reference Manual as: > > | 2.8.6 NIU Clock gating reliance > | > | A part of niu clocks have a dependence on another niu clock in order to > | sharing the internal bus. When these clocks are in use, another niu > | clock must be opened, and cannot be gated. These clocks and the special > | clock on which they are relied are as following: > | > | Clocks which have dependency The clock which can not be gated > | ----------------------------------------------------------------- > | ... > | pclk_vo_niu, hclk_vo_s_niu hclk_vo_niu > | ... > > The clock framework doesn't offer a way to enable clock B whenever clock A is > enabled, at least not when B is not an ancestor of A. Workaround this by > marking hclk_vo as critical so it is never disabled. This is suboptimal in > terms of power consumption, but a stop gap solution until the clock framework > has a way to deal with this. > > We have this clock tree: > > | aclk_vo 2 2 0 300000000 0 0 50000 Y > | aclk_hdcp 0 0 0 300000000 0 0 50000 N > | pclk_vo 2 3 0 75000000 0 0 50000 Y > | pclk_edp_ctrl 0 0 0 75000000 0 0 50000 N > | pclk_dsitx_1 0 0 0 75000000 0 0 50000 N > | pclk_dsitx_0 1 2 0 75000000 0 0 50000 Y > | pclk_hdmi_host 1 2 0 75000000 0 0 50000 Y > | pclk_hdcp 0 0 0 75000000 0 0 50000 N > | hclk_vo 2 5 0 150000000 0 0 50000 Y > | hclk_hdcp 0 0 0 150000000 0 0 50000 N > | hclk_vop 0 2 0 150000000 0 0 50000 N > > Without this patch the edp, dsitx, hdmi and hdcp driver would enable their > clocks which then enables pclk_vo, but hclk_vo stays disabled and register > accesses just hang. hclk_vo is enabled by the VOP2 driver, so reproducibility > of this issue depends on the probe order. FWIW, Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Notes: > Changes since v8: > - new patch > > drivers/clk/rockchip/clk-rk3568.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/rockchip/clk-rk3568.c b/drivers/clk/rockchip/clk-rk3568.c > index 63dfbeeeb06d9..62694d95173ab 100644 > --- a/drivers/clk/rockchip/clk-rk3568.c > +++ b/drivers/clk/rockchip/clk-rk3568.c > @@ -1591,6 +1591,7 @@ static const char *const rk3568_cru_critical_clocks[] __initconst = { > "hclk_php", > "pclk_php", > "hclk_usb", > + "hclk_vo", > }; > > static const char *const rk3568_pmucru_critical_clocks[] __initconst = {
diff --git a/drivers/clk/rockchip/clk-rk3568.c b/drivers/clk/rockchip/clk-rk3568.c index 63dfbeeeb06d9..62694d95173ab 100644 --- a/drivers/clk/rockchip/clk-rk3568.c +++ b/drivers/clk/rockchip/clk-rk3568.c @@ -1591,6 +1591,7 @@ static const char *const rk3568_cru_critical_clocks[] __initconst = { "hclk_php", "pclk_php", "hclk_usb", + "hclk_vo", }; static const char *const rk3568_pmucru_critical_clocks[] __initconst = {
Whenever pclk_vo is enabled hclk_vo must be enabled as well. This is described in the Reference Manual as: | 2.8.6 NIU Clock gating reliance | | A part of niu clocks have a dependence on another niu clock in order to | sharing the internal bus. When these clocks are in use, another niu | clock must be opened, and cannot be gated. These clocks and the special | clock on which they are relied are as following: | | Clocks which have dependency The clock which can not be gated | ----------------------------------------------------------------- | ... | pclk_vo_niu, hclk_vo_s_niu hclk_vo_niu | ... The clock framework doesn't offer a way to enable clock B whenever clock A is enabled, at least not when B is not an ancestor of A. Workaround this by marking hclk_vo as critical so it is never disabled. This is suboptimal in terms of power consumption, but a stop gap solution until the clock framework has a way to deal with this. We have this clock tree: | aclk_vo 2 2 0 300000000 0 0 50000 Y | aclk_hdcp 0 0 0 300000000 0 0 50000 N | pclk_vo 2 3 0 75000000 0 0 50000 Y | pclk_edp_ctrl 0 0 0 75000000 0 0 50000 N | pclk_dsitx_1 0 0 0 75000000 0 0 50000 N | pclk_dsitx_0 1 2 0 75000000 0 0 50000 Y | pclk_hdmi_host 1 2 0 75000000 0 0 50000 Y | pclk_hdcp 0 0 0 75000000 0 0 50000 N | hclk_vo 2 5 0 150000000 0 0 50000 Y | hclk_hdcp 0 0 0 150000000 0 0 50000 N | hclk_vop 0 2 0 150000000 0 0 50000 N Without this patch the edp, dsitx, hdmi and hdcp driver would enable their clocks which then enables pclk_vo, but hclk_vo stays disabled and register accesses just hang. hclk_vo is enabled by the VOP2 driver, so reproducibility of this issue depends on the probe order. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- Notes: Changes since v8: - new patch drivers/clk/rockchip/clk-rk3568.c | 1 + 1 file changed, 1 insertion(+)