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

[Cédric Le Goater]

On 10/2/21 23:44, pdel@fbc.om wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> Following up from
>
> https://lore.kernel.org/qemu-devel/20210930004235.1656003-1-pdel@fb.com/
>
> This is a resubmission of Andrew Jeffery's ADC model, but with the
> registers converted from typed-member-fields to a regs[] array.
> Otherwise, it should be pretty much equivalent.

Thanks for taking over.

> References to the original patches from Andrew:
>
> https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4
> https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f
>
> I did A/B "--trace aspeed_adc_engine_*" testing and the output from the
> boot sequence was equivalent, and I tested that the channel simulation
> code produces the same sequence of values.
>
> Here's the initialization sequence:
>
> aspeed_adc_engine_write engine[0] 0x0 0xf  ; Engine init
> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set
> aspeed_adc_engine_write engine[0] 0x0 0x2f ; Auto compensating sensing mode
> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set (redoing engine init, 
> lol), bit 5 reset
>
> Here's some of the channel simulation:
>
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00070005
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x000e000a
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x0015000f
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x001c0014
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00230019
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x002a001e
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00310023
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00380028
>
> This was tested with a Fuji OpenBMC image:
>
> https://github.com/peterdelevoryas/openbmc/releases/download/fuji.mtd.0/fuji.mtd
>
> My refactoring was kinda awkward, it took me a few tries to finally get
> something that made sense to me. In particular, I don't know if you guys
> realized this previously, but in the AST2600, there's 2 engines, but
> still only 16 channels: each engine only has half the registers, as far
> as I understand.

yes.

 
> That's why I added the "nr-channels" property to the engine. I also
> tried to add guest-error logs when a driver tries to read or write to
> the upper-eight channel registers in the 2600 engines. For example:
>
> (qemu) xp 0x1e6e9000
> 000000001e6e9000: 0xffff010f
> (qemu) xp 0x1e6e9020
> aspeed_adc_engine_read: engine[0]: data register 4 invalid, only 0...3 valid
> 000000001e6e9020: 0x00000000
> (qemu) xp 0x1e6e9030
> 000000001e6e9030: 0x00000000
> (qemu) xp 0x1e6e9040
> 000000001e6e9040: 0x00000000
> (qemu) xp 0x1e6e9050
> aspeed_adc_engine_read: engine[0]: bounds register 8 invalid, only 0...7 valid
> 000000001e6e9050: 0x00000000
> (qemu) xp 0x1e6e9060
> aspeed_adc_engine_read: engine[0]: bounds register 12 invalid, only 0...7 valid
> 000000001e6e9060: 0x00000000
> (qemu) xp 0x1e6e9070
> 000000001e6e9070: 0x00000000
> (qemu) xp 0x1e6e9080
> 000000001e6e9080: 0x00000000
> (qemu) xp 0x1e6e9090
> aspeed_adc_engine_read: engine[0]: hysteresis register 8 invalid, only 0...7 
> valid
> 000000001e6e9090: 0x00000000
> (qemu) xp 0x1e6e90a0
> aspeed_adc_engine_read: engine[0]: hysteresis register 12 invalid, only 0...7 
> valid
> 000000001e6e90a0: 0x00000000

This looks nice.

> It might turn out that we should just stick with a closer version of
> Andrew's original patch, and that would be totally fine with me, I just
> want to get the ADC support merged one way or another.


Do you have a test case we could include in qtest ? This is not a
requirement but it's always good to have.

Thanks,

C.

> Also, I'm interested in resubmitting Andrew's work on supporting
> unaligned accesses for models that only implement aligned access, to
> make supporting 8-bit and 16-bit reads transparent to this model, but
> I'll submit that separately.
>
>
> Reference: 
> https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/
>
> Thanks,
> Peter
>
> Andrew Jeffery (2):
>    hw/adc: Add basic Aspeed ADC model
>    hw/arm: Integrate ADC model into Aspeed SoC
>
>   hw/adc/aspeed_adc.c         | 416 ++++++++++++++++++++++++++++++++++++
>   hw/adc/meson.build          |   1 +
>   hw/adc/trace-events         |   3 +
>   hw/arm/aspeed_ast2600.c     |  11 +
>   hw/arm/aspeed_soc.c         |  11 +
>   include/hw/adc/aspeed_adc.h |  55 +++++
>   include/hw/arm/aspeed_soc.h |   2 +
>   7 files changed, 499 insertions(+)
>   create mode 100644 hw/adc/aspeed_adc.c
>   create mode 100644 include/hw/adc/aspeed_adc.h