[v4,5/5] dmaengine: sun6i-dma: Add support for Allwinner A733 DMA controller
Commit Message
Implement support for the Allwinner A733 DMA controller by defining
A733-specific register offsets, bitfield masks, and the
`sun60i_a733_dma_cfg` configuration structure.
This includes the IRQ stride, offsets, and channel counts specific to the
A733, as well as the 32G mask for high-address fields in the LLI. Add
`sun60i_a733_dma_cfg`, which ties all the refactored functionality
together for A733.
Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
drivers/dma/sun6i-dma.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Missing mandatory function pointers in sun60i_a733_dma_cfg leads to unconditionally dereferencing NULL pointers during DMA preparation.
Pre-existing issues:
- [High] Incorrect physical channel indexing in the DMA interrupt handler maps interrupts for all registers to the first register's channels.
- [High] Missed interrupts due to integer division truncation in the DMA interrupt handler causes higher channel interrupts to be ignored.
--
commit 9fc6c319ac37c3c86b13dd65e2affac0589606b0
Author: Yuanshen Cao <alex.caoys@gmail.com>
dmaengine: sun6i-dma: Add support for Allwinner A733 DMA controller
Implement support for the Allwinner A733 DMA controller by defining
A733-specific register offsets, bitfield masks, and the
sun60i_a733_dma_cfg configuration structure.
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index b96d7d90f6b53..fb371b57d792a 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -1321,6 +1327,29 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
> SUN6I_DMA_IRQ_A31_COMMON_CFG
> };
>
> +/*
> + * The A733 binding uses the number of dma channels from the
> + * device tree node.
> + */
> +static struct sun6i_dma_config sun60i_a733_dma_cfg = {
> + .clock_autogate_enable = sun6i_enable_clock_autogate_h3,
> + .src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .src_high_addr_mask = SRC_HIGH_ADDR_32G_MASK,
> + .dst_high_addr_mask = DST_HIGH_ADDR_32G_MASK,
> + .has_mbus_clk = true,
> + .irq_stride = DMA_IRQ_STRIDE_A733,
> + .irq_en_offset = DMA_IRQ_EN_OFFSET_A733,
> + .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A733,
> + .num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
> +};
> +
[Severity: High]
Does sun60i_a733_dma_cfg omit the set_burst_length, set_drq, and set_mode
function pointers?
When sun6i_dma_prep_dma_memcpy(), sun6i_dma_prep_slave_sg(), or
sun6i_dma_prep_dma_cyclic() are called, they unconditionally invoke these
function pointers:
sdev->cfg->set_burst_length(&v_lli->cfg, burst, burst);
sdev->cfg->set_drq(&v_lli->cfg, DRQ_SDRAM, DRQ_SDRAM);
sdev->cfg->set_mode(&v_lli->cfg, LINEAR_MODE, LINEAR_MODE);
Could this result in a NULL pointer dereference on the A733 platform?
[Severity: High]
This is a pre-existing issue, but does setting num_channels_per_reg to 1 for
the A733 platform expose a physical channel indexing issue in
sun6i_dma_interrupt()?
When an interrupt occurs on a secondary IRQ register, sun6i_dma_interrupt()
reads the correct status register i, but calculates the physical channel
pointer as:
pchan = sdev->pchans + j;
Since it omits the i * sdev->cfg->num_channels_per_reg offset, hardware
interrupts for higher channels appear to be routed to lower channels. On
A733, this would map all channel interrupts to channel 0.
[Severity: High]
This is a pre-existing issue, but does the loop bound in
sun6i_dma_interrupt() truncate due to integer division on platforms with
non-multiple channel counts?
for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
status = sun6i_read_irq_stat(sdev, i);
If a platform has 12 physical channels and 8 channels per register (like H3),
the division evaluates to 1, causing the loop to only read interrupts for
channels 0-7 and ignore the final register. Could this cause transfers on the
highest channels to time out?
On Wed, Aug 26, 2026 at 01:30:07AM +0000, Yuanshen Cao wrote:
> Implement support for the Allwinner A733 DMA controller by defining
> A733-specific register offsets, bitfield masks, and the
> `sun60i_a733_dma_cfg` configuration structure.
>
> This includes the IRQ stride, offsets, and channel counts specific to the
> A733, as well as the 32G mask for high-address fields in the LLI. Add
> `sun60i_a733_dma_cfg`, which ties all the refactored functionality
> together for A733.
>
> Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
> ---
> drivers/dma/sun6i-dma.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index b96d7d90f6b5..fb371b57d792 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -57,10 +57,14 @@
> * Interrupts specific registers
> */
> #define DMA_IRQ_STRIDE_A31 0x04
> +#define DMA_IRQ_STRIDE_A733 0x40
> #define DMA_IRQ_EN_OFFSET_A31 0x00
> +#define DMA_IRQ_EN_OFFSET_A733 0x134
> #define DMA_IRQ_STAT_OFFSET_A31 0x10
> +#define DMA_IRQ_STAT_OFFSET_A733 0x138
>
> #define DMA_IRQ_CHAN_NR_A31 8
> +#define DMA_IRQ_CHAN_NR_A733 1
>
> /*
> * Channels specific registers
> @@ -110,6 +114,8 @@
> */
> #define SRC_HIGH_ADDR_MASK GENMASK(17, 16)
> #define DST_HIGH_ADDR_MASK GENMASK(19, 18)
> +#define SRC_HIGH_ADDR_32G_MASK GENMASK(13, 11)
> +#define DST_HIGH_ADDR_32G_MASK GENMASK(17, 15)
>
> /*
> * Various hardware related defines
> @@ -1321,6 +1327,29 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
> SUN6I_DMA_IRQ_A31_COMMON_CFG
> };
>
> +/*
> + * The A733 binding uses the number of dma channels from the
> + * device tree node.
> + */
> +static struct sun6i_dma_config sun60i_a733_dma_cfg = {
> + .clock_autogate_enable = sun6i_enable_clock_autogate_h3,
> + .src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .src_high_addr_mask = SRC_HIGH_ADDR_32G_MASK,
> + .dst_high_addr_mask = DST_HIGH_ADDR_32G_MASK,
> + .has_mbus_clk = true,
> + .irq_stride = DMA_IRQ_STRIDE_A733,
> + .irq_en_offset = DMA_IRQ_EN_OFFSET_A733,
> + .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A733,
> + .num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
Nit: if only use once, needn't define macro, you can put value to here.
Also please check previous prepare patch.
Frank
> +};
> +
> /*
> * The V3s have only 8 physical channels, a maximum DRQ port id of 23,
> * and a total of 24 usable source and destination endpoints.
> @@ -1355,6 +1384,7 @@ static const struct of_device_id sun6i_dma_match[] = {
> { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
> { .compatible = "allwinner,sun50i-a100-dma", .data = &sun50i_a100_dma_cfg },
> { .compatible = "allwinner,sun50i-h6-dma", .data = &sun50i_h6_dma_cfg },
> + { .compatible = "allwinner,sun60i-a733-dma", .data = &sun60i_a733_dma_cfg },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, sun6i_dma_match);
>
> --
> 2.55.0
>
@@ -57,10 +57,14 @@
* Interrupts specific registers
*/
#define DMA_IRQ_STRIDE_A31 0x04
+#define DMA_IRQ_STRIDE_A733 0x40
#define DMA_IRQ_EN_OFFSET_A31 0x00
+#define DMA_IRQ_EN_OFFSET_A733 0x134
#define DMA_IRQ_STAT_OFFSET_A31 0x10
+#define DMA_IRQ_STAT_OFFSET_A733 0x138
#define DMA_IRQ_CHAN_NR_A31 8
+#define DMA_IRQ_CHAN_NR_A733 1
/*
* Channels specific registers
@@ -110,6 +114,8 @@
*/
#define SRC_HIGH_ADDR_MASK GENMASK(17, 16)
#define DST_HIGH_ADDR_MASK GENMASK(19, 18)
+#define SRC_HIGH_ADDR_32G_MASK GENMASK(13, 11)
+#define DST_HIGH_ADDR_32G_MASK GENMASK(17, 15)
/*
* Various hardware related defines
@@ -1321,6 +1327,29 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
SUN6I_DMA_IRQ_A31_COMMON_CFG
};
+/*
+ * The A733 binding uses the number of dma channels from the
+ * device tree node.
+ */
+static struct sun6i_dma_config sun60i_a733_dma_cfg = {
+ .clock_autogate_enable = sun6i_enable_clock_autogate_h3,
+ .src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
+ .dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
+ .src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ .src_high_addr_mask = SRC_HIGH_ADDR_32G_MASK,
+ .dst_high_addr_mask = DST_HIGH_ADDR_32G_MASK,
+ .has_mbus_clk = true,
+ .irq_stride = DMA_IRQ_STRIDE_A733,
+ .irq_en_offset = DMA_IRQ_EN_OFFSET_A733,
+ .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A733,
+ .num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
+};
+
/*
* The V3s have only 8 physical channels, a maximum DRQ port id of 23,
* and a total of 24 usable source and destination endpoints.
@@ -1355,6 +1384,7 @@ static const struct of_device_id sun6i_dma_match[] = {
{ .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
{ .compatible = "allwinner,sun50i-a100-dma", .data = &sun50i_a100_dma_cfg },
{ .compatible = "allwinner,sun50i-h6-dma", .data = &sun50i_h6_dma_cfg },
+ { .compatible = "allwinner,sun60i-a733-dma", .data = &sun60i_a733_dma_cfg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sun6i_dma_match);