On Tue, Dec 22, 2009 at 12:01 AM, Roman Fietze
<roman.fie...@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.fie...@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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
