pon., 3 paź 2022 o 19:29 Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> napisał(a): > > On 03/10/2022 19:06, Michał Grzelak wrote: > > On 02/10/2022 10:23, Marcin Wojtas wrote: > >> niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski > >> <krzysztof.kozlowski@xxxxxxxxxx> napisał(a): > >>> > >>> On 01/10/2022 17:53, Michał Grzelak wrote: > >>>> Hi Krzysztof, > >>>> > >>>> Thanks for your comments and time spent on reviewing my patch. > >>>> All of those improvements will be included in next version. > >>>> Also, I would like to know your opinion about one. > >>>> > >>>>>> + > >>>>>> + marvell,system-controller: > >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle > >>>>>> + description: a phandle to the system controller. > >>>>>> + > >>>>>> +patternProperties: > >>>>>> + '^eth[0-9a-f]*(@.*)?$': > >>>>> > >>>>> The name should be "(ethernet-)?port", unless anything depends on > >>>>> particular naming? > >>>> > >>>> What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"? > >>>> It resembles pattern found in net/ethernet-phy.yaml like > >>>> properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while > >>>> still passing `dt_binding_check' and `dtbs_check'. It should also > >>>> comply with your comment. > >>> > >>> Node names like ethernet-eth do not make much sense because they contain > >>> redundant ethernet or eth. AFAIK, all other bindings like that call > >>> these ethernet-ports (or sometimes shorter - ports). Unless this device > >>> is different than all others? > >>> > >> > >> IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine > >> (as long as we don't have to modify the existing .dtsi files) - there > >> is no dependency in the driver code on that. > > > > Indeed, driver's code isn't dependent; however, there is a dependency > > on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.: > > > > https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190 > > https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72 > > > > Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files > > including the above would have to be modified to pass through `dtbs_check'. > > I didn't get it. What is the "dependency"? Usage of some names is not a > dependency... Old bindings were not precising any specific name of > subnodes, therefore I commented to change it. If the DTS already use > some other name, you can change them if none of upstream implementations > (BSD, bootloaders, firmware, Linux kernel) depend on it. > None of the PP2 drivers depends on nodes' names, so indeed we can safely modify that and update the relevant .dtsi files. One comment here, though - if we switch to e.g. ethernet-port@0 subnode, there is a requirement of specifying a 'reg' property: See below warning: Documentation/devicetree/bindings/net/marvell,pp2.example.dts:36.27-41.13: Warning (unit_address_vs_reg): /example-0/ethernet@f0000/ethernet-port@0: node has a unit name, but no reg or ranges property I think this convention is good and my idea is to use 'reg' property as a port ID (like the DSA does) - it would become required. However, we should retain 'port-id' to maintain backward compatibility. Once this schema gets accepted, I'll prepare a driver update. Best regards, Marcin
