From patchwork Fri Jan 2 09:25:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Vivier X-Patchwork-Id: 424965 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id DD77D1400B7 for ; Fri, 2 Jan 2015 20:26:00 +1100 (AEDT) Received: from localhost ([::1]:50670 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y6yUW-0001Zd-KX for incoming@patchwork.ozlabs.org; Fri, 02 Jan 2015 04:25:56 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y6yUC-0001JD-68 for qemu-devel@nongnu.org; Fri, 02 Jan 2015 04:25:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y6yU8-00013r-TW for qemu-devel@nongnu.org; Fri, 02 Jan 2015 04:25:36 -0500 Received: from mout.kundenserver.de ([212.227.17.24]:62167) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y6yU8-00013k-HP for qemu-devel@nongnu.org; Fri, 02 Jan 2015 04:25:32 -0500 Received: from [192.168.100.1] ([78.238.229.36]) by mrelayeu.kundenserver.de (mreue101) with ESMTPSA (Nemesis) id 0MQ6Al-1YB7UH2UNP-005HaS; Fri, 02 Jan 2015 10:25:30 +0100 Message-ID: <54A66408.3040406@Vivier.EU> Date: Fri, 02 Jan 2015 10:25:28 +0100 From: Laurent Vivier User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: =?windows-1252?Q?Herv=E9_Poussineau?= , qemu-devel@nongnu.org References: <1419813550-26182-1-git-send-email-laurent@vivier.eu> <54A5B5A0.7000600@reactos.org> <54A5F5C0.4030402@Vivier.EU> In-Reply-To: <54A5F5C0.4030402@Vivier.EU> X-Provags-ID: V03:K0:6rYZbauXZ8U/6vZqvgJFtunHHhR/haCPsKiEeNDM9OI6ICcMgXc 0Rnzlx0l/YXpM0AhgVUXl2q2RgNPyl/bGtDjgzNKde1VcoSB/lGCMOfDcgFAgI2sPeda+9N WoqOoVnA4QtTo+1byR76R6jY3EM5yrobJGPKmAibYFw420U8jbLg6qBvf4+1eI9ZmHtbRVX Mg+6fngBwaVsehYk7MX1g== X-UI-Out-Filterresults: notjunk:1; X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 212.227.17.24 Cc: Leon Alrae , Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH 0/3] dp8393x update X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Le 02/01/2015 02:34, Laurent Vivier a écrit : > Hi Hervé, > > Le 01/01/2015 22:01, Hervé Poussineau a écrit : >> Hi Laurent, >> >> Le 29/12/2014 01:39, Laurent Vivier a écrit : >>> This is a series of patches I wrote to use dp8393x (SONIC) with >>> Quadra 800 emulation. I think it is interesting to share them with the >>> mainline. >>> >>> Qdev'ifying allows to remove the annoying warning: >>> "requested NIC (anonymous, model dp83932) was not created >>> (not supported by this machine?)" >>> >>> [PATCH 1/3] dp8393x: add registers offset >>> [PATCH 2/3] dp8393x: add PROM to store MAC address >>> [PATCH 3/3] qdev'ify dp8393x >>> >> >> I also had some patches to QOM'ify dp8393x. >> Those are available at >> http://repo.or.cz/w/qemu/hpoussin.git/shortlog/refs/heads/sonic >> >> Main differences are: >> - dp8393x uses an AddressSpace, instead of an offset in a MemoryRegion >> in yours >> - no PROM support, but should be easy to add >> - rc4030 (MIPS Jazz chipset) also converted to QOM (but that was not the >> goal of your patch series) >> >> Minor points are: >> - have load/save support >> - all functions have the same dp8393x_ prefix >> - old_mmio-style functions are not used anymore >> >> What do you think of them? > > I don't know if it's a good idea to use AddressSpace into device. For > me, AddressSpace must stay in the machine definition. SysBus is there > for that. But it seems to be a good way to do DMA. I have to think about > that... Using AddressSpace is really a very good idea, in fact, but I don't like the way you pass it to the device (a qdev_set_prop()). I think we should do as it is done in PCI. This must be managed at sysbus level, not at the device level. Find attached a (not tested) patch to show what I'm thinking about. Regards, Laurent diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 69960ac..8902a4f 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -148,7 +148,6 @@ typedef struct dp8393xState { /* Hardware */ uint8_t it_shift; - void *as_opaque; qemu_irq irq; #ifdef DEBUG_SONIC int irq_level; @@ -200,7 +199,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) while (s->regs[SONIC_CDC] & 0x1f) { /* Fill current entry */ - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP], (uint8_t *)data, size, 0); s->cam[index][0] = data[1 * width] & 0xff; @@ -219,7 +218,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) } /* Read CAM enable */ - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP], (uint8_t *)data, size, 0); s->regs[SONIC_CE] = data[0 * width]; @@ -239,7 +238,7 @@ static void dp8393x_do_read_rra(dp8393xState *s) /* Read memory */ width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; size = sizeof(uint16_t) * 4 * width; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_RRP], (uint8_t *)data, size, 0); @@ -352,7 +351,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_CTDA]); size = sizeof(uint16_t) * 6 * width; s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA]; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * width, (uint8_t *)data, size, 0); tx_len = 0; @@ -378,7 +377,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) if (tx_len + len > sizeof(s->tx_buffer)) { len = sizeof(s->tx_buffer) - tx_len; } - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), (s->regs[SONIC_TSA1] << 16) | s->regs[SONIC_TSA0], &s->tx_buffer[tx_len], len, 0); tx_len += len; @@ -387,7 +386,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) if (i != s->regs[SONIC_TFC]) { /* Read next fragment details */ size = sizeof(uint16_t) * 3 * width; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * i) * width, (uint8_t *)data, size, 0); s->regs[SONIC_TSA0] = data[0 * width]; @@ -421,14 +420,14 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) /* Write status */ data[0 * width] = s->regs[SONIC_TCR] & 0x0fff; /* status */ size = sizeof(uint16_t) * width; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA], (uint8_t *)data, size, 1); if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) { /* Read footer of packet */ size = sizeof(uint16_t) * width; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC]) * width, (uint8_t *)data, size, 0); s->regs[SONIC_CTDA] = data[0 * width] & ~0x1; @@ -695,7 +694,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Are we still in resource exhaustion? */ size = sizeof(uint16_t) * 1 * width; address = ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width; - address_space_rw(s->as, address, (uint8_t*)data, size, 0); + sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)data, size, 0); if (data[0 * width] & 0x1) { /* Still EOL ; stop reception */ return -1; @@ -714,9 +713,9 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Put packet into RBA */ DPRINTF("Receive packet at %08x\n", (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0]); address = (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0]; - address_space_rw(s->as, address, (uint8_t*)buf, rx_len, 1); + sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)buf, rx_len, 1); address += rx_len; - address_space_rw(s->as, address, (uint8_t*)&checksum, 4, 1); + sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)&checksum, 4, 1); rx_len += 4; s->regs[SONIC_CRBA1] = address >> 16; s->regs[SONIC_CRBA0] = address & 0xffff; @@ -744,11 +743,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, data[3 * width] = s->regs[SONIC_TRBA1]; /* pkt_ptr1 */ data[4 * width] = s->regs[SONIC_RSC]; /* seq_no */ size = sizeof(uint16_t) * 5 * width; - address_space_rw(s->as, (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA], (uint8_t *)data, size, 1); + sysbus_dma_rw(SYS_BUS_DEVICE(s), + (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA], (uint8_t *)data, size, 1); /* Move to next descriptor */ size = sizeof(uint16_t) * width; - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width, (uint8_t *)data, size, 0); s->regs[SONIC_LLFA] = data[0 * width]; @@ -757,7 +757,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, s->regs[SONIC_ISR] |= SONIC_ISR_RDE; } else { data[0 * width] = 0; /* in_use */ - address_space_rw(s->as, + sysbus_dma_rw(SYS_BUS_DEVICE(s), ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width, (uint8_t *)data, size, 1); s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; @@ -828,12 +828,12 @@ void dp83932_init(NICInfo *nd, hwaddr base, int it_shift, dev = qdev_create(NULL, "dp83932"); qdev_prop_set_uint8(dev, "it-shift", it_shift); - qdev_prop_set_ptr(dev, "address-space", as); qdev_set_nic_properties(dev, nd); qdev_init_nofail(dev); sysbus = SYS_BUS_DEVICE(dev); sysbus_mmio_map(sysbus, 0, base); sysbus_connect_irq(sysbus, 0, irq); + sysbus_set_address_space(sysbus, as); } static int dp8393x_initfn(SysBusDevice *sysbus) @@ -841,7 +841,6 @@ static int dp8393x_initfn(SysBusDevice *sysbus) DeviceState *dev = DEVICE(sysbus); dp8393xState *s = DP8393X(sysbus); - s->as = s->as_opaque; /* cast address space to right type */ s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s); s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */ @@ -871,7 +870,6 @@ static const VMStateDescription vmstate_dp8393x = { static Property dp8393x_properties[] = { DEFINE_PROP_UINT8("it-shift", dp8393xState, it_shift, 2), - DEFINE_PROP_PTR("address-space", dp8393xState, as_opaque), DEFINE_NIC_PROPERTIES(dp8393xState, conf), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h index d1f3f00..d1e027b 100644 --- a/include/hw/sysbus.h +++ b/include/hw/sysbus.h @@ -3,6 +3,7 @@ /* Devices attached directly to the main system bus. */ +#include "sysemu/dma.h" #include "hw/qdev.h" #include "exec/memory.h" @@ -53,6 +54,7 @@ struct SysBusDevice { hwaddr addr; MemoryRegion *memory; } mmio[QDEV_MAX_MMIO]; + AddressSpace *sysbus_as; int num_pio; pio_addr_t pio[QDEV_MAX_PIO]; }; @@ -78,6 +80,37 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr, MemoryRegion *mem); MemoryRegion *sysbus_address_space(SysBusDevice *dev); +static inline AddressSpace *sysbus_get_address_space(SysBusDevice *dev) +{ + return dev->sysbus_as; +} + +static inline void sysbus_set_address_space(SysBusDevice *dev, + AddressSpace *as) +{ + dev->sysbus_as = as; +} + +static inline int sysbus_dma_rw(SysBusDevice *dev, dma_addr_t addr, + void *buf, dma_addr_t len, DMADirection dir) +{ + dma_memory_rw(sysbus_get_address_space(dev), addr, buf, len, dir); + return 0; +} + +static inline int sysbus_dma_read(SysBusDevice *dev, dma_addr_t addr, + void *buf, dma_addr_t len) +{ + return sysbus_dma_rw(dev, addr, buf, len, DMA_DIRECTION_TO_DEVICE); +} + +static inline int sysbus_dma_write(SysBusDevice *dev, dma_addr_t addr, + const void *buf, dma_addr_t len) +{ + return sysbus_dma_rw(dev, addr, (void *) buf, len, + DMA_DIRECTION_FROM_DEVICE); +} + /* Call func for every dynamically created sysbus device in the system */ void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque);