Message-ID: <CAJhJPsU_WiUAuxsrnWi+mFiwjo0Xf3N1Wwv5f2JFwTKY8KwT5g@mail.gmail.com>
Date:   Tue, 22 Aug 2023 15:58:32 +0800
From:   Keguang Zhang <keguang.zhang@...il.com>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Serge Semin <Sergey.Semin@...kalelectronics.ru>
Subject: Re: [PATCH 4/5] net: stmmac: Add glue layer for Loongson-1 SoC

On Mon, Aug 21, 2023 at 10:16 PM Serge Semin <fancer.lancer@...il.com> wrote:
>
> On Mon, Aug 21, 2023 at 09:24:17PM +0800, Keguang Zhang wrote:
> > On Sat, Aug 19, 2023 at 12:19 AM Serge Semin <fancer.lancer@...il.com> wrote:
> > >
> > > On Fri, Aug 18, 2023 at 08:37:27PM +0800, Keguang Zhang wrote:
> > > > On Wed, Aug 16, 2023 at 9:30 PM Serge Semin <fancer.lancer@...il.com> wrote:
> > > > >
> > > > > On Sat, Aug 12, 2023 at 11:11:34PM +0800, Keguang Zhang wrote:
> > > > > > This glue driver is created based on the arch-code
> > > > > > implemented earlier with the platform-specific settings.
> > > > > >
> > > > > > Use syscon for SYSCON register access.
> > > > > >
> > > > > > Partialy based on the previous work by Serge Semin.
> > > > > >
> > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@...il.com>
> > > > > > ---
> > > > > >  drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
> > > > > >  drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
> > > > > >  .../ethernet/stmicro/stmmac/dwmac-loongson1.c | 257 ++++++++++++++++++
> > > > > >  3 files changed, 269 insertions(+)
> > > > > >  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > > > > > index 06c6871f8788..a2b9e289aa36 100644
> > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > > > > > @@ -239,6 +239,17 @@ config DWMAC_INTEL_PLAT
> > > > > >         the stmmac device driver. This driver is used for the Intel Keem Bay
> > > > > >         SoC.
> > > > > >
> > > > > > +config DWMAC_LOONGSON1
> > > > > > +     tristate "Loongson1 GMAC support"
> > > > > > +     default MACH_LOONGSON32
> > > > > > +     depends on OF && (MACH_LOONGSON32 || COMPILE_TEST)
> > > > > > +     help
> > > > > > +       Support for ethernet controller on Loongson1 SoC.
> > > > > > +
> > > > > > +       This selects Loongson1 SoC glue layer support for the stmmac
> > > > > > +       device driver. This driver is used for Loongson1-based boards
> > > > > > +       like Loongson LS1B/LS1C.
> > > > > > +
> > > > > >  config DWMAC_TEGRA
> > > > > >       tristate "NVIDIA Tegra MGBE support"
> > > > > >       depends on ARCH_TEGRA || COMPILE_TEST
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > > > > > index 5b57aee19267..80e598bd4255 100644
> > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > > > > > @@ -29,6 +29,7 @@ obj-$(CONFIG_DWMAC_SUNXI)   += dwmac-sunxi.o
> > > > > >  obj-$(CONFIG_DWMAC_SUN8I)    += dwmac-sun8i.o
> > > > > >  obj-$(CONFIG_DWMAC_DWC_QOS_ETH)      += dwmac-dwc-qos-eth.o
> > > > > >  obj-$(CONFIG_DWMAC_INTEL_PLAT)       += dwmac-intel-plat.o
> > > > > > +obj-$(CONFIG_DWMAC_LOONGSON1)        += dwmac-loongson1.o
> > > > > >  obj-$(CONFIG_DWMAC_GENERIC)  += dwmac-generic.o
> > > > > >  obj-$(CONFIG_DWMAC_IMX8)     += dwmac-imx.o
> > > > > >  obj-$(CONFIG_DWMAC_TEGRA)    += dwmac-tegra.o
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..368d6cd2cb78
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> > > > > > @@ -0,0 +1,257 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/*
> > > > > > + * Loongson-1 DWMAC glue layer
> > > > > > + *
> > > > > > + * Copyright (C) 2011-2023 Keguang Zhang <keguang.zhang@...il.com>
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/mfd/syscon.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/phy.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/regmap.h>
> > > > > > +
> > > > > > +#include "stmmac.h"
> > > > > > +#include "stmmac_platform.h"
> > > > > > +
> > > > > > +/* Loongson-1 SYSCON Registers */
> > > > > > +#define LS1X_SYSCON0         (0x0)
> > > > > > +#define LS1X_SYSCON1         (0x4)
> > > > > > +
> > > > > > +struct ls1x_dwmac_syscon {
> > > > > > +     const struct reg_field *reg_fields;
> > > > > > +     unsigned int nr_reg_fields;
> > > > > > +     int (*syscon_init)(struct plat_stmmacenet_data *plat);
> > > > > > +};
> > > > > > +
> > > > > > +struct ls1x_dwmac {
> > > > > > +     struct device *dev;
> > > > > > +     struct plat_stmmacenet_data *plat_dat;
> > > > > > +     const struct ls1x_dwmac_syscon *syscon;
> > > > > > +     struct regmap *regmap;
> > > > > > +     struct regmap_field *regmap_fields[];
> > > > > > +};
> > > > > > +
> > > > > > +enum ls1b_dwmac_syscon_regfield {
> > > > > > +     GMAC1_USE_UART1,
> > > > > > +     GMAC1_USE_UART0,
> > > > > > +     GMAC1_SHUT,
> > > > > > +     GMAC0_SHUT,
> > > > > > +     GMAC1_USE_TXCLK,
> > > > > > +     GMAC0_USE_TXCLK,
> > > > > > +     GMAC1_USE_PWM23,
> > > > > > +     GMAC0_USE_PWM01,
> > > > > > +};
> > > > > > +
> > > > > > +enum ls1c_dwmac_syscon_regfield {
> > > > > > +     GMAC_SHUT,
> > > > > > +     PHY_INTF_SELI,
> > > > > > +};
> > > > > > +
> > > > > > +const struct reg_field ls1b_dwmac_syscon_regfields[] = {
> > > > > > +     [GMAC1_USE_UART1]       = REG_FIELD(LS1X_SYSCON0, 4, 4),
> > > > > > +     [GMAC1_USE_UART0]       = REG_FIELD(LS1X_SYSCON0, 3, 3),
> > > > > > +     [GMAC1_SHUT]            = REG_FIELD(LS1X_SYSCON1, 13, 13),
> > > > > > +     [GMAC0_SHUT]            = REG_FIELD(LS1X_SYSCON1, 12, 12),
> > > > > > +     [GMAC1_USE_TXCLK]       = REG_FIELD(LS1X_SYSCON1, 3, 3),
> > > > > > +     [GMAC0_USE_TXCLK]       = REG_FIELD(LS1X_SYSCON1, 2, 2),
> > > > > > +     [GMAC1_USE_PWM23]       = REG_FIELD(LS1X_SYSCON1, 1, 1),
> > > > > > +     [GMAC0_USE_PWM01]       = REG_FIELD(LS1X_SYSCON1, 0, 0)
> > > > > > +};
> > > > > > +
> > > > > > +const struct reg_field ls1c_dwmac_syscon_regfields[] = {
> > > > > > +     [GMAC_SHUT]             = REG_FIELD(LS1X_SYSCON0, 6, 6),
> > > > > > +     [PHY_INTF_SELI]         = REG_FIELD(LS1X_SYSCON1, 28, 30)
> > > > > > +};
> > > > >
> > > > > Emm, using regmap fields looks so over-complicated in this case seeing
> > > > > you only need to set/clear several bits in the syscon. What about
> > > > > defining macros with the particular flag as it's already done in the
> > > > > "asm/mach-loongson32/regs-mux.h" file and using regmap_update_bits()?
> > > > >
> > >
> > > > To use regmap_update_bits(), I have to store and pass reg_offset and
> > > > mask, which is similar to the definition of regmap fields.
> > >
> > > Em, not really. And what offset are you talking about? Anyway you
> > > don't need one. Moreover you'll be able to reduce the number of IOs:
> > >
> > > +#define GMAC1_USE_UART1                 BIT(4)
> > > +#define GMAC1_USE_UART0                 BIT(3)
> > > ...
> > > +#define GMAC1_SHUT                      BIT(13)
> > > ...
> > > +#define GMAC1_USE_TXCLK                 BIT(3)
> > > +#define GMAC1_USE_PWM23                 BIT(1)
> > >
> > > +static int ls1b_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> > > +{
> > > +       struct ls1x_dwmac *dwmac = plat->bsp_priv;
> > > +       struct regmap *syscon = dwmac->regmap;
> > > +
> > > +       if (plat->bus_id) {
> > > +               regmap_update_bits(syscon, LS1X_SYSCON0,
> > > +                                  GMAC1_USE_UART1 | GMAC1_USE_UART0,
> > > +                                  GMAC1_USE_UART1 | GMAC1_USE_UART0);
> > > +
> > > +               switch (plat->phy_interface) {
> > > +               case PHY_INTERFACE_MODE_RGMII:
> > > +                       regmap_update_bits(syscon, LS1X_SYSCON1,
> > > +                                          GMAC1_USE_TXCLK | GMAC1_USE_TXCLK, 0);
> > > +                       break;
> > > +               case PHY_INTERFACE_MODE_MII:
> > > +                       regmap_update_bits(syscon, LS1X_SYSCON1,
> > > +                                          GMAC1_USE_TXCLK | GMAC1_USE_TXCLK
> > > +                                          GMAC1_USE_TXCLK | GMAC1_USE_TXCLK);
> > > +                       break;
> > > +               default:
> > > +                       dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> > > +                               plat->phy_interface);
> > > +                       return -EOPNOTSUPP;
> > > +               }
> > > +
> > > +               regmap_field_write(syscon, LS1X_SYSCON1, GMAC1_SHUT, 0);
> > > +       } //...
> > > +
> > > +       return 0;
> > > +}
> > >
> > > This doesn't look in anyway less readable then your implementation
> > > but in fact simpler.
> > >
> > > > In addition, the regmap fields are very clear and leave the trouble to
> > > > the internal implementation.
> > >
> > > In this case it brings much more troubles and no clarity. You need to create
> > > an additional mainly redundant abstraction, waste memory for it,
> > > define additional const arrays. Using it won't improve the code
> > > readability seeing you need to set/clear a few flags only. So all of
> > > the troubles for nothing. See the code above. It's simple and clear.
> > > Just several regmap_update_bits()..
> > >
> > OK. I will use regmap instead of regmap fields.
> >
> > > BTW why have you chosen to define syscon instead of creating a pinctrl
> > > driver? What if Loongson1 is embedded into a platform with, for
> > > instance, UART0 and UART1 utilized instead of the GMAC1?
> > >
>
> > As you can see, the two registers contains miscellaneous settings.
> > Besides ‘USE’ bits, there are ‘RESET‘ bits, 'EN' bits, 'SHUT' bits, ...
> > So they are not pinctrl registers.
>
> You could have defined a device node which would export "reset",
> "power" and "pinctrl" functionality, like it's normally done for the
> RCU devices (Reset and Clock unit devices).
>
> > Actually, there is a dedicated pin controller which controls the
> > multiplexing of pads.
>
> Do you mean that there is another controller in the Loongson1 SoC
> which controls the pads multiplexing?
>
Yes. There is another contoller that really controls the pads multiplexing.

> If so what is the purpose of the GMAC1_USE_UART1, GMAC1_USE_UART0 and
> GMAC1_USE_TXCLK, GMAC1_USE_PWM23 flags in MUX controller then? Is it
> just another pinctl space with additional reset/power controls or what?
>
>From my perspective, these ‘USE’ bits should be regarded as
device/module multiplexing rather than pads multiplexing.
Although the two registers were called MUX_CTRL in LS1B datasheet,
they had beed renamed to MISC_CTRL in LS1C datasheet.
So it is supposed to be considered syscon.
> >
> > > >
> > > > > > +
> > > > >
> > > > > > +static int ls1b_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> > > > > > +{
> > > > >
> > > > > As I already told you this part is better to be called from the
> > > > > plat_stmmacenet_data.fix_mac_speed() because PHY interface mode can
> > > > > differ from one interface open cycle to another as per the phylink
> > > > > design.
> > > > >
> > > > I have considered .fix_mac_speed(), which will be called every time
> > > > the link is up, and passes the current speed.
> > > > However, the PHY interface mode is determined by the hardware design -
> > > > the schematic.
> > > > In other words, once the schematic is done, the PHY interface mode is fixed.
> > > > Therefore, PHY interface mode should be configured one time at the
> > > > initialization.
> > > > And the plat_stmmacenet_data.init() is the proper place to do this.
> > >
> > > Ok. If no actual clock change is needed then indeed init() will be the
> > > proper place.
> > >
> > > >
> > > > > > +     struct ls1x_dwmac *dwmac = plat->bsp_priv;
> > > > > > +     struct regmap_field **regmap_fields = dwmac->regmap_fields;
> > > > > > +
> > > > >
> > > > > > +     if (plat->bus_id) {
> > > > >
> > > > > Using bus_id doesn't look correct to determine the CSRs responsible
> > > > > for the interface mode selection because it's calculated based on the
> > > > > DT ethernet-alias which doesn't guarantee to have a particular device
> > > > > assigned with the alias. Alias node can be absent after all. What
> > > > > could be better in this case is for instance to use the regs physical
> > > > > address. Any better idea?
> > > > >
> > >
> > > > The purpose of alias is to bind the a particular device with a
> > > > particular alias even some aliases are absent.
> > > > Because of_alias_get_id() gets the alias id.
> > > > For example, LS1B has two GMAC controllers, gmac0 and gmac1.
> > > > I have tried the Ethernet with only one alias as follows.
> > > >        aliases {
> > > >                ethernet1 = &gmac1;
> > > >        };
> > > > In this case, plat->bus_id is still 1.
> > > > And both gmac0 and gmac1 work.
> > >
> > > If no alias specified? If both aliases a non zero? If the IDs are
> > > confused? If any of these is true you are in trouble. Your code
> > > shouldn't rely on the aliases in this case. You need to come up with a
> > > way to certainly distinguish one MAC from another. A physical base
> > > address is one possible option.
> > >
>
> > I see.
> > But It seems unusual to determine device IDs by physical base address.
> > What about adding a new property? such as loongson,dwmac-id
>
> IMO It's better to have a DT-property-independent way for it. If I
> were you I would have utilized the reg-space
> physical-base-address-based approach since you need to have it
> specified anyway and it determines the particular MAC for sure.
> Thus your driver won't be dependent in the device tree in that aspect.
>
The physical base address may vary from SoC to SoC.
It means that the driver has to remember MAC base addresses for different SoCs.

> Some DW *MAC drivers pass additional number to the syscon phandle node
> in the syscon-property to identify the MAC on the board, like
> dwmac-socfpga.c or dwmac-sti.c. If you get to implement a similar way
> then you'll need to have the DT-bindings properly describing that. But
> IMO my approach seems less clumsy and simpler.
>
I prefer the DT-bindings property way. At least many off-the-shelf
examples can be found.

> >
> > > Note the /alias node is an informational node. It doesn't describe
> > > devices. Just recent Krzysztof comment in a similar situation:
> > > https://lore.kernel.org/netdev/20230814112539.70453-1-sriranjani.p@samsung.com/T/#m3972e40bd2fa323a3bdb2fbf07bde47ba6752439
> > >
> > > Aliases are normally used by OS to for instance fix the device
> > > enumeration (see SPI, I2C, I3C, MTD, MMC, RTC, TTY/Serial, Watchdog,
> > > MDIO-GPIO, etc) - pre-define the device ID from the kernel or OS point
> > > of view. In your case the IDs can't be changed. GMAC0 must be assigned
> > > with ID0 and GMAC1 must be assigned with non-zero. Doing otherwise
> > > will be break the interfaces functionality which isn't acceptable.
> > >
> > > >
> > > > > > +             regmap_field_write(regmap_fields[GMAC1_USE_UART1], 1);
> > > > > > +             regmap_field_write(regmap_fields[GMAC1_USE_UART0], 1);
> > > > > > +
> > > > > > +             switch (plat->phy_interface) {
> > > > > > +             case PHY_INTERFACE_MODE_RGMII:
> > > > > > +                     regmap_field_write(regmap_fields[GMAC1_USE_TXCLK], 0);
> > > > > > +                     regmap_field_write(regmap_fields[GMAC1_USE_PWM23], 0);
> > > > > > +                     break;
> > > > > > +             case PHY_INTERFACE_MODE_MII:
> > > > > > +                     regmap_field_write(regmap_fields[GMAC1_USE_TXCLK], 1);
> > > > > > +                     regmap_field_write(regmap_fields[GMAC1_USE_PWM23], 1);
> > > > > > +                     break;
> > > > > > +             default:
> > > > > > +                     dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> > > > > > +                             plat->phy_interface);
> > > > > > +                     return -EOPNOTSUPP;
> > > > > > +             }
> > > > > > +
> > > > > > +             regmap_field_write(regmap_fields[GMAC1_SHUT], 0);
> > > > > > +     } else {
> > > > > > +             switch (plat->phy_interface) {
> > > > > > +             case PHY_INTERFACE_MODE_RGMII:
> > > > > > +                     regmap_field_write(regmap_fields[GMAC0_USE_TXCLK], 0);
> > > > > > +                     regmap_field_write(regmap_fields[GMAC0_USE_PWM01], 0);
> > > > > > +                     break;
> > > > > > +             case PHY_INTERFACE_MODE_MII:
> > > > > > +                     regmap_field_write(regmap_fields[GMAC0_USE_TXCLK], 1);
> > > > > > +                     regmap_field_write(regmap_fields[GMAC0_USE_PWM01], 1);
> > > > > > +                     break;
> > > > > > +             default:
> > > > > > +                     dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> > > > > > +                             plat->phy_interface);
> > > > > > +                     return -EOPNOTSUPP;
> > > > > > +             }
> > > > > > +
> > > > > > +             regmap_field_write(regmap_fields[GMAC0_SHUT], 0);
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int ls1c_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> > > > > > +{
> > > > > > +     struct ls1x_dwmac *dwmac = plat->bsp_priv;
> > > > > > +     struct regmap_field **regmap_fields = dwmac->regmap_fields;
> > > > > > +
> > > > > > +     if (plat->phy_interface == PHY_INTERFACE_MODE_RMII) {
> > > > > > +             regmap_field_write(regmap_fields[PHY_INTF_SELI], 0x4);
> > > > > > +     } else {
> > > > > > +             dev_err(dwmac->dev, "Unsupported PHY-mode %u\n",
> > > > > > +                     plat->phy_interface);
> > > > > > +             return -EOPNOTSUPP;
> > > > > > +     }
> > > > > > +
> > > > > > +     regmap_field_write(regmap_fields[GMAC_SHUT], 0);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct ls1x_dwmac_syscon ls1b_dwmac_syscon = {
> > > > > > +     .reg_fields = ls1b_dwmac_syscon_regfields,
> > > > > > +     .nr_reg_fields = ARRAY_SIZE(ls1b_dwmac_syscon_regfields),
> > > > > > +     .syscon_init = ls1b_dwmac_syscon_init,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct ls1x_dwmac_syscon ls1c_dwmac_syscon = {
> > > > > > +     .reg_fields = ls1c_dwmac_syscon_regfields,
> > > > > > +     .nr_reg_fields = ARRAY_SIZE(ls1c_dwmac_syscon_regfields),
> > > > > > +     .syscon_init = ls1c_dwmac_syscon_init,
> > > > > > +};
> > > > > > +
> > > > > > +static int ls1x_dwmac_init(struct platform_device *pdev, void *priv)
> > > > > > +{
> > > > > > +     struct ls1x_dwmac *dwmac = priv;
> > > > > > +     int ret;
> > > > > > +
> > > > >
> > > > > > +     ret = devm_regmap_field_bulk_alloc(dwmac->dev, dwmac->regmap,
> > > > > > +                                        dwmac->regmap_fields,
> > > > > > +                                        dwmac->syscon->reg_fields,
> > > > > > +                                        dwmac->syscon->nr_reg_fields);
> > > > >
> > > > > Please see my first comment about this.
> > > > >
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     if (dwmac->syscon->syscon_init) {
> > > > > > +             ret = dwmac->syscon->syscon_init(dwmac->plat_dat);
> > > > > > +             if (ret)
> > > > > > +                     return ret;
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct of_device_id ls1x_dwmac_syscon_match[] = {
> > > > > > +     { .compatible = "loongson,ls1b-syscon", .data = &ls1b_dwmac_syscon },
> > > > > > +     { .compatible = "loongson,ls1c-syscon", .data = &ls1c_dwmac_syscon },
> > > > > > +     { }
> > > > > > +};
> > > > > > +
> > > > > > +static int ls1x_dwmac_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct plat_stmmacenet_data *plat_dat;
> > > > > > +     struct stmmac_resources stmmac_res;
> > > > > > +     struct device_node *syscon_np;
> > > > > > +     const struct of_device_id *match;
> > > > > > +     struct regmap *regmap;
> > > > > > +     struct ls1x_dwmac *dwmac;
> > > > > > +     const struct ls1x_dwmac_syscon *syscon;
> > > > > > +     size_t size;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > >
> > > > > > +     /* Probe syscon */
> > > > > > +     syscon_np = of_parse_phandle(pdev->dev.of_node, "syscon", 0);
> > > > >
> > > > > it's vendor-specific property so it is supposed to have a
> > > > > vendor-specific prefix and possibly ls1-specific name.
> > > > >
> > > > This has been fixed in v2.
> > > > Could you please review v2?
> > > > Thanks!
> > > >
> > > > > > +     if (!syscon_np)
> > > > > > +             return -ENODEV;
> > > > > > +
> > > > > > +     match = of_match_node(ls1x_dwmac_syscon_match, syscon_np);
> > > > > > +     if (!match) {
> > > > > > +             of_node_put(syscon_np);
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > > +     syscon = (const struct ls1x_dwmac_syscon *)match->data;
> >
>
> > Please note that of_match_node() is used for syscon matching.
>
> I noticed it in the first place. Please see my next comment.
>
> >
> > > > > > +
> > > > > > +     regmap = syscon_node_to_regmap(syscon_np);
> > > > > > +     of_node_put(syscon_np);
> > > > > > +     if (IS_ERR(regmap)) {
> > > > > > +             ret = PTR_ERR(regmap);
> > > > > > +             dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
> > > > > > +             return ret;
> > > > > > +     }
> > > > >
> > > > > or you can use syscon_regmap_lookup_by_phandle(). Using
> > > > > of_match_node() doesn't seem necessary since it's unlikely to have
> > > > > moee than one system controller available on the LS1b or LS1c chips.
> > > > >
> > >
> > > > I planned to use syscon_regmap_lookup_by_phandle().
> > > > Thus the compatible
> > > > "loongson,ls1b-dwmac-syscon"/"loongson,ls1c-dwmac-syscon" would become
> > > > useless.
> > > > I'm not sure about this.
> > >
> > > The compatible strings should be left despite of the
> > > syscon_regmap_lookup_by_phandle() usage. But again "dwmac" suffix is
> > > redundant. Based on the CSRs definition in regs-mux.h, selecting
> > > (G)MAC pins mode is only a small part of the Loongson1 SoC system
> > > controllers functionality.
> > > "loongson,ls1b-syscon"/"loongson,ls1c-syscon" looks more appropriate.
> > >
> > That's what I did in PATCH 2/5.
> > I've just explained this to Krzysztof.
> > And will change back to "loongson,ls1b-syscon"/"loongson,ls1c-syscon"
> > in next version.
> >
>
> > In addition, syscon_regmap_lookup_by_phandle() returns regmap pointer directly.
> > Then, there wil be no way to do syscon matching without its device_node.
> > How will I know whether the syscon is loongson,ls1b-syscon or
> > loongson,ls1c-syscon?
>
> Do you have both of these syscons available in the same SoC? I don't
> think so. Thus you don't need such validation since the LS1C SoC dts
> file will have the "loongson,ls1c-syscon" syscon node defined only and
> the LS1B SoC dts file - "loongson,ls1b-syscon" only.
>
I believe you have noticed the difference of syscon_init() between
LS1B and LS1C.
The syscon matching is for finding the right syscon_init().

Thanks for your review!

> -Serge(y)
>
> >
> > Thanks for your review!
> >
> >
> >
> >
> >
> > > -Serge(y)
> > >
> > > >
> > > > > > +
> > > > > > +     size = syscon->nr_reg_fields * sizeof(struct regmap_field *);
> > > > > > +     dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac) + size, GFP_KERNEL);
> > > > > > +     if (!dwmac)
> > > > > > +             return -ENOMEM;
> > > > > > +
> > > > > > +     plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > > > > > +     if (IS_ERR(plat_dat)) {
> > > > > > +             dev_err(&pdev->dev, "dt configuration failed\n");
> > > > > > +             return PTR_ERR(plat_dat);
> > > > > > +     }
> > > > > > +
> > > > > > +     plat_dat->bsp_priv = dwmac;
> > > > > > +     plat_dat->init = ls1x_dwmac_init;
> > > > > > +     dwmac->dev = &pdev->dev;
> > > > > > +     dwmac->plat_dat = plat_dat;
> > > > > > +     dwmac->syscon = syscon;
> > > > > > +     dwmac->regmap = regmap;
> > > > > > +
> > > > > > +     ret = stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
> > > > > > +     if (ret)
> > > > > > +             goto err_remove_config_dt;
> > > > > > +
> > > > > > +     return 0;
> > > > > > +
> > > > > > +err_remove_config_dt:
> > > > >
> > > > > > +     if (pdev->dev.of_node)
> > > > >
> > > > > Is this conditional statement necessary here?
> > > > >
> > > > You're right.
> > > > Will remove this condition in next version.
> > > > Thanks!
> > > >
> > > > > -Serge
> > > > >
> > > > > > +             stmmac_remove_config_dt(pdev, plat_dat);
> > > > > > +
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct of_device_id ls1x_dwmac_match[] = {
> > > > > > +     { .compatible = "loongson,ls1b-dwmac" },
> > > > > > +     { .compatible = "loongson,ls1c-dwmac" },
> > > > > > +     { }
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, ls1x_dwmac_match);
> > > > > > +
> > > > > > +static struct platform_driver ls1x_dwmac_driver = {
> > > > > > +     .probe = ls1x_dwmac_probe,
> > > > > > +     .remove_new = stmmac_pltfr_remove,
> > > > > > +     .driver = {
> > > > > > +             .name = "loongson1-dwmac",
> > > > > > +             .of_match_table = ls1x_dwmac_match,
> > > > > > +     },
> > > > > > +};
> > > > > > +module_platform_driver(ls1x_dwmac_driver);
> > > > > > +
> > > > > > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@...il.com>");
> > > > > > +MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
> > > > > > +MODULE_LICENSE("GPL");
> > > > > > --
> > > > > > 2.39.2
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > >
> > > > Keguang Zhang
> >
> >
> >
> > --
> > Best regards,
> >
> > Keguang Zhang



-- 
Best regards,

Keguang Zhang

Powered by blists - more mailing lists