pon., 3 paź 2022 o 19:29 Krzysztof Kozlowski
[off-list ref] napisał(a):

On 03/10/2022 19:06, Michał Grzelak wrote:

quoted

On 02/10/2022 10:23, Marcin Wojtas wrote:

quoted

niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
[off-list ref] napisał(a):

quoted

On 01/10/2022 17:53, Michał Grzelak wrote:

quoted

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.

quoted

quoted

+
+  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