Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

Am 2021-02-13 19:56, schrieb Vladimir Oltean:

On Sat, Feb 13, 2021 at 06:09:13PM +0100, Michael Walle wrote:

quoted

Am 2021-02-13 17:53, schrieb Michael Walle:

quoted

Am 2021-02-13 01:36, schrieb Vladimir Oltean:
But the Atheros PHY seems to have a problem with the SGMII link
if there is no autoneg.
No matter what I do, I can't get any traffic though if its not
gigabit on the copper side. Unfortunately, I don't have access
to an oscilloscope right now to see whats going on on the SGMII
link.

Scrap that. It will work if I set the speed/duplex mode in BMCR
correctly. (I tried that before, but I shifted one bit. doh).

So that will work, but when will it be done? There is no
callback to configure the PCS side of the PHY if a link up is
detected.

That's interesting/odd, on VSC8514 there is no need to force the speed
of the system side to what was negotiated on media side. I took a quick
look through the AR8033 datasheet and there isn't any mentioning the
ability to program the SGMII link according to internal state as 
opposed
to register settings, but it's equally possible that I'm simply not
seeing it.

I couldn't find anything in the AR8031 datasheet either.

On the other hand, I never meant for the inband autoneg setting to only
be configurable both ways.

Then why is there a "bool enabled"?

I expect some PHYs are not able to operate
using noinband mode, and for those I guess you should simply return
-EINVAL, allowing the system designer to know that the configuration
will not work and why.

You mean like this:

static int at803x_config_inband_aneg(struct phy_device *phydev, bool 
enabled)
{
	if (!enabled)
		return -EINVAL;
	/* enable SGMII autoneg */
	return phy_write_paged(...);
}

But then why bother with config_inband_aneg() at all and just enable
it unconditionally in config_init(). [and maybe keep the return 
-EINVAL].
Which then begs the question, does it makes sense on (Q)SGMII links at
all?

I think you could hook into .config_aneg_done, for the autoneg=true
case, and into .config_aneg for autoneg=false (I'm talking about 
autoneg
on media side here), but honestly I think the PHY is pretty broken for
requiring external coordination between the clause 28 and the clause 37
PCS. So unless there is a real need to configure noinband mode, I would
probably not bother.

Probably not.

-michael