diff mbox series

[1/1] hw/arm/sbsa-ref: set 'slots' property of xhci

Message ID 20230710063750.473510-2-wangyuquan1236@phytium.com.cn (mailing list archive)
State New, archived
Headers show
Series hw/arm/sbsa-ref: set 'slots' property of xhci | expand

Commit Message

Yuquan Wang July 10, 2023, 6:37 a.m. UTC
This extends the slots of xhci to 64, since the default xhci_sysbus
just supports one slot.

Signed-off-by: Wang Yuquan <wangyuquan1236@phytium.com.cn>
Signed-off-by: Chen Baozi <chenbaozi@phytium.com.cn>
---
 hw/arm/sbsa-ref.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Richard Henderson July 10, 2023, 7:28 a.m. UTC | #1
On 7/10/23 07:37, Yuquan Wang wrote:
> This extends the slots of xhci to 64, since the default xhci_sysbus
> just supports one slot.
> 
> Signed-off-by: Wang Yuquan <wangyuquan1236@phytium.com.cn>
> Signed-off-by: Chen Baozi <chenbaozi@phytium.com.cn>
> ---
>   hw/arm/sbsa-ref.c | 1 +
>   1 file changed, 1 insertion(+)


hw/usb/hcd-xhci-nec.c:    DEFINE_PROP_UINT32("slots", XHCINecState, slots, XHCI_MAXSLOTS),
hw/usb/hcd-xhci-sysbus.c:    DEFINE_PROP_UINT32("slots", XHCISysbusState, xhci.numslots, 
XHCI_MAXSLOTS),

The default is XCHI_MAXSLOTS, not 1.  So I can't see why you'd need this.

r~

> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 64e1cbce17..bc89eb4806 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -611,6 +611,7 @@ static void create_xhci(const SBSAMachineState *sms)
>       hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
>       int irq = sbsa_ref_irqmap[SBSA_XHCI];
>       DeviceState *dev = qdev_new(TYPE_XHCI_SYSBUS);
> +    qdev_prop_set_uint32(dev, "slots", XHCI_MAXSLOTS);
>   
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
Marcin Juszkiewicz July 10, 2023, 7:33 a.m. UTC | #2
W dniu 10.07.2023 o 09:28, Richard Henderson pisze:
> 
> hw/usb/hcd-xhci-nec.c:    DEFINE_PROP_UINT32("slots", XHCINecState, 
> slots, XHCI_MAXSLOTS),
> hw/usb/hcd-xhci-sysbus.c:    DEFINE_PROP_UINT32("slots", 
> XHCISysbusState, xhci.numslots, XHCI_MAXSLOTS),
> 
> The default is XCHI_MAXSLOTS, not 1.  So I can't see why you'd need this.

There are two systems using XHCI: i386/microvm and arm/sbsa. First
one sets amount of slots already.

Without this patch Linux complains that there is only one port and
refuses to connect second usb device:

xhci-hcd PNP0D10:00: Error while assigning device slot ID: No Slots Available Error
xhci-hcd PNP0D10:00: Max number of devices this xHCI host supports is 1.
usb usb1-port2: couldn't allocate usb_device

So it looks like default being XHCI_MAXSLOTS is not applied somewhere.
Marcin Juszkiewicz July 10, 2023, 7:34 a.m. UTC | #3
W dniu 10.07.2023 o 08:37, Yuquan Wang pisze:
> This extends the slots of xhci to 64, since the default xhci_sysbus
> just supports one slot.
> 
> Signed-off-by: Wang Yuquan <wangyuquan1236@phytium.com.cn>
> Signed-off-by: Chen Baozi <chenbaozi@phytium.com.cn>
> ---
>   hw/arm/sbsa-ref.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 64e1cbce17..bc89eb4806 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -611,6 +611,7 @@ static void create_xhci(const SBSAMachineState *sms)
>       hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
>       int irq = sbsa_ref_irqmap[SBSA_XHCI];
>       DeviceState *dev = qdev_new(TYPE_XHCI_SYSBUS);
> +    qdev_prop_set_uint32(dev, "slots", XHCI_MAXSLOTS);

Looks like tab instead of spaces.

>   
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);

Tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Reviewed-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>


Before:

