Messages in this thread | | | Date | Fri, 6 Jan 2023 16:18:09 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation support from registers |
| |
On Thu, Jan 05, 2023 at 06:51:33PM +0000, Russell King (Oracle) wrote: > On Thu, Jan 05, 2023 at 07:43:42PM +0200, Vladimir Oltean wrote: > > On Thu, Jan 05, 2023 at 02:40:50PM +0000, Russell King (Oracle) wrote: > > > > If the PHY firmware uses a combination like this: 10GBASE-R/XFI for > > > > media speeds of 10G, 5G, 2.5G (rate adapted), and SGMII for 1G, 100M > > > > and 10M, a call to your implementation of > > > > aqr107_get_rate_matching(PHY_INTERFACE_MODE_10GBASER) would return > > > > RATE_MATCH_NONE, right? So only ETHTOOL_LINK_MODE_10000baseT_Full_BIT > > > > would be advertised on the media side? > > > > > > No, beause of the special condition in phylink that if it's a clause 45 > > > PHY and we use something like 10GBASE-R, we don't limit to just 10G > > > speed, but try all interface modes - on the assumption that the PHY > > > will switch its host interface. > > > > > > RATE_MATCH_NONE doesn't state anything about whether the PHY operates > > > in a single interface mode or not - with 10G PHYs (and thus clause 45 > > > PHYs) it seems very common from current observations for > > > implementations to do this kind of host-interface switching. > > > > So you mention commits > > 7642cc28fd37 ("net: phylink: fix PHY validation with rate adaption") and > > df3f57ac9605 ("net: phylink: extend clause 45 PHY validation workaround"). > > > > IIUC, these allow the advertised capabilities to be more than 10G (based > > on supported_interfaces), on the premise that it's possible for the PHY > > to switch SERDES protocol to achieve lower speeds. > > I didn't mention any commits, but yes, it's ever since the second commit > you list above, which was necessary to get PHYs which switch their > interface mode to work sanely. It essentially allows everything that > the combination of host and PHY supports, because we couldn't do much > better at the time that commit was written. > > > This does partly correct the last part of my question, but I believe > > that the essence of it still remains. We won't make use of PAUSE rate > > adaptation to support the speeds which aren't directly covered by the > > supported_interfaces. Aren't we interpreting the PHY provisioning somewhat > > too conservatively in this case, or do you believe that this is just an > > academic concern? > > Do you have a better idea how to come up with a list of link modes that > the PHY should advertise to its link partner and also report as > supported given the combination of: > > - PHYs that switch their host interface > - PHYs that may support some kind of rate adaption > - PCS/MACs that may support half-duplex at some speeds > - PCS/MACs that might support pause modes, and might support them only > with certain interface modes > > Layered on top of that is being able to determine which interface a PHY/ > PCS/MAC should be using when e.g. a 10G copper PHY is inserted (which > could be inserted into a host which only supports up to 1G.) > > I've spent considerable time trying to work out a solution to this, and > even before we had rate adaption, it isn't easy to solve. I've > experimented with several different solutions, and it's from numerous > trials that led to this host_interfaces/mac_capabilities structure - > but that still doesn't let us solve the problems I mention above since > we have no idea what the PHY itself is capable of, or how it's going to > behave, or really which interface modes it might switch between if it's > a clause 45 PHY. > > I've experimented with adding phy->supported_interfaces so a phylib > driver can advertise what interfaces it supports. I've also > experimented with phy->possible_interfaces which reports the interface > modes that the PHY _is_ going to switch between having selected its > operating mode. I've not submitted them because even with this, it all > still seems rather inadequate - and there is a huge amount of work to > update all the phylib drivers to provide even that basic information, > let alone have much confidence that it is correct. > > You can find these experiments, as normal, in my net-queue branch in > my git tree. These date from before we had rate adaption, so they take > no account of the recent addition of this extra variable.
Don't we actually need an API for the PHY resembling the following?
struct phy_host_cfg { phy_interface_t interface; int rate_matching; };
/* Caller must kfree() @host_cfg */ int phy_get_host_cfg_for_linkmode(struct phy_device *phydev, enum ethtool_link_mode_bit_indices linkmode, struct phy_host_cfg **host_cfg, int *num_host_cfg) { if (!phydev->drv->get_host_cfg_for_linkmode) { /* Assume that PHYs can't change host interface and don't * support rate matching */ *host_cfg = kcalloc(sizeof(*host_cfg), GFP_KERNEL); *num_host_cfg = 1; *host_cfg[0].interface = phydev->interface; *host_cfg[0].rate_matching = RATE_MATCH_NONE;
return 0; }
return phydev->drv->get_host_cfg_for_linkmode(phydev, linkmode, host_cfg, num_host_cfg); }
/* Calling this is only necessary if @num_host_cfg returned by * phy_get_host_cfg_for_linkmode() is larger than 1. */ int phy_set_host_cfg_for_linkmode(struct phy_device *phydev, enum ethtool_link_mode_bit_indices linkmode, const struct phy_host_cfg *host_cfg) { if (!phydev->drv->set_host_cfg_for_linkmode) return -EOPNOTSUPP;
return phydev->drv->set_host_cfg_for_linkmode(phydev, linkmode, host_cfg); }
Based on the host_cfg array returned by the PHY for each link mode, phylink could figure out (by intersecting with the MAC/PCS's host_interfaces/mac_capabilities) what should be advertised and what shouldn't.
| |