Re: [PATCH v2 2/3] dma: arm-dma350: add support for shared interrupt mode

Jun Guo
2026-01-13T01:56:49Z

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