Re: [PATCH 5/6] clk: qcom: gcc-sdm660: Account for needed adjustments in probe function

[Konrad Dybcio]

>>>> +
>>>> +       /* Keep bimc gfx clock port on all the time */
>>>> +       clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk);
>>>> +
>>> Preferably just set these various bits with regmap_update_bits() during
>>> probe. Also, please do it before regsitering the clks, not after.
>> To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot.
> This is how it's been done in various other qcom clk drivers. Usually
> there is a comment about what is enabled, but really it's just setting
> random bits that sadly aren't already set by default.

But why.. why should we give Linux less information about the hardware it's running on? It's a kernel after all, I know some parties would prefer to keep the hardware away from its users, but cmon, it's OSSLand out there!

Allocating a few bytes more in memory is proooobably a good trade-off for keeping an eye on the state of various clocks instead of simply setting some seemingly random bits and hoping nothing bad happens under the hood.. This isn't only a case for this clock, but for all ones that are effectively pinky-promise-trusted to function properly with no way of checking if they're even still alive other than poking the registers manually.. As of v5.12-rc2, there are *46* such trust credits..

It's NOT easy to track down issues in big monolithic kernels, especially when people submitting changes to common code seem to think that only their hardware uses it (no hard feelings, but drm/msm broke on !a6xx *at least* two times within the last year) and since making sure the clocks are ticking properly is one of the most crucial things when looking into more complex issues, which may or may not randomly happen on a platform that is just being brought up for various reasons (e.g. half of mdm9607 hardware doesn't wanna play along without a ICC driver), I *really* think we should use CLK_IS_CRITICAL instead and give developers a way to check if everything's in tact with the clock, while still keeping the "never turn it off, don't touch it!" aspect.


>>>> +       /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */
>>>> +       clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000);
>>> Is this not already the case?
>>
>> This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear.
>>
> What does the bootloader set it to?

Sorry but I can't check, nor do I remember right now. But we still shouldn't add a variable that might come from a lazy OEM not incorporating the fix into their release, especially since this one is used for clocking the AP.


Konrad