[v2,1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller

Commit Message

Ng, Adrian Ho Yin July 24, 2023, 6:36 a.m. UTC

From: Adrian Ng Ho Yin <[e-mail]>

Existing binding intel,keembay-dwc3.yaml does not have the required
properties for Intel SoCFPGA devices.
Introduce new binding description for Intel SoCFPGA USB controller
which will be used for current and future SoCFPGA devices.

Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
---
 .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

Comments

Krzysztof Kozlowski July 24, 2023, 7:04 a.m. UTC | #1

On 24/07/2023 08:36, [e-mail] wrote:
> From: Adrian Ng Ho Yin <[e-mail]>
> 
> Existing binding intel,keembay-dwc3.yaml does not have the required
> properties for Intel SoCFPGA devices.
> Introduce new binding description for Intel SoCFPGA USB controller
> which will be used for current and future SoCFPGA devices.
> 
> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> ---
>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> new file mode 100644
> index 000000000000..e36b087c2651
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel SoCFPGA DWC3 USB controller
> +
> +maintainers:
> +  - Adrian Ng Ho Yin <[e-mail]>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - intel,agilex5-dwc3
> +      - const: intel,socfpga-dwc3

So you did not even wait for my answer? What happened here with this
compatible? I asked you to change file name, not add intel,socfpga-dwc3.
Again - why using different style for Agilex? Which style is correct?

> +
> +  reg:
> +    description: Offset and length of DWC3 controller register

What happened here? It wasn't here before.

> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Controller Master/Core clock
> +      - description: Controller Suspend clock
> +
> +  ranges: true
> +
> +  resets:
> +    description: A list of phandles for resets listed in reset-names

Neither was this useless description, it is obvious.

> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: dwc3
> +      - const: dwc3-ecc
> +
> +  '#address-cells':
> +    enum: [ 1, 2 ]
> +
> +  '#size-cells':
> +    enum: [ 1, 2 ]
> +
> +# Required child node:
> +
> +patternProperties:
> +  "^usb@[0-9a-f]+$":
> +    $ref: snps,dwc3.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/reset/altr,rst-mgr.h>
> +
> +    usb@11000000 {
> +          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";

Still wrong indentation....

> +          reg = <0x11000000 0x100000>;
> +          ranges;
> +          clocks = <&clkmgr 54>,
> +                   <&clkmgr 55>;
> +          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
> +          reset-names = "dwc3", "dwc3-ecc";
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +
> +          usb@11000000 {
> +                compatible = "snps,dwc3";
> +                reg = <0x11000000 0x100000>;
> +                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> +                dr_mode = "host";
> +          };
> +    };
> +

Drop trailing line.

Instead of resending just after your reply, allow for reviewer to respond.

Best regards,
Krzysztof

Ng, Adrian Ho Yin July 24, 2023, 7:18 a.m. UTC | #2

> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Monday, 24 July, 2023 3:05 PM
> To: Ng, Adrian Ho Yin <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller
> 
> On 24/07/2023 08:36, [e-mail] wrote:
> > From: Adrian Ng Ho Yin <[e-mail]>
> >
> > Existing binding intel,keembay-dwc3.yaml does not have the required
> > properties for Intel SoCFPGA devices.
> > Introduce new binding description for Intel SoCFPGA USB controller
> > which will be used for current and future SoCFPGA devices.
> >
> > Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> > ---
> >  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > new file mode 100644
> > index 000000000000..e36b087c2651
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Intel SoCFPGA DWC3 USB controller
> > +
> > +maintainers:
> > +  - Adrian Ng Ho Yin <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - intel,agilex5-dwc3
> > +      - const: intel,socfpga-dwc3
> 
> So you did not even wait for my answer? What happened here with this
> compatible? I asked you to change file name, not add intel,socfpga-dwc3.
> Again - why using different style for Agilex? Which style is correct?
> 

The intention is to use a common binding for Intel SoCFPGA products that is using DWC3 controller.
This is done with reference to qcom,dwc3.yaml. 

> > +
> > +  reg:
> > +    description: Offset and length of DWC3 controller register
> 
> What happened here? It wasn't here before.

Remove unnecessary description in V3.

> 
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Controller Master/Core clock
> > +      - description: Controller Suspend clock
> > +
> > +  ranges: true
> > +
> > +  resets:
> > +    description: A list of phandles for resets listed in reset-names
> 
> Neither was this useless description, it is obvious.

Remove unnecessary description in V3.

> 
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: dwc3
> > +      - const: dwc3-ecc
> > +
> > +  '#address-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +  '#size-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +# Required child node:
> > +
> > +patternProperties:
> > +  "^usb@[0-9a-f]+$":
> > +    $ref: snps,dwc3.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/reset/altr,rst-mgr.h>
> > +
> > +    usb@11000000 {
> > +          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";
> 
> Still wrong indentation....
> 

Update indentation in V3

> > +          reg = <0x11000000 0x100000>;
> > +          ranges;
> > +          clocks = <&clkmgr 54>,
> > +                   <&clkmgr 55>;
> > +          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
> > +          reset-names = "dwc3", "dwc3-ecc";
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +
> > +          usb@11000000 {
> > +                compatible = "snps,dwc3";
> > +                reg = <0x11000000 0x100000>;
> > +                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > +                dr_mode = "host";
> > +          };
> > +    };
> > +
> 
> Drop trailing line.

Drop trailing line in V3.

> 
> Instead of resending just after your reply, allow for reviewer to respond.
> 

My apologies will wait for reply.

Thank You
Adrian

Krzysztof Kozlowski July 24, 2023, 7:27 a.m. UTC | #3

On 24/07/2023 09:18, Ng, Adrian Ho Yin wrote:
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Monday, 24 July, 2023 3:05 PM
>> To: Ng, Adrian Ho Yin <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller
>>
>> On 24/07/2023 08:36, [e-mail] wrote:
>>> From: Adrian Ng Ho Yin <[e-mail]>
>>>
>>> Existing binding intel,keembay-dwc3.yaml does not have the required
>>> properties for Intel SoCFPGA devices.
>>> Introduce new binding description for Intel SoCFPGA USB controller
>>> which will be used for current and future SoCFPGA devices.
>>>
>>> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
>>> ---
>>>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>>>  1 file changed, 84 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> new file mode 100644
>>> index 000000000000..e36b087c2651
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>> @@ -0,0 +1,84 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Intel SoCFPGA DWC3 USB controller
>>> +
>>> +maintainers:
>>> +  - Adrian Ng Ho Yin <[e-mail]>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - intel,agilex5-dwc3
>>> +      - const: intel,socfpga-dwc3
>>
>> So you did not even wait for my answer? What happened here with this
>> compatible? I asked you to change file name, not add intel,socfpga-dwc3.
>> Again - why using different style for Agilex? Which style is correct?
>>
> 
> The intention is to use a common binding for Intel SoCFPGA products that is using DWC3 controller.
> This is done with reference to qcom,dwc3.yaml. 

Nope, your driver change does not match it at all. Your explanation does
not make any sense.

Don't answer only half of my questions. So third time - the last: since
you add new style for Agilex, which style of Agilex compatibles is correct?


Best regards,
Krzysztof

Ng, Adrian Ho Yin July 24, 2023, 7:53 a.m. UTC | #4

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Monday, 24 July, 2023 3:28 PM
> To: Ng, Adrian Ho Yin <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller
> 
> On 24/07/2023 09:18, Ng, Adrian Ho Yin wrote:
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Monday, 24 July, 2023 3:05 PM
> >> To: Ng, Adrian Ho Yin <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> >> controller
> >>
> >> On 24/07/2023 08:36, [e-mail] wrote:
> >>> From: Adrian Ng Ho Yin <[e-mail]>
> >>>
> >>> Existing binding intel,keembay-dwc3.yaml does not have the required
> >>> properties for Intel SoCFPGA devices.
> >>> Introduce new binding description for Intel SoCFPGA USB controller
> >>> which will be used for current and future SoCFPGA devices.
> >>>
> >>> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> >>> ---
> >>>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
> >>>  1 file changed, 84 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>> b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>> new file mode 100644
> >>> index 000000000000..e36b087c2651
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>> @@ -0,0 +1,84 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Intel SoCFPGA DWC3 USB controller
> >>> +
> >>> +maintainers:
> >>> +  - Adrian Ng Ho Yin <[e-mail]>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - intel,agilex5-dwc3
> >>> +      - const: intel,socfpga-dwc3
> >>
> >> So you did not even wait for my answer? What happened here with this
> >> compatible? I asked you to change file name, not add intel,socfpga-dwc3.
> >> Again - why using different style for Agilex? Which style is correct?
> >>
> >
> > The intention is to use a common binding for Intel SoCFPGA products that is
> using DWC3 controller.
> > This is done with reference to qcom,dwc3.yaml.
> 
> Nope, your driver change does not match it at all. Your explanation does not
> make any sense.
> 
> Don't answer only half of my questions. So third time - the last: since you add
> new style for Agilex, which style of Agilex compatibles is correct?
> 

My apologies.
In your opinion which is the proper practice?
1. Create new binding for new products that is using the same controller.
2. Create a common binding that will be used by products using the same controller?
Referring to the current bindings that are available the two options are being practiced at the moment.

If option 1 is the proper practice the correct Agilex compatible is intel,agilex5-dwc3.
To rework the binding to cater for agilex5-dwc3 only. The compatible in glue driver will remain the same. 

If option 2 is the proper practice then the correct Agilex compatible is intel,socfpga-dwc3.
To update compatible in glue driver in V3. 

Thank You
Adrian Ng

Krzysztof Kozlowski July 24, 2023, 8:55 a.m. UTC | #5

On 24/07/2023 09:53, Ng, Adrian Ho Yin wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Monday, 24 July, 2023 3:28 PM
>> To: Ng, Adrian Ho Yin <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB controller
>>
>> On 24/07/2023 09:18, Ng, Adrian Ho Yin wrote:
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: Monday, 24 July, 2023 3:05 PM
>>>> To: Ng, Adrian Ho Yin <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
>>>> controller
>>>>
>>>> On 24/07/2023 08:36, [e-mail] wrote:
>>>>> From: Adrian Ng Ho Yin <[e-mail]>
>>>>>
>>>>> Existing binding intel,keembay-dwc3.yaml does not have the required
>>>>> properties for Intel SoCFPGA devices.
>>>>> Introduce new binding description for Intel SoCFPGA USB controller
>>>>> which will be used for current and future SoCFPGA devices.
>>>>>
>>>>> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
>>>>> ---
>>>>>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>>>>>  1 file changed, 84 insertions(+)
>>>>>  create mode 100644
>>>>> Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>>>> b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..e36b087c2651
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
>>>>> @@ -0,0 +1,84 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Intel SoCFPGA DWC3 USB controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Adrian Ng Ho Yin <[e-mail]>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - intel,agilex5-dwc3
>>>>> +      - const: intel,socfpga-dwc3
>>>>
>>>> So you did not even wait for my answer? What happened here with this
>>>> compatible? I asked you to change file name, not add intel,socfpga-dwc3.
>>>> Again - why using different style for Agilex? Which style is correct?
>>>>
>>>
>>> The intention is to use a common binding for Intel SoCFPGA products that is
>> using DWC3 controller.
>>> This is done with reference to qcom,dwc3.yaml.
>>
>> Nope, your driver change does not match it at all. Your explanation does not
>> make any sense.
>>
>> Don't answer only half of my questions. So third time - the last: since you add
>> new style for Agilex, which style of Agilex compatibles is correct?

I still did not receive here answer. Which style, naming convention for
agilex is correct for your platform?

Why this:
git grep agilex | grep intel,

gives different compatibles than you start here? I assume Intel/Altera
knows better the platform so will provide here some guidance. If unsure,
please consult your colleagues.


>>
> 
> My apologies.
> In your opinion which is the proper practice?
> 1. Create new binding for new products that is using the same controller.

What is "new binding"? What do you mean by that? New file, then not.

> 2. Create a common binding that will be used by products using the same controller?
> Referring to the current bindings that are available the two options are being practiced at the moment.
> 
> If option 1 is the proper practice the correct Agilex compatible is intel,agilex5-dwc3.
> To rework the binding to cater for agilex5-dwc3 only. The compatible in glue driver will remain the same. 
> 
> If option 2 is the proper practice then the correct Agilex compatible is intel,socfpga-dwc3.
> To update compatible in glue driver in V3. 
> 


Recommended practice is to use specific compatible for both: your device
and as fallback for any future devices. In certain cases, option 2 is okay.


Best regards,
Krzysztof

[e-mail] July 25, 2023, 1:25 p.m. UTC | #6

On Mon, Jul 24, 2023 at 02:36:58PM +0800, [e-mail] wrote:
> From: Adrian Ng Ho Yin <[e-mail]>
> 
> Existing binding intel,keembay-dwc3.yaml does not have the required
> properties for Intel SoCFPGA devices.
> Introduce new binding description for Intel SoCFPGA USB controller
> which will be used for current and future SoCFPGA devices.
> 
> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> ---
>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml

You don't seem to have followed the rules that your employer has for
kernel contributions, so we can't take this, sorry.  Please work with
the Intel Linux kernel group to resolve those issues first, and then
resubmit the patches properly.

thanks,

greg k-h

Rob Herring (Arm) July 26, 2023, 4:29 p.m. UTC | #7

On Mon, Jul 24, 2023 at 02:36:58PM +0800, [e-mail] wrote:
> From: Adrian Ng Ho Yin <[e-mail]>
> 
> Existing binding intel,keembay-dwc3.yaml does not have the required
> properties for Intel SoCFPGA devices.
> Introduce new binding description for Intel SoCFPGA USB controller
> which will be used for current and future SoCFPGA devices.
> 
> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> ---
>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> new file mode 100644
> index 000000000000..e36b087c2651
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel SoCFPGA DWC3 USB controller
> +
> +maintainers:
> +  - Adrian Ng Ho Yin <[e-mail]>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - intel,agilex5-dwc3
> +      - const: intel,socfpga-dwc3
> +
> +  reg:
> +    description: Offset and length of DWC3 controller register
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Controller Master/Core clock
> +      - description: Controller Suspend clock
> +
> +  ranges: true
> +
> +  resets:
> +    description: A list of phandles for resets listed in reset-names
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: dwc3
> +      - const: dwc3-ecc
> +
> +  '#address-cells':
> +    enum: [ 1, 2 ]
> +
> +  '#size-cells':
> +    enum: [ 1, 2 ]
> +
> +# Required child node:
> +
> +patternProperties:
> +  "^usb@[0-9a-f]+$":
> +    $ref: snps,dwc3.yaml#

One node, no wrapper node and dwc3 child node please unless you have 
actual registers for the wrapper. Based on the example having the same 
register addresses in both nodes, you don't need the wrapper.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/reset/altr,rst-mgr.h>
> +
> +    usb@11000000 {
> +          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";
> +          reg = <0x11000000 0x100000>;

You really have 1MB worth of registers? That chews up 1MB of 
kernel virtual space. Not a big deal for 64-bit, but it is a problem on 
32-bit systems. Define the length to just what you need.

> +          ranges;
> +          clocks = <&clkmgr 54>,
> +                   <&clkmgr 55>;
> +          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
> +          reset-names = "dwc3", "dwc3-ecc";
> +          #address-cells = <1>;
> +          #size-cells = <1>;

BTW, this implies that you can only DMA 32-bit bits. Not sure if the 
dwc3 supports more. 

> +
> +          usb@11000000 {
> +                compatible = "snps,dwc3";
> +                reg = <0x11000000 0x100000>;
> +                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> +                dr_mode = "host";
> +          };
> +    };
> +
> -- 
> 2.26.2
>

Ng, Adrian Ho Yin July 27, 2023, 2:09 a.m. UTC | #8

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Monday, 24 July, 2023 4:55 PM
> To: Ng, Adrian Ho Yin <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> controller
> 
> On 24/07/2023 09:53, Ng, Adrian Ho Yin wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Monday, 24 July, 2023 3:28 PM
> >> To: Ng, Adrian Ho Yin <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> >> controller
> >>
> >> On 24/07/2023 09:18, Ng, Adrian Ho Yin wrote:
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: Monday, 24 July, 2023 3:05 PM
> >>>> To: Ng, Adrian Ho Yin <[e-mail]>;
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; linux-
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]
> >>>> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> >>>> controller
> >>>>
> >>>> On 24/07/2023 08:36, [e-mail] wrote:
> >>>>> From: Adrian Ng Ho Yin <[e-mail]>
> >>>>>
> >>>>> Existing binding intel,keembay-dwc3.yaml does not have the
> >>>>> required properties for Intel SoCFPGA devices.
> >>>>> Introduce new binding description for Intel SoCFPGA USB controller
> >>>>> which will be used for current and future SoCFPGA devices.
> >>>>>
> >>>>> Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> >>>>> ---
> >>>>>  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84
> +++++++++++++++++++
> >>>>>  1 file changed, 84 insertions(+)
> >>>>>  create mode 100644
> >>>>> Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>>>>
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>>>> b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..e36b087c2651
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-
> dwc3.yam
> >>>>> +++ l
> >>>>> @@ -0,0 +1,84 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> >>>>> +1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Intel SoCFPGA DWC3 USB controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Adrian Ng Ho Yin <[e-mail]>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - enum:
> >>>>> +          - intel,agilex5-dwc3
> >>>>> +      - const: intel,socfpga-dwc3
> >>>>
> >>>> So you did not even wait for my answer? What happened here with
> >>>> this compatible? I asked you to change file name, not add intel,socfpga-
> dwc3.
> >>>> Again - why using different style for Agilex? Which style is correct?
> >>>>
> >>>
> >>> The intention is to use a common binding for Intel SoCFPGA products
> >>> that is
> >> using DWC3 controller.
> >>> This is done with reference to qcom,dwc3.yaml.
> >>
> >> Nope, your driver change does not match it at all. Your explanation
> >> does not make any sense.
> >>
> >> Don't answer only half of my questions. So third time - the last:
> >> since you add new style for Agilex, which style of Agilex compatibles is
> correct?
> 
> I still did not receive here answer. Which style, naming convention for agilex
> is correct for your platform?
> 
> Why this:
> git grep agilex | grep intel,
> 
> gives different compatibles than you start here? I assume Intel/Altera knows
> better the platform so will provide here some guidance. If unsure, please
> consult your colleagues.
> 
> 

