[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/8] Introduce xilinx dpdma.
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH 6/8] Introduce xilinx dpdma. |
|
Date: |
Mon, 18 May 2015 01:17:01 -0700 |
On Wed, May 13, 2015 at 12:12 PM, <address@hidden> wrote:
> From: KONRAD Frederic <address@hidden>
>
> This is the implementation of the DPDMA.
>
> Signed-off-by: KONRAD Frederic <address@hidden>
> ---
> hw/dma/Makefile.objs | 1 +
> hw/dma/xilinx_dpdma.c | 1149
> +++++++++++++++++++++++++++++++++++++++++++++++++
> hw/dma/xilinx_dpdma.h | 71 +++
> 3 files changed, 1221 insertions(+)
> create mode 100644 hw/dma/xilinx_dpdma.c
> create mode 100644 hw/dma/xilinx_dpdma.h
>
> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
> index 0e65ed0..7198e5a 100644
> --- a/hw/dma/Makefile.objs
> +++ b/hw/dma/Makefile.objs
> @@ -8,6 +8,7 @@ common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
> common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
> common-obj-$(CONFIG_STP2000) += sparc32_dma.o
> common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
> +common-obj-y += xilinx_dpdma.o
Conditional.
>
> obj-$(CONFIG_OMAP) += omap_dma.o soc_dma.o
> obj-$(CONFIG_PXA2XX) += pxa2xx_dma.o
> diff --git a/hw/dma/xilinx_dpdma.c b/hw/dma/xilinx_dpdma.c
> new file mode 100644
> index 0000000..6479148
> --- /dev/null
> +++ b/hw/dma/xilinx_dpdma.c
> @@ -0,0 +1,1149 @@
> +/*
> + * xilinx_dpdma.c
> + *
> + * Copyright (C) 2015 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: address@hidden
> + *
> + * Developed by :
> + * Frederic Konrad <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "xilinx_dpdma.h"
> +
> +#ifndef DEBUG_DPDMA
> +#define DEBUG_DPDMA 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) do {
> \
> + if (DEBUG_DPDMA) {
> \
> + qemu_log("xilinx_dpdma: " fmt , ## __VA_ARGS__);
> \
> + }
> \
> +} while (0);
> +
> +/*
> + * Registers offset for DPDMA.
> + */
> +#define DPDMA_ERR_CTRL (0x00000000)
With only a 0x1000 register address range shouldnt need the 8 hex
digits for offests.
> +#define DPDMA_ISR (0x00000004 >> 2)
> +#define DPDMA_IMR (0x00000008 >> 2)
> +#define DPDMA_IEN (0x0000000C >> 2)
> +#define DPDMA_IDS (0x00000010 >> 2)
> +#define DPDMA_EISR (0x00000014 >> 2)
> +#define DPDMA_EIMR (0x00000018 >> 2)
> +#define DPDMA_EIEN (0x0000001C >> 2)
> +#define DPDMA_EIDS (0x00000020 >> 2)
> +#define DPDMA_CNTL (0x00000100 >> 2)
> +#define DPDMA_GBL (0x00000104 >> 2)
> +#define DPDMA_ALC0_CNTL (0x00000108 >> 2)
> +#define DPDMA_ALC0_STATUS (0x0000010C >> 2)
> +#define DPDMA_ALC0_MAX (0x00000110 >> 2)
> +#define DPDMA_ALC0_MIN (0x00000114 >> 2)
> +#define DPDMA_ALC0_ACC (0x00000118 >> 2)
> +#define DPDMA_ALC0_ACC_TRAN (0x0000011C >> 2)
> +#define DPDMA_ALC1_CNTL (0x00000120 >> 2)
> +#define DPDMA_ALC1_STATUS (0x00000124 >> 2)
> +#define DPDMA_ALC1_MAX (0x00000128 >> 2)
> +#define DPDMA_ALC1_MIN (0x0000012C >> 2)
> +#define DPDMA_ALC1_ACC (0x00000130 >> 2)
> +#define DPDMA_ALC1_ACC_TRAN (0x00000134 >> 2)
> +#define DPDMA_CH0_DSCR_STRT_ADDRE (0x00000200 >> 2)
> +#define DPDMA_CH0_DSCR_STRT_ADDR (0x00000204 >> 2)
> +#define DPDMA_CH0_DSCR_NEXT_ADDRE (0x00000208 >> 2)
> +#define DPDMA_CH0_DSCR_NEXT_ADDR (0x0000020C >> 2)
> +#define DPDMA_CH0_PYLD_CUR_ADDRE (0x00000210 >> 2)
> +#define DPDMA_CH0_PYLD_CUR_ADDR (0x00000214 >> 2)
> +#define DPDMA_CH0_CNTL (0x00000218 >> 2)
> +#define DPDMA_CH0_STATUS (0x0000021C >> 2)
> +#define DPDMA_CH0_VDO (0x00000220 >> 2)
> +#define DPDMA_CH0_PYLD_SZ (0x00000224 >> 2)
> +#define DPDMA_CH0_DSCR_ID (0x00000228 >> 2)
These per-channel addresses should be collapsable using a macro:
#define DPDMA_DSCR_ID_CH(n) ((0x00000228 + n * 100) >> 2)
> +#define DPDMA_CH1_DSCR_STRT_ADDRE (0x00000300 >> 2)
> +#define DPDMA_CH1_DSCR_STRT_ADDR (0x00000304 >> 2)
> +#define DPDMA_CH1_DSCR_NEXT_ADDRE (0x00000308 >> 2)
> +#define DPDMA_CH1_DSCR_NEXT_ADDR (0x0000030C >> 2)
> +#define DPDMA_CH1_PYLD_CUR_ADDRE (0x00000310 >> 2)
> +#define DPDMA_CH1_PYLD_CUR_ADDR (0x00000314 >> 2)
> +#define DPDMA_CH1_CNTL (0x00000318 >> 2)
> +#define DPDMA_CH1_STATUS (0x0000031C >> 2)
> +#define DPDMA_CH1_VDO (0x00000320 >> 2)
> +#define DPDMA_CH1_PYLD_SZ (0x00000324 >> 2)
> +#define DPDMA_CH1_DSCR_ID (0x00000328 >> 2)
> +#define DPDMA_CH2_DSCR_STRT_ADDRE (0x00000400 >> 2)
> +#define DPDMA_CH2_DSCR_STRT_ADDR (0x00000404 >> 2)
> +#define DPDMA_CH2_DSCR_NEXT_ADDRE (0x00000408 >> 2)
> +#define DPDMA_CH2_DSCR_NEXT_ADDR (0x0000040C >> 2)
> +#define DPDMA_CH2_PYLD_CUR_ADDRE (0x00000410 >> 2)
> +#define DPDMA_CH2_PYLD_CUR_ADDR (0x00000414 >> 2)
> +#define DPDMA_CH2_CNTL (0x00000418 >> 2)
> +#define DPDMA_CH2_STATUS (0x0000041C >> 2)
> +#define DPDMA_CH2_VDO (0x00000420 >> 2)
> +#define DPDMA_CH2_PYLD_SZ (0x00000424 >> 2)
> +#define DPDMA_CH2_DSCR_ID (0x00000428 >> 2)
> +#define DPDMA_CH3_DSCR_STRT_ADDRE (0x00000500 >> 2)
> +#define DPDMA_CH3_DSCR_STRT_ADDR (0x00000504 >> 2)
> +#define DPDMA_CH3_DSCR_NEXT_ADDRE (0x00000508 >> 2)
> +#define DPDMA_CH3_DSCR_NEXT_ADDR (0x0000050C >> 2)
> +#define DPDMA_CH3_PYLD_CUR_ADDRE (0x00000510 >> 2)
> +#define DPDMA_CH3_PYLD_CUR_ADDR (0x00000514 >> 2)
> +#define DPDMA_CH3_CNTL (0x00000518 >> 2)
> +#define DPDMA_CH3_STATUS (0x0000051C >> 2)
> +#define DPDMA_CH3_VDO (0x00000520 >> 2)
> +#define DPDMA_CH3_PYLD_SZ (0x00000524 >> 2)
> +#define DPDMA_CH3_DSCR_ID (0x00000528 >> 2)
> +#define DPDMA_CH4_DSCR_STRT_ADDRE (0x00000600 >> 2)
> +#define DPDMA_CH4_DSCR_STRT_ADDR (0x00000604 >> 2)
> +#define DPDMA_CH4_DSCR_NEXT_ADDRE (0x00000608 >> 2)
> +#define DPDMA_CH4_DSCR_NEXT_ADDR (0x0000060C >> 2)
> +#define DPDMA_CH4_PYLD_CUR_ADDRE (0x00000610 >> 2)
> +#define DPDMA_CH4_PYLD_CUR_ADDR (0x00000614 >> 2)
> +#define DPDMA_CH4_CNTL (0x00000618 >> 2)
> +#define DPDMA_CH4_STATUS (0x0000061C >> 2)
> +#define DPDMA_CH4_VDO (0x00000620 >> 2)
> +#define DPDMA_CH4_PYLD_SZ (0x00000624 >> 2)
> +#define DPDMA_CH4_DSCR_ID (0x00000628 >> 2)
> +#define DPDMA_CH5_DSCR_STRT_ADDRE (0x00000700 >> 2)
> +#define DPDMA_CH5_DSCR_STRT_ADDR (0x00000704 >> 2)
> +#define DPDMA_CH5_DSCR_NEXT_ADDRE (0x00000708 >> 2)
> +#define DPDMA_CH5_DSCR_NEXT_ADDR (0x0000070C >> 2)
> +#define DPDMA_CH5_PYLD_CUR_ADDRE (0x00000710 >> 2)
> +#define DPDMA_CH5_PYLD_CUR_ADDR (0x00000714 >> 2)
> +#define DPDMA_CH5_CNTL (0x00000718 >> 2)
> +#define DPDMA_CH5_STATUS (0x0000071C >> 2)
> +#define DPDMA_CH5_VDO (0x00000720 >> 2)
> +#define DPDMA_CH5_PYLD_SZ (0x00000724 >> 2)
> +#define DPDMA_CH5_DSCR_ID (0x00000728 >> 2)
> +#define DPDMA_ECO (0x00000FFC >> 2)
> +
Drop ECO field.
> +/*
> + * Descriptor control field.
> + */
> +#define CONTROL_PREAMBLE_VALUE 0xA5
> +
> +#define CONTROL_PREAMBLE 0xFF
> +#define EN_DSCR_DONE_INTR (1 << 8)
> +#define EN_DSCR_UPDATE (1 << 9)
> +#define IGNORE_DONE (1 << 10)
> +#define AXI_BURST_TYPE (1 << 11)
> +#define AXCACHE (0x0F << 12)
> +#define AXPROT (0x2 << 16)
> +#define DESCRIPTOR_MODE (1 << 18)
> +#define LAST_DESCRIPTOR (1 << 19)
> +#define ENABLE_CRC (1 << 20)
> +#define LAST_DESCRIPTOR_OF_FRAME (1 << 21)
> +
> +typedef enum DPDMABurstType {
> + DPDMA_INCR = 0,
> + DPDMA_FIXED = 1
> +} DPDMABurstType;
> +
> +typedef enum DPDMAMode {
> + DPDMA_CONTIGOUS = 0,
> + DPDMA_FRAGMENTED = 1
> +} DPDMAMode;
> +
> +typedef struct DPDMADescriptor {
> + uint32_t control;
> + uint32_t descriptor_id;
> + /* transfer size in byte. */
> + uint32_t xfer_size;
> + uint32_t line_size_stride;
> + uint32_t timestamp_lsb;
> + uint32_t timestamp_msb;
> + /* contains extension for both descriptor and source. */
> + uint32_t address_extension;
> + uint32_t next_descriptor;
> + uint32_t source_address;
> + uint32_t address_extension_23;
> + uint32_t address_extension_45;
> + uint32_t source_address2;
> + uint32_t source_address3;
> + uint32_t source_address4;
> + uint32_t source_address5;
> + uint32_t crc;
> +} DPDMADescriptor;
> +
> +static bool xilinx_dpdma_desc_is_last(DPDMADescriptor *desc)
> +{
> + return ((desc->control & 0x00080000) != 0);
Single bit extract32s are cleaner IMO (check the AArch64 translate
code where they started using them).
> +}
> +
> +static bool xilinx_dpdma_desc_is_last_of_frame(DPDMADescriptor *desc)
> +{
> + return ((desc->control & 0x00200000) != 0);
> +}
> +
> +static uint64_t xilinx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
> + uint8_t frag)
> +{
> + uint64_t addr = 0;
> + assert(frag < 5);
> +
> + switch (frag) {
> + case 0:
> + addr = desc->source_address
> + + (extract32(desc->address_extension, 16, 12) << 20);
> + break;
> + case 1:
> + addr = desc->source_address2
> + + (extract32(desc->address_extension_23, 0, 12) << 8);
> + break;
> + case 2:
> + addr = desc->source_address3
> + + (extract32(desc->address_extension_23, 16, 12) << 20);
> + break;
> + case 3:
> + addr = desc->source_address4
> + + (extract32(desc->address_extension_45, 0, 12) << 8);
> + break;
> + case 4:
> + addr = desc->source_address5
> + + (extract32(desc->address_extension_45, 16, 12) << 20);
> + break;
> + default:
> + addr = 0;
> + break;
> + }
> +
> + return addr;
> +}
> +
> +static uint32_t xilinx_dpdma_desc_get_transfer_size(DPDMADescriptor *desc)
> +{
> + return desc->xfer_size;
> +}
> +
> +static uint32_t xilinx_dpdma_desc_get_line_size(DPDMADescriptor *desc)
> +{
> + return desc->line_size_stride & 0x3FFFF;
extract.
> +}
> +
> +static uint32_t xilinx_dpdma_desc_get_line_stride(DPDMADescriptor *desc)
> +{
> + return (desc->line_size_stride >> 18) * 16;
extract.
> +}
> +
> +static inline bool xilinx_dpdma_desc_crc_enabled(DPDMADescriptor *desc)
> +{
> + return ((desc->control & (1 << 20)) != 0);
> +}
> +
> +static inline bool xilinx_dpdma_desc_check_crc(DPDMADescriptor *desc)
> +{
> + uint32_t *p = (uint32_t *)(desc);
parenthesis not needed.
> + uint32_t crc = 0;
> + uint8_t i;
> +
> + for (i = 0; i < 15; i++) {
Does 15 need a macro? Is it the descriptor length?
> + crc += p[i];
> + }
> +
> + return (crc == desc->crc);
> +}
> +
> +static inline bool xilinx_dpdma_desc_completion_interrupt(DPDMADescriptor
> *desc)
> +{
> + return ((desc->control & (1 << 8)) != 0);
> +}
> +
> +static inline bool xilinx_dpdma_desc_is_valid(DPDMADescriptor *desc)
> +{
> + return ((desc->control & 0xFF) == 0xA5);
> +}
> +
> +static inline bool xilinx_dpdma_desc_is_contiguous(DPDMADescriptor *desc)
> +{
> + return ((desc->control & 0x00040000) == 0);
> +}
> +
> +static inline bool xilinx_dpdma_desc_update_enabled(DPDMADescriptor *desc)
> +{
> + return ((desc->control & (1 << 9)) != 0);
> +}
> +
> +static inline void xilinx_dpdma_desc_set_done(DPDMADescriptor *desc)
> +{
> + desc->timestamp_msb |= (1 << 31);
> +}
> +
> +static inline bool xilinx_dpdma_desc_is_already_done(DPDMADescriptor *desc)
> +{
> + return ((desc->timestamp_msb & (1 << 31)) != 0);
> +}
> +
> +static inline bool xilinx_dpdma_desc_ignore_done_bit(DPDMADescriptor *desc)
> +{
> + return ((desc->control & (1 << 10)) != 0);
> +}
> +
> +static const VMStateDescription vmstate_xilinx_dpdma = {
> + .name = TYPE_XILINX_DPDMA,
> + .version_id = 1,
> + .fields = (VMStateField[]) {
> +
I think this needs population?
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void xilinx_dpdma_update_irq(XilinxDPDMAState *s)
> +{
> + uint32_t flags;
> +
> + flags = ((s->registers[DPDMA_ISR] & (~s->registers[DPDMA_IMR]))
> + | (s->registers[DPDMA_EISR] & (~s->registers[DPDMA_EIMR])));
> + qemu_set_irq(s->irq, flags != 0);
> +}
> +
> +static uint64_t xilinx_dpdma_descriptor_start_address(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + switch (channel) {
> + case 0:
> + return (s->registers[DPDMA_CH0_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH0_DSCR_STRT_ADDR];
> + break;
> + case 1:
> + return (s->registers[DPDMA_CH1_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH1_DSCR_STRT_ADDR];
> + break;
> + case 2:
> + return (s->registers[DPDMA_CH2_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH2_DSCR_STRT_ADDR];
> + break;
> + case 3:
> + return (s->registers[DPDMA_CH3_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH3_DSCR_STRT_ADDR];
> + break;
> + case 4:
> + return (s->registers[DPDMA_CH4_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH4_DSCR_STRT_ADDR];
> + break;
> + case 5:
> + return (s->registers[DPDMA_CH5_DSCR_STRT_ADDRE] << 16)
> + + s->registers[DPDMA_CH5_DSCR_STRT_ADDR];
> + break;
Can the 6X repetition be collapsed using some indexing math?
> + default:
> + /* Should not happen. */
> + return 0;
> + break;
> + }
> +}
> +
> +static uint64_t xilinx_dpdma_descriptor_next_address(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + switch (channel) {
> + case 0:
> + return ((uint64_t)s->registers[DPDMA_CH0_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH0_DSCR_NEXT_ADDR];
> + break;
> + case 1:
> + return ((uint64_t)s->registers[DPDMA_CH1_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH1_DSCR_NEXT_ADDR];
> + break;
> + case 2:
> + return ((uint64_t)s->registers[DPDMA_CH2_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH2_DSCR_NEXT_ADDR];
> + break;
> + case 3:
> + return ((uint64_t)s->registers[DPDMA_CH3_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH3_DSCR_NEXT_ADDR];
> + break;
> + case 4:
> + return ((uint64_t)s->registers[DPDMA_CH4_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH4_DSCR_NEXT_ADDR];
> + break;
> + case 5:
> + return ((uint64_t)s->registers[DPDMA_CH5_DSCR_NEXT_ADDRE] << 32)
> + + s->registers[DPDMA_CH5_DSCR_NEXT_ADDR];
> + break;
same.
> + default:
> + /* Should not happen. */
> + return 0;
> + break;
> + }
> +}
> +
> +static inline void xilinx_dpdma_set_desc_next_address(XilinxDPDMAState *s,
> + uint8_t channel,
> + uint64_t addr)
> +{
> + switch (channel) {
> + case 0:
> + s->registers[DPDMA_CH0_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH0_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
extract64
> + break;
> + case 1:
> + s->registers[DPDMA_CH1_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH1_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
> + break;
> + case 2:
> + s->registers[DPDMA_CH2_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH2_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
> + break;
> + case 3:
> + s->registers[DPDMA_CH3_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH3_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
> + break;
> + case 4:
> + s->registers[DPDMA_CH4_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH4_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
> + break;
> + case 5:
> + s->registers[DPDMA_CH5_DSCR_NEXT_ADDRE] = addr >> 32;
> + s->registers[DPDMA_CH5_DSCR_NEXT_ADDR] = addr & 0xFFFFFFFF;
> + break;
repetition.
> + default:
> + /* Should not happen. */
> + break;
> + }
> +}
> +
> +static bool xilinx_dpdma_is_channel_enabled(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + switch (channel) {
> + case 0:
> + return ((s->registers[DPDMA_CH0_CNTL] & 0x01) != 0);
0x1 needs macro definition.
> + break;
unreachable break statements.
> + case 1:
> + return ((s->registers[DPDMA_CH1_CNTL] & 0x01) != 0);
> + break;
> + case 2:
> + return ((s->registers[DPDMA_CH2_CNTL] & 0x01) != 0);
> + break;
> + case 3:
> + return ((s->registers[DPDMA_CH3_CNTL] & 0x01) != 0);
> + break;
> + case 4:
> + return ((s->registers[DPDMA_CH4_CNTL] & 0x01) != 0);
> + break;
> + case 5:
> + return ((s->registers[DPDMA_CH5_CNTL] & 0x01) != 0);
> + break;
> + default:
> + /* Should not happen. */
> + return 0;
> + break;
> + }
> +}
> +
> +static bool xilinx_dpdma_is_channel_paused(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + switch (channel) {
> + case 0:
> + return ((s->registers[DPDMA_CH0_CNTL] & 0x02) != 0);
> + break;
> + case 1:
> + return ((s->registers[DPDMA_CH1_CNTL] & 0x02) != 0);
> + break;
> + case 2:
> + return ((s->registers[DPDMA_CH2_CNTL] & 0x02) != 0);
> + break;
> + case 3:
> + return ((s->registers[DPDMA_CH3_CNTL] & 0x02) != 0);
> + break;
> + case 4:
> + return ((s->registers[DPDMA_CH4_CNTL] & 0x02) != 0);
> + break;
> + case 5:
> + return ((s->registers[DPDMA_CH5_CNTL] & 0x02) != 0);
> + break;
Same comments as above.
> + default:
> + /* Should not happen. */
> + return 0;
> + break;
> + }
> +}
> +
> +static inline bool xilinx_dpdma_is_channel_retriggered(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + return s->registers[DPDMA_GBL] & ((1 << 6) << channel);
> +}
> +
> +static inline bool xilinx_dpdma_is_channel_triggered(XilinxDPDMAState *s,
> + uint8_t channel)
> +{
> + return s->registers[DPDMA_GBL] & (1 << channel);
> +}
> +
> +static void xilinx_dpdma_update_desc_info(XilinxDPDMAState *s, uint8_t
> channel,
> + DPDMADescriptor *desc)
> +{
> + switch (channel) {
> + case 0:
> + s->registers[DPDMA_CH0_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
extract for consistency with code below.
> + s->registers[DPDMA_CH0_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH0_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH0_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH0_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH0_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH0_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH0_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 11);
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH0_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
What does this mean?
> + break;
> + case 1:
> + s->registers[DPDMA_CH1_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
> + s->registers[DPDMA_CH1_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH1_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH1_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH1_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH1_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH1_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH1_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 11);
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH1_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
> + break;
> + case 2:
> + s->registers[DPDMA_CH2_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
> + s->registers[DPDMA_CH2_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH2_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH2_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH2_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH2_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH2_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH2_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 11);
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH2_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
> + break;
Ok definately want to do something about the repetition on this one.
If the variable numbers are regular use math to calculate them. If
they are irregular make some data table arrays that can be indexed on
the switch variable.
> + case 3:
> + s->registers[DPDMA_CH3_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
> + s->registers[DPDMA_CH3_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH3_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH3_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH3_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH3_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH3_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH3_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 11);
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH3_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
> + break;
> + case 4:
> + s->registers[DPDMA_CH4_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
> + s->registers[DPDMA_CH4_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH4_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH4_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH4_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH4_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH4_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH4_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 11);
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH4_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
> + break;
> + case 5:
> + s->registers[DPDMA_CH5_DSCR_NEXT_ADDRE] = desc->address_extension
> + & 0x0000FFFF;
> + s->registers[DPDMA_CH5_DSCR_NEXT_ADDR] = desc->next_descriptor;
> + s->registers[DPDMA_CH5_PYLD_CUR_ADDRE] =
> + extract32(desc->address_extension, 16,
> 16);
> + s->registers[DPDMA_CH5_PYLD_CUR_ADDR] = desc->source_address;
> + s->registers[DPDMA_CH5_VDO] = extract32(desc->line_size_stride, 18,
> 14)
> + + (extract32(desc->line_size_stride, 0,
> 18)
> + << 14);
> + s->registers[DPDMA_CH5_PYLD_SZ] = desc->xfer_size;
> + s->registers[DPDMA_CH5_DSCR_ID] = desc->descriptor_id;
> +
> + /* Compute the status register with the descriptor information. */
> + s->registers[DPDMA_CH5_STATUS] = (desc->control & 0xFF) << 13;
> + if ((desc->control & (1 << 8)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 12);
> + }
> + if ((desc->control & (1 << 9)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 11);
Number 9 and 11 and those below need macroification.
> + }
> + if ((desc->timestamp_msb & (1 << 31)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 10);
> + }
> + if ((desc->control & (1 << 10)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 9);
> + }
> + if ((desc->control & (1 << 21)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 8);
> + }
> + if ((desc->control & (1 << 19)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 7);
> + }
> + if ((desc->control & (1 << 20)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 6);
> + }
> + if ((desc->control & (1 << 18)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 5);
> + }
> + if ((desc->control & (1 << 11)) != 0) {
> + s->registers[DPDMA_CH5_STATUS] |= (1 << 4);
> + }
> + /* XXX: BURST_LEN? */
> + break;
> + default:
> + break;
> + }
> +}
> +
> +#ifdef DEBUG_DPDMA
> +static void xilinx_dpdma_dump_descriptor(DPDMADescriptor *desc)
> +{
> + uint8_t *p = ((uint8_t *)(desc));
Two sets of unneeded parenthesis.
> + size_t i;
> +
> + qemu_log("DUMP DESCRIPTOR:\n");
> + for (i = 0; i < 64; i++) {
> + qemu_log(" 0x%2.2X", *p++);
The 0x infront of each byte is hard to read.
Would you need a PRIx8 here?
> + if (((i + 1) % 4) == 0) {
> + qemu_log("\n");
> + }
> + }
qemu_hexdump can do this though.
> +}
> +#endif
> +
> +static uint64_t xilinx_dpdma_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + XilinxDPDMAState *s = XILINX_DPDMA(opaque);
Blank line.
> + assert(size == 4);
> + assert((offset % 4) == 0);
Memory API can enforce this with the MemoryRegionOps and assertions
can be dropped.
> + offset = offset >> 2;
> + DPRINTF("read @%" PRIx64 "\n", offset << 2);
> +
> + switch (offset) {
> + /*
> + * Trying to read a write only register.
> + */
> + case DPDMA_GBL:
> + return 0;
> + break;
Unreachable break.
> + default:
> + assert(offset <= (0xFFC >> 2));
> + return s->registers[offset];
> + break;
> + }
> + return 0;
> +}
> +
> +static void xilinx_dpdma_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + XilinxDPDMAState *s = XILINX_DPDMA(opaque);
> + assert(size == 4);
> + assert((offset % 4) == 0);
Same comments.
> + offset = offset >> 2;
> + DPRINTF("write @%" PRIx64 " = 0x%8.8lX\n", offset << 2, value);
Formats dont look right. Should it be a HWADDR_PRIx and then a PRIx64?
Print first then shift.
> +
> + switch (offset) {
> + case DPDMA_ISR:
> + value = ~value;
> + s->registers[DPDMA_ISR] &= value;
&= ~value to save a LOC.
> + xilinx_dpdma_update_irq(s);
> + break;
> + case DPDMA_IEN:
> + value = ~value;
> + s->registers[DPDMA_IMR] &= value;
> + break;
> + case DPDMA_IDS:
> + s->registers[DPDMA_IMR] |= value;
> + break;
> + case DPDMA_EISR:
> + value = ~value;
> + s->registers[DPDMA_EISR] &= value;
> + xilinx_dpdma_update_irq(s);
> + break;
> + case DPDMA_EIEN:
> + value = ~value;
> + s->registers[DPDMA_EIMR] &= value;
> + break;
> + case DPDMA_EIDS:
> + s->registers[DPDMA_EIMR] |= value;
> + break;
> + case DPDMA_IMR:
> + case DPDMA_EIMR:
> + case DPDMA_CH0_DSCR_NEXT_ADDRE:
> + case DPDMA_CH0_DSCR_NEXT_ADDR:
> + case DPDMA_CH1_DSCR_NEXT_ADDRE:
> + case DPDMA_CH1_DSCR_NEXT_ADDR:
> + case DPDMA_CH2_DSCR_NEXT_ADDRE:
> + case DPDMA_CH2_DSCR_NEXT_ADDR:
> + case DPDMA_CH3_DSCR_NEXT_ADDRE:
> + case DPDMA_CH3_DSCR_NEXT_ADDR:
> + case DPDMA_CH4_DSCR_NEXT_ADDRE:
> + case DPDMA_CH4_DSCR_NEXT_ADDR:
> + case DPDMA_CH5_DSCR_NEXT_ADDRE:
> + case DPDMA_CH5_DSCR_NEXT_ADDR:
> + case DPDMA_CH0_PYLD_CUR_ADDRE:
> + case DPDMA_CH0_PYLD_CUR_ADDR:
> + case DPDMA_CH1_PYLD_CUR_ADDRE:
> + case DPDMA_CH1_PYLD_CUR_ADDR:
> + case DPDMA_CH2_PYLD_CUR_ADDRE:
> + case DPDMA_CH2_PYLD_CUR_ADDR:
> + case DPDMA_CH3_PYLD_CUR_ADDRE:
> + case DPDMA_CH3_PYLD_CUR_ADDR:
> + case DPDMA_CH4_PYLD_CUR_ADDRE:
> + case DPDMA_CH4_PYLD_CUR_ADDR:
> + case DPDMA_CH5_PYLD_CUR_ADDRE:
> + case DPDMA_CH5_PYLD_CUR_ADDR:
> + case DPDMA_CH0_STATUS:
> + case DPDMA_CH1_STATUS:
> + case DPDMA_CH2_STATUS:
> + case DPDMA_CH3_STATUS:
> + case DPDMA_CH4_STATUS:
> + case DPDMA_CH5_STATUS:
> + case DPDMA_CH0_VDO:
> + case DPDMA_CH1_VDO:
> + case DPDMA_CH2_VDO:
> + case DPDMA_CH3_VDO:
> + case DPDMA_CH4_VDO:
> + case DPDMA_CH5_VDO:
> + case DPDMA_CH0_PYLD_SZ:
> + case DPDMA_CH1_PYLD_SZ:
> + case DPDMA_CH2_PYLD_SZ:
> + case DPDMA_CH3_PYLD_SZ:
> + case DPDMA_CH4_PYLD_SZ:
> + case DPDMA_CH5_PYLD_SZ:
> + case DPDMA_CH0_DSCR_ID:
> + case DPDMA_CH1_DSCR_ID:
> + case DPDMA_CH2_DSCR_ID:
> + case DPDMA_CH3_DSCR_ID:
> + case DPDMA_CH4_DSCR_ID:
> + case DPDMA_CH5_DSCR_ID:
Any opportunities for case FOO ... BAR in this?
> + /*
> + * Trying to write to a read only register..
> + */
> + break;
> + case DPDMA_GBL:
> + /*
> + * This is a write only register so it's read as zero in the read
> + * callback.
> + * We store the value anyway so we can know if the channel is
> + * enabled.
> + */
> + s->registers[offset] = value & 0x00000FFF;
> + break;
> + case DPDMA_CH0_DSCR_STRT_ADDRE:
> + case DPDMA_CH1_DSCR_STRT_ADDRE:
> + case DPDMA_CH2_DSCR_STRT_ADDRE:
> + case DPDMA_CH3_DSCR_STRT_ADDRE:
> + case DPDMA_CH4_DSCR_STRT_ADDRE:
> + case DPDMA_CH5_DSCR_STRT_ADDRE:
> + value &= 0x0000FFFF;
> + s->registers[offset] = value;
> + break;
> + case DPDMA_CH0_CNTL:
> + case DPDMA_CH1_CNTL:
> + case DPDMA_CH2_CNTL:
> + case DPDMA_CH3_CNTL:
> + case DPDMA_CH4_CNTL:
> + case DPDMA_CH5_CNTL:
> + value &= 0x3FFFFFFF;
> + s->registers[offset] = value;
> + break;
> + default:
> + assert(offset <= (0xFFC >> 2));
> + s->registers[offset] = value;
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps dma_ops = {
> + .read = xilinx_dpdma_read,
> + .write = xilinx_dpdma_write,
The MMIO size and alignment restrictions go here.
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void xilinx_dpdma_init(Object *obj)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> + XilinxDPDMAState *s = XILINX_DPDMA(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &dma_ops, s,
> + TYPE_XILINX_DPDMA, 0x1000);
> + sysbus_init_mmio(sbd, &s->iomem);
> + sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void xilinx_dpdma_reset(DeviceState *dev)
> +{
> + XilinxDPDMAState *s = XILINX_DPDMA(dev);
blank line.
> + memset(s->registers, 0, sizeof(s->registers));
> + s->registers[DPDMA_IMR] = 0x07FFFFFF;
> + s->registers[DPDMA_EIMR] = 0xFFFFFFFF;
> + s->registers[DPDMA_ALC0_MIN] = 0x0000FFFF;
> + s->registers[DPDMA_ALC1_MIN] = 0x0000FFFF;
> +}
> +
> +static void xilinx_dpdma_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->vmsd = &vmstate_xilinx_dpdma;
> + dc->reset = xilinx_dpdma_reset;
> +}
> +
> +static const TypeInfo xilinx_dpdma_info = {
> + .name = TYPE_XILINX_DPDMA,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(XilinxDPDMAState),
> + .instance_init = xilinx_dpdma_init,
> + .class_init = xilinx_dpdma_class_init,
> +};
> +
> +static void xilinx_dpdma_register_types(void)
> +{
> + type_register_static(&xilinx_dpdma_info);
> +}
> +
> +size_t xilinx_dpdma_start_operation(XilinxDPDMAState *s, uint8_t channel,
> + bool one_desc)
> +{
> + uint64_t desc_addr;
> + uint64_t source_addr[6];
> + DPDMADescriptor desc;
> + bool done = false;
> + size_t ptr = 0;
> +
> + assert(channel <= 5);
> +
> + if (channel == 3) {
> + s->registers[DPDMA_ISR] |= (1 << 27);
> + xilinx_dpdma_update_irq(s);
> + }
Interesting special case for #3. does it need a comment?
> +
> + DPRINTF("dpdma_start_channel() on channel %u\n", channel);
> +
> + if (!xilinx_dpdma_is_channel_triggered(s, channel)) {
> + DPRINTF("Channel isn't triggered..\n");
> + return 0;
> + }
> +
> + if (!xilinx_dpdma_is_channel_enabled(s, channel)) {
> + DPRINTF("Channel isn't enabled..\n");
> + return 0;
> + }
> +
> + if (xilinx_dpdma_is_channel_paused(s, channel)) {
> + DPRINTF("Channel is paused..\n");
> + return 0;
> + }
> +
> + do {
> + if ((s->operation_finished[channel])
> + || xilinx_dpdma_is_channel_retriggered(s, channel)) {
> + desc_addr = xilinx_dpdma_descriptor_start_address(s, channel);
> + s->operation_finished[channel] = false;
> + } else {
> + desc_addr = xilinx_dpdma_descriptor_next_address(s, channel);
> + }
> +
> + if (dma_memory_read(&address_space_memory, desc_addr, &desc,
> + sizeof(DPDMADescriptor))) {
> + s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
> + xilinx_dpdma_update_irq(s);
> + s->operation_finished[channel] = true;
> + DPRINTF("Can't get the descriptor.\n");
> + break;
> + }
> +
> + xilinx_dpdma_update_desc_info(s, channel, &desc);
> +
> + #ifdef DEBUG_DPDMA
No leading whitespace before #if
> + xilinx_dpdma_dump_descriptor(&desc);
> + #endif
> +
> + DPRINTF("location of the descriptor: 0x%8.8lx\n", desc_addr);
Should this be PRIx64? (there's more of these below).
> + if (!xilinx_dpdma_desc_is_valid(&desc)) {
> + s->registers[DPDMA_EISR] |= ((1 << 7) << channel);
> + xilinx_dpdma_update_irq(s);
> + s->operation_finished[channel] = true;
> + DPRINTF("Invalid descriptor..\n");
> + break;
> + }
> +
> + if (xilinx_dpdma_desc_crc_enabled(&desc)
> + & !xilinx_dpdma_desc_check_crc(&desc)) {
&&
> + s->registers[DPDMA_EISR] |= ((1 << 13) << channel);
> + xilinx_dpdma_update_irq(s);
> + s->operation_finished[channel] = true;
> + DPRINTF("Bad CRC for descriptor..\n");
> + break;
> + }
> +
> + if (xilinx_dpdma_desc_is_already_done(&desc)
> + && !xilinx_dpdma_desc_ignore_done_bit(&desc)) {
> + /* We are trying to process an already processed descriptor. */
> + s->registers[DPDMA_EISR] |= ((1 << 25) << channel);
> + xilinx_dpdma_update_irq(s);
> + s->operation_finished[channel] = true;
> + DPRINTF("Already processed descriptor..\n");
> + break;
> + }
> +
> + done = xilinx_dpdma_desc_is_last(&desc)
> + | xilinx_dpdma_desc_is_last_of_frame(&desc);
||. these "is" function results should be treated as logicals.
> +
> + s->operation_finished[channel] = done;
> + if (s->data[channel]) {
> + int64_t transfer_len =
> + xilinx_dpdma_desc_get_transfer_size(&desc);
> + uint32_t line_size = xilinx_dpdma_desc_get_line_size(&desc);
> + uint32_t line_stride = xilinx_dpdma_desc_get_line_stride(&desc);
> + if (xilinx_dpdma_desc_is_contiguous(&desc)) {
> + source_addr[0] =
> + xilinx_dpdma_desc_get_source_address(&desc, 0);
> + while (transfer_len != 0) {
> + if (dma_memory_read(&address_space_memory,
> + source_addr[0],
> + &(s->data[channel][ptr]),
parenthesis not needed.
> + line_size)) {
> + s->registers[DPDMA_ISR] |= ((1 << 12) << channel);
> + xilinx_dpdma_update_irq(s);
> + DPRINTF("Can't get data.\n");
> + break;
> + }
> + ptr += line_size;
> + transfer_len -= line_size;
> + source_addr[0] += line_stride;
> + }
> + } else {
> + DPRINTF("Source address:\n");
> + int frag;
> + for (frag = 0; frag < 5; frag++) {
> + source_addr[frag] =
> + xilinx_dpdma_desc_get_source_address(&desc, frag);
> + DPRINTF("Fragment %u: 0x%8.8lX\n", frag + 1,
> + source_addr[frag]);
> + }
> +
> + frag = 0;
> + while (transfer_len < 0) {
&& frag < 5 ?
> + if (frag >= 5) {
> + break;
> + }
To drop this.
> + size_t fragment_len = 4096 - (source_addr[frag] % 4096);
Magic number 4096.
> +
> + if (dma_memory_read(&address_space_memory,
> + source_addr[frag],
> + &(s->data[channel][ptr]),
> + fragment_len)) {
> + s->registers[DPDMA_ISR] |= ((1 << 12) << channel);
> + xilinx_dpdma_update_irq(s);
> + DPRINTF("Can't get data.\n");
> + break;
> + }
> + ptr += fragment_len;
> + transfer_len -= fragment_len;
> + frag += 1;
> + }
> + }
> + }
> +
> + if (xilinx_dpdma_desc_update_enabled(&desc)) {
> + /* The descriptor need to be updated when it's completed. */
> + DPRINTF("update the descriptor with the done flag set.\n");
> + xilinx_dpdma_desc_set_done(&desc);
> + if (dma_memory_write(&address_space_memory, desc_addr, &desc,
> + sizeof(DPDMADescriptor))) {
> + abort();
It should be left the the memory API to determine if its a memory
write is fatal. There are system level changed beyond this modules
control that could cause memory API to say no to this op. A quick grep
suggest its normal to just post the write and not assert success. The
one user of the return value I can see (ohci) does a graceful failure
on it.
> + }
> + }
> +
> + if (xilinx_dpdma_desc_completion_interrupt(&desc)) {
> + DPRINTF("completion interrupt enabled!\n");
> + s->registers[DPDMA_ISR] |= (1 << channel);
> + xilinx_dpdma_update_irq(s);
> + }
> +
> + } while (!done && !one_desc);
> +
> + return ptr;
> +}
> +
> +/*
> + * Set the host location to be filled with the data.
> + */
Non-static function can just rely on comment in heade.
> +void xilinx_dpdma_set_host_data_location(XilinxDPDMAState *s, uint8_t
> channel,
> + void *p)
> +{
> + if (!s) {
> + qemu_log_mask(LOG_UNIMP, "DPDMA client not attached to valid DPDMA"
> + " instance\n");
> + return;
> + }
> +
> + assert(channel <= 5);
> + s->data[channel] = p;
> +}
> +
> +type_init(xilinx_dpdma_register_types)
> diff --git a/hw/dma/xilinx_dpdma.h b/hw/dma/xilinx_dpdma.h
> new file mode 100644
> index 0000000..f92167d
> --- /dev/null
> +++ b/hw/dma/xilinx_dpdma.h
> @@ -0,0 +1,71 @@
> +/*
> + * xilinx_dpdma.h
> + *
> + * Copyright (C) 2015 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: address@hidden
> + *
> + * Developed by :
> + * Frederic Konrad <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef XILINX_DPDMA_H
> +#define XILINX_DPDMA_H
> +
> +#include "hw/sysbus.h"
> +#include "ui/console.h"
> +#include "sysemu/dma.h"
> +
> +struct XilinxDPDMAState {
/*< private >*/
> + SysBusDevice parent_obj;
/*< public >*/
> + MemoryRegion iomem;
> + uint32_t registers[0x1000 >> 2];
> + uint8_t *data[6];
> + bool operation_finished[6];
> + qemu_irq irq;
> +};
> +
> +typedef struct XilinxDPDMAState XilinxDPDMAState;
Can you just define and typedef together? Thats quite common. unless
you have a circular ref between module or need to define the type
opaquely.
> +
> +#define TYPE_XILINX_DPDMA "xlnx.dpdma"
> +#define XILINX_DPDMA(obj) OBJECT_CHECK(XilinxDPDMAState, (obj),
> \
> + TYPE_XILINX_DPDMA)
> +
> +/*
> + * \func xilinx_dpdma_start_operation.
> + * \brief Start the operation on the specified channel. The DPDMA get the
> + * current descriptor and retrieve data to the buffer specified by
> + * dpdma_set_host_data_location.
> + * \arg s The DPDMA instance.
> + * \arg channel The channel to start.
> + * \return the number of byte transfered by the DPDMA or 0 if an error
> occured.
> + */
> +size_t xilinx_dpdma_start_operation(XilinxDPDMAState *s, uint8_t channel,
> + bool one_desc);
> +
> +/*
> + * \func xilinx_dpdma_set_host_data_location.
> + * \brief Set the location in the host memory where to store the data out
> from
> + * the dma channel.
> + * \arg s The DPDMA instance.
> + * \arg channel The channel associated to the pointer.
> + * \arg p The buffer where to store the data.
> + */
What is this documentation style? I can only see it in one other file
in the tree (target-xtensa/helper.c). The doxygen * @ style is more
common. Check bitops.h for a fuller example.
> +/* XXX: add a maximum size arg and send an interrupt in case of overflow. */
Does an interrupt have physical meaning? A host buffer overrun is some
sort of fatal error isnt it?
Regards,
Peter
> +void xilinx_dpdma_set_host_data_location(XilinxDPDMAState *s, uint8_t
> channel,
> + void *p);
> +
> +#endif /* !XILINX_DPDMA_H */
> --
> 1.9.0
>
>
>
- [Qemu-devel] [PATCH 3/8] console: add qemu_alloc_display_format., (continued)
- [Qemu-devel] [PATCH 3/8] console: add qemu_alloc_display_format., fred . konrad, 2015/05/13
- [Qemu-devel] [PATCH 4/8] introduce dpcd module., fred . konrad, 2015/05/13
- [Qemu-devel] [PATCH 5/8] hw/i2c-ddc.c: Implement DDC I2C slave, fred . konrad, 2015/05/13
- [Qemu-devel] [PATCH 8/8] arm: xlnx-zynqmp: Add DisplayPort and DPDMA., fred . konrad, 2015/05/13
- [Qemu-devel] [PATCH 6/8] Introduce xilinx dpdma., fred . konrad, 2015/05/13
- Re: [Qemu-devel] [PATCH 6/8] Introduce xilinx dpdma.,
Peter Crosthwaite <=
- [Qemu-devel] [PATCH 7/8] Introduce xilinx dp., fred . konrad, 2015/05/13