On 4/11/25 00:40, Nicholas Piggin wrote:
On Sun Apr 6, 2025 at 12:00 AM AEST, Guenter Roeck wrote:
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.
This should probably be reworded, it's not so much that Linux does it,
this kind of implies a Linux bug. It is that the hardware requires it
and Linux works around such quirk.
According to the XHCI specification, ERSTBA should be written in Low-High
order, however some controllers have a quirk that requires the low
word to be written last.
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.
Could you include a reference to the commit in the normal form?
The following information is found in the changelog for Linux kernel
commit sha ("blah").
Add property to support writing the high word first.
Signed-off-by: Guenter Roeck <li...@roeck-us.net>
---
hw/usb/hcd-xhci.c | 8 +++++++-
hw/usb/hcd-xhci.h | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 64c3a23b9b..8c0ba569c8 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3107,10 +3107,15 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
} else {
intr->erstba_low = val & 0xffffffc0;
}
+ if (xhci->erstba_hi_lo) {
+ xhci_er_reset(xhci, v);
+ }
break;
case 0x14: /* ERSTBA high */
intr->erstba_high = val;
- xhci_er_reset(xhci, v);
+ if (!xhci->erstba_hi_lo) {
+ xhci_er_reset(xhci, v);
+ }
break;
case 0x18: /* ERDP low */
if (val & ERDP_EHB) {
@@ -3636,6 +3641,7 @@ static const Property xhci_properties[] = {
DEFINE_PROP_UINT32("p3", XHCIState, numports_3, 4),
DEFINE_PROP_LINK("host", XHCIState, hostOpaque, TYPE_DEVICE,
DeviceState *),
+ DEFINE_PROP_BOOL("erstba-hi-lo", XHCIState, erstba_hi_lo, false),
};
static void xhci_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 9c3974f148..cf3f074261 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -189,6 +189,7 @@ typedef struct XHCIState {
uint32_t numports_3;
uint32_t numintrs;
uint32_t numslots;
+ bool erstba_hi_lo;
Could you use the "quirk" prefix for the device and property name?
With those changes,
Reviewed-by: Nicholas Piggin <npig...@gmail.com>
With your patch, if the target does do a 64-bit write to the address,
what happens? I wonder if that's something the device is supposed to
Sorry, I have no means to test this, so I have no idea what happens
in this situation.
cope with but doesn't work or just works by luck today... I would say
that's a separate problem though, if you can get Linux working okay
with this approach.
Linux always writes four bytes at a time, so any change in qemu to explicitly
handle 8-byte writes would not make a difference.
Guenter
