diff mbox series

[5/5] powerpc: dts: remove label = "cpu" from DSA dt-binding

Message ID 20221130141040.32447-6-arinc.unal@arinc9.com (mailing list archive)
State Not Applicable
Headers show
Series remove label = "cpu" from DSA dt-binding | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Arınç ÜNAL Nov. 30, 2022, 2:10 p.m. UTC
This is not used by the DSA dt-binding, so remove it from all devicetrees.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 arch/powerpc/boot/dts/turris1x.dts | 2 --
 1 file changed, 2 deletions(-)

Comments

Michael Ellerman Dec. 1, 2022, 10:40 a.m. UTC | #1
Arınç ÜNAL <arinc.unal@arinc9.com> writes:
> This is not used by the DSA dt-binding, so remove it from all devicetrees.
>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  arch/powerpc/boot/dts/turris1x.dts | 2 --
>  1 file changed, 2 deletions(-)

Adding Pali to Cc.

These were only recently updated in commit:

  8bf056f57f1d ("powerpc: dts: turris1x.dts: Fix labels in DSA cpu port nodes")

Which said:

  DSA cpu port node has to be marked with "cpu" label.

But if the binding doesn't use them then I'm confused why they needed to
be updated.

cheers


> diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
> index 045af668e928..3841c8d96d00 100644
> --- a/arch/powerpc/boot/dts/turris1x.dts
> +++ b/arch/powerpc/boot/dts/turris1x.dts
> @@ -147,7 +147,6 @@ ports {
>  
>  					port@0 {
>  						reg = <0>;
> -						label = "cpu";
>  						ethernet = <&enet1>;
>  						phy-mode = "rgmii-id";
>  
> @@ -184,7 +183,6 @@ port@5 {
>  
>  					port@6 {
>  						reg = <6>;
> -						label = "cpu";
>  						ethernet = <&enet0>;
>  						phy-mode = "rgmii-id";
>  
> -- 
> 2.34.1
Pali Rohár Dec. 1, 2022, 5:39 p.m. UTC | #2
On Thursday 01 December 2022 21:40:03 Michael Ellerman wrote:
> Arınç ÜNAL <arinc.unal@arinc9.com> writes:
> > This is not used by the DSA dt-binding, so remove it from all devicetrees.
> >
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > ---
> >  arch/powerpc/boot/dts/turris1x.dts | 2 --
> >  1 file changed, 2 deletions(-)
> 
> Adding Pali to Cc.
> 
> These were only recently updated in commit:
> 
>   8bf056f57f1d ("powerpc: dts: turris1x.dts: Fix labels in DSA cpu port nodes")
> 
> Which said:
> 
>   DSA cpu port node has to be marked with "cpu" label.
> 
> But if the binding doesn't use them then I'm confused why they needed to
> be updated.
> 
> cheers

I was told by Marek (CCed) that DSA port connected to CPU should have
label "cpu" and not "cpu<number>". Modern way for specifying CPU port is
by defining reference to network device, which there is already (&enet1
and &enet0). So that change just "fixed" incorrect naming cpu0 and cpu1.

So probably linux kernel does not need label = "cpu" in DTS anymore. But
this is not the reason to remove this property. Linux kernel does not
use lot of other nodes and properties too... Device tree should describe
hardware and not its usage in Linux. "label" property is valid in device
tree and it exactly describes what or where is this node connected. And
it may be used for other systems.

So I do not see a point in removing "label" properties from turris1x.dts
file, nor from any other dts file.

> 
> > diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
> > index 045af668e928..3841c8d96d00 100644
> > --- a/arch/powerpc/boot/dts/turris1x.dts
> > +++ b/arch/powerpc/boot/dts/turris1x.dts
> > @@ -147,7 +147,6 @@ ports {
> >  
> >  					port@0 {
> >  						reg = <0>;
> > -						label = "cpu";
> >  						ethernet = <&enet1>;
> >  						phy-mode = "rgmii-id";
> >  
> > @@ -184,7 +183,6 @@ port@5 {
> >  
> >  					port@6 {
> >  						reg = <6>;
> > -						label = "cpu";
> >  						ethernet = <&enet0>;
> >  						phy-mode = "rgmii-id";
> >  
> > -- 
> > 2.34.1
Rob Herring (Arm) Dec. 1, 2022, 11:44 p.m. UTC | #3
On Thu, Dec 01, 2022 at 06:39:02PM +0100, Pali Rohár wrote:
> On Thursday 01 December 2022 21:40:03 Michael Ellerman wrote:
> > Arınç ÜNAL <arinc.unal@arinc9.com> writes:
> > > This is not used by the DSA dt-binding, so remove it from all devicetrees.
> > >
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > >  arch/powerpc/boot/dts/turris1x.dts | 2 --
> > >  1 file changed, 2 deletions(-)
> > 
> > Adding Pali to Cc.
> > 
> > These were only recently updated in commit:
> > 
> >   8bf056f57f1d ("powerpc: dts: turris1x.dts: Fix labels in DSA cpu port nodes")
> > 
> > Which said:
> > 
> >   DSA cpu port node has to be marked with "cpu" label.
> > 
> > But if the binding doesn't use them then I'm confused why they needed to
> > be updated.
> > 
> > cheers
> 
> I was told by Marek (CCed) that DSA port connected to CPU should have
> label "cpu" and not "cpu<number>". Modern way for specifying CPU port is
> by defining reference to network device, which there is already (&enet1
> and &enet0). So that change just "fixed" incorrect naming cpu0 and cpu1.
> 
> So probably linux kernel does not need label = "cpu" in DTS anymore. But
> this is not the reason to remove this property. Linux kernel does not
> use lot of other nodes and properties too... Device tree should describe
> hardware and not its usage in Linux. "label" property is valid in device
> tree and it exactly describes what or where is this node connected. And
> it may be used for other systems.
> 
> So I do not see a point in removing "label" properties from turris1x.dts
> file, nor from any other dts file.

Well, it seems like a bit of an abuse of 'label' to me. 'label' should 
be aligned with a sticker or other identifier identifying something to a 
human. Software should never care what the value of 'label' is.

Rob
Pali Rohár Dec. 2, 2022, 7:35 p.m. UTC | #4
On Thursday 01 December 2022 17:44:00 Rob Herring wrote:
> On Thu, Dec 01, 2022 at 06:39:02PM +0100, Pali Rohár wrote:
> > On Thursday 01 December 2022 21:40:03 Michael Ellerman wrote:
> > > Arınç ÜNAL <arinc.unal@arinc9.com> writes:
> > > > This is not used by the DSA dt-binding, so remove it from all devicetrees.
> > > >
> > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > ---
> > > >  arch/powerpc/boot/dts/turris1x.dts | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > 
> > > Adding Pali to Cc.
> > > 
> > > These were only recently updated in commit:
> > > 
> > >   8bf056f57f1d ("powerpc: dts: turris1x.dts: Fix labels in DSA cpu port nodes")
> > > 
> > > Which said:
> > > 
> > >   DSA cpu port node has to be marked with "cpu" label.
> > > 
> > > But if the binding doesn't use them then I'm confused why they needed to
> > > be updated.
> > > 
> > > cheers
> > 
> > I was told by Marek (CCed) that DSA port connected to CPU should have
> > label "cpu" and not "cpu<number>". Modern way for specifying CPU port is
> > by defining reference to network device, which there is already (&enet1
> > and &enet0). So that change just "fixed" incorrect naming cpu0 and cpu1.
> > 
> > So probably linux kernel does not need label = "cpu" in DTS anymore. But
> > this is not the reason to remove this property. Linux kernel does not
> > use lot of other nodes and properties too... Device tree should describe
> > hardware and not its usage in Linux. "label" property is valid in device
> > tree and it exactly describes what or where is this node connected. And
> > it may be used for other systems.
> > 
> > So I do not see a point in removing "label" properties from turris1x.dts
> > file, nor from any other dts file.
> 
> Well, it seems like a bit of an abuse of 'label' to me. 'label' should 
> be aligned with a sticker or other identifier identifying something to a 
> human. Software should never care what the value of 'label' is.
> 
> Rob

But it already does. "label" property is used for setting (initial)
network interface name for DSA drivers. And you can try to call e.g.
git grep '"cpu"' net/dsa drivers/net/dsa to see that cpu is still
present on some dsa places (probably relict or backward compatibility
before eth reference).

I agree with you that in this case it is abuse. But I would not say that
software should not care about "label". I think that software should
care about "label" but only in situation in which it presents
information to user. So if user wants to see device with labels *ABC*
(meaning show me anything which stickers contains substring ABC) then
software should filter devices and turns that with asked label.

The main problem here is _existing_ software. New software should really
do not use cpu label for deciding if network port is connected to cpu or
not and it should be designed correctly. But you cannot change nor fix
old / existing software...

The worst thing which can be done is breaking updated version of (old)
software. Prevention is always testing software and in this case testing
on the real hardware. I know, it is hard as developers do not have
such lot of hardware devices and configurations.
Vladimir Oltean Dec. 4, 2022, 6:59 p.m. UTC | #5
Hi Pali,

On Fri, Dec 02, 2022 at 08:35:52PM +0100, Pali Rohár wrote:
> On Thursday 01 December 2022 17:44:00 Rob Herring wrote:
> > On Thu, Dec 01, 2022 at 06:39:02PM +0100, Pali Rohár wrote:
> > > I was told by Marek (CCed) that DSA port connected to CPU should have
> > > label "cpu" and not "cpu<number>". Modern way for specifying CPU port is
> > > by defining reference to network device, which there is already (&enet1
> > > and &enet0). So that change just "fixed" incorrect naming cpu0 and cpu1.
> > > 
> > > So probably linux kernel does not need label = "cpu" in DTS anymore. But
> > > this is not the reason to remove this property. Linux kernel does not
> > > use lot of other nodes and properties too... Device tree should describe
> > > hardware and not its usage in Linux. "label" property is valid in device
> > > tree and it exactly describes what or where is this node connected. And
> > > it may be used for other systems.
> > > 
> > > So I do not see a point in removing "label" properties from turris1x.dts
> > > file, nor from any other dts file.
> > 
> > Well, it seems like a bit of an abuse of 'label' to me. 'label' should 
> > be aligned with a sticker or other identifier identifying something to a 
> > human. Software should never care what the value of 'label' is.
> 
> But it already does. "label" property is used for setting (initial)
> network interface name for DSA drivers. And you can try to call e.g.
> git grep '"cpu"' net/dsa drivers/net/dsa to see that cpu is still
> present on some dsa places (probably relict or backward compatibility
> before eth reference).

Can you try to eliminate the word "probably" from the information you
transmit and be specific about when did the DSA binding parse or require
the 'label = "cpu"' property for CPU ports in any way?
Arınç ÜNAL Dec. 5, 2022, 7:15 p.m. UTC | #6
On 4.12.2022 21:59, Vladimir Oltean wrote:
> Hi Pali,
> 
> On Fri, Dec 02, 2022 at 08:35:52PM +0100, Pali Rohár wrote:
>> On Thursday 01 December 2022 17:44:00 Rob Herring wrote:
>>> On Thu, Dec 01, 2022 at 06:39:02PM +0100, Pali Rohár wrote:
>>>> I was told by Marek (CCed) that DSA port connected to CPU should have
>>>> label "cpu" and not "cpu<number>". Modern way for specifying CPU port is
>>>> by defining reference to network device, which there is already (&enet1
>>>> and &enet0). So that change just "fixed" incorrect naming cpu0 and cpu1.
>>>>
>>>> So probably linux kernel does not need label = "cpu" in DTS anymore. But
>>>> this is not the reason to remove this property. Linux kernel does not
>>>> use lot of other nodes and properties too... Device tree should describe
>>>> hardware and not its usage in Linux. "label" property is valid in device
>>>> tree and it exactly describes what or where is this node connected. And
>>>> it may be used for other systems.
>>>>
>>>> So I do not see a point in removing "label" properties from turris1x.dts
>>>> file, nor from any other dts file.
>>>
>>> Well, it seems like a bit of an abuse of 'label' to me. 'label' should
>>> be aligned with a sticker or other identifier identifying something to a
>>> human. Software should never care what the value of 'label' is.
>>
>> But it already does. "label" property is used for setting (initial)
>> network interface name for DSA drivers. And you can try to call e.g.
>> git grep '"cpu"' net/dsa drivers/net/dsa to see that cpu is still
>> present on some dsa places (probably relict or backward compatibility
>> before eth reference).
> 
> Can you try to eliminate the word "probably" from the information you
> transmit and be specific about when did the DSA binding parse or require
> the 'label = "cpu"' property for CPU ports in any way?

As Jonas (on CC) pointed out, I only see this being used in the swconfig 
b53 driver which uses the label to identify the cpu port.

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/drivers/net/phy/b53/b53_common.c;h=87d731ec3e2a868dc8389f554b1dc9ab42c30be2;hb=HEAD#l1508

Maybe this got into DSA dt-bindings unchecked before it was decided to 
move forward with DSA instead of swconfig on Linux.

Arınç
Vladimir Oltean Dec. 7, 2022, 1:13 p.m. UTC | #7
On Mon, Dec 05, 2022 at 10:15:16PM +0300, Arınç ÜNAL wrote:
> As Jonas (on CC) pointed out, I only see this being used in the swconfig b53
> driver which uses the label to identify the cpu port.
> 
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/drivers/net/phy/b53/b53_common.c;h=87d731ec3e2a868dc8389f554b1dc9ab42c30be2;hb=HEAD#l1508
> 
> Maybe this got into DSA dt-bindings unchecked before it was decided to move
> forward with DSA instead of swconfig on Linux.

Yes, but swconfig is not DSA and their bindings are not compatible
anyway (swconfig lacks an ethernet = <&phandle> property that would
allow DSA to work). Still waiting for Pali to clarify what he had in mind.
diff mbox series

Patch

diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index 045af668e928..3841c8d96d00 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -147,7 +147,6 @@  ports {
 
 					port@0 {
 						reg = <0>;
-						label = "cpu";
 						ethernet = <&enet1>;
 						phy-mode = "rgmii-id";
 
@@ -184,7 +183,6 @@  port@5 {
 
 					port@6 {
 						reg = <6>;
-						label = "cpu";
 						ethernet = <&enet0>;
 						phy-mode = "rgmii-id";