Re: [PATCH RFC/RFT 2/2] net: stmmac: Add support for Allwinner A733 GMAC210

[Jerome Brunet <jbrunet@baylibre.com>]

On sam. 01 août 2026 at 16:30, sashiko-bot@kernel.org wrote:
>
>> +	if (!plat->stmmac_rst || !plat->stmmac_ahb_rst) {
>> +		dev_err(dev, "missing required reset controls\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	/*
>> +	 * The configuration registers are inside the controller
>> +	 * reset domain, so the reset must happen before any write to them
>> +	 * and should not be done again by stmmac or the configuration will
>> +	 * be lost.
>> +	 */
>> +	ret = reset_control_assert(plat->stmmac_rst);
>> +	if (!ret)
>> +		ret = reset_control_deassert(plat->stmmac_rst);
>
> [Severity: High]
> Will this cause a bus hang when writing to the MMIO registers?
>

No it does not. I've checked.
Actually touching the ahb reset here has negative effect. stmmac does
not properly detect the link type then (GMII instead of RGMII)

> The code checks that plat->stmmac_ahb_rst is present earlier in this
> function, but only toggles the main mac reset here. 
>
> When sun55i_gmac200_setup() later calls regmap_write(), accessing an IP
> block over the AHB bus while its AHB interface is still held in reset
> typically causes a synchronous external abort or bus hang.
>
>> +
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "device reset failed\n");
>
> [ ... ]
>
>>  static int sun55i_gmac200_setup(struct platform_device *pdev,
>>  				struct plat_stmmacenet_data *plat,
>>  				const struct sun55i_gmac_data *data)
>>  {
>
> [ ... ]
>
>> -	ret = regmap_write(regmap, SYSCON_REG, reg);
>> +	ret = regmap_write(regmap, data->offset, reg);
>>  	if (ret < 0)
>>  		return dev_err_probe(dev, ret, "Failed to write to syscon\n");
>>  
>> +	plat->flags |= data->flags;
>> +	plat->host_dma_width = 32;
>> +
>>  	return 0;
>>  }
>
> [Severity: High]
> Are these custom hardware settings lost after a system suspend?
>
> The driver configures critical hardware properties like TX/RX delays and
> PHY interface modes in the MAC's internal glue configuration registers
> here, but does not appear to assign a plat->resume callback to the
> plat_stmmacenet_data structure.
>
> If the MAC loses power during suspend, won't these registers revert to
> their default states on resume, leaving the network broken because the
> generic stmmac_resume() has no way to invoke this glue logic?

Assuming there is an issue here, it would be a pre-existing one for this
driver, same as the sun55i-a523.

Let's not get ahead of ourselves. Support for this chip is still in the
very early phase. I don't think we have the means to test suspend
yet.

-- 
Jerome