Date:   Tue, 21 Jan 2020 08:18:10 +0000
From:   Alexandru Marginean <alexandru.marginean@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Vladimir Oltean <olteanv@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 1/2] net: dsa: felix: Handle PAUSE RX regardless
 of AN result

resent from NXP account, apparently google thinks this is just spam.

On 1/20/2020 10:43 AM, Russell King - ARM Linux admin wrote:
> On Thu, Jan 16, 2020 at 08:19:32PM +0200, Vladimir Oltean wrote:
>> From: Alex Marginean <alexandru.marginean@....com>
>>
>> Flow control is used with 2500Base-X and AQR PHYs to do rate adaptation
>> between line side 100/1000 links and MAC running at 2.5G.
>>
>> This is independent of the flow control configuration settled on line
>> side though AN.
>>
>> In general, allowing the MAC to handle flow control even if not
>> negotiated with the link partner should not be a problem, so the patch
>> just enables it in all cases.
>>
>> Signed-off-by: Alex Marginean <alexandru.marginean@....com>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> 
> I think this is not the best approach - you're working around the
> issue in your network driver, rather than recognising that it's a
> larger problem than just your network driver.

Both are true, this is a work-around so our system isn't functionally 
broken and it's clear that the general issue has to be handled elsewhere.


> Rate adaption is present in other PHYs using exactly the same
> mechanism so why do we want to hack around this in each network
> driver?  It is a property of the PHY, not of the network driver.
> 
> Surely it not be better to address this in phylib/phylink - after
> all, there are several aspects to this:
> 
> 1) separation of the MAC configuration (reported to the MAC) from
>     the negotiation results (reported to the user).
> 2) we need the MAC to be able to receive and act on flow control.
> 3) we need to report the correct speed setting to the MAC.
> 
> I already have patches to improve the current phylib method of
> reporting the flow control information to MAC drivers with the resolved
> flow state rather than just the current link partner advertisement
> bits, which should make (2) fairly easy to achieve.  (1) and (3) will
> require additional work.

I think this won't be trivial to address, let's see how it will go.

At the PHY level we would add a capability indicating that flow control 
can be used as a way to do rate adaptation.  If that's just a capability 
bit declared at probing, it should probably imply that the PHY driver 
also provides run-time configuration.  This way flow control can be 
enabled/disabled so that the PHY configuration is adjusted based on 
phylink resolved state, this is probably the ideal case.

I'm not sure how many PHYs are going to be that flexible though, we may 
need quirks for:
- the PHY supports flow control but it's not configurable, potentially 
baked into the firmware,
- flow control is available for certain interface types but not for others,
- flow control is available for certain link speeds but not for others, 
or other restrictions like these.

Should PHY level indication of flow control support be a static flag, or 
a function of interface type/link speed?  The latter would allow the PHY 
driver to address any quirkiness internally.

We've been working with Aquantia PHYs and they are somewhat quirky, it 
would be useful to hear about other PHYs if anyone has any first-hand 
experience with other PHYs doing flow control.

Phylink should then take in MAC capabilites, PHY capabilities and if:
- MAC supports flow control configuration or Rx is always on, and
- PHY supports flow control (either as a generic capability flag plus 
optional quirks, or as a function of interface type/link speed), and
- the current interface type does not allow dealing with rate adaptation 
internally (like XFI, 2500base-x as currently used in the kernel), and maybe
- link speed on line side is below the capacity of the system interface,
then instruct the PHY to do rate adaptation using flow control and 
enable flow control Rx in the MAC.

If the conditions aren't met maybe phylink should issue a warning or 
some sort of indication to the user.  If the system ends up with a 1G 
link on line side, XFI and no rate adaptation, some networking protocols 
aren't going to work too well.

At the user level I think we're going to have to present more 
information, what the peers advertised during AN, what the result of the 
AN was and also what the configuration of the MAC is, as now this could 
be different due to rate adaptation.  That way the user can tell for 
instance that flow control was disabled as part of AN but it is enabled 
in the MAC for the purpose of doing rate adaptation between MAC and PHY.

ethtool -A should probably not control the actual MAC configuration 
either, to keep rate adaptation sane, but rather be part of the phylink 
algorithm.  This is a little tricky to do, given that ethtool ops are 
now implemented by Eth drivers.

Is this how you see it working out?

Thanks!
Alex

> 
>> ---
>>   drivers/net/dsa/ocelot/felix.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
>> index d6ee089dbfe1..46334436a8fe 100644
>> --- a/drivers/net/dsa/ocelot/felix.c
>> +++ b/drivers/net/dsa/ocelot/felix.c
>> @@ -222,8 +222,12 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
>>   	 * specification in incoming pause frames.
>>   	 */
>>   	mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(state->speed);
>> -	if (state->pause & MLO_PAUSE_RX)
>> -		mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
>> +
>> +	/* handle Rx pause in all cases, with 2500base-X this is used for rate
>> +	 * adaptation.
>> +	 */
>> +	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
>> +
>>   	if (state->pause & MLO_PAUSE_TX)
>>   		mac_fc_cfg |= SYS_MAC_FC_CFG_TX_FC_ENA |
>>   			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
>> -- 
>> 2.17.1
>>
>>
> 

Powered by blists - more mailing lists