| Message ID | 20230717-topic-branch_aon_cleanup-v1-3-27784d27a4f4@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unregister critical branch clocks + some RPM | expand |
On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote: > Some clocks need to be always-on, but we don't really do anything > with them, other than calling enable() once and telling Linux they're > enabled. > > Unregister them to save a couple of bytes and, perhaps more > importantly, allow for runtime suspend of the clock controller device, > as CLK_IS_CRITICAL prevents the latter. But this doesn't sound right. How can you disable a controller which still has clocks enabled? Shouldn't instead these clocks be modelled properly so that they are only enabled when actually needed? Johan
On 18.07.2023 15:20, Johan Hovold wrote: > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote: >> Some clocks need to be always-on, but we don't really do anything >> with them, other than calling enable() once and telling Linux they're >> enabled. >> >> Unregister them to save a couple of bytes and, perhaps more >> importantly, allow for runtime suspend of the clock controller device, >> as CLK_IS_CRITICAL prevents the latter. > > But this doesn't sound right. How can you disable a controller which > still has clocks enabled? > > Shouldn't instead these clocks be modelled properly so that they are > only enabled when actually needed? Hm.. We do have clk_branch2_aon_ops, but something still needs to toggle these clocks. I *think* we could leave a permanent vote in probe() without breaking runtime pm! I'll give it a spin bit later.. Konrad
On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote: > On 18.07.2023 15:20, Johan Hovold wrote: > > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote: > >> Some clocks need to be always-on, but we don't really do anything > >> with them, other than calling enable() once and telling Linux they're > >> enabled. > >> > >> Unregister them to save a couple of bytes and, perhaps more > >> importantly, allow for runtime suspend of the clock controller device, > >> as CLK_IS_CRITICAL prevents the latter. > > > > But this doesn't sound right. How can you disable a controller which > > still has clocks enabled? > > > > Shouldn't instead these clocks be modelled properly so that they are > > only enabled when actually needed? > Hm.. We do have clk_branch2_aon_ops, but something still needs to > toggle these clocks. > Before we started replacing these clocks with static votes, I handled exactly this problem in the turingcc-qcs404 driver by registering the ahb clock with a pm_clk_add(). The clock framework will then automagically keep the clock enabled around operations, but it will also keep the runtime state active as long as the clock is prepared. As mentioned in an earlier reply today, there's no similarity to this in the reset or gdsc code, so we'd need to add that in order to rely on such mechanism. > I *think* we could leave a permanent vote in probe() without breaking > runtime pm! I'll give it a spin bit later.. > Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would properly connect the two, and thereby handle probe order between the two clock controllers. But it would prevent the power-domain of the parent provider to ever suspending. Using pm_clk_add() this would at least depend on client votes. Regards, Bjorn
On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote: > On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote: > > On 18.07.2023 15:20, Johan Hovold wrote: > > > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote: > > >> Some clocks need to be always-on, but we don't really do anything > > >> with them, other than calling enable() once and telling Linux they're > > >> enabled. > > >> > > >> Unregister them to save a couple of bytes and, perhaps more > > >> importantly, allow for runtime suspend of the clock controller device, > > >> as CLK_IS_CRITICAL prevents the latter. > > > > > > But this doesn't sound right. How can you disable a controller which > > > still has clocks enabled? > > > > > > Shouldn't instead these clocks be modelled properly so that they are > > > only enabled when actually needed? > > Hm.. We do have clk_branch2_aon_ops, but something still needs to > > toggle these clocks. > > > > Before we started replacing these clocks with static votes, I handled > exactly this problem in the turingcc-qcs404 driver by registering the > ahb clock with a pm_clk_add(). The clock framework will then > automagically keep the clock enabled around operations, but it will also > keep the runtime state active as long as the clock is prepared. > > As mentioned in an earlier reply today, there's no similarity to this in > the reset or gdsc code, so we'd need to add that in order to rely on > such mechanism. This reminds me of: 4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls") which recently removed a broken attempt to implement this for gdscs. Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at least for genpd (but see below). > > I *think* we could leave a permanent vote in probe() without breaking > > runtime pm! I'll give it a spin bit later.. > > > > Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would > properly connect the two, and thereby handle probe order between the two > clock controllers. Yeah, this dependency really should be described eventually. > But it would prevent the power-domain of the parent provider to ever > suspending. Using pm_clk_add() this would at least depend on client > votes. IIUC using pm_clk_add() would also prevent the parent from suspending due to that resume call in clk_prepare(). And this mechanism is also used for GENPD_FLAG_PM_CLK... Johan
On 19.07.2023 10:43, Johan Hovold wrote: > On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote: >> On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote: >>> On 18.07.2023 15:20, Johan Hovold wrote: >>>> On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote: >>>>> Some clocks need to be always-on, but we don't really do anything >>>>> with them, other than calling enable() once and telling Linux they're >>>>> enabled. >>>>> >>>>> Unregister them to save a couple of bytes and, perhaps more >>>>> importantly, allow for runtime suspend of the clock controller device, >>>>> as CLK_IS_CRITICAL prevents the latter. >>>> >>>> But this doesn't sound right. How can you disable a controller which >>>> still has clocks enabled? >>>> >>>> Shouldn't instead these clocks be modelled properly so that they are >>>> only enabled when actually needed? >>> Hm.. We do have clk_branch2_aon_ops, but something still needs to >>> toggle these clocks. >>> >> >> Before we started replacing these clocks with static votes, I handled >> exactly this problem in the turingcc-qcs404 driver by registering the >> ahb clock with a pm_clk_add(). The clock framework will then >> automagically keep the clock enabled around operations, but it will also >> keep the runtime state active as long as the clock is prepared. >> >> As mentioned in an earlier reply today, there's no similarity to this in >> the reset or gdsc code, so we'd need to add that in order to rely on >> such mechanism. > > This reminds me of: > > 4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls") > > which recently removed a broken attempt to implement this for gdscs. > > Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at > least for genpd (but see below). > >>> I *think* we could leave a permanent vote in probe() without breaking >>> runtime pm! I'll give it a spin bit later.. >>> >> >> Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would >> properly connect the two, and thereby handle probe order between the two >> clock controllers. > > Yeah, this dependency really should be described eventually. > >> But it would prevent the power-domain of the parent provider to ever >> suspending. Using pm_clk_add() this would at least depend on client >> votes. > > IIUC using pm_clk_add() would also prevent the parent from suspending > due to that resume call in clk_prepare(). > > And this mechanism is also used for GENPD_FLAG_PM_CLK... So.. how do we go about solving the issue that this patch tried to address? Konrad
diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c index e94e88bdfb91..14dafea45ac9 100644 --- a/drivers/clk/qcom/gcc-sm6375.c +++ b/drivers/clk/qcom/gcc-sm6375.c @@ -1742,22 +1742,6 @@ static struct clk_branch gcc_cam_throttle_rt_clk = { }, }; -static struct clk_branch gcc_camera_ahb_clk = { - .halt_reg = 0x17008, - .halt_check = BRANCH_HALT_DELAY, - .hwcg_reg = 0x17008, - .hwcg_bit = 1, - .clkr = { - .enable_reg = 0x17008, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gcc_camera_ahb_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_camss_axi_clk = { .halt_reg = 0x58044, .halt_check = BRANCH_HALT, @@ -2308,22 +2292,6 @@ static struct clk_branch gcc_cfg_noc_usb3_prim_axi_clk = { }, }; -static struct clk_branch gcc_disp_ahb_clk = { - .halt_reg = 0x1700c, - .halt_check = BRANCH_HALT_VOTED, - .hwcg_reg = 0x1700c, - .hwcg_bit = 1, - .clkr = { - .enable_reg = 0x1700c, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gcc_disp_ahb_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_regmap_div gcc_disp_gpll0_clk_src = { .reg = 0x17058, .shift = 0, @@ -2454,22 +2422,6 @@ static struct clk_branch gcc_gp3_clk = { }, }; -static struct clk_branch gcc_gpu_cfg_ahb_clk = { - .halt_reg = 0x36004, - .halt_check = BRANCH_HALT_VOTED, - .hwcg_reg = 0x36004, - .hwcg_bit = 1, - .clkr = { - .enable_reg = 0x36004, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gcc_gpu_cfg_ahb_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_gpu_gpll0_clk_src = { .halt_check = BRANCH_HALT_DELAY, .clkr = { @@ -3093,26 +3045,6 @@ static struct clk_branch gcc_sdcc2_apps_clk = { }, }; -static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = { - .halt_reg = 0x2b06c, - .halt_check = BRANCH_HALT_VOTED, - .hwcg_reg = 0x2b06c, - .hwcg_bit = 1, - .clkr = { - .enable_reg = 0x79004, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gcc_sys_noc_cpuss_ahb_clk", - .parent_hws = (const struct clk_hw*[]) { - &gcc_cpuss_ahb_postdiv_clk_src.clkr.hw, - }, - .num_parents = 1, - .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_sys_noc_ufs_phy_axi_clk = { .halt_reg = 0x45098, .halt_check = BRANCH_HALT, @@ -3432,22 +3364,6 @@ static struct clk_branch gcc_venus_ctl_axi_clk = { }, }; -static struct clk_branch gcc_video_ahb_clk = { - .halt_reg = 0x17004, - .halt_check = BRANCH_HALT_DELAY, - .hwcg_reg = 0x17004, - .hwcg_bit = 1, - .clkr = { - .enable_reg = 0x17004, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gcc_video_ahb_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gcc_video_axi0_clk = { .halt_reg = 0x1701c, .halt_check = BRANCH_HALT_VOTED, @@ -3614,7 +3530,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = { [GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr, [GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr, [GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr, - [GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr, [GCC_CAMSS_AXI_CLK] = &gcc_camss_axi_clk.clkr, [GCC_CAMSS_AXI_CLK_SRC] = &gcc_camss_axi_clk_src.clkr, [GCC_CAMSS_CCI_0_CLK] = &gcc_camss_cci_0_clk.clkr, @@ -3670,7 +3585,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = { [GCC_CFG_NOC_USB3_PRIM_AXI_CLK] = &gcc_cfg_noc_usb3_prim_axi_clk.clkr, [GCC_CPUSS_AHB_CLK_SRC] = &gcc_cpuss_ahb_clk_src.clkr, [GCC_CPUSS_AHB_POSTDIV_CLK_SRC] = &gcc_cpuss_ahb_postdiv_clk_src.clkr, - [GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr, [GCC_DISP_GPLL0_CLK_SRC] = &gcc_disp_gpll0_clk_src.clkr, [GCC_DISP_GPLL0_DIV_CLK_SRC] = &gcc_disp_gpll0_div_clk_src.clkr, [GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr, @@ -3682,7 +3596,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = { [GCC_GP2_CLK_SRC] = &gcc_gp2_clk_src.clkr, [GCC_GP3_CLK] = &gcc_gp3_clk.clkr, [GCC_GP3_CLK_SRC] = &gcc_gp3_clk_src.clkr, - [GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr, [GCC_GPU_GPLL0_CLK_SRC] = &gcc_gpu_gpll0_clk_src.clkr, [GCC_GPU_GPLL0_DIV_CLK_SRC] = &gcc_gpu_gpll0_div_clk_src.clkr, [GCC_GPU_MEMNOC_GFX_CLK] = &gcc_gpu_memnoc_gfx_clk.clkr, @@ -3738,7 +3651,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = { [GCC_SDCC2_AHB_CLK] = &gcc_sdcc2_ahb_clk.clkr, [GCC_SDCC2_APPS_CLK] = &gcc_sdcc2_apps_clk.clkr, [GCC_SDCC2_APPS_CLK_SRC] = &gcc_sdcc2_apps_clk_src.clkr, - [GCC_SYS_NOC_CPUSS_AHB_CLK] = &gcc_sys_noc_cpuss_ahb_clk.clkr, [GCC_SYS_NOC_UFS_PHY_AXI_CLK] = &gcc_sys_noc_ufs_phy_axi_clk.clkr, [GCC_SYS_NOC_USB3_PRIM_AXI_CLK] = &gcc_sys_noc_usb3_prim_axi_clk.clkr, [GCC_UFS_PHY_AHB_CLK] = &gcc_ufs_phy_ahb_clk.clkr, @@ -3765,7 +3677,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = { [GCC_VCODEC0_AXI_CLK] = &gcc_vcodec0_axi_clk.clkr, [GCC_VENUS_AHB_CLK] = &gcc_venus_ahb_clk.clkr, [GCC_VENUS_CTL_AXI_CLK] = &gcc_venus_ctl_axi_clk.clkr, - [GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr, [GCC_VIDEO_AXI0_CLK] = &gcc_video_axi0_clk.clkr, [GCC_VIDEO_THROTTLE_CORE_CLK] = &gcc_video_throttle_core_clk.clkr, [GCC_VIDEO_VCODEC0_SYS_CLK] = &gcc_video_vcodec0_sys_clk.clkr, @@ -3883,11 +3794,23 @@ static int gcc_sm6375_probe(struct platform_device *pdev) /* * Keep the following clocks always on: - * GCC_CAMERA_XO_CLK, GCC_CPUSS_GNOC_CLK, GCC_DISP_XO_CLK + * GCC_CAMERA_XO_CLK + * GCC_CPUSS_GNOC_CLK + * GCC_DISP_XO_CLK + * GCC_CAMERA_AHB_CLK + * GCC_DISP_AHB_CLK + * GCC_GPU_CFG_AHB_CLK + * GCC_SYS_NOC_CPUSS_AHB_CLK + * GCC_VIDEO_AHB_CLK */ qcom_branch_set_clk_en(regmap, 0x17028); qcom_branch_set_clk_en(regmap, 0x2b004); qcom_branch_set_clk_en(regmap, 0x1702c); + qcom_branch_set_clk_en(regmap, 0x17008); + qcom_branch_set_clk_en(regmap, 0x1700c); + qcom_branch_set_clk_en(regmap, 0x36004); + qcom_branch_set_clk_en(regmap, 0x2b06c); + qcom_branch_set_clk_en(regmap, 0x17004); clk_lucid_pll_configure(&gpll10, regmap, &gpll10_config); clk_lucid_pll_configure(&gpll11, regmap, &gpll11_config);
Some clocks need to be always-on, but we don't really do anything with them, other than calling enable() once and telling Linux they're enabled. Unregister them to save a couple of bytes and, perhaps more importantly, allow for runtime suspend of the clock controller device, as CLK_IS_CRITICAL prevents the latter. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/clk/qcom/gcc-sm6375.c | 103 ++++++------------------------------------ 1 file changed, 13 insertions(+), 90 deletions(-)