Question: pause mode disabled for marvell 88e151x phy

Question: pause mode disabled for marvell 88e151x phy

[Yunsheng Lin]

Hi, all
	When using the marvel 88e151x phy driver, the pause mode is disabled
by commit 6623c0fba10e when phy is inited as below:

 	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		u32 pause;
+
 		/* Select page 18 */
 		err = marvell_set_page(phydev, 18);
 		if (err < 0)
@@ -902,6 +904,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
 		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			return err;
+
+		/* There appears to be a bug in the 88e1512 when used in
+		 * SGMII to copper mode, where the AN advertisment register
+		 * clears the pause bits each time a negotiation occurs.
+		 * This means we can never be truely sure what was advertised,
+		 * so disable Pause support.
+		 */
+		pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+		phydev->supported &= ~pause;
+		phydev->advertising &= ~pause;
 	}


According to the commit log:
"Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
is unreliable.  While the pause bits can be set in the advertisment
register, they clear shortly after negotiation with a link partner
commences irrespective of the cause of the negotiation."

There seems to be some problem with pause subsequent negotiation.
We reverted the above patch and tried to reproduce the above problem
by triggering another negotiation by reconnection of the cable, using
ethtool -a cmd shows both still have tx and rx pause enable.

Some doubt about the above problem:
According to 88e151x datasheet and marvel phy driver in linux, the
88e1512 and 88e1514 support SGMII-to-Copper mode, so commit above
disables the pause support for all 88e151x phy using SGMII-to-Copper
mode.

1. Does all the 88e151x supporting SGMII-to-Copper have the above problem?

2. If not, can we use revision id field in phydev->phy_id to only disable
   the pause support for specific 88e151x phy? We can not find some useful
   revision info in datasheet, and by printing the phy id when phy init, we
   are able to find that the phy we are using has a phy id as 0x1d10dd1,
   which has revision id as 0x1.

3. Does this problem only happen marvel 88e1512 phy with some specific partner
   phy? We are unable to reproduce this problem, so any suggestion to reproduce
   this would be very helpful to us too.

4. Also the commit disables the pause support completely, if using revision id
   can not aviod this problem, can we only disable pause support when negotiation
   by only clearing pause support in phydev->advertising, but not phydev->supported?

If there are any better info or suggestion regarding this problem, it would be very
helpful, thanks in advance.

reference:
[1] https://lists.openwall.net/netdev/2017/12/15/174
[2] https://web.pa.msu.edu/hep/atlas/l1calo/htm/hardware/components/Enet_Phy/marvell_alaska_phy_88e151x_datasheet_jan18.pdf

Re: Question: pause mode disabled for marvell 88e151x phy

[Russell King - ARM Linux]

On Sat, Dec 15, 2018 at 04:07:42PM +0800, Yunsheng Lin wrote:
> There seems to be some problem with pause subsequent negotiation.
> We reverted the above patch and tried to reproduce the above problem
> by triggering another negotiation by reconnection of the cable, using
> ethtool -a cmd shows both still have tx and rx pause enable.

That's where the problem is - as far as the network device and Linux
is concerned, pause was successfully negotiated.  However, as the
advertisment register has ended up with the pause mode bits cleared,
Linux doesn't realise that what we conveyed to the partner was an
advertisment containing no pause mode bits.

ethtool doesn't read the PHY advertisment register when displaying
what we advertised, it returns what's in phydev->advertising - it
gives you the cached value not the this-is-what-the-hardware-is-doing
value.

> 1. Does all the 88e151x supporting SGMII-to-Copper have the above problem?

Unknown.

> 2. If not, can we use revision id field in phydev->phy_id to only disable
>    the pause support for specific 88e151x phy? We can not find some useful
>    revision info in datasheet, and by printing the phy id when phy init, we
>    are able to find that the phy we are using has a phy id as 0x1d10dd1,
>    which has revision id as 0x1.

