Re: [Qemu-devel] [PATCH v5 03/24] hw/arm: add Faraday FTINTC020 interrupt controller support

[Kuo-Jung Su]

2013/3/4 Peter Crosthwaite <address@hidden>:
> Hi Kuo-Jung,
>
> On Mon, Mar 4, 2013 at 4:20 PM, Kuo-Jung Su <address@hidden> wrote:
>> 2013/3/2 Peter Crosthwaite <address@hidden>:
>>> Hi Kuo-Jung,
> [Snip]
>>>> +        return s->irq_lvl[1];
>>>> +    case REG_EIRQSR:
>>>> +        return s->irq_src[1] & s->irq_ena[1];
>>>> +
>>>
>>> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
>>> Can you #define some symbols accrordingly and remove all the magic
>>> numberage with the [0]'s and [1]'s?
>>>
>>
>> Sure, the ftintc020 is going to be redesigned with the 'hw/pl190.c' as 
>> template.
>> And all the coding style issues would be updated.
>>
>>>> +    /*
>>>> +     * FIQ
>>>> +     */
>>>> +    case REG_FIQSRC:
>>>> +        return s->fiq_src[0];
>>>> +    case REG_FIQENA:
>>>> +        return s->fiq_ena[0];
>>>> +    case REG_FIQMDR:
>>>> +        return s->fiq_mod[0];
>>>> +    case REG_FIQLVR:
>>>> +        return s->fiq_lvl[0];
>>>> +    case REG_FIQSR:
>>>> +        return s->fiq_src[0] & s->fiq_ena[0];
>>>> +    case REG_EFIQSRC:
>>>> +        return s->fiq_src[1];
>>>> +    case REG_EFIQENA:
>>>> +        return s->fiq_ena[1];
>>>> +    case REG_EFIQMDR:
>>>> +        return s->fiq_mod[1];
>>>> +    case REG_EFIQLVR:
>>>> +        return s->fiq_lvl[1];
>>>> +    case REG_EFIQSR:
>>>> +        return s->fiq_src[1] & s->fiq_ena[1];
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "ftintc020: undefined memory address@hidden", addr);
>>>> +        return 0;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void
>>>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned 
>>>> size)
>>>> +{
>>>> +    Ftintc020State *s = FTINTC020(opaque);
>>>> +
>>>> +    switch (addr) {
>>>> +    /*
>>>> +     * IRQ
>>>> +     */
>>>
>>> Ok to use one line comment. And elsewhere
>>>
>>>> +    case REG_IRQENA:
>>>> +        s->irq_ena[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQSCR:
>>>> +        value = ~(value & s->irq_mod[0]);
>>>> +        s->irq_src[0] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQMDR:
>>>> +        s->irq_mod[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQLVR:
>>>> +        s->irq_lvl[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQENA:
>>>> +        s->irq_ena[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQSCR:
>>>> +        value = ~(value & s->irq_mod[1]);
>>>> +        s->irq_src[1] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQMDR:
>>>> +        s->irq_mod[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQLVR:
>>>> +        s->irq_lvl[1] = (uint32_t)value;
>>>> +        break;
>>>> +
>>>> +    /*
>>>> +     * FIQ
>>>> +     */
>>>> +    case REG_FIQENA:
>>>> +        s->fiq_ena[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQSCR:
>>>> +        value = ~(value & s->fiq_mod[0]);
>>>> +        s->fiq_src[0] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQMDR:
>>>> +        s->fiq_mod[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQLVR:
>>>> +        s->fiq_lvl[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQENA:
>>>> +        s->fiq_ena[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQSCR:
>>>> +        value = ~(value & s->fiq_mod[1]);
>>>> +        s->fiq_src[1] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQMDR:
>>>> +        s->fiq_mod[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQLVR:
>>>> +        s->fiq_lvl[1] = (uint32_t)value;
>>>> +        break;
>>>> +
>>>> +    /* don't care */
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "ftintc020: undefined memory address@hidden", addr);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ftintc020_update(s);
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps mmio_ops = {
>>>> +    .read  = ftintc020_mem_read,
>>>> +    .write = ftintc020_mem_write,
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +    .valid = {
>>>> +        .min_access_size = 4,
>>>> +        .max_access_size = 4,
>>>> +    }
>>>> +};
>>>> +
>>>> +static void ftintc020_reset(DeviceState *ds)
>>>> +{
>>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>>> +
>>>> +    s->irq_pin[0] = 0;
>>>> +    s->irq_pin[1] = 0;
>>>> +    s->fiq_pin[0] = 0;
>>>> +    s->fiq_pin[1] = 0;
>>>> +
>>>> +    s->irq_src[0] = 0;
>>>> +    s->irq_src[1] = 0;
>>>> +    s->irq_ena[0] = 0;
>>>> +    s->irq_ena[1] = 0;
>>>> +    s->fiq_src[0] = 0;
>>>> +    s->fiq_src[1] = 0;
>>>> +    s->fiq_ena[0] = 0;
>>>> +    s->fiq_ena[1] = 0;
>>>> +
>>>> +    ftintc020_update(s);
>>>> +}
>>>> +
>>>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>>>
>>> I'm not sure this is the right place for this, I think device creation
>>> helpers belong on the machine / SoC level. Keep the device model self
>>> contained and clients call qdev_init themselves. Andreas have the
>>> rules change on this recently?
>>>
>>>> +{
>>>> +    int i;
>>>> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
>>>
>>> As the device is intended for use in an SoC, the SoC potentially needs
>>> to hold a pointer to it in order to call device destructors. This
>>> function should return ds for future use by the instantiator.
>>>
>>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>>> +
>>>
>>> Use an Object cast macro. Andreas, can we make this easier for
>>> reviewers and developers by adding blacklisted identifiers to
>>> checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?
>>>
>>>> +    s->cpu = cpu;
>>>
>>> Im thinking this is a QOM link. Its a connection from one device to
>>> another and the machine should be aware of it.
>>>
>>
>> Sorry, I can't get you well....
>> Did you mean adding the QOM link to cpu like this?
>>
>>     .........
>>     s->cpu = cpu_arm_init(s->cpu_model);
>>     if (!s->cpu) {
>>         hw_error("a369: Unable to find CPU definition\n");
>>         exit(1);
>>     }
>>     object_property_add_child(qdev_get_machine(),
>>                               "cpu",
>>                               OBJECT(s->cpu),
>>                               NULL);
>>     .........
>>
>
> Definately not a child note. object_property_add_link.
>

Got it, thanks.

>> However this would lead to an error like this:
>>
>> **
>> ERROR:qom/object.c:899:object_property_add_child: assertion failed:
>> (child->parent == NULL)
>>
>
> The CPU is already parented, which is why this is asserting. You can't
> adopt it as your own child here.
>
> But I wouldn't bother with a link approach at all anymore, see my
> later reply regarding changing over to a GPIO out approach which is
> more modern. Removes the need for this cpu linkage completely. Your
> rewrite based on the pl190 VIC should fix this anyway. This will all
> go away.
>

There is one more place that the 'cpu' pointer would be referenced,
please check here:

hw/arm/faraday_a369.c:

......
    if (args->kernel_filename) {
        ......
        arm_load_kernel(s->cpu, s->bi);
        ......
    }
......

I guess I still have to make a QOM link to the cpu pointer.

> Regards,
> Peter



--
Best wishes,
Kuo-Jung Su