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

[Peter Maydell]

On 4 July 2011 23:39, andrzej zaborowski <address@hidden> 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