[PATCH 0/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[PATCH 0/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[wangyuquan1236]

From: Yuquan Wang <wangyuquan1236@phytium.com.cn>

Please review the change.
 - Replace EHCI with XHCI on sbsa-ref board.

Yuquan Wang (1):
  hw/arm/sbsa-ref: use XHCI to replace EHCI

 hw/arm/sbsa-ref.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[wangyuquan1236]

From: Yuquan Wang <wangyuquan1236@phytium.com.cn>

The current sbsa-ref cannot use EHCI controller which is only
able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
Hence, this uses XHCI to provide a usb controller with 64-bit
DMA capablity instead of EHCI.

Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
Change-Id: I1376f8bbc0e25dcd9d8a22b6e061cb56b3486394
---
 hw/arm/sbsa-ref.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 792371fdce..f9c0647353 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -81,7 +81,7 @@ enum {
     SBSA_SECURE_UART_MM,
     SBSA_SECURE_MEM,
     SBSA_AHCI,
-    SBSA_EHCI,
+    SBSA_XHCI,
 };
 
 struct SBSAMachineState {
@@ -118,7 +118,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
     [SBSA_SMMU] =               { 0x60050000, 0x00020000 },
     /* Space here reserved for more SMMUs */
     [SBSA_AHCI] =               { 0x60100000, 0x00010000 },
-    [SBSA_EHCI] =               { 0x60110000, 0x00010000 },
+    [SBSA_XHCI] =               { 0x60110000, 0x00010000 },
     /* Space here reserved for other devices */
     [SBSA_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
     /* 32-bit address PCIE MMIO space */
@@ -138,7 +138,7 @@ static const int sbsa_ref_irqmap[] = {
     [SBSA_SECURE_UART] = 8,
     [SBSA_SECURE_UART_MM] = 9,
     [SBSA_AHCI] = 10,
-    [SBSA_EHCI] = 11,
+    [SBSA_XHCI] = 11,
     [SBSA_SMMU] = 12, /* ... to 15 */
     [SBSA_GWDT_WS0] = 16,
 };
@@ -558,12 +558,12 @@ static void create_ahci(const SBSAMachineState *sms)
     }
 }
 
-static void create_ehci(const SBSAMachineState *sms)
+static void create_xhci(const SBSAMachineState *sms)
 {
-    hwaddr base = sbsa_ref_memmap[SBSA_EHCI].base;
-    int irq = sbsa_ref_irqmap[SBSA_EHCI];
+    hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
+    int irq = sbsa_ref_irqmap[SBSA_XHCI];
 
-    sysbus_create_simple("platform-ehci-usb", base,
+    sysbus_create_simple("sysbus-xhci", base,
                          qdev_get_gpio_in(sms->gic, irq));
 }
 
@@ -785,7 +785,7 @@ static void sbsa_ref_init(MachineState *machine)
 
     create_ahci(sms);
 
-    create_ehci(sms);
+    create_xhci(sms);
 
     create_pcie(sms);
 
-- 
2.34.1

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Graeme Gregory]

On Wed, May 31, 2023 at 03:02:29PM +0800, wangyuquan1236@phytium.com.cn wrote:
> From: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> 
> The current sbsa-ref cannot use EHCI controller which is only
> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
> Hence, this uses XHCI to provide a usb controller with 64-bit
> DMA capablity instead of EHCI.
> 

Should this be below 4G?

Also has EHCI never worked, or has it worked in some modes and so this
change should be versioned?

Graeme

> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> Change-Id: I1376f8bbc0e25dcd9d8a22b6e061cb56b3486394
> ---
>  hw/arm/sbsa-ref.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 792371fdce..f9c0647353 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -81,7 +81,7 @@ enum {
>      SBSA_SECURE_UART_MM,
>      SBSA_SECURE_MEM,
>      SBSA_AHCI,
> -    SBSA_EHCI,
> +    SBSA_XHCI,
>  };
>  
>  struct SBSAMachineState {
> @@ -118,7 +118,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
>      [SBSA_SMMU] =               { 0x60050000, 0x00020000 },
>      /* Space here reserved for more SMMUs */
>      [SBSA_AHCI] =               { 0x60100000, 0x00010000 },
> -    [SBSA_EHCI] =               { 0x60110000, 0x00010000 },
> +    [SBSA_XHCI] =               { 0x60110000, 0x00010000 },
>      /* Space here reserved for other devices */
>      [SBSA_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
>      /* 32-bit address PCIE MMIO space */
> @@ -138,7 +138,7 @@ static const int sbsa_ref_irqmap[] = {
>      [SBSA_SECURE_UART] = 8,
>      [SBSA_SECURE_UART_MM] = 9,
>      [SBSA_AHCI] = 10,
> -    [SBSA_EHCI] = 11,
> +    [SBSA_XHCI] = 11,
>      [SBSA_SMMU] = 12, /* ... to 15 */
>      [SBSA_GWDT_WS0] = 16,
>  };
> @@ -558,12 +558,12 @@ static void create_ahci(const SBSAMachineState *sms)
>      }
>  }
>  
> -static void create_ehci(const SBSAMachineState *sms)
> +static void create_xhci(const SBSAMachineState *sms)
>  {
> -    hwaddr base = sbsa_ref_memmap[SBSA_EHCI].base;
> -    int irq = sbsa_ref_irqmap[SBSA_EHCI];
> +    hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
> +    int irq = sbsa_ref_irqmap[SBSA_XHCI];
>  
> -    sysbus_create_simple("platform-ehci-usb", base,
> +    sysbus_create_simple("sysbus-xhci", base,
>                           qdev_get_gpio_in(sms->gic, irq));
>  }
>  
> @@ -785,7 +785,7 @@ static void sbsa_ref_init(MachineState *machine)
>  
>      create_ahci(sms);
>  
> -    create_ehci(sms);
> +    create_xhci(sms);
>  
>      create_pcie(sms);
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Peter Maydell]

On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
>
> On Wed, May 31, 2023 at 03:02:29PM +0800, wangyuquan1236@phytium.com.cn wrote:
> > From: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> >
> > The current sbsa-ref cannot use EHCI controller which is only
> > able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
> > Hence, this uses XHCI to provide a usb controller with 64-bit
> > DMA capablity instead of EHCI.
> >
>
> Should this be below 4G?

It would be pretty disruptive to try to rearrange the memory
map to put RAM below 4GB at this point, though in theory it's
possible I guess. (I have a vague recollection that there was
some reason the RAM was all put above 4GB, but can't find
anything about that in my email archives. Perhaps Leif remembers?)

> Also has EHCI never worked, or has it worked in some modes and so this
> change should be versioned?

AIUI, EHCI has never worked and can never have worked, because
this board's RAM is all above 4G and the QEMU EHCI controller
implementation only allows DMA descriptors with 32-bit addresses.

Looking back at the archives, it seems we discussed XHCI vs
EHCI when the sbsa-ref board went in, and the conclusion was
that XHCI would be better. But there wasn't a sysbus XHCI device
at that point, so we ended up committing the sbsa-ref board
with EHCI and a plan to switch to XHCI when the sysbus-xhci
device was done, which we then forgot about:
https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html

thanks
-- PMM

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Marcin Juszkiewicz]

W dniu 31.05.2023 o 17:27, Peter Maydell pisze:
> On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
>> On Wed, May 31, 2023 at 03:02:29PM +0800, wangyuquan1236@phytium.com.cn wrote:
>>> From: Yuquan Wang <wangyuquan1236@phytium.com.cn>
>>>
>>> The current sbsa-ref cannot use EHCI controller which is only
>>> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
>>> Hence, this uses XHCI to provide a usb controller with 64-bit
>>> DMA capablity instead of EHCI.

>> Should this be below 4G?

> It would be pretty disruptive to try to rearrange the memory
> map to put RAM below 4GB at this point, though in theory it's
> possible I guess. (I have a vague recollection that there was
> some reason the RAM was all put above 4GB, but can't find
> anything about that in my email archives. Perhaps Leif remembers?)

I thought that memory starts at 40bit was to not use Cortex-A53 cpu with 
sbsa-ref. Nowadays it also removed Cortex-A76.

> Looking back at the archives, it seems we discussed XHCI vs
> EHCI when the sbsa-ref board went in, and the conclusion was
> that XHCI would be better. But there wasn't a sysbus XHCI device
> at that point, so we ended up committing the sbsa-ref board
> with EHCI and a plan to switch to XHCI when the sysbus-xhci
> device was done, which we then forgot about:
> https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html

Moving from EHCI to XHCI on sysbus requires also firmware changes.

Or we can just add "qemu-xhci" pcie card and have USB running.

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Leif Lindholm]

