On Fri, Jul 14, 2023 at 7:05 PM Marc Zyngier [off-list ref] wrote:

On Fri, 14 Jul 2023 10:35:34 +0100,
Anup Patel [off-list ref] wrote:

quoted

On Fri, Jul 14, 2023 at 2:31 PM Marc Zyngier [off-list ref] wrote:

quoted

Anup,

On Fri, 14 Jul 2023 00:56:22 +0100,
Saravana Kannan [off-list ref] wrote:

quoted

On Mon, Jul 10, 2023 at 2:44 AM Anup Patel [off-list ref] wrote:

quoted

The RISC-V advanced interrupt architecture (AIA) specification defines
a new interrupt controller for managing wired interrupts on a RISC-V
platform. This new interrupt controller is referred to as advanced
platform-level interrupt controller (APLIC) which can forward wired
interrupts to CPUs (or HARTs) as local interrupts OR as message
signaled interrupts.
(For more details refer https://github.com/riscv/riscv-aia)

This patch adds an irqchip driver for RISC-V APLIC found on RISC-V
platforms.

Signed-off-by: Anup Patel <redacted>

[...]

quoted

quoted

+static int __init aplic_dt_init(struct device_node *node,
+                               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;

Please stop spamming us with broken patches. Already told you this is
not an option.

Nack.

What puzzles me here is that *no other arch* requires this sort of
hack. What is so special about the APLIC that it requires it? I see
nothing in this patch that even hints at it, despite the "discussion"
in the last round.

The rules are simple:

- either the APLIC is so fundamental to the system that it has to be
  initialised super early, much like the GIC on arm64, at which point
  it cannot be a platform device, and the story is pretty simple.

- or it isn't that fundamental, and it can be probed as a platform
  device using the dependency infrastructure that is already used by
  multiple other interrupt controller drivers, without any need to
  mess with internal flags. Again, this should be simple enough.

APLIC manages all wired interrupts whereas IMSIC manages all
MSIs. Both APLIC and IMSIC are fundamental devices which need
to be probed super early.

Now APLIC has two modes of operations:
1) Direct mode where there is no IMSIC in the system and APLIC
    directly injects interrupt to CPUs
2) MSI mode where IMSIC is present in the system and APLIC
    converts wired interrupts into MSIs

The APLIC driver added by this patch is a common driver for
both above modes.

Which it doesn't need to be. You are pointlessly making life difficult
for yourself, and everyone else. The MSI bridge behaviour has *zero*
reason to be the same driver as the main "I need it super early"
driver. They may be called the same, but they *are* different things
in the system.

They can share code, but they are fundamentally a different thing in
the system. And I guess this silly approach has other ramifications:
the IMSIC is also some early driver when it really doesn't need to be.
Who needs MSIs that early in the life of the system? I don't buy this
for even a second.

IMSIC also provides IPIs which are required super early so I think
we can't make IMSIC as a platform driver.

Frankly, this whole thing needs to be taken apart and rebuilt from the
ground up.

quoted

For #2, APLIC needs to be a platform device to create a device
MSI domain using platform_msi_create_device_domain() which
is why the APLIC driver is a platform driver.

You can't have your cake and eat it. If needed super early, and it
cannot be a platform driver. End of the story.

And to my earlier point: IMSIC and APLIC-as-MSI-bridge have no purpose
being early drivers. They must be platform drivers, and only that.

We can have IMSIC and APLIC-Direct-Mode as non-platform driver
whereas APLIC-as-MSI-bridge will be a platform driver.

Both APLIC-Direct-Mode and APLIC-as-MSI-bridge can share a large
part of the current driver.

quoted

quoted

If these rules don't apply to your stuff, please explain what is so
different. And I mean actually explain the issue. Which isn't telling
us "it doesn't work without it". Because as things stand, there is no
way I will even consider taking this ugly mix of probing methods.

Yes, I don't want this ugly FWNODE_FLAG_BEST_EFFORT hack
in this driver.

And yet you are hammering it even when told this is wrong.

quoted

I tried several things but setting the FWNODE_FLAG_BEST_EFFORT
flag is the only thing which works right now.

How about you take a step back and realise that the way you've
architected your drivers makes little sense? I don't think you have
tried *that*.

Both APLIC and IMSIC are separate devices as defined by the AIA spec.

There are three possible systems:
1) Systems with only APLIC (i.e. only wired interrupts)
2) Systems with only IMSIC (i.e. only MSIs)
3) Systems with both APLIC and IMSIC (i.e. both wired interrupts and MSIs)

To address the above, APLIC and IMSIC are separate drivers. I am okay
with splitting the APLIC driver into two separate drivers .

Regards,
Anup