Re: [PATCH v4 04/20] hw/arm/allwinner-h3: add USB host controller

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/2/20 8:33 PM, Niek Linnenbank wrote:
> On Sun, Jan 19, 2020 at 7:44 PM Philippe Mathieu-Daudé 
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     On 1/19/20 7:37 PM, Philippe Mathieu-Daudé wrote:
>      > On 1/19/20 1:50 AM, Niek Linnenbank wrote:
>      >> The Allwinner H3 System on Chip contains multiple USB 2.0 bus
>      >> connections which provide software access using the Enhanced
>      >> Host Controller Interface (EHCI) and Open Host Controller
>      >> Interface (OHCI) interfaces. This commit adds support for
>      >> both interfaces in the Allwinner H3 System on Chip.
>      >>
>      >> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com
>     <mailto:nieklinnenbank@gmail.com>>
>      >> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com
>     <mailto:kraxel@redhat.com>>
>      >> ---
>      >>   hw/usb/hcd-ehci.h             |  1 +
>      >>   include/hw/arm/allwinner-h3.h |  8 ++++++
>      >>   hw/arm/allwinner-h3.c         | 52
>     ++++++++++++++++++++++++++++++++---
>      >>   hw/usb/hcd-ehci-sysbus.c      | 17 ++++++++++++
>      >>   4 files changed, 74 insertions(+), 4 deletions(-)
>      >>
>      >> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
>      >> index 0298238f0b..edb59311c4 100644
>      >> --- a/hw/usb/hcd-ehci.h
>      >> +++ b/hw/usb/hcd-ehci.h
>      >> @@ -342,6 +342,7 @@ typedef struct EHCIPCIState {
>      >>   #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
>      >>   #define TYPE_PLATFORM_EHCI "platform-ehci-usb"
>      >>   #define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
>      >> +#define TYPE_AW_H3_EHCI "aw-h3-ehci-usb"
>      >>   #define TYPE_TEGRA2_EHCI "tegra2-ehci-usb"
>      >>   #define TYPE_PPC4xx_EHCI "ppc4xx-ehci-usb"
>      >>   #define TYPE_FUSBH200_EHCI "fusbh200-ehci-usb"
>      >> diff --git a/include/hw/arm/allwinner-h3.h
>      >> b/include/hw/arm/allwinner-h3.h
>      >> index abdc20871a..4f4dcbcd17 100644
>      >> --- a/include/hw/arm/allwinner-h3.h
>      >> +++ b/include/hw/arm/allwinner-h3.h
>      >> @@ -56,6 +56,14 @@ enum {
>      >>       AW_H3_SRAM_A1,
>      >>       AW_H3_SRAM_A2,
>      >>       AW_H3_SRAM_C,
>      >> +    AW_H3_EHCI0,
>      >> +    AW_H3_OHCI0,
>      >> +    AW_H3_EHCI1,
>      >> +    AW_H3_OHCI1,
>      >> +    AW_H3_EHCI2,
>      >> +    AW_H3_OHCI2,
>      >> +    AW_H3_EHCI3,
>      >> +    AW_H3_OHCI3,
>      >>       AW_H3_CCU,
>      >>       AW_H3_PIT,
>      >>       AW_H3_UART0,
>      >> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
>      >> index 8df8e3e05e..f360625ee9 100644
>      >> --- a/hw/arm/allwinner-h3.c
>      >> +++ b/hw/arm/allwinner-h3.c
>      >> @@ -28,6 +28,7 @@
>      >>   #include "hw/sysbus.h"
>      >>   #include "hw/char/serial.h"
>      >>   #include "hw/misc/unimp.h"
>      >> +#include "hw/usb/hcd-ehci.h"
>      >>   #include "sysemu/sysemu.h"
>      >>   #include "hw/arm/allwinner-h3.h"
>      >> @@ -36,6 +37,14 @@ const hwaddr allwinner_h3_memmap[] = {
>      >>       [AW_H3_SRAM_A1]    = 0x00000000,
>      >>       [AW_H3_SRAM_A2]    = 0x00044000,
>      >>       [AW_H3_SRAM_C]     = 0x00010000,
>      >> +    [AW_H3_EHCI0]      = 0x01c1a000,
>      >> +    [AW_H3_OHCI0]      = 0x01c1a400,
>      >> +    [AW_H3_EHCI1]      = 0x01c1b000,
>      >> +    [AW_H3_OHCI1]      = 0x01c1b400,
>      >> +    [AW_H3_EHCI2]      = 0x01c1c000,
>      >> +    [AW_H3_OHCI2]      = 0x01c1c400,
>      >> +    [AW_H3_EHCI3]      = 0x01c1d000,
>      >> +    [AW_H3_OHCI3]      = 0x01c1d400,
>      >>       [AW_H3_CCU]        = 0x01c20000,
>      >>       [AW_H3_PIT]        = 0x01c20c00,
>      >>       [AW_H3_UART0]      = 0x01c28000,
>      >> @@ -73,10 +82,10 @@ struct AwH3Unimplemented {
>      >>       { "msgbox",    0x01c17000, 4 * KiB },
>      >>       { "spinlock",  0x01c18000, 4 * KiB },
>      >>       { "usb0-otg",  0x01c19000, 4 * KiB },
>      >> -    { "usb0",      0x01c1a000, 4 * KiB },
>      >> -    { "usb1",      0x01c1b000, 4 * KiB },
>      >> -    { "usb2",      0x01c1c000, 4 * KiB },
>      >> -    { "usb3",      0x01c1d000, 4 * KiB },
>      >> +    { "usb0-phy",  0x01c1a000, 4 * KiB },
>      >> +    { "usb1-phy",  0x01c1b000, 4 * KiB },
>      >> +    { "usb2-phy",  0x01c1c000, 4 * KiB },
>      >> +    { "usb3-phy",  0x01c1d000, 4 * KiB },
>      >
>      > As in v3 comment, this can be done in patch #1.
>      >
>      > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>      > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
> 
>     Err, this patch is incomplete, when using ./configure
>     --without-default-devices:
> 
>     $ qemu-system-arm -M orangepi-pc
>     qemu-system-arm: invalid accelerator kvm
>     qemu-system-arm: falling back to tcg
>     qemu-system-arm: Unknown device 'aw-h3-ehci-usb' for default sysbus
>     qemu-system-arm: Unknown device 'sysbus-ohci' for default sysbus
>     Aborted (core dumped)
> 
> 
> Thanks for pointing this out, I was not aware at all that the 
> --without-default-devices option existed.
> It's not in the configure --help message also.

Indeed. This is https://bugs.launchpad.net/qemu/+bug/1836537

> I tried to re-produce the error by running:
> $ ./configure --target-list=arm-softmmu --without-default-devices; make -j5
> $ ./arm-softmmu/qemu-system-arm -M orangepi-pc
> 
> On my laptop it didn't give the error, I think because somehow the build 
> system did select
> the USB config items (even tho they were missing for the ALLWINNER_H3 
> item in hw/arm/Kconfig):
> 
> $ grep USB arm-softmmu/config-devices.mak
> CONFIG_TUSB6010=y
> CONFIG_USB=y
> CONFIG_USB_EHCI=y
> CONFIG_USB_EHCI_SYSBUS=y
> CONFIG_USB_MUSB=y
> CONFIG_USB_OHCI=y
> 
> Is there any other option you used in addition to 
> --without-default-devices to trigger the error?

Uff this was 1 month ago... I might have done:

  $ echo CONFIG_ALLWINNER_H3=y > arm-softmmu/config-devices.mak
  $ make arm-softmmu/all

> I also searched for something in configure to select/filter on machines 
> to build, but I don't see that yet.
> 
> Nevertheless I will surely add the fix below to the patch.
> 
> Regards,
> Niek
> 
> 
> 
>     You need to amend:
> 
>     -- >8 --
>     diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>     index bb75c1de17..57b29cc522 100644
>     --- a/hw/arm/Kconfig
>     +++ b/hw/arm/Kconfig
>     @@ -302,6 +302,8 @@ config ALLWINNER_H3
>            select ARM_TIMER
>            select ARM_GIC
>            select UNIMP
>     +    select USB_OHCI
>     +    select USB_EHCI_SYSBUS
> 
>        config RASPI
>            bool
>     ---
> 
>     R/T-b tags can stay with this amended.