On 2/25/11, andrzej zaborowski <balr...@gmail.com> wrote:
> Hi Dmitry,
>
> On 20 February 2011 14:50, Dmitry Eremin-Solenikov <dbarysh...@gmail.com>
> 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