On 2023-05-31 16:27, Peter Maydell wrote:
> On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
>>> The current sbsa-ref cannot use EHCI controller which is only
>>> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
>>> Hence, this uses XHCI to provide a usb controller with 64-bit
>>> DMA capablity instead of EHCI.
>>
>> Should this be below 4G?
> 
> It would be pretty disruptive to try to rearrange the memory
> map to put RAM below 4GB at this point, though in theory it's
> possible I guess. (I have a vague recollection that there was
> some reason the RAM was all put above 4GB, but can't find
> anything about that in my email archives. Perhaps Leif remembers?)

I think Graeme was just pointing out a typo in Marcin's email.

Yeah, we're not changing the DRAM base at this stage.
I think the reason we put no RAM below 4GB was simply that we had 
several real-world platforms where that was true, and given the intended 
use-case for the platform, we explicitly wanted to trigger issues those 
platforms might encounter.

>> Also has EHCI never worked, or has it worked in some modes and so this
>> change should be versioned?
> 
> AIUI, EHCI has never worked and can never have worked, because
> this board's RAM is all above 4G and the QEMU EHCI controller
> implementation only allows DMA descriptors with 32-bit addresses.
> 
> Looking back at the archives, it seems we discussed XHCI vs
> EHCI when the sbsa-ref board went in, and the conclusion was
> that XHCI would be better. But there wasn't a sysbus XHCI device
> at that point, so we ended up committing the sbsa-ref board
> with EHCI and a plan to switch to XHCI when the sysbus-xhci
> device was done, which we then forgot about:
> https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html

