[PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API

[PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API

[Cristian Marussi]

Support also atomic enable/disable clk_ops beside the bare non-atomic one
(prepare/unprepare) when the underlying SCMI transport is configured to
support atomic transactions for synchronous commands.

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>
---
NOTE THAT STILL THERE'S NO FINE GRAIN CONTROL OVER SELECTION
OF WHICH CLOCK DEVICES CAN SUPPORT ATOMIC AND WHICH SHOULD NOT
BASED ON CLOCK DEVICES ENABLE LATENCY.
THIS HAS STILL TO BE ADDED IN SCMI PROTOCOL SPEC.

v7 --> v8
- provide prepare/unprepare only on non-atomic transports
V5 --> V6
- add concurrent availability of atomic and non atomic reqs
---
 drivers/clk/clk-scmi.c | 54 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 1e357d364ca2..3da99f81ead9 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -88,21 +88,51 @@ static void scmi_clk_disable(struct clk_hw *hw)
 	scmi_proto_clk_ops->disable(clk->ph, clk->id);
 }
 
+static int scmi_clk_atomic_enable(struct clk_hw *hw)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	return scmi_proto_clk_ops->enable_atomic(clk->ph, clk->id);
+}
+
+static void scmi_clk_atomic_disable(struct clk_hw *hw)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	scmi_proto_clk_ops->disable_atomic(clk->ph, clk->id);
+}
+
+/*
+ * We can provide enable/disable 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,
-	/*
-	 * We can't provide enable/disable callback as we can't perform the same
-	 * in atomic context. Since the clock framework provides standard API
-	 * clk_prepare_enable that helps cases using clk_enable in non-atomic
-	 * context, it should be fine providing prepare/unprepare.
-	 */
 	.prepare = scmi_clk_enable,
 	.unprepare = scmi_clk_disable,
 };
 
-static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
+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,
+};
+
+static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
+			     const struct clk_ops *scmi_ops)
 {
 	int ret;
 	unsigned long min_rate, max_rate;
@@ -110,7 +140,7 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
 	struct clk_init_data init = {
 		.flags = CLK_GET_RATE_NOCACHE,
 		.num_parents = 0,
-		.ops = &scmi_clk_ops,
+		.ops = scmi_ops,
 		.name = sclk->info->name,
 	};
 
@@ -145,6 +175,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	struct device_node *np = dev->of_node;
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
+	const struct clk_ops *scmi_ops;
 
 	if (!handle)
 		return -ENODEV;
@@ -168,6 +199,11 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	clk_data->num = count;
 	hws = clk_data->hws;
 
+	if (handle->is_transport_atomic(handle))
+		scmi_ops = &scmi_atomic_clk_ops;
+	else
+		scmi_ops = &scmi_clk_ops;
+
 	for (idx = 0; idx < count; idx++) {
 		struct scmi_clk *sclk;
 
@@ -184,7 +220,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		sclk->id = idx;
 		sclk->ph = ph;
 
-		err = scmi_clk_ops_init(dev, sclk);
+		err = scmi_clk_ops_init(dev, sclk, scmi_ops);
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk);
-- 
2.17.1

Re: [PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API

[Stephen Boyd]

Quoting Cristian Marussi (2021-12-20 11:56:46)
> Support also atomic enable/disable clk_ops beside the bare non-atomic one
> (prepare/unprepare) when the underlying SCMI transport is configured to
> support atomic transactions for synchronous commands.
> 
> 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>
> ---
> NOTE THAT STILL THERE'S NO FINE GRAIN CONTROL OVER SELECTION
> OF WHICH CLOCK DEVICES CAN SUPPORT ATOMIC AND WHICH SHOULD NOT
> BASED ON CLOCK DEVICES ENABLE LATENCY.
> THIS HAS STILL TO BE ADDED IN SCMI PROTOCOL SPEC.

Why are you yelling on the internet? :-) I guess I need to ack this.

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

Re: [PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API

[Sudeep Holla]

On Fri, Jan 14, 2022 at 03:08:37PM -0800, Stephen Boyd wrote:
> Quoting Cristian Marussi (2021-12-20 11:56:46)
> > Support also atomic enable/disable clk_ops beside the bare non-atomic one
> > (prepare/unprepare) when the underlying SCMI transport is configured to
> > support atomic transactions for synchronous commands.
> > 
> > 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>
> > ---
> > NOTE THAT STILL THERE'S NO FINE GRAIN CONTROL OVER SELECTION
> > OF WHICH CLOCK DEVICES CAN SUPPORT ATOMIC AND WHICH SHOULD NOT
> > BASED ON CLOCK DEVICES ENABLE LATENCY.
> > THIS HAS STILL TO BE ADDED IN SCMI PROTOCOL SPEC.
> 
> Why are you yelling on the internet? :-) I guess I need to ack this.
>

It is for the partners who request such changes. We are trying to prototype
and share the code and ask for feedback before we finalise the specification.

In fact it is other way around for you 😁. Not to ack as it is not yet final
😉. At least I need to wait until the spec contents are finalised before I
can merge with your ack 😁. But I agree RFC would have indicated that along
with the above background instead of *yelling*. Cristian assumed everyone
is aware of the content as quite a few are involved in offline discussions.

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

Thanks anyways, will use it if nothing changes.

-- 
Regards,
Sudeep

Re: [PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API

[Cristian Marussi]

On Mon, Jan 17, 2022 at 10:31:00AM +0000, Sudeep Holla wrote:
> On Fri, Jan 14, 2022 at 03:08:37PM -0800, Stephen Boyd wrote:
> > Quoting Cristian Marussi (2021-12-20 11:56:46)
> > > Support also atomic enable/disable clk_ops beside the bare non-atomic one
> > > (prepare/unprepare) when the underlying SCMI transport is configured to
> > > support atomic transactions for synchronous commands.
> > > 

Hi,

> > > 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>
> > > ---
> > > NOTE THAT STILL THERE'S NO FINE GRAIN CONTROL OVER SELECTION
> > > OF WHICH CLOCK DEVICES CAN SUPPORT ATOMIC AND WHICH SHOULD NOT
> > > BASED ON CLOCK DEVICES ENABLE LATENCY.
> > > THIS HAS STILL TO BE ADDED IN SCMI PROTOCOL SPEC.
> > 
> > Why are you yelling on the internet? :-) I guess I need to ack this.
> >
> 

Sorry I did not mean to yell really, just to warn partners using this.

> It is for the partners who request such changes. We are trying to prototype
> and share the code and ask for feedback before we finalise the specification.
> 
> In fact it is other way around for you 😁. Not to ack as it is not yet final
> 😉. At least I need to wait until the spec contents are finalised before I
> can merge with your ack 😁. But I agree RFC would have indicated that along
> with the above background instead of *yelling*. Cristian assumed everyone
> is aware of the content as quite a few are involved in offline discussions.
> 
> > Acked-by: Stephen Boyd <sboyd@kernel.org>
> 
> Thanks anyways, will use it if nothing changes.
> 

As Sudeep said, V8 it is indeed not the final version which is going to
be posted soon after the merge-windows (without yelling :D) and which it
will be indeed still marked as RFC since it does include a new protocol
feature which is still under review and not published.

Sorry for the noise.

Thanks,
Cristian