[Qemu-devel] [PATCH 0/3] omap_gpio: convert to qdev

[Qemu-devel] [PATCH 0/3] omap_gpio: convert to qdev

[Peter Maydell]

These patches are changes from the meego omap3 tree which convert
the omap GPIO module device to use qdev.

My general plan for landing the omap3 changes is to start by peeling
off reasonably independent chunks which can be presented as fixes
or cleanups to the existing omap1/omap2 code, and this is the first
of those. Once I run out of those the core "add omap3" patchset should
be much smaller and more manageable...

Sorry patch 3 is so large; I couldn't see anything more that I could
pull out as a separate patch.

(The Signed-off-by lines are a bit convoluted but so is the history
of the patchset; I hope and believe they're satisfactory to all the
participants.)

Juha Riihimäki (2):
  hw/omap_l4.c: Add helper function omap_l4_base
  hw/omap_gpio.c: Convert to qdev

Peter Maydell (1):
  hw/omap_gpio.c: Don't complain about some writes to r/o registers

 hw/nseries.c   |   47 +++++------
 hw/omap.h      |   21 +-----
 hw/omap1.c     |   10 ++-
 hw/omap2.c     |   26 ++++--
 hw/omap_gpio.c |  248 +++++++++++++++++++++++++++----------------------------
 hw/omap_l4.c   |    5 +
 hw/palm.c      |   26 +++---
 7 files changed, 189 insertions(+), 194 deletions(-)

[Qemu-devel] [PATCH 1/3] hw/omap_l4.c: Add helper function omap_l4_base

[Peter Maydell]

From: Juha Riihimäki <juha.riihimaki@nokia.com>

Add helper function omap_l4_base() to return the base address of a
particular region of an L4 target agent.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
[Riku Voipio: Fixes and restructuring patchset]
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
[Peter Maydell: More fixes and cleanups for upstream submission]
Signed-off-by:  Peter Maydell <peter.maydell@linaro.org>
---
 hw/omap.h    |    1 +
 hw/omap_l4.c |    5 +++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/omap.h b/hw/omap.h
index c227a82..339cc00 100644
--- a/hw/omap.h
+++ b/hw/omap.h
@@ -93,6 +93,7 @@ struct omap_target_agent_s *omap_l4ta_get(
     int cs);
 target_phys_addr_t omap_l4_attach(struct omap_target_agent_s *ta, int region,
                 int iotype);
+target_phys_addr_t omap_l4_base(struct omap_target_agent_s *ta, int region);
 int l4_register_io_memory(CPUReadMemoryFunc * const *mem_read,
                 CPUWriteMemoryFunc * const *mem_write, void *opaque);
 
diff --git a/hw/omap_l4.c b/hw/omap_l4.c
index 4af0ca8..8ab811d 100644
--- a/hw/omap_l4.c
+++ b/hw/omap_l4.c
@@ -146,6 +146,11 @@ struct omap_l4_s *omap_l4_init(target_phys_addr_t base, int ta_num)
     return bus;
 }
 
+target_phys_addr_t omap_l4_base(struct omap_target_agent_s *ta, int region)
+{
+    return ta->bus->base + ta->start[region].offset;
+}
+
 static uint32_t omap_l4ta_read(void *opaque, target_phys_addr_t addr)
 {
     struct omap_target_agent_s *s = (struct omap_target_agent_s *) opaque;
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] hw/omap_gpio.c: Don't complain about some writes to r/o registers

[Peter Maydell]

Don't complain about some writes to r/o OMAP2 GPIO registers, because the
kernel will do them anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/omap_gpio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/omap_gpio.c b/hw/omap_gpio.c
index 478f7d9..b53b13b 100644
--- a/hw/omap_gpio.c
+++ b/hw/omap_gpio.c
@@ -385,7 +385,7 @@ static void omap2_gpio_module_write(void *opaque, target_phys_addr_t addr,
     case 0x00:	/* GPIO_REVISION */
     case 0x14:	/* GPIO_SYSSTATUS */
     case 0x38:	/* GPIO_DATAIN */
-        OMAP_RO_REG(addr);
+        /* read-only, ignore quietly */
         break;
 
     case 0x10:	/* GPIO_SYSCONFIG */
@@ -531,7 +531,7 @@ static void omap2_gpio_module_writep(void *opaque, target_phys_addr_t addr,
     case 0x00:	/* GPIO_REVISION */
     case 0x14:	/* GPIO_SYSSTATUS */
     case 0x38:	/* GPIO_DATAIN */
-        OMAP_RO_REG(addr);
+        /* read-only, ignore quietly */
         break;
 
     case 0x10:	/* GPIO_SYSCONFIG */
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] hw/omap_gpio.c: Convert to qdev

[Peter Maydell]

From: Juha Riihimäki <juha.riihimaki@nokia.com>

Convert the OMAP GPIO module to qdev.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
[Riku Voipio: Fixes and restructuring patchset]
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
[Peter Maydell: More fixes and cleanups for upstream submission]
Signed-off-by:  Peter Maydell <peter.maydell@linaro.org>
---
 hw/nseries.c   |   47 +++++------
 hw/omap.h      |   20 +-----
 hw/omap1.c     |   10 ++-
 hw/omap2.c     |   26 ++++--
 hw/omap_gpio.c |  244 +++++++++++++++++++++++++++----------------------------
 hw/palm.c      |   26 +++---
 6 files changed, 181 insertions(+), 192 deletions(-)

diff --git a/hw/nseries.c b/hw/nseries.c
index 2f6f473..4ea2d6b 100644
--- a/hw/nseries.c
+++ b/hw/nseries.c
@@ -134,9 +134,9 @@ static void n800_mmc_cs_cb(void *opaque, int line, int level)
 static void n8x0_gpio_setup(struct n800_s *s)
 {
     qemu_irq *mmc_cs = qemu_allocate_irqs(n800_mmc_cs_cb, s->cpu->mmc, 1);
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_MMC_CS_GPIO, mmc_cs[0]);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_MMC_CS_GPIO, mmc_cs[0]);
 
-    qemu_irq_lower(omap2_gpio_in_get(s->cpu->gpif, N800_BAT_COVER_GPIO)[0]);
+    qemu_irq_lower(qdev_get_gpio_in(s->cpu->gpio, N800_BAT_COVER_GPIO));
 }
 
 #define MAEMO_CAL_HEADER(...)				\
@@ -168,8 +168,8 @@ static void n8x0_nand_setup(struct n800_s *s)
     omap_gpmc_attach(s->cpu->gpmc, N8X0_ONENAND_CS, 0, onenand_base_update,
                     onenand_base_unmap,
                     (s->nand = onenand_init(0xec4800, 1,
-                                            omap2_gpio_in_get(s->cpu->gpif,
-                                                    N8X0_ONENAND_GPIO)[0])));
+                                            qdev_get_gpio_in(s->cpu->gpio,
+                                                    N8X0_ONENAND_GPIO))));
     otp_region = onenand_raw_otp(s->nand);
 
     memcpy(otp_region + 0x000, n8x0_cal_wlan_mac, sizeof(n8x0_cal_wlan_mac));
@@ -180,7 +180,7 @@ static void n8x0_nand_setup(struct n800_s *s)
 static void n8x0_i2c_setup(struct n800_s *s)
 {
     DeviceState *dev;
-    qemu_irq tmp_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TMP105_GPIO)[0];
+    qemu_irq tmp_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TMP105_GPIO);
 
     /* Attach the CPU on one end of our I2C bus.  */
     s->i2c = omap_i2c_bus(s->cpu->i2c[0]);
@@ -249,8 +249,8 @@ static void n800_tsc_kbd_setup(struct n800_s *s)
     /* XXX: are the three pins inverted inside the chip between the
      * tsc and the cpu (N4111)?  */
     qemu_irq penirq = NULL;	/* NC */
-    qemu_irq kbirq = omap2_gpio_in_get(s->cpu->gpif, N800_TSC_KP_IRQ_GPIO)[0];
-    qemu_irq dav = omap2_gpio_in_get(s->cpu->gpif, N800_TSC_TS_GPIO)[0];
+    qemu_irq kbirq = qdev_get_gpio_in(s->cpu->gpio, N800_TSC_KP_IRQ_GPIO);
+    qemu_irq dav = qdev_get_gpio_in(s->cpu->gpio, N800_TSC_TS_GPIO);
 
     s->ts.chip = tsc2301_init(penirq, kbirq, dav);
     s->ts.opaque = s->ts.chip->opaque;
