Add property to support writing ERSTBA in high-low order

[PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Guenter Roeck 8 months, 2 weeks ago

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.

Signed-off-by: Guenter Roeck <linux@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;
     uint32_t flags;
     uint32_t max_pstreams_mask;
     void (*intr_update)(XHCIState *s, int n, bool enable);
-- 
2.45.2

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Nicholas Piggin 8 months, 1 week ago

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 <linux@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 <npiggin@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
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.

Thanks,
Nick

>      uint32_t flags;
>      uint32_t max_pstreams_mask;
>      void (*intr_update)(XHCIState *s, int n, bool enable);

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Guenter Roeck 8 months ago

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 <linux@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?
> 

Done all, except then I noticed that you want me to prepend "quirk"
and I appended it out of habit. So I'll need to start over :-(.

Guenter

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Bernhard Beschow 5 months ago


Am 13. April 2025 16:03:16 UTC schrieb Guenter Roeck <linux@roeck-us.net>:
>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 <linux@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?
>> 
>
>Done all, except then I noticed that you want me to prepend "quirk"
>and I appended it out of habit. So I'll need to start over :-(.

Ping

Any news? Would be nice if a fix made it into 10.1.

>
>Guenter
>

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Guenter Roeck 5 months ago

On Wed, Jul 16, 2025 at 07:59:16PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 13. April 2025 16:03:16 UTC schrieb Guenter Roeck <linux@roeck-us.net>:
> >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 <linux@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?
> >> 
> >
> >Done all, except then I noticed that you want me to prepend "quirk"
> >and I appended it out of habit. So I'll need to start over :-(.
> 
> Ping
> 
> Any news? Would be nice if a fix made it into 10.1.
> 

Sorry, I ran out of time and won't be able to look into this again
anytime soon. I don't mind if someone else wants to pick it up.

Guenter

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Bernhard Beschow 5 months ago


Am 16. Juli 2025 20:07:25 UTC schrieb Guenter Roeck <linux@roeck-us.net>:
>On Wed, Jul 16, 2025 at 07:59:16PM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 13. April 2025 16:03:16 UTC schrieb Guenter Roeck <linux@roeck-us.net>:
>> >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 <linux@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?
>> >> 
>> >
>> >Done all, except then I noticed that you want me to prepend "quirk"
>> >and I appended it out of habit. So I'll need to start over :-(.
>> 
>> Ping
>> 
>> Any news? Would be nice if a fix made it into 10.1.
>> 
>
>Sorry, I ran out of time and won't be able to look into this again
>anytime soon. I don't mind if someone else wants to pick it up.

Okay, I will look into it as soon as I find some time.

Bernhard

>
>Guenter

Re: [PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order

Posted by Guenter Roeck 8 months, 1 week ago

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 <linux@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 <npiggin@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

[PATCH 1/2] hw: usb: xhci: Add property to support writing ERSTBA in high-low order
[PATCH 2/2] hw/usb/hcd-dwc3: Set erstba-hi-lo property