Noted. Will consult Intel Linux group to get the correct naming convention and 
go through another round of internal review prior to address the issues/concerns 
raised before submitting the revised patches.

> >>
> >
> > My apologies.
> > In your opinion which is the proper practice?
> > 1. Create new binding for new products that is using the same controller.
> 
> What is "new binding"? What do you mean by that? New file, then not.
> 

Yes. New Binding means new file. Will not proceed with option 1.

> > 2. Create a common binding that will be used by products using the same
> controller?
> > Referring to the current bindings that are available the two options are
> being practiced at the moment.
> >
> > If option 1 is the proper practice the correct Agilex compatible is
> intel,agilex5-dwc3.
> > To rework the binding to cater for agilex5-dwc3 only. The compatible in
> glue driver will remain the same.
> >
> > If option 2 is the proper practice then the correct Agilex compatible is
> intel,socfpga-dwc3.
> > To update compatible in glue driver in V3.
> >
> 
> 
> Recommended practice is to use specific compatible for both: your device
> and as fallback for any future devices. In certain cases, option 2 is okay.
> 
> 

Noted. Thank you for the feedback. 

Thank You
Adrian  Ng

Ng, Adrian Ho Yin July 27, 2023, 2:11 a.m. UTC | #9

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Tuesday, 25 July, 2023 9:26 PM
> To: Ng, Adrian Ho Yin <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> controller
> 
> On Mon, Jul 24, 2023 at 02:36:58PM +0800, [e-mail] wrote:
> > From: Adrian Ng Ho Yin <[e-mail]>
> >
> > Existing binding intel,keembay-dwc3.yaml does not have the required
> > properties for Intel SoCFPGA devices.
> > Introduce new binding description for Intel SoCFPGA USB controller
> > which will be used for current and future SoCFPGA devices.
> >
> > Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> > ---
> >  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> 
> You don't seem to have followed the rules that your employer has for kernel
> contributions, so we can't take this, sorry.  Please work with the Intel Linux
> kernel group to resolve those issues first, and then resubmit the patches
> properly.
> 

