Re: [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Cédric Le Goater]

> > > +static void aspeed_adc_instance_init(Object *obj)
> > > +{
> > > +    AspeedADCState *s = ASPEED_ADC(obj);
> > > +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
> > > +    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
> > > +
> > > +    for (int i = 0; i < aac->nr_engines; i++) {
> > > +        AspeedADCEngineState *engine = &s->engines[i];
> > > +        object_initialize_child(obj, "engine[*]", engine,
> > > +                                TYPE_ASPEED_ADC_ENGINE);
> > > +        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);
> >
> > Why have you moved the property initialization in the instance_init routine ?
>
> I think for some reason I thought this was necessary, or maybe I thought it
> was more appropriate? I think I was looking at aspeed_soc.c and saw
> most of the child initialization in the init() functions, not realize(), but

The only one is "silicon-rev" which is a constant for the SoC. That's why
we set it from the instance_init routine and I think we need it to initialize
other models also.

> I’ll just put both properties back in realize(), I don’t think there was any
> functional reason.

No. It makes sense. "engine-id" is an internal id which has no reason to
change after init. "nr-channels" is constant and could be derived from
AspeedADCClass. Idealy, we would compute "nr-channels" in AspeedADCEngineState
but this would require another property on the owning AspeedADCState object.

Let's it keep it. Unless someone has a better idea.

One extra remark below.

> > > +        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
> > > +    }
> > > +}
> > > +
> > > +static void aspeed_adc_set_irq(void *opaque, int n, int level)
> > > +{
> > > +    AspeedADCState *s = opaque;
> > > +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
> > > +    uint32_t pending = 0;
> > > +
> > > +    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
> > > +    for (int i = 0; i < aac->nr_engines; i++) {
> > > +        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
> > > +        pending |= irq_status << (i * 8);
> > > +    }
> > > +
> > > +    qemu_set_irq(s->irq, !!pending);
> > > +}
> > > +
> > > +static void aspeed_adc_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    AspeedADCState *s = ASPEED_ADC(dev);
> > > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > > +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
> > > +
> > > +    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
> > > +                                        s, NULL, aac->nr_engines);
> > > +
> > > +    sysbus_init_irq(sbd, &s->irq);
> > > +
> > > +    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
> > > +
> > > +    sysbus_init_mmio(sbd, &s->mmio);
> > > +
> > > +    for (int i = 0; i < aac->nr_engines; i++) {
> > > +        Object *eng = OBJECT(&s->engines[i]);
> > > +
> > > +        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
> > > +            return;
> > > +        }
> > > +        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
> > > +                           qdev_get_gpio_in(DEVICE(sbd), i));
> > > +        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);

Since we use 0x100 twice (my fault). May be add a define for it ?

Thanks,

C.

