diff mbox series

[net-next,v5,1/9] net: phylink: provide mac_get_pcs_neg_mode() function

Message ID 20240215030500.3067426-2-yong.liang.choong@linux.intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Enable SGMII and 2500BASEX interface mode switching for Intel platforms | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1128 this patch: 1128
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1012 this patch: 1012
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1156 this patch: 1156
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 37 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-16--06-00 (tests: 1443)

Commit Message

Choong Yong Liang Feb. 15, 2024, 3:04 a.m. UTC
Phylink invokes the 'mac_get_pcs_neg_mode' function during interface mode
switching and initial startup.

This function is optional; if 'phylink_pcs_neg_mode' fails to accurately
reflect the current PCS negotiation mode, the MAC driver can determine the
mode based on the interface mode, current link negotiation mode, and
advertising link mode.

For instance, if the interface switches from 2500baseX to SGMII mode,
and the current link mode is MLO_AN_PHY, calling 'phylink_pcs_neg_mode'
would yield PHYLINK_PCS_NEG_OUTBAND. Since the MAC and PCS driver require
PHYLINK_PCS_NEG_INBAND_ENABLED, the 'mac_get_pcs_neg_mode' function
will calculate the mode based on the interface, current link negotiation
mode, and advertising link mode, returning PHYLINK_PCS_NEG_OUTBAND to
enable the PCS to configure the correct settings.

Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 drivers/net/phy/phylink.c | 14 +++++++++++---
 include/linux/phylink.h   |  5 +++++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Russell King (Oracle) Feb. 15, 2024, 4:26 p.m. UTC | #1
On Thu, Feb 15, 2024 at 11:04:51AM +0800, Choong Yong Liang wrote:
> Phylink invokes the 'mac_get_pcs_neg_mode' function during interface mode
> switching and initial startup.
> 
> This function is optional; if 'phylink_pcs_neg_mode' fails to accurately
> reflect the current PCS negotiation mode, the MAC driver can determine the
> mode based on the interface mode, current link negotiation mode, and
> advertising link mode.
> 
> For instance, if the interface switches from 2500baseX to SGMII mode,
> and the current link mode is MLO_AN_PHY, calling 'phylink_pcs_neg_mode'
> would yield PHYLINK_PCS_NEG_OUTBAND. Since the MAC and PCS driver require
> PHYLINK_PCS_NEG_INBAND_ENABLED, the 'mac_get_pcs_neg_mode' function
> will calculate the mode based on the interface, current link negotiation
> mode, and advertising link mode, returning PHYLINK_PCS_NEG_OUTBAND to
> enable the PCS to configure the correct settings.

This paragraph doesn't make sense - at least to me. It first talks about
requiring PHYLINK_PCS_NEG_INBAND_ENABLED when in SGMII mode. On this:

1) are you sure that the hardware can't be programmed for the SGMII
symbol repititions? 

2) what happens if you're paired with a PHY (e.g. on a SFP module)
which uses SGMII but has no capability of providing the inband data?
(They do exist.) If your hardware truly does require inband data, it
is going to be fundamentally inoperative with these modules.

Next, you then talk about returning PHYLINK_PCS_NEG_OUTBAND for the
"correct settings". How does this relate to the first part where you
basically describe the problem as SGMII requring inband? Basically
the two don't follow.

How, from a design point of view, because this fundamentally allows
drivers to change how the system behaves, it will allow radically
different behaviours for the same parameters between different drivers.
I am opposed to that - I want to see a situation where we have uniform
behaviour for the same configuration, and where hardware doesn't
support something, we have some way to indicate that via some form
of capabilities.

The issue of whether 2500base-X has inband or not is a long standing
issue, and there are arguments (and hardware) that take totally
opposing views on this. There is hardware where 2500base-X inband
_must_ be used or the link doesn't come up. There is also hardware
where 2500base-X inband is not "supported" in documentation but works
in practice. There is also hardware where 2500base-X inband doesn't
work. The whole thing is a total mess (thanks IEEE 802.3 for not
getting on top of this early enough... and what's now stated in 802.3
for 2500base-X is now irrelevant because they were too late to the
party.)

