| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 May 2022 16:40:24 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Frank Wunderlich <frank-w@...lic-files.de>,
Greg Ungerer <gerg@...nel.org>,
René van Dorst <opensource@...rst.com>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>
Cc: Frank Wunderlich <linux@...web.de>, Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Sean Wang <sean.wang@...iatek.com>,
Landen Chao <Landen.Chao@...iatek.com>,
DENG Qingfang <dqfext@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for
mediatek switches
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@...aro.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@...lic-files.de>
>>>
>>> Convert txt binding to yaml binding for Mediatek switches.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@...lic-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@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
Powered by blists - more mailing lists