From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuGee-0005Db-58 for qemu-devel@nongnu.org; Mon, 18 May 2015 04:44:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuGeZ-0002Kp-2X for qemu-devel@nongnu.org; Mon, 18 May 2015 04:44:08 -0400 Received: from greensocs.com ([193.104.36.180]:52485) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuGeY-0002Iy-E4 for qemu-devel@nongnu.org; Mon, 18 May 2015 04:44:03 -0400 Message-ID: <5559A647.2090406@greensocs.com> Date: Mon, 18 May 2015 10:43:51 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1431544326-13372-1-git-send-email-fred.konrad@greensocs.com> <1431544326-13372-7-git-send-email-fred.konrad@greensocs.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/8] Introduce xilinx dpdma. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Mark Burton , "qemu-devel@nongnu.org Developers" , hyunk@xilinx.com On 18/05/2015 10:17, Peter Crosthwaite wrote: > On Wed, May 13, 2015 at 12:12 PM, wrote: >> From: KONRAD Frederic >> >> This is the implementation of the DPDMA. >> >> Signed-off-by: KONRAD Frederic >> --- >> 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. Do you want me to use this condition: obj-$(CONFIG_XLNX_ZYNQMP) ? Seems making sense as it will be the only platform using it.? > >> 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: info@greensocs.com >> + * >> + * Developed by : >> + * Frederic Konrad >> + * >> + * 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 . >> + * >> + */ >> + >> +#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. Ok. > >> +#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) true and it will probably help for the repetitions below. > >> +#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? Yes. > >> + 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? It means burst lenght is missing from the status register. Seems impossible to get and information for that. Probably the best option is to hardcode something? > >> + 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? Yes good point. Strangely it compiled on both 32 and 64bits guest. > 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? What do you mean? > >> + /* >> + * 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? Yes this is the VSYNC interrupt from DPDMA. BTW probably 27 needs a macro too, to be clearer. > >> + >> + 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). Definitely. > >> + 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)) { > && oops didn't see that :). > >> + 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 ? yes good point. > >> + 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: info@greensocs.com >> + * >> + * Developed by : >> + * Frederic Konrad >> + * >> + * 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 . >> + * >> + */ >> + >> +#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. Ok I'll convert. > >> +/* 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? You mean better aborting when it happens? > > Regards, > Peter > >> +void xilinx_dpdma_set_host_data_location(XilinxDPDMAState *s, uint8_t channel, >> + void *p); >> + >> +#endif /* !XILINX_DPDMA_H */ >> -- >> 1.9.0 >> >> >>