* [Qemu-devel] [PATCH 2/2] exynos4210: Add ehci support
2012-12-01 15:27 [Qemu-devel] [PATCH 1/2] usb/ehci: Refactor ehci-sysbus to add exynos4210 ehci support Liming Wang
@ 2012-12-01 15:27 ` Liming Wang
2012-12-01 19:47 ` [Qemu-devel] [PATCH 1/2] usb/ehci: Refactor ehci-sysbus to add exynos4210 " Andreas Färber
1 sibling, 0 replies; 3+ messages in thread
From: Liming Wang @ 2012-12-01 15:27 UTC (permalink / raw)
To: qemu-devel, Gerd Hoffmann, Peter Crosthwaite, Mitsyanko Igor
Cc: Peter Maydell
Add ehci host controller to exynos4210.
Signed-off-by: Liming Wang <walimisdev@gmail.com>
---
hw/exynos4210.c | 6 ++++++
hw/exynos4210_gic.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index 00d4db8..e0913a5 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -72,6 +72,9 @@
/* Display controllers (FIMD) */
#define EXYNOS4210_FIMD0_BASE_ADDR 0x11C00000
+/* EHCI */
+#define EXYNOS4210_EHCI_BASE_ADDR 0x12580000
+
static uint8_t chipid_and_omr[] = { 0x11, 0x02, 0x21, 0x43,
0x09, 0x00, 0x00, 0x00 };
@@ -334,5 +337,8 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
s->irq_table[exynos4210_get_irq(11, 2)],
NULL);
+ sysbus_create_simple("exynos4210.usb", EXYNOS4210_EHCI_BASE_ADDR,
+ s->irq_table[exynos4210_get_irq(28, 3)]);
+
return s;
}
diff --git a/hw/exynos4210_gic.c b/hw/exynos4210_gic.c
index 4fea098..959de56 100644
--- a/hw/exynos4210_gic.c
+++ b/hw/exynos4210_gic.c
@@ -140,7 +140,7 @@ combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
EXT_GIC_ID_I2C4, EXT_GIC_ID_I2C5, EXT_GIC_ID_I2C6,
EXT_GIC_ID_I2C7 },
/* int combiner group 28 */
- { EXT_GIC_ID_SPI0, EXT_GIC_ID_SPI1, EXT_GIC_ID_SPI2 },
+ { EXT_GIC_ID_SPI0, EXT_GIC_ID_SPI1, EXT_GIC_ID_SPI2 , EXT_GIC_ID_USB_HOST},
/* int combiner group 29 */
{ EXT_GIC_ID_HSMMC0, EXT_GIC_ID_HSMMC1, EXT_GIC_ID_HSMMC2,
EXT_GIC_ID_HSMMC3, EXT_GIC_ID_SDMMC },
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] usb/ehci: Refactor ehci-sysbus to add exynos4210 ehci support
2012-12-01 15:27 [Qemu-devel] [PATCH 1/2] usb/ehci: Refactor ehci-sysbus to add exynos4210 ehci support Liming Wang
2012-12-01 15:27 ` [Qemu-devel] [PATCH 2/2] exynos4210: Add " Liming Wang
@ 2012-12-01 19:47 ` Andreas Färber
1 sibling, 0 replies; 3+ messages in thread
From: Andreas Färber @ 2012-12-01 19:47 UTC (permalink / raw)
To: Liming Wang
Cc: Peter Crosthwaite, Peter Maydell, Mitsyanko Igor, qemu-devel,
Gerd Hoffmann
Am 01.12.2012 16:27, schrieb Liming Wang:
> Refactor ehci-sysbus to accept different capsbase and opregbase.
> And then add exynos4210 ehci support to ehci-sysbus.
>
> Signed-off-by: Liming Wang <walimisdev@gmail.com>
> ---
> hw/usb/hcd-ehci-sysbus.c | 48 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 803df92..f4f3ffe 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -23,6 +23,11 @@ typedef struct EHCISysBusState {
> EHCIState ehci;
> } EHCISysBusState;
>
> +typedef struct EHCISysBusClass {
> + SysBusDeviceClass sdc;
> + EHCIState ehci;
> +} EHCISysBusClass;
This is still not the right way to do this. And please keep me CC'ed.
FWIW sdc should be named parent_class and separated from normal fields.
> +
> static const VMStateDescription vmstate_ehci_sysbus = {
> .name = "ehci-sysbus",
> .version_id = 2,
> @@ -41,10 +46,11 @@ static Property ehci_sysbus_properties[] = {
> static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
> {
> EHCISysBusState *i = FROM_SYSBUS(EHCISysBusState, dev);
> + EHCISysBusClass *c = (EHCISysBusClass *)object_get_class(OBJECT(dev));
If you add a class, you also need to add a matching cast macro (and in
this case TypeInfo) and use that. I'm not aware of any other example
that does it like this, otherwise please point it out so that it can be
fixed.
> EHCIState *s = &i->ehci;
>
> - s->capsbase = 0x100;
> - s->opregbase = 0x140;
> + s->capsbase = c->ehci.capsbase;
> + s->opregbase = c->ehci.opregbase;
> s->dma = &dma_context_memory;
>
> usb_ehci_initfn(s, DEVICE(dev));
> @@ -56,23 +62,45 @@ static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
> static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> - SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> + EHCISysBusClass *k = (EHCISysBusClass *)klass;
> + EHCISysBusClass *c = (EHCISysBusClass *)data;
>
> - k->init = usb_ehci_sysbus_initfn;
> + k->sdc.init = usb_ehci_sysbus_initfn;
Never access the parent field directly, use a cast macro.
> + k->ehci = c->ehci;
> dc->vmsd = &vmstate_ehci_sysbus;
> dc->props = ehci_sysbus_properties;
> }
>
> -TypeInfo ehci_xlnx_type_info = {
> - .name = "xlnx,ps7-usb",
> - .parent = TYPE_SYS_BUS_DEVICE,
> - .instance_size = sizeof(EHCISysBusState),
> - .class_init = ehci_sysbus_class_init,
> +static TypeInfo ehci_sysbus_type_info[] = {
> + {
> + .name = "xlnx,ps7-usb",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(EHCISysBusState),
> + .class_init = ehci_sysbus_class_init,
> + .class_size = sizeof(EHCISysBusClass),
> + .class_data = (EHCISysBusClass[]) {{
> + .ehci.capsbase = 0x100,
> + .ehci.opregbase = 0x140,
> + } }
Neither is this. As discussed before, this is misusing QOM concepts. The
instance state has no business being stored in the class, and the class
shouldn't be duplicated to initialize itself, even less so when it's
only about two integer values.
> + }, {
> + .name = "exynos4210.usb",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(EHCISysBusState),
> + .class_init = ehci_sysbus_class_init,
> + .class_size = sizeof(EHCISysBusClass),
> + .class_data = (EHCISysBusClass[]) {{
> + .ehci.capsbase = 0x0,
> + .ehci.opregbase = 0x40,
> + } }
> + },
> };
>
> static void ehci_sysbus_register_types(void)
> {
> - type_register_static(&ehci_xlnx_type_info);
> + int i;
> + for (i = 0; i < ARRAY_SIZE(ehci_sysbus_type_info); i++) {
> + type_register_static(&ehci_sysbus_type_info[i]);
If a loop becomes necessary, this is indicating that something is in the
odds, complicating things. As indicated to Gerd at KVM Forum, I didn't
get around to sending out a cleanup yet (and have been busy since):
We need an abstract intermediate type whose class specifies only the
needed fields (capsbase, opregbase), has cast macros to access them and
stores the majority of TypeInfo cruft that you are duplicating here. The
specific types (xlnx, exynos, tegra) would then add their own class_init
on top to set/override those two values, and the base class'
instance_init would copy them over. I'll try to find some spare minutes
tomorrow.
Regards,
Andreas
> + }
> }
>
> type_init(ehci_sysbus_register_types)
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 3+ messages in thread