Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Andrew Lunn <andrew@lunn.ch>]

> +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.

> +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