[PATCH V3 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms

[PATCH V3 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms

[Peng Fan (OSS)]

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

System Controller Management Interface(SCMI) firmwares might have
different designs by SCMI firmware developers. So the maximum receive
channel timeout value might also varies in the various designs.

So introduce property mbox-rx-timeout-ms to let each platform could
set its own timeout value in device tree.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 Add minimum: 1, because 0 is invalid. maximum is not set,
 because it is platform specific and unknown.
V2:
 Drop defaults, update description.

 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 ebf384e76df1..f84a978a36b2 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
+      mailbox maximum timeout value for 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.37.1

[PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Peng Fan (OSS)]

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

Each platform might have its own maximum mailbox receive channel timeout
value, so get property max-rx-timeout-ms from device tree and use it. If
the property does not exist, use mailbox 'scmi_desc' fixed value 30ms as
before.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 None
V2:
 Fix build error for raw mode.

 drivers/firmware/arm_scmi/driver.c   | 19 ++++++++++++++-----
 drivers/firmware/arm_scmi/raw_mode.c | 13 +++++++++----
 drivers/firmware/arm_scmi/raw_mode.h |  3 ++-
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6b6957f4743f..1aa613d4cb43 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -162,6 +162,7 @@ struct scmi_debug_info {
  * @devreq_mtx: A mutex to serialize device creation for this SCMI instance
  * @dbg: A pointer to debugfs related data (if any)
  * @raw: An opaque reference handle used by SCMI Raw mode.
+ * @max_rx_timeout_ms: the maximum receive channel timeout value
  */
 struct scmi_info {
 	int id;
@@ -188,6 +189,7 @@ struct scmi_info {
 	struct mutex devreq_mtx;
 	struct scmi_debug_info *dbg;
 	void *raw;
+	unsigned int max_rx_timeout_ms;
 };
 
 #define handle_to_scmi_info(h)	container_of(h, struct scmi_info, handle)
@@ -1302,11 +1304,11 @@ static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
 
 	trace_scmi_xfer_response_wait(xfer->transfer_id, xfer->hdr.id,
 				      xfer->hdr.protocol_id, xfer->hdr.seq,
-				      info->desc->max_rx_timeout_ms,
+				      info->max_rx_timeout_ms,
 				      xfer->hdr.poll_completion);
 
 	return scmi_wait_for_reply(dev, info->desc, cinfo, xfer,
-				   info->desc->max_rx_timeout_ms);
+				   info->max_rx_timeout_ms);
 }
 
 /**
@@ -2614,7 +2616,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
 	if (!cinfo)
 		return -ENOMEM;
 
-	cinfo->rx_timeout_ms = info->desc->max_rx_timeout_ms;
+	cinfo->rx_timeout_ms = info->max_rx_timeout_ms;
 
 	/* Create a unique name for this transport device */
 	snprintf(name, 32, "__scmi_transport_device_%s_%02X",
@@ -2888,7 +2890,7 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
 	debugfs_create_bool("is_atomic", 0400, trans, &dbg->is_atomic);
 
 	debugfs_create_u32("max_rx_timeout_ms", 0400, trans,
-			   (u32 *)&info->desc->max_rx_timeout_ms);
+			   (u32 *)&info->max_rx_timeout_ms);
 
 	debugfs_create_u32("max_msg_size", 0400, trans,
 			   (u32 *)&info->desc->max_msg_size);
@@ -2940,7 +2942,8 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 
 	info->raw = scmi_raw_mode_init(&info->handle, info->dbg->top_dentry,
 				       info->id, channels, num_chans,
-				       info->desc, info->tx_minfo.max_msg);
+				       info->desc, info->tx_minfo.max_msg,
+				       info->max_rx_timeout_ms);
 	if (IS_ERR(info->raw)) {
 		dev_err(info->dev, "Failed to initialize SCMI RAW Mode !\n");
 		ret = PTR_ERR(info->raw);
@@ -2953,6 +2956,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
+	u32 timeout;
 	char *err_str = "probe failure\n";
 	struct scmi_handle *handle;
 	const struct scmi_desc *desc;
@@ -3002,6 +3006,11 @@ static int scmi_probe(struct platform_device *pdev)
 			 info->atomic_threshold);
 	handle->is_transport_atomic = scmi_is_transport_atomic;
 
+	if (!of_property_read_u32(np, "max-rx-timeout-ms", &timeout))
+		info->max_rx_timeout_ms = timeout;
+	else
+		info->max_rx_timeout_ms = info->desc->max_rx_timeout_ms;
+
 	if (desc->ops->link_supplier) {
 		ret = desc->ops->link_supplier(dev);
 		if (ret) {
diff --git a/drivers/firmware/arm_scmi/raw_mode.c b/drivers/firmware/arm_scmi/raw_mode.c
index 130d13e9cd6b..06a764d106f8 100644
--- a/drivers/firmware/arm_scmi/raw_mode.c
+++ b/drivers/firmware/arm_scmi/raw_mode.c
@@ -165,6 +165,7 @@ struct scmi_raw_queue {
  * @wait_wq: A workqueue reference to the created workqueue
  * @dentry: Top debugfs root dentry for SCMI Raw
  * @gid: A group ID used for devres accounting
+ * @max_rx_timeout_ms: Max receive channel timeout value
  *
  * Note that this descriptor is passed back to the core after SCMI Raw is
  * initialized as an opaque handle to use by subsequent SCMI Raw call hooks.
@@ -187,6 +188,7 @@ struct scmi_raw_mode_info {
 	struct workqueue_struct	*wait_wq;
 	struct dentry *dentry;
 	void *gid;
+	u32 max_rx_timeout_ms;
 };
 
 /**
@@ -379,7 +381,7 @@ static void scmi_xfer_raw_waiter_enqueue(struct scmi_raw_mode_info *raw,
 	trace_scmi_xfer_response_wait(rw->xfer->transfer_id, rw->xfer->hdr.id,
 				      rw->xfer->hdr.protocol_id,
 				      rw->xfer->hdr.seq,
-				      raw->desc->max_rx_timeout_ms,
+				      raw->max_rx_timeout_ms,
 				      rw->xfer->hdr.poll_completion);
 
 	mutex_lock(&raw->active_mtx);
@@ -437,7 +439,7 @@ static void scmi_xfer_raw_worker(struct work_struct *work)
 
 	raw = container_of(work, struct scmi_raw_mode_info, waiters_work);
 	dev = raw->handle->dev;
-	max_tmo = msecs_to_jiffies(raw->desc->max_rx_timeout_ms);
+	max_tmo = msecs_to_jiffies(raw->max_rx_timeout_ms);
 
 	do {
 		int ret = 0;
@@ -574,7 +576,7 @@ static int scmi_xfer_raw_get_init(struct scmi_raw_mode_info *raw, void *buf,
 			dev_dbg(dev,
 				"...retrying[%d] inflight registration\n",
 				retry);
-			msleep(raw->desc->max_rx_timeout_ms /
+			msleep(raw->max_rx_timeout_ms /
 			       SCMI_XFER_RAW_MAX_RETRIES);
 		}
 	} while (ret && --retry);
@@ -1162,6 +1164,7 @@ static int scmi_raw_mode_setup(struct scmi_raw_mode_info *raw,
  * @num_chans: The number of entries in @channels
  * @desc: Reference to the transport operations
  * @tx_max_msg: Max number of in-flight messages allowed by the transport
+ * @max_rx_timeout_ms: Max receive channel timeout value
  *
  * This function prepare the SCMI Raw stack and creates the debugfs API.
  *
@@ -1170,7 +1173,8 @@ static int scmi_raw_mode_setup(struct scmi_raw_mode_info *raw,
 void *scmi_raw_mode_init(const struct scmi_handle *handle,
 			 struct dentry *top_dentry, int instance_id,
 			 u8 *channels, int num_chans,
-			 const struct scmi_desc *desc, int tx_max_msg)
+			 const struct scmi_desc *desc, int tx_max_msg,
+			 u32 max_rx_timeout_ms)
 {
 	int ret;
 	struct scmi_raw_mode_info *raw;
@@ -1188,6 +1192,7 @@ void *scmi_raw_mode_init(const struct scmi_handle *handle,
 	raw->desc = desc;
 	raw->tx_max_msg = tx_max_msg;
 	raw->id = instance_id;
+	raw->max_rx_timeout_ms = max_rx_timeout_ms;
 
 	ret = scmi_raw_mode_setup(raw, channels, num_chans);
 	if (ret) {
diff --git a/drivers/firmware/arm_scmi/raw_mode.h b/drivers/firmware/arm_scmi/raw_mode.h
index 8af756a83fd1..25d4a46261e7 100644
--- a/drivers/firmware/arm_scmi/raw_mode.h
+++ b/drivers/firmware/arm_scmi/raw_mode.h
@@ -20,7 +20,8 @@ enum {
 void *scmi_raw_mode_init(const struct scmi_handle *handle,
 			 struct dentry *top_dentry, int instance_id,
 			 u8 *channels, int num_chans,
-			 const struct scmi_desc *desc, int tx_max_msg);
+			 const struct scmi_desc *desc, int tx_max_msg,
+			 u32 max_rx_timeout_ms);
 void scmi_raw_mode_cleanup(void *raw);
 
 void scmi_raw_message_report(void *raw, struct scmi_xfer *xfer,
-- 
2.37.1

RE: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Peng Fan]

Hi Cristian,

> Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> value from device tree

The binding has got R-b from Rob, will you pick this patch in your next
Patchset?

Thanks,
Peng.

Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Krzysztof Kozlowski]

On 18/07/2024 04:24, Peng Fan wrote:
> Hi Cristian,
> 
>> Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
>> value from device tree
> 
> The binding has got R-b from Rob, will you pick this patch in your next
> Patchset?

Please don't ping during the merge window. Is this a fix? Why shall it
be picked up during merge window?

Best regards,
Krzysztof

RE: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Peng Fan]

> Subject: Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> value from device tree
> 
> On 18/07/2024 04:24, Peng Fan wrote:
> > Hi Cristian,
> >
> >> Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> value
> >> from device tree
> >
> > The binding has got R-b from Rob, will you pick this patch in your
> > next Patchset?
> 
> Please don't ping during the merge window. Is this a fix? Why shall it
> be picked up during merge window?

This is not to ask Sudeep for merge. Cristian is doing a patchset
for scmi transport, he said if the binding patch 1 got R-b, he would take
patch 2 into his scmi transport patchset to avoid conflict.

Regards,
Peng.

> 
> Best regards,
> Krzysztof

Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Cristian Marussi]

On Thu, Jul 18, 2024 at 02:24:15AM +0000, Peng Fan wrote:
> Hi Cristian,
> 

Hi Peng,

> > Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> > value from device tree
> 
> The binding has got R-b from Rob, will you pick this patch in your next
> Patchset?
> 

I am going to post a new transport_drivers_V3 and, as a separate
series on top of that, your latest "max-rx-timeuout" DT patch as
reviewed by Rob, plus the related new logic based on transport_drivers_V3
and a few more similar changes as requested by Nikunj.

Thanks,
Cristian

RE: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Peng Fan]

Hi Cristian,

> Subject: Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> value from device tree
> 
> On Thu, Jul 18, 2024 at 02:24:15AM +0000, Peng Fan wrote:
> > Hi Cristian,
> >
> 
> Hi Peng,
> 
> > > Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> > > value from device tree
> >
> > The binding has got R-b from Rob, will you pick this patch in your
> > next Patchset?
> >
> 
> I am going to post a new transport_drivers_V3 and, as a separate series
> on top of that, your latest "max-rx-timeuout" DT patch as reviewed by
> Rob, plus the related new logic based on transport_drivers_V3 and a
> few more similar changes as requested by Nikunj.

Ok, so your V3 not include this patchset. Do you have time to give
a look on the 2nd patch? We are still waiting this got R-b to land
this in Google GKI kernel.

Thanks,
Peng.

> 
> Thanks,
> Cristian

Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout value from device tree

[Cristian Marussi]

On Tue, Jul 30, 2024 at 02:24:17PM +0000, Peng Fan wrote:
> Hi Cristian,
> 
> > Subject: Re: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> > value from device tree
> > 
> > On Thu, Jul 18, 2024 at 02:24:15AM +0000, Peng Fan wrote:
> > > Hi Cristian,
> > >
> > 
> > Hi Peng,
> > 
> > > > Subject: [PATCH V3 2/2] firmware: arm_scmi: set mailbox timeout
> > > > value from device tree
> > >
> > > The binding has got R-b from Rob, will you pick this patch in your
> > > next Patchset?
> > >
> > 
> > I am going to post a new transport_drivers_V3 and, as a separate series
> > on top of that, your latest "max-rx-timeuout" DT patch as reviewed by
> > Rob, plus the related new logic based on transport_drivers_V3 and a
> > few more similar changes as requested by Nikunj.
> 
> Ok, so your V3 not include this patchset. Do you have time to give
> a look on the 2nd patch? We are still waiting this got R-b to land
> this in Google GKI kernel.
> 

As said above, your max-rx-timeout-ms DT patch (with Rob R-b) and more is 
included in this small series (just posted):

https://lore.kernel.org/linux-arm-kernel/20240730144707.1647025-1-cristian.marussi@arm.com/T/#t

based on top of transport_drivers_V3.

Thanks,
Cristian

Re: [PATCH V3 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms

[Rob Herring (Arm)]

On Tue, 09 Jul 2024 22:09:56 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> System Controller Management Interface(SCMI) firmwares might have
> different designs by SCMI firmware developers. So the maximum receive
> channel timeout value might also varies in the various designs.
> 
> So introduce property mbox-rx-timeout-ms to let each platform could
> set its own timeout value in device tree.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V3:
>  Add minimum: 1, because 0 is invalid. maximum is not set,
>  because it is platform specific and unknown.
> V2:
>  Drop defaults, update description.
> 
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>