xhci-hcd PNP0D10:00: Error while assigning device slot ID: No Slots Available Error
xhci-hcd PNP0D10:00: Max number of devices this xHCI host supports is 1.
usb usb1-port2: couldn't allocate usb_device


After:

input: QEMU QEMU USB Keyboard as /devices/platform/PNP0D10:00/usb1/1-1/1-1:1.0/0003:0627:0001.0001/input/input0
hid-generic 0003:0627:0001.0001: input,hidraw0: USB HID v1.11 Keyboard [QEMU QEMU USB Keyboard] on usb-PNP0D10:00-1/input0
input: QEMU QEMU USB Tablet as /devices/platform/PNP0D10:00/usb1/1-2/1-2:1.0/0003:0627:0001.0002/input/input1
hid-generic 0003:0627:0001.0002: input,hidraw1: USB HID v0.01 Mouse [QEMU QEMU USB Tablet] on usb-PNP0D10:00-2/input0
Richard Henderson July 10, 2023, 8:14 a.m. UTC | #4
On 7/10/23 08:33, Marcin Juszkiewicz wrote:
> W dniu 10.07.2023 o 09:28, Richard Henderson pisze:
>>
>> hw/usb/hcd-xhci-nec.c:    DEFINE_PROP_UINT32("slots", XHCINecState, slots, XHCI_MAXSLOTS),
>> hw/usb/hcd-xhci-sysbus.c:    DEFINE_PROP_UINT32("slots", XHCISysbusState, xhci.numslots, 
>> XHCI_MAXSLOTS),
>>
>> The default is XCHI_MAXSLOTS, not 1.  So I can't see why you'd need this.
> 
> There are two systems using XHCI: i386/microvm and arm/sbsa. First
> one sets amount of slots already.
> 
> Without this patch Linux complains that there is only one port and
> refuses to connect second usb device:
> 
> xhci-hcd PNP0D10:00: Error while assigning device slot ID: No Slots Available Error
> xhci-hcd PNP0D10:00: Max number of devices this xHCI host supports is 1.
> usb usb1-port2: couldn't allocate usb_device
> 
> So it looks like default being XHCI_MAXSLOTS is not applied somewhere.

It looks like the problem is that we're performing init and realize in one step.  The 
defaults would be applied later, along with other properties from the command-line.

As this is the last full day before softfreeze, and we have many other instances of the 
same pattern, I'll give this an

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Philippe Mathieu-Daudé July 10, 2023, 10:17 a.m. UTC | #5
On 10/7/23 09:33, Marcin Juszkiewicz wrote:
> W dniu 10.07.2023 o 09:28, Richard Henderson pisze:
>>
>> hw/usb/hcd-xhci-nec.c:    DEFINE_PROP_UINT32("slots", XHCINecState, 
>> slots, XHCI_MAXSLOTS),
>> hw/usb/hcd-xhci-sysbus.c:    DEFINE_PROP_UINT32("slots", 
>> XHCISysbusState, xhci.numslots, XHCI_MAXSLOTS),
>>
>> The default is XCHI_MAXSLOTS, not 1.  So I can't see why you'd need this.
> 
> There are two systems using XHCI: i386/microvm and arm/sbsa. First
> one sets amount of slots already.
> 
> Without this patch Linux complains that there is only one port and
> refuses to connect second usb device:
> 
> xhci-hcd PNP0D10:00: Error while assigning device slot ID: No Slots 
> Available Error
> xhci-hcd PNP0D10:00: Max number of devices this xHCI host supports is 1.
> usb usb1-port2: couldn't allocate usb_device
> 
> So it looks like default being XHCI_MAXSLOTS is not applied somewhere.

Or something sets it to 1 elsewhere.

At a glance xhci_sysbus_class_init() doesn't call
device_class_set_parent_FOO().
diff mbox series

Patch

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 64e1cbce17..bc89eb4806 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -611,6 +611,7 @@  static void create_xhci(const SBSAMachineState *sms)
     hwaddr base = sbsa_ref_memmap[SBSA_XHCI].base;
     int irq = sbsa_ref_irqmap[SBSA_XHCI];
     DeviceState *dev = qdev_new(TYPE_XHCI_SYSBUS);
+    qdev_prop_set_uint32(dev, "slots", XHCI_MAXSLOTS);
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);