0x01d10dd1 doesn't look to be a Marvell part - Marvell parts generally
start with 0x0141....  Is your 0x1d1 a typo?  My device is 0x01410dd1.

> 3. Does this problem only happen marvel 88e1512 phy with some specific partner
>    phy? We are unable to reproduce this problem, so any suggestion to reproduce
>    this would be very helpful to us too.

I don't think you've proven that you do not have a problem (see below
for how to do this.)

> 4. Also the commit disables the pause support completely, if using revision id
>    can not aviod this problem, can we only disable pause support when negotiation
>    by only clearing pause support in phydev->advertising, but not phydev->supported?

No comment at present.


I think you first need to ensure that your observations are correct.
You are basing your assumptions on ethtool -a's output, which is
definitely wrong as I've mentioned above.

You need to read directly from the hardware using mii-diag -v ethN
and manually decode the advertisment register (register 4) checking
bits 11 and 10 (the pause mode bits).  My observation is that Linux
can set these bits, but then both bits clear during the negotiation
process.

If you then find that these bits do not clear, then we need further
research to work out why there's a different behaviour between the
device I've tested here and the device that you are testing.

-- 
RMK's Patch system: http://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: Question: pause mode disabled for marvell 88e151x phy

[Yunsheng Lin]

On 2018/12/15 18:37, Russell King - ARM Linux wrote:
> On Sat, Dec 15, 2018 at 04:07:42PM +0800, Yunsheng Lin wrote:
>> There seems to be some problem with pause subsequent negotiation.
>> We reverted the above patch and tried to reproduce the above problem
>> by triggering another negotiation by reconnection of the cable, using
>> ethtool -a cmd shows both still have tx and rx pause enable.
> 
> That's where the problem is - as far as the network device and Linux
> is concerned, pause was successfully negotiated.  However, as the
> advertisment register has ended up with the pause mode bits cleared,
> Linux doesn't realise that what we conveyed to the partner was an
> advertisment containing no pause mode bits.
> 
> ethtool doesn't read the PHY advertisment register when displaying
> what we advertised, it returns what's in phydev->advertising - it
> gives you the cached value not the this-is-what-the-hardware-is-doing
> value.
> 
>> 1. Does all the 88e151x supporting SGMII-to-Copper have the above problem?
> 
> Unknown.
> 
>> 2. If not, can we use revision id field in phydev->phy_id to only disable
>>    the pause support for specific 88e151x phy? We can not find some useful
>>    revision info in datasheet, and by printing the phy id when phy init, we
>>    are able to find that the phy we are using has a phy id as 0x1d10dd1,
>>    which has revision id as 0x1.
> 
> 0x01d10dd1 doesn't look to be a Marvell part - Marvell parts generally
> start with 0x0141....  Is your 0x1d1 a typo?  My device is 0x01410dd1.

Sorry, 0x1d1 is a typo.
My device is also 0x01410dd1.

> 
>> 3. Does this problem only happen marvel 88e1512 phy with some specific partner
>>    phy? We are unable to reproduce this problem, so any suggestion to reproduce
>>    this would be very helpful to us too.
> 
> I don't think you've proven that you do not have a problem (see below
> for how to do this.)
> 
>> 4. Also the commit disables the pause support completely, if using revision id
>>    can not aviod this problem, can we only disable pause support when negotiation
>>    by only clearing pause support in phydev->advertising, but not phydev->supported?
> 
> No comment at present.
> 
> 
> I think you first need to ensure that your observations are correct.
> You are basing your assumptions on ethtool -a's output, which is
> definitely wrong as I've mentioned above.
> 
> You need to read directly from the hardware using mii-diag -v ethN
> and manually decode the advertisment register (register 4) checking
> bits 11 and 10 (the pause mode bits).  My observation is that Linux
> can set these bits, but then both bits clear during the negotiation
> process.

Thanks for the info.

Using arm64 with marvel 88e1512 phy connected to a X86 with intel phy,
The 88e1512 phy' advertisment register did change after negotiation:

