[PATCH 0/7] SCMI V3.2 Misc updates

[PATCH 0/7] SCMI V3.2 Misc updates

[Cristian Marussi]

Hi,

another round of updates related to the last v3.2 SCMI spec, mostly around
Clock protocol.

Note that the series is based on sudeep/for-next/scmi/updates on top of

  7dd3d11f4dac ("clk: scmi: Add support for forbidden clock state controls")

and patch [1/7], which was included in the recently posted [1], it is
included also here just for ease of usage. (since needed also here ofc)

Having said that, [2/7] add a centralized support to the SCMI core to
handle v3.2 optional protocol version negotiation, so that at protocol
initialization time, mif the platform advertised version is newer than
supported by the kernel and protocol version negotiation is supported,
the SCMI core will attempt to negotiate an older protocol version.

Patches 3,4,5 adds the remaining last missing bits of Clock v3.2 protocol
and bumps the supported protocol version to 0x30000 (v3.2).

On top of these new SCMI additions, [6/7] reworks at first slightly how the
clk-scmi driver configures per-clock CLK ops, and then [7/7] adds support
for clock get/set duty cycle, as allowed by the last v3.2 spec additions,
but only if the related SCMI clk domain supports that specific clock
permissions.

Thanks,
Cristian

[1]: https://lore.kernel.org/linux-arm-kernel/20240212123233.1230090-3-cristian.marussi@arm.com/
---

Cristian Marussi (7):
  firmware: arm_scmi: Add a common helper to check if a message is
    supported
  firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION
  firmware: arm_scmi: Add Clock check for extended config support
  firmware: arm_scmi: Add standard Clock OEM definitions
  firmware: arm_scmi: Update supported Clock protocol version
  clk: scmi: Allocate CLK operations dynamically
  clk: scmi: Support get/set duty_cycle operations

 drivers/clk/clk-scmi.c                | 168 +++++++++++++++++---------
 drivers/firmware/arm_scmi/clock.c     |  67 +++++++---
 drivers/firmware/arm_scmi/driver.c    |  99 ++++++++++++++-
 drivers/firmware/arm_scmi/protocols.h |   5 +
 include/linux/scmi_protocol.h         |  15 ++-
 5 files changed, 270 insertions(+), 84 deletions(-)

-- 
2.43.0

[PATCH 0/7] SCMI V3.2 Misc updates

[Cristian Marussi]

Hi,

another round of updates related to the last v3.2 SCMI spec, mostly around
Clock protocol.

Note that the series is based on sudeep/for-next/scmi/updates on top of

  7dd3d11f4dac ("clk: scmi: Add support for forbidden clock state controls")

and patch [1/7], which was included in the recently posted [1], it is
included also here just for ease of usage. (since needed also here ofc)

Having said that, [2/7] add a centralized support to the SCMI core to
handle v3.2 optional protocol version negotiation, so that at protocol
initialization time, mif the platform advertised version is newer than
supported by the kernel and protocol version negotiation is supported,
the SCMI core will attempt to negotiate an older protocol version.

Patches 3,4,5 adds the remaining last missing bits of Clock v3.2 protocol
and bumps the supported protocol version to 0x30000 (v3.2).

On top of these new SCMI additions, [6/7] reworks at first slightly how the
clk-scmi driver configures per-clock CLK ops, and then [7/7] adds support
for clock get/set duty cycle, as allowed by the last v3.2 spec additions,
but only if the related SCMI clk domain supports that specific clock
permissions.

Thanks,
Cristian

[1]: https://lore.kernel.org/linux-arm-kernel/20240212123233.1230090-3-cristian.marussi@arm.com/
---

Cristian Marussi (7):
  firmware: arm_scmi: Add a common helper to check if a message is
    supported
  firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION
  firmware: arm_scmi: Add Clock check for extended config support
  firmware: arm_scmi: Add standard Clock OEM definitions
  firmware: arm_scmi: Update supported Clock protocol version
  clk: scmi: Allocate CLK operations dynamically
  clk: scmi: Support get/set duty_cycle operations

 drivers/clk/clk-scmi.c                | 168 +++++++++++++++++---------
 drivers/firmware/arm_scmi/clock.c     |  67 +++++++---
 drivers/firmware/arm_scmi/driver.c    |  99 ++++++++++++++-
 drivers/firmware/arm_scmi/protocols.h |   5 +
 include/linux/scmi_protocol.h         |  15 ++-
 5 files changed, 270 insertions(+), 84 deletions(-)

-- 
2.43.0


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

[PATCH 1/7] firmware: arm_scmi: Add a common helper to check if a message is supported

