Re: [PATCH v1 1/3] dt-bindings: usb: Add HPE GXP UDCG Controller

[Krzysztof Kozlowski]

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