[PATCH v2 0/2] firmware: arm_scmi: add notification completion channel

[PATCH v2 0/2] firmware: arm_scmi: add notification completion channel

[Peng Fan (OSS)]

Per spec:
Completion interrupts This transport supports polling or interrupt driven
modes of communication. In interrupt mode, when the callee completes
processing a message, it raises an interrupt to the caller. Hardware
support for completion interrupts is optional.

i.MX95 SCMI firmware is fully interrupt driven, so Platform expects
completion interrupt for Platform to Agent(P2A) notifictions.

Add another optional mailbox channel for Agent to notify Platform that
notification message has been accepted

After notification channel status become freed, Agent will use the
new mailbox channel to send completion interrupt to Platform.

Add shmem_channel_intr_enabled to check channel flags.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Support bidirectional channel and update commit log to include
  bidirectional channel in patch 2.
- Link to v1: https://lore.kernel.org/r/20240507-scmi-notify-v1-0-1fdefc984d53@nxp.com

---
Peng Fan (2):
      dt-bindings: firmware: arm,scmi: Support notification completion channel
      firmware: arm_scmi: mailbox: support P2A channel completion

 .../devicetree/bindings/firmware/arm,scmi.yaml     | 12 +++--
 drivers/firmware/arm_scmi/common.h                 |  1 +
 drivers/firmware/arm_scmi/mailbox.c                | 60 +++++++++++++++++++---
 drivers/firmware/arm_scmi/shmem.c                  |  5 ++
 4 files changed, 68 insertions(+), 10 deletions(-)
---
base-commit: 9221b2819b8a4196eecf5476d66201be60fbcf29
change-id: 20240507-scmi-notify-07e87a8b9a23

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

[PATCH v2 1/2] dt-bindings: firmware: arm,scmi: Support notification completion channel

[Peng Fan (OSS)]

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

Per System Control Management Interface specification:
" Completion interrupts:This transport supports polling or interrupt driven
  modes of communication. In interrupt mode, when the callee completes
  processing a message, it raises an interrupt to the caller. Hardware
  support for completion interrupts is optional."
So add an optional mailbox channel for notification completion interrupts.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 7de2c29606e5..308af58180d1 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -72,14 +72,17 @@ properties:
           - const: tx
           - const: tx_reply
           - const: rx
+          - const: rx_reply
         minItems: 2
 
   mboxes:
     description:
       List of phandle and mailbox channel specifiers. It should contain
-      exactly one, two or three mailboxes; the first one or two for transmitting
-      messages ("tx") and another optional ("rx") for receiving notifications
-      and delayed responses, if supported by the platform.
+      exactly one, two, three or four mailboxes; the first one or two for
+      transmitting messages ("tx") and another optional ("rx") for receiving
+      notifications and delayed responses, if supported by the platform.
+      The optional ("rx_reply") is for notifications completion interrupt,
+      if supported by the platform.
       The number of mailboxes needed for transmitting messages depends on the
       type of channels exposed by the specific underlying mailbox controller;
       one single channel descriptor is enough if such channel is bidirectional,
@@ -92,9 +95,10 @@ properties:
        2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
        2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
        3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
+       4 mbox / 2 shmem => SCMI TX and RX over 4 mailbox unidirectional channels
       Any other combination of mboxes and shmem is invalid.
     minItems: 1
-    maxItems: 3
+    maxItems: 4
 
   shmem:
     description:

-- 
2.37.1

Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: Support notification completion channel

[Rob Herring (Arm)]

On Fri, 10 May 2024 11:19:47 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Per System Control Management Interface specification:
> " Completion interrupts:This transport supports polling or interrupt driven
>   modes of communication. In interrupt mode, when the callee completes
>   processing a message, it raises an interrupt to the caller. Hardware
>   support for completion interrupts is optional."
> So add an optional mailbox channel for notification completion interrupts.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

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

[PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Peng Fan (OSS)]

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

i.MX95 System Manager firmware is fully interrupt driven. The notification
channel needs completion interrupt to drive its notification queue. Without
completion interrupt, the notification will work abnormal.

- Add an optional unidirectional mailbox channel. If the channel flag has
  INTR set, and the completion interrupt channel is provided, issue the
  mbox message to Platform after the channel is freed.
- Support bidirectional channel completion interrupt.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/common.h  |  1 +
 drivers/firmware/arm_scmi/mailbox.c | 60 +++++++++++++++++++++++++++++++++----
 drivers/firmware/arm_scmi/shmem.c   |  5 ++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index b5ac25dbc1ca..4b8c5250cdb5 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -326,6 +326,7 @@ void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem);
 bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
 		     struct scmi_xfer *xfer);
 bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem);
+bool shmem_channel_intr_enabled(struct scmi_shared_mem __iomem *shmem);
 
 /* declarations for message passing transports */
 struct scmi_msg_payld;
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 615a3b2ad83d..adb69a6a0223 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -21,6 +21,7 @@
  * @cl: Mailbox Client
  * @chan: Transmit/Receive mailbox uni/bi-directional channel
  * @chan_receiver: Optional Receiver mailbox unidirectional channel
+ * @chan_platform_receiver: Optional Platform Receiver mailbox unidirectional channel
  * @cinfo: SCMI channel info
  * @shmem: Transmit/Receive shared memory area
  */
@@ -28,6 +29,7 @@ struct scmi_mailbox {
 	struct mbox_client cl;
 	struct mbox_chan *chan;
 	struct mbox_chan *chan_receiver;
+	struct mbox_chan *chan_platform_receiver;
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
 };
@@ -91,6 +93,8 @@ static bool mailbox_chan_available(struct device_node *of_node, int idx)
  *		 for replies on the a2p channel. Set as zero if not present.
  * @p2a_chan: A reference to the optional p2a channel.
  *	      Set as zero if not present.
+ * @p2a_rx_chan: A reference to the optional p2a completion channel.
+ *	      Set as zero if not present.
  *
  * At first, validate the transport configuration as described in terms of
  * 'mboxes' and 'shmem', then determin which mailbox channel indexes are
