[PATCH 0/5] Expose SCMI Transport properties

[PATCH 0/5] Expose SCMI Transport properties

[Cristian Marussi]

Hi,

SCMI transports are characterized by a number of properties: the values
assumed by some of them tightly depend on the choices taken at design
time and on the overall archiecture of the specific platform: things like
timeouts, maximum message size and number of in-flight messages are closely
tied to the architecture of the platform like number of SCMI agents on the
system, physical memory available to the SCMI server...so on and so forth.

Moreover, since the SCMI specification does not delve into the details
of specific transports, that are, indeed, outside the scope of the
specification itself, such characteristics are NOT even discoverable
at run-time on an SCMI platform.

Currently such properties are simple default values defined at build time,
but the increasing number and variety of platforms using SCMI with a wide
range of designs has increased the need to have a way to describe such
properties across all these platforms.

This series, at first removes a few ambiguities in how some of the current
built-in properties are interpreted, then lays out a way for the core to
propagate back to the transports any possible setting gathered at runtime
from devicetree and finally introduce a pair of new properties used to
describe such per-platform transport characteristics.

Based on v6.12-rc3.

Any feedback welcome.

Thanks,
Cristian

Cristian Marussi (5):
  firmware: arm_scmi: Account for SHMEM memory overhead
  firmware: arm_scmi: Calculate virtio PDU max size dynamically
  dt-bindings: firmware: arm,scmi: Introduce more transport properties
  firmware: arm_scmi: Use max_msg and max_msg_size devicetree properties
  firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

 .../bindings/firmware/arm,scmi.yaml           | 16 +++++++
 drivers/firmware/arm_scmi/common.h            | 11 ++++-
 drivers/firmware/arm_scmi/driver.c            | 42 +++++++++++--------
 drivers/firmware/arm_scmi/shmem.c             |  7 ++++
 .../firmware/arm_scmi/transports/mailbox.c    |  4 +-
 drivers/firmware/arm_scmi/transports/optee.c  |  2 +-
 drivers/firmware/arm_scmi/transports/smc.c    |  4 +-
 drivers/firmware/arm_scmi/transports/virtio.c | 15 +++----
 8 files changed, 72 insertions(+), 29 deletions(-)

-- 
2.46.1

[PATCH 1/5] firmware: arm_scmi: Account for SHMEM memory overhead

[Cristian Marussi]

Transports using shared memory have to consider the overhead due to the
layout area when determining the area effectively available for messages.

Till now, such definitions were ambiguos across the SCMI stack and the
overhead layout area was not considered at all.

Add proper checks in the shmem layer to validate the provided max_msg_size
against the effectively available memory area, less the layout.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Note that as a consequence of this fix the default max_msg_size is reduced
to 104 bytes for shmem-based transports, in order to fit into the most
common implementations where the whole shmem area is sized at 128,
including the 24 bytes of standard layout area.

This should have NO bad side effects, since the current maximum payload
size of any messages across any protocol (including all the known vendor
ones) is 76 bytes.
---
 drivers/firmware/arm_scmi/common.h             | 4 +++-
 drivers/firmware/arm_scmi/driver.c             | 1 +
 drivers/firmware/arm_scmi/shmem.c              | 7 +++++++
 drivers/firmware/arm_scmi/transports/mailbox.c | 4 +++-
 drivers/firmware/arm_scmi/transports/optee.c   | 2 +-
 drivers/firmware/arm_scmi/transports/smc.c     | 4 +++-
 6 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 6c2032d4f767..d867bcc6883b 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -165,6 +165,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
  *	 channel
  * @is_p2a: A flag to identify a channel as P2A (RX)
  * @rx_timeout_ms: The configured RX timeout in milliseconds.
+ * @max_msg_size: Maximum size of message payload.
  * @handle: Pointer to SCMI entity handle
  * @no_completion_irq: Flag to indicate that this channel has no completion
  *		       interrupt mechanism for synchronous commands.
