Re: [Qemu-devel] [PATCH v1 1/1] xilinx_axidma.c: Fix up the stream_running() function

[Peter Crosthwaite]

On Wed, May 27, 2015 at 12:37 AM, Alistair Francis
<address@hidden> wrote:
> Previously the stream_running() function didn't check
> if the DMA was halted. This caused hangs in recent versions
> of MicroBlaze u-boot. Correct stream_running() to check
> DMASR_HALTED as well as DMACR_RUNSTOP.
>

So I'm stuggling with this one. Partly because I think HALTED might be
misimplemented in existing code. I did some digging, and AFAICS,
HALTED is conditional on !DAMCR_RUNSTOP. I think i might have got
210914e29975d17e635f9e8c1f7478c0ed7a208f wrong:

@@ -276,7 +276,7 @@ static void stream_process_mem2s(struct Stream *s,
         stream_desc_load(s, s->regs[R_CURDESC]);

         if (s->desc.status & SDESC_STATUS_COMPLETE) {
-            s->regs[R_DMASR] |= DMASR_IDLE;
+            s->regs[R_DMASR] |= DMASR_HALTED;
             break;
         }

Stepping back and ignoring the existing implementation of HALTED there
are 4 states of RS:H (RUNSTOP and HALTED):

!RS &&  H - this is the off state. doc refers to this as the "halted" state.
 RS && !H - This is the running state.
!RS && !H - This is the transient state. Software has cleared RS but
there s still something on AXI bus so cant assert halted yet.
 RS &&  H - This is an invalid state.

Current code reaches the invalid state on the ring buffer full case
due to the bug above. My thoery is
210914e29975d17e635f9e8c1f7478c0ed7a208f should have just been:

         if (s->desc.status & SDESC_STATUS_COMPLETE) {
-            s->regs[R_DMASR] |= DMASR_IDLE;
             break;
         }

Now I think there is yet another bug in that clearing RS doesn't seem
to be able to reliably set the HALTED bit (only in the unrelated case
of a ring buffer fill).

I'm starting to question whether the HALTED bit as far as QEMU is
concerned should just be a straight negation of RS. Depending on what
the conditions cause a transient and what doesn't, the transient as I
describe above may evaporate as we can get away with this simple
shortcut.

This would make this patch obsolete without fixing your bug :).

So running on the assumption that HALTED is misimplemented your patch
is doing something with that behaviour. The misimplemented HALTED is
currently holding the state of "we are blocked on a full buffer". If
you can point me which of the 3 call sites of stream_running was
giving you problems I might have more clues.

FYI you patch may still be correct but I wondering whether is has
uncovered a bug that should lead to a rework of this.

Regards,
Peter

> Signed-off-by: Alistair Francis <address@hidden>
> Reviewed-by: Sai Pavan Boddu <address@hidden>
> ---
>  hw/dma/xilinx_axidma.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index d06002d..27fba40 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -154,7 +154,8 @@ static inline int stream_resetting(struct Stream *s)
>
>  static inline int stream_running(struct Stream *s)
>  {
> -    return s->regs[R_DMACR] & DMACR_RUNSTOP;
> +    return s->regs[R_DMACR] & DMACR_RUNSTOP &&
> +           !(s->regs[R_DMASR] & DMASR_HALTED);
>  }
>
>  static inline int stream_idle(struct Stream *s)
> --
> 1.7.1
>
>