@@ -98,8 +102,8 @@ static bool mailbox_chan_available(struct device_node *of_node, int idx)
  *
  * Return: 0 on Success or error
  */
-static int mailbox_chan_validate(struct device *cdev,
-				 int *a2p_rx_chan, int *p2a_chan)
+static int mailbox_chan_validate(struct device *cdev, int *a2p_rx_chan,
+				 int *p2a_chan, int *p2a_rx_chan)
 {
 	int num_mb, num_sh, ret = 0;
 	struct device_node *np = cdev->of_node;
@@ -109,8 +113,9 @@ static int mailbox_chan_validate(struct device *cdev,
 	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb, num_sh);
 
 	/* Bail out if mboxes and shmem descriptors are inconsistent */
-	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
-	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)) {
+	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 4 ||
+	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2) ||
+	    (num_mb == 4 && num_sh != 2)) {
 		dev_warn(cdev,
 			 "Invalid channel descriptor for '%s' - mbs:%d  shm:%d\n",
 			 of_node_full_name(np), num_mb, num_sh);
@@ -139,6 +144,7 @@ static int mailbox_chan_validate(struct device *cdev,
 		case 1:
 			*a2p_rx_chan = 0;
 			*p2a_chan = 0;
+			*p2a_rx_chan = 0;
 			break;
 		case 2:
 			if (num_sh == 2) {
@@ -148,10 +154,17 @@ static int mailbox_chan_validate(struct device *cdev,
 				*a2p_rx_chan = 1;
 				*p2a_chan = 0;
 			}
+			*p2a_rx_chan = 0;
 			break;
 		case 3:
 			*a2p_rx_chan = 1;
 			*p2a_chan = 2;
+			*p2a_rx_chan = 0;
+			break;
+		case 4:
+			*a2p_rx_chan = 1;
+			*p2a_chan = 2;
+			*p2a_rx_chan = 3;
 			break;
 		}
 	}
@@ -166,12 +179,12 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	struct device *cdev = cinfo->dev;
 	struct scmi_mailbox *smbox;
 	struct device_node *shmem;
-	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
+	int ret, a2p_rx_chan, p2a_chan, p2a_rx_chan, idx = tx ? 0 : 1;
 	struct mbox_client *cl;
 	resource_size_t size;
 	struct resource res;
 
-	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
+	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan, &p2a_rx_chan);
 	if (ret)
 		return ret;
 
@@ -229,6 +242,17 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		}
 	}
 
+	if (!tx && p2a_rx_chan) {
+		smbox->chan_platform_receiver = mbox_request_channel(cl, p2a_rx_chan);
+		if (IS_ERR(smbox->chan_platform_receiver)) {
+			ret = PTR_ERR(smbox->chan_platform_receiver);
+			if (ret != -EPROBE_DEFER)
+				dev_err(cdev, "failed to request SCMI P2A Receiver mailbox\n");
+			return ret;
+		}
+	}
+
+
 	cinfo->transport_info = smbox;
 	smbox->cinfo = cinfo;
 
@@ -243,9 +267,11 @@ static int mailbox_chan_free(int id, void *p, void *data)
 	if (smbox && !IS_ERR(smbox->chan)) {
 		mbox_free_channel(smbox->chan);
 		mbox_free_channel(smbox->chan_receiver);
+		mbox_free_channel(smbox->chan_platform_receiver);
 		cinfo->transport_info = NULL;
 		smbox->chan = NULL;
 		smbox->chan_receiver = NULL;
+		smbox->chan_platform_receiver = NULL;
 		smbox->cinfo = NULL;
 	}
 
@@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
 static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
 {
 	struct scmi_mailbox *smbox = cinfo->transport_info;
+	struct device *cdev = cinfo->dev;
+	struct mbox_chan *intr;
+	int ret;
 
 	shmem_clear_channel(smbox->shmem);
+
+	if (!shmem_channel_intr_enabled(smbox->shmem))
+		return;
+
+	if (smbox->chan_platform_receiver)
+		intr = smbox->chan_platform_receiver;
+	else if (smbox->chan)
+		intr = smbox->chan;
+	else {
+		dev_err(cdev, "Channel INTR wrongly set?\n");
+		return;
+	}
+
+	ret = mbox_send_message(intr, NULL);
+	/* mbox_send_message returns non-negative value on success, so reset */
+	if (ret > 0)
+		ret = 0;
+
+	mbox_client_txdone(intr, ret);
 }
 
 static bool
diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c
index 8bf495bcad09..b74e5a740f2c 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -128,3 +128,8 @@ bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem)
 	return (ioread32(&shmem->channel_status) &
 			SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
 }
+
+bool shmem_channel_intr_enabled(struct scmi_shared_mem __iomem *shmem)
+{
+	return ioread32(&shmem->flags) & SCMI_SHMEM_FLAG_INTR_ENABLED;
+}

-- 
2.37.1

Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Cristian Marussi]

On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX95 System Manager firmware is fully interrupt driven. The notification
> channel needs completion interrupt to drive its notification queue. Without
> completion interrupt, the notification will work abnormal.
> 

Hi Peng,

Thanks to have addressed also the case of mailbox controllers with unidirectional
channels for P2A completion, but I have a further observation down
below, which I missed last time.

