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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <541ce5de-88b2-7d67-6aaa-2faaeb4f6703@linaro.org>
Date:   Thu, 24 Aug 2023 08:31:50 +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 23/08/2023 18:07, Yu, Richard wrote:
> 
> Thank you, Mr. Kozlowski.
> 
>>> 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.
> 
> I will remove any references to "virtual" in comment and documentation.
> 
> 
>>>>>> + 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?
> 
> That is correct. Each port/devices will have to allocate resources and create device descriptor entry.

The answer to "why" is not "that is correct".

> Currently, I set the number of downstream ports to be 4. Thus, I will have:
> 
> /sys/bus/platform/devices/80400800.vhub/80400800.vhub:p1   <=== for kvm keyboard/mouse
> /sys/bus/platform/devices/80400800.vhub/80400800.vhub:p2   <=== for virtual CD/DVD/ISO image
> /sys/bus/platform/devices/80400800.vhub/80400800.vhub:p3   <=== for virtual USB key
> /sys/bus/platform/devices/80400800.vhub/80400800.vhub:p4   <=== for virtual NIC

So resources in Linux? That's not really relevant and important. I still
do not see the need of this property.

> 
> Just like aspeed:
> In g5 (aspeed-g5.dtsi), aspeed,vhub-downstream-ports = <5>;
> In g6 (aspeed-g6.dtsi), aspeed,vhub-downstream-ports = <7>;

I did not review that. Poor or incorrect example is not an argument. If
they introduced obvious bugs or obvious non-DT properties, shall you do
the same?

Best regards,
Krzysztof