From: Vladimir Oltean <olteanv@gmail.com>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Antoine Tenart <atenart@kernel.org>,
Quentin Schulz <quentin.schulz@bootlin.com>,
Michael Walle <michael@walle.cc>,
netdev@vger.kernel.org, Heiner Kallweit <hkallweit1@gmail.com>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
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: Sun, 14 Feb 2021 13:10:14 +0200 [thread overview]
Message-ID: <20210214111014.edr7uqezqdzrrr7w@skbuf> (raw)
In-Reply-To: <20210214103529.GT1463@shell.armlinux.org.uk>
On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote:
> > + if (ret && ret != -EOPNOTSUPP) {
> > + phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
> > + ret);
>
> Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value.
I didn't know that was possible, thanks for the hint.
> As mentioned in this thread, we have at least one PHY which is unable
> to provide the inband signalling in any mode (BCM84881). Currently,
> phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
> adjusts not to use inband mode. This would need to be addressed if we
> are creating an alterative way to discover whether the PHY supports
> inband mode or not.
So I haven't studied the SFP code path too deeply, but I think part of
the issue is the order in which things are done. It's almost as if there
should be a validation phase for PHY inband abilities too.
phylink_sfp_connect_phy
-> phylink_sfp_config:
-> first this checks if phylink_phy_no_inband
-> then this changes pl->link_config.interface and pl->cur_link_an_mode
-> phylink_bringup_phy:
-> this is where I'm adding the new phy_config_inband_aneg function
If we were to use only my phy_config_inband_aneg function, it would need
to be moved upwards in the code path, to be precise where phylink_phy_no_inband
currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback
to MLO_AN_PHY if that fails. I think this would unnecessarily complicate
the code.
Alternatively, I could create a second PHY driver method, simply for
validation of supported inband modes. The validation can be done in
place of the current phylink_phy_noinband(), and I can still have the
phy_config_inband_aneg() where I put it now, since we shouldn't have any
surprises w.r.t. supported operating mode, and there should be no reason
to repeat the attempt as there would be with a single PHY driver method.
Thoughts?
> Also, there needs to be consideration of PHYs that dynamically change
> their interface type, and whether they support inband signalling.
> For example, a PHY may support a mode where it dynamically selects
> between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
> mode may have inband signalling enabled or disabled. This is not a
> theoretical case; we have a PHY like that supported in the kernel and
> boards use it. What would the semantics of your new call be for a PHY
> that performs this?
>
> Should we also have a phydev->inband tristate, taking values "unknown,
> enabled, disabled" which the PHY driver is required to update in their
> read_status callback if they dynamically change their interface type?
> (Although then phylink will need to figure out how to deal with that.)
I don't have such PHY to test with, but I think the easiest way would be
to just call the validation method again, after we change the
phydev->interface value. The PHY driver can easily take phydev->interface
into consideration when answering the question "is inband aneg supported
or not". I don't think that making phydev->inband a stateful value is
going to be as useful as making it a function, since as you say, we will
be required to keep it up to date from generic PHY driver methods, but
only phylink will use it.
next prev parent reply other threads:[~2021-02-14 11:11 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
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 [this message]
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=20210214111014.edr7uqezqdzrrr7w@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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).