I haven't been able to look at this issue over the last few weeks
because of being at a summit, and then suffering with flu and its
recovery. However, I have been working on how we can identify the
capabilities of the PCS and PHY w.r.t. inband support in various
interface modes, and how we can handle the result. That work is
ongoing (as and when I have a clear head from after-flu effects.)
Voon, Weifeng Feb. 23, 2024, 6:58 a.m. UTC | #2
> > For instance, if the interface switches from 2500baseX to SGMII mode,
> > and the current link mode is MLO_AN_PHY, calling
> 'phylink_pcs_neg_mode'
> > would yield PHYLINK_PCS_NEG_OUTBAND. Since the MAC and PCS driver
> > require PHYLINK_PCS_NEG_INBAND_ENABLED, the
> 'mac_get_pcs_neg_mode'
> > function will calculate the mode based on the interface, current link
> > negotiation mode, and advertising link mode, returning
> > PHYLINK_PCS_NEG_OUTBAND to enable the PCS to configure the correct
> settings.
> 
> This paragraph doesn't make sense - at least to me. It first talks about
> requiring PHYLINK_PCS_NEG_INBAND_ENABLED when in SGMII mode. On
> this:

The example given here is a very specific condition and that probably why there are some confusions here. Basically, this patch provides an optional function for MAC driver to change the phy interface on-the-fly without the need of reinitialize the Ethernet driver. As we know that the 2500base-x is messy, in our case the 2500base-x does not support inband. To complete the picture, we are using SGMII c37 to handle speed 10/100/1000. Hence, to enable user to switch link speed from 2500 to 1000/100/10 and vice versa on-the-fly, the phy interface need to be configured to inband SGMII for speed 10/100/1000, and outband 2500base-x for speed 2500. Lastly, the newly introduced "mac_get_pcs_neg_mode"callback function enables MAC driver to reconfigure pcs negotiation mode to inband or outband based on the interface mode, current link negotiation mode, and advertising link mode.

> 
> 1) are you sure that the hardware can't be programmed for the SGMII
> symbol repititions?
> 

No, the HW can be program for SGMII symbol repetitions.

> 2) what happens if you're paired with a PHY (e.g. on a SFP module) which
> uses SGMII but has no capability of providing the inband data?
> (They do exist.) If your hardware truly does require inband data, it is going to
> be fundamentally inoperative with these modules.
> 

Above explanation should have already cleared your doubts. Inband or outband capability is configured based on the phy interface.

> Next, you then talk about returning PHYLINK_PCS_NEG_OUTBAND for the
> "correct settings". How does this relate to the first part where you basically
> describe the problem as SGMII requring inband? Basically the two don't
> follow.

It should be a typo mistake. SGMII should return PHYLINK_PCS_NEG_INBAND_ENABLED.

> 
> How, from a design point of view, because this fundamentally allows drivers
> to change how the system behaves, it will allow radically different behaviours
> for the same parameters between different drivers.
> I am opposed to that - I want to see a situation where we have uniform
> behaviour for the same configuration, and where hardware doesn't support
> something, we have some way to indicate that via some form of capabilities.
> 

Hi Russell,
If I understand you correctly, MAC driver should not interfere with pcs negotiation mode and it should be standardized in the generic function, e.g., phylink_pcs_neg_mode()?

> The issue of whether 2500base-X has inband or not is a long standing issue,
> and there are arguments (and hardware) that take totally opposing views on
> this. There is hardware where 2500base-X inband _must_ be used or the link
> doesn't come up. There is also hardware where 2500base-X inband is not
> "supported" in documentation but works in practice. There is also hardware
> where 2500base-X inband doesn't work. The whole thing is a total mess
> (thanks IEEE 802.3 for not getting on top of this early enough... and what's
> now stated in 802.3 for 2500base-X is now irrelevant because they were too
> late to the
> party.)
> 

