[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 1/3] i.MX: Add GPIO device
|
From: |
Jean-Christophe DUBOIS |
|
Subject: |
Re: [Qemu-devel] [PATCH v1 1/3] i.MX: Add GPIO device |
|
Date: |
Sun, 6 Sep 2015 14:55:31 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
<address@hidden> wrote:
Signed-off-by: Jean-Christophe Dubois <address@hidden>
---
hw/gpio/Makefile.objs | 1 +
hw/gpio/imx_gpio.c | 358 +++++++++++++++++++++++++++++++++++++++++++++
include/hw/gpio/imx_gpio.h | 60 ++++++++
3 files changed, 419 insertions(+)
create mode 100644 hw/gpio/imx_gpio.c
create mode 100644 include/hw/gpio/imx_gpio.h
diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
index 1abcf17..52233f7 100644
--- a/hw/gpio/Makefile.objs
+++ b/hw/gpio/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-$(CONFIG_ZAURUS) += zaurus.o
common-obj-$(CONFIG_E500) += mpc8xxx.o
obj-$(CONFIG_OMAP) += omap_gpio.o
+obj-$(CONFIG_IMX) += imx_gpio.o
diff --git a/hw/gpio/imx_gpio.c b/hw/gpio/imx_gpio.c
new file mode 100644
index 0000000..8ec1d4c
--- /dev/null
+++ b/hw/gpio/imx_gpio.c
@@ -0,0 +1,358 @@
+/*
+ * i.MX processors GPIO emulation.
+ *
+ * Copyright (C) 2015 Jean-Christophe Dubois <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 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * 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/gpio/imx_gpio.h"
+
+#ifndef IMX_GPIO_DEBUG
+#define IMX_GPIO_DEBUG 0
+#endif
+
+#if IMX_GPIO_DEBUG
+# define DPRINTF(fmt, args...) \
+ do { fprintf(stder, "%s: " fmt , __func__, ##args); } while (0)
+
Use a regular if for debug conditional. This is so the debug code is
always compiled.
Not sure what you mean here ... Most files in qemu have DEBUG functions
compiled out in the normal case ...
Is there a change in "politic" on this topic?
Could you point to an implementation doing things right?
+static char const *imx_gpio_reg_name(uint32_t reg)
+{
+ switch (reg) {
+ case DR_ADDR:
+ return "DR";
+ case GDIR_ADDR:
+ return "GDIR";
+ case PSR_ADDR:
+ return "PSR";
+ case ICR1_ADDR:
+ return "ICR1";
+ case ICR2_ADDR:
+ return "ICR2";
+ case IMR_ADDR:
+ return "IMR";
+ case ISR_ADDR:
+ return "ISR";
+ case EDGE_SEL_ADDR:
+ /* This register is not present in i.MX31 */
+ return "EDGE_SEL";
+ default:
+ return "[?]";
+ }
But I'm guessing this will be an issue for the non debug case. Perhaps
return "" from here in non-debug or just leave this always compiled
(optimisiser should be smart enough to trim it).
+}
+#else
+# define DPRINTF(fmt, args...) do {} while (0)
+#endif
+
+static void imx_gpio_update_int(IMXGPIOState *s)
+{
+ if (s->isr) {
+ qemu_irq_raise(s->irq);
+ } else {
+ qemu_irq_lower(s->irq);
+ }
qemu_set_irq.
OK
+}
+
+static void imx_gpio_set_int_line(IMXGPIOState *s, int line, int level)
+{
+ /* is this signal configured as an interrupt source */
+ if (extract32(s->imr, line, 1)) {
Short return on this condition (negated) rather than indent entire logic.
OK
+ /* When set EDGE_SEL override the ICR config */
+ if (extract32(s->edge_sel, line, 1)) {
+ /* we detect interrupt on rising and falling edge */
+ if (extract32(s->psr, line, 1) != level) {
This is dangerous, as level is boolean and this will promote to
integer for comparison. I think last time people talked about this on
list, we decided that all users or GPIO setters should use 0 vs 1 but
it may not be the in-tree case. !!level will resolve.
OK, I'll work out something with an enum to be explicit.
+ /* level changed */
+ s->isr = deposit32(s->isr, line, 1, 1);
+ }
+ } else if (extract64(s->icr, 2*line + 1, 1)) {
+ /* interrupt is edge sensitive */
+ if (extract32(s->psr, line, 1) != level) {
+ /* level changed */
+ int falling = extract64(s->icr, 2*line, 1);
+
+ if ((falling && !level) || (!falling && level)) {
falling == !level
OK
+ s->isr = deposit32(s->isr, line, 1, 1);
+ }
+ }
+ } else {
+ /* interrupt is level sensitive */
+ int high = extract64(s->icr, 2*line, 1);
+
+ if ((high && level) || (!high && !level)) {
==
OK
+ s->isr = deposit32(s->isr, line, 1, 1);
+ }
+ }
+ }
+}
+
+static void imx_gpio_set(void *opaque, int line, int level)
+{
+ IMXGPIOState *s = IMX_GPIO(opaque);
+
+ /* if the line is configured as output nothing to do */
+ if (extract32(s->gdir, line, 1)) {
+ /* actually we should not be called */
+ qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: requesting to set line %d"
+ " to %d but this is an output line\n",
+ TYPE_IMX_GPIO, __func__, line, level);
Short return.
OK
+ } else {
+ imx_gpio_set_int_line(s, line, level);
+
+ /* this is an input signal, so set PSR */
+ s->psr = deposit32(s->psr, line, 1, level);
+
+ imx_gpio_update_int(s);
+ }
+}
+
+static void imx_gpio_set_all_int_lines(IMXGPIOState *s)
+{
+ int i;
+
+ for (i = 0; i < 32; i++) {
32 should be macroified.
OK
+ imx_gpio_set_int_line(s, i, extract32(s->psr, i, 1));
+ }
+
+ imx_gpio_update_int(s);
+}
+
+static uint64_t imx_gpio_read(void *opaque, hwaddr offset, unsigned size)
+{
+ IMXGPIOState *s = IMX_GPIO(opaque);
+ uint32_t reg_value = 0;
+ int i;
+
+ switch (offset) {
+ case DR_ADDR: /* DATA REGISTER */
+ for (i = 0; i < 32; i++) {
+ uint32_t ref_value;
+
+ /*
+ * depending on the "line" configuration, the bit values
+ * are comming either from DR ou PSR
"coming" "or"
OK
+ */
+ if (extract32(s->gdir, i, 1)) {
+ ref_value = s->dr;
+ } else {
+ ref_value = s->psr;
+ }
+
+ reg_value = deposit32(reg_value, i, 1,
+ extract32(ref_value, i, 1));
This can be one-shot with some bitwise logicals to avoid the loop.
reg_value = s->dr & s->gdir | s->psr & ~s->gdir
I think.
OK
+ }
+ break;
+
+ case GDIR_ADDR: /* DIRECTION REGISTER */
These trailing comments are duped between read and write handlers. I
would move to header as a more global single reference.
I was thinking it was more helpful here to help understand the code
without having to refer to a header file.
The point is that the abreviated spec name is not always very helpful.
But, OK, I'll move them to the header file ...
+ reg_value = s->gdir;
+ break;
+
+ case PSR_ADDR: /* PAD STATUS REGISTER */
+ reg_value = s->psr;
+ break;
+
+ case ICR1_ADDR: /* INTERRUPT CONFIGURATION REGISTER 1 */
+ reg_value = (uint32_t) (s->icr & 0xffffffff);
+ break;
+
+ case ICR2_ADDR: /* INTERRUPT CONFIGURATION REGISTER 2 */
+ reg_value = (uint32_t) (s->icr >> 32);
+ break;
+
+ case IMR_ADDR: /* INTERRUPT MASK REGISTER */
+ reg_value = s->imr;
+ break;
+
+ case ISR_ADDR: /* INTERRUPT STATUS REGISTER */
+ reg_value = s->isr;
+ break;
+
+ case EDGE_SEL_ADDR: /* EDGE SEL REGISTER */
+ /* This register is not present in i.MX31 */
+ reg_value = s->edge_sel;
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: Bad register at offset %d\n",
+ TYPE_IMX_GPIO, __func__, (int)offset);
+ break;
+ }
+
+ DPRINTF("(%s) = 0x%08x\n", imx_gpio_reg_name(offset), reg_value);
PRIx32.
OK
+
+ return reg_value;
+}
+
+static inline void imx_gpio_set_output(IMXGPIOState *s)
set_all_outputs to be consistent with set_all_int_lines. Group the two
functions and any similars together (either here or above with
int_lines).
OK
+{
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ /*
+ * if the line is set as output, then forward the line
+ * level to its user.
+ */
+ if (extract32(s->gdir, i, 1) && s->handler[i]) {
+ qemu_set_irq(s->handler[i], extract32(s->dr, i, 1));
+ }
+ }
+}
+
+static void imx_gpio_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ IMXGPIOState *s = IMX_GPIO(opaque);
+
+ DPRINTF("(%s, value = 0x%08x)\n", imx_gpio_reg_name(offset),
+ (uint32_t)value);
PRIx32.
OK
+
+ switch (offset) {
+ case DR_ADDR: /* DATA REGISTER */
+ s->dr = (uint32_t)value;
cast un-needed.
OK
+
+ imx_gpio_set_output(s);
+
+ break;
+
+ case GDIR_ADDR: /* DIRECTION REGISTER */
+ s->gdir = (uint32_t)value;
cast un-needed (theres a few more below).
OK
+
+ imx_gpio_set_output(s);
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
+ case ICR1_ADDR: /* INTERRUPT CONFIGURATION REGISTER 1 */
+ s->icr = (s->icr | 0xffffffff) & (uint32_t)value;
deposit64.
OK
+
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
+ case ICR2_ADDR: /* INTERRUPT CONFIGURATION REGISTER 2 */
+ s->icr = (s->icr | 0xffffffff00000000) & (value << 32);
deposit64.
OK
+
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
+ case IMR_ADDR: /* INTERRUPT MASK REGISTER */
+ s->imr = (uint32_t)value;
+
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
+ case ISR_ADDR: /* INTERRUPT STATUS REGISTER */
+ s->isr |= ~(uint32_t)value;
+
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
+ case EDGE_SEL_ADDR: /* EDGE SEL REGISTER */
+ /* This register is not present in i.MX31 */
+ s->edge_sel = (uint32_t)value;
+
+ imx_gpio_set_all_int_lines(s);
+
+ break;
+
I don't think you need the extra white. For short side effects it is
usual to just format as:
case ADDR:
register = value;
side_effects();
break;
OK
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: Bad register at offset %d\n",
+ TYPE_IMX_GPIO, __func__, (int)offset);
+ break;
+ }
+
+ return;
+}
+
+static void imx_gpio_reset(DeviceState *dev)
+{
+ IMXGPIOState *s = IMX_GPIO(dev);
+
+ s->dr = 0;
+ s->gdir = 0;
+ s->psr = 0;
+ s->icr = 0;
+ s->imr = 0;
+ s->isr = 0;
+ s->edge_sel = 0;
+
+ imx_gpio_set_output(s);
+ imx_gpio_update_int(s);
+}
+
+static const MemoryRegionOps imx_gpio_ops = {
+ .read = imx_gpio_read,
+ .write = imx_gpio_write,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription vmstate_imx_gpio = {
+ .name = TYPE_IMX_GPIO,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(dr, IMXGPIOState),
+ VMSTATE_UINT32(gdir, IMXGPIOState),
+ VMSTATE_UINT32(psr, IMXGPIOState),
+ VMSTATE_UINT64(icr, IMXGPIOState),
+ VMSTATE_UINT32(imr, IMXGPIOState),
+ VMSTATE_UINT32(isr, IMXGPIOState),
+ /* This register is not present in i.MX31 */
+ VMSTATE_UINT32(edge_sel, IMXGPIOState),
You can make a boolean property of the device and turn its presence on
and off to get the functionality.
OK
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void imx_gpio_realize(DeviceState *dev, Error **errp)
+{
+ IMXGPIOState *s = IMX_GPIO(dev);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &imx_gpio_ops, s,
+ TYPE_IMX_GPIO, IMX_GPIO_MEM_SIZE);
+
+ qdev_init_gpio_in(DEVICE(s), imx_gpio_set, 32);
+ qdev_init_gpio_out(DEVICE(s), s->handler, 32);
+
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
+}
+
+static void imx_gpio_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = imx_gpio_realize;
+ dc->reset = imx_gpio_reset;
+ dc->vmsd = &vmstate_imx_gpio;
+ dc->desc = "i.MX GPIO controller";
dc->props = ...
for the "has-edge-sel" property.
OK
+}
+
+static const TypeInfo imx_gpio_info = {
+ .name = TYPE_IMX_GPIO,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(IMXGPIOState),
+ .class_init = imx_gpio_class_init,
+};
+
+static void imx_gpio_register_types(void)
+{
+ type_register_static(&imx_gpio_info);
+}
+
+type_init(imx_gpio_register_types)
diff --git a/include/hw/gpio/imx_gpio.h b/include/hw/gpio/imx_gpio.h
new file mode 100644
index 0000000..6d3f149
--- /dev/null
+++ b/include/hw/gpio/imx_gpio.h
@@ -0,0 +1,60 @@
+/*
+ * i.MX processors GPIO registers definition.
+ *
+ * Copyright (C) 2015 Jean-Christophe Dubois <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 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * 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 __IMX_GPIO_H_
+#define __IMX_GPIO_H_
+
+#include <hw/sysbus.h>
+
+#define TYPE_IMX_GPIO "imx.gpio"
+#define IMX_GPIO(obj) OBJECT_CHECK(IMXGPIOState, (obj), TYPE_IMX_GPIO)
+
+#define IMX_GPIO_MEM_SIZE 0x20
+
+/* i.MX GPIO memory map */
+#define DR_ADDR 0x00
+#define GDIR_ADDR 0x04
+#define PSR_ADDR 0x08
+#define ICR1_ADDR 0x0c
+#define ICR2_ADDR 0x10
+#define IMR_ADDR 0x14
+#define ISR_ADDR 0x18
+#define EDGE_SEL_ADDR 0x1c
+
+typedef struct IMXGPIOState {
+ /*< private >*/
+ SysBusDevice parent_obj;
+
+ /*< public >*/
+ MemoryRegion iomem;
+
+ uint32_t dr;
+ uint32_t gdir;
+ uint32_t psr;
+ uint64_t icr;
+ uint32_t imr;
+ uint32_t isr;
+ /* This register is not present in i.MX31 */
+ uint32_t edge_sel;
+
+ qemu_irq irq;
+ qemu_irq handler[32];
Should this just be "outputs"? Im guessing this is a bidirectional pin
which QEMU has trouble modelleing and this is the output mode variant
of that (hence it probably has no name in the TRM to use).
OK
Regards,
Peter
+} IMXGPIOState;
+
+#endif /* __IMX_GPIO_H_ */
--
2.1.4
[Qemu-devel] [PATCH v1 2/3] i.MX: Add GPIO device to i.MX31 SOC, Jean-Christophe Dubois, 2015/09/05
[Qemu-devel] [PATCH v1 3/3] i.MX: Add GPIO device to i.MX25 SOC, Jean-Christophe Dubois, 2015/09/05