On Fri, 14 Jul 2023 15:05:07 +0100, Anup Patel <anup@xxxxxxxxxxxxxx> wrote: > > On Fri, Jul 14, 2023 at 7:05 PM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > > On Fri, 14 Jul 2023 10:35:34 +0100, > > Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote: > > > > > > On Fri, Jul 14, 2023 at 2:31 PM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > > > > > > Anup, > > > > > > > > On Fri, 14 Jul 2023 00:56:22 +0100, > > > > Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Jul 10, 2023 at 2:44 AM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > 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 <apatel@xxxxxxxxxxxxxxxx> > > > > > > > > [...] > > > > > > > > > > +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. Then split this part further. Just because the architecture lumps two completely unrelated concepts together doesn't mean we need to follow the same organisation. > > > > > Frankly, this whole thing needs to be taken apart and rebuilt from the > > ground up. > > > > > 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. > > > > > > > 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. > > > > > 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) How is that possible? Are you saying that even things like timers are firing as 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 . Again, we don't have to follow the split established by the architecture. Instead, we should follow what is *functionally correct* for the kernel. If we were to write Risc-V-OS, that'd be an acceptable solution. But this is Linux, and the constraints are different. My take on this discussion is that we should have: - Direct-mode APLIC + IPI support as a an early irqchip driver - MSI-bridge APLIC + MSI support as platform driver Yes, these will likely share most of their code. But at least the split will be manageable, and will avoid ugly hacks. M. -- Without deviation from the norm, progress is not possible.
