Re: [PATCH net-next v3 2/2] net: lan743x: add support for RMII interface

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: lan743x: add support for RMII interface

This commit adds RMII interface support to the lan743x driver, wiring
up phy_interface selection, phylink supported_interfaces and MAC
capabilities, and enabling the RMII front-end via a new RMII_CTL
register write in lan743x_hardware_init().

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 0798f3f1f435..ad3f833c8d86 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c

[ ... ]

> @@ -3562,6 +3574,11 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
>  			sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
>  		}
>  		lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> +		if (adapter->is_rmii_en) {
> +			rmii_ctl = lan743x_csr_read(adapter, RMII_CTL);
> +			rmii_ctl |= RMII_CTL_RMII_ENABLE_;
> +			lan743x_csr_write(adapter, RMII_CTL, rmii_ctl);
> +		}

[Medium]
Should the RMII_CTL programming mirror the symmetric pattern used for
SGMII_CTL just above it?

The SGMII block sets or clears the enable/power-down bits in both
directions:

	sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
	if (adapter->is_sgmii_en) {
		sgmii_ctl |= SGMII_CTL_SGMII_ENABLE_;
		sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
	} else {
		sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
		sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
	}
	lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);

The new RMII block only sets RMII_CTL_RMII_ENABLE_ when is_rmii_en is
true and never clears it on the other paths.

If RMII_CTL_RMII_ENABLE_ is left set by a previous agent (for example,
a prior driver instance with a different strap, board firmware, or
state preserved across a warm reset where the driver itself sets
HW_CFG_RST_PROTECT_ during PM suspend, and pci11x1x_strap_get_status()
already accounts for cfg_load/HW_CFG_RST_PROTECT_), can the RMII
front-end remain enabled while the driver is configured for RGMII or
SGMII?

Would adding an explicit clear in the !is_rmii_en path, mirroring the
SGMII handling, give the register a known state on every probe?