[PATCH v2 0/2] firmware: arm_scmi: clock: support parents commands

[PATCH v2 0/2] firmware: arm_scmi: clock: support parents commands

[Peng Fan (OSS)]

V2:
 Add determine_rate hooks

SCMI v3.2 spec adds parents commands, this patchset is to support them:
CLOCK_POSSIBLE_PARENTS_GET
CLOCK_PARENT_SET
CLOCK_PARENT_GET

Besides firmware api clock driver update, the clk_scmi driver also
updated to support set_parent and get_parent ops.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (2):
      firmware: arm_scmi: clock: support clock parents
      clk: scmi: add set/get_parent support

 drivers/clk/clk-scmi.c            |  50 +++++++++++-
 drivers/firmware/arm_scmi/clock.c | 156 ++++++++++++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h     |   5 ++
 3 files changed, 204 insertions(+), 7 deletions(-)
---
base-commit: 8fff9184d1b5810dca5dd1a02726d4f844af88fc
change-id: 20230925-scmi-clock-v2-042cf8e5cb77

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] firmware: arm_scmi: clock: support clock parents

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

SCMI v3.2 spec introduces CLOCK_POSSIBLE_PARENTS_GET, CLOCK_PARENT_SET
and CLOCK_PARENT_GET. This patch is to add the upper three new
commands.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/clock.c | 156 ++++++++++++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h     |   5 ++
 2 files changed, 155 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index d18bf789fc24..38278922890a 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -22,6 +22,9 @@ enum scmi_clock_protocol_cmd {
 	CLOCK_RATE_NOTIFY = 0x9,
 	CLOCK_RATE_CHANGE_REQUESTED_NOTIFY = 0xA,
 	CLOCK_CONFIG_GET = 0xB,
+	CLOCK_POSSIBLE_PARENTS_GET = 0xC,
+	CLOCK_PARENT_SET = 0xD,
+	CLOCK_PARENT_GET = 0xE,
 };
 
 enum clk_state {
@@ -42,10 +45,28 @@ struct scmi_msg_resp_clock_attributes {
 #define SUPPORTS_RATE_CHANGED_NOTIF(x)		((x) & BIT(31))
 #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x)	((x) & BIT(30))
 #define SUPPORTS_EXTENDED_NAMES(x)		((x) & BIT(29))
+#define SUPPORTS_PARENT_CLOCK(x)		((x) & BIT(28))
 	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 	__le32 clock_enable_latency;
 };
 
+struct scmi_msg_clock_possible_parents {
+	__le32 id;
+	__le32 skip_parents;
+};
+
+struct scmi_msg_resp_clock_possible_parents {
+	__le32 num_parent_flags;
+#define NUM_PARENTS_RETURNED(x)		((x) & 0xff)
+#define NUM_PARENTS_REMAINING(x)	((x) >> 24)
+	u32 possible_parents[];
+};
+
+struct scmi_msg_clock_set_parent {
+	__le32 id;
+	__le32 parent_id;
+};
+
 struct scmi_msg_clock_config_set_v2 {
 	__le32 id;
 	__le32 attributes;
@@ -167,6 +188,81 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+struct scmi_clk_ipriv {
+	struct device *dev;
+	u32 clk_id;
+	struct scmi_clock_info *clk;
+};
+
+static void iter_clk_possible_parents_prepare_message(void *message, unsigned int desc_index,
+						      const void *priv)
+{
+	struct scmi_msg_clock_possible_parents *msg = message;
+	const struct scmi_clk_ipriv *p = priv;
+
+	msg->id = cpu_to_le32(p->clk_id);
+	/* Set the number of OPPs to be skipped/already read */
+	msg->skip_parents = cpu_to_le32(desc_index);
+}
+
+static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st,
+						  const void *response, void *priv)
+{
+	const struct scmi_msg_resp_clock_possible_parents *r = response;
+	u32 flags;
+
+	flags = le32_to_cpu(r->num_parent_flags);
+	st->num_returned = NUM_PARENTS_RETURNED(flags);
+	st->num_remaining = NUM_PARENTS_REMAINING(flags);
+
+	return 0;
+}
+
+static int iter_clk_possible_parents_process_response(const struct scmi_protocol_handle *ph,
+						      const void *response,
+						      struct scmi_iterator_state *st,
+						      void *priv)
+{
+	const struct scmi_msg_resp_clock_possible_parents *r = response;
+	struct scmi_clk_ipriv *p = priv;
+
+	u32 *parent = &p->clk->parents[st->desc_index + st->loop_idx];
+
+	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
+	p->clk->num_parents++;
+
+	return 0;
+}
+
+static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u32 clk_id,
+				       struct scmi_clock_info *clk)
+{
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_clk_possible_parents_prepare_message,
+		.update_state = iter_clk_possible_parents_update_state,
+		.process_response = iter_clk_possible_parents_process_response,
+	};
+
+	struct scmi_clk_ipriv ppriv = {
+		.clk_id = clk_id,
+		.clk = clk,
+		.dev = ph->dev,
+	};
+	void *iter;
+	int ret;
+
+	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_PARENTS,
+					    CLOCK_POSSIBLE_PARENTS_GET,
+					    sizeof(struct scmi_msg_clock_possible_parents),
+					    &ppriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	ret = ph->hops->iter_response_run(iter);
+
+	return ret;
+}
+
 static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 				     u32 clk_id, struct scmi_clock_info *clk,
 				     u32 version)
