Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 26 Jan 2022 10:46:57 +0000
Message-ID: <87fspa7p0e.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Anup Patel <apatel@ventanamicro.com>
Cc:     Anup Patel <anup@brainfault.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Atish Patra <atishp@atishpatra.org>,
        Alistair Francis <Alistair.Francis@wdc.com>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/6] irqchip/riscv-intc: Set intc domain as the default host
In-Reply-To: <CAK9=C2Xi60XqzPxTOO+gT3B+-1v4i0QT-9nkhN7hLFX9SchQng@mail.gmail.com>
References: <20220125054217.383482-1-apatel@ventanamicro.com>
        <20220125054217.383482-3-apatel@ventanamicro.com>
        <87lez37k8h.wl-maz@kernel.org>
        <CAAhSdy0NrB4Q-mYtMH_HCtbfm5OXi-cxXhiu1AxKdCkTv4cskQ@mail.gmail.com>
        <87ilu67tvw.wl-maz@kernel.org>
        <CAK9=C2Xi60XqzPxTOO+gT3B+-1v4i0QT-9nkhN7hLFX9SchQng@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/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 26 Jan 2022 10:12:25 +0000,
Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Wed, Jan 26, 2022 at 2:31 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 26 Jan 2022 03:16:55 +0000,
> > Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Tue, 25 Jan 2022 05:42:13 +0000,
> > > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > > >
> > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver,
> > > > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a
> > > > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default
> > > > > host so that these drivers can directly create local interrupt mapping
> > > > > using standardized local interrupt numbers
> > > > >
> > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > > ---
> > > > >  drivers/clocksource/timer-riscv.c | 17 +----------------
> > > > >  drivers/irqchip/irq-riscv-intc.c  |  9 +++++++++
> > > > >  2 files changed, 10 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > > > > index 1767f8bf2013..dd6916ae6365 100644
> > > > > --- a/drivers/clocksource/timer-riscv.c
> > > > > +++ b/drivers/clocksource/timer-riscv.c
> > > > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
> > > > >  static int __init riscv_timer_init_dt(struct device_node *n)
> > > > >  {
> > > > >       int cpuid, hartid, error;
> > > > > -     struct device_node *child;
> > > > > -     struct irq_domain *domain;
> > > > >
> > > > >       hartid = riscv_of_processor_hartid(n);
> > > > >       if (hartid < 0) {
> > > > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> > > > >       if (cpuid != smp_processor_id())
> > > > >               return 0;
> > > > >
> > > > > -     domain = NULL;
> > > > > -     child = of_get_compatible_child(n, "riscv,cpu-intc");
> > > > > -     if (!child) {
> > > > > -             pr_err("Failed to find INTC node [%pOF]\n", n);
> > > > > -             return -ENODEV;
> > > > > -     }
> > > > > -     domain = irq_find_host(child);
> > > > > -     of_node_put(child);
> > > > > -     if (!domain) {
> > > > > -             pr_err("Failed to find IRQ domain for node [%pOF]\n", n);
> > > > > -             return -ENODEV;
> > > > > -     }
> > > > > -
> > > > > -     riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
> > > > > +     riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER);
> > > > >       if (!riscv_clock_event_irq) {
> > > > >               pr_err("Failed to map timer interrupt for node [%pOF]\n", n);
> > > > >               return -ENODEV;
> > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > > > > index b65bd8878d4f..9f0a7a8a5c4d 100644
> > > > > --- a/drivers/irqchip/irq-riscv-intc.c
> > > > > +++ b/drivers/irqchip/irq-riscv-intc.c
> > > > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node,
> > > > >               return rc;
> > > > >       }
> > > > >
> > > > > +     /*
> > > > > +      * Make INTC as the default domain which will allow drivers
> > > > > +      * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI
> > > > > +      * driver, RISC-V timer driver, RISC-V PMU driver, etc) can
> > > > > +      * directly create local interrupt mapping using standardized
> > > > > +      * local interrupt numbers.
> > > > > +      */
> > > > > +     irq_set_default_host(intc_domain);
> > > >
> > > > No, please. This really is a bad idea. This sort of catch-all have
> > > > constantly proven to be a nuisance, because they discard all the
> > > > topology information. Eventually, you realise that you need to know
> > > > where this is coming from, but it really is too late.
> > > >
> > > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do
> > > > this.
> > >
> > > In absence of INTC as the default domain, currently we have various
> > > drivers looking up INTC irq_domain from DT (or ACPI). This is quite a
> > > bit of duplicate code across various drivers.
> > >
> > > How about having a irq_domain lookup routine (riscv_intc_find_irq_domain())
> > > exported by the RISC-V INTC driver or arch/riscv ?
> > > OR
> > > Do you have an alternative suggestion ?
> >
> > But *why* don't you provide an interrupt controller node for DT? I
> > really don't think that's outlandish to require.
> 
> Historically, all RISC-V SBI related drivers never had any DT/ACPI
> node because we can always query/discover the SBI functionality
> at runtime.
> 
> The mechanism to query/discover SBI IPI, Timer and PMU is
> through SBI base functions. Also, local interrupts used by these
> drivers are specified by the RISC-V specification. This means having
> a DT/ACPI node for these drivers doesn't provide any info.
> 
> We will be having KVM RISC-V AIA support in future which will not
> have a DT/ACPI node as well because this can be discovered as a
> CPU capability and the local interrupt to be used is specified by the
> RISC-V hypervisor specification.
> 
> >
> > For ACPI, we already have an interface that allows a fwnode to be
> > registered (acpi_set_irq_model) and interrupts mapped
> > (acpi_register_gsi).
> 
> The ACPI specification being proposed for RISC-V does not have
> any details for SBI IPI, Timer, and PMU for the same reasons
> mentioned above.

Neither does it on the other architectures.

And yet we are able to synthesise fwnodes and use the whole of the
infrastructure as intended without having to resort to this crap that
was only introduced to cope with 20 year old PPC board files.

Only dead architectures are using irq_set_default_host().

> 
> >
> > You should already have all the required tools you need.
> 
> Are you okay if arch/riscv exports riscv_intc_find_irq_domain() ?
> OR
> Maybe export riscv_intc_find_irq_domain() from INTC driver ?

Neither. That's just papering over the core problem.

Either you start creating fwnodes out of thin air, which is what we do
for both x86 and arm64 when using ACPI, or you add support for SBI (or
whatever new spec the RISC-V people come up with) as a provider of
fwnodes.

Anything else looks like a pretty bad regression.

Thanks,

	M.

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