| Message ID | E1tJ8NR-006L5P-E3@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: net: add negotiation of in-band capabilities (remainder) | expand |
On Thu, Dec 05, 2024 at 09:42:29AM +0000, Russell King (Oracle) wrote: > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > index 4f63abe638c4..7de804535229 100644 > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > } > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > + phy_interface_t interface) > +{ > + switch (interface) { > + case PHY_INTERFACE_MODE_1000BASEX: > + case PHY_INTERFACE_MODE_2500BASEX: > + case PHY_INTERFACE_MODE_SGMII: > + case PHY_INTERFACE_MODE_QSGMII: QSGMII is not supported by this PCS. Apart from that looks good to me. Reviewed-by: Daniel Golle <daniel@makrotopia.org>
On Sun, Dec 08, 2024 at 03:01:38AM +0000, Daniel Golle wrote: > On Thu, Dec 05, 2024 at 09:42:29AM +0000, Russell King (Oracle) wrote: > > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > > index 4f63abe638c4..7de804535229 100644 > > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > > } > > > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > > + phy_interface_t interface) > > +{ > > + switch (interface) { > > + case PHY_INTERFACE_MODE_1000BASEX: > > + case PHY_INTERFACE_MODE_2500BASEX: > > + case PHY_INTERFACE_MODE_SGMII: > > + case PHY_INTERFACE_MODE_QSGMII: > > QSGMII is not supported by this PCS. Well... - lynx_pcs_get_state(), lynx_pcs_config(), and lynx_pcs_link_up() include QSGMII in their case statements. - lynx_pcs_config_giga() refers to QSGMII in a comment, grouping it with SGMII configuration. I think if the hardware doesn't support QSGMII, these references to it should be removed? I also think that adding a .pcs_validate() method would be a good idea to reject interface modes that the PCS does not support. > Apart from that looks good to me. > > Reviewed-by: Daniel Golle <daniel@makrotopia.org> Thanks.
On 12/5/24 10:42 AM, Russell King (Oracle) wrote: > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Reviewed-by: Daniel Golle <daniel@makrotopia.org> > --- > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > index 4f63abe638c4..7de804535229 100644 > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > } > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > + phy_interface_t interface) > +{ > + switch (interface) { > + case PHY_INTERFACE_MODE_1000BASEX: > + case PHY_INTERFACE_MODE_2500BASEX: Isn't this the place now where to report to phylink, that this PCS does not support in-band at 2500base-x? Best regards, Eric > + case PHY_INTERFACE_MODE_SGMII: > + case PHY_INTERFACE_MODE_QSGMII: > + return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE; > + > + default: > + return 0; > + } > +} > + > static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs, > struct phylink_link_state *state) > { > @@ -241,6 +256,7 @@ static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs) > } > > static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = { > + .pcs_inband_caps = mtk_pcs_lynxi_inband_caps, > .pcs_get_state = mtk_pcs_lynxi_get_state, > .pcs_config = mtk_pcs_lynxi_config, > .pcs_an_restart = mtk_pcs_lynxi_restart_an,
On Tue, Dec 17, 2024 at 08:49:58AM +0100, Eric Woudstra wrote: > On 12/5/24 10:42 AM, Russell King (Oracle) wrote: > > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Daniel Golle <daniel@makrotopia.org> > > --- > > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > > index 4f63abe638c4..7de804535229 100644 > > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > > } > > > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > > + phy_interface_t interface) > > +{ > > + switch (interface) { > > + case PHY_INTERFACE_MODE_1000BASEX: > > + case PHY_INTERFACE_MODE_2500BASEX: > > Isn't this the place now where to report to phylink, that this PCS does > not support in-band at 2500base-x? No - look at the arguments to this function. What arguments would this function make a decision whether in-band is supported in any interface mode? The correct place is the .pcs_inband_caps(), which from reading the code, I understood that in-band can be used at 2500base-X with this PCS. See https://patch.msgid.link/E1tJ8NR-006L5P-E3@rmk-PC.armlinux.org.uk which was merged at the beginning of December, and if you are correct, the patch was wrong.
On Sun, Dec 08, 2024 at 09:13:11AM +0000, Russell King (Oracle) wrote: > On Sun, Dec 08, 2024 at 03:01:38AM +0000, Daniel Golle wrote: > > On Thu, Dec 05, 2024 at 09:42:29AM +0000, Russell King (Oracle) wrote: > > > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > > > index 4f63abe638c4..7de804535229 100644 > > > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > > > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > > > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > > > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > > > } > > > > > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > > > + phy_interface_t interface) > > > +{ > > > + switch (interface) { > > > + case PHY_INTERFACE_MODE_1000BASEX: > > > + case PHY_INTERFACE_MODE_2500BASEX: > > > + case PHY_INTERFACE_MODE_SGMII: > > > + case PHY_INTERFACE_MODE_QSGMII: > > > > QSGMII is not supported by this PCS. > > Well... > - lynx_pcs_get_state(), lynx_pcs_config(), and lynx_pcs_link_up() > include QSGMII in their case statements. > - lynx_pcs_config_giga() refers to QSGMII in a comment, grouping it > with SGMII configuration. These functions are in pcs-lynx.c and not in pcs-mtk-lynxi.c. There is no reference to QSGMII anywhere in pcs-mtk-lynxi.c, what made you assume it would be supported? > > I think if the hardware doesn't support QSGMII, these references to it > should be removed? > > I also think that adding a .pcs_validate() method would be a good idea > to reject interface modes that the PCS does not support. I can add the pcs_validate function and send the patch doing that to be included in net-next.
On Thu, Jan 02, 2025 at 08:59:13AM +0000, Russell King (Oracle) wrote: > On Tue, Dec 17, 2024 at 08:49:58AM +0100, Eric Woudstra wrote: > > On 12/5/24 10:42 AM, Russell King (Oracle) wrote: > > > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > Reviewed-by: Daniel Golle <daniel@makrotopia.org> > > > --- > > > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > > > index 4f63abe638c4..7de804535229 100644 > > > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > > > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > > > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > > > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > > > } > > > > > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > > > + phy_interface_t interface) > > > +{ > > > + switch (interface) { > > > + case PHY_INTERFACE_MODE_1000BASEX: > > > + case PHY_INTERFACE_MODE_2500BASEX: > > > > Isn't this the place now where to report to phylink, that this PCS does > > not support in-band at 2500base-x? > > No - look at the arguments to this function. What arguments would this > function make a decision whether in-band is supported in any interface > mode? > > The correct place is the .pcs_inband_caps(), which from reading the > code, I understood that in-band can be used at 2500base-X with this > PCS. See > https://patch.msgid.link/E1tJ8NR-006L5P-E3@rmk-PC.armlinux.org.uk > which was merged at the beginning of December, and if you are correct, > the patch was wrong. Yes, that patch was wrong. Neither is QSGMII supported at all by the LynxI hardware, nor can in-band-status be used in 2500Base-X mode. I will send a patch to fix that.
On Thu, Jan 02, 2025 at 12:26:52PM +0000, Daniel Golle wrote: > On Sun, Dec 08, 2024 at 09:13:11AM +0000, Russell King (Oracle) wrote: > > On Sun, Dec 08, 2024 at 03:01:38AM +0000, Daniel Golle wrote: > > > On Thu, Dec 05, 2024 at 09:42:29AM +0000, Russell King (Oracle) wrote: > > > > Report the PCS in-band capabilities to phylink for the LynxI PCS. > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > --- > > > > drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > > > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c > > > > index 4f63abe638c4..7de804535229 100644 > > > > --- a/drivers/net/pcs/pcs-mtk-lynxi.c > > > > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c > > > > @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) > > > > return container_of(pcs, struct mtk_pcs_lynxi, pcs); > > > > } > > > > > > > > +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, > > > > + phy_interface_t interface) > > > > +{ > > > > + switch (interface) { > > > > + case PHY_INTERFACE_MODE_1000BASEX: > > > > + case PHY_INTERFACE_MODE_2500BASEX: > > > > + case PHY_INTERFACE_MODE_SGMII: > > > > + case PHY_INTERFACE_MODE_QSGMII: > > > > > > QSGMII is not supported by this PCS. > > > > Well... > > - lynx_pcs_get_state(), lynx_pcs_config(), and lynx_pcs_link_up() > > include QSGMII in their case statements. > > - lynx_pcs_config_giga() refers to QSGMII in a comment, grouping it > > with SGMII configuration. > > These functions are in pcs-lynx.c and not in pcs-mtk-lynxi.c. > There is no reference to QSGMII anywhere in pcs-mtk-lynxi.c, what made > you assume it would be supported? > > > > > I think if the hardware doesn't support QSGMII, these references to it > > should be removed? > > > > I also think that adding a .pcs_validate() method would be a good idea > > to reject interface modes that the PCS does not support. > > I can add the pcs_validate function and send the patch doing that to be > included in net-next. With the addition of the supported_interfaces bitmap, there's no point if all the validation function is doing is checking the interfaces. The supported_interfaces bitmap takes over that in core phylink code (and if a MAC supplies a PCS for an interface that is in the MAC's supported_interfaces bitmap but does not have the interface bit set in the PCS's supported_interfaces, phylink will produce a kernel message at error severity before .pcs_validate is called. At a point in the near future, I will be making the PCS supported_interfaces mandatory - but in the mean time this patch set makes it optional.
diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c index 4f63abe638c4..7de804535229 100644 --- a/drivers/net/pcs/pcs-mtk-lynxi.c +++ b/drivers/net/pcs/pcs-mtk-lynxi.c @@ -88,6 +88,21 @@ static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs) return container_of(pcs, struct mtk_pcs_lynxi, pcs); } +static unsigned int mtk_pcs_lynxi_inband_caps(struct phylink_pcs *pcs, + phy_interface_t interface) +{ + switch (interface) { + case PHY_INTERFACE_MODE_1000BASEX: + case PHY_INTERFACE_MODE_2500BASEX: + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_QSGMII: + return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE; + + default: + return 0; + } +} + static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state) { @@ -241,6 +256,7 @@ static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs) } static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = { + .pcs_inband_caps = mtk_pcs_lynxi_inband_caps, .pcs_get_state = mtk_pcs_lynxi_get_state, .pcs_config = mtk_pcs_lynxi_config, .pcs_an_restart = mtk_pcs_lynxi_restart_an,
Report the PCS in-band capabilities to phylink for the LynxI PCS. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-mtk-lynxi.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)