@@ -269,7 +269,7 @@ static void n800_tsc_kbd_setup(struct n800_s *s)
 
 static void n810_tsc_setup(struct n800_s *s)
 {
-    qemu_irq pintdav = omap2_gpio_in_get(s->cpu->gpif, N810_TSC_TS_GPIO)[0];
+    qemu_irq pintdav = qdev_get_gpio_in(s->cpu->gpio, N810_TSC_TS_GPIO);
 
     s->ts.opaque = tsc2005_init(pintdav);
     s->ts.txrx = tsc2005_txrx;
@@ -361,7 +361,7 @@ static int n810_keys[0x80] = {
 
 static void n810_kbd_setup(struct n800_s *s)
 {
-    qemu_irq kbd_irq = omap2_gpio_in_get(s->cpu->gpif, N810_KEYBOARD_GPIO)[0];
+    qemu_irq kbd_irq = qdev_get_gpio_in(s->cpu->gpio, N810_KEYBOARD_GPIO);
     DeviceState *dev;
     int i;
 
@@ -726,15 +726,15 @@ static void n8x0_dss_setup(struct n800_s *s)
 
 static void n8x0_cbus_setup(struct n800_s *s)
 {
-    qemu_irq dat_out = omap2_gpio_in_get(s->cpu->gpif, N8X0_CBUS_DAT_GPIO)[0];
-    qemu_irq retu_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_RETU_GPIO)[0];
-    qemu_irq tahvo_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TAHVO_GPIO)[0];
+    qemu_irq dat_out = qdev_get_gpio_in(s->cpu->gpio, N8X0_CBUS_DAT_GPIO);
+    qemu_irq retu_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_RETU_GPIO);
+    qemu_irq tahvo_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TAHVO_GPIO);
 
     CBus *cbus = cbus_init(dat_out);
 
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_CLK_GPIO, cbus->clk);
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_DAT_GPIO, cbus->dat);
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_SEL_GPIO, cbus->sel);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_CLK_GPIO, cbus->clk);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_DAT_GPIO, cbus->dat);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_SEL_GPIO, cbus->sel);
 
     cbus_attach(cbus, s->retu = retu_init(retu_irq, 1));
     cbus_attach(cbus, s->tahvo = tahvo_init(tahvo_irq, 1));
@@ -743,13 +743,12 @@ static void n8x0_cbus_setup(struct n800_s *s)
 static void n8x0_uart_setup(struct n800_s *s)
 {
     CharDriverState *radio = uart_hci_init(
-                    omap2_gpio_in_get(s->cpu->gpif,
-                            N8X0_BT_HOST_WKUP_GPIO)[0]);
+                    qdev_get_gpio_in(s->cpu->gpio, N8X0_BT_HOST_WKUP_GPIO));
 
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_BT_RESET_GPIO,
-                    csrhci_pins_get(radio)[csrhci_pin_reset]);
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_BT_WKUP_GPIO,
-                    csrhci_pins_get(radio)[csrhci_pin_wakeup]);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_BT_RESET_GPIO,
+                          csrhci_pins_get(radio)[csrhci_pin_reset]);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_BT_WKUP_GPIO,
+                          csrhci_pins_get(radio)[csrhci_pin_wakeup]);
 
     omap_uart_attach(s->cpu->uart[BT_UART], radio);
 }
@@ -763,7 +762,7 @@ static void n8x0_usb_power_cb(void *opaque, int line, int level)
 
 static void n8x0_usb_setup(struct n800_s *s)
 {
-    qemu_irq tusb_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TUSB_INT_GPIO)[0];
+    qemu_irq tusb_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TUSB_INT_GPIO);
     qemu_irq tusb_pwr = qemu_allocate_irqs(n8x0_usb_power_cb, s, 1)[0];
     TUSBState *tusb = tusb6010_init(tusb_irq);
 
@@ -774,7 +773,7 @@ static void n8x0_usb_setup(struct n800_s *s)
                     tusb6010_sync_io(tusb), NULL, NULL, tusb);
 
     s->usb = tusb;
-    omap2_gpio_out_set(s->cpu->gpif, N8X0_TUSB_ENABLE_GPIO, tusb_pwr);
+    qdev_connect_gpio_out(s->cpu->gpio, N8X0_TUSB_ENABLE_GPIO, tusb_pwr);
 }
 
 /* Setup done before the main bootloader starts by some early setup code
@@ -1020,7 +1019,7 @@ static void n8x0_boot_init(void *opaque)
 
     /* If the machine has a slided keyboard, open it */
     if (s->kbd)
-        qemu_irq_raise(omap2_gpio_in_get(s->cpu->gpif, N810_SLIDE_GPIO)[0]);
+        qemu_irq_raise(qdev_get_gpio_in(s->cpu->gpio, N810_SLIDE_GPIO));
 }
 
 #define OMAP_TAG_NOKIA_BT	0x4e01
diff --git a/hw/omap.h b/hw/omap.h
index 339cc00..2a5b90c 100644
--- a/hw/omap.h
+++ b/hw/omap.h
@@ -682,22 +682,6 @@ qemu_irq *omap_mpuio_in_get(struct omap_mpuio_s *s);
 void omap_mpuio_out_set(struct omap_mpuio_s *s, int line, qemu_irq handler);
 void omap_mpuio_key(struct omap_mpuio_s *s, int row, int col, int down);
 
