RE: [PATCH v4 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack()

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Geert,

Thanks for your feedback!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 24 February 2025 12:59
> Subject: Re: [PATCH v4 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack()
> 
> Hi Fabrizio,
> 
> On Thu, 20 Feb 2025 at 16:01, Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > On the Renesas RZ/V2H(P) family of SoCs, DMAC IPs are connected
> > to the Interrupt Control Unit (ICU).
> > For DMA transfers, a request number and an ack number must be
> > registered with the ICU, which means that the DMAC driver has
> > to be able to instruct the ICU driver with the registration of
> > such ids.
> >
> > Export rzv2h_icu_register_dma_req_ack() so that the DMA driver
> > can register both ids in one go.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> > v3->v4:
> > * No change.
> > v2->v3:
> > * Replaced rzv2h_icu_register_dma_req_ack with
> >   rzv2h_icu_register_dma_req_ack() in changelog.
> > * Added dummy for rzv2h_icu_register_dma_req_ack().
> > * Added Rb Thomas.
> 
> Thanks for the update!
> 
> > --- a/drivers/irqchip/irq-renesas-rzv2h.c
> > +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> 
> > @@ -94,6 +110,45 @@ struct rzv2h_icu_priv {
> >         raw_spinlock_t                  lock;
> >  };
> >
> > +void rzv2h_icu_register_dma_req_ack(struct platform_device *icu_dev, u8 dmac_index, u8
> dmac_channel,
> > +                                   u16 req_no, u8 ack_no)
> > +{
> > +       struct rzv2h_icu_priv *priv = platform_get_drvdata(icu_dev);
> > +       u32 icu_dmackselk, dmaack, dmaack_mask;
> > +       u32 icu_dmksely, dmareq, dmareq_mask;
> > +       u8 k, field_no;
> > +       u8 y, upper;
> > +
> > +       if (req_no >= RZV2H_ICU_DMAC_REQ_NO_MIN_FIX_OUTPUT)
> > +               req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT;
> 
> What is the purpose of this check?

This is mainly to standardize the values for fixed outputs.

> The hardware register field size is 10 bits, so I think it is better
> to just limit it to 0x3ff.
> Checking for RZV2H_ICU_DMAC_REQ_NO_MIN_FIX_OUTPUT means you will have to
> update this check when a new SoC supports higher values than 0x1b5.

Or I can take this check out, the caller deals with it anyways.

> 
> > +
> > +       if (ack_no >= RZV2H_ICU_DMAC_ACK_NO_MIN_FIX_OUTPUT)
> > +               ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;
> 
> What is the purpose of this check?
> There are only 23 DMACKSELk registers, so using
> RZV2H_ICU_DMAC_ACK_NO_DEFAULT = 0x7f will write beyond the last
> register below. And drivers/dma/sh/rz-dmac.c does call this
> function with req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT and ack_no =
> RZV2H_ICU_DMAC_ACK_NO_DEFAULT...

We don’t have everything we need to test ack_no properly in upstream,
yet, that's probably why this wasn't spotted. I am going to drop the
ack_no bits for now, and add them later on once I can test it properly.

I'll rework this patch to deal only with req_no only, and I'll update
the driver patch accordingly.

Thanks!

Fab

> 
> > +
> > +       y = dmac_channel / 2;
> > +       upper = dmac_channel % 2;
> > +
> > +       dmareq = ICU_DMAC_PREP_DMAREQ(req_no, upper);
> > +       dmareq_mask = ICU_DMAC_DMAREQ_MASK(upper);
> > +
> > +       k  = ack_no / 4;
> > +       field_no = ack_no % 4;
> > +
> > +       dmaack_mask = ICU_DMAC_DACK_SEL_MASK(field_no);
> > +       dmaack = ICU_DMAC_PREP_DACK_SEL(ack_no, field_no);
> > +
> > +       guard(raw_spinlock_irqsave)(&priv->lock);
> > +
> > +       icu_dmksely = readl(priv->base + ICU_DMkSELy(dmac_index, y));
> > +       icu_dmksely = (icu_dmksely & ~dmareq_mask) | dmareq;
> > +       writel(icu_dmksely, priv->base + ICU_DMkSELy(dmac_index, y));
> > +
> > +       icu_dmackselk = readl(priv->base + ICU_DMACKSELk(k));
> > +       icu_dmackselk = (icu_dmackselk & ~dmaack_mask) | dmaack;
> > +       writel(icu_dmackselk, priv->base + ICU_DMACKSELk(k));
> > +}
> > +EXPORT_SYMBOL_GPL(rzv2h_icu_register_dma_req_ack);
> > +
> >  static inline struct rzv2h_icu_priv *irq_data_to_priv(struct irq_data *data)
> >  {
> >         return data->domain->host_data;
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds