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

[Chen-Yu Tsai <wens@kernel.org>]

On Wed, Jul 2, 2025 at 10:00 AM Yanteng Si <si.yanteng@linux.dev> wrote:
>
> 在 7/2/25 12:57 AM, Chen-Yu Tsai 写道:
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > 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@csie.org>
> > ---
> >   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@csie.org>
> > + *
> > + * syscon parts taken from dwmac-sun8i.c, which is
> > + *
> > + * Copyright (C) 2017 Corentin Labbe <clabbe.montjoie@gmail.com>
> > + */
> > +
> > +#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