Re: [PATCH v2] mailbox: omap-mailbox: Check for pending msgs only when mbox is exclusive

[Andrew Davis <afd@ti.com>]

On 11/3/25 2:11 PM, Beleswar Padhi wrote:
> On TI K3 devices, the mailbox resides in the Always-On power domain
> (LPSC_main_alwayson) and is shared among multiple processors. The
> mailbox is not solely exclusive to Linux.
> 
> Currently, the suspend path checks all FIFO queues for pending messages
> and blocks suspend if any are present. This behavior is unnecessary for
> K3 devices, since some of the FIFOs are used for RTOS<->RTOS
> communication and are independent of Linux.
> 
> For FIFOs used in Linux<->RTOS communication, any pending message would
> trigger an interrupt, which naturally prevents suspend from completing.
> Hence, there is no need for the mailbox driver to explicitly check for
> pending messages on K3 platforms.
> 
> Introduce a device match flag to indicate whether the mailbox instance
> is exclusive to Linux, and skip the pending message check for
> non-exclusive instances (such as in K3).
> 
> Fixes: a49f991e740f ("arm64: dts: ti: k3-am62-verdin: Add missing cfg for TI IPC Firmware")
> Closes: https://lore.kernel.org/all/sid7gtg5vay5qgicsl6smnzwg5mnneoa35cempt5ddwjvedaio@hzsgcx6oo74l/
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>
> Cc: Hiago De Franco <hiago.franco@toradex.com>
> Please help in testing the patch on Toradex platforms.
> 
> Testing Done:
> 1. Tested Boot across all TI K3 EVM/SK boards.
> 2. Tested IPC on all TI K3 J7* EVM/SK boards (& AM62x SK).
> 3. Tested mbox driver probe & device boot on AM57x-evm (OMAP5 based).
> 4. Tested that the patch generates no new warnings/errors.
> 
> Changes since v1:
> 1. Use device_get_match_data() in probe and store result for re-use.
> 
> Link to v1:
> https://lore.kernel.org/all/20251103075920.2611642-1-b-padhi@ti.com/
> 
> Changes since RFC:
> 1. Skip checking pending messages instead of flushing
> them explicitly for K3 devices.
> 
> Link to RFC Version:
> https://lore.kernel.org/all/20251022102015.1345696-1-b-padhi@ti.com/
> 
>   drivers/mailbox/omap-mailbox.c | 35 +++++++++++++++++++---------------
>   1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
> index 680243751d62..17fe6545875d 100644
> --- a/drivers/mailbox/omap-mailbox.c
> +++ b/drivers/mailbox/omap-mailbox.c
> @@ -68,6 +68,7 @@ struct omap_mbox_fifo {
>   
>   struct omap_mbox_match_data {
>   	u32 intr_type;
> +	bool is_exclusive;
>   };
>   
>   struct omap_mbox_device {
> @@ -78,6 +79,7 @@ struct omap_mbox_device {
>   	u32 num_users;
>   	u32 num_fifos;
>   	u32 intr_type;
> +	const struct omap_mbox_match_data *mbox_data;

Not a fan of this, you could have stored just the relevant
flag from the match data here in the instance data, not a
pointer to the whole const struct. The issue being now you
have the same info stored in two places, `intr_type` in the
match data above, and intr_type on the line above that.

Looking more into it, the use of `intr_type` doesn't need to be
stored in each mbox's instance data either, but that is an existing
issue. For now I don't want to hold up an otherwise good bugfix
over that, but we will want to come back here and clean some of
this up next cycle, for now,

Reviewed-by: Andrew Davis <afd@ti.com>

>   };
>   
>   struct omap_mbox {
> @@ -341,11 +343,13 @@ static int omap_mbox_suspend(struct device *dev)
>   	if (pm_runtime_status_suspended(dev))
>   		return 0;
>   
> -	for (fifo = 0; fifo < mdev->num_fifos; fifo++) {
> -		if (mbox_read_reg(mdev, MAILBOX_MSGSTATUS(fifo))) {
> -			dev_err(mdev->dev, "fifo %d has unexpected unread messages\n",
> -				fifo);
> -			return -EBUSY;
> +	if (mdev->mbox_data->is_exclusive) {
> +		for (fifo = 0; fifo < mdev->num_fifos; fifo++) {
> +			if (mbox_read_reg(mdev, MAILBOX_MSGSTATUS(fifo))) {
> +				dev_err(mdev->dev, "fifo %d has unexpected unread messages\n",
> +					fifo);
> +				return -EBUSY;
> +			}
>   		}
>   	}
>   
> @@ -378,8 +382,9 @@ static const struct dev_pm_ops omap_mbox_pm_ops = {
>   	SET_SYSTEM_SLEEP_PM_OPS(omap_mbox_suspend, omap_mbox_resume)
>   };
>   
> -static const struct omap_mbox_match_data omap2_data = { MBOX_INTR_CFG_TYPE1 };
> -static const struct omap_mbox_match_data omap4_data = { MBOX_INTR_CFG_TYPE2 };
> +static const struct omap_mbox_match_data omap2_data = { MBOX_INTR_CFG_TYPE1, true };
> +static const struct omap_mbox_match_data omap4_data = { MBOX_INTR_CFG_TYPE2, true };
> +static const struct omap_mbox_match_data am654_data = { MBOX_INTR_CFG_TYPE2, false };
>   
>   static const struct of_device_id omap_mailbox_of_match[] = {
>   	{
> @@ -396,11 +401,11 @@ static const struct of_device_id omap_mailbox_of_match[] = {
>   	},
>   	{
>   		.compatible	= "ti,am654-mailbox",
> -		.data		= &omap4_data,
> +		.data		= &am654_data,
>   	},
>   	{
>   		.compatible	= "ti,am64-mailbox",
> -		.data		= &omap4_data,
> +		.data		= &am654_data,
>   	},
>   	{
>   		/* end */
> @@ -449,7 +454,6 @@ static int omap_mbox_probe(struct platform_device *pdev)
>   	struct omap_mbox_fifo *fifo;
>   	struct device_node *node = pdev->dev.of_node;
>   	struct device_node *child;
> -	const struct omap_mbox_match_data *match_data;
>   	struct mbox_controller *controller;
>   	u32 intr_type, info_count;
>   	u32 num_users, num_fifos;
> @@ -462,11 +466,6 @@ static int omap_mbox_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   	}
>   
> -	match_data = of_device_get_match_data(&pdev->dev);
> -	if (!match_data)
> -		return -ENODEV;
> -	intr_type = match_data->intr_type;
> -
>   	if (of_property_read_u32(node, "ti,mbox-num-users", &num_users))
>   		return -ENODEV;
>   
> @@ -483,6 +482,12 @@ static int omap_mbox_probe(struct platform_device *pdev)
>   	if (!mdev)
>   		return -ENOMEM;
>   
> +	mdev->mbox_data = device_get_match_data(&pdev->dev);
> +	if (!mdev->mbox_data)
> +		return -ENODEV;
> +
> +	intr_type = mdev->mbox_data->intr_type;
> +
>   	mdev->mbox_base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(mdev->mbox_base))
>   		return PTR_ERR(mdev->mbox_base);