[Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c

Philippe Mathieu-Daudé posted 3 patches 7 years ago
There is a newer version of this series
[Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Philippe Mathieu-Daudé 7 years ago
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS            |  1 +
 hw/arm/xilinx_zynq.c   | 18 ++----------------
 hw/dma/pl330.c         |  2 +-
 include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 17 deletions(-)
 create mode 100644 include/hw/dma/pl330.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d794bd7a66..647e2aa0d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -452,6 +452,7 @@ F: hw/display/pl110*
 F: hw/dma/pl080.c
 F: include/hw/dma/pl080.h
 F: hw/dma/pl330.c
+F: include/hw/dma/pl330.h
 F: hw/gpio/pl061.c
 F: hw/input/pl050.c
 F: hw/intc/pl190.c
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 57497b0c4d..a4c4d44f00 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -34,6 +34,7 @@
 #include "hw/char/cadence_uart.h"
 #include "hw/net/cadence_gem.h"
 #include "hw/cpu/a9mpcore.h"
+#include "hw/dma/pl330.h"
 
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
@@ -278,22 +279,7 @@ static void zynq_init(MachineState *machine)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
 
-    dev = qdev_create(NULL, "pl330");
-    qdev_prop_set_uint8(dev, "num_chnls",  8);
-    qdev_prop_set_uint8(dev, "num_periph_req",  4);
-    qdev_prop_set_uint8(dev, "num_events",  16);
-
-    qdev_prop_set_uint8(dev, "data_width",  64);
-    qdev_prop_set_uint8(dev, "wr_cap",  8);
-    qdev_prop_set_uint8(dev, "wr_q_dep",  16);
-    qdev_prop_set_uint8(dev, "rd_cap",  8);
-    qdev_prop_set_uint8(dev, "rd_q_dep",  16);
-    qdev_prop_set_uint16(dev, "data_buffer_dep",  256);
-
-    qdev_init_nofail(dev);
-    busdev = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(busdev, 0, 0xF8003000);
-    sysbus_connect_irq(busdev, 0, pic[45-IRQ_OFFSET]); /* abort irq line */
+    pl330_init(0xf8003000, pic[45 - IRQ_OFFSET], 4); /* abort irq line */
     for (n = 0; n < ARRAY_SIZE(dma_irqs); ++n) { /* event irqs */
         sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
     }
diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index d071049233..711cf9a605 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -20,6 +20,7 @@
 #include "qemu/timer.h"
 #include "sysemu/dma.h"
 #include "qemu/log.h"
+#include "hw/dma/pl330.h"
 
 #ifndef PL330_ERR_DEBUG
 #define PL330_ERR_DEBUG 0
@@ -271,7 +272,6 @@ struct PL330State {
 
 };
 
-#define TYPE_PL330 "pl330"
 #define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
 
 static const VMStateDescription vmstate_pl330 = {
diff --git a/include/hw/dma/pl330.h b/include/hw/dma/pl330.h
new file mode 100644
index 0000000000..9a586c0df9
--- /dev/null
+++ b/include/hw/dma/pl330.h
@@ -0,0 +1,41 @@
+/*
+ * ARM PrimeCell PL330 DMA Controller
+ *
+ * Copyright (c) 2009 Samsung Electronics.
+ * Contributed by Kirill Batuzov <batuzovk@ispras.ru>
+ * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
+ * Copyright (c) 2012 PetaLogix Pty Ltd.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_DMA_PL330_H
+#define HW_DMA_PL330_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_PL330 "pl330"
+
+static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
+{
+    SysBusDevice *busdev;
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, TYPE_PL330);
+    qdev_prop_set_uint8(dev, "num_chnls", 8);
+    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
+    qdev_prop_set_uint8(dev, "num_events", 16);
+    qdev_prop_set_uint8(dev, "data_width", 64);
+    qdev_prop_set_uint8(dev, "wr_cap", 8);
+    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
+    qdev_prop_set_uint8(dev, "rd_cap", 8);
+    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
+    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
+    qdev_init_nofail(dev);
+
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(busdev, 0, base);
+    sysbus_connect_irq(busdev, 0, irq);
+}
+
+#endif /* HW_DMA_PL330_H */
-- 
2.17.2


Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Alistair Francis 7 years ago
On Mon, Oct 29, 2018 at 4:24 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  MAINTAINERS            |  1 +
>  hw/arm/xilinx_zynq.c   | 18 ++----------------
>  hw/dma/pl330.c         |  2 +-
>  include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 45 insertions(+), 17 deletions(-)
>  create mode 100644 include/hw/dma/pl330.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d794bd7a66..647e2aa0d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -452,6 +452,7 @@ F: hw/display/pl110*
>  F: hw/dma/pl080.c
>  F: include/hw/dma/pl080.h
>  F: hw/dma/pl330.c
> +F: include/hw/dma/pl330.h
>  F: hw/gpio/pl061.c
>  F: hw/input/pl050.c
>  F: hw/intc/pl190.c
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 57497b0c4d..a4c4d44f00 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -34,6 +34,7 @@
>  #include "hw/char/cadence_uart.h"
>  #include "hw/net/cadence_gem.h"
>  #include "hw/cpu/a9mpcore.h"
> +#include "hw/dma/pl330.h"
>
>  #define NUM_SPI_FLASHES 4
>  #define NUM_QSPI_FLASHES 2
> @@ -278,22 +279,7 @@ static void zynq_init(MachineState *machine)
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
>
> -    dev = qdev_create(NULL, "pl330");
> -    qdev_prop_set_uint8(dev, "num_chnls",  8);
> -    qdev_prop_set_uint8(dev, "num_periph_req",  4);
> -    qdev_prop_set_uint8(dev, "num_events",  16);
> -
> -    qdev_prop_set_uint8(dev, "data_width",  64);
> -    qdev_prop_set_uint8(dev, "wr_cap",  8);
> -    qdev_prop_set_uint8(dev, "wr_q_dep",  16);
> -    qdev_prop_set_uint8(dev, "rd_cap",  8);
> -    qdev_prop_set_uint8(dev, "rd_q_dep",  16);
> -    qdev_prop_set_uint16(dev, "data_buffer_dep",  256);
> -
> -    qdev_init_nofail(dev);
> -    busdev = SYS_BUS_DEVICE(dev);
> -    sysbus_mmio_map(busdev, 0, 0xF8003000);
> -    sysbus_connect_irq(busdev, 0, pic[45-IRQ_OFFSET]); /* abort irq line */
> +    pl330_init(0xf8003000, pic[45 - IRQ_OFFSET], 4); /* abort irq line */
>      for (n = 0; n < ARRAY_SIZE(dma_irqs); ++n) { /* event irqs */
>          sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
>      }
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index d071049233..711cf9a605 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -20,6 +20,7 @@
>  #include "qemu/timer.h"
>  #include "sysemu/dma.h"
>  #include "qemu/log.h"
> +#include "hw/dma/pl330.h"
>
>  #ifndef PL330_ERR_DEBUG
>  #define PL330_ERR_DEBUG 0
> @@ -271,7 +272,6 @@ struct PL330State {
>
>  };
>
> -#define TYPE_PL330 "pl330"
>  #define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
>
>  static const VMStateDescription vmstate_pl330 = {
> diff --git a/include/hw/dma/pl330.h b/include/hw/dma/pl330.h
> new file mode 100644
> index 0000000000..9a586c0df9
> --- /dev/null
> +++ b/include/hw/dma/pl330.h
> @@ -0,0 +1,41 @@
> +/*
> + * ARM PrimeCell PL330 DMA Controller
> + *
> + * Copyright (c) 2009 Samsung Electronics.
> + * Contributed by Kirill Batuzov <batuzovk@ispras.ru>
> + * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
> + * Copyright (c) 2012 PetaLogix Pty Ltd.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_DMA_PL330_H
> +#define HW_DMA_PL330_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_PL330 "pl330"
> +
> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
> +{
> +    SysBusDevice *busdev;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, TYPE_PL330);
> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
> +    qdev_prop_set_uint8(dev, "num_events", 16);
> +    qdev_prop_set_uint8(dev, "data_width", 64);
> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
> +    qdev_init_nofail(dev);
> +
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(busdev, 0, base);
> +    sysbus_connect_irq(busdev, 0, irq);
> +}
> +
> +#endif /* HW_DMA_PL330_H */
> --
> 2.17.2
>
>

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Richard Henderson 7 years ago
On 10/29/18 11:20 PM, Philippe Mathieu-Daudé wrote:
> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
> +{
> +    SysBusDevice *busdev;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, TYPE_PL330);
> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
> +    qdev_prop_set_uint8(dev, "num_events", 16);
> +    qdev_prop_set_uint8(dev, "data_width", 64);
> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
> +    qdev_init_nofail(dev);
> +
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(busdev, 0, base);
> +    sysbus_connect_irq(busdev, 0, irq);
> +}

Why is this inline instead of in hw/dma/pl300.c?
There should be nothing performance sensative here...


r~

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Philippe Mathieu-Daudé 7 years ago
On 30/10/18 9:18, Richard Henderson wrote:
> On 10/29/18 11:20 PM, Philippe Mathieu-Daudé wrote:
>> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
>> +{
>> +    SysBusDevice *busdev;
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create(NULL, TYPE_PL330);
>> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
>> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
>> +    qdev_prop_set_uint8(dev, "num_events", 16);
>> +    qdev_prop_set_uint8(dev, "data_width", 64);
>> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
>> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
>> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
>> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
>> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
>> +    qdev_init_nofail(dev);
>> +
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(busdev, 0, base);
>> +    sysbus_connect_irq(busdev, 0, irq);
>> +}
> 
> Why is this inline instead of in hw/dma/pl300.c?
> There should be nothing performance sensative here...

Yeah I didn't like it much neither and wondered the same :)
I was looking a examples in hw/char:

[phil@x1w qemu]$ git grep -hA 2 'static inline' include/hw/char/
52:static inline DeviceState *cadence_uart_create(hwaddr addr,
53-                                        qemu_irq irq,
54-                                        Chardev *chr)
--
52:static inline DeviceState *cmsdk_apb_uart_create(hwaddr addr,
53-                                                 qemu_irq txint,
54-                                                 qemu_irq rxint,
--
18:static inline DeviceState *pl011_create(hwaddr addr,
19-                                        qemu_irq irq,
20-                                        Chardev *chr)
--
35:static inline DeviceState *pl011_luminary_create(hwaddr addr,
36-                                                 qemu_irq irq,
37-                                                 Chardev *chr)
--
18:static inline DeviceState *xilinx_uartlite_create(hwaddr addr,
19-                                        qemu_irq irq,
20-                                        Chardev *chr)

I'll clean it and add a docstring.

Thanks for the review,

Phil.

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Peter Maydell 7 years ago
On 29 October 2018 at 23:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS            |  1 +
>  hw/arm/xilinx_zynq.c   | 18 ++----------------
>  hw/dma/pl330.c         |  2 +-
>  include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 45 insertions(+), 17 deletions(-)
>  create mode 100644 include/hw/dma/pl330.h

> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
> +{
> +    SysBusDevice *busdev;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, TYPE_PL330);
> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
> +    qdev_prop_set_uint8(dev, "num_events", 16);
> +    qdev_prop_set_uint8(dev, "data_width", 64);
> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
> +    qdev_init_nofail(dev);

These are the settings the Xilinx board uses, but are
they really the settings every SoC that has a PL330 will use ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Philippe Mathieu-Daudé 7 years ago
On 30/10/18 10:36, Peter Maydell wrote:
> On 29 October 2018 at 23:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   MAINTAINERS            |  1 +
>>   hw/arm/xilinx_zynq.c   | 18 ++----------------
>>   hw/dma/pl330.c         |  2 +-
>>   include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 45 insertions(+), 17 deletions(-)
>>   create mode 100644 include/hw/dma/pl330.h
> 
>> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
>> +{
>> +    SysBusDevice *busdev;
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create(NULL, TYPE_PL330);
>> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
>> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
>> +    qdev_prop_set_uint8(dev, "num_events", 16);
>> +    qdev_prop_set_uint8(dev, "data_width", 64);
>> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
>> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
>> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
>> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
>> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
>> +    qdev_init_nofail(dev);
> 
> These are the settings the Xilinx board uses, but are
> they really the settings every SoC that has a PL330 will use ?

Except "num_periph_req", all are pl330_properties defaults.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Peter Maydell 7 years ago
On 30 October 2018 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 30/10/18 10:36, Peter Maydell wrote:
>>
>> On 29 October 2018 at 23:20, Philippe Mathieu-Daudé <philmd@redhat.com>
>> wrote:
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   MAINTAINERS            |  1 +
>>>   hw/arm/xilinx_zynq.c   | 18 ++----------------
>>>   hw/dma/pl330.c         |  2 +-
>>>   include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 45 insertions(+), 17 deletions(-)
>>>   create mode 100644 include/hw/dma/pl330.h
>>
>>
>>> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
>>> +{
>>> +    SysBusDevice *busdev;
>>> +    DeviceState *dev;
>>> +
>>> +    dev = qdev_create(NULL, TYPE_PL330);
>>> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
>>> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
>>> +    qdev_prop_set_uint8(dev, "num_events", 16);
>>> +    qdev_prop_set_uint8(dev, "data_width", 64);
>>> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
>>> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
>>> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
>>> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
>>> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
>>> +    qdev_init_nofail(dev);
>>
>>
>> These are the settings the Xilinx board uses, but are
>> they really the settings every SoC that has a PL330 will use ?
>
>
> Except "num_periph_req", all are pl330_properties defaults.

If they're all the device's defaults there's not much point
in setting them by hand. But my point is that the reason they're
properties is that in the real hardware these are configurable
values in the RTL. So any given SoC model needs to be able to
set them appropriately. Having a helper function that doesn't
let you set them makes it too easy for people modelling SoCs
not to think about the question, I think...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Philippe Mathieu-Daudé 7 years ago
On 30/10/18 13:42, Peter Maydell wrote:
> On 30 October 2018 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 30/10/18 10:36, Peter Maydell wrote:
>>>
>>> On 29 October 2018 at 23:20, Philippe Mathieu-Daudé <philmd@redhat.com>
>>> wrote:
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>    MAINTAINERS            |  1 +
>>>>    hw/arm/xilinx_zynq.c   | 18 ++----------------
>>>>    hw/dma/pl330.c         |  2 +-
>>>>    include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 45 insertions(+), 17 deletions(-)
>>>>    create mode 100644 include/hw/dma/pl330.h
>>>
>>>
>>>> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
>>>> +{
>>>> +    SysBusDevice *busdev;
>>>> +    DeviceState *dev;
>>>> +
>>>> +    dev = qdev_create(NULL, TYPE_PL330);
>>>> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
>>>> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
>>>> +    qdev_prop_set_uint8(dev, "num_events", 16);
>>>> +    qdev_prop_set_uint8(dev, "data_width", 64);
>>>> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
>>>> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
>>>> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
>>>> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
>>>> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
>>>> +    qdev_init_nofail(dev);
>>>
>>>
>>> These are the settings the Xilinx board uses, but are
>>> they really the settings every SoC that has a PL330 will use ?
>>
>>
>> Except "num_periph_req", all are pl330_properties defaults.
> 
> If they're all the device's defaults there's not much point
> in setting them by hand. But my point is that the reason they're
> properties is that in the real hardware these are configurable
> values in the RTL. So any given SoC model needs to be able to
> set them appropriately. Having a helper function that doesn't
> let you set them makes it too easy for people modelling SoCs
> not to think about the question, I think...

Yes, I understood and agree.

I now respined v2 without this.

Thanks for your review!

Phil.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Posted by Philippe Mathieu-Daudé 7 years ago
On 30/10/18 0:20, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   MAINTAINERS            |  1 +
>   hw/arm/xilinx_zynq.c   | 18 ++----------------
>   hw/dma/pl330.c         |  2 +-
>   include/hw/dma/pl330.h | 41 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 45 insertions(+), 17 deletions(-)
>   create mode 100644 include/hw/dma/pl330.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d794bd7a66..647e2aa0d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -452,6 +452,7 @@ F: hw/display/pl110*
>   F: hw/dma/pl080.c
>   F: include/hw/dma/pl080.h
>   F: hw/dma/pl330.c
> +F: include/hw/dma/pl330.h
>   F: hw/gpio/pl061.c
>   F: hw/input/pl050.c
>   F: hw/intc/pl190.c
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 57497b0c4d..a4c4d44f00 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -34,6 +34,7 @@
>   #include "hw/char/cadence_uart.h"
>   #include "hw/net/cadence_gem.h"
>   #include "hw/cpu/a9mpcore.h"
> +#include "hw/dma/pl330.h"
>   
>   #define NUM_SPI_FLASHES 4
>   #define NUM_QSPI_FLASHES 2
> @@ -278,22 +279,7 @@ static void zynq_init(MachineState *machine)
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
>       sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
>   
> -    dev = qdev_create(NULL, "pl330");
> -    qdev_prop_set_uint8(dev, "num_chnls",  8);
> -    qdev_prop_set_uint8(dev, "num_periph_req",  4);
> -    qdev_prop_set_uint8(dev, "num_events",  16);
> -
> -    qdev_prop_set_uint8(dev, "data_width",  64);
> -    qdev_prop_set_uint8(dev, "wr_cap",  8);
> -    qdev_prop_set_uint8(dev, "wr_q_dep",  16);
> -    qdev_prop_set_uint8(dev, "rd_cap",  8);
> -    qdev_prop_set_uint8(dev, "rd_q_dep",  16);
> -    qdev_prop_set_uint16(dev, "data_buffer_dep",  256);
> -
> -    qdev_init_nofail(dev);
> -    busdev = SYS_BUS_DEVICE(dev);
> -    sysbus_mmio_map(busdev, 0, 0xF8003000);
> -    sysbus_connect_irq(busdev, 0, pic[45-IRQ_OFFSET]); /* abort irq line */
> +    pl330_init(0xf8003000, pic[45 - IRQ_OFFSET], 4); /* abort irq line */
>       for (n = 0; n < ARRAY_SIZE(dma_irqs); ++n) { /* event irqs */
>           sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);

Sorry, this is a buggy patch   ^ busdev is now invalid.

>       }
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index d071049233..711cf9a605 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -20,6 +20,7 @@
>   #include "qemu/timer.h"
>   #include "sysemu/dma.h"
>   #include "qemu/log.h"
> +#include "hw/dma/pl330.h"
>   
>   #ifndef PL330_ERR_DEBUG
>   #define PL330_ERR_DEBUG 0
> @@ -271,7 +272,6 @@ struct PL330State {
>   
>   };
>   
> -#define TYPE_PL330 "pl330"
>   #define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
>   
>   static const VMStateDescription vmstate_pl330 = {
> diff --git a/include/hw/dma/pl330.h b/include/hw/dma/pl330.h
> new file mode 100644
> index 0000000000..9a586c0df9
> --- /dev/null
> +++ b/include/hw/dma/pl330.h
> @@ -0,0 +1,41 @@
> +/*
> + * ARM PrimeCell PL330 DMA Controller
> + *
> + * Copyright (c) 2009 Samsung Electronics.
> + * Contributed by Kirill Batuzov <batuzovk@ispras.ru>
> + * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
> + * Copyright (c) 2012 PetaLogix Pty Ltd.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_DMA_PL330_H
> +#define HW_DMA_PL330_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_PL330 "pl330"
> +
> +static inline void pl330_init(uint32_t base, qemu_irq irq, int nreq)
> +{
> +    SysBusDevice *busdev;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, TYPE_PL330);
> +    qdev_prop_set_uint8(dev, "num_chnls", 8);
> +    qdev_prop_set_uint8(dev, "num_periph_req", nreq);
> +    qdev_prop_set_uint8(dev, "num_events", 16);
> +    qdev_prop_set_uint8(dev, "data_width", 64);
> +    qdev_prop_set_uint8(dev, "wr_cap", 8);
> +    qdev_prop_set_uint8(dev, "wr_q_dep", 16);
> +    qdev_prop_set_uint8(dev, "rd_cap", 8);
> +    qdev_prop_set_uint8(dev, "rd_q_dep", 16);
> +    qdev_prop_set_uint16(dev, "data_buffer_dep", 256);
> +    qdev_init_nofail(dev);
> +
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(busdev, 0, base);
> +    sysbus_connect_irq(busdev, 0, irq);
> +}
> +
> +#endif /* HW_DMA_PL330_H */
>