[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v3 0/2] hw/adc: Add basic Aspeed ADC model
From: |
pdel |
Subject: |
[PATCH v3 0/2] hw/adc: Add basic Aspeed ADC model |
Date: |
Mon, 4 Oct 2021 22:26:02 -0700 |
From: Peter Delevoryas <pdel@fb.com>
v1: 20211002214414.2858382-1-pdel@fbc.om/">https://lore.kernel.org/qemu-devel/20211002214414.2858382-1-pdel@fbc.om/
v2: 20211003191850.513658-1-pdel@fb.com/">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
- [PATCH v3 0/2] hw/adc: Add basic Aspeed ADC model,
pdel <=