Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

[Krzysztof Kozlowski]

On 03/05/2022 16:10, Frank Wunderlich wrote:
> Hi,
> 
> thank you for first review.
> 
>> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski at linaro.org>
>> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>>
>> On 02/05/2022 17:32, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w at public-files.de>
>>>
>>> Convert txt binding to yaml binding for Mediatek switches.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w at public-files.de>
>>> ---
>>>  .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
>>>  .../devicetree/bindings/net/dsa/mt7530.txt    | 327 -------------
>>>  2 files changed, 435 insertions(+), 327 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>> new file mode 100644
>>> index 000000000000..c1724809d34e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>
>> Specific name please, so previous (with vendor prefix) was better:
>> mediatek,mt7530.yaml
> 
> ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".
> 
>>> @@ -0,0 +1,435 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>
>> You should CC previous contributors and get their acks on this. You
>> copied here a lot of description.
> 
> added 3 Persons that made commits to txt before to let them know about this change
> 
> and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
> compatible in subnode.

I don't remember such syntax.

(...)

> 
>>> if defined, indicates that either MT7530 is the part
>>> +      on multi-chip module belong to MT7623A has or the remotely standalone
>>> +      chip as the function MT7623N reference board provided for.
>>> +
>>> +  reset-gpios:
>>> +    description: |
>>> +      Should be a gpio specifier for a reset line.
>>> +    maxItems: 1
>>> +
>>> +  reset-names:
>>> +    description: |
>>> +      Should be set to "mcm".
>>> +    const: mcm
>>> +
>>> +  resets:
>>> +    description: |
>>> +      Phandle pointing to the system reset controller with
>>> +      line index for the ethsys.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>
>> What about address/size cells?
> 
> you're right even if they are const to a value they need to be set
> 
>>> +
>>> +allOf:
>>> +  - $ref: "dsa.yaml#"
>>> +  - if:
>>> +      required:
>>> +        - mediatek,mcm
>>
>> Original bindings had this reversed.
> 
> i know, but i think it is better readable and i will drop the else-part later.
> Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
> as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.
> 
> i left this as separate commit to be posted later to have a nearly 1:1 conversion here.

Ah, I missed that actually your syntax is better. No need to
reverse/negate and the changes do not have to be strict 1:1.

> 
>>> +    then:
>>> +      required:
>>> +        - resets
>>> +        - reset-names
>>> +    else:
>>> +      required:
>>> +        - reset-gpios
>>> +
>>> +  - if:
>>> +      required:
>>> +        - interrupt-controller
>>> +    then:
>>> +      required:
>>> +        - "#interrupt-cells"
>>
>> This should come from dt schema already...
> 
> so i should drop (complete block for interrupt controller)?

The interrupts you need. What I mean, you can skip requirement of cells.

> 
>>> +        - interrupts
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          items:
>>> +            - const: mediatek,mt7530
>>> +    then:
>>> +      required:
>>> +        - core-supply
>>> +        - io-supply
>>> +
>>> +
>>> +patternProperties:
>>> +  "^ports$":
>>
>> It''s not a pattern, so put it under properties, like regular property.
> 
> can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"

Yes, subnodes stay with patternProperties.

> 
>   ports:
>     type: object
> 
>     patternProperties:
>       "^port@[0-9]+$":
>         type: object
>         description: Ethernet switch ports
> 
>         properties:
>           reg:
>             description: |
>               Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.
> 
>         unevaluatedProperties: false
> 
>         allOf:
>           - $ref: dsa-port.yaml#
>           - if:
> ....
> 
> basicly this "ports"-property should be required too, right?

Previous binding did not enforce it, I think, but it is reasonable to
require ports.

> 
> 
>>> +    type: object
>>> +
>>> +    patternProperties:
>>> +      "^port@[0-9]+$":
>>> +        type: object
>>> +        description: Ethernet switch ports
>>> +
>>> +        $ref: dsa-port.yaml#
>>
>> This should go to allOf below.
> 
> see above
> 
>>> +
>>> +        properties:
>>> +          reg:
>>> +            description: |
>>> +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
>>> +
>>> +        unevaluatedProperties: false
>>> +
>>> +        allOf:
>>> +          - if:
>>> +              properties:
>>> +                label:
>>> +                  items:
>>> +                    - const: cpu
>>> +            then:
>>> +              required:
>>> +                - reg
>>> +                - phy-mode
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    mdio0 {
>>
>> Just mdio
> 
> ok
> 
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        switch at 0 {
>>> +            compatible = "mediatek,mt7530";
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +            reg = <0>;
>>> +
>>> +            core-supply = <&mt6323_vpa_reg>;
>>> +            io-supply = <&mt6323_vemc3v3_reg>;
>>> +            reset-gpios = <&pio 33 0>;
>>
>> Use GPIO flag define/constant.
> 
> this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
> constants too.
> 
> i guess
> include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0
> 
> for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
> header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)

ok, then my comment


Best regards,
Krzysztof