Messages in this thread | | | Date | Thu, 28 Mar 2024 15:12:44 +0530 | Subject | Re: [PATCH 4/5] clk: qcom: Add camera clock controller driver for SM8150 | From | "Satya Priya Kakitapalli (Temp)" <> |
| |
On 3/8/2024 5:24 PM, Dmitry Baryshkov wrote: > On Fri, 8 Mar 2024 at 12:47, Satya Priya Kakitapalli (Temp) > <quic_skakitap@quicinc.com> wrote: >> >> On 3/6/2024 7:25 PM, Bryan O'Donoghue wrote: >>> On 06/03/2024 08:30, Satya Priya Kakitapalli (Temp) wrote: >>>>> Anyway I suspect the right thing to do is to define a >>>>> titan_top_gdsc_clk with shared ops to "park" the GDSC clock to 19.2 >>>>> MHz instead of turning it off. >>>>> >>>>> You can get rid of the hard-coded always-on and indeed represent the >>>>> clock in /sysfs - which is preferable IMO to just whacking registers >>>>> to keep clocks always-on in probe anyway. >>>>> >>>>> Please try to define the titan_top_gdsc_clk as a shared_ops clock >>>>> instead of hard coding to always on. >>>>> >>>> Defining the gdsc clk allows consumers to control it, we do not want >>>> this clock to be disabled/controlled from consumers. Hence it is >>>> better to not model this clock and just keep it always on from probe. >>> Not if you mark it critical >>> >> Marking the clock as critical keeps the associated power domain >> always-on which impacts power. For this reason we are not using >> CLK_IS_CRITICAL and instead making them always on from probe. > Please consider using pm_clk instead. This is a cleaner solution > compared to keeping the clocks always on.
In this case i think we cannot use pm_clk because, the clock that we are trying to keep always on here belongs to same camcc and it is not possible to create a PM dependency with the same dev that is camcc itself.
>>> static struct clk_branch cam_cc_gdsc_clk = { >>> .halt_reg = 0xc1e4, >>> .halt_check = BRANCH_HALT, >>> .clkr = { >>> .enable_reg = 0xc1e4, >>> .enable_mask = BIT(0), >>> .hw.init = &(struct clk_init_data){ >>> .name = "cam_cc_gdsc_clk", >>> .parent_hws = (const struct clk_hw*[]){ >>> &cam_cc_xo_clk_src.clkr.hw >>> }, >>> .num_parents = 1, >>> .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, >>> .ops = &clk_branch2_ops, >>> }, >>> }, >>> }; >>> >>> and then add this to your camss clocks >>> >>> <&clock_camcc CAM_CC_GDSC_CLK>; >>> >>> The practice we have of just whacking clocks always-on in the probe() >>> of the clock driver feels lazy to me, leaving the broken cleanups we >>> have aside. >>> >>> As a user of the system I'd rather see correct/complete data in >>> /sys/kernel/debug/clk/clk_summary >>> >>> Anyway I'm fine with setting the clock always on, I can always send >>> out a series to address this bug-bear myself. >>> >>> So yeah just fix the cleanup and then please feel free to add my >>> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >
| |