@@ -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,
},
From: Peter Delevoryas <pdel@fb.com> v1: https://lore.kernel.org/qemu-devel/20211002214414.2858382-1-pdel@fbc.om/ v2: https://lore.kernel.org/qemu-devel/20211003191850.513658-1-pdel@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: