lkml.org 
[lkml]   [2022]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation
From
Date


On 7/20/22 2:50 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:57PM -0400, Sean Anderson wrote:
>> If the phy is configured to use pause-based rate adaptation, ensure that
>> the link is full duplex with pause frame reception enabled. As
>> suggested, if pause-based rate adaptation is enabled by the phy, then
>> pause reception is unconditionally enabled.
>>
>> The interface duplex is determined based on the rate adaptation type.
>> When rate adaptation is enabled, so is the speed. We assume the maximum
>> interface speed is used. This is only relevant for MLO_AN_PHY. For
>> MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v2:
>> - Use the phy's rate adaptation setting to determine whether to use its
>> link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
>> - Always use the rate adaptation setting to determine the interface
>> speed/duplex (instead of sometimes using the interface mode).
>>
>> drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++----
>> include/linux/phylink.h | 1 +
>> 2 files changed, 114 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index da0623d94a64..619ef553476f 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode)
>> * @state: A link state
>> *
>> * Update the .speed and .duplex members of @state. We can determine them based
>> - * on the .link_speed and .link_duplex. This function should be called whenever
>> - * .link_speed and .link_duplex are updated. For example, userspace deals with
>> - * link speed and duplex, and not the interface speed and duplex. Similarly,
>> - * phys deal with link speed and duplex and only implicitly the interface speed
>> - * and duplex.
>> + * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This
>> + * function should be called whenever .link_speed and .link_duplex are updated.
>> + * For example, userspace deals with link speed and duplex, and not the
>> + * interface speed and duplex. Similarly, phys deal with link speed and duplex
>> + * and only implicitly the interface speed and duplex.
>> */
>> static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
>> {
>> - state->speed = state->link_speed;
>> - state->duplex = state->link_duplex;
>> + switch (state->rate_adaptation) {
>> + case RATE_ADAPT_NONE:
>> + state->speed = state->link_speed;
>> + state->duplex = state->link_duplex;
>> + return;
>> + case RATE_ADAPT_PAUSE:
>> + state->duplex = DUPLEX_FULL;
>> + break;
>> + case RATE_ADAPT_CRS:
>> + state->duplex = DUPLEX_HALF;
>> + break;
>> + case RATE_ADAPT_OPEN_LOOP:
>> + state->duplex = state->link_duplex;
>> + break;
>> + }
>> +
>> + /* Use the max speed of the interface */
>> + switch (state->interface) {
>> + case PHY_INTERFACE_MODE_100BASEX:
>> + case PHY_INTERFACE_MODE_REVRMII:
>> + case PHY_INTERFACE_MODE_RMII:
>> + case PHY_INTERFACE_MODE_SMII:
>> + case PHY_INTERFACE_MODE_REVMII:
>> + case PHY_INTERFACE_MODE_MII:
>> + state->speed = SPEED_100;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_TBI:
>> + case PHY_INTERFACE_MODE_MOCA:
>> + case PHY_INTERFACE_MODE_RTBI:
>> + case PHY_INTERFACE_MODE_1000BASEX:
>> + case PHY_INTERFACE_MODE_1000BASEKX:
>> + case PHY_INTERFACE_MODE_TRGMII:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII:
>> + case PHY_INTERFACE_MODE_QSGMII:
>> + case PHY_INTERFACE_MODE_SGMII:
>> + case PHY_INTERFACE_MODE_GMII:
>> + state->speed = SPEED_1000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_2500BASEX:
>> + state->speed = SPEED_2500;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_5GBASER:
>> + state->speed = SPEED_5000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_XGMII:
>> + case PHY_INTERFACE_MODE_RXAUI:
>> + case PHY_INTERFACE_MODE_XAUI:
>> + case PHY_INTERFACE_MODE_10GBASER:
>> + case PHY_INTERFACE_MODE_10GKR:
>> + case PHY_INTERFACE_MODE_USXGMII:
>> + state->speed = SPEED_10000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_25GBASER:
>> + state->speed = SPEED_25000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_XLGMII:
>> + state->speed = SPEED_40000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_INTERNAL:
>> + state->speed = state->link_speed;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_NA:
>> + case PHY_INTERFACE_MODE_MAX:
>> + state->speed = SPEED_UNKNOWN;
>> + return;
>> + }
>> +
>> + WARN_ON(1);
>> }
>>
>> /**
>> @@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl,
>> const struct phylink_link_state *state)
>> {
>> phylink_dbg(pl,
>> - "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>> + "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>> __func__, phylink_an_mode_str(pl->cur_link_an_mode),
>> phy_modes(state->interface),
>> phy_speed_to_str(state->speed),
>> phy_duplex_to_str(state->duplex),
>> + phy_rate_adaptation_to_str(state->rate_adaptation),
>> __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>> state->pause, state->link, state->an_enabled);
>>
>> @@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>> linkmode_zero(state->lp_advertising);
>> state->interface = pl->link_config.interface;
>> state->an_enabled = pl->link_config.an_enabled;
>> + state->rate_adaptation = pl->link_config.rate_adaptation;
>> if (state->an_enabled) {
>> state->link_speed = SPEED_UNKNOWN;
>> state->link_duplex = DUPLEX_UNKNOWN;
>> @@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>> else
>> state->link = 0;
>>
>> - state->link_speed = state->speed;
>> - state->link_duplex = state->duplex;
>> + if (state->rate_adaptation == RATE_ADAPT_NONE) {
>> + state->link_speed = state->speed;
>> + state->link_duplex = state->duplex;
>> + }
>
> So we need to have every PCS driver be udpated to fill in link_speed
> and link_duplex if rate_adaption != none.

The PCS doesn't know what the link speed/duplex is. If rate adaptation is
enabled, then the PCS only knows what the interface speed/duplex is.

> There's got to be a better way - maybe what I suggested in the last
> round of only doing the rate adaption thing in the link_up() functions,
> since that seems to be the only real difference.
>
> I'm not even sure we need to do that - in the "open loop" case, we
> need to be passing the media speed to the MAC driver with the knowledge
> that it should be increasing the IPG.
>
> So, I'm thinking we don't want any of these changes, what we instead
> should be doing is passing the media speed/duplex and the interface
> speed/duplex to the PCS and MAC.

> We can do that by storing the PHY rate adaption state, and processing
> that in phylink_link_up().

This approach sounds better. You patch below looks good. I'll test it
and use it for v3.

--Sean

\
 
 \ /
  Last update: 2022-07-21 18:26    [W:0.094 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site