Agreed. And I have also seen some of your comments regarding the 2500SGMII and 2500BASEX.
Choong Yong Liang March 5, 2024, 4:20 a.m. UTC | #3
On 23/2/2024 2:58 pm, Voon, Weifeng wrote:
>>> For instance, if the interface switches from 2500baseX to SGMII mode,
>>> and the current link mode is MLO_AN_PHY, calling
>> 'phylink_pcs_neg_mode'
>>> would yield PHYLINK_PCS_NEG_OUTBAND. Since the MAC and PCS driver
>>> require PHYLINK_PCS_NEG_INBAND_ENABLED, the
>> 'mac_get_pcs_neg_mode'
>>> function will calculate the mode based on the interface, current link
>>> negotiation mode, and advertising link mode, returning
>>> PHYLINK_PCS_NEG_OUTBAND to enable the PCS to configure the correct
>> settings.
>>
>> This paragraph doesn't make sense - at least to me. It first talks about
>> requiring PHYLINK_PCS_NEG_INBAND_ENABLED when in SGMII mode. On
>> this:
> 
> The example given here is a very specific condition and that probably why there are some confusions here. Basically, this patch provides an optional function for MAC driver to change the phy interface on-the-fly without the need of reinitialize the Ethernet driver. As we know that the 2500base-x is messy, in our case the 2500base-x does not support inband. To complete the picture, we are using SGMII c37 to handle speed 10/100/1000. Hence, to enable user to switch link speed from 2500 to 1000/100/10 and vice versa on-the-fly, the phy interface need to be configured to inband SGMII for speed 10/100/1000, and outband 2500base-x for speed 2500. Lastly, the newly introduced "mac_get_pcs_neg_mode"callback function enables MAC driver to reconfigure pcs negotiation mode to inband or outband based on the interface mode, current link negotiation mode, and advertising link mode.
> 
>>
>> 1) are you sure that the hardware can't be programmed for the SGMII
>> symbol repititions?
>>
> 
> No, the HW can be program for SGMII symbol repetitions.
> 
>> 2) what happens if you're paired with a PHY (e.g. on a SFP module) which
>> uses SGMII but has no capability of providing the inband data?
>> (They do exist.) If your hardware truly does require inband data, it is going to
>> be fundamentally inoperative with these modules.
>>
> 
> Above explanation should have already cleared your doubts. Inband or outband capability is configured based on the phy interface.
> 
>> Next, you then talk about returning PHYLINK_PCS_NEG_OUTBAND for the
>> "correct settings". How does this relate to the first part where you basically
>> describe the problem as SGMII requring inband? Basically the two don't
>> follow.
> 
> It should be a typo mistake. SGMII should return PHYLINK_PCS_NEG_INBAND_ENABLED.
> 
>>
>> How, from a design point of view, because this fundamentally allows drivers
>> to change how the system behaves, it will allow radically different behaviours
>> for the same parameters between different drivers.
>> I am opposed to that - I want to see a situation where we have uniform
>> behaviour for the same configuration, and where hardware doesn't support
>> something, we have some way to indicate that via some form of capabilities.
>>
> 
> Hi Russell,
> If I understand you correctly, MAC driver should not interfere with pcs negotiation mode and it should be standardized in the generic function, e.g., phylink_pcs_neg_mode()?
> 
>> The issue of whether 2500base-X has inband or not is a long standing issue,
>> and there are arguments (and hardware) that take totally opposing views on
>> this. There is hardware where 2500base-X inband _must_ be used or the link
>> doesn't come up. There is also hardware where 2500base-X inband is not
>> "supported" in documentation but works in practice. There is also hardware
>> where 2500base-X inband doesn't work. The whole thing is a total mess
>> (thanks IEEE 802.3 for not getting on top of this early enough... and what's
>> now stated in 802.3 for 2500base-X is now irrelevant because they were too
>> late to the
>> party.)
>>
> 
> Agreed. And I have also seen some of your comments regarding the 2500SGMII and 2500BASEX.
> 
Hi Russell,

Did the previous reply clear your doubt?

If we understand you correctly that MAC driver should not interfere with 
pcs negotiation mode and it should be standardized in the generic function.
If implement what you suggested earlier that during interface mode change 
then update the `cur_link_an_mode` but not cfg_link_an_mode: 
https://patchwork.kernel.org/project/netdevbpf/patch/20230921121946.3025771-4-yong.liang.choong@linux.intel.com/?

Would that be a better solution?
So that 'phylink_pcs_neg_mode' function still can serve as the generic for 
all the drivers.

