[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA mode
Edgar E. Iglesias
Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model
Tue, 23 Feb 2021 10:33:06 +0100
On Tue, Feb 23, 2021 at 05:23:43PM +0800, Bin Meng wrote:
> Hi Edgar,
> On Tue, Feb 23, 2021 at 5:21 PM Edgar E. Iglesias
> <firstname.lastname@example.org> wrote:
> > On Mon, Feb 22, 2021 at 09:05:10PM +0800, Bin Meng wrote:
> > > From: Xuzhou Cheng <email@example.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 <firstname.lastname@example.org>
> > > Signed-off-by: Bin Meng <email@example.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?
It's not in silicon yet, yeah, we can add it later.
> > > 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.
This is a common interrupt handling pattern in most Xilinx devices.
You can look at the xlnx-zdma.c IMR, ISR, IEN, IDS regs as an example.
[PATCH v4 4/5] hw/ssi: xilinx_spips: Clean up coding convention issues, Bin Meng, 2021/02/22
[PATCH v4 5/5] hw/ssi: xilinx_spips: Remove DMA related dead codes from zynqmp_spips, Bin Meng, 2021/02/22