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

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

[Cristian Marussi]

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(-)

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 = {
-	.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,
-	.set_parent = scmi_clk_set_parent,
-	.get_parent = scmi_clk_get_parent,
-	.determine_rate = scmi_clk_determine_rate,
-};
-
-static const struct clk_ops scmi_no_state_ctrl_clk_ops = {
-	.recalc_rate = scmi_clk_recalc_rate,
-	.round_rate = scmi_clk_round_rate,
-	.set_rate = scmi_clk_set_rate,
-	.set_parent = scmi_clk_set_parent,
-	.get_parent = scmi_clk_get_parent,
-	.determine_rate = scmi_clk_determine_rate,
-};
-
 static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 			     const struct clk_ops *scmi_ops)
 {
@@ -239,6 +194,71 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 	return ret;
 }
 
+/**
+ * scmi_clk_ops_alloc() - Alloc and configure CLK ops
+ * @sclk: A reference to an SCMI clock descriptor
+ * @atomic_capable: A flag to indicate if atomic mode is supported by the
+ *		    transport
+ * @atomic_threshold: Platform atomic threshold value
+ *
+ * Allocate and configure a proper set of CLK operations depending on the
+ * specific SCMI clock characteristics and platform atomic operation capability.
+ *
+ * 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.
+ *
+ * Return: A pointer to the allocated and configured clk_ops on Success,
+ *	   NULL otherwise.
+ */
+static const struct clk_ops *
+scmi_clk_ops_alloc(struct scmi_clk *sclk, bool atomic_capable,
+		   unsigned int atomic_threshold)
+{
+	const struct scmi_clock_info *ci = sclk->info;
+	struct clk_ops *ops;
+
+	ops = devm_kzalloc(sclk->dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return NULL;
+
+	/*
+	 * Note that when transport is atomic but SCMI protocol did not
+	 * specify (or support) an enable_latency associated with a
+	 * clock, we default to use atomic operations mode.
+	 */
+	if (!ci->state_ctrl_forbidden) {
+		if (atomic_capable && ci->enable_latency <= atomic_threshold) {
+			ops->enable = scmi_clk_atomic_enable;
+			ops->disable = scmi_clk_atomic_disable;
+		} else {
+			ops->prepare = scmi_clk_enable;
+			ops->unprepare = scmi_clk_disable;
+		}
+	}
+
+	if (atomic_capable)
+		ops->is_enabled = scmi_clk_atomic_is_enabled;
+
+	/* Rate ops */
+	ops->recalc_rate = scmi_clk_recalc_rate;
+	ops->round_rate = scmi_clk_round_rate;
+	ops->determine_rate = scmi_clk_determine_rate;
+	if (!ci->rate_ctrl_forbidden)
+		ops->set_rate = scmi_clk_set_rate;
+
+	/* Parent ops */
+	ops->get_parent = scmi_clk_get_parent;
+	if (!ci->parent_ctrl_forbidden)
+		ops->set_parent = scmi_clk_set_parent;
+
+	return ops;
+}
+
 static int scmi_clocks_probe(struct scmi_device *sdev)
 {
 	int idx, count, err;
@@ -294,18 +314,9 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		sclk->ph = ph;
 		sclk->dev = dev;
 
-		/*
-		 * Note that when transport is atomic but SCMI protocol did not
-		 * specify (or support) an enable_latency associated with a
-		 * clock, we default to use atomic operations mode.
-		 */
-		if (sclk->info->state_ctrl_forbidden)
-			scmi_ops = &scmi_no_state_ctrl_clk_ops;
-		else if (is_atomic &&
-			 sclk->info->enable_latency <= atomic_threshold)
-			scmi_ops = &scmi_atomic_clk_ops;
-		else
-			scmi_ops = &scmi_clk_ops;
+		scmi_ops = scmi_clk_ops_alloc(sclk, is_atomic, atomic_threshold);
+		if (!scmi_ops)
+			return -ENOMEM;
 
 		/* Initialize clock parent data. */
 		if (sclk->info->num_parents > 0) {
@@ -324,13 +335,13 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk->parent_data);
+			devm_kfree(dev, scmi_ops);
 			devm_kfree(dev, sclk);
 			hws[idx] = NULL;
 		} else {
 			dev_dbg(dev, "Registered clock:%s%s\n",
 				sclk->info->name,
-				scmi_ops == &scmi_atomic_clk_ops ?
-				" (atomic ops)" : "");
+				scmi_ops->enable ? " (atomic ops)" : "");
 			hws[idx] = &sclk->hw;
 		}
 	}
-- 
2.43.0

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

[Stephen Boyd]

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,

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

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

[Stephen Boyd]

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.

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

[Cristian Marussi]

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

[PATCH 7/7] clk: scmi: Support get/set duty_cycle operations

[Cristian Marussi]

Provide the CLK framework callbacks related to get/set clock duty cycle if
the relared SCMI clock supports OEM extended configurations.

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 | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index b91a0dbd2fe0..9d3b1e749226 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -158,6 +158,45 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
 	return !!enabled;
 }
 
+static int scmi_clk_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
+{
+	int ret;
+	u32 val;
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	ret = scmi_proto_clk_ops->config_oem_get(clk->ph, clk->id,
+						 SCMI_CLOCK_CFG_DUTY_CYCLE,
+						 &val, NULL, false);
+	if (!ret) {
+		duty->num = val;
+		duty->den = 100;
+	} else {
+		dev_warn(clk->dev,
+			 "Failed to get duty cycle for clock ID %d\n", clk->id);
+	}
+
+	return ret;
+}
+
+static int scmi_clk_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
+{
+	int ret;
+	u32 val;
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	/* SCMI OEM Duty Cycle is expressed as a percentage */
+	val = (duty->num * 100) / duty->den;
+	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
+						 SCMI_CLOCK_CFG_DUTY_CYCLE,
+						 val, false);
+	if (ret)
+		dev_warn(clk->dev,
+			 "Failed to set duty cycle(%u/%u) for clock ID %d\n",
+			 duty->num, duty->den, clk->id);
+
+	return ret;
+}
+
 static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 			     const struct clk_ops *scmi_ops)
 {
@@ -256,6 +295,12 @@ scmi_clk_ops_alloc(struct scmi_clk *sclk, bool atomic_capable,
 	if (!ci->parent_ctrl_forbidden)
 		ops->set_parent = scmi_clk_set_parent;
 
+	/* Duty cycle */
+	if (ci->extended_config) {
+		ops->get_duty_cycle = scmi_clk_get_duty_cycle;
+		ops->set_duty_cycle = scmi_clk_set_duty_cycle;
+	}
+
 	return ops;
 }
 
-- 
2.43.0

Re: [PATCH 7/7] clk: scmi: Support get/set duty_cycle operations

[Stephen Boyd]

Quoting Cristian Marussi (2024-02-14 10:30:06)
> Provide the CLK framework callbacks related to get/set clock duty cycle if
> the relared SCMI clock supports OEM extended configurations.
> 
> 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>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>