09:12 <Marijn[m]> bamse: lumag who is the "SoC maintainer" for SM6125 (the display portion)? In my series krzk pointed out that I should ask whether an ABI break in dispcc clock array is acceptible (the wrong bindings were contributed by me before, they don't represent the hardware)

09:13 <krzk> Marijn[m]: SoC/Platform maintainer, so Bjorn.

09:15 <krzk> Marijn[m]: and as said many times - ABI breaks needs some justification and are frequently accepted, if the justification (e.g. development or never-working solution) is correct. That's said, it is still better to avoid ABI breaks, even these justified, if it can be easily avoided.

09:16 <krzk> Like putting the clock to the end of the list instead of in the middle...

09:21 <Marijn[m]> krzk: sure, I can move the clock to the end to reduce the scope of the break, but as we found out that conflicts with another clock that was removed from the end of the list in an earlier patch...

09:24 <krzk> Marijn[m]: life, it should be then explained it will break the ABI with its justification and be doen with it

09:25 <Marijn[m]> Yes sure, I'll add such a comment to both commits after receiving confirmation from bamse. Thanks for all the assistance and coping with my misunderstanding!

09:26 <krzk> I don't think that bamse will object here :)

09:27 <Marijn[m]> Okay, maybe I'll send v2 with what we discussed after work even if I don't hear anything :)

12:15 <ungeskriptet[m]> Is it possible for Linux to disable a clock that isn't defined in gcc/dispcc? I'm trying to figure out which clocks are needed to drive simple framebuffer and with almost all clocks defined in my dts the screen stays black

12:16 <ungeskriptet[m]> My device is Xiaomi Redmi Note 9 Pro with SM7125

12:30 <konradybcio> if clk_ignore_unused fixes it, chances are you hit some weird insider bug and the 200 clocks you passed are not fully parse

12:31 <konradybcio> s/clk_ignore_unused/clk\_ignore\_unused/, s/parse/parsed/

12:34 <ungeskriptet[m]> It does fix it

14:34 <Marijn[m]> Typically it takes a bit (±10s) for clk_ignore_unused to kick in, or maybe my device is just slow with all the probe deferrals :)

14:35 <Marijn[m]> Err I mean: for unused clocks to be turned off unless clk_ignore_unused is set

15:28 <bamse> Marijn[m]: if you really have to break compatibilty, document that in your change and be aware that you might be causing headache for users

15:29 <Marijn[m]> bamse: I'm fairly confident we're the only consumers of SM6125 dispcc

15:29 <Marijn[m]> As mentioned many times in that thread the DT-bindings don't match the hardware nor the driver so it's useless either way:)

15:39 <bamse> Marijn[m]: then let's fix the binding before you get more users

15:39 <Marijn[m]> So I will remove one unused clock, and append a used-but-undefined clock at the end of the array in dt-bindings

15:41 <bamse> Marijn[m]: is that fixing it, or just patching it up?

15:41 <Marijn[m]> How do you define either?

15:41 <bamse> is the outcome the way you want it, or is it the smallest possible change to get what you want?

15:41 <Marijn[m]> bamse: perhaps it is easier to look at the series: https://lore.kernel.org/linux-arm-msm/20230624-sm6125-dpu-v1-0-1d5a638cebf2@somainline.org/T/#u

15:42 <Marijn[m]> That series is currently the way that I want it, but I'll move gcc_disp_gpll0_div_clk_src to the end of the list to turn it into the smallest possible change

15:42 <Marijn[m]> It could be even smaller if we don't drop the "unused" cfg_ahb_clk

15:43 <bamse> you mark cfg_ahb_clk as always-on in gcc?

15:55 <Marijn[m]> bamse: we have `CLK_IS_CRITICAL` in dispcc

