[v1,2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget

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

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
  

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