| Message ID | YfwRN2ObqFbrw/fF@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: mt7530: updates for phylink changes | expand |
On Thu, Feb 03, 2022 at 05:30:31PM +0000, Russell King (Oracle) wrote: > Hi, > > This series is a partial conversion of the mt7530 DSA driver to the > modern phylink infrastructure. This driver has some exceptional cases > which prevent - at the moment - its full conversion (particularly with > the Autoneg bit) to using phylink_generic_validate(). > > What stands in the way is this if() condition in > mt753x_phylink_validate(): > > if (state->interface != PHY_INTERFACE_MODE_TRGMII || > !phy_interface_mode_is_8023z(state->interface)) { > > reduces to being always true. I highlight this here for the attention > of the driver maintainers. I'm intending to submit this series later today, preserving the above behaviour, as I like to keep drivers bug-for-bug compatible, with the assumption that they've been tested as working, even if the code looks wrong. However, it would be good if this point could be addressed. Thanks.
On Wed, 2022-02-09 at 21:06 +0800, Russell King (Oracle) wrote: > On Thu, Feb 03, 2022 at 05:30:31PM +0000, Russell King (Oracle) > wrote: > > Hi, > > > > This series is a partial conversion of the mt7530 DSA driver to the > > modern phylink infrastructure. This driver has some exceptional > > cases > > which prevent - at the moment - its full conversion (particularly > > with > > the Autoneg bit) to using phylink_generic_validate(). > > > > What stands in the way is this if() condition in > > mt753x_phylink_validate(): > > > > if (state->interface != PHY_INTERFACE_MODE_TRGMII || > > !phy_interface_mode_is_8023z(state->interface)) { > > > > reduces to being always true. I highlight this here for the > > attention > > of the driver maintainers. Hi Russel, The above behaviour is really a bug. "&&" should be used to prevent setting MAC_10, MAC_100 and Antoneg capability in particular interface mode in original code. However, these capability depend on the link partner of the MAC, such as Ethernet phy. It's okay to keep setting them. Thanks for this series. > > I'm intending to submit this series later today, preserving the above > behaviour, as I like to keep drivers bug-for-bug compatible, with the > assumption that they've been tested as working, even if the code > looks > wrong. However, it would be good if this point could be addressed. > Thanks. >
On Thu, Feb 10, 2022 at 01:33:34AM +0800, Landen Chao wrote: > On Wed, 2022-02-09 at 21:06 +0800, Russell King (Oracle) wrote: > > On Thu, Feb 03, 2022 at 05:30:31PM +0000, Russell King (Oracle) > > wrote: > > > Hi, > > > > > > This series is a partial conversion of the mt7530 DSA driver to the > > > modern phylink infrastructure. This driver has some exceptional > > > cases > > > which prevent - at the moment - its full conversion (particularly > > > with > > > the Autoneg bit) to using phylink_generic_validate(). > > > > > > What stands in the way is this if() condition in > > > mt753x_phylink_validate(): > > > > > > if (state->interface != PHY_INTERFACE_MODE_TRGMII || > > > !phy_interface_mode_is_8023z(state->interface)) { > > > > > > reduces to being always true. I highlight this here for the > > > attention > > > of the driver maintainers. > Hi Russel, > > The above behaviour is really a bug. "&&" should be used to prevent > setting MAC_10, MAC_100 and Antoneg capability in particular interface > mode in original code. However, these capability depend on the link > partner of the MAC, such as Ethernet phy. It's okay to keep setting > them. Hi Landen, Thanks for the response. I think you have a slight misunderstanding about these capabilities, both in the old code and the new code. You shouldn't care about e.g. the ethernet PHY's capabilities in the validate() callback at all - phylink will look at the capabilities reported by phylib, and mask out anything that the MAC says shouldn't be supported, which has the effect of restricting what the ethernet PHY will advertise. In the old code, the validate() callback should only be concerned with what the MAC and PCS can support - e.g. if the MAC isn't capable of supportig 1G half-duplex, then the 1G HD capabilities should be masked out. With the new code, PCS gain their own validation function, which means that the validate() callback then becomes very much just about the MAC, and with phylink_generic_validate(), we can get away with just specifying a bitmap of the supported interface types for the MAC/PCS end of the system, and the MAC speeds that are supported. Given your feedback, I will re-jig the series to take account of your comments - thanks.
On Thu, 2022-02-10 at 01:47 +0800, Russell King (Oracle) wrote: > On Thu, Feb 10, 2022 at 01:33:34AM +0800, Landen Chao wrote: > > On Wed, 2022-02-09 at 21:06 +0800, Russell King (Oracle) wrote: > > > On Thu, Feb 03, 2022 at 05:30:31PM +0000, Russell King (Oracle) > > > wrote: > > > > Hi, > > > > > > > > This series is a partial conversion of the mt7530 DSA driver to > > > > the > > > > modern phylink infrastructure. This driver has some exceptional > > > > cases > > > > which prevent - at the moment - its full conversion > > > > (particularly > > > > with > > > > the Autoneg bit) to using phylink_generic_validate(). > > > > > > > > What stands in the way is this if() condition in > > > > mt753x_phylink_validate(): > > > > > > > > if (state->interface != PHY_INTERFACE_MODE_TRGMII || > > > > !phy_interface_mode_is_8023z(state->interface)) { > > > > > > > > reduces to being always true. I highlight this here for the > > > > attention > > > > of the driver maintainers. > > > > Hi Russel, > > > > The above behaviour is really a bug. "&&" should be used to prevent > > setting MAC_10, MAC_100 and Antoneg capability in particular > > interface > > mode in original code. However, these capability depend on the link > > partner of the MAC, such as Ethernet phy. It's okay to keep setting > > them. > > Hi Landen, > > Thanks for the response. I think you have a slight misunderstanding > about these capabilities, both in the old code and the new code. > > You shouldn't care about e.g. the ethernet PHY's capabilities in the > validate() callback at all - phylink will look at the capabilities > reported by phylib, and mask out anything that the MAC says shouldn't > be supported, which has the effect of restricting what the ethernet > PHY will advertise. > > In the old code, the validate() callback should only be concerned > with > what the MAC and PCS can support - e.g. if the MAC isn't capable of > supportig 1G half-duplex, then the 1G HD capabilities should be > masked > out. > > With the new code, PCS gain their own validation function, which > means > that the validate() callback then becomes very much just about the > MAC, > and with phylink_generic_validate(), we can get away with just > specifying a bitmap of the supported interface types for the MAC/PCS > end of the system, and the MAC speeds that are supported. > > Given your feedback, I will re-jig the series to take account of your > comments - thanks. > Hi Russell, Thanks for your guidance. I've been stuck with an unnecessary problem, "should I export MAC_1000/MAC_100/MAC_10 capability of MAC when PCS is connected with an Ethernet PHY supports both 2500base-X and SGMII mode?" Now I know the answer, just export all capability that MAC and PCS can support in the validate(). phylink will help to find out the final configuration by coworking with phylib.
Hi, This series is a partial conversion of the mt7530 DSA driver to the modern phylink infrastructure. This driver has some exceptional cases which prevent - at the moment - its full conversion (particularly with the Autoneg bit) to using phylink_generic_validate(). What stands in the way is this if() condition in mt753x_phylink_validate(): if (state->interface != PHY_INTERFACE_MODE_TRGMII || !phy_interface_mode_is_8023z(state->interface)) { reduces to being always true. I highlight this here for the attention of the driver maintainers. Patch 1 populates the supported_interfaces for each port Patch 2 removes the interface checks that become unnecessary as a result of patch 1. Patch 3 removes use of phylink_helper_basex_speed() which is no longer required by phylink. Patch 4 becomes possible after patch 3, only indicating the ethtool modes that can be supported with a particular interface mode - this involves removing some modes and adding others as per phylink documentation. Patch 5 continues patch 4, as RGMII can support 1000base-X ethtool link mode with an appropriate external PHY. Patch 6 switches the driver to use phylink_get_linkmodes(), which moves the driver as close as we can to phylink_generic_validate() due to the Autoneg bit issue mentioned above. Patch 7 marks the driver as non-legacy. drivers/net/dsa/mt7530.c | 166 ++++++++++++++++------------------------------- drivers/net/dsa/mt7530.h | 5 +- 2 files changed, 58 insertions(+), 113 deletions(-)