Heads up: phylink changes for next merge window

Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]

Hi,

During the next round of development changes, I wish to make some
changes to phylink which will affect almost every user out there,
as it affects the interfaces to the MAC drivers.

The reason behind the change is to allow us to support situations
where the MAC is not closely coupled with its associated PCS, such
as is found in mvneta and mvpp2.  This is necessary to support
already existing hardware properly, such as Marvell DSA and Xilinx
AXI ethernet drivers, and there seems to be a growing need for this.

What I'm proposing to do is to move the MAC setup for the negotiated
speed, duplex and pause settings to the mac_link_up() method, out of
the existing mac_config() method.  I have already converted the
axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
know enough about the hardware to do the conversion myself.

The other benefit this has is that mac_config() called with
SPEED_UNKNOWN and DUPLEX_UNKNOWN has been a pain point for some;
mac_link_up() should never be called with speed or duplex set to these
- if it is, something is wrong with phylib or the mac_pcs_get_state()
implementation.  Hence, this pain point should be gone with this
proposal.

Once net-next has opened, and when the pre-requisit patches have been
reviewed, I'll post the series that includes these conversions.

The queued work can be found in my "phy" branch, viewable at:

  http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy

which I maintain as a re-sortable queue of pending patches; it is
based on v5.5 but I track most commits that impact on the pending
patches through a merge window.  Essentially, "net: phylink: allow
in-band AN for USXGMII" and preceding patches are in 5.6-rc1.

The initial plan will be to send:

- Batch 1:
   net: linkmode: make linkmode_test_bit() take const pointer
   net: add helpers to resolve negotiated flow control
   net: add linkmode helper for setting flow control advertisement
   net: phylink: remove pause mode ethtool setting for fixed links
   net: phylink: ensure manual flow control is selected appropriately
   net: phylink: use phylib resolved flow control modes
   net: phylink: resolve fixed link flow control
   net: phylink: allow ethtool -A to change flow control advertisement
   net: phylink: improve initial mac configuration
   net: phylink: clarify flow control settings in documentation
- Batch 2:
   net: phy: marvell10g: read copper results from CSSR1
   net: phy: marvell: don't interpret PHY status unless resolved
   net: phy: add resolved pause support
   net: phy: marvell*: add support for hw resolved pause modes
- Maybe some others from there, and then a series which starts the ball
  rollong on this:
   net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant
   net: mii: add linkmode_adv_to_mii_adv_x()
   net: phylink: propagate resolved link config via mac_link_up()
   net: dsa: propagate resolved link config via mac_link_up()
   net: mv88e6xxx: use resolved link config in mac_link_up()
   net: axienet: use resolved link config in mac_link_up()
   net: dpaa2-mac: use resolved link config in mac_link_up()
   net: macb: use resolved link config in mac_link_up()
   net: mvneta: use resolved link config in mac_link_up()
   net: mvpp2: use resolved link config in mac_link_up()

When all drivers have been converted, I propose to make phylink stop
calling mac_config() just before a link-up event for the MLO_AN_PHY
and MLO_AN_FIXED cases, unless a major reconfiguration of the MAC is
required (e.g. the PHY_INTERFACE_MODE has changed.)  Hence why all
existing drivers will need to be converted before this step.

I have a Coccinelle script that finds unconverted drivers, which I've
attached, runnable with:

spatch -D report --very-quiet --sp-file phylink-config.cocci drivers/net

The script will warn about the use of deprecated phylink_link_state
members, and error about use of ->link which has never been guaranteed
to be correct (there are code paths where it is definitely not correct,
particularly through the ethtool code paths, so it's use by drivers is
already buggy - bcm_sf2 seems to be the only user.)

I'm considering adding this coccinelle script to scripts/coccinelle so
that 0-day can catch any future inappropriate uses, although I would
like to eventually revise mac_config() sometime in the future so that it's
not possible for this to happen in the first place.

The remaining files are:

drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/bcm_sf2.c
drivers/net/dsa/mt7530.c
drivers/net/dsa/sja1105/sja1105_main.c
drivers/net/ethernet/mediatek/mtk_eth_soc.c
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Russell.

-- 
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

