Re: [Qemu-devel] [RFC/RFT v1 4/8] xilinx: Inline usages of xilinx_intc_create()

[Peter Crosthwaite <peter.crosthwaite@xilinx.com>]

Hi Edgar,

On Sun, Jan 12, 2014 at 11:32 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Wed, Jan 01, 2014 at 04:03:06PM -0800, Peter Crosthwaite wrote:
>> Inline these usages. Converts these init to at least a semi-recent QOM
>> styling.
>
> Thanks Peter,
>
> I dont mind the patches but can you clarify the win with this? Looks
> like we are mostly reducing code reusage?
>

The code reusage is a minimal win as there are only at most 3 usages
and we have no plans to add more. On the plus side, the APIs that are
wrapped do serve nicely as self-documenting code when pulled back into
the machine model:

qdev_prop_set_uint32(dev, "kind-of-intr",
                     1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);

inline in the machine model, is much clearer than having to check
xilinx.h to see what the 3rd argument of xilinx_intc_create means.

The other motivation is for the per-master-address work which I wanted
to look into bringing online for device-land (via some patches to
sysbus etc.). To register a non-root address space for each device,
the AddressSpace argument would have to be added to all the xilinx.h
APIs increasing the footprint of these helper functions just a little
more. The diff on converting just your lead example of the single
microblaze machine to PMA is minimised with this series as it doesn't
affect the other two xilinx.h dependent boards anymore.

> A similar patch was discussed a while back ago:
> http://lists.gnu.org/archive/html/qemu-devel/2009-09/msg00978.html
>

I agree with Markus' original patch, but mainly because since 2009
(and that discussion) QOM styling has moved a long way in favor of
Markus' arguments.

Regards,
Peter

> Cheers,
> Edgar
>
>
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/microblaze/petalogix_ml605_mmu.c      |  6 +++++-
>>  hw/microblaze/petalogix_s3adsp1800_mmu.c |  7 ++++++-
>>  hw/ppc/virtex_ml507.c                    |  6 +++++-
>>  include/hw/xilinx.h                      | 13 -------------
>>  4 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>> index 5dfc0fa..4354971 100644
>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>> @@ -126,7 +126,11 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
>>
>>
>>      cpu_irq = microblaze_pic_init_cpu(env);
>> -    dev = xilinx_intc_create(INTC_BASEADDR, cpu_irq[0], 4);
>> +    dev = qdev_create(NULL, "xlnx.xps-intc");
>> +    qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ);
>> +    qdev_init_nofail(dev);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq[0]);
>>      for (i = 0; i < 32; i++) {
>>          irq[i] = qdev_get_gpio_in(dev, i);
>>      }
>> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> index 1972fea..6598687 100644
>> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> @@ -101,7 +101,12 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
>>                            1, 0x89, 0x18, 0x0000, 0x0, 1);
>>
>>      cpu_irq = microblaze_pic_init_cpu(env);
>> -    dev = xilinx_intc_create(INTC_BASEADDR, cpu_irq[0], 0xA);
>> +    dev = qdev_create(NULL, "xlnx.xps-intc");
>> +    qdev_prop_set_uint32(dev, "kind-of-intr",
>> +                         1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);
>> +    qdev_init_nofail(dev);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq[0]);
>>      for (i = 0; i < 32; i++) {
>>          irq[i] = qdev_get_gpio_in(dev, i);
>>      }
>> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
>> index 88312f8..1e2f532 100644
>> --- a/hw/ppc/virtex_ml507.c
>> +++ b/hw/ppc/virtex_ml507.c
>> @@ -218,7 +218,11 @@ static void virtex_init(QEMUMachineInitArgs *args)
>>                            1, 0x89, 0x18, 0x0000, 0x0, 1);
>>
>>      cpu_irq = (qemu_irq *) &env->irq_inputs[PPC40x_INPUT_INT];
>> -    dev = xilinx_intc_create(0x81800000, cpu_irq[0], 0);
>> +    dev = qdev_create(NULL, "xlnx.xps-intc");
>> +    qdev_prop_set_uint32(dev, "kind-of-intr", 0);
>> +    qdev_init_nofail(dev);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq[0]);
>>      for (i = 0; i < 32; i++) {
>>          irq[i] = qdev_get_gpio_in(dev, i);
>>      }
>> diff --git a/include/hw/xilinx.h b/include/hw/xilinx.h
>> index 9d6debe..02400cd 100644
>> --- a/include/hw/xilinx.h
>> +++ b/include/hw/xilinx.h
>> @@ -7,19 +7,6 @@
>>  #include "hw/stream.h"
>>  #include "net/net.h"
>>
>> -static inline DeviceState *
>> -xilinx_intc_create(hwaddr base, qemu_irq irq, int kind_of_intr)
>> -{
>> -    DeviceState *dev;
>> -
>> -    dev = qdev_create(NULL, "xlnx.xps-intc");
>> -    qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr);
>> -    qdev_init_nofail(dev);
>> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>> -    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
>> -    return dev;
>> -}
>> -
>>  /* OPB Timer/Counter.  */
>>  static inline DeviceState *
>>  xilinx_timer_create(hwaddr base, qemu_irq irq, int oto, int freq)
>> --
>> 1.8.5.2
>>
>