Re: [Qemu-devel] [Qemu-arm] [PATCH v2 3/9] hw/arm/mps2: Add UARTs

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On 14 July 2017 at 16:52, Alex Bennée <address@hidden> wrote:
>>
>> Peter Maydell <address@hidden> writes:
>>
>>> Add the UARTs to the MPS2 board models.
>>>
>>> Unfortunately the details of the wiring of the interrupts through
>>> various OR gates differ between AN511 and AN385 so this can't
>>> be purely a data-driven difference.
>>>
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> Reviewed-by: Alistair Francis <address@hidden>
>>> ---
>>>  hw/arm/mps2.c | 86 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 86 insertions(+)
>>>
>>> diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
>>> index 3dad02d..180c5d2 100644
>>> --- a/hw/arm/mps2.c
>>> +++ b/hw/arm/mps2.c
>>> @@ -27,9 +27,12 @@
>>>  #include "qemu/error-report.h"
>>>  #include "hw/arm/arm.h"
>>>  #include "hw/arm/armv7m.h"
>>> +#include "hw/or-irq.h"
>>>  #include "hw/boards.h"
>>>  #include "exec/address-spaces.h"
>>> +#include "sysemu/sysemu.h"
>>>  #include "hw/misc/unimp.h"
>>> +#include "hw/char/cmsdk-apb-uart.h"
>>>
>>>  typedef enum MPS2FPGAType {
>>>      FPGA_AN385,
>>> @@ -206,6 +209,89 @@ static void mps2_common_init(MachineState *machine)
>>>      create_unimplemented_device("Ethernet", 0x40200000, 0x00100000);
>>>      create_unimplemented_device("VGA", 0x41000000, 0x0200000);
>>>
>>> +    switch (mmc->fpga_type) {
>>> +    case FPGA_AN385:
>>> +    {
>>> +        /* The overflow IRQs for UARTs 0, 1 and 2 are ORed together.
>>> +         * Overflow for UARTs 4 and 5 doesn't trigger any interrupt.
>>> +         */
>>> +        Object *orgate;
>>> +        DeviceState *orgate_dev;
>>> +        int i;
>>> +
>>> +        orgate = object_new(TYPE_OR_IRQ);
>>> +        object_property_set_int(orgate, 6, "num-lines", &error_fatal);
>>> +        object_property_set_bool(orgate, true, "realized", &error_fatal);
>>> +        orgate_dev = DEVICE(orgate);
>>> +        qdev_connect_gpio_out(orgate_dev, 0, qdev_get_gpio_in(armv7m, 12));
>>> +
>>> +        for (i = 0; i < 5; i++) {
>>> +            hwaddr uartbase[] = {0x40004000, 0x40005000, 0x40006000,
>>> +                                 0x40007000, 0x40009000};
>>
>> I would expect these to be something like:
>>
>>   static hwaddr an385_uartbase[] = {0x40004000, 0x40005000, 0x40006000,
>>                                     0x40007000, 0x40009000};
>>   static hwaddr an511_uartbase[] = {0x40004000, 0x40005000, 0x4002c000,
>>                                     0x4002d000, 0x4002e000};
>>
>> to save the compiler from filling in the table every loop. Not that it
>> makes much different for an init routine.
>
> I'm slightly surprised the compiler can't tell that these
> are never written and emit the same code for all of them,
> but yeah, let's just 'static const' all of them.

I was just curious so I objdumped - maybe we should poke our TCWG
friends ;-)

--
Alex Bennée