@@ -177,6 +178,7 @@ struct scmi_chan_info {
 	struct device *dev;
 	bool is_p2a;
 	unsigned int rx_timeout_ms;
+	unsigned int max_msg_size;
 	struct scmi_handle *handle;
 	bool no_completion_irq;
 	void *transport_info;
@@ -224,7 +226,7 @@ struct scmi_transport_ops {
  * @max_msg: Maximum number of messages for a channel type (tx or rx) that can
  *	be pending simultaneously in the system. May be overridden by the
  *	get_max_msg op.
- * @max_msg_size: Maximum size of data per message that can be handled.
+ * @max_msg_size: Maximum size of data payload per message that can be handled.
  * @force_polling: Flag to force this whole transport to use SCMI core polling
  *		   mechanism instead of completion interrupts even if available.
  * @sync_cmds_completed_on_ret: Flag to indicate that the transport assures
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index dccd066e3ba8..015a4d52ae37 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2645,6 +2645,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
 
 	cinfo->is_p2a = !tx;
 	cinfo->rx_timeout_ms = info->desc->max_rx_timeout_ms;
+	cinfo->max_msg_size = info->desc->max_msg_size;
 
 	/* Create a unique name for this transport device */
 	snprintf(name, 32, "__scmi_transport_device_%s_%02X",
diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c
index e9f30ab671a8..11c347bff766 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -16,6 +16,8 @@
 
 #include "common.h"
 
+#define SCMI_SHMEM_LAYOUT_OVERHEAD	24
+
 /*
  * SCMI specification requires all parameters, message headers, return
  * arguments or any protocol data to be expressed in little endian
@@ -221,6 +223,11 @@ static void __iomem *shmem_setup_iomap(struct scmi_chan_info *cinfo,
 	}
 
 	size = resource_size(res);
+	if (cinfo->max_msg_size + SCMI_SHMEM_LAYOUT_OVERHEAD > size) {
+		dev_err(dev, "misconfigured SCMI shared memory\n");
+		return IOMEM_ERR_PTR(-ENOSPC);
+	}
+
 	addr = devm_ioremap(dev, res->start, size);
 	if (!addr) {
 		dev_err(dev, "failed to ioremap SCMI %s shared memory\n", desc);
diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index e7efa3376aae..4e0396250ad0 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -16,6 +16,8 @@
 
 #include "../common.h"
 
+#define SCMI_MAILBOX_MAX_MSG_SIZE	104
+
 /**
  * struct scmi_mailbox - Structure representing a SCMI mailbox transport
  *
@@ -371,7 +373,7 @@ static struct scmi_desc scmi_mailbox_desc = {
 	.ops = &scmi_mailbox_ops,
 	.max_rx_timeout_ms = 30, /* We may increase this if required */
 	.max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */
-	.max_msg_size = 128,
+	.max_msg_size = SCMI_MAILBOX_MAX_MSG_SIZE,
 };
 
 static const struct of_device_id scmi_of_match[] = {
diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
index 663272879edf..9c0bc2c4dbcd 100644
--- a/drivers/firmware/arm_scmi/transports/optee.c
+++ b/drivers/firmware/arm_scmi/transports/optee.c
@@ -17,7 +17,7 @@
 
 #include "../common.h"
 
-#define SCMI_OPTEE_MAX_MSG_SIZE		128
+#define SCMI_OPTEE_MAX_MSG_SIZE		104
 
 enum scmi_optee_pta_cmd {
 	/*
diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
index 2f0e981e7599..098bbd7e67b8 100644
--- a/drivers/firmware/arm_scmi/transports/smc.c
+++ b/drivers/firmware/arm_scmi/transports/smc.c
@@ -22,6 +22,8 @@
 
 #include "../common.h"
 
+#define SCMI_SMC_MAX_MSG_SIZE	104
+
 /*
  * The shmem address is split into 4K page and offset.
  * This is to make sure the parameters fit in 32bit arguments of the
@@ -282,7 +284,7 @@ static struct scmi_desc scmi_smc_desc = {
 	.ops = &scmi_smc_ops,
 	.max_rx_timeout_ms = 30,
 	.max_msg = 20,
-	.max_msg_size = 128,
+	.max_msg_size = SCMI_SMC_MAX_MSG_SIZE,
 	/*
 	 * Setting .sync_cmds_atomic_replies to true for SMC assumes that,
 	 * once the SMC instruction has completed successfully, the issued
-- 
2.46.1

[PATCH 2/5] firmware: arm_scmi: Calculate virtio PDU max size dynamically

[Cristian Marussi]

SCMI virtio transport maximum PDU size is currently hardcoded at build
time; this will not play well with the possibile retrieval of a different
size at run-time.

Make the virtio transport derive the maximum PDU size from the max_msg_size
provided by the SCMI core.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/transports/virtio.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
index d349766bc0b2..41aea33776a9 100644
--- a/drivers/firmware/arm_scmi/transports/virtio.c
+++ b/drivers/firmware/arm_scmi/transports/virtio.c
@@ -32,8 +32,8 @@
 
 #define VIRTIO_MAX_RX_TIMEOUT_MS	60000
 #define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */
-#define VIRTIO_SCMI_MAX_PDU_SIZE \
-	(VIRTIO_SCMI_MAX_MSG_SIZE + SCMI_MSG_MAX_PROT_OVERHEAD)
+#define VIRTIO_SCMI_MAX_PDU_SIZE(ci) \
+	((ci)->max_msg_size + SCMI_MSG_MAX_PROT_OVERHEAD)
 #define DESCRIPTORS_PER_TX_MSG 2
 
 /**
@@ -90,6 +90,7 @@ enum poll_states {
  * @input: SDU used for (delayed) responses and notifications
  * @list: List which scmi_vio_msg may be part of
  * @rx_len: Input SDU size in bytes, once input has been received
+ * @max_len: Maximumm allowed SDU size in bytes
  * @poll_idx: Last used index registered for polling purposes if this message
  *	      transaction reply was configured for polling.
  * @poll_status: Polling state for this message.
@@ -102,6 +103,7 @@ struct scmi_vio_msg {
 	struct scmi_msg_payld *input;
 	struct list_head list;
 	unsigned int rx_len;
+	unsigned int max_len;
 	unsigned int poll_idx;
 	enum poll_states poll_status;
 	/* Lock to protect access to poll_status */
@@ -234,7 +236,7 @@ static int scmi_vio_feed_vq_rx(struct scmi_vio_channel *vioch,
 	unsigned long flags;
 	struct device *dev = &vioch->vqueue->vdev->dev;
 
-	sg_init_one(&sg_in, msg->input, VIRTIO_SCMI_MAX_PDU_SIZE);
+	sg_init_one(&sg_in, msg->input, msg->max_len);
 
 	spin_lock_irqsave(&vioch->lock, flags);
 
@@ -439,9 +441,9 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		if (!msg)
 			return -ENOMEM;
 
+		msg->max_len = VIRTIO_SCMI_MAX_PDU_SIZE(cinfo);
 		if (tx) {
-			msg->request = devm_kzalloc(dev,
-						    VIRTIO_SCMI_MAX_PDU_SIZE,
+			msg->request = devm_kzalloc(dev, msg->max_len,
 						    GFP_KERNEL);
 			if (!msg->request)
 				return -ENOMEM;
@@ -449,8 +451,7 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 			refcount_set(&msg->users, 1);
 		}
 
-		msg->input = devm_kzalloc(dev, VIRTIO_SCMI_MAX_PDU_SIZE,
-					  GFP_KERNEL);
+		msg->input = devm_kzalloc(dev, msg->max_len, GFP_KERNEL);
 		if (!msg->input)
 			return -ENOMEM;
 
-- 
2.46.1

[PATCH 3/5] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Cristian Marussi]

Depending on specific hardware and firmware design choices, it may be
possible for different platforms to end up having different requirements
regarding the same transport characteristics.

Introduce max-msg-size and max-msg properties to describe such platform
specific transport constraints, since they cannot be discovered otherwise.

Cc: devicetree@vger.kernel.org
Cc: Rob Herring (Arm) <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 54d7d11bfed4..42852ed887f2 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -131,6 +131,22 @@ properties:
       be a non-zero value if set.
     minimum: 1
 
+  max-msg-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      An optional value, expressed in bytes, representing the maximum size
+      allowed for the payload of messages transmitted on this transport.
+      If set it is recommended to be greater or equal than the minimum size
+      required to support all the messages defined by the set of protocols
+      implemented on this platform.
+
+  max-msg:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      An optional value representing the maximum number of concurrent in-flight
+      messages allowed by this transport. If set, the value should be non-zero.
+    minimum: 1
+
   arm,smc-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.46.1

Re: [PATCH 3/5] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Rob Herring]

On Fri, Oct 18, 2024 at 09:06:00AM +0100, Cristian Marussi wrote:
> Depending on specific hardware and firmware design choices, it may be
> possible for different platforms to end up having different requirements
> regarding the same transport characteristics.
> 
> Introduce max-msg-size and max-msg properties to describe such platform
> specific transport constraints, since they cannot be discovered otherwise.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring (Arm) <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 54d7d11bfed4..42852ed887f2 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -131,6 +131,22 @@ properties:
>        be a non-zero value if set.
>      minimum: 1
>  
> +  max-msg-size:

Vendor prefix needed.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      An optional value, expressed in bytes, representing the maximum size
> +      allowed for the payload of messages transmitted on this transport.
> +      If set it is recommended to be greater or equal than the minimum size
> +      required to support all the messages defined by the set of protocols
> +      implemented on this platform.

Sounds kind of broken if less than the minimum...

> +
> +  max-msg:

Vendor prefix and could be a bit more specific what this is.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      An optional value representing the maximum number of concurrent in-flight
> +      messages allowed by this transport. If set, the value should be non-zero.
> +    minimum: 1
> +
>    arm,smc-id:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> -- 
> 2.46.1
> 

Re: [PATCH 3/5] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Cristian Marussi]

On Fri, Oct 18, 2024 at 08:33:40AM -0500, Rob Herring wrote:
> On Fri, Oct 18, 2024 at 09:06:00AM +0100, Cristian Marussi wrote:
> > Depending on specific hardware and firmware design choices, it may be
> > possible for different platforms to end up having different requirements
> > regarding the same transport characteristics.
> > 

Hi Rob,

thanks for having a look.

> > Introduce max-msg-size and max-msg properties to describe such platform
> > specific transport constraints, since they cannot be discovered otherwise.
> > 
> > Cc: devicetree@vger.kernel.org
> > Cc: Rob Herring (Arm) <robh@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 54d7d11bfed4..42852ed887f2 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -131,6 +131,22 @@ properties:
> >        be a non-zero value if set.
> >      minimum: 1
> >  
> > +  max-msg-size:
> 
> Vendor prefix needed.

Ok, but these are common properties like max-rx-timeout-ms and others in this
bindings so I did not expect to have to explicit set a vendor...well I
really dont know what's the rule here, so I went for similarity.

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      An optional value, expressed in bytes, representing the maximum size
> > +      allowed for the payload of messages transmitted on this transport.
> > +      If set it is recommended to be greater or equal than the minimum size
> > +      required to support all the messages defined by the set of protocols
> > +      implemented on this platform.
> 
> Sounds kind of broken if less than the minimum...
> 

Well, absolutely but the minimum payload really depends on how many
protocols the SCMI server in firmware supports on this platform...the
set of implemented messages (some are optional) AND the spec version
number...so I cannot state really a fixed safe minimum here...it was
just a polite warning for the user not to shot himself in the foot...
..but whoever missteps over this will see a lot of comms errors
indeed...I will drop the warning in V2

> > +
> > +  max-msg:
> 
> Vendor prefix and could be a bit more specific what this is.

Ok I will explain more in v2.

Thanks,
Cristian

Re: [PATCH 3/5] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Rob Herring]

On Fri, Oct 18, 2024 at 09:06:00AM +0100, Cristian Marussi wrote:
> Depending on specific hardware and firmware design choices, it may be
> possible for different platforms to end up having different requirements
> regarding the same transport characteristics.
> 
> Introduce max-msg-size and max-msg properties to describe such platform
> specific transport constraints, since they cannot be discovered otherwise.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring (Arm) <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Also, you missed Conor and that not the right address for Krzysztof. Use 
get_maintainers.pl or a tool that uses it (b4).

Also, no need to store maintainer addresses in the commit msg.

Rob

Re: [PATCH 3/5] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Cristian Marussi]

On Fri, Oct 18, 2024 at 08:37:25AM -0500, Rob Herring wrote:
> On Fri, Oct 18, 2024 at 09:06:00AM +0100, Cristian Marussi wrote:
> > Depending on specific hardware and firmware design choices, it may be
> > possible for different platforms to end up having different requirements
> > regarding the same transport characteristics.
> > 
> > Introduce max-msg-size and max-msg properties to describe such platform
> > specific transport constraints, since they cannot be discovered otherwise.
> > 
> > Cc: devicetree@vger.kernel.org
> > Cc: Rob Herring (Arm) <robh@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Also, you missed Conor and that not the right address for Krzysztof. Use 
> get_maintainers.pl or a tool that uses it (b4).
> 
> Also, no need to store maintainer addresses in the commit msg.

Ok, my bad I ahev not checked with the tool this time but just grabbed
from a recent commit log. I will fix in V2

Thanks,
Cristian

[PATCH 4/5] firmware: arm_scmi: Use max_msg and max_msg_size devicetree properties

[Cristian Marussi]

Override the default built-in max_msg and max_msg_size transport properties
when the corresponding properties were found to be described in the
devicetree.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 015a4d52ae37..09eb0713d91c 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3056,8 +3056,20 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 	if (ret && ret != -EINVAL)
 		dev_err(dev, "Malformed max-rx-timeout-ms DT property.\n");
 
-	dev_info(dev, "SCMI max-rx-timeout: %dms\n",
-		 trans->desc->max_rx_timeout_ms);
+	ret = of_property_read_u32(dev->of_node, "max-msg-size",
+				   &trans->desc->max_msg_size);
+	if (ret && ret != -EINVAL)
+		dev_err(dev, "Malformed max-msg-size DT property.\n");
+
+	ret = of_property_read_u32(dev->of_node, "max-msg",
+				   &trans->desc->max_msg);
+	if (ret && ret != -EINVAL)
+		dev_err(dev, "Malformed max-msg DT property.\n");
+
+	dev_info(dev,
+		 "SCMI max-rx-timeout: %dms / max-msg-size: %dbytes / max-msg: %d\n",
+		 trans->desc->max_rx_timeout_ms, trans->desc->max_msg_size,
+		 trans->desc->max_msg);
 
 	return trans->desc;
 }
