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

  SCMI Clocks descriptors expose and increasing number of properties that in
turn lead to a different set of supported CLK operations to be associated
dynamically with a clock.

Providing statically pre-defined CLK operations structs for all the
possible combinations of allowed properties is cumbersome and error-prone.

Allocate per-clock operations descriptor dynamically and populate it with
the strictly needed set of operations depending on the advertised clock
properties.

CC: Michael Turquette <mturquette@baylibre.com>
CC: Stephen Boyd <sboyd@kernel.org>
CC: linux-clk@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/clk/clk-scmi.c | 129 ++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 59 deletions(-)
  

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;
>  }
>  
> -/*
> - * 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?

> -       .recalc_rate = scmi_clk_recalc_rate,
> -       .round_rate = scmi_clk_round_rate,
> -       .set_rate = scmi_clk_set_rate,
> -       .enable = scmi_clk_atomic_enable,
> -       .disable = scmi_clk_atomic_disable,
> -       .is_enabled = scmi_clk_atomic_is_enabled,
  

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
  

Quoting Cristian Marussi (2024-02-22 00:28:41)
> On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote:
> > 
> > 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

SCMI is a server!? :)

> 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.
> 

Yes please don't allocate a clk_op per clk. And, please add these
answers to the commit text so that we know why it's not possible to know
all combinations or fail clk_ops calls.
  

On Wed, Feb 28, 2024 at 06:20:34PM -0800, Stephen Boyd wrote:
> Quoting Cristian Marussi (2024-02-22 00:28:41)
> > On Wed, Feb 21, 2024 at 09:44:14PM -0800, Stephen Boyd wrote:
> > > 
> > > 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
> 
> SCMI is a server!? :)
> 

..well the platform fw act as a server in the client-server SCMI
model...so...I know these days it's cooler to be "serverless" but..hey...
..at least is not a BO2k server :P

> > 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.
> > 
> 
> Yes please don't allocate a clk_op per clk. And, please add these
> answers to the commit text so that we know why it's not possible to know
> all combinations or fail clk_ops calls.

Sure I posted this series a couple of days ago about this rework:

	https://lore.kernel.org/linux-arm-kernel/20240227194812.1209532-1-cristian.marussi@arm.com/

with a bit of context in the cover-letter and in the commit...but I can
add more commenting of course if needed.

Thanks for the review,
Cristian