There are a couple of things I'm not confident about here:
- what should be in init vs realize?
- should the irq state be in vmstate?
- is there a better way to do composition of classes (patch 3)?

v3:
- didn't have each gpio set up as an irq 
- now can't access set AC on ast2400 (only exists on ast2500)
- added ast2600 implementation (patch 3)
- renamed a couple of variables for clarity


v2: Addressed Andrew's feedback, added debounce regs, renamed get/set to
read/write to minimise confusion with a 'set' of registers.

Rashmica Gupta (3):
  hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500
  aspeed: add a GPIO controller to the SoC
  hw/gpio: Add in AST2600 specific implementation

 hw/arm/aspeed_soc.c           |   17 +
 hw/gpio/Makefile.objs         |    1 +
 hw/gpio/aspeed_gpio.c         | 1103 +++++++++++++++++++++++++++++++++
 include/hw/arm/aspeed_soc.h   |    3 +
 include/hw/gpio/aspeed_gpio.h |   91 +++
 5 files changed, 1215 insertions(+)
 create mode 100644 hw/gpio/aspeed_gpio.c
 create mode 100644 include/hw/gpio/aspeed_gpio.h

-- 
2.20.1

On 30/07/2019 07:44, Rashmica Gupta wrote:
> There are a couple of things I'm not confident about here:
> - what should be in init vs realize?
> - should the irq state be in vmstate?
> - is there a better way to do composition of classes (patch 3)?

You can not do twice : 

     obj = object_new(TYPE_ASPEED_GPIO "-ast2600");

in aspeed_2600_gpio_realize(). This feels wrong. 

Let's see if we can instantiate two GPIO devices with different types 
under the AST2600 SoC instead.

Thanks,

C.

> 
> v3:
> - didn't have each gpio set up as an irq 
> - now can't access set AC on ast2400 (only exists on ast2500)
> - added ast2600 implementation (patch 3)
> - renamed a couple of variables for clarity
> 
> 
> v2: Addressed Andrew's feedback, added debounce regs, renamed get/set to
> read/write to minimise confusion with a 'set' of registers.
> 
> Rashmica Gupta (3):
>   hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500
>   aspeed: add a GPIO controller to the SoC
>   hw/gpio: Add in AST2600 specific implementation
> 
>  hw/arm/aspeed_soc.c           |   17 +
>  hw/gpio/Makefile.objs         |    1 +
>  hw/gpio/aspeed_gpio.c         | 1103 +++++++++++++++++++++++++++++++++
>  include/hw/arm/aspeed_soc.h   |    3 +
>  include/hw/gpio/aspeed_gpio.h |   91 +++
>  5 files changed, 1215 insertions(+)
>  create mode 100644 hw/gpio/aspeed_gpio.c
>  create mode 100644 include/hw/gpio/aspeed_gpio.h
> 

On Tue, 30 Jul 2019 at 06:45, Rashmica Gupta <rashmica.g@gmail.com> wrote:
>
> There are a couple of things I'm not confident about here:
> - what should be in init vs realize?

We are not very good at documenting this distinction (and there's
some bits of it I'm not sure about either), but:
 * init cannot contain anything that might fail
 * init cannot contain anything that affects the simulation
 * init shouldn't do anything that needs explicit cleanup
   (eg memory allocation)
 * property creation has to happen in init, because properties
   are set after init but before realize

Thomas did a good blog post on this:
http://people.redhat.com/~thuth/blog/qemu/2018/09/10/instance-init-realize.html


> - should the irq state be in vmstate?

I guess you mean the "uint32_t int_status;" field here?
If it's state that's in your device then it needs to be
in vmstate (this roughly corresponds to 'is there a flipflop
in the hardware that holds this state', though that is not
a 100% perfect guide). A 'qemu_irq' holds no state of its own,
so you cannot query it for the state of the line after migration.
So if your device model needs to be able to know that state
itself then it has to keep it in a struct field and migrate
that struct field. (For some devices the state of the outbound
interrupt line is a purely combinatorial result of some other
state: "int_output = int_status & int_mask" is a common one
where there's a "raw interrupt status" and a mask bit that
suppresses the outbound irq line. In that case the int_output
need not be in the device's state struct or migrated, because
we can just calculate it when we need it from the int_status
and int_mask state.)

> - is there a better way to do composition of classes (patch 3)?

No strong opinion. From a quick scan through of patch 3 it
didn't look obviously doing something in a suboptimal way.

thanks
-- PMM