Re: [PATCH 4/5] clk: qcom: Add camera clock controller driver for SM8150

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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>




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux