> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
>
> 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.)
These are PHYs connected using an XFI interface that indeed can operate at lower
speeds and are capable of rate adaptation using pause frames.
Also, I've used PHY_INTERFACE_MODE_10GKR since a dedicated XFI mode is not
available.
>
> > > + 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.
Agreed. We can just remove that.
>
> 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.
>
> --