Ah, thanks! That explains why we did the thing that made no sense :)

To skip the migration hazard, my prefernece is we just leave the EHCI 
device in for now, and add a separate XHCI on PCIe. We can drop the
EHCI device at some point in the future.

/
     Leif

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Graeme Gregory]

On Wed, 31 May 2023, at 5:36 PM, Leif Lindholm wrote:
> On 2023-05-31 16:27, Peter Maydell wrote:
>> On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
>>>> The current sbsa-ref cannot use EHCI controller which is only
>>>> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
>>>> Hence, this uses XHCI to provide a usb controller with 64-bit
>>>> DMA capablity instead of EHCI.
>>>
>>> Should this be below 4G?
>> 
>> It would be pretty disruptive to try to rearrange the memory
>> map to put RAM below 4GB at this point, though in theory it's
>> possible I guess. (I have a vague recollection that there was
>> some reason the RAM was all put above 4GB, but can't find
>> anything about that in my email archives. Perhaps Leif remembers?)
>
> I think Graeme was just pointing out a typo in Marcin's email.
>

Yes the typo!

Graeme

> Yeah, we're not changing the DRAM base at this stage.
> I think the reason we put no RAM below 4GB was simply that we had 
> several real-world platforms where that was true, and given the intended 
> use-case for the platform, we explicitly wanted to trigger issues those 
> platforms might encounter.
>
>>> Also has EHCI never worked, or has it worked in some modes and so this
>>> change should be versioned?
>> 
>> AIUI, EHCI has never worked and can never have worked, because
>> this board's RAM is all above 4G and the QEMU EHCI controller
>> implementation only allows DMA descriptors with 32-bit addresses.
>> 
>> Looking back at the archives, it seems we discussed XHCI vs
>> EHCI when the sbsa-ref board went in, and the conclusion was
>> that XHCI would be better. But there wasn't a sysbus XHCI device
>> at that point, so we ended up committing the sbsa-ref board
>> with EHCI and a plan to switch to XHCI when the sysbus-xhci
>> device was done, which we then forgot about:
>> https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
>
> Ah, thanks! That explains why we did the thing that made no sense :)
>
> To skip the migration hazard, my prefernece is we just leave the EHCI 
> device in for now, and add a separate XHCI on PCIe. We can drop the
> EHCI device at some point in the future.
>
> /
>      Leif

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Chen Baozi]

