Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Feb 07, 2020 at 10:25:01PM +0100, Niek Linnenbank wrote:
> Hi Guenter,
> 
> On Fri, Feb 7, 2020 at 6:46 PM Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Initialize EHCI controllers on AST2600 using the existing
> > TYPE_PLATFORM_EHCI. After this change, booting ast2600-evb
> > into Linux successfully instantiates a USB interface after
> > the necessary changes are made to its devicetree files.
> >
> > ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
> > ehci-platform: EHCI generic platform driver
> > ehci-platform 1e6a3000.usb: EHCI Host Controller
> > ehci-platform 1e6a3000.usb: new USB bus registered, assigned bus number 1
> > ehci-platform 1e6a3000.usb: irq 25, io mem 0x1e6a3000
> > ehci-platform 1e6a3000.usb: USB 2.0 started, EHCI 1.00
> > usb usb1: Manufacturer: Linux 5.5.0-09825-ga0802f2d0ef5-dirty ehci_hcd
> > usb 1-1: new high-speed USB device number 2 using ehci-platform
> >
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v2: Rebased to master (to fix context conflict)
> >     Added Reviewed-by: tag
> >
> >  hw/arm/aspeed_ast2600.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 90cf1c755d..446b44d31c 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -31,6 +31,8 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> >      [ASPEED_FMC]       = 0x1E620000,
> >      [ASPEED_SPI1]      = 0x1E630000,
> >      [ASPEED_SPI2]      = 0x1E641000,
> > +    [ASPEED_EHCI1]     = 0x1E6A1000,
> > +    [ASPEED_EHCI2]     = 0x1E6A3000,
> >      [ASPEED_MII1]      = 0x1E650000,
> >      [ASPEED_MII2]      = 0x1E650008,
> >      [ASPEED_MII3]      = 0x1E650010,
> > @@ -79,6 +81,8 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> >      [ASPEED_ADC]       = 78,
> >      [ASPEED_XDMA]      = 6,
> >      [ASPEED_SDHCI]     = 43,
> > +    [ASPEED_EHCI1]     = 5,
> > +    [ASPEED_EHCI2]     = 9,
> >      [ASPEED_EMMC]      = 15,
> >      [ASPEED_GPIO]      = 40,
> >      [ASPEED_GPIO_1_8V] = 11,
> > @@ -166,6 +170,11 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >                                sizeof(s->spi[i]), typename);
> >      }
> >
> > +    for (i = 0; i < sc->ehcis_num; i++) {
> > +        sysbus_init_child_obj(obj, "ehci[*]", OBJECT(&s->ehci[i]),
> > +                              sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
> > +    }
> > +
> >      snprintf(typename, sizeof(typename), "aspeed.sdmc-%s", socname);
> >      sysbus_init_child_obj(obj, "sdmc", OBJECT(&s->sdmc), sizeof(s->sdmc),
> >                            typename);
> > @@ -416,6 +425,19 @@ static void aspeed_soc_ast2600_realize(DeviceState
> > *dev, Error **errp)
> >                          s->spi[i].ctrl->flash_window_base);
> >      }
> >
> > +    /* EHCI */
> > +    for (i = 0; i < sc->ehcis_num; i++) {
> > +        object_property_set_bool(OBJECT(&s->ehci[i]), true, "realized",
> > &err);
> > +        if (err) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> >
> 
> Would it make sense to directly use error_fatal in the call to
> object_property_set_bool?
> That way you can avoid the additional code for propagating the error.
> 

The code matches the pattern used in the rest of the function.
Given that, I would be hesitant to change it for this one case.

> 
> > +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
> > +                        sc->memmap[ASPEED_EHCI1 + i]);
> > +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->ehci[i]), 0,
> > +                           aspeed_soc_get_irq(s, ASPEED_EHCI1 + i));
> > +    }
> > +
> >      /* SDMC - SDRAM Memory Controller */
> >      object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
> >      if (err) {
> > @@ -534,6 +556,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass
> > *oc, void *data)
> >      sc->silicon_rev  = AST2600_A0_SILICON_REV;
> >      sc->sram_size    = 0x10000;
> >      sc->spis_num     = 2;
> > +    sc->ehcis_num    = 2;
> >
> 
> Since this field is only set once here, does it need to be part of the
> class state?
> 

The same applies to spis_num, wdts_num, and macs_num.
AspeedSoCClass is defined for all ast2X00 SoCs, and
the same mechanism is used for all of them. I don't see
the benefit of deviating from a common mechanism.

Guenter