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

[Peter Delevoryas <pdel@fb.com>]

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

Yeah absolutely! I have to go look more at qtest a little, but I’m sure I
can add some kind of test for this.

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