Hi Leif,

> On Jun 1, 2023, at 00:36, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> 
> On 2023-05-31 16:27, Peter Maydell wrote:
>> On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
>>>> The current sbsa-ref cannot use EHCI controller which is only
>>>> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
>>>> Hence, this uses XHCI to provide a usb controller with 64-bit
>>>> DMA capablity instead of EHCI.
>>> 
>>> Should this be below 4G?
>> It would be pretty disruptive to try to rearrange the memory
>> map to put RAM below 4GB at this point, though in theory it's
>> possible I guess. (I have a vague recollection that there was
>> some reason the RAM was all put above 4GB, but can't find
>> anything about that in my email archives. Perhaps Leif remembers?)
> 
> I think Graeme was just pointing out a typo in Marcin's email.
> 
> Yeah, we're not changing the DRAM base at this stage.
> I think the reason we put no RAM below 4GB was simply that we had several real-world platforms where that was true, and given the intended use-case for the platform, we explicitly wanted to trigger issues those platforms might encounter.
> 
>>> Also has EHCI never worked, or has it worked in some modes and so this
>>> change should be versioned?
>> AIUI, EHCI has never worked and can never have worked, because
>> this board's RAM is all above 4G and the QEMU EHCI controller
>> implementation only allows DMA descriptors with 32-bit addresses.
>> Looking back at the archives, it seems we discussed XHCI vs
>> EHCI when the sbsa-ref board went in, and the conclusion was
>> that XHCI would be better. But there wasn't a sysbus XHCI device
>> at that point, so we ended up committing the sbsa-ref board
>> with EHCI and a plan to switch to XHCI when the sysbus-xhci
>> device was done, which we then forgot about:
>> https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
> 
> Ah, thanks! That explains why we did the thing that made no sense :)
> 
> To skip the migration hazard, my prefernece is we just leave the EHCI device in for now, and add a separate XHCI on PCIe. We can drop the
> EHCI device at some point in the future.

What about introducing another SMMU for all the platform devices on sbsa-ref? I was thinking over this solution for some time. If that can be feasible, we then also have a prototype of IOMMU for platform device.

Regards,

Baozi.

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Peter Maydell]

On Wed, 31 May 2023 at 17:37, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2023-05-31 16:27, Peter Maydell wrote:
> > On Wed, 31 May 2023 at 15:58, Graeme Gregory <graeme@xora.org.uk> wrote:
> >>> The current sbsa-ref cannot use EHCI controller which is only
> >>> able to do 32-bit DMA, since sbsa-ref doesn't have RAM above 4GB.
> >>> Hence, this uses XHCI to provide a usb controller with 64-bit
> >>> DMA capablity instead of EHCI.
> >>
> >> Should this be below 4G?
> >
> > It would be pretty disruptive to try to rearrange the memory
> > map to put RAM below 4GB at this point, though in theory it's
> > possible I guess. (I have a vague recollection that there was
> > some reason the RAM was all put above 4GB, but can't find
> > anything about that in my email archives. Perhaps Leif remembers?)
>
> I think Graeme was just pointing out a typo in Marcin's email.
>
> Yeah, we're not changing the DRAM base at this stage.
> I think the reason we put no RAM below 4GB was simply that we had
> several real-world platforms where that was true, and given the intended
> use-case for the platform, we explicitly wanted to trigger issues those
> platforms might encounter.
>
> >> Also has EHCI never worked, or has it worked in some modes and so this
> >> change should be versioned?
> >
> > AIUI, EHCI has never worked and can never have worked, because
> > this board's RAM is all above 4G and the QEMU EHCI controller
> > implementation only allows DMA descriptors with 32-bit addresses.
> >
> > Looking back at the archives, it seems we discussed XHCI vs
> > EHCI when the sbsa-ref board went in, and the conclusion was
> > that XHCI would be better. But there wasn't a sysbus XHCI device
> > at that point, so we ended up committing the sbsa-ref board
> > with EHCI and a plan to switch to XHCI when the sysbus-xhci
> > device was done, which we then forgot about:
> > https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
>
> Ah, thanks! That explains why we did the thing that made no sense :)
>
> To skip the migration hazard, my prefernece is we just leave the EHCI
> device in for now, and add a separate XHCI on PCIe. We can drop the
> EHCI device at some point in the future.