Noted. Will work with the intel Linux group to resolve the issues/concerns prior to resubmitting the patches.
Thank You for the feedback and sorry for the inconveniences. 

Thank You.
Adrian Ng

Ng, Adrian Ho Yin July 27, 2023, 8:11 a.m. UTC | #10

> -----Original Message-----
> From: Rob Herring <[e-mail]>
> Sent: Thursday, 27 July, 2023 12:29 AM
> To: Ng, Adrian Ho Yin <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add Intel SoCFPGA USB
> controller
> 
> On Mon, Jul 24, 2023 at 02:36:58PM +0800, [e-mail] wrote:
> > From: Adrian Ng Ho Yin <[e-mail]>
> >
> > Existing binding intel,keembay-dwc3.yaml does not have the required
> > properties for Intel SoCFPGA devices.
> > Introduce new binding description for Intel SoCFPGA USB controller
> > which will be used for current and future SoCFPGA devices.
> >
> > Signed-off-by: Adrian Ng Ho Yin <[e-mail]>
> > ---
> >  .../bindings/usb/intel,socfpga-dwc3.yaml      | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > new file mode 100644
> > index 000000000000..e36b087c2651
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Intel SoCFPGA DWC3 USB controller
> > +
> > +maintainers:
> > +  - Adrian Ng Ho Yin <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - intel,agilex5-dwc3
> > +      - const: intel,socfpga-dwc3
> > +
> > +  reg:
> > +    description: Offset and length of DWC3 controller register
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Controller Master/Core clock
> > +      - description: Controller Suspend clock
> > +
> > +  ranges: true
> > +
> > +  resets:
> > +    description: A list of phandles for resets listed in reset-names
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: dwc3
> > +      - const: dwc3-ecc
> > +
> > +  '#address-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +  '#size-cells':
> > +    enum: [ 1, 2 ]
> > +
> > +# Required child node:
> > +
> > +patternProperties:
> > +  "^usb@[0-9a-f]+$":
> > +    $ref: snps,dwc3.yaml#
> 
> One node, no wrapper node and dwc3 child node please unless you have
> actual registers for the wrapper. Based on the example having the same
> register addresses in both nodes, you don't need the wrapper.
> 

