Re: [net-next,v4,2/3] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing

The second Ethernet controller (eth1) on the Eswin EIC7700 SoC may fail
to sample RX data correctly at Gigabit speed due to EIC7700-specific
receive clock to data skew at the MAC input in the silicon. The existing
internal delay configuration does not provide sufficient adjustment range
to compensate for this condition at 1000Mbps. Update the EIC7700 DWMAC
glue driver to apply EIC7700-specific clock sampling inversion only
during Gigabit operation on MAC instances that require it.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> index bcb8e000e720..b230bc5b902a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c

[ ... ]

> @@ -61,8 +81,26 @@ static int eic7700_clks_config(void *priv, bool enabled)
>  static int eic7700_dwmac_init(struct device *dev, void *priv)
>  {
>  	struct eic7700_qos_priv *dwc = priv;
> +	int ret;
> +
> +	ret = eic7700_clks_config(dwc, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(dwc->eic7700_hsp_regmap,
> +			      dwc->eth_phy_ctrl_offset,
> +			      EIC7700_ETH_TX_CLK_SEL |
> +			      EIC7700_ETH_PHY_INTF_SELI);
> +	if (ret)
> +		return ret;

Does this code leak the enabled clocks? The function calls
eic7700_clks_config(dwc, true) to enable the clocks, then later calls
regmap_set_bits(). If regmap_set_bits() fails, the function returns
without disabling the clocks.

Looking at the caller in stmmac_main.c:stmmac_dvr_probe():

    if (priv->plat->init) {
        ret = priv->plat->init(priv->device, priv->plat->bsp_priv);
        if (ret)
            return ret;
    }

    ret = __stmmac_dvr_probe(priv->device, priv->plat, &res);
    if (ret)
        goto error_exit_init;

The exit handler is only called when __stmmac_dvr_probe() fails, not
when init() fails. This leaves the bulk clocks in the prepared-and-enabled
state on this error path.

> +
> +	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_axi_lp_ctrl_offset,
> +		     EIC7700_ETH_CSYSREQ_VAL);
>
> -	return eic7700_clks_config(dwc, true);
> +	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_txd_offset, 0);
> +	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_rxd_offset, 0);
> +
> +	return 0;
>  }

[ ... ]