arm64 with marvel 88e1512 phy:
 MII PHY #1 transceiver registers:
   3100 796d 0141 0dd1 05e1 cde1 000d 2001
   4006 0200 3800 0000 0000 0003 0000 3000
   3060 af08 0000 0000 0020 0000 0000 0000
   0000 0000 0040 0000 0000 0000 0000 0000

X86 with intel phy:
   1140 796d 0154 03b1 0de1 c5e1 000d 2001
   6801 0600 7800 0000 0000 0000 0000 3000
   0000 000a 840a 1075 0000 000c ff08 3048
   0000 816c 1ac6 0003 210a 1f55 0000 c064

But ethtool -a on both arm64 and X86 shows that tx and rx pause are
both enabled.

It seems ADVERTISE_PAUSE_ASYM is cleared on arm64 with marvel 88e1512 phy.
Does your device with marvel 88e1512 phy have the smiliar output as above?

And in include/linux/mii.h, we have:
/**
 * mii_resolve_flowctrl_fdx
 * @lcladv: value of MII ADVERTISE register
 * @rmtadv: value of MII LPA register
 *
 * Resolve full duplex flow control as per IEEE 802.3-2005 table 28B-3
 */
static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
{
	u8 cap = 0;

	if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
		cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
	} else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
		if (lcladv & ADVERTISE_PAUSE_CAP)
			cap = FLOW_CTRL_RX;
		else if (rmtadv & ADVERTISE_PAUSE_CAP)
			cap = FLOW_CTRL_TX;
	}

	return cap;
}

As the comment has pointed to IEEE 802.3-2005 table 28B-3:
http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf

ADVERTISE_PAUSE_ASYM on local and remote is a "Don’t care"
bit when they both support ADVERTISE_PAUSE_CAP, so maybe it is ok
that marvel phy clears the ADVERTISE_PAUSE_ASYM bit.

Any thought?


> 
> If you then find that these bits do not clear, then we need further
> research to work out why there's a different behaviour between the
> device I've tested here and the device that you are testing.
> 

Re: Question: pause mode disabled for marvell 88e151x phy

[Russell King - ARM Linux]