[-- Attachment #2: phylink-config.cocci --]
[-- Type: text/plain, Size: 1634 bytes --]

// spatch -D report --very-quiet --sp-file phylink-link.cocci \
//    drivers/net/ethernet net/dsa

virtual report

@ ops @
identifier I, func;
@@
(
  struct phylink_mac_ops I = { 
  	.mac_config = func,
  };
|
  struct dsa_switch_ops I = {
  	.phylink_mac_config = func,
  };
)

@ mac_config_link @
identifier ops.func, S;
position p;
@@
  func(..., const struct phylink_link_state *S)
  {
  ... when exists
    S->link @p
  ... when exists
  }

@ mac_config_spd_dpx @
identifier ops.func, S;
identifier member =~ "^(speed|duplex)$";
position p;
@@
  func(..., const struct phylink_link_state *S)
  {
  ... when exists
    S->member @p
  ... when exists
  }

@ mac_config_pause @
identifier ops.func, S;
identifier mask =~ "^MLO_PAUSE_(TX|RX|TXRX_MASK)$";
position p;
@@
  func(..., const struct phylink_link_state *S)
  {
  ... when exists
    S->pause & mask @p
  ... when exists
  }


@ script:python depends on report @
func << ops.func;
pos << mac_config_spd_dpx.p;
var << mac_config_spd_dpx.S;
memb << mac_config_spd_dpx.member;
@@
coccilib.report.print_report(pos[0],
	"WARNING: use of %s->%s in %s is deprecated" % (var, memb, func))

@ script:python depends on report @
func << ops.func;
pos << mac_config_pause.p;
var << mac_config_pause.S;
mask << mac_config_pause.mask;
@@
coccilib.report.print_report(pos[0],
	"WARNING: use of %s->pause %s flag in %s is deprecated" % (var, mask, func))

@ script:python depends on report @
func << ops.func;
pos << mac_config_link.p;
var << mac_config_link.S;
@@
coccilib.report.print_report(pos[0],
	"ERROR: use of %s->link in %s is unreliable and should never be used" % (var, func))

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

[Recipient list updated; removed addresses that bounce, added Ioana
Ciornei for dpaa2 and DSA issue mentioned below.]

On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> Hi,
> 
> During the next round of development changes, I wish to make some
> changes to phylink which will affect almost every user out there,
> as it affects the interfaces to the MAC drivers.
> 
> The reason behind the change is to allow us to support situations
> where the MAC is not closely coupled with its associated PCS, such
> as is found in mvneta and mvpp2.  This is necessary to support
> already existing hardware properly, such as Marvell DSA and Xilinx
> AXI ethernet drivers, and there seems to be a growing need for this.
> 
> What I'm proposing to do is to move the MAC setup for the negotiated
> speed, duplex and pause settings to the mac_link_up() method, out of
> the existing mac_config() method.  I have already converted the
> axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> know enough about the hardware to do the conversion myself.

I should also have pointed out that with mv88e6xxx, the patch
"net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
side-effect an issue that Andrew has mentioned, where inter-DSA ports
get configured down to 10baseHD speed.  This is by no means a true fix
for that problem - which is way deeper than this series can address.
The reason it fixes it is because we no longer set the speed/duplex
in mac_config() but set it in mac_link_up() - but mac_link_up() is
never called for CPU and DSA ports.

However, I think there may be another side-effect of that - any fixed
link declaration in DT may not be respected after this patch.

I believe the root of this goes back to this commit:

  commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
  Author: Ioana Ciornei <ioana.ciornei@nxp.com>
  Date:   Tue May 28 20:38:16 2019 +0300

  net: dsa: Use PHYLINK for the CPU/DSA ports

and, in the case of no fixed-link declaration, phylink has no idea what
the link parameters should be (and hence the initial bug, where
mac_config gets called with speed=0 duplex=0, which gets interpreted as
10baseHD.)  Moreover, as far as phylink is concerned, these links never
come up. Essentially, this commit was not fully tested with inter-DSA
links, and probably was never tested with phylink debugging enabled.

There is currently no fix for this, and it is not an easy problem to
resolve, irrespective of the patches I'm proposing.

There's another issue that this commit introduced - phylink has always
had the requirement that if it has been started (via phylink_start())
it _must_ be stopped prior to being destroyed.  The above commit does
not do this, which is another bug - and for this I do have a patch
elsewhere in my git tree.

> The other benefit this has is that mac_config() called with
> SPEED_UNKNOWN and DUPLEX_UNKNOWN has been a pain point for some;
> mac_link_up() should never be called with speed or duplex set to these
> - if it is, something is wrong with phylib or the mac_pcs_get_state()
> implementation.  Hence, this pain point should be gone with this
> proposal.
> 
> Once net-next has opened, and when the pre-requisit patches have been
> reviewed, I'll post the series that includes these conversions.
> 
> The queued work can be found in my "phy" branch, viewable at:
> 
>   http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy
> 
> which I maintain as a re-sortable queue of pending patches; it is
> based on v5.5 but I track most commits that impact on the pending
> patches through a merge window.  Essentially, "net: phylink: allow
> in-band AN for USXGMII" and preceding patches are in 5.6-rc1.
> 
> The initial plan will be to send:
> 
> - Batch 1:
>    net: linkmode: make linkmode_test_bit() take const pointer
>    net: add helpers to resolve negotiated flow control
>    net: add linkmode helper for setting flow control advertisement
>    net: phylink: remove pause mode ethtool setting for fixed links
>    net: phylink: ensure manual flow control is selected appropriately
>    net: phylink: use phylib resolved flow control modes
>    net: phylink: resolve fixed link flow control
>    net: phylink: allow ethtool -A to change flow control advertisement
>    net: phylink: improve initial mac configuration
>    net: phylink: clarify flow control settings in documentation
> - Batch 2:
>    net: phy: marvell10g: read copper results from CSSR1
>    net: phy: marvell: don't interpret PHY status unless resolved
>    net: phy: add resolved pause support
>    net: phy: marvell*: add support for hw resolved pause modes
> - Maybe some others from there, and then a series which starts the ball
>   rollong on this:
>    net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant
>    net: mii: add linkmode_adv_to_mii_adv_x()
>    net: phylink: propagate resolved link config via mac_link_up()
>    net: dsa: propagate resolved link config via mac_link_up()
>    net: mv88e6xxx: use resolved link config in mac_link_up()
>    net: axienet: use resolved link config in mac_link_up()
>    net: dpaa2-mac: use resolved link config in mac_link_up()
>    net: macb: use resolved link config in mac_link_up()
>    net: mvneta: use resolved link config in mac_link_up()
>    net: mvpp2: use resolved link config in mac_link_up()
> 
> When all drivers have been converted, I propose to make phylink stop
> calling mac_config() just before a link-up event for the MLO_AN_PHY
> and MLO_AN_FIXED cases, unless a major reconfiguration of the MAC is
> required (e.g. the PHY_INTERFACE_MODE has changed.)  Hence why all
> existing drivers will need to be converted before this step.
> 
> I have a Coccinelle script that finds unconverted drivers, which I've
> attached, runnable with:
> 
> spatch -D report --very-quiet --sp-file phylink-config.cocci drivers/net
> 
> The script will warn about the use of deprecated phylink_link_state
> members, and error about use of ->link which has never been guaranteed
> to be correct (there are code paths where it is definitely not correct,
> particularly through the ethtool code paths, so it's use by drivers is
> already buggy - bcm_sf2 seems to be the only user.)
> 
> I'm considering adding this coccinelle script to scripts/coccinelle so
> that 0-day can catch any future inappropriate uses, although I would
> like to eventually revise mac_config() sometime in the future so that it's
> not possible for this to happen in the first place.
> 
> The remaining files are:
> 
> drivers/net/dsa/b53/b53_common.c
> drivers/net/dsa/bcm_sf2.c
> drivers/net/dsa/mt7530.c
> drivers/net/dsa/sja1105/sja1105_main.c
> drivers/net/ethernet/mediatek/mtk_eth_soc.c
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> Russell.
> 
> -- 
> 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

> // spatch -D report --very-quiet --sp-file phylink-link.cocci \
> //    drivers/net/ethernet net/dsa
> 
> virtual report
> 
> @ ops @
> identifier I, func;
> @@
> (
>   struct phylink_mac_ops I = { 
>   	.mac_config = func,
>   };
> |
>   struct dsa_switch_ops I = {
>   	.phylink_mac_config = func,
>   };
> )
> 
> @ mac_config_link @
> identifier ops.func, S;
> position p;
> @@
>   func(..., const struct phylink_link_state *S)
>   {
>   ... when exists
>     S->link @p
>   ... when exists
>   }
> 
> @ mac_config_spd_dpx @
> identifier ops.func, S;
> identifier member =~ "^(speed|duplex)$";
> position p;
> @@
>   func(..., const struct phylink_link_state *S)
>   {
>   ... when exists
>     S->member @p
>   ... when exists
>   }
> 
> @ mac_config_pause @
> identifier ops.func, S;
> identifier mask =~ "^MLO_PAUSE_(TX|RX|TXRX_MASK)$";
> position p;
> @@
>   func(..., const struct phylink_link_state *S)
>   {
>   ... when exists
>     S->pause & mask @p
>   ... when exists
>   }
> 
> 
> @ script:python depends on report @
> func << ops.func;
> pos << mac_config_spd_dpx.p;
> var << mac_config_spd_dpx.S;
> memb << mac_config_spd_dpx.member;
> @@
> coccilib.report.print_report(pos[0],
> 	"WARNING: use of %s->%s in %s is deprecated" % (var, memb, func))
> 
> @ script:python depends on report @
> func << ops.func;
> pos << mac_config_pause.p;
> var << mac_config_pause.S;
> mask << mac_config_pause.mask;
> @@
> coccilib.report.print_report(pos[0],
> 	"WARNING: use of %s->pause %s flag in %s is deprecated" % (var, mask, func))
> 
> @ script:python depends on report @
> func << ops.func;
> pos << mac_config_link.p;
> var << mac_config_link.S;
> @@
> coccilib.report.print_report(pos[0],
> 	"ERROR: use of %s->link in %s is unreliable and should never be used" % (var, func))


-- 
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

Re: Heads up: phylink changes for next merge window

[Vladimir Oltean]

Hi Russell,

On Thu, 13 Feb 2020 at 16:46, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> [Recipient list updated; removed addresses that bounce, added Ioana
> Ciornei for dpaa2 and DSA issue mentioned below.]
>
> On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > Hi,
>
> I should also have pointed out that with mv88e6xxx, the patch
> "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> side-effect an issue that Andrew has mentioned, where inter-DSA ports
> get configured down to 10baseHD speed.  This is by no means a true fix
> for that problem - which is way deeper than this series can address.
> The reason it fixes it is because we no longer set the speed/duplex
> in mac_config() but set it in mac_link_up() - but mac_link_up() is
> never called for CPU and DSA ports.
>
> However, I think there may be another side-effect of that - any fixed
> link declaration in DT may not be respected after this patch.
>
> I believe the root of this goes back to this commit:
>
>   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
>   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
>   Date:   Tue May 28 20:38:16 2019 +0300
>
>   net: dsa: Use PHYLINK for the CPU/DSA ports
>
> and, in the case of no fixed-link declaration, phylink has no idea what
> the link parameters should be (and hence the initial bug, where
> mac_config gets called with speed=0 duplex=0, which gets interpreted as
> 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> come up. Essentially, this commit was not fully tested with inter-DSA
> links, and probably was never tested with phylink debugging enabled.
>
> There is currently no fix for this, and it is not an easy problem to
> resolve, irrespective of the patches I'm proposing.
>
> --
> 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

Correct me if I'm wrong, but if the lack of fixed-link specifier for
CPU and DSA ports was not a problem before, but has suddenly started
to become a problem with that patch, then simply reverting to the old
"legacy" logic from dsa_port_link_register_of() should restore the
behavior for those device tree blobs that don't specify an explicit
fixed-link?
In the longer term, can't we just require fixed-link specifiers for
non-netdev ports on new boards, keep the adjust_link code in DSA for
the legacy blobs (which works through some sort of magic), and be done
with it?

Regards,
-Vladimir

Re: Heads up: phylink changes for next merge window

[Andrew Lunn]

> Correct me if I'm wrong, but if the lack of fixed-link specifier for
> CPU and DSA ports was not a problem before, but has suddenly started
> to become a problem with that patch, then simply reverting to the old
> "legacy" logic from dsa_port_link_register_of() should restore the
> behavior for those device tree blobs that don't specify an explicit
> fixed-link?

DSA defines that the DSA driver should initialize CPU ports and DSA
ports to their maximum speed. You only need fixed-link properties when
you need to slow the port down, e.g. the SoC on the other end is
slower. That does not happen very often, so most boards don't have
fixed-link properties.

It just happens that most of the boards i test with, have a Fast
Ethernet SoC port, so do have fixed-link properties, and i missed the
issue for a long time.

	   Andrew

Re: Heads up: phylink changes for next merge window

[Vladimir Oltean]

Hi Andrew,

On Thu, 13 Feb 2020 at 18:06, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Correct me if I'm wrong, but if the lack of fixed-link specifier for
> > CPU and DSA ports was not a problem before, but has suddenly started
> > to become a problem with that patch, then simply reverting to the old
> > "legacy" logic from dsa_port_link_register_of() should restore the
> > behavior for those device tree blobs that don't specify an explicit
> > fixed-link?
>
> DSA defines that the DSA driver should initialize CPU ports and DSA
> ports to their maximum speed. You only need fixed-link properties when
> you need to slow the port down, e.g. the SoC on the other end is
> slower. That does not happen very often, so most boards don't have
> fixed-link properties.
>
> It just happens that most of the boards i test with, have a Fast
> Ethernet SoC port, so do have fixed-link properties, and i missed the
> issue for a long time.
>
>            Andrew

Grepping for "ethernet = " in arch/arm/boot/dts, I see that the blobs
without fixed-link aren't even in the majority here. There are plenty
of blobs that specify fixed-link even if running at full interface
speed. I wasn't even aware that "no fixed-link specifier" is even a
thing when working with Ioana on that patch.

-Vladimir

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Thu, Feb 13, 2020 at 05:57:36PM +0200, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Thu, 13 Feb 2020 at 16:46, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > [Recipient list updated; removed addresses that bounce, added Ioana
> > Ciornei for dpaa2 and DSA issue mentioned below.]
> >
> > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > Hi,
> >
> > I should also have pointed out that with mv88e6xxx, the patch
> > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > get configured down to 10baseHD speed.  This is by no means a true fix
> > for that problem - which is way deeper than this series can address.
> > The reason it fixes it is because we no longer set the speed/duplex
> > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > never called for CPU and DSA ports.
> >
> > However, I think there may be another side-effect of that - any fixed
> > link declaration in DT may not be respected after this patch.
> >
> > I believe the root of this goes back to this commit:
> >
> >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> >   Date:   Tue May 28 20:38:16 2019 +0300
> >
> >   net: dsa: Use PHYLINK for the CPU/DSA ports
> >
> > and, in the case of no fixed-link declaration, phylink has no idea what
> > the link parameters should be (and hence the initial bug, where
> > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > come up. Essentially, this commit was not fully tested with inter-DSA
> > links, and probably was never tested with phylink debugging enabled.
> >
> > There is currently no fix for this, and it is not an easy problem to
> > resolve, irrespective of the patches I'm proposing.
> >
> > --
> > 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
> 
> Correct me if I'm wrong, but if the lack of fixed-link specifier for
> CPU and DSA ports was not a problem before, but has suddenly started
> to become a problem with that patch, then simply reverting to the old
> "legacy" logic from dsa_port_link_register_of() should restore the
> behavior for those device tree blobs that don't specify an explicit
> fixed-link?

That's a good idea, but presumably the change was done in order to
be able to support something, so reverting it may also cause
regressions.  For example, the mv88e6xxx driver has no adjust_link
support anymore as of:

commit 7fb5a711545d7d25fe9726a9ad277474dd83bd06
Author: Hubert Feurstein <h.feurstein@gmail.com>
Date:   Wed Jul 31 17:42:39 2019 +0200

    net: dsa: mv88e6xxx: drop adjust_link to enabled phylink

Since DSA has been whinging about adjust_link being set, I suspect
this is not the only DSA driver to have dropped adjust_link support.

The problem has basically been spotted way too late, and there's
too much on top for a simple revert to work.

> In the longer term, can't we just require fixed-link specifiers for
> non-netdev ports on new boards, keep the adjust_link code in DSA for
> the legacy blobs (which works through some sort of magic), and be done
> with it?

That would be a nice idea - making them behave exactly the same way
as any other network connection, but that is something for the DSA
maintainers to decide.

-- 
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

Re: Heads up: phylink changes for next merge window

[Andrew Lunn]

On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> [Recipient list updated; removed addresses that bounce, added Ioana
> Ciornei for dpaa2 and DSA issue mentioned below.]
> 
> On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > Hi,
> > 
> > During the next round of development changes, I wish to make some
> > changes to phylink which will affect almost every user out there,
> > as it affects the interfaces to the MAC drivers.
> > 
> > The reason behind the change is to allow us to support situations
> > where the MAC is not closely coupled with its associated PCS, such
> > as is found in mvneta and mvpp2.  This is necessary to support
> > already existing hardware properly, such as Marvell DSA and Xilinx
> > AXI ethernet drivers, and there seems to be a growing need for this.
> > 
> > What I'm proposing to do is to move the MAC setup for the negotiated
> > speed, duplex and pause settings to the mac_link_up() method, out of
> > the existing mac_config() method.  I have already converted the
> > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > know enough about the hardware to do the conversion myself.
> 
> I should also have pointed out that with mv88e6xxx, the patch
> "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> side-effect an issue that Andrew has mentioned, where inter-DSA ports
> get configured down to 10baseHD speed.  This is by no means a true fix
> for that problem - which is way deeper than this series can address.
> The reason it fixes it is because we no longer set the speed/duplex
> in mac_config() but set it in mac_link_up() - but mac_link_up() is
> never called for CPU and DSA ports.
> 
> However, I think there may be another side-effect of that - any fixed
> link declaration in DT may not be respected after this patch.
> 
> I believe the root of this goes back to this commit:
> 
>   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
>   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
>   Date:   Tue May 28 20:38:16 2019 +0300
> 
>   net: dsa: Use PHYLINK for the CPU/DSA ports
> 
> and, in the case of no fixed-link declaration, phylink has no idea what
> the link parameters should be (and hence the initial bug, where
> mac_config gets called with speed=0 duplex=0, which gets interpreted as
> 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> come up. Essentially, this commit was not fully tested with inter-DSA
> links, and probably was never tested with phylink debugging enabled.
> 
> There is currently no fix for this, and it is not an easy problem to
> resolve, irrespective of the patches I'm proposing.

Hi Russell

I've been playing around with this a bit. I have a partial fix:

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 9b54e5a76297..dc4da4dc44f5 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 int dsa_port_link_register_of(struct dsa_port *dp)
 {
        struct dsa_switch *ds = dp->ds;
+       struct device_node *phy_np;
 
-       if (!ds->ops->adjust_link)
-               return dsa_port_phylink_register(dp);
+       if (!ds->ops->adjust_link) {
+               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+               if (of_phy_is_fixed_link(dp->dn) || phy_np)
+                       return dsa_port_phylink_register(dp);
+               return 0;
+       }
 
        dev_warn(ds->dev,
                 "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
@@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
 {
        struct dsa_switch *ds = dp->ds;
 
-       if (!ds->ops->adjust_link) {
+       if (!ds->ops->adjust_link && dp->pl) {
                rtnl_lock();
                phylink_disconnect_phy(dp->pl);
                rtnl_unlock();
                phylink_destroy(dp->pl);
+               dp->pl = NULL;
                return;
        }

So basically only instantiate phylink if there is a fixed-link
property, or a phy-handle.

What i think is still broken is if there is a phy-mode property, and
nothing else. e.g. to set RGMII delays. I think that will get ignored.

	Andrew

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Thu, Feb 13, 2020 at 05:00:04PM +0100, Andrew Lunn wrote:
> On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> > [Recipient list updated; removed addresses that bounce, added Ioana
> > Ciornei for dpaa2 and DSA issue mentioned below.]
> > 
> > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > Hi,
> > > 
> > > During the next round of development changes, I wish to make some
> > > changes to phylink which will affect almost every user out there,
> > > as it affects the interfaces to the MAC drivers.
> > > 
> > > The reason behind the change is to allow us to support situations
> > > where the MAC is not closely coupled with its associated PCS, such
> > > as is found in mvneta and mvpp2.  This is necessary to support
> > > already existing hardware properly, such as Marvell DSA and Xilinx
> > > AXI ethernet drivers, and there seems to be a growing need for this.
> > > 
> > > What I'm proposing to do is to move the MAC setup for the negotiated
> > > speed, duplex and pause settings to the mac_link_up() method, out of
> > > the existing mac_config() method.  I have already converted the
> > > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > > know enough about the hardware to do the conversion myself.
> > 
> > I should also have pointed out that with mv88e6xxx, the patch
> > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > get configured down to 10baseHD speed.  This is by no means a true fix
> > for that problem - which is way deeper than this series can address.
> > The reason it fixes it is because we no longer set the speed/duplex
> > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > never called for CPU and DSA ports.
> > 
> > However, I think there may be another side-effect of that - any fixed
> > link declaration in DT may not be respected after this patch.
> > 
> > I believe the root of this goes back to this commit:
> > 
> >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> >   Date:   Tue May 28 20:38:16 2019 +0300
> > 
> >   net: dsa: Use PHYLINK for the CPU/DSA ports
> > 
> > and, in the case of no fixed-link declaration, phylink has no idea what
> > the link parameters should be (and hence the initial bug, where
> > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > come up. Essentially, this commit was not fully tested with inter-DSA
> > links, and probably was never tested with phylink debugging enabled.
> > 
> > There is currently no fix for this, and it is not an easy problem to
> > resolve, irrespective of the patches I'm proposing.
> 
> Hi Russell
> 
> I've been playing around with this a bit. I have a partial fix:
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 9b54e5a76297..dc4da4dc44f5 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
>  int dsa_port_link_register_of(struct dsa_port *dp)
>  {
>         struct dsa_switch *ds = dp->ds;
> +       struct device_node *phy_np;
>  
> -       if (!ds->ops->adjust_link)
> -               return dsa_port_phylink_register(dp);
> +       if (!ds->ops->adjust_link) {
> +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> +                       return dsa_port_phylink_register(dp);
> +               return 0;
> +       }
>  
>         dev_warn(ds->dev,
>                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> @@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
>  {
>         struct dsa_switch *ds = dp->ds;
>  
> -       if (!ds->ops->adjust_link) {
> +       if (!ds->ops->adjust_link && dp->pl) {
>                 rtnl_lock();
>                 phylink_disconnect_phy(dp->pl);
>                 rtnl_unlock();
>                 phylink_destroy(dp->pl);
> +               dp->pl = NULL;
>                 return;
>         }
> 
> So basically only instantiate phylink if there is a fixed-link
> property, or a phy-handle.
> 
> What i think is still broken is if there is a phy-mode property, and
> nothing else. e.g. to set RGMII delays. I think that will get ignored.

Can you please verify that mac_link_up() gets called for these if
there is a fixed-link property or phy-handle?

Also, there is another way around this, which is for phylink_create()
to callback through the mac_ops to request the default configuration.
That could be plumbed down through the various DSA layers such that
the old "max speed / max link" business could be setup.  However,
that brings with it a new problem: if we default to a fixed-link, then
attempting to connect a phy later will be ignored.  However, deferring
the default create-time configuration setup to phylink_start() would
work around it, but brings with it a bit more complexity.

-- 
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

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Thu, Feb 13, 2020 at 05:16:02PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 13, 2020 at 05:00:04PM +0100, Andrew Lunn wrote:
> > On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> > > [Recipient list updated; removed addresses that bounce, added Ioana
> > > Ciornei for dpaa2 and DSA issue mentioned below.]
> > > 
> > > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > > Hi,
> > > > 
> > > > During the next round of development changes, I wish to make some
> > > > changes to phylink which will affect almost every user out there,
> > > > as it affects the interfaces to the MAC drivers.
> > > > 
> > > > The reason behind the change is to allow us to support situations
> > > > where the MAC is not closely coupled with its associated PCS, such
> > > > as is found in mvneta and mvpp2.  This is necessary to support
> > > > already existing hardware properly, such as Marvell DSA and Xilinx
> > > > AXI ethernet drivers, and there seems to be a growing need for this.
> > > > 
> > > > What I'm proposing to do is to move the MAC setup for the negotiated
> > > > speed, duplex and pause settings to the mac_link_up() method, out of
> > > > the existing mac_config() method.  I have already converted the
> > > > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > > > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > > > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > > > know enough about the hardware to do the conversion myself.
> > > 
> > > I should also have pointed out that with mv88e6xxx, the patch
> > > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > > get configured down to 10baseHD speed.  This is by no means a true fix
> > > for that problem - which is way deeper than this series can address.
> > > The reason it fixes it is because we no longer set the speed/duplex
> > > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > > never called for CPU and DSA ports.
> > > 
> > > However, I think there may be another side-effect of that - any fixed
> > > link declaration in DT may not be respected after this patch.
> > > 
> > > I believe the root of this goes back to this commit:
> > > 
> > >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> > >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> > >   Date:   Tue May 28 20:38:16 2019 +0300
> > > 
> > >   net: dsa: Use PHYLINK for the CPU/DSA ports
> > > 
> > > and, in the case of no fixed-link declaration, phylink has no idea what
> > > the link parameters should be (and hence the initial bug, where
> > > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > > come up. Essentially, this commit was not fully tested with inter-DSA
> > > links, and probably was never tested with phylink debugging enabled.
> > > 
> > > There is currently no fix for this, and it is not an easy problem to
> > > resolve, irrespective of the patches I'm proposing.
> > 
> > Hi Russell
> > 
> > I've been playing around with this a bit. I have a partial fix:
> > 
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index 9b54e5a76297..dc4da4dc44f5 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
> >  int dsa_port_link_register_of(struct dsa_port *dp)
> >  {
> >         struct dsa_switch *ds = dp->ds;
> > +       struct device_node *phy_np;
> >  
> > -       if (!ds->ops->adjust_link)
> > -               return dsa_port_phylink_register(dp);
> > +       if (!ds->ops->adjust_link) {
> > +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> > +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> > +                       return dsa_port_phylink_register(dp);
> > +               return 0;
> > +       }
> >  
> >         dev_warn(ds->dev,
> >                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> > @@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
> >  {
> >         struct dsa_switch *ds = dp->ds;
> >  
> > -       if (!ds->ops->adjust_link) {
> > +       if (!ds->ops->adjust_link && dp->pl) {
> >                 rtnl_lock();
> >                 phylink_disconnect_phy(dp->pl);
> >                 rtnl_unlock();
> >                 phylink_destroy(dp->pl);
> > +               dp->pl = NULL;
> >                 return;
> >         }
> > 
> > So basically only instantiate phylink if there is a fixed-link
> > property, or a phy-handle.
> > 
> > What i think is still broken is if there is a phy-mode property, and
> > nothing else. e.g. to set RGMII delays. I think that will get ignored.
> 
> Can you please verify that mac_link_up() gets called for these if
> there is a fixed-link property or phy-handle?
> 
> Also, there is another way around this, which is for phylink_create()
> to callback through the mac_ops to request the default configuration.
> That could be plumbed down through the various DSA layers such that
> the old "max speed / max link" business could be setup.  However,
> that brings with it a new problem: if we default to a fixed-link, then
> attempting to connect a phy later will be ignored.  However, deferring
> the default create-time configuration setup to phylink_start() would
> work around it, but brings with it a bit more complexity.

Hmm.

Andrew, it is possible that what I have in the ZII branch does end
up fixing this problem by accident without requiring any further
code changes in DSA.

If a node has no fixed-link nor PHY, phylink defaults to MLO_AN_PHY
mode - and without a PHY, phylink will never believe that the link
has come up.  Hence, mac_link_up() will never be called.

If a node has a fixed-link property, then phylink will parse it,
and by default, fixed-links without GPIOs or a callback function
will come up at initialisation.  Hence, we get a mac_link_up()
call with the fixed-link parameters.

If a node has a PHY, then DSA will instruct phylink to connect to
it, and it will behave as any other PHY-attached MAC, calling
mac_link_up() at the appropriate time.

The mv88e6xxx driver by default configures CPU and DSA ports to
maximum speed, which is what we get if mac_link_up() is never
called.  On the other hand, if there's a fixed-link or PHY, we'll
call mac_link_up() to program those parameters.

So, provided all drivers are converted to program the speed/duplex
etc in mac_link_up(), we might not have a problem at all - and
that would be an additional motivation for everyone to update to
the changes I've proposed!

What I don't like - if the above is correct - is that this is
merely coincidental.

Now, while that sounds great, something's niggling me about it.
If the link is forced up by the DSA driver initialisation defaulting
the speed to maximum speed, but phylink believes the link to be down,
and then we get a mac_link_up() call, with the patches as they stand
we end up changing the port configuration while the link is up, which
I'm sure I've read shouldn't be done in some of the DSA switch
functional specs.  Hmm.

-- 
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

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Thu, Feb 13, 2020 at 05:45:00PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 13, 2020 at 05:16:02PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Feb 13, 2020 at 05:00:04PM +0100, Andrew Lunn wrote:
> > > On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> > > > [Recipient list updated; removed addresses that bounce, added Ioana
> > > > Ciornei for dpaa2 and DSA issue mentioned below.]
> > > > 
> > > > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > > > Hi,
> > > > > 
> > > > > During the next round of development changes, I wish to make some
> > > > > changes to phylink which will affect almost every user out there,
> > > > > as it affects the interfaces to the MAC drivers.
> > > > > 
> > > > > The reason behind the change is to allow us to support situations
> > > > > where the MAC is not closely coupled with its associated PCS, such
> > > > > as is found in mvneta and mvpp2.  This is necessary to support
> > > > > already existing hardware properly, such as Marvell DSA and Xilinx
> > > > > AXI ethernet drivers, and there seems to be a growing need for this.
> > > > > 
> > > > > What I'm proposing to do is to move the MAC setup for the negotiated
> > > > > speed, duplex and pause settings to the mac_link_up() method, out of
> > > > > the existing mac_config() method.  I have already converted the
> > > > > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > > > > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > > > > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > > > > know enough about the hardware to do the conversion myself.
> > > > 
> > > > I should also have pointed out that with mv88e6xxx, the patch
> > > > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > > > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > > > get configured down to 10baseHD speed.  This is by no means a true fix
> > > > for that problem - which is way deeper than this series can address.
> > > > The reason it fixes it is because we no longer set the speed/duplex
> > > > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > > > never called for CPU and DSA ports.
> > > > 
> > > > However, I think there may be another side-effect of that - any fixed
> > > > link declaration in DT may not be respected after this patch.
> > > > 
> > > > I believe the root of this goes back to this commit:
> > > > 
> > > >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> > > >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > >   Date:   Tue May 28 20:38:16 2019 +0300
> > > > 
> > > >   net: dsa: Use PHYLINK for the CPU/DSA ports
> > > > 
> > > > and, in the case of no fixed-link declaration, phylink has no idea what
> > > > the link parameters should be (and hence the initial bug, where
> > > > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > > > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > > > come up. Essentially, this commit was not fully tested with inter-DSA
> > > > links, and probably was never tested with phylink debugging enabled.
> > > > 
> > > > There is currently no fix for this, and it is not an easy problem to
> > > > resolve, irrespective of the patches I'm proposing.
> > > 
> > > Hi Russell
> > > 
> > > I've been playing around with this a bit. I have a partial fix:
> > > 
> > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > index 9b54e5a76297..dc4da4dc44f5 100644
> > > --- a/net/dsa/port.c
> > > +++ b/net/dsa/port.c
> > > @@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
> > >  int dsa_port_link_register_of(struct dsa_port *dp)
> > >  {
> > >         struct dsa_switch *ds = dp->ds;
> > > +       struct device_node *phy_np;
> > >  
> > > -       if (!ds->ops->adjust_link)
> > > -               return dsa_port_phylink_register(dp);
> > > +       if (!ds->ops->adjust_link) {
> > > +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> > > +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> > > +                       return dsa_port_phylink_register(dp);
> > > +               return 0;
> > > +       }
> > >  
> > >         dev_warn(ds->dev,
> > >                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> > > @@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
> > >  {
> > >         struct dsa_switch *ds = dp->ds;
> > >  
> > > -       if (!ds->ops->adjust_link) {
> > > +       if (!ds->ops->adjust_link && dp->pl) {
> > >                 rtnl_lock();
> > >                 phylink_disconnect_phy(dp->pl);
> > >                 rtnl_unlock();
> > >                 phylink_destroy(dp->pl);
> > > +               dp->pl = NULL;
> > >                 return;
> > >         }
> > > 
> > > So basically only instantiate phylink if there is a fixed-link
> > > property, or a phy-handle.
> > > 
> > > What i think is still broken is if there is a phy-mode property, and
> > > nothing else. e.g. to set RGMII delays. I think that will get ignored.
> > 
> > Can you please verify that mac_link_up() gets called for these if
> > there is a fixed-link property or phy-handle?
> > 
> > Also, there is another way around this, which is for phylink_create()
> > to callback through the mac_ops to request the default configuration.
> > That could be plumbed down through the various DSA layers such that
> > the old "max speed / max link" business could be setup.  However,
> > that brings with it a new problem: if we default to a fixed-link, then
> > attempting to connect a phy later will be ignored.  However, deferring
> > the default create-time configuration setup to phylink_start() would
> > work around it, but brings with it a bit more complexity.
> 
> Hmm.
> 
> Andrew, it is possible that what I have in the ZII branch does end
> up fixing this problem by accident without requiring any further
> code changes in DSA.
> 
> If a node has no fixed-link nor PHY, phylink defaults to MLO_AN_PHY
> mode - and without a PHY, phylink will never believe that the link
> has come up.  Hence, mac_link_up() will never be called.
> 
> If a node has a fixed-link property, then phylink will parse it,
> and by default, fixed-links without GPIOs or a callback function
> will come up at initialisation.  Hence, we get a mac_link_up()
> call with the fixed-link parameters.
> 
> If a node has a PHY, then DSA will instruct phylink to connect to
> it, and it will behave as any other PHY-attached MAC, calling
> mac_link_up() at the appropriate time.
> 
> The mv88e6xxx driver by default configures CPU and DSA ports to
> maximum speed, which is what we get if mac_link_up() is never
> called.  On the other hand, if there's a fixed-link or PHY, we'll
> call mac_link_up() to program those parameters.
> 
> So, provided all drivers are converted to program the speed/duplex
> etc in mac_link_up(), we might not have a problem at all - and
> that would be an additional motivation for everyone to update to
> the changes I've proposed!
> 
> What I don't like - if the above is correct - is that this is
> merely coincidental.
> 
> Now, while that sounds great, something's niggling me about it.
> If the link is forced up by the DSA driver initialisation defaulting
> the speed to maximum speed, but phylink believes the link to be down,
> and then we get a mac_link_up() call, with the patches as they stand
> we end up changing the port configuration while the link is up, which
> I'm sure I've read shouldn't be done in some of the DSA switch
> functional specs.  Hmm.

Hi Andrew,

Having added some additional debug into mv88e6xxx, it seems that it's
not working as I thought it would be on the ZII dev rev C, but does on
the ZII dev rev B.

On the ZII dev rev C:

[   23.082294] mv88e6085 0.1:00: p0: Force link down
[   23.095527] mv88e6085 0.1:00: p0: Force link up
[   23.116490] mv88e6085 0.1:00: p1: Force link down
[   23.122065] mv88e6085 0.1:00: p1: Unforce link down
[   23.137928] mv88e6085 0.1:00: p2: Force link down
[   23.143487] mv88e6085 0.1:00: p2: Unforce link down
[   23.159416] mv88e6085 0.1:00: p3: Force link down
[   23.164986] mv88e6085 0.1:00: p3: Unforce link down
[   23.181277] mv88e6085 0.1:00: p4: Force link down
[   23.186925] mv88e6085 0.1:00: p4: Unforce link down
[   23.202864] mv88e6085 0.1:00: p9: Force link down
[   23.208434] mv88e6085 0.1:00: p9: Unforce link down
[   23.227698] mv88e6085 0.1:00: p10: Force link down
[   23.233372] mv88e6085 0.1:00: p10: Force link up
...
[   24.294145] mv88e6085 0.1:00: configuring for fixed/ link mode
[   24.300435] mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=0 an=1
[   24.300474] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_config()
[   24.302905] mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=1 an=1
[   24.303035] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_config()
[   24.303756] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_link_up()
[   24.303797] mv88e6085 0.1:00: Link is Up - 100Mbps/Full - flow control off
...
[   24.436179] mv88e6085 0.1:00: configuring for phy/xaui link mode
[   24.442578] mv88e6085 0.1:00: phylink_mac_config: mode=phy/xaui/Unsupported (update phy-core.c)/Half adv=000,00000000,00000000 pause=00 link=0 an=0
[   24.442615] mv88e6085 0.1:00: p10: dsa_port_phylink_mac_config()

This is with:

        if (reg & MV88E6XXX_PORT_MAC_CTL_FORCE_LINK &&
            reg & MV88E6XXX_PORT_MAC_CTL_LINK_UP)
                dev_err(chip->dev, "p%d: forcing link speed/duplex with port up\n", port);

added in to mv88e6xxx_port_set_speed_duplex(), which never fires.  Yet,
we can see from the above that port 0 and port 10 were forced up
earlier.

Hence, phylink isn't playing a part in setting the speed and duplex on
this port, yet port 0's mac control is 0x203e and port 10's is 0x203f.
They are both forcing link up, forced full duplex, and port 0 is forced
to 1G and port 10 to 10G.  So, the problem you mentioned does still
exist.

This comes down to my comment in the code (in mac_link_up and
mac_link_down):

        /* Internal PHYs propagate their configuration directly to the MAC.
         * External PHYs depend on whether the PPU is enabled for this port.
         * FIXME: we should be using the PPU enable state here. What about
         * an automedia port?
         */
        if (!mv88e6xxx_phy_is_internal(ds, port) && ops->port_set_link) {

and suggests this is the wrong test to be using here.  I'm not sure
if using the PPU PHY detect bit is correct either, but looking at
what I have here, it seems to be _more_ correct than not.

For the 88e6390, mv88e6xxx_phy_is_internal() will be true for all ports
0 through 8, and for the 88e6352, ports 0 through 4.  So, these two
methods will only force settings for port 9 and 10 for the 88e6390 and
port 5 and 6 on the 88e6352.

The reasoning is, if the PHY detect bit is set, the PPU should be
polling the attached PHY and automatically updating the MAC to reflect
the PHY status.  This seems great...

On the ZII dev rev C, we have the following port status values:
- port 0 = 0xe04
- port 1 through 8 = 0x100f
- port 9 = 0x49
- port 10 = 0xcf4c

On the ZII dev rev B, port 4 (which is one of the optical ports) has a
value of 0xde09, despite being linked to the on-board serdes.  It seems
that the PPU on the 88e6352 automatically propagates the status from the
serdes there.

So, it looks to me like using the PHY detect bit is the right solution,
we just need access to it at this mid-layer...

On the ZII dev rev B, I see what I expect:

mv88e6085 0.1:00: p0: Force link down
mv88e6085 0.1:00: p0: Unforce link down
mv88e6085 0.1:00: p1: Force link down
mv88e6085 0.1:00: p1: Unforce link down
mv88e6085 0.1:00: p2: Force link down
mv88e6085 0.1:00: p2: Unforce link down
mv88e6085 0.1:00: p4: Force link down
mv88e6085 0.1:00: p4: Unforce link down
mv88e6085 0.1:00: p5: Force link down
mv88e6085 0.1:00: p5: Force link up
mv88e6085 0.1:00: p6: Force link down
mv88e6085 0.1:00: p6: Force link up
...
mv88e6085 0.1:00: configuring for fixed/rgmii-txid link mode
mv88e6085 0.1:00: phylink_mac_config: mode=fixed/rgmii-txid/1Gbps/Full adv=000,00000000,00002220 pause=00 link=0 an=1
mv88e6085 0.1:00: p5: dsa_port_phylink_mac_config()
mv88e6085 0.1:00: phylink_mac_config: mode=fixed/rgmii-txid/1Gbps/Full adv=000,00000000,00002220 pause=00 link=1 an=1
mv88e6085 0.1:00: p5: dsa_port_phylink_mac_config()
mv88e6085 0.1:00: p5: dsa_port_phylink_mac_link_up()
mv88e6085 0.1:00: p5: forcing link speed/duplex with port up
mv88e6085 0.1:00: p5: Force link up
mv88e6085 0.1:00: Link is Up - 1Gbps/Full - flow control off
mv88e6085 0.1:00: configuring for fixed/ link mode
mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=0 an=1
mv88e6085 0.1:00: p6: dsa_port_phylink_mac_config()
mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=1 an=1
mv88e6085 0.1:00: p6: dsa_port_phylink_mac_config()
mv88e6085 0.1:00: p6: dsa_port_phylink_mac_link_up()
mv88e6085 0.1:00: p6: forcing link speed/duplex with port up
mv88e6085 0.1:00: p6: Force link up
mv88e6085 0.1:00: Link is Up - 100Mbps/Full - flow control off

which shows that we're changing the speed and duplex with the port up
as I suspected in my reply yesterday, which the functional specs say we
shouldn't do.

-- 
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

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Fri, Feb 14, 2020 at 10:41:48AM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 13, 2020 at 05:45:00PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Feb 13, 2020 at 05:16:02PM +0000, Russell King - ARM Linux admin wrote:
> > > On Thu, Feb 13, 2020 at 05:00:04PM +0100, Andrew Lunn wrote:
> > > > On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> > > > > [Recipient list updated; removed addresses that bounce, added Ioana
> > > > > Ciornei for dpaa2 and DSA issue mentioned below.]
> > > > > 
> > > > > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > During the next round of development changes, I wish to make some
> > > > > > changes to phylink which will affect almost every user out there,
> > > > > > as it affects the interfaces to the MAC drivers.
> > > > > > 
> > > > > > The reason behind the change is to allow us to support situations
> > > > > > where the MAC is not closely coupled with its associated PCS, such
> > > > > > as is found in mvneta and mvpp2.  This is necessary to support
> > > > > > already existing hardware properly, such as Marvell DSA and Xilinx
> > > > > > AXI ethernet drivers, and there seems to be a growing need for this.
> > > > > > 
> > > > > > What I'm proposing to do is to move the MAC setup for the negotiated
> > > > > > speed, duplex and pause settings to the mac_link_up() method, out of
> > > > > > the existing mac_config() method.  I have already converted the
> > > > > > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > > > > > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > > > > > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > > > > > know enough about the hardware to do the conversion myself.
> > > > > 
> > > > > I should also have pointed out that with mv88e6xxx, the patch
> > > > > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > > > > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > > > > get configured down to 10baseHD speed.  This is by no means a true fix
> > > > > for that problem - which is way deeper than this series can address.
> > > > > The reason it fixes it is because we no longer set the speed/duplex
> > > > > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > > > > never called for CPU and DSA ports.
> > > > > 
> > > > > However, I think there may be another side-effect of that - any fixed
> > > > > link declaration in DT may not be respected after this patch.
> > > > > 
> > > > > I believe the root of this goes back to this commit:
> > > > > 
> > > > >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> > > > >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > > >   Date:   Tue May 28 20:38:16 2019 +0300
> > > > > 
> > > > >   net: dsa: Use PHYLINK for the CPU/DSA ports
> > > > > 
> > > > > and, in the case of no fixed-link declaration, phylink has no idea what
> > > > > the link parameters should be (and hence the initial bug, where
> > > > > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > > > > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > > > > come up. Essentially, this commit was not fully tested with inter-DSA
> > > > > links, and probably was never tested with phylink debugging enabled.
> > > > > 
> > > > > There is currently no fix for this, and it is not an easy problem to
> > > > > resolve, irrespective of the patches I'm proposing.
> > > > 
> > > > Hi Russell
> > > > 
> > > > I've been playing around with this a bit. I have a partial fix:
> > > > 
> > > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > > index 9b54e5a76297..dc4da4dc44f5 100644
> > > > --- a/net/dsa/port.c
> > > > +++ b/net/dsa/port.c
> > > > @@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
> > > >  int dsa_port_link_register_of(struct dsa_port *dp)
> > > >  {
> > > >         struct dsa_switch *ds = dp->ds;
> > > > +       struct device_node *phy_np;
> > > >  
> > > > -       if (!ds->ops->adjust_link)
> > > > -               return dsa_port_phylink_register(dp);
> > > > +       if (!ds->ops->adjust_link) {
> > > > +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> > > > +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> > > > +                       return dsa_port_phylink_register(dp);
> > > > +               return 0;
> > > > +       }
> > > >  
> > > >         dev_warn(ds->dev,
> > > >                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> > > > @@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
> > > >  {
> > > >         struct dsa_switch *ds = dp->ds;
> > > >  
> > > > -       if (!ds->ops->adjust_link) {
> > > > +       if (!ds->ops->adjust_link && dp->pl) {
> > > >                 rtnl_lock();
> > > >                 phylink_disconnect_phy(dp->pl);
> > > >                 rtnl_unlock();
> > > >                 phylink_destroy(dp->pl);
> > > > +               dp->pl = NULL;
> > > >                 return;
> > > >         }
> > > > 
> > > > So basically only instantiate phylink if there is a fixed-link
> > > > property, or a phy-handle.
> > > > 
> > > > What i think is still broken is if there is a phy-mode property, and
> > > > nothing else. e.g. to set RGMII delays. I think that will get ignored.
> > > 
> > > Can you please verify that mac_link_up() gets called for these if
> > > there is a fixed-link property or phy-handle?
> > > 
> > > Also, there is another way around this, which is for phylink_create()
> > > to callback through the mac_ops to request the default configuration.
> > > That could be plumbed down through the various DSA layers such that
> > > the old "max speed / max link" business could be setup.  However,
> > > that brings with it a new problem: if we default to a fixed-link, then
> > > attempting to connect a phy later will be ignored.  However, deferring
> > > the default create-time configuration setup to phylink_start() would
> > > work around it, but brings with it a bit more complexity.
> > 
> > Hmm.
> > 
> > Andrew, it is possible that what I have in the ZII branch does end
> > up fixing this problem by accident without requiring any further
> > code changes in DSA.
> > 
> > If a node has no fixed-link nor PHY, phylink defaults to MLO_AN_PHY
> > mode - and without a PHY, phylink will never believe that the link
> > has come up.  Hence, mac_link_up() will never be called.
> > 
> > If a node has a fixed-link property, then phylink will parse it,
> > and by default, fixed-links without GPIOs or a callback function
> > will come up at initialisation.  Hence, we get a mac_link_up()
> > call with the fixed-link parameters.
> > 
> > If a node has a PHY, then DSA will instruct phylink to connect to
> > it, and it will behave as any other PHY-attached MAC, calling
> > mac_link_up() at the appropriate time.
> > 
> > The mv88e6xxx driver by default configures CPU and DSA ports to
> > maximum speed, which is what we get if mac_link_up() is never
> > called.  On the other hand, if there's a fixed-link or PHY, we'll
> > call mac_link_up() to program those parameters.
> > 
> > So, provided all drivers are converted to program the speed/duplex
> > etc in mac_link_up(), we might not have a problem at all - and
> > that would be an additional motivation for everyone to update to
> > the changes I've proposed!
> > 
> > What I don't like - if the above is correct - is that this is
> > merely coincidental.
> > 
> > Now, while that sounds great, something's niggling me about it.
> > If the link is forced up by the DSA driver initialisation defaulting
> > the speed to maximum speed, but phylink believes the link to be down,
> > and then we get a mac_link_up() call, with the patches as they stand
> > we end up changing the port configuration while the link is up, which
> > I'm sure I've read shouldn't be done in some of the DSA switch
> > functional specs.  Hmm.
> 
> Hi Andrew,
> 
> Having added some additional debug into mv88e6xxx, it seems that it's
> not working as I thought it would be on the ZII dev rev C, but does on
> the ZII dev rev B.
> 
> On the ZII dev rev C:
> 
> [   23.082294] mv88e6085 0.1:00: p0: Force link down
> [   23.095527] mv88e6085 0.1:00: p0: Force link up
> [   23.116490] mv88e6085 0.1:00: p1: Force link down
> [   23.122065] mv88e6085 0.1:00: p1: Unforce link down
> [   23.137928] mv88e6085 0.1:00: p2: Force link down
> [   23.143487] mv88e6085 0.1:00: p2: Unforce link down
> [   23.159416] mv88e6085 0.1:00: p3: Force link down
> [   23.164986] mv88e6085 0.1:00: p3: Unforce link down
> [   23.181277] mv88e6085 0.1:00: p4: Force link down
> [   23.186925] mv88e6085 0.1:00: p4: Unforce link down
> [   23.202864] mv88e6085 0.1:00: p9: Force link down
> [   23.208434] mv88e6085 0.1:00: p9: Unforce link down
> [   23.227698] mv88e6085 0.1:00: p10: Force link down
> [   23.233372] mv88e6085 0.1:00: p10: Force link up
> ...
> [   24.294145] mv88e6085 0.1:00: configuring for fixed/ link mode
> [   24.300435] mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=0 an=1
> [   24.300474] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_config()
> [   24.302905] mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=1 an=1
> [   24.303035] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_config()
> [   24.303756] mv88e6085 0.1:00: p0: dsa_port_phylink_mac_link_up()
> [   24.303797] mv88e6085 0.1:00: Link is Up - 100Mbps/Full - flow control off
> ...
> [   24.436179] mv88e6085 0.1:00: configuring for phy/xaui link mode
> [   24.442578] mv88e6085 0.1:00: phylink_mac_config: mode=phy/xaui/Unsupported (update phy-core.c)/Half adv=000,00000000,00000000 pause=00 link=0 an=0
> [   24.442615] mv88e6085 0.1:00: p10: dsa_port_phylink_mac_config()
> 
> This is with:
> 
>         if (reg & MV88E6XXX_PORT_MAC_CTL_FORCE_LINK &&
>             reg & MV88E6XXX_PORT_MAC_CTL_LINK_UP)
>                 dev_err(chip->dev, "p%d: forcing link speed/duplex with port up\n", port);
> 
> added in to mv88e6xxx_port_set_speed_duplex(), which never fires.  Yet,
> we can see from the above that port 0 and port 10 were forced up
> earlier.
> 
> Hence, phylink isn't playing a part in setting the speed and duplex on
> this port, yet port 0's mac control is 0x203e and port 10's is 0x203f.
> They are both forcing link up, forced full duplex, and port 0 is forced
> to 1G and port 10 to 10G.  So, the problem you mentioned does still
> exist.
> 
> This comes down to my comment in the code (in mac_link_up and
> mac_link_down):
> 
>         /* Internal PHYs propagate their configuration directly to the MAC.
>          * External PHYs depend on whether the PPU is enabled for this port.
>          * FIXME: we should be using the PPU enable state here. What about
>          * an automedia port?
>          */
>         if (!mv88e6xxx_phy_is_internal(ds, port) && ops->port_set_link) {
> 
> and suggests this is the wrong test to be using here.  I'm not sure
> if using the PPU PHY detect bit is correct either, but looking at
> what I have here, it seems to be _more_ correct than not.
> 
> For the 88e6390, mv88e6xxx_phy_is_internal() will be true for all ports
> 0 through 8, and for the 88e6352, ports 0 through 4.  So, these two
> methods will only force settings for port 9 and 10 for the 88e6390 and
> port 5 and 6 on the 88e6352.
> 
> The reasoning is, if the PHY detect bit is set, the PPU should be
> polling the attached PHY and automatically updating the MAC to reflect
> the PHY status.  This seems great...
> 
> On the ZII dev rev C, we have the following port status values:
> - port 0 = 0xe04
> - port 1 through 8 = 0x100f
> - port 9 = 0x49
> - port 10 = 0xcf4c
> 
> On the ZII dev rev B, port 4 (which is one of the optical ports) has a
> value of 0xde09, despite being linked to the on-board serdes.  It seems
> that the PPU on the 88e6352 automatically propagates the status from the
> serdes there.
> 
> So, it looks to me like using the PHY detect bit is the right solution,
> we just need access to it at this mid-layer...
> 
> On the ZII dev rev B, I see what I expect:
> 
> mv88e6085 0.1:00: p0: Force link down
> mv88e6085 0.1:00: p0: Unforce link down
> mv88e6085 0.1:00: p1: Force link down
> mv88e6085 0.1:00: p1: Unforce link down
> mv88e6085 0.1:00: p2: Force link down
> mv88e6085 0.1:00: p2: Unforce link down
> mv88e6085 0.1:00: p4: Force link down
> mv88e6085 0.1:00: p4: Unforce link down
> mv88e6085 0.1:00: p5: Force link down
> mv88e6085 0.1:00: p5: Force link up
> mv88e6085 0.1:00: p6: Force link down
> mv88e6085 0.1:00: p6: Force link up
> ...
> mv88e6085 0.1:00: configuring for fixed/rgmii-txid link mode
> mv88e6085 0.1:00: phylink_mac_config: mode=fixed/rgmii-txid/1Gbps/Full adv=000,00000000,00002220 pause=00 link=0 an=1
> mv88e6085 0.1:00: p5: dsa_port_phylink_mac_config()
> mv88e6085 0.1:00: phylink_mac_config: mode=fixed/rgmii-txid/1Gbps/Full adv=000,00000000,00002220 pause=00 link=1 an=1
> mv88e6085 0.1:00: p5: dsa_port_phylink_mac_config()
> mv88e6085 0.1:00: p5: dsa_port_phylink_mac_link_up()
> mv88e6085 0.1:00: p5: forcing link speed/duplex with port up
> mv88e6085 0.1:00: p5: Force link up
> mv88e6085 0.1:00: Link is Up - 1Gbps/Full - flow control off
> mv88e6085 0.1:00: configuring for fixed/ link mode
> mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=0 an=1
> mv88e6085 0.1:00: p6: dsa_port_phylink_mac_config()
> mv88e6085 0.1:00: phylink_mac_config: mode=fixed//100Mbps/Full adv=000,00000000,00002208 pause=00 link=1 an=1
> mv88e6085 0.1:00: p6: dsa_port_phylink_mac_config()
> mv88e6085 0.1:00: p6: dsa_port_phylink_mac_link_up()
> mv88e6085 0.1:00: p6: forcing link speed/duplex with port up
> mv88e6085 0.1:00: p6: Force link up
> mv88e6085 0.1:00: Link is Up - 100Mbps/Full - flow control off
> 
> which shows that we're changing the speed and duplex with the port up
> as I suspected in my reply yesterday, which the functional specs say we
> shouldn't do.

I've now pushed out the debugging changes, and a change to use the
PHY_DETECT bit instead in my zii branch.

Note that the "changing with link up" issue affects several calls
in addition to the port_set_speed_duplex() methods, including
port_set_rgmii_delay(), and likely the port_set_cmode() methods.

I'm also wondering whether forcing the link down, updating the
configuration, and then restoring the link status gains us anything.
When we do that, we could be forcing the link down in the middle of
a packet - and we won't wait for that packet to finish before making
the change.  If we're not forcing the link down and merely change
the configuration, how is that any different - we still end up with
a corrupted packet.  Maybe the forcing-link-down event marks any
ingressing packet bad, but what about egressing packets...  Not sure.

-- 
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

Re: Heads up: phylink changes for next merge window

[Andrew Lunn]

> The reasoning is, if the PHY detect bit is set, the PPU should be
> polling the attached PHY and automatically updating the MAC to reflect
> the PHY status.  This seems great...
> 
> On the ZII dev rev C, we have the following port status values:
> - port 0 = 0xe04
> - port 1 through 8 = 0x100f
> - port 9 = 0x49
> - port 10 = 0xcf4c
> 
> On the ZII dev rev B, port 4 (which is one of the optical ports) has a
> value of 0xde09, despite being linked to the on-board serdes.  It seems
> that the PPU on the 88e6352 automatically propagates the status from the
> serdes there.
> 
> So, it looks to me like using the PHY detect bit is the right solution,
> we just need access to it at this mid-layer...

Hi Russell

We need to be careful of the PPU. I'm assuming it uses MDIO to access
the PHY registers. We have code which allows the PHY page to the
changed, e.g. hwmon to get the temperature sensors, and i will soon
have code for cable diagnostics. We don't want the PPU reading some
temperature sensor registers and configuring the MAC based on that.

For cases not using a PHY, e.g. the SERDES on the 6352, it might be
safe to use the PPU.

     Andrew

Re: Heads up: phylink changes for next merge window

[Russell King - ARM Linux admin]

On Fri, Feb 14, 2020 at 04:08:26PM +0100, Andrew Lunn wrote:
> > The reasoning is, if the PHY detect bit is set, the PPU should be
> > polling the attached PHY and automatically updating the MAC to reflect
> > the PHY status.  This seems great...
> > 
> > On the ZII dev rev C, we have the following port status values:
> > - port 0 = 0xe04
> > - port 1 through 8 = 0x100f
> > - port 9 = 0x49
> > - port 10 = 0xcf4c
> > 
> > On the ZII dev rev B, port 4 (which is one of the optical ports) has a
> > value of 0xde09, despite being linked to the on-board serdes.  It seems
> > that the PPU on the 88e6352 automatically propagates the status from the
> > serdes there.
> > 
> > So, it looks to me like using the PHY detect bit is the right solution,
> > we just need access to it at this mid-layer...
> 
> Hi Russell
> 
> We need to be careful of the PPU. I'm assuming it uses MDIO to access
> the PHY registers. We have code which allows the PHY page to the
> changed, e.g. hwmon to get the temperature sensors, and i will soon
> have code for cable diagnostics. We don't want the PPU reading some
> temperature sensor registers and configuring the MAC based on that.
> 
> For cases not using a PHY, e.g. the SERDES on the 6352, it might be
> safe to use the PPU.

Bear in mind that the PPU has been used for some time.

However, what I read in some of the functional specs is the built-in
PHYs use a more "direct" method to communicate their negotiated results
to the MAC.  Whether that also applies to the 6352 Serdes or not, I
don't know, but as port 4 will automatically switch between the
built-in copper PHY and Serdes depending on which link comes up first.

It's interesting, however, reading some of the functional specs, where
some say that the PPU must be globally disabled before access is
allowed to the MDIO bus, others the bit for globally disabling the PPU
is "reserved".

That all said, using the PPU PHY_DETECT bit seems to me to be the right
solution - if the chip is polling the PHYs itself, we want to unforce
the speed, duplex and pause, otherwise we need to force them to the
link parameters.  If we need to disable the PPU for a port, then
disabling PHY_DETECT seems like the right answer.

-- 
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