netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Michael Walle <michael@walle.cc>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Antoine Tenart <atenart@kernel.org>,
	Quentin Schulz <quentin.schulz@bootlin.com>,
	netdev@vger.kernel.org, Heiner Kallweit <hkallweit1@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Maxim Kochetkov <fido_max@inbox.ru>,
	Bjarni Jonasson <bjarni.jonasson@microchip.com>,
	Steen Hegelund <steen.hegelund@microchip.com>,
	UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it
Date: Sat, 13 Feb 2021 22:12:55 +0200	[thread overview]
Message-ID: <20210213201255.ygjn7rexook2ngqe@skbuf> (raw)
In-Reply-To: <4b3f06686cb58dcdda582bfdbd0abb85@walle.cc>

On Sat, Feb 13, 2021 at 08:57:46PM +0100, Michael Walle wrote:
> > 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"?

Let me stress the word _only_ both ways. The whole point of the "bool
enabled" is to attempt coordination with the 'managed = "in-band-status"'
property device tree property of the MAC, or to error out if it is not
possible.

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

Because .config_init() is generic code, while .config_inband_autoneg()
is phylink-specific. Generally I don't want to make any assumption about
the state in which a PHY driver used to operate prior to this series.
If you are sure that at803x.c user relies on a prior bootloader stage
having disabled in-band AN, then sure, I suppose you can enable it
unconditionally in .config_init().

For VSC8514 I put the configuration deliberately in a phylink-specific
callback since I trust that at least the MAC-side drivers were reviewed
for proper use of MLO_AN_PHY vs MLO_AN_INBAND.

> Which then begs the question, does it makes sense on (Q)SGMII links at
> all?

See above. In the general case we need to assume a wild world where the
same PHY driver operates as inband on some platforms and noinband on
others (and most importantly, it works on both, which we'd like to
preserve at least). I would be glad if we didn't need to make that
assumption though.

  reply	other threads:[~2021-02-13 20:13 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-12 17:23 [PATCH net-next 0/2] Let phylink manage in-band AN for the PHY Vladimir Oltean
2021-02-12 17:23 ` [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it Vladimir Oltean
2021-02-12 22:40   ` Michael Walle
2021-02-13  0:18     ` Russell King - ARM Linux admin
2021-02-13 16:41       ` Michael Walle
2021-02-13 16:59         ` Andrew Lunn
2021-02-13 17:06         ` Russell King - ARM Linux admin
2021-02-13  0:36     ` Vladimir Oltean
2021-02-13 16:53       ` Michael Walle
2021-02-13 17:09         ` Michael Walle
2021-02-13 18:56           ` Vladimir Oltean
2021-02-13 19:57             ` Michael Walle
2021-02-13 20:12               ` Vladimir Oltean [this message]
2021-02-13 20:16               ` Russell King - ARM Linux admin
2021-02-14 10:35   ` Russell King - ARM Linux admin
2021-02-14 11:10     ` Vladimir Oltean
2021-02-14 13:18       ` Russell King - ARM Linux admin
2021-02-12 17:23 ` [PATCH net-next 2/2] net: phy: mscc: configure in-band auto-negotiation for VSC8514 Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210213201255.ygjn7rexook2ngqe@skbuf \
    --to=olteanv@gmail.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=atenart@kernel.org \
    --cc=bjarni.jonasson@microchip.com \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=fido_max@inbox.ru \
    --cc=hkallweit1@gmail.com \
    --cc=ioana.ciornei@nxp.com \
    --cc=kuba@kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=michael@walle.cc \
    --cc=netdev@vger.kernel.org \
    --cc=quentin.schulz@bootlin.com \
    --cc=steen.hegelund@microchip.com \
    --subject='Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).