Re: [PATCH RFT net-next 02/10] net: stmmac: Add support for Allwinner A523 GMAC200

From: Chen-Yu Tsai
Date: Tue Jul 01 2025 - 22:10:13 EST

On Wed, Jul 2, 2025 at 10:00 AM Yanteng Si <si.yanteng@xxxxxxxxx> wrote:
>
> 在 7/2/25 12:57 AM, Chen-Yu Tsai 写道:
> > From: Chen-Yu Tsai <wens@xxxxxxxx>
> >
> > The Allwinner A523 SoC family has a second Ethernet controller, called
> > the GMAC200 in the BSP and T527 datasheet, and referred to as GMAC1 for
> > numbering. This controller, according to BSP sources, is fully
> > compatible with a slightly newer version of the Synopsys DWMAC core.
> > The glue layer around the controller is the same as found around older
> > DWMAC cores on Allwinner SoCs. The only slight difference is that since
> > this is the second controller on the SoC, the register for the clock
> > delay controls is at a different offset. Last, the integration includes
> > a dedicated clock gate for the memory bus and the whole thing is put in
> > a separately controllable power domain.
> >
> > Add a new driver for this hardware supporting the integration layer.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@xxxxxxxx>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 ++
> > drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
> > .../ethernet/stmicro/stmmac/dwmac-sun55i.c | 161 ++++++++++++++++++
> > 3 files changed, 174 insertions(+)
> > create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 67fa879b1e52..38ce9a0cfb5b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -263,6 +263,18 @@ config DWMAC_SUN8I
> > stmmac device driver. This driver is used for H3/A83T/A64
> > EMAC ethernet controller.
> >
> > +config DWMAC_SUN55I
> > + tristate "Allwinner sun55i GMAC200 support"
> > + default ARCH_SUNXI
> > + depends on OF && (ARCH_SUNXI || COMPILE_TEST)
> > + select MDIO_BUS_MUX
> > + help
> > + Support for Allwinner A523/T527 GMAC200 ethernet controllers.
> > +
> > + This selects Allwinner SoC glue layer support for the
> > + stmmac device driver. This driver is used for A523/T527
> > + GMAC200 ethernet controller.
> > +
> > config DWMAC_THEAD
> > tristate "T-HEAD dwmac support"
> > depends on OF && (ARCH_THEAD || COMPILE_TEST)
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > index b591d93f8503..51e068e26ce4 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > @@ -31,6 +31,7 @@ obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o
> > obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o
> > obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
> > obj-$(CONFIG_DWMAC_SUN8I) += dwmac-sun8i.o
> > +obj-$(CONFIG_DWMAC_SUN55I) += dwmac-sun55i.o
> > obj-$(CONFIG_DWMAC_THEAD) += dwmac-thead.o
> > obj-$(CONFIG_DWMAC_DWC_QOS_ETH) += dwmac-dwc-qos-eth.o
> > obj-$(CONFIG_DWMAC_INTEL_PLAT) += dwmac-intel-plat.o
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> > new file mode 100644
> > index 000000000000..7fadb90e3098
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * dwmac-sun55i.c - Allwinner sun55i GMAC200 specific glue layer
> > + *
> > + * Copyright (C) 2025 Chen-Yu Tsai <wens@xxxxxxxx>
> > + *
> > + * syscon parts taken from dwmac-sun8i.c, which is
> > + *
> > + * Copyright (C) 2017 Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/bitfield.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/regmap.h>
> > +#include <linux/regulator/consumer.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) */
> insert a blankline.

OK.

> > +/* Generic system control EMAC_CLK bits */
> > +#define SYSCON_ETXDC_MASK GENMASK(12, 10)
> > +#define SYSCON_ERXDC_MASK GENMASK(9, 5)
> ditto.

OK.

> > +/* EMAC PHY Interface Type */
> > +#define SYSCON_EPIT BIT(2) /* 1: RGMII, 0: MII */
> > +#define SYSCON_ETCS_MASK GENMASK(1, 0)
> > +#define SYSCON_ETCS_MII 0x0
> > +#define SYSCON_ETCS_EXT_GMII 0x1
> > +#define SYSCON_ETCS_INT_GMII 0x2
> > +
> > +#define MASK_TO_VAL(mask) ((mask) >> (__builtin_ffsll(mask) - 1))
> > +
> > +static int sun55i_gmac200_set_syscon(struct device *dev,
> > + struct plat_stmmacenet_data *plat)
> > +{
> > + struct device_node *node = dev->of_node;
> > + struct regmap *regmap;
> > + u32 val, reg = 0;
> > +
> > + regmap = syscon_regmap_lookup_by_phandle(node, "syscon");
> > + if (IS_ERR(regmap))
> > + return dev_err_probe(dev, PTR_ERR(regmap), "Unable to map syscon\n");
> > +
> -----------
> > + if (!of_property_read_u32(node, "allwinner,tx-delay-ps", &val)) {
> > + if (val % 100) {
> > + dev_err(dev, "tx-delay must be a multiple of 100\n");
> > + return -EINVAL;
> > + }
> > + val /= 100;
> > + dev_dbg(dev, "set tx-delay to %x\n", val);
> > + if (val > MASK_TO_VAL(SYSCON_ETXDC_MASK))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid TX clock delay: %d\n",
> > + val);
> > +
> > + reg |= FIELD_PREP(SYSCON_ETXDC_MASK, val);
> > + }
> > +
> > + if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
> > + if (val % 100) {
> > + dev_err(dev, "rx-delay must be a multiple of 100\n");
> > + return -EINVAL;
> > + }
> > + val /= 100;
> > + dev_dbg(dev, "set rx-delay to %x\n", val);
> > + if (val > MASK_TO_VAL(SYSCON_ERXDC_MASK))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid RX clock delay: %d\n",
> > + val);
> > +
> > + reg |= FIELD_PREP(SYSCON_ERXDC_MASK, val);
> > + }
> ------------
> These two parts of the code are highly similar.
> Can you construct a separate function?

As in, have a function that sets up either TX or RX delay based on
a parameter? That also means constructing the property name on the
fly or using ternary ops. And chopping up the log messages.

I don't think this makes it easier to read. And chopping up the log
message makes it harder to grep.

> > +
> > + switch (plat->mac_interface) {
>
> > + case PHY_INTERFACE_MODE_MII:
> > + /* default */
> > + break;
> This line of comment seems a bit abrupt here.

Default as in this is the 0 value register default.

OTOH the integration doesn't support MII, so I think I should drop this case.


Thanks
ChenYu