* phylink: flow control on fixed-link not working.
@ 2019-07-17 21:31 René van Dorst
2019-07-17 21:51 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 5+ messages in thread
From: René van Dorst @ 2019-07-17 21:31 UTC (permalink / raw)
To: netdev; +Cc: Russell King
Hi,
I am trying to enable flow control/pause on PHYLINK and fixed-link.
My setup SOC mac (mt7621) <-> RGMII <-> SWITCH mac (mt7530).
It seems that in fixed-link mode all the flow control/pause bits are
cleared in
phylink_parse_fixedlink(). If I read phylink_parse_fixedlink() [0] correctly,
I see that pl->link_config.advertising is AND with pl->supprted which has only
the PHY_SETTING() modes bits set. pl->link_config.advertising is losing Pause
bits. pl->link_config.advertising is used in phylink_resolve_flow() to set the
MLO_PAUSE_RX/TX BITS.
I think this is an error.
Because in phylink_start() see this part [1].
/* Apply the link configuration to the MAC when starting. This allows
* a fixed-link to start with the correct parameters, and also
* ensures that we set the appropriate advertisement for Serdes links.
*/
phylink_resolve_flow(pl, &pl->link_config);
phylink_mac_config(pl, &pl->link_config);
If I add a this hacky patch below, flow control is enabled on the fixed-link.
if (s) {
__set_bit(s->bit, pl->supported);
+ if (phylink_test(pl->link_config.advertising, Pause))
+ phylink_set(pl->supported, Pause);
} else {
So is phylink_parse_fixedlink() broken or should it handled in a other way?
Greats,
René
[0]:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phylink.c#L196
[1]:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phylink.c#L897
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: phylink: flow control on fixed-link not working.
2019-07-17 21:31 phylink: flow control on fixed-link not working René van Dorst
@ 2019-07-17 21:51 ` Russell King - ARM Linux admin
2019-07-17 22:58 ` René van Dorst
0 siblings, 1 reply; 5+ messages in thread
From: Russell King - ARM Linux admin @ 2019-07-17 21:51 UTC (permalink / raw)
To: René van Dorst; +Cc: netdev
On Wed, Jul 17, 2019 at 09:31:11PM +0000, René van Dorst wrote:
> Hi,
>
> I am trying to enable flow control/pause on PHYLINK and fixed-link.
>
> My setup SOC mac (mt7621) <-> RGMII <-> SWITCH mac (mt7530).
>
> It seems that in fixed-link mode all the flow control/pause bits are cleared
> in
> phylink_parse_fixedlink(). If I read phylink_parse_fixedlink() [0] correctly,
> I see that pl->link_config.advertising is AND with pl->supprted which has only
> the PHY_SETTING() modes bits set. pl->link_config.advertising is losing Pause
> bits. pl->link_config.advertising is used in phylink_resolve_flow() to set the
> MLO_PAUSE_RX/TX BITS.
>
> I think this is an error.
> Because in phylink_start() see this part [1].
>
> /* Apply the link configuration to the MAC when starting. This allows
> * a fixed-link to start with the correct parameters, and also
> * ensures that we set the appropriate advertisement for Serdes links.
> */
> phylink_resolve_flow(pl, &pl->link_config);
> phylink_mac_config(pl, &pl->link_config);
>
>
> If I add a this hacky patch below, flow control is enabled on the fixed-link.
> if (s) {
> __set_bit(s->bit, pl->supported);
> + if (phylink_test(pl->link_config.advertising, Pause))
> + phylink_set(pl->supported, Pause);
> } else {
>
> So is phylink_parse_fixedlink() broken or should it handled in a other way?
Quite simply, if the MAC says it doesn't support pause modes (i.o.w.
the validate callback clears them) then pause modes aren't supported.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: phylink: flow control on fixed-link not working.
2019-07-17 21:51 ` Russell King - ARM Linux admin
@ 2019-07-17 22:58 ` René van Dorst
2019-07-19 19:52 ` René van Dorst
0 siblings, 1 reply; 5+ messages in thread
From: René van Dorst @ 2019-07-17 22:58 UTC (permalink / raw)
To: Russell King - ARM Linux admin; +Cc: netdev
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
> On Wed, Jul 17, 2019 at 09:31:11PM +0000, René van Dorst wrote:
>> Hi,
>>
>> I am trying to enable flow control/pause on PHYLINK and fixed-link.
>>
>> My setup SOC mac (mt7621) <-> RGMII <-> SWITCH mac (mt7530).
>>
>> It seems that in fixed-link mode all the flow control/pause bits are cleared
>> in
>> phylink_parse_fixedlink(). If I read phylink_parse_fixedlink() [0]
>> correctly,
>> I see that pl->link_config.advertising is AND with pl->supprted
>> which has only
>> the PHY_SETTING() modes bits set. pl->link_config.advertising is
>> losing Pause
>> bits. pl->link_config.advertising is used in phylink_resolve_flow()
>> to set the
>> MLO_PAUSE_RX/TX BITS.
>>
>> I think this is an error.
>> Because in phylink_start() see this part [1].
>>
>> /* Apply the link configuration to the MAC when starting. This allows
>> * a fixed-link to start with the correct parameters, and also
>> * ensures that we set the appropriate advertisement for Serdes links.
>> */
>> phylink_resolve_flow(pl, &pl->link_config);
>> phylink_mac_config(pl, &pl->link_config);
>>
>>
>> If I add a this hacky patch below, flow control is enabled on the
>> fixed-link.
>> if (s) {
>> __set_bit(s->bit, pl->supported);
>> + if (phylink_test(pl->link_config.advertising, Pause))
>> + phylink_set(pl->supported, Pause);
>> } else {
>>
>> So is phylink_parse_fixedlink() broken or should it handled in a other way?
>
> Quite simply, if the MAC says it doesn't support pause modes (i.o.w.
> the validate callback clears them) then pause modes aren't supported.
Hi Russel,
Thanks for your response.
I believe that I am setting pause bits right on both ends see SOC [0] and
SWITCH [1] and also in the DTS [2].
Correct me if it is not the right way.
Maybe I am looking in the wrong part of the code.
But I added many debug lines in phylink_parse_fixedlink() [3] to see what
happens with the Pause bit in the pl->link_config.advertising and
pl->supported.
This is the dmesg output.
[ 1.991245] libphy: Fixed MDIO Bus: probed
[ 2.031260] phylink_create: config0: Pause
[ 2.039410] phylink_create: supported: Pause
[ 2.047904] mtk_validate: mask: Pause
[ 2.055186] mtk_validate: supported: Pause
[ 2.063332] mtk_validate: advertising: Pause
[ 2.071825] phylink_create: config1: Pause
[ 2.079966] phylink_create: config2: Pause
[ 2.088132] phylink_parse_fixedlink: config: Pause
[ 2.097660] phylink_parse_fixedlink: support: Pause
[ 2.107366] mtk_validate: mask: Pause
[ 2.114647] mtk_validate: supported: Pause
[ 2.122792] mtk_validate: advertising: Pause
[ 2.131283] phylink_parse_fixedlink: config2: Pause
[ 2.140971] phylink_parse_fixedlink: support2: Pause
[ 2.150845] phylink_parse_fixedlink: config3: Pause
[ 2.160546] phylink_parse_fixedlink: support3: Pause
[ 2.170420] phylink_parse_fixedlink: config4: Pause
[ 2.180120] phylink_parse_fixedlink: config5: Pause
[ 5.854674] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
[ 5.867665] phylink_resolve_flow: PAUSE_AN: pause: 0, 12, 8dfba630
[ 5.867670] phylink_resolve_flow: new_pause: 0
[ 5.879980] mt7530 mdio-bus:1f: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
[ 6.651239] DSA: tree 0 setup
[ 6.658192] input: gpio-keys as /devices/platform/gpio-keys/input/input0
[ 6.672108] mt7530 mdio-bus:1f: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
[ 28.937543] mtk_soc_eth 1e100000.ethernet eth0: configuring for
fixed/trgmii link mode
[ 28.965884] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
[ 29.000740] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
[ 29.026392] mtk_soc_eth 1e100000.ethernet eth0: Link is Up -
1Gbps/Full - flow control off
[ 29.373577] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
I don't see the "config6:" [4] debug.
I think the pause bits are always cleared in
pl->link_config.advertising by phylink_parse_fixedlink()
Again I may understand the code wrong or I am looking at the wrong place.
So I hope you can point me in the right direction.
Greats,
René
[0]:
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L502
[1]:
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/dsa/mt7530.c#L1468
[2]:
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/staging/mt7621-dts/UBNT-ER-e50.dtsi#L122
[3]:
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L214
[4]:
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L263
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: phylink: flow control on fixed-link not working.
2019-07-17 22:58 ` René van Dorst
@ 2019-07-19 19:52 ` René van Dorst
2019-07-26 21:28 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 5+ messages in thread
From: René van Dorst @ 2019-07-19 19:52 UTC (permalink / raw)
To: Russell King - ARM Linux admin; +Cc: netdev
Quoting René van Dorst <opensource@vdorst.com>:
> Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
>
>> On Wed, Jul 17, 2019 at 09:31:11PM +0000, René van Dorst wrote:
>>> Hi,
>>>
>>> I am trying to enable flow control/pause on PHYLINK and fixed-link.
>>>
>>> My setup SOC mac (mt7621) <-> RGMII <-> SWITCH mac (mt7530).
>>>
>>> It seems that in fixed-link mode all the flow control/pause bits
>>> are cleared
>>> in
>>> phylink_parse_fixedlink(). If I read phylink_parse_fixedlink() [0]
>>> correctly,
>>> I see that pl->link_config.advertising is AND with pl->supprted
>>> which has only
>>> the PHY_SETTING() modes bits set. pl->link_config.advertising is
>>> losing Pause
>>> bits. pl->link_config.advertising is used in
>>> phylink_resolve_flow() to set the
>>> MLO_PAUSE_RX/TX BITS.
>>>
>>> I think this is an error.
>>> Because in phylink_start() see this part [1].
>>>
>>> /* Apply the link configuration to the MAC when starting. This allows
>>> * a fixed-link to start with the correct parameters, and also
>>> * ensures that we set the appropriate advertisement for Serdes links.
>>> */
>>> phylink_resolve_flow(pl, &pl->link_config);
>>> phylink_mac_config(pl, &pl->link_config);
>>>
>>>
>>> If I add a this hacky patch below, flow control is enabled on the
>>> fixed-link.
>>> if (s) {
>>> __set_bit(s->bit, pl->supported);
>>> + if (phylink_test(pl->link_config.advertising, Pause))
>>> + phylink_set(pl->supported, Pause);
>>> } else {
>>>
>>> So is phylink_parse_fixedlink() broken or should it handled in a other way?
>>
>> Quite simply, if the MAC says it doesn't support pause modes (i.o.w.
>> the validate callback clears them) then pause modes aren't supported.
>
> Hi Russel,
>
> Thanks for your response.
>
> I believe that I am setting pause bits right on both ends see SOC [0] and
> SWITCH [1] and also in the DTS [2].
>
> Correct me if it is not the right way.
>
>
> Maybe I am looking in the wrong part of the code.
> But I added many debug lines in phylink_parse_fixedlink() [3] to see what
> happens with the Pause bit in the pl->link_config.advertising and
> pl->supported.
>
>
> This is the dmesg output.
> [ 1.991245] libphy: Fixed MDIO Bus: probed
> [ 2.031260] phylink_create: config0: Pause
> [ 2.039410] phylink_create: supported: Pause
> [ 2.047904] mtk_validate: mask: Pause
> [ 2.055186] mtk_validate: supported: Pause
> [ 2.063332] mtk_validate: advertising: Pause
> [ 2.071825] phylink_create: config1: Pause
> [ 2.079966] phylink_create: config2: Pause
> [ 2.088132] phylink_parse_fixedlink: config: Pause
> [ 2.097660] phylink_parse_fixedlink: support: Pause
> [ 2.107366] mtk_validate: mask: Pause
> [ 2.114647] mtk_validate: supported: Pause
> [ 2.122792] mtk_validate: advertising: Pause
> [ 2.131283] phylink_parse_fixedlink: config2: Pause
> [ 2.140971] phylink_parse_fixedlink: support2: Pause
> [ 2.150845] phylink_parse_fixedlink: config3: Pause
> [ 2.160546] phylink_parse_fixedlink: support3: Pause
> [ 2.170420] phylink_parse_fixedlink: config4: Pause
> [ 2.180120] phylink_parse_fixedlink: config5: Pause
>
> [ 5.854674] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [ 5.867665] phylink_resolve_flow: PAUSE_AN: pause: 0, 12, 8dfba630
> [ 5.867670] phylink_resolve_flow: new_pause: 0
> [ 5.879980] mt7530 mdio-bus:1f: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12
> link=1 an=1
> [ 6.651239] DSA: tree 0 setup
> [ 6.658192] input: gpio-keys as /devices/platform/gpio-keys/input/input0
> [ 6.672108] mt7530 mdio-bus:1f: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12
> link=1 an=1
> [ 28.937543] mtk_soc_eth 1e100000.ethernet eth0: configuring for
> fixed/trgmii link mode
> [ 28.965884] mtk_soc_eth 1e100000.ethernet eth0:
> phylink_mac_config: mode=fixed/trgmii/1Gbps/Full
> adv=00,00000000,00000220 pause=12 link=1 an=1
> [ 29.000740] mtk_soc_eth 1e100000.ethernet eth0:
> phylink_mac_config: mode=fixed/trgmii/1Gbps/Full
> adv=00,00000000,00000220 pause=12 link=1 an=1
> [ 29.026392] mtk_soc_eth 1e100000.ethernet eth0: Link is Up -
> 1Gbps/Full - flow control off
> [ 29.373577] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>
>
> I don't see the "config6:" [4] debug.
> I think the pause bits are always cleared in
> pl->link_config.advertising by phylink_parse_fixedlink()
>
> Again I may understand the code wrong or I am looking at the wrong place.
> So I hope you can point me in the right direction...
Hi Russel,
If I use this patch below:
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5d0af041b8f9..a6aebaa14338 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -216,6 +216,8 @@ static int phylink_parse_fixedlink(struct phylink *pl,
pl->supported, true);
linkmode_zero(pl->supported);
phylink_set(pl->supported, MII);
+ phylink_set(pl->supported, Pause);
+ phylink_set(pl->supported, Asym_Pause);
if (s) {
__set_bit(s->bit, pl->supported);
} else {
Which is similar thing also done in phylink_parse_mode().
I get these results:
- DTS = 'Pause' is set in the fixed-link
- validate = No means phylink_set(mask, Pause) is not used in validate
callback.
- flow = results reported my link is Up line.
+-----+----------+-------+
| DTS | validate | flow |
+-----+----------+-------+
| Yes | Yes | rx/tx |
| No | Yes | off |
| Yes | No | off |
+-----+----------+-------+
What do you think?
Can this be a correct fix?
Greats,
René
>
> Greats,
>
> René
>
>
> [0]:
> https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L502
> [1]:
> https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/dsa/mt7530.c#L1468
> [2]:
> https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/staging/mt7621-dts/UBNT-ER-e50.dtsi#L122
> [3]:
> https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L214
> [4]:
> https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L263
>
>>
>> --
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down
>> 622kbps up
>> According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: phylink: flow control on fixed-link not working.
2019-07-19 19:52 ` René van Dorst
@ 2019-07-26 21:28 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 5+ messages in thread
From: Russell King - ARM Linux admin @ 2019-07-26 21:28 UTC (permalink / raw)
To: René van Dorst; +Cc: netdev
On Fri, Jul 19, 2019 at 07:52:26PM +0000, René van Dorst wrote:
> Hi Russel,
>
> If I use this patch below:
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 5d0af041b8f9..a6aebaa14338 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -216,6 +216,8 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> pl->supported, true);
> linkmode_zero(pl->supported);
> phylink_set(pl->supported, MII);
> + phylink_set(pl->supported, Pause);
> + phylink_set(pl->supported, Asym_Pause);
> if (s) {
> __set_bit(s->bit, pl->supported);
> } else {
>
> Which is similar thing also done in phylink_parse_mode().
Yep, that's what should be there - please submit as a fix patch, thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-26 21:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17 21:31 phylink: flow control on fixed-link not working René van Dorst
2019-07-17 21:51 ` Russell King - ARM Linux admin
2019-07-17 22:58 ` René van Dorst
2019-07-19 19:52 ` René van Dorst
2019-07-26 21:28 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).