mvneta: SGMII fixed-link not so fixed

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

I've been bringing up the mainline kernel on a new board, which has
an Marvell SoC with a mvneta interface connected to a Marvell DSA
switch.  The DSA switch is a 88E6176.

In the DT block for the interface, I specify:

	ethernet at ... {
	        phy-mode = "sgmii";
		status = "okay";

		fixed-link {
			speed = <1000>;
			full-duplex;
		};
	};

However, this doesn't work without patching mvneta to disable the
autonegotiation and forcing the link up:

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 62e48bc0cb23..e1698731e429 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
 		val |= MVNETA_GMAC_INBAND_AN_ENABLE |
 		       MVNETA_GMAC_AN_SPEED_EN |
 		       MVNETA_GMAC_AN_DUPLEX_EN;
+		/* We appear to need the interface forced for DSA switches */
+		val &= ~(MVNETA_GMAC_AN_DUPLEX_EN |
+			 MVNETA_GMAC_AN_SPEED_EN);
+		val |= MVNETA_GMAC_FORCE_LINK_PASS;
 		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
 		val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER);
 		val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;

As Marvell likes to keep all their documentation secret, it's very hard
to know what's going on, and why this would be necessary.  However, when
running the board under a kernel built from Marvell's code base, it
sets this register in a similar way.

Since the link is definitely a fixed-link operating at 1G, full duplex,
with no autonegotiation, I think using the fixed-link in DT is correct,
but the mvneta driver is wrong to abuse fixed-link to mean "SGMII but
with in-band autonegotiation".

The other issue I've seen is that even with the fixed-link settings, I
have seen via ethtool that the link is reported as 10mbit - because
mvneta_fixed_link_update() changes the settings on the fixed link.  So,
fixed-link doesn't seem to be quite as fixed as "fixed" says it should
be.

Shouldn't SGMII (which is always gigabit) be treated as gigabit with
in-band negotiation when phy-mode = "sgmii" but no fixed-link, but a
real fixed speed, other parameters and forced up when phy-mode = "sgmii"
and there is a fixed link.  It seems to me that there's been a design
mistake here, and my fear is that as we seem to have had this mistake
in the tree since April, it's now almost impossible to support this
setup without breaking DT compatibility.

However, it could be that the switch is misconfigured, and some register
bit somewhere isn't set to indicate that it should provide in-band
signalling on its SGMII interface.  I've trawled the net looking at
various bits of Marvell code for driving their switches, and I see
nothing which would cause them to enable in-band signalling, but maybe
I'm missing something.

Here's the switch registers - the mvneta is connected to port 5:

    GLOBAL GLOBAL2   0    1    2    3    4    5    6
 0:  c844       0  100f 100f 100f 100f 100f  e09  e07
 1:     0       0     3    3    3    3    3   3e    3
 2:     0    ffff     0    0    0    0    0    0    0
 3:     0    ffff  1761 1761 1761 1761 1761 1761 1761
 4:  6000     258   430  430  430  430  430 373f  430
 5:     0      ff     0    0    0    0    0    0    0
 6:     0    1f0f    7e   7d   7b   77   6f 505f   3f
 7:     0    707f     0    0    0    0    0    0    0
 8:     0    7800  2080 2080 2080 2080 2080 2080 2080
 9:     0    1600     1    1    1    1    1    1    1
 a:   148       0     0    0    0    0    0    0    0
 b:  6000    1000     1    2    4    8   10   20   40
 c:     0      7f     0    0    0    0    0    0    0
 d:     0     502     0    0    0    0    0    0    0
 e:     0       0     0    0    0    0    0    0    0
 f:     0     f00  dada dada dada dada dada dada dada
10:     0       0     0    0    0    0    0    0    0
11:     0       0     0    0    0    0    0    0    0
12:  5555       0     0    0    0    0    0    0    0
13:  5555       0     0    0    0    0    0    0    0
14:  aaaa     400     0    0    0    0    0    0    0
15:  aaaa       0     0    0    0    0    0    0    0
16:  ffff       0    33   33   33   33   33   33    0
17:  ffff       0     0    0    0    0    0    0    0
18:  fa41    1884  3210 3210 3210 3210 3210 3210 3210
19:     0     1e1  7654 7654 7654 7654 7654 7654 7654
1a:  5550       0     0    0    0    0    0    0    0
1b:   1ff    f869  8000 8000 8000 8000 8000 8000 8000
1c:     0       0     0    0    0    0    0    0    0
1d:  1000       0     0    0    0    0    0    0    0
1e:     0       0     0    0    0    0    0    0    0
1f:     0       0     0    0    0    0    0    0    0

The DSA switch is configured in DT with:

	dsa at 0 {
		compatible = "marvell,dsa";
		dsa,ethernet = <&eth1>;
		dsa,mii-bus = <&mdio>;
		#address-cells = <2>;
		#size-cells = <0>;
		switch at 0 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <4 0>;
...
			port at 5 {
				reg = <5>;
				label = "cpu";
			};
		};
	};

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

14.09.2015 13:32, Russell King - ARM Linux ?????:
> I've been bringing up the mainline kernel on a new board, which has
> an Marvell SoC with a mvneta interface connected to a Marvell DSA
> switch.  The DSA switch is a 88E6176.
>
> In the DT block for the interface, I specify:
>
> 	ethernet at ... {
> 	        phy-mode = "sgmii";
> 		status = "okay";
>
> 		fixed-link {
> 			speed = <1000>;
> 			full-duplex;
> 		};
> 	};
>
> However, this doesn't work without patching mvneta to disable the
> autonegotiation and forcing the link up:
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 62e48bc0cb23..e1698731e429 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>   		val |= MVNETA_GMAC_INBAND_AN_ENABLE |
>   		       MVNETA_GMAC_AN_SPEED_EN |
>   		       MVNETA_GMAC_AN_DUPLEX_EN;
> +		/* We appear to need the interface forced for DSA switches */
> +		val &= ~(MVNETA_GMAC_AN_DUPLEX_EN |
> +			 MVNETA_GMAC_AN_SPEED_EN);
> +		val |= MVNETA_GMAC_FORCE_LINK_PASS;
>   		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
>   		val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER);
>   		val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;
Hello Russell, just to make sure, aren't you missing this by any chance:
https://lkml.org/lkml/2015/7/20/710

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Mon, Sep 14, 2015 at 02:06:13PM +0300, Stas Sergeev wrote:
> 14.09.2015 13:32, Russell King - ARM Linux ?????:
> >I've been bringing up the mainline kernel on a new board, which has
> >an Marvell SoC with a mvneta interface connected to a Marvell DSA
> >switch.  The DSA switch is a 88E6176.
> >
> >In the DT block for the interface, I specify:
> >
> >	ethernet at ... {
> >	        phy-mode = "sgmii";
> >		status = "okay";
> >
> >		fixed-link {
> >			speed = <1000>;
> >			full-duplex;
> >		};
> >	};
> >
> >However, this doesn't work without patching mvneta to disable the
> >autonegotiation and forcing the link up:
> >
> >diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> >index 62e48bc0cb23..e1698731e429 100644
> >--- a/drivers/net/ethernet/marvell/mvneta.c
> >+++ b/drivers/net/ethernet/marvell/mvneta.c
> >@@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
> >  		val |= MVNETA_GMAC_INBAND_AN_ENABLE |
> >  		       MVNETA_GMAC_AN_SPEED_EN |
> >  		       MVNETA_GMAC_AN_DUPLEX_EN;
> >+		/* We appear to need the interface forced for DSA switches */
> >+		val &= ~(MVNETA_GMAC_AN_DUPLEX_EN |
> >+			 MVNETA_GMAC_AN_SPEED_EN);
> >+		val |= MVNETA_GMAC_FORCE_LINK_PASS;
> >  		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
> >  		val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER);
> >  		val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;
> Hello Russell, just to make sure, aren't you missing this by any chance:
> https://lkml.org/lkml/2015/7/20/710