On Mon, Dec 17, 2018 at 05:42:20PM +0800, Yunsheng Lin wrote:
> On 2018/12/15 18:37, Russell King - ARM Linux wrote:
> > On Sat, Dec 15, 2018 at 04:07:42PM +0800, Yunsheng Lin wrote:
> >> There seems to be some problem with pause subsequent negotiation.
> >> We reverted the above patch and tried to reproduce the above problem
> >> by triggering another negotiation by reconnection of the cable, using
> >> ethtool -a cmd shows both still have tx and rx pause enable.
> > 
> > That's where the problem is - as far as the network device and Linux
> > is concerned, pause was successfully negotiated.  However, as the
> > advertisment register has ended up with the pause mode bits cleared,
> > Linux doesn't realise that what we conveyed to the partner was an
> > advertisment containing no pause mode bits.
> > 
> > ethtool doesn't read the PHY advertisment register when displaying
> > what we advertised, it returns what's in phydev->advertising - it
> > gives you the cached value not the this-is-what-the-hardware-is-doing
> > value.
> > 
> >> 1. Does all the 88e151x supporting SGMII-to-Copper have the above problem?
> > 
> > Unknown.
> > 
> >> 2. If not, can we use revision id field in phydev->phy_id to only disable
> >>    the pause support for specific 88e151x phy? We can not find some useful
> >>    revision info in datasheet, and by printing the phy id when phy init, we
> >>    are able to find that the phy we are using has a phy id as 0x1d10dd1,
> >>    which has revision id as 0x1.
> > 
> > 0x01d10dd1 doesn't look to be a Marvell part - Marvell parts generally
> > start with 0x0141....  Is your 0x1d1 a typo?  My device is 0x01410dd1.
> 
> Sorry, 0x1d1 is a typo.
> My device is also 0x01410dd1.
> 
> > 
> >> 3. Does this problem only happen marvel 88e1512 phy with some specific partner
> >>    phy? We are unable to reproduce this problem, so any suggestion to reproduce
> >>    this would be very helpful to us too.
> > 
> > I don't think you've proven that you do not have a problem (see below
> > for how to do this.)
> > 
> >> 4. Also the commit disables the pause support completely, if using revision id
> >>    can not aviod this problem, can we only disable pause support when negotiation
> >>    by only clearing pause support in phydev->advertising, but not phydev->supported?
> > 
> > No comment at present.
> > 
> > 
> > I think you first need to ensure that your observations are correct.
> > You are basing your assumptions on ethtool -a's output, which is
> > definitely wrong as I've mentioned above.
> > 
> > You need to read directly from the hardware using mii-diag -v ethN
> > and manually decode the advertisment register (register 4) checking
> > bits 11 and 10 (the pause mode bits).  My observation is that Linux
> > can set these bits, but then both bits clear during the negotiation
> > process.
> 
> Thanks for the info.
> 
> Using arm64 with marvel 88e1512 phy connected to a X86 with intel phy,
> The 88e1512 phy' advertisment register did change after negotiation:
> 
> arm64 with marvel 88e1512 phy:
>  MII PHY #1 transceiver registers:
>    3100 796d 0141 0dd1 05e1 cde1 000d 2001
>    4006 0200 3800 0000 0000 0003 0000 3000
>    3060 af08 0000 0000 0020 0000 0000 0000
>    0000 0000 0040 0000 0000 0000 0000 0000
> 
> X86 with intel phy:
>    1140 796d 0154 03b1 0de1 c5e1 000d 2001
>    6801 0600 7800 0000 0000 0000 0000 3000
>    0000 000a 840a 1075 0000 000c ff08 3048
>    0000 816c 1ac6 0003 210a 1f55 0000 c064
> 
> But ethtool -a on both arm64 and X86 shows that tx and rx pause are
> both enabled.

I'll say this again, ignore ethtool when it comes to this problem.
ethtool uses cached information to compute the pause settings.

> And in include/linux/mii.h, we have:
> /**
>  * mii_resolve_flowctrl_fdx
>  * @lcladv: value of MII ADVERTISE register
>  * @rmtadv: value of MII LPA register
>  *
>  * Resolve full duplex flow control as per IEEE 802.3-2005 table 28B-3
>  */
> static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
> {
> 	u8 cap = 0;
> 
> 	if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
> 		cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> 	} else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
> 		if (lcladv & ADVERTISE_PAUSE_CAP)
> 			cap = FLOW_CTRL_RX;
> 		else if (rmtadv & ADVERTISE_PAUSE_CAP)
> 			cap = FLOW_CTRL_TX;
> 	}
> 
> 	return cap;
> }

Not used by the marvell PHY driver.  It uses this code instead:

                if (phydev->duplex == DUPLEX_FULL) {
                        phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
                        phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
                }

and then its up to the network driver to decide what to do with
phydev->pause and phydev->asym_pause.


> As the comment has pointed to IEEE 802.3-2005 table 28B-3:
> http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf
> 
> ADVERTISE_PAUSE_ASYM on local and remote is a "Don’t care"
> bit when they both support ADVERTISE_PAUSE_CAP, so maybe it is ok
> that marvel phy clears the ADVERTISE_PAUSE_ASYM bit.

As I've previously stated, the behaviour I've seen is _both_ pause bits
clear:

If I set bit 10 (pause), and read back to confirm:

  MII PHY #0 transceiver registers:
   1000 796d 0141 0dd1 05e1 c5e1 000d 2001
                       ^^^^
   4806 0200 3800 0000 0000 0003 0000 3000
   3060 af48 0000 7c40 0020 0000 0000 0000
   0000 0000 0040 0000 0000 0000 0000 0000.

