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.
> };
>
> #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.
> +
> +#define RZV2H_MAX_DMAC_INDEX 4
> +#define RZV2H_ICU_PROPERTY "renesas,icu"
Please don't obfuscate DT property handling, and drop this define.
> +
> /*
> * -----------------------------------------------------------------------------
> * 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 {
...
}
>
> 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...
>
> 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).
> + 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.
> + }
> +
> + 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?
> +
> + 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?
> - 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...
> }
>
> 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...
>
> /* 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?
> + 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.
> + 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
