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

[Yu, Richard]

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>;
};

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

> Best regards,
> Krzysztof

Thank very much.

Richard.