Hi Marc,
On Sat, Jun 25, 2022 at 12:52 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sat, 25 Jun 2022 10:54:44 +0100,
> "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > Thank you for the review.
> >
> > On Sat, Jun 25, 2022 at 10:03 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Fri, 24 Jun 2022 19:03:11 +0100,
> > > Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > The Renesas RZ/Five SoC has a RISC-V AX45MP AndesCore with NCEPLIC100. The
> > > > NCEPLIC100 supports both edge-triggered and level-triggered interrupts. In
> > > > case of edge-triggered interrupts NCEPLIC100 ignores the next interrupt
> > > > edge until the previous completion message has been received and
> > > > NCEPLIC100 doesn't support pending interrupt counter, hence losing the
> > > > interrupts if not acknowledged in time.
> > > >
> > > > So the workaround for edge-triggered interrupts to be handled correctly
> > > > and without losing is that it needs to be acknowledged first and then
> > > > handler must be run so that we don't miss on the next edge-triggered
> > > > interrupt.
> > > >
> > > > This patch adds a new compatible string for Renesas RZ/Five SoC and
> > > > changes the chained interrupt haindler for RZ/Five SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > ---
> > > > RFC-->v1:
> > > > * Fixed review comments pointed by Geert
> > > > * Dropped handle_fasteoi_ack_irq support as for the PLIC we need to
> > > > claim the interrupt by reading the register and then acknowledge it.
> > >
> > > Why? This is exactly what the fasteoi_ack flow gives you, and your
> > > initial patch was much better that this one in that regard.
> > >
> > > > * Add a new chained handler for RZ/Five SoC.
> > > > ---
> > > > drivers/irqchip/irq-sifive-plic.c | 95 +++++++++++++++++++++++++++++--
> > > > 1 file changed, 91 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > > index 173446cc9204..f53dff49e122 100644
> > > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > > @@ -60,10 +60,13 @@
> > > > #define PLIC_DISABLE_THRESHOLD 0x7
> > > > #define PLIC_ENABLE_THRESHOLD 0
> > > >
> > > > +#define PLIC_INTERRUPT_CELL_SIZE2 2
> > > > +
> > > > struct plic_priv {
> > > > struct cpumask lmask;
> > > > struct irq_domain *irqdomain;
> > > > void __iomem *regs;
> > > > + u32 intsize;
> > > > };
> > > >
> > > > struct plic_handler {
> > > > @@ -163,7 +166,7 @@ static int plic_set_affinity(struct irq_data *d,
> > > > }
> > > > #endif
> > > >
> > > > -static void plic_irq_eoi(struct irq_data *d)
> > > > +static void plic_irq_ack(struct irq_data *d)
> > > > {
> > > > struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > > >
> > > > @@ -176,6 +179,23 @@ static void plic_irq_eoi(struct irq_data *d)
> > > > }
> > > > }
> > > >
> > > > +static void plic_irq_eoi(struct irq_data *d)
> > > > +{
> > > > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > > > + unsigned int irq = irq_find_mapping(handler->priv->irqdomain, d->hwirq);
> > > > +
> > > > + /*
> > > > + * For Renesas RZ/Five (R9A07G043) SoC if the interrupt type is
> > > > + * IRQ_TYPE_EDGE_RISING we have already acknowledged it in the
> > > > + * handler.
> > > > + */
> > > > + if (handler->priv->intsize == PLIC_INTERRUPT_CELL_SIZE2 &&
> > >
> > > This costs you an extra two reads on the fast path, which is an
> > > unnecessary overhead for existing systems that do not suffer from this
> > > problem. Consider turning it into a static key.
> > >
> > Sorry, by static key what did you mean?
>
> See Documentation/staging/static-keys.rst
>
Thanks for the pointer.
> >
> > > Also, blindly renaming plic_irq_eoi() to ack() is extremely
> > > confusing. I really think you should have your own callbacks instead
> > > of making a mess of the existing one.
> > >
> > Ok will do.
> >
> > > > + (irq_get_trigger_type(irq) & IRQ_TYPE_EDGE_RISING))
> > > > + return;
> > > > +
> > > > + plic_irq_ack(d);
> > > > +}
> > > > +
> > > > static const struct irq_chip plic_chip = {
> > > > .name = "SiFive PLIC",
> > > > .irq_mask = plic_irq_mask,
> > > > @@ -198,6 +218,19 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > > > return 0;
> > > > }
> > > >
> > > > +static int plic_irq_domain_translate(struct irq_domain *d,
> > > > + struct irq_fwspec *fwspec,
> > > > + unsigned long *hwirq,
> > > > + unsigned int *type)
> > > > +{
> > > > + struct plic_priv *priv = d->host_data;
> > > > +
> > > > + if (priv->intsize == PLIC_INTERRUPT_CELL_SIZE2)
> > > > + return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> > > > +
> > > > + return irq_domain_translate_onecell(d, fwspec, hwirq, type);
> > > > +}
> > > > +
> > > > static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > > > unsigned int nr_irqs, void *arg)
> > > > {
> > > > @@ -206,7 +239,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > > > unsigned int type;
> > > > struct irq_fwspec *fwspec = arg;
> > > >
> > > > - ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> > > > + ret = plic_irq_domain_translate(domain, fwspec, &hwirq, &type);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > @@ -220,11 +253,55 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > > > }
> > > >
> > > > static const struct irq_domain_ops plic_irqdomain_ops = {
> > > > - .translate = irq_domain_translate_onecell,
> > > > + .translate = plic_irq_domain_translate,
> > > > .alloc = plic_irq_domain_alloc,
> > > > .free = irq_domain_free_irqs_top,
> > > > };
> > > >
> > > > +/*
> > > > + * On Renesas RZ/Five (R9A07G043) SoC IRQ_TYPE_LEVEL_HIGH and
> > > > + * IRQ_TYPE_EDGE_RISING interrupts are the supported interrupt types.
> > > > + * If the global interrupt source was edge-triggered NCEPLIC100 (PLIC
> > > > + * core on Renesas RZ/Five SoC) ignores next edge interrupts until the
> > > > + * previous completion message is received. NCEPLIC100 on Renesas RZ/Five
> > > > + * SoC doesn't stack the pending interrupts so in case there is a delay
> > > > + * in handling the IRQ_TYPE_EDGE_RISING interrupt we lose the subsequent
> > > > + * interrupts. The workaround for IRQ_TYPE_EDGE_RISING interrupt is to
> > > > + * first we have to claim the interrupt by reading the claim register,
> > > > + * then quickly issue an complete interrupt by writing the source ID
> > > > + * register back to the claim register and then later run the handler.
> > > > + */
> > > > +static void renesas_rzfive_plic_handle_irq(struct irq_desc *desc)
> > > > +{
> > > > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > > + void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
> > > > + irq_hw_number_t hwirq;
> > > > + unsigned int irq;
> > > > + int err;
> > > > +
> > > > + WARN_ON_ONCE(!handler->present);
> > > > +
> > > > + chained_irq_enter(chip, desc);
> > > > +
> > > > + while ((hwirq = readl(claim))) {
> > > > + irq = irq_find_mapping(handler->priv->irqdomain, hwirq);
> > > > + if (!irq) {
> > > > + pr_warn_ratelimited("can't find mapping for hwirq %lu\n", hwirq);
> > > > + break;
> > > > + }
> > > > +
> > > > + if (irq_get_trigger_type(irq) & IRQ_TYPE_EDGE_RISING)
> > > > + plic_irq_ack(irq_get_irq_data(irq));
> > > > +
> > > > + err = generic_handle_irq(irq);
> > >
> > > No. We're not going back to this sort of constructs. Using the
> > > fasteoi_ack flow should work if properly configured. Also, looking up
> > > the interrupt *four* times in various tables/trees is not exactly the
> > > sort of things I want to see for a driver written in this century.
> > >
> > > Please explain why fasteoi_ack doesn't work. It really should work out
> > > of the box (I asked you to look into debugfs last time, but didn't ear
> > > anything from you on the subject). And if something is broken, let's
> > > fix it. But none of the above, please.
> > >
> > Handling an interrupt is a two-step process [0] first you claim the
> > interrupt by reading the claim register, then you complete the
> > interrupt by writing that source ID back to the same claim register.
>
> I'm familiar with the architecture.
>
> >
> > Now if we go with fasteoi_ack flow this wont fit as we are first
> > writing into the claim register (Interrupt completion) and then in the
> > chained handler we are reading the claim register (claim the
> > interrupt) and then run the handler (which my RFC patch did).
> >
> > With this patch I make sure we follow [0] for LEVEL interrupt and and
> > for EDGE we first claim then issue interrupt completion if EDGE
> > interrupt and then later run the handler (due to the core issue).
> >
> > Let me know if my understanding is wrong here.
>
> You are just reinventing the wheel we are already have, except that
> yours is a bit square ;-). What really should happen is that the
> set_type method should set the correct flow depending on the trigger
> of the interrupt, and *never* have to check the configuration on the
> handling path.
>
A Bit lost here..
We have the below chained handler:
static void plic_handle_irq(struct irq_desc *desc)
{
struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
struct irq_chip *chip = irq_desc_get_chip(desc);
void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
irq_hw_number_t hwirq;
WARN_ON_ONCE(!handler->present);
chained_irq_enter(chip, desc);
while ((hwirq = readl(claim))) {
int err = generic_handle_domain_irq(handler->priv->irqdomain,
hwirq);
if (unlikely(err))
pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
hwirq);
}
chained_irq_exit(chip, desc);
}
static void plic_irq_eoi(struct irq_data *d)
{
struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
if (irqd_irq_masked(d)) {
plic_irq_unmask(d);
writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
plic_irq_mask(d);
} else {
writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
}
}
Where it's claiming -> handling interrupt -> interrupt completion in
eoi which is according to architecture.
Now with fasteoi_ack flow If I introduce the below ack callback to
issue interrupt completion.
static void plic_irq_ack(struct irq_data *d)
{
struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
if (irqd_irq_masked(d)) {
plic_irq_unmask(d);
writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
plic_irq_mask(d);
} else {
writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
}
}
Here we are issuing an interrupt completion first, and later in the
handler plic_handle_irq() we are claiming the interrupt by reading
the claim register. With this we are not following [0].
Do you think this flow is OK (interrupt completion -> Interrupt claim
-> handle IRQ)?
[0] https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-flow
Cheers,
Prabhakar
Attachment:
interrupt.png
Description: PNG image
