[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] hw/misc: Add support for ADC controller in X
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] hw/misc: Add support for ADC controller in Xilinx Zynq 7000 |
|
Date: |
Sun, 13 Sep 2015 12:08:21 -0700 |
On Sat, Sep 12, 2015 at 2:08 PM, Guenter Roeck <address@hidden> wrote:
> Add support for the Xilinx XADC core used in Zynq 7000.
>
> References:
> - Zynq-7000 All Programmable SoC Technical Reference Manual
> - 7 Series FPGAs and Zynq-7000 All Programmable SoC XADC
> Dual 12-Bit 1 MSPS Analog-to-Digital Converter
>
> Tested with Linux using qemu machine xilinx-zynq-a9 with devicetree
> files zynq-zc702.dtb and zynq-zc706.dtb, and kernel configuration
> multi_v7_defconfig.
>
> Signed-off-by: Guenter Roeck <address@hidden>
>
> ---
> v2: Use extract32()
> Merge zynq_xadc_reset() and _zynq_xadc_reset() into one function
> Use "xlnx,zynq_xadc"
> Move device model to include/hw/misc/zynq_xadc.h
> irq -> qemu_irq
> xadc_dfifo_depth -> xadc_dfifo_entries
> Dropped unnecessary comments
> Merged zynq_xadc_realize() into zynq_xadc_init()
>
> hw/arm/xilinx_zynq.c | 6 +
> hw/misc/Makefile.objs | 1 +
> hw/misc/zynq_xadc.c | 270
> ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/misc/zynq_xadc.h | 49 ++++++++
> 4 files changed, 326 insertions(+)
> create mode 100644 hw/misc/zynq_xadc.c
> create mode 100644 include/hw/misc/zynq_xadc.h
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index a4e7b5c..f933f81 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -24,6 +24,7 @@
> #include "hw/block/flash.h"
> #include "sysemu/block-backend.h"
> #include "hw/loader.h"
> +#include "hw/misc/zynq_xadc.h"
> #include "hw/ssi.h"
> #include "qemu/error-report.h"
>
> @@ -225,6 +226,11 @@ static void zynq_init(MachineState *machine)
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
>
> + dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
> + qdev_init_nofail(dev);
> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[39-IRQ_OFFSET]);
> +
> dev = qdev_create(NULL, "pl330");
> qdev_prop_set_uint8(dev, "num_chnls", 8);
> qdev_prop_set_uint8(dev, "num_periph_req", 4);
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 4aa76ff..5f76f05 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -36,6 +36,7 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
> obj-$(CONFIG_OMAP) += omap_tap.o
> obj-$(CONFIG_SLAVIO) += slavio_misc.o
> obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> +obj-$(CONFIG_ZYNQ) += zynq_xadc.o
> obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> diff --git a/hw/misc/zynq_xadc.c b/hw/misc/zynq_xadc.c
> new file mode 100644
> index 0000000..dc67b73
> --- /dev/null
> +++ b/hw/misc/zynq_xadc.c
> @@ -0,0 +1,270 @@
> +/*
> + * ADC registers for Xilinx Zynq Platform
> + *
> + * Copyright (c) 2015 Guenter Roeck
> + * Based on hw/misc/zynq_slcr.c, written by Michal Simek
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/misc/zynq_xadc.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +
> +enum {
These names should match the TRM, which has them as:
> + CFG = 0x000 / 4,
_CFG (ok)
> + INTSTS,
_INT_STS (just needs _)
> + INTMSK,
_INT_MASK
> + STATUS,
_MSTS
> + CFIFO,
_CMDFIFO
> + DFIFO,
_RDFIFO
> + CTL,
_MCTL
I have dropped the XADCIF_ from the names as that is implicit but the
remainder should be recognisable to TRM.
> +};
> +
> +#define XADC_ZYNQ_CFG_ENABLE BIT(31)
> +#define XADC_ZYNQ_CFG_CFIFOTH_RD(x) (((x) >> 20) & 0x0f)
> +#define XADC_ZYNQ_CFG_DFIFOTH_RD(x) (((x) >> 16) & 0x0f)
> +#define XADC_ZYNQ_CFG_WEDGE BIT(13)
> +#define XADC_ZYNQ_CFG_REDGE BIT(12)
> +#define XADC_ZYNQ_CFG_TCKRATE_DIV2 (0x0 << 8)
> +#define XADC_ZYNQ_CFG_TCKRATE_DIV4 (0x1 << 8)
> +#define XADC_ZYNQ_CFG_TCKRATE_DIV8 (0x2 << 8)
> +#define XADC_ZYNQ_CFG_TCKRATE_DIV16 (0x3 << 8)
> +#define XADC_ZYNQ_CFG_IGAP_MASK 0x1f
> +#define XADC_ZYNQ_CFG_IGAP(x) ((x) & XADC_ZYNQ_CFG_IGAP_MASK)
> +
These defs have the same name stem but, are inconsistent and cannot be
used interchangeably. Some, are extractors (CFIFOTH_RD), some are
boolean masks (WEDGE) and some are depositors of specific values
(TCKRATE_DIV2) It means readers need to refer back to this def table
to figure out what the macro operation is for each field. It probably
needs a suffix system. _MASK, _EXTRACT, _DEPOSIT. Line wrap is a
problem, so maybe truncate to shorthands.
> +#define XADC_ZYNQ_INT_CFIFO_LTH BIT(9)
> +#define XADC_ZYNQ_INT_DFIFO_GTH BIT(8)
> +#define XADC_ZYNQ_INT_ALARM_MASK 0xff
7f? TRM has OT interrupt as bit 7.
> +#define XADC_ZYNQ_INT_ALARM_OFFSET 0
> +
> +#define XADC_ZYNQ_STATUS_DFIFO_LVL(x) (((x) & 0x0f) << 12)
> +#define XADC_ZYNQ_STATUS_CFIFOF BIT(11)
> +#define XADC_ZYNQ_STATUS_CFIFOE BIT(10)
> +#define XADC_ZYNQ_STATUS_DFIFOF BIT(9)
> +#define XADC_ZYNQ_STATUS_DFIFOE BIT(8)
> +#define XADC_ZYNQ_STATUS_OT BIT(7)
> +#define XADC_ZYNQ_STATUS_ALM(x) BIT(x)
> +
> +#define XADC_ZYNQ_CTL_RESET BIT(4)
> +
> +#define XADC_ZYNQ_CMD_NOP 0x00
> +#define XADC_ZYNQ_CMD_READ 0x01
> +#define XADC_ZYNQ_CMD_WRITE 0x02
> +
Having both ZYNQ and XADC in the name stems in useful if these ever
end up in header, but I think it should be reversed, major subsystem
(i.e. ZYNQ) first then minor (XADC) for:
ZYNQ_XADC_{REG}_{FIELD}
> +static void zynq_xadc_reset(DeviceState *d)
> +{
> + ZynqXADCState *s = ZYNQ_XADC(d);
> + int i;
> +
> + s->regs[CFG] = XADC_ZYNQ_CFG_IGAP(0x14) | XADC_ZYNQ_CFG_TCKRATE_DIV4 |
> + XADC_ZYNQ_CFG_REDGE;
Personally prefer to indent one past the = to line up with XADC_ZYNQ_CFG_IGAP.
> + s->regs[INTSTS] = XADC_ZYNQ_INT_CFIFO_LTH;
> + s->regs[INTMSK] = 0xffffffff;
> + s->regs[STATUS] = 0;
> + s->regs[CFIFO] = 0;
> + s->regs[DFIFO] = 0;
> + s->regs[CTL] = XADC_ZYNQ_CTL_RESET;
> +
> + for (i = 0; i < ARRAY_SIZE(s->xadc_regs); i++) {
> + s->xadc_regs[i] = 0;
> + }
> + for (i = 0; i < ARRAY_SIZE(s->xadc_dfifo); i++) {
> + s->xadc_dfifo[i] = 0;
> + }
memset 0.
> + s->xadc_dfifo_entries = 0;
_update_ints(). Although see my comment below about the scope of the
soft reset for more on this.
> +}
> +
> +static bool zynq_xadc_check_offset(hwaddr offset, bool rnw)
> +{
> + switch (offset) {
> + case CFG:
> + case INTMSK:
> + case INTSTS:
> + case CTL:
> + return true;
> + case DFIFO:
> + case STATUS:
> + return rnw; /* read only */
> + case CFIFO:
> + return !rnw; /* write only */
> + default:
> + return false;
> + }
> +}
> +
> +static void zynq_xadc_update_ints(ZynqXADCState *s)
> +{
> + qemu_set_irq(s->qemu_irq, !!(s->regs[INTSTS] & ~s->regs[INTMSK]));
> +}
> +
> +static uint64_t zynq_xadc_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + ZynqXADCState *s = opaque;
> + int reg = offset / 4;
> + uint64_t rv = 0;
> + int i;
> +
> + if (!zynq_xadc_check_offset(reg, true)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "zynq_xadc: Invalid read access to "
> + " addr %" HWADDR_PRIx "\n", offset);
> + }
> +
> + switch (reg) {
> + case CFG:
> + case INTMSK:
> + case INTSTS:
> + case CTL:
> + rv = s->regs[reg];
> + break;
> + case STATUS:
> + rv = XADC_ZYNQ_STATUS_CFIFOE;
> + rv |= XADC_ZYNQ_STATUS_DFIFO_LVL(s->xadc_dfifo_entries);
> + if (!s->xadc_dfifo_entries) {
> + rv |= XADC_ZYNQ_STATUS_DFIFOE;
> + } else if (s->xadc_dfifo_entries >= ARRAY_SIZE(s->xadc_dfifo)) {
> + rv |= XADC_ZYNQ_STATUS_DFIFOF;
> + }
> + break;
> + case DFIFO:
> + rv = s->xadc_dfifo[0];
> + if (s->xadc_dfifo_entries > 0) {
> + s->xadc_dfifo_entries--;
> + }
> + for (i = 0; i < s->xadc_dfifo_entries; i++) {
> + s->xadc_dfifo[i] = s->xadc_dfifo[i + 1];
> + }
> + s->xadc_dfifo[s->xadc_dfifo_entries] = 0;
> + zynq_xadc_update_ints(s);
> + break;
> + }
> + return rv;
> +}
> +
> +static void xadc_add_dfifo(ZynqXADCState *s, uint16_t regval)
> +{
> + if (s->xadc_dfifo_entries < ARRAY_SIZE(s->xadc_dfifo)) {
> + s->xadc_dfifo[s->xadc_dfifo_entries++] =
> s->xadc_read_reg_previous;
> + }
> + s->xadc_read_reg_previous = regval;
> + if (s->xadc_dfifo_entries > XADC_ZYNQ_CFG_DFIFOTH_RD(s->regs[CFG])) {
> + s->regs[INTSTS] |= XADC_ZYNQ_INT_DFIFO_GTH;
> + }
> + zynq_xadc_update_ints(s);
> +}
> +
> +static void zynq_xadc_write(void *opaque, hwaddr offset, uint64_t val,
> + unsigned size)
> +{
> + ZynqXADCState *s = (ZynqXADCState *)opaque;
> + int reg = offset / 4;
> + int xadc_reg;
> + int xadc_cmd;
> + int xadc_data;
> +
> + if (!zynq_xadc_check_offset(reg, false)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "zynq_xadc: Invalid write access to "
> + "addr %" HWADDR_PRIx "\n", offset);
> + return;
> + }
> +
> + switch (reg) {
> + case CFG:
> + s->regs[CFG] = val;
> + break;
> + case INTSTS:
> + s->regs[INTSTS] &= ~val;
> + zynq_xadc_update_ints(s);
> + break;
> + case INTMSK:
> + s->regs[INTMSK] = val & 0x003ff;
> + zynq_xadc_update_ints(s);
> + break;
> + case CFIFO:
> + xadc_cmd = extract32(val, 26, 4);
> + xadc_reg = extract32(val, 16, 7);
> + xadc_data = extract32(val, 0, 16);
> +
> + switch (xadc_cmd) {
> + case XADC_ZYNQ_CMD_READ:
> + xadc_add_dfifo(s, s->xadc_regs[xadc_reg]);
> + break;
> + case XADC_ZYNQ_CMD_WRITE:
> + s->xadc_regs[xadc_reg] = xadc_data;
> + break;
> + case XADC_ZYNQ_CMD_NOP:
> + xadc_add_dfifo(s, 0);
> + break;
> + }
> + s->regs[INTSTS] |= XADC_ZYNQ_INT_CFIFO_LTH;
Worth a comment that ops are instantaneous which is why the FIFO is
always empty.
You are however implementing this as a level crossing, that is, when
the FIFO level transitions from 1 to 0 you set the interrupt. Given
that the reset value is 1 that's probably not how it works. Instead
with your instantaneous commands assumption, the correct logic is to
just force CFIFO_LTH to always be 1 both on reset and after a WTC of
this bit. Otherwise this bit is falsely 0 after being cleared but the
FIFO is still empty. The probably applies to DFIFO_GTH. One way to
solve this, is to set these two in _update_ints. That is, force
CFIFO_LTH to its 1 and do the >= for DFIFO_GTH. That forces the
interrupt back up after a clear but when the underlying triggering
condition is still true.
> + zynq_xadc_update_ints(s);
> + break;
> + case CTL:
> + if (val & XADC_ZYNQ_CTL_RESET) {
> + zynq_xadc_reset(DEVICE(s));
> + }
>From the TRM:
"
This bit will reset the communication channel
between the PS and XADC.
If set, the PS-XADC communication channel will
remain in reset until a 0 is written to this bit.
"
Which suggests that this is a level sensitive reset. This logic is
only implementing a single reset event on control register write. It
is not clear to me whether this is full IP soft-reset and affects the
register values (which zynq_xadc_reset does). But whatever the
side-effects of this reset are need to be perisistent, i.e. you
currently reset all control and xadc registers and purge the fifos, so
as long as this bit is set, the core must hold those registers to this
value until this is cleared.
Im wondering if the direct registers (_CFG .. _CTL) are not reset by
this bit at all, and only the indirect regs (fifo, xadc_regs) are what
is reset here? The direct registers are probably under the control of
the devcfg and it's reset infrastructure.
So this reset may need to do less, but also need guards on register
operations to nop while this bit remains set.
> + s->regs[CTL] = val & 0x00fffeff;
What is up with bit 8?
The TRM has everything other than bit 4 as reserved so if might be
cleaner just to RAZWI them all (despite TRM having different types of
reserved for different fields). I can't see anything special for bit
8.
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps xadc_ops = {
> + .read = zynq_xadc_read,
> + .write = zynq_xadc_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void zynq_xadc_init(Object *obj)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> + ZynqXADCState *s = ZYNQ_XADC(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &xadc_ops, s, "xadc",
> + ZYNQ_XADC_MMIO_SIZE);
> + sysbus_init_mmio(sbd, &s->iomem);
> + sysbus_init_irq(sbd, &s->qemu_irq);
> +}
> +
> +static const VMStateDescription vmstate_zynq_xadc = {
> + .name = "zynq_xadc",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32_ARRAY(regs, ZynqXADCState, ZYNQ_XADC_NUM_IO_REGS),
> + VMSTATE_UINT16_ARRAY(xadc_regs, ZynqXADCState,
> ZYNQ_XADC_NUM_ADC_REGS),
> + VMSTATE_UINT16_ARRAY(xadc_dfifo, ZynqXADCState,
> ZYNQ_XADC_FIFO_DEPTH),
> + VMSTATE_UINT16(xadc_read_reg_previous, ZynqXADCState),
> + VMSTATE_UINT16(xadc_dfifo_entries, ZynqXADCState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void zynq_xadc_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &vmstate_zynq_xadc;
> + dc->reset = zynq_xadc_reset;
> +}
> +
> +static const TypeInfo zynq_xadc_info = {
> + .class_init = zynq_xadc_class_init,
> + .name = TYPE_ZYNQ_XADC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(ZynqXADCState),
> + .instance_init = zynq_xadc_init,
> +};
> +
> +static void zynq_xadc_register_types(void)
> +{
> + type_register_static(&zynq_xadc_info);
> +}
> +
> +type_init(zynq_xadc_register_types)
> diff --git a/include/hw/misc/zynq_xadc.h b/include/hw/misc/zynq_xadc.h
> new file mode 100644
> index 0000000..495be06
> --- /dev/null
> +++ b/include/hw/misc/zynq_xadc.h
> @@ -0,0 +1,49 @@
> +/*---------------------------------------------------------------------------
Generally don't have the "----------------" in file header comments.
Regards,
Peter
> + *
> + * Device model for Zynq ADC controller
> + *
> + * Copyright (c) 2015 Guenter Roeck <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + *---------------------------------------------------------------------------
> + */
> +
> +#ifndef ZYNQ_XADC_H
> +#define ZYNQ_XADC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define ZYNQ_XADC_MMIO_SIZE 0x0020
> +#define ZYNQ_XADC_NUM_IO_REGS (ZYNQ_XADC_MMIO_SIZE / 4)
> +#define ZYNQ_XADC_NUM_ADC_REGS 128
> +#define ZYNQ_XADC_FIFO_DEPTH 15
> +
> +#define TYPE_ZYNQ_XADC "xlnx,zynq_xadc"
> +#define ZYNQ_XADC(obj) \
> + OBJECT_CHECK(ZynqXADCState, (obj), TYPE_ZYNQ_XADC)
> +
> +typedef struct ZynqXADCState {
> + /*< private >*/
> + SysBusDevice parent_obj;
> +
> + /*< public >*/
> + MemoryRegion iomem;
> +
> + uint32_t regs[ZYNQ_XADC_NUM_IO_REGS];
> + uint16_t xadc_regs[ZYNQ_XADC_NUM_ADC_REGS];
> + uint16_t xadc_read_reg_previous;
> + uint16_t xadc_dfifo[ZYNQ_XADC_FIFO_DEPTH];
> + uint16_t xadc_dfifo_entries;
> +
> + struct IRQState *qemu_irq;
> +
> +} ZynqXADCState;
> +
> +#endif /* ZYNQ_XADC_H */
> --
> 2.1.4
>