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.
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@xxxxxxxxxx>
