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

  From: Richard Yu <richard.yu@hpe.com>

Provide access to the two register regions for GXP Virtual EHCI
controller through the hpe,gxp-udcg binding.

Signed-off-by: Richard Yu <richard.yu@hpe.com>
---
 .../devicetree/bindings/usb/hpe,gxp-udcg.yaml | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml
  

On 06/07/2023 23:59, richard.yu@hpe.com wrote:
> From: Richard Yu <richard.yu@hpe.com>
> 
> Provide access to the two register regions for GXP Virtual EHCI
> controller through the hpe,gxp-udcg binding.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Richard Yu <richard.yu@hpe.com>
> ---
>  .../devicetree/bindings/usb/hpe,gxp-udcg.yaml | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml b/Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml
> new file mode 100644
> index 000000000000..e6746374f97d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/hpe,gxp-udcg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +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.

> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +  - Richard Yu <richard.yu@hpe.com>
> +
> +description: |+

Drop |+

> +  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.

> +properties:
> +  compatible:
> +    enum:
> +      - hpe,gxp-udcg
> +
> +  reg:
> +    items:
> +      - description: UDC Global (UDCG) config controller
> +      - description: UDC Invidual config/interrupt controllers
> +
> +  reg-names:
> +    items:
> +      - const: udcg
> +      - const: udc
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  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?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 4
> +    minimum: 1
> +    maximum: 8
> +
> +  hpe,vehci-generic-endpoints:
> +    description: Number of generic endpoints supported by the GXP
> +    $ref: /schemas/types.yaml#/definitions/uint32

Same concerns.

> +    default: 16
> +    minimum: 1
> +    maximum: 16
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - hpe,vehci-downstream-ports
> +  - hpe,vehci-generic-endpoints
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    udcg@80400800 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof
  

>> +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
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"

>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + default: 4
>> + minimum: 1
>> + maximum: 8
>> +
>> + hpe,vehci-generic-endpoints:
>> + description: Number of generic endpoints supported by the GXP
>> + $ref: /schemas/types.yaml#/definitions/uint32


> Same concerns.

We intend to change this to "hpe,vhub-generic-endpoints" following
ASPEED's "aspeed,vhub-generic-endpoints"

>> +examples:
>> + - |
>> + udcg@80400800 {


> Node names should be generic. See also an explanation and list of
...
We will use usb-hub.

Thank you for your feedback,
Richard Yu
  

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

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

Best regards,
Krzysztof
  

Thank you, Mr. Kozlowski, for your feedback.

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: Saturday, August 5, 2023 2:09 PM
To: Yu, Richard <richard.yu@hpe.com>; Verdun, Jean-Marie <verdun@hpe.com>; Hawkins, Nick <nick.hawkins@hpe.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.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@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.