From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Sean Anderson <sean.anderson@seco.com>
Cc: netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
linux-kernel@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
Heiner Kallweit <hkallweit1@gmail.com>,
Claudiu Beznea <claudiu.beznea@microchip.com>,
Nicolas Ferre <nicolas.ferre@microchip.com>
Subject: Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks
Date: Thu, 7 Oct 2021 17:23:36 +0100 [thread overview]
Message-ID: <YV8fCAkcIGY+yEmQ@shell.armlinux.org.uk> (raw)
In-Reply-To: <YV7Z/MF7geOp+JM2@shell.armlinux.org.uk>
On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:
> Here's a patch which illustrates roughly what I'm thinking at the
> moment - only build tested.
>
> mac_select_pcs() should not ever fail in phylink_major_config() - that
> would be a bug. I've hooked mac_select_pcs() also into the validate
> function so we can catch problems there, but we will need to involve
> the PCS in the interface selection for SFPs etc.
>
> Note that mac_select_pcs() must be inconsequential - it's asking the
> MAC which PCS it wishes to use for the interface mode.
>
> I am still very much undecided whether we wish phylink to parse the
> pcs-handle property and if present, override the MAC - I feel that
> logic depends in the MAC driver, since a single PCS can be very
> restrictive in terms of what interface modes are supportable. If the
> MAC wishes pcs-handle to override its internal ones, then it can
> always do:
>
> if (port->external_pcs)
> return port->external_pcs;
>
> in its mac_select_pcs() implementation. This gives us a bit of future
> flexibility.
>
> If we parse pcs-handle in phylink, then if we end up with multiple PCS
> to choose from, we then need to work out how to either allow the MAC
> driver to tell phylink not to parse pcs-handle, or we need some way for
> phylink to ask the MAC "these are the PCS I have, which one should I
> use" which is yet another interface.
>
> What I don't like about the patch is the need to query the PCS based on
> interface - when we have a SFP plugged in, it may support multiple
> interfaces. I think we still need the MAC to restrict what it returns
> in its validate() method according to the group of PCS that it has
> available for the SFP interface selection to work properly. Things in
> this regard should become easier _if_ I can switch phylink over to
> selecting interface based on phy_interface_t bitmaps rather than the
> current bodge using ethtool link modes, but that needs changes to phylib
> and all MAC drivers, otherwise we have to support two entirely separate
> ways to select the interface mode.
>
> My argument against that is... I'll end up converting the network
> interfaces that I use to the new implementation, and the old version
> will start to rot. I've already stopped testing phylink without a PCS
> attached for this very reason. The more legacy code we keep, the worse
> this problem becomes.
Having finished off the SFP side of the phy_interface_t bitmap
(http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
and I think the mac_select_pcs() approach will work.
See commit
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
where we have a set of phy_interface_t to choose one from, and if
we add PCS selection into that logic, the loop becomes:
static phy_interface_t phylink_select_interface(struct phylink *pl,
const unsigned long *intf
const char *intf_name)
{
phy_interface_t interface, intf;
...
interface = PHY_INTERFACE_MODE_NA;
for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
intf = phylink_sfp_interface_preference[i];
if (!test_bit(intf, u))
continue;
pcs = pl->pcs;
if (pl->mac_ops->mac_select_pcs) {
pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
if (!pcs)
continue;
}
if (pcs && !test_bit(intf, pcs->supported_interfaces))
continue;
interface = intf;
break;
}
...
}
The alternative would be to move some of that logic into
phylink_sfp_config_nophy(), and will mean knocking out bits from
the mask supplied to phylink_select_interface() each time we select
an interface mode that the PCS doesn't support... which sounds rather
more yucky to me.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
next prev parent reply other threads:[~2021-10-07 16:23 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-04 19:15 [RFC net-next PATCH 00/16] Add support for Xilinx PCS Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property Sean Anderson
2021-10-05 9:39 ` Russell King (Oracle)
2021-10-05 16:18 ` Sean Anderson
2021-10-12 13:16 ` Rob Herring
2021-10-12 16:18 ` Sean Anderson
2021-10-12 16:44 ` Rob Herring
2021-10-12 17:01 ` Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 02/16] dt-bindings: net: Add binding for Xilinx PCS Sean Anderson
2021-10-05 12:26 ` Rob Herring
2021-10-04 19:15 ` [RFC net-next PATCH 03/16] net: sfp: Fix typo in state machine debug string Sean Anderson
2021-10-04 21:31 ` Andrew Lunn
2021-10-04 19:15 ` [RFC net-next PATCH 04/16] net: phylink: Move phylink_set_pcs before phylink_create Sean Anderson
2021-10-05 9:43 ` Russell King (Oracle)
2021-10-04 19:15 ` [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices Sean Anderson
2021-10-05 9:48 ` Russell King (Oracle)
2021-10-05 16:42 ` Sean Anderson
2021-10-07 10:23 ` Russell King (Oracle)
2021-10-08 0:14 ` Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 06/16] net: phylink: Add function for optionally adding a PCS Sean Anderson
2021-10-05 9:51 ` Russell King (Oracle)
2021-10-05 13:43 ` Andrew Lunn
2021-10-05 16:17 ` Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 07/16] net: phylink: Add helpers for c22 registers without MDIO Sean Anderson
2021-10-22 12:33 ` Russell King (Oracle)
2021-10-04 19:15 ` [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate Sean Anderson
2021-10-04 23:04 ` Russell King (Oracle)
2021-10-04 23:09 ` Sean Anderson
2021-10-07 13:22 ` Nicolas Ferre
2021-10-08 0:20 ` Sean Anderson
2021-10-08 8:12 ` Nicolas Ferre
2021-10-04 19:15 ` [RFC net-next PATCH 09/16] net: macb: Move most of mac_prepare to mac_config Sean Anderson
2021-10-04 23:05 ` Russell King (Oracle)
2021-10-04 23:09 ` Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks Sean Anderson
2021-10-05 10:06 ` Russell King (Oracle)
2021-10-05 16:03 ` Sean Anderson
2021-10-05 18:53 ` Russell King (Oracle)
2021-10-05 21:44 ` Sean Anderson
2021-10-05 22:19 ` Russell King (Oracle)
2021-10-07 10:34 ` Russell King (Oracle)
2021-10-07 11:29 ` Russell King (Oracle)
2021-10-07 16:23 ` Russell King (Oracle) [this message]
2021-10-07 17:04 ` Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 11/16] net: macb: Support restarting PCS autonegotiation Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 12/16] net: macb: Support external PCSs Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 13/16] net: phy: Export get_phy_c22_id Sean Anderson
2021-10-05 10:12 ` Russell King (Oracle)
2021-10-04 19:15 ` [RFC net-next PATCH 14/16] net: mdio: Add helper functions for accessing MDIO devices Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 15/16] net: pcs: Add Xilinx PCS driver Sean Anderson
2021-10-04 19:15 ` [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs Sean Anderson
2021-10-04 22:01 ` Andrew Lunn
2021-10-05 10:33 ` Russell King (Oracle)
2021-10-05 16:45 ` Sean Anderson
2021-10-05 18:10 ` Sean Anderson
2021-10-05 19:12 ` Russell King (Oracle)
2021-10-05 20:38 ` Sean Anderson
2021-10-05 22:17 ` Russell King (Oracle)
2021-10-05 23:16 ` Sean Anderson
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=YV8fCAkcIGY+yEmQ@shell.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=andrew@lunn.ch \
--cc=claudiu.beznea@microchip.com \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=sean.anderson@seco.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).