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

[andrzej zaborowski]

Hi,

On 5 July 2011 15:19, Peter Maydell <address@hidden> wrote:
> On 4 July 2011 23:39, andrzej zaborowski <address@hidden> 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