Why PCIe for the XHCI and not sysbus? At the time the board
was originally added the argument was in favour of using
a sysbus USB controller (you can see Ard making that point
in the linked archive thread).

thanks
-- PMM

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Marcin Juszkiewicz]

W dniu 1.06.2023 o 17:01, Peter Maydell pisze:
> On Wed, 31 May 2023 at 17:37, Leif Lindholm <quic_llindhol@quicinc.com> wrote:

>> Ah, thanks! That explains why we did the thing that made no sense :)
>>
>> To skip the migration hazard, my prefernece is we just leave the EHCI
>> device in for now, and add a separate XHCI on PCIe. We can drop the
>> EHCI device at some point in the future.
> 
> Why PCIe for the XHCI and not sysbus? At the time the board
> was originally added the argument was in favour of using
> a sysbus USB controller (you can see Ard making that point
> in the linked archive thread).

So something like below? I only tested does system boot into Debian.
To make it work also changes to EDK2 would be needed to list XHCI
controller in DSDT.

"info qtree" in QEMU monitor lists controller with usb devices
attached (added them by cli arguments).

 From 8f5af99a670be226a1dfc5b06cbdd3eff4841d27 Mon Sep 17 00:00:00 2001
From: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Date: Thu, 1 Jun 2023 17:27:24 +0200
Subject: [PATCH] WIP: arm/sbsa-ref: add XHCI on sysbus

EHCI controller is not working as it requires 32-bit memory.
XHCI one should work fine.
---
  hw/arm/sbsa-ref.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index de21200ff9..0bc87abbf4 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -83,6 +83,7 @@ enum {
      SBSA_SECURE_MEM,
      SBSA_AHCI,
      SBSA_EHCI,
+    SBSA_XHCI,
  };
  
  struct SBSAMachineState {
@@ -120,6 +121,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
      /* Space here reserved for more SMMUs */
      [SBSA_AHCI] =               { 0x60100000, 0x00010000 },
      [SBSA_EHCI] =               { 0x60110000, 0x00010000 },
+    [SBSA_XHCI] =               { 0x60120000, 0x00010000 },
      /* Space here reserved for other devices */
      [SBSA_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
      /* 32-bit address PCIE MMIO space */
@@ -142,6 +144,7 @@ static const int sbsa_ref_irqmap[] = {
      [SBSA_EHCI] = 11,
      [SBSA_SMMU] = 12, /* ... to 15 */
      [SBSA_GWDT_WS0] = 16,
+    [SBSA_XHCI] = 17,
  };
  
  static const char * const valid_cpus[] = {
@@ -575,6 +578,15 @@ static void create_ahci(const SBSAMachineState *sms)
      }
  }
  
+static void create_xhci(const SBSAMachineState *sms)
+{
+    hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
+    int irq = sbsa_ref_irqmap[SBSA_XHCI];
+
+    sysbus_create_simple("sysbus-xhci", base,
+                         qdev_get_gpio_in(sms->gic, irq));
+}
+
  static void create_ehci(const SBSAMachineState *sms)
  {
      hwaddr base = sbsa_ref_memmap[SBSA_EHCI].base;
@@ -804,6 +816,7 @@ static void sbsa_ref_init(MachineState *machine)
      create_ahci(sms);
  
      create_ehci(sms);
+    create_xhci(sms);
  
      create_pcie(sms);
  
-- 
2.40.1

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Peter Maydell]

On Thu, 1 Jun 2023 at 16:30, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> W dniu 1.06.2023 o 17:01, Peter Maydell pisze:
> > On Wed, 31 May 2023 at 17:37, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> >> Ah, thanks! That explains why we did the thing that made no sense :)
> >>
> >> To skip the migration hazard, my prefernece is we just leave the EHCI
> >> device in for now, and add a separate XHCI on PCIe. We can drop the
> >> EHCI device at some point in the future.
> >
> > Why PCIe for the XHCI and not sysbus? At the time the board
> > was originally added the argument was in favour of using
> > a sysbus USB controller (you can see Ard making that point
> > in the linked archive thread).
>
> So something like below? I only tested does system boot into Debian.
> To make it work also changes to EDK2 would be needed to list XHCI
> controller in DSDT.

