Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 09:05:37 +0100
Message-ID: <868rbfufn2.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Anup Patel <anup@brainfault.org>
Cc:     Anup Patel <apatel@ventanamicro.com>,
        Saravana Kannan <saravanak@google.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Atish Patra <atishp@atishpatra.org>,
        Andrew Jones <ajones@ventanamicro.com>,
        Sunil V L <sunilvl@ventanamicro.com>,
        Conor Dooley <conor@kernel.org>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v5 7/9] irqchip: Add RISC-V advanced PLIC driver
In-Reply-To: <CAAhSdy2sAaA_dmVCt9162kpw8-Ub1wjH_MNKxPOFN_VbW7M7vQ@mail.gmail.com>
References: <20230710094321.1378351-1-apatel@ventanamicro.com>
        <20230710094321.1378351-8-apatel@ventanamicro.com>
        <CAGETcx8kH8cJVdhcv5K4qNUo58godFZEBnOfTGKUUQ6VuUguvQ@mail.gmail.com>
        <86jzv2vpdb.wl-maz@kernel.org>
        <CAK9=C2ULxsXednwnoyzYKjQFpe_zBSZ4v0FqUYFnxK2TpbiMtg@mail.gmail.com>
        <86cz0uvcof.wl-maz@kernel.org>
        <CAAhSdy2sAaA_dmVCt9162kpw8-Ub1wjH_MNKxPOFN_VbW7M7vQ@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 14 Jul 2023 15:05:07 +0100,
Anup Patel <anup@brainfault.org> wrote:
>=20
> On Fri, Jul 14, 2023 at 7:05=E2=80=AFPM Marc Zyngier <maz@kernel.org> wro=
te:
> >
> > On Fri, 14 Jul 2023 10:35:34 +0100,
> > Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Fri, Jul 14, 2023 at 2:31=E2=80=AFPM Marc Zyngier <maz@kernel.org>=
 wrote:
> > > >
> > > > Anup,
> > > >
> > > > On Fri, 14 Jul 2023 00:56:22 +0100,
> > > > Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Mon, Jul 10, 2023 at 2:44=E2=80=AFAM Anup Patel <apatel@ventan=
amicro.com> wrote:
> > > > > >
> > > > > > The RISC-V advanced interrupt architecture (AIA) specification =
defines
> > > > > > a new interrupt controller for managing wired interrupts on a R=
ISC-V
> > > > > > platform. This new interrupt controller is referred to as advan=
ced
> > > > > > platform-level interrupt controller (APLIC) which can forward w=
ired
> > > > > > 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 RIS=
C-V
> > > > > > platforms.
> > > > > >
> > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > >
> > > > [...]
> > > >
> > > > > > +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 wh=
ich
> > > > > > +        *    provides a hint to the device driver core to prob=
e 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 |=3D FWNODE_FLAG_BEST_EFFORT;
> > > > >
> > > > > Please stop spamming us with broken patches. Already told you thi=
s 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 "discussio=
n"
> > > > 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 poi=
nt
> > > >   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.
>=20
> 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.

>=20
> >
> > 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.
>=20
> We can have IMSIC and APLIC-Direct-Mode as non-platform driver
> whereas APLIC-as-MSI-bridge will be a platform driver.
>=20
> Both APLIC-Direct-Mode and APLIC-as-MSI-bridge can share a large
> part of the current driver.
>=20
> >
> > > > 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 telli=
ng
> > > > 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*.
>=20
> Both APLIC and IMSIC are separate devices as defined by the AIA spec.
>=20
> 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)
>=20
> 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.

--=20
Without deviation from the norm, progress is not possible.