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

[Anup Patel]

On Fri, Jun 23, 2023 at 6:19 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> [here, let me trim all of this nonsense...]
>
> On Fri, 23 Jun 2023 12:47:00 +0100,
> Anup Patel <apatel@xxxxxxxxxxxxxxxx> 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