Re: [PATCH v3 13/17] hw/usb/hcd-ehci: Add descriptor address offset property

["Cédric Le Goater" <clg@kaod.org>]

On 4/20/26 07:47, Jamin Lin wrote:
> Hi Philippe, Cédric
> 
>>>> -    return ehci_get_buf_addr(s, s->ctrldssegment, low, UINT32_MAX);
>>>> +    uint64_t addr;
>>>> +
>>>> +    addr = ehci_get_buf_addr(s, s->ctrldssegment, low, UINT32_MAX);
>>>> +    addr += s->descriptor_addr_offset;
>>>
>>>
>>> Instead, can we force a default value for ctrldssegment when
>>> caps_64bit_addr is set ?
>>>
>>> I am still trying to digest :
>>>
>>>     https://lore.kernel.org/qemu-devel/
>>>
>> TYPPR06MB8206C7461E004C4FF07B8E92FC242@TYPPR06MB8206.apcprd06.
>> prod.out
>>> look.com/
>>
>> Same here, but since it doesn't break and I don't have much more time to
>> dedicate to this I ended thinking "good enough for now, could be improved
>> later", so no objection on my side.
>>
>>>
>>> We have time let's take a moment to think about the workaround.
>>
>> Then keep me in the loop!
> 
> 
> Thanks for the review and the suggestion.
> 
> I tried to address this by introducing a new property:
> aspeed-ast2700-workaround.

This is not specific to AST2700.

I think a forced default value for CTRLDSSEGMENT would be more
appropriate. Perhaps "ctrldssegment-default", as a uint32_t,
which defaults to 0x0 and when the ast2700 SoC is realized,
set to BIT(2).


> 
> When this property is enabled:
> 
> 1. We force bit 2 of the CTRLDSSEGMENT register to 1 during initialization (when caps_64bit_addr is set).
> 2. Additionally, if firmware writes to the CTRLDSSEGMENT register, we also ensure that bit 2 remains set.
> 
> This keeps the behavior contained to AST2700-specific usage and avoids impacting other platforms.
> Let me know if you think this approach makes sense, or if you'd prefer a different direction.
> 
> Jamin
> 
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index af8c080c60..0a58dca668 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -263,6 +263,7 @@ struct EHCIState {
>       /* properties */
>       uint32_t maxframes;
>       bool caps_64bit_addr;
> +    bool aspeed_ast2700_workaround;
> 
>       /*
>        *  EHCI spec version 1.0 Section 2.3
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 4a1f7cad73..83114bae90 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -858,6 +858,9 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < sc->ehcis_num; i++) {
>           object_property_set_bool(OBJECT(&s->ehci[i]), "caps-64bit-addr", true,
>                                    &error_abort);
> +        object_property_set_bool(OBJECT(&s->ehci[i]),
> +                                 "aspeed-ast2700-workaround", true,
> +                                 &error_abort);
>           if (!sysbus_realize(SYS_BUS_DEVICE(&s->ehci[i]), errp)) {
>               return;
>           }
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 61215e9f3d..01f72a5a1e 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -36,6 +36,9 @@ static const Property ehci_sysbus_properties[] = {
>                        false),
>       DEFINE_PROP_BOOL("caps-64bit-addr", EHCISysBusState, ehci.caps_64bit_addr,
>                        false),
> +    DEFINE_PROP_BOOL("aspeed-ast2700-workaround", EHCISysBusState,
> +                     ehci.aspeed_ast2700_workaround,
> +                     false),
>   };
> 
>   static void usb_ehci_sysbus_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 7f90fd119d..403d7e84a7 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1133,6 +1133,9 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
>                             "      64-bit addressing capability is disabled\n");
>               return;
>           }
> +        if (s->aspeed_ast2700_workaround) {
> +            val |= BIT(2);
> +        }

With the proposal above, this would be changed to :

	val |= s->ctrldssegment_default;

>           break;
>       case ASYNCLISTADDR:
>           if (ehci_async_enabled(s)) {
> @@ -2582,6 +2585,9 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
>       }
>       if (s->caps_64bit_addr) {
>           s->caps[0x08] |= BIT(0);
> +        if (s->aspeed_ast2700_workaround) {
> +            s->ctrldssegment |= BIT(2);

Shouldn't these assignments be in the reset handlers ?

Thanks,

C.


> +        }
>       }