[RFC/RFT,2/2] net: stmmac: Add support for Allwinner A733 GMAC210
Commit Message
The A733 GMAC210 is a DesignWare MAC 5.20 using an Allwinner glue close to
the A523 GMAC200 already supported by this driver, with the following
differences to handle:
* The glue configuration registers are a dedicated MMIO region of the
controller instead of a syscon register. This new register region
appears to be in the reset domain of stmmac's main reset. This requires
the glue driver to handle the reset rather than letting stmmac deal with
it.
* The TX clock delay value is 5 bits wide, split over 2 register fields,
allowing delays up to 3100ps.
* The DMA channels have their own interrupt lines, so the per-DMA-channel
interrupt mode is enabled, as the vendor SDK does.
* TX LPI clock gating is supported, as the vendor SDK indicates. Follow
the PHY capability with STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP rather than
forcing it on with the deprecated 'snps,en-tx-lpi-clockgating' DT
property.
Unlike the A523, both GMAC instances are supported by the same driver.
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c | 145 ++++++++++++++++++---
1 file changed, 127 insertions(+), 18 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing AHB reset deassertion before MMIO access causes bus hang.
- [High] Missing suspend/resume callback permanently breaks network after sleep.
--
--- Patch [2]: [PATCH RFC/RFT 2/2] net: stmmac: Add support for Allwinner A733 GMAC210 ---
commit 00717a07b743c8a2cc8bc07f479795f3772f17e1
Author: Jerome Brunet <jbrunet@baylibre.com>
net: stmmac: Add support for Allwinner A733 GMAC210
This commit adds support for the Allwinner A733 GMAC210 controller. It
handles a dedicated MMIO region for glue configuration, specific TX clock
delay fields, and per-DMA-channel interrupts.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> index 862df173d963f..3d434a52fb02d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
[ ... ]
> +static struct regmap *sun60i_gmac210_get_regmap(struct platform_device *pdev,
> + struct plat_stmmacenet_data *plat)
> +{
[ ... ]
> + 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?
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?
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.
@@ -10,21 +10,22 @@
*/
#include <linux/bitfield.h>
+#include <linux/bitops.h>
#include <linux/bits.h>
#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
#include <linux/stmmac.h>
#include "stmmac.h"
#include "stmmac_platform.h"
-#define SYSCON_REG 0x34
-
/* RMII specific bits */
#define SYSCON_RMII_EN BIT(13) /* 1: enable RMII (overrides EPIT) */
/* Generic system control EMAC_CLK bits */
@@ -37,17 +38,99 @@
#define SYSCON_ETCS_EXT_GMII 0x1
#define SYSCON_ETCS_INT_GMII 0x2
-static int sun55i_gmac200_set_syscon(struct device *dev,
- struct plat_stmmacenet_data *plat)
+struct sun55i_gmac_data {
+ struct regmap *(*get_regmap)(struct platform_device *pdev,
+ struct plat_stmmacenet_data *plat);
+ unsigned int flags;
+ u32 etxdc_ext_mask;
+ u32 offset;
+};
+
+static struct regmap *sun55i_gmac200_get_regmap(struct platform_device *pdev,
+ struct plat_stmmacenet_data *plat)
+{
+ struct regmap *map =
+ syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "syscon");
+
+ if (IS_ERR(map))
+ dev_err_probe(&pdev->dev, PTR_ERR(map), "Unable to map syscon\n");
+
+ return map;
+}
+
+static const struct regmap_config sun60i_a733_regmap_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static void sun60i_gmac210_reset_assert(void *data)
{
- struct device_node *node = dev->of_node;
+ struct reset_control *rst = data;
+
+ reset_control_assert(rst);
+}
+
+static struct regmap *sun60i_gmac210_get_regmap(struct platform_device *pdev,
+ struct plat_stmmacenet_data *plat)
+{
+ struct device *dev = &pdev->dev;
+ void __iomem *base;
+ int ret;
+
+ base = devm_platform_ioremap_resource(pdev, 1);
+ if (IS_ERR(base)) {
+ dev_err_probe(dev, PTR_ERR(base), "unable to get glue memory region\n");
+ return ERR_CAST(base);
+ }
+
+ 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);
+
+ if (ret) {
+ dev_err_probe(dev, ret, "device reset failed\n");
+ return ERR_PTR(ret);
+ }
+
+ ret = devm_add_action_or_reset(dev, sun60i_gmac210_reset_assert,
+ plat->stmmac_rst);
+ if (ret)
+ return ERR_PTR(ret);
+
+ plat->stmmac_rst = NULL;
+
+ return devm_regmap_init_mmio(&pdev->dev, base, &sun60i_a733_regmap_cfg);
+}
+
+static int sun55i_gmac200_setup(struct platform_device *pdev,
+ struct plat_stmmacenet_data *plat,
+ const struct sun55i_gmac_data *data)
+{
+ unsigned int lo_bits = hweight32(SYSCON_ETXDC_MASK);
+ struct device_node *node = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
struct regmap *regmap;
u32 val, reg = 0;
+ u32 max_delay;
int ret;
- regmap = syscon_regmap_lookup_by_phandle(node, "syscon");
+ max_delay = (1U << (lo_bits + hweight32(data->etxdc_ext_mask))) - 1;
+
+ regmap = data->get_regmap(pdev, plat);
if (IS_ERR(regmap))
- return dev_err_probe(dev, PTR_ERR(regmap), "Unable to map syscon\n");
+ return PTR_ERR(regmap);
if (!of_property_read_u32(node, "tx-internal-delay-ps", &val)) {
if (val % 100)
@@ -55,12 +138,15 @@ static int sun55i_gmac200_set_syscon(struct device *dev,
"tx-delay must be a multiple of 100ps\n");
val /= 100;
dev_dbg(dev, "set tx-delay to %x\n", val);
- if (!FIELD_FIT(SYSCON_ETXDC_MASK, val))
+ if (val > max_delay)
return dev_err_probe(dev, -EINVAL,
- "TX clock delay exceeds maximum (%u00ps > %lu00ps)\n",
- val, FIELD_MAX(SYSCON_ETXDC_MASK));
+ "TX clock delay exceeds maximum (%u00ps > %u00ps)\n",
+ val, max_delay);
- reg |= FIELD_PREP(SYSCON_ETXDC_MASK, val);
+ reg |= field_prep(SYSCON_ETXDC_MASK, val);
+ if (data->etxdc_ext_mask)
+ reg |= field_prep(data->etxdc_ext_mask,
+ val >> lo_bits);
}
if (!of_property_read_u32(node, "rx-internal-delay-ps", &val)) {
@@ -95,21 +181,29 @@ static int sun55i_gmac200_set_syscon(struct device *dev,
phy_modes(plat->phy_interface));
}
- 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;
}
static int sun55i_gmac200_probe(struct platform_device *pdev)
{
struct plat_stmmacenet_data *plat_dat;
+ const struct sun55i_gmac_data *data;
struct stmmac_resources stmmac_res;
struct device *dev = &pdev->dev;
struct clk *clk;
int ret;
+ data = device_get_match_data(dev);
+ if (!data)
+ return -EINVAL;
+
ret = stmmac_get_platform_resources(pdev, &stmmac_res);
if (ret)
return ret;
@@ -118,11 +212,7 @@ static int sun55i_gmac200_probe(struct platform_device *pdev)
if (IS_ERR(plat_dat))
return PTR_ERR(plat_dat);
- /* BSP disables it */
- plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE;
- plat_dat->host_dma_width = 32;
-
- ret = sun55i_gmac200_set_syscon(dev, plat_dat);
+ ret = sun55i_gmac200_setup(pdev, plat_dat, data);
if (ret)
return ret;
@@ -138,8 +228,27 @@ static int sun55i_gmac200_probe(struct platform_device *pdev)
return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
}
+static const struct sun55i_gmac_data sun55i_a523_gmac200_data = {
+ .get_regmap = sun55i_gmac200_get_regmap,
+ .flags = STMMAC_FLAG_SPH_DISABLE,
+ .offset = 0x34,
+ .etxdc_ext_mask = 0,
+};
+
+static const struct sun55i_gmac_data sun60i_a733_gmac210_data = {
+ .get_regmap = sun60i_gmac210_get_regmap,
+ .flags = (STMMAC_FLAG_SPH_DISABLE |
+ STMMAC_FLAG_MULTI_MSI_EN |
+ STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP),
+ .offset = 0x0,
+ .etxdc_ext_mask = GENMASK(17, 16),
+};
+
static const struct of_device_id sun55i_gmac200_match[] = {
- { .compatible = "allwinner,sun55i-a523-gmac200" },
+ { .compatible = "allwinner,sun55i-a523-gmac200",
+ .data = &sun55i_a523_gmac200_data },
+ { .compatible = "allwinner,sun60i-a733-gmac210",
+ .data = &sun60i_a733_gmac210_data },
{ }
};
MODULE_DEVICE_TABLE(of, sun55i_gmac200_match);