On 09/08/2023 17:52, Yu, Richard wrote:
> Thank you, Mr. Kozlowski, for your feedback.
>
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Saturday, August 5, 2023 2:09 PM
> To: Yu, Richard <richard.yu@xxxxxxx>; Verdun, Jean-Marie <verdun@xxxxxxx>; Hawkins, Nick <nick.hawkins@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 1/3] dt-bindings: usb: Add HPE GXP UDCG Controller
>
> On 01/08/2023 20:07, Yu, Richard wrote:
>>>
>>>>> +title: HPE GXP USB Virtual EHCI controller
>>>
>>>> The word "virtual" in bindings pretty often raises questions, because
>>>> we describe usually real hardware, not virtual. Some explanation in
>>>> description would be useful.
>>>
>>> Here we are working with virtual devices that are created and have no
>
>> Unfortunately I do not know what are virtual devices which do not exist physically.
>> I have serious doubts that they fit Devicetree purpose...
>
> In our HPE gxp, we have an EHCI device which it is present to host as
> standard EHCI controller. However, this EHCI controller does not have
> any physical port. Users can figure this EHCI controller to have 1 to 8 ports
> (we call it as virtual ports) and attached 1 to 8 UDC devices (we
> call them as virtual devices). User can figure each port/device to
> have 1 to 16 endpoints.
>
> I am writing his driver to create the device descriptor entry for each port/UDC.
> /sys/bus/platform/devices/80400800.vhub/80400800.vhub:p1 .... Thus,
> the OpenBmc KVM can use them as virtual mouse/keyboard, virtual NIC ....
>
> I am implementing this driver using the Aspeed virtual hub driver as example.
>
> Just like the Aspeed virtual hub is in the Devicetree:
>
> vhub: usb-vhub@1e6a0000 {
> compatible = "aspeed,ast2600-usb-vhub";
> reg = <0x1e6a0000 0x350>;
> interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
> aspeed,vhub-downstream-ports = <7>;
> aspeed,vhub-generic-endpoints = <21>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_usb2ad_default>;
> status = "disabled";
> };
>
> In my case: (I am replacing "udcg" with "vhub" and remove the vehci reference).
>
> vhub: usb-vhub@80400800 {
> compatible = "hpe,gxp-vhub";
> reg = <0x80400800 0x0200>, <0x80401000 0x8000>;
> reg-names = "vhub", "udc";
> interrupts = <13>;
> interrupt-parent = <&vic1>;
> hpe,vhub-downstream-ports = <4>;
> hpe,vhub-generic-endpoints = <16>;
> };
The hub is not virtual, it is real. I understand that it is some
software block or FPGA, but still I propose to skip any references to
virtual.
>
>>> physical presence. We have modeled our code off of ASPEED's VHUB
>>> implementation to comply with the implementation in OpenBMC.
>>>
>>>>> + The HPE GXP USB Virtual EHCI Controller implements 1 set of USB
>>>>> + EHCI register and several sets of device and endpoint registers to
>>>>> + support the virtual EHCI's downstream USB devices.
>>>>> +
>>>
>>>
>>>> If this is EHCI controller, then I would expect here reference to usb-hcd.
>>>
>>> We will remove references to EHCI in code and documentation. It has
>>> been modeled to following ASPEEDs approach as mentioned above.
>>>
>>>> + hpe,vehci-downstream-ports:
>>>> + description: Number of downstream ports supported by the GXP
>>
>>
>>>> Why do you need this property in DT and what exactly does it represent?
>>>> You have one device - EHCI controller - and on some boards it is
>>>> further customized? Even though it is the same device?
>>>
>>> That is correct. We can configure this VHUB Controller to have one to
>>> 8 virtual ports. This is similar to the aspeed virtual USB HUB
>>> "aspeed,vhub-downstream-ports" moving forward in the next patch we are
>>> going to use "hpe,vhub-downstream-ports"
>
>> Moving forward you need to address this lack of physical presence...
>> Aren't these different devices and you just forgot to customize the compatible?
>
> I don’t fully understand here. Isn't the lack of physical presence similar to the
> Aspeed virtual hub driver?
I don't know Aspeed virtual hub driver. In any case, driver is
irrelevant to the bindings.
Why setting maximum number of downstream ports or devices would be
needed per-board? Do you save some resources that way?
Best regards,
Krzysztof
