Message-ID: <Y4h0UGXR7znwMWLo@gvm01>
Date:   Thu, 1 Dec 2022 10:30:56 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org
Subject: Re: Help on PHY not supporting autoneg

On Thu, Dec 01, 2022 at 03:14:51AM +0100, Andrew Lunn wrote:
> > /root # ethtool eth0
> > Settings for eth0:
> >         Supported ports: [ MII ]
> >         Supported link modes:   10baseT1S_P2MP/Half
> >         Supported pause frame use: Symmetric Receive-only
> >         Supports auto-negotiation: No
> >         Supported FEC modes: Not reported
> >         Advertised link modes:  10baseT1S_P2MP/Half
> >         Advertised pause frame use: Symmetric Receive-only
> 
> That looks odd. The PHY should indicate if it supports pause
> negotiation. Since this PHY does not support autoneg, it should not be
> saying it can negotiate pause. So i'm wondering why it is saying this
> here. Same for 'Supported pause'.
This is indeed a good question. This is the code snippet for the PHY
driver:

static int ncn26000_get_features(struct phy_device *phydev) {
	linkmode_zero(phydev->supported);
	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);

	linkmode_set_bit(
		ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
		phydev->supported
	);

	linkmode_copy(phydev->advertising, phydev->supported);
	return 0;
}

static int ncn26000_config_aneg(struct phy_device *phydev) {
	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
	phydev->mdix = ETH_TP_MDI;
	phydev->pause = 0;
	phydev->asym_pause = 0;
	phydev->speed = SPEED_10;
	phydev->duplex = DUPLEX_HALF;

	// bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
	// clear also ISOLATE mode and Collision Test
	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
}

As you can see there is no mention of PAUSE support in the features, and
the pause/asym_pause flags in phydev are set to 0.

Could it be a problem of the userspace tool?
Any advice on where to start looking?
 
> >         Advertised auto-negotiation: No
> >         Advertised FEC modes: Not reported
> >         Speed: 10Mb/s
> >         Duplex: Half
> >         Auto-negotiation: off
> >         Port: Twisted Pair
> >         PHYAD: 8
> >         Transceiver: external
> >         MDI-X: off (auto)
> 
> Given that this is a T1 PHY does MDI-X have any meaning? The (auto)
> indicates the PHY is returning mdix_ctrl=ETH_TP_MDI_AUTO, when it
> should be returning ETH_TP_MDI_INVALID to indicate it is not
> supported.
This is in fact debatable. The 10BASE-T1S has a unique feature making it
polarity insensitive. It's not like other xBASE-T1 PHYs that
auto-detects the polarity, the line coding (Differential Manchester) is
intrinsically polarity agnostic. Therefore I'm personally undecided on
the best approach.

On one hand, If the driver reports that MDI-X is not supported, the user
might think he needs to care about the polarity, which could be
misleading. On the other hand, there is no real auto-switch of TX/RX.

I'm not sure if/how 100BASE-T1 for example handles polarity, but we
probably need a common approach.

Thoughts?
 
> >         Supports Wake-on: d
> >         Wake-on: d
> >         Current message level: 0x0000003f (63)
> >                                drv probe link timer ifdown ifup
> >         Link detected: yes
> > 
> > 
> > > What exactly is LINK_CONTROL. It is not one of the Linux names for a
> > > bit in BMCR.
> > The 802.3cg standard define link_control as a varibale set by autoneg.
> > In factm it is tied to the BMCR_ANENABLE bit. The standard further
> > specifies that when AN is not supported, this bit can be supplied in a
> > vendor-specific way. A common thing to do is to just leave it tied to
> > the BMCR_ANENABLE bit.
> > 
> > So, the "problem" seems to lie in the genphy_setup_forced() function.
> > More precisely, where you pointed me at: 
> > >       return phy_modify(phydev, MII_BMCR,
> > >                         ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);
> > > 
> > 
> > In my view we have two choices to handle LINK_CONTROL.
> > 
> > 1. Just let the PHY driver override the config_aneg() callback as I did.
> > This may be a bit counter-intuitive because of the function name, but it
> > works.
> > 
> > 2. in phylib, distinguish the case of a PHY having aneg disabled from a
> > PHY NOT supporting aneg. In the latter case, don't touch the control
> > register and/or provide a separate callback for "starting" the PHY
> > (e.g., set_link_ctrl)
> 
> 2) sounds wrong. As you said, it is vendor-specific. As such you
> cannot trust it to mean anything. The standard has left it undefined,
> so you cannot do anything in generic code. I would also worry about
> breaking older PHYs who have never had this bit set.
Maybe I did not explain what I had in mind correctly.
I did not mean to set this bit, it is indeed vendor-specific.
The idea would be to define a new callback that by default does what we
have right now (clear all bits in register 0). But PHY drivers may
override it to actually do something meaningful, like setting the AN
bit.
> 
> So i would go with 1). As i said, the function name is not ideal, but
> it has been like this since forever.
I'm not strongly biased between 1 and 2, but I would first re-consider 2
before making a final decision.

Thank you very much for your kind reply.
Piergiorgio

Powered by blists - more mailing lists