Do you have anything in mind that we can handle better for this patch series?
or the solution can be aligned with what you are going to implement in the 
future?
Russell King (Oracle) March 5, 2024, 8:39 a.m. UTC | #4
On Tue, Mar 05, 2024 at 12:20:39PM +0800, Choong Yong Liang wrote:
> 
> 
> On 23/2/2024 2:58 pm, Voon, Weifeng wrote:
> > > > For instance, if the interface switches from 2500baseX to SGMII mode,
> > > > and the current link mode is MLO_AN_PHY, calling
> > > 'phylink_pcs_neg_mode'
> > > > would yield PHYLINK_PCS_NEG_OUTBAND. Since the MAC and PCS driver
> > > > require PHYLINK_PCS_NEG_INBAND_ENABLED, the
> > > 'mac_get_pcs_neg_mode'
> > > > function will calculate the mode based on the interface, current link
> > > > negotiation mode, and advertising link mode, returning
> > > > PHYLINK_PCS_NEG_OUTBAND to enable the PCS to configure the correct
> > > settings.
> > > 
> > > This paragraph doesn't make sense - at least to me. It first talks about
> > > requiring PHYLINK_PCS_NEG_INBAND_ENABLED when in SGMII mode. On
> > > this:
> > 
> > The example given here is a very specific condition and that probably why there are some confusions here. Basically, this patch provides an optional function for MAC driver to change the phy interface on-the-fly without the need of reinitialize the Ethernet driver. As we know that the 2500base-x is messy, in our case the 2500base-x does not support inband. To complete the picture, we are using SGMII c37 to handle speed 10/100/1000. Hence, to enable user to switch link speed from 2500 to 1000/100/10 and vice versa on-the-fly, the phy interface need to be configured to inband SGMII for speed 10/100/1000, and outband 2500base-x for speed 2500. Lastly, the newly introduced "mac_get_pcs_neg_mode"callback function enables MAC driver to reconfigure pcs negotiation mode to inband or outband based on the interface mode, current link negotiation mode, and advertising link mode.
> > 
> > > 
> > > 1) are you sure that the hardware can't be programmed for the SGMII
> > > symbol repititions?
> > > 
> > 
> > No, the HW can be program for SGMII symbol repetitions.
> > 
> > > 2) what happens if you're paired with a PHY (e.g. on a SFP module) which
> > > uses SGMII but has no capability of providing the inband data?
> > > (They do exist.) If your hardware truly does require inband data, it is going to
> > > be fundamentally inoperative with these modules.
> > > 
> > 
> > Above explanation should have already cleared your doubts. Inband or outband capability is configured based on the phy interface.
> > 
> > > Next, you then talk about returning PHYLINK_PCS_NEG_OUTBAND for the
> > > "correct settings". How does this relate to the first part where you basically
> > > describe the problem as SGMII requring inband? Basically the two don't
> > > follow.
> > 
> > It should be a typo mistake. SGMII should return PHYLINK_PCS_NEG_INBAND_ENABLED.
> > 
> > > 
> > > How, from a design point of view, because this fundamentally allows drivers
> > > to change how the system behaves, it will allow radically different behaviours
> > > for the same parameters between different drivers.
> > > I am opposed to that - I want to see a situation where we have uniform
> > > behaviour for the same configuration, and where hardware doesn't support
> > > something, we have some way to indicate that via some form of capabilities.
> > > 
> > 
> > Hi Russell,
> > If I understand you correctly, MAC driver should not interfere with pcs negotiation mode and it should be standardized in the generic function, e.g., phylink_pcs_neg_mode()?
> > 
> > > The issue of whether 2500base-X has inband or not is a long standing issue,
> > > and there are arguments (and hardware) that take totally opposing views on
> > > this. There is hardware where 2500base-X inband _must_ be used or the link
> > > doesn't come up. There is also hardware where 2500base-X inband is not
> > > "supported" in documentation but works in practice. There is also hardware
> > > where 2500base-X inband doesn't work. The whole thing is a total mess
> > > (thanks IEEE 802.3 for not getting on top of this early enough... and what's
> > > now stated in 802.3 for 2500base-X is now irrelevant because they were too
> > > late to the
> > > party.)
> > > 
> > 
> > Agreed. And I have also seen some of your comments regarding the 2500SGMII and 2500BASEX.
> > 
> Hi Russell,
> 
> Did the previous reply clear your doubt?

I am working on the problem as and when I can find the time (which is
not much spare time.) I have some prototype code at the moment.
diff mbox series

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 503fd7c40523..b38b39a6d1f0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1151,9 +1151,17 @@  static void phylink_major_config(struct phylink *pl, bool restart,
 
 	phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
 
-	pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
-						state->interface,
-						state->advertising);
+	if (pl->mac_ops->mac_get_pcs_neg_mode) {
+		pl->pcs_neg_mode = pl->mac_ops->mac_get_pcs_neg_mode
+						(pl->config,
+						 pl->cur_link_an_mode,
+						 state->interface,
+						 state->advertising);
+	} else {
+		pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
+							state->interface,
+							state->advertising);
+	}
 
 	if (pl->using_mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6ba411732a0d..f0a6c00e8dab 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -168,6 +168,7 @@  void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
  * @mac_finish: finish a major reconfiguration of the interface.
  * @mac_link_down: take the link down.
  * @mac_link_up: allow the link to come up.
+ * @mac_get_pcs_neg_mode: Get PCS negotiation mode for interface mode.
  *
  * The individual methods are described more fully below.
  */
@@ -188,6 +189,10 @@  struct phylink_mac_ops {
 			    struct phy_device *phy, unsigned int mode,
 			    phy_interface_t interface, int speed, int duplex,
 			    bool tx_pause, bool rx_pause);
+	unsigned int (*mac_get_pcs_neg_mode)(struct phylink_config *config,
+					     unsigned int mode,
+					     phy_interface_t interface,
+					     const unsigned long *advertising);
 };
 
 #if 0 /* For kernel-doc purposes only. */