15:56 <Marijn[m]> And downstream anyway gets+puts it, so that should only check if the clock exists but should not (afaik) prevent it from being gated (if it wasn't for the equivalent of CLK_IS_CRITICAL)?

15:56 <bamse> Marijn[m]: okay, you should replace that with a write in probe, like the other drivers...because IS_CRITICAL will keep the power-domain always-on...

15:57 <Marijn[m]> The power-domain of dispcc?

15:57 <bamse> Marijn[m]: we typically don't toggle the cfg_ahb_clk...but in the event that you build both gcc and dispcc into the kernel, you need something to ensure the ordering (which i guess that pll would handle for you...)

15:58 <Marijn[m]> Ah it is only sm6125/sm6350/sm6375 still setting CLK_IS_CRITICAL in dispcc, no other driver sets it

15:58 <bamse> Marijn[m]: the power-domain of the clock controller owning the clock

15:58 <Marijn[m]> Yeah, so that's dispcc, which owns gcc_disp_ahb_clk

15:59 <Marijn[m]> Err, no, I mean GCC

15:59 <bamse> Marijn[m]: and the reason you want your clock controller in a power-domain is so that the gdscs get a parent...so that you can cast votes on the gdsc, affecting e.g. cx, when you do frequency scaling in your clients

15:59 <konradybcio> except different GDSCs have different parents and we're slacking off on fixing this :

15:59 <konradybcio> :/

16:00 <bamse> konradybcio: do you have an example where that's the case?

16:00 <Marijn[m]> Actually there are a lot of clock controllers still setting CLK_IS_CRITICAL

16:00 <konradybcio> sm6375's gpu_cc_gx_gdsc is powered by VDD_GFX

16:00 <konradybcio> cx_gdsc is powered by VDD_CX

16:01 <Marijn[m]> Anyway, thanks for saying this. Previously reviewers have to told to use a write the bits in probe instead of using CLK_IS_CRITICAL "so that there are less clocks for Linux to track", but there's actually a secondary important reason to it...

16:01 <bamse> konradybcio: ahh, you're right

16:01 <bamse> Marijn[m]: yeah, that's a reasonable argument...but the power-domain being kept on is a bigger issue...

16:01 <bamse> and kudos to konradybcio for highlighting that

16:02 <Marijn[m]> bamse: so anyway back to the original patch, I'll clean up dt-bindings to not take `gcc_disp_ahb_clk`

16:02 <Marijn[m]> And maybe separately, later, should we clean up all the CLK_IS_CRITICAL (all at once?), that doesn't concern this series as much

16:02 <Marijn[m]> bamse: hehe exactly my point ;)

16:02 <Marijn[m]> The typical case of "doing the right thing for the wrong reasons"

16:02 <bamse> Marijn[m]: you need to have some gcc clock there, to ensure ordering between the write in gcc_probe() and dispcc trying to access its registers

16:04 <Marijn[m]> bamse: ah, sure, that's going to be the (mandatory because dispcc parents it!) `gcc_disp_gpll0_div_clk_src` clock in the next patch then

16:04 <bamse> Marijn[m]: yeah, that should be sufficient

16:04 <bamse> Marijn[m]: but...

16:04 <Marijn[m]> So I'm not sure why the downstream clock needed that cfg_ahb_clk hack... Maybe to happen before register writes?

16:05 <bamse> Marijn[m]: you're removing the clock from the dt binding under the premise that in linux we just statically vote the clock on, so therefore we don't need to see it in dispcc

16:05 <Marijn[m]> So that it first makes sure GCC clocks don't return -EDEFER_PROBE, before doing bitfield writes?

16:06 <Marijn[m]> bamse: I can also leave it in dt-bindings in case we want to vote it later, even if the driver doesn't use it. As said many times the dt-bindings represent the hardware... but I have no hardware manual so have to build off of the spaghetti that dares to call itself a downstream kernel

16:06 <bamse> Marijn[m]: i'd prefer that you keep it in the binding and just completely ignore it in the implementation

16:06 <Marijn[m]> 👍️

16:06 <Marijn[m]> Will drop that patch