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

[Paolo Abeni]

On 6/10/26 6:26 PM, Simon Horman wrote:
> 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?

I think this one is worth addressing, as it's reported by both sashikos
and could break functionality in hard-to-debug scenarios.

/P