> - Add an optional unidirectional mailbox channel. If the channel flag has
>   INTR set, and the completion interrupt channel is provided, issue the
>   mbox message to Platform after the channel is freed.
> - Support bidirectional channel completion interrupt.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/common.h  |  1 +
>  drivers/firmware/arm_scmi/mailbox.c | 60 +++++++++++++++++++++++++++++++++----
>  drivers/firmware/arm_scmi/shmem.c   |  5 ++++
>  3 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index b5ac25dbc1ca..4b8c5250cdb5 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -326,6 +326,7 @@ void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem);
>  bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
>  		     struct scmi_xfer *xfer);
>  bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem);
> +bool shmem_channel_intr_enabled(struct scmi_shared_mem __iomem *shmem);
>  
>  /* declarations for message passing transports */
>  struct scmi_msg_payld;
> diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> index 615a3b2ad83d..adb69a6a0223 100644
> --- a/drivers/firmware/arm_scmi/mailbox.c
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -21,6 +21,7 @@
>   * @cl: Mailbox Client
>   * @chan: Transmit/Receive mailbox uni/bi-directional channel
>   * @chan_receiver: Optional Receiver mailbox unidirectional channel
> + * @chan_platform_receiver: Optional Platform Receiver mailbox unidirectional channel
>   * @cinfo: SCMI channel info
>   * @shmem: Transmit/Receive shared memory area
>   */
> @@ -28,6 +29,7 @@ struct scmi_mailbox {
>  	struct mbox_client cl;
>  	struct mbox_chan *chan;
>  	struct mbox_chan *chan_receiver;
> +	struct mbox_chan *chan_platform_receiver;
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
>  };
> @@ -91,6 +93,8 @@ static bool mailbox_chan_available(struct device_node *of_node, int idx)
>   *		 for replies on the a2p channel. Set as zero if not present.
>   * @p2a_chan: A reference to the optional p2a channel.
>   *	      Set as zero if not present.
> + * @p2a_rx_chan: A reference to the optional p2a completion channel.
> + *	      Set as zero if not present.
>   *
>   * At first, validate the transport configuration as described in terms of
>   * 'mboxes' and 'shmem', then determin which mailbox channel indexes are
> @@ -98,8 +102,8 @@ static bool mailbox_chan_available(struct device_node *of_node, int idx)
>   *
>   * Return: 0 on Success or error
>   */
> -static int mailbox_chan_validate(struct device *cdev,
> -				 int *a2p_rx_chan, int *p2a_chan)
> +static int mailbox_chan_validate(struct device *cdev, int *a2p_rx_chan,
> +				 int *p2a_chan, int *p2a_rx_chan)
>  {
>  	int num_mb, num_sh, ret = 0;
>  	struct device_node *np = cdev->of_node;
> @@ -109,8 +113,9 @@ static int mailbox_chan_validate(struct device *cdev,
>  	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb, num_sh);
>  
>  	/* Bail out if mboxes and shmem descriptors are inconsistent */
> -	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
> -	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)) {
> +	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 4 ||
> +	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2) ||
> +	    (num_mb == 4 && num_sh != 2)) {
>  		dev_warn(cdev,
>  			 "Invalid channel descriptor for '%s' - mbs:%d  shm:%d\n",
>  			 of_node_full_name(np), num_mb, num_sh);
> @@ -139,6 +144,7 @@ static int mailbox_chan_validate(struct device *cdev,
>  		case 1:
>  			*a2p_rx_chan = 0;
>  			*p2a_chan = 0;
> +			*p2a_rx_chan = 0;
>  			break;
>  		case 2:
>  			if (num_sh == 2) {
> @@ -148,10 +154,17 @@ static int mailbox_chan_validate(struct device *cdev,
>  				*a2p_rx_chan = 1;
>  				*p2a_chan = 0;
>  			}
> +			*p2a_rx_chan = 0;
>  			break;
>  		case 3:
>  			*a2p_rx_chan = 1;
>  			*p2a_chan = 2;
> +			*p2a_rx_chan = 0;
> +			break;
> +		case 4:
> +			*a2p_rx_chan = 1;
> +			*p2a_chan = 2;
> +			*p2a_rx_chan = 3;
>  			break;
>  		}
>  	}
> @@ -166,12 +179,12 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>  	struct device *cdev = cinfo->dev;
>  	struct scmi_mailbox *smbox;
>  	struct device_node *shmem;
> -	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
> +	int ret, a2p_rx_chan, p2a_chan, p2a_rx_chan, idx = tx ? 0 : 1;
>  	struct mbox_client *cl;
>  	resource_size_t size;
>  	struct resource res;
>  
> -	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
> +	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan, &p2a_rx_chan);
>  	if (ret)
>  		return ret;
>  
> @@ -229,6 +242,17 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>  		}
>  	}
>  
> +	if (!tx && p2a_rx_chan) {
> +		smbox->chan_platform_receiver = mbox_request_channel(cl, p2a_rx_chan);
> +		if (IS_ERR(smbox->chan_platform_receiver)) {
> +			ret = PTR_ERR(smbox->chan_platform_receiver);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(cdev, "failed to request SCMI P2A Receiver mailbox\n");
> +			return ret;
> +		}
> +	}
> +
> +
>  	cinfo->transport_info = smbox;
>  	smbox->cinfo = cinfo;
>  
> @@ -243,9 +267,11 @@ static int mailbox_chan_free(int id, void *p, void *data)
>  	if (smbox && !IS_ERR(smbox->chan)) {
>  		mbox_free_channel(smbox->chan);
>  		mbox_free_channel(smbox->chan_receiver);
> +		mbox_free_channel(smbox->chan_platform_receiver);
>  		cinfo->transport_info = NULL;
>  		smbox->chan = NULL;
>  		smbox->chan_receiver = NULL;
> +		smbox->chan_platform_receiver = NULL;
>  		smbox->cinfo = NULL;
>  	}
>  
> @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
>  static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
>  {
>  	struct scmi_mailbox *smbox = cinfo->transport_info;
> +	struct device *cdev = cinfo->dev;
> +	struct mbox_chan *intr;
> +	int ret;
>  
>  	shmem_clear_channel(smbox->shmem);
> +
> +	if (!shmem_channel_intr_enabled(smbox->shmem))
> +		return;
> +
> +	if (smbox->chan_platform_receiver)
> +		intr = smbox->chan_platform_receiver;
> +	else if (smbox->chan)
> +		intr = smbox->chan;
> +	else {
> +		dev_err(cdev, "Channel INTR wrongly set?\n");
> +		return;
> +	}
> +
> +	ret = mbox_send_message(intr, NULL);
> +	/* mbox_send_message returns non-negative value on success, so reset */
> +	if (ret > 0)
> +		ret = 0;
> +
> +	mbox_client_txdone(intr, ret);

Looking at mbox_client_txdone() implementation this call is allowed only
if the txdone_method is TXDONE_BY_ACK, which in turn depend on the type of
underlying mbox controller that you are using AND/OR the SCMI client configuration
(knows_tx_done), so I dont think you can call this unconditionally without the
risk of hitting the related dev_err() in mbox_client_txdone if the underlying
mbox controller was instead supposed to issue an mbox_chan_txdone() on its own.

IOW, if the mbox controller is operating in TXDONE_BY_IRQ mode or in
TXDONE_BY_POLL (and that would be the case if polling is used since the RX channel
does NOT set the client flag cl->knows_txdone to true, so TXDONE_BY_ACK is not used
as an override) this should hit the dev_err() mentioned above...

dont you see any error ?

which mailbox controller do you use ?

does your mailbox controller NOT set txdone_irq and NEITHER txdone_poll ? 
(so defaulting to TDONE_BY_ACK in your case ?)

Thanks,
Cristian

RE: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Peng Fan]

