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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <834800aa-65f4-4c99-4586-51a24355bc59@linaro.org>
Date:   Sat, 19 Aug 2023 20:30:30 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     "Yu, Richard" <richard.yu@....com>,
        "Verdun, Jean-Marie" <verdun@....com>,
        "Hawkins, Nick" <nick.hawkins@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Chang, Clay" <clayc@....com>
Subject: Re: [PATCH v1 1/3] dt-bindings: usb: Add HPE GXP UDCG Controller

On 09/08/2023 17:52, Yu, Richard wrote:
> Thank you, Mr. Kozlowski, for your feedback.
> 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> 
> Sent: Saturday, August 5, 2023 2:09 PM
> To: Yu, Richard <richard.yu@....com>; Verdun, Jean-Marie <verdun@....com>; Hawkins, Nick <nick.hawkins@....com>; gregkh@...uxfoundation.org; robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org; conor+dt@...nel.org; linux-usb@...r.kernel.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
> 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@...a0000 {
> 	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@...00800 {
> 	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