Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Linus Walleij]

On Sat, May 11, 2013 at 1:31 PM, Heiko Stübner <heiko@xxxxxxxxx> wrote:

> This adds a new driver to support the s3c24xx dma using the dmaengine
> and make the old one in mach-s3c24xx obsolete in the long run.
>
> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
>
> The driver currently only supports the "newer" SoCs which can use any
> physical channel for any dma slave. Support for the older SoCs where
> each channel only supports a subset of possible dma slaves will have to
> be added later.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
(...)
> +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
> +{
> +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> +       struct s3c24xx_txd *txd = s3cchan->at;
> +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> +
> +       switch (txd->dcon & DCON_DSZ_MASK) {
> +       case DCON_DSZ_BYTE:
> +               return tc;
> +       case DCON_DSZ_HALFWORD:
> +               return tc * 2;
> +       case DCON_DSZ_WORD:
> +               return tc * 4;
> +       default:
> +               break;
> +       }
> +
> +       BUG();

Isn't that a bit nasty. This macro should be used with care and we
should recover if possible. dev_err()?

> +/*
> + * Set the initial DMA register values.
> + * Put them into the hardware and start the transfer.
> + */
> +static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
> +{
> +       struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
> +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> +       struct virt_dma_desc *vd = vchan_next_desc(&s3cchan->vc);
> +       struct s3c24xx_txd *txd = to_s3c24xx_txd(&vd->tx);
> +       u32 dcon = txd->dcon;
> +       u32 val;
> +
> +       list_del(&txd->vd.node);
> +
> +       s3cchan->at = txd;
> +
> +       /* Wait for channel inactive */
> +       while (s3c24xx_dma_phy_busy(phy))
> +               cpu_relax();
> +
> +       /* transfer-size and -count from len and width */
> +       switch (txd->width) {
> +       case 1:
> +               dcon |= DCON_DSZ_BYTE | txd->len;
> +               break;
> +       case 2:
> +               dcon |= DCON_DSZ_HALFWORD | (txd->len / 2);
> +               break;
> +       case 4:
> +               dcon |= DCON_DSZ_WORD | (txd->len / 4);
> +               break;
> +       }
> +
> +       if (s3cchan->slave) {
> +               if (s3cdma->sdata->has_reqsel) {
> +                       int reqsel = s3cdma->pdata->reqsel_map[s3cchan->id];
> +                       writel((reqsel << 1) | DMAREQSEL_HW,
> +                              phy->base + DMAREQSEL);
> +               } else {
> +                       dev_err(&s3cdma->pdev->dev, "non-reqsel unsupported\n");
> +                       return;
> +                       dcon |= DCON_HWTRIG;
> +               }
> +       } else {
> +               if (s3cdma->sdata->has_reqsel) {
> +                       writel(0, phy->base + DMAREQSEL);
> +               } else {
> +                       dev_err(&s3cdma->pdev->dev, "non-reqsel unsupported\n");
> +                       return;
> +               }
> +       }
> +
> +       writel(txd->src_addr, phy->base + DISRC);
> +       writel(txd->disrcc, phy->base + DISRCC);
> +       writel(txd->dst_addr, phy->base + DIDST);
> +       writel(txd->didstc, phy->base + DIDSTC);
> +
> +       writel(dcon, phy->base + DCON);
> +
> +       val = readl(phy->base + DMASKTRIG);
> +       val &= ~DMASKTRIG_STOP;
> +       val |= DMASKTRIG_ON;
> +       writel(val, phy->base + DMASKTRIG);
> +
> +       /* trigger the dma operation for memcpy transfers */
> +       if (!s3cchan->slave) {
> +               val = readl(phy->base + DMASKTRIG);
> +               val |= DMASKTRIG_SWTRIG;
> +               writel(val, phy->base + DMASKTRIG);
> +       }
> +}

Since you have a few readl() and writel() in potentially
time-critical code, consider using readl_relaxed() and
writel_relaxed().

> +static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
> +{
> +       struct s3c24xx_dma_phy *phy = data;
> +       struct s3c24xx_dma_chan *s3cchan = phy->serving;
> +       struct s3c24xx_txd *txd;
> +
> +       dev_dbg(&phy->host->pdev->dev, "interrupt on channel %d\n", phy->id);
> +
> +       if (!s3cchan) {
> +               dev_err(&phy->host->pdev->dev, "interrupt on unused channel %d\n",
> +                       phy->id);
> +               return IRQ_NONE;
> +       }
> +
> +       spin_lock(&s3cchan->vc.lock);
> +       txd = s3cchan->at;
> +       if (txd) {
> +               s3cchan->at = NULL;
> +               vchan_cookie_complete(&txd->vd);
> +
> +               /*
> +                * And start the next descriptor (if any),
> +                * otherwise free this channel.
> +                */
> +               if (vchan_next_desc(&s3cchan->vc))
> +                       s3c24xx_dma_start_next_txd(s3cchan);
> +               else
> +                       s3c24xx_dma_phy_free(s3cchan);
> +       }
> +       spin_unlock(&s3cchan->vc.lock);
> +
> +       return IRQ_HANDLED;
> +}

OK so one IRQ per channel. Is there really no status
register to check or flag to clear on these IRQs?

Apart from these smallish things it looks good to me:
Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/