Re: [PATCH v4 08/10] irqchip: Add RISC-V advanced PLIC driver

Anup Patel
2023-06-23T13:52:44Z

On Fri, Jun 23, 2023 at 6:19 PM Marc Zyngier <maz@kernel.org> wrote:
>
> [here, let me trim all of this nonsense...]
>
> On Fri, 23 Jun 2023 12:47:00 +0100,
> Anup Patel <apatel@ventanamicro.com> wrote:
> > > No. My previous email asking you to NOT use IRQCHIP_DECLARE() and
> > > instead use IRQCHIP_PLATFORM_DRIVER_BEGIN/END() macros.
> >
> > I tried IRQCHIP_PLATFORM_DRIVER_BEGIN/END() macros but these
> > macros are not suitable for APLIC driver because we need platform device
> > pointer in the APLIC probe() to create platform MSI device domain (refer,
> > platform_msi_create_device_domain()).
>
> Oh come on. How hard have you tried? Have you even looked at the other
> drivers in the tree to see how they solve this insurmountable problem
> with a *single* line of code?
>
> pdev = of_find_device_by_node(node);
>
> That's it.

Please see the below diff. I tried the same thing but still the APLIC does
not get probed without the FWNODE_FLAG_BEST_EFFORT flag. Please
note that the current APLIC driver works unmodified for both DT and ACPI
but using of_find_device_by_node() here breaks ACPI support.

diff --git a/drivers/irqchip/irq-riscv-aplic.c
b/drivers/irqchip/irq-riscv-aplic.c
index 1e710fdf5608..9ae9e7fb905f 100644
--- a/drivers/irqchip/irq-riscv-aplic.c
+++ b/drivers/irqchip/irq-riscv-aplic.c
@@ -17,6 +17,7 @@
#include <linux/irqdomain.h>
#include <linux/module.h>
#include <linux/msi.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/smp.h>

@@ -730,36 +731,12 @@ static int aplic_probe(struct platform_device *pdev)
return 0;
}

-static const struct of_device_id aplic_match[] = {
- { .compatible = "riscv,aplic" },
- {}
-};
-
-static struct platform_driver aplic_driver = {
- .driver = {
- .name = "riscv-aplic",
- .of_match_table = aplic_match,
- },
- .probe = aplic_probe,
-};
-builtin_platform_driver(aplic_driver);
-
-static int __init aplic_dt_init(struct device_node *node,
+static int __init aplic_of_init(struct device_node *dn,
struct device_node *parent)
{
- /*
- * The APLIC platform driver needs to be probed early
- * so for device tree:
- *
- * 1) Set the FWNODE_FLAG_BEST_EFFORT flag in fwnode which
- * provides a hint to the device driver core to probe the
- * platform driver early.
- * 2) Clear the OF_POPULATED flag in device_node because
- * of_irq_init() sets it which prevents creation of
- * platform device.
- */
- node->fwnode.flags |= FWNODE_FLAG_BEST_EFFORT;
- of_node_clear_flag(node, OF_POPULATED);
- return 0;
+ return aplic_probe(of_find_device_by_node(dn));
}
-IRQCHIP_DECLARE(riscv_aplic, "riscv,aplic", aplic_dt_init);
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(aplic)
+IRQCHIP_MATCH("riscv,aplic", aplic_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(aplic)

>
> > Further, I tried setting the "suppress_bind_attrs" flag in "struct
> > platform_driver aplic_driver" just like the
> > IRQCHIP_PLATFORM_DRIVER_END() macro but this did not work.
>
> I'm not sure how relevant this is to the conversation.

It's relevant because the only difference in the platform_driver
registered by IRQCHIP_PLATFORM_DRIVER_END() and
"struct platform_driver aplic_driver" is the "suppress_bind_attrs" flag.

Unfortunately, setting the "suppress_bind_attrs" flag does not
help as well.

>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

Regards,
Anup