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

* [PATCH v3 0/2] hw/adc: Add basic Aspeed ADC model
@ 2021-10-05  5:26 pdel
  2021-10-05  5:26 ` [PATCH v3 1/2] " pdel
  2021-10-05  5:26 ` [PATCH v3 2/2] hw/arm: Integrate ADC model into Aspeed SoC pdel
  0 siblings, 2 replies; 3+ messages in thread
From: pdel @ 2021-10-05  5:26 UTC (permalink / raw)
  Cc: alistair, peter.maydell, clg, andrew, joel, qemu-arm, qemu-devel,
	zhdaniel, patrick, billy_tsai, Peter Delevoryas

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:

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



^ permalink raw reply related	[flat|nested] 3+ messages in thread