[PATCH v1 0/6] Add SCMI transport descriptors

[PATCH v1 0/6] Add SCMI transport descriptors

[Cristian Marussi]

Hi,

this small series is an extended version of this recent, already reviewed,
series [1] posted by Peng to add a new arm,scmi property to describe some
platform-specific SCMI timeout constraints.

On top of that, this adds 2 more properties to describe a couple more
platform-specific transport characteristics.

To minimize conflicts, the whole series is based on top of another recent
series, which represents a rework of the core SCMI stack to split SCMI
transports as standalone drivers. [2]

Thanks,
Cristian

[1]: https://lore.kernel.org/linux-arm-kernel/20240709140957.3171255-1-peng.fan@oss.nxp.com/
[2]: https://lore.kernel.org/linux-arm-kernel/20240730133318.1573765-1-cristian.marussi@arm.com/T/#t

---

Cristian Marussi (5):
  firmware: arm_scmi: Remove const from transport descriptors
  firmware: arm_scmi: Use max-rx-timeout-ms from devicetree
  dt-bindings: firmware: arm,scmi: Introduce more transport properties
  firmware: arm_scmi: Use max_msg and max_msg_size from devicetree
  firmware: arm_scmi: Relocate atomic_threshold to scmi_desc

Peng Fan (1):
  dt-bindings: firmware: arm,scmi: Introduce property max-rx-timeout-ms

 .../bindings/firmware/arm,scmi.yaml           | 22 +++++++++
 drivers/firmware/arm_scmi/common.h            |  9 +++-
 drivers/firmware/arm_scmi/driver.c            | 46 ++++++++++++-------
 .../arm_scmi/scmi_transport_mailbox.c         |  2 +-
 .../firmware/arm_scmi/scmi_transport_optee.c  |  2 +-
 .../firmware/arm_scmi/scmi_transport_smc.c    |  2 +-
 .../firmware/arm_scmi/scmi_transport_virtio.c |  2 +-
 7 files changed, 64 insertions(+), 21 deletions(-)

-- 
2.45.2

[PATCH v1 1/6] firmware: arm_scmi: Remove const from transport descriptors

[Cristian Marussi]

The descriptor structure scmi_desc contains a variety of fields related to
the transport functionalities and it is defined by the transport drivers
themselves; such elements, though, serve varied purposes and have different
lifetime.

In particular, while there are some of those elements that provide a
description of transport features that are supposed to be immutable, on
the other side there are present also some other characteristics that are
instead supposed to be configurable on a per-platform base since they
represent configuration features tied to the specific hardware/firmware
system.

The immutable fields are already qualified as const on the their own; get
rid of the structure-level const qualifier which is not needed, so as to
enable possible runtime customization of the mutable configuration
features.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h                 | 2 +-
 drivers/firmware/arm_scmi/scmi_transport_mailbox.c | 2 +-
 drivers/firmware/arm_scmi/scmi_transport_optee.c   | 2 +-
 drivers/firmware/arm_scmi/scmi_transport_smc.c     | 2 +-
 drivers/firmware/arm_scmi/scmi_transport_virtio.c  | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 77721cb4ac34..5261dc9b5f96 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -381,7 +381,7 @@ struct scmi_transport_core_operations {
  */
 struct scmi_transport {
 	struct device *supplier;
-	const struct scmi_desc *desc;
+	struct scmi_desc *desc;
 	struct scmi_transport_core_operations **core_ops;
 };
 
diff --git a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
index 75483b6778ee..dc5ca894d5eb 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
@@ -355,7 +355,7 @@ static const struct scmi_transport_ops scmi_mailbox_ops = {
 	.poll_done = mailbox_poll_done,
 };
 
-static const struct scmi_desc scmi_mailbox_desc = {
+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 */
diff --git a/drivers/firmware/arm_scmi/scmi_transport_optee.c b/drivers/firmware/arm_scmi/scmi_transport_optee.c
index 974128d73f2d..08911f40d1ff 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_optee.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_optee.c
@@ -510,7 +510,7 @@ static int scmi_optee_ctx_match(struct tee_ioctl_version_data *ver, const void *
 	return ver->impl_id == TEE_IMPL_ID_OPTEE;
 }
 
-static const struct scmi_desc scmi_optee_desc = {
+static struct scmi_desc scmi_optee_desc = {
 	.ops = &scmi_optee_ops,
 	.max_rx_timeout_ms = 30,
 	.max_msg = 20,
diff --git a/drivers/firmware/arm_scmi/scmi_transport_smc.c b/drivers/firmware/arm_scmi/scmi_transport_smc.c
index 1c203a68acd4..c6c69a17a9cc 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_smc.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_smc.c
@@ -273,7 +273,7 @@ static const struct scmi_transport_ops scmi_smc_ops = {
 	.fetch_response = smc_fetch_response,
 };
 
-static const struct scmi_desc scmi_smc_desc = {
+static struct scmi_desc scmi_smc_desc = {
 	.ops = &scmi_smc_ops,
 	.max_rx_timeout_ms = 30,
 	.max_msg = 20,
diff --git a/drivers/firmware/arm_scmi/scmi_transport_virtio.c b/drivers/firmware/arm_scmi/scmi_transport_virtio.c
index 55d7245fa7ee..d711ba6e07f6 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_virtio.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_virtio.c
@@ -790,7 +790,7 @@ static const struct scmi_transport_ops scmi_virtio_ops = {
 	.poll_done = virtio_poll_done,
 };
 
-static const struct scmi_desc scmi_virtio_desc = {
+static struct scmi_desc scmi_virtio_desc = {
 	.ops = &scmi_virtio_ops,
 	/* for non-realtime virtio devices */
 	.max_rx_timeout_ms = VIRTIO_MAX_RX_TIMEOUT_MS,
-- 
2.45.2

[PATCH v1 2/6] dt-bindings: firmware: arm,scmi: Introduce property max-rx-timeout-ms

[Cristian Marussi]

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

System Controller Management Interface(SCMI) firmwares might have different
designs depending on the platform: the maximum receive channel timeout
value might vary depending on the specific underlying hardware and firmware
design choices.

Introduce the general property max-rx-timeout-ms property to describe the
transport needs of a specific platform design.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
[Cristian: reworded commit message, s/mailbox/transport in description]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 4d823f3b1f0e..7c9fa07e2ff2 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -121,6 +121,13 @@ properties:
       atomic mode of operation, even if requested.
     default: 0
 
+  max-rx-timeout-ms:
+    description:
+      An optional time value, expressed in milliseconds, representing the
+      transport maximum timeout value for the receive channel. The value should
+      be a non-zero value if set.
+    minimum: 1
+
   arm,smc-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.45.2

[PATCH v1 3/6] firmware: arm_scmi: Use max-rx-timeout-ms from devicetree

[Cristian Marussi]

Override default maximum RX timeout with the value picked from the
devicetree, when provided.

Suggested-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 332cd5207bbc..e7dab0eea540 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2964,6 +2964,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 {
 	struct scmi_transport *trans;
+	int ret;
 
 	trans = dev_get_platdata(dev);
 	if (!trans || !trans->desc || !trans->supplier || !trans->core_ops)
@@ -2980,6 +2981,14 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 
 	dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
 
+	ret = of_property_read_u32(dev->of_node, "max-rx-timeout-ms",
+				   &trans->desc->max_rx_timeout_ms);
+	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);
+
 	return trans->desc;
 }
 
-- 
2.45.2

[PATCH v1 4/6] dt-bindings: firmware: arm,scmi: Introduce more transport properties

[Cristian Marussi]

Depending on hardware and firmware design choices different platforms can
end-up having different requirements around transport features.

Introduce max-msg-size and max-msg properties to describe platform specific
transport constraints.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml    | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 7c9fa07e2ff2..998fde321d82 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -128,6 +128,21 @@ 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 transport maximum
+      message size. The value should at least allow for the 4-bytes SCMI message
+      header to be transmitted if set.
+    minimum: 4
+
+  max-msg:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      An optional value representing the transport maximum number of concurrent
+      in-flight messages. The value should be a non-zero value if set.
+    minimum: 1
+
   arm,smc-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.45.2

[PATCH v1 5/6] firmware: arm_scmi: Use max_msg and max_msg_size from devicetree

[Cristian Marussi]

Override max_msg and max_msg_size transport properties when corresponding
devicetree properties are available.

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 e7dab0eea540..8295e63be38e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2986,8 +2986,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.45.2

[PATCH v1 6/6] 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 5261dc9b5f96..3bb5fdc0d57d 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -223,6 +223,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 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
@@ -241,6 +247,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 8295e63be38e..26781e678cc3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -147,12 +147,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
@@ -178,7 +172,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;
@@ -2427,7 +2420,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;
 }
@@ -2892,7 +2885,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);
 
@@ -3001,6 +2994,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;
 }
 
@@ -3050,13 +3050,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.45.2

RE: [PATCH v1 0/6] Add SCMI transport descriptors

[Peng Fan]

> Subject: [PATCH v1 0/6] Add SCMI transport descriptors
> 
> Hi,
> 
> this small series is an extended version of this recent, already reviewed,
> series [1] posted by Peng to add a new arm,scmi property to describe
> some platform-specific SCMI timeout constraints.
> 
> On top of that, this adds 2 more properties to describe a couple more
> platform-specific transport characteristics.
> 
> To minimize conflicts, the whole series is based on top of another
> recent series, which represents a rework of the core SCMI stack to split
> SCMI transports as standalone drivers. [2]

For the patchset,
Tested-by: Peng Fan <peng.fan@nxp.com>  #i.MX95 19x19 EVK

Regards,
Peng.

> 
> Thanks,
> Cristian
> 
> 
> ---
> 
> Cristian Marussi (5):
>   firmware: arm_scmi: Remove const from transport descriptors
>   firmware: arm_scmi: Use max-rx-timeout-ms from devicetree
>   dt-bindings: firmware: arm,scmi: Introduce more transport properties
>   firmware: arm_scmi: Use max_msg and max_msg_size from
> devicetree
>   firmware: arm_scmi: Relocate atomic_threshold to scmi_desc
> 
> Peng Fan (1):
>   dt-bindings: firmware: arm,scmi: Introduce property max-rx-timeout-
> ms
> 
>  .../bindings/firmware/arm,scmi.yaml           | 22 +++++++++
>  drivers/firmware/arm_scmi/common.h            |  9 +++-
>  drivers/firmware/arm_scmi/driver.c            | 46 ++++++++++++-------
>  .../arm_scmi/scmi_transport_mailbox.c         |  2 +-
>  .../firmware/arm_scmi/scmi_transport_optee.c  |  2 +-
>  .../firmware/arm_scmi/scmi_transport_smc.c    |  2 +-
>  .../firmware/arm_scmi/scmi_transport_virtio.c |  2 +-
>  7 files changed, 64 insertions(+), 21 deletions(-)
> 
> --
> 2.45.2
> 

Re: [PATCH v1 0/6] Add SCMI transport descriptors

[Cristian Marussi]

On Wed, Jul 31, 2024 at 01:13:14PM +0000, Peng Fan wrote:
> > Subject: [PATCH v1 0/6] Add SCMI transport descriptors
> > 
> > Hi,
> > 
> > this small series is an extended version of this recent, already reviewed,
> > series [1] posted by Peng to add a new arm,scmi property to describe
> > some platform-specific SCMI timeout constraints.
> > 
> > On top of that, this adds 2 more properties to describe a couple more
> > platform-specific transport characteristics.
> > 
> > To minimize conflicts, the whole series is based on top of another
> > recent series, which represents a rework of the core SCMI stack to split
> > SCMI transports as standalone drivers. [2]
> 
> For the patchset,
> Tested-by: Peng Fan <peng.fan@nxp.com>  #i.MX95 19x19 EVK
> 

Thanks for testing.
Cristian

Re: [PATCH v1 0/6] Add SCMI transport descriptors

[Nikunj Kela]

On 7/30/2024 7:47 AM, Cristian Marussi wrote:
> Hi,
>
> this small series is an extended version of this recent, already reviewed,
> series [1] posted by Peng to add a new arm,scmi property to describe some
> platform-specific SCMI timeout constraints.
>
> On top of that, this adds 2 more properties to describe a couple more
> platform-specific transport characteristics.
>
> To minimize conflicts, the whole series is based on top of another recent
> series, which represents a rework of the core SCMI stack to split SCMI
> transports as standalone drivers. [2]
>
> Thanks,
> Cristian
>
> [1]: https://lore.kernel.org/linux-arm-kernel/20240709140957.3171255-1-peng.fan@oss.nxp.com/
> [2]: https://lore.kernel.org/linux-arm-kernel/20240730133318.1573765-1-cristian.marussi@arm.com/T/#t
>
> ---

Thanks Cristian for the series. I have validated them on Qualcomm
SA8255p(to be upstreamed) Ride platform.

-Nikunj

>
> Cristian Marussi (5):
>   firmware: arm_scmi: Remove const from transport descriptors
>   firmware: arm_scmi: Use max-rx-timeout-ms from devicetree
>   dt-bindings: firmware: arm,scmi: Introduce more transport properties
>   firmware: arm_scmi: Use max_msg and max_msg_size from devicetree
>   firmware: arm_scmi: Relocate atomic_threshold to scmi_desc
>
> Peng Fan (1):
>   dt-bindings: firmware: arm,scmi: Introduce property max-rx-timeout-ms
>
>  .../bindings/firmware/arm,scmi.yaml           | 22 +++++++++
>  drivers/firmware/arm_scmi/common.h            |  9 +++-
>  drivers/firmware/arm_scmi/driver.c            | 46 ++++++++++++-------
>  .../arm_scmi/scmi_transport_mailbox.c         |  2 +-
>  .../firmware/arm_scmi/scmi_transport_optee.c  |  2 +-
>  .../firmware/arm_scmi/scmi_transport_smc.c    |  2 +-
>  .../firmware/arm_scmi/scmi_transport_virtio.c |  2 +-
>  7 files changed, 64 insertions(+), 21 deletions(-)
>

Re: [PATCH v1 0/6] Add SCMI transport descriptors

[Cristian Marussi]

On Thu, Aug 01, 2024 at 12:17:36PM -0700, Nikunj Kela wrote:
> 
> On 7/30/2024 7:47 AM, Cristian Marussi wrote:
> > Hi,
> >
> > this small series is an extended version of this recent, already reviewed,
> > series [1] posted by Peng to add a new arm,scmi property to describe some
> > platform-specific SCMI timeout constraints.
> >
> > On top of that, this adds 2 more properties to describe a couple more
> > platform-specific transport characteristics.
> >
> > To minimize conflicts, the whole series is based on top of another recent
> > series, which represents a rework of the core SCMI stack to split SCMI
> > transports as standalone drivers. [2]
> >
> > Thanks,
> > Cristian
> >
> > [1]: https://lore.kernel.org/linux-arm-kernel/20240709140957.3171255-1-peng.fan@oss.nxp.com/
> > [2]: https://lore.kernel.org/linux-arm-kernel/20240730133318.1573765-1-cristian.marussi@arm.com/T/#t
> >
> > ---
> 
> Thanks Cristian for the series. I have validated them on Qualcomm
> SA8255p(to be upstreamed) Ride platform.
> 

Thanks for testing...does this solves your issues ?
(assuming that will go through as it is ...)

Cristian

Re: [PATCH v1 0/6] Add SCMI transport descriptors

[Nikunj Kela]

On 8/1/2024 12:23 PM, Cristian Marussi wrote:
> On Thu, Aug 01, 2024 at 12:17:36PM -0700, Nikunj Kela wrote:
>> On 7/30/2024 7:47 AM, Cristian Marussi wrote:
>>> Hi,
>>>
>>> this small series is an extended version of this recent, already reviewed,
>>> series [1] posted by Peng to add a new arm,scmi property to describe some
>>> platform-specific SCMI timeout constraints.
>>>
>>> On top of that, this adds 2 more properties to describe a couple more
>>> platform-specific transport characteristics.
>>>
>>> To minimize conflicts, the whole series is based on top of another recent
>>> series, which represents a rework of the core SCMI stack to split SCMI
>>> transports as standalone drivers. [2]
>>>
>>> Thanks,
>>> Cristian
>>>
>>> [1]: https://lore.kernel.org/linux-arm-kernel/20240709140957.3171255-1-peng.fan@oss.nxp.com/
>>> [2]: https://lore.kernel.org/linux-arm-kernel/20240730133318.1573765-1-cristian.marussi@arm.com/T/#t
>>>
>>> ---
>> Thanks Cristian for the series. I have validated them on Qualcomm
>> SA8255p(to be upstreamed) Ride platform.
>>
> Thanks for testing...does this solves your issues ?
> (assuming that will go through as it is ...)
>
> Cristian

Yes it works like a charm! Hope this series goes through.

-Nikunj

Re: [PATCH v1 0/6] Add SCMI transport descriptors

[Sudeep Holla]

On Tue, 30 Jul 2024 15:47:01 +0100, Cristian Marussi wrote:
> this small series is an extended version of this recent, already reviewed,
> series [1] posted by Peng to add a new arm,scmi property to describe some
> platform-specific SCMI timeout constraints.
> 
> On top of that, this adds 2 more properties to describe a couple more
> platform-specific transport characteristics.
> 
> [...]

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

[1/6] firmware: arm_scmi: Remove const from transport descriptors
      https://git.kernel.org/sudeep.holla/c/668f0cb2337f
[2/6] dt-bindings: firmware: arm,scmi: Introduce property max-rx-timeout-ms
      https://git.kernel.org/sudeep.holla/c/3a5e6ab06eab
[3/6] firmware: arm_scmi: Use max-rx-timeout-ms from devicetree
      https://git.kernel.org/sudeep.holla/c/1780e411ef94

--
Regards,
Sudeep

Re: [PATCH v1 5/6] firmware: arm_scmi: Use max_msg and max_msg_size from devicetree

[Sudeep Holla]

On Tue, Jul 30, 2024 at 03:47:06PM +0100, Cristian Marussi wrote:
> Override max_msg and max_msg_size transport properties when corresponding
> devicetree properties are available.
>

I am holding off on these changes to think how these max_msg and max_msg_size
can be used without breaking any platforms allowing space for the header
and the message itself which is around 28 bytes. The binding itself looks
good. We may have to adjust the value passed to the core driver taking
the above into consideration.

--
Regards,
Sudeep

Re: [PATCH v1 5/6] firmware: arm_scmi: Use max_msg and max_msg_size from devicetree

[Nikunj Kela]

On 8/20/2024 7:48 AM, Sudeep Holla wrote:
> On Tue, Jul 30, 2024 at 03:47:06PM +0100, Cristian Marussi wrote:
>> Override max_msg and max_msg_size transport properties when corresponding
>> devicetree properties are available.
>>
> I am holding off on these changes to think how these max_msg and max_msg_size
> can be used without breaking any platforms allowing space for the header
> and the message itself which is around 28 bytes. The binding itself looks
> good. We may have to adjust the value passed to the core driver taking
> the above into consideration.
>
> --
> Regards,
> Sudeep

Hi Sudeep,

Qualcomm is interested in these parameters being configurable. Just
wanted to make sure you are aware.

Thanks,

-Nikunj

Re: [PATCH v1 3/6] firmware: arm_scmi: Use max-rx-timeout-ms from devicetree

[Florian Fainelli]

On 7/30/24 07:47, Cristian Marussi wrote:
> Override default maximum RX timeout with the value picked from the
> devicetree, when provided.
> 
> Suggested-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>   drivers/firmware/arm_scmi/driver.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 332cd5207bbc..e7dab0eea540 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2964,6 +2964,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
>   static const struct scmi_desc *scmi_transport_setup(struct device *dev)
>   {
>   	struct scmi_transport *trans;
> +	int ret;
>   
>   	trans = dev_get_platdata(dev);
>   	if (!trans || !trans->desc || !trans->supplier || !trans->core_ops)
> @@ -2980,6 +2981,14 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
>   
>   	dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
>   
> +	ret = of_property_read_u32(dev->of_node, "max-rx-timeout-ms",
> +				   &trans->desc->max_rx_timeout_ms);
> +	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);

I am bit on the fence on that change, it is useful, and we have done 
similar things before using a command line parameter.

This is definitively useful when bringing up new systems where you might 
be sprinkling enough debugging messages that this pushes your message 
processing logic too close to the default 30ms timeout. For normal use 
cases, we really want the message timeout to be as small as possible for 
most SCMI traffic but if we want the timeout to be configurable, that 
might have have to be on a per-message basis.
-- 
Florian

Re: [PATCH v1 3/6] firmware: arm_scmi: Use max-rx-timeout-ms from devicetree

[Cristian Marussi]

On Thu, Sep 12, 2024 at 02:05:53PM -0700, Florian Fainelli wrote:
> On 7/30/24 07:47, Cristian Marussi wrote:
> > Override default maximum RX timeout with the value picked from the
> > devicetree, when provided.
> > 
> > Suggested-by: Peng Fan <peng.fan@nxp.com>
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >   drivers/firmware/arm_scmi/driver.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 332cd5207bbc..e7dab0eea540 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2964,6 +2964,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
> >   static const struct scmi_desc *scmi_transport_setup(struct device *dev)
> >   {
> >   	struct scmi_transport *trans;
> > +	int ret;
> >   	trans = dev_get_platdata(dev);
> >   	if (!trans || !trans->desc || !trans->supplier || !trans->core_ops)
> > @@ -2980,6 +2981,14 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
> >   	dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
> > +	ret = of_property_read_u32(dev->of_node, "max-rx-timeout-ms",
> > +				   &trans->desc->max_rx_timeout_ms);
> > +	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);
>

Hi Florian,
 
> I am bit on the fence on that change, it is useful, and we have done similar
> things before using a command line parameter.
> 

I think the requirement around this as it came from from NXP/Peng was that,
depending on the design (HW/FW) of the SCMI platform at hand it can be that
the same default transport timeout (that is by itself much larger than
usually needed) could still be not enough for the transport: I think the
example was mentioning a system with many agents, so that your request
could end-up been "bootle-necked" in the execution queue of the server (AFAIU)

In such a case (if you cannot avoid the issue at first by reviewing your
design...) you have the effective need of describing that specific
platform/transport timeout characteristic and override the built-in default....

...it was not meant to be usead as a plain configuration....even though I
suppose this is sort of a matter of interepretation (and that can be
abused downstream...)

> This is definitively useful when bringing up new systems where you might be
> sprinkling enough debugging messages that this pushes your message
> processing logic too close to the default 30ms timeout. For normal use
> cases, we really want the message timeout to be as small as possible for
> most SCMI traffic but if we want the timeout to be configurable, that might
> have have to be on a per-message basis.

A per-message timeout would need a lot of re-design, but how would you tune
then all the possibly different timeouts across different platforms and/or
transports combinations...this seems something that could need a lot of
runtime calibration and re-configuration on a live system....

...have you specifically seen the need for such varying per-message
timeout in some specific case ?

Thanks,
Cristian

Re: [PATCH v1 5/6] firmware: arm_scmi: Use max_msg and max_msg_size from devicetree

[Cristian Marussi]

On Thu, Sep 12, 2024 at 12:22:48PM -0700, Nikunj Kela wrote:
> 
> On 8/20/2024 7:48 AM, Sudeep Holla wrote:
> > On Tue, Jul 30, 2024 at 03:47:06PM +0100, Cristian Marussi wrote:
> >> Override max_msg and max_msg_size transport properties when corresponding
> >> devicetree properties are available.
> >>
> > I am holding off on these changes to think how these max_msg and max_msg_size
> > can be used without breaking any platforms allowing space for the header
> > and the message itself which is around 28 bytes. The binding itself looks
> > good. We may have to adjust the value passed to the core driver taking
> > the above into consideration.
> >
> > --
> > Regards,
> > Sudeep
> 

Hi Nikunj,

these additional patches of initial series was still not reviewed by DT
maintainers really and it needed some rework for the reason Sudeep
mmentioned above. This is in my soon-ish todo list :P

Thanks,
Cristian