-/* omap1 gpio module interface */
-struct omap_gpio_s;
-struct omap_gpio_s *omap_gpio_init(target_phys_addr_t base,
-                qemu_irq irq, omap_clk clk);
-void omap_gpio_reset(struct omap_gpio_s *s);
-qemu_irq *omap_gpio_in_get(struct omap_gpio_s *s);
-void omap_gpio_out_set(struct omap_gpio_s *s, int line, qemu_irq handler);
-
-/* omap2 gpio interface */
-struct omap_gpif_s;
-struct omap_gpif_s *omap2_gpio_init(struct omap_target_agent_s *ta,
-                qemu_irq *irq, omap_clk *fclk, omap_clk iclk, int modules);
-void omap_gpif_reset(struct omap_gpif_s *s);
-qemu_irq *omap2_gpio_in_get(struct omap_gpif_s *s, int start);
-void omap2_gpio_out_set(struct omap_gpif_s *s, int line, qemu_irq handler);
-
 struct uWireSlave {
     uint16_t (*receive)(void *opaque);
     void (*send)(void *opaque, uint16_t data);
@@ -851,7 +835,7 @@ struct omap_mpu_state_s {
     /* MPUI-TIPB peripherals */
     struct omap_uart_s *uart[3];
 
-    struct omap_gpio_s *gpio;
+    DeviceState *gpio;
 
     struct omap_mcbsp_s *mcbsp1;
     struct omap_mcbsp_s *mcbsp3;
@@ -949,8 +933,6 @@ struct omap_mpu_state_s {
     struct omap_gpmc_s *gpmc;
     struct omap_sysctl_s *sysc;
 
-    struct omap_gpif_s *gpif;
-
     struct omap_mcspi_s *mcspi[2];
 
     struct omap_dss_s *dss;
diff --git a/hw/omap1.c b/hw/omap1.c
index 364c26f..625f7cc 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -27,6 +27,7 @@
 #include "pc.h"
 #include "blockdev.h"
 #include "range.h"
+#include "sysbus.h"
 
 /* Should signal the TCMI/GPMC */
 uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
@@ -3585,7 +3586,6 @@ static void omap1_mpu_reset(void *opaque)
     omap_uart_reset(mpu->uart[2]);
     omap_mmc_reset(mpu->mmc);
     omap_mpuio_reset(mpu->mpuio);
-    omap_gpio_reset(mpu->gpio);
     omap_uwire_reset(mpu->microwire);
     omap_pwl_reset(mpu);
     omap_pwt_reset(mpu);
@@ -3845,8 +3845,12 @@ struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
                     s->irq[1][OMAP_INT_KEYBOARD], s->irq[1][OMAP_INT_MPUIO],
                     s->wakeup, omap_findclk(s, "clk32-kHz"));
 
-    s->gpio = omap_gpio_init(0xfffce000, s->irq[0][OMAP_INT_GPIO_BANK1],
-                    omap_findclk(s, "arm_gpio_ck"));
+    s->gpio = qdev_create(NULL, "omap_gpio");
+    qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
+    qdev_init_nofail(s->gpio);
+    sysbus_connect_irq(sysbus_from_qdev(s->gpio), 0,
+                       s->irq[0][OMAP_INT_GPIO_BANK1]);
+    sysbus_mmio_map(sysbus_from_qdev(s->gpio), 0, 0xfffce000);
 
     s->microwire = omap_uwire_init(0xfffb3000, &s->irq[1][OMAP_INT_uWireTX],
                     s->drq[OMAP_DMA_UWIRE_TX], omap_findclk(s, "mpuper_ck"));
diff --git a/hw/omap2.c b/hw/omap2.c
index 0f13272..3b48696 100644
--- a/hw/omap2.c
+++ b/hw/omap2.c
@@ -27,6 +27,7 @@
 #include "qemu-char.h"
 #include "flash.h"
 #include "soc_dma.h"
+#include "sysbus.h"
 #include "audio/audio.h"
 
 /* Enhanced Audio Controller (CODEC only) */
@@ -2203,7 +2204,6 @@ static void omap2_mpu_reset(void *opaque)
     omap_uart_reset(mpu->uart[1]);
     omap_uart_reset(mpu->uart[2]);
     omap_mmc_reset(mpu->mmc);
-    omap_gpif_reset(mpu->gpif);
     omap_mcspi_reset(mpu->mcspi[0]);
     omap_mcspi_reset(mpu->mcspi[1]);
     omap_i2c_reset(mpu->i2c[0]);
@@ -2232,9 +2232,10 @@ struct omap_mpu_state_s *omap2420_mpu_init(unsigned long sdram_size,
     ram_addr_t sram_base, q2_base;
     qemu_irq *cpu_irq;
     qemu_irq dma_irqs[4];
-    omap_clk gpio_clks[4];
     DriveInfo *dinfo;
     int i;
+    SysBusDevice *busdev;
+    struct omap_target_agent_s *ta;
 
     /* Core */
     s->mpu_model = omap2420;
@@ -2377,13 +2378,20 @@ struct omap_mpu_state_s *omap2420_mpu_init(unsigned long sdram_size,
                     omap_findclk(s, "i2c2.fclk"),
                     omap_findclk(s, "i2c2.iclk"));
 
-    gpio_clks[0] = omap_findclk(s, "gpio1_dbclk");
-    gpio_clks[1] = omap_findclk(s, "gpio2_dbclk");
-    gpio_clks[2] = omap_findclk(s, "gpio3_dbclk");
-    gpio_clks[3] = omap_findclk(s, "gpio4_dbclk");
-    s->gpif = omap2_gpio_init(omap_l4ta(s->l4, 3),
-                    &s->irq[0][OMAP_INT_24XX_GPIO_BANK1],
-                    gpio_clks, omap_findclk(s, "gpio_iclk"), 4);
+    s->gpio = qdev_create(NULL, "omap_gpio");
+    qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
+    qdev_init_nofail(s->gpio);
+    busdev = sysbus_from_qdev(s->gpio);
+    sysbus_connect_irq(busdev, 0, s->irq[0][OMAP_INT_24XX_GPIO_BANK1]);
+    sysbus_connect_irq(busdev, 3, s->irq[0][OMAP_INT_24XX_GPIO_BANK2]);
+    sysbus_connect_irq(busdev, 6, s->irq[0][OMAP_INT_24XX_GPIO_BANK3]);
+    sysbus_connect_irq(busdev, 9, s->irq[0][OMAP_INT_24XX_GPIO_BANK4]);
+    ta = omap_l4ta(s->l4, 3);
+    sysbus_mmio_map(busdev, 0, omap_l4_base(ta, 1));
+    sysbus_mmio_map(busdev, 1, omap_l4_base(ta, 0));
+    sysbus_mmio_map(busdev, 2, omap_l4_base(ta, 2));
+    sysbus_mmio_map(busdev, 3, omap_l4_base(ta, 4));
+    sysbus_mmio_map(busdev, 4, omap_l4_base(ta, 5));
 
     s->sdrc = omap_sdrc_init(0x68009000);
     s->gpmc = omap_gpmc_init(0x6800a000, s->irq[0][OMAP_INT_24XX_GPMC_IRQ]);
diff --git a/hw/omap_gpio.c b/hw/omap_gpio.c
index b53b13b..410d8a6 100644
--- a/hw/omap_gpio.c
+++ b/hw/omap_gpio.c
@@ -20,10 +20,10 @@
 
 #include "hw.h"
 #include "omap.h"
-/* General-Purpose I/O */
+#include "sysbus.h"
+
 struct omap_gpio_s {
     qemu_irq irq;
-    qemu_irq *in;
     qemu_irq handler[16];
 
     uint16_t inputs;
@@ -35,9 +35,44 @@ struct omap_gpio_s {
     uint16_t pins;
 };
 
+struct omap2_gpio_s {
+    qemu_irq irq[2];
+    qemu_irq wkup;
+    qemu_irq *handler;
+
+    uint8_t revision;
+    uint8_t config[2];
+    uint32_t inputs;
+    uint32_t outputs;
+    uint32_t dir;
+    uint32_t level[2];
+    uint32_t edge[2];
+    uint32_t mask[2];
+    uint32_t wumask;
+    uint32_t ints[2];
+    uint32_t debounce;
+    uint8_t delay;
+};
+
+struct omap_gpif_s {
+    SysBusDevice busdev;
+    int mpu_model;
+    union {
+        struct omap_gpio_s omap1;
+        struct {
+            int modulecount;
+            struct omap2_gpio_s *modules;
+            qemu_irq *handler;
+            int autoidle;
+            int gpo;
+        } omap2;
+    } regs;
+};
+
+/* General-Purpose I/O of OMAP1 */
 static void omap_gpio_set(void *opaque, int line, int level)
 {
-    struct omap_gpio_s *s = (struct omap_gpio_s *) opaque;
+    struct omap_gpio_s *s = &((struct omap_gpif_s *)opaque)->regs.omap1;
     uint16_t prev = s->inputs;
 
     if (level)
@@ -160,7 +195,7 @@ static CPUWriteMemoryFunc * const omap_gpio_writefn[] = {
     omap_badwidth_write16,
 };
 
-void omap_gpio_reset(struct omap_gpio_s *s)
+static void omap_gpio_reset(struct omap_gpio_s *s)
 {
     s->inputs = 0;
     s->outputs = ~0;
@@ -171,58 +206,9 @@ void omap_gpio_reset(struct omap_gpio_s *s)
     s->pins = ~0;
 }
 
-struct omap_gpio_s *omap_gpio_init(target_phys_addr_t base,
-                qemu_irq irq, omap_clk clk)
-{
-    int iomemtype;
-    struct omap_gpio_s *s = (struct omap_gpio_s *)
-            qemu_mallocz(sizeof(struct omap_gpio_s));
-
-    s->irq = irq;
-    s->in = qemu_allocate_irqs(omap_gpio_set, s, 16);
-    omap_gpio_reset(s);
-
-    iomemtype = cpu_register_io_memory(omap_gpio_readfn,
-                    omap_gpio_writefn, s, DEVICE_NATIVE_ENDIAN);
-    cpu_register_physical_memory(base, 0x1000, iomemtype);
-
-    return s;
-}
-
-qemu_irq *omap_gpio_in_get(struct omap_gpio_s *s)
-{
-    return s->in;
-}
-
-void omap_gpio_out_set(struct omap_gpio_s *s, int line, qemu_irq handler)
-{
-    if (line >= 16 || line < 0)
-        hw_error("%s: No GPIO line %i\n", __FUNCTION__, line);
-    s->handler[line] = handler;
-}
-
-/* General-Purpose Interface of OMAP2 */
-struct omap2_gpio_s {
-    qemu_irq irq[2];
-    qemu_irq wkup;
-    qemu_irq *in;
-    qemu_irq handler[32];
-
-    uint8_t config[2];
-    uint32_t inputs;
-    uint32_t outputs;
-    uint32_t dir;
-    uint32_t level[2];
-    uint32_t edge[2];
-    uint32_t mask[2];
-    uint32_t wumask;
-    uint32_t ints[2];
-    uint32_t debounce;
-    uint8_t delay;
-};
-
+/* General-Purpose Interface of OMAP2/3 */
 static inline void omap2_gpio_module_int_update(struct omap2_gpio_s *s,
-                int line)
+                                                int line)
 {
     qemu_set_irq(s->irq[line], s->ints[line] & s->mask[line]);
 }
@@ -271,8 +257,9 @@ static inline void omap2_gpio_module_int(struct omap2_gpio_s *s, int line)
 
 static void omap2_gpio_module_set(void *opaque, int line, int level)
 {
-    struct omap2_gpio_s *s = (struct omap2_gpio_s *) opaque;
-
+    struct omap_gpif_s *p = opaque;
+    struct omap2_gpio_s *s = &p->regs.omap2.modules[line >> 5];
+    line &= 31;
     if (level) {
         if (s->dir & (1 << line) & ((~s->inputs & s->edge[0]) | s->level[1]))
             omap2_gpio_module_int(s, line);
@@ -308,7 +295,7 @@ static uint32_t omap2_gpio_module_read(void *opaque, target_phys_addr_t addr)
 
     switch (addr) {
     case 0x00:	/* GPIO_REVISION */
-        return 0x18;
+        return s->revision;
 
     case 0x10:	/* GPIO_SYSCONFIG */
         return s->config[0];
@@ -583,40 +570,20 @@ static CPUWriteMemoryFunc * const omap2_gpio_module_writefn[] = {
     omap2_gpio_module_write,
 };
 
-static void omap2_gpio_module_init(struct omap2_gpio_s *s,
-                struct omap_target_agent_s *ta, int region,
-                qemu_irq mpu, qemu_irq dsp, qemu_irq wkup,
-                omap_clk fclk, omap_clk iclk)
-{
-    int iomemtype;
-
-    s->irq[0] = mpu;
-    s->irq[1] = dsp;
-    s->wkup = wkup;
-    s->in = qemu_allocate_irqs(omap2_gpio_module_set, s, 32);
-
-    iomemtype = l4_register_io_memory(omap2_gpio_module_readfn,
-                    omap2_gpio_module_writefn, s);
-    omap_l4_attach(ta, region, iomemtype);
-}
-
-struct omap_gpif_s {
-    struct omap2_gpio_s module[5];
-    int modules;
-
-    int autoidle;
-    int gpo;
-};
-
-void omap_gpif_reset(struct omap_gpif_s *s)
+static void omap_gpif_reset(DeviceState *dev)
 {
     int i;
-
-    for (i = 0; i < s->modules; i ++)
-        omap2_gpio_module_reset(s->module + i);
-
-    s->autoidle = 0;
-    s->gpo = 0;
+    struct omap_gpif_s *s = FROM_SYSBUS(struct omap_gpif_s,
+                                        sysbus_from_qdev(dev));
+    if (s->mpu_model < omap2410) {
+        omap_gpio_reset(&s->regs.omap1);
+    } else {
+        for (i = 0; i < s->regs.omap2.modulecount; i++) {
+            omap2_gpio_module_reset(&s->regs.omap2.modules[i]);
+        }
+        s->regs.omap2.autoidle = 0;
+        s->regs.omap2.gpo = 0;
+    }
 }
 
 static uint32_t omap_gpif_top_read(void *opaque, target_phys_addr_t addr)
@@ -628,7 +595,7 @@ static uint32_t omap_gpif_top_read(void *opaque, target_phys_addr_t addr)
         return 0x18;
 
     case 0x10:	/* IPGENERICOCPSPL_SYSCONFIG */
-        return s->autoidle;
+        return s->regs.omap2.autoidle;
 
     case 0x14:	/* IPGENERICOCPSPL_SYSSTATUS */
         return 0x01;
@@ -637,7 +604,7 @@ static uint32_t omap_gpif_top_read(void *opaque, target_phys_addr_t addr)
         return 0x00;
 
     case 0x40:	/* IPGENERICOCPSPL_GPO */
-        return s->gpo;
+        return s->regs.omap2.gpo;
 
     case 0x50:	/* IPGENERICOCPSPL_GPI */
         return 0x00;
@@ -662,12 +629,12 @@ static void omap_gpif_top_write(void *opaque, target_phys_addr_t addr,
 
     case 0x10:	/* IPGENERICOCPSPL_SYSCONFIG */
         if (value & (1 << 1))					/* SOFTRESET */
-            omap_gpif_reset(s);
-        s->autoidle = value & 1;
+            omap_gpif_reset(&s->busdev.qdev);
+        s->regs.omap2.autoidle = value & 1;
         break;
 
     case 0x40:	/* IPGENERICOCPSPL_GPO */
-        s->gpo = value & 1;
+        s->regs.omap2.gpo = value & 1;
         break;
 
     default:
@@ -688,38 +655,67 @@ static CPUWriteMemoryFunc * const omap_gpif_top_writefn[] = {
     omap_gpif_top_write,
 };
 
-struct omap_gpif_s *omap2_gpio_init(struct omap_target_agent_s *ta,
-                qemu_irq *irq, omap_clk *fclk, omap_clk iclk, int modules)
+static int omap_gpio_init(SysBusDevice *dev)
 {
-    int iomemtype, i;
-    struct omap_gpif_s *s = (struct omap_gpif_s *)
-            qemu_mallocz(sizeof(struct omap_gpif_s));
-    int region[4] = { 0, 2, 4, 5 };
-
-    s->modules = modules;
-    for (i = 0; i < modules; i ++)
-        omap2_gpio_module_init(s->module + i, ta, region[i],
-                              irq[i], NULL, NULL, fclk[i], iclk);
-
-    omap_gpif_reset(s);
-
-    iomemtype = l4_register_io_memory(omap_gpif_top_readfn,
-                    omap_gpif_top_writefn, s);
-    omap_l4_attach(ta, 1, iomemtype);
-
-    return s;
+    int i;
+    struct omap_gpif_s *s = FROM_SYSBUS(struct omap_gpif_s, dev);
+    if (s->mpu_model < omap2410) {
+        qdev_init_gpio_in(&dev->qdev, omap_gpio_set, 16);
+        qdev_init_gpio_out(&dev->qdev, s->regs.omap1.handler, 16);
+        sysbus_init_irq(dev, &s->regs.omap1.irq);
+        sysbus_init_mmio(dev, 0x1000,
+                         cpu_register_io_memory(omap_gpio_readfn,
+                                                omap_gpio_writefn,
+                                                &s->regs.omap1,
+                                                DEVICE_NATIVE_ENDIAN));
+    } else {
+        if (s->mpu_model < omap3430) {
+            s->regs.omap2.modulecount = (s->mpu_model < omap2430) ? 4 : 5;
+            sysbus_init_mmio(dev, 0x1000,
+                             cpu_register_io_memory(omap_gpif_top_readfn,
+                                                    omap_gpif_top_writefn, s,
+                                                    DEVICE_NATIVE_ENDIAN));
+        } else {
+            s->regs.omap2.modulecount = 6;
+        }
+        s->regs.omap2.modules = qemu_mallocz(s->regs.omap2.modulecount *
+                                             sizeof(struct omap2_gpio_s));
+        s->regs.omap2.handler = qemu_mallocz(s->regs.omap2.modulecount * 32 *
+                                             sizeof(qemu_irq));
+        qdev_init_gpio_in(&dev->qdev, omap2_gpio_module_set,
+                          s->regs.omap2.modulecount * 32);
+        qdev_init_gpio_out(&dev->qdev, s->regs.omap2.handler,
+                           s->regs.omap2.modulecount * 32);
+        for (i = 0; i < s->regs.omap2.modulecount; i++) {
+            struct omap2_gpio_s *m = &s->regs.omap2.modules[i];
+            m->revision = (s->mpu_model < omap3430) ? 0x18 : 0x25;
+            m->handler = &s->regs.omap2.handler[i * 32];
+            sysbus_init_irq(dev, &m->irq[0]); /* mpu irq */
+            sysbus_init_irq(dev, &m->irq[1]); /* dsp irq */
+            sysbus_init_irq(dev, &m->wkup);
+            sysbus_init_mmio(dev, 0x1000,
+                             cpu_register_io_memory(omap2_gpio_module_readfn,
+                                                    omap2_gpio_module_writefn,
+                                                    m, DEVICE_NATIVE_ENDIAN));
+        }
+    }
+    return 0;
 }
 
-qemu_irq *omap2_gpio_in_get(struct omap_gpif_s *s, int start)
-{
-    if (start >= s->modules * 32 || start < 0)
-        hw_error("%s: No GPIO line %i\n", __FUNCTION__, start);
-    return s->module[start >> 5].in + (start & 31);
-}
+static SysBusDeviceInfo omap_gpio_info = {
+    .init = omap_gpio_init,
+    .qdev.name = "omap_gpio",
+    .qdev.size = sizeof(struct omap_gpif_s),
+    .qdev.reset = omap_gpif_reset,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_INT32("mpu_model", struct omap_gpif_s, mpu_model, 0),
+        DEFINE_PROP_END_OF_LIST()
+    }
+};
 
-void omap2_gpio_out_set(struct omap_gpif_s *s, int line, qemu_irq handler)
+static void omap_gpio_register_device(void)
 {
-    if (line >= s->modules * 32 || line < 0)
-        hw_error("%s: No GPIO line %i\n", __FUNCTION__, line);
-    s->module[line >> 5].handler[line & 31] = handler;
+    sysbus_register_withprop(&omap_gpio_info);
 }
+
+device_init(omap_gpio_register_device)
diff --git a/hw/palm.c b/hw/palm.c
index f22d777..1c97394 100644
--- a/hw/palm.c
+++ b/hw/palm.c
@@ -94,7 +94,7 @@ static void palmte_microwire_setup(struct omap_mpu_state_s *cpu)
 {
     uWireSlave *tsc;
 
-    tsc = tsc2102_init(omap_gpio_in_get(cpu->gpio)[PALMTE_PINTDAV_GPIO]);
+    tsc = tsc2102_init(qdev_get_gpio_in(cpu->gpio, PALMTE_PINTDAV_GPIO));
 
     omap_uwire_attach(cpu->microwire, tsc, 0);
     omap_mcbsp_i2s_attach(cpu->mcbsp1, tsc210x_codec(tsc));
@@ -163,24 +163,24 @@ static void palmte_gpio_setup(struct omap_mpu_state_s *cpu)
     qemu_irq *misc_gpio;
 
     omap_mmc_handlers(cpu->mmc,
-                    omap_gpio_in_get(cpu->gpio)[PALMTE_MMC_WP_GPIO],
-                    qemu_irq_invert(omap_mpuio_in_get(cpu->mpuio)
-                            [PALMTE_MMC_SWITCH_GPIO]));
+                      qdev_get_gpio_in(cpu->gpio, PALMTE_MMC_WP_GPIO),
+                      qemu_irq_invert(omap_mpuio_in_get(cpu->mpuio)
+                                      [PALMTE_MMC_SWITCH_GPIO]));
 
     misc_gpio = qemu_allocate_irqs(palmte_onoff_gpios, cpu, 7);
-    omap_gpio_out_set(cpu->gpio, PALMTE_MMC_POWER_GPIO,	misc_gpio[0]);
-    omap_gpio_out_set(cpu->gpio, PALMTE_SPEAKER_GPIO,	misc_gpio[1]);
-    omap_gpio_out_set(cpu->gpio, 11,			misc_gpio[2]);
-    omap_gpio_out_set(cpu->gpio, 12,			misc_gpio[3]);
-    omap_gpio_out_set(cpu->gpio, 13,			misc_gpio[4]);
+    qdev_connect_gpio_out(cpu->gpio, PALMTE_MMC_POWER_GPIO, misc_gpio[0]);
+    qdev_connect_gpio_out(cpu->gpio, PALMTE_SPEAKER_GPIO,   misc_gpio[1]);
+    qdev_connect_gpio_out(cpu->gpio, 11,                    misc_gpio[2]);
+    qdev_connect_gpio_out(cpu->gpio, 12,                    misc_gpio[3]);
+    qdev_connect_gpio_out(cpu->gpio, 13,                    misc_gpio[4]);
     omap_mpuio_out_set(cpu->mpuio, 1,			misc_gpio[5]);
     omap_mpuio_out_set(cpu->mpuio, 3,			misc_gpio[6]);
 
     /* Reset some inputs to initial state.  */
-    qemu_irq_lower(omap_gpio_in_get(cpu->gpio)[PALMTE_USBDETECT_GPIO]);
-    qemu_irq_lower(omap_gpio_in_get(cpu->gpio)[PALMTE_USB_OR_DC_GPIO]);
-    qemu_irq_lower(omap_gpio_in_get(cpu->gpio)[4]);
-    qemu_irq_lower(omap_gpio_in_get(cpu->gpio)[PALMTE_HEADPHONES_GPIO]);
+    qemu_irq_lower(qdev_get_gpio_in(cpu->gpio, PALMTE_USBDETECT_GPIO));
+    qemu_irq_lower(qdev_get_gpio_in(cpu->gpio, PALMTE_USB_OR_DC_GPIO));
+    qemu_irq_lower(qdev_get_gpio_in(cpu->gpio, 4));
+    qemu_irq_lower(qdev_get_gpio_in(cpu->gpio, PALMTE_HEADPHONES_GPIO));
     qemu_irq_lower(omap_mpuio_in_get(cpu->mpuio)[PALMTE_DC_GPIO]);
     qemu_irq_raise(omap_mpuio_in_get(cpu->mpuio)[6]);
     qemu_irq_raise(omap_mpuio_in_get(cpu->mpuio)[7]);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] hw/omap_gpio.c: Don't complain about some writes to r/o registers

[Edgar E. Iglesias]

On Wed, Jun 29, 2011 at 07:53:10PM +0100, Peter Maydell wrote:
> Don't complain about some writes to r/o OMAP2 GPIO registers, because the
> kernel will do them anyway.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi Peter,

I usually find this kind of logs useful.
Maybe we should turn them into _log_mask(~0, xxx) so they only come out
when -d is enabled?

Cheers


> ---
>  hw/omap_gpio.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/omap_gpio.c b/hw/omap_gpio.c
> index 478f7d9..b53b13b 100644
> --- a/hw/omap_gpio.c
> +++ b/hw/omap_gpio.c
> @@ -385,7 +385,7 @@ static void omap2_gpio_module_write(void *opaque, target_phys_addr_t addr,
>      case 0x00:	/* GPIO_REVISION */
>      case 0x14:	/* GPIO_SYSSTATUS */
>      case 0x38:	/* GPIO_DATAIN */
> -        OMAP_RO_REG(addr);
> +        /* read-only, ignore quietly */
>          break;
>  
>      case 0x10:	/* GPIO_SYSCONFIG */
> @@ -531,7 +531,7 @@ static void omap2_gpio_module_writep(void *opaque, target_phys_addr_t addr,
>      case 0x00:	/* GPIO_REVISION */
>      case 0x14:	/* GPIO_SYSSTATUS */
>      case 0x38:	/* GPIO_DATAIN */
> -        OMAP_RO_REG(addr);
> +        /* read-only, ignore quietly */
>          break;
>  
>      case 0x10:	/* GPIO_SYSCONFIG */
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hw/omap_gpio.c: Don't complain about some writes to r/o registers

[Peter Maydell]

On 29 June 2011 21:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Wed, Jun 29, 2011 at 07:53:10PM +0100, Peter Maydell wrote:
>> Don't complain about some writes to r/o OMAP2 GPIO registers, because the
>> kernel will do them anyway.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi Peter,
>
> I usually find this kind of logs useful.
> Maybe we should turn them into _log_mask(~0, xxx) so they only come out
> when -d is enabled?

I think the ideal would be to have all of this kind of message go
through some consistent path (ie not just printf) so you could
tell all of qemu "I'm doing OS development, warn about things which
indicate driver bugs" vs the more common "I'm just a user and I
don't really care".

You'd also want to be able to trace the guest program counter
and to do things like drop into gdb if running with the gdb stub
enabled.

In the absence of that kind of infrastructure I tend to default
to removing this kind of printf or relegating it to DPRINTF.

-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/omap_gpio.c: Don't complain about some writes to r/o registers

[Edgar E. Iglesias]

On Wed, Jun 29, 2011 at 09:39:52PM +0100, Peter Maydell wrote:
> On 29 June 2011 21:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Wed, Jun 29, 2011 at 07:53:10PM +0100, Peter Maydell wrote:
> >> Don't complain about some writes to r/o OMAP2 GPIO registers, because the
> >> kernel will do them anyway.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Hi Peter,
> >
> > I usually find this kind of logs useful.
> > Maybe we should turn them into _log_mask(~0, xxx) so they only come out
> > when -d is enabled?
> 
> I think the ideal would be to have all of this kind of message go
> through some consistent path (ie not just printf) so you could
> tell all of qemu "I'm doing OS development, warn about things which
> indicate driver bugs" vs the more common "I'm just a user and I
> don't really care".
> 
> You'd also want to be able to trace the guest program counter
> and to do things like drop into gdb if running with the gdb stub
> enabled.
> 
> In the absence of that kind of infrastructure I tend to default
> to removing this kind of printf or relegating it to DPRINTF.

Yes. I usually find -d exec good enough for this simple errors.
The GDB connection I find useful for more complex hw-errors where
PC location isn't enough to figure out what caused the hw-errror.

I dont feel very strongly about it though.

Cheers

Re: [Qemu-devel] [PATCH 3/3] hw/omap_gpio.c: Convert to qdev

[andrzej zaborowski]

Hi,

On 29 June 2011 20:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: Juha Riihimäki <juha.riihimaki@nokia.com>
>
> Convert the OMAP GPIO module to qdev.
>
> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> [Riku Voipio: Fixes and restructuring patchset]
> Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
> [Peter Maydell: More fixes and cleanups for upstream submission]
> Signed-off-by:  Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/nseries.c   |   47 +++++------
>  hw/omap.h      |   20 +-----
>  hw/omap1.c     |   10 ++-
>  hw/omap2.c     |   26 ++++--
>  hw/omap_gpio.c |  244 +++++++++++++++++++++++++++----------------------------
>  hw/palm.c      |   26 +++---
>  6 files changed, 181 insertions(+), 192 deletions(-)
>
> diff --git a/hw/nseries.c b/hw/nseries.c
> index 2f6f473..4ea2d6b 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -134,9 +134,9 @@ static void n800_mmc_cs_cb(void *opaque, int line, int level)
>  static void n8x0_gpio_setup(struct n800_s *s)
>  {
>     qemu_irq *mmc_cs = qemu_allocate_irqs(n800_mmc_cs_cb, s->cpu->mmc, 1);
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_MMC_CS_GPIO, mmc_cs[0]);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_MMC_CS_GPIO, mmc_cs[0]);
>
> -    qemu_irq_lower(omap2_gpio_in_get(s->cpu->gpif, N800_BAT_COVER_GPIO)[0]);
> +    qemu_irq_lower(qdev_get_gpio_in(s->cpu->gpio, N800_BAT_COVER_GPIO));
>  }
>
>  #define MAEMO_CAL_HEADER(...)                          \
> @@ -168,8 +168,8 @@ static void n8x0_nand_setup(struct n800_s *s)
>     omap_gpmc_attach(s->cpu->gpmc, N8X0_ONENAND_CS, 0, onenand_base_update,
>                     onenand_base_unmap,
>                     (s->nand = onenand_init(0xec4800, 1,
> -                                            omap2_gpio_in_get(s->cpu->gpif,
> -                                                    N8X0_ONENAND_GPIO)[0])));
> +                                            qdev_get_gpio_in(s->cpu->gpio,
> +                                                    N8X0_ONENAND_GPIO))));
>     otp_region = onenand_raw_otp(s->nand);
>
>     memcpy(otp_region + 0x000, n8x0_cal_wlan_mac, sizeof(n8x0_cal_wlan_mac));
> @@ -180,7 +180,7 @@ static void n8x0_nand_setup(struct n800_s *s)
>  static void n8x0_i2c_setup(struct n800_s *s)
>  {
>     DeviceState *dev;
> -    qemu_irq tmp_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TMP105_GPIO)[0];
> +    qemu_irq tmp_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TMP105_GPIO);
>
>     /* Attach the CPU on one end of our I2C bus.  */
>     s->i2c = omap_i2c_bus(s->cpu->i2c[0]);
> @@ -249,8 +249,8 @@ static void n800_tsc_kbd_setup(struct n800_s *s)
>     /* XXX: are the three pins inverted inside the chip between the
>      * tsc and the cpu (N4111)?  */
>     qemu_irq penirq = NULL;    /* NC */
> -    qemu_irq kbirq = omap2_gpio_in_get(s->cpu->gpif, N800_TSC_KP_IRQ_GPIO)[0];
> -    qemu_irq dav = omap2_gpio_in_get(s->cpu->gpif, N800_TSC_TS_GPIO)[0];
> +    qemu_irq kbirq = qdev_get_gpio_in(s->cpu->gpio, N800_TSC_KP_IRQ_GPIO);
> +    qemu_irq dav = qdev_get_gpio_in(s->cpu->gpio, N800_TSC_TS_GPIO);
>
>     s->ts.chip = tsc2301_init(penirq, kbirq, dav);
>     s->ts.opaque = s->ts.chip->opaque;
> @@ -269,7 +269,7 @@ static void n800_tsc_kbd_setup(struct n800_s *s)
>
>  static void n810_tsc_setup(struct n800_s *s)
>  {
> -    qemu_irq pintdav = omap2_gpio_in_get(s->cpu->gpif, N810_TSC_TS_GPIO)[0];
> +    qemu_irq pintdav = qdev_get_gpio_in(s->cpu->gpio, N810_TSC_TS_GPIO);
>
>     s->ts.opaque = tsc2005_init(pintdav);
>     s->ts.txrx = tsc2005_txrx;
> @@ -361,7 +361,7 @@ static int n810_keys[0x80] = {
>
>  static void n810_kbd_setup(struct n800_s *s)
>  {
> -    qemu_irq kbd_irq = omap2_gpio_in_get(s->cpu->gpif, N810_KEYBOARD_GPIO)[0];
> +    qemu_irq kbd_irq = qdev_get_gpio_in(s->cpu->gpio, N810_KEYBOARD_GPIO);
>     DeviceState *dev;
>     int i;
>
> @@ -726,15 +726,15 @@ static void n8x0_dss_setup(struct n800_s *s)
>
>  static void n8x0_cbus_setup(struct n800_s *s)
>  {
> -    qemu_irq dat_out = omap2_gpio_in_get(s->cpu->gpif, N8X0_CBUS_DAT_GPIO)[0];
> -    qemu_irq retu_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_RETU_GPIO)[0];
> -    qemu_irq tahvo_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TAHVO_GPIO)[0];
> +    qemu_irq dat_out = qdev_get_gpio_in(s->cpu->gpio, N8X0_CBUS_DAT_GPIO);
> +    qemu_irq retu_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_RETU_GPIO);
> +    qemu_irq tahvo_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TAHVO_GPIO);
>
>     CBus *cbus = cbus_init(dat_out);
>
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_CLK_GPIO, cbus->clk);
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_DAT_GPIO, cbus->dat);
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_CBUS_SEL_GPIO, cbus->sel);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_CLK_GPIO, cbus->clk);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_DAT_GPIO, cbus->dat);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_CBUS_SEL_GPIO, cbus->sel);
>
>     cbus_attach(cbus, s->retu = retu_init(retu_irq, 1));
>     cbus_attach(cbus, s->tahvo = tahvo_init(tahvo_irq, 1));
> @@ -743,13 +743,12 @@ static void n8x0_cbus_setup(struct n800_s *s)
>  static void n8x0_uart_setup(struct n800_s *s)
>  {
>     CharDriverState *radio = uart_hci_init(
> -                    omap2_gpio_in_get(s->cpu->gpif,
> -                            N8X0_BT_HOST_WKUP_GPIO)[0]);
> +                    qdev_get_gpio_in(s->cpu->gpio, N8X0_BT_HOST_WKUP_GPIO));
>
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_BT_RESET_GPIO,
> -                    csrhci_pins_get(radio)[csrhci_pin_reset]);
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_BT_WKUP_GPIO,
> -                    csrhci_pins_get(radio)[csrhci_pin_wakeup]);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_BT_RESET_GPIO,
> +                          csrhci_pins_get(radio)[csrhci_pin_reset]);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_BT_WKUP_GPIO,
> +                          csrhci_pins_get(radio)[csrhci_pin_wakeup]);
>
>     omap_uart_attach(s->cpu->uart[BT_UART], radio);
>  }
> @@ -763,7 +762,7 @@ static void n8x0_usb_power_cb(void *opaque, int line, int level)
>
>  static void n8x0_usb_setup(struct n800_s *s)
>  {
> -    qemu_irq tusb_irq = omap2_gpio_in_get(s->cpu->gpif, N8X0_TUSB_INT_GPIO)[0];
> +    qemu_irq tusb_irq = qdev_get_gpio_in(s->cpu->gpio, N8X0_TUSB_INT_GPIO);
>     qemu_irq tusb_pwr = qemu_allocate_irqs(n8x0_usb_power_cb, s, 1)[0];
>     TUSBState *tusb = tusb6010_init(tusb_irq);
>
> @@ -774,7 +773,7 @@ static void n8x0_usb_setup(struct n800_s *s)
>                     tusb6010_sync_io(tusb), NULL, NULL, tusb);
>
>     s->usb = tusb;
> -    omap2_gpio_out_set(s->cpu->gpif, N8X0_TUSB_ENABLE_GPIO, tusb_pwr);
> +    qdev_connect_gpio_out(s->cpu->gpio, N8X0_TUSB_ENABLE_GPIO, tusb_pwr);
>  }
>
>  /* Setup done before the main bootloader starts by some early setup code
> @@ -1020,7 +1019,7 @@ static void n8x0_boot_init(void *opaque)
>
>     /* If the machine has a slided keyboard, open it */
>     if (s->kbd)
> -        qemu_irq_raise(omap2_gpio_in_get(s->cpu->gpif, N810_SLIDE_GPIO)[0]);
> +        qemu_irq_raise(qdev_get_gpio_in(s->cpu->gpio, N810_SLIDE_GPIO));
>  }
>
>  #define OMAP_TAG_NOKIA_BT      0x4e01
> diff --git a/hw/omap.h b/hw/omap.h
> index 339cc00..2a5b90c 100644
> --- a/hw/omap.h
> +++ b/hw/omap.h
> @@ -682,22 +682,6 @@ qemu_irq *omap_mpuio_in_get(struct omap_mpuio_s *s);
>  void omap_mpuio_out_set(struct omap_mpuio_s *s, int line, qemu_irq handler);
>  void omap_mpuio_key(struct omap_mpuio_s *s, int row, int col, int down);
>
> -/* omap1 gpio module interface */
> -struct omap_gpio_s;
> -struct omap_gpio_s *omap_gpio_init(target_phys_addr_t base,
> -                qemu_irq irq, omap_clk clk);
> -void omap_gpio_reset(struct omap_gpio_s *s);
> -qemu_irq *omap_gpio_in_get(struct omap_gpio_s *s);
> -void omap_gpio_out_set(struct omap_gpio_s *s, int line, qemu_irq handler);
> -
> -/* omap2 gpio interface */
> -struct omap_gpif_s;
> -struct omap_gpif_s *omap2_gpio_init(struct omap_target_agent_s *ta,
> -                qemu_irq *irq, omap_clk *fclk, omap_clk iclk, int modules);
> -void omap_gpif_reset(struct omap_gpif_s *s);
> -qemu_irq *omap2_gpio_in_get(struct omap_gpif_s *s, int start);
> -void omap2_gpio_out_set(struct omap_gpif_s *s, int line, qemu_irq handler);
> -
>  struct uWireSlave {
>     uint16_t (*receive)(void *opaque);
>     void (*send)(void *opaque, uint16_t data);
> @@ -851,7 +835,7 @@ struct omap_mpu_state_s {
>     /* MPUI-TIPB peripherals */
>     struct omap_uart_s *uart[3];
>
> -    struct omap_gpio_s *gpio;
> +    DeviceState *gpio;
>
>     struct omap_mcbsp_s *mcbsp1;
>     struct omap_mcbsp_s *mcbsp3;
> @@ -949,8 +933,6 @@ struct omap_mpu_state_s {
>     struct omap_gpmc_s *gpmc;
>     struct omap_sysctl_s *sysc;
>
> -    struct omap_gpif_s *gpif;
> -
>     struct omap_mcspi_s *mcspi[2];
>
>     struct omap_dss_s *dss;
> diff --git a/hw/omap1.c b/hw/omap1.c
> index 364c26f..625f7cc 100644
> --- a/hw/omap1.c
> +++ b/hw/omap1.c
> @@ -27,6 +27,7 @@
>  #include "pc.h"
>  #include "blockdev.h"
>  #include "range.h"
> +#include "sysbus.h"
>
>  /* Should signal the TCMI/GPMC */
>  uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
> @@ -3585,7 +3586,6 @@ static void omap1_mpu_reset(void *opaque)
>     omap_uart_reset(mpu->uart[2]);
>     omap_mmc_reset(mpu->mmc);
>     omap_mpuio_reset(mpu->mpuio);
> -    omap_gpio_reset(mpu->gpio);
>     omap_uwire_reset(mpu->microwire);
>     omap_pwl_reset(mpu);
>     omap_pwt_reset(mpu);
> @@ -3845,8 +3845,12 @@ struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
>                     s->irq[1][OMAP_INT_KEYBOARD], s->irq[1][OMAP_INT_MPUIO],
>                     s->wakeup, omap_findclk(s, "clk32-kHz"));
>
> -    s->gpio = omap_gpio_init(0xfffce000, s->irq[0][OMAP_INT_GPIO_BANK1],
> -                    omap_findclk(s, "arm_gpio_ck"));
> +    s->gpio = qdev_create(NULL, "omap_gpio");
> +    qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
> +    qdev_init_nofail(s->gpio);
> +    sysbus_connect_irq(sysbus_from_qdev(s->gpio), 0,
> +                       s->irq[0][OMAP_INT_GPIO_BANK1]);
> +    sysbus_mmio_map(sysbus_from_qdev(s->gpio), 0, 0xfffce000);
>
>     s->microwire = omap_uwire_init(0xfffb3000, &s->irq[1][OMAP_INT_uWireTX],
>                     s->drq[OMAP_DMA_UWIRE_TX], omap_findclk(s, "mpuper_ck"));
> diff --git a/hw/omap2.c b/hw/omap2.c
> index 0f13272..3b48696 100644
> --- a/hw/omap2.c
> +++ b/hw/omap2.c
> @@ -27,6 +27,7 @@
>  #include "qemu-char.h"
>  #include "flash.h"
>  #include "soc_dma.h"
> +#include "sysbus.h"
>  #include "audio/audio.h"
>
>  /* Enhanced Audio Controller (CODEC only) */
> @@ -2203,7 +2204,6 @@ static void omap2_mpu_reset(void *opaque)
>     omap_uart_reset(mpu->uart[1]);
>     omap_uart_reset(mpu->uart[2]);
>     omap_mmc_reset(mpu->mmc);
> -    omap_gpif_reset(mpu->gpif);
>     omap_mcspi_reset(mpu->mcspi[0]);
>     omap_mcspi_reset(mpu->mcspi[1]);
>     omap_i2c_reset(mpu->i2c[0]);
> @@ -2232,9 +2232,10 @@ struct omap_mpu_state_s *omap2420_mpu_init(unsigned long sdram_size,
>     ram_addr_t sram_base, q2_base;
>     qemu_irq *cpu_irq;
>     qemu_irq dma_irqs[4];
> -    omap_clk gpio_clks[4];
>     DriveInfo *dinfo;
>     int i;
> +    SysBusDevice *busdev;
> +    struct omap_target_agent_s *ta;
>
>     /* Core */
>     s->mpu_model = omap2420;
> @@ -2377,13 +2378,20 @@ struct omap_mpu_state_s *omap2420_mpu_init(unsigned long sdram_size,
>                     omap_findclk(s, "i2c2.fclk"),
>                     omap_findclk(s, "i2c2.iclk"));
>
> -    gpio_clks[0] = omap_findclk(s, "gpio1_dbclk");
> -    gpio_clks[1] = omap_findclk(s, "gpio2_dbclk");
> -    gpio_clks[2] = omap_findclk(s, "gpio3_dbclk");
> -    gpio_clks[3] = omap_findclk(s, "gpio4_dbclk");
> -    s->gpif = omap2_gpio_init(omap_l4ta(s->l4, 3),
> -                    &s->irq[0][OMAP_INT_24XX_GPIO_BANK1],
> -                    gpio_clks, omap_findclk(s, "gpio_iclk"), 4);
> +    s->gpio = qdev_create(NULL, "omap_gpio");
> +    qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
> +    qdev_init_nofail(s->gpio);
> +    busdev = sysbus_from_qdev(s->gpio);
> +    sysbus_connect_irq(busdev, 0, s->irq[0][OMAP_INT_24XX_GPIO_BANK1]);
> +    sysbus_connect_irq(busdev, 3, s->irq[0][OMAP_INT_24XX_GPIO_BANK2]);
> +    sysbus_connect_irq(busdev, 6, s->irq[0][OMAP_INT_24XX_GPIO_BANK3]);
> +    sysbus_connect_irq(busdev, 9, s->irq[0][OMAP_INT_24XX_GPIO_BANK4]);
> +    ta = omap_l4ta(s->l4, 3);
> +    sysbus_mmio_map(busdev, 0, omap_l4_base(ta, 1));
> +    sysbus_mmio_map(busdev, 1, omap_l4_base(ta, 0));
> +    sysbus_mmio_map(busdev, 2, omap_l4_base(ta, 2));
> +    sysbus_mmio_map(busdev, 3, omap_l4_base(ta, 4));
> +    sysbus_mmio_map(busdev, 4, omap_l4_base(ta, 5));
>
>     s->sdrc = omap_sdrc_init(0x68009000);
>     s->gpmc = omap_gpmc_init(0x6800a000, s->irq[0][OMAP_INT_24XX_GPMC_IRQ]);
> diff --git a/hw/omap_gpio.c b/hw/omap_gpio.c
> index b53b13b..410d8a6 100644
> --- a/hw/omap_gpio.c
> +++ b/hw/omap_gpio.c
> @@ -20,10 +20,10 @@
>
>  #include "hw.h"
>  #include "omap.h"
> -/* General-Purpose I/O */
> +#include "sysbus.h"
> +
>  struct omap_gpio_s {
>     qemu_irq irq;
> -    qemu_irq *in;
>     qemu_irq handler[16];
>
>     uint16_t inputs;
> @@ -35,9 +35,44 @@ struct omap_gpio_s {
>     uint16_t pins;
>  };
>
> +struct omap2_gpio_s {
> +    qemu_irq irq[2];
> +    qemu_irq wkup;
> +    qemu_irq *handler;
> +
> +    uint8_t revision;
> +    uint8_t config[2];
> +    uint32_t inputs;
> +    uint32_t outputs;
> +    uint32_t dir;
> +    uint32_t level[2];
> +    uint32_t edge[2];
> +    uint32_t mask[2];
> +    uint32_t wumask;
> +    uint32_t ints[2];
> +    uint32_t debounce;
> +    uint8_t delay;
> +};
> +
> +struct omap_gpif_s {
> +    SysBusDevice busdev;
> +    int mpu_model;
> +    union {
> +        struct omap_gpio_s omap1;
> +        struct {
> +            int modulecount;
> +            struct omap2_gpio_s *modules;
> +            qemu_irq *handler;
> +            int autoidle;
> +            int gpo;
> +        } omap2;
> +    } regs;
> +};
> +
> +/* General-Purpose I/O of OMAP1 */
>  static void omap_gpio_set(void *opaque, int line, int level)
>  {
> -    struct omap_gpio_s *s = (struct omap_gpio_s *) opaque;
> +    struct omap_gpio_s *s = &((struct omap_gpif_s *)opaque)->regs.omap1;
>     uint16_t prev = s->inputs;
>
>     if (level)
> @@ -160,7 +195,7 @@ static CPUWriteMemoryFunc * const omap_gpio_writefn[] = {
>     omap_badwidth_write16,
>  };
>
> -void omap_gpio_reset(struct omap_gpio_s *s)
> +static void omap_gpio_reset(struct omap_gpio_s *s)
>  {
>     s->inputs = 0;
>     s->outputs = ~0;
> @@ -171,58 +206,9 @@ void omap_gpio_reset(struct omap_gpio_s *s)
>     s->pins = ~0;
>  }
>
> -struct omap_gpio_s *omap_gpio_init(target_phys_addr_t base,
> -                qemu_irq irq, omap_clk clk)
> -{
> -    int iomemtype;
> -    struct omap_gpio_s *s = (struct omap_gpio_s *)
> -            qemu_mallocz(sizeof(struct omap_gpio_s));
> -
> -    s->irq = irq;
> -    s->in = qemu_allocate_irqs(omap_gpio_set, s, 16);
> -    omap_gpio_reset(s);
> -
> -    iomemtype = cpu_register_io_memory(omap_gpio_readfn,
> -                    omap_gpio_writefn, s, DEVICE_NATIVE_ENDIAN);
> -    cpu_register_physical_memory(base, 0x1000, iomemtype);
> -
> -    return s;
> -}
> -
> -qemu_irq *omap_gpio_in_get(struct omap_gpio_s *s)
> -{
> -    return s->in;
> -}
> -
> -void omap_gpio_out_set(struct omap_gpio_s *s, int line, qemu_irq handler)
> -{
> -    if (line >= 16 || line < 0)
> -        hw_error("%s: No GPIO line %i\n", __FUNCTION__, line);
> -    s->handler[line] = handler;
> -}
> -
> -/* General-Purpose Interface of OMAP2 */
> -struct omap2_gpio_s {
> -    qemu_irq irq[2];
> -    qemu_irq wkup;
> -    qemu_irq *in;
> -    qemu_irq handler[32];
> -
> -    uint8_t config[2];
> -    uint32_t inputs;
> -    uint32_t outputs;
> -    uint32_t dir;
> -    uint32_t level[2];
> -    uint32_t edge[2];
> -    uint32_t mask[2];
> -    uint32_t wumask;
> -    uint32_t ints[2];
> -    uint32_t debounce;
> -    uint8_t delay;
> -};
> -
> +/* General-Purpose Interface of OMAP2/3 */
>  static inline void omap2_gpio_module_int_update(struct omap2_gpio_s *s,
> -                int line)
> +                                                int line)
>  {
>     qemu_set_irq(s->irq[line], s->ints[line] & s->mask[line]);
>  }
> @@ -271,8 +257,9 @@ static inline void omap2_gpio_module_int(struct omap2_gpio_s *s, int line)
>
>  static void omap2_gpio_module_set(void *opaque, int line, int level)
>  {
> -    struct omap2_gpio_s *s = (struct omap2_gpio_s *) opaque;
> -
> +    struct omap_gpif_s *p = opaque;
> +    struct omap2_gpio_s *s = &p->regs.omap2.modules[line >> 5];
> +    line &= 31;

Patch looks good overall, but for consistency we should rename
functions which start with omap2_gpio_module_ to omap2_gpio_ if the
state pointer passed is no longer the module pointer but instead the
whole thing pointer.  But maybe it would make more sense for each
module to be a device?  It looks like vmsaving/vmloading the union
structure may be problematic and we can also save some cycles.

Similarly omap_l4_base would be better named omap_l4_region_base or
similar.  I'd also prefer that omap2_gpio_init remains a function that
does all the qdev magic in omap_gpio.c and with the current signature
(so for example clocks usage can be kept track of).  Would it make
more sense to pass the ta structures instead of base adresses to the
qdev?  Have you considered how difficult l4 would be to convert to a
QBus?

Cheers

Re: [Qemu-devel] [PATCH 3/3] hw/omap_gpio.c: Convert to qdev

[Peter Maydell]

On 4 July 2011 23:39, andrzej zaborowski <balrogg@gmail.com> wrote:
> Patch looks good overall, but for consistency we should rename
> functions which start with omap2_gpio_module_ to omap2_gpio_ if the
> state pointer passed is no longer the module pointer but instead the
> whole thing pointer.  But maybe it would make more sense for each
> module to be a device?  It looks like vmsaving/vmloading the union
> structure may be problematic and we can also save some cycles.

Yes, I think you're right and we should just have two separate
devices rather than one with a union.

> Similarly omap_l4_base would be better named omap_l4_region_base or
> similar.

Happy to change this, I'm not very attached to the name.

> I'd also prefer that omap2_gpio_init remains a function that
> does all the qdev magic in omap_gpio.c and with the current signature

I'm not convinced about this. Implementing the GPIO module as
a sysbus device seems like the right thing, and it's the job
of omap2.c to know how to map and wire the resources the sysbus
device provides. Hiding that in a helper function in omap_gpio.c
doesn't seem quite right.

> (so for example clocks usage can be kept track of).

You're right, we probably should do something with the clocks (even
though omap_gpio doesn't use them). We'd probably wind up with a
'wire up clocks' function per device type though, unless we want to
try to make all omap devices inherit from a common thing that knows
about clocks (so we could have an omap_connect_clk() function like
sysbus_connect_irq()). [This is about the only thing inclining
me in favour of an _init routine. I guess you touch on the idea
of an omap-specific bus below.]

PS: on the subject of clocks, this is how a later qdevification patch
in my stack does the wiring up of the uart:

    s->uart[0] = qdev_create(NULL, "omap_uart");
    s->uart[0]->id = "uart1";
    qdev_prop_set_uint32(s->uart[0], "mmio_size", 0x1000);
    qdev_prop_set_uint32(s->uart[0], "baudrate",
                         omap_clk_getrate(omap_findclk(s, "uart1_fclk")) / 16);
    qdev_prop_set_chr(s->uart[0], "chardev", serial_hds[0]);
    qdev_init_nofail(s->uart[0]);
    busdev = sysbus_from_qdev(s->uart[0]);
    sysbus_connect_irq(busdev, 0, s->irq[0][OMAP_INT_24XX_UART1_IRQ]);
    sysbus_connect_irq(busdev, 1, s->drq[OMAP24XX_DMA_UART1_TX]);
    sysbus_connect_irq(busdev, 2, s->drq[OMAP24XX_DMA_UART1_RX]);
    sysbus_mmio_map(busdev, 0, omap_l4_base(omap_l4ta(s->l4, 19), 0));

...it would be useful to know if you're going to object to how it's
dealing with the clock there so I can fix it in advance and avoid
a round of review.

> Would it make more sense to pass the ta structures instead of base
> adresses to the qdev? Have you considered how difficult l4 would be
> to convert to a QBus?

Hmm. I hadn't thought about that. Does it buy us anything useful?
I'm not sure how it would interact with wanting to be able to
put plain-old-sysbus devices onto the bus (eg the OHCI USB).

Thanks for the review.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/omap_gpio.c: Convert to qdev

[andrzej zaborowski]

Hi,

On 5 July 2011 15:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 July 2011 23:39, andrzej zaborowski <balrogg@gmail.com> wrote:
>> I'd also prefer that omap2_gpio_init remains a function that
>> does all the qdev magic in omap_gpio.c and with the current signature
>
> I'm not convinced about this. Implementing the GPIO module as
> a sysbus device seems like the right thing, and it's the job
> of omap2.c to know how to map and wire the resources the sysbus
> device provides. Hiding that in a helper function in omap_gpio.c
> doesn't seem quite right.

Good point.

>
>> (so for example clocks usage can be kept track of).
>
> You're right, we probably should do something with the clocks (even
> though omap_gpio doesn't use them). We'd probably wind up with a
> 'wire up clocks' function per device type though, unless we want to
> try to make all omap devices inherit from a common thing that knows
> about clocks (so we could have an omap_connect_clk() function like
> sysbus_connect_irq()). [This is about the only thing inclining
> me in favour of an _init routine. I guess you touch on the idea
> of an omap-specific bus below.]
>
> PS: on the subject of clocks, this is how a later qdevification patch
> in my stack does the wiring up of the uart:
>
>    s->uart[0] = qdev_create(NULL, "omap_uart");
>    s->uart[0]->id = "uart1";
>    qdev_prop_set_uint32(s->uart[0], "mmio_size", 0x1000);
>    qdev_prop_set_uint32(s->uart[0], "baudrate",
>                         omap_clk_getrate(omap_findclk(s, "uart1_fclk")) / 16);
>    qdev_prop_set_chr(s->uart[0], "chardev", serial_hds[0]);
>    qdev_init_nofail(s->uart[0]);
>    busdev = sysbus_from_qdev(s->uart[0]);
>    sysbus_connect_irq(busdev, 0, s->irq[0][OMAP_INT_24XX_UART1_IRQ]);
>    sysbus_connect_irq(busdev, 1, s->drq[OMAP24XX_DMA_UART1_TX]);
>    sysbus_connect_irq(busdev, 2, s->drq[OMAP24XX_DMA_UART1_RX]);
>    sysbus_mmio_map(busdev, 0, omap_l4_base(omap_l4ta(s->l4, 19), 0));
>
> ...it would be useful to know if you're going to object to how it's
> dealing with the clock there so I can fix it in advance and avoid
> a round of review.

Other than the clock and the address passing it looks ok.  Optimally
we should pass the clock as a pointer or name, not only the baudrate
(clocks can be stopped, reparented, or have frequency changed
dynamically -- in fact the OS will most likely manipulate the uart
source clock frequency during init) and the TA region as such instead
of only the base.  Maybe let's have a short omap_dev_connect_clock
function that would use or alias prop_set_ptr or something like that
(and a similar thing for the target agent).

>
>> Would it make more sense to pass the ta structures instead of base
>> adresses to the qdev? Have you considered how difficult l4 would be
>> to convert to a QBus?
>
> Hmm. I hadn't thought about that. Does it buy us anything useful?

Probably not if we can pass the resources to qdev in a nice way.

Cheers

Re: [Qemu-devel] [PATCH 3/3] hw/omap_gpio.c: Convert to qdev

[Peter Maydell]

On 7 July 2011 16:29, andrzej zaborowski <balrogg@gmail.com> wrote:
> Optimally
> we should pass the clock as a pointer or name, not only the baudrate
> (clocks can be stopped, reparented, or have frequency changed
> dynamically -- in fact the OS will most likely manipulate the uart
> source clock frequency during init) and the TA region as such instead
> of only the base.  Maybe let's have a short omap_dev_connect_clock
> function that would use or alias prop_set_ptr or something like that
> (and a similar thing for the target agent).

How about we just have the devices define string properties for the
clocks? (and then the device initfn would look the clock up with
omap_findclk()). That seems more useful as a user-visible property
than a raw pointer. Something like:

   s->uart[0] = qdev_create(NULL, "omap_uart");
   s->uart[0]->id = "uart1";
   qdev_prop_set_uint32(s->uart[0], "mmio_size", 0x1000);
   qdev_prop_set_string(s->uart[0], "clock", "uart1_fclk"));
   qdev_prop_set_chr(s->uart[0], "chardev", serial_hds[0]);
   qdev_init_nofail(s->uart[0]);

-- PMM