From: Sunil V L <sunilvl@ventanamicro.com>

Add ACPI support for the RISC-V SBI message proxy (MPXY) based
mailbox driver.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/mailbox/riscv-sbi-mpxy-mbox.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/riscv-sbi-mpxy-mbox.c b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
index 129710f947ae..deb269a9a844 100644
--- a/drivers/mailbox/riscv-sbi-mpxy-mbox.c
+++ b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
@@ -5,9 +5,11 @@
  * Copyright (C) 2025 Ventana Micro Systems Inc.
  */
 
+#include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/errno.h>
 #include <linux/init.h>
+#include <linux/irqchip/riscv-imsic.h>
 #include <linux/mailbox_controller.h>
 #include <linux/mailbox/riscv-rpmi-message.h>
 #include <linux/mm.h>
@@ -779,6 +781,7 @@ static int mpxy_mbox_probe(struct platform_device *pdev)
 	u32 i, *channel_ids __free(kfree) = NULL;
 	struct device *dev = &pdev->dev;
 	struct mpxy_mbox_channel *mchan;
+	struct irq_domain *msi_domain;
 	struct mpxy_mbox *mbox;
 	int msi_idx, rc;
 
@@ -901,6 +904,8 @@ static int mpxy_mbox_probe(struct platform_device *pdev)
 
 	/* Set the MSI domain if not available */
 	if (!dev_get_msi_domain(dev)) {
+		struct fwnode_handle *fwnode = dev_fwnode(dev);
+
 		/*
 		 * The device MSI domain for OF devices is only set at the
 		 * time of populating/creating OF device. If the device MSI
@@ -908,8 +913,13 @@ static int mpxy_mbox_probe(struct platform_device *pdev)
 		 * then we need to set it explicitly before using any platform
 		 * MSI functions.
 		 */
-		if (dev_of_node(dev))
+		if (is_of_node(fwnode)) {
 			of_msi_configure(dev, dev_of_node(dev));
+		} else if (is_acpi_device_node(fwnode)) {
+			msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
+							      DOMAIN_BUS_PLATFORM_MSI);
+			dev_set_msi_domain(dev, msi_domain);
+		}
 	}
 
 	/* Setup MSIs for mailbox (if required) */
@@ -954,6 +964,13 @@ static int mpxy_mbox_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+#ifdef CONFIG_ACPI
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev)
+		acpi_dev_clear_dependencies(adev);
+#endif
+
 	dev_info(dev, "mailbox registered with %d channels\n",
 		 mbox->channel_count);
 	return 0;
@@ -973,10 +990,17 @@ static const struct of_device_id mpxy_mbox_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mpxy_mbox_of_match);
 
+static const struct acpi_device_id mpxy_mbox_acpi_match[] = {
+	{ "RSCV0005" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, mpxy_mbox_acpi_match);
+
 static struct platform_driver mpxy_mbox_driver = {
 	.driver = {
 		.name = "riscv-sbi-mpxy-mbox",
 		.of_match_table = mpxy_mbox_of_match,
+		.acpi_match_table = mpxy_mbox_acpi_match,
 	},
 	.probe = mpxy_mbox_probe,
 	.remove = mpxy_mbox_remove,
-- 
2.43.0

On Wed, Jul 02, 2025 at 10:43:42AM +0530, Anup Patel wrote:
> From: Sunil V L <sunilvl@ventanamicro.com>
> 
> Add ACPI support for the RISC-V SBI message proxy (MPXY) based
> mailbox driver.

...

> -		if (dev_of_node(dev))
> +		if (is_of_node(fwnode)) {
>  			of_msi_configure(dev, dev_of_node(dev));
> +		} else if (is_acpi_device_node(fwnode)) {
> +			msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
> +							      DOMAIN_BUS_PLATFORM_MSI);
> +			dev_set_msi_domain(dev, msi_domain);
> +		}

Actually you don't need to have the if-else-if if I am not mistaken.
The OF does almost the same as it's done in the second branch for ACPI case.
How many MSI parents this may have?

-- 
With Best Regards,
Andy Shevchenko

On Wed, Jul 02, 2025 at 03:28:45PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 10:43:42AM +0530, Anup Patel wrote:
> > From: Sunil V L <sunilvl@ventanamicro.com>
> > 
> > Add ACPI support for the RISC-V SBI message proxy (MPXY) based
> > mailbox driver.
> 
> ...
> 
> > -		if (dev_of_node(dev))
> > +		if (is_of_node(fwnode)) {
> >  			of_msi_configure(dev, dev_of_node(dev));
> > +		} else if (is_acpi_device_node(fwnode)) {
> > +			msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
> > +							      DOMAIN_BUS_PLATFORM_MSI);
> > +			dev_set_msi_domain(dev, msi_domain);
> > +		}
> 
> Actually you don't need to have the if-else-if if I am not mistaken.
> The OF does almost the same as it's done in the second branch for ACPI case.
> How many MSI parents this may have?
> 
OF already has a well defined interface to configure the MSI domain. The
mechanisms existing today are different for DT vs ACPI to find out the
fwnode of the MSI controller. So, it is done differently.

In RISC-V case at least, there will be only one MSI parent.

Thanks,
Sunil

On Thu, Jul 03, 2025 at 04:24:18PM +0530, Sunil V L wrote:
> On Wed, Jul 02, 2025 at 03:28:45PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 10:43:42AM +0530, Anup Patel wrote:

...