Yes, and you also want to drop the useless EHCI controller,
and (as you note) both these things mean firmware changes
so presumably that's a version-bump event?

-- PMM

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Marcin Juszkiewicz]

W dniu 1.06.2023 o 18:39, Peter Maydell pisze:
>> So something like below? I only tested does system boot into Debian.
>> To make it work also changes to EDK2 would be needed to list XHCI
>> controller in DSDT.

> Yes, and you also want to drop the useless EHCI controller,
> and (as you note) both these things mean firmware changes
> so presumably that's a version-bump event?

First we need to get two patches through TF-A to get current information 
from QEMU. And it takes time.

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20871

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20953

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Leif Lindholm]

+Ard

On Thu, Jun 01, 2023 at 16:01:43 +0100, Peter Maydell wrote:
> > >> Also has EHCI never worked, or has it worked in some modes and so this
> > >> change should be versioned?
> > >
> > > AIUI, EHCI has never worked and can never have worked, because
> > > this board's RAM is all above 4G and the QEMU EHCI controller
> > > implementation only allows DMA descriptors with 32-bit addresses.
> > >
> > > Looking back at the archives, it seems we discussed XHCI vs
> > > EHCI when the sbsa-ref board went in, and the conclusion was
> > > that XHCI would be better. But there wasn't a sysbus XHCI device
> > > at that point, so we ended up committing the sbsa-ref board
> > > with EHCI and a plan to switch to XHCI when the sysbus-xhci
> > > device was done, which we then forgot about:
> > > https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
> >
> > Ah, thanks! That explains why we did the thing that made no sense :)
> >
> > To skip the migration hazard, my prefernece is we just leave the EHCI
> > device in for now, and add a separate XHCI on PCIe. We can drop the
> > EHCI device at some point in the future.
> 
> Why PCIe for the XHCI and not sysbus? At the time the board
> was originally added the argument was in favour of using
> a sysbus USB controller (you can see Ard making that point
> in the linked archive thread).

The original argument was that having the device on the sysbus
1) enabled codepaths we wanted to exercise and
2) more closely resembled the development systems available at the
time.

1 still applies, but I'm not sure 2 does. Ard?

/
    Leif

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Ard Biesheuvel]

On Thu, 1 Jun 2023 at 20:00, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> +Ard
>
> On Thu, Jun 01, 2023 at 16:01:43 +0100, Peter Maydell wrote:
> > > >> Also has EHCI never worked, or has it worked in some modes and so this
> > > >> change should be versioned?
> > > >
> > > > AIUI, EHCI has never worked and can never have worked, because
> > > > this board's RAM is all above 4G and the QEMU EHCI controller
> > > > implementation only allows DMA descriptors with 32-bit addresses.
> > > >
> > > > Looking back at the archives, it seems we discussed XHCI vs
> > > > EHCI when the sbsa-ref board went in, and the conclusion was
> > > > that XHCI would be better. But there wasn't a sysbus XHCI device
> > > > at that point, so we ended up committing the sbsa-ref board
> > > > with EHCI and a plan to switch to XHCI when the sysbus-xhci
> > > > device was done, which we then forgot about:
> > > > https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
> > >
> > > Ah, thanks! That explains why we did the thing that made no sense :)
> > >
> > > To skip the migration hazard, my prefernece is we just leave the EHCI
> > > device in for now, and add a separate XHCI on PCIe. We can drop the
> > > EHCI device at some point in the future.
> >
> > Why PCIe for the XHCI and not sysbus? At the time the board
> > was originally added the argument was in favour of using
> > a sysbus USB controller (you can see Ard making that point
> > in the linked archive thread).
>
> The original argument was that having the device on the sysbus
> 1) enabled codepaths we wanted to exercise and
> 2) more closely resembled the development systems available at the
> time.
>
> 1 still applies, but I'm not sure 2 does. Ard?
>