-- 
2.46.1

[PATCH 5/5] firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

[Cristian Marussi]

Relocate the atomic_threshold field to scmi_desc and move the related code
to scmi_transport_setup.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h |  7 +++++++
 drivers/firmware/arm_scmi/driver.c | 25 +++++++++----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index d867bcc6883b..058d4b0e3de9 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -227,6 +227,12 @@ struct scmi_transport_ops {
  *	be pending simultaneously in the system. May be overridden by the
  *	get_max_msg op.
  * @max_msg_size: Maximum size of data payload per message that can be handled.
+ * @atomic_threshold: Optional system wide DT-configured threshold, expressed
+ *		      in microseconds, for atomic operations.
+ *		      Only SCMI synchronous commands reported by the platform
+ *		      to have an execution latency lesser-equal to the threshold
+ *		      should be considered for atomic mode operation: such
+ *		      decision is finally left up to the SCMI drivers.
  * @force_polling: Flag to force this whole transport to use SCMI core polling
  *		   mechanism instead of completion interrupts even if available.
  * @sync_cmds_completed_on_ret: Flag to indicate that the transport assures
@@ -245,6 +251,7 @@ struct scmi_desc {
 	int max_rx_timeout_ms;
 	int max_msg;
 	int max_msg_size;
+	unsigned int atomic_threshold;
 	const bool force_polling;
 	const bool sync_cmds_completed_on_ret;
 	const bool atomic_enabled;
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 09eb0713d91c..c9adf91643ed 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -149,12 +149,6 @@ struct scmi_debug_info {
  *		   base protocol
  * @active_protocols: IDR storing device_nodes for protocols actually defined
  *		      in the DT and confirmed as implemented by fw.
- * @atomic_threshold: Optional system wide DT-configured threshold, expressed
- *		      in microseconds, for atomic operations.
- *		      Only SCMI synchronous commands reported by the platform
- *		      to have an execution latency lesser-equal to the threshold
- *		      should be considered for atomic mode operation: such
- *		      decision is finally left up to the SCMI drivers.
  * @notify_priv: Pointer to private data structure specific to notifications.
  * @node: List head
  * @users: Number of users of this instance
@@ -180,7 +174,6 @@ struct scmi_info {
 	struct mutex protocols_mtx;
 	u8 *protocols_imp;
 	struct idr active_protocols;
-	unsigned int atomic_threshold;
 	void *notify_priv;
 	struct list_head node;
 	int users;
@@ -2445,7 +2438,7 @@ static bool scmi_is_transport_atomic(const struct scmi_handle *handle,
 	ret = info->desc->atomic_enabled &&
 		is_transport_polling_capable(info->desc);
 	if (ret && atomic_threshold)
-		*atomic_threshold = info->atomic_threshold;
+		*atomic_threshold = info->desc->atomic_threshold;
 
 	return ret;
 }
@@ -2959,7 +2952,7 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
 			   (char **)&dbg->name);
 
 	debugfs_create_u32("atomic_threshold_us", 0400, top_dentry,
-			   &info->atomic_threshold);
+			   (u32 *)&info->desc->atomic_threshold);
 
 	debugfs_create_str("type", 0400, trans, (char **)&dbg->type);
 
@@ -3071,6 +3064,13 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 		 trans->desc->max_rx_timeout_ms, trans->desc->max_msg_size,
 		 trans->desc->max_msg);
 
+	/* System wide atomic threshold for atomic ops .. if any */
+	if (!of_property_read_u32(dev->of_node, "atomic-threshold-us",
+				  &trans->desc->atomic_threshold))
+		dev_info(dev,
+			 "SCMI System wide atomic threshold set to %d us\n",
+			 trans->desc->atomic_threshold);
+
 	return trans->desc;
 }
 
