> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
>
> > +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;
> > + 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.
>
> But lets see what Russell says.
We cannot do reconfiguration of the interface mode at runtime.
The SERDES speaks an ethernet/sata/pcie coding that is configurable at reset
time.
>
> > +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;
> > +
> > + dpmac_state->up = !!state->link;
> > + if (dpmac_state->up) {
> > + 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.
>
> As the name says, dpaa2_mac_config is about the MAC.
>
> Andrew
Yes, the dpaa2_mac_config should take care of only the MAC but, in this case,
we cannot convey
piecemeal link state information through the firmware to the Ethernet driver -
it has to come all at once.
--
Ioana