Thanks, I think that will solve it.  I have to wonder why that patch
(f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's
billed as a regression that occurred in the previous merge window, and
given that it was sent in July, and we're now in September.  As it
wasn't in v4.2, it looks like it should be a stable candidate.

David, any objections to having the stable guys pick this regression
fix up, if not already done so?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[David Miller]

From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Mon, 14 Sep 2015 12:42:09 +0100

> Thanks, I think that will solve it.  I have to wonder why that patch
> (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's
> billed as a regression that occurred in the previous merge window, and
> given that it was sent in July, and we're now in September.  As it
> wasn't in v4.2, it looks like it should be a stable candidate.

The series had a whole bunch of non bug fixes in it and we were in
the final phases of 4.2, in which case I defer to applying patches
to net-next only unless I'm told otherwise.

It's up the the patch/series author to let me know that an important
regression fix is hidden in there, but they should have submitted
it seperately from the rest in that kind of situation anyways.

> David, any objections to having the stable guys pick this regression
> fix up, if not already done so?

More than this patch is needed, the one before it (3/4) instantiates
the necessary property in the DT, for example.

I can queue up the whole series for -stable if you want.

mvneta: SGMII fixed-link not so fixed

[Florian Fainelli]

On 17/09/15 15:12, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Mon, 14 Sep 2015 12:42:09 +0100
> 
>> Thanks, I think that will solve it.  I have to wonder why that patch
>> (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's
>> billed as a regression that occurred in the previous merge window, and
>> given that it was sent in July, and we're now in September.  As it
>> wasn't in v4.2, it looks like it should be a stable candidate.
> 
> The series had a whole bunch of non bug fixes in it and we were in
> the final phases of 4.2, in which case I defer to applying patches
> to net-next only unless I'm told otherwise.

To your defense, Staas and I kept arguing for a while, slowing the
entire process down until we agreed on a proper solution, the submission
was targeting your 'net' tree, but I did not realize until now that
these got applied to 'net-next'.

> 
> It's up the the patch/series author to let me know that an important
> regression fix is hidden in there, but they should have submitted
> it seperately from the rest in that kind of situation anyways.
> 
>> David, any objections to having the stable guys pick this regression
>> fix up, if not already done so?
> 
> More than this patch is needed, the one before it (3/4) instantiates
> the necessary property in the DT, for example.
> 
> I can queue up the whole series for -stable if you want.

I think this would be a good thing, mvneta-based platforms are fairly
popular.

Thank you!
-- 
Florian

mvneta: SGMII fixed-link not so fixed

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 17 Sep 2015 16:02:41 -0700

> On 17/09/15 15:12, David Miller wrote:
>> I can queue up the whole series for -stable if you want.
> 
> I think this would be a good thing, mvneta-based platforms are fairly
> popular.

Done.

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Thu, Sep 17, 2015 at 03:12:47PM -0700, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Mon, 14 Sep 2015 12:42:09 +0100
> 
> > Thanks, I think that will solve it.  I have to wonder why that patch
> > (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's
> > billed as a regression that occurred in the previous merge window, and
> > given that it was sent in July, and we're now in September.  As it
> > wasn't in v4.2, it looks like it should be a stable candidate.
> 
> The series had a whole bunch of non bug fixes in it and we were in
> the final phases of 4.2, in which case I defer to applying patches
> to net-next only unless I'm told otherwise.
> 
> It's up the the patch/series author to let me know that an important
> regression fix is hidden in there, but they should have submitted
> it seperately from the rest in that kind of situation anyways.
> 
> > David, any objections to having the stable guys pick this regression
> > fix up, if not already done so?
> 
> More than this patch is needed, the one before it (3/4) instantiates
> the necessary property in the DT, for example.
> 
> I can queue up the whole series for -stable if you want.

Sorry in advance for this rambling reply...

I'm not entirely certain that'd be a good idea at the moment, for a
number of reasons, which are coming up because I'm looking at getting
a SFP cage supported with mvneta hardware.

1. Serdes gigabit ethernet links have two operating modes for in-band
   "negotiation" - Cisco SGMII format, and 1000base-X format.  Both use
   exactly the same encoding on the wire, the only differences between
   them are the contents of a 16-bit configuration word and how each
   end of the link handles that.  SFP can use either format depending
   on the module hot-plugged in - fiber modules will normally use
   1000base-X, but copper modules which contain a PHY may use either
   SGMII or 1000base-X.  (Fiber modules for 100baseFX will probably
   use SGMII though.)

   The issue there is two-fold: that the new DT property just says it's
   "in-band" or "auto" but there's no way to specify the format of the
   in-band configuration.

2. With Serdes, the PCS layer of the PHY, which does the autonegotiation,
   is moved to the MAC.  When connected to a SGMII PHY, the PHY may report
   over the Serdes connection the Cisco SGMII configuration word which
   instructs the MAC how to configure itself.  It's not "negotiation" by
   any means, but "phy telling the MAC how to configure itself" word.

   Having "in-band" enabled pretty much requires the use of the "fixed-link"
   property, which seems to be a total hack around the PCS layer being in
   the MAC - the "fixed-link" phy is no longer fixed, but is used as a
   means to convey the negotiated results from the MAC side PCS to the
   software-emulated PHY, only to have them pop back out into the MAC
   driver.

   If you specify "in-band" without a "fixed-link" but have other MACs
   making use of the fixed-link support, all hell breaks loose, because
   mvneta will call the fixed-link update function with the real phy
   with the in-band results, and this can hit a fixed-link PHY for some
   other network adapter.  The fixed-link PHY code makes no attempt to
   validate that the phy_device passed in really is a fixed-link phy
   and not a MDIO phy.

3. Having DT specify a fixed-link with parameters along with in-band
   negotiation results in the fixed-link parameters being ignored.
   This means if a fixed-link DT declaration specifies a speed, that
   declaration will be ignored.  What I'm basically saying is that:

		phy-mode = "sgmii";
		fixed-link {
			speed = <1000>;
		};

   specifies a fixed-speed serdes link at 1000mbps, but:

		phy-mode = "sgmii";
		managed = "in-band-status";
		fixed-link {
			speed = <1000>;
		};

   does not fix the speed at all.  _But_ using the in-band status
   property fundamentally requires this for mvneta to behave correctly:

		phy-mode = "sgmii";
		managed = "in-band-status";
		fixed-link {
		};

   with _no_ phy node.

4. Going back to the SFP problem, the link is only up when the SFP
   module pins indicate that there's no transmitter fault, no loss of
   signal _and_ the PCS layer at the MAC indicates that it has completed
   negotiation.  This pretty much rules out trying to emulate a SFP cage
   as a software-based PHY.  I've code right now doing exactly that, and
   it results in netif_carrier_on() being called far too early.

What I don't know is how many generations of the mvneta hardware have
support for both serdes modes, but what I'm basically saying is that
the solution we now have seems to be somewhat lacking - maybe it should
have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
ability to add additional modes later.

The other point I'm making above is that I'm forming the opinion that
the existing PHY layer isn't flexible enough for supporting SFP, and I
need some way to represent at least part of the autonegotiation at the
MAC level without involving the PHY level - especially when considering
that a real PHY might be inside the SFP cage which can be talked to
over I2C.

This is the problem I'm presently grappling with, and it's taking lots
of thought right now.  I'm aware of other drivers in the kernel which
support SFP, each using their own implementations to support that.


Lastly, while looking at this, I've a small stack of patches for the PHY
code resolving some of the issues I've mentioned above, and fixing broken
reference counting and mdio bus module removal issues:

 phy: fixed-phy: properly validate phy in fixed_phy_update_state()
 net: fix phy refcounting in a bunch of drivers
 of_mdio: fix MDIO phy device refcounting
 phy: add proper phy struct device refcounting
 phy: fix mdiobus module safety
 phy: fix of_mdio_find_bus() device refcount leak

I hope to be able to send those out in the next few days - they have
nothing to do with SFP itself but are the results of looking through the
PHY code.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Florian Fainelli]

On 17/09/15 16:14, Russell King - ARM Linux wrote:

[snip]

>    with _no_ phy node.
> 
> 4. Going back to the SFP problem, the link is only up when the SFP
>    module pins indicate that there's no transmitter fault, no loss of
>    signal _and_ the PCS layer at the MAC indicates that it has completed
>    negotiation.  This pretty much rules out trying to emulate a SFP cage
>    as a software-based PHY.  I've code right now doing exactly that, and
>    it results in netif_carrier_on() being called far too early.

Andrew recently added support for having the fixed PHY emulated PHY poll
a GPIO to determine link status. If this information comes differently
(e.g: via MMIO/MAC registers) in your case, I guess we could either
extend the fixed PHY to support that scheme, or just open-code the
carrier state change in the mvneta driver.

> 
> What I don't know is how many generations of the mvneta hardware have
> support for both serdes modes, but what I'm basically saying is that
> the solution we now have seems to be somewhat lacking - maybe it should
> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
> ability to add additional modes later.

Staas and I had quite some discussions about this topic, and I think I
changed my mind at least once about his initial proposal, but I seem to
recall that I suggested making the auto-negotiation property something
more flexible than it currently is (or maybe Staas did, and my mind
plays tricks on me).

> 
> The other point I'm making above is that I'm forming the opinion that
> the existing PHY layer isn't flexible enough for supporting SFP, and I
> need some way to represent at least part of the autonegotiation at the
> MAC level without involving the PHY level - especially when considering
> that a real PHY might be inside the SFP cage which can be talked to
> over I2C.

This is a fair conclusion, the PHY library is really designed to support
well 10/100/1000 PHYs, anything else, including 10Gbits PHY are not that
well supported. Anything that is not MDIO is not that well supported,
and the fixed PHY has its limitations.

It is unclear to me how much of the PHY library: state machine, link
management, device abstraction, and ethtool interface is going to be
useful if you have a SFP instead of a MDIO-connected PHY.

> 
> This is the problem I'm presently grappling with, and it's taking lots
> of thought right now.  I'm aware of other drivers in the kernel which
> support SFP, each using their own implementations to support that.
> 
> 
> Lastly, while looking at this, I've a small stack of patches for the PHY
> code resolving some of the issues I've mentioned above, and fixing broken
> reference counting and mdio bus module removal issues:
> 
>  phy: fixed-phy: properly validate phy in fixed_phy_update_state()
>  net: fix phy refcounting in a bunch of drivers
>  of_mdio: fix MDIO phy device refcounting
>  phy: add proper phy struct device refcounting
>  phy: fix mdiobus module safety
>  phy: fix of_mdio_find_bus() device refcount leak
> 
> I hope to be able to send those out in the next few days - they have
> nothing to do with SFP itself but are the results of looking through the
> PHY code.
> 
-- 
Florian

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Thu, Sep 17, 2015 at 04:36:44PM -0700, Florian Fainelli wrote:
> On 17/09/15 16:14, Russell King - ARM Linux wrote:
> 
> [snip]
> 
> >    with _no_ phy node.
> > 
> > 4. Going back to the SFP problem, the link is only up when the SFP
> >    module pins indicate that there's no transmitter fault, no loss of
> >    signal _and_ the PCS layer at the MAC indicates that it has completed
> >    negotiation.  This pretty much rules out trying to emulate a SFP cage
> >    as a software-based PHY.  I've code right now doing exactly that, and
> >    it results in netif_carrier_on() being called far too early.
> 
> Andrew recently added support for having the fixed PHY emulated PHY poll
> a GPIO to determine link status. If this information comes differently
> (e.g: via MMIO/MAC registers) in your case, I guess we could either
> extend the fixed PHY to support that scheme, or just open-code the
> carrier state change in the mvneta driver.

It's not that simple.  The "is the carrier up" is the logical and of:
- no TX fault (at SFP level)
- no RX loss (at SFP level)
- Serdes says link is up (at MAC level)

If TX fault is asserted, then there is error recovery that can be done,
so it's not as simple as the MAC monitoring that GPIO - unless we want
to do what's already been done in other MAC drivers, and integrate the
SFP driver logic into the MAC driver rather than doing the same thing
and having it as a library.

> This is a fair conclusion, the PHY library is really designed to support
> well 10/100/1000 PHYs, anything else, including 10Gbits PHY are not that
> well supported. Anything that is not MDIO is not that well supported,
> and the fixed PHY has its limitations.
> 
> It is unclear to me how much of the PHY library: state machine, link
> management, device abstraction, and ethtool interface is going to be
> useful if you have a SFP instead of a MDIO-connected PHY.

Well, it gets even _more_ fun when you consider these, which are all
a very real possibility thanks to the hardware designs out there:

MAC ---SGMII---> Phy ---> Copper
MAC ---1000baseX---> Phy ---> 1000baseT Copper
MAC ---SGMII---> SFP ---> Phy ---> Copper
MAC ---SGMII---> SFP ---> Phy ---> Optical
MAC ---1000baseX---> SFP ---> Optical

                    .---1000base-X---> SFP ---> Optical
MAC ---SGMII---> Phy
                    `---> Copper

                    .---SGMII---> SFP ---> Phy ---> Optical
MAC ---SGMII---> Phy
                    `---> Copper

                    .---1000base-X---> SFP ---> Phy ---> 1000baseT Copper
MAC ---SGMII---> Phy
                    `---> Copper

                    .---SGMII---> SFP ---> Phy ---> Copper
MAC ---SGMII---> Phy
                    `---> Copper

and when you realise that you can talk to all the PHYs in those trees.
In the last four cases, the first level phy decides between the copper
connection and the "other" connection based on programmable priority.

At the moment, I only have two optical SFP modules, which fall into
the 5th setup above - you can't use SGMII with "dumb" optical modules
as SGMII's control word is an instruction from the PHY to the MAC
about the link properties, it isn't any sort of negotiation.

What I'm worried about is having to redo the DT properties, and if
the change makes it into stable trees, then we have to support the
existing design as well as whatever solution to this problem.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 02:14, Russell King - ARM Linux ?????:
> 3. Having DT specify a fixed-link with parameters along with in-band
>    negotiation results in the fixed-link parameters being ignored.
>    This means if a fixed-link DT declaration specifies a speed, that
>    declaration will be ignored.  What I'm basically saying is that:
> 
> 		phy-mode = "sgmii";
> 		fixed-link {
> 			speed = <1000>;
> 		};
> 
>    specifies a fixed-speed serdes link at 1000mbps, but:
> 
> 		phy-mode = "sgmii";
> 		managed = "in-band-status";
> 		fixed-link {
> 			speed = <1000>;
> 		};
> 
>    does not fix the speed at all.
I think the fixed-link DT parsing should be moved from of_mdio.c
to fixed-phy.c. Then fixed-phy will be able to back up the initial
values and use them whenever needed.
I can even do such a simple patch, but since it is not strictly
speaking a regression from my changes, I'd rather prefer someone
else do. :)


>  _But_ using the in-band status
>    property fundamentally requires this for mvneta to behave correctly:
> 
> 		phy-mode = "sgmii";
> 		managed = "in-band-status";
> 		fixed-link {
> 		};
> 
>    with _no_ phy node.
I don't understand this one.
At least for me it works without empty fixed-link.
There may be some bug.

> 
> 4. Going back to the SFP problem, the link is only up when the SFP
>    module pins indicate that there's no transmitter fault, no loss of
>    signal _and_ the PCS layer at the MAC indicates that it has completed
>    negotiation.  This pretty much rules out trying to emulate a SFP cage
>    as a software-based PHY.  I've code right now doing exactly that, and
>    it results in netif_carrier_on() being called far too early.
> 
> What I don't know is how many generations of the mvneta hardware have
> support for both serdes modes, but what I'm basically saying is that
> the solution we now have seems to be somewhat lacking - maybe it should
> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
> ability to add additional modes later.
Well, you need to be quick with such a change, esp now when the patch
was queued to -stable.
What alternatives do we have here?
Will the following work too?
 		phy-mode = "1000base-x";
 		managed = "in-band-status";

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
> 18.09.2015 02:14, Russell King - ARM Linux ?????:
> >  _But_ using the in-band status
> >    property fundamentally requires this for mvneta to behave correctly:
> > 
> > 		phy-mode = "sgmii";
> > 		managed = "in-band-status";
> > 		fixed-link {
> > 		};
> > 
> >    with _no_ phy node.
> I don't understand this one.
> At least for me it works without empty fixed-link.
> There may be some bug.

        if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
                u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);

                mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
                if (pp->use_inband_status && (cause_misc &
                                (MVNETA_CAUSE_PHY_STATUS_CHANGE |
                                 MVNETA_CAUSE_LINK_CHANGE |
                                 MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
                        mvneta_fixed_link_update(pp, pp->phy_dev);
                }

pp->use_inband_status is set when managed = "in-band-status" is set.
We detect changes in the in-band status, and call mvneta_fixed_link_update():

mvneta_fixed_link_update() reads the status, and communicates that into
the fixed-link phy:

        u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);

	... code setting status.* values from gmac_stat ...
        changed.link = 1;
        changed.speed = 1;
        changed.duplex = 1;
	fixed_phy_update_state(phy, &status, &changed);

fixed_phy_update_state() then looks up the phy in its list, comparing only
the address:

        if (!phydev || !phydev->bus)
                return -EINVAL;

        list_for_each_entry(fp, &fmb->phys, node) {
                if (fp->addr == phydev->addr) {

updating fp->* with the new state, calling fixed_phy_update_regs().  This
updates the fixed-link phy emulated registers, and phylib then notices
the change in link status, and notifies the netdevice attached to the
PHY it found of the change.

Now, one of two things happens as a result of this:

1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
   phy to update its "fixed-link" properties, and the "not so fixed" phy
   changes its parameters according to the new status.

2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
   phy, the fixed-link phy state is updated with the new parameters, and
   some other net device in the system changes link state - the settings
   are not communicated back to the mvneta instance which changed link
   state.

3. If pp->phy_dev is a MDIO phy but does not match a fixed-link phy,
   nothing happens and fixed_phy_update_state() returns -ENOENT.  Again,
   the settings are not communicated back to the mvneta instance which
   changed link state.

Now, a fixed-link phy is only created in mvneta when there is no MDIO phy
specified, but when there is a fixed-link specification in DT:

        phy_node = of_parse_phandle(dn, "phy", 0);
        if (!phy_node) {
                if (!of_phy_is_fixed_link(dn)) {
                        dev_err(&pdev->dev, "no PHY specified\n");
                        err = -ENODEV;
                        goto err_free_irq;
                }

                err = of_phy_register_fixed_link(dn);
                if (err < 0) {
                        dev_err(&pdev->dev, "cannot register fixed PHY\n");
                        goto err_free_irq;
                }

If there's neither a MDIO PHY nor a fixed-link, then the network driver
fails to initialise the device.

> > What I don't know is how many generations of the mvneta hardware have
> > support for both serdes modes, but what I'm basically saying is that
> > the solution we now have seems to be somewhat lacking - maybe it should
> > have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
> > ability to add additional modes later.
>
> Well, you need to be quick with such a change, esp now when the patch
> was queued to -stable.
> What alternatives do we have here?
> Will the following work too?
>  		phy-mode = "1000base-x";
>  		managed = "in-band-status";

There's no chance of being "quick" here - the issues are complex and not
trivial to solve in a day - I've already spent all week thinking about
the issues here, and I'm only _just_ starting to come up with a potential
solution this morning, though I haven't yet assessed whether it'll be
feasible.

The problem I have with the above is that it fixes the phy mode to either
SGMII or 1000base-X, whereas what we need for the SFP case is to have the
down-link object tell the MAC driver whether it wants SGMII or 1000base-X
mode.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 15:13, Russell King - ARM Linux ?????:
> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
>>>  _But_ using the in-band status
>>>    property fundamentally requires this for mvneta to behave correctly:
>>>
>>> 		phy-mode = "sgmii";
>>> 		managed = "in-band-status";
>>> 		fixed-link {
>>> 		};
>>>
>>>    with _no_ phy node.
>> I don't understand this one.
>> At least for me it works without empty fixed-link.
>> There may be some bug.
> 
>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
> 
>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
>                 if (pp->use_inband_status && (cause_misc &
>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
>                                  MVNETA_CAUSE_LINK_CHANGE |
>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
>                         mvneta_fixed_link_update(pp, pp->phy_dev);
>                 }
> 
> pp->use_inband_status is set when managed = "in-band-status" is set.
> We detect changes in the in-band status, and call mvneta_fixed_link_update():
> 
> mvneta_fixed_link_update() reads the status, and communicates that into
> the fixed-link phy:
> 
>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
> 
> 	... code setting status.* values from gmac_stat ...
>         changed.link = 1;
>         changed.speed = 1;
>         changed.duplex = 1;
> 	fixed_phy_update_state(phy, &status, &changed);
> 
> fixed_phy_update_state() then looks up the phy in its list, comparing only
> the address:
> 
>         if (!phydev || !phydev->bus)
>                 return -EINVAL;
> 
>         list_for_each_entry(fp, &fmb->phys, node) {
>                 if (fp->addr == phydev->addr) {
> 
> updating fp->* with the new state, calling fixed_phy_update_regs().  This
> updates the fixed-link phy emulated registers, and phylib then notices
> the change in link status, and notifies the netdevice attached to the
> PHY it found of the change.
> 
> Now, one of two things happens as a result of this:
> 
> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
>    phy to update its "fixed-link" properties, and the "not so fixed" phy
>    changes its parameters according to the new status.
> 
> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
>    phy,
Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
I don't think MDIO PHYs can appear there. If they can - the bug is
very nasty. Have you seen exactly when/why that happens?


> Now, a fixed-link phy is only created in mvneta when there is no MDIO phy
> specified, but when there is a fixed-link specification in DT:
> 
>         phy_node = of_parse_phandle(dn, "phy", 0);
>         if (!phy_node) {
>                 if (!of_phy_is_fixed_link(dn)) {
>                         dev_err(&pdev->dev, "no PHY specified\n");
>                         err = -ENODEV;
>                         goto err_free_irq;
>                 }
> 
>                 err = of_phy_register_fixed_link(dn);
>                 if (err < 0) {
>                         dev_err(&pdev->dev, "cannot register fixed PHY\n");
>                         goto err_free_irq;
>                 }
> 
> If there's neither a MDIO PHY nor a fixed-link, then the network driver
> fails to initialise the device.
But it does.
In fact, of_mdio.c has this code:

        err = of_property_read_string(np, "managed", &managed);
        if (err == 0) {
                if (strcmp(managed, "in-band-status") == 0) {
                        /* status is zeroed, namely its .link member */
                        phy = fixed_phy_register(PHY_POLL, &status, np);
                        return IS_ERR(phy) ? PTR_ERR(phy) : 0;
                }
        }

Which is exactly for the case you describe.


>>> What I don't know is how many generations of the mvneta hardware have
>>> support for both serdes modes, but what I'm basically saying is that
>>> the solution we now have seems to be somewhat lacking - maybe it should
>>> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
>>> ability to add additional modes later.
>>
>> Well, you need to be quick with such a change, esp now when the patch
>> was queued to -stable.
>> What alternatives do we have here?
>> Will the following work too?
>>  		phy-mode = "1000base-x";
>>  		managed = "in-band-status";
> 
> There's no chance of being "quick" here - the issues are complex and not
> trivial to solve in a day - I've already spent all week thinking about
> the issues here, and I'm only _just_ starting to come up with a potential
> solution this morning, though I haven't yet assessed whether it'll be
> feasible.
> 
> The problem I have with the above is that it fixes the phy mode to either
> SGMII or 1000base-X, whereas what we need for the SFP case is to have the
> down-link object tell the MAC driver whether it wants SGMII or 1000base-X
> mode.
Not that I understand that SFP business at all.
Maybe if some downlink tells the MAC what autoneg protocol will
be used, you can have:
  		phy-mode = "1000base-x" | "sgmii" | "serdes-auto";
  		managed = "in-band-status";

and "serdes-auto" will use either "1000base-x" or "sgmii", depending
on what the downlink says?

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
> 18.09.2015 15:13, Russell King - ARM Linux ?????:
> > On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
> >> 18.09.2015 02:14, Russell King - ARM Linux ?????:
> >>>  _But_ using the in-band status
> >>>    property fundamentally requires this for mvneta to behave correctly:
> >>>
> >>> 		phy-mode = "sgmii";
> >>> 		managed = "in-band-status";
> >>> 		fixed-link {
> >>> 		};
> >>>
> >>>    with _no_ phy node.
> >> I don't understand this one.
> >> At least for me it works without empty fixed-link.
> >> There may be some bug.
> > 
> >         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
> >                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
> > 
> >                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> >                 if (pp->use_inband_status && (cause_misc &
> >                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
> >                                  MVNETA_CAUSE_LINK_CHANGE |
> >                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
> >                         mvneta_fixed_link_update(pp, pp->phy_dev);
> >                 }
> > 
> > pp->use_inband_status is set when managed = "in-band-status" is set.
> > We detect changes in the in-band status, and call mvneta_fixed_link_update():
> > 
> > mvneta_fixed_link_update() reads the status, and communicates that into
> > the fixed-link phy:
> > 
> >         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
> > 
> > 	... code setting status.* values from gmac_stat ...
> >         changed.link = 1;
> >         changed.speed = 1;
> >         changed.duplex = 1;
> > 	fixed_phy_update_state(phy, &status, &changed);
> > 
> > fixed_phy_update_state() then looks up the phy in its list, comparing only
> > the address:
> > 
> >         if (!phydev || !phydev->bus)
> >                 return -EINVAL;
> > 
> >         list_for_each_entry(fp, &fmb->phys, node) {
> >                 if (fp->addr == phydev->addr) {
> > 
> > updating fp->* with the new state, calling fixed_phy_update_regs().  This
> > updates the fixed-link phy emulated registers, and phylib then notices
> > the change in link status, and notifies the netdevice attached to the
> > PHY it found of the change.
> > 
> > Now, one of two things happens as a result of this:
> > 
> > 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
> >    phy to update its "fixed-link" properties, and the "not so fixed" phy
> >    changes its parameters according to the new status.
> > 
> > 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
> >    phy,
> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
> I don't think MDIO PHYs can appear there. If they can - the bug is
> very nasty. Have you seen exactly when/why that happens?

I think I explained it fully - please follow the code paths I've detailed
above.

Specifically, look at this code:

         if (!phydev || !phydev->bus)
                 return -EINVAL;

         list_for_each_entry(fp, &fmb->phys, node) {
                 if (fp->addr == phydev->addr) {

Consider what the effect is if you have a MDIO phy at address 0 on eth0
which has in-band-status enabled.

Now if you have a fixed-link phy on the fixed-link bus at address 0
connected to eth1.

Bring eth1 up, everything works as one would expect, it gains the fixed
link settings.

Now bring eth0 up.  Because there's no distinction in mvneta between a
fixed-link phy and a MDIO phy, it ends up calling fixed_phy_update_state()
with the MDIO phy, which has phydev->addr = 0.

The fixed-link code scans every phy on the fixed-link bus, looking for
one with address 0, and it finds that, and modifies the state of that
phy.

eth1 now gains the settings that eth0 communicated into the fixed-link
phy layer.

> > Now, a fixed-link phy is only created in mvneta when there is no MDIO phy
> > specified, but when there is a fixed-link specification in DT:
> > 
> >         phy_node = of_parse_phandle(dn, "phy", 0);
> >         if (!phy_node) {
> >                 if (!of_phy_is_fixed_link(dn)) {
> >                         dev_err(&pdev->dev, "no PHY specified\n");
> >                         err = -ENODEV;
> >                         goto err_free_irq;
> >                 }
> > 
> >                 err = of_phy_register_fixed_link(dn);
> >                 if (err < 0) {
> >                         dev_err(&pdev->dev, "cannot register fixed PHY\n");
> >                         goto err_free_irq;
> >                 }
> > 
> > If there's neither a MDIO PHY nor a fixed-link, then the network driver
> > fails to initialise the device.
> But it does.

Please, look again at the code I quoted above.

> In fact, of_mdio.c has this code:
> 
>         err = of_property_read_string(np, "managed", &managed);
>         if (err == 0) {
>                 if (strcmp(managed, "in-band-status") == 0) {
>                         /* status is zeroed, namely its .link member */
>                         phy = fixed_phy_register(PHY_POLL, &status, np);
>                         return IS_ERR(phy) ? PTR_ERR(phy) : 0;
>                 }
>         }
> 
> Which is exactly for the case you describe.

That code is in of_phy_register_fixed_link().  That code will _NOT_ be
reached if a MDIO phy is specified.  Again, please read the code.

> >>> What I don't know is how many generations of the mvneta hardware have
> >>> support for both serdes modes, but what I'm basically saying is that
> >>> the solution we now have seems to be somewhat lacking - maybe it should
> >>> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the
> >>> ability to add additional modes later.
> >>
> >> Well, you need to be quick with such a change, esp now when the patch
> >> was queued to -stable.
> >> What alternatives do we have here?
> >> Will the following work too?
> >>  		phy-mode = "1000base-x";
> >>  		managed = "in-band-status";
> > 
> > There's no chance of being "quick" here - the issues are complex and not
> > trivial to solve in a day - I've already spent all week thinking about
> > the issues here, and I'm only _just_ starting to come up with a potential
> > solution this morning, though I haven't yet assessed whether it'll be
> > feasible.
> > 
> > The problem I have with the above is that it fixes the phy mode to either
> > SGMII or 1000base-X, whereas what we need for the SFP case is to have the
> > down-link object tell the MAC driver whether it wants SGMII or 1000base-X
> > mode.
> Not that I understand that SFP business at all.
> Maybe if some downlink tells the MAC what autoneg protocol will
> be used, you can have:
>   		phy-mode = "1000base-x" | "sgmii" | "serdes-auto";
>   		managed = "in-band-status";
> 
> and "serdes-auto" will use either "1000base-x" or "sgmii", depending
> on what the downlink says?

Maybe, but rather than guessing and getting it wrong, let's wait until
we know what kind of a solution is necessary here.  Rushing this will
only create another design mistake and an even larger can of worms.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 16:12, Russell King - ARM Linux ?????:
> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
>> 18.09.2015 15:13, Russell King - ARM Linux ?????:
>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
>>>>>  _But_ using the in-band status
>>>>>    property fundamentally requires this for mvneta to behave correctly:
>>>>>
>>>>> 		phy-mode = "sgmii";
>>>>> 		managed = "in-band-status";
>>>>> 		fixed-link {
>>>>> 		};
>>>>>
>>>>>    with _no_ phy node.
>>>> I don't understand this one.
>>>> At least for me it works without empty fixed-link.
>>>> There may be some bug.
>>>
>>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
>>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
>>>
>>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
>>>                 if (pp->use_inband_status && (cause_misc &
>>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
>>>                                  MVNETA_CAUSE_LINK_CHANGE |
>>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
>>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
>>>                 }
>>>
>>> pp->use_inband_status is set when managed = "in-band-status" is set.
>>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
>>>
>>> mvneta_fixed_link_update() reads the status, and communicates that into
>>> the fixed-link phy:
>>>
>>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
>>>
>>> 	... code setting status.* values from gmac_stat ...
>>>         changed.link = 1;
>>>         changed.speed = 1;
>>>         changed.duplex = 1;
>>> 	fixed_phy_update_state(phy, &status, &changed);
>>>
>>> fixed_phy_update_state() then looks up the phy in its list, comparing only
>>> the address:
>>>
>>>         if (!phydev || !phydev->bus)
>>>                 return -EINVAL;
>>>
>>>         list_for_each_entry(fp, &fmb->phys, node) {
>>>                 if (fp->addr == phydev->addr) {
>>>
>>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
>>> updates the fixed-link phy emulated registers, and phylib then notices
>>> the change in link status, and notifies the netdevice attached to the
>>> PHY it found of the change.
>>>
>>> Now, one of two things happens as a result of this:
>>>
>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
>>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
>>>    changes its parameters according to the new status.
>>>
>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
>>>    phy,
>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
>> I don't think MDIO PHYs can appear there. If they can - the bug is
>> very nasty. Have you seen exactly when/why that happens?
> 
> I think I explained it fully - please follow the code paths I've detailed
> above.
I try. What I don't understand is why both PHYs get the
same address on the "Fixed MDIO bus".

> 
> Specifically, look at this code:
> 
>          if (!phydev || !phydev->bus)
>                  return -EINVAL;
> 
>          list_for_each_entry(fp, &fmb->phys, node) {
>                  if (fp->addr == phydev->addr) {
> 
> Consider what the effect is if you have a MDIO phy at address 0 on eth0
> which has in-band-status enabled.
So as I understand, you have MDIO phy with DT looking like this:
ethernet at 70000 {
  status = "okay";
  phy-mode = "sgmii";
  managed = "in-band-status";
}
W/O either "phy" of "fixed-link" nodes. Correct?
mvneta calls of_phy_register_fixed_link(dn) on it after not
finding the "phy" node. And it will do the same with the second
non-MDIO phy. What I don't see is how do they get the same addr
on the same bus, could you please clarify that a bit?

>>> Now, a fixed-link phy is only created in mvneta when there is no MDIO phy
>>> specified, but when there is a fixed-link specification in DT:
>>>
>>>         phy_node = of_parse_phandle(dn, "phy", 0);
>>>         if (!phy_node) {
>>>                 if (!of_phy_is_fixed_link(dn)) {
>>>                         dev_err(&pdev->dev, "no PHY specified\n");
>>>                         err = -ENODEV;
>>>                         goto err_free_irq;
>>>                 }
>>>
>>>                 err = of_phy_register_fixed_link(dn);
>>>                 if (err < 0) {
>>>                         dev_err(&pdev->dev, "cannot register fixed PHY\n");
>>>                         goto err_free_irq;
>>>                 }
>>>
>>> If there's neither a MDIO PHY nor a fixed-link, then the network driver
>>> fails to initialise the device.
>> But it does.
> 
> Please, look again at the code I quoted above.
> 
>> In fact, of_mdio.c has this code:
>>
>>         err = of_property_read_string(np, "managed", &managed);
>>         if (err == 0) {
>>                 if (strcmp(managed, "in-band-status") == 0) {
>>                         /* status is zeroed, namely its .link member */
>>                         phy = fixed_phy_register(PHY_POLL, &status, np);
>>                         return IS_ERR(phy) ? PTR_ERR(phy) : 0;
>>                 }
>>         }
>>
>> Which is exactly for the case you describe.
> 
> That code is in of_phy_register_fixed_link().  That code will _NOT_ be
> reached if a MDIO phy is specified.  Again, please read the code.
I think the DT quote is missing here for me to understand.
This code is for managed = "in-band-status", which is what I thought
you are talking about.
Could you please quote the DT that creates 2 PHYs with the
same addr on fmb? I'll try it here and from that will understand.


>> Maybe if some downlink tells the MAC what autoneg protocol will
>> be used, you can have:
>>   		phy-mode = "1000base-x" | "sgmii" | "serdes-auto";
>>   		managed = "in-band-status";
>>
>> and "serdes-auto" will use either "1000base-x" or "sgmii", depending
>> on what the downlink says?
> 
> Maybe, but rather than guessing and getting it wrong, let's wait until
> we know what kind of a solution is necessary here.  Rushing this will
> only create another design mistake and an even larger can of worms.
That was just an idea of how to get it without changing the current
values of the "managed" property.

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
> 18.09.2015 16:12, Russell King - ARM Linux ?????:
> > On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
> >> 18.09.2015 15:13, Russell King - ARM Linux ?????:
> >>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
> >>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
> >>>>>  _But_ using the in-band status
> >>>>>    property fundamentally requires this for mvneta to behave correctly:
> >>>>>
> >>>>> 		phy-mode = "sgmii";
> >>>>> 		managed = "in-band-status";
> >>>>> 		fixed-link {
> >>>>> 		};
> >>>>>
> >>>>>    with _no_ phy node.
> >>>> I don't understand this one.
> >>>> At least for me it works without empty fixed-link.
> >>>> There may be some bug.
> >>>
> >>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
> >>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
> >>>
> >>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> >>>                 if (pp->use_inband_status && (cause_misc &
> >>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
> >>>                                  MVNETA_CAUSE_LINK_CHANGE |
> >>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
> >>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
> >>>                 }
> >>>
> >>> pp->use_inband_status is set when managed = "in-band-status" is set.
> >>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
> >>>
> >>> mvneta_fixed_link_update() reads the status, and communicates that into
> >>> the fixed-link phy:
> >>>
> >>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
> >>>
> >>> 	... code setting status.* values from gmac_stat ...
> >>>         changed.link = 1;
> >>>         changed.speed = 1;
> >>>         changed.duplex = 1;
> >>> 	fixed_phy_update_state(phy, &status, &changed);
> >>>
> >>> fixed_phy_update_state() then looks up the phy in its list, comparing only
> >>> the address:
> >>>
> >>>         if (!phydev || !phydev->bus)
> >>>                 return -EINVAL;
> >>>
> >>>         list_for_each_entry(fp, &fmb->phys, node) {
> >>>                 if (fp->addr == phydev->addr) {
> >>>
> >>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
> >>> updates the fixed-link phy emulated registers, and phylib then notices
> >>> the change in link status, and notifies the netdevice attached to the
> >>> PHY it found of the change.
> >>>
> >>> Now, one of two things happens as a result of this:
> >>>
> >>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
> >>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
> >>>    changes its parameters according to the new status.
> >>>
> >>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
> >>>    phy,
> >> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
> >> I don't think MDIO PHYs can appear there. If they can - the bug is
> >> very nasty. Have you seen exactly when/why that happens?
> > 
> > I think I explained it fully - please follow the code paths I've detailed
> > above.
> I try. What I don't understand is why both PHYs get the
> same address on the "Fixed MDIO bus".

They aren't both on the fixed MDIO bus - that's the whole point I'm
making.  They get the same phydev->addr but on _different_ buses.

> > Specifically, look at this code:
> > 
> >          if (!phydev || !phydev->bus)
> >                  return -EINVAL;
> > 
> >          list_for_each_entry(fp, &fmb->phys, node) {
> >                  if (fp->addr == phydev->addr) {

Look at this closely - at what point is there any validation that "phydev"
is actually a fixed-link phy?  There is no validation done.  The only
criteria there are:
- phydev is not NULL
- phydev->bus is not NULL (which is true of any registered phy)
- phydev->addr matches _any_ fixed-link phy.

I've already sent a patch earlier today to address this issue.

If you place a WARN_ON(fp->phydev != phydev) then that'll show you
when it incorrectly matches.

> > Consider what the effect is if you have a MDIO phy at address 0 on eth0
> > which has in-band-status enabled.
> So as I understand, you have MDIO phy with DT looking like this:
> ethernet at 70000 {
>   status = "okay";
>   phy-mode = "sgmii";
>   managed = "in-band-status";
> }
> W/O either "phy" of "fixed-link" nodes. Correct?
> mvneta calls of_phy_register_fixed_link(dn) on it after not
> finding the "phy" node. And it will do the same with the second
> non-MDIO phy. What I don't see is how do they get the same addr
> on the same bus, could you please clarify that a bit?

		mdio at 72004 {
			phy_dedicated: ethernet-phy at 0 {
				reg = <0>;
			};
		};

		eth1: ethernet at 30000 {
			phy = <&phy_dedicated>;
			phy-mode = "sgmii";
			managed = "in-band-status";
		};

		eth0: ethernet at 70000 {
			phy-mode = "sgmii";
			fixed-link {
				speed = 1000;
				full-duplex;
			};
		};

Bring eth0 up first, everything works.  Then, bring eth1 up, and eth0
goes down.

This happens because when eth1 is brought up, eth1's mvneta calls into
fixed_phy_update_state() with a pointer to the "phy_dedicated" PHY at
address 0.  fixed_phy_update_state() scans the fixed-link phys for one
at address 0, and finds the fixed-link phy associated with eth0.
This causes the fixed link code to change the settings for eth0.

As I have already shown in my previous setup diagrams, it is _entirely_
reasonable to use in-band status with SGMII with a phy attached.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 16:57, Russell King - ARM Linux ?????:
> On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
>> 18.09.2015 16:12, Russell King - ARM Linux ?????:
>>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
>>>> 18.09.2015 15:13, Russell King - ARM Linux ?????:
>>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
>>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
>>>>>>>  _But_ using the in-band status
>>>>>>>    property fundamentally requires this for mvneta to behave correctly:
>>>>>>>
>>>>>>> 		phy-mode = "sgmii";
>>>>>>> 		managed = "in-band-status";
>>>>>>> 		fixed-link {
>>>>>>> 		};
>>>>>>>
>>>>>>>    with _no_ phy node.
>>>>>> I don't understand this one.
>>>>>> At least for me it works without empty fixed-link.
>>>>>> There may be some bug.
>>>>>
>>>>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
>>>>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
>>>>>
>>>>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
>>>>>                 if (pp->use_inband_status && (cause_misc &
>>>>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
>>>>>                                  MVNETA_CAUSE_LINK_CHANGE |
>>>>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
>>>>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
>>>>>                 }
>>>>>
>>>>> pp->use_inband_status is set when managed = "in-band-status" is set.
>>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
>>>>>
>>>>> mvneta_fixed_link_update() reads the status, and communicates that into
>>>>> the fixed-link phy:
>>>>>
>>>>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
>>>>>
>>>>> 	... code setting status.* values from gmac_stat ...
>>>>>         changed.link = 1;
>>>>>         changed.speed = 1;
>>>>>         changed.duplex = 1;
>>>>> 	fixed_phy_update_state(phy, &status, &changed);
>>>>>
>>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only
>>>>> the address:
>>>>>
>>>>>         if (!phydev || !phydev->bus)
>>>>>                 return -EINVAL;
>>>>>
>>>>>         list_for_each_entry(fp, &fmb->phys, node) {
>>>>>                 if (fp->addr == phydev->addr) {
>>>>>
>>>>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
>>>>> updates the fixed-link phy emulated registers, and phylib then notices
>>>>> the change in link status, and notifies the netdevice attached to the
>>>>> PHY it found of the change.
>>>>>
>>>>> Now, one of two things happens as a result of this:
>>>>>
>>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
>>>>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
>>>>>    changes its parameters according to the new status.
>>>>>
>>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
>>>>>    phy,
>>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
>>>> I don't think MDIO PHYs can appear there. If they can - the bug is
>>>> very nasty. Have you seen exactly when/why that happens?
>>>
>>> I think I explained it fully - please follow the code paths I've detailed
>>> above.
>> I try. What I don't understand is why both PHYs get the
>> same address on the "Fixed MDIO bus".
> 
> They aren't both on the fixed MDIO bus - that's the whole point I'm
> making.  They get the same phydev->addr but on _different_ buses.
So you have an MDIO phy for which mvneta seems to have
use_inband_status==true, correct? AFAICS if it has use_inband_status==true,
then it went through of_phy_register_fixed_link(dn), which, in case
of managed="in-band-status", was created by fixed_phy_register(), in
which case it should be on the "fixed MDIO bus". So if they are on a
_different_ buses, then I would expect for the MDIO phy to have
use_inband_status==false.
You described the update status path very precisely in your prev mail,
but this doesn't help because what I don't understand is the particular
_setup_ path that leads to use_inband_status==true yet the phy is not
on the fmb.


>>> Specifically, look at this code:
>>>
>>>          if (!phydev || !phydev->bus)
>>>                  return -EINVAL;
>>>
>>>          list_for_each_entry(fp, &fmb->phys, node) {
>>>                  if (fp->addr == phydev->addr) {
> 
> Look at this closely - at what point is there any validation that "phydev"
> is actually a fixed-link phy?  There is no validation done.  The only
> criteria there are:
> - phydev is not NULL
> - phydev->bus is not NULL (which is true of any registered phy)
> - phydev->addr matches _any_ fixed-link phy.
> 
> I've already sent a patch earlier today to address this issue.
> 
> If you place a WARN_ON(fp->phydev != phydev) then that'll show you
> when it incorrectly matches.
Yes, I realize if the MDIO phy is passed there, then all you say
will happen. That's pretty trivial and doesn't need any explanations.
But how was that phydev created, not with fixed_phy_register() I suppose?
Then it should have use_inband_status==false, right?

>>> Consider what the effect is if you have a MDIO phy at address 0 on eth0
>>> which has in-band-status enabled.
>> So as I understand, you have MDIO phy with DT looking like this:
>> ethernet at 70000 {
>>   status = "okay";
>>   phy-mode = "sgmii";
>>   managed = "in-band-status";
>> }
>> W/O either "phy" of "fixed-link" nodes. Correct?
>> mvneta calls of_phy_register_fixed_link(dn) on it after not
>> finding the "phy" node. And it will do the same with the second
>> non-MDIO phy. What I don't see is how do they get the same addr
>> on the same bus, could you please clarify that a bit?
> 
> 		mdio at 72004 {
> 			phy_dedicated: ethernet-phy at 0 {
> 				reg = <0>;
> 			};
> 		};
> 
> 		eth1: ethernet at 30000 {
> 			phy = <&phy_dedicated>;
> 			phy-mode = "sgmii";
> 			managed = "in-band-status";
> 		};
> 
> 		eth0: ethernet at 70000 {
> 			phy-mode = "sgmii";
> 			fixed-link {
> 				speed = 1000;
> 				full-duplex;
> 			};
> 		};
> 
> Bring eth0 up first, everything works.  Then, bring eth1 up, and eth0
> goes down.
I don't have an MDIO phy here, but I'll make an attempt to replicate
this setup.


> This happens because when eth1 is brought up, eth1's mvneta calls into
> fixed_phy_update_state() with a pointer to the "phy_dedicated" PHY at
> address 0.  fixed_phy_update_state() scans the fixed-link phys for one
> at address 0, and finds the fixed-link phy associated with eth0.
Unfortunately this is not what needs to be explained.
I am confused about the setup path, not update path.

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
> 18.09.2015 16:57, Russell King - ARM Linux ?????:
> > On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
> >> 18.09.2015 16:12, Russell King - ARM Linux ?????:
> >>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
> >>>> 18.09.2015 15:13, Russell King - ARM Linux ?????:
> >>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
> >>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
> >>>>>>>  _But_ using the in-band status
> >>>>>>>    property fundamentally requires this for mvneta to behave correctly:
> >>>>>>>
> >>>>>>> 		phy-mode = "sgmii";
> >>>>>>> 		managed = "in-band-status";
> >>>>>>> 		fixed-link {
> >>>>>>> 		};
> >>>>>>>
> >>>>>>>    with _no_ phy node.
> >>>>>> I don't understand this one.
> >>>>>> At least for me it works without empty fixed-link.
> >>>>>> There may be some bug.
> >>>>>
> >>>>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
> >>>>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
> >>>>>
> >>>>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> >>>>>                 if (pp->use_inband_status && (cause_misc &
> >>>>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
> >>>>>                                  MVNETA_CAUSE_LINK_CHANGE |
> >>>>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
> >>>>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
> >>>>>                 }
> >>>>>
> >>>>> pp->use_inband_status is set when managed = "in-band-status" is set.
> >>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
> >>>>>
> >>>>> mvneta_fixed_link_update() reads the status, and communicates that into
> >>>>> the fixed-link phy:
> >>>>>
> >>>>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
> >>>>>
> >>>>> 	... code setting status.* values from gmac_stat ...
> >>>>>         changed.link = 1;
> >>>>>         changed.speed = 1;
> >>>>>         changed.duplex = 1;
> >>>>> 	fixed_phy_update_state(phy, &status, &changed);
> >>>>>
> >>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only
> >>>>> the address:
> >>>>>
> >>>>>         if (!phydev || !phydev->bus)
> >>>>>                 return -EINVAL;
> >>>>>
> >>>>>         list_for_each_entry(fp, &fmb->phys, node) {
> >>>>>                 if (fp->addr == phydev->addr) {
> >>>>>
> >>>>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
> >>>>> updates the fixed-link phy emulated registers, and phylib then notices
> >>>>> the change in link status, and notifies the netdevice attached to the
> >>>>> PHY it found of the change.
> >>>>>
> >>>>> Now, one of two things happens as a result of this:
> >>>>>
> >>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
> >>>>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
> >>>>>    changes its parameters according to the new status.
> >>>>>
> >>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
> >>>>>    phy,
> >>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
> >>>> I don't think MDIO PHYs can appear there. If they can - the bug is
> >>>> very nasty. Have you seen exactly when/why that happens?
> >>>
> >>> I think I explained it fully - please follow the code paths I've detailed
> >>> above.
> >> I try. What I don't understand is why both PHYs get the
> >> same address on the "Fixed MDIO bus".
> > 
> > They aren't both on the fixed MDIO bus - that's the whole point I'm
> > making.  They get the same phydev->addr but on _different_ buses.
>
> So you have an MDIO phy for which mvneta seems to have
> use_inband_status==true, correct?

That is one very real possibililty.  Cisco SGMII in-band status is for a
SGMII PHY to inform the MAC about the properties of the link to which the
PHY is attached.

So, specifying "managed = in-band-status" along with a real PHY is very
much a _real_ possibility, as I've previously explained.

> AFAICS if it has use_inband_status==true,
> then it went through of_phy_register_fixed_link(dn),

That's totally incorrect.  The test for setting use_inband_status in
mvneta is:

        err = of_property_read_string(dn, "managed", &managed);
        pp->use_inband_status = (err == 0 &&
                                 strcmp(managed, "in-band-status") == 0);

So, use_inband_status can be set _whatever_.  It doesn't matter if
there's a fixed-phy being used, or whether a real MDIO phy has been
specified.

The _actual_ situation I had when I encountered the problem was:

eth0 - connected to a DSA switch
eth1 - connected to SFP "phy" with in-band-status enabled (for 1000base-X)
       where this phy is sitting on its own virtual MDIO bus.
eth2 - connected to a RGMII phy.

At boot, eth2 is brought up by DHCP, and eth0 is configured up as part
of the switch initialisation.  eth0 comes up fine.

Then, I manually brought up eth1, very unexpectedly eth0 immediately went
down - and this was completely repeatable, caused by this problem.

> You described the update status path very precisely in your prev mail,
> but this doesn't help because what I don't understand is the particular
> _setup_ path that leads to use_inband_status==true yet the phy is not
> on the fmb.

I think I covered that in the email to which you're replying, where I
show you that I have a "phy=" node within the ethernet definition.
That means:

        phy_node = of_parse_phandle(dn, "phy", 0);

will return non-NULL, and that means:

        if (!phy_node) {
                if (!of_phy_is_fixed_link(dn)) {
                        dev_err(&pdev->dev, "no PHY specified\n");
                        err = -ENODEV;
                        goto err_free_irq;
                }

                err = of_phy_register_fixed_link(dn);

the code here is never reached.

I'm failing to see what the problem is - the code you keep referring
me to won't be called in the situation I'm describing.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 18:43, Russell King - ARM Linux ?????:
> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
>> 18.09.2015 16:57, Russell King - ARM Linux ?????:
>>> On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
>>>> 18.09.2015 16:12, Russell King - ARM Linux ?????:
>>>>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
>>>>>> 18.09.2015 15:13, Russell King - ARM Linux ?????:
>>>>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
>>>>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
>>>>>>>>>  _But_ using the in-band status
>>>>>>>>>    property fundamentally requires this for mvneta to behave correctly:
>>>>>>>>>
>>>>>>>>> 		phy-mode = "sgmii";
>>>>>>>>> 		managed = "in-band-status";
>>>>>>>>> 		fixed-link {
>>>>>>>>> 		};
>>>>>>>>>
>>>>>>>>>    with _no_ phy node.
>>>>>>>> I don't understand this one.
>>>>>>>> At least for me it works without empty fixed-link.
>>>>>>>> There may be some bug.
>>>>>>>
>>>>>>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
>>>>>>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
>>>>>>>
>>>>>>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
>>>>>>>                 if (pp->use_inband_status && (cause_misc &
>>>>>>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
>>>>>>>                                  MVNETA_CAUSE_LINK_CHANGE |
>>>>>>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
>>>>>>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
>>>>>>>                 }
>>>>>>>
>>>>>>> pp->use_inband_status is set when managed = "in-band-status" is set.
>>>>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
>>>>>>>
>>>>>>> mvneta_fixed_link_update() reads the status, and communicates that into
>>>>>>> the fixed-link phy:
>>>>>>>
>>>>>>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
>>>>>>>
>>>>>>> 	... code setting status.* values from gmac_stat ...
>>>>>>>         changed.link = 1;
>>>>>>>         changed.speed = 1;
>>>>>>>         changed.duplex = 1;
>>>>>>> 	fixed_phy_update_state(phy, &status, &changed);
>>>>>>>
>>>>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only
>>>>>>> the address:
>>>>>>>
>>>>>>>         if (!phydev || !phydev->bus)
>>>>>>>                 return -EINVAL;
>>>>>>>
>>>>>>>         list_for_each_entry(fp, &fmb->phys, node) {
>>>>>>>                 if (fp->addr == phydev->addr) {
>>>>>>>
>>>>>>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
>>>>>>> updates the fixed-link phy emulated registers, and phylib then notices
>>>>>>> the change in link status, and notifies the netdevice attached to the
>>>>>>> PHY it found of the change.
>>>>>>>
>>>>>>> Now, one of two things happens as a result of this:
>>>>>>>
>>>>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
>>>>>>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
>>>>>>>    changes its parameters according to the new status.
>>>>>>>
>>>>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
>>>>>>>    phy,
>>>>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
>>>>>> I don't think MDIO PHYs can appear there. If they can - the bug is
>>>>>> very nasty. Have you seen exactly when/why that happens?
>>>>>
>>>>> I think I explained it fully - please follow the code paths I've detailed
>>>>> above.
>>>> I try. What I don't understand is why both PHYs get the
>>>> same address on the "Fixed MDIO bus".
>>>
>>> They aren't both on the fixed MDIO bus - that's the whole point I'm
>>> making.  They get the same phydev->addr but on _different_ buses.
>>
>> So you have an MDIO phy for which mvneta seems to have
>> use_inband_status==true, correct?
> 
> That is one very real possibililty.  Cisco SGMII in-band status is for a
> SGMII PHY to inform the MAC about the properties of the link to which the
> PHY is attached.
> 
> So, specifying "managed = in-band-status" along with a real PHY is very
> much a _real_ possibility, as I've previously explained.
> 
>> AFAICS if it has use_inband_status==true,
>> then it went through of_phy_register_fixed_link(dn),
> 
> That's totally incorrect.  The test for setting use_inband_status in
> mvneta is:
> 
>         err = of_property_read_string(dn, "managed", &managed);
>         pp->use_inband_status = (err == 0 &&
>                                  strcmp(managed, "in-band-status") == 0);

Arrrr! I was looking at the branch without the last
patch applied, so it occurred to me as

	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
		fixed_phy;

Sorry for that.
So we seem to indeed have a nasty regression with the patch
that just went to stable. :( Great news.
Thanks for you time.

I still have problems with this part though:
> If there's neither a MDIO PHY nor a fixed-link, then the network driver
> fails to initialise the device.

I think I am looking into the right source this time, seems like
if we don't have both but still have managed="in-band-status", that
should go the fixed-link path and still work... no?

mvneta: SGMII fixed-link not so fixed

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote:
> 18.09.2015 18:43, Russell King - ARM Linux ?????:
> > On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
> >> AFAICS if it has use_inband_status==true,
> >> then it went through of_phy_register_fixed_link(dn),
> > 
> > That's totally incorrect.  The test for setting use_inband_status in
> > mvneta is:
> > 
> >         err = of_property_read_string(dn, "managed", &managed);
> >         pp->use_inband_status = (err == 0 &&
> >                                  strcmp(managed, "in-band-status") == 0);
> 
> Arrrr! I was looking at the branch without the last
> patch applied, so it occurred to me as
> 
> 	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
> 		fixed_phy;
> 
> Sorry for that.

Yay :)

> So we seem to indeed have a nasty regression with the patch
> that just went to stable. :( Great news.

Yes.

> Thanks for you time.
> 
> I still have problems with this part though:
> > If there's neither a MDIO PHY nor a fixed-link, then the network driver
> > fails to initialise the device.
> 
> I think I am looking into the right source this time, seems like
> if we don't have both but still have managed="in-band-status", that
> should go the fixed-link path and still work... no?

If we have no fixed-link and no phy, then you're correct.

However, I really don't like the idea of abusing "fixed-link" as a
method to generate an ethtool/miitool/miidiag compatible output for
this, but I'm willing to let that pass for the moment. :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

mvneta: SGMII fixed-link not so fixed

[Florian Fainelli]

On 18/09/15 10:22, Russell King - ARM Linux wrote:
> On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote:
>> 18.09.2015 18:43, Russell King - ARM Linux ?????:
>>> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
>>>> AFAICS if it has use_inband_status==true,
>>>> then it went through of_phy_register_fixed_link(dn),
>>>
>>> That's totally incorrect.  The test for setting use_inband_status in
>>> mvneta is:
>>>
>>>         err = of_property_read_string(dn, "managed", &managed);
>>>         pp->use_inband_status = (err == 0 &&
>>>                                  strcmp(managed, "in-band-status") == 0);
>>
>> Arrrr! I was looking at the branch without the last
>> patch applied, so it occurred to me as
>>
>> 	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
>> 		fixed_phy;
>>
>> Sorry for that.
> 
> Yay :)
> 
>> So we seem to indeed have a nasty regression with the patch
>> that just went to stable. :( Great news.
> 
> Yes.
> 
>> Thanks for you time.
>>
>> I still have problems with this part though:
>>> If there's neither a MDIO PHY nor a fixed-link, then the network driver
>>> fails to initialise the device.
>>
>> I think I am looking into the right source this time, seems like
>> if we don't have both but still have managed="in-band-status", that
>> should go the fixed-link path and still work... no?
> 
> If we have no fixed-link and no phy, then you're correct.
> 
> However, I really don't like the idea of abusing "fixed-link" as a
> method to generate an ethtool/miitool/miidiag compatible output for
> this, but I'm willing to let that pass for the moment. :)

It is not just for that, you get all the goodies from the PHY library
without modifying your Ethernet MAC driver specifically for it:
phy_connect, adjust_link and phy_ethtool_{set,get}.

The solution that was judged being the less intrusive back then was to
provide MII-like registers, getting you all user-land to work
transparently for free, but now that I think about it, having this "MII"
translation seems a bit unnecessary, if not confusing. It may be better
to remove some of register update logic and just assign phydev members
directly...
-- 
Florian

mvneta: SGMII fixed-link not so fixed

[Stas Sergeev]

18.09.2015 20:30, Florian Fainelli ?????:
> On 18/09/15 10:22, Russell King - ARM Linux wrote:
>> On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote:
>>> 18.09.2015 18:43, Russell King - ARM Linux ?????:
>>>> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
>>>>> AFAICS if it has use_inband_status==true,
>>>>> then it went through of_phy_register_fixed_link(dn),
>>>> That's totally incorrect.  The test for setting use_inband_status in
>>>> mvneta is:
>>>>
>>>>          err = of_property_read_string(dn, "managed", &managed);
>>>>          pp->use_inband_status = (err == 0 &&
>>>>                                   strcmp(managed, "in-band-status") == 0);
>>> Arrrr! I was looking at the branch without the last
>>> patch applied, so it occurred to me as
>>>
>>> 	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
>>> 		fixed_phy;
>>>
>>> Sorry for that.
>> Yay :)
>>
>>> So we seem to indeed have a nasty regression with the patch
>>> that just went to stable. :( Great news.
>> Yes.
>>
>>> Thanks for you time.
>>>
>>> I still have problems with this part though:
>>>> If there's neither a MDIO PHY nor a fixed-link, then the network driver
>>>> fails to initialise the device.
>>> I think I am looking into the right source this time, seems like
>>> if we don't have both but still have managed="in-band-status", that
>>> should go the fixed-link path and still work... no?
>> If we have no fixed-link and no phy, then you're correct.
>>
>> However, I really don't like the idea of abusing "fixed-link" as a
>> method to generate an ethtool/miitool/miidiag compatible output for
>> this, but I'm willing to let that pass for the moment. :)
> It is not just for that, you get all the goodies from the PHY library
> without modifying your Ethernet MAC driver specifically for it:
> phy_connect, adjust_link and phy_ethtool_{set,get}.
But you still need to modify the MAC driver, and in a bit
nasty way. Eg when you want the AN to work right, you
need to do of_phy_find_device() and feed the current AN
settings to fixed-phy manually, or, after phy_connect(), you'll
run with wrong link state for some time (and there is still
a bit of race anyway, as the link state could change between
of_phy_find_device() and phy_connect()).
So I am not sure what would the best API look like, but
my overall impression was it is currently not the one.