From: Peter Delevoryas <p...@fb.com> v1: https://lore.kernel.org/qemu-devel/20211002214414.2858382-1-p...@fbc.om/ v2: https://lore.kernel.org/qemu-devel/20211003191850.513658-1-p...@fb.com/ v3: - Reduced the minimum access size to 2, to allow 16-bit reads - Shifted the read value down 16 bits when reading an odd channel's data register.
So, v1 and v2 only attempted to support 32-bit reads and writes, but Patrick was testing Witherspoon with my patches and revealed that the upstream kernel driver (I was looking at 5.10 and 5.14) definitely performs 16-bit reads of each channel, and that my patches crash when that happens. https://lore.kernel.org/openbmc/YVtJTrgm3b3W4PY9@heinlein/ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { struct aspeed_adc_data *data = iio_priv(indio_dev); const struct aspeed_adc_model_data *model_data = of_device_get_match_data(data->dev); switch (mask) { case IIO_CHAN_INFO_RAW: if (!strcmp(model_data->model_name, "ast2600-adc")) { *val = readw(data->base + chan->address) + data->cv; ^^^^^ } else { *val = readw(data->base + chan->address); ^^^^^ } return IIO_VAL_INT; I actually tested an image that uses this driver, but I wasn't testing reads through the driver, I was just using the QEMU monitor, so it didn't crash. It's easy to at least relax the restrictions enough to allow the 16-bit reads, and it's also not that hard to return the correct channel from the channel data sampling. I didn't attempt to do anything special for correctness of other registers, because I think we just perform 32-bit reads and writes for them, and I didn't attempt to do the correct thing for 16-bit writes to the data registers since that's not done by the driver. (And I'm not sure the hardware supports that anyways?) 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 | 427 ++++++++++++++++++++++++++++++++++++ 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, 510 insertions(+) create mode 100644 hw/adc/aspeed_adc.c create mode 100644 include/hw/adc/aspeed_adc.h Interdiff against v2: diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c index fcd93d6853..c5fcae29f6 100644 --- a/hw/adc/aspeed_adc.c +++ b/hw/adc/aspeed_adc.c @@ -148,6 +148,11 @@ static uint64_t aspeed_adc_engine_read(void *opaque, hwaddr addr, /* fallthrough */ case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6: value = read_channel_sample(s, reg); + /* Allow 16-bit reads of the data registers */ + if (addr & 0x2) { + assert(size == 2); + value >>= 16; + } break; default: qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: 0x%" HWADDR_PRIx "\n", @@ -234,7 +239,7 @@ static const MemoryRegionOps aspeed_adc_engine_ops = { .write = aspeed_adc_engine_write, .endianness = DEVICE_LITTLE_ENDIAN, .valid = { - .min_access_size = 4, + .min_access_size = 2, .max_access_size = 4, .unaligned = false, }, -- 2.30.2
