All of lore.kernel.org
 help / color / mirror / Atom feed
* [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

end of thread, other threads:[~2021-10-05  5:33 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05  5:26 [PATCH v3 0/2] hw/adc: Add basic Aspeed ADC model 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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.