Re: [PATCH net-next 4/4] net: stmmac: dwmac-sun8i: Add Allwinner H616 EMAC clock index and internal PHY support

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 9/16/26 08:24, Maxime Chevallier wrote:
> Hi,
> 
> On 9/16/26 06:41, Alastair D'Silva wrote:
>> Support EMAC1 and the internal Fast Ethernet PHY (AC300) on Allwinner
>> H616 and H618 SoCs:
>> - Allow reading a register index from the syscon property to offset the
>>    regmap field for EMAC1's dedicated clock control register.
>> - Add emac_variant_h616_internal with support for internal PHY, MII,
>>    RMII, and RGMII modes.
>> - Delay the MAC software reset until sun8i_dwmac_init() when using the
>>    internal PHY, ensuring the PHY has powered on and begun generating
>>    the receive clock before the MAC attempts to reset.
>> - Skip setting H3_EPHY_SELECT for H616 internal EMAC, which does not
>>    use the H3-style EPHY select bit.
>> - Handle -EPROBE_DEFER cleanly during clock and reset control acquisition
>>    in get_ephy_nodes().
>>
>> Assisted-by: LLM
>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>> ---
> 
> Even if this is a duplicate some of the work done by James Hilliard, this
> raises interesting points.

I think the point is that this patch is just wrong, in many parts ;-)
I think much of the code stems from the original Allwinner BSP code, 
which is just, say, misinformed in many respects. The use of compatible 
strings, and shoehorning the PHY clock into the MAC code are just 
examples. More below.

>> Notes:
>>      Tested on Mellow Fly-C5 (Allwinner H618) under Armbian, verifying stable
>>      EMAC1 probe, deferred soft reset with the internal AC300 EPHY, and 100M full
>>      duplex operation under load.
>>
>>   .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 65 ++++++++++++++++---
>>   1 file changed, 55 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> index 48c52eb96233..1728893589a6 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>> @@ -130,6 +130,16 @@ static const struct emac_variant emac_variant_a64 = {
>>   	.tx_delay_max = 7,
>>   };
>>   
>> +static const struct emac_variant emac_variant_h616_internal = {
>> +	.syscon_field = &sun8i_syscon_reg_field,
>> +	.soc_has_internal_phy = true,

For instance this cannot be true. internal_phy refers to the H3 
generation, which works quite differently. We had this issue already 
with the H6, and there is a comment there mentioning this.

And this becomes obvious in other parts of the code, where this patch 
reverts the has_internal_phy tests, because they don't apply here.

>> +	.support_mii = true,
>> +	.support_rmii = true,
>> +	.support_rgmii = true,


>> +	.rx_delay_max = 31,
>> +	.tx_delay_max = 7,
>> +};
> James's series [1] says this SoC only supports RMII (yet includes RGMII timing
> control), and here for seemingly the same device, there's MII and RGMII
> support listed.
> 
> [1] : https://lore.kernel.org/r/20260915-submit-h616-emac1-v1-v1-0-195de0bb1f8a@gmail.com
> 
> Maybe you can coordinate with one another, can we connect that SoC to a non-internal PHY ?
> If so, can we use MII, RMII and RGMII ? or is it just RMII ?

This MAC is *only* connected to the AC200/AC300 PHY, which is a separate 
die, but co-packaged into the SoC. So that's set in stone. And while the 
MAC IP might support more MII modes (because it's maybe the same IP as 
the primary MAC? We don't know), it is irrelevant here, since only RMII 
is usable.

Cheers,
Andre

> 
> Maxime