[RFC,1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

  From: Yang Xiwen <forbidden405@outlook.com>

Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
compatible lists.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
 .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
 2 files changed, 115 insertions(+), 71 deletions(-)
  

On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
> compatible lists.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>  .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>  2 files changed, 115 insertions(+), 71 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> new file mode 100644
> index 000000000000..73256eee10f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
> +
> +maintainers:
> +  - Yang Xiwen <forbidden405@outlook.com>
> +
> +properties:
> +  compatible:
> +    minItems: 1

No, why? Compatibles must be fixed/constrained.

> +    items:
> +      - enum:
> +          - hisilicon,hi3798cv200-usb2-phy
> +          - hisilicon,hi3798mv100-usb2-phy

This wasn't here before. Not explained in commit msg.

> +      - const: hisilicon,inno-usb2-phy
> +
> +  reg:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Should be the address space for PHY configuration register in peripheral
> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
> +      Or direct MMIO address space.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  clocks:
> +    maxItems: 1
> +    description: reference clock
> +
> +  resets:
> +    maxItems: 1
> +
> +patternProperties:
> +  'phy@[0-9a-f]+':
> +    type: object
> +    additionalProperties: false
> +    description: individual ports provided by INNO PHY
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      '#phy-cells':
> +        const: 0
> +
> +      resets:
> +        maxItems: 1
> +
> +    required: [reg, '#phy-cells', resets]

One item per line. Look at other bindings or example-schema.

> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/histb-clock.h>
> +
> +    peripheral-controller@8a20000 {
> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
> +        reg = <0x8a20000 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0x0 0x8a20000 0x1000>;

Drop the node, not related to this binding. If this binding is supposed
to be part of other device in case of MFD devices or some tightly
coupled ones, then could be included in the example there.

> +
> +        usb2-phy@120 {
> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> +            reg = <0x120 0x4>;
> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
> +            resets = <&crg 0xbc 4>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            phy@0 {
> +                reg = <0>;
> +                #phy-cells = <0>;
> +                resets = <&crg 0xbc 8>;
> +            };
> +
> +            phy@1 {
> +                reg = <1>;
> +                #phy-cells = <0>;
> +                resets = <&crg 0xbc 9>;
> +            };
> +        };
> +
> +        usb2-phy@124 {
> +            compatible = "hisilicon,hi3798cv200-usb2-phy";

You can keep only one example, because they are basically the same.


Best regards,
Krzysztof
  

On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>> compatible lists.
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
Will fix in next version
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> new file mode 100644
>> index 000000000000..73256eee10f9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> @@ -0,0 +1,115 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    minItems: 1
> No, why? Compatibles must be fixed/constrained.
>
>> +    items:
>> +      - enum:
>> +          - hisilicon,hi3798cv200-usb2-phy
>> +          - hisilicon,hi3798mv100-usb2-phy
> This wasn't here before. Not explained in commit msg.
The commit 3940ffc65492 ("phy: hisilicon: Add inno-usb2-phy driver for 
Hi3798MV100") does not have dt-binding change commit along with. Will 
explain this in commit log.
>
>> +      - const: hisilicon,inno-usb2-phy
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: |
> Do not need '|' unless you need to preserve formatting.
>
>> +      Should be the address space for PHY configuration register in peripheral
>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>> +      Or direct MMIO address space.
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: reference clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  'phy@[0-9a-f]+':
>> +    type: object
>> +    additionalProperties: false
>> +    description: individual ports provided by INNO PHY
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +
>> +      '#phy-cells':
>> +        const: 0
>> +
>> +      resets:
>> +        maxItems: 1
>> +
>> +    required: [reg, '#phy-cells', resets]
> One item per line. Look at other bindings or example-schema.
>
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - resets
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/histb-clock.h>
>> +
>> +    peripheral-controller@8a20000 {
>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>> +        reg = <0x8a20000 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges = <0x0 0x8a20000 0x1000>;
> Drop the node, not related to this binding. If this binding is supposed
> to be part of other device in case of MFD devices or some tightly
> coupled ones, then could be included in the example there.
For CV200, this binding is supposed to be always inside the perictrl 
device. The PHY address space are accessed from a bus implemented by 
perictrl.
>
>> +
>> +        usb2-phy@120 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> +            reg = <0x120 0x4>;
>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>> +            resets = <&crg 0xbc 4>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            phy@0 {
>> +                reg = <0>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 8>;
>> +            };
>> +
>> +            phy@1 {
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 9>;
>> +            };
>> +        };
>> +
>> +        usb2-phy@124 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> You can keep only one example, because they are basically the same.
Will remove
>
>
> Best regards,
> Krzysztof
>
  

On 17/02/2024 11:24, Yang Xiwen wrote:

>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/histb-clock.h>
>>> +
>>> +    peripheral-controller@8a20000 {
>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>> +        reg = <0x8a20000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges = <0x0 0x8a20000 0x1000>;
>> Drop the node, not related to this binding. If this binding is supposed
>> to be part of other device in case of MFD devices or some tightly
>> coupled ones, then could be included in the example there.
> For CV200, this binding is supposed to be always inside the perictrl 
> device. The PHY address space are accessed from a bus implemented by 
> perictrl.

Every node is supposes to be somewhere in something, so with this logic
you would start from soc@. What's wrong in putting it in parent schema?

Best regards,
Krzysztof
  

On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 11:24, Yang Xiwen wrote:
>
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>> +
>>>> +    peripheral-controller@8a20000 {
>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>> +        reg = <0x8a20000 0x1000>;
>>>> +        #address-cells = <1>;
>>>> +        #size-cells = <1>;
>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>> Drop the node, not related to this binding. If this binding is supposed
>>> to be part of other device in case of MFD devices or some tightly
>>> coupled ones, then could be included in the example there.
>> For CV200, this binding is supposed to be always inside the perictrl
>> device. The PHY address space are accessed from a bus implemented by
>> perictrl.
> Every node is supposes to be somewhere in something, so with this logic
> you would start from soc@. What's wrong in putting it in parent schema?

When the ports reg property only has an dummy address (no size), it must 
be inside the perictrl node, accessed from the bus implemented by perictrl.

But when the ports has its own MMIO address space (mv200), it should be 
located under a simple-bus node instead.

So it's either:

perictrl@8a20000 {

     usb2-phy@120: {

         reg = <0x120 0x4>; // this is the register that controls writes 
and reads to the phy, implemented by perictrl. (just like SPI/I2C)

         phy@0: {

             reg = <0>; // the reg is used as an index

         };

     };

};

or:

soc@0 {

     usb2-phy@f9865000 { // MMIO

         reg = <0xf9865000 0x1000>

         port0@0 {

             reg = <0x0 0x400>; // used as MMIO address space

         };

     };

};

So here is why i include perictrl node in the example. If the ports are 
not mmio, the phy must be under a perictrl node. Or if the ports has its 
own address space, it should not be under a perictrl node, but rather an 
simple-bus node.

>
> Best regards,
> Krzysztof
>
  

On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>> compatible lists.
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> new file mode 100644
>> index 000000000000..73256eee10f9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> @@ -0,0 +1,115 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    minItems: 1
> No, why? Compatibles must be fixed/constrained.
Hi3798CV200 has only the first compatible listed in its device tree. But 
you are right i can add it to hi3798mv200.dtsi so that `minItems` can be 
removed
>
>> +    items:
>> +      - enum:
>> +          - hisilicon,hi3798cv200-usb2-phy
>> +          - hisilicon,hi3798mv100-usb2-phy
> This wasn't here before. Not explained in commit msg.
>
>> +      - const: hisilicon,inno-usb2-phy
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: |
> Do not need '|' unless you need to preserve formatting.
>
>> +      Should be the address space for PHY configuration register in peripheral
>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>> +      Or direct MMIO address space.
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: reference clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  'phy@[0-9a-f]+':
>> +    type: object
>> +    additionalProperties: false
>> +    description: individual ports provided by INNO PHY
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +
>> +      '#phy-cells':
>> +        const: 0
>> +
>> +      resets:
>> +        maxItems: 1
>> +
>> +    required: [reg, '#phy-cells', resets]
> One item per line. Look at other bindings or example-schema.
>
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - resets
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/histb-clock.h>
>> +
>> +    peripheral-controller@8a20000 {
>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>> +        reg = <0x8a20000 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges = <0x0 0x8a20000 0x1000>;
> Drop the node, not related to this binding. If this binding is supposed
> to be part of other device in case of MFD devices or some tightly
> coupled ones, then could be included in the example there.
>
>> +
>> +        usb2-phy@120 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> +            reg = <0x120 0x4>;
>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>> +            resets = <&crg 0xbc 4>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            phy@0 {
>> +                reg = <0>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 8>;
>> +            };
>> +
>> +            phy@1 {
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 9>;
>> +            };
>> +        };
>> +
>> +        usb2-phy@124 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> You can keep only one example, because they are basically the same.

It is listed here just because cv200 (and the upcoming mv200) actually 
has two INNO phys in the SoC. And coincidently for both SoCs, one with 
two ports is wired to USB2 controller(EHCI &OHCI), while the other one 
with only one port is wired to DWC3 controller. The example here is 
borrowed directly from hi3798cv200.dtsi.

>
> Best regards,
> Krzysztof
>
  

On 17/02/2024 11:54, Yang Xiwen wrote:
> On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 11:24, Yang Xiwen wrote:
>>
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>>> +
>>>>> +    peripheral-controller@8a20000 {
>>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>>> +        reg = <0x8a20000 0x1000>;
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <1>;
>>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>>> Drop the node, not related to this binding. If this binding is supposed
>>>> to be part of other device in case of MFD devices or some tightly
>>>> coupled ones, then could be included in the example there.
>>> For CV200, this binding is supposed to be always inside the perictrl
>>> device. The PHY address space are accessed from a bus implemented by
>>> perictrl.
>> Every node is supposes to be somewhere in something, so with this logic
>> you would start from soc@. What's wrong in putting it in parent schema?
> 
> When the ports reg property only has an dummy address (no size), it must 
> be inside the perictrl node, accessed from the bus implemented by perictrl.
> 
> But when the ports has its own MMIO address space (mv200), it should be 
> located under a simple-bus node instead.
> 
> So it's either:
> 
> perictrl@8a20000 {
> 
>      usb2-phy@120: {
> 
>          reg = <0x120 0x4>; // this is the register that controls writes 
> and reads to the phy, implemented by perictrl. (just like SPI/I2C)
> 
>          phy@0: {
> 
>              reg = <0>; // the reg is used as an index
> 
>          };
> 
>      };
> 
> };
> 
> or:
> 
> soc@0 {
> 
>      usb2-phy@f9865000 { // MMIO
> 
>          reg = <0xf9865000 0x1000>
> 
>          port0@0 {
> 
>              reg = <0x0 0x400>; // used as MMIO address space
> 
>          };
> 
>      };
> 
> };
> 
> So here is why i include perictrl node in the example. If the ports are 
> not mmio, the phy must be under a perictrl node. Or if the ports has its 
> own address space, it should not be under a perictrl node, but rather an 
> simple-bus node.

I don't understand why you keep insisting and discussing this. You are
adding other compatibles to this schema example, which usually we try to
avoid. You entirely ignored my comment above and pasted DTS which is no
related to the topic we discuss here. I did not question whether this
can be or cannot be in some node.

Best regards,
Krzysztof
  

On 17/02/2024 14:14, Yang Xiwen wrote:
> On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>>> compatible lists.
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>>
>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>> ---
>>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>> new file mode 100644
>>> index 000000000000..73256eee10f9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>> @@ -0,0 +1,115 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>>> +
>>> +maintainers:
>>> +  - Yang Xiwen <forbidden405@outlook.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    minItems: 1
>> No, why? Compatibles must be fixed/constrained.
> Hi3798CV200 has only the first compatible listed in its device tree. But 
> you are right i can add it to hi3798mv200.dtsi so that `minItems` can be 
> removed
>>
>>> +    items:
>>> +      - enum:
>>> +          - hisilicon,hi3798cv200-usb2-phy
>>> +          - hisilicon,hi3798mv100-usb2-phy
>> This wasn't here before. Not explained in commit msg.
>>
>>> +      - const: hisilicon,inno-usb2-phy
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +    description: |
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +      Should be the address space for PHY configuration register in peripheral
>>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>>> +      Or direct MMIO address space.
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 0
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description: reference clock
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +patternProperties:
>>> +  'phy@[0-9a-f]+':
>>> +    type: object
>>> +    additionalProperties: false
>>> +    description: individual ports provided by INNO PHY
>>> +
>>> +    properties:
>>> +      reg:
>>> +        maxItems: 1
>>> +
>>> +      '#phy-cells':
>>> +        const: 0
>>> +
>>> +      resets:
>>> +        maxItems: 1
>>> +
>>> +    required: [reg, '#phy-cells', resets]
>> One item per line. Look at other bindings or example-schema.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - reg
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +  - resets
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/histb-clock.h>
>>> +
>>> +    peripheral-controller@8a20000 {
>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>> +        reg = <0x8a20000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges = <0x0 0x8a20000 0x1000>;
>> Drop the node, not related to this binding. If this binding is supposed
>> to be part of other device in case of MFD devices or some tightly
>> coupled ones, then could be included in the example there.
>>
>>> +
>>> +        usb2-phy@120 {
>>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>>> +            reg = <0x120 0x4>;
>>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>>> +            resets = <&crg 0xbc 4>;
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            phy@0 {
>>> +                reg = <0>;
>>> +                #phy-cells = <0>;
>>> +                resets = <&crg 0xbc 8>;
>>> +            };
>>> +
>>> +            phy@1 {
>>> +                reg = <1>;
>>> +                #phy-cells = <0>;
>>> +                resets = <&crg 0xbc 9>;
>>> +            };
>>> +        };
>>> +
>>> +        usb2-phy@124 {
>>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> You can keep only one example, because they are basically the same.
> 
> It is listed here just because cv200 (and the upcoming mv200) actually 
> has two INNO phys in the SoC. And coincidently for both SoCs, one with 
> two ports is wired to USB2 controller(EHCI &OHCI), while the other one 
> with only one port is wired to DWC3 controller. The example here is 
> borrowed directly from hi3798cv200.dtsi.
> 

I see the differences and one difference means they are basically the same.

Best regards,
Krzysztof
  

On 2/17/2024 9:39 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 11:54, Yang Xiwen wrote:
>> On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
>>> On 17/02/2024 11:24, Yang Xiwen wrote:
>>>
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>>>> +
>>>>>> +    peripheral-controller@8a20000 {
>>>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>>>> +        reg = <0x8a20000 0x1000>;
>>>>>> +        #address-cells = <1>;
>>>>>> +        #size-cells = <1>;
>>>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>>>> Drop the node, not related to this binding. If this binding is supposed
>>>>> to be part of other device in case of MFD devices or some tightly
>>>>> coupled ones, then could be included in the example there.
>>>> For CV200, this binding is supposed to be always inside the perictrl
>>>> device. The PHY address space are accessed from a bus implemented by
>>>> perictrl.
>>> Every node is supposes to be somewhere in something, so with this logic
>>> you would start from soc@. What's wrong in putting it in parent schema?
>> When the ports reg property only has an dummy address (no size), it must
>> be inside the perictrl node, accessed from the bus implemented by perictrl.
>>
>> But when the ports has its own MMIO address space (mv200), it should be
>> located under a simple-bus node instead.
>>
>> So it's either:
>>
>> perictrl@8a20000 {
>>
>>       usb2-phy@120: {
>>
>>           reg = <0x120 0x4>; // this is the register that controls writes
>> and reads to the phy, implemented by perictrl. (just like SPI/I2C)
>>
>>           phy@0: {
>>
>>               reg = <0>; // the reg is used as an index
>>
>>           };
>>
>>       };
>>
>> };
>>
>> or:
>>
>> soc@0 {
>>
>>       usb2-phy@f9865000 { // MMIO
>>
>>           reg = <0xf9865000 0x1000>
>>
>>           port0@0 {
>>
>>               reg = <0x0 0x400>; // used as MMIO address space
>>
>>           };
>>
>>       };
>>
>> };
>>
>> So here is why i include perictrl node in the example. If the ports are
>> not mmio, the phy must be under a perictrl node. Or if the ports has its
>> own address space, it should not be under a perictrl node, but rather an
>> simple-bus node.
> I don't understand why you keep insisting and discussing this. You are
> adding other compatibles to this schema example, which usually we try to
> avoid. You entirely ignored my comment above and pasted DTS which is no
> related to the topic we discuss here. I did not question whether this
> can be or cannot be in some node.
okay, I can remove the parent node if it's preferred. This is simply the 
most usual example that exists in real dts.
>
> Best regards,
> Krzysztof
>