@@ -211,6 +307,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 			clk->rate_changed_notifications = true;
 		if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
 			clk->rate_change_requested_notifications = true;
+		if (SUPPORTS_PARENT_CLOCK(attributes))
+			scmi_clock_possible_parents(ph, clk_id, clk);
 	}
 
 	return ret;
@@ -228,12 +326,6 @@ static int rate_cmp_func(const void *_r1, const void *_r2)
 		return 1;
 }
 
-struct scmi_clk_ipriv {
-	struct device *dev;
-	u32 clk_id;
-	struct scmi_clock_info *clk;
-};
-
 static void iter_clk_describe_prepare_message(void *message,
 					      const unsigned int desc_index,
 					      const void *priv)
@@ -457,6 +549,56 @@ scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 	return ret;
 }
 
+static int
+scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
+		      u32 parent_id)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_clock_set_parent *cfg;
+	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk = ci->clk + clk_id;
+
+	ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
+				      sizeof(*cfg), 0, &t);
+	if (ret)
+		return ret;
+
+	t->hdr.poll_completion = false;
+
+	cfg = t->tx.buf;
+	cfg->id = cpu_to_le32(clk_id);
+	cfg->parent_id = cpu_to_le32(clk->parents[parent_id]);
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int
+scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
+		      u32 *parent_id)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_GET,
+				      sizeof(__le32), sizeof(u32), &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(clk_id, t->tx.buf);
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret)
+		*parent_id = get_unaligned_le32(t->rx.buf);
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
 static int
 scmi_clock_config_set_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
 			  enum clk_state state, u8 oem_type, u32 oem_val,
@@ -647,6 +789,8 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
 	.state_get = scmi_clock_state_get,
 	.config_oem_get = scmi_clock_config_oem_get,
 	.config_oem_set = scmi_clock_config_oem_set,
+	.parent_set = scmi_clock_set_parent,
+	.parent_get = scmi_clock_get_parent,
 };
 
 static int scmi_clk_rate_notify(const struct scmi_protocol_handle *ph,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 27bfa5a65b45..3175a9b4b8d8 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -16,6 +16,7 @@
 #define SCMI_MAX_STR_SIZE		64
 #define SCMI_SHORT_NAME_MAX_SIZE	16
 #define SCMI_MAX_NUM_RATES		16
+#define SCMI_MAX_NUM_PARENTS		8
 
 /**
  * struct scmi_revision_info - version information structure
@@ -58,6 +59,8 @@ struct scmi_clock_info {
 			u64 step_size;
 		} range;
 	};
+	int num_parents;
+	u32 parents[SCMI_MAX_NUM_PARENTS];
 };
 
 enum scmi_power_scale {
@@ -104,6 +107,8 @@ struct scmi_clk_proto_ops {
 			      bool atomic);
 	int (*config_oem_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			      u8 oem_type, u32 oem_val, bool atomic);
+	int (*parent_get)(const struct scmi_protocol_handle *ph, u32 clk_id, u32 *parent_id);
+	int (*parent_set)(const struct scmi_protocol_handle *ph, u32 clk_id, u32 parent_id);
 };
 
 struct scmi_perf_domain_info {

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] clk: scmi: add set/get_parent support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

SCMI v3.2 adds set/get parent clock commands, so update the clk driver
to support them.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk-scmi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 2e1337b511eb..5aaca674830f 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -24,6 +24,7 @@ struct scmi_clk {
 	struct clk_hw hw;
 	const struct scmi_clock_info *info;
 	const struct scmi_protocol_handle *ph;
+	struct clk_parent_data *parent_data;
 };
 
 #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw)
@@ -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate);
 }
 
+static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
+}
+
+static u8 scmi_clk_get_parent(struct clk_hw *hw)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+	u32 parent_id;
+	int ret;
+
+	ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id);
+	if (ret)
+		return 0;
+
+	return parent_id;
+}
+
+static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	/*
+	 * Suppose all the requested rates are supported, and let firmware
+	 * to handle the left work.
+	 */
+	return 0;
+}
+
 static int scmi_clk_enable(struct clk_hw *hw)
 {
 	struct scmi_clk *clk = to_scmi_clk(hw);
@@ -139,6 +169,9 @@ static const struct clk_ops scmi_clk_ops = {
 	.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 = {
@@ -148,6 +181,9 @@ static const struct clk_ops scmi_atomic_clk_ops = {
 	.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 int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
@@ -158,9 +194,10 @@ 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,
+		.num_parents = sclk->info->num_parents,
 		.ops = scmi_ops,
 		.name = sclk->info->name,
+		.parent_data = sclk->parent_data,
 	};
 
 	sclk->hw.init = &init;
@@ -250,6 +287,17 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		else
 			scmi_ops = &scmi_clk_ops;
 
+		/* Initialize clock parent data. */
+		if (sclk->info->num_parents > 0) {
+			sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents,
+							 sizeof(*sclk->parent_data), GFP_KERNEL);
+
+			for (int i = 0; i < sclk->info->num_parents; i++) {
+				sclk->parent_data[i].index = sclk->info->parents[i];
+				sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
+			}
+		}
+
 		err = scmi_clk_ops_init(dev, sclk, scmi_ops);
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] firmware: arm_scmi: clock: support clock parents

[Cristian Marussi]

On Mon, Sep 25, 2023 at 04:47:42PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> SCMI v3.2 spec introduces CLOCK_POSSIBLE_PARENTS_GET, CLOCK_PARENT_SET
> and CLOCK_PARENT_GET. This patch is to add the upper three new
> commands.
> 

Hi,

a few notes down below.

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/clock.c | 156 ++++++++++++++++++++++++++++++++++++--
>  include/linux/scmi_protocol.h     |   5 ++
>  2 files changed, 155 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index d18bf789fc24..38278922890a 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -22,6 +22,9 @@ enum scmi_clock_protocol_cmd {
>  	CLOCK_RATE_NOTIFY = 0x9,
>  	CLOCK_RATE_CHANGE_REQUESTED_NOTIFY = 0xA,
>  	CLOCK_CONFIG_GET = 0xB,
> +	CLOCK_POSSIBLE_PARENTS_GET = 0xC,
> +	CLOCK_PARENT_SET = 0xD,
> +	CLOCK_PARENT_GET = 0xE,
>  };
>  
>  enum clk_state {
> @@ -42,10 +45,28 @@ struct scmi_msg_resp_clock_attributes {
>  #define SUPPORTS_RATE_CHANGED_NOTIF(x)		((x) & BIT(31))
>  #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x)	((x) & BIT(30))
>  #define SUPPORTS_EXTENDED_NAMES(x)		((x) & BIT(29))
> +#define SUPPORTS_PARENT_CLOCK(x)		((x) & BIT(28))
>  	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
>  	__le32 clock_enable_latency;
>  };
>  
> +struct scmi_msg_clock_possible_parents {
> +	__le32 id;
> +	__le32 skip_parents;
> +};
> +
> +struct scmi_msg_resp_clock_possible_parents {
> +	__le32 num_parent_flags;
> +#define NUM_PARENTS_RETURNED(x)		((x) & 0xff)
> +#define NUM_PARENTS_REMAINING(x)	((x) >> 24)
> +	u32 possible_parents[];
> +};
> +
> +struct scmi_msg_clock_set_parent {
> +	__le32 id;
> +	__le32 parent_id;
> +};
> +
>  struct scmi_msg_clock_config_set_v2 {
>  	__le32 id;
>  	__le32 attributes;
> @@ -167,6 +188,81 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +struct scmi_clk_ipriv {
> +	struct device *dev;
> +	u32 clk_id;
> +	struct scmi_clock_info *clk;
> +};
> +
> +static void iter_clk_possible_parents_prepare_message(void *message, unsigned int desc_index,
> +						      const void *priv)
> +{
> +	struct scmi_msg_clock_possible_parents *msg = message;
> +	const struct scmi_clk_ipriv *p = priv;
> +
> +	msg->id = cpu_to_le32(p->clk_id);
> +	/* Set the number of OPPs to be skipped/already read */
> +	msg->skip_parents = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st,
> +						  const void *response, void *priv)
> +{
> +	const struct scmi_msg_resp_clock_possible_parents *r = response;
> +	u32 flags;
> +
> +	flags = le32_to_cpu(r->num_parent_flags);
> +	st->num_returned = NUM_PARENTS_RETURNED(flags);
> +	st->num_remaining = NUM_PARENTS_REMAINING(flags);
> +
> +	return 0;
> +}
> +
> +static int iter_clk_possible_parents_process_response(const struct scmi_protocol_handle *ph,
> +						      const void *response,
> +						      struct scmi_iterator_state *st,
> +						      void *priv)
> +{
> +	const struct scmi_msg_resp_clock_possible_parents *r = response;
> +	struct scmi_clk_ipriv *p = priv;
> +
> +	u32 *parent = &p->clk->parents[st->desc_index + st->loop_idx];
> +
> +	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
> +	p->clk->num_parents++;
> +
> +	return 0;
> +}
> +
> +static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u32 clk_id,
> +				       struct scmi_clock_info *clk)
> +{
> +	struct scmi_iterator_ops ops = {
> +		.prepare_message = iter_clk_possible_parents_prepare_message,
> +		.update_state = iter_clk_possible_parents_update_state,
> +		.process_response = iter_clk_possible_parents_process_response,
> +	};
> +
> +	struct scmi_clk_ipriv ppriv = {
> +		.clk_id = clk_id,
> +		.clk = clk,
> +		.dev = ph->dev,
> +	};
> +	void *iter;
> +	int ret;
> +
> +	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_PARENTS,

SCMI_MAX_NUM_PARENTS is arbitrarily set to 8 elsewhere, but this is
out-of-spec, even though possibly reasonable.

Indeed, the spec for this protocol does not give you a way to know
upfront how many parents are possibly defined for the clock at hand, so
I suppose that's the reason why you have set the max number arbitrarily
to 8.

Even though this is true, we can really deduce the maximum number of
parents looking at the first reply: max_parents = remaining + returned.

It is a bit tricky but this can be done inside the .update_state callback
of the iterator, so that on the first invocation you can calculate
st->max_resources and allocate dynamically the needed resources.

You can find an example of something similar to this inside

drivers/firmware/arm_scmi/sensors.c::iter_intervals_update_state().

> +					    CLOCK_POSSIBLE_PARENTS_GET,
> +					    sizeof(struct scmi_msg_clock_possible_parents),
> +					    &ppriv);
> +	if (IS_ERR(iter))
> +		return PTR_ERR(iter);
> +
> +	ret = ph->hops->iter_response_run(iter);
> +
> +	return ret;
> +}
> +
>  static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>  				     u32 clk_id, struct scmi_clock_info *clk,
>  				     u32 version)
> @@ -211,6 +307,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>  			clk->rate_changed_notifications = true;
>  		if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
>  			clk->rate_change_requested_notifications = true;
> +		if (SUPPORTS_PARENT_CLOCK(attributes))
> +			scmi_clock_possible_parents(ph, clk_id, clk);
>  	}
>  
>  	return ret;
> @@ -228,12 +326,6 @@ static int rate_cmp_func(const void *_r1, const void *_r2)
>  		return 1;
>  }
>  
> -struct scmi_clk_ipriv {
> -	struct device *dev;
> -	u32 clk_id;
> -	struct scmi_clock_info *clk;
> -};
> -
>  static void iter_clk_describe_prepare_message(void *message,
>  					      const unsigned int desc_index,
>  					      const void *priv)
> @@ -457,6 +549,56 @@ scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
>  	return ret;
>  }
>  
> +static int
> +scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
> +		      u32 parent_id)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_clock_set_parent *cfg;
> +	struct clock_info *ci = ph->get_priv(ph);
> +	struct scmi_clock_info *clk = ci->clk + clk_id;
> +