Now if I trigger a renegotiation of any kind, and read-back the registers:

 MII PHY #0 transceiver registers:
   1000 7949 0141 0dd1 01e1 0000 0004 2001
                       ^^^^
   0000 0200 0000 0000 0000 0003 0000 3000
   3060 8000 0000 0040 0020 0000 0000 0000
   0000 0000 0040 0000 0000 0000 0000 0000.
...
 MII PHY #0 transceiver registers:
   1000 796d 0141 0dd1 01e1 c5e1 000d 2001
                       ^^^^
   4806 0200 3800 0000 0000 0003 0000 3000
   3060 af48 0000 7c40 0020 0000 0000 0000
   0000 0000 0040 0000 0000 0000 0000 0000.

See that register 4 now has the pause bit cleared.

I don't know what causes it, other than the fact it does occur.  My
supposition is that it's something to do with the SGMII connection to
the MAC, possibly a Marvell extension that somehow causes the copper
advertisment register to adopt values from the SGMII side, maybe bits
in the config word embedded in the SGMII stream.  If that is the case,
it's likely to be MAC specific.

I don't have anything further beyond the observed behaviour right now,
and the fact that if the bits are set, then we end up with a mismatched
pause negotiation status (pause apparently negotiated as far as _we_
are concerned, but _not_ as far as the link partner is concerned.)

I'll try to do further diagnosis over Christmas in case I've missed
something, but I suspect it may be one of those "weird behaviour" issues
where the safest action is to disable pause mode as per my commit -
which is far saner than having mismatched pause status on either end
of a link.  However, given that Marvell specs are all NDA-only, it's
very difficult to investigate beyond "this is the observed behaviour".

-- 
RMK's Patch system: http://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: Question: pause mode disabled for marvell 88e151x phy

[Yunsheng Lin]

