[PATCH 04/13] mpc52xx: LocalPlus driver: rewrite interrupt routines, fix errors

Grant Likely grant.likely at secretlab.ca
Tue Jan 12 06:44:40 EST 2010


On Tue, Dec 22, 2009 at 12:01 AM, Roman Fietze
<roman.fietze at telemotive.de> wrote:
>
> Use SCLPC bit definitions from mpc52xx.h for better readability.

The changes of is_write etc. are intermingled with the functional
changes being made.  The functional behaviour of this thing is subtle,
and I'd prefer the stylistic stuff handled in a separate patch.

> Rewrite IRQ handlers, make them work for DMA.

Details please.  As far as my testing goes, dma irqs are working fine.

> Fix module unload error.

ditto here.

>
> Signed-off-by: Roman Fietze <roman.fietze at telemotive.de>
> ---
>  arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |  306 ++++++++++++-------------
>  1 files changed, 149 insertions(+), 157 deletions(-)
>
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> index 2763d5e..2fd1f3f 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> @@ -46,6 +46,34 @@ struct mpc52xx_lpbfifo {
>  /* The MPC5200 has only one fifo, so only need one instance structure */
>  static struct mpc52xx_lpbfifo lpbfifo;
>
> +
> +/**
> + * mpc52xx_lpbfifo_is_write - return true if it's a WRITE request
> + */
> +static inline int mpc52xx_lpbfifo_is_write(int flags)
> +{
> +       return flags & MPC52XX_LPBFIFO_FLAG_WRITE;
> +}
> +
> +
> +/**
> + * mpc52xx_lpbfifo_is_dma - return true if it's a DMA request
> + */
> +static inline int mpc52xx_lpbfifo_is_dma(int flags)
> +{
> +       return !(flags & MPC52XX_LPBFIFO_FLAG_NO_DMA);
> +}
> +
> +
> +/**
> + * mpc52xx_lpbfifo_is_poll_dma - return true if it's a polled DMA request
> + */
> +static inline int mpc52xx_lpbfifo_is_poll_dma(int flags)
> +{
> +       return flags & MPC52XX_LPBFIFO_FLAG_POLL_DMA;
> +}
> +
> +

I'm not (yet) convinced that adding these is a benefit.

>  /**
>  * mpc52xx_lpbfifo_kick - Trigger the next block of data to be transfered
>  */
> @@ -57,16 +85,23 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>        u32 *data;
>        int i;
>        int bit_fields;
> -       int dma = !(req->flags & MPC52XX_LPBFIFO_FLAG_NO_DMA);
> -       int write = req->flags & MPC52XX_LPBFIFO_FLAG_WRITE;
> -       int poll_dma = req->flags & MPC52XX_LPBFIFO_FLAG_POLL_DMA;
> +       int rflags = req->flags;
>
>        /* Set and clear the reset bits; is good practice in User Manual */
> -       out_be32(&lpbfifo.regs->enable, 0x01010000);
> +       out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
> +
> +       /* Set width, chip select and READ mode */
> +       out_be32(&lpbfifo.regs->start_address, req->offset + req->pos);
> +
> +       /* Set CS and BPT */
> +       bit_fields = MPC52xx_SCLPC_CONTROL_CS(req->cs) | 0x8;
> +       if (!(mpc52xx_lpbfifo_is_write(rflags))) {
> +               bit_fields |= MPC52xx_SCLPC_CONTROL_RWB_RECEIVE;        /* read mode */
> +               bit_fields |= MPC52xx_SCLPC_CONTROL_FLUSH;
> +       }
> +       out_be32(&lpbfifo.regs->control, bit_fields);
>
> -       /* set master enable bit */
> -       out_be32(&lpbfifo.regs->enable, 0x00000001);

My experimenting has found that clearing the reset bits and setting
the master enable bit is needed before programming the FIFO.  It looks
to me like this patch drops the above line which does so.

> -       if (!dma) {
> +       if (!mpc52xx_lpbfifo_is_dma(rflags)) {
>                /* While the FIFO can be setup for transfer sizes as large as
>                 * 16M-1, the FIFO itself is only 512 bytes deep and it does
>                 * not generate interrupts for FIFO full events (only transfer
> @@ -80,7 +115,7 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                        transfer_size = 512;
>
>                /* Load the FIFO with data */
> -               if (write) {
> +               if (mpc52xx_lpbfifo_is_write(rflags)) {
>                        reg = &lpbfifo.regs->fifo_data;
>                        data = req->data + req->pos;
>                        for (i = 0; i < transfer_size; i += 4)
> @@ -88,7 +123,9 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                }
>
>                /* Unmask both error and completion irqs */
> -               out_be32(&lpbfifo.regs->enable, 0x00000301);
> +               out_be32(&lpbfifo.regs->enable, (MPC52xx_SCLPC_ENABLE_AIE |
> +                                                MPC52xx_SCLPC_ENABLE_NIE |
> +                                                MPC52xx_SCLPC_ENABLE_ME));
>        } else {
>                /* Choose the correct direction
>                 *
> @@ -97,16 +134,16 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                 * there is a performance impacit.  However, if it is wrong there
>                 * is a risk of DMA not transferring the last chunk of data
>                 */
> -               if (write) {
> -                       out_be32(&lpbfifo.regs->fifo_alarm, 0x1e4);
> -                       out_8(&lpbfifo.regs->fifo_control, 7);
> +               if (mpc52xx_lpbfifo_is_write(rflags)) {
> +                       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;
>                } else {
> -                       out_be32(&lpbfifo.regs->fifo_alarm, 0x1ff);
> -                       out_8(&lpbfifo.regs->fifo_control, 0);
> +                       out_be32(&lpbfifo.regs->fifo_alarm, MPC52xx_SCLPC_FIFO_SIZE - 1);
> +                       out_be32(&lpbfifo.regs->fifo_control, MPC52xx_SLPC_FIFO_CONTROL_GR(0));

More stylistic changes in a patch that also changes functional
behaviour.  Please split into separate patches.

>                        lpbfifo.bcom_cur_task = lpbfifo.bcom_rx_task;
>
> -                       if (poll_dma) {
> +                       if (mpc52xx_lpbfifo_is_poll_dma(rflags)) {
>                                if (lpbfifo.dma_irqs_enabled) {
>                                        disable_irq(bcom_get_task_irq(lpbfifo.bcom_rx_task));
>                                        lpbfifo.dma_irqs_enabled = 0;
> @@ -119,63 +156,34 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                        }
>                }
>
> +               /* error irq & master enabled bit */
> +               out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_AIE | MPC52xx_SCLPC_ENABLE_NIE | MPC52xx_SCLPC_ENABLE_ME);
> +

You'll need to explain this change it greater detail.  The old code
only enabled the NIE irq when in non-polled DMA mode.  You're changing
it and you need to explain why.  Not just for me but for future
readers.

I'm stopping here on this particular patch.  A number of comments have
been removed that describe the behaviour of the driver without being
replaced with comments describing the new behaviour.  There are also
several whitespace and style only change that should be done in a
separate patch.  Keeping them separate means I can merge
uncontroversial stuff while still debating the other points.

Please respin and describe not just what you've change, but how you
changed it, why the change is needed, and it should be clear what the
new behaviour model is.

g.

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


More information about the Linuxppc-dev mailing list