Re: [Qemu-devel] [PATCH v4 3/3] hw/gpio: Add in AST2600 specific implementation

[Rashmica Gupta]

On Wed, 2019-08-14 at 14:37 +0200, Cédric Le Goater wrote:
> On 14/08/2019 09:14, Rashmica Gupta wrote:

...

> > +static void aspeed_2600_gpio_realize(DeviceState *dev, Error
> > **errp)
> > +{
> > +    AspeedGPIOState *s = ASPEED_GPIO(dev);
> > +    AspeedGPIOState *s_1_8, *s_3_6;
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +    AspeedGPIOClass *agc, *agc2;
> > +    int pin;
> > +    void *obj;
> > +
> > +    /* Create and setup the 1.8V gpio state/class */
> > +    obj = object_new(TYPE_ASPEED_GPIO "-ast2600");
> > +    s_1_8 = ASPEED_GPIO(obj);
> > +    object_property_add_child(OBJECT(dev), TYPE_ASPEED_GPIO "-
> > ast2600-1.8v",
> > +                              obj, errp);
> > +    if (error_abort) {
> > +        error_propagate(errp, error_abort);
> > +    }
> 
> This looks wrong. 
> 
> Shouldn't we instantiate 2 GPIOs devices under the AST2600 SoC with
> different
> types ? 
> 

We were trying to keep one GPIO device with two states, one state for
3.6V gpios and one for the 1.8V gpios. As you can see I couldn't find
a nice way to do that. I'll have a go at just making two seperate
devices.


> C.
> 
> 
> > +    agc = ASPEED_GPIO_GET_CLASS(s_1_8);
> > +    agc->ctrl = (void *)&aspeed_gpio_ast2600_1_8v_controller;
> > +    aspeed_gpio_init(obj);
> > +
> > +    /* Create and setup the 3.6V gpio state/class */
> > +    obj = object_new(TYPE_ASPEED_GPIO "-ast2600");
> > +    s_3_6 = ASPEED_GPIO(obj);
> > +    object_property_add_child(OBJECT(dev), TYPE_ASPEED_GPIO "-
> > ast2600-3.6v",
> > +                              obj, errp);
> > +    if (error_abort) {
> > +        error_propagate(errp, error_abort);
> > +    }
> > +    agc2 = ASPEED_GPIO_GET_CLASS(s_3_6);
> > +    agc2->ctrl = (void *)&aspeed_gpio_ast2600_3_6v_controller;
> > +    aspeed_gpio_init(obj);
> > +
> > +    for (pin = 0; pin < agc->ctrl->nr_gpio_pins; pin++) {
> > +        sysbus_init_irq(sbd, &s->gpios[pin]);
> > +    }
> > +
> > +    memory_region_init_io(&s_3_6->iomem, OBJECT(s_3_6),
> > &aspeed_gpio_ops, s_3_6,
> > +            TYPE_ASPEED_GPIO, GPIO_3_6V_MEM_SIZE +
> > GPIO_1_8V_MEM_SIZE);
> > +    s_3_6->lookup = aspeed_3_6v_gpios;
> > +
> > +    memory_region_init_io(&s_1_8->iomem, OBJECT(s_1_8),
> > &aspeed_gpio_ops, s_1_8,
> > +            TYPE_ASPEED_GPIO, GPIO_1_8V_MEM_SIZE);
> > +    memory_region_add_subregion(&s_3_6->iomem,
> > GPIO_1_8V_REG_OFFSET,
> > +                                &s_1_8->iomem);
> > +    s_1_8->lookup = aspeed_1_8v_gpios;
> > +
> > +    sysbus_init_mmio(sbd, &s_3_6->iomem);
> > +    sysbus_init_mmio(sbd, &s_1_8->iomem);
> > +}
> > +
> >  static const VMStateDescription vmstate_gpio_regs = {
> >      .name = TYPE_ASPEED_GPIO"/regs",
> >      .version_id = 1,
> > @@ -830,6 +991,16 @@ static const VMStateDescription
> > vmstate_aspeed_gpio = {
> >     }
> >  };
> >  
> > +static void aspeed_gpio_ast2600_class_init(ObjectClass *klass,
> > void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = aspeed_2600_gpio_realize;
> > +    dc->reset = aspeed_gpio_reset;
> > +    dc->desc = "Aspeed GPIO Controller";
> > +    dc->vmsd = &vmstate_aspeed_gpio;
> > +}
> > +
> >  static void aspeed_gpio_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -866,11 +1037,18 @@ static const TypeInfo
> > aspeed_gpio_ast2500_info = {
> >      .class_data     = (void *)&aspeed_gpio_ast2500_controller,
> >  };
> >  
> > +static const TypeInfo aspeed_gpio_ast2600_info = {
> > +    .name           = TYPE_ASPEED_GPIO "-ast2600",
> > +    .parent         = TYPE_ASPEED_GPIO,
> > +    .class_init     = aspeed_gpio_ast2600_class_init,
> > +};
> > +
> >  static void aspeed_gpio_register_types(void)
> >  {
> >      type_register_static(&aspeed_gpio_info);
> >      type_register_static(&aspeed_gpio_ast2400_info);
> >      type_register_static(&aspeed_gpio_ast2500_info);
> > +    type_register_static(&aspeed_gpio_ast2600_info);
> >  }
> >  
> >  type_init(aspeed_gpio_register_types);
> > diff --git a/slirp b/slirp
> > index f0da672620..126c04acba 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit f0da6726207b740f6101028b2992f918477a4b08
> > +Subproject commit 126c04acbabd7ad32c2b018fe10dfac2a3bc1210
> >