On 6.03.2023 12:28, Konrad Dybcio wrote: > > > On 6.03.2023 02:21, Dmitry Baryshkov wrote: >> On Sat, 4 Mar 2023 at 15:27, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote: >>> >>> Some bus clock should always have a minimum (19.2 MHz) vote cast on >>> them, otherwise the platform will fall apart, hang and reboot. >>> >>> Add support for specifying which clocks should be kept alive and >>> always keep a vote on XO_A to make sure the clock tree doesn't >>> collapse. This removes the need to keep a maximum vote that was >>> previously guaranteed by clk_smd_rpm_handoff. >>> >>> This commit is a combination of existing (not-exactly-upstream) work >>> by Taniya Das, Shawn Guo and myself. >>> >>> Co-developed-by: Shawn Guo <shawn.guo@xxxxxxxxxx> >>> Co-developed-by: Taniya Das <quic_tdas@xxxxxxxxxxx> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> >>> --- [...] >> >>> + >>> + ret = clk_set_rate(keepalive_clks[i]->clk, 19200000); >> >> Don't we also need to provide a determine_rate() that will not allow >> one to set clock frequency below 19.2 MHz? > Hm, sounds like a good idea.. Thinking about it again, I'd have to do it before the clocks are registered and we'd either end up with 2 loops, one assigning the CLK_IS_CRITICAL flag and the other one setting the rate.. Will that not be too hacky? Konrad > >> >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + /* Keep an active vote on CXO in case no other driver votes for it. */ >>> + if (rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC]) >>> + return clk_prepare_enable(rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC]->hw.clk); >>> + >>> return 0; >>> err: >>> dev_err(&pdev->dev, "Error registering SMD clock driver (%d)\n", ret); >> >> >> I have mixed feelings towards this patch (and the rest of the >> patchset). It looks to me like we are trying to patch an issue of the >> interconnect drivers (or in kernel configuration). > Well, as you noticed, this patch tries to address a situation where a > critical clock could be disabled. The interconnect driver (as per my > other recent patchset) also has a concept of "keepalive", but: > > 1. not very many SoCs already have a functional icc driver > 2. devices with an existing interconnect driver should also be > testable without one (through painful ripping out everything-icc > from the dts) for regression tracking > > Konrad > >> >> >> -- >> With best wishes >> Dmitry
