Re: [PATCH 0/2] Add property to support writing ERSTBA in high-low order

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Guenter,

On 5/4/25 16:00, Guenter Roeck wrote:
> This series is needed to support the USB interface on imx8mp-evk when
> booting the Linux kernel.
> 
> According to the XHCI specification, ERSTBA should be written in Low-High
> order. The Linux kernel writes the high word first. This results in an
> initialization failure.
> 
> The following information is found in the Linux kernel commit log.
> 
> [Synopsys]- The host controller was design to support ERST setting
> during the RUN state. But since there is a limitation in controller
> in supporting separate ERSTBA_HI and ERSTBA_LO programming,
> It is supported when the ERSTBA is programmed in 64bit,
> or in 32 bit mode ERSTBA_HI before ERSTBA_LO
> 
> [Synopsys]- The internal initialization of event ring fetches
> the "Event Ring Segment Table Entry" based on the indication of
> ERSTBA_LO written.
> 
> Add property to support writing the high word first. Enable it
> for dwc3.
> 
> ----------------------------------------------------------------
> Guenter Roeck (2):
>        hw: usb: xhci: Add property to support writing ERSTBA in high-low order
>        hw/usb/hcd-dwc3: Set erstba-hi-lo property

What about using .impl.min_access_size = 8 instead?

Could you try this patch, or provide me with a reproducer?

-- >8 --
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 64c3a23b9b7..bce61a287bf 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3043,7 +3043,4 @@ static uint64_t xhci_runtime_read(void *ptr, 
hwaddr reg,
          switch (reg & 0x1f) {
-        case 0x00: /* IMAN */
-            ret = intr->iman;
-            break;
-        case 0x04: /* IMOD */
-            ret = intr->imod;
+        case 0x00: /* IMAN & IMOD */
+            ret = deposit64(intr->iman, 32, 32, intr->imod);
              break;
@@ -3052,13 +3049,7 @@ static uint64_t xhci_runtime_read(void *ptr, 
hwaddr reg,
              break;
-        case 0x10: /* ERSTBA low */
-            ret = intr->erstba_low;
+        case 0x10: /* ERSTBA */
+            ret = deposit64(intr->erstba_low, 32, 32, intr->erstba_high);
              break;
-        case 0x14: /* ERSTBA high */
-            ret = intr->erstba_high;
-            break;
-        case 0x18: /* ERDP low */
-            ret = intr->erdp_low;
-            break;
-        case 0x1c: /* ERDP high */
-            ret = intr->erdp_high;
+        case 0x18: /* ERDP */
+            ret = deposit64(intr->erdp_low, 32, 32, intr->erdp_high);
              break;
@@ -3088,3 +3079,3 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
      switch (reg & 0x1f) {
-    case 0x00: /* IMAN */
+    case 0x00: /* IMAN & IMOD */
          if (val & IMAN_IP) {
@@ -3094,23 +3085,19 @@ static void xhci_runtime_write(void *ptr, hwaddr 
reg,
          intr->iman |= val & IMAN_IE;
+        intr->imod = extract64(val, 32, 32);
          xhci_intr_update(xhci, v);
          break;
-    case 0x04: /* IMOD */
-        intr->imod = val;
-        break;
      case 0x08: /* ERSTSZ */
-        intr->erstsz = val & 0xffff;
+        intr->erstsz = extract64(val, 0, 16);
          break;
-    case 0x10: /* ERSTBA low */
+    case 0x10: /* ERSTBA */
          if (xhci->nec_quirks) {
              /* NEC driver bug: it doesn't align this to 64 bytes */
-            intr->erstba_low = val & 0xfffffff0;
+            intr->erstba_low = extract64(val, 4, 28);
          } else {
-            intr->erstba_low = val & 0xffffffc0;
+            intr->erstba_low = extract64(val, 6, 26);
          }
-        break;
-    case 0x14: /* ERSTBA high */
-        intr->erstba_high = val;
+        intr->erstba_high = extract64(val, 32, 32);
          xhci_er_reset(xhci, v);
          break;
-    case 0x18: /* ERDP low */
+    case 0x18: /* ERDP */
          if (val & ERDP_EHB) {
@@ -3128,5 +3115,3 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
          }
-        break;
-    case 0x1c: /* ERDP high */
-        intr->erdp_high = val;
+        intr->erdp_high = extract64(val, 32, 32);
          break;
@@ -3216,2 +3201,3 @@ static const MemoryRegionOps xhci_runtime_ops = {
      .write = xhci_runtime_write,
+    .impl.min_access_size = 8,
      .valid.min_access_size = 4,
---

Regards,

Phil.