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
