[v5,5/5] dmaengine: sun6i-dma: Implement 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 | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the interrupt handler incorrectly resolves the physical channel for IRQs by ignoring the IRQ register bank index, routing interrupts to the wrong channel.
- [High] This problem wasn't introduced by this patch, but `sun6i_dma_interrupt` uses truncating integer division for its outer loop bound, causing higher channels to permanently hang on SoCs with unaligned channel counts.
--
commit 0187d2887a53e78b96b7bae3842e4d6f56758d3b
Author: Yuanshen Cao <alex.caoys@gmail.com>
Subject: dmaengine: sun6i-dma: Implement support for Allwinner A733 DMA controller
This commit implements support for the Allwinner A733 DMA controller. It
defines A733-specific register offsets, bitfield masks, and adds the
sun60i_a733_dma_cfg configuration structure.
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index ffb63212bea71..66281e01972dd 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -1318,6 +1324,32 @@ 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 = {
[ ... ]
> + .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]
This isn't a bug introduced by this patch, but since DMA_IRQ_CHAN_NR_A733 is 1,
does the pre-existing interrupt handler correctly resolve the physical channel
for the A733?
Looking at sun6i_dma_interrupt(), it ignores the outer loop index 'i' when
resolving the channel:
for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
...
for (j = 0; (j < sdev->cfg->num_channels_per_reg) && status; j++) {
pchan = sdev->pchans + j;
Since 'j' will always be 0 for the A733 configuration, won't this route all
interrupts for higher channels (where 'i' > 0) to channel 0 instead of adding
the register offset? Could this cause hardware channels to hang permanently
and active descriptors on the wrongly targeted channel to be completed and
freed prematurely, leading to a Use-After-Free?
[Severity: High]
This is also a pre-existing issue, but while reviewing the loop boundary in
sun6i_dma_interrupt(), it appears to use truncating integer division:
for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
For SoCs where the number of channels isn't an exact multiple of
num_channels_per_reg (like the H3 configuration with 12 channels and 8
channels per register), won't the truncating division evaluate to 1 and
completely skip the second register bank? Does this result in unhandled
interrupt storms and permanent DMA channel hangs for trailing channels?
> +};
@@ -54,10 +54,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
@@ -107,6 +111,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
@@ -1318,6 +1324,32 @@ 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,
+ .set_burst_length = sun6i_set_burst_length_h3,
+ .set_drq = sun6i_set_drq_h6,
+ .set_mode = sun6i_set_mode_h6,
+ .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.
@@ -1352,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);