| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJhJPsUE0Y8stryP+NcHeaG0LF8aE-Ef8qrg40fPL6rkoJ9izg@mail.gmail.com>
Date: Tue, 22 Aug 2023 20:09:12 +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 Tue, Aug 22, 2023 at 6:53 PM Serge Semin <fancer.lancer@...il.com> wrote:
>
> On Tue, Aug 22, 2023 at 03:58:32PM +0800, Keguang Zhang wrote:
> > 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.
>
> Could you elaborate what does "device/module multiplexing" mean?
>
Pherhaps the device switch is more accurate.
Just means which devices could not work at the time.
> > 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.
>
> You've got only two SoCs, right? LS1C and LS1B. Each of them has
> a specific set of NICs: two loongson,ls1b-gmac on LS1B and
> one loongson,ls1c-mac on LS1C. So LS1B has only two physical addresses
> you can use to distinguish one interface from another and find out
> what flags to toggle in the syscon. LS1C has a single MAC so you won't
> even need any additional info since there is only one flag to
> toggle.
>
Is there any driver that uses the physical-base-address way?
Could you provide me some existing examples?
Thanks!
> Physical address is constant from one LS1C instance to another, and
> similarly from one LS1B instance to another. It's defined by means
> of the macros:
> LS1X_GMAC0_BASE
> LS1X_GMAC1_BASE
> What physical address varying are you talking about? I don't see that
> in the current platform driver.
>
> > It means that the driver has to remember MAC base addresses for different SoCs.
>
> Yes. Just two physical addresses.
>
Two chips for now.
What about the future chips?
> >
> > > 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.
>
> Just two examples with not the best solution implemented.
>
> Anyway it's up to you to decide after all. I don't mind if it will be
> the property-based way. But as for me it looks redundant seeing there
> is base address always available and which you can utilise. Moreover
> if you get to add any new Loongson MAC to the driver you'll need to
> update it anyway at least in a way the syscon flags are toggled. So
> even from that perspective using the physical base address seems as at
> least the equally complex and maintainable solution.
>
> >
> > > >
> > > > > 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().
>
> There are other ways to select syscon_init(). Syscon node compatible
> string is just one of them. You can do that by one of the next way:
> 1. Based on the syscon-property name: "loongson,ls1b-syscon" property
> always points to the LS1B syscon phandle so the
> ls1b_dwmac_syscon_init() function is supposed to be selected;
> "loongson,ls1c-syscon" property always points to the LS1C syscon so
> ls1c_dwmac_syscon_init() is supposed to be utilized.
> 2. Based on the MAC compatible string. loongson,ls1b-gmac can be found
> on the LS1B SoC only which is equipped with the ls1b-syscon system
> controller only so ls1b_dwmac_syscon_init() must be selected. Likewise
> you can determine the init function on the LS1C SoC.
>
OK. I once used the second way for my draft version.
static const struct of_device_id ls1x_dwmac_match[] = {
{ .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon, },
{ .compatible = "loongson,ls1c-mac" .data = &ls1c_dwmac_syscon,
},
{ }
};
Then, I will change back to this implementation in the next version.
> -Serge(y)
>
> >
> > 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
--
Best regards,
Keguang Zhang
Powered by blists - more mailing lists