[Cristian Marussi]

A common helper is provided to check if a specific protocol message is
supported or not.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 34 +++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h |  4 ++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3ea64b22cf0d..4a64ad5c21ee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1754,10 +1754,44 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
 #endif
 }
 
+/**
+ * scmi_protocol_msg_check  - Check protocol message attributes
+ *
+ * @ph: A reference to the protocol handle.
+ * @message_id: The ID of the message to check.
+ * @attributes: A parameter to optionally return the retrieved message
+ *		attributes, in case of Success.
+ *
+ * An helper to check protocol message attributes for a specific protocol
+ * and message pair.
+ *
+ * Return: 0 on SUCCESS
+ */
+static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
+				   u32 message_id, u32 *attributes)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(message_id, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret && attributes)
+		*attributes = get_unaligned_le32(t->rx.buf);
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
 	.iter_response_init = scmi_iterator_init,
 	.iter_response_run = scmi_iterator_run,
+	.protocol_msg_check = scmi_protocol_msg_check,
 	.fastchannel_init = scmi_common_fastchannel_init,
 	.fastchannel_db_ring = scmi_common_fastchannel_db_ring,
 };
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index e683c26f24eb..26a3edd49fea 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -251,6 +251,8 @@ struct scmi_fc_info {
  *			provided in @ops.
  * @iter_response_run: A common helper to trigger the run of a previously
  *		       initialized iterator.
+ * @protocol_msg_check: A common helper to check is a specific protocol message
+ *			is supported.
  * @fastchannel_init: A common helper used to initialize FC descriptors by
  *		      gathering FC descriptions from the SCMI platform server.
  * @fastchannel_db_ring: A common helper to ring a FC doorbell.
@@ -264,6 +266,8 @@ struct scmi_proto_helpers_ops {
 				    unsigned int max_resources, u8 msg_id,
 				    size_t tx_size, void *priv);
 	int (*iter_response_run)(void *iter);
+	int (*protocol_msg_check)(const struct scmi_protocol_handle *ph,
+				  u32 message_id, u32 *attributes);
 	void (*fastchannel_init)(const struct scmi_protocol_handle *ph,
 				 u8 describe_id, u32 message_id,
 				 u32 valid_size, u32 domain,
-- 
2.43.0

[PATCH 1/7] firmware: arm_scmi: Add a common helper to check if a message is supported

[Cristian Marussi]

A common helper is provided to check if a specific protocol message is
supported or not.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 34 +++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h |  4 ++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3ea64b22cf0d..4a64ad5c21ee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1754,10 +1754,44 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
 #endif
 }
 
+/**
+ * scmi_protocol_msg_check  - Check protocol message attributes
+ *
+ * @ph: A reference to the protocol handle.
+ * @message_id: The ID of the message to check.
+ * @attributes: A parameter to optionally return the retrieved message
+ *		attributes, in case of Success.
+ *
+ * An helper to check protocol message attributes for a specific protocol
+ * and message pair.
+ *
+ * Return: 0 on SUCCESS
+ */
+static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
+				   u32 message_id, u32 *attributes)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(message_id, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret && attributes)
+		*attributes = get_unaligned_le32(t->rx.buf);
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
 	.iter_response_init = scmi_iterator_init,
 	.iter_response_run = scmi_iterator_run,
