Re: [PATCH] phy: nxp-c45: add driver for tja1103

[Andrew Lunn]

> +static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
> +	{ "phy_symbol_error_cnt", MDIO_MMD_VEND1, SYMBOL_ERROR_COUNTER, 0, GENMASK(15, 0) },
> +	{ "phy_link_status_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 8, GENMASK(13, 8) },
> +	{ "phy_link_availability_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 0, GENMASK(5, 0) },

netdev tries to keep with the old 80 character limit. Please wrap the
long lines.

> +static void nxp_c45_set_delays(struct phy_device *phydev)
> +{
> +	struct nxp_c45_phy *priv = phydev->priv;
> +	u64 tx_delay = priv->tx_delay;
> +	u64 rx_delay = priv->rx_delay;
> +	u64 degree;
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +		degree = tx_delay / PS_PER_DEGREE;
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
> +			      ID_ENABLE | nxp_c45_get_phase_shift(degree));
> +	}

You are missing an else clause. You need to ensure the delay is 0 if
delays are not required. You have no idea what the bootloader has
done.

> +static int nxp_c45_get_delays(struct phy_device *phydev)
> +{
> +	struct nxp_c45_phy *priv = phydev->priv;
> +	int ret;
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +		ret = device_property_read_u32(&phydev->mdio.dev, "tx-internal-delay-ps",
> +					       &priv->tx_delay);
> +		if (ret) {
> +			phydev_err(phydev, "tx-internal-delay-ps property missing\n");

This is not normally mandatory. Default to 2ns if not specified in DT.

> +static int nxp_c45_set_phy_mode(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
> +	phydev_dbg(phydev, "Clause 45 managed PHY abilities 0x%x\n", ret);
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		if (!(ret & RGMII_ABILITY)) {
> +			phydev_err(phydev, "rgmii mode not supported\n");
> +			return -EINVAL;
> +		}
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		if (!(ret & RGMII_ID_ABILITY)) {
> +			phydev_err(phydev, "rgmii-id, rgmii-txid, rgmii-rxid modes are not supported\n");
> +			return -EINVAL;
> +		}
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> +		ret = nxp_c45_get_delays(phydev);
> +		if (ret)
> +			return ret;
> +
> +		nxp_c45_set_delays(phydev);
> +		break;

Again, for PHY_INTERFACE_MODE_RGMII you need to ensure the hardware is
not inserting a delay.

> +	case PHY_INTERFACE_MODE_SGMII:
> +		if (!(ret & SGMII_ABILITY)) {
> +			phydev_err(phydev, "sgmii mode not supported\n");
> +			return -EINVAL;
> +		}
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_SGMII);
> +		break;

Interested. What gets reported over the inband signalling?

	    Andrew