Re: [PATCH] dw_spi: add DMA support

* Re: [PATCH] dw_spi: add DMA support
       [not found] ` <20101118200935.26334.53708.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2010-11-23  6:48   ` Linus Walleij
       [not found]     ` <AANLkTik8v=H=exeOO93Q_oOxVU=snck_SGWDXuy+-db7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2010-11-23  6:48 UTC (permalink / raw)
  To: Alan Cox
  Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, spi mailing list, Dan Williams

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

2010/11/18 Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>:

> +       /* 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

^ permalink raw reply	[flat|nested] 9+ messages in thread