Re: [Qemu-devel] [PATCH 07/13] mxs/imx23: Implements the pin mux, GPIOs

2014-01-08 Thread M P 
All noted, and thanks for all the bits you reviewed so far, I'll do the
changes and resubmit.

M



On 6 January 2014 15:52, Peter Maydell peter.mayd...@linaro.org wrote:

 On 11 December 2013 13:56, Michel Pollet buser...@gmail.com wrote:
  Implements the pinctrl and GPIO block for the imx23
  It handles GPIO output, and GPIO input from qemu translated
  into pin values and interrupts, if appropriate.
 
  Signed-off-by: Michel Pollet buser...@gmail.com
  ---
   hw/arm/Makefile.objs   |   2 +-
   hw/arm/imx23_pinctrl.c | 293
 +
   2 files changed, 294 insertions(+), 1 deletion(-)
   create mode 100644 hw/arm/imx23_pinctrl.c
 
  diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
  index 9adcb96..ea53988 100644
  --- a/hw/arm/Makefile.objs
  +++ b/hw/arm/Makefile.objs
  @@ -5,4 +5,4 @@ obj-y += tosa.o versatilepb.o vexpress.o virt.o
 xilinx_zynq.o z2.o
 
   obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
   obj-y += omap1.o omap2.o strongarm.o
  -obj-$(CONFIG_MXS) += imx23_digctl.o
  +obj-$(CONFIG_MXS) += imx23_digctl.o imx23_pinctrl.o
  diff --git a/hw/arm/imx23_pinctrl.c b/hw/arm/imx23_pinctrl.c
  new file mode 100644
  index 000..ecfb755
  --- /dev/null
  +++ b/hw/arm/imx23_pinctrl.c
  @@ -0,0 +1,293 @@
  +/*
  + * imx23_pinctrl.c
  + *
  + * Copyright: Michel Pollet buser...@gmail.com
  + *
  + * QEMU Licence
  + */
  +
  +/*
  + * Implements the pinctrl and GPIO block for the imx23
  + * It handles GPIO output, and GPIO input from qemu translated
  + * into pin values and interrupts, if appropriate.
  + */
  +#include hw/sysbus.h
  +#include hw/arm/mxs.h
  +
  +#define D(w)
  +
  +enum {
  +PINCTRL_BANK_COUNT = 3,
  +
  +PINCTRL_CTRL = 0,
  +PINCTRL_BANK_MUXSEL = 0x10,
  +PINCTRL_BANK_BASE = 0x40,
  +
  +/* these are not  4 register numbers, these are  8 register
 numbers */
  +PINCTRL_BANK_PULL = 0x4,
  +PINCTRL_BANK_OUT = 0x5,
  +PINCTRL_BANK_DIN = 0x6,
  +PINCTRL_BANK_DOE = 0x7,
  +PINCTRL_BANK_PIN2IRQ = 0x8,
  +PINCTRL_BANK_IRQEN = 0x9,
  +PINCTRL_BANK_IRQLEVEL = 0xa,
  +PINCTRL_BANK_IRQPOL = 0xb,
  +PINCTRL_BANK_IRQSTAT = 0xc,
  +
  +PINCTRL_BANK_INTERNAL_STATE = 0xd,
  +PINCTRL_MAX = 0xe0,
  +};
  +
  +#define PINCTRL_BANK_REG(_bank, _reg) ((_reg  8) | (_bank  4))
  +
  +enum {
  +MUX_GPIO = 0x3,
  +};
  +
  +
  +typedef struct imx23_pinctrl_state {
  +SysBusDevice busdev;
  +MemoryRegion iomem;
  +
  +uint32_t r[PINCTRL_MAX];
  +qemu_irq irq_in[3];
  +qemu_irq irq_out[PINCTRL_BANK_COUNT * 32];
  +
  +uint32_t state[PINCTRL_BANK_COUNT];
  +} imx23_pinctrl_state;
  +
  +static uint64_t imx23_pinctrl_read(
  +void *opaque, hwaddr offset, unsigned size)
  +{
  +imx23_pinctrl_state *s = (imx23_pinctrl_state *) opaque;
  +uint32_t res = 0;
  +
  +switch (offset  4) {
  +case 0 ... PINCTRL_MAX:
  +res = s-r[offset  4];
  +break;
  +default:
  +qemu_log_mask(LOG_GUEST_ERROR,
  +%s: bad offset 0x%x\n, __func__, (int) offset);
  +break;
  +}
  +
  +return res;
  +}
  +
  +static uint8_t imx23_pinctrl_getmux(
  +imx23_pinctrl_state *s, int pin)
  +{
  +int base = pin / 16, offset = pin % 16;
  +return (s-r[PINCTRL_BANK_MUXSEL + base]  (offset * 2))  0x3;
  +}
  +
  +/*
  + * usage imx23_pinctrl_getbit(s, PINCTRL_BANK_IRQEN, 48)...
  + */
  +static uint8_t imx23_pinctrl_getbit(
  +imx23_pinctrl_state *s, uint16_t reg, int pin)
  +{
  +int bank = pin / 32, offset = pin % 32;
  +uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
  +//printf(%s bank %d offset %d reg %d : %04x=%08x\n, __func__,
 bank, offset, reg,
  +//PINCTRL_BANK_REG(bank, reg),
  +//*latch);
  +return (*latch  offset)  0x1;
  +}
  +
  +static void imx23_pinctrl_setbit(
  +imx23_pinctrl_state *s, uint16_t reg, int pin, int value)
  +{
  +int bank = pin / 32, offset = pin % 32;
  +uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
  +*latch = (*latch  ~(1  offset)) | (!!value  offset);

 deposit32() will make this clearer to read.

  +}
  +
  +static void imx23_pinctrl_write_bank(
  +imx23_pinctrl_state *s, int bank,
  +int reg, uint32_t value,
  +uint32_t mask)
  +{
  +int set, pin;
  +switch (reg) {
  +/*
  + * Linux has a way of using the DOEPULL register to toggle the
 pin
  + */

 Why is this comment here? We should ideally not care what
 guest OS we run, we should just implement the h/w correctly.

  +case PINCTRL_BANK_PULL:
  +case PINCTRL_BANK_DOE:
  +/*
  + * Writing to the Data OUT register just triggers the
  + * output qemu IRQ for any further peripherals
  + */
  +case PINCTRL_BANK_OUT: {
  +while ((set = ffs(mask))  0) {
  +

Re: [Qemu-devel] [PATCH 07/13] mxs/imx23: Implements the pin mux, GPIOs

2014-01-06 Thread Peter Maydell 
On 11 December 2013 13:56, Michel Pollet buser...@gmail.com wrote:
 Implements the pinctrl and GPIO block for the imx23
 It handles GPIO output, and GPIO input from qemu translated
 into pin values and interrupts, if appropriate.

 Signed-off-by: Michel Pollet buser...@gmail.com
 ---
  hw/arm/Makefile.objs   |   2 +-
  hw/arm/imx23_pinctrl.c | 293 
 +
  2 files changed, 294 insertions(+), 1 deletion(-)
  create mode 100644 hw/arm/imx23_pinctrl.c

 diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
 index 9adcb96..ea53988 100644
 --- a/hw/arm/Makefile.objs
 +++ b/hw/arm/Makefile.objs
 @@ -5,4 +5,4 @@ obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o 
 z2.o

  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
  obj-y += omap1.o omap2.o strongarm.o
 -obj-$(CONFIG_MXS) += imx23_digctl.o
 +obj-$(CONFIG_MXS) += imx23_digctl.o imx23_pinctrl.o
 diff --git a/hw/arm/imx23_pinctrl.c b/hw/arm/imx23_pinctrl.c
 new file mode 100644
 index 000..ecfb755
 --- /dev/null
 +++ b/hw/arm/imx23_pinctrl.c
 @@ -0,0 +1,293 @@
 +/*
 + * imx23_pinctrl.c
 + *
 + * Copyright: Michel Pollet buser...@gmail.com
 + *
 + * QEMU Licence
 + */
 +
 +/*
 + * Implements the pinctrl and GPIO block for the imx23
 + * It handles GPIO output, and GPIO input from qemu translated
 + * into pin values and interrupts, if appropriate.
 + */
 +#include hw/sysbus.h
 +#include hw/arm/mxs.h
 +
 +#define D(w)
 +
 +enum {
 +PINCTRL_BANK_COUNT = 3,
 +
 +PINCTRL_CTRL = 0,
 +PINCTRL_BANK_MUXSEL = 0x10,
 +PINCTRL_BANK_BASE = 0x40,
 +
 +/* these are not  4 register numbers, these are  8 register numbers 
 */
 +PINCTRL_BANK_PULL = 0x4,
 +PINCTRL_BANK_OUT = 0x5,
 +PINCTRL_BANK_DIN = 0x6,
 +PINCTRL_BANK_DOE = 0x7,
 +PINCTRL_BANK_PIN2IRQ = 0x8,
 +PINCTRL_BANK_IRQEN = 0x9,
 +PINCTRL_BANK_IRQLEVEL = 0xa,
 +PINCTRL_BANK_IRQPOL = 0xb,
 +PINCTRL_BANK_IRQSTAT = 0xc,
 +
 +PINCTRL_BANK_INTERNAL_STATE = 0xd,
 +PINCTRL_MAX = 0xe0,
 +};
 +
 +#define PINCTRL_BANK_REG(_bank, _reg) ((_reg  8) | (_bank  4))
 +
 +enum {
 +MUX_GPIO = 0x3,
 +};
 +
 +
 +typedef struct imx23_pinctrl_state {
 +SysBusDevice busdev;
 +MemoryRegion iomem;
 +
 +uint32_t r[PINCTRL_MAX];
 +qemu_irq irq_in[3];
 +qemu_irq irq_out[PINCTRL_BANK_COUNT * 32];
 +
 +uint32_t state[PINCTRL_BANK_COUNT];
 +} imx23_pinctrl_state;
 +
 +static uint64_t imx23_pinctrl_read(
 +void *opaque, hwaddr offset, unsigned size)
 +{
 +imx23_pinctrl_state *s = (imx23_pinctrl_state *) opaque;
 +uint32_t res = 0;
 +
 +switch (offset  4) {
 +case 0 ... PINCTRL_MAX:
 +res = s-r[offset  4];
 +break;
 +default:
 +qemu_log_mask(LOG_GUEST_ERROR,
 +%s: bad offset 0x%x\n, __func__, (int) offset);
 +break;
 +}
 +
 +return res;
 +}
 +
 +static uint8_t imx23_pinctrl_getmux(
 +imx23_pinctrl_state *s, int pin)
 +{
 +int base = pin / 16, offset = pin % 16;
 +return (s-r[PINCTRL_BANK_MUXSEL + base]  (offset * 2))  0x3;
 +}
 +
 +/*
 + * usage imx23_pinctrl_getbit(s, PINCTRL_BANK_IRQEN, 48)...
 + */
 +static uint8_t imx23_pinctrl_getbit(
 +imx23_pinctrl_state *s, uint16_t reg, int pin)
 +{
 +int bank = pin / 32, offset = pin % 32;
 +uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
 +//printf(%s bank %d offset %d reg %d : %04x=%08x\n, __func__, bank, 
 offset, reg,
 +//PINCTRL_BANK_REG(bank, reg),
 +//*latch);
 +return (*latch  offset)  0x1;
 +}
 +
 +static void imx23_pinctrl_setbit(
 +imx23_pinctrl_state *s, uint16_t reg, int pin, int value)
 +{
 +int bank = pin / 32, offset = pin % 32;
 +uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
 +*latch = (*latch  ~(1  offset)) | (!!value  offset);

deposit32() will make this clearer to read.

 +}
 +
 +static void imx23_pinctrl_write_bank(
 +imx23_pinctrl_state *s, int bank,
 +int reg, uint32_t value,
 +uint32_t mask)
 +{
 +int set, pin;
 +switch (reg) {
 +/*
 + * Linux has a way of using the DOEPULL register to toggle the pin
 + */

Why is this comment here? We should ideally not care what
guest OS we run, we should just implement the h/w correctly.

 +case PINCTRL_BANK_PULL:
 +case PINCTRL_BANK_DOE:
 +/*
 + * Writing to the Data OUT register just triggers the
 + * output qemu IRQ for any further peripherals
 + */
 +case PINCTRL_BANK_OUT: {
 +while ((set = ffs(mask))  0) {
 +set--;
 +mask = ~(1  set);
 +pin = (bank * 32) + set;
 +/*
 + * For a reason that is not clear, the pullup bit appears
 + * inverted (!) ignoring for now, assume hardware pullup
 + */

You should figure out what's actually

[Qemu-devel] [PATCH 07/13] mxs/imx23: Implements the pin mux, GPIOs

2013-12-11 Thread Michel Pollet 
Implements the pinctrl and GPIO block for the imx23
It handles GPIO output, and GPIO input from qemu translated
into pin values and interrupts, if appropriate.

Signed-off-by: Michel Pollet buser...@gmail.com
---
 hw/arm/Makefile.objs   |   2 +-
 hw/arm/imx23_pinctrl.c | 293 +
 2 files changed, 294 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/imx23_pinctrl.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 9adcb96..ea53988 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -5,4 +5,4 @@ obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o 
z2.o
 
 obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
 obj-y += omap1.o omap2.o strongarm.o
-obj-$(CONFIG_MXS) += imx23_digctl.o
+obj-$(CONFIG_MXS) += imx23_digctl.o imx23_pinctrl.o
diff --git a/hw/arm/imx23_pinctrl.c b/hw/arm/imx23_pinctrl.c
new file mode 100644
index 000..ecfb755
--- /dev/null
+++ b/hw/arm/imx23_pinctrl.c
@@ -0,0 +1,293 @@
+/*
+ * imx23_pinctrl.c
+ *
+ * Copyright: Michel Pollet buser...@gmail.com
+ *
+ * QEMU Licence
+ */
+
+/*
+ * Implements the pinctrl and GPIO block for the imx23
+ * It handles GPIO output, and GPIO input from qemu translated
+ * into pin values and interrupts, if appropriate.
+ */
+#include hw/sysbus.h
+#include hw/arm/mxs.h
+
+#define D(w)
+
+enum {
+PINCTRL_BANK_COUNT = 3,
+
+PINCTRL_CTRL = 0,
+PINCTRL_BANK_MUXSEL = 0x10,
+PINCTRL_BANK_BASE = 0x40,
+
+/* these are not  4 register numbers, these are  8 register numbers */
+PINCTRL_BANK_PULL = 0x4,
+PINCTRL_BANK_OUT = 0x5,
+PINCTRL_BANK_DIN = 0x6,
+PINCTRL_BANK_DOE = 0x7,
+PINCTRL_BANK_PIN2IRQ = 0x8,
+PINCTRL_BANK_IRQEN = 0x9,
+PINCTRL_BANK_IRQLEVEL = 0xa,
+PINCTRL_BANK_IRQPOL = 0xb,
+PINCTRL_BANK_IRQSTAT = 0xc,
+
+PINCTRL_BANK_INTERNAL_STATE = 0xd,
+PINCTRL_MAX = 0xe0,
+};
+
+#define PINCTRL_BANK_REG(_bank, _reg) ((_reg  8) | (_bank  4))
+
+enum {
+MUX_GPIO = 0x3,
+};
+
+
+typedef struct imx23_pinctrl_state {
+SysBusDevice busdev;
+MemoryRegion iomem;
+
+uint32_t r[PINCTRL_MAX];
+qemu_irq irq_in[3];
+qemu_irq irq_out[PINCTRL_BANK_COUNT * 32];
+
+uint32_t state[PINCTRL_BANK_COUNT];
+} imx23_pinctrl_state;
+
+static uint64_t imx23_pinctrl_read(
+void *opaque, hwaddr offset, unsigned size)
+{
+imx23_pinctrl_state *s = (imx23_pinctrl_state *) opaque;
+uint32_t res = 0;
+
+switch (offset  4) {
+case 0 ... PINCTRL_MAX:
+res = s-r[offset  4];
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+%s: bad offset 0x%x\n, __func__, (int) offset);
+break;
+}
+
+return res;
+}
+
+static uint8_t imx23_pinctrl_getmux(
+imx23_pinctrl_state *s, int pin)
+{
+int base = pin / 16, offset = pin % 16;
+return (s-r[PINCTRL_BANK_MUXSEL + base]  (offset * 2))  0x3;
+}
+
+/*
+ * usage imx23_pinctrl_getbit(s, PINCTRL_BANK_IRQEN, 48)...
+ */
+static uint8_t imx23_pinctrl_getbit(
+imx23_pinctrl_state *s, uint16_t reg, int pin)
+{
+int bank = pin / 32, offset = pin % 32;
+uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
+//printf(%s bank %d offset %d reg %d : %04x=%08x\n, __func__, bank, 
offset, reg,
+//PINCTRL_BANK_REG(bank, reg),
+//*latch);
+return (*latch  offset)  0x1;
+}
+
+static void imx23_pinctrl_setbit(
+imx23_pinctrl_state *s, uint16_t reg, int pin, int value)
+{
+int bank = pin / 32, offset = pin % 32;
+uint32_t * latch = s-r[PINCTRL_BANK_REG(bank, reg)  4];
+*latch = (*latch  ~(1  offset)) | (!!value  offset);
+}
+
+static void imx23_pinctrl_write_bank(
+imx23_pinctrl_state *s, int bank,
+int reg, uint32_t value,
+uint32_t mask)
+{
+int set, pin;
+switch (reg) {
+/*
+ * Linux has a way of using the DOEPULL register to toggle the pin
+ */
+case PINCTRL_BANK_PULL:
+case PINCTRL_BANK_DOE:
+/*
+ * Writing to the Data OUT register just triggers the
+ * output qemu IRQ for any further peripherals
+ */
+case PINCTRL_BANK_OUT: {
+while ((set = ffs(mask))  0) {
+set--;
+mask = ~(1  set);
+pin = (bank * 32) + set;
+/*
+ * For a reason that is not clear, the pullup bit appears
+ * inverted (!) ignoring for now, assume hardware pullup
+ */
+// imx23_pinctrl_getbit(s, PINCTRL_BANK_PULL, pin)
+int val =
+imx23_pinctrl_getbit(s, PINCTRL_BANK_DOE, pin) ?
+imx23_pinctrl_getbit(s, PINCTRL_BANK_OUT, pin) 
:
+1;
+D(printf(%s set %2d to OUT:%d DOE:%d (PULL:%d) = %d\n, 
__func__,
+pin,
+