| Message ID | 1465724913-14553-1-git-send-email-zhengxing@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Xing, On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng <zhengxing@rock-chips.com> wrote: > The pclk_vio_grf supply power for GRF IOs, if it is disabled, will > cause abnormal operation of the GRF. > > The clock tree of the pclk_vio like this: > | --> pclk_vio_grf > ... pclk_vio | --> pclk_mipi_dsi1 > | --> pclk_mipi_dsi0 > > and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag > CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused > when startup: > clk_disable_unused > --> clk_disable_unprepare > --> clk_disable > --> clk_core_disable(core->parent) > > then, the pclk_vio_grf also is disabled. Therefore, we need to add > pclk_vio_grf to critical clock and avoid to disable pclk_vio and > pclk_vio_grf. > > Tested-by: Yakir Yang <ykk@rock-chips.com> > Signed-off-by: Yakir Yang <ykk@rock-chips.com> > Signed-off-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Xing Zheng <zhengxing@rock-chips.com> > --- > > drivers/clk/rockchip/clk-rk3399.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c > index b6742fa..7ecb12c3 100644 > --- a/drivers/clk/rockchip/clk-rk3399.c > +++ b/drivers/clk/rockchip/clk-rk3399.c > @@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = { > "gpll_hclk_perilp1_src", > "gpll_aclk_perilp0_src", > "gpll_aclk_perihp_src", > + "pclk_vio_grf", This clock is only needed when doing video output (like eDP), right? That means it is not really a critical clock. Critical clocks are supposed to be ones that are needed for the basic functioning of the system and that can never be turned off in any circumstances. In this case, if someone were running a rk3399 device and didn't have any video output they would want this clock off. Can you figure out in exactly which circumstances this clock needs to be on and then add a proper consumer of this clock? For instance, if this clock is needed whenever the VOP is outputting data, then the VOP should be a client and should turn this clock on and off when video is being output. If this clock is needed whenever you access VOP registers, then the VOP should be a client and turn this clock on around register accesses. -Doug
Hi Doug, On 2016年06月13日 05:32, Doug Anderson wrote: > Xing, > > On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng<zhengxing@rock-chips.com> wrote: >> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will >> cause abnormal operation of the GRF. >> >> The clock tree of the pclk_vio like this: >> | --> pclk_vio_grf >> ... pclk_vio | --> pclk_mipi_dsi1 >> | --> pclk_mipi_dsi0 >> >> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag >> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused >> when startup: >> clk_disable_unused >> --> clk_disable_unprepare >> --> clk_disable >> --> clk_core_disable(core->parent) >> >> then, the pclk_vio_grf also is disabled. Therefore, we need to add >> pclk_vio_grf to critical clock and avoid to disable pclk_vio and >> pclk_vio_grf. >> >> Tested-by: Yakir Yang<ykk@rock-chips.com> >> Signed-off-by: Yakir Yang<ykk@rock-chips.com> >> Signed-off-by: Brian Norris<briannorris@chromium.org> >> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com> >> --- >> >> drivers/clk/rockchip/clk-rk3399.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c >> index b6742fa..7ecb12c3 100644 >> --- a/drivers/clk/rockchip/clk-rk3399.c >> +++ b/drivers/clk/rockchip/clk-rk3399.c >> @@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = { >> "gpll_hclk_perilp1_src", >> "gpll_aclk_perilp0_src", >> "gpll_aclk_perihp_src", >> + "pclk_vio_grf", > This clock is only needed when doing video output (like eDP), right? > That means it is not really a critical clock. Critical clocks are > supposed to be ones that are needed for the basic functioning of the > system and that can never be turned off in any circumstances. In this > case, if someone were running a rk3399 device and didn't have any > video output they would want this clock off. > > Can you figure out in exactly which circumstances this clock needs to > be on and then add a proper consumer of this clock? For instance, if > this clock is needed whenever the VOP is outputting data, then the VOP > should be a client and should turn this clock on and off when video is > being output. If this clock is needed whenever you access VOP > registers, then the VOP should be a client and turn this clock on > around register accesses. > > -Doug > Yes, the pclk_vio_grf is needed for doing video output. andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp From our design folks, we have many GRF registers in different power domains, and these GRF gates should be always enabled. In this case, we can avoid some of the operations GRF registers exception problems, and it is only a very small increase in power consumption (aboult <=1ma). I will refer the latest TRM to update a new patch for always enable these GRFs. Please drop this patch. Thanks.
Hi Doug, On 2016年06月13日 11:10, Xing Zheng wrote: > Hi Doug, > > On 2016年06月13日 05:32, Doug Anderson wrote: >> Xing, >> >> On Sun, Jun 12, 2016 at 2:48 AM, Xing >> Zheng<zhengxing@rock-chips.com> wrote: >>> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will >>> cause abnormal operation of the GRF. >>> >>> The clock tree of the pclk_vio like this: >>> | --> pclk_vio_grf >>> ... pclk_vio | --> pclk_mipi_dsi1 >>> | --> pclk_mipi_dsi0 >>> >>> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag >>> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused >>> when startup: >>> clk_disable_unused >>> --> clk_disable_unprepare >>> --> clk_disable >>> --> clk_core_disable(core->parent) >>> >>> then, the pclk_vio_grf also is disabled. Therefore, we need to add >>> pclk_vio_grf to critical clock and avoid to disable pclk_vio and >>> pclk_vio_grf. >>> >>> Tested-by: Yakir Yang<ykk@rock-chips.com> >>> Signed-off-by: Yakir Yang<ykk@rock-chips.com> >>> Signed-off-by: Brian Norris<briannorris@chromium.org> >>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com> >>> --- >>> >>> drivers/clk/rockchip/clk-rk3399.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/clk/rockchip/clk-rk3399.c >>> b/drivers/clk/rockchip/clk-rk3399.c >>> index b6742fa..7ecb12c3 100644 >>> --- a/drivers/clk/rockchip/clk-rk3399.c >>> +++ b/drivers/clk/rockchip/clk-rk3399.c >>> @@ -1485,6 +1485,7 @@ static const char *const >>> rk3399_cru_critical_clocks[] __initconst = { >>> "gpll_hclk_perilp1_src", >>> "gpll_aclk_perilp0_src", >>> "gpll_aclk_perihp_src", >>> + "pclk_vio_grf", >> This clock is only needed when doing video output (like eDP), right? >> That means it is not really a critical clock. Critical clocks are >> supposed to be ones that are needed for the basic functioning of the >> system and that can never be turned off in any circumstances. In this >> case, if someone were running a rk3399 device and didn't have any >> video output they would want this clock off. >> >> Can you figure out in exactly which circumstances this clock needs to >> be on and then add a proper consumer of this clock? For instance, if >> this clock is needed whenever the VOP is outputting data, then the VOP >> should be a client and should turn this clock on and off when video is >> being output. If this clock is needed whenever you access VOP >> registers, then the VOP should be a client and turn this clock on >> around register accesses. >> >> Additional, we are discussing that we should turn the "pclk_vio" on and off in video drivers when the video consumer needs to this clock. Thanks. >> > Yes, the pclk_vio_grf is needed for doing video output. > andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp > > From our design folks, we have many GRF registers in different power > domains, > and these GRF gates should be always enabled. In this case, we can > avoid some > of the operations GRF registers exception problems, and it is only a > very small > increase in power consumption (aboult <=1ma). > > I will refer the latest TRM to update a new patch for always enable > these GRFs. > > Please drop this patch.
Xing, On Sun, Jun 12, 2016 at 8:10 PM, Xing Zheng <zhengxing@rock-chips.com> wrote: > Hi Doug, > > > On 2016年06月13日 05:32, Doug Anderson wrote: >> >> Xing, >> >> On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng<zhengxing@rock-chips.com> >> wrote: >>> >>> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will >>> cause abnormal operation of the GRF. >>> >>> The clock tree of the pclk_vio like this: >>> | --> pclk_vio_grf >>> ... pclk_vio | --> pclk_mipi_dsi1 >>> | --> pclk_mipi_dsi0 >>> >>> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag >>> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused >>> when startup: >>> clk_disable_unused >>> --> clk_disable_unprepare >>> --> clk_disable >>> --> clk_core_disable(core->parent) >>> >>> then, the pclk_vio_grf also is disabled. Therefore, we need to add >>> pclk_vio_grf to critical clock and avoid to disable pclk_vio and >>> pclk_vio_grf. >>> >>> Tested-by: Yakir Yang<ykk@rock-chips.com> >>> Signed-off-by: Yakir Yang<ykk@rock-chips.com> >>> Signed-off-by: Brian Norris<briannorris@chromium.org> >>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com> >>> --- >>> >>> drivers/clk/rockchip/clk-rk3399.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/clk/rockchip/clk-rk3399.c >>> b/drivers/clk/rockchip/clk-rk3399.c >>> index b6742fa..7ecb12c3 100644 >>> --- a/drivers/clk/rockchip/clk-rk3399.c >>> +++ b/drivers/clk/rockchip/clk-rk3399.c >>> @@ -1485,6 +1485,7 @@ static const char *const >>> rk3399_cru_critical_clocks[] __initconst = { >>> "gpll_hclk_perilp1_src", >>> "gpll_aclk_perilp0_src", >>> "gpll_aclk_perihp_src", >>> + "pclk_vio_grf", >> >> This clock is only needed when doing video output (like eDP), right? >> That means it is not really a critical clock. Critical clocks are >> supposed to be ones that are needed for the basic functioning of the >> system and that can never be turned off in any circumstances. In this >> case, if someone were running a rk3399 device and didn't have any >> video output they would want this clock off. >> >> Can you figure out in exactly which circumstances this clock needs to >> be on and then add a proper consumer of this clock? For instance, if >> this clock is needed whenever the VOP is outputting data, then the VOP >> should be a client and should turn this clock on and off when video is >> being output. If this clock is needed whenever you access VOP >> registers, then the VOP should be a client and turn this clock on >> around register accesses. >> >> -Doug >> > Yes, the pclk_vio_grf is needed for doing video output. > andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp > > From our design folks, we have many GRF registers in different power > domains, > and these GRF gates should be always enabled. In this case, we can avoid > some > of the operations GRF registers exception problems, and it is only a very > small > increase in power consumption (aboult <=1ma). Even if it's not much power, it seems like we should still turn it off and on in the right place. Unless I'm mistaken it should be such a simple patch provide the clock to the right driver and then get the clock when appropriate. > I will refer the latest TRM to update a new patch for always enable these > GRFs. Does that mean you're going to make these all critical clocks? That doesn't sound so great... -Doug
Hi Doug, On 2016年06月14日 07:46, Doug Anderson wrote: > > Even if it's not much power, it seems like we should still turn it off > and on in the right place. Unless I'm mistaken it should be such a > simple patch provide the clock to the right driver and then get the > clock when appropriate. Yes, I talked with Yakir and we intent to enable/disable the pclk_vio_grf in video drivers, so this patch will be dropped. >> I will refer the latest TRM to update a new patch for always enable these >> GRFs. > Does that mean you're going to make these all critical clocks? That > doesn't sound so great... > > -Doug Maybe, I heard that they are removed in the updated TRM, but I have not got the TRM yet. I will double check it, and it seems that you do not agree to remove these clock... Thanks.
Hi, On Mon, Jun 13, 2016 at 8:02 PM, Xing Zheng <zhengxing@rock-chips.com> wrote: > Hi Doug, > > On 2016年06月14日 07:46, Doug Anderson wrote: >> >> >> Even if it's not much power, it seems like we should still turn it off >> and on in the right place. Unless I'm mistaken it should be such a >> simple patch provide the clock to the right driver and then get the >> clock when appropriate. > > Yes, I talked with Yakir and we intent to enable/disable the pclk_vio_grf in > video drivers, > so this patch will be dropped. >>> >>> I will refer the latest TRM to update a new patch for always enable these >>> GRFs. >> >> Does that mean you're going to make these all critical clocks? That >> doesn't sound so great... >> >> -Doug > > Maybe, I heard that they are removed in the updated TRM, but I have not got > the TRM yet. > I will double check it, and it seems that you do not agree to remove these > clock... Well, if it were to be removed from the TRM then that would be a strong sign that the SoC designers think that this clock should never ever be turned off. If that were the case I don't think I could object to leaving this clock on all the time. Presumably then we'd totally remove the clock from the clock tree and rely on firmware to leave it on? Technically removing this clock is not really device-tree backward compatible, but I guess if there are no current users... ...note: if the clock IS listed in the TRM and there's ever a chance that we'd want to turn it off, it's much easier to set that up all now. Trying to later go in and decide that these clocks are no longer "always on" gets into all sorts of weird device tree backward compatibility corner cases. -Doug
Am Montag, 13. Juni 2016, 20:49:39 schrieb Doug Anderson: > Hi, > > On Mon, Jun 13, 2016 at 8:02 PM, Xing Zheng <zhengxing@rock-chips.com> wrote: > > Hi Doug, > > > > On 2016年06月14日 07:46, Doug Anderson wrote: > >> Even if it's not much power, it seems like we should still turn it off > >> and on in the right place. Unless I'm mistaken it should be such a > >> simple patch provide the clock to the right driver and then get the > >> clock when appropriate. > > > > Yes, I talked with Yakir and we intent to enable/disable the pclk_vio_grf > > in video drivers, > > so this patch will be dropped. > > > >>> I will refer the latest TRM to update a new patch for always enable > >>> these > >>> GRFs. > >> > >> Does that mean you're going to make these all critical clocks? That > >> doesn't sound so great... > >> > >> -Doug > > > > Maybe, I heard that they are removed in the updated TRM, but I have not > > got > > the TRM yet. > > I will double check it, and it seems that you do not agree to remove these > > clock... > > Well, if it were to be removed from the TRM then that would be a > strong sign that the SoC designers think that this clock should never > ever be turned off. If that were the case I don't think I could > object to leaving this clock on all the time. Presumably then we'd > totally remove the clock from the clock tree and rely on firmware to > leave it on? Technically removing this clock is not really > device-tree backward compatible, but I guess if there are no current > users... > > ...note: if the clock IS listed in the TRM and there's ever a chance > that we'd want to turn it off, it's much easier to set that up all > now. Trying to later go in and decide that these clocks are no longer > "always on" gets into all sorts of weird device tree backward > compatibility corner cases. PCLK_VIO_GRF gets already exported as clock-id, so we already have the wired corner-case :-) . But we'll see how this plays out.
diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c index b6742fa..7ecb12c3 100644 --- a/drivers/clk/rockchip/clk-rk3399.c +++ b/drivers/clk/rockchip/clk-rk3399.c @@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = { "gpll_hclk_perilp1_src", "gpll_aclk_perilp0_src", "gpll_aclk_perihp_src", + "pclk_vio_grf", }; static const char *const rk3399_pmucru_critical_clocks[] __initconst = {
The pclk_vio_grf supply power for GRF IOs, if it is disabled, will cause abnormal operation of the GRF. The clock tree of the pclk_vio like this: | --> pclk_vio_grf ... pclk_vio | --> pclk_mipi_dsi1 | --> pclk_mipi_dsi0 and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused when startup: clk_disable_unused --> clk_disable_unprepare --> clk_disable --> clk_core_disable(core->parent) then, the pclk_vio_grf also is disabled. Therefore, we need to add pclk_vio_grf to critical clock and avoid to disable pclk_vio and pclk_vio_grf. Tested-by: Yakir Yang <ykk@rock-chips.com> Signed-off-by: Yakir Yang <ykk@rock-chips.com> Signed-off-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com> --- drivers/clk/rockchip/clk-rk3399.c | 1 + 1 file changed, 1 insertion(+)