From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXOwP-0003Ji-D2 for qemu-devel@nongnu.org; Thu, 03 Sep 2015 03:28:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXOwM-0005fK-3e for qemu-devel@nongnu.org; Thu, 03 Sep 2015 03:28:13 -0400 Received: from greensocs.com ([193.104.36.180]:47635) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXOwL-0005eO-Nk for qemu-devel@nongnu.org; Thu, 03 Sep 2015 03:28:10 -0400 Message-ID: <55E7F682.9050507@greensocs.com> Date: Thu, 03 Sep 2015 09:28:02 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1437499031-11741-1-git-send-email-fred.konrad@greensocs.com> <1437499031-11741-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 V4 6/8] Introduce xilinx dpdma. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: Peter Maydell , Peter Crosthwaite , hyunk@xilinx.com, Mark Burton , "qemu-devel@nongnu.org Developers" , Guillaume Delbergue On 02/09/2015 23:39, Alistair Francis wrote: > On Tue, Jul 21, 2015 at 10:17 AM, wrote: >> From: KONRAD Frederic >> >> This is the implementation of the DPDMA. >> >> Signed-off-by: KONRAD Frederic >> --- >> hw/dma/Makefile.objs | 1 + >> hw/dma/xlnx_dpdma.c | 790 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/dma/xlnx_dpdma.h | 85 ++++++ >> 3 files changed, 876 insertions(+) >> create mode 100644 hw/dma/xlnx_dpdma.c >> create mode 100644 hw/dma/xlnx_dpdma.h > What are the rules with the placement of the header files? Should the > header be in the include directory instead of being here? Probably moving headers to include/hw makes sense here.. I will change it. [...] >> + >> + if (xlnx_dpdma_desc_is_already_done(&desc) >> + && !xlnx_dpdma_desc_ignore_done_bit(&desc)) { > The second line of the if should be indented across. > > Also, I generally think the operator should go on the first line, but > that doesn't seem worth changing throughout. Ok I can do that. > >> + /* We are trying to process an already processed descriptor. */ >> + s->registers[DPDMA_EISR] |= ((1 << 25) << channel); >> + xlnx_dpdma_update_irq(s); >> + s->operation_finished[channel] = true; >> + DPRINTF("Already processed descriptor..\n"); >> + break; >> + } >> + >> + done = xlnx_dpdma_desc_is_last(&desc) >> + || xlnx_dpdma_desc_is_last_of_frame(&desc); >> + >> + s->operation_finished[channel] = done; >> + if (s->data[channel]) { >> + int64_t transfer_len = >> + xlnx_dpdma_desc_get_transfer_size(&desc); > Why is this over two lines? Probably because of the 79~80 columns limitation. Thanks, Fred > > Functioanlly it looks fine. > > Besides the header file location, which someone else will need to comment on: > > Reviewed-by: Alistair Francis > Tested-By: Hyun Kwon > > Thanks, > > Alistair > >> + uint32_t line_size = xlnx_dpdma_desc_get_line_size(&desc); >> + uint32_t line_stride = xlnx_dpdma_desc_get_line_stride(&desc); >> + if (xlnx_dpdma_desc_is_contiguous(&desc)) { >> + source_addr[0] = >> + xlnx_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], >> + line_size)) { >> + s->registers[DPDMA_ISR] |= ((1 << 12) << channel); >> + xlnx_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] = >> + xlnx_dpdma_desc_get_source_address(&desc, frag); >> + DPRINTF("Fragment %u: %" PRIx64 "\n", frag + 1, >> + source_addr[frag]); >> + } >> + >> + frag = 0; >> + while ((transfer_len < 0) && (frag < 5)) { >> + size_t fragment_len = DPDMA_FRAG_MAX_SZ >> + - (source_addr[frag] % DPDMA_FRAG_MAX_SZ); >> + >> + if (dma_memory_read(&address_space_memory, >> + source_addr[frag], >> + &(s->data[channel][ptr]), >> + fragment_len)) { >> + s->registers[DPDMA_ISR] |= ((1 << 12) << channel); >> + xlnx_dpdma_update_irq(s); >> + DPRINTF("Can't get data.\n"); >> + break; >> + } >> + ptr += fragment_len; >> + transfer_len -= fragment_len; >> + frag += 1; >> + } >> + } >> + } >> + >> + if (xlnx_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"); >> + xlnx_dpdma_desc_set_done(&desc); >> + dma_memory_write(&address_space_memory, desc_addr, &desc, >> + sizeof(DPDMADescriptor)); >> + } >> + >> + if (xlnx_dpdma_desc_completion_interrupt(&desc)) { >> + DPRINTF("completion interrupt enabled!\n"); >> + s->registers[DPDMA_ISR] |= (1 << channel); >> + xlnx_dpdma_update_irq(s); >> + } >> + >> + } while (!done && !one_desc); >> + >> + return ptr; >> +} >> + >> +void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *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; >> +} >> + >> +void xlnx_dpdma_trigger_vsync_irq(XlnxDPDMAState *s) >> +{ >> + s->registers[DPDMA_ISR] |= (1 << 27); >> + xlnx_dpdma_update_irq(s); >> +} >> + >> +type_init(xlnx_dpdma_register_types) >> diff --git a/hw/dma/xlnx_dpdma.h b/hw/dma/xlnx_dpdma.h >> new file mode 100644 >> index 0000000..ae571a0 >> --- /dev/null >> +++ b/hw/dma/xlnx_dpdma.h >> @@ -0,0 +1,85 @@ >> +/* >> + * xlnx_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 XLNX_DPDMA_H >> +#define XLNX_DPDMA_H >> + >> +#include "hw/sysbus.h" >> +#include "ui/console.h" >> +#include "sysemu/dma.h" >> + >> +#define XLNX_DPDMA_REG_ARRAY_SIZE (0x1000 >> 2) >> + >> +struct XlnxDPDMAState { >> + /*< private >*/ >> + SysBusDevice parent_obj; >> + /*< public >*/ >> + MemoryRegion iomem; >> + uint32_t registers[XLNX_DPDMA_REG_ARRAY_SIZE]; >> + uint8_t *data[6]; >> + bool operation_finished[6]; >> + qemu_irq irq; >> +}; >> + >> +typedef struct XlnxDPDMAState XlnxDPDMAState; >> + >> +#define TYPE_XLNX_DPDMA "xlnx.dpdma" >> +#define XLNX_DPDMA(obj) OBJECT_CHECK(XlnxDPDMAState, (obj), TYPE_XLNX_DPDMA) >> + >> +/* >> + * xlnx_dpdma_start_operation: Start the operation on the specified channel. The >> + * DPDMA gets the current descriptor and retrieves >> + * data to the buffer specified by >> + * dpdma_set_host_data_location(). >> + * >> + * Returns The number of bytes transfered by the DPDMA or 0 if an error occured. >> + * >> + * @s The DPDMA state. >> + * @channel The channel to start. >> + */ >> +size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel, >> + bool one_desc); >> + >> +/* >> + * xlnx_dpdma_set_host_data_location: Set the location in the host memory where >> + * to store the data out from the dma >> + * channel. >> + * >> + * @s The DPDMA state. >> + * @channel The channel associated to the pointer. >> + * @p The buffer where to store the data. >> + */ >> +/* XXX: add a maximum size arg and send an interrupt in case of overflow. */ >> +void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t channel, >> + void *p); >> + >> +/* >> + * xlnx_dpdma_trigger_vsync_irq: Trigger a VSYNC IRQ when the display is >> + * updated. >> + * >> + * @s The DPDMA state. >> + */ >> +void xlnx_dpdma_trigger_vsync_irq(XlnxDPDMAState *s); >> + >> +#endif /* !XLNX_DPDMA_H */ >> -- >> 1.9.0 >> >>