@@ -3120,13 +3120,6 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->devm_protocol_acquire = scmi_devm_protocol_acquire;
 	handle->devm_protocol_get = scmi_devm_protocol_get;
 	handle->devm_protocol_put = scmi_devm_protocol_put;
-
-	/* System wide atomic threshold for atomic ops .. if any */
-	if (!of_property_read_u32(np, "atomic-threshold-us",
-				  &info->atomic_threshold))
-		dev_info(dev,
-			 "SCMI System wide atomic threshold set to %d us\n",
-			 info->atomic_threshold);
 	handle->is_transport_atomic = scmi_is_transport_atomic;
 
 	/* Setup all channels described in the DT at first */
-- 
2.46.1

Re: [PATCH 5/5] firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

[Dan Carpenter]

On Fri, Oct 18, 2024 at 09:06:02AM +0100, Cristian Marussi wrote:
> @@ -2959,7 +2952,7 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
>  			   (char **)&dbg->name);
>  
>  	debugfs_create_u32("atomic_threshold_us", 0400, top_dentry,
> -			   &info->atomic_threshold);
> +			   (u32 *)&info->desc->atomic_threshold);

This cast is unnecessary.

>  
>  	debugfs_create_str("type", 0400, trans, (char **)&dbg->type);
>  
> @@ -3071,6 +3064,13 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
>  		 trans->desc->max_rx_timeout_ms, trans->desc->max_msg_size,
>  		 trans->desc->max_msg);
>  
> +	/* System wide atomic threshold for atomic ops .. if any */
> +	if (!of_property_read_u32(dev->of_node, "atomic-threshold-us",
> +				  &trans->desc->atomic_threshold))
> +		dev_info(dev,
> +			 "SCMI System wide atomic threshold set to %d us\n",
                                                                   ^^
%u for unsigned int.

> +			 trans->desc->atomic_threshold);
> +
>  	return trans->desc;
>  }