> > > -		if (dev_of_node(dev))
> > > +		if (is_of_node(fwnode)) {
> > >  			of_msi_configure(dev, dev_of_node(dev));
> > > +		} else if (is_acpi_device_node(fwnode)) {
> > > +			msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
> > > +							      DOMAIN_BUS_PLATFORM_MSI);
> > > +			dev_set_msi_domain(dev, msi_domain);
> > > +		}
> > 
> > Actually you don't need to have the if-else-if if I am not mistaken.
> > The OF does almost the same as it's done in the second branch for ACPI case.
> > How many MSI parents this may have?
> > 
> OF already has a well defined interface to configure the MSI domain. The
> mechanisms existing today are different for DT vs ACPI to find out the
> fwnode of the MSI controller. So, it is done differently.

I don't see how. The only difference I see is that OF iterates over all listed
parents, if any, ACPI tries only one.

So, perhaps it's a time to have a common API somewhere for this to be agnostic?
Something like fwnode_msi_configure() in somewhere of IRQ MSI core?

> In RISC-V case at least, there will be only one MSI parent.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Jul 3, 2025 at 7:24 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jul 03, 2025 at 04:24:18PM +0530, Sunil V L wrote:
> > On Wed, Jul 02, 2025 at 03:28:45PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 02, 2025 at 10:43:42AM +0530, Anup Patel wrote:
>
> ...
>
> > > > -         if (dev_of_node(dev))
> > > > +         if (is_of_node(fwnode)) {
> > > >                   of_msi_configure(dev, dev_of_node(dev));
> > > > +         } else if (is_acpi_device_node(fwnode)) {
> > > > +                 msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
> > > > +                                                       DOMAIN_BUS_PLATFORM_MSI);
> > > > +                 dev_set_msi_domain(dev, msi_domain);
> > > > +         }
> > >
> > > Actually you don't need to have the if-else-if if I am not mistaken.
> > > The OF does almost the same as it's done in the second branch for ACPI case.
> > > How many MSI parents this may have?
> > >
> > OF already has a well defined interface to configure the MSI domain. The
> > mechanisms existing today are different for DT vs ACPI to find out the
> > fwnode of the MSI controller. So, it is done differently.
>
> I don't see how. The only difference I see is that OF iterates over all listed
> parents, if any, ACPI tries only one.
>
> So, perhaps it's a time to have a common API somewhere for this to be agnostic?
> Something like fwnode_msi_configure() in somewhere of IRQ MSI core?
>

There is an issue/gap in the DD framework which is being work-around
here. This issue manifest mostly in RISC-V land because in RISC-V both
MSI controller driver and drivers using MSI are regular platform drivers
while the probe ordering is ensured by dev_link support of DD framework
or the frameworks (like ACPI) creating the device.

As-per this issue, when platform devices (DT or ACPI) are created the
MSI domain instance is not available and hence set to NULL. The MSI
domain instance is only available after MSI controller driver is probed
so currently we explicitly do of_msi_configure() or dev_set_msi_domain()
in the driver using MSI as a work-around. Adding a common
fwnode_msi_configure() is only going to be an improvement to the
existing work-around which we should not have in the first place
hence not the right approach IMO.

In the long run, we need a clean fix for the above issue in the DD
framework such that platform drivers using MSI don't have to explicitly
do of_msi_configure() or dev_set_msi_domain().

Regards,
Anup

On Thu, Jul 03, 2025 at 07:56:52PM +0530, Anup Patel wrote:
> On Thu, Jul 3, 2025 at 7:24 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jul 03, 2025 at 04:24:18PM +0530, Sunil V L wrote:
> > > On Wed, Jul 02, 2025 at 03:28:45PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jul 02, 2025 at 10:43:42AM +0530, Anup Patel wrote:

...

> > > > > -         if (dev_of_node(dev))
> > > > > +         if (is_of_node(fwnode)) {
> > > > >                   of_msi_configure(dev, dev_of_node(dev));
> > > > > +         } else if (is_acpi_device_node(fwnode)) {
> > > > > +                 msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev),
> > > > > +                                                       DOMAIN_BUS_PLATFORM_MSI);
> > > > > +                 dev_set_msi_domain(dev, msi_domain);
> > > > > +         }
> > > >
> > > > Actually you don't need to have the if-else-if if I am not mistaken.
> > > > The OF does almost the same as it's done in the second branch for ACPI case.
> > > > How many MSI parents this may have?
> > > >
> > > OF already has a well defined interface to configure the MSI domain. The
> > > mechanisms existing today are different for DT vs ACPI to find out the
> > > fwnode of the MSI controller. So, it is done differently.
> >
> > I don't see how. The only difference I see is that OF iterates over all listed
> > parents, if any, ACPI tries only one.
> >
> > So, perhaps it's a time to have a common API somewhere for this to be agnostic?
> > Something like fwnode_msi_configure() in somewhere of IRQ MSI core?
> 
> There is an issue/gap in the DD framework which is being work-around
> here. This issue manifest mostly in RISC-V land because in RISC-V both
> MSI controller driver and drivers using MSI are regular platform drivers
> while the probe ordering is ensured by dev_link support of DD framework
> or the frameworks (like ACPI) creating the device.
> 
> As-per this issue, when platform devices (DT or ACPI) are created the
> MSI domain instance is not available and hence set to NULL. The MSI
> domain instance is only available after MSI controller driver is probed
> so currently we explicitly do of_msi_configure() or dev_set_msi_domain()
> in the driver using MSI as a work-around. Adding a common
> fwnode_msi_configure() is only going to be an improvement to the
> existing work-around which we should not have in the first place
> hence not the right approach IMO.
> 
> In the long run, we need a clean fix for the above issue in the DD
> framework such that platform drivers using MSI don't have to explicitly
> do of_msi_configure() or dev_set_msi_domain().

I see, thanks a lot for this explanation. Can you add a summary as a comment on
top of the if-else-if in case it's not there already?

-- 
With Best Regards,
Andy Shevchenko