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

[Peter Delevoryas <pdel@fb.com>]

> On Oct 3, 2021, at 10:34 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
>>>> 
>>>> +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.

Yeah initially I was going to try to compute “nr-channels” in AspeedADCEngineState
but I came to the same conclusion, it would require adding properties in other
places.

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

Oh ok then, sounds good!

> 
> 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 ?

Oh yeah sure: I can also add a define for the parent region’s size, 0x1000,
even though it’s not used in two places, since somebody who’s interested
in one is probably interested in knowing the parent region size too.

#define ASPEED_ADC_MEMORY_REGION_SIZE        0x1000
#define ASPEED_ADC_ENGINE_MEMORY_REGION_SIZE 0x100

> 
> 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 */