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

From: Bin Meng <bmeng.cn@gmail.com>
To: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Xuzhou Cheng <xuzhou.cheng@windriver.com>,
	Bin Meng <bin.meng@windriver.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	Francisco Iglesias <francisco.iglesias@xilinx.com>,
	qemu-arm <qemu-arm@nongnu.org>,
	Alistair Francis <alistair.francis@wdc.com>
Subject: Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model
Date: Tue, 23 Feb 2021 17:23:43 +0800	[thread overview]
Message-ID: <CAEUhbmX-nLxDYAHDZQga4ADpy2+2cnKsfxMYucxNC=zUdfFupw@mail.gmail.com> (raw)
In-Reply-To: <20210223092127.GU477672@toto>

Hi Edgar,

On Tue, Feb 23, 2021 at 5:21 PM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> 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.

I think Xilinx folks can add the "byte-align" property in the future
patches. Is this a new feature for Versal?

>
> >   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.

Without these interrupt fixes, our tests cannot pass. We will have a
further look at your comments.

Regards,
Bin