On 2018/12/17 22:36, Russell King - ARM Linux wrote:
> On Mon, Dec 17, 2018 at 05:42:20PM +0800, Yunsheng Lin wrote:
>> On 2018/12/15 18:37, Russell King - ARM Linux wrote:
>>> On Sat, Dec 15, 2018 at 04:07:42PM +0800, Yunsheng Lin wrote:
>>>> There seems to be some problem with pause subsequent negotiation.
>>>> We reverted the above patch and tried to reproduce the above problem
>>>> by triggering another negotiation by reconnection of the cable, using
>>>> ethtool -a cmd shows both still have tx and rx pause enable.
>>>
>>> That's where the problem is - as far as the network device and Linux
>>> is concerned, pause was successfully negotiated.  However, as the
>>> advertisment register has ended up with the pause mode bits cleared,
>>> Linux doesn't realise that what we conveyed to the partner was an
>>> advertisment containing no pause mode bits.
>>>
>>> ethtool doesn't read the PHY advertisment register when displaying
>>> what we advertised, it returns what's in phydev->advertising - it
>>> gives you the cached value not the this-is-what-the-hardware-is-doing
>>> value.
>>>
>>>> 1. Does all the 88e151x supporting SGMII-to-Copper have the above problem?
>>>
>>> Unknown.
>>>
>>>> 2. If not, can we use revision id field in phydev->phy_id to only disable
>>>>    the pause support for specific 88e151x phy? We can not find some useful
>>>>    revision info in datasheet, and by printing the phy id when phy init, we
>>>>    are able to find that the phy we are using has a phy id as 0x1d10dd1,
>>>>    which has revision id as 0x1.
>>>
>>> 0x01d10dd1 doesn't look to be a Marvell part - Marvell parts generally
>>> start with 0x0141....  Is your 0x1d1 a typo?  My device is 0x01410dd1.
>>
>> Sorry, 0x1d1 is a typo.
>> My device is also 0x01410dd1.
>>
>>>
>>>> 3. Does this problem only happen marvel 88e1512 phy with some specific partner
>>>>    phy? We are unable to reproduce this problem, so any suggestion to reproduce
>>>>    this would be very helpful to us too.
>>>
>>> I don't think you've proven that you do not have a problem (see below
>>> for how to do this.)
>>>
>>>> 4. Also the commit disables the pause support completely, if using revision id
>>>>    can not aviod this problem, can we only disable pause support when negotiation
>>>>    by only clearing pause support in phydev->advertising, but not phydev->supported?
>>>
>>> No comment at present.
>>>
>>>
>>> I think you first need to ensure that your observations are correct.
>>> You are basing your assumptions on ethtool -a's output, which is
>>> definitely wrong as I've mentioned above.
>>>
>>> You need to read directly from the hardware using mii-diag -v ethN
>>> and manually decode the advertisment register (register 4) checking
>>> bits 11 and 10 (the pause mode bits).  My observation is that Linux
>>> can set these bits, but then both bits clear during the negotiation
>>> process.
>>
>> Thanks for the info.
>>
>> Using arm64 with marvel 88e1512 phy connected to a X86 with intel phy,
>> The 88e1512 phy' advertisment register did change after negotiation:
>>
>> arm64 with marvel 88e1512 phy:
>>  MII PHY #1 transceiver registers:
>>    3100 796d 0141 0dd1 05e1 cde1 000d 2001
>>    4006 0200 3800 0000 0000 0003 0000 3000
>>    3060 af08 0000 0000 0020 0000 0000 0000
>>    0000 0000 0040 0000 0000 0000 0000 0000
>>
>> X86 with intel phy:
>>    1140 796d 0154 03b1 0de1 c5e1 000d 2001
>>    6801 0600 7800 0000 0000 0000 0000 3000
>>    0000 000a 840a 1075 0000 000c ff08 3048
>>    0000 816c 1ac6 0003 210a 1f55 0000 c064
>>
>> But ethtool -a on both arm64 and X86 shows that tx and rx pause are
>> both enabled.
> 
> I'll say this again, ignore ethtool when it comes to this problem.
> ethtool uses cached information to compute the pause settings.
> 
>> And in include/linux/mii.h, we have:
>> /**
>>  * mii_resolve_flowctrl_fdx
>>  * @lcladv: value of MII ADVERTISE register
>>  * @rmtadv: value of MII LPA register
>>  *
>>  * Resolve full duplex flow control as per IEEE 802.3-2005 table 28B-3
>>  */
>> static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
>> {
>> 	u8 cap = 0;
>>
>> 	if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
>> 		cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
>> 	} else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
>> 		if (lcladv & ADVERTISE_PAUSE_CAP)
>> 			cap = FLOW_CTRL_RX;
>> 		else if (rmtadv & ADVERTISE_PAUSE_CAP)
>> 			cap = FLOW_CTRL_TX;
>> 	}
>>
>> 	return cap;
>> }
> 
> Not used by the marvell PHY driver.  It uses this code instead:
> 
>                 if (phydev->duplex == DUPLEX_FULL) {
>                         phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
>                         phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
>                 }
> 
> and then its up to the network driver to decide what to do with
> phydev->pause and phydev->asym_pause.

Thanks, I see.

> 
> 
>> As the comment has pointed to IEEE 802.3-2005 table 28B-3:
>> http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf
>>
>> ADVERTISE_PAUSE_ASYM on local and remote is a "Don’t care"
>> bit when they both support ADVERTISE_PAUSE_CAP, so maybe it is ok
>> that marvel phy clears the ADVERTISE_PAUSE_ASYM bit.
> 
> As I've previously stated, the behaviour I've seen is _both_ pause bits
> clear:
> 
> If I set bit 10 (pause), and read back to confirm:
> 
>   MII PHY #0 transceiver registers:
>    1000 796d 0141 0dd1 05e1 c5e1 000d 2001
>                        ^^^^
>    4806 0200 3800 0000 0000 0003 0000 3000
>    3060 af48 0000 7c40 0020 0000 0000 0000
>    0000 0000 0040 0000 0000 0000 0000 0000.
> 
> Now if I trigger a renegotiation of any kind, and read-back the registers:
> 
>  MII PHY #0 transceiver registers:
>    1000 7949 0141 0dd1 01e1 0000 0004 2001
>                        ^^^^
>    0000 0200 0000 0000 0000 0003 0000 3000
>    3060 8000 0000 0040 0020 0000 0000 0000
>    0000 0000 0040 0000 0000 0000 0000 0000.
> ...
>  MII PHY #0 transceiver registers:
>    1000 796d 0141 0dd1 01e1 c5e1 000d 2001
>                        ^^^^
>    4806 0200 3800 0000 0000 0003 0000 3000
>    3060 af48 0000 7c40 0020 0000 0000 0000
>    0000 0000 0040 0000 0000 0000 0000 0000.
> 
> See that register 4 now has the pause bit cleared.