Hi Cristian,

> Subject: Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A
> channel completion
> 
> On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > i.MX95 System Manager firmware is fully interrupt driven. The
> > notification channel needs completion interrupt to drive its
> > notification queue. Without completion interrupt, the notification will work
> abnormal.
> >
> 
> Hi Peng,
> 
> Thanks to have addressed also the case of mailbox controllers with
> unidirectional channels for P2A completion, but I have a further observation
> down below, which I missed last time.
> 
> > - Add an optional unidirectional mailbox channel. If the channel flag has
> >   INTR set, and the completion interrupt channel is provided, issue the
> >   mbox message to Platform after the channel is freed.
> > - Support bidirectional channel completion interrupt.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/common.h  |  1 +
> > drivers/firmware/arm_scmi/mailbox.c | 60
> +++++++++++++++++++++++++++++++++----
> >  drivers/firmware/arm_scmi/shmem.c   |  5 ++++
> >  3 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h
> > b/drivers/firmware/arm_scmi/common.h
> > index b5ac25dbc1ca..4b8c5250cdb5 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -326,6 +326,7 @@ void shmem_clear_channel(struct
> scmi_shared_mem
> > __iomem *shmem);  bool shmem_poll_done(struct scmi_shared_mem
> __iomem *shmem,
> >  		     struct scmi_xfer *xfer);
> >  bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem);
> > +bool shmem_channel_intr_enabled(struct scmi_shared_mem __iomem
> > +*shmem);
> >
> >  /* declarations for message passing transports */  struct
> > scmi_msg_payld; diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > b/drivers/firmware/arm_scmi/mailbox.c
> > index 615a3b2ad83d..adb69a6a0223 100644
> > --- a/drivers/firmware/arm_scmi/mailbox.c
> > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > @@ -21,6 +21,7 @@
> >   * @cl: Mailbox Client
> >   * @chan: Transmit/Receive mailbox uni/bi-directional channel
> >   * @chan_receiver: Optional Receiver mailbox unidirectional channel
> > + * @chan_platform_receiver: Optional Platform Receiver mailbox
> > + unidirectional channel
> >   * @cinfo: SCMI channel info
> >   * @shmem: Transmit/Receive shared memory area
> >   */
> > @@ -28,6 +29,7 @@ struct scmi_mailbox {
> >  	struct mbox_client cl;
> >  	struct mbox_chan *chan;
> >  	struct mbox_chan *chan_receiver;
> > +	struct mbox_chan *chan_platform_receiver;
> >  	struct scmi_chan_info *cinfo;
> >  	struct scmi_shared_mem __iomem *shmem;  }; @@ -91,6 +93,8
> @@ static
> > bool mailbox_chan_available(struct device_node *of_node, int idx)
> >   *		 for replies on the a2p channel. Set as zero if not present.
> >   * @p2a_chan: A reference to the optional p2a channel.
> >   *	      Set as zero if not present.
> > + * @p2a_rx_chan: A reference to the optional p2a completion channel.
> > + *	      Set as zero if not present.
> >   *
> >   * At first, validate the transport configuration as described in terms of
> >   * 'mboxes' and 'shmem', then determin which mailbox channel indexes
> > are @@ -98,8 +102,8 @@ static bool mailbox_chan_available(struct
> device_node *of_node, int idx)
> >   *
> >   * Return: 0 on Success or error
> >   */
> > -static int mailbox_chan_validate(struct device *cdev,
> > -				 int *a2p_rx_chan, int *p2a_chan)
> > +static int mailbox_chan_validate(struct device *cdev, int *a2p_rx_chan,
> > +				 int *p2a_chan, int *p2a_rx_chan)
> >  {
> >  	int num_mb, num_sh, ret = 0;
> >  	struct device_node *np = cdev->of_node; @@ -109,8 +113,9 @@
> static
> > int mailbox_chan_validate(struct device *cdev,
> >  	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb,
> num_sh);
> >
> >  	/* Bail out if mboxes and shmem descriptors are inconsistent */
> > -	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
> > -	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh !=
> 2)) {
> > +	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 4 ||
> > +	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)
> ||
> > +	    (num_mb == 4 && num_sh != 2)) {
> >  		dev_warn(cdev,
> >  			 "Invalid channel descriptor for '%s' - mbs:%d
> shm:%d\n",
> >  			 of_node_full_name(np), num_mb, num_sh); @@ -
> 139,6 +144,7 @@
> > static int mailbox_chan_validate(struct device *cdev,
> >  		case 1:
> >  			*a2p_rx_chan = 0;
> >  			*p2a_chan = 0;
> > +			*p2a_rx_chan = 0;
> >  			break;
> >  		case 2:
> >  			if (num_sh == 2) {
> > @@ -148,10 +154,17 @@ static int mailbox_chan_validate(struct device
> *cdev,
> >  				*a2p_rx_chan = 1;
> >  				*p2a_chan = 0;
> >  			}
> > +			*p2a_rx_chan = 0;
> >  			break;
> >  		case 3:
> >  			*a2p_rx_chan = 1;
> >  			*p2a_chan = 2;
> > +			*p2a_rx_chan = 0;
> > +			break;
> > +		case 4:
> > +			*a2p_rx_chan = 1;
> > +			*p2a_chan = 2;
> > +			*p2a_rx_chan = 3;
> >  			break;
> >  		}
> >  	}
> > @@ -166,12 +179,12 @@ static int mailbox_chan_setup(struct
> scmi_chan_info *cinfo, struct device *dev,
> >  	struct device *cdev = cinfo->dev;
> >  	struct scmi_mailbox *smbox;
> >  	struct device_node *shmem;
> > -	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
> > +	int ret, a2p_rx_chan, p2a_chan, p2a_rx_chan, idx = tx ? 0 : 1;
> >  	struct mbox_client *cl;
> >  	resource_size_t size;
> >  	struct resource res;
> >
> > -	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
> > +	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan,
> > +&p2a_rx_chan);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -229,6 +242,17 @@ static int mailbox_chan_setup(struct
> scmi_chan_info *cinfo, struct device *dev,
> >  		}
> >  	}
> >
> > +	if (!tx && p2a_rx_chan) {
> > +		smbox->chan_platform_receiver = mbox_request_channel(cl,
> p2a_rx_chan);
> > +		if (IS_ERR(smbox->chan_platform_receiver)) {
> > +			ret = PTR_ERR(smbox->chan_platform_receiver);
> > +			if (ret != -EPROBE_DEFER)
> > +				dev_err(cdev, "failed to request SCMI P2A
> Receiver mailbox\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +
> >  	cinfo->transport_info = smbox;
> >  	smbox->cinfo = cinfo;
> >
> > @@ -243,9 +267,11 @@ static int mailbox_chan_free(int id, void *p, void
> *data)
> >  	if (smbox && !IS_ERR(smbox->chan)) {
> >  		mbox_free_channel(smbox->chan);
> >  		mbox_free_channel(smbox->chan_receiver);
> > +		mbox_free_channel(smbox->chan_platform_receiver);
> >  		cinfo->transport_info = NULL;
> >  		smbox->chan = NULL;
> >  		smbox->chan_receiver = NULL;
> > +		smbox->chan_platform_receiver = NULL;
> >  		smbox->cinfo = NULL;
> >  	}
> >
> > @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct
> > scmi_chan_info *cinfo,  static void mailbox_clear_channel(struct
> > scmi_chan_info *cinfo)  {
> >  	struct scmi_mailbox *smbox = cinfo->transport_info;
> > +	struct device *cdev = cinfo->dev;
> > +	struct mbox_chan *intr;
> > +	int ret;
> >
> >  	shmem_clear_channel(smbox->shmem);
> > +
> > +	if (!shmem_channel_intr_enabled(smbox->shmem))
> > +		return;
> > +
> > +	if (smbox->chan_platform_receiver)
> > +		intr = smbox->chan_platform_receiver;
> > +	else if (smbox->chan)
> > +		intr = smbox->chan;
> > +	else {
> > +		dev_err(cdev, "Channel INTR wrongly set?\n");
> > +		return;
> > +	}
> > +
> > +	ret = mbox_send_message(intr, NULL);
> > +	/* mbox_send_message returns non-negative value on success, so
> reset */
> > +	if (ret > 0)
> > +		ret = 0;
> > +
> > +	mbox_client_txdone(intr, ret);
> 
> Looking at mbox_client_txdone() implementation this call is allowed only if
> the txdone_method is TXDONE_BY_ACK, which in turn depend on the type of
> underlying mbox controller that you are using AND/OR the SCMI client
> configuration (knows_tx_done), so I dont think you can call this
> unconditionally without the risk of hitting the related dev_err() in
> mbox_client_txdone if the underlying mbox controller was instead supposed
> to issue an mbox_chan_txdone() on its own.
> 
> IOW, if the mbox controller is operating in TXDONE_BY_IRQ mode or in
> TXDONE_BY_POLL (and that would be the case if polling is used since the RX
> channel does NOT set the client flag cl->knows_txdone to true, so
> TXDONE_BY_ACK is not used as an override) this should hit the dev_err()
> mentioned above...
> 
> dont you see any error ?

No error in my side.

> 
> which mailbox controller do you use ?

drivers/mailbox/imx-mu.c. The tx channel is IMX_MU_TYPE_TXDB_V2
> 
> does your mailbox controller NOT set txdone_irq and NEITHER txdone_poll ?
> (so defaulting to TDONE_BY_ACK in your case ?)

Use TXDONE_BY_ACK, no irq and no poll.

i.MX Message Unit(MU) supports many types, but for the i.MX SCMI MU,
we are using GIR(General Interrupt request) to the other side. 
And GIR will not trigger interrupt on the local side.
That means Linux write GIRA will trigger interrupt on M33 side, but no
interrupt in linux side. M33 write GIRB will trigger interrupt on
Linux side, but no interrupt in M33 side.

Regards,
Peng.

> 
> Thanks,
> Cristian

Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Cristian Marussi]

On Tue, Jun 04, 2024 at 01:06:08AM +0000, Peng Fan wrote:
> Hi Cristian,
> 
> > Subject: Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A
> > channel completion
> > 
> > On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > i.MX95 System Manager firmware is fully interrupt driven. The
> > > notification channel needs completion interrupt to drive its
> > > notification queue. Without completion interrupt, the notification will work
> > abnormal.
> > >
> > 
> > Hi Peng,
> > 
> > Thanks to have addressed also the case of mailbox controllers with
> > unidirectional channels for P2A completion, but I have a further observation
> > down below, which I missed last time.
> > 
> > > - Add an optional unidirectional mailbox channel. If the channel flag has
> > >   INTR set, and the completion interrupt channel is provided, issue the
> > >   mbox message to Platform after the channel is freed.
> > > - Support bidirectional channel completion interrupt.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/firmware/arm_scmi/common.h  |  1 +
> > > drivers/firmware/arm_scmi/mailbox.c | 60
> > +++++++++++++++++++++++++++++++++----
> > >  drivers/firmware/arm_scmi/shmem.c   |  5 ++++
> > >  3 files changed, 60 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/common.h
> > > b/drivers/firmware/arm_scmi/common.h
> > > index b5ac25dbc1ca..4b8c5250cdb5 100644
> > > --- a/drivers/firmware/arm_scmi/common.h
> > > +++ b/drivers/firmware/arm_scmi/common.h
> > > @@ -326,6 +326,7 @@ void shmem_clear_channel(struct
> > scmi_shared_mem
> > > __iomem *shmem);  bool shmem_poll_done(struct scmi_shared_mem
> > __iomem *shmem,
> > >  		     struct scmi_xfer *xfer);
> > >  bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem);
> > > +bool shmem_channel_intr_enabled(struct scmi_shared_mem __iomem
> > > +*shmem);
> > >
> > >  /* declarations for message passing transports */  struct
> > > scmi_msg_payld; diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > > b/drivers/firmware/arm_scmi/mailbox.c
> > > index 615a3b2ad83d..adb69a6a0223 100644
> > > --- a/drivers/firmware/arm_scmi/mailbox.c
> > > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > > @@ -21,6 +21,7 @@
> > >   * @cl: Mailbox Client
> > >   * @chan: Transmit/Receive mailbox uni/bi-directional channel
> > >   * @chan_receiver: Optional Receiver mailbox unidirectional channel
> > > + * @chan_platform_receiver: Optional Platform Receiver mailbox
> > > + unidirectional channel
> > >   * @cinfo: SCMI channel info
> > >   * @shmem: Transmit/Receive shared memory area
> > >   */
> > > @@ -28,6 +29,7 @@ struct scmi_mailbox {
> > >  	struct mbox_client cl;
> > >  	struct mbox_chan *chan;
> > >  	struct mbox_chan *chan_receiver;
> > > +	struct mbox_chan *chan_platform_receiver;
> > >  	struct scmi_chan_info *cinfo;
> > >  	struct scmi_shared_mem __iomem *shmem;  }; @@ -91,6 +93,8
> > @@ static
> > > bool mailbox_chan_available(struct device_node *of_node, int idx)
> > >   *		 for replies on the a2p channel. Set as zero if not present.
> > >   * @p2a_chan: A reference to the optional p2a channel.
> > >   *	      Set as zero if not present.
> > > + * @p2a_rx_chan: A reference to the optional p2a completion channel.
> > > + *	      Set as zero if not present.
> > >   *
> > >   * At first, validate the transport configuration as described in terms of
> > >   * 'mboxes' and 'shmem', then determin which mailbox channel indexes
> > > are @@ -98,8 +102,8 @@ static bool mailbox_chan_available(struct
> > device_node *of_node, int idx)
> > >   *
> > >   * Return: 0 on Success or error
> > >   */
> > > -static int mailbox_chan_validate(struct device *cdev,
> > > -				 int *a2p_rx_chan, int *p2a_chan)
> > > +static int mailbox_chan_validate(struct device *cdev, int *a2p_rx_chan,
> > > +				 int *p2a_chan, int *p2a_rx_chan)
> > >  {
> > >  	int num_mb, num_sh, ret = 0;
> > >  	struct device_node *np = cdev->of_node; @@ -109,8 +113,9 @@
> > static
> > > int mailbox_chan_validate(struct device *cdev,
> > >  	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb,
> > num_sh);
> > >
> > >  	/* Bail out if mboxes and shmem descriptors are inconsistent */
> > > -	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
> > > -	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh !=
> > 2)) {
> > > +	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 4 ||
> > > +	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)
> > ||
> > > +	    (num_mb == 4 && num_sh != 2)) {
> > >  		dev_warn(cdev,
> > >  			 "Invalid channel descriptor for '%s' - mbs:%d
> > shm:%d\n",
> > >  			 of_node_full_name(np), num_mb, num_sh); @@ -
> > 139,6 +144,7 @@
> > > static int mailbox_chan_validate(struct device *cdev,
> > >  		case 1:
> > >  			*a2p_rx_chan = 0;
> > >  			*p2a_chan = 0;
> > > +			*p2a_rx_chan = 0;
> > >  			break;
> > >  		case 2:
> > >  			if (num_sh == 2) {
> > > @@ -148,10 +154,17 @@ static int mailbox_chan_validate(struct device
> > *cdev,
> > >  				*a2p_rx_chan = 1;
> > >  				*p2a_chan = 0;
> > >  			}
> > > +			*p2a_rx_chan = 0;
> > >  			break;
> > >  		case 3:
> > >  			*a2p_rx_chan = 1;
> > >  			*p2a_chan = 2;
> > > +			*p2a_rx_chan = 0;
> > > +			break;
> > > +		case 4:
> > > +			*a2p_rx_chan = 1;
> > > +			*p2a_chan = 2;
> > > +			*p2a_rx_chan = 3;
> > >  			break;
> > >  		}
> > >  	}
> > > @@ -166,12 +179,12 @@ static int mailbox_chan_setup(struct
> > scmi_chan_info *cinfo, struct device *dev,
> > >  	struct device *cdev = cinfo->dev;
> > >  	struct scmi_mailbox *smbox;
> > >  	struct device_node *shmem;
> > > -	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
> > > +	int ret, a2p_rx_chan, p2a_chan, p2a_rx_chan, idx = tx ? 0 : 1;
> > >  	struct mbox_client *cl;
> > >  	resource_size_t size;
> > >  	struct resource res;
> > >
> > > -	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
> > > +	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan,
> > > +&p2a_rx_chan);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > @@ -229,6 +242,17 @@ static int mailbox_chan_setup(struct
> > scmi_chan_info *cinfo, struct device *dev,
> > >  		}
> > >  	}
> > >
> > > +	if (!tx && p2a_rx_chan) {
> > > +		smbox->chan_platform_receiver = mbox_request_channel(cl,
> > p2a_rx_chan);
> > > +		if (IS_ERR(smbox->chan_platform_receiver)) {
> > > +			ret = PTR_ERR(smbox->chan_platform_receiver);
> > > +			if (ret != -EPROBE_DEFER)
> > > +				dev_err(cdev, "failed to request SCMI P2A
> > Receiver mailbox\n");
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +
> > >  	cinfo->transport_info = smbox;
> > >  	smbox->cinfo = cinfo;
> > >
> > > @@ -243,9 +267,11 @@ static int mailbox_chan_free(int id, void *p, void
> > *data)
> > >  	if (smbox && !IS_ERR(smbox->chan)) {
> > >  		mbox_free_channel(smbox->chan);
> > >  		mbox_free_channel(smbox->chan_receiver);
> > > +		mbox_free_channel(smbox->chan_platform_receiver);
> > >  		cinfo->transport_info = NULL;
> > >  		smbox->chan = NULL;
> > >  		smbox->chan_receiver = NULL;
> > > +		smbox->chan_platform_receiver = NULL;
> > >  		smbox->cinfo = NULL;
> > >  	}
> > >
> > > @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct
> > > scmi_chan_info *cinfo,  static void mailbox_clear_channel(struct
> > > scmi_chan_info *cinfo)  {
> > >  	struct scmi_mailbox *smbox = cinfo->transport_info;
> > > +	struct device *cdev = cinfo->dev;
> > > +	struct mbox_chan *intr;
> > > +	int ret;
> > >
> > >  	shmem_clear_channel(smbox->shmem);
> > > +
> > > +	if (!shmem_channel_intr_enabled(smbox->shmem))
> > > +		return;
> > > +
> > > +	if (smbox->chan_platform_receiver)
> > > +		intr = smbox->chan_platform_receiver;
> > > +	else if (smbox->chan)
> > > +		intr = smbox->chan;
> > > +	else {
> > > +		dev_err(cdev, "Channel INTR wrongly set?\n");
> > > +		return;
> > > +	}
> > > +
> > > +	ret = mbox_send_message(intr, NULL);
> > > +	/* mbox_send_message returns non-negative value on success, so
> > reset */
> > > +	if (ret > 0)
> > > +		ret = 0;
> > > +
> > > +	mbox_client_txdone(intr, ret);
> > 
> > Looking at mbox_client_txdone() implementation this call is allowed only if
> > the txdone_method is TXDONE_BY_ACK, which in turn depend on the type of
> > underlying mbox controller that you are using AND/OR the SCMI client
> > configuration (knows_tx_done), so I dont think you can call this
> > unconditionally without the risk of hitting the related dev_err() in
> > mbox_client_txdone if the underlying mbox controller was instead supposed
> > to issue an mbox_chan_txdone() on its own.
> > 
> > IOW, if the mbox controller is operating in TXDONE_BY_IRQ mode or in
> > TXDONE_BY_POLL (and that would be the case if polling is used since the RX
> > channel does NOT set the client flag cl->knows_txdone to true, so
> > TXDONE_BY_ACK is not used as an override) this should hit the dev_err()
> > mentioned above...
> > 
> > dont you see any error ?
> 
> No error in my side.
> 
> > 
> > which mailbox controller do you use ?
> 
> drivers/mailbox/imx-mu.c. The tx channel is IMX_MU_TYPE_TXDB_V2
> > 
> > does your mailbox controller NOT set txdone_irq and NEITHER txdone_poll ?
> > (so defaulting to TDONE_BY_ACK in your case ?)
> 
> Use TXDONE_BY_ACK, no irq and no poll.
> 

Ok that explains the fact you dont see any errors in your setup.

> i.MX Message Unit(MU) supports many types, but for the i.MX SCMI MU,
> we are using GIR(General Interrupt request) to the other side. 
> And GIR will not trigger interrupt on the local side.
> That means Linux write GIRA will trigger interrupt on M33 side, but no
> interrupt in linux side. M33 write GIRB will trigger interrupt on
> Linux side, but no interrupt in M33 side.
> 

Yes the problem I mentioned will be visible ONLY if txdone_irq OR
txdone_poll are explicitly set AND the FW asked for a completion
interrupt on P2A; and I have seen similar issues if you have a controller
with a TxAck IRQ on A2P and you try to use it (which is not needed
really in SCMI).

Having said that, given that your case is the only case at the moment,
and you see no issues, I'd say to just merge your patch as it is now and
then I will eventually push a proper fix for both problems.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Sudeep Holla]

On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote:

There was some coding style error reported(unbalanced {}) which made me
look at the code again. I don't think we need to splat out error.

> @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
>  static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
>  {
>  	struct scmi_mailbox *smbox = cinfo->transport_info;
> +	struct device *cdev = cinfo->dev;
> +	struct mbox_chan *intr;
> +	int ret;
>  
>  	shmem_clear_channel(smbox->shmem);
> +
> +	if (!shmem_channel_intr_enabled(smbox->shmem))
> +		return;
> +
> +	if (smbox->chan_platform_receiver)
> +		intr = smbox->chan_platform_receiver;
> +	else if (smbox->chan)
> +		intr = smbox->chan;
> +	else {
> +		dev_err(cdev, "Channel INTR wrongly set?\n");
> +		return;
> +	}
> 

If it is OK I would like to fix it up with below change.

Regards,
Sudeep

-->8

diff --git i/drivers/firmware/arm_scmi/mailbox.c w/drivers/firmware/arm_scmi/mailbox.c
index adb69a6a0223..3bb3fba8f478 100644
--- i/drivers/firmware/arm_scmi/mailbox.c
+++ w/drivers/firmware/arm_scmi/mailbox.c
@@ -326,30 +326,25 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
 static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
 {
        struct scmi_mailbox *smbox = cinfo->transport_info;
-       struct device *cdev = cinfo->dev;
-       struct mbox_chan *intr;
+       struct mbox_chan *intr_chan = NULL;
        int ret;

        shmem_clear_channel(smbox->shmem);

-       if (!shmem_channel_intr_enabled(smbox->shmem))
-               return;
-
        if (smbox->chan_platform_receiver)
-               intr = smbox->chan_platform_receiver;
+               intr_chan = smbox->chan_platform_receiver;
        else if (smbox->chan)
-               intr = smbox->chan;
-       else {
-               dev_err(cdev, "Channel INTR wrongly set?\n");
+               intr_chan = smbox->chan;
+
+       if (!(intr_chan && shmem_channel_intr_enabled(smbox->shmem)))
                return;
-       }

-       ret = mbox_send_message(intr, NULL);
+       ret = mbox_send_message(intr_chan, NULL);
        /* mbox_send_message returns non-negative value on success, so reset */
        if (ret > 0)
                ret = 0;

-       mbox_client_txdone(intr, ret);
+       mbox_client_txdone(intr_chan, ret);
 }

 static bool

Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Cristian Marussi]

On Fri, Jun 14, 2024 at 10:19:42AM +0100, Sudeep Holla wrote:
> On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote:
> 
> There was some coding style error reported(unbalanced {}) which made me
> look at the code again. I don't think we need to splat out error.
> 
> > @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
> >  static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
> >  {
> >  	struct scmi_mailbox *smbox = cinfo->transport_info;
> > +	struct device *cdev = cinfo->dev;
> > +	struct mbox_chan *intr;
> > +	int ret;
> >  
> >  	shmem_clear_channel(smbox->shmem);
> > +
> > +	if (!shmem_channel_intr_enabled(smbox->shmem))
> > +		return;
> > +
> > +	if (smbox->chan_platform_receiver)
> > +		intr = smbox->chan_platform_receiver;
> > +	else if (smbox->chan)
> > +		intr = smbox->chan;
> > +	else {
> > +		dev_err(cdev, "Channel INTR wrongly set?\n");
> > +		return;
> > +	}
> > 
> 
> If it is OK I would like to fix it up with below change.
> 

Hi,

> Regards,
> Sudeep
> 
> -->8
> 
> diff --git i/drivers/firmware/arm_scmi/mailbox.c w/drivers/firmware/arm_scmi/mailbox.c
> index adb69a6a0223..3bb3fba8f478 100644
> --- i/drivers/firmware/arm_scmi/mailbox.c
> +++ w/drivers/firmware/arm_scmi/mailbox.c
> @@ -326,30 +326,25 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
>  static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
>  {
>         struct scmi_mailbox *smbox = cinfo->transport_info;
> -       struct device *cdev = cinfo->dev;
> -       struct mbox_chan *intr;
> +       struct mbox_chan *intr_chan = NULL;
>         int ret;
> 
>         shmem_clear_channel(smbox->shmem);
> 
> -       if (!shmem_channel_intr_enabled(smbox->shmem))
> -               return;
> -
>         if (smbox->chan_platform_receiver)
> -               intr = smbox->chan_platform_receiver;
> +               intr_chan = smbox->chan_platform_receiver;
>         else if (smbox->chan)
> -               intr = smbox->chan;
> -       else {
> -               dev_err(cdev, "Channel INTR wrongly set?\n");
> +               intr_chan = smbox->chan;
> +
> +       if (!(intr_chan && shmem_channel_intr_enabled(smbox->shmem)))
>                 return;
> -       }

Fine with dropping the dev_err() but is not this cumulative negated-if a
bit cryptic...also you can bail out early straight away as before when
platform has not required any P2A completion irq...I mean something like

	
       struct mbox_chan *intr_chan = NULL;

       shmem_clear_channel(smbox->shmem);
       if (!shmem_channel_intr_enabled(smbox->shmem))
		return;

       if (smbox->chan_platform_receiver)
		intr_chan = smbox->chan_platform_receiver;
       else if (smbox->chan)
		intr_chan = smbox->chan;

       if (!intr_chan)
	       return;

(or just a dangling else return;)


.. no strongs opinion here really, though.

Thanks,
Cristian

Re: [PATCH v2 2/2] firmware: arm_scmi: mailbox: support P2A channel completion

[Sudeep Holla]

On Fri, Jun 14, 2024 at 11:31:43AM +0100, Cristian Marussi wrote:
> On Fri, Jun 14, 2024 at 10:19:42AM +0100, Sudeep Holla wrote:
> > 
> > diff --git i/drivers/firmware/arm_scmi/mailbox.c w/drivers/firmware/arm_scmi/mailbox.c
> > index adb69a6a0223..3bb3fba8f478 100644
> > --- i/drivers/firmware/arm_scmi/mailbox.c
> > +++ w/drivers/firmware/arm_scmi/mailbox.c
> > @@ -326,30 +326,25 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
> >  static void mailbox_clear_channel(struct scmi_chan_info *cinfo)
> >  {
> >         struct scmi_mailbox *smbox = cinfo->transport_info;
> > -       struct device *cdev = cinfo->dev;
> > -       struct mbox_chan *intr;
> > +       struct mbox_chan *intr_chan = NULL;
> >         int ret;
> > 
> >         shmem_clear_channel(smbox->shmem);
> > 
> > -       if (!shmem_channel_intr_enabled(smbox->shmem))
> > -               return;
> > -
> >         if (smbox->chan_platform_receiver)
> > -               intr = smbox->chan_platform_receiver;
> > +               intr_chan = smbox->chan_platform_receiver;
> >         else if (smbox->chan)
> > -               intr = smbox->chan;
> > -       else {
> > -               dev_err(cdev, "Channel INTR wrongly set?\n");
> > +               intr_chan = smbox->chan;
> > +
> > +       if (!(intr_chan && shmem_channel_intr_enabled(smbox->shmem)))
> >                 return;
> > -       }
> 
> Fine with dropping the dev_err() but is not this cumulative negated-if a
> bit cryptic...also you can bail out early straight away as before when
> platform has not required any P2A completion irq...I mean something like
>

Agreed, updated now.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/2] firmware: arm_scmi: add notification completion channel

[Sudeep Holla]

On Fri, 10 May 2024 11:19:46 +0800, Peng Fan (OSS) wrote:
> Per spec:
> Completion interrupts This transport supports polling or interrupt driven
> modes of communication. In interrupt mode, when the callee completes
> processing a message, it raises an interrupt to the caller. Hardware
> support for completion interrupts is optional.
>
> i.MX95 SCMI firmware is fully interrupt driven, so Platform expects
> completion interrupt for Platform to Agent(P2A) notifictions.
>
> [...]

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

[1/2] dt-bindings: firmware: arm,scmi: Support notification completion channel
      https://git.kernel.org/sudeep.holla/c/d0398f51ffb3
[2/2] firmware: arm_scmi: mailbox: support P2A channel completion
      https://git.kernel.org/sudeep.holla/c/fa8b28ba22d9
--
Regards,
Sudeep