Re: [PATCH net-next v5 3/7] net: ftgmac100: Add AST2700 compatible support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: ftgmac100: missing reset deassertion for AST2700 in ftgmac100_probe()
--

commit 1b009bb5f71bd76cdeadd675e668f4c589bb3d51
Author: Jacky Chou <jacky_chou@aspeedtech.com>

net: ftgmac100: Add AST2700 compatible support

This commit adds match data and the OF compatible entry for the Aspeed
AST2700 FTGMAC100 Ethernet controller.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 6d2fe5c2f3903..67b1fa464a421 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -2017,7 +2018,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  
>  	if (priv->mac_id == FTGMAC100_AST2400 ||
>  	    priv->mac_id == FTGMAC100_AST2500 ||
> -	    priv->mac_id == FTGMAC100_AST2600) {
> +	    priv->mac_id == FTGMAC100_AST2600 ||
> +	    priv->mac_id == FTGMAC100_AST2700) {
>  		priv->rxdes0_edorr_mask = BIT(30);
>  		priv->txdes0_edotr_mask = BIT(30);
>  		priv->is_aspeed = true;

[Severity: High]
Does AST2700 need its reset control deasserted and clocks enabled during
ftgmac100_probe() before accessing registers or bringing up the interface?

On AST2400, AST2500, and AST2600, the MAC clock gates in clk-aspeed.c and
clk-ast2600.c include a coupled reset index, so enabling the clock in
ftgmac100_setup_clk() automatically deasserts the SCU MAC reset.

On AST2700, however, the MAC clock gates in drivers/clk/aspeed/clk-ast2700.c
only control SCU1_CLK_STOP via ast2700_clk_enable() and do not deassert
SCU1_RESET_MAC0..2:

drivers/clk/aspeed/clk-ast2700.c:
	GATE_CLK(SCU1_CLK_GATE_MAC0CLK, CLK_GATE_ASPEED, "mac0clk-gate", -1,
		 SCU1_CLK_STOP, 8, 0),
	GATE_CLK(SCU1_CLK_GATE_MAC1CLK, CLK_GATE_ASPEED, "mac1clk-gate", -1,
		 SCU1_CLK_STOP, 9, 0),
	GATE_CLK(SCU1_CLK_GATE_MAC2CLK, CLK_GATE_ASPEED, "mac2clk-gate", -1,
		 SCU1_CLK_STOP, 10, 0),

In ftgmac100_probe(), priv->rst is acquired via
devm_reset_control_get_optional_exclusive(), but reset_control_deassert()
is never called during probe:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_probe() {
	...
	/* MAC address from chip or random one */
	err = ftgmac100_initial_mac(priv);
	...
	priv->rst = devm_reset_control_get_optional_exclusive(priv->dev, NULL);
	if (IS_ERR(priv->rst)) {
		err = PTR_ERR(priv->rst);
		goto err;
	}

	if (priv->is_aspeed) {
		err = ftgmac100_setup_clk(priv);
		if (err)
			goto err;
	}
	...
}

Furthermore, ftgmac100_reset_and_config_mac() only asserts and deasserts
priv->rst when phydev->interface is PHY_INTERFACE_MODE_RMII:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_reset_and_config_mac() {
	...
	if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII) {
		int err;

		err = reset_control_assert(priv->rst);
		...
		err = reset_control_deassert(priv->rst);
		...
	}
	...
}

If the bootloader leaves the MAC in reset, or when operating in RGMII mode,
won't ftgmac100_initial_mac() read FTGMAC100_OFFSET_MAC_MADR and
FTGMAC100_OFFSET_MAC_LADR while the clock is still gated and reset is
asserted, and won't ftgmac100_open() -> ftgmac100_reset_and_config_mac() ->
ftgmac100_reset_mac() fail with -EIO because priv->rst was never deasserted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-support_ast2700-v5-0-44057f4dde75@aspeedtech.com?part=3