[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCH] dw_spi: add DMA support
From:       Linus Walleij <linus.ml.walleij () gmail ! com>
Date:       2010-11-23 6:48:39
Message-ID: AANLkTik8v=H=exeOO93Q_oOxVU=snck_SGWDXuy+-db7 () mail ! gmail ! com
[Download RAW message or body]

This is much better than last time but I still have questions...

2010/11/18 Alan Cox <alan@lxorguk.ukuu.org.uk>:

> +       /* 1. Init rx channel */
> +       rxs = &dw_dma->dmas_rx;
> +       ds = &rxs->dma_slave;
> +       ds->direction = DMA_FROM_DEVICE;
> +       rxs->hs_mode = LNW_DMA_HW_HS;
> +       rxs->cfg_mode = LNW_DMA_PER_TO_MEM;
> +       ds->src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +       ds->dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +       ds->src_maxburst = 16;
> +       ds->dst_maxburst = 16;

This is great stuff! That is exactly how ds is to be set up.
I would prefer that you don't dereference the this rxs thing here
but whatever.

> +       dma_cap_zero(mask);
> +       dma_cap_set(DMA_MEMCPY, mask);
> +       dma_cap_set(DMA_SLAVE, mask);

This is not elegant. Are you going to do memcpy() or slave
transfers? What you want to do is fix your DMA engine so that
just asking for DMA_SLAVE works.

> +       dma_cap_set(DMA_SLAVE, mask);
> +       dma_cap_set(DMA_MEMCPY, mask);

Here again...

> +static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
> +{
(...)
> +       flag = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +       if (dws->tx_dma) {
> +               txdesc = txchan->device->device_prep_dma_memcpy(txchan,
> +                               dws->dma_addr, dws->tx_dma,
> +                               dws->len, flag);
> +               txdesc->callback = dw_spi_dma_done;
> +               txdesc->callback_param = &dws->tx_param;
> +       }
> +
> +       /* 3. start the RX dma transfer */
> +       if (dws->rx_dma) {
> +               rxdesc = rxchan->device->device_prep_dma_memcpy(rxchan,
> +                               dws->rx_dma, dws->dma_addr,
> +                               dws->len, flag);
> +               rxdesc->callback = dw_spi_dma_done;
> +               rxdesc->callback_param = &dws->rx_param;
> +       }

Using device_prep_dma_memcpy() for this is still nonsense, it should be
device_prep_slave_sg().

I know the DMA driver needs fixing in order for this to work properly,
so why not fix it?

These are the most important concerns I raised last iteration, so
I challenge you to fix drivers/dma/dw_dmac.c or wherever the real
problem sits.

Can you describe where the problem with fixing this to use real
slave sglists is?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]