On Fri, 8 Mar 2024 at 12:47, Satya Priya Kakitapalli (Temp) <quic_skakitap@xxxxxxxxxxx> 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. > > 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> > -- With best wishes Dmitry