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

[Peter Crosthwaite]

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.

> 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.

Regards,
Peter