regards,
dan carpenter

Re: [PATCH 5/5] firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

[Cristian Marussi]

On Wed, Oct 23, 2024 at 04:20:53PM +0300, Dan Carpenter wrote:
> On Fri, Oct 18, 2024 at 09:06:02AM +0100, Cristian Marussi wrote:

Hi Dan,

thanks for having a look.

> > @@ -2959,7 +2952,7 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
> >  			   (char **)&dbg->name);
> >  
> >  	debugfs_create_u32("atomic_threshold_us", 0400, top_dentry,
> > -			   &info->atomic_threshold);
> > +			   (u32 *)&info->desc->atomic_threshold);
> 
> This cast is unnecessary.

I was indeed wondering why I added that....then I remember something
about debugfs_create....without that (u32 *):

drivers/firmware/arm_scmi/driver.c: In function ‘scmi_debugfs_common_setup’:
drivers/firmware/arm_scmi/driver.c:2988:28: warning: passing argument 4 of ‘debugfs_create_u32’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
                             &info->desc->atomic_threshold);
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

since the enclosing struct ->desc is const AND debugfs_create_u32 is NOT
smart enough to expect a const when the property is R_ONLY...unless I am
missing something.

> 
> >  
> >  	debugfs_create_str("type", 0400, trans, (char **)&dbg->type);
> >  
> > @@ -3071,6 +3064,13 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
> >  		 trans->desc->max_rx_timeout_ms, trans->desc->max_msg_size,
> >  		 trans->desc->max_msg);
> >  
> > +	/* System wide atomic threshold for atomic ops .. if any */
> > +	if (!of_property_read_u32(dev->of_node, "atomic-threshold-us",
> > +				  &trans->desc->atomic_threshold))
> > +		dev_info(dev,
> > +			 "SCMI System wide atomic threshold set to %d us\n",
>                                                                    ^^
> %u for unsigned int.
>

I will fix.

Thanks,
Cristian

Re: [PATCH 5/5] firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

[Dan Carpenter]

On Fri, Oct 25, 2024 at 03:35:57PM +0100, Cristian Marussi wrote:
> On Wed, Oct 23, 2024 at 04:20:53PM +0300, Dan Carpenter wrote:
> > On Fri, Oct 18, 2024 at 09:06:02AM +0100, Cristian Marussi wrote:
> 
> Hi Dan,
> 
> thanks for having a look.
> 
> > > @@ -2959,7 +2952,7 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
> > >  			   (char **)&dbg->name);
> > >  
> > >  	debugfs_create_u32("atomic_threshold_us", 0400, top_dentry,
> > > -			   &info->atomic_threshold);
> > > +			   (u32 *)&info->desc->atomic_threshold);
> > 
> > This cast is unnecessary.
> 
> I was indeed wondering why I added that....then I remember something
> about debugfs_create....without that (u32 *):
> 
> drivers/firmware/arm_scmi/driver.c: In function ‘scmi_debugfs_common_setup’:
> drivers/firmware/arm_scmi/driver.c:2988:28: warning: passing argument 4 of ‘debugfs_create_u32’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>                              &info->desc->atomic_threshold);
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> since the enclosing struct ->desc is const AND debugfs_create_u32 is NOT
> smart enough to expect a const when the property is R_ONLY...unless I am
> missing something.
> 

Ah, I missed the const.  Sorry about that.

regards,
dan carpenter