I wonder when the the pause bit was clear, is it during negotiation
process or after negotiation? if the partner phy see the pause bit before
the pause bit is clear?

Also, it seems the phy driver always set the pause bit back before
begining a negotiation. But I am not sure it makes a difference here.

> 
> I don't know what causes it, other than the fact it does occur.  My
> supposition is that it's something to do with the SGMII connection to
> the MAC, possibly a Marvell extension that somehow causes the copper
> advertisment register to adopt values from the SGMII side, maybe bits
> in the config word embedded in the SGMII stream.  If that is the case,
> it's likely to be MAC specific.
> 
> I don't have anything further beyond the observed behaviour right now,
> and the fact that if the bits are set, then we end up with a mismatched
> pause negotiation status (pause apparently negotiated as far as _we_
> are concerned, but _not_ as far as the link partner is concerned.)
> 
> I'll try to do further diagnosis over Christmas in case I've missed

Thanks for this. If there is anything you like to try on our device, we
are happy to do it and report back.

> something, but I suspect it may be one of those "weird behaviour" issues
> where the safest action is to disable pause mode as per my commit -
> which is far saner than having mismatched pause status on either end
> of a link.  However, given that Marvell specs are all NDA-only, it's
> very difficult to investigate beyond "this is the observed behaviour".
> 

Re: Question: pause mode disabled for marvell 88e151x phy

[Russell King - ARM Linux]

On Tue, Dec 18, 2018 at 05:34:27PM +0800, Yunsheng Lin wrote:
> On 2018/12/17 22:36, Russell King - ARM Linux wrote:
> > As I've previously stated, the behaviour I've seen is _both_ pause bits
> > clear:
> > 
> > If I set bit 10 (pause), and read back to confirm:
> > 
> >   MII PHY #0 transceiver registers:
> >    1000 796d 0141 0dd1 05e1 c5e1 000d 2001
> >                        ^^^^
> >    4806 0200 3800 0000 0000 0003 0000 3000
> >    3060 af48 0000 7c40 0020 0000 0000 0000
> >    0000 0000 0040 0000 0000 0000 0000 0000.
> > 
> > Now if I trigger a renegotiation of any kind, and read-back the registers:
> > 
> >  MII PHY #0 transceiver registers:
> >    1000 7949 0141 0dd1 01e1 0000 0004 2001
> >                        ^^^^
> >    0000 0200 0000 0000 0000 0003 0000 3000
> >    3060 8000 0000 0040 0020 0000 0000 0000
> >    0000 0000 0040 0000 0000 0000 0000 0000.
> > ...
> >  MII PHY #0 transceiver registers:
> >    1000 796d 0141 0dd1 01e1 c5e1 000d 2001
> >                        ^^^^
> >    4806 0200 3800 0000 0000 0003 0000 3000
> >    3060 af48 0000 7c40 0020 0000 0000 0000
> >    0000 0000 0040 0000 0000 0000 0000 0000.
> > 
> > See that register 4 now has the pause bit cleared.
> 
> I wonder when the the pause bit was clear, is it during negotiation
> process or after negotiation? if the partner phy see the pause bit before
> the pause bit is clear?