Noted.  To remove child node in next patch submission.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/reset/altr,rst-mgr.h>
> > +
> > +    usb@11000000 {
> > +          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";
> > +          reg = <0x11000000 0x100000>;
> 
> You really have 1MB worth of registers? That chews up 1MB of kernel virtual
> space. Not a big deal for 64-bit, but it is a problem on 32-bit systems. Define
> the length to just what you need.
> 

To update reg length in next patch submission.

> > +          ranges;
> > +          clocks = <&clkmgr 54>,
> > +                   <&clkmgr 55>;
> > +          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
> > +          reset-names = "dwc3", "dwc3-ecc";
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> 
> BTW, this implies that you can only DMA 32-bit bits. Not sure if the
> dwc3 supports more.
> 

DWC3 supports DMA 64 bits.
In properties address-cells support value 1 and 2.
To update example to indicate 64-bit DMA support.

Thank You.
Adrian Ng

Patch

diff --git a/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
new file mode 100644
index 000000000000..e36b087c2651
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/intel,socfpga-dwc3.yaml
@@ -0,0 +1,84 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/intel,socfpga-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel SoCFPGA DWC3 USB controller
+
+maintainers:
+  - Adrian Ng Ho Yin <[e-mail]>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - intel,agilex5-dwc3
+      - const: intel,socfpga-dwc3
+
+  reg:
+    description: Offset and length of DWC3 controller register
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Controller Master/Core clock
+      - description: Controller Suspend clock
+
+  ranges: true
+
+  resets:
+    description: A list of phandles for resets listed in reset-names
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: dwc3
+      - const: dwc3-ecc
+
+  '#address-cells':
+    enum: [ 1, 2 ]
+
+  '#size-cells':
+    enum: [ 1, 2 ]
+
+# Required child node:
+
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: snps,dwc3.yaml#
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/reset/altr,rst-mgr.h>
+
+    usb@11000000 {
+          compatible = "intel,agilex5-dwc3", "intel,socfpga-dwc3";
+          reg = <0x11000000 0x100000>;
+          ranges;
+          clocks = <&clkmgr 54>,
+                   <&clkmgr 55>;
+          resets = <&rst USB0_RESET>, <&rst USB1_RESET>;
+          reset-names = "dwc3", "dwc3-ecc";
+          #address-cells = <1>;
+          #size-cells = <1>;
+
+          usb@11000000 {
+                compatible = "snps,dwc3";
+                reg = <0x11000000 0x100000>;
+                interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+                dr_mode = "host";
+          };
+    };
+