Re: [Qemu-devel] [PATCH 02/10] pxa2xx_pic: update to use qdev and arm-pic

[Dmitry Eremin-Solenikov]

On 2/25/11, andrzej zaborowski <address@hidden> wrote:
> Hi Dmitry,
>
> On 20 February 2011 14:50, Dmitry Eremin-Solenikov <address@hidden>
> wrote:
>> Use qdev/sysbus framework to handle pxa2xx-pic. Instead of exposing IRQs
>> via array, reference them via qdev_get_gpio_in(). Also pxa2xx_pic
>> duplicated
>> some code from arm-pic. Drop it, replacing with references to arm-pic,
>> as all other ARM SoCs do for their PIC code.
>
> As I said earlier not using arm-pic was deliberate (and I also asked
> what the gain was from converting the pic to a separate sysbus device
> from the CPU) so I skipped this part of the patch and pushed the rest
> of it, please check that everything works.

The primary goal was using arm-pic IRQs in pxa2xx-gpio and not having to
mess with passing CPUEnv around. Moreover all other ARM SoCs use
arm-pic w/o any references to performance gains/loses.

I can still provide a patch that will use arm-pic only for
pxa2xx-gpio, will that
be suitable for you?

BTW: it seems that your version won't work: using of sysbus_init_mmio()
is hackish and there is no place where that mmio region will be mapped to base.

About mapping pic to a separate device from CPU. Initially I wanted to reuse
somehow pxa2xx-pic for sa-11[0-1]0 emulation. It doesn't seem reasonable
for me anymore anyway. Second, the pic is already in separate
module, so I didn't want to disturb main pxa2xx.c with it.
I might still later use pxa2xx-pic for allocating main CPU structure and
making all other device hang on ot.

>> diff --git a/hw/mainstone.c b/hw/mainstone.c
>> index aec8d34..4eabdb9 100644
>> --- a/hw/mainstone.c
>> +++ b/hw/mainstone.c
>> @@ -140,7 +140,7 @@ static void mainstone_common_init(ram_addr_t ram_size,
>>     }
>>
>>     mst_irq = sysbus_create_simple("mainstone-fpga", MST_FPGA_PHYS,
>> -                    cpu->pic[PXA2XX_PIC_GPIO_0]);
>> +                    qdev_get_gpio_in(cpu->pic, PXA2XX_PIC_GPIO_0));
>
> I'm also wondering if this device should really use the interrupt line
> instead of using a GPIO.  It seems wrong that both the fpga and the
> gpio module are connected to the same line.

Fixed, will submit a fix soon.

>> @@ -241,53 +239,33 @@ static CPUWriteMemoryFunc * const
>> pxa2xx_pic_writefn[] = {
>>     pxa2xx_pic_mem_write,
>>  };
>>
>> -static void pxa2xx_pic_save(QEMUFile *f, void *opaque)
>> +static int pxa2xx_pic_post_load(void *opaque, int version_id)
>>  {
>> -    PXA2xxPICState *s = (PXA2xxPICState *) opaque;
>> -    int i;
>> -
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_put_be32s(f, &s->int_enabled[i]);
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_put_be32s(f, &s->int_pending[i]);
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_put_be32s(f, &s->is_fiq[i]);
>> -    qemu_put_be32s(f, &s->int_idle);
>> -    for (i = 0; i < PXA2XX_PIC_SRCS; i ++)
>> -        qemu_put_be32s(f, &s->priority[i]);
>> +    pxa2xx_pic_update(opaque);
>> +    return 0;
>>  }
>>
>> -static int pxa2xx_pic_load(QEMUFile *f, void *opaque, int version_id)
>> +DeviceState *pxa2xx_pic_init(target_phys_addr_t base, CPUState *env,
>> +        qemu_irq *arm_pic)
>>  {
>> -    PXA2xxPICState *s = (PXA2xxPICState *) opaque;
>> -    int i;
>> -
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_get_be32s(f, &s->int_enabled[i]);
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_get_be32s(f, &s->int_pending[i]);
>> -    for (i = 0; i < 2; i ++)
>> -        qemu_get_be32s(f, &s->is_fiq[i]);
>> -    qemu_get_be32s(f, &s->int_idle);
>> -    for (i = 0; i < PXA2XX_PIC_SRCS; i ++)
>> -        qemu_get_be32s(f, &s->priority[i]);
>> +    DeviceState *dev;
>>
>> -    pxa2xx_pic_update(opaque);
>> -    return 0;
>> +    dev = sysbus_create_varargs("pxa2xx_pic", base,
>> +            arm_pic[ARM_PIC_CPU_IRQ],
>> +            arm_pic[ARM_PIC_CPU_FIQ],
>> +            arm_pic[ARM_PIC_CPU_WAKE],
>> +            NULL);
>> +
>> +    /* Enable IC coprocessor access.  */
>> +    cpu_arm_set_cp_io(env, 6, pxa2xx_pic_cp_read, pxa2xx_pic_cp_write,
>> dev);
>
> I changed the last parameter to "s" as passing dev here was hacky.


Fine with me.


BTW: what about all other patches?

-- 
With best wishes
Dmitry