| Message ID | 20221103160625.15574-1-richard.yu@hpe.com |
|---|---|
| Headers | show |
| Series | Add USB Driver for HPE GXP Architecture | expand |
On 03/11/2022 12:06, richard.yu@hpe.com wrote: > From: Richard Yu <richard.yu@hpe.com> > Subject: Drop redundant second "binding" word. > Create documentation for the hpe,gxp-udc binding to support access to > the virtual USB device. > > Signed-off-by: Richard Yu <richard.yu@hpe.com> > --- > .../devicetree/bindings/usb/hpe,gxp-udc.yaml | 57 +++++++++++++++++++ > 1 file changed, 57 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > > diff --git a/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml b/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > new file mode 100644 > index 000000000000..f1ec4df8c3d3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > @@ -0,0 +1,57 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/usb/hpe,gxp-udc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: HPE GXP Gadget Universal Device Controller (UDC) > + > +maintainers: > + - Richard Yu <richard.yu@hpe.com> > + - Jean-Marie Verdun <verdun@hpe.com> > + - Nick Hawkins <nick.hawkins@hpe.com> > + > +properties: > + compatible: > + const: hpe,gxp-udc > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + vdevnum: > + description: > + virtual device number. That's unusual property... Why numbering devices is part of DT (hardware description)? > + > + fepnum: > + description: > + number of the flexible end-points this device is needed. Similar question. BTW, if you end sentence with '.', it means it is an sentence, so you need to start it with capital letter. > + > + hpe,syscon-phandle: phandle is redudant. You need rather specific name, so "hpe,ehci-syscon" or whatever it is. > + $ref: '/schemas/types.yaml#/definitions/phandle' No quotes. > + description: > + Phandle to the gxp vEHCI controller access vDevice registers. Drop "Phandle to" Isn't "gxp" a GXP? > + > +required: > + - compatible > + - reg > + - interrupts > + - vdevnum > + - fepnum > + - hpe,syscon-phandle > + > +additionalProperties: false > + > +examples: > + - | > + udc@80401000 { Node name "usb", I think it is more popular for USB controllers. > + compatible = "hpe,gxp-udc"; > + reg = <0x80401000 0x1000>; > + interrupts = <13>; > + interrupt-parent = <&vic1>; > + vdevnum = <0>; > + fepnum = <7>; > + hpe,syscon-phandle = <&udc_system_controller>; > + }; Best regards, Krzysztof
Hi Mr. Kozlowski, Thank you very much for your quick review and feedbacks. I will modify the patches based on your feedback accordingly. On this specific patch, you have questions on how we defined the device/gadget configurations: vdevnum and fepnum. Please see my answers following the questions: > + vdevnum: > + description: > + virtual device number. That's unusual property... Why numbering devices is part of DT (hardware description)? >> Richard: In HPE GXP virtual EHCI controller chipset, it can support up to 8 virtual devices(gadgets). Each device/gadget will be represented by a bit in 8 bits register. For example, the interrupt register bit 0 indicates the interrupt from device 0, bit 1 for device 1 ... so on. When an user defines a device/gadget, he/she can define the device number as between 0 and 7. Thus, the driver can up to the bit position. That is why we have numbering devices as port of DT. > + > + fepnum: > + description: > + number of the flexible end-points this device is needed. Similar question. >>Richard: In HPE GXP virtual EHCI Controller chipset, there is a flexible EP pool. Each flexible EP has its own mapping register. The mapping register bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. The device driver configures the mapping register to assign a flexible EP to a specific device. Here, "fepnum" is the input letting the driver know how many EP is needed for this device/gadget. Hope I have answered your questions on "vdevnum" and "fepnum". I will modify this patch to address your other review feedback. Thank you very much again. Richard. -----Original Message----- From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Sent: Thursday, November 3, 2022 11:30 AM 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; linux@armlinux.org.uk; balbi@kernel.org; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget On 03/11/2022 12:06, richard.yu@hpe.com wrote: > From: Richard Yu <richard.yu@hpe.com> > Subject: Drop redundant second "binding" word. > Create documentation for the hpe,gxp-udc binding to support access to > the virtual USB device. > > Signed-off-by: Richard Yu <richard.yu@hpe.com> > --- > .../devicetree/bindings/usb/hpe,gxp-udc.yaml | 57 > +++++++++++++++++++ > 1 file changed, 57 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > > diff --git a/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > b/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > new file mode 100644 > index 000000000000..f1ec4df8c3d3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml > @@ -0,0 +1,57 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2 > +--- > +$id: > +INVALID URI REMOVED > +-udc.yaml*__;Iw!!NpxR!lKDMWE_W38KLY2gXH0dY1rG-bU4JwIyme_DMzeppYO9DQ1S > +wvXeID3RDEEuKBSG81_hsD5gntF_elZhC9yiXT-MvFA$ > +$schema: > +INVALID URI REMOVED > +aml*__;Iw!!NpxR!lKDMWE_W38KLY2gXH0dY1rG-bU4JwIyme_DMzeppYO9DQ1SwvXeID > +3RDEEuKBSG81_hsD5gntF_elZhC9yi3VX-gPg$ > + > +title: HPE GXP Gadget Universal Device Controller (UDC) > + > +maintainers: > + - Richard Yu <richard.yu@hpe.com> > + - Jean-Marie Verdun <verdun@hpe.com> > + - Nick Hawkins <nick.hawkins@hpe.com> > + > +properties: > + compatible: > + const: hpe,gxp-udc > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + vdevnum: > + description: > + virtual device number. That's unusual property... Why numbering devices is part of DT (hardware description)? > + > + fepnum: > + description: > + number of the flexible end-points this device is needed. Similar question. BTW, if you end sentence with '.', it means it is an sentence, so you need to start it with capital letter. > + > + hpe,syscon-phandle: phandle is redudant. You need rather specific name, so "hpe,ehci-syscon" or whatever it is. > + $ref: '/schemas/types.yaml#/definitions/phandle' No quotes. > + description: > + Phandle to the gxp vEHCI controller access vDevice registers. Drop "Phandle to" Isn't "gxp" a GXP? > + > +required: > + - compatible > + - reg > + - interrupts > + - vdevnum > + - fepnum > + - hpe,syscon-phandle > + > +additionalProperties: false > + > +examples: > + - | > + udc@80401000 { Node name "usb", I think it is more popular for USB controllers. > + compatible = "hpe,gxp-udc"; > + reg = <0x80401000 0x1000>; > + interrupts = <13>; > + interrupt-parent = <&vic1>; > + vdevnum = <0>; > + fepnum = <7>; > + hpe,syscon-phandle = <&udc_system_controller>; > + }; Best regards, Krzysztof
On 04/11/2022 16:03, Yu, Richard wrote: > Hi Mr. Kozlowski, > > Thank you very much for your quick review and feedbacks. > > I will modify the patches based on your feedback accordingly. > > On this specific patch, you have questions on how we defined the device/gadget configurations: vdevnum and fepnum. > > Please see my answers following the questions: > >> + vdevnum: >> + description: >> + virtual device number. > > That's unusual property... Why numbering devices is part of DT (hardware description)? > >>> Richard: In HPE GXP virtual EHCI controller chipset, it can support up to 8 virtual devices(gadgets). Each device/gadget will be represented by a bit in 8 bits register. For example, the interrupt register bit 0 indicates the interrupt from device 0, bit 1 for device 1 ... so on. When an user defines a device/gadget, he/she can define the device number as between 0 and 7. Thus, the driver can up to the bit position. That is why we have numbering devices as port of DT. > >> + >> + fepnum: >> + description: >> + number of the flexible end-points this device is needed. > > Similar question. > >>> Richard: In HPE GXP virtual EHCI Controller chipset, there is a flexible EP pool. Each flexible EP has its own mapping register. The mapping register bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. The device driver configures the mapping register to assign a flexible EP to a specific device. Here, "fepnum" is the input letting the driver know how many EP is needed for this device/gadget. > > Hope I have answered your questions on "vdevnum" and "fepnum". Unfortunately I don't see your answers... Or actually I am not sure what is the answer and what is not. What is unusual, you did not quote my email but quoted something else. Please send it again, but following normal mailing list netiquette for replies. Here is one: https://en.opensuse.org/openSUSE:Mailing_list_netiquette Just don't use corporate style of emails on mailing list. We usually cannot handle them... Best regards, Krzysztof
Hi Mr. Kozlowski, > > + > > + vdevnum: > > + description: > > + virtual device number. > That's unusual property... Why numbering devices is part of DT (hardware description)? In HPE GXP virtual EHCI controller chipset, it can support up to 8 virtual devices(gadgets). Each device/gadget will be represented by a bit in 8 bits register. For example, the interrupt register bit 0 indicates the interrupt from device 0, bit 1 for device 1 ... so on. When a user defines a device/gadget, he/she can define the device number as between 0 and 7. Thus, the driver can look up to the bit position. That is why we have numbering devices as part of DT. > > + > > + fepnum: > > + description: > > + number of the flexible end-points this device is needed. > Similar question. In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. Each flexible EP has its own mapping register. The mapping register bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. The device driver configures the mapping register to assign a flexible EP to a specific device. Here, "fepnum" is the input letting the driver know how many EPs are needed for this device/gadget. Thanks Richard.
On 07/11/2022 21:16, Yu, Richard wrote: > Hi Mr. Kozlowski, > >>> + >>> + vdevnum: >>> + description: >>> + virtual device number. > >> That's unusual property... Why numbering devices is part of DT (hardware description)? > > In HPE GXP virtual EHCI controller chipset, it can support up to 8 virtual devices(gadgets). Each device/gadget will be represented by a bit in 8 bits register. For example, the interrupt register bit 0 indicates the interrupt from device 0, bit 1 for device 1 ... so on. When a user defines a device/gadget, he/she can define the device number as between 0 and 7. Thus, the driver can look up to the bit position. That is why we have numbering devices as part of DT. Wrap your lines properly, it's impossible to reply in-line to such messages. Then how do you specify two devices? You allow here only one, right? Which bit in which register? Your devices have separate address space, so why they cannot poke the same register, right? Then just always set it to 0... I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings. > >>> + >>> + fepnum: >>> + description: >>> + number of the flexible end-points this device is needed. > >> Similar question. > > In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. Each flexible EP has its own mapping register. The mapping register bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. The device driver configures the mapping register to assign a flexible EP to a specific device. Here, "fepnum" is the input letting the driver know how many EPs are needed for this device/gadget. Nope. So you create here some weird IDs to poke into syscon register. First, syscon has offset if you need. You could treat it maybe as bits? I don't know... but even then your design is poor - two devices changing the same register. Even though it is sunchronized by regmap, it is conflicting, obfuscated access. Best regards, Krzysztof
Hi Mr. Kozlowski, Thank you very much for inputs. >>>> + >>>> + vdevnum: >>>> + description: >>>> + virtual device number. > >>> That's unusual property... Why numbering devices is part of DT (hardware description)? > >> In HPE GXP virtual EHCI controller chipset, it can support up to 8 >> virtual devices(gadgets). Each device/gadget will be represented by >> a bit in 8 bits register. For example, the interrupt register bit 0 >> indicates the interrupt from device 0, bit 1 for device 1 ... so on. >> When a user defines a device/gadget, he/she can define the device >> number as between 0 and 7. Thus, the driver can look up to the bit >> position. That is why we have numbering devices as part of DT. > Wrap your lines properly, it's impossible to reply in-line to such messages. Sorry for the improper wrapping. Hope the above fixed the problem. > Then how do you specify two devices? You allow here only one, right? In our current design, to specify two devices, we added the gadget structure into the device tree, such as gadget0:udc@80401000{}; gadget1:udc@80402000{};.... No, we can allow up to 8 devices by adding the gadget structure, such as gadget0:udc@80401000{}; gadget1:udc@80402000{};....gadget8:udc@80408000{}; > Which bit in which register? Your devices have separate address space, so why they cannot poke the same register, right? Then just always set it to 0... In HPE GXP vEHCI controller, there are three register groups: standard USB EHCI registers, virtual device global registers, and virtual device registers. Standard USB EHCI registers ---- We defined as "hpe,gxp-vudc" in the device tree (vuhc0) Virtual device global registers --- We defined as "hpe,gxp-udcg" Virtual device registers -- We defined as "hpe,gxp-udc" Each virtual device will have its own separate address space. There is only single address space for the virtual device global registers. The virtual device global registers are including vDevice Global Interrupt Status register(EVGISTAT), vDevice Global Interrupt Enable register(EVGIEN), vEHCI FlexEndpoint Mapping register (EVFEMAP) .... We need the vdevnum for the bit position in EVGISTAT and EVGIEN for each device. We write vdevnum into the EVFEMAP register to assign an EP to a specific device. > I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings. We create "vdevnum" as device configuration parameter due to our hardware need. >>>> + >>>> + fepnum: >>>> + description: >>> >+ number of the flexible end-points this device is needed. > > >> >Similar question. > > > >In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. >>Each flexible EP has its own mapping register. The mapping register >>bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. >>The device driver configures the mapping register to assign a flexible >>EP to a specific device. Here, "fepnum" is the input letting the >>driver know how many EPs are needed for this device/gadget. >Nope. So you create here some weird IDs to poke into syscon register. >First, syscon has offset if you need. You could treat it maybe as bits? >I don't know... but even then your design is poor - two devices >changing the same register. Even though it is sunchronized by regmap, it is conflicting, obfuscated access. The "fepnum" is the input parameter to define how many end-points (EPs) is needed for the device. You are correct that all devices need to access the virtual device global registers during the runtime. Thus, we create " hpe,syscon-phandle = <&udc_system_controller>;' for the driver getting the vDevice Global registers address. In our current chip registers layout with the vDevice Global registers, I don’t see a way to avoid "two devices changing the same register". Thank you very much for your time. Richard.
On 09/11/2022 04:37, Yu, Richard wrote: > Hi Mr. Kozlowski, > > Thank you very much for inputs. > >>>>> + >>>>> + vdevnum: >>>>> + description: >>>>> + virtual device number. >> >>>> That's unusual property... Why numbering devices is part of DT (hardware description)? >> >>> In HPE GXP virtual EHCI controller chipset, it can support up to 8 >>> virtual devices(gadgets). Each device/gadget will be represented by >>> a bit in 8 bits register. For example, the interrupt register bit 0 >>> indicates the interrupt from device 0, bit 1 for device 1 ... so on. >>> When a user defines a device/gadget, he/she can define the device >>> number as between 0 and 7. Thus, the driver can look up to the bit >>> position. That is why we have numbering devices as part of DT. > >> Wrap your lines properly, it's impossible to reply in-line to such messages. > > Sorry for the improper wrapping. Hope the above fixed the problem. > >> Then how do you specify two devices? You allow here only one, right? > > In our current design, to specify two devices, we added the gadget > structure into the device tree, such as gadget0:udc@80401000{}; gadget1:udc@80402000{};.... > > No, we can allow up to 8 devices by adding the gadget structure, > such as gadget0:udc@80401000{}; gadget1:udc@80402000{};....gadget8:udc@80408000{}; > >> Which bit in which register? Your devices have separate address space, so why they cannot poke the same register, right? Then just always set it to 0... > > In HPE GXP vEHCI controller, there are three register groups: standard USB EHCI registers, > virtual device global registers, and virtual device registers. > > Standard USB EHCI registers ---- We defined as "hpe,gxp-vudc" in the device tree (vuhc0) > Virtual device global registers --- We defined as "hpe,gxp-udcg" > Virtual device registers -- We defined as "hpe,gxp-udc" > > Each virtual device will have its own separate address space. > There is only single address space for the virtual device global registers. > > The virtual device global registers are including vDevice Global Interrupt Status register(EVGISTAT), > vDevice Global Interrupt Enable register(EVGIEN), vEHCI FlexEndpoint Mapping register (EVFEMAP) .... > We need the vdevnum for the bit position in EVGISTAT and EVGIEN for each device. > We write vdevnum into the EVFEMAP register to assign an EP to a specific device. > >> I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings. > > We create "vdevnum" as device configuration parameter due to our hardware need. That's not an argument... everything can be a "hardware need". > >>>>> + >>>>> + fepnum: >>>>> + description: >>>>> + number of the flexible end-points this device is needed. >>> >>>> Similar question. >>> >>> In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. >>> Each flexible EP has its own mapping register. The mapping register >>> bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device. >>> The device driver configures the mapping register to assign a flexible >>> EP to a specific device. Here, "fepnum" is the input letting the >>> driver know how many EPs are needed for this device/gadget. > >> Nope. So you create here some weird IDs to poke into syscon register. >> First, syscon has offset if you need. You could treat it maybe as bits? >> I don't know... but even then your design is poor - two devices >> changing the same register. Even though it is sunchronized by regmap, it is conflicting, obfuscated access. > > The "fepnum" is the input parameter to define how many end-points (EPs) is needed > for the device. > > You are correct that all devices need to access the virtual > device global registers during the runtime. > Thus, we create " hpe,syscon-phandle = <&udc_system_controller>;' > for the driver getting the vDevice Global registers address. And how do you solve poking into the same register by two devices? Who owns it? You don't... > > In our current chip registers layout with the vDevice Global registers, I don’t see > a way to avoid "two devices changing the same register". I see at least an idea - create proper hierarchy, where parent device instantiates its children (thus knows and increments the IDs) and is responsible for proper handling of shared register (thus the parent owns the register). I understand why you created vdevnum/fepnum properties but the reason is not matching DT bindings. These are not additional hardware properties which deserve their own DT properties - they are already part of unit address and/or just incremented ID based on device number managed by a parent. Best regards, Krzysztof
From: Richard Yu <richard.yu@hpe.com> The GXP vEHCI controller presents an 8 port EHCI compatible PCI function to host software. Each EHCI port is logically connected to a corresponding set of virtual device registers. Richard Yu (7): usb: gadget: udc: gxp_udc: add gxp USB support dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget dt-bindings: usb: hpe,gxp-udcg: Add binding for gxp gadget group dt-bindings: usb: hpe,gxp-vuhc: add binding for gxp vEHCI ARM: dts: hpe: Add UDC nodes ARM: configs: multi_v7_defconfig: Enable HPE GXP USB Driver MAINTAINERS: add USB support to GXP .../devicetree/bindings/usb/hpe,gxp-udc.yaml | 57 + .../devicetree/bindings/usb/hpe,gxp-udcg.yaml | 34 + .../devicetree/bindings/usb/hpe,gxp-vuhc.yaml | 34 + MAINTAINERS | 5 + arch/arm/boot/dts/hpe-gxp.dtsi | 30 + arch/arm/configs/multi_v7_defconfig | 1 + drivers/usb/gadget/udc/Kconfig | 6 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/gxp_udc.c | 1300 +++++++++++++++++ 9 files changed, 1468 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-udc.yaml create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-udcg.yaml create mode 100644 Documentation/devicetree/bindings/usb/hpe,gxp-vuhc.yaml create mode 100644 drivers/usb/gadget/udc/gxp_udc.c