2013/3/4 Peter Crosthwaite <peter.crosthwa...@xilinx.com>:
> Hi Kuo-Jung,
>
> On Mon, Mar 4, 2013 at 4:20 PM, Kuo-Jung Su <dant...@gmail.com> wrote:
>> 2013/3/2 Peter Crosthwaite <peter.crosthwa...@xilinx.com>:
>>> 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 access@0x%llx\n",
>>>> 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 access@0x%llx\n",
>>>> 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
