| Message ID | 20210504222915.17206-19-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multiple improvement to qca8k stability | expand |
On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > Define get_phy_flags to pass switch_Revision needed to tweak the > internal PHY with debug values based on the revision. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index b4cd891ad35d..237e09bb1425 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > return ret; > } > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > +{ > + struct qca8k_priv *priv = ds->priv; > + > + pr_info("revision from phy %d", priv->switch_revision); Log spam. > + /* Communicate to the phy internal driver the switch revision. > + * Based on the switch revision different values needs to be > + * set to the dbg and mmd reg on the phy. > + * The first 2 bit are used to communicate the switch revision > + * to the phy driver. > + */ > + if (port > 0 && port < 6) > + return priv->switch_revision; > + > + return 0; > +} > + > static enum dsa_tag_protocol > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > enum dsa_tag_protocol mp) > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > .phylink_mac_config = qca8k_phylink_mac_config, > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > + .get_phy_flags = qca8k_get_phy_flags, > }; > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > -- > 2.30.2 > Florian, I think at one point you said that a correct user of phydev->dev_flags should first check the PHY revision and not apply dev_flags in blind, since they are namespaced to each PHY driver? It sounds a bit circular to pass the PHY revision to the PHY through phydev->dev_flags, either that or I'm missing some piece.
On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > +{ > + struct qca8k_priv *priv = ds->priv; > + > + pr_info("revision from phy %d", priv->switch_revision); Should this be a "pr_info" ? > + > + /* Communicate to the phy internal driver the switch revision. > + * Based on the switch revision different values needs to be > + * set to the dbg and mmd reg on the phy. > + * The first 2 bit are used to communicate the switch revision > + * to the phy driver. > + */ > + if (port > 0 && port < 6) > + return priv->switch_revision; We had some discussion back in February about the PHY flags argument ("Rework of phydev->dev_flags") as there is a need to generically identify whether a PHY is on a SFP module or not. This discussion hasn't progressed to any changes (yet) but some of the points remain valid: if we do go down the route of needing to have generic PHY flags in dev_flags, then we need vendor specific stuff to avoid those bits. So, rather than introduce a new case of passing some undefined data through the flags argument, can we come up with some sort of proposal for this. It may also be a good idea if we document it. Maybe something like "low 16 bits are free for driver use, upper 16 bits are reserved for generic use"? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote: > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > > Define get_phy_flags to pass switch_Revision needed to tweak the > > internal PHY with debug values based on the revision. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > index b4cd891ad35d..237e09bb1425 100644 > > --- a/drivers/net/dsa/qca8k.c > > +++ b/drivers/net/dsa/qca8k.c > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > > return ret; > > } > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > > +{ > > + struct qca8k_priv *priv = ds->priv; > > + > > + pr_info("revision from phy %d", priv->switch_revision); > > Log spam. > > > + /* Communicate to the phy internal driver the switch revision. > > + * Based on the switch revision different values needs to be > > + * set to the dbg and mmd reg on the phy. > > + * The first 2 bit are used to communicate the switch revision > > + * to the phy driver. > > + */ > > + if (port > 0 && port < 6) > > + return priv->switch_revision; > > + > > + return 0; > > +} > > + > > static enum dsa_tag_protocol > > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > > enum dsa_tag_protocol mp) > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > .phylink_mac_config = qca8k_phylink_mac_config, > > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > > + .get_phy_flags = qca8k_get_phy_flags, > > }; > > > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > > -- > > 2.30.2 > > > > Florian, I think at one point you said that a correct user of > phydev->dev_flags should first check the PHY revision and not apply > dev_flags in blind, since they are namespaced to each PHY driver? > It sounds a bit circular to pass the PHY revision to the PHY through > phydev->dev_flags, either that or I'm missing some piece. Just to make sure. This is the SWITCH revision not the PHY revision. It was pointed out in old version that I should get this value from the PHY regs but they are different values. This is why the dsa driver needs to use the dev_flags to pass the SWITCH revision to the phy driver. Am I implementing this in the wrong way and I should declare something to pass this value in a more standard way? (anyway i'm pushing v4 so i don't know if we should continue that there)
On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote: > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote: > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > > > Define get_phy_flags to pass switch_Revision needed to tweak the > > > internal PHY with debug values based on the revision. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > --- > > > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > > index b4cd891ad35d..237e09bb1425 100644 > > > --- a/drivers/net/dsa/qca8k.c > > > +++ b/drivers/net/dsa/qca8k.c > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > > > return ret; > > > } > > > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > > > +{ > > > + struct qca8k_priv *priv = ds->priv; > > > + > > > + pr_info("revision from phy %d", priv->switch_revision); > > > > Log spam. > > > > > + /* Communicate to the phy internal driver the switch revision. > > > + * Based on the switch revision different values needs to be > > > + * set to the dbg and mmd reg on the phy. > > > + * The first 2 bit are used to communicate the switch revision > > > + * to the phy driver. > > > + */ > > > + if (port > 0 && port < 6) > > > + return priv->switch_revision; > > > + > > > + return 0; > > > +} > > > + > > > static enum dsa_tag_protocol > > > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > > > enum dsa_tag_protocol mp) > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > > .phylink_mac_config = qca8k_phylink_mac_config, > > > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > > > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > > > + .get_phy_flags = qca8k_get_phy_flags, > > > }; > > > > > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > > > -- > > > 2.30.2 > > > > > > > Florian, I think at one point you said that a correct user of > > phydev->dev_flags should first check the PHY revision and not apply > > dev_flags in blind, since they are namespaced to each PHY driver? > > It sounds a bit circular to pass the PHY revision to the PHY through > > phydev->dev_flags, either that or I'm missing some piece. > > Just to make sure. This is the SWITCH revision not the PHY revision. It > was pointed out in old version that I should get this value from the PHY > regs but they are different values. This is why the dsa driver needs to > use the dev_flags to pass the SWITCH revision to the phy driver. Am I > implementing this in the wrong way and I should declare something to > pass this value in a more standard way? (anyway i'm pushing v4 so i > don't know if we should continue that there) Vladimir is confused - it is not PHY revision at all, but the PHY identifiers. What was actually suggested was checking the PHY identifiers before passing PHY-driver specific flags, so that we didn't end up setting driver private flags that are intending for one driver, but end up actually binding a different driver, and mis-interpreting the flags. This is one of the problems of the current scheme: it's just a meaningless opaque u32 variable with no defined structure to it that the various PHY drivers themselves use in whatever way they see fit. That is only fine to use _if_ you know for certain which driver is going to bind ahead of time. As I mentioned in direct reply to your patch, there was discussions about this back in February, but they seem to have stalled. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote: > On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote: > > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote: > > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > > > > Define get_phy_flags to pass switch_Revision needed to tweak the > > > > internal PHY with debug values based on the revision. > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > --- > > > > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > > > > 1 file changed, 19 insertions(+) > > > > > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > > > index b4cd891ad35d..237e09bb1425 100644 > > > > --- a/drivers/net/dsa/qca8k.c > > > > +++ b/drivers/net/dsa/qca8k.c > > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > > > > return ret; > > > > } > > > > > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > > > > +{ > > > > + struct qca8k_priv *priv = ds->priv; > > > > + > > > > + pr_info("revision from phy %d", priv->switch_revision); > > > > > > Log spam. > > > > > > > + /* Communicate to the phy internal driver the switch revision. > > > > + * Based on the switch revision different values needs to be > > > > + * set to the dbg and mmd reg on the phy. > > > > + * The first 2 bit are used to communicate the switch revision > > > > + * to the phy driver. > > > > + */ > > > > + if (port > 0 && port < 6) > > > > + return priv->switch_revision; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static enum dsa_tag_protocol > > > > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > > > > enum dsa_tag_protocol mp) > > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > > > .phylink_mac_config = qca8k_phylink_mac_config, > > > > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > > > > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > > > > + .get_phy_flags = qca8k_get_phy_flags, > > > > }; > > > > > > > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > > > > -- > > > > 2.30.2 > > > > > > > > > > Florian, I think at one point you said that a correct user of > > > phydev->dev_flags should first check the PHY revision and not apply > > > dev_flags in blind, since they are namespaced to each PHY driver? > > > It sounds a bit circular to pass the PHY revision to the PHY through > > > phydev->dev_flags, either that or I'm missing some piece. > > > > Just to make sure. This is the SWITCH revision not the PHY revision. It > > was pointed out in old version that I should get this value from the PHY > > regs but they are different values. This is why the dsa driver needs to > > use the dev_flags to pass the SWITCH revision to the phy driver. Am I > > implementing this in the wrong way and I should declare something to > > pass this value in a more standard way? (anyway i'm pushing v4 so i > > don't know if we should continue that there) > > Vladimir is confused - it is not PHY revision at all, but the PHY > identifiers. > > What was actually suggested was checking the PHY identifiers before > passing PHY-driver specific flags, so that we didn't end up setting > driver private flags that are intending for one driver, but end up > actually binding a different driver, and mis-interpreting the flags. > > This is one of the problems of the current scheme: it's just a > meaningless opaque u32 variable with no defined structure to it that > the various PHY drivers themselves use in whatever way they see fit. > That is only fine to use _if_ you know for certain which driver is > going to bind ahead of time. > The problem here was find a way to pass data from the dsa driver to the phy driver. In this specific case the phy driver is an internal phy present in the switch so it won't appear on anything else. Aside from this I agree that it seems wrong that random values are used without some type of rules or definition but I think that try to address this problem is too much for this already large series. In theory this should be safe to use as this driver would only be used by qca8k dsa driver. > As I mentioned in direct reply to your patch, there was discussions > about this back in February, but they seem to have stalled. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> The problem here was find a way to pass data from the dsa driver to the > phy driver. In this specific case the phy driver is an internal phy > present in the switch so it won't appear on anything else. For internal PHYs, you are safe. But please keep in mind any RGMII ports which the switch might have. Somebody could attach an external PHY on such a port. So you should not return any flags for such ports. Andrew
On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote: > On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote: > > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote: > > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > > > > Define get_phy_flags to pass switch_Revision needed to tweak the > > > > internal PHY with debug values based on the revision. > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > --- > > > > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > > > > 1 file changed, 19 insertions(+) > > > > > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > > > index b4cd891ad35d..237e09bb1425 100644 > > > > --- a/drivers/net/dsa/qca8k.c > > > > +++ b/drivers/net/dsa/qca8k.c > > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > > > > return ret; > > > > } > > > > > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > > > > +{ > > > > + struct qca8k_priv *priv = ds->priv; > > > > + > > > > + pr_info("revision from phy %d", priv->switch_revision); > > > > > > Log spam. > > > > > > > + /* Communicate to the phy internal driver the switch revision. > > > > + * Based on the switch revision different values needs to be > > > > + * set to the dbg and mmd reg on the phy. > > > > + * The first 2 bit are used to communicate the switch revision > > > > + * to the phy driver. > > > > + */ > > > > + if (port > 0 && port < 6) > > > > + return priv->switch_revision; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static enum dsa_tag_protocol > > > > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > > > > enum dsa_tag_protocol mp) > > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > > > .phylink_mac_config = qca8k_phylink_mac_config, > > > > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > > > > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > > > > + .get_phy_flags = qca8k_get_phy_flags, > > > > }; > > > > > > > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > > > > -- > > > > 2.30.2 > > > > > > > > > > Florian, I think at one point you said that a correct user of > > > phydev->dev_flags should first check the PHY revision and not apply > > > dev_flags in blind, since they are namespaced to each PHY driver? > > > It sounds a bit circular to pass the PHY revision to the PHY through > > > phydev->dev_flags, either that or I'm missing some piece. > > > > Just to make sure. This is the SWITCH revision not the PHY revision. It > > was pointed out in old version that I should get this value from the PHY > > regs but they are different values. This is why the dsa driver needs to > > use the dev_flags to pass the SWITCH revision to the phy driver. Am I > > implementing this in the wrong way and I should declare something to > > pass this value in a more standard way? (anyway i'm pushing v4 so i > > don't know if we should continue that there) > > Vladimir is confused - it is not PHY revision at all, but the PHY > identifiers. > > What was actually suggested was checking the PHY identifiers before > passing PHY-driver specific flags, so that we didn't end up setting > driver private flags that are intending for one driver, but end up > actually binding a different driver, and mis-interpreting the flags. > > This is one of the problems of the current scheme: it's just a > meaningless opaque u32 variable with no defined structure to it that > the various PHY drivers themselves use in whatever way they see fit. > That is only fine to use _if_ you know for certain which driver is > going to bind ahead of time. > > As I mentioned in direct reply to your patch, there was discussions > about this back in February, but they seem to have stalled. Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID and revision concepts in one big fuzzy notion. I remembered Heiner's suggestion to do something similar to mv88e6xxx_mdio_read from here: https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/ (where the problem is that some internal PHYs are lacking a device identifier) and thought that the problem here is the same. Nonetheless, now it is clear to me that with care (don't set dev_flags except for internal PHYs which are statically known), it is possible for the PHY driver to have a larger identifier (PHY ID concatenated with switch revision passed through dev_flags) based on which it can configure the hardware. Sorry.
On Sat, May 08, 2021 at 09:26:20PM +0300, Vladimir Oltean wrote: > On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote: > > On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote: > > > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote: > > > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote: > > > > > Define get_phy_flags to pass switch_Revision needed to tweak the > > > > > internal PHY with debug values based on the revision. > > > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > --- > > > > > drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ > > > > > 1 file changed, 19 insertions(+) > > > > > > > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > > > > index b4cd891ad35d..237e09bb1425 100644 > > > > > --- a/drivers/net/dsa/qca8k.c > > > > > +++ b/drivers/net/dsa/qca8k.c > > > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, > > > > > return ret; > > > > > } > > > > > > > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) > > > > > +{ > > > > > + struct qca8k_priv *priv = ds->priv; > > > > > + > > > > > + pr_info("revision from phy %d", priv->switch_revision); > > > > > > > > Log spam. > > > > > > > > > + /* Communicate to the phy internal driver the switch revision. > > > > > + * Based on the switch revision different values needs to be > > > > > + * set to the dbg and mmd reg on the phy. > > > > > + * The first 2 bit are used to communicate the switch revision > > > > > + * to the phy driver. > > > > > + */ > > > > > + if (port > 0 && port < 6) > > > > > + return priv->switch_revision; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static enum dsa_tag_protocol > > > > > qca8k_get_tag_protocol(struct dsa_switch *ds, int port, > > > > > enum dsa_tag_protocol mp) > > > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > > > > .phylink_mac_config = qca8k_phylink_mac_config, > > > > > .phylink_mac_link_down = qca8k_phylink_mac_link_down, > > > > > .phylink_mac_link_up = qca8k_phylink_mac_link_up, > > > > > + .get_phy_flags = qca8k_get_phy_flags, > > > > > }; > > > > > > > > > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > > > > > -- > > > > > 2.30.2 > > > > > > > > > > > > > Florian, I think at one point you said that a correct user of > > > > phydev->dev_flags should first check the PHY revision and not apply > > > > dev_flags in blind, since they are namespaced to each PHY driver? > > > > It sounds a bit circular to pass the PHY revision to the PHY through > > > > phydev->dev_flags, either that or I'm missing some piece. > > > > > > Just to make sure. This is the SWITCH revision not the PHY revision. It > > > was pointed out in old version that I should get this value from the PHY > > > regs but they are different values. This is why the dsa driver needs to > > > use the dev_flags to pass the SWITCH revision to the phy driver. Am I > > > implementing this in the wrong way and I should declare something to > > > pass this value in a more standard way? (anyway i'm pushing v4 so i > > > don't know if we should continue that there) > > > > Vladimir is confused - it is not PHY revision at all, but the PHY > > identifiers. > > > > What was actually suggested was checking the PHY identifiers before > > passing PHY-driver specific flags, so that we didn't end up setting > > driver private flags that are intending for one driver, but end up > > actually binding a different driver, and mis-interpreting the flags. > > > > This is one of the problems of the current scheme: it's just a > > meaningless opaque u32 variable with no defined structure to it that > > the various PHY drivers themselves use in whatever way they see fit. > > That is only fine to use _if_ you know for certain which driver is > > going to bind ahead of time. > > > > As I mentioned in direct reply to your patch, there was discussions > > about this back in February, but they seem to have stalled. > > Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID > and revision concepts in one big fuzzy notion. I remembered Heiner's > suggestion to do something similar to mv88e6xxx_mdio_read from here: > https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/ > (where the problem is that some internal PHYs are lacking a device > identifier) and thought that the problem here is the same. > > Nonetheless, now it is clear to me that with care (don't set dev_flags > except for internal PHYs which are statically known), it is possible for > the PHY driver to have a larger identifier (PHY ID concatenated with > switch revision passed through dev_flags) based on which it can > configure the hardware. We do have the problem with Marvell DSA vs Marvell PHY setup in that the Marvell DSA driver assumes that all integrated PHYs that do not have an ID are all the same. They are most definitely not, and this shows itself up when we register the hwmon stuff inappropriately, or access the wrong registers to report hwmon values. We really need to solve this problem properly rather than bodging around it with driver specific usage of dev_flags. We already have the ability for drivers to have custom match functions (match_phy_device) that do not depend on the probed ID - maybe we should have an additional u32 member in struct phy_device for the switch_id that the PHY is a part of that PHY drivers can check in their match_phy_device method if necessary, or otherwise use that to parse the switch revision from. Or something like that? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> We do have the problem with Marvell DSA vs Marvell PHY setup in that > the Marvell DSA driver assumes that all integrated PHYs that do not > have an ID are all the same. They are most definitely not, and this > shows itself up when we register the hwmon stuff inappropriately, or > access the wrong registers to report hwmon values. Hi Russell This was to some degree fixed recently. Rather than always use the 6390 ID for everything, the family ID is now used. This should avoid the issue with the SERDES being incorrectly considered a PHY, since that particular family ID is not listed in the PHY driver. Andrew
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index b4cd891ad35d..237e09bb1425 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port, return ret; } +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port) +{ + struct qca8k_priv *priv = ds->priv; + + pr_info("revision from phy %d", priv->switch_revision); + + /* Communicate to the phy internal driver the switch revision. + * Based on the switch revision different values needs to be + * set to the dbg and mmd reg on the phy. + * The first 2 bit are used to communicate the switch revision + * to the phy driver. + */ + if (port > 0 && port < 6) + return priv->switch_revision; + + return 0; +} + static enum dsa_tag_protocol qca8k_get_tag_protocol(struct dsa_switch *ds, int port, enum dsa_tag_protocol mp) @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { .phylink_mac_config = qca8k_phylink_mac_config, .phylink_mac_link_down = qca8k_phylink_mac_link_down, .phylink_mac_link_up = qca8k_phylink_mac_link_up, + .get_phy_flags = qca8k_get_phy_flags, }; static int qca8k_read_switch_id(struct qca8k_priv *priv)
Define get_phy_flags to pass switch_Revision needed to tweak the internal PHY with debug values based on the revision. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)