It was always primarily about #1. This was also the reason for putting
all DRAM above 4G.

I'm surprised that the EHCI never worked, though - I don't see the
point in keeping it in that case.

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Yuquan Wang]

Hi, Leif

On Thu, 1 Jun 2023 18:59:56 +0100, Leif Lindholm wrote:
> 
> +Ard
> 
> On Thu, Jun 01, 2023 at 16:01:43 +0100, Peter Maydell wrote:
> > > >> Also has EHCI never worked, or has it worked in some modes and so this
> > > >> change should be versioned?
> > > >
> > > > AIUI, EHCI has never worked and can never have worked, because
> > > > this board's RAM is all above 4G and the QEMU EHCI controller
> > > > implementation only allows DMA descriptors with 32-bit addresses.
> > > >
> > > > Looking back at the archives, it seems we discussed XHCI vs
> > > > EHCI when the sbsa-ref board went in, and the conclusion was
> > > > that XHCI would be better. But there wasn't a sysbus XHCI device
> > > > at that point, so we ended up committing the sbsa-ref board
> > > > with EHCI and a plan to switch to XHCI when the sysbus-xhci
> > > > device was done, which we then forgot about:
> > > > https://mail.gnu.org/archive/html/qemu-arm/2018-11/msg00638.html
> > >
> > > Ah, thanks! That explains why we did the thing that made no sense :)
> > >
> > > To skip the migration hazard, my prefernece is we just leave the EHCI
> > > device in for now, and add a separate XHCI on PCIe. We can drop the
> > > EHCI device at some point in the future.
> > 
> > Why PCIe for the XHCI and not sysbus? At the time the board
> > was originally added the argument was in favour of using
> > a sysbus USB controller (you can see Ard making that point
> > in the linked archive thread).
> 
> The original argument was that having the device on the sysbus
> 1) enabled codepaths we wanted to exercise and

Sorry, for my poor engineering experience, I am confused about the meaning 
of "enabled codepaths" here. Is it like a code target that to realize the 
original purpose of this board ?

Yuquan







信息安全声明：本邮件包含信息归发件人所在组织所有,发件人所在组织对该邮件拥有所有权利。请接收者注意保密,未经发件人书面许可,不得向任何第三方组织和个人透露本邮件所含信息。
Information Security Notice: The information contained in this mail is solely property of the sender's organization.This mail communication is confidential.Recipients named above are obligated to maintain secrecy and are not permitted to disclose the contents of this communication to others.

Re: [PATCH 1/1] hw/arm/sbsa-ref: use XHCI to replace EHCI

[Leif Lindholm]

Hi Yuquan,

On Fri, Jun 02, 2023 at 11:24:11 +0800, Yuquan Wang wrote:
> > > > To skip the migration hazard, my prefernece is we just leave the EHCI
> > > > device in for now, and add a separate XHCI on PCIe. We can drop the
> > > > EHCI device at some point in the future.
> > > 
> > > Why PCIe for the XHCI and not sysbus? At the time the board
> > > was originally added the argument was in favour of using
> > > a sysbus USB controller (you can see Ard making that point
> > > in the linked archive thread).
> > 
> > The original argument was that having the device on the sysbus
> > 1) enabled codepaths we wanted to exercise and
> 
> Sorry, for my poor engineering experience, I am confused about the meaning 
> of "enabled codepaths" here. Is it like a code target that to realize the 
> original purpose of this board ?

It is a bit of a convoluted term.

sbsa-ref isn't a normal platform. We are using it as a vehicle for
developing and verifying common firmware and software for sbsa (or now
SystemReady SR) compliant platforms.

This means that we ideally want it to expose *permitted* but not
necessarily ideal behaviours, so that the parts of software that deals
with those situations get frequently exercised (enabled).
It's code coverage for the hw-interacting pieces of code.

/
    Leif