As was stated in the original commit:

    While these bits may be correctly conveyed to the link partner on the
    first negotiation, a subsequent negotiation (eg, due to negotiation
    restart by the link partner, or reconnection of the cable) will result
    in the link partner seeing these bits as zero, while the kernel
    believes that it has advertised pause modes.

To state in a different way:

    On the _first_ negotiation, the pause bits as seen by the link partner
    will be what we wrote into the register.  However, during that
    negotiation the bits in the register clear _after_ the base page has
    been sent to the link partner.

    A subsequent negotiation will send the now-clear pause bits to the
    link partner.

I hope that's now clear.

> Also, it seems the phy driver always set the pause bit back before
> begining a negotiation. But I am not sure it makes a difference here.

There is a world of difference between asking the kernel to do a
renegotiation, and unplugging/replugging the cable, or asking the
link partner to do a renegotiation.

In the former case, yes, the kernel will reprogram the advertisement
register.  In the latter two cases, the kernel does not and this is
where the problem occurs.

-- 
RMK's Patch system: http://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: Question: pause mode disabled for marvell 88e151x phy

[Yunsheng Lin]

On 2018/12/17 22:36, Russell King - ARM Linux wrote:
> I'll try to do further diagnosis over Christmas in case I've missed
> something, but I suspect it may be one of those "weird behaviour" issues
> where the safest action is to disable pause mode as per my commit -
> which is far saner than having mismatched pause status on either end
> of a link.  However, given that Marvell specs are all NDA-only, it's
> very difficult to investigate beyond "this is the observed behaviour".

Hi,

Is there any update on the further diagnosis?

> 

Re: Question: pause mode disabled for marvell 88e151x phy

[Russell King - ARM Linux admin]

On Sat, Jan 05, 2019 at 11:28:19AM +0800, Yunsheng Lin wrote:
> On 2018/12/17 22:36, Russell King - ARM Linux wrote:
> > I'll try to do further diagnosis over Christmas in case I've missed
> > something, but I suspect it may be one of those "weird behaviour" issues
> > where the safest action is to disable pause mode as per my commit -
> > which is far saner than having mismatched pause status on either end
> > of a link.  However, given that Marvell specs are all NDA-only, it's
> > very difficult to investigate beyond "this is the observed behaviour".
> 
> Hi,
> 
> Is there any update on the further diagnosis?

Hi,

I've finally been able to do some further diagnosis (with a 'scope).
It would appear that the network adapter had PHY polling enabled,
which meant that it overwrote the PHYs advertisement register
during negotiation.  I thought I'd checked that scenario, but alas
clearing the PHY poll enable bit on its own doesn't stop it polling!

I'll send a revert for the commit shortly.

-- 
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: Question: pause mode disabled for marvell 88e151x phy

[Yunsheng Lin]

On 2019/1/24 6:41, Russell King - ARM Linux admin wrote:
> On Sat, Jan 05, 2019 at 11:28:19AM +0800, Yunsheng Lin wrote:
>> On 2018/12/17 22:36, Russell King - ARM Linux wrote:
>>> I'll try to do further diagnosis over Christmas in case I've missed
>>> something, but I suspect it may be one of those "weird behaviour" issues
>>> where the safest action is to disable pause mode as per my commit -
>>> which is far saner than having mismatched pause status on either end
>>> of a link.  However, given that Marvell specs are all NDA-only, it's
>>> very difficult to investigate beyond "this is the observed behaviour".
>>
>> Hi,
>>
>> Is there any update on the further diagnosis?
> 
> Hi,
> 
> I've finally been able to do some further diagnosis (with a 'scope).
> It would appear that the network adapter had PHY polling enabled,
> which meant that it overwrote the PHYs advertisement register
> during negotiation.  I thought I'd checked that scenario, but alas
> clearing the PHY poll enable bit on its own doesn't stop it polling!
> 
> I'll send a revert for the commit shortly.

Great. Thanks for the further diagnosis.

>