Hi, On 5 July 2011 15:19, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 4 July 2011 23:39, andrzej zaborowski <balr...@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