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

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Guenter Roeck <linux@roeck-us.net>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	Igor Mitsyanko <i.mitsyanko@gmail.com>,
	Alistair Francis <alistair@alistair23.me>,
	QEMU Developers <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH 2/3] hw/dma/pl330: Factor out pl330_init() from hw/arm/xilinx_zynq.c
Date: Tue, 30 Oct 2018 14:01:31 +0100	[thread overview]
Message-ID: <2aba69a5-c466-b94c-0d83-3f4fd9d1722c@redhat.com> (raw)
In-Reply-To: <CAFEAcA_6mDdxVU_TY583uOTgAX_eT-s52QRCDWTd0_uCrUTwhA@mail.gmail.com>

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
> 