The arm dma350 controller's hardware implementation varies: some
designs dedicate a separate interrupt line for each channel, while
others have all channels sharing a single interrupt.This patch adds
support for the hardware design where all DMA channels share a
single interrupt.
Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
1 file changed, 114 insertions(+), 10 deletions(-)
diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 9efe2ca7d5ec..6bea18521edd 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -14,6 +14,7 @@
#include "virt-dma.h"
#define DMAINFO 0x0f00
+#define DRIVER_NAME "arm-dma350"
#define DMA_BUILDCFG0 0xb0
#define DMA_CFG_DATA_WIDTH GENMASK(18, 16)
@@ -142,6 +143,9 @@
#define LINK_LINKADDR BIT(30)
#define LINK_LINKADDRHI BIT(31)
+/* DMA NONSECURE CONTROL REGISTER */
+#define DMANSECCTRL 0x20c
+#define INTREN_ANYCHINTR_EN BIT(0)
enum ch_ctrl_donetype {
CH_CTRL_DONETYPE_NONE = 0,
@@ -192,11 +196,16 @@ struct d350_chan {
struct d350 {
struct dma_device dma;
+ void __iomem *base;
int nchan;
int nreq;
struct d350_chan channels[] __counted_by(nchan);
};
+struct d350_driver_data {
+ bool combined_irq;
+};
+
static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
{
return container_of(chan, struct d350_chan, vc.chan);
@@ -461,7 +470,61 @@ static void d350_issue_pending(struct dma_chan *chan)
spin_unlock_irqrestore(&dch->vc.lock, flags);
}
-static irqreturn_t d350_irq(int irq, void *data)
+static irqreturn_t d350_global_irq(int irq, void *data)
+{
+ struct d350 *dmac = (struct d350 *)data;
+ struct device *dev = dmac->dma.dev;
+ irqreturn_t ret = IRQ_NONE;
+ int i;
+
+ for (i = 0; i < dmac->nchan; i++) {
+ struct d350_chan *dch = &dmac->channels[i];
+ u32 ch_status;
+
+ ch_status = readl(dch->base + CH_STATUS);
+ if (!ch_status)
+ continue;
+
+ ret = IRQ_HANDLED;
+
+ if (ch_status & CH_STAT_INTR_ERR) {
+ struct virt_dma_desc *vd = &dch->desc->vd;
+ u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
+
+ if (errinfo &
+ (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
+ vd->tx_result.result = DMA_TRANS_READ_FAILED;
+ else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
+ vd->tx_result.result = DMA_TRANS_WRITE_FAILED;
+ else
+ vd->tx_result.result = DMA_TRANS_ABORTED;
+
+ vd->tx_result.residue = d350_get_residue(dch);
+ } else if (!(ch_status & CH_STAT_INTR_DONE)) {
+ dev_warn(dev, "Channel %d unexpected IRQ: 0x%08x\n", i,
+ ch_status);
+ }
+
+ writel_relaxed(ch_status, dch->base + CH_STATUS);
+
+ spin_lock(&dch->vc.lock);
+ if (ch_status & CH_STAT_INTR_DONE) {
+ vchan_cookie_complete(&dch->desc->vd);
+ dch->status = DMA_COMPLETE;
+ dch->residue = 0;
+ d350_start_next(dch);
+ } else if (ch_status & CH_STAT_INTR_ERR) {
+ vchan_cookie_complete(&dch->desc->vd);
+ dch->status = DMA_ERROR;
+ dch->residue = dch->desc->vd.tx_result.residue;
+ }
+ spin_unlock(&dch->vc.lock);
+ }
+
+ return ret;
+}
+
+static irqreturn_t d350_channel_irq(int irq, void *data)
{
struct d350_chan *dch = data;
struct device *dev = dch->vc.chan.device->dev;
@@ -506,10 +569,18 @@ static irqreturn_t d350_irq(int irq, void *data)
static int d350_alloc_chan_resources(struct dma_chan *chan)
{
struct d350_chan *dch = to_d350_chan(chan);
- int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
- dev_name(&dch->vc.chan.dev->device), dch);
- if (!ret)
- writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
+ int ret = 0;
+
+ if (dch->irq) {
+ ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
+ dev_name(&dch->vc.chan.dev->device), dch);
+ if (ret) {
+ dev_err(chan->device->dev, "Failed to request IRQ %d\n", dch->irq);
+ return ret;
+ }
+ }
+
+ writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
return ret;
}
@@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
static int d350_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct d350 *dmac;
+ struct d350 *dmac = NULL;
+ const struct d350_driver_data *data;
void __iomem *base;
u32 reg;
int ret, nchan, dw, aw, r, p;
@@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
return -ENOMEM;
dmac->nchan = nchan;
+ dmac->base = base;
reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
@@ -582,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
dmac->dma.device_issue_pending = d350_issue_pending;
INIT_LIST_HEAD(&dmac->dma.channels);
+ data = device_get_match_data(dev);
+ /* Cix Sky1 has a common host IRQ for all its channels. */
+ if (data && data->combined_irq) {
+ int host_irq = platform_get_irq(pdev, 0);
+
+ if (host_irq < 0)
+ return dev_err_probe(dev, host_irq,
+ "Failed to get IRQ\n");
+
+ ret = devm_request_irq(&pdev->dev, host_irq, d350_global_irq,
+ IRQF_SHARED, DRIVER_NAME, dmac);
+ if (ret)
+ return dev_err_probe(
+ dev, ret,
+ "Failed to request the combined IRQ %d\n",
+ host_irq);
+ }
+
+ /* Combined Non-Secure Channel Interrupt Enable */
+ writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
+
/* Would be nice to have per-channel caps for this... */
memset = true;
for (int i = 0; i < nchan; i++) {
@@ -595,10 +689,15 @@ static int d350_probe(struct platform_device *pdev)
dev_warn(dev, "No command link support on channel %d\n", i);
continue;
}
- dch->irq = platform_get_irq(pdev, i);
- if (dch->irq < 0)
- return dev_err_probe(dev, dch->irq,
- "Failed to get IRQ for channel %d\n", i);
+
+ if (!data) {
+ dch->irq = platform_get_irq(pdev, i);
+ if (dch->irq < 0)
+ return dev_err_probe(
+ dev, dch->irq,
+ "Failed to get IRQ for channel %d\n",
+ i);
+ }
dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
@@ -639,7 +738,12 @@ static void d350_remove(struct platform_device *pdev)
dma_async_device_unregister(&dmac->dma);
}
+static const struct d350_driver_data sky1_dma350_data = {
+ .combined_irq = true,
+};
+
static const struct of_device_id d350_of_match[] __maybe_unused = {
+ { .compatible = "cix,sky1-dma-350", .data = &sky1_dma350_data },
{ .compatible = "arm,dma-350" },
{}
};
--
2.34.1On 2025-12-16 12:30 pm, Jun Guo wrote:
> The arm dma350 controller's hardware implementation varies: some
> designs dedicate a separate interrupt line for each channel, while
> others have all channels sharing a single interrupt.This patch adds
> support for the hardware design where all DMA channels share a
> single interrupt.
>
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
> drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
> 1 file changed, 114 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 9efe2ca7d5ec..6bea18521edd 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -14,6 +14,7 @@
> #include "virt-dma.h"
>
> #define DMAINFO 0x0f00
> +#define DRIVER_NAME "arm-dma350"
>
> #define DMA_BUILDCFG0 0xb0
> #define DMA_CFG_DATA_WIDTH GENMASK(18, 16)
> @@ -142,6 +143,9 @@
> #define LINK_LINKADDR BIT(30)
> #define LINK_LINKADDRHI BIT(31)
>
> +/* DMA NONSECURE CONTROL REGISTER */
> +#define DMANSECCTRL 0x20c
> +#define INTREN_ANYCHINTR_EN BIT(0)
>
> enum ch_ctrl_donetype {
> CH_CTRL_DONETYPE_NONE = 0,
> @@ -192,11 +196,16 @@ struct d350_chan {
>
> struct d350 {
> struct dma_device dma;
> + void __iomem *base;
> int nchan;
> int nreq;
> struct d350_chan channels[] __counted_by(nchan);
> };
>
> +struct d350_driver_data {
> + bool combined_irq;
> +};
> +
> static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
> {
> return container_of(chan, struct d350_chan, vc.chan);
> @@ -461,7 +470,61 @@ static void d350_issue_pending(struct dma_chan *chan)
> spin_unlock_irqrestore(&dch->vc.lock, flags);
> }
>
> -static irqreturn_t d350_irq(int irq, void *data)
> +static irqreturn_t d350_global_irq(int irq, void *data)
> +{
> + struct d350 *dmac = (struct d350 *)data;
> + struct device *dev = dmac->dma.dev;
> + irqreturn_t ret = IRQ_NONE;
> + int i;
> +
> + for (i = 0; i < dmac->nchan; i++) {
> + struct d350_chan *dch = &dmac->channels[i];
> + u32 ch_status;
> +
> + ch_status = readl(dch->base + CH_STATUS);
> + if (!ch_status)
> + continue;
> +
> + ret = IRQ_HANDLED;
> +
> + if (ch_status & CH_STAT_INTR_ERR) {
> + struct virt_dma_desc *vd = &dch->desc->vd;
> + u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
> +
> + if (errinfo &
> + (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
> + vd->tx_result.result = DMA_TRANS_READ_FAILED;
> + else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
> + vd->tx_result.result = DMA_TRANS_WRITE_FAILED;
> + else
> + vd->tx_result.result = DMA_TRANS_ABORTED;
> +
> + vd->tx_result.residue = d350_get_residue(dch);
> + } else if (!(ch_status & CH_STAT_INTR_DONE)) {
> + dev_warn(dev, "Channel %d unexpected IRQ: 0x%08x\n", i,
> + ch_status);
> + }
> +
> + writel_relaxed(ch_status, dch->base + CH_STATUS);
> +
> + spin_lock(&dch->vc.lock);
> + if (ch_status & CH_STAT_INTR_DONE) {
> + vchan_cookie_complete(&dch->desc->vd);
> + dch->status = DMA_COMPLETE;
> + dch->residue = 0;
> + d350_start_next(dch);
> + } else if (ch_status & CH_STAT_INTR_ERR) {
> + vchan_cookie_complete(&dch->desc->vd);
> + dch->status = DMA_ERROR;
> + dch->residue = dch->desc->vd.tx_result.residue;
> + }
> + spin_unlock(&dch->vc.lock);
> + }
> +
> + return ret;
> +}
> +
> +static irqreturn_t d350_channel_irq(int irq, void *data)
> {
> struct d350_chan *dch = data;
> struct device *dev = dch->vc.chan.device->dev;
> @@ -506,10 +569,18 @@ static irqreturn_t d350_irq(int irq, void *data)
> static int d350_alloc_chan_resources(struct dma_chan *chan)
> {
> struct d350_chan *dch = to_d350_chan(chan);
> - int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
> - dev_name(&dch->vc.chan.dev->device), dch);
> - if (!ret)
> - writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
> + int ret = 0;
> +
> + if (dch->irq) {
> + ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
> + dev_name(&dch->vc.chan.dev->device), dch);
> + if (ret) {
> + dev_err(chan->device->dev, "Failed to request IRQ %d\n", dch->irq);
> + return ret;
> + }
> + }
> +
> + writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
>
> return ret;
> }
> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
> static int d350_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct d350 *dmac;
> + struct d350 *dmac = NULL;
> + const struct d350_driver_data *data;
> void __iomem *base;
> u32 reg;
> int ret, nchan, dw, aw, r, p;
> @@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> dmac->nchan = nchan;
> + dmac->base = base;
>
> reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
> dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
> @@ -582,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
> dmac->dma.device_issue_pending = d350_issue_pending;
> INIT_LIST_HEAD(&dmac->dma.channels);
>
> + data = device_get_match_data(dev);
> + /* Cix Sky1 has a common host IRQ for all its channels. */
> + if (data && data->combined_irq) {
> + int host_irq = platform_get_irq(pdev, 0);
> +
> + if (host_irq < 0)
> + return dev_err_probe(dev, host_irq,
> + "Failed to get IRQ\n");
> +
> + ret = devm_request_irq(&pdev->dev, host_irq, d350_global_irq,
> + IRQF_SHARED, DRIVER_NAME, dmac);
> + if (ret)
> + return dev_err_probe(
> + dev, ret,
> + "Failed to request the combined IRQ %d\n",
> + host_irq);
> + }
> +
> + /* Combined Non-Secure Channel Interrupt Enable */
> + writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
This one line is all that should be needed - all the rest is pointless
overcomplication and churn. And either way, copy-pasting the entire IRQ
handler is not OK.
Thanks,
Robin.
> +
> /* Would be nice to have per-channel caps for this... */
> memset = true;
> for (int i = 0; i < nchan; i++) {
> @@ -595,10 +689,15 @@ static int d350_probe(struct platform_device *pdev)
> dev_warn(dev, "No command link support on channel %d\n", i);
> continue;
> }
> - dch->irq = platform_get_irq(pdev, i);
> - if (dch->irq < 0)
> - return dev_err_probe(dev, dch->irq,
> - "Failed to get IRQ for channel %d\n", i);
> +
> + if (!data) {
> + dch->irq = platform_get_irq(pdev, i);
> + if (dch->irq < 0)
> + return dev_err_probe(
> + dev, dch->irq,
> + "Failed to get IRQ for channel %d\n",
> + i);
> + }
>
> dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
> dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
> @@ -639,7 +738,12 @@ static void d350_remove(struct platform_device *pdev)
> dma_async_device_unregister(&dmac->dma);
> }
>
> +static const struct d350_driver_data sky1_dma350_data = {
> + .combined_irq = true,
> +};
> +
> static const struct of_device_id d350_of_match[] __maybe_unused = {
> + { .compatible = "cix,sky1-dma-350", .data = &sky1_dma350_data },
> { .compatible = "arm,dma-350" },
> {}
> };
On 12/16/2025 8:51 PM, Robin Murphy wrote:
> On 2025-12-16 12:30 pm, Jun Guo wrote:
>> The arm dma350 controller's hardware implementation varies: some
>> designs dedicate a separate interrupt line for each channel, while
>> others have all channels sharing a single interrupt.This patch adds
>> support for the hardware design where all DMA channels share a
>> single interrupt.
>>
>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>> ---
>> drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
>> 1 file changed, 114 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
>> index 9efe2ca7d5ec..6bea18521edd 100644
>> --- a/drivers/dma/arm-dma350.c
>> +++ b/drivers/dma/arm-dma350.c
>> @@ -14,6 +14,7 @@
>> #include "virt-dma.h"
>>
>> #define DMAINFO 0x0f00
>> +#define DRIVER_NAME "arm-dma350"
>>
>> #define DMA_BUILDCFG0 0xb0
>> #define DMA_CFG_DATA_WIDTH GENMASK(18, 16)
>> @@ -142,6 +143,9 @@
>> #define LINK_LINKADDR BIT(30)
>> #define LINK_LINKADDRHI BIT(31)
>>
>> +/* DMA NONSECURE CONTROL REGISTER */
>> +#define DMANSECCTRL 0x20c
>> +#define INTREN_ANYCHINTR_EN BIT(0)
>>
>> enum ch_ctrl_donetype {
>> CH_CTRL_DONETYPE_NONE = 0,
>> @@ -192,11 +196,16 @@ struct d350_chan {
>>
>> struct d350 {
>> struct dma_device dma;
>> + void __iomem *base;
>> int nchan;
>> int nreq;
>> struct d350_chan channels[] __counted_by(nchan);
>> };
>>
>> +struct d350_driver_data {
>> + bool combined_irq;
>> +};
>> +
>> static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
>> {
>> return container_of(chan, struct d350_chan, vc.chan);
>> @@ -461,7 +470,61 @@ static void d350_issue_pending(struct dma_chan
>> *chan)
>> spin_unlock_irqrestore(&dch->vc.lock, flags);
>> }
>>
>> -static irqreturn_t d350_irq(int irq, void *data)
>> +static irqreturn_t d350_global_irq(int irq, void *data)
>> +{
>> + struct d350 *dmac = (struct d350 *)data;
>> + struct device *dev = dmac->dma.dev;
>> + irqreturn_t ret = IRQ_NONE;
>> + int i;
>> +
>> + for (i = 0; i < dmac->nchan; i++) {
>> + struct d350_chan *dch = &dmac->channels[i];
>> + u32 ch_status;
>> +
>> + ch_status = readl(dch->base + CH_STATUS);
>> + if (!ch_status)
>> + continue;
>> +
>> + ret = IRQ_HANDLED;
>> +
>> + if (ch_status & CH_STAT_INTR_ERR) {
>> + struct virt_dma_desc *vd = &dch->desc->vd;
>> + u32 errinfo = readl_relaxed(dch->base +
>> CH_ERRINFO);
>> +
>> + if (errinfo &
>> + (CH_ERRINFO_AXIRDPOISERR |
>> CH_ERRINFO_AXIRDRESPERR))
>> + vd->tx_result.result =
>> DMA_TRANS_READ_FAILED;
>> + else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
>> + vd->tx_result.result =
>> DMA_TRANS_WRITE_FAILED;
>> + else
>> + vd->tx_result.result = DMA_TRANS_ABORTED;
>> +
>> + vd->tx_result.residue = d350_get_residue(dch);
>> + } else if (!(ch_status & CH_STAT_INTR_DONE)) {
>> + dev_warn(dev, "Channel %d unexpected IRQ:
>> 0x%08x\n", i,
>> + ch_status);
>> + }
>> +
>> + writel_relaxed(ch_status, dch->base + CH_STATUS);
>> +
>> + spin_lock(&dch->vc.lock);
>> + if (ch_status & CH_STAT_INTR_DONE) {
>> + vchan_cookie_complete(&dch->desc->vd);
>> + dch->status = DMA_COMPLETE;
>> + dch->residue = 0;
>> + d350_start_next(dch);
>> + } else if (ch_status & CH_STAT_INTR_ERR) {
>> + vchan_cookie_complete(&dch->desc->vd);
>> + dch->status = DMA_ERROR;
>> + dch->residue = dch->desc->vd.tx_result.residue;
>> + }
>> + spin_unlock(&dch->vc.lock);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static irqreturn_t d350_channel_irq(int irq, void *data)
>> {
>> struct d350_chan *dch = data;
>> struct device *dev = dch->vc.chan.device->dev;
>> @@ -506,10 +569,18 @@ static irqreturn_t d350_irq(int irq, void *data)
>> static int d350_alloc_chan_resources(struct dma_chan *chan)
>> {
>> struct d350_chan *dch = to_d350_chan(chan);
>> - int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
>> - dev_name(&dch->vc.chan.dev->device), dch);
>> - if (!ret)
>> - writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base
>> + CH_INTREN);
>> + int ret = 0;
>> +
>> + if (dch->irq) {
>> + ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
>> + dev_name(&dch->vc.chan.dev->device),
>> dch);
>> + if (ret) {
>> + dev_err(chan->device->dev, "Failed to request
>> IRQ %d\n", dch->irq);
>> + return ret;
>> + }
>> + }
>> +
>> + writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base +
>> CH_INTREN);
>>
>> return ret;
>> }
>> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct
>> dma_chan *chan)
>> static int d350_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> - struct d350 *dmac;
>> + struct d350 *dmac = NULL;
>> + const struct d350_driver_data *data;
>> void __iomem *base;
>> u32 reg;
>> int ret, nchan, dw, aw, r, p;
>> @@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
>> return -ENOMEM;
>>
>> dmac->nchan = nchan;
>> + dmac->base = base;
>>
>> reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>> dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>> @@ -582,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
>> dmac->dma.device_issue_pending = d350_issue_pending;
>> INIT_LIST_HEAD(&dmac->dma.channels);
>>
>> + data = device_get_match_data(dev);
>> + /* Cix Sky1 has a common host IRQ for all its channels. */
>> + if (data && data->combined_irq) {
>> + int host_irq = platform_get_irq(pdev, 0);
>> +
>> + if (host_irq < 0)
>> + return dev_err_probe(dev, host_irq,
>> + "Failed to get IRQ\n");
>> +
>> + ret = devm_request_irq(&pdev->dev, host_irq,
>> d350_global_irq,
>> + IRQF_SHARED, DRIVER_NAME, dmac);
>> + if (ret)
>> + return dev_err_probe(
>> + dev, ret,
>> + "Failed to request the combined IRQ %d\n",
>> + host_irq);
>> + }
>> +
>> + /* Combined Non-Secure Channel Interrupt Enable */
>> + writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
>
> This one line is all that should be needed - all the rest is pointless
> overcomplication and churn. And either way, copy-pasting the entire IRQ
> handler is not OK.
>
> Thanks,
> Robin.
If the design uses a single interrupt line for all channels, then I only
need to request one interrupt. When the interrupt occurs, I have to poll
within the interrupt handler to determine which channel triggered it.
Are you saying that just this one line is enough to achieve that? I
don't quite understand.
Best wishes,
Jun Guo
Hi Robin,
Regarding your previous comments on this patch, I wrote some
questions in my last email and would like to continue the discussion.
On 12/17/2025 10:12 AM, Jun Guo wrote:
>
> On 12/16/2025 8:51 PM, Robin Murphy wrote:
>> On 2025-12-16 12:30 pm, Jun Guo wrote:
>>> The arm dma350 controller's hardware implementation varies: some
>>> designs dedicate a separate interrupt line for each channel, while
>>> others have all channels sharing a single interrupt.This patch adds
>>> support for the hardware design where all DMA channels share a
>>> single interrupt.
>>>
>>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>>> ---
>>> drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 114 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
>>> index 9efe2ca7d5ec..6bea18521edd 100644
>>> --- a/drivers/dma/arm-dma350.c
>>> +++ b/drivers/dma/arm-dma350.c
>>> @@ -14,6 +14,7 @@
>>> #include "virt-dma.h"
>>>
>>> #define DMAINFO 0x0f00
>>> +#define DRIVER_NAME "arm-dma350"
>>>
>>> #define DMA_BUILDCFG0 0xb0
>>> #define DMA_CFG_DATA_WIDTH GENMASK(18, 16)
>>> @@ -142,6 +143,9 @@
>>> #define LINK_LINKADDR BIT(30)
>>> #define LINK_LINKADDRHI BIT(31)
>>>
>>> +/* DMA NONSECURE CONTROL REGISTER */
>>> +#define DMANSECCTRL 0x20c
>>> +#define INTREN_ANYCHINTR_EN BIT(0)
>>>
>>> enum ch_ctrl_donetype {
>>> CH_CTRL_DONETYPE_NONE = 0,
>>> @@ -192,11 +196,16 @@ struct d350_chan {
>>>
>>> struct d350 {
>>> struct dma_device dma;
>>> + void __iomem *base;
>>> int nchan;
>>> int nreq;
>>> struct d350_chan channels[] __counted_by(nchan);
>>> };
>>>
>>> +struct d350_driver_data {
>>> + bool combined_irq;
>>> +};
>>> +
>>> static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
>>> {
>>> return container_of(chan, struct d350_chan, vc.chan);
>>> @@ -461,7 +470,61 @@ static void d350_issue_pending(struct dma_chan
>>> *chan)
>>> spin_unlock_irqrestore(&dch->vc.lock, flags);
>>> }
>>>
>>> -static irqreturn_t d350_irq(int irq, void *data)
>>> +static irqreturn_t d350_global_irq(int irq, void *data)
>>> +{
>>> + struct d350 *dmac = (struct d350 *)data;
>>> + struct device *dev = dmac->dma.dev;
>>> + irqreturn_t ret = IRQ_NONE;
>>> + int i;
>>> +
>>> + for (i = 0; i < dmac->nchan; i++) {
>>> + struct d350_chan *dch = &dmac->channels[i];
>>> + u32 ch_status;
>>> +
>>> + ch_status = readl(dch->base + CH_STATUS);
>>> + if (!ch_status)
>>> + continue;
>>> +
>>> + ret = IRQ_HANDLED;
>>> +
>>> + if (ch_status & CH_STAT_INTR_ERR) {
>>> + struct virt_dma_desc *vd = &dch->desc->vd;
>>> + u32 errinfo = readl_relaxed(dch->base +
>>> CH_ERRINFO);
>>> +
>>> + if (errinfo &
>>> + (CH_ERRINFO_AXIRDPOISERR |
>>> CH_ERRINFO_AXIRDRESPERR))
>>> + vd->tx_result.result =
>>> DMA_TRANS_READ_FAILED;
>>> + else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
>>> + vd->tx_result.result =
>>> DMA_TRANS_WRITE_FAILED;
>>> + else
>>> + vd->tx_result.result = DMA_TRANS_ABORTED;
>>> +
>>> + vd->tx_result.residue = d350_get_residue(dch);
>>> + } else if (!(ch_status & CH_STAT_INTR_DONE)) {
>>> + dev_warn(dev, "Channel %d unexpected IRQ:
>>> 0x%08x\n", i,
>>> + ch_status);
>>> + }
>>> +
>>> + writel_relaxed(ch_status, dch->base + CH_STATUS);
>>> +
>>> + spin_lock(&dch->vc.lock);
>>> + if (ch_status & CH_STAT_INTR_DONE) {
>>> + vchan_cookie_complete(&dch->desc->vd);
>>> + dch->status = DMA_COMPLETE;
>>> + dch->residue = 0;
>>> + d350_start_next(dch);
>>> + } else if (ch_status & CH_STAT_INTR_ERR) {
>>> + vchan_cookie_complete(&dch->desc->vd);
>>> + dch->status = DMA_ERROR;
>>> + dch->residue = dch->desc->vd.tx_result.residue;
>>> + }
>>> + spin_unlock(&dch->vc.lock);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static irqreturn_t d350_channel_irq(int irq, void *data)
>>> {
>>> struct d350_chan *dch = data;
>>> struct device *dev = dch->vc.chan.device->dev;
>>> @@ -506,10 +569,18 @@ static irqreturn_t d350_irq(int irq, void *data)
>>> static int d350_alloc_chan_resources(struct dma_chan *chan)
>>> {
>>> struct d350_chan *dch = to_d350_chan(chan);
>>> - int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
>>> - dev_name(&dch->vc.chan.dev->device), dch);
>>> - if (!ret)
>>> - writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch-
>>> >base + CH_INTREN);
>>> + int ret = 0;
>>> +
>>> + if (dch->irq) {
>>> + ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
>>> + dev_name(&dch->vc.chan.dev->device),
>>> dch);
>>> + if (ret) {
>>> + dev_err(chan->device->dev, "Failed to request
>>> IRQ %d\n", dch->irq);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base +
>>> CH_INTREN);
>>>
>>> return ret;
>>> }
>>> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct
>>> dma_chan *chan)
>>> static int d350_probe(struct platform_device *pdev)
>>> {
>>> struct device *dev = &pdev->dev;
>>> - struct d350 *dmac;
>>> + struct d350 *dmac = NULL;
>>> + const struct d350_driver_data *data;
>>> void __iomem *base;
>>> u32 reg;
>>> int ret, nchan, dw, aw, r, p;
>>> @@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>>
>>> dmac->nchan = nchan;
>>> + dmac->base = base;
>>>
>>> reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>>> dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>>> @@ -582,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
>>> dmac->dma.device_issue_pending = d350_issue_pending;
>>> INIT_LIST_HEAD(&dmac->dma.channels);
>>>
>>> + data = device_get_match_data(dev);
>>> + /* Cix Sky1 has a common host IRQ for all its channels. */
>>> + if (data && data->combined_irq) {
>>> + int host_irq = platform_get_irq(pdev, 0);
>>> +
>>> + if (host_irq < 0)
>>> + return dev_err_probe(dev, host_irq,
>>> + "Failed to get IRQ\n");
>>> +
>>> + ret = devm_request_irq(&pdev->dev, host_irq,
>>> d350_global_irq,
>>> + IRQF_SHARED, DRIVER_NAME, dmac);
>>> + if (ret)
>>> + return dev_err_probe(
>>> + dev, ret,
>>> + "Failed to request the combined IRQ %d\n",
>>> + host_irq);
>>> + }
>>> +
>>> + /* Combined Non-Secure Channel Interrupt Enable */
>>> + writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
>>
>> This one line is all that should be needed - all the rest is pointless
>> overcomplication and churn. And either way, copy-pasting the entire IRQ
>> handler is not OK.
>>
>> Thanks,
>> Robin.
>
> If the design uses a single interrupt line for all channels, then I only
> need to request one interrupt. When the interrupt occurs, I have to poll
> within the interrupt handler to determine which channel triggered it.
> Are you saying that just this one line is enough to achieve that? I
> don't quite understand.
>
Best wishes,
Jun Guo
© 2016 - 2026 Red Hat, Inc.