Better to add a check here that the provided clk_id is within the
boundary of the existing clocks (i.e. clk_id < ci->num_clocks) before
dereferencing, given that the argument is provided by a caller.
(even though a kernel/driver caller I understand...)

> +	ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> +				      sizeof(*cfg), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	t->hdr.poll_completion = false;
> +
> +	cfg = t->tx.buf;
> +	cfg->id = cpu_to_le32(clk_id);
> +	cfg->parent_id = cpu_to_le32(clk->parents[parent_id]);
> +

Same here, parent_id, which is proivided by the caller, should have been
checked previously against clk->num_parents, bailing out on out of
boundary.

> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int
> +scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
> +		      u32 *parent_id)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +
> +	ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_GET,
> +				      sizeof(__le32), sizeof(u32), &t);
> +	if (ret)
> +		return ret;
> +
> +	put_unaligned_le32(clk_id, t->tx.buf);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret)
> +		*parent_id = get_unaligned_le32(t->rx.buf);
> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
>  static int
>  scmi_clock_config_set_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
>  			  enum clk_state state, u8 oem_type, u32 oem_val,
> @@ -647,6 +789,8 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
>  	.state_get = scmi_clock_state_get,
>  	.config_oem_get = scmi_clock_config_oem_get,
>  	.config_oem_set = scmi_clock_config_oem_set,
> +	.parent_set = scmi_clock_set_parent,
> +	.parent_get = scmi_clock_get_parent,
>  };
>  
>  static int scmi_clk_rate_notify(const struct scmi_protocol_handle *ph,
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 27bfa5a65b45..3175a9b4b8d8 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -16,6 +16,7 @@
>  #define SCMI_MAX_STR_SIZE		64
>  #define SCMI_SHORT_NAME_MAX_SIZE	16
>  #define SCMI_MAX_NUM_RATES		16
> +#define SCMI_MAX_NUM_PARENTS		8

Should be dynamically calculated as said above.

>  
>  /**
>   * struct scmi_revision_info - version information structure
> @@ -58,6 +59,8 @@ struct scmi_clock_info {
>  			u64 step_size;
>  		} range;
>  	};
> +	int num_parents;
> +	u32 parents[SCMI_MAX_NUM_PARENTS];

As said, you can dynamnically allocate this on protocol_init when
querying the parents.

>  };
>  
>  enum scmi_power_scale {
> @@ -104,6 +107,8 @@ struct scmi_clk_proto_ops {
>  			      bool atomic);
>  	int (*config_oem_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
>  			      u8 oem_type, u32 oem_val, bool atomic);
> +	int (*parent_get)(const struct scmi_protocol_handle *ph, u32 clk_id, u32 *parent_id);
> +	int (*parent_set)(const struct scmi_protocol_handle *ph, u32 clk_id, u32 parent_id);
>  };
>  

Please add related comments for this new operations in the block comment
above the scmi_clk_proto_ops.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel