diff mbox series

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

Message ID 20181029232100.8454-3-philmd@redhat.com
State New
Headers show
Series hw/arm/exynos4: Add DMA support for SMDKC210 board | expand

Commit Message

Philippe Mathieu-Daudé Oct. 29, 2018, 11:20 p.m. UTC 
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

Comments

Alistair Francis Oct. 29, 2018, 11:49 p.m. UTC | #1 
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
>
>
Richard Henderson Oct. 30, 2018, 8:18 a.m. UTC | #2 
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~
Philippe Mathieu-Daudé Oct. 30, 2018, 8:35 a.m. UTC | #3 
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.
Peter Maydell Oct. 30, 2018, 9:36 a.m. UTC | #4 
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
Philippe Mathieu-Daudé Oct. 30, 2018, 11:26 a.m. UTC | #5 
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 */
>
Philippe Mathieu-Daudé Oct. 30, 2018, 11:28 a.m. UTC | #6 
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
>
Peter Maydell Oct. 30, 2018, 12:42 p.m. UTC | #7 
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
Philippe Mathieu-Daudé Oct. 30, 2018, 1:01 p.m. UTC | #8 
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
>
diff mbox series

Patch

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 */