[PATCH 06/13] powerpc/5200: LocalPlus driver: map and unmap DMA areas

[Grant Likely]

On Tue, Dec 22, 2009 at 12:04 AM, Roman Fietze
<roman.fietze at telemotive.de> wrote:
>
> Signed-off-by: Roman Fietze <roman.fietze at telemotive.de>

Yes, this is definitely needed.  Please respin this patch and move it
earlier in your series so I can apply it to mainline.

More comments below.

g.

> ---
>  arch/powerpc/include/asm/mpc52xx.h            |    2 +-
>  arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   26 +++++++++++++++++++-----
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mpc52xx.h b/arch/powerpc/include/asm/mpc52xx.h
> index c659d1d..043458e 100644
> --- a/arch/powerpc/include/asm/mpc52xx.h
> +++ b/arch/powerpc/include/asm/mpc52xx.h
> @@ -347,7 +347,7 @@ struct mpc52xx_lpbfifo_request {
>
>        /* Memory address */
>        void *data;
> -       phys_addr_t data_phys;
> +       dma_addr_t data_dma;
>
>        /* Details of transfer */
>        size_t size;
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> index 1e4f725..8d8a63a 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
> +#include <linux/dma-mapping.h>
>  #include <asm/io.h>
>  #include <asm/prom.h>
>  #include <asm/mpc52xx.h>
> @@ -138,6 +139,7 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                        out_be32(&lpbfifo.regs->fifo_alarm, MPC52xx_SCLPC_FIFO_SIZE - 28);
>                        out_be32(&lpbfifo.regs->fifo_control, MPC52xx_SLPC_FIFO_CONTROL_GR(7));
>                        lpbfifo.bcom_cur_task = lpbfifo.bcom_tx_task;
> +                       req->data_dma = dma_map_single(lpbfifo.dev, req->data, req->size, DMA_TO_DEVICE);
>                } else {
>                        out_be32(&lpbfifo.regs->fifo_alarm, MPC52xx_SCLPC_FIFO_SIZE - 1);
>                        out_be32(&lpbfifo.regs->fifo_control, MPC52xx_SLPC_FIFO_CONTROL_GR(0));
> @@ -154,6 +156,7 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                                        lpbfifo.dma_irqs_enabled = 1;
>                                }
>                        }
> +                       req->data_dma = dma_map_single(lpbfifo.dev, req->data, req->size, DMA_FROM_DEVICE);
>                }

Need to ensure the return value != NULL

>
>                /* error irq & master enabled bit */
> @@ -161,7 +164,7 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>
>                bd = bcom_prepare_next_buffer(lpbfifo.bcom_cur_task);
>                bd->status = transfer_size;
> -               bd->data[0] = req->data_phys + req->pos;
> +               bd->data[0] = req->data_dma + req->pos;
>                bcom_submit_next_buffer(lpbfifo.bcom_cur_task, NULL);
>        }
>
> @@ -236,12 +239,13 @@ static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>        }
>
>        rflags = req->flags;
> +       status_count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
>
> -       /* check normal termination bit */
> +       /* Check normal termination bit */
>        if (!(status_count & MPC52xx_SCLPC_STATUS_NT))
>                goto out;
>
> -       /* check abort bit */
> +       /* Check abort bit */

unrelated changes

>        if (status_count & MPC52xx_SCLPC_STATUS_AT) {
>                out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
>                do_callback = 1;
> @@ -250,7 +254,7 @@ static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>
>        if (!mpc52xx_lpbfifo_is_dma(rflags)) {
>
> -               /* bytes done */
> +               /* Bytes done */

ditto

>                status_count &= MPC52xx_SCLPC_STATUS_BYTES_DONE_MASK;
>
>                if (!mpc52xx_lpbfifo_is_write(rflags)) {
> @@ -336,6 +340,16 @@ static irqreturn_t mpc52xx_lpbfifo_bcom_irq(int irq, void *dev_id)
>        bcom_retrieve_buffer(lpbfifo->bcom_cur_task, NULL, NULL);
>        // req->irq_ticks += get_tbl() - ts;
>
> +       if (lpbfifo->req) {
> +               if (mpc52xx_lpbfifo_is_write(lpbfifo->req->flags))
> +                       dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_TO_DEVICE);
> +               else
> +                       dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_FROM_DEVICE);
> +       } else
> +       {
> +               dev_err(lpbfifo->dev, "request is NULL\n");
> +       }
> +

The ->req pointer was verified earlier in this function.  It will
never be null here.

>        spin_unlock_irqrestore(&lpbfifo->lock, flags);
>
>        return IRQ_HANDLED;
> @@ -439,7 +453,7 @@ mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
>                goto err_irq;
>
>        /* Request the Bestcomm receive (fifo --> memory) task and IRQ */
> -       lpbfifo.bcom_rx_task = bcom_gen_bd_rx_init(16,
> +       lpbfifo.bcom_rx_task = bcom_gen_bd_rx_init(4,
>                                                   res.start + offsetof(struct mpc52xx_sclpc, fifo_data),
>                                                   BCOM_INITIATOR_SCLPC, BCOM_IPR_SCLPC,
>                                                   16*1024*1024);

Why?

> @@ -453,7 +467,7 @@ mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
>                goto err_bcom_rx_irq;
>
>        /* Request the Bestcomm transmit (memory --> fifo) task and IRQ */
> -       lpbfifo.bcom_tx_task = bcom_gen_bd_tx_init(16,
> +       lpbfifo.bcom_tx_task = bcom_gen_bd_tx_init(4,
>                                                   res.start + offsetof(struct mpc52xx_sclpc, fifo_data),
>                                                   BCOM_INITIATOR_SCLPC,
>                                                   BCOM_IPR_SCLPC);

Ditto.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.