Re: [PATCH 6/7] clk: scmi: Allocate CLK operations dynamically

[Cristian Marussi]

On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote:
> Quoting Cristian Marussi (2024-02-14 10:30:05)
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index 5747b6d651f0..b91a0dbd2fe0 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -158,51 +158,6 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
> >         return !!enabled;
> >  }
> >  

Hi Stephen,

thanks for having a look.

> > -/*
> > - * We can provide enable/disable/is_enabled atomic callbacks only if the
> > - * underlying SCMI transport for an SCMI instance is configured to handle
> > - * SCMI commands in an atomic manner.
> > - *
> > - * When no SCMI atomic transport support is available we instead provide only
> > - * the prepare/unprepare API, as allowed by the clock framework when atomic
> > - * calls are not available.
> > - *
> > - * Two distinct sets of clk_ops are provided since we could have multiple SCMI
> > - * instances with different underlying transport quality, so they cannot be
> > - * shared.
> > - */
> > -static const struct clk_ops scmi_clk_ops = {
> > -       .recalc_rate = scmi_clk_recalc_rate,
> > -       .round_rate = scmi_clk_round_rate,
> > -       .set_rate = scmi_clk_set_rate,
> > -       .prepare = scmi_clk_enable,
> > -       .unprepare = scmi_clk_disable,
> > -       .set_parent = scmi_clk_set_parent,
> > -       .get_parent = scmi_clk_get_parent,
> > -       .determine_rate = scmi_clk_determine_rate,
> > -};
> > -
> > -static const struct clk_ops scmi_atomic_clk_ops = {
> 
> It's not great to move these function pointer structs out of RO memory
> to RW. I'm also not convinced that it's any better to construct them at
> runtime. Isn't there a constant set of possible clk configurations? Or
> why can't we simply add some failures to the clk_ops functions instead?

Well, the real clock devices managed by the SCMI server can be a of
varying nature and so the minimum set of possible clk configurations
to cover will amount to all the possible combinations of supported ops
regarding the specific clock properties (i.e. .set_parent / .set_rate /
enable / .get/set_duty_cycle / atomic_capability ... for now)...we
simply cannot know in advance what the backend SCMI server is handling.

These seemed to me too much in number (and growing) to be pre-allocated
in all possible combinations. (and mostly wasted since you dont really
probably use all combinations all the time)

Moreover, SCMI latest spec now exposes some clock properties (or not) to
be able avoid even sending an actual SCMI message that we know will be
denied all the time; one option is that we return an error,, as you said,
but what is the point (I thought) to provide at all a clk-callback that
we know upfront will fail to be executed every time ? (and some consumer
drivers have been reported by partners not to be happy with these errors)

What I think could be optimized here instead, and I will try in the next
respin, it is that now I am allocating one set of custom ops for each clock
at the end, even if exactly the same ops are provided since the clock
capabilities are the same; I could instead allocate dynamically and fill only
one single set of ops for each distinct set of combinations, so as to avoid
useless duplication and use only the miminum strict amount of RW memory
needed.

Thanks,
Cristian