| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87ad76b4-0141-46d9-81df-1e2948f46803@cixtech.com>
Date: Wed, 17 Dec 2025 10:12:50 +0800
From: Jun Guo <jun.guo@...tech.com>
To: Robin Murphy <robin.murphy@....com>, peter.chen@...tech.com,
fugang.duan@...tech.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, vkoul@...nel.org, ychuang3@...oton.com,
schung@...oton.com
Cc: dmaengine@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, cix-kernel-upstream@...tech.com,
linux-arm-kernel@...ts.infradead.org, jelly.jia@...tech.com
Subject: Re: [PATCH v2 2/3] dma: arm-dma350: add support for shared interrupt
mode
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@...tech.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
Powered by blists - more mailing lists