Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:05:10PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> 
> ZynqMP QSPI supports SPI transfer using DMA mode, but currently this
> is unimplemented. When QSPI is programmed to use DMA mode, QEMU will
> crash. This is observed when testing VxWorks 7.
> 
> This adds a Xilinx CSU DMA model and the implementation is based on
> https://github.com/Xilinx/qemu/blob/master/hw/dma/csu_stream_dma.c.
> The DST part of the model is verified along with ZynqMP GQSPI model.
> 
> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v4:
> - Add complete CSU DMA model based on Edgar's branch
> - Differences with Edgar's branch:
>   1. Match the registers' FIELD to UG1807.
>   2. Remove "byte-align" property. Per UG1807, SIZE and ADDR registers
>      must be word aligned.

The relaxation of alignment is a new feature, not included on the ZynqMP but
it will be included in future versions. Would be nice to keep it but we can
also add it later since it's not really related to QSPI.

>   3. Make the values of int_enable and int_disable mutually exclusive
>      otherwise IRQ cannot be delivered.

This doesn't sound right. The enable and disable regs are stateless.
They both indirectly modify the MASK register.

I.e, setting a bit in the enable register will clear the correspoding bit in the
mask register, atomically, without the need for read-modify-write of MASK.

The disable register does the opposite.

>   4. Clear int_status after int_disable is set.

This doesn't sound right either. Status is a w1c register, i.e bits get set
when the interrupt event happens in the DMA and bits only get cleared when
SW writes a 1 to the STATUS reg to clear bits (write one to clear, w1c).

Other than the interrupt issues, I think this looks good.

Cheers,
Edgar