On Wed, May 27, 2015 at 12:37 AM, Alistair Francis
<alistair.fran...@xilinx.com> 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 <alistair.fran...@xilinx.com>
> Reviewed-by: Sai Pavan Boddu <saip...@xilinx.com>
> ---
> 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
>
>
