| Message ID | 20211008002225.2426-9-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Multiple improvement for qca8337 switch | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Fri, Oct 08, 2021 at 02:22:18AM +0200, Ansuel Smith wrote: > Add names and decriptions of additional PORT0_PAD_CTRL properties. > Document new binding qca,mac6_exchange that exchange the mac0 port > with mac6. > qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock > phase to failling edge. > > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > Documentation/devicetree/bindings/net/dsa/qca8k.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt > index 9383d6bf2426..208ee5bc1bbb 100644 > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt > @@ -13,6 +13,11 @@ Required properties: > Optional properties: > > - reset-gpios: GPIO to be used to reset the whole device > +- qca,mac6-exchange: Internally swap MAC0 with MAC6. > +- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge. > + Mostly used in qca8327 with CPU port 0 set to > + sgmii. > +- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge. > - qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port. > This is needed for qca8337 and toggles the supply voltage > from 1.5v to 1.8v. For the specific regs it was observed The edge configuration is a port configuration. So it should be inside the port DT node it applies to. That also gives a clean way forward when a new switch appears with more SGMII interfaces, each with its own edge configuration. But that then leads into the MAC0/MAC6 swap mess. I need to think about that some more, how do we cleanly describe that in DT. Andrew
On Sat, Oct 09, 2021 at 07:07:16PM +0200, Andrew Lunn wrote: > On Fri, Oct 08, 2021 at 02:22:18AM +0200, Ansuel Smith wrote: > > Add names and decriptions of additional PORT0_PAD_CTRL properties. > > Document new binding qca,mac6_exchange that exchange the mac0 port > > with mac6. > > qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock > > phase to failling edge. > > > > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > Documentation/devicetree/bindings/net/dsa/qca8k.txt | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt > > index 9383d6bf2426..208ee5bc1bbb 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt > > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt > > @@ -13,6 +13,11 @@ Required properties: > > Optional properties: > > > > - reset-gpios: GPIO to be used to reset the whole device > > +- qca,mac6-exchange: Internally swap MAC0 with MAC6. > > +- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge. > > + Mostly used in qca8327 with CPU port 0 set to > > + sgmii. > > +- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge. > > - qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port. > > This is needed for qca8337 and toggles the supply voltage > > from 1.5v to 1.8v. For the specific regs it was observed > > The edge configuration is a port configuration. So it should be inside > the port DT node it applies to. That also gives a clean way forward > when a new switch appears with more SGMII interfaces, each with its > own edge configuration. > Problem here is that from Documentation falling edge can be set only on PAD0. PAD5 and PAD6 have the related bit reserved. But anyway qca8k support only single sgmii and it's not supported a config with multiple sgmii. Do we have standard binding for this? > But that then leads into the MAC0/MAC6 swap mess. I need to think > about that some more, how do we cleanly describe that in DT. > > Andrew About the mac swap. Do we really need to implement a complex thing for something that is really implemented internally to the switch? With this option MAC6 is swapped with MAC0. But with the port configuration in DT it doesn't change anything. Same reg, no change. It's really that some OEM connect the secondary port instead of the primary (for some reason, hw choice?) and swap them internally. Anyway some question. I will move the falling binding to the port DT node and move the configuration to mac_config. Should I keep the dedicated function to setup PAD0 swap or I can directly add the check in the qca8k_setup for only the bit related to enable the swap?
> Problem here is that from Documentation falling edge can be set only on > PAD0. PAD5 and PAD6 have the related bit reserved. Meaning in future, they could be used for this, if those ports get support for SGMII. > But anyway qca8k support only single sgmii and it's not supported a > config with multiple sgmii. Yet, until such hardware appears. We do see more support for SFPs. And more support for multi-gigi ports. Both of which use a SERDES interface which can support SGMII. So i would not be too surprised if future versions of the switch have more ports like this. > Do we have standard binding for this? No, there is no standard binding for this. This seems specific to these devices, maybe a proprietary extension to SGMII? > About the mac swap. Do we really need to implement a complex thing for > something that is really implemented internally to the switch? If it was truly internal to the switch, no. But i don't think it is. The DSA core has no idea the ports are swapped, and so i think will put the names on the wrong ports. Does devlink understand the ports are swapped? How about .ndo_get_phys_port_name? Will udev mix up the ports? The way you wanted to look in the other ports DT properties suggests it is not internal to the switch. I think to help my understanding, we need some examples of DTS files with and without the swap, where the properties are read from, what the interface names are, etc. > I will move the falling binding to the port DT node and move the > configuration to mac_config. Should I keep the > dedicated function to setup PAD0 swap or I can directly add the check in > the qca8k_setup for only the bit related to enable the swap? That does not matter too much. DT is an ABI, we should not change it later, so we need to look forward. C code is not ABI, it can be changed if/when more SGMII ports actually arrive. Andrew
On Sat, Oct 09, 2021 at 09:47:57PM +0200, Andrew Lunn wrote: > > Problem here is that from Documentation falling edge can be set only on > > PAD0. PAD5 and PAD6 have the related bit reserved. > > Meaning in future, they could be used for this, if those ports get > support for SGMII. > Ok. Then I will move all to DT port. Consider falling is set to PAD0 for both cpu port0 and cpu port6, should I make it hardcoded or should I add a condition and force the reg to PAD0 only for the current supported switch? Hope you understand what I mean. (Yes we have some config with cpu port6 set to sgmii and that require falling edge) > > But anyway qca8k support only single sgmii and it's not supported a > > config with multiple sgmii. > > Yet, until such hardware appears. We do see more support for SFPs. And > more support for multi-gigi ports. Both of which use a SERDES > interface which can support SGMII. So i would not be too surprised if > future versions of the switch have more ports like this. > > > Do we have standard binding for this? > > No, there is no standard binding for this. This seems specific to > these devices, maybe a proprietary extension to SGMII? > Then we are stuck to the special qca,... naming. > > About the mac swap. Do we really need to implement a complex thing for > > something that is really implemented internally to the switch? > > If it was truly internal to the switch, no. But i don't think it > is. The DSA core has no idea the ports are swapped, and so i think > will put the names on the wrong ports. Does devlink understand the > ports are swapped? How about .ndo_get_phys_port_name? Will udev mix up > the ports? > > The way you wanted to look in the other ports DT properties suggests > it is not internal to the switch. > > I think to help my understanding, we need some examples of DTS files > with and without the swap, where the properties are read from, what > the interface names are, etc. > Here is 2 configuration one from an Netgear r7800 qca8337: switch@10 { compatible = "qca,qca8337"; #address-cells = <1>; #size-cells = <0>; reg = <0x10>; qca8k,rgmii0_1_8v; qca8k,rgmii56_1_8v; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "cpu"; ethernet = <&gmac1>; phy-mode = "rgmii-id"; fixed-link { speed = <1000>; full-duplex; }; }; port@1 { reg = <1>; label = "lan1"; phy-mode = "internal"; phy-handle = <&phy_port1>; }; port@2 { reg = <2>; label = "lan2"; phy-mode = "internal"; phy-handle = <&phy_port2>; }; port@3 { reg = <3>; label = "lan3"; phy-mode = "internal"; phy-handle = <&phy_port3>; }; port@4 { reg = <4>; label = "lan4"; phy-mode = "internal"; phy-handle = <&phy_port4>; }; port@5 { reg = <5>; label = "wan"; phy-mode = "internal"; phy-handle = <&phy_port5>; }; port@6 { reg = <6>; label = "cpu"; ethernet = <&gmac2>; phy-mode = "sgmii"; fixed-link { speed = <1000>; full-duplex; }; }; }; And here is one with mac swap Tp-Link Archer c7 v4 qca8327 switch0@10 { compatible = "qca,qca8337"; #address-cells = <1>; #size-cells = <0>; reg = <0>; qca,sgmii-rxclk-falling-edge; qca,mac6-exchange; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "cpu"; ethernet = <ð0>; phy-mode = "sgmii"; fixed-link { speed = <1000>; full-duplex; }; }; port@1 { reg = <1>; label = "wan"; phy-mode = "internal"; phy-handle = <&phy_port1>; }; port@2 { reg = <2>; label = "lan1"; phy-mode = "internal"; phy-handle = <&phy_port2>; }; port@3 { reg = <3>; label = "lan2"; phy-mode = "internal"; phy-handle = <&phy_port3>; }; port@4 { reg = <4>; label = "lan3"; phy-mode = "internal"; phy-handle = <&phy_port4>; }; port@5 { reg = <5>; label = "lan4"; phy-mode = "internal"; phy-handle = <&phy_port5>; }; }; As you can see we use the mac06_exchange but we declare it as port0. DSA see it as port0 (as it should as it's internall swapped). We also don't need to apply some special function or stuff to apply the correct port in the ACL/regs/VLAN. The switch will treat MAC6 as MAC0 and MAC6 as MAC0. That is what we observed. Someone would say... Considering this switch is 2 port... and currently we use only one port. Why not drop this and use whatever is connected to port 0. Problem is that some device have the secondary cpu port NOT connected and they use mac6-exchange and that would result in no connection since cpu port 0 is not connected and cpu port 6 is never swapped. > > I will move the falling binding to the port DT node and move the > > configuration to mac_config. Should I keep the > > dedicated function to setup PAD0 swap or I can directly add the check in > > the qca8k_setup for only the bit related to enable the swap? > > That does not matter too much. DT is an ABI, we should not change it > later, so we need to look forward. C code is not ABI, it can be > changed if/when more SGMII ports actually arrive. > > Andrew
> Here is 2 configuration one from an Netgear r7800 qca8337: > > switch@10 { > compatible = "qca,qca8337"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x10>; > > qca8k,rgmii0_1_8v; > qca8k,rgmii56_1_8v; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > label = "cpu"; > ethernet = <&gmac1>; > phy-mode = "rgmii-id"; > > fixed-link { > speed = <1000>; > full-duplex; > }; > }; > > port@1 { > reg = <1>; > label = "lan1"; > phy-mode = "internal"; > phy-handle = <&phy_port1>; > }; > > port@2 { > reg = <2>; > label = "lan2"; > phy-mode = "internal"; > phy-handle = <&phy_port2>; > }; > > port@3 { > reg = <3>; > label = "lan3"; > phy-mode = "internal"; > phy-handle = <&phy_port3>; > }; > > port@4 { > reg = <4>; > label = "lan4"; > phy-mode = "internal"; > phy-handle = <&phy_port4>; > }; > > port@5 { > reg = <5>; > label = "wan"; > phy-mode = "internal"; > phy-handle = <&phy_port5>; > }; > > port@6 { > reg = <6>; > label = "cpu"; > ethernet = <&gmac2>; > phy-mode = "sgmii"; > > fixed-link { > speed = <1000>; > full-duplex; > }; So here, it is a second CPU port. But some other board could connect an SGMII PHY, and call the port lan5. Or it could be connected to an SFP cage, and used that way. Or are you forced to use it as a CPU port, or not use it at all? > And here is one with mac swap Tp-Link Archer c7 v4 qca8327 > > switch0@10 { > compatible = "qca,qca8337"; > #address-cells = <1>; > #size-cells = <0>; > > reg = <0>; > qca,sgmii-rxclk-falling-edge; > qca,mac6-exchange; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > label = "cpu"; > ethernet = <ð0>; > phy-mode = "sgmii"; > > fixed-link { > speed = <1000>; > full-duplex; > }; So when looking for SGMI properties, you need to look here. Where as in the previous example, you would look in port 6. And the reverse is true for RGMII delays. > }; > > port@1 { > reg = <1>; > label = "wan"; > phy-mode = "internal"; > phy-handle = <&phy_port1>; > }; > > port@2 { > reg = <2>; > label = "lan1"; > phy-mode = "internal"; > phy-handle = <&phy_port2>; > }; > > port@3 { > reg = <3>; > label = "lan2"; > phy-mode = "internal"; > phy-handle = <&phy_port3>; > }; > > port@4 { > reg = <4>; > label = "lan3"; > phy-mode = "internal"; > phy-handle = <&phy_port4>; > }; > > port@5 { > reg = <5>; > label = "lan4"; > phy-mode = "internal"; > phy-handle = <&phy_port5>; > }; > }; So here, port '6' is not used. But it could be connected to an RGMII PHY and called lan5. Would the naming work out? What does devlink think of it, etc. What about phy-handle? Is there an external MDIO bus? What address would be used if there is no phy-handle? Andrew
On Sat, Oct 09, 2021 at 11:37:10PM +0200, Andrew Lunn wrote: > > Here is 2 configuration one from an Netgear r7800 qca8337: > > > > switch@10 { > > compatible = "qca,qca8337"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0x10>; > > > > qca8k,rgmii0_1_8v; > > qca8k,rgmii56_1_8v; > > > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > > > port@0 { > > reg = <0>; > > label = "cpu"; > > ethernet = <&gmac1>; > > phy-mode = "rgmii-id"; > > > > fixed-link { > > speed = <1000>; > > full-duplex; > > }; > > }; > > > > port@1 { > > reg = <1>; > > label = "lan1"; > > phy-mode = "internal"; > > phy-handle = <&phy_port1>; > > }; > > > > port@2 { > > reg = <2>; > > label = "lan2"; > > phy-mode = "internal"; > > phy-handle = <&phy_port2>; > > }; > > > > port@3 { > > reg = <3>; > > label = "lan3"; > > phy-mode = "internal"; > > phy-handle = <&phy_port3>; > > }; > > > > port@4 { > > reg = <4>; > > label = "lan4"; > > phy-mode = "internal"; > > phy-handle = <&phy_port4>; > > }; > > > > port@5 { > > reg = <5>; > > label = "wan"; > > phy-mode = "internal"; > > phy-handle = <&phy_port5>; > > }; > > > > port@6 { > > reg = <6>; > > label = "cpu"; > > ethernet = <&gmac2>; > > phy-mode = "sgmii"; > > > > fixed-link { > > speed = <1000>; > > full-duplex; > > }; > > So here, it is a second CPU port. But some other board could connect > an SGMII PHY, and call the port lan5. Or it could be connected to an > SFP cage, and used that way. Or are you forced to use it as a CPU > port, or not use it at all? > We have a bit to set the mode. So yes it can be used to different modes. (base-x, phy and mac) > > And here is one with mac swap Tp-Link Archer c7 v4 qca8327 > > > > switch0@10 { > > compatible = "qca,qca8337"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > reg = <0>; > > qca,sgmii-rxclk-falling-edge; > > qca,mac6-exchange; > > > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > > > port@0 { > > reg = <0>; > > label = "cpu"; > > ethernet = <ð0>; > > phy-mode = "sgmii"; > > > > fixed-link { > > speed = <1000>; > > full-duplex; > > }; > > So when looking for SGMI properties, you need to look here. Where as > in the previous example, you would look in port 6. And the reverse is > true for RGMII delays. > > > }; > > > > port@1 { > > reg = <1>; > > label = "wan"; > > phy-mode = "internal"; > > phy-handle = <&phy_port1>; > > }; > > > > port@2 { > > reg = <2>; > > label = "lan1"; > > phy-mode = "internal"; > > phy-handle = <&phy_port2>; > > }; > > > > port@3 { > > reg = <3>; > > label = "lan2"; > > phy-mode = "internal"; > > phy-handle = <&phy_port3>; > > }; > > > > port@4 { > > reg = <4>; > > label = "lan3"; > > phy-mode = "internal"; > > phy-handle = <&phy_port4>; > > }; > > > > port@5 { > > reg = <5>; > > label = "lan4"; > > phy-mode = "internal"; > > phy-handle = <&phy_port5>; > > }; > > }; > > So here, port '6' is not used. But it could be connected to an RGMII > PHY and called lan5. Would the naming work out? What does devlink > think of it, etc. What about phy-handle? Is there an external MDIO > bus? What address would be used if there is no phy-handle? > > Andrew In this case port6 is not used as it's not connected at all in hardware. From the configuration list yes, it can be used as lan5 in phy mode and it would have address 5 (internally the address are with an offset of -1). Anyway I honestly think that we are putting too much effort in something that can and should be handled differently. I agree that all this mac exchange is bs and doesn't make much sense. I tried to implement this as we currently qca8k is hardcoded to expect the cpu port 0 for everything and doesn't actually found a valid cpu port (aka it doesn't expect a configuration with cpu6) I think the driver was writtent with the concept of mac exchange from the start. That's why it's hardoced to port0. I actually tied for fun running the switch using only the port6 cpu port and it worked just right. So I think I will just drop the mac exchange and fix the code to make it dynamic.
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt index 9383d6bf2426..208ee5bc1bbb 100644 --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt @@ -13,6 +13,11 @@ Required properties: Optional properties: - reset-gpios: GPIO to be used to reset the whole device +- qca,mac6-exchange: Internally swap MAC0 with MAC6. +- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge. + Mostly used in qca8327 with CPU port 0 set to + sgmii. +- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge. - qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port. This is needed for qca8337 and toggles the supply voltage from 1.5v to 1.8v. For the specific regs it was observed