+	.protocol_msg_check = scmi_protocol_msg_check,
 	.fastchannel_init = scmi_common_fastchannel_init,
 	.fastchannel_db_ring = scmi_common_fastchannel_db_ring,
 };
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index e683c26f24eb..26a3edd49fea 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -251,6 +251,8 @@ struct scmi_fc_info {
  *			provided in @ops.
  * @iter_response_run: A common helper to trigger the run of a previously
  *		       initialized iterator.
+ * @protocol_msg_check: A common helper to check is a specific protocol message
+ *			is supported.
  * @fastchannel_init: A common helper used to initialize FC descriptors by
  *		      gathering FC descriptions from the SCMI platform server.
  * @fastchannel_db_ring: A common helper to ring a FC doorbell.
@@ -264,6 +266,8 @@ struct scmi_proto_helpers_ops {
 				    unsigned int max_resources, u8 msg_id,
 				    size_t tx_size, void *priv);
 	int (*iter_response_run)(void *iter);
+	int (*protocol_msg_check)(const struct scmi_protocol_handle *ph,
+				  u32 message_id, u32 *attributes);
 	void (*fastchannel_init)(const struct scmi_protocol_handle *ph,
 				 u8 describe_id, u32 message_id,
 				 u32 valid_size, u32 domain,
-- 
2.43.0


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

[PATCH 2/7] firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION

[Cristian Marussi]

Freshly introduced NEGOTIATE_PROTOCOL_VERSION allows the agent to ascertain
upfront if a specific, usually older, protocol version is supported by the
platform.

It is used by the agent in case the platform has advertised the support of
a newer protocol version than the latest version supported by the agent,
since backward compatibility cannot be automatically assumed.

Emit a warning about possible incompatibility when negotiation was not
possible or just print the successfully negotiated protocol.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 65 ++++++++++++++++++++++++---
 drivers/firmware/arm_scmi/protocols.h |  1 +
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 4a64ad5c21ee..34d77802c990 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -86,6 +86,12 @@ struct scmi_xfers_info {
  * @users: A refcount to track effective users of this protocol.
  * @priv: Reference for optional protocol private data.
  * @version: Protocol version supported by the platform as detected at runtime.
+ * @negotiated_version: When the platform supports a newer protocol version,
+ *			the agent will try to negotiate with the platform the
+ *			usage of the newest version known to it, since
+ *			backward compatibility is NOT automatically assured.
+ *			This field is NON-zero when a successful negotiation
+ *			has completed.
  * @ph: An embedded protocol handle that will be passed down to protocol
  *	initialization code to identify this instance.
  *
@@ -99,6 +105,7 @@ struct scmi_protocol_instance {
 	refcount_t			users;
 	void				*priv;
 	unsigned int			version;
+	unsigned int			negotiated_version;
 	struct scmi_protocol_handle	ph;
 };
 
@@ -1815,6 +1822,44 @@ scmi_revision_area_get(const struct scmi_protocol_handle *ph)
 	return pi->handle->version;
 }
 
+/**
+ * scmi_protocol_version_negotiate  - Negotiate protocol version
+ *
+ * @ph: A reference to the protocol handle.
+ *
+ * An helper to negotiate a protocol version different from the latest
+ * advertised as supported from the platform: on Success backward
+ * compatibility is assured by the platform.
+ *
+ * Return: 0 on Success
+ */
+static int scmi_protocol_version_negotiate(struct scmi_protocol_handle *ph)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_protocol_instance *pi = ph_to_pi(ph);
+
+	/* At first check if NEGOTIATE_PROTOCOL_VERSION is supported ... */
+	ret = scmi_protocol_msg_check(ph, NEGOTIATE_PROTOCOL_VERSION, NULL);
+	if (ret)
+		return ret;
+
+	/* ... then attempt protocol version negotiation */
+	ret = xfer_get_init(ph, NEGOTIATE_PROTOCOL_VERSION,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(pi->proto->supported_version, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret)
+		pi->negotiated_version = pi->proto->supported_version;
+
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 /**
  * scmi_alloc_init_protocol_instance  - Allocate and initialize a protocol
  * instance descriptor.
@@ -1887,11 +1932,21 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	devres_close_group(handle->dev, pi->gid);
 	dev_dbg(handle->dev, "Initialized protocol: 0x%X\n", pi->proto->id);
 
-	if (pi->version > proto->supported_version)
-		dev_warn(handle->dev,
-			 "Detected UNSUPPORTED higher version 0x%X for protocol 0x%X."
-			 "Backward compatibility is NOT assured.\n",
-			 pi->version, pi->proto->id);
+	if (pi->version > proto->supported_version) {
+		ret = scmi_protocol_version_negotiate(&pi->ph);
+		if (!ret) {
+			dev_info(handle->dev,
+				 "Protocol 0x%X successfully negotiated version 0x%X\n",
+				 proto->id, pi->negotiated_version);
+		} else {
+			dev_warn(handle->dev,
+				 "Detected UNSUPPORTED higher version 0x%X for protocol 0x%X.\n",
+				 pi->version, pi->proto->id);
+			dev_warn(handle->dev,
+				 "Trying version 0x%X. Backward compatibility is NOT assured.\n",
+				 pi->proto->supported_version);
+		}
+	}
 
 	return pi;
 
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 26a3edd49fea..693019fff0f6 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -33,6 +33,7 @@ enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
 	PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
+	NEGOTIATE_PROTOCOL_VERSION = 0x10,
 };
 
 /**
-- 
2.43.0


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

[PATCH 2/7] firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION

[Cristian Marussi]

Freshly introduced NEGOTIATE_PROTOCOL_VERSION allows the agent to ascertain
upfront if a specific, usually older, protocol version is supported by the
platform.

It is used by the agent in case the platform has advertised the support of
a newer protocol version than the latest version supported by the agent,
since backward compatibility cannot be automatically assumed.

Emit a warning about possible incompatibility when negotiation was not
possible or just print the successfully negotiated protocol.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 65 ++++++++++++++++++++++++---
 drivers/firmware/arm_scmi/protocols.h |  1 +
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 4a64ad5c21ee..34d77802c990 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -86,6 +86,12 @@ struct scmi_xfers_info {
  * @users: A refcount to track effective users of this protocol.
  * @priv: Reference for optional protocol private data.
  * @version: Protocol version supported by the platform as detected at runtime.
+ * @negotiated_version: When the platform supports a newer protocol version,
+ *			the agent will try to negotiate with the platform the
+ *			usage of the newest version known to it, since
+ *			backward compatibility is NOT automatically assured.
+ *			This field is NON-zero when a successful negotiation
+ *			has completed.
  * @ph: An embedded protocol handle that will be passed down to protocol
  *	initialization code to identify this instance.
  *
@@ -99,6 +105,7 @@ struct scmi_protocol_instance {
 	refcount_t			users;
 	void				*priv;
 	unsigned int			version;
+	unsigned int			negotiated_version;
 	struct scmi_protocol_handle	ph;
 };
 
@@ -1815,6 +1822,44 @@ scmi_revision_area_get(const struct scmi_protocol_handle *ph)
 	return pi->handle->version;
 }
 
+/**
+ * scmi_protocol_version_negotiate  - Negotiate protocol version
+ *
+ * @ph: A reference to the protocol handle.
+ *
+ * An helper to negotiate a protocol version different from the latest
+ * advertised as supported from the platform: on Success backward
+ * compatibility is assured by the platform.
+ *
+ * Return: 0 on Success
+ */
+static int scmi_protocol_version_negotiate(struct scmi_protocol_handle *ph)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_protocol_instance *pi = ph_to_pi(ph);
+
+	/* At first check if NEGOTIATE_PROTOCOL_VERSION is supported ... */
+	ret = scmi_protocol_msg_check(ph, NEGOTIATE_PROTOCOL_VERSION, NULL);
+	if (ret)
+		return ret;
+
+	/* ... then attempt protocol version negotiation */
+	ret = xfer_get_init(ph, NEGOTIATE_PROTOCOL_VERSION,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(pi->proto->supported_version, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret)
+		pi->negotiated_version = pi->proto->supported_version;
+
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 /**
  * scmi_alloc_init_protocol_instance  - Allocate and initialize a protocol
  * instance descriptor.
@@ -1887,11 +1932,21 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	devres_close_group(handle->dev, pi->gid);
 	dev_dbg(handle->dev, "Initialized protocol: 0x%X\n", pi->proto->id);
 
-	if (pi->version > proto->supported_version)
-		dev_warn(handle->dev,
-			 "Detected UNSUPPORTED higher version 0x%X for protocol 0x%X."
-			 "Backward compatibility is NOT assured.\n",
-			 pi->version, pi->proto->id);
+	if (pi->version > proto->supported_version) {
+		ret = scmi_protocol_version_negotiate(&pi->ph);
+		if (!ret) {
+			dev_info(handle->dev,
+				 "Protocol 0x%X successfully negotiated version 0x%X\n",
+				 proto->id, pi->negotiated_version);
+		} else {
+			dev_warn(handle->dev,
+				 "Detected UNSUPPORTED higher version 0x%X for protocol 0x%X.\n",
+				 pi->version, pi->proto->id);
+			dev_warn(handle->dev,
+				 "Trying version 0x%X. Backward compatibility is NOT assured.\n",
+				 pi->proto->supported_version);
+		}
+	}
 
 	return pi;
 
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 26a3edd49fea..693019fff0f6 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -33,6 +33,7 @@ enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
 	PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
+	NEGOTIATE_PROTOCOL_VERSION = 0x10,
 };
 
 /**
-- 
2.43.0

[PATCH 3/7] firmware: arm_scmi: Add Clock check for extended config support

[Cristian Marussi]

SCMI v3.2 added support to set/get Clock custom OEM types; such support is
conditionally present, though, depending on an extended config attribute
bit possibly advertised by the platform server on a per-domain base.

Add a check to verify if OEM types are supported before allowing any kind
of OEM-specific get/set operation and also a check around all Clock v3.2
features.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
base-commit: 7dd3d11f4dac301e958f00e18db4901a35cefc70
---
 drivers/firmware/arm_scmi/clock.c | 33 +++++++++++++++++++++++++------
 include/linux/scmi_protocol.h     |  1 +
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 959e48aba1b5..9d80ad95b467 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -54,6 +54,7 @@ struct scmi_msg_resp_clock_attributes {
 #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))
+#define SUPPORTS_EXTENDED_CONFIG(x)		((x) & BIT(27))
 #define SUPPORTS_GET_PERMISSIONS(x)		((x) & BIT(1))
 	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 	__le32 clock_enable_latency;
@@ -372,10 +373,14 @@ 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);
-		if (SUPPORTS_GET_PERMISSIONS(attributes))
-			scmi_clock_get_permissions(ph, clk_id, clk);
+		if (PROTOCOL_REV_MAJOR(version) >= 0x3) {
+			if (SUPPORTS_PARENT_CLOCK(attributes))
+				scmi_clock_possible_parents(ph, clk_id, clk);
+			if (SUPPORTS_GET_PERMISSIONS(attributes))
+				scmi_clock_get_permissions(ph, clk_id, clk);
+			if (SUPPORTS_EXTENDED_CONFIG(attributes))
+				clk->extended_config = true;
+		}
 	}
 
 	return ret;
@@ -684,7 +689,7 @@ scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
 	return ret;
 }
 
-/* For SCMI clock v2.1 and onwards */
+/* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 enum clk_state state, u8 oem_type, u32 oem_val,
@@ -757,7 +762,7 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
 				    NULL_OEM_TYPE, 0, atomic);
 }
 
-/* For SCMI clock v2.1 and onwards */
+/* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 u8 oem_type, u32 *attributes, bool *enabled,
@@ -844,6 +849,14 @@ static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
 				     bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk;
+
+	clk = scmi_clock_domain_lookup(ci, clk_id);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	if (!clk->extended_config)
+		return -EOPNOTSUPP;
 
 	return ci->clock_config_set(ph, clk_id, CLK_STATE_UNCHANGED,
 				    oem_type, oem_val, atomic);
@@ -854,6 +867,14 @@ static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
 				     u32 *attributes, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk;
+
+	clk = scmi_clock_domain_lookup(ci, clk_id);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	if (!clk->extended_config)
+		return -EOPNOTSUPP;
 
 	return ci->clock_config_get(ph, clk_id, oem_type, attributes,
 				    NULL, oem_val, atomic);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0cc40af5519a..caeca3f51be2 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -50,6 +50,7 @@ struct scmi_clock_info {
 	bool state_ctrl_forbidden;
 	bool rate_ctrl_forbidden;
 	bool parent_ctrl_forbidden;
+	bool extended_config;
 	union {
 		struct {
 			int num_rates;
-- 
2.43.0


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

[PATCH 3/7] firmware: arm_scmi: Add Clock check for extended config support

[Cristian Marussi]

SCMI v3.2 added support to set/get Clock custom OEM types; such support is
conditionally present, though, depending on an extended config attribute
bit possibly advertised by the platform server on a per-domain base.

Add a check to verify if OEM types are supported before allowing any kind
of OEM-specific get/set operation and also a check around all Clock v3.2
features.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
base-commit: 7dd3d11f4dac301e958f00e18db4901a35cefc70
---
 drivers/firmware/arm_scmi/clock.c | 33 +++++++++++++++++++++++++------
 include/linux/scmi_protocol.h     |  1 +
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 959e48aba1b5..9d80ad95b467 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -54,6 +54,7 @@ struct scmi_msg_resp_clock_attributes {
 #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))
+#define SUPPORTS_EXTENDED_CONFIG(x)		((x) & BIT(27))
 #define SUPPORTS_GET_PERMISSIONS(x)		((x) & BIT(1))
 	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 	__le32 clock_enable_latency;
@@ -372,10 +373,14 @@ 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);
-		if (SUPPORTS_GET_PERMISSIONS(attributes))
-			scmi_clock_get_permissions(ph, clk_id, clk);
+		if (PROTOCOL_REV_MAJOR(version) >= 0x3) {
+			if (SUPPORTS_PARENT_CLOCK(attributes))
+				scmi_clock_possible_parents(ph, clk_id, clk);
+			if (SUPPORTS_GET_PERMISSIONS(attributes))
+				scmi_clock_get_permissions(ph, clk_id, clk);
+			if (SUPPORTS_EXTENDED_CONFIG(attributes))
+				clk->extended_config = true;
+		}
 	}
 
 	return ret;
@@ -684,7 +689,7 @@ scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
 	return ret;
 }
 
-/* For SCMI clock v2.1 and onwards */
+/* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 enum clk_state state, u8 oem_type, u32 oem_val,
@@ -757,7 +762,7 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
 				    NULL_OEM_TYPE, 0, atomic);
 }
 
-/* For SCMI clock v2.1 and onwards */
+/* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 u8 oem_type, u32 *attributes, bool *enabled,
@@ -844,6 +849,14 @@ static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
 				     bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk;
+
+	clk = scmi_clock_domain_lookup(ci, clk_id);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	if (!clk->extended_config)
+		return -EOPNOTSUPP;
 
 	return ci->clock_config_set(ph, clk_id, CLK_STATE_UNCHANGED,
 				    oem_type, oem_val, atomic);
@@ -854,6 +867,14 @@ static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
 				     u32 *attributes, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk;
+
+	clk = scmi_clock_domain_lookup(ci, clk_id);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	if (!clk->extended_config)
+		return -EOPNOTSUPP;
 
 	return ci->clock_config_get(ph, clk_id, oem_type, attributes,
 				    NULL, oem_val, atomic);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0cc40af5519a..caeca3f51be2 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -50,6 +50,7 @@ struct scmi_clock_info {
 	bool state_ctrl_forbidden;
 	bool rate_ctrl_forbidden;
 	bool parent_ctrl_forbidden;
+	bool extended_config;
 	union {
 		struct {
 			int num_rates;
-- 
2.43.0

[PATCH 4/7] firmware: arm_scmi: Add standard Clock OEM definitions

[Cristian Marussi]

Add a common enum to define the standard Clock OEM types defined by the
SCMI specification, so as to enable the configuration of such extended
configuration properties with the existent clock protocol operations.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 32 ++++++++++++++++++-------------
 include/linux/scmi_protocol.h     | 14 +++++++++++---
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 9d80ad95b467..4e7f072142b9 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -163,10 +163,12 @@ struct clock_info {
 	struct scmi_clock_info *clk;
 	int (*clock_config_set)(const struct scmi_protocol_handle *ph,
 				u32 clk_id, enum clk_state state,
-				u8 oem_type, u32 oem_val, bool atomic);
+				enum scmi_clock_oem_config oem_type,
+				u32 oem_val, bool atomic);
 	int (*clock_config_get)(const struct scmi_protocol_handle *ph,
-				u32 clk_id, u8 oem_type, u32 *attributes,
-				bool *enabled, u32 *oem_val, bool atomic);
+				u32 clk_id, enum scmi_clock_oem_config oem_type,
+				u32 *attributes, bool *enabled, u32 *oem_val,
+				bool atomic);
 };
 
 static enum scmi_clock_protocol_cmd evt_2_cmd[] = {
@@ -602,7 +604,8 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
-		      enum clk_state state, u8 __unused0, u32 __unused1,
+		      enum clk_state state,
+		      enum scmi_clock_oem_config __unused0, u32 __unused1,
 		      bool atomic)
 {
 	int ret;
@@ -692,7 +695,8 @@ scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
 /* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 enum clk_state state, u8 oem_type, u32 oem_val,
+			 enum clk_state state,
+			 enum scmi_clock_oem_config oem_type, u32 oem_val,
 			 bool atomic)
 {
 	int ret;
@@ -765,8 +769,8 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
 /* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 u8 oem_type, u32 *attributes, bool *enabled,
-			 u32 *oem_val, bool atomic)
+			 enum scmi_clock_oem_config oem_type, u32 *attributes,
+			 bool *enabled, u32 *oem_val, bool atomic)
 {
 	int ret;
 	u32 flags;
@@ -807,8 +811,8 @@ scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 
 static int
 scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-		      u8 oem_type, u32 *attributes, bool *enabled,
-		      u32 *oem_val, bool atomic)
+		      enum scmi_clock_oem_config oem_type, u32 *attributes,
+		      bool *enabled, u32 *oem_val, bool atomic)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -845,8 +849,9 @@ static int scmi_clock_state_get(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
-				     u32 clk_id, u8 oem_type, u32 oem_val,
-				     bool atomic)
+				     u32 clk_id,
+				     enum scmi_clock_oem_config oem_type,
+				     u32 oem_val, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
 	struct scmi_clock_info *clk;
@@ -863,8 +868,9 @@ static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
-				     u32 clk_id, u8 oem_type, u32 *oem_val,
-				     u32 *attributes, bool atomic)
+				     u32 clk_id,
+				     enum scmi_clock_oem_config oem_type,
+				     u32 *oem_val, u32 *attributes, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
 	struct scmi_clock_info *clk;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index caeca3f51be2..24e8de581744 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -76,6 +76,13 @@ struct scmi_handle;
 struct scmi_device;
 struct scmi_protocol_handle;
 
+enum scmi_clock_oem_config {
+	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
+	SCMI_CLOCK_CFG_PHASE,
+	SCMI_CLOCK_CFG_OEM_START = 0x80,
+	SCMI_CLOCK_CFG_OEM_END = 0xFF,
+};
+
 /**
  * struct scmi_clk_proto_ops - represents the various operations provided
  *	by SCMI Clock Protocol
@@ -108,10 +115,11 @@ struct scmi_clk_proto_ops {
 	int (*state_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 bool *enabled, bool atomic);
 	int (*config_oem_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      u8 oem_type, u32 *oem_val, u32 *attributes,
-			      bool atomic);
+			      enum scmi_clock_oem_config oem_type,
+			      u32 *oem_val, u32 *attributes, bool atomic);
 	int (*config_oem_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      u8 oem_type, u32 oem_val, bool atomic);
+			      enum scmi_clock_oem_config 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);
 };
-- 
2.43.0


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

[PATCH 4/7] firmware: arm_scmi: Add standard Clock OEM definitions

[Cristian Marussi]

Add a common enum to define the standard Clock OEM types defined by the
SCMI specification, so as to enable the configuration of such extended
configuration properties with the existent clock protocol operations.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 32 ++++++++++++++++++-------------
 include/linux/scmi_protocol.h     | 14 +++++++++++---
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 9d80ad95b467..4e7f072142b9 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -163,10 +163,12 @@ struct clock_info {
 	struct scmi_clock_info *clk;
 	int (*clock_config_set)(const struct scmi_protocol_handle *ph,
 				u32 clk_id, enum clk_state state,
-				u8 oem_type, u32 oem_val, bool atomic);
+				enum scmi_clock_oem_config oem_type,
+				u32 oem_val, bool atomic);
 	int (*clock_config_get)(const struct scmi_protocol_handle *ph,
-				u32 clk_id, u8 oem_type, u32 *attributes,
-				bool *enabled, u32 *oem_val, bool atomic);
+				u32 clk_id, enum scmi_clock_oem_config oem_type,
+				u32 *attributes, bool *enabled, u32 *oem_val,
+				bool atomic);
 };
 
 static enum scmi_clock_protocol_cmd evt_2_cmd[] = {
@@ -602,7 +604,8 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
-		      enum clk_state state, u8 __unused0, u32 __unused1,
+		      enum clk_state state,
+		      enum scmi_clock_oem_config __unused0, u32 __unused1,
 		      bool atomic)
 {
 	int ret;
@@ -692,7 +695,8 @@ scmi_clock_get_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
 /* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 enum clk_state state, u8 oem_type, u32 oem_val,
+			 enum clk_state state,
+			 enum scmi_clock_oem_config oem_type, u32 oem_val,
 			 bool atomic)
 {
 	int ret;
@@ -765,8 +769,8 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
 /* For SCMI clock v3.0 and onwards */
 static int
 scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 u8 oem_type, u32 *attributes, bool *enabled,
-			 u32 *oem_val, bool atomic)
+			 enum scmi_clock_oem_config oem_type, u32 *attributes,
+			 bool *enabled, u32 *oem_val, bool atomic)
 {
 	int ret;
 	u32 flags;
@@ -807,8 +811,8 @@ scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 
 static int
 scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-		      u8 oem_type, u32 *attributes, bool *enabled,
-		      u32 *oem_val, bool atomic)
+		      enum scmi_clock_oem_config oem_type, u32 *attributes,
+		      bool *enabled, u32 *oem_val, bool atomic)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -845,8 +849,9 @@ static int scmi_clock_state_get(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
-				     u32 clk_id, u8 oem_type, u32 oem_val,
-				     bool atomic)
+				     u32 clk_id,
+				     enum scmi_clock_oem_config oem_type,
+				     u32 oem_val, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
 	struct scmi_clock_info *clk;
@@ -863,8 +868,9 @@ static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
-				     u32 clk_id, u8 oem_type, u32 *oem_val,
-				     u32 *attributes, bool atomic)
+				     u32 clk_id,
+				     enum scmi_clock_oem_config oem_type,
+				     u32 *oem_val, u32 *attributes, bool atomic)
 {
 	struct clock_info *ci = ph->get_priv(ph);
 	struct scmi_clock_info *clk;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index caeca3f51be2..24e8de581744 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -76,6 +76,13 @@ struct scmi_handle;
 struct scmi_device;
 struct scmi_protocol_handle;
 
+enum scmi_clock_oem_config {
+	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
+	SCMI_CLOCK_CFG_PHASE,
+	SCMI_CLOCK_CFG_OEM_START = 0x80,
+	SCMI_CLOCK_CFG_OEM_END = 0xFF,
+};
+
 /**
  * struct scmi_clk_proto_ops - represents the various operations provided
  *	by SCMI Clock Protocol
@@ -108,10 +115,11 @@ struct scmi_clk_proto_ops {
 	int (*state_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			 bool *enabled, bool atomic);
 	int (*config_oem_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      u8 oem_type, u32 *oem_val, u32 *attributes,
-			      bool atomic);
+			      enum scmi_clock_oem_config oem_type,
+			      u32 *oem_val, u32 *attributes, bool atomic);
 	int (*config_oem_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      u8 oem_type, u32 oem_val, bool atomic);
+			      enum scmi_clock_oem_config 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);
 };
-- 
2.43.0

[PATCH 5/7] firmware: arm_scmi: Update supported Clock protocol version

[Cristian Marussi]

Update supported Clock protocol version to v3.2 (0x30000)

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4e7f072142b9..ca1729fd42cf 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -13,7 +13,7 @@
 #include "notify.h"
 
 /* Updated only after ALL the mandatory features for that version are merged */
-#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x20000
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x30000
 
 enum scmi_clock_protocol_cmd {
 	CLOCK_ATTRIBUTES = 0x3,
-- 
2.43.0


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

[PATCH 5/7] firmware: arm_scmi: Update supported Clock protocol version

[Cristian Marussi]

Update supported Clock protocol version to v3.2 (0x30000)

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4e7f072142b9..ca1729fd42cf 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -13,7 +13,7 @@
 #include "notify.h"
 
 /* Updated only after ALL the mandatory features for that version are merged */
-#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x20000
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x30000
 
 enum scmi_clock_protocol_cmd {
 	CLOCK_ATTRIBUTES = 0x3,
-- 
2.43.0

[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

[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


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

[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

[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


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

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

[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,

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

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>

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>

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

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

[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

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

Re: (subset) [PATCH 0/7] SCMI V3.2 Misc updates

[Sudeep Holla]

On Wed, 14 Feb 2024 18:29:59 +0000, Cristian Marussi wrote:
> another round of updates related to the last v3.2 SCMI spec, mostly around
> Clock protocol.
>
> Note that the series is based on sudeep/for-next/scmi/updates on top of
>
>   7dd3d11f4dac ("clk: scmi: Add support for forbidden clock state controls")
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/7] firmware: arm_scmi: Add a common helper to check if a message is supported
      https://git.kernel.org/sudeep.holla/c/637b6d6cae9c
[2/7] firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION
      https://git.kernel.org/sudeep.holla/c/8c80c42ad401
[3/7] firmware: arm_scmi: Add Clock check for extended config support
      https://git.kernel.org/sudeep.holla/c/e4ad2b0130ef
[4/7] firmware: arm_scmi: Add standard Clock OEM definitions
      https://git.kernel.org/sudeep.holla/c/62092c428fb5
[5/7] firmware: arm_scmi: Update supported Clock protocol version
      https://git.kernel.org/sudeep.holla/c/5e0d2fe70cb8

--
Regards,
Sudeep

Re: (subset) [PATCH 0/7] SCMI V3.2 Misc updates

[Sudeep Holla]

On Wed, 14 Feb 2024 18:29:59 +0000, Cristian Marussi wrote:
> another round of updates related to the last v3.2 SCMI spec, mostly around
> Clock protocol.
>
> Note that the series is based on sudeep/for-next/scmi/updates on top of
>
>   7dd3d11f4dac ("clk: scmi: Add support for forbidden clock state controls")
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/7] firmware: arm_scmi: Add a common helper to check if a message is supported
      https://git.kernel.org/sudeep.holla/c/637b6d6cae9c
[2/7] firmware: arm_scmi: Add support for v3.2 NEGOTIATE_PROTOCOL_VERSION
      https://git.kernel.org/sudeep.holla/c/8c80c42ad401
[3/7] firmware: arm_scmi: Add Clock check for extended config support
      https://git.kernel.org/sudeep.holla/c/e4ad2b0130ef
[4/7] firmware: arm_scmi: Add standard Clock OEM definitions
      https://git.kernel.org/sudeep.holla/c/62092c428fb5
[5/7] firmware: arm_scmi: Update supported Clock protocol version
      https://git.kernel.org/sudeep.holla/c/5e0d2fe70cb8

--
Regards,
Sudeep


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

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

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

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

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

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

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