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@xxxxxxx>
> 
> 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@xxxxxxx>
> ---
>  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