| Message ID | 20230706215910.78772-2-richard.yu@hpe.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add USB driver for HPE GXP Architecture | expand |
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
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.
On 09/08/2023 17:52, Yu, Richard wrote: > 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>; > }; 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
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@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. 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. 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 Just like aspeed: In g5 (aspeed-g5.dtsi), aspeed,vhub-downstream-ports = <5>; In g6 (aspeed-g6.dtsi), aspeed,vhub-downstream-ports = <7>; > Best regards, > Krzysztof Thanks Richard.
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@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. > > 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
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 + +maintainers: + - Nick Hawkins <nick.hawkins@hpe.com> + - Richard Yu <richard.yu@hpe.com> + +description: |+ + 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. + +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 + $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 + 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 { + compatible = "hpe,gxp-udcg"; + reg = <0x80400800 0x0200>, <0x80401000 0x8000>; + reg-names = "udcg", "udc"; + interrupts = <13>; + interrupt-parent = <&vic1>; + hpe,vehci-downstream-ports = <4>; + hpe,vehci-generic-endpoints = <16>; + };