| Message ID | 20211117225050.18395-5-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Extend `phy-mode` to string array | expand |
On Wed, Nov 17, 2021 at 11:50:46PM +0100, Marek Behún wrote: > Now that the 'phy-mode' property can be a string array containing more > PHY modes (all that are supported by the board), update the bitmap of > interfaces supported by the MAC with this property. > > Normally this would be a simple intersection (of interfaces supported by > the current implementation of the driver and interfaces supported by the > board), but we need to keep being backwards compatible with older DTs, > which may only define one mode, since, as Russell King says, > conventionally phy-mode has meant "this is the mode we want to operate > the PHY interface in" which was fine when PHYs didn't change their > mode depending on the media speed > > An example is DT defining > phy-mode = "sgmii"; > but the board supporting also 1000base-x and 2500base-x. > > Add the following logic to keep this backwards compatiblity: > - if more PHY modes are defined, do a simple intersection > - if one PHY mode is defined: > - if it is sgmii, 1000base-x or 2500base-x, add all three and then do > the intersection > - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r, > 2500base-x, 1000base-x and sgmii, and then do the intersection > > This is simple enough and should work for all boards. > > Nonetheless it is possible (although extremely unlikely, in my opinion) > that a board will be found that (for example) defines > phy-mode = "sgmii"; > and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the > board DOESN'T support 2500base-x, because of electrical reasons (since > the frequency is 2.5x of sgmii). > Our code will in this case incorrectly infer also support for > 2500base-x. To avoid this, the board maintainer should either change DTS > to > phy-mode = "sgmii", "1000base-x"; > and update device tree on all boards, or, if that is impossible, add a > fix into the function we are introducing in this commit. > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++++ > include/linux/phy.h | 6 ++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index f7156b6868e7..6d7c216a5dea 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -563,6 +563,67 @@ static int phylink_parse_fixedlink(struct phylink *pl, > return 0; > } > > +static void phylink_update_phy_modes(struct phylink *pl, > + struct fwnode_handle *fwnode) > +{ > + unsigned long *supported = pl->config->supported_interfaces; > + DECLARE_PHY_INTERFACE_MASK(modes); > + > + if (fwnode_get_phy_modes(fwnode, modes) < 0) > + return; > + > + if (phy_interface_empty(modes)) > + return; > + > + /* If supported is empty, just copy modes defined in fwnode. */ > + if (phy_interface_empty(supported)) > + return phy_interface_copy(supported, modes); Doesn't this mean we always end up with the supported_interfaces field filled in, even for drivers that haven't yet been converted? It will have the effect of locking the driver to the interface mode in "modes" where only one interface mode is mentioned in DT. At the moment, I think the only drivers that would be affected would be some DSA drivers, stmmac and macb as they haven't yet been converted.
> > + /* If supported is empty, just copy modes defined in fwnode. */ > > + if (phy_interface_empty(supported)) > > + return phy_interface_copy(supported, modes); > > Doesn't this mean we always end up with the supported_interfaces field > filled in, even for drivers that haven't yet been converted? It will > have the effect of locking the driver to the interface mode in "modes" > where only one interface mode is mentioned in DT. > > At the moment, I think the only drivers that would be affected would be > some DSA drivers, stmmac and macb as they haven't yet been converted. Hi Russell What do you think the best way forward is? Got those converted before merging this? Andrew
On Thu, Nov 18, 2021 at 05:33:33PM +0100, Andrew Lunn wrote: > > > + /* If supported is empty, just copy modes defined in fwnode. */ > > > + if (phy_interface_empty(supported)) > > > + return phy_interface_copy(supported, modes); > > > > Doesn't this mean we always end up with the supported_interfaces field > > filled in, even for drivers that haven't yet been converted? It will > > have the effect of locking the driver to the interface mode in "modes" > > where only one interface mode is mentioned in DT. > > > > At the moment, I think the only drivers that would be affected would be > > some DSA drivers, stmmac and macb as they haven't yet been converted. > > Hi Russell > > What do you think the best way forward is? Got those converted before > merging this? The situation is as follows: - For macb, I have a bunch of patches ready to go against net-next (in git log order): net: macb: use phylink_generic_validate() net: macb: clean up macb_validate() net: macb: remove interface checks in macb_validate() net: macb: populate supported_interfaces member but I think Sean and myself need to finish discussing PCS capabilities before I send those. - For mv88e6xxx DSA, I have some patches - I need to check with Marek whether he has any further changes for those as he's been looking over them and checking with the Marvell specs before I can send them. - For mt7530 DSA, I also have some patches, but no way to test them. All of the above patches are in my net-queue branch: http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue - For stmmac, I pinged Jose Abreu about it. stmmac's validate function does not check the interface mode at all if there is no XPCS attached. Jose says that which interface modes are supported is up to the integrator to decide, and it seems there is nothing in the current driver to work out which interface modes can be supported. If there is an XPCS attached, it defers interface mode checks to xpcs_validate(), which uses the interface mode to walk an array to find a list of linkmodes that the PCS supports. So, I think it's going to take a bit of unpicking to work out what to do here, and may depend on what we come up with with Sean for PCS. That leaves quite a number of DSA drivers untouched. So, I think we need to realise that even though we have all the users in the kernel, making changes to phylink's API is always going to be difficult, and we always need to maintain compatibility for old ways of doing stuff - at least until every user can be converted. However, that brings with it its own problem - there is then no motivation for people to adapt to the new way. This can be seen as we have the compatibility for calling mac_config() whenever the link comes up 16 months after the PCS stuff was introduced. One of the problem drivers for this is mtk_eth_soc: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=278da006a936c0743c7fc261c23e7de992ac78e6 René van Dorst said in response to me posting that patch in July 2020: > I know, you have pointed that out before. But I don't know how to fix > mtk_gmac0_rgmii_adjust(). This function changes the PLL of the MAC. > But without documentation I am not sure what all the bits are used > for. and my attempts to discuss a way forward didn't get a reply. So, mtk_eth_soc has become an example of a problematical driver that makes ongoing phylink changes difficult, and I fear stmmac may become another example. I'm quite certain that as we try to develop phylink, such as adding extra facilities like specifying the interface modes, we're going to end up running into these kinds of problems that we can't solve, and we are going to have to keep compatibility for the old ways of doing stuff going for years to come - which is just going to get more and more painful.
On Thu, 18 Nov 2021 17:08:10 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > I'm quite certain that as we try to develop phylink, such as adding > extra facilities like specifying the interface modes, we're going to > end up running into these kinds of problems that we can't solve, and > we are going to have to keep compatibility for the old ways of doing > stuff going for years to come - which is just going to get more and > more painful. One way to move this forward would be to check compatible of the corresponding MAC in this new function. If it belongs to an unconverted driver, we can ensure the old behaviour. This would require to add a comment to the driver saying "if you convert this, please drop the exception in phylink_update_phy_modes()". Yes, it is an ugly solution, but it would work and this function already is meant to be solving backward compatibility problems by such mechanisms. The comment says: + * Nonetheless it is possible that a device may support only one mode, + * for example 1000base-x, due to strapping pins or some other reasons. + * If a specific device supports only the mode mentioned in DT, the + * exception should be made here with of_machine_is_compatible(). BTW, I now realize that of_machine_is_compatible() is not the best solution. I will change the comment in v2. Marek
On Thu, Nov 18, 2021 at 06:33:01PM +0100, Marek Behún wrote: > On Thu, 18 Nov 2021 17:08:10 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > I'm quite certain that as we try to develop phylink, such as adding > > extra facilities like specifying the interface modes, we're going to > > end up running into these kinds of problems that we can't solve, and > > we are going to have to keep compatibility for the old ways of doing > > stuff going for years to come - which is just going to get more and > > more painful. > > One way to move this forward would be to check compatible of the > corresponding MAC in this new function. If it belongs to an unconverted > driver, we can ensure the old behaviour. This would require to add a > comment to the driver saying "if you convert this, please drop the > exception in phylink_update_phy_modes()". That could work when drivers pass the fwnode as the "device" node. Some drivers don't do that, they pass a sub-node instead - such as mvpp2. However, mvpp2 is of course up to date with all phylink developments. However, the same issue exists with DSA - the fwnode passed to phylink doesn't have a compatible. I suppose we could walk up the fwnode tree until we do find a node with a compatible property, that may work.
On 11/17/21 5:50 PM, Marek Behún wrote: > Now that the 'phy-mode' property can be a string array containing more > PHY modes (all that are supported by the board), update the bitmap of > interfaces supported by the MAC with this property. > > Normally this would be a simple intersection (of interfaces supported by > the current implementation of the driver and interfaces supported by the > board), but we need to keep being backwards compatible with older DTs, > which may only define one mode, since, as Russell King says, > conventionally phy-mode has meant "this is the mode we want to operate > the PHY interface in" which was fine when PHYs didn't change their > mode depending on the media speed > > An example is DT defining > phy-mode = "sgmii"; > but the board supporting also 1000base-x and 2500base-x. > > Add the following logic to keep this backwards compatiblity: > - if more PHY modes are defined, do a simple intersection > - if one PHY mode is defined: > - if it is sgmii, 1000base-x or 2500base-x, add all three and then do > the intersection > - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r, > 2500base-x, 1000base-x and sgmii, and then do the intersection > > This is simple enough and should work for all boards. > > Nonetheless it is possible (although extremely unlikely, in my opinion) > that a board will be found that (for example) defines > phy-mode = "sgmii"; > and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the > board DOESN'T support 2500base-x, because of electrical reasons (since > the frequency is 2.5x of sgmii). > Our code will in this case incorrectly infer also support for > 2500base-x. To avoid this, the board maintainer should either change DTS > to > phy-mode = "sgmii", "1000base-x"; > and update device tree on all boards, or, if that is impossible, add a > fix into the function we are introducing in this commit. Can you touch on the 5G/10G stuff as well in the message? > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++++ > include/linux/phy.h | 6 ++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index f7156b6868e7..6d7c216a5dea 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -563,6 +563,67 @@ static int phylink_parse_fixedlink(struct phylink *pl, > return 0; > } > > +static void phylink_update_phy_modes(struct phylink *pl, > + struct fwnode_handle *fwnode) > +{ > + unsigned long *supported = pl->config->supported_interfaces; > + DECLARE_PHY_INTERFACE_MASK(modes); > + > + if (fwnode_get_phy_modes(fwnode, modes) < 0) > + return; > + > + if (phy_interface_empty(modes)) > + return; > + > + /* If supported is empty, just copy modes defined in fwnode. */ > + if (phy_interface_empty(supported)) > + return phy_interface_copy(supported, modes); > + > + /* We want the intersection of given supported modes with those defined > + * in DT. > + * > + * Some older device-trees mention only one of `sgmii`, `1000base-x` or > + * `2500base-x`, while supporting all three. Other mention `10gbase-r` > + * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`, > + * `2500base-x` and `5gbase-r`. > + * For backwards compatibility with these older DTs, make it so that if > + * one of these modes is mentioned in DT and MAC supports more of them, > + * keep all that are supported according to the logic above. > + * > + * Nonetheless it is possible that a device may support only one mode, > + * for example 1000base-x, due to strapping pins or some other reasons. > + * If a specific device supports only the mode mentioned in DT, the > + * exception should be made here with of_machine_is_compatible(). > + */ > + if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) { > + bool lower = false; > + > + if (test_bit(PHY_INTERFACE_MODE_10GBASER, modes) || > + test_bit(PHY_INTERFACE_MODE_USXGMII, modes)) { > + if (test_bit(PHY_INTERFACE_MODE_5GBASER, supported)) > + __set_bit(PHY_INTERFACE_MODE_5GBASER, modes); > + if (test_bit(PHY_INTERFACE_MODE_10GBASER, supported)) > + __set_bit(PHY_INTERFACE_MODE_10GBASER, modes); > + if (test_bit(PHY_INTERFACE_MODE_USXGMII, supported)) > + __set_bit(PHY_INTERFACE_MODE_USXGMII, modes); How about DECLARE_PHY_INTERFACE_MASK(upper_modes); __set_bit(upper_modes, PHY_INTERFACE_MODE_5GBASER); __set_bit(upper_modes, PHY_INTERFACE_MODE_10GBASER); __set_bit(upper_modes, PHY_INTERFACE_MODE_USXGMII); phy_interface_and(upper_modes, supported, upper_modes); phy_interface_or(modes, modes, upper_modes); same linecount but less duplication > + lower = true; > + } > + > + if (lower || (test_bit(PHY_INTERFACE_MODE_SGMII, modes) || > + test_bit(PHY_INTERFACE_MODE_1000BASEX, modes) || > + test_bit(PHY_INTERFACE_MODE_2500BASEX, modes))) { > + if (test_bit(PHY_INTERFACE_MODE_SGMII, supported)) > + __set_bit(PHY_INTERFACE_MODE_SGMII, modes); > + if (test_bit(PHY_INTERFACE_MODE_1000BASEX, supported)) > + __set_bit(PHY_INTERFACE_MODE_1000BASEX, modes); > + if (test_bit(PHY_INTERFACE_MODE_2500BASEX, supported)) > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, modes); ditto --Sean > + } > + } > + > + phy_interface_and(supported, supported, modes); > +} > + > static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode) > { > struct fwnode_handle *dn; > @@ -1156,6 +1217,8 @@ struct phylink *phylink_create(struct phylink_config *config, > __set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state); > timer_setup(&pl->link_poll, phylink_fixed_poll, 0); > > + phylink_update_phy_modes(pl, fwnode); > + > bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS); > linkmode_copy(pl->link_config.advertising, pl->supported); > phylink_validate(pl, pl->supported, &pl->link_config); > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 1e57cdd95da3..83ae15ab1676 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -169,6 +169,12 @@ static inline bool phy_interface_empty(const unsigned long *intf) > return bitmap_empty(intf, PHY_INTERFACE_MODE_MAX); > } > > +static inline void phy_interface_copy(unsigned long *dst, > + const unsigned long *src) > +{ > + bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX); > +} > + > static inline void phy_interface_and(unsigned long *dst, const unsigned long *a, > const unsigned long *b) > { >
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index f7156b6868e7..6d7c216a5dea 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -563,6 +563,67 @@ static int phylink_parse_fixedlink(struct phylink *pl, return 0; } +static void phylink_update_phy_modes(struct phylink *pl, + struct fwnode_handle *fwnode) +{ + unsigned long *supported = pl->config->supported_interfaces; + DECLARE_PHY_INTERFACE_MASK(modes); + + if (fwnode_get_phy_modes(fwnode, modes) < 0) + return; + + if (phy_interface_empty(modes)) + return; + + /* If supported is empty, just copy modes defined in fwnode. */ + if (phy_interface_empty(supported)) + return phy_interface_copy(supported, modes); + + /* We want the intersection of given supported modes with those defined + * in DT. + * + * Some older device-trees mention only one of `sgmii`, `1000base-x` or + * `2500base-x`, while supporting all three. Other mention `10gbase-r` + * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`, + * `2500base-x` and `5gbase-r`. + * For backwards compatibility with these older DTs, make it so that if + * one of these modes is mentioned in DT and MAC supports more of them, + * keep all that are supported according to the logic above. + * + * Nonetheless it is possible that a device may support only one mode, + * for example 1000base-x, due to strapping pins or some other reasons. + * If a specific device supports only the mode mentioned in DT, the + * exception should be made here with of_machine_is_compatible(). + */ + if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) { + bool lower = false; + + if (test_bit(PHY_INTERFACE_MODE_10GBASER, modes) || + test_bit(PHY_INTERFACE_MODE_USXGMII, modes)) { + if (test_bit(PHY_INTERFACE_MODE_5GBASER, supported)) + __set_bit(PHY_INTERFACE_MODE_5GBASER, modes); + if (test_bit(PHY_INTERFACE_MODE_10GBASER, supported)) + __set_bit(PHY_INTERFACE_MODE_10GBASER, modes); + if (test_bit(PHY_INTERFACE_MODE_USXGMII, supported)) + __set_bit(PHY_INTERFACE_MODE_USXGMII, modes); + lower = true; + } + + if (lower || (test_bit(PHY_INTERFACE_MODE_SGMII, modes) || + test_bit(PHY_INTERFACE_MODE_1000BASEX, modes) || + test_bit(PHY_INTERFACE_MODE_2500BASEX, modes))) { + if (test_bit(PHY_INTERFACE_MODE_SGMII, supported)) + __set_bit(PHY_INTERFACE_MODE_SGMII, modes); + if (test_bit(PHY_INTERFACE_MODE_1000BASEX, supported)) + __set_bit(PHY_INTERFACE_MODE_1000BASEX, modes); + if (test_bit(PHY_INTERFACE_MODE_2500BASEX, supported)) + __set_bit(PHY_INTERFACE_MODE_2500BASEX, modes); + } + } + + phy_interface_and(supported, supported, modes); +} + static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode) { struct fwnode_handle *dn; @@ -1156,6 +1217,8 @@ struct phylink *phylink_create(struct phylink_config *config, __set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state); timer_setup(&pl->link_poll, phylink_fixed_poll, 0); + phylink_update_phy_modes(pl, fwnode); + bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS); linkmode_copy(pl->link_config.advertising, pl->supported); phylink_validate(pl, pl->supported, &pl->link_config); diff --git a/include/linux/phy.h b/include/linux/phy.h index 1e57cdd95da3..83ae15ab1676 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -169,6 +169,12 @@ static inline bool phy_interface_empty(const unsigned long *intf) return bitmap_empty(intf, PHY_INTERFACE_MODE_MAX); } +static inline void phy_interface_copy(unsigned long *dst, + const unsigned long *src) +{ + bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX); +} + static inline void phy_interface_and(unsigned long *dst, const unsigned long *a, const unsigned long *b) {
Now that the 'phy-mode' property can be a string array containing more PHY modes (all that are supported by the board), update the bitmap of interfaces supported by the MAC with this property. Normally this would be a simple intersection (of interfaces supported by the current implementation of the driver and interfaces supported by the board), but we need to keep being backwards compatible with older DTs, which may only define one mode, since, as Russell King says, conventionally phy-mode has meant "this is the mode we want to operate the PHY interface in" which was fine when PHYs didn't change their mode depending on the media speed An example is DT defining phy-mode = "sgmii"; but the board supporting also 1000base-x and 2500base-x. Add the following logic to keep this backwards compatiblity: - if more PHY modes are defined, do a simple intersection - if one PHY mode is defined: - if it is sgmii, 1000base-x or 2500base-x, add all three and then do the intersection - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r, 2500base-x, 1000base-x and sgmii, and then do the intersection This is simple enough and should work for all boards. Nonetheless it is possible (although extremely unlikely, in my opinion) that a board will be found that (for example) defines phy-mode = "sgmii"; and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the board DOESN'T support 2500base-x, because of electrical reasons (since the frequency is 2.5x of sgmii). Our code will in this case incorrectly infer also support for 2500base-x. To avoid this, the board maintainer should either change DTS to phy-mode = "sgmii", "1000base-x"; and update device tree on all boards, or, if that is impossible, add a fix into the function we are introducing in this commit. Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 6 ++++ 2 files changed, 69 insertions(+)