Hi Marc,
Thank you for the review.
On Sun, Jun 26, 2022 at 9:56 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sun, 26 Jun 2022 01:43:26 +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 adds
> > support to change interrupt flow based on the interrupt type. It also
> > implements irq_ack and irq_set_type callbacks.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Implemented IRQ flow as suggested by Marc
> >
> > 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.
> > * Add a new chained handler for RZ/Five SoC.
> > ---
> > drivers/irqchip/Kconfig | 1 +
> > drivers/irqchip/irq-sifive-plic.c | 73 ++++++++++++++++++++++++++++++-
> > 2 files changed, 72 insertions(+), 2 deletions(-)
>
> [...]
>
> >
> > +static int plic_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > +
> > + if (handler->priv->of_data != RENESAS_R9A07G043_PLIC)
> > + return 0;
> > +
> > + switch (type) {
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + irq_set_chip_handler_name_locked(d, &renesas_rzfive_edge_plic_chip,
> > + handle_fasteoi_ack_irq,
> > + "Edge");
> > + break;
> > +
> > + case IRQ_TYPE_EDGE_RISING:
> > + irq_set_chip_handler_name_locked(d, &plic_chip,
> > + handle_fasteoi_irq,
> > + "Level");
> > + break;
>
> Really? Have you even tested this?
>
Ouch my bad, while rebasing I did swap this up!
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hwirq)
> > {
> > @@ -198,6 +248,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->of_data == RENESAS_R9A07G043_PLIC)
> > + 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 +269,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,7 +283,7 @@ 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,
> > };
> > @@ -293,6 +356,11 @@ static int __init plic_init(struct device_node *node,
> > if (!priv)
> > return -ENOMEM;
> >
> > + if (of_device_is_compatible(node, "renesas,r9a07g043-plic")) {
> > + priv->of_data = RENESAS_R9A07G043_PLIC;
> > + plic_chip.name = "Renesas RZ/Five PLIC";
>
> NAK. The irq_chip structure isn't the place for platform marketing.
> This is way too long anyway (and same for the edge version), and you
> even sent me a patch to make that structure const...
>
My bad will drop this.
Cheers,
Prabhakar
