From: Russell King - ARM Linux admin <email@example.com> To: Vladimir Oltean <firstname.lastname@example.org> Cc: Andrew Lunn <email@example.com>, Florian Fainelli <firstname.lastname@example.org>, Heiner Kallweit <email@example.com>, "David S. Miller" <firstname.lastname@example.org>, netdev <email@example.com> Subject: Re: [PATCH net-next 0/5] add phylink support for PCS Date: Wed, 11 Mar 2020 17:05:41 +0000 [thread overview] Message-ID: <20200311170541.GP25745@shell.armlinux.org.uk> (raw) In-Reply-To: <CA+h21hpk+TMofHFjg_Z-UZOPp+7zn29ZNLFP+JKreJtbZouiZQ@mail.gmail.com> On Wed, Mar 11, 2020 at 03:57:18PM +0200, Vladimir Oltean wrote: > On Wed, 11 Mar 2020 at 14:54, Russell King - ARM Linux admin > <firstname.lastname@example.org> wrote: > > > > On Wed, Mar 11, 2020 at 02:46:33PM +0200, Vladimir Oltean wrote: > > > Hi Russell, > > > > > > On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin > > > <email@example.com> wrote: > > > > > > > > Hi, > > > > > > > > This series adds support for IEEE 802.3 register set compliant PCS > > > > for phylink. In order to do this, we: > > > > > > > > 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function > > > > to a linkmode variant. > > > > 2. add a helper for clause 37 advertisements, supporting both the > > > > 1000baseX and defacto 2500baseX variants. Note that ethtool does > > > > not support half duplex for either of these, and we make no effort > > > > to do so. > > > > 3. add accessors for modifying a MDIO device register, and use them in > > > > phylib, rather than duplicating the code from phylib. > > > > > > Have you considered accessing the PCS as a phy_device structure, a la > > > drivers/net/dsa/ocelot/felix_vsc9959.c? > > > > I don't want to tie this into phylib, because I don't think phylib > > should be dealing with PCS. It brings with it many problems, such as: > > > > Agree that the struct mdio_device -> struct phy_device diff is pretty > much useless to a PCS. > > > 1. how do we know whether the Clause 22 registers are supposed to be > > Clause 37 format. > > Well, they are, aren't they? For a PCS, yes, but phylib generally deals with clause 22 format registers (which define the copper capabilities rather than 1000baseX which clause 37 covers.) > > > 2. how do we program the PCS appropriately for the negotiation results > > (which phylib doesn't support). > > You mean how to read the LPA and logically-AND it with ADV? > The PCS doesn't need to be "programmed" according to the resolved link > state. Maybe the MAC does. No, I'm talking about configuring the PCS for SGMII mode when in-band AN is not being used. > > 3. how do we deal with selecting the appropriate device for the mode > > selected (LX2160A has multiple different PCS which depend on the > > mode selected.) > > What I've been doing is to call get_phy_device with an is_c45 argument > depending on the PHY interface type. > Actually the real problem in your case is that the LX2160A doesn't > expose a valid PHY ID in registers 2&3 (unlike other Layerscape PCS > implementations), so get_phy_device is likely going to fail unless > some sort of PHY ID fixup is not done. What SolidRun are pressing NXP for on the LX2160A is to move the networking support to a more dynamic arrangement than it is at present - I know there was a conference call on Monday about this, but I only found out about it too late to be part of it, and so far no one has filled me in on what was discussed. However, SolidRun wish the networking to be more dynamically configurable on the LX2160A - right now, we have a problem that we can either configure _all_ the QSFP+ and SFP ports (e.g.) to 10G mode, or _all_ QSFP+ and SFP ports to 1G mode - which basically makes the QSFP+ useless. It's too inflexible. What I would like to see are individual ports or groups of ports being able to be reconfigured on the fly, which means sticking to one particular PCS will not be possible. Other platforms do support dynamically switching between different components depending on the speed, and I see no reason to prevent such flexibility in phylink by designing into it a "you can only have one PCS device" assumption. > > Note that a phy_device structure embeds a mdio_device structure, and > > so these helpers can be used inside phylib if one desires - so this > > approach is more flexible than "bolt it into phylib" approach would > > be. > > > > It's hard to really say without seeing more than one caller of these > new helpers. > For example the sja1105 DSA switch has a PCS for SGMII (not supported > yet in mainline) that kind-of-emulates a C22 register map, except that > it's accessed over SPI, and that the "pcs_get_state" needs to look at > some vendor-specific registers too. I don't think "it's accessed over SPI" is much of an issue at all. The solution to that is trivial - as has been already shown with PHYs that are accessed over I2C. > From that perspective, I was > thinking that PHYLINK could be given a phy_device structure with the > advertising, supported and lp_advertising linkmode bit fields > populated who-knows-how, and PHYLINK just resolves that into its > phylink_link_state structure. > But then I guess that sort of hardware is not among your target > candidates for the generic helpers. Whoever can't expose an MDIO bus > or needs to access any vendor-specific register just shouldn't use > these functions. And maybe you're right, I don't really know what the > balance in practice will be. I don't see why you'd think that having a phy_device structure would make it easier to access a SPI based PHY. If you can't access the PHY over MDIO, none of the phylib helpers can be used, so you're just using struct phy_device as a container and wrapping it with a load of custom code. As long as the phy_device structure is registered with the device model, phy drivers can potentially bind with the "special" phy device and attempt to access it via the MDIO bus - it sounds like that's something you don't want to happen. So, I'd question whether it makes any sense to (ab)use struct phy_device for something that is not going to use phylib at all. It seems way more sensible to have a "struct pcs_device" that operates entirely separately to phylib - and maybe we can lift some of the phylib functionality to mdio_device level (such as what I've done with accessors, but maybe more stuff) so it can be spared between PCS and conventional phylib users. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
next prev parent reply other threads:[~2020-03-11 17:05 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-11 12:06 Russell King - ARM Linux admin 2020-03-11 12:07 ` [PATCH net-next 1/5] net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant Russell King 2020-03-11 12:07 ` [PATCH net-next 2/5] net: mii: add linkmode_adv_to_mii_adv_x() Russell King 2020-03-11 12:07 ` [PATCH net-next 3/5] net: mdiobus: add APIs for modifying a MDIO device register Russell King 2020-03-11 12:07 ` [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers Russell King 2020-03-11 14:06 ` Vladimir Oltean 2020-03-11 17:09 ` Russell King - ARM Linux admin 2020-03-11 18:44 ` Vladimir Oltean 2020-03-11 19:32 ` Russell King - ARM Linux admin 2020-03-11 19:59 ` Vladimir Oltean 2020-03-11 20:32 ` Russell King - ARM Linux admin 2020-03-12 12:54 ` Vladimir Oltean 2020-03-12 13:13 ` Russell King - ARM Linux admin 2020-03-12 13:35 ` Vladimir Oltean 2020-03-11 12:07 ` [PATCH net-next 5/5] net: phylink: pcs: add 802.3 clause 45 helpers Russell King 2020-03-11 12:46 ` [PATCH net-next 0/5] add phylink support for PCS Vladimir Oltean 2020-03-11 12:54 ` Russell King - ARM Linux admin 2020-03-11 13:57 ` Vladimir Oltean 2020-03-11 17:05 ` Russell King - ARM Linux admin [this message] 2020-03-11 18:16 ` 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=20200311170541.GP25745@shell.armlinux.org.uk \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH net-next 0/5] add phylink support for PCS' \ /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).