| Message ID | 20190730054501.32727-1-rashmica.g@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Aspeed GPIO controller model | expand |
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
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 >