diff mbox

[0/3] dp8393x update

Message ID 54A66408.3040406@Vivier.EU
State New
Headers show

Commit Message

Laurent Vivier Jan. 2, 2015, 9:25 a.m. UTC 
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

Comments

Peter Maydell Jan. 2, 2015, 10:19 a.m. UTC | #1 
On 2 January 2015 at 09:25, Laurent Vivier <Laurent@vivier.eu> wrote:
> 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.

Actually I think using properties is the direction we're
headed for this currently. (Consider the case of a sysbus
device which has two DMA master ports, like an ARM trustzone
aware device.) However I have a feeling the plan was to use
MemoryRegion properties rather than AddressSpaces.

Can anybody remember if we have an example of this in-tree?

-- PMM
Laurent Vivier Jan. 2, 2015, 11:33 a.m. UTC | #2 
Le 02/01/2015 11:19, Peter Maydell a écrit :
> On 2 January 2015 at 09:25, Laurent Vivier <Laurent@vivier.eu> wrote:
>> 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.
> 
> Actually I think using properties is the direction we're
> headed for this currently. (Consider the case of a sysbus
> device which has two DMA master ports, like an ARM trustzone

Does it means that these two DMA master ports can access different
address spaces ?

> aware device.) However I have a feeling the plan was to use
> MemoryRegion properties rather than AddressSpaces.

Is it possible to use something like dma_memory_rw() with MemoryRegion ?

Regards,
Laurent
Peter Maydell Jan. 2, 2015, 12:31 p.m. UTC | #3 
On 2 January 2015 at 11:33, Laurent Vivier <Laurent@vivier.eu> wrote:
> Le 02/01/2015 11:19, Peter Maydell a écrit :
>> On 2 January 2015 at 09:25, Laurent Vivier <Laurent@vivier.eu> wrote:
>>> 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.
>>
>> Actually I think using properties is the direction we're
>> headed for this currently. (Consider the case of a sysbus
>> device which has two DMA master ports, like an ARM trustzone
>
> Does it means that these two DMA master ports can access different
> address spaces ?

Yes. (At least, that's how we're planning to model TrustZone:
the secure and non-secure worlds will be different AddressSpaces.)

>> aware device.) However I have a feeling the plan was to use
>> MemoryRegion properties rather than AddressSpaces.
>
> Is it possible to use something like dma_memory_rw() with MemoryRegion ?

The point is that it's always possible to create an AddressSpace
corresponding to a MemoryRegion (which you do if you're a bus
master going to DMA into it). But if you're an SoC model and
you want to add a few of your own devices and pass the whole
thing on to a CPU or other bus master, then you have to do that
using MemoryRegions. So we should use MemoryRegion properties
everywhere, as they're more flexible, and only create the
AddressSpaces where needed locally inside the bus-master
devices.

-- PMM
diff mbox

Patch

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);