Hi Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 24 February 2025 13:20
> Subject: Re: [PATCH v4 6/7] dmaengine: sh: rz-dmac: Add RZ/V2H(P) support
>
> Hi Fabrizio,
>
> On Thu, 20 Feb 2025 at 16:01, Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > The DMAC IP found on the Renesas RZ/V2H(P) family of SoCs is
> > similar to the version found on the Renesas RZ/G2L family of
> > SoCs, but there are some differences:
> > * It only uses one register area
> > * It only uses one clock
> > * It only uses one reset
> > * Instead of using MID/IRD it uses REQ NO/ACK NO
> > * It is connected to the Interrupt Control Unit (ICU)
> > * On the RZ/G2L there is only 1 DMAC, on the RZ/V2H(P) there are 5
> >
> > Add specific support for the Renesas RZ/V2H(P) family of SoC by
> > tackling the aforementioned differences.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > ---
> > v3->v4:
> > * Fixed an issue with mid_rid/req_no/ack_no initialization
>
> Thanks for your patch!
>
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> > @@ -14,6 +14,7 @@
> > #include <linux/dmaengine.h>
> > #include <linux/interrupt.h>
> > #include <linux/iopoll.h>
> > +#include <linux/irqchip/irq-renesas-rzv2h.h>
> > #include <linux/list.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -73,7 +74,6 @@ struct rz_dmac_chan {
> >
> > u32 chcfg;
> > u32 chctrl;
> > - int mid_rid;
> >
> > struct list_head ld_free;
> > struct list_head ld_queue;
> > @@ -85,20 +85,36 @@ struct rz_dmac_chan {
> > struct rz_lmdesc *tail;
> > dma_addr_t base_dma;
> > } lmdesc;
> > +
> > + union {
> > + int mid_rid;
> > + struct {
> > + u16 req_no;
> > + u8 ack_no;
> > + };
> > + };
>
> Please add comments (with/without ICU), so the casual reader knows
> the meaning of the union.
> Note that I am no longer convinced we need a union, as REQ_NO seems
> to be just the new name for MID/RID.
Since I am dropping ACK NO (for now), I think I can reuse mid_rid for V2H,
and document its use for V2H and non V2H platforms.
No need for a union anymore, let's postpone that to when we have a
valid use case for ACK NO.
>
>
> > };
> >
> > #define to_rz_dmac_chan(c) container_of(c, struct rz_dmac_chan, vc.chan)
> >
> > +struct rz_dmac_icu {
> > + struct platform_device *pdev;
> > + u8 dmac_index;
> > +};
> > +
> > struct rz_dmac {
> > struct dma_device engine;
> > struct device *dev;
> > struct reset_control *rstc;
> > + struct rz_dmac_icu icu;
> > void __iomem *base;
> > void __iomem *ext_base;
> >
> > unsigned int n_channels;
> > struct rz_dmac_chan *channels;
> >
> > + bool has_icu;
> > +
> > DECLARE_BITMAP(modules, 1024);
> > };
> >
> > @@ -167,6 +183,23 @@ struct rz_dmac {
> > #define RZ_DMAC_MAX_CHANNELS 16
> > #define DMAC_NR_LMDESC 64
> >
> > +/* RZ/V2H ICU related */
> > +#define RZV2H_REQ_NO_MASK GENMASK(9, 0)
>
> FTR, this is identical to MID_RID_MASK.
>
> > +#define RZV2H_ACK_NO_MASK GENMASK(16, 10)
>
> This is a new field.
>
> > +#define RZV2H_HIEN_MASK BIT(17)
> > +#define RZV2H_LVL_MASK BIT(18)
> > +#define RZV2H_AM_MASK GENMASK(21, 19)
> > +#define RZV2H_TM_MASK BIT(22)
> > +#define RZV2H_EXTRACT_REQ_NO(x) FIELD_GET(RZV2H_REQ_NO_MASK, (x))
> > +#define RZV2H_EXTRACT_ACK_NO(x) FIELD_GET(RZV2H_ACK_NO_MASK, (x))
> > +#define RZVH2_EXTRACT_CHCFG(x) ((FIELD_GET(RZV2H_HIEN_MASK, (x)) << 5) | \
> > + (FIELD_GET(RZV2H_LVL_MASK, (x)) << 6) | \
> > + (FIELD_GET(RZV2H_AM_MASK, (x)) << 8) | \
> > + (FIELD_GET(RZV2H_TM_MASK, (x)) << 22))
>
> If the new field would be moved up in the configuration word,
> the above would become identical to the existing CHCFG handling.
Yes. I originally thought about differentiating V2H from other platforms from the
start, as we may need to add other parameters as well (e.g. TEND No.), but we may
never get around to do that, therefore no point in differentiating them now
(especially considering I am dropping ACK No. for now).
>
> > +
> > +#define RZV2H_MAX_DMAC_INDEX 4
> > +#define RZV2H_ICU_PROPERTY "renesas,icu"
>
> Please don't obfuscate DT property handling, and drop this define.
Will do.
>
> > +
> > /*
> > * -----------------------------------------------------------------------------
> > * Device access
> > @@ -324,7 +357,15 @@ static void rz_dmac_prepare_desc_for_memcpy(struct rz_dmac_chan *channel)
> > lmdesc->chext = 0;
> > lmdesc->header = HEADER_LV;
> >
> > - rz_dmac_set_dmars_register(dmac, channel->index, 0);
> > + if (!dmac->has_icu) {
> > + rz_dmac_set_dmars_register(dmac, channel->index, 0);
> > + } else {
> > + rzv2h_icu_register_dma_req_ack(dmac->icu.pdev,
> > + dmac->icu.dmac_index,
> > + channel->index,
> > + RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
> > + RZV2H_ICU_DMAC_ACK_NO_DEFAULT);
> > + }
>
> If you do have both branches of an if-statement, please drop the
> negation from the test to improve readability (everywhere):
>
> if (dmac->has_icu) {
> ...
> } else {
> ...
> }
Will do
>
> >
> > channel->chcfg = chcfg;
> > channel->chctrl = CHCTRL_STG | CHCTRL_SETEN;
>
> > @@ -452,9 +501,15 @@ static void rz_dmac_free_chan_resources(struct dma_chan *chan)
> > list_splice_tail_init(&channel->ld_active, &channel->ld_free);
> > list_splice_tail_init(&channel->ld_queue, &channel->ld_free);
> >
> > - if (channel->mid_rid >= 0) {
> > - clear_bit(channel->mid_rid, dmac->modules);
> > - channel->mid_rid = -EINVAL;
> > + if (!dmac->has_icu) {
> > + if (channel->mid_rid >= 0) {
> > + clear_bit(channel->mid_rid, dmac->modules);
> > + channel->mid_rid = -EINVAL;
> > + }
> > + } else {
> > + clear_bit(channel->req_no, dmac->modules);
> > + channel->req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT;
> > + channel->ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;
> > }
>
> Without a union, both branches would be almost the same...
Indeed.
>
> >
> > spin_unlock_irqrestore(&channel->vc.lock, flags);
>
> > @@ -727,13 +790,30 @@ static bool rz_dmac_chan_filter(struct dma_chan *chan, void *arg)
> > struct rz_dmac *dmac = to_rz_dmac(chan->device);
> > struct of_phandle_args *dma_spec = arg;
> > u32 ch_cfg;
> > + u16 req_no;
> > +
> > + if (!dmac->has_icu) {
> > + channel->mid_rid = dma_spec->args[0] & MID_RID_MASK;
>
> So mid_rid would fit in a short, just like req_no (ignoring the latter
> is unsigned, which could be changed).
Yes. I am probably going to leave this alone in the next version of the series.
>
> > + ch_cfg = (dma_spec->args[0] & CHCFG_MASK) >> 10;
> > + channel->chcfg = CHCFG_FILL_TM(ch_cfg) | CHCFG_FILL_AM(ch_cfg) |
> > + CHCFG_FILL_LVL(ch_cfg) | CHCFG_FILL_HIEN(ch_cfg);
> > +
> > + return !test_and_set_bit(channel->mid_rid, dmac->modules);
>
> Please don't return early, but use an else branch for the ICU case,
> to show symmetry.
If I use `mid_rid` for both V2H and other platforms rz_dmac_chan_filter() should
be identical for both cases (if I also take out the check).
>
> > + }
> > +
> > + req_no = RZV2H_EXTRACT_REQ_NO(dma_spec->args[0]);
> > + if (req_no >= RZV2H_ICU_DMAC_REQ_NO_MIN_FIX_OUTPUT)
> > + return false;
>
> Do you need this check?
I will take it out.
>
> > +
> > + channel->req_no = req_no;
> > +
> > + channel->ack_no = RZV2H_EXTRACT_ACK_NO(dma_spec->args[0]);
> > + if (channel->ack_no >= RZV2H_ICU_DMAC_ACK_NO_MIN_FIX_OUTPUT)
> > + channel->ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;
>
> Do you need this check?
I am dropping ACK No.
>
> > - channel->mid_rid = dma_spec->args[0] & MID_RID_MASK;
> > - ch_cfg = (dma_spec->args[0] & CHCFG_MASK) >> 10;
> > - channel->chcfg = CHCFG_FILL_TM(ch_cfg) | CHCFG_FILL_AM(ch_cfg) |
> > - CHCFG_FILL_LVL(ch_cfg) | CHCFG_FILL_HIEN(ch_cfg);
> > + channel->chcfg = RZVH2_EXTRACT_CHCFG(dma_spec->args[0]);
> >
> > - return !test_and_set_bit(channel->mid_rid, dmac->modules);
> > + return !test_and_set_bit(channel->req_no, dmac->modules);
>
> Without a union, both branches would be almost the same...
Indeed.
>
> > }
> >
> > static struct dma_chan *rz_dmac_of_xlate(struct of_phandle_args *dma_spec,
> > @@ -768,7 +848,12 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
> > int ret;
> >
> > channel->index = index;
> > - channel->mid_rid = -EINVAL;
> > + if (!dmac->has_icu) {
> > + channel->mid_rid = -EINVAL;
> > + } else {
> > + channel->req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT;
> > + channel->ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;
> > + }
>
> Without a union, both branches would be almost the same...
Indeed.
>
> >
> > /* Request the channel interrupt. */
> > scnprintf(pdev_irqname, sizeof(pdev_irqname), "ch%u", index);
> > @@ -824,6 +909,41 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
> > return 0;
> > }
> >
> > +static int rz_dmac_parse_of_icu(struct device *dev, struct rz_dmac *dmac)
> > +{
> > + struct device_node *icu_np, *np = dev->of_node;
> > + struct of_phandle_args args;
> > + uint32_t dmac_index;
> > + int ret;
> > +
> > + ret = of_parse_phandle_with_fixed_args(np, RZV2H_ICU_PROPERTY, 1, 0, &args);
> > + if (ret)
> > + return ret;
> > +
> > + icu_np = args.np;
> > + dmac_index = args.args[0];
> > +
> > + if (dmac_index > RZV2H_MAX_DMAC_INDEX) {
> > + dev_err(dev, "DMAC index %u invalid.\n", dmac_index);
> > + ret = -EINVAL;
> > + goto free_icu_np;
> > + }
> > +
> > + dmac->icu.pdev = of_find_device_by_node(icu_np);
> > + if (!dmac->icu.pdev) {
> > + dev_err(dev, "ICU device not found.\n");
> > + ret = -ENODEV;
> > + goto free_icu_np;
> > + }
> > +
> > + dmac->icu.dmac_index = dmac_index;
> > +
> > +free_icu_np:
> > + of_node_put(icu_np);
> > +
> > + return ret;
> > +}
> > +
> > static int rz_dmac_parse_of(struct device *dev, struct rz_dmac *dmac)
> > {
> > struct device_node *np = dev->of_node;
> > @@ -840,6 +960,10 @@ static int rz_dmac_parse_of(struct device *dev, struct rz_dmac *dmac)
> > return -EINVAL;
> > }
> >
> > + dmac->has_icu = of_property_present(np, RZV2H_ICU_PROPERTY);
>
> Doesn't of_parse_phandle_with_fixed_args() in rz_dmac_parse_of_icu()
> return -ENOENT if the property is not present, so you don't have to
> check for presence here?
I'll rework accordingly.
>
> > + if (dmac->has_icu)
> > + return rz_dmac_parse_of_icu(dev, dmac);
> > +
> > return 0;
> > }
> >
>
> > @@ -991,9 +1117,13 @@ static void rz_dmac_remove(struct platform_device *pdev)
> > reset_control_assert(dmac->rstc);
> > pm_runtime_put(&pdev->dev);
> > pm_runtime_disable(&pdev->dev);
> > +
> > + if (dmac->has_icu)
>
> No need to check for a NULL pointer.
Right! I'll take it out.
Thanks!
Fab
>
> > + platform_device_put(dmac->icu.pdev);
> > }
> >
> > static const struct of_device_id of_rz_dmac_match[] = {
> > + { .compatible = "renesas,r9a09g057-dmac", },
> > { .compatible = "renesas,rz-dmac", },
> > { /* Sentinel */ }
> > };
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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
