From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50995) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXn20-0001NY-2E for qemu-devel@nongnu.org; Fri, 04 Sep 2015 05:11:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXn1v-0002M2-5o for qemu-devel@nongnu.org; Fri, 04 Sep 2015 05:11:35 -0400 Received: from greensocs.com ([193.104.36.180]:41536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXn1u-0002Ll-SX for qemu-devel@nongnu.org; Fri, 04 Sep 2015 05:11:31 -0400 Message-ID: <55E95DFF.6040106@greensocs.com> Date: Fri, 04 Sep 2015 11:01:51 +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> <55E7F682.9050507@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 04/09/2015 01:34, Alistair Francis wrote: > On Thu, Sep 3, 2015 at 12:28 AM, Frederic Konrad > wrote: >> 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. > Hey Fred, > > Great, thanks > >> [...] >>>> + >>>> + 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. > It looks like it is less then 80 columns though. Oh yes missed that when I replaced xilinx by xlnx in functions.. I fixed that. Thanks, Fred > > Thanks, > > Alistair > >> 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 >>>> >>>>