On Fri, Jun 14, 2019 at 03:42:23AM +0200, Andrew Lunn wrote: > > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) > > +{ > > + switch (eth_if) { > > + case DPMAC_ETH_IF_RGMII: > > + return PHY_INTERFACE_MODE_RGMII; > > So the MAC cannot insert RGMII delays? I didn't see anything in the > PHY object about configuring the delays. Does the PCB need to add > delays via squiggles in the tracks? > > > +static void dpaa2_mac_validate(struct phylink_config *config, > > + unsigned long *supported, > > + struct phylink_link_state *state) > > +{ > > + struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config); > > + struct dpmac_link_state *dpmac_state = &priv->state; > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > + > > + phylink_set(mask, Autoneg); > > + phylink_set_port_modes(mask); > > + > > + switch (state->interface) { > > + case PHY_INTERFACE_MODE_10GKR: > > + phylink_set(mask, 10baseT_Full); > > + phylink_set(mask, 100baseT_Full); > > + phylink_set(mask, 1000baseT_Full); > > + phylink_set(mask, 10000baseT_Full); > > + break;
How does 10GBASE-KR mode support these lesser speeds - 802.3 makes no provision for slower speeds for a 10GBASE-KR link, it is a fixed speed link. I don't see any other possible phy interface mode supported that would allow for the 1G, 100M and 10M speeds (i.o.w. SGMII). If SGMII is not supported, then how do you expect these other speeds to work? Does your PHY do speed conversion - if so, we need to come up with a much better way of handling that (we need phylib to indicate that the PHY is so capable.) > > + case PHY_INTERFACE_MODE_QSGMII: > > + case PHY_INTERFACE_MODE_RGMII: > > + case PHY_INTERFACE_MODE_RGMII_ID: > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > + phylink_set(mask, 10baseT_Full); > > + phylink_set(mask, 100baseT_Full); > > + phylink_set(mask, 1000baseT_Full); > > + break; > > + case PHY_INTERFACE_MODE_USXGMII: > > + phylink_set(mask, 10baseT_Full); > > + phylink_set(mask, 100baseT_Full); > > + phylink_set(mask, 1000baseT_Full); > > + phylink_set(mask, 10000baseT_Full); > > + break; > > + default: > > + goto empty_set; > > + } > > I think this is wrong. This is about validating what the MAC can > do. The state->interface should not matter. The PHY will indicate what > interface mode should be used when auto-neg has completed. The MAC is > then expected to change its interface to fit. The question is whether a PHY/MAC wired up using a particular topology can switch between other interface types. For example, SGMII, 802.3z and 10GBASE-KR all use a single serdes lane which means that as long as both ends are configured for the same protocol, the result should work. As an example, Marvell 88x3310 PHYs switch between these three modes depending on the negotiated speed. So, this is more to do with saying what the MAC can support with a particular wiring topology rather than the strict PHY interface type. Take mvneta: /* Half-duplex at speeds higher than 100Mbit is unsupported */ if (pp->comphy || state->interface != PHY_INTERFACE_MODE_2500BASEX) { phylink_set(mask, 1000baseT_Full); phylink_set(mask, 1000baseX_Full); } if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX) { phylink_set(mask, 2500baseX_Full); } If we have a comphy, we can switch the MAC speed between 1G and 2.5G here, so we allow both 1G and 2.5G to be set in the supported mask. If we do not have a comphy, we are not able to change the MAC speed at runtime, so we are more restrictive with the support mask. > > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int > > mode, > > + const struct phylink_link_state *state) > > +{ > > + struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config); > > + struct dpmac_link_state *dpmac_state = &priv->state; > > + struct device *dev = &priv->mc_dev->dev; > > + int err; > > + > > + if (state->speed == SPEED_UNKNOWN && state->duplex == DUPLEX_UNKNOWN) > > + return; As I've already pointed out, state->speed and state->duplex are only valid for fixed-link and PHY setups. They are not valid for SGMII and 802.3z, which use in-band configuration/negotiation, but then in your validate callback, it seems you don't support these. Since many SFP modules require SGMII and 802.3z, I wonder how this is going to work. > > + > > + dpmac_state->up = !!state->link; > > + if (dpmac_state->up) { No, whether the link is up or down is not a concern for this function, and it's not guaranteed to be valid here. I can see I made a bad choice when designing this interface - it was simpler to have just one structure for reading the link state from the MAC and setting the configuration, because the two were very similar. I can see I should've made them separate and specific to each call (which would necessitate additional code, but for the sake of enforcing correct programming interface usage, it would've been the right thing.) > > + dpmac_state->rate = state->speed; > > + > > + if (!state->duplex) > > + dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX; > > + else > > + dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX; > > + > > + if (state->an_enabled) > > + dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG; > > + else > > + dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG; > > As Russell pointed out, this auto-neg is only valid in a limited > context. The MAC generally does not perform auto-neg. The MAC is only > involved in auto-neg when inband signalling is used between the MAC > and PHY in 802.3z. or SGMII. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up