devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] firmware: arm_scmi: introduce max-rx-timeout-ms property
@ 2024-06-21 12:46 Peng Fan (OSS)
  2024-06-21 12:46 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms Peng Fan (OSS)
  2024-06-21 12:46 ` [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree Peng Fan (OSS)
  0 siblings, 2 replies; 8+ messages in thread
From: Peng Fan (OSS) @ 2024-06-21 12:46 UTC (permalink / raw)
  To: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: arm-scmi, linux-arm-kernel, devicetree, linux-kernel, Peng Fan

The current mailbox scmi_desc set the timeout value to 30ms, but
each platform might have its own maximum receive channel timeout value
based on the System Control Management Interface(SCMI) firmware design
and hardware attributes, so introduce a property for that and use it.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (2):
      dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
      firmware: arm_scmi: set mailbox timeout value from device tree

 .../devicetree/bindings/firmware/arm,scmi.yaml        |  6 ++++++
 drivers/firmware/arm_scmi/driver.c                    | 19 ++++++++++++++-----
 drivers/firmware/arm_scmi/raw_mode.c                  | 11 +++++++----
 drivers/firmware/arm_scmi/raw_mode.h                  |  3 ++-
 4 files changed, 29 insertions(+), 10 deletions(-)
---
base-commit: 76db4c64526c5e8ba0f56ad3d890dce8f9b00bbc
change-id: 20240618-scmi-mailbox-v1-9b2c19663986

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
  2024-06-21 12:46 [PATCH 0/2] firmware: arm_scmi: introduce max-rx-timeout-ms property Peng Fan (OSS)
@ 2024-06-21 12:46 ` Peng Fan (OSS)
  2024-06-27 21:46   ` Rob Herring
  2024-06-21 12:46 ` [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree Peng Fan (OSS)
  1 sibling, 1 reply; 8+ messages in thread
From: Peng Fan (OSS) @ 2024-06-21 12:46 UTC (permalink / raw)
  To: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: arm-scmi, linux-arm-kernel, devicetree, linux-kernel, Peng Fan

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>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 4d823f3b1f0e..d6cc2bf4c819 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -121,6 +121,12 @@ properties:
       atomic mode of operation, even if requested.
     default: 0
 
+  max-rx-timeout-ms:
+    description:
+      An optional time value, expressed in milliseconds, representing, on this
+      platform, the mailbox maximum timeout value for receive channel.
+    default: 0
+
   arm,smc-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:

-- 
2.37.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree
  2024-06-21 12:46 [PATCH 0/2] firmware: arm_scmi: introduce max-rx-timeout-ms property Peng Fan (OSS)
  2024-06-21 12:46 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms Peng Fan (OSS)
@ 2024-06-21 12:46 ` Peng Fan (OSS)
  2024-06-28  2:59   ` kernel test robot
  1 sibling, 1 reply; 8+ messages in thread
From: Peng Fan (OSS) @ 2024-06-21 12:46 UTC (permalink / raw)
  To: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: arm-scmi, linux-arm-kernel, devicetree, linux-kernel, Peng Fan

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>
---
 drivers/firmware/arm_scmi/driver.c   | 19 ++++++++++++++-----
 drivers/firmware/arm_scmi/raw_mode.c | 11 +++++++----
 drivers/firmware/arm_scmi/raw_mode.h |  3 ++-
 3 files changed, 23 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..d4f37ee664a2 100644
--- a/drivers/firmware/arm_scmi/raw_mode.c
+++ b/drivers/firmware/arm_scmi/raw_mode.c
@@ -379,7 +379,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 +437,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 +574,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 +1162,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 +1171,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 +1190,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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
  2024-06-21 12:46 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms Peng Fan (OSS)
@ 2024-06-27 21:46   ` Rob Herring
  2024-06-27 23:17     ` Peng Fan
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2024-06-27 21:46 UTC (permalink / raw)
  To: Peng Fan (OSS)
  Cc: Sudeep Holla, Cristian Marussi, Krzysztof Kozlowski, Conor Dooley,
	arm-scmi, linux-arm-kernel, devicetree, linux-kernel, Peng Fan

On Fri, Jun 21, 2024 at 08:46:57PM +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>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 4d823f3b1f0e..d6cc2bf4c819 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -121,6 +121,12 @@ properties:
>        atomic mode of operation, even if requested.
>      default: 0
>  
> +  max-rx-timeout-ms:
> +    description:
> +      An optional time value, expressed in milliseconds, representing, on this
> +      platform, the mailbox maximum timeout value for receive channel.

"on this platform"? Doesn't every property apply to the given platform?

> +    default: 0

0 means no timeout or response is instant?

> +
>    arm,smc-id:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> 
> -- 
> 2.37.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
  2024-06-27 21:46   ` Rob Herring
@ 2024-06-27 23:17     ` Peng Fan
  2024-07-01  9:06       ` Sudeep Holla
  0 siblings, 1 reply; 8+ messages in thread
From: Peng Fan @ 2024-06-27 23:17 UTC (permalink / raw)
  To: Rob Herring, Peng Fan (OSS)
  Cc: Sudeep Holla, Cristian Marussi, Krzysztof Kozlowski, Conor Dooley,
	arm-scmi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org

> Subject: Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce
> property mbox-rx-timeout-ms
> 
> On Fri, Jun 21, 2024 at 08:46:57PM +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>
> > ---
> >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6
> ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git
> a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 4d823f3b1f0e..d6cc2bf4c819 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -121,6 +121,12 @@ properties:
> >        atomic mode of operation, even if requested.
> >      default: 0
> >
> > +  max-rx-timeout-ms:
> > +    description:
> > +      An optional time value, expressed in milliseconds, representing,
> on this
> > +      platform, the mailbox maximum timeout value for receive
> channel.
> 
> "on this platform"? Doesn't every property apply to the given platform?

Yeah, apply to all the use mailbox.

> 
> > +    default: 0
> 
> 0 means no timeout or response is instant?

I should use 30ms same as what the driver currently use.

Thanks,
Peng.

> 
> > +
> >    arm,smc-id:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description:
> >
> > --
> > 2.37.1
> >


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree
  2024-06-21 12:46 ` [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree Peng Fan (OSS)
@ 2024-06-28  2:59   ` kernel test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-06-28  2:59 UTC (permalink / raw)
  To: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: oe-kbuild-all, arm-scmi, linux-arm-kernel, devicetree,
	linux-kernel, Peng Fan

Hi Peng,

kernel test robot noticed the following build errors:

[auto build test ERROR on 76db4c64526c5e8ba0f56ad3d890dce8f9b00bbc]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/dt-bindings-firmware-arm-scmi-introduce-property-mbox-rx-timeout-ms/20240625-163117
base:   76db4c64526c5e8ba0f56ad3d890dce8f9b00bbc
patch link:    https://lore.kernel.org/r/20240621-scmi-mailbox-v1-v1-2-8ed450735f46%40nxp.com
patch subject: [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree
config: i386-buildonly-randconfig-003-20240628 (https://download.01.org/0day-ci/archive/20240628/202406281000.agOs4t8T-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240628/202406281000.agOs4t8T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406281000.agOs4t8T-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/arm_scmi/raw_mode.c: In function 'scmi_xfer_raw_waiter_enqueue':
>> drivers/firmware/arm_scmi/raw_mode.c:382:42: error: 'struct scmi_raw_mode_info' has no member named 'max_rx_timeout_ms'
     382 |                                       raw->max_rx_timeout_ms,
         |                                          ^~
   drivers/firmware/arm_scmi/raw_mode.c: In function 'scmi_xfer_raw_worker':
   drivers/firmware/arm_scmi/raw_mode.c:440:39: error: 'struct scmi_raw_mode_info' has no member named 'max_rx_timeout_ms'
     440 |         max_tmo = msecs_to_jiffies(raw->max_rx_timeout_ms);
         |                                       ^~
   drivers/firmware/arm_scmi/raw_mode.c: In function 'scmi_xfer_raw_get_init':
   drivers/firmware/arm_scmi/raw_mode.c:577:35: error: 'struct scmi_raw_mode_info' has no member named 'max_rx_timeout_ms'
     577 |                         msleep(raw->max_rx_timeout_ms /
         |                                   ^~
   drivers/firmware/arm_scmi/raw_mode.c: In function 'scmi_raw_mode_init':
   drivers/firmware/arm_scmi/raw_mode.c:1193:12: error: 'struct scmi_raw_mode_info' has no member named 'max_rx_timeout_ms'
    1193 |         raw->max_rx_timeout_ms = max_rx_timeout_ms;
         |            ^~


vim +382 drivers/firmware/arm_scmi/raw_mode.c

   372	
   373	static void scmi_xfer_raw_waiter_enqueue(struct scmi_raw_mode_info *raw,
   374						 struct scmi_xfer_raw_waiter *rw)
   375	{
   376		/* A timestamp for the deferred worker to know how much this has aged */
   377		rw->start_jiffies = jiffies;
   378	
   379		trace_scmi_xfer_response_wait(rw->xfer->transfer_id, rw->xfer->hdr.id,
   380					      rw->xfer->hdr.protocol_id,
   381					      rw->xfer->hdr.seq,
 > 382					      raw->max_rx_timeout_ms,
   383					      rw->xfer->hdr.poll_completion);
   384	
   385		mutex_lock(&raw->active_mtx);
   386		list_add_tail(&rw->node, &raw->active_waiters);
   387		mutex_unlock(&raw->active_mtx);
   388	
   389		/* kick waiter work */
   390		queue_work(raw->wait_wq, &raw->waiters_work);
   391	}
   392	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
  2024-06-27 23:17     ` Peng Fan
@ 2024-07-01  9:06       ` Sudeep Holla
  2024-07-01 12:41         ` Peng Fan
  0 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2024-07-01  9:06 UTC (permalink / raw)
  To: Peng Fan
  Cc: Rob Herring, Peng Fan (OSS), Sudeep Holla, Cristian Marussi,
	Krzysztof Kozlowski, Conor Dooley, arm-scmi@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org

On Thu, Jun 27, 2024 at 11:17:49PM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce
> > property mbox-rx-timeout-ms
> >
> > On Fri, Jun 21, 2024 at 08:46:57PM +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>
> > > ---
> > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6
> > ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 4d823f3b1f0e..d6cc2bf4c819 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -121,6 +121,12 @@ properties:
> > >        atomic mode of operation, even if requested.
> > >      default: 0
> > >
> > > +  max-rx-timeout-ms:
> > > +    description:
> > > +      An optional time value, expressed in milliseconds, representing,
> > on this
> > > +      platform, the mailbox maximum timeout value for receive
> > channel.
> >
> > "on this platform"? Doesn't every property apply to the given platform?
>
> Yeah, apply to all the use mailbox.
>
> >
> > > +    default: 0
> >
> > 0 means no timeout or response is instant?
>
> I should use 30ms same as what the driver currently use.
>

That sounds very wrong to me. The binding is independent of current driver
behaviour. How the driver handles the case of default 0 value is different
from what the default value in the DT means IMO. You can't just set a default
value in the DT binding based on the current driver setting.

We can always say since it is optional, absence of it is what driver handles
as 30ms. 0ms is impossible or incorrect value as transport involves some
delay even if it is in terms of uS. So I prefer to set a value of > 0 in DT
and make that a requirement.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms
  2024-07-01  9:06       ` Sudeep Holla
@ 2024-07-01 12:41         ` Peng Fan
  0 siblings, 0 replies; 8+ messages in thread
From: Peng Fan @ 2024-07-01 12:41 UTC (permalink / raw)
  To: Sudeep Holla, robh@kernel.org
  Cc: Peng Fan (OSS), Cristian Marussi, Krzysztof Kozlowski,
	Conor Dooley, arm-scmi@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org

Hi Rob, Sudeep

> Subject: Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce
> property mbox-rx-timeout-ms
> 
> On Thu, Jun 27, 2024 at 11:17:49PM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi:
> introduce
> > > property mbox-rx-timeout-ms
> > >
> > > On Fri, Jun 21, 2024 at 08:46:57PM +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>
> > > > ---
> > > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6
> > > ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git
> > > a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > index 4d823f3b1f0e..d6cc2bf4c819 100644
> > > > ---
> a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > +++
> b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > @@ -121,6 +121,12 @@ properties:
> > > >        atomic mode of operation, even if requested.
> > > >      default: 0
> > > >
> > > > +  max-rx-timeout-ms:
> > > > +    description:
> > > > +      An optional time value, expressed in milliseconds,
> > > > + representing,
> > > on this
> > > > +      platform, the mailbox maximum timeout value for receive
> > > channel.
> > >
> > > "on this platform"? Doesn't every property apply to the given
> platform?
> >
> > Yeah, apply to all the use mailbox.
> >
> > >
> > > > +    default: 0
> > >
> > > 0 means no timeout or response is instant?
> >
> > I should use 30ms same as what the driver currently use.
> >
> 
> That sounds very wrong to me. The binding is independent of current
> driver behaviour. How the driver handles the case of default 0 value is
> different from what the default value in the DT means IMO. You can't
> just set a default value in the DT binding based on the current driver
> setting.
> 
> We can always say since it is optional, absence of it is what driver
> handles as 30ms. 0ms is impossible or incorrect value as transport
> involves some delay even if it is in terms of uS. So I prefer to set a value
> of > 0 in DT and make that a requirement.

How about this?
  max-rx-timeout-ms:                                                                                
    $ref: /schemas/types.yaml#/definitions/uint32                                                   
    description:                                                                                    
      An optional time value, expressed in milliseconds, representing the                           
      mailbox maximum timeout value for receive channel.                                            
    minimum: 1                                                                                      
    maximum: 5000

or?

  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.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-01 12:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-21 12:46 [PATCH 0/2] firmware: arm_scmi: introduce max-rx-timeout-ms property Peng Fan (OSS)
2024-06-21 12:46 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms Peng Fan (OSS)
2024-06-27 21:46   ` Rob Herring
2024-06-27 23:17     ` Peng Fan
2024-07-01  9:06       ` Sudeep Holla
2024-07-01 12:41         ` Peng Fan
2024-06-21 12:46 ` [PATCH 2/2] firmware: arm_scmi: set mailbox timeout value from device tree Peng Fan (OSS)
2024-06-28  2:59   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).