From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f176.google.com (mail-qk1-f176.google.com [209.85.222.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 516EF27816C for ; Tue, 30 Jun 2026 21:00:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782853249; cv=none; b=AVN5SpT7X/jqZGh+xlOm0lo4w9YLkIu3IlGxnaDdO9kSiueuyX5RLYiKnRfLo862KRPZc8X21K7EztXhcC2oxpgztYAUozzwYYrhGJfgsL2egeVhzWs4XiEuNhyFzDJD1egJn0q/rUzdPvy5CFaIn8SFDCt7+Q8bqxM9iMirnck= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782853249; c=relaxed/simple; bh=/AtII+nQ/mtZfVzSGI6/dSsXyw92XdhX00/4p2rR+2o=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EhKRTbUFsD5MiED8OgSAw2fshDvSts992ZIe41ssgMivtNvAUXoavnpdxYniTunIzLf08TeVd/2vkCXBDQesUT7hxBU6G6oxfezIe66uyppvjnZzcFdG4d6s8+pc1l6k7EbnU+IZ3uOWKOD4DzfHdRxFnuXoLb/3przjIz1GFos= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=nCfdYVyP; arc=none smtp.client-ip=209.85.222.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="nCfdYVyP" Received: by mail-qk1-f176.google.com with SMTP id af79cd13be357-92e533aacf2so151107385a.2 for ; Tue, 30 Jun 2026 14:00:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782853246; x=1783458046; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=U9TexMUsTMAbS8it8WaFvlrCqT4rsw90jOJV1JjeB4E=; b=nCfdYVyPgOgLQS1/PmWy1dONQsG5PUrJlG9pkMGzZb1dDt6AM3k7RGr+h4wrnsxmPN tyLw2y+DziHn34hL5/Rqfrz69izq58q4X5JYjtgK7xBfg2sq43wv4h6yhahKjy4FJ/XP Nr6PBfh7KD5PdDbw4J5PWWSwbfgaI52PB+DR3V+x/xq1E+VAtwTnZaWtHVMDCoNgOg1G WHHjrZgVUgJ1Cz01yl3zrzIcricn7S37rSOnkmxsfV2fEv4Je2L56Bv1flw+1Fn+w1ui fYob26O71xHA/UkTja+xfyJFmgM2lfrl98mcrNOMcb75v/t0XUE42brvVcZNRhOBC3Ta JS+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782853246; x=1783458046; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=U9TexMUsTMAbS8it8WaFvlrCqT4rsw90jOJV1JjeB4E=; b=SZ3s78WQd+dyazkvhahpjaJ7kU4bT5stbEEK5Th96q/wWEuWe5RiP2Yyn0N5USfq4V zGGPET5z5Tb7cGw0IJkmITB9uaXkY+vOtfDEb4Zh4N3cO9+RxZkao+URhJNF1VffHT2K L87NC+ubGJd+6tMICxFIqvQzDb/bh5dudvxhj0k26L3VjFrCs14D6wm4UeWGSr/zo21a LHeaXwwLVyi8rEaPFdoEe4AplHJKHudHMrmgcFvaXhB/44btzMtc/Ye0ivzeEHD2xDCN HXUudky5N0zwck2hvHBvJsdq5YKDH5vI9mTHixBXD1srUqa65tffG0IR4zPNutJnP5fJ FeHw== X-Forwarded-Encrypted: i=1; AFNElJ/bs1WF/jo9491lEPSxZVe7tm6NqwYOdIrXyKVX0IAM6Nc7kHaqQ2mlzwg335XjNTnE5lKDIgB9/T0BWuU=@vger.kernel.org X-Gm-Message-State: AOJu0Yx97VdccoWuEmvV6ZquozAL24NrCKBmbbrlEyT2EYwF4bMWRIFK zdL4XfftB3XurhhYpaENGtGM2Q4ONuP2UoawFsBRPJRBq6auhyO9Pt68 X-Gm-Gg: AfdE7ckscIB8VKfsZEioqsGHH+6FoMzCviU14SVsjkpXMzfhvURB3rwTJ1Ipv29o6kW gsCNqid8h0nHusBtvdGgayckolhTS3MwGceMbNUIkHBfiCfg9ye1r6SCDp8F7ldtbcx/AE2vMN/ zbsuTu1tLRhTU0p9/A4T65mQPGZH40CX85aKGg9qeKgfgPv/gxzSVDGGD/ngyAZDOOI22W8z+G4 7gawuIXxZNy/XH+LWZBXSLgA9IdU+H1mH01FXVCTUH0N0HUXNjQ9tMmNyNxsYLD8s2BNRu9IxeU ppE3TtGUYDNJULwp3Zirg+DaNq82suHmBn8ndp/c/UFa1rLipEd4/RAs9E7mrAOgZhVOH5wI2tV bdM387/iMTgHmhHIlztRT1O0oFV5d+SHg2MLPqQ45HmuPLO+FOWJOrWFssqsv3sdgioOKhNus0/ C3ZXf0zjr30hsqTw== X-Received: by 2002:a05:620a:4805:b0:92a:a400:df70 with SMTP id af79cd13be357-92e62673174mr898729285a.47.1782853245056; Tue, 30 Jun 2026 14:00:45 -0700 (PDT) Received: from b82beb281c41 ([138.28.231.64]) by smtp.gmail.com with ESMTPSA id af79cd13be357-92e6218fa27sm327885585a.16.2026.06.30.14.00.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jun 2026 14:00:44 -0700 (PDT) Date: Tue, 30 Jun 2026 21:00:41 +0000 From: Yuanshen Cao To: Andre Przywara Cc: conor+dt@kernel.org, mripard@kernel.org, krzk+dt@kernel.org, robh@kernel.org, samuel@sholland.org, wens@kernel.org, jernej.skrabec@gmail.com, Frank.Li@kernel.org, vkoul@kernel.org, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Frank Li Subject: Re: [PATCH v3 1/5] dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling Message-ID: References: <20260622-sun60i-a733-dma-v3-0-f697ef296cbc@gmail.com> <20260622-sun60i-a733-dma-v3-1-f697ef296cbc@gmail.com> <20260629003505.18f0053d@ryzen.lan> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260629003505.18f0053d@ryzen.lan> On Mon, Jun 29, 2026 at 12:35:05AM +0200, Andre Przywara wrote: > On Mon, 22 Jun 2026 01:36:23 +0000 > Yuanshen Cao wrote: > > Hi, > > first, many thanks for sending this, also for structuring the changes > nicely, so that they remain reviewable! > > > Refactor to support the Allwinner A733 DMA controller. Currently, the > > `sun6i-dma` driver has several functions related to interrupt handling > > (reading/writing interrupt enable and status registers) and register > > dumping that are hardcoded. > > > > To support the A733, which has different register layouts and interrupt > > handling logic, these functions are being moved into the > > `sun6i_dma_config` structure as function pointers. > > So I see that this driver already makes use of per-device function > pointer, though personally I don't like this approach very much, as it > decreases the readability, and suggests significant differences between > the SoC generations that are not really there: each function just reads > or write an MMIO register, it's just the offset that differs. Yes, I considered to do this but since the original mainline code and BSP code all use function-pointer so I impelement this the same way. > > So I think it's better to express the differences through data > entries in the config struct, for the IRQ enable/stat functions I think > this should be something like this: > > struct sun6i_dma_config { > ... > u32 irq_stride; > u32 irq_en_offset; > u32 irq_stat_offset; > ... > }; > > - irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg)); > + irq_val = readl(sdev->base + sdev->cfg->irq_en_offset + irq_reg * sdev->cfg->irq_stride); > > the existing configs set .stride to 0x04, and .en_offset to 0x0, the > A733 later uses .stride = 0x40 and .en_offset = 0x134. > Maybe we still move that now longish line into a helper function, but > not a config specific one. > > I think that's more readable, and avoids unnecessary redirections and > potential pipeline stalls. > > dump_com_regs is a different story, since the two instances of that > function are significantly different. > > What do you think? I am okay to change this one but should we do the same for the other function pointers as well? Just to keep them aligned. Let me know what do you think. > > This allows the > > driver to use a polymorphic approach where the specific implementation > > is determined by the hardware configuration assigned during device > > probing. > > > > Changes: > > - Added function pointers to `struct sun6i_dma_config` for: > > By the way: the preferred style to list changes in commit messages in > imperative mood [1], not in past tense. Think about you ask the > code base what to change: > > Add function pointers to ... > Implement generic functions ... Thanks for the advice! I'll make sure to follow them in the future. Best, Alex > Cheers, > Andre > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n94 > > > - `dump_com_regs` > > - `read_irq_en` > > - `write_irq_en` > > - `read_irq_stat` > > - `write_irq_stat` > > - Implemented generic `sun6i_read/write_irq_*` functions for existing > > hardware. > > - Added a macro and updated existing `sun6i_dma_config` instances (A31, > > A23, H3, A64, A100, H6, V3S) to use these new function pointers. > > > > Reviewed-by: Frank Li > > Signed-off-by: Yuanshen Cao > > --- > > drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 45 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index a9a254dbf8cb..ef3052c4ab36 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -138,6 +138,11 @@ struct sun6i_dma_config { > > void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst); > > void (*set_drq)(u32 *p_cfg, s8 src_drq, s8 dst_drq); > > void (*set_mode)(u32 *p_cfg, s8 src_mode, s8 dst_mode); > > + void (*dump_com_regs)(struct sun6i_dma_dev *sdev); > > + u32 (*read_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg); > > + void (*write_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val); > > + u32 (*read_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg); > > + void (*write_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status); > > u32 src_burst_lengths; > > u32 dst_burst_lengths; > > u32 src_addr_widths; > > @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode) > > DMA_CHAN_CFG_DST_MODE_H6(dst_mode); > > } > > > > +static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg) > > +{ > > + return readl(sdev->base + DMA_IRQ_EN(irq_reg)); > > +} > > + > > +static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val) > > +{ > > + writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg)); > > +} > > + > > +static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg) > > +{ > > + return readl(sdev->base + DMA_IRQ_STAT(irq_reg)); > > +} > > + > > +static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status) > > +{ > > + writel(status, sdev->base + DMA_IRQ_STAT(irq_reg)); > > +} > > + > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > { > > struct sun6i_desc *txd = pchan->desc; > > @@ -460,16 +485,16 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan) > > > > vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE; > > > > - irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg)); > > + irq_val = sdev->cfg->read_irq_en(sdev, irq_reg); > > irq_val &= ~((DMA_IRQ_HALF | DMA_IRQ_PKG | DMA_IRQ_QUEUE) << > > (irq_offset * DMA_IRQ_CHAN_WIDTH)); > > irq_val |= vchan->irq_type << (irq_offset * DMA_IRQ_CHAN_WIDTH); > > - writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg)); > > + sdev->cfg->write_irq_en(sdev, irq_reg, irq_val); > > > > writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR); > > writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE); > > > > - sun6i_dma_dump_com_regs(sdev); > > + sdev->cfg->dump_com_regs(sdev); > > sun6i_dma_dump_chan_regs(sdev, pchan); > > > > return 0; > > @@ -549,14 +574,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id) > > u32 status; > > > > for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) { > > - status = readl(sdev->base + DMA_IRQ_STAT(i)); > > + status = sdev->cfg->read_irq_stat(sdev, i); > > if (!status) > > continue; > > > > dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n", > > str_high_low(i), status); > > > > - writel(status, sdev->base + DMA_IRQ_STAT(i)); > > + sdev->cfg->write_irq_stat(sdev, i, status); > > > > for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) { > > pchan = sdev->pchans + j; > > @@ -1101,6 +1126,13 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev) > > } > > } > > > > +#define SUN6I_DMA_IRQ_A31_COMMON_OPS \ > > + .dump_com_regs = sun6i_dma_dump_com_regs, \ > > + .read_irq_en = sun6i_read_irq_en, \ > > + .write_irq_en = sun6i_write_irq_en, \ > > + .read_irq_stat = sun6i_read_irq_stat, \ > > + .write_irq_stat = sun6i_write_irq_stat, > > + > > /* > > * For A31: > > * > > @@ -1132,6 +1164,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = { > > .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1155,6 +1188,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { > > .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > static struct sun6i_dma_config sun8i_a83t_dma_cfg = { > > @@ -1173,6 +1207,7 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = { > > .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1200,6 +1235,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_8_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1221,6 +1257,7 @@ static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_8_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1244,6 +1281,7 @@ static struct sun6i_dma_config sun50i_a100_dma_cfg = { > > BIT(DMA_SLAVE_BUSWIDTH_8_BYTES), > > .has_high_addr = true, > > .has_mbus_clk = true, > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1266,6 +1304,7 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = { > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_8_BYTES), > > .has_mbus_clk = true, > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > /* > > @@ -1289,6 +1328,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = { > > .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES), > > + SUN6I_DMA_IRQ_A31_COMMON_OPS > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > >