| Message ID | 20230717-topic-branch_aon_cleanup-v2-6-2a583460ef26@linaro.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | Unregister critical branch clocks + some RPM | expand |
On 29/11/2023 18:59, 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. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/gpucc-sm6115.c | 33 +++------------------------------ > 1 file changed, 3 insertions(+), 30 deletions(-) > > diff --git a/drivers/clk/qcom/gpucc-sm6115.c b/drivers/clk/qcom/gpucc-sm6115.c > index fb71c21c9a89..93a50431aef8 100644 > --- a/drivers/clk/qcom/gpucc-sm6115.c > +++ b/drivers/clk/qcom/gpucc-sm6115.c > @@ -234,20 +234,6 @@ static struct clk_rcg2 gpu_cc_gx_gfx3d_clk_src = { > }, > }; > > -static struct clk_branch gpu_cc_ahb_clk = { > - .halt_reg = 0x1078, > - .halt_check = BRANCH_HALT_DELAY, > - .clkr = { > - .enable_reg = 0x1078, > - .enable_mask = BIT(0), > - .hw.init = &(struct clk_init_data){ > - .name = "gpu_cc_ahb_clk", > - .flags = CLK_IS_CRITICAL, > - .ops = &clk_branch2_ops, > - }, > - }, > -}; > - > static struct clk_branch gpu_cc_crc_ahb_clk = { > .halt_reg = 0x107c, > .halt_check = BRANCH_HALT_DELAY, > @@ -336,20 +322,6 @@ static struct clk_branch gpu_cc_cxo_clk = { > }, > }; > > -static struct clk_branch gpu_cc_gx_cxo_clk = { > - .halt_reg = 0x1060, > - .halt_check = BRANCH_HALT_DELAY, > - .clkr = { > - .enable_reg = 0x1060, > - .enable_mask = BIT(0), > - .hw.init = &(struct clk_init_data){ > - .name = "gpu_cc_gx_cxo_clk", > - .flags = CLK_IS_CRITICAL, > - .ops = &clk_branch2_ops, > - }, > - }, > -}; > - > static struct clk_branch gpu_cc_gx_gfx3d_clk = { > .halt_reg = 0x1054, > .halt_check = BRANCH_HALT_SKIP, > @@ -418,7 +390,6 @@ static struct gdsc gpu_gx_gdsc = { > }; > > static struct clk_regmap *gpu_cc_sm6115_clocks[] = { > - [GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr, > [GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr, > [GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr, > [GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr, > @@ -426,7 +397,6 @@ static struct clk_regmap *gpu_cc_sm6115_clocks[] = { > [GPU_CC_CXO_AON_CLK] = &gpu_cc_cxo_aon_clk.clkr, > [GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr, > [GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr, > - [GPU_CC_GX_CXO_CLK] = &gpu_cc_gx_cxo_clk.clkr, OTOH. Seems a pity to remove these clocks - generally for the series I mean - from the debug view in /sys/kernel/debug/clk_summary. In the ideal case we have pm runtime functional without dropping these clocks from the view in /sys/kernel/debug/clk_summary. Certainly I've found that interface useful when launching a real product. It might be confusing to _not_ see the always-on clocks enumerated there. --- bod
On 29.11.2023 22:14, Bryan O'Donoghue wrote: > On 29/11/2023 18:59, 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. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- [...] > > OTOH. > > Seems a pity to remove these clocks - generally for the series I mean - from the debug view in /sys/kernel/debug/clk_summary. > > In the ideal case we have pm runtime functional without dropping these clocks from the view in /sys/kernel/debug/clk_summary. > > Certainly I've found that interface useful when launching a real product. It might be confusing to _not_ see the always-on clocks enumerated there. > > --- I have shared the very same concern in the past.. I did also however realize that debugfs is not accurate, especially with funky clocks that need only be enabled once (*even if you gate the e.g. MM subsystem*) and debugcc should be used instead Konrad
diff --git a/drivers/clk/qcom/gpucc-sm6115.c b/drivers/clk/qcom/gpucc-sm6115.c index fb71c21c9a89..93a50431aef8 100644 --- a/drivers/clk/qcom/gpucc-sm6115.c +++ b/drivers/clk/qcom/gpucc-sm6115.c @@ -234,20 +234,6 @@ static struct clk_rcg2 gpu_cc_gx_gfx3d_clk_src = { }, }; -static struct clk_branch gpu_cc_ahb_clk = { - .halt_reg = 0x1078, - .halt_check = BRANCH_HALT_DELAY, - .clkr = { - .enable_reg = 0x1078, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gpu_cc_ahb_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gpu_cc_crc_ahb_clk = { .halt_reg = 0x107c, .halt_check = BRANCH_HALT_DELAY, @@ -336,20 +322,6 @@ static struct clk_branch gpu_cc_cxo_clk = { }, }; -static struct clk_branch gpu_cc_gx_cxo_clk = { - .halt_reg = 0x1060, - .halt_check = BRANCH_HALT_DELAY, - .clkr = { - .enable_reg = 0x1060, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "gpu_cc_gx_cxo_clk", - .flags = CLK_IS_CRITICAL, - .ops = &clk_branch2_ops, - }, - }, -}; - static struct clk_branch gpu_cc_gx_gfx3d_clk = { .halt_reg = 0x1054, .halt_check = BRANCH_HALT_SKIP, @@ -418,7 +390,6 @@ static struct gdsc gpu_gx_gdsc = { }; static struct clk_regmap *gpu_cc_sm6115_clocks[] = { - [GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr, [GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr, [GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr, [GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr, @@ -426,7 +397,6 @@ static struct clk_regmap *gpu_cc_sm6115_clocks[] = { [GPU_CC_CXO_AON_CLK] = &gpu_cc_cxo_aon_clk.clkr, [GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr, [GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr, - [GPU_CC_GX_CXO_CLK] = &gpu_cc_gx_cxo_clk.clkr, [GPU_CC_GX_GFX3D_CLK] = &gpu_cc_gx_gfx3d_clk.clkr, [GPU_CC_GX_GFX3D_CLK_SRC] = &gpu_cc_gx_gfx3d_clk_src.clkr, [GPU_CC_PLL0] = &gpu_cc_pll0.clkr, @@ -488,6 +458,9 @@ static int gpu_cc_sm6115_probe(struct platform_device *pdev) qcom_branch_set_force_mem_core(regmap, gpu_cc_gx_gfx3d_clk, true); qcom_branch_set_force_periph_on(regmap, gpu_cc_gx_gfx3d_clk, true); + qcom_branch_set_clk_en(regmap, 0x1078); /* GPU_CC_AHB_CLK */ + qcom_branch_set_clk_en(regmap, 0x1060); /* GPU_CC_GX_CXO_CLK */ + return qcom_cc_really_probe(pdev, &gpu_cc_sm6115_desc, regmap); }
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/gpucc-sm6115.c | 33 +++------------------------------ 1 file changed, 3 insertions(+), 30 deletions(-)