| Message ID | 20221130141040.32447-6-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | remove label = "cpu" from DSA dt-binding | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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
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
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
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.
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?
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ç
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 --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";
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(-)