Hi Guenter,
On Fri, Feb 7, 2020 at 6:46 PM Guenter Roeck <li...@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 <c...@kaod.org>
> Signed-off-by: Guenter Roeck <li...@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.
> + 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?
> sc->wdts_num = 4;
> sc->macs_num = 4;
> sc->irqmap = aspeed_soc_ast2600_irqmap;
> --
> 2.17.1
>
>
>
Rest looks good to me.
Regards,
Niek
--
Niek Linnenbank