> > > +    }
> > > +}
> > > +
> > > +static void aspeed_adc_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
> > > +
> > > +    dc->realize = aspeed_adc_realize;
> > > +    dc->desc = "Aspeed Analog-to-Digital Converter";
> > > +    aac->nr_engines = 1;
> > > +}
> > > +
> > > +static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
> > > +
> > > +    dc->desc = "ASPEED 2600 ADC Controller";
> > > +    aac->nr_engines = 2;
> > > +}
> > > +
> > > +static const TypeInfo aspeed_adc_info = {
> > > +    .name = TYPE_ASPEED_ADC,
> > > +    .parent = TYPE_SYS_BUS_DEVICE,
> > > +    .instance_init = aspeed_adc_instance_init,
> > > +    .instance_size = sizeof(AspeedADCState),
> > > +    .class_init = aspeed_adc_class_init,
> > > +    .class_size = sizeof(AspeedADCClass),
> > > +    .abstract   = true,
> > > +};
> > > +
> > > +static const TypeInfo aspeed_2400_adc_info = {
> > > +    .name = TYPE_ASPEED_2400_ADC,
> > > +    .parent = TYPE_ASPEED_ADC,
> > > +};
> > > +
> > > +static const TypeInfo aspeed_2500_adc_info = {
> > > +    .name = TYPE_ASPEED_2500_ADC,
> > > +    .parent = TYPE_ASPEED_ADC,
> > > +};
> > > +
> > > +static const TypeInfo aspeed_2600_adc_info = {
> > > +    .name = TYPE_ASPEED_2600_ADC,
> > > +    .parent = TYPE_ASPEED_ADC,
> > > +    .class_init = aspeed_2600_adc_class_init,
> > > +};
> > > +
> > > +static void aspeed_adc_register_types(void)
> > > +{
> > > +    type_register_static(&aspeed_adc_engine_info);
> > > +    type_register_static(&aspeed_adc_info);
> > > +    type_register_static(&aspeed_2400_adc_info);
> > > +    type_register_static(&aspeed_2500_adc_info);
> > > +    type_register_static(&aspeed_2600_adc_info);
> > > +}
> > > +
> > > +type_init(aspeed_adc_register_types);
> > > diff --git a/hw/adc/meson.build b/hw/adc/meson.build
> > > index ac4f093fea..b29ac7ccdf 100644
> > > --- a/hw/adc/meson.build
> > > +++ b/hw/adc/meson.build
> > > @@ -1,4 +1,5 @@
> > >   softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: 
> > > files('stm32f2xx_adc.c'))
> > > +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
> > >   softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
> > >   softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
> > >   softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
> > > diff --git a/hw/adc/trace-events b/hw/adc/trace-events
> > > index 456f21c8f4..5a4c444d77 100644
> > > --- a/hw/adc/trace-events
> > > +++ b/hw/adc/trace-events
> > > @@ -3,3 +3,6 @@
> > >   # npcm7xx_adc.c
> > >   npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" 
> > > PRIx64 " value 0x%04" PRIx32
> > >   npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" 
> > > PRIx64 " value 0x%04" PRIx32
> > > +
> > > +aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 
> > > 0x%" PRIx64 " 0x%" PRIx64
> > > +aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 
> > > 0x%" PRIx64 " 0x%" PRIx64
> > > diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
> > > new file mode 100644
> > > index 0000000000..2f166e8be1
> > > --- /dev/null
> > > +++ b/include/hw/adc/aspeed_adc.h
> > > @@ -0,0 +1,55 @@
> > > +/*
> > > + * Aspeed ADC
> > > + *
> > > + * Copyright 2017-2021 IBM Corp.
> > > + *
> > > + * Andrew Jeffery <andrew@aj.id.au>
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + */
> > > +
> > > +#ifndef HW_ADC_ASPEED_ADC_H
> > > +#define HW_ADC_ASPEED_ADC_H
> > > +
> > > +#include "hw/sysbus.h"
> > > +
> > > +#define TYPE_ASPEED_ADC "aspeed.adc"
> > > +#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
> > > +#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
> > > +#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
> > > +OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
> > > +
> > > +#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
> > > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
> > > +
> > > +#define ASPEED_ADC_NR_CHANNELS 16
> > > +#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
> > > +
> > > +struct AspeedADCEngineState {
> > > +    /* <private> */
> > > +    SysBusDevice parent;
> > > +
> > > +    MemoryRegion mmio;
> > > +    qemu_irq irq;
> > > +    uint32_t engine_id;
> > > +    uint32_t nr_channels;
> > > +    uint32_t regs[ASPEED_ADC_NR_REGS];
> > > +};
> > > +
> > > +struct AspeedADCState {
> > > +    /* <private> */
> > > +    SysBusDevice parent;
> > > +
> > > +    MemoryRegion mmio;
> > > +    qemu_irq irq;
> > > +
> > > +    AspeedADCEngineState engines[2];
> > > +};
> > > +
> > > +struct AspeedADCClass {
> > > +    SysBusDeviceClass parent_class;
> > > +
> > > +    uint32_t nr_engines;
> > > +};
> > > +
> > > +#endif /* HW_ADC_ASPEED_ADC_H */