When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x
(it also supports 2500base-x) in device-tree I find that
phylink_helper_basex_speed() changes interface to
PHY_INTERFACE_MODE_2500BASEX. The Ethernet adapter connecting to this
switch port doesn't support 2500BASEX so it never establishes a link.
If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works
fine.
state->an_enabled is true when phylink_helper_basex_speed() is called
even when configured with fixed-link. This causes it to change the
interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in
state->advertising which it always is on the first call because
phylink_create calls bitmap_fill(pl->supported,
__ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled
be true with MLO_AN_FIXED?
I've also noticed that phylink_validate (which ends up calling
phylink_helper_basex_speed) is called before phylink_parse_mode in
phylink_create. If phylink_helper_basex_speed changes the interface
mode this influences whether phylink_parse_mode (for MLO_AN_INBAND)
sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then
copied to pl->advertising). phylink_helper_basex_speed is then called
again (via phylink_validate) which uses advertising to decide how to
set interface. This seems like circular logic.
To make matters even more confusing I see that
mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether
to set state->speed to SPEED_1000 or SPEED_2500.
I've been thinking through how to get the desired behavior but I'm not
even sure what the desired behavior is. If you set phy-mode to
"1000base-x" in device-tree do you ever want interface to be set to
PHY_INTERFACE_MODE_2500BASEX? If so just for MLO_AN_INBAND or also for
ML_AN_FIXED? Do we want phylink_validate called in phylink_create even
though it gets called anyway for MLO_AN_INBAND and ML_AN_FIXED later?
Regards,
George McCollister
Hi George,
On Thu, Mar 25, 2021 at 11:36:26AM -0500, George McCollister wrote:
When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x
(it also supports 2500base-x) in device-tree I find that
phylink_helper_basex_speed() changes interface to
PHY_INTERFACE_MODE_2500BASEX. The Ethernet adapter connecting to this
switch port doesn't support 2500BASEX so it never establishes a link.
If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works
fine.
state->an_enabled is true when phylink_helper_basex_speed() is called
even when configured with fixed-link. This causes it to change the
interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in
state->advertising which it always is on the first call because
phylink_create calls bitmap_fill(pl->supported,
__ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled
be true with MLO_AN_FIXED?
I've also noticed that phylink_validate (which ends up calling
phylink_helper_basex_speed) is called before phylink_parse_mode in
phylink_create. If phylink_helper_basex_speed changes the interface
mode this influences whether phylink_parse_mode (for MLO_AN_INBAND)
sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then
copied to pl->advertising). phylink_helper_basex_speed is then called
again (via phylink_validate) which uses advertising to decide how to
set interface. This seems like circular logic.
To make matters even more confusing I see that
mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether
to set state->speed to SPEED_1000 or SPEED_2500.
I've been thinking through how to get the desired behavior but I'm not
even sure what the desired behavior is. If you set phy-mode to
"1000base-x" in device-tree do you ever want interface to be set to
PHY_INTERFACE_MODE_2500BASEX? If so just for MLO_AN_INBAND or also for
ML_AN_FIXED? Do we want phylink_validate called in phylink_create even
though it gets called anyway for MLO_AN_INBAND and ML_AN_FIXED later?
I think these are good and valid questions. If I had a good justification
for the current phylink behavior I would have answered them, but I don't.
Hi,
I hadn't responded earlier because I wanted to think about it more,
but then forgot about this email.
On Thu, Mar 25, 2021 at 11:36:26AM -0500, George McCollister wrote:
When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x
(it also supports 2500base-x) in device-tree I find that
phylink_helper_basex_speed() changes interface to
PHY_INTERFACE_MODE_2500BASEX.
If both 2500base-X and 1000base-X are reported as being advertised,
then yes, it will. This is to support SFPs that can operate in either
mode. The key thing here is that both speeds are being advertised
and we're in either 2500base-X or 1000base-X mode.
This gives userspace a way to switch between those two supported modes
on the SFP.
The Ethernet adapter connecting to this
switch port doesn't support 2500BASEX so it never establishes a link.
You mean the remote side only supports 1000base-X?
If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works
fine.
state->an_enabled is true when phylink_helper_basex_speed() is called
even when configured with fixed-link. This causes it to change the
interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in
state->advertising which it always is on the first call because
phylink_create calls bitmap_fill(pl->supported,
__ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled
be true with MLO_AN_FIXED?
Historically, it has been (by the original fixed-phy implementation)
and I don't wish to change that as that would be a user-visible
change. Turning off state->an_enabled will make the interface depend
on state->speed instead.
I've also noticed that phylink_validate (which ends up calling
phylink_helper_basex_speed) is called before phylink_parse_mode in
phylink_create. If phylink_helper_basex_speed changes the interface
mode this influences whether phylink_parse_mode (for MLO_AN_INBAND)
sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then
copied to pl->advertising). phylink_helper_basex_speed is then called
again (via phylink_validate) which uses advertising to decide how to
set interface. This seems like circular logic.
I'm wondering if we should postpone the initial call to
phylink_validate() to just before the "pl->cur_link_an_mode =
pl->cfg_link_an_mode;" in there, and only if we're still in MLO_AN_PHY
mode - it will already have been called via the other methods. Would
that help to solve the problem?
To make matters even more confusing I see that
mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether
to set state->speed to SPEED_1000 or SPEED_2500.
There is no real report from the hardware to indicate the speed -
2500base-X looks like 1000base-X except for the different interface
mode.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Apr 1, 2021 at 5:33 PM Russell King - ARM Linux admin
[off-list ref] wrote:
Hi,
I hadn't responded earlier because I wanted to think about it more,
but then forgot about this email.
On Thu, Mar 25, 2021 at 11:36:26AM -0500, George McCollister wrote:
quoted
When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x
(it also supports 2500base-x) in device-tree I find that
phylink_helper_basex_speed() changes interface to
PHY_INTERFACE_MODE_2500BASEX.
If both 2500base-X and 1000base-X are reported as being advertised,
then yes, it will. This is to support SFPs that can operate in either
mode. The key thing here is that both speeds are being advertised
and we're in either 2500base-X or 1000base-X mode.
This gives userspace a way to switch between those two supported modes
on the SFP.
quoted
The Ethernet adapter connecting to this
switch port doesn't support 2500BASEX so it never establishes a link.
You mean the remote side only supports 1000base-X?
Yes, the Ethernet controller on the board that connects to port 9 on
the mv88e6390 doesn't support 2500base-X.
quoted
If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works
fine.
state->an_enabled is true when phylink_helper_basex_speed() is called
even when configured with fixed-link. This causes it to change the
interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in
state->advertising which it always is on the first call because
phylink_create calls bitmap_fill(pl->supported,
__ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled
be true with MLO_AN_FIXED?
Historically, it has been (by the original fixed-phy implementation)
and I don't wish to change that as that would be a user-visible
change. Turning off state->an_enabled will make the interface depend
on state->speed instead.
quoted
I've also noticed that phylink_validate (which ends up calling
phylink_helper_basex_speed) is called before phylink_parse_mode in
phylink_create. If phylink_helper_basex_speed changes the interface
mode this influences whether phylink_parse_mode (for MLO_AN_INBAND)
sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then
copied to pl->advertising). phylink_helper_basex_speed is then called
again (via phylink_validate) which uses advertising to decide how to
set interface. This seems like circular logic.
I'm wondering if we should postpone the initial call to
phylink_validate() to just before the "pl->cur_link_an_mode =
pl->cfg_link_an_mode;" in there, and only if we're still in MLO_AN_PHY
mode - it will already have been called via the other methods. Would
that help to solve the problem?
I had wondered if precisely this would fix it. I tested it and it
does. The question I can't answer is will it break anything else?
Should I send a patch?
quoted
To make matters even more confusing I see that
mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether
to set state->speed to SPEED_1000 or SPEED_2500.
There is no real report from the hardware to indicate the speed -
2500base-X looks like 1000base-X except for the different interface
mode.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!