On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx> wrote: > > On 04/10/2023 01:31, Dmitry Baryshkov wrote: > > clk_rcg2_shared_ops implements support for the case of the RCG which > > must not be completely turned off. However its design has one major > > drawback: it doesn't allow us to properly implement the is_enabled > > callback, which causes different kinds of misbehaviour from the CCF. > > > > Follow the idea behind clk_regmap_phy_mux_ops and implement the new > > clk_rcg2_parked_ops. It also targets the clocks which must not be fully > > switched off (and shared most of the implementation with > > clk_rcg2_shared_ops). The major difference is that it requires that the > > parent map doesn't conain the safe (parked) clock source. Instead if the > > CFG_REG register points to the safe source, the clock is considered to > > be disabled. > > Why not have a new bit in .flags ? > > Instead of lying about the clock being off, mark the clock as "parked", > or "safe parked" or whatever term we choose for it ? The main problem with adding flags doesn't fully scale. From the CCF perspective, what should be the difference between parked and disabled clocks? How should it treat the parked one? >From my point of view, the 'parked' clock is as good as 'disabled', because the end devices do not work with the corresponding clock being sourced from XO. We already have a similar implementation for the PCIe PIPE clocks, which reinterpret physical 'XO' source as a 'disabled' state to facilitate disabling the clock before turning the genpd off. > > I feel 'disabled' should mean disabled and 'enabled' should me enabled > when I read a value from debugfs and if we are parking a clock it should > have a clear means of being flagged as a clock that should be parked. > > An example. I recently inherited some autogenerated code for camcc on > sc8280xp. > > One of the clocks is marked as CLK_IS_CRITICAL by the autogen code > meaning "never switch off" but CLK_IS_CRITICAL stops the camcc driver > from doing runtime pm_ops to power collapse. > > The solution I have is to remove CLK_IS_CRITICAL and to hard-code in > probe() the clock to always on. > > But if we had a CLK_DISABLE_SAFE_PARK flag - then not just for rcg but > for branch clocks we could differentiate away from hard-coded always on > in probe... > > ? > > --- > bod -- With best wishes Dmitry
