Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On Fri, 7 Feb 2020 at 17:45, Guenter Roeck 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 > Signed-off-by: Guenter Roeck Applied to target-arm.next, thanks. -- PMM
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On Tue, Feb 11, 2020 at 07:13:48PM +, Peter Maydell wrote: > On Tue, 11 Feb 2020 at 08:12, Philippe Mathieu-Daudé > wrote: > > > > On 2/7/20 11:48 PM, Guenter Roeck wrote: > > > On Fri, Feb 07, 2020 at 02:04:09PM -0800, no-re...@patchew.org wrote: > > >> Patchew URL: > > >> https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/ > > >> > > >> > > >> > > >> Hi, > > >> > > >> This series failed the docker-mingw@fedora build test. Please find the > > >> testing commands and > > >> their output below. If you have Docker installed, you can probably > > >> reproduce it > > >> locally. > > >> > > > I forgot to mention that the patch depends on the similar > > > patch for ast2400/ast2500. Sorry for that. Not sure though how > > > to tell that to the test build system. > > > > You mean the "Aspeed: machine extensions and fixes" series? > > Seems unlikely given that series is from 2019 and went into > master last year... Probably Guenter means > "ast2400/ast2500: Wire up EHCI controllers" > https://patchew.org/QEMU/20200206183437.3979-1-li...@roeck-us.net/ > > (which I had dropped from my to-review queue because I'd > misunderstood Cedric's review and was expecting to see > a v2 of that which covered all of ast2400/2500/2600 -- > I'll put it back on my list to queue before this one) > Yes, that is correct; the patch for ast2600 depends on the patch for ast2400/ast2500. Please let me know if I need to resend both. Thanks, Guenter
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On Tue, 11 Feb 2020 at 08:12, Philippe Mathieu-Daudé wrote: > > On 2/7/20 11:48 PM, Guenter Roeck wrote: > > On Fri, Feb 07, 2020 at 02:04:09PM -0800, no-re...@patchew.org wrote: > >> Patchew URL: > >> https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/ > >> > >> > >> > >> Hi, > >> > >> This series failed the docker-mingw@fedora build test. Please find the > >> testing commands and > >> their output below. If you have Docker installed, you can probably > >> reproduce it > >> locally. > >> > > I forgot to mention that the patch depends on the similar > > patch for ast2400/ast2500. Sorry for that. Not sure though how > > to tell that to the test build system. > > You mean the "Aspeed: machine extensions and fixes" series? Seems unlikely given that series is from 2019 and went into master last year... Probably Guenter means "ast2400/ast2500: Wire up EHCI controllers" https://patchew.org/QEMU/20200206183437.3979-1-li...@roeck-us.net/ (which I had dropped from my to-review queue because I'd misunderstood Cedric's review and was expecting to see a v2 of that which covered all of ast2400/2500/2600 -- I'll put it back on my list to queue before this one) thanks -- PMM
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On 2/7/20 11:48 PM, Guenter Roeck wrote: On Fri, Feb 07, 2020 at 02:04:09PM -0800, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. I forgot to mention that the patch depends on the similar patch for ast2400/ast2500. Sorry for that. Not sure though how to tell that to the test build system. You mean the "Aspeed: machine extensions and fixes" series? Use the 'based-on' tag with the series cover message-id in your cover/patch: Based-on: <20190904070506.1052-1-...@kaod.org>
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
Patchew URL: https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/gdbstub-xml.o
CC aarch64-softmmu/target/arm/translate.o
CC aarch64-softmmu/trace/generated-helpers.o
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: error: 'ASPEED_EHCI1'
undeclared here (not in a function)
[ASPEED_EHCI1] = 0x1E6A1000,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:5: error: array index in
initializer not of integer type
[ASPEED_EHCI1] = 0x1E6A1000,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:5: error: (near initialization
for 'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: error: 'ASPEED_EHCI2'
undeclared here (not in a function)
[ASPEED_EHCI2] = 0x1E6A3000,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:5: error: array index in
initializer not of integer type
[ASPEED_EHCI2] = 0x1E6A3000,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:5: error: (near initialization
for 'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:5: error: array index in
initializer not of integer type
[ASPEED_EHCI1] = 5,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:5: error: (near initialization
for 'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:5: error: array index in
initializer not of integer type
[ASPEED_EHCI2] = 9,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:5: error: (near initialization
for 'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_init':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:173:23: error: 'AspeedSoCClass' has
no member named 'ehcis_num'
for (i = 0; i < sc->ehcis_num; i++) {
^
In file included from /tmp/qemu-test/src/include/hw/qdev-core.h:6:0,
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:174:56: error: 'AspeedSoCState' has
no member named 'ehci'
sysbus_init_child_obj(obj, "ehci[*]", OBJECT(&s->ehci[i]),
^
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:39: error: 'AspeedSoCState' has
no member named 'ehci'
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: error: 'TYPE_PLATFORM_EHCI'
undeclared (first use in this function)
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: note: each undeclared
identifier is reported only once for each function it appears in
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_realize':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:429:23: error: 'AspeedSoCClass' has
no member named 'ehcis_num'
for (i = 0; i < sc->ehcis_num; i++) {
^
In file included from /tmp/qemu-test/src/include/hw/qdev-core.h:6:0,
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:430:43: error: 'AspeedSoCState' has
no member named 'ehci'
object_property_set_bool(OBJECT(&s->ehci[i]), true, "realized", &err);
^
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:435:42: error: 'AspeedSoCState' has
no member named 'ehci'
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
^
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
---
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:435:25: note: in expansion of macro
'SYS_BUS_DEVICE'
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
^
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:437:45: error: 'AspeedSoCState' has
no member named 'ehci'
sysbus_connect_irq(SYS_BUS_DEVICE(&s->ehci[i]), 0,
^
/tmp/qemu-test/src/include/
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
Patchew URL: https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/target/arm/translate.o
CC aarch64-softmmu/trace/generated-helpers.o
CC aarch64-softmmu/target/arm/translate-sve.o
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: error: 'ASPEED_EHCI1'
undeclared here (not in a function); did you mean 'ASPEED_MII1'?
[ASPEED_EHCI1] = 0x1E6A1000,
^~~~
ASPEED_MII1
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: error: array index in
initializer not of integer type
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: note: (near initialization for
'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: error: 'ASPEED_EHCI2'
undeclared here (not in a function); did you mean 'ASPEED_MII2'?
[ASPEED_EHCI2] = 0x1E6A3000,
^~~~
ASPEED_MII2
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: error: array index in
initializer not of integer type
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: note: (near initialization for
'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:6: error: array index in
initializer not of integer type
[ASPEED_EHCI1] = 5,
^~~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:6: note: (near initialization for
'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:6: error: array index in
initializer not of integer type
[ASPEED_EHCI2] = 9,
^~~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:6: note: (near initialization for
'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_init':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:173:25: error: 'AspeedSoCClass' {aka
'struct AspeedSoCClass'} has no member named 'ehcis_num'; did you mean
'spis_num'?
for (i = 0; i < sc->ehcis_num; i++) {
^
spis_num
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:174:58: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sysbus_init_child_obj(obj, "ehci[*]", OBJECT(&s->ehci[i]),
^~~~
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:41: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^~~~
sdhci
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: error: 'TYPE_PLATFORM_EHCI'
undeclared (first use in this function); did you mean 'POWER_PLATFORM_ROLE'?
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^~
POWER_PLATFORM_ROLE
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: note: each undeclared
identifier is reported only once for each function it appears in
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_realize':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:429:25: error: 'AspeedSoCClass' {aka
'struct AspeedSoCClass'} has no member named 'ehcis_num'; did you mean
'spis_num'?
for (i = 0; i < sc->ehcis_num; i++) {
^
spis_num
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:430:45: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
object_property_set_bool(OBJECT(&s->ehci[i]), true, "realized", &err);
^~~~
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:435:44: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On Fri, Feb 07, 2020 at 02:04:09PM -0800, no-re...@patchew.org wrote: > Patchew URL: > https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/ > > > > Hi, > > This series failed the docker-mingw@fedora build test. Please find the > testing commands and > their output below. If you have Docker installed, you can probably reproduce > it > locally. > I forgot to mention that the patch depends on the similar patch for ast2400/ast2500. Sorry for that. Not sure though how to tell that to the test build system. Guenter
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
On Fri, Feb 7, 2020, 22:34 Guenter Roeck wrote:
> 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 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
> > > Signed-off-by: Guenter Roeck
> > > ---
> > > 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] = 0x1E62,
> > > [ASPEED_SPI1] = 0x1E63,
> > > [ASPEED_SPI2] = 0x1E641000,
> > > +[ASPEED_EHCI1] = 0x1E6A1000,
> > > +[ASPEED_EHCI2] = 0x1E6A3000,
> > > [ASPEED_MII1] = 0x1E65,
> > > [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.
>
I see. There are some uses of error_fatal already in the function, but
improving that might be something for another patch I guess.
>
> >
> > > +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= 0x1;
> > > 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.
>
Ignore my comment here, i did not see this patch was part of previous work
done for the other AST socs as well.
Reviewed-by: Niek Linnenbank
>
> Guenter
>
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
Patchew URL: https://patchew.org/QEMU/20200207174548.9087-1-li...@roeck-us.net/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/target/arm/translate.o
CC aarch64-softmmu/trace/generated-helpers.o
CC aarch64-softmmu/target/arm/translate-sve.o
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: error: 'ASPEED_EHCI1'
undeclared here (not in a function); did you mean 'ASPEED_MII1'?
[ASPEED_EHCI1] = 0x1E6A1000,
^~~~
ASPEED_MII1
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: error: array index in
initializer not of integer type
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:34:6: note: (near initialization for
'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: error: 'ASPEED_EHCI2'
undeclared here (not in a function); did you mean 'ASPEED_MII2'?
[ASPEED_EHCI2] = 0x1E6A3000,
^~~~
ASPEED_MII2
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: error: array index in
initializer not of integer type
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:35:6: note: (near initialization for
'aspeed_soc_ast2600_memmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:6: error: array index in
initializer not of integer type
[ASPEED_EHCI1] = 5,
^~~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:84:6: note: (near initialization for
'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:6: error: array index in
initializer not of integer type
[ASPEED_EHCI2] = 9,
^~~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:85:6: note: (near initialization for
'aspeed_soc_ast2600_irqmap')
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_init':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:173:25: error: 'AspeedSoCClass' {aka
'struct AspeedSoCClass'} has no member named 'ehcis_num'; did you mean
'spis_num'?
for (i = 0; i < sc->ehcis_num; i++) {
^
spis_num
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:174:58: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sysbus_init_child_obj(obj, "ehci[*]", OBJECT(&s->ehci[i]),
^~~~
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:41: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^~~~
sdhci
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: error: 'TYPE_PLATFORM_EHCI'
undeclared (first use in this function); did you mean 'POWER_PLATFORM_ROLE'?
sizeof(s->ehci[i]), TYPE_PLATFORM_EHCI);
^~
POWER_PLATFORM_ROLE
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:175:51: note: each undeclared
identifier is reported only once for each function it appears in
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c: In function
'aspeed_soc_ast2600_realize':
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:429:25: error: 'AspeedSoCClass' {aka
'struct AspeedSoCClass'} has no member named 'ehcis_num'; did you mean
'spis_num'?
for (i = 0; i < sc->ehcis_num; i++) {
^
spis_num
---
from /tmp/qemu-test/src/target/arm/cpu-qom.h:23,
from /tmp/qemu-test/src/target/arm/cpu.h:25,
from /tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:12:
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:430:45: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
object_property_set_bool(OBJECT(&s->ehci[i]), true, "realized", &err);
^~~~
/tmp/qemu-test/src/include/qom/object.h:511:17: note: in definition of macro
'OBJECT'
((Object *)(obj))
^~~
/tmp/qemu-test/src/hw/arm/aspeed_ast2600.c:435:44: error: 'AspeedSoCState' {aka
'struct AspeedSoCState'} has no member named 'ehci'; did you mean 'sdhci'?
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
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 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
> > Signed-off-by: Guenter Roeck
> > ---
> > 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] = 0x1E62,
> > [ASPEED_SPI1] = 0x1E63,
> > [ASPEED_SPI2] = 0x1E641000,
> > +[ASPEED_EHCI1] = 0x1E6A1000,
> > +[ASPEED_EHCI2] = 0x1E6A3000,
> > [ASPEED_MII1] = 0x1E65,
> > [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= 0x1;
> > 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
Re: [PATCH v2] hw/arm: ast2600: Wire up EHCI controllers
Hi Guenter,
On Fri, Feb 7, 2020 at 6:46 PM Guenter Roeck 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
> Signed-off-by: Guenter Roeck
> ---
> 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] = 0x1E62,
> [ASPEED_SPI1] = 0x1E63,
> [ASPEED_SPI2] = 0x1E641000,
> +[ASPEED_EHCI1] = 0x1E6A1000,
> +[ASPEED_EHCI2] = 0x1E6A3000,
> [ASPEED_MII1] = 0x1E65,
> [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= 0x1;
> 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
