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

[Philippe Mathieu-Daudé]

On 30/10/18 13:42, Peter Maydell wrote:
> On 30 October 2018 at 11:28, Philippe Mathieu-Daudé <address@hidden> wrote:
> > On 30/10/18 10:36, Peter Maydell wrote:
> > > On 29 October 2018 at 23:20, Philippe Mathieu-Daudé <address@hidden>
> > > wrote:
> > > > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > > > ---
> > > >    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