mbox series

[net-next,00/11] add basic support for i.MX95 NETC

Message ID 20241009095116.147412-1-wei.fang@nxp.com
Headers show
Series add basic support for i.MX95 NETC | expand

Message

Wei Fang Oct. 9, 2024, 9:51 a.m. UTC
This is first time that the NETC IP is applied on i.MX MPU platform.
Its revision has been upgraded to 4.1, which is very different from
the NETC of LS1028A (its revision is 1.0). Therefore, some existing
drivers of NETC devices in the Linux kernel are not compatible with
the current hardware. For example, the fsl-enetc driver is used to
drive the ENETC PF of LS1028A, but for i.MX95 ENETC PF, its registers
and tables configuration are very different from those of LS1028A,
and only the station interface (SI) part remains basically the same.
For the SI part, Vladimir has separated the fsl-enetc-core driver, so
we can reuse this driver on i.MX95. However, for other parts of PF,
the fsl-enetc driver cannot be reused, so the nxp-enetc4 driver is
added to support revision 4.1 and later.

During the development process, we found that the two PF drivers have
some interfaces with basically the same logic, and the only difference
is the hardware configuration. So in order to reuse these interfaces
and reduce code redundancy, we extracted these interfaces and compiled
them into a separate nxp-enetc-pf-common driver for use by the two PF
drivers.

In addition, we have developed the nxp-netc-blk-ctrl driver, which
is used to control three blocks, namely Integrated Endpoint Register
Block (IERB), Privileged Register Block (PRB) and NETCMIX block. The
IERB contains registers that are used for pre-boot initialization,
debug, and non-customer configuration. The PRB controls global reset
and global error handling for NETC. The NETCMIX block is mainly used
to set MII protocol and PCS protocol of the links, it also contains
settings for some other functions.

Clark Wang (1):
  net: enetc: optimize the allocation of tx_bdr

Vladimir Oltean (1):
  net: enetc: only enable ERR050089 workaround on LS1028A

Wei Fang (9):
  dt-bindings: net: add compatible string for i.MX95 EMDIO
  dt-bindings: net: add i.MX95 ENETC support
  dt-bindings: net: add bindings for NETC blocks control
  net: enetc: add initial netc-blk-ctrl driver support
  net: enetc: add enetc-pf-common driver support
  PCI: Add NXP NETC vendor ID and device IDs
  net: enetc: add i.MX95 EMDIO support
  net: enetc: add preliminary support for i.MX95 ENETC PF
  MAINTAINERS: update ENETC driver files and maintainers

 .../bindings/net/fsl,enetc-mdio.yaml          |  15 +-
 .../devicetree/bindings/net/fsl,enetc.yaml    |  23 +-
 .../bindings/net/nxp,netc-blk-ctrl.yaml       | 107 +++
 MAINTAINERS                                   |   9 +
 drivers/net/ethernet/freescale/enetc/Kconfig  |  40 +
 drivers/net/ethernet/freescale/enetc/Makefile |   9 +
 drivers/net/ethernet/freescale/enetc/enetc.c  | 171 ++--
 drivers/net/ethernet/freescale/enetc/enetc.h  |  19 +-
 .../net/ethernet/freescale/enetc/enetc4_hw.h  | 153 ++++
 .../net/ethernet/freescale/enetc/enetc4_pf.c  | 761 ++++++++++++++++++
 .../ethernet/freescale/enetc/enetc_ethtool.c  |  68 +-
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  46 +-
 .../ethernet/freescale/enetc/enetc_pci_mdio.c |  21 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 350 +-------
 .../net/ethernet/freescale/enetc/enetc_pf.h   |  37 +
 .../freescale/enetc/enetc_pf_common.c         | 383 +++++++++
 .../ethernet/freescale/enetc/netc_blk_ctrl.c  | 476 +++++++++++
 include/linux/fsl/netc_global.h               |  39 +
 include/linux/pci_ids.h                       |   7 +
 19 files changed, 2302 insertions(+), 432 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
 create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_hw.h
 create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_pf.c
 create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
 create mode 100644 drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
 create mode 100644 include/linux/fsl/netc_global.h

Comments

Frank Li Oct. 9, 2024, 5:02 p.m. UTC | #1
On Wed, Oct 09, 2024 at 05:51:09PM +0800, Wei Fang wrote:
> The netc-blk-ctrl driver is used to configure Integrated Endpoint
> Register Block (IERB) and Privileged Register Block (PRB) of NETC.
> For i.MX platforms, it is also used to configure the NETCMIX block.
>
> The IERB contains registers that are used for pre-boot initialization,
> debug, and non-customer configuration. The PRB controls global reset
> and global error handling for NETC. The NETCMIX block is mainly used
> to set MII protocol and PCS protocol of the links, it also contains
> settings for some other functions.
>
> Note the IERB configuration registers can only be written after being
> unlocked by PRB, otherwise, all write operations are inhibited. A warm
> reset is performed when the IERB is unlocked, and it results in an FLR
> to all NETC devices. Therefore, all NETC device drivers must be probed
> or initialized after the warm reset is finished.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/Kconfig  |  14 +
>  drivers/net/ethernet/freescale/enetc/Makefile |   3 +
>  .../ethernet/freescale/enetc/netc_blk_ctrl.c  | 476 ++++++++++++++++++
>  include/linux/fsl/netc_global.h               |  39 ++
>  4 files changed, 532 insertions(+)
>  create mode 100644 drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
>  create mode 100644 include/linux/fsl/netc_global.h
>
> diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
> index 4d75e6807e92..51d80ea959d4 100644
> --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> @@ -75,3 +75,17 @@ config FSL_ENETC_QOS
>  	  enable/disable from user space via Qos commands(tc). In the kernel
>  	  side, it can be loaded by Qos driver. Currently, it is only support
>  	  taprio(802.1Qbv) and Credit Based Shaper(802.1Qbu).
> +
> +config NXP_NETC_BLK_CTRL
> +	tristate "NETC blocks control driver"
> +	help
> +	  This driver configures Integrated Endpoint Register Block (IERB) and
> +	  Privileged Register Block (PRB) of NETC. For i.MX platforms, it also
> +	  includes the configuration of NETCMIX block.
> +	  The IERB contains registers that are used for pre-boot initialization,
> +	  debug, and non-customer configuration. The PRB controls global reset
> +	  and global error handling for NETC. The NETCMIX block is mainly used
> +	  to set MII protocol and PCS protocol of the links, it also contains
> +	  settings for some other functions.
> +
> +	  If compiled as module (M), the module name is nxp-netc-blk-ctrl.
> diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
> index b13cbbabb2ea..5c277910d538 100644
> --- a/drivers/net/ethernet/freescale/enetc/Makefile
> +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> @@ -19,3 +19,6 @@ fsl-enetc-mdio-y := enetc_pci_mdio.o enetc_mdio.o
>
>  obj-$(CONFIG_FSL_ENETC_PTP_CLOCK) += fsl-enetc-ptp.o
>  fsl-enetc-ptp-y := enetc_ptp.o
> +
> +obj-$(CONFIG_NXP_NETC_BLK_CTRL) += nxp-netc-blk-ctrl.o
> +nxp-netc-blk-ctrl-y := netc_blk_ctrl.o
> \ No newline at end of file
> diff --git a/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> new file mode 100644
> index 000000000000..b8eec980c199
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + * NXP NETC Blocks Control Driver
> + *
> + * Copyright 2024 NXP
> + */
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/fsl/netc_global.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_net.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +
> +/* NETCMIX registers */
> +#define IMX95_CFG_LINK_IO_VAR		0x0
> +#define  IO_VAR_16FF_16G_SERDES		0x1
> +#define  IO_VAR(port, var)		(((var) & 0xf) << ((port) << 2))
> +
> +#define IMX95_CFG_LINK_MII_PROT		0x4
> +#define CFG_LINK_MII_PORT_0		GENMASK(3, 0)
> +#define CFG_LINK_MII_PORT_1		GENMASK(7, 4)

need #include <linux/bits.h>

> +#define  MII_PROT_MII			0x0
> +#define  MII_PROT_RMII			0x1
> +#define  MII_PROT_RGMII			0x2
> +#define  MII_PROT_SERIAL		0x3
> +#define  MII_PROT(port, prot)		(((prot) & 0xf) << ((port) << 2))
> +
> +#define IMX95_CFG_LINK_PCS_PROT(a)	(0x8 + (a) * 4)
> +#define PCS_PROT_1G_SGMII		BIT(0)
> +#define PCS_PROT_2500M_SGMII		BIT(1)
> +#define PCS_PROT_XFI			BIT(3)
> +#define PCS_PROT_SFI			BIT(4)
> +#define PCS_PROT_10G_SXGMII		BIT(6)
> +
> +/* NETC privileged register block register */
> +#define PRB_NETCRR			0x100
> +#define  NETCRR_SR			BIT(0)
> +#define  NETCRR_LOCK			BIT(1)
> +
> +#define PRB_NETCSR			0x104
> +#define  NETCSR_ERROR			BIT(0)
> +#define  NETCSR_STATE			BIT(1)
> +
> +/* NETC integrated endpoint register block register */
> +#define IERB_EMDIOFAUXR			0x344
> +#define IERB_T0FAUXR			0x444
> +#define IERB_EFAUXR(a)			(0x3044 + 0x100 * (a))
> +#define IERB_VFAUXR(a)			(0x4004 + 0x40 * (a))
> +#define FAUXR_LDID			GENMASK(3, 0)
> +
> +/* Platform information */
> +#define IMX95_ENETC0_BUS_DEVFN		0x0
> +#define IMX95_ENETC1_BUS_DEVFN		0x40
> +#define IMX95_ENETC2_BUS_DEVFN		0x80
> +
> +/* Flags for different platforms */
> +#define NETC_HAS_NETCMIX		BIT(0)
> +
> +struct netc_devinfo {
> +	u32 flags;
> +	int (*netcmix_init)(struct platform_device *pdev);
> +	int (*ierb_init)(struct platform_device *pdev);
> +};
> +
> +struct netc_blk_ctrl {
> +	void __iomem *prb;
> +	void __iomem *ierb;
> +	void __iomem *netcmix;
> +	struct clk *ipg_clk;
> +
> +	const struct netc_devinfo *devinfo;
> +	struct platform_device *pdev;
> +	struct dentry *debugfs_root;
> +};
> +
> +static void netc_reg_write(void __iomem *base, u32 offset, u32 val)
> +{
> +	netc_write(base + offset, val);
> +}
> +
> +static u32 netc_reg_read(void __iomem *base, u32 offset)
> +{
> +	return netc_read(base + offset);
> +}
> +
> +static int netc_of_pci_get_bus_devfn(struct device_node *np)
> +{
> +	u32 reg[5];
> +	int error;
> +
> +	error = of_property_read_u32_array(np, "reg", reg, ARRAY_SIZE(reg));
> +	if (error)
> +		return error;

Avoid parse these common property "reg". if you need untranslate address
you can use of_property_read_reg(), if you need bus translated cpu
address, you can use platform_get_resource().

Frank
> +
> +	return (reg[0] >> 8) & 0xffff;
> +}
> +
> +static int netc_get_link_mii_protocol(phy_interface_t interface)
> +{
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		return MII_PROT_MII;
> +	case PHY_INTERFACE_MODE_RMII:
> +		return MII_PROT_RMII;
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		return MII_PROT_RGMII;
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_XGMII:
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		return MII_PROT_SERIAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int imx95_netcmix_init(struct platform_device *pdev)
> +{
> +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> +	struct device_node *np = pdev->dev.of_node;
> +	phy_interface_t interface;
> +	int bus_devfn, mii_proto;
> +	u32 val;
> +	int err;
> +
> +	/* Default setting of MII protocol */
> +	val = MII_PROT(0, MII_PROT_RGMII) | MII_PROT(1, MII_PROT_RGMII) |
> +	      MII_PROT(2, MII_PROT_SERIAL);
> +
> +	/* Update the link MII protocol through parsing phy-mode */
> +	for_each_available_child_of_node_scoped(np, child) {
> +		for_each_available_child_of_node_scoped(child, gchild) {
> +			if (!of_device_is_compatible(gchild, "nxp,imx95-enetc"))
> +				continue;
> +
> +			bus_devfn = netc_of_pci_get_bus_devfn(gchild);
> +			if (bus_devfn < 0)
> +				return -EINVAL;
> +
> +			if (bus_devfn == IMX95_ENETC2_BUS_DEVFN)
> +				continue;
> +
> +			err = of_get_phy_mode(gchild, &interface);
> +			if (err)
> +				continue;
> +
> +			mii_proto = netc_get_link_mii_protocol(interface);
> +			if (mii_proto < 0)
> +				return -EINVAL;
> +
> +			switch (bus_devfn) {
> +			case IMX95_ENETC0_BUS_DEVFN:
> +				val = u32_replace_bits(val, mii_proto,
> +						       CFG_LINK_MII_PORT_0);
> +				break;
> +			case IMX95_ENETC1_BUS_DEVFN:
> +				val = u32_replace_bits(val, mii_proto,
> +						       CFG_LINK_MII_PORT_1);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	/* Configure Link I/O variant */
> +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_IO_VAR,
> +		       IO_VAR(2, IO_VAR_16FF_16G_SERDES));
> +	/* Configure Link 2 PCS protocol */
> +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_PCS_PROT(2),
> +		       PCS_PROT_10G_SXGMII);
> +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_MII_PROT, val);
> +
> +	return 0;
> +}
> +
> +static bool netc_ierb_is_locked(struct netc_blk_ctrl *priv)
> +{
> +	return !!(netc_reg_read(priv->prb, PRB_NETCRR) & NETCRR_LOCK);
> +}
> +
> +static int netc_lock_ierb(struct netc_blk_ctrl *priv)
> +{
> +	u32 val;
> +
> +	netc_reg_write(priv->prb, PRB_NETCRR, NETCRR_LOCK);
> +
> +	return read_poll_timeout(netc_reg_read, val, !(val & NETCSR_STATE),
> +				 100, 2000, false, priv->prb, PRB_NETCSR);
> +}
> +
> +static int netc_unlock_ierb_with_warm_reset(struct netc_blk_ctrl *priv)
> +{
> +	u32 val;
> +
> +	netc_reg_write(priv->prb, PRB_NETCRR, 0);
> +
> +	return read_poll_timeout(netc_reg_read, val, !(val & NETCRR_LOCK),
> +				 1000, 100000, true, priv->prb, PRB_NETCRR);
> +}
> +
> +static int imx95_ierb_init(struct platform_device *pdev)
> +{
> +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> +
> +	/* EMDIO : No MSI-X intterupt */
> +	netc_reg_write(priv->ierb, IERB_EMDIOFAUXR, 0);
> +	/* ENETC0 PF */
> +	netc_reg_write(priv->ierb, IERB_EFAUXR(0), 0);
> +	/* ENETC0 VF0 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(0), 1);
> +	/* ENETC0 VF1 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(1), 2);
> +	/* ENETC1 PF */
> +	netc_reg_write(priv->ierb, IERB_EFAUXR(1), 3);
> +	/* ENETC1 VF0 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(2), 5);
> +	/* ENETC1 VF1 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(3), 6);
> +	/* ENETC2 PF */
> +	netc_reg_write(priv->ierb, IERB_EFAUXR(2), 4);
> +	/* ENETC2 VF0 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(4), 5);
> +	/* ENETC2 VF1 */
> +	netc_reg_write(priv->ierb, IERB_VFAUXR(5), 6);
> +	/* NETC TIMER */
> +	netc_reg_write(priv->ierb, IERB_T0FAUXR, 7);
> +
> +	return 0;
> +}
> +
> +static int netc_ierb_init(struct platform_device *pdev)
> +{
> +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> +	const struct netc_devinfo *devinfo = priv->devinfo;
> +	int err;
> +
> +	if (netc_ierb_is_locked(priv)) {
> +		err = netc_unlock_ierb_with_warm_reset(priv);
> +		if (err) {
> +			dev_err(&pdev->dev, "Unlock IERB failed.\n");
> +			return err;
> +		}
> +	}
> +
> +	if (devinfo->ierb_init) {
> +		err = devinfo->ierb_init(pdev);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = netc_lock_ierb(priv);
> +	if (err) {
> +		dev_err(&pdev->dev, "Lock IERB failed.\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static int netc_prb_show(struct seq_file *s, void *data)
> +{
> +	struct netc_blk_ctrl *priv = s->private;
> +	u32 val;
> +
> +	val = netc_reg_read(priv->prb, PRB_NETCRR);
> +	seq_printf(s, "[PRB NETCRR] Lock:%d SR:%d\n",
> +		   (val & NETCRR_LOCK) ? 1 : 0,
> +		   (val & NETCRR_SR) ? 1 : 0);
> +
> +	val = netc_reg_read(priv->prb, PRB_NETCSR);
> +	seq_printf(s, "[PRB NETCSR] State:%d Error:%d\n",
> +		   (val & NETCSR_STATE) ? 1 : 0,
> +		   (val & NETCSR_ERROR) ? 1 : 0);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(netc_prb);
> +
> +static void netc_blk_ctrl_create_debugfs(struct netc_blk_ctrl *priv)
> +{
> +	struct dentry *root;
> +
> +	root = debugfs_create_dir("netc_blk_ctrl", NULL);
> +	if (IS_ERR(root))
> +		return;
> +
> +	priv->debugfs_root = root;
> +
> +	debugfs_create_file("prb", 0444, root, priv, &netc_prb_fops);
> +}
> +
> +static void netc_blk_ctrl_remove_debugfs(struct netc_blk_ctrl *priv)
> +{
> +	debugfs_remove_recursive(priv->debugfs_root);
> +	priv->debugfs_root = NULL;
> +}
> +
> +#else
> +
> +static void netc_blk_ctrl_create_debugfs(struct netc_blk_ctrl *priv)
> +{
> +}
> +
> +static void netc_blk_ctrl_remove_debugfs(struct netc_blk_ctrl *priv)
> +{
> +}
> +#endif
> +
> +static int netc_prb_check_error(struct netc_blk_ctrl *priv)
> +{
> +	u32 val;
> +
> +	val = netc_reg_read(priv->prb, PRB_NETCSR);
> +	if (val & NETCSR_ERROR)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static const struct netc_devinfo imx95_devinfo = {
> +	.flags = NETC_HAS_NETCMIX,
> +	.netcmix_init = imx95_netcmix_init,
> +	.ierb_init = imx95_ierb_init,
> +};
> +
> +static const struct of_device_id netc_blk_ctrl_match[] = {
> +	{ .compatible = "nxp,imx95-netc-blk-ctrl", .data = &imx95_devinfo },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, netc_blk_ctrl_match);
> +
> +static int netc_blk_ctrl_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	const struct netc_devinfo *devinfo;
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *id;
> +	struct netc_blk_ctrl *priv;
> +	void __iomem *regs;
> +	int err;
> +
> +	if (!node || !of_device_is_available(node)) {
> +		dev_info(dev, "Device is disabled, skipping\n");
> +		return -ENODEV;
> +	}

look like needn't above check.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pdev = pdev;
> +	priv->ipg_clk = devm_clk_get_optional(dev, "ipg_clk");
> +	if (IS_ERR(priv->ipg_clk)) {
> +		dev_err(dev, "Get ipg_clk failed\n");
> +		err = PTR_ERR(priv->ipg_clk);
> +		return err;

return dev_err_probe(dev, err, "...");
same for below error path.

> +	}
> +
> +	err = clk_prepare_enable(priv->ipg_clk);
> +	if (err) {
> +		dev_err(dev, "Enable ipg_clk failed\n");
> +		goto disable_ipg_clk;
> +	}
> +
> +	id = of_match_device(netc_blk_ctrl_match, dev);
> +	if (!id) {
> +		dev_err(dev, "Cannot match device\n");
> +		err = -EINVAL;
> +		goto disable_ipg_clk;
> +	}
> +
> +	devinfo = (struct netc_devinfo *)id->data;
> +	if (!devinfo) {
> +		dev_err(dev, "No device information\n");
> +		err = -EINVAL;
> +		goto disable_ipg_clk;
> +	}
> +	priv->devinfo = devinfo;
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "ierb");
> +	if (IS_ERR(regs)) {
> +		err = PTR_ERR(regs);
> +		dev_err(dev, "Missing IERB resource\n");
> +		goto disable_ipg_clk;
> +	}
> +	priv->ierb = regs;
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "prb");
> +	if (IS_ERR(regs)) {
> +		err = PTR_ERR(regs);
> +		dev_err(dev, "Missing PRB resource\n");
> +		goto disable_ipg_clk;
> +	}
> +	priv->prb = regs;
> +
> +	if (devinfo->flags & NETC_HAS_NETCMIX) {
> +		regs = devm_platform_ioremap_resource_byname(pdev, "netcmix");
> +		if (IS_ERR(regs)) {
> +			err = PTR_ERR(regs);
> +			dev_err(dev, "Missing NETCMIX resource\n");
> +			goto disable_ipg_clk;
> +		}
> +		priv->netcmix = regs;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (devinfo->netcmix_init) {
> +		err = devinfo->netcmix_init(pdev);
> +		if (err) {
> +			dev_err(dev, "Initializing NETCMIX failed\n");
> +			goto disable_ipg_clk;
> +		}
> +	}
> +
> +	err = netc_ierb_init(pdev);
> +	if (err) {
> +		dev_err(dev, "Initializing IERB failed.\n");
> +		goto disable_ipg_clk;
> +	}
> +
> +	if (netc_prb_check_error(priv) < 0)
> +		dev_warn(dev, "The current IERB configuration is invalid.\n");
> +
> +	netc_blk_ctrl_create_debugfs(priv);
> +
> +	err = of_platform_populate(node, NULL, NULL, dev);
> +	if (err) {
> +		dev_err(dev, "of_platform_populate failed\n");
> +		goto remove_debugfs;
> +	}
> +
> +	return 0;
> +
> +remove_debugfs:
> +	netc_blk_ctrl_remove_debugfs(priv);
> +disable_ipg_clk:
> +	clk_disable_unprepare(priv->ipg_clk);
> +
> +	return err;
> +}
> +
> +static void netc_blk_ctrl_remove(struct platform_device *pdev)
> +{
> +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> +
> +	of_platform_depopulate(&pdev->dev);
> +	netc_blk_ctrl_remove_debugfs(priv);
> +	clk_disable_unprepare(priv->ipg_clk);
> +}
> +
> +static struct platform_driver netc_blk_ctrl_driver = {
> +	.driver = {
> +		.name = "nxp-netc-blk-ctrl",
> +		.of_match_table = netc_blk_ctrl_match,
> +	},
> +	.probe = netc_blk_ctrl_probe,
> +	.remove = netc_blk_ctrl_remove,
> +};
> +
> +module_platform_driver(netc_blk_ctrl_driver);
> +
> +MODULE_DESCRIPTION("NXP NETC Blocks Control Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
> new file mode 100644
> index 000000000000..f26b1b6f8813
> --- /dev/null
> +++ b/include/linux/fsl/netc_global.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/* Copyright 2024 NXP
> + */
> +#ifndef __NETC_GLOBAL_H
> +#define __NETC_GLOBAL_H
> +
> +#include <linux/io.h>
> +
> +static inline u32 netc_read(void __iomem *reg)
> +{
> +	return ioread32(reg);
> +}
> +
> +#ifdef ioread64
> +static inline u64 netc_read64(void __iomem *reg)
> +{
> +	return ioread64(reg);
> +}
> +#else
> +static inline u64 netc_read64(void __iomem *reg)
> +{
> +	u32 low, high;
> +	u64 val;
> +
> +	low = ioread32(reg);
> +	high = ioread32(reg + 4);
> +
> +	val = (u64)high << 32 | low;
> +
> +	return val;
> +}
> +#endif
> +
> +static inline void netc_write(void __iomem *reg, u32 val)
> +{
> +	iowrite32(val, reg);
> +}

why need two layer register read/write wrap?

netc_reg_write() -> netc_write() -> iowrite32();

Frank
> +
> +#endif
> --
> 2.34.1
>
Frank Li Oct. 9, 2024, 5:16 p.m. UTC | #2
On Wed, Oct 09, 2024 at 05:51:10PM +0800, Wei Fang wrote:
> The ENETC of LS1028A is revision 1.0. Now, ENETC is used on the i.MX95
> platform and the revision is upgraded to version 4.1. The two versions
> are incompatible except for the station interface (SI) part. Therefore,
> we need to add a new driver for ENETC revision 4.1 and later. However,
  ^^^^^^^^^^
 just Add a new driver ...

> the logic of some interfaces of the two drivers is basically the same,
> and the only difference is the hardware configuration. So in order to
> reuse these interfaces and reduce code redundancy, we extract these
                                                     ^^
extract these .. in order to ...

> interfaces and compile them into a separate enetc-pf-common driver for
> use by these two PF drivers. Note that the ENETC PF 4.1 driver will be
                               ^^^
Prepare to add support ENETC PF 4.1 driver in subsequent patches.

> supported in subsequent patches.

>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/Kconfig  |   9 +
>  drivers/net/ethernet/freescale/enetc/Makefile |   3 +
>  .../net/ethernet/freescale/enetc/enetc_pf.c   | 350 +---------------
>  .../net/ethernet/freescale/enetc/enetc_pf.h   |  28 ++
>  .../freescale/enetc/enetc_pf_common.c         | 375 ++++++++++++++++++
>  5 files changed, 431 insertions(+), 334 deletions(-)
>  create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
>
> diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
> index 51d80ea959d4..6f3306f14060 100644
> --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> @@ -7,10 +7,19 @@ config FSL_ENETC_CORE
>
>  	  If compiled as module (M), the module name is fsl-enetc-core.
>
> +config NXP_ENETC_PF_COMMON
> +	tristate "ENETC PF common functionality driver"
> +	help
> +	  This module supports common functionality between drivers of
> +	  different versions of NXP ENETC PF controllers.
> +
> +	  If compiled as module (M), the module name is nxp-enetc-pf-common.
> +
>  config FSL_ENETC
>  	tristate "ENETC PF driver"
>  	depends on PCI_MSI
>  	select MDIO_DEVRES
> +	select NXP_ENETC_PF_COMMON
>  	select FSL_ENETC_CORE
>  	select FSL_ENETC_IERB
>  	select FSL_ENETC_MDIO
> diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
> index 5c277910d538..b81ca462e358 100644
> --- a/drivers/net/ethernet/freescale/enetc/Makefile
> +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> @@ -3,6 +3,9 @@
>  obj-$(CONFIG_FSL_ENETC_CORE) += fsl-enetc-core.o
>  fsl-enetc-core-y := enetc.o enetc_cbdr.o enetc_ethtool.o
>
> +obj-$(CONFIG_NXP_ENETC_PF_COMMON) += nxp-enetc-pf-common.o
> +nxp-enetc-pf-common-y := enetc_pf_common.o
> +

I am not sure why you can't link enetc_pf_common.o into enetc_pf.o?

Frank

>  obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
>  fsl-enetc-y := enetc_pf.o
>  fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 8f6b0bf48139..dae8be4a1607 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -33,18 +33,15 @@ static void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si,
>  	__raw_writew(lower, hw->port + ENETC_PSIPMAR1(si));
>  }
>
> -static int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr)
> +static struct phylink_pcs *enetc_pf_create_pcs(struct enetc_pf *pf,
> +					       struct mii_bus *bus)
>  {
> -	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> -	struct sockaddr *saddr = addr;
> -
> -	if (!is_valid_ether_addr(saddr->sa_data))
> -		return -EADDRNOTAVAIL;
> -
> -	eth_hw_addr_set(ndev, saddr->sa_data);
> -	enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data);
> +	return lynx_pcs_create_mdiodev(bus, 0);
> +}
>
> -	return 0;
> +static void enetc_pf_destroy_pcs(struct phylink_pcs *pcs)
> +{
> +	lynx_pcs_destroy(pcs);
>  }
>
>  static void enetc_set_vlan_promisc(struct enetc_hw *hw, char si_map)
> @@ -393,56 +390,6 @@ static int enetc_pf_set_vf_spoofchk(struct net_device *ndev, int vf, bool en)
>  	return 0;
>  }
>
> -static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf,
> -				   int si)
> -{
> -	struct device *dev = &pf->si->pdev->dev;
> -	struct enetc_hw *hw = &pf->si->hw;
> -	u8 mac_addr[ETH_ALEN] = { 0 };
> -	int err;
> -
> -	/* (1) try to get the MAC address from the device tree */
> -	if (np) {
> -		err = of_get_mac_address(np, mac_addr);
> -		if (err == -EPROBE_DEFER)
> -			return err;
> -	}
> -
> -	/* (2) bootloader supplied MAC address */
> -	if (is_zero_ether_addr(mac_addr))
> -		enetc_pf_get_primary_mac_addr(hw, si, mac_addr);
> -
> -	/* (3) choose a random one */
> -	if (is_zero_ether_addr(mac_addr)) {
> -		eth_random_addr(mac_addr);
> -		dev_info(dev, "no MAC address specified for SI%d, using %pM\n",
> -			 si, mac_addr);
> -	}
> -
> -	enetc_pf_set_primary_mac_addr(hw, si, mac_addr);
> -
> -	return 0;
> -}
> -
> -static int enetc_setup_mac_addresses(struct device_node *np,
> -				     struct enetc_pf *pf)
> -{
> -	int err, i;
> -
> -	/* The PF might take its MAC from the device tree */
> -	err = enetc_setup_mac_address(np, pf, 0);
> -	if (err)
> -		return err;
> -
> -	for (i = 0; i < pf->total_vfs; i++) {
> -		err = enetc_setup_mac_address(NULL, pf, i + 1);
> -		if (err)
> -			return err;
> -	}
> -
> -	return 0;
> -}
> -
>  static void enetc_port_assign_rfs_entries(struct enetc_si *si)
>  {
>  	struct enetc_pf *pf = enetc_si_priv(si);
> @@ -656,55 +603,6 @@ void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int vf_id, u16 *status)
>  	}
>  }
>
> -#ifdef CONFIG_PCI_IOV
> -static int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs)
> -{
> -	struct enetc_si *si = pci_get_drvdata(pdev);
> -	struct enetc_pf *pf = enetc_si_priv(si);
> -	int err;
> -
> -	if (!num_vfs) {
> -		enetc_msg_psi_free(pf);
> -		kfree(pf->vf_state);
> -		pf->num_vfs = 0;
> -		pci_disable_sriov(pdev);
> -	} else {
> -		pf->num_vfs = num_vfs;
> -
> -		pf->vf_state = kcalloc(num_vfs, sizeof(struct enetc_vf_state),
> -				       GFP_KERNEL);
> -		if (!pf->vf_state) {
> -			pf->num_vfs = 0;
> -			return -ENOMEM;
> -		}
> -
> -		err = enetc_msg_psi_init(pf);
> -		if (err) {
> -			dev_err(&pdev->dev, "enetc_msg_psi_init (%d)\n", err);
> -			goto err_msg_psi;
> -		}
> -
> -		err = pci_enable_sriov(pdev, num_vfs);
> -		if (err) {
> -			dev_err(&pdev->dev, "pci_enable_sriov err %d\n", err);
> -			goto err_en_sriov;
> -		}
> -	}
> -
> -	return num_vfs;
> -
> -err_en_sriov:
> -	enetc_msg_psi_free(pf);
> -err_msg_psi:
> -	kfree(pf->vf_state);
> -	pf->num_vfs = 0;
> -
> -	return err;
> -}
> -#else
> -#define enetc_sriov_configure(pdev, num_vfs)	(void)0
> -#endif
> -
>  static int enetc_pf_set_features(struct net_device *ndev,
>  				 netdev_features_t features)
>  {
> @@ -775,187 +673,6 @@ static const struct net_device_ops enetc_ndev_ops = {
>  	.ndo_xdp_xmit		= enetc_xdp_xmit,
>  };
>
> -static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> -				  const struct net_device_ops *ndev_ops)
> -{
> -	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> -
> -	SET_NETDEV_DEV(ndev, &si->pdev->dev);
> -	priv->ndev = ndev;
> -	priv->si = si;
> -	priv->dev = &si->pdev->dev;
> -	si->ndev = ndev;
> -
> -	priv->msg_enable = (NETIF_MSG_WOL << 1) - 1;
> -	ndev->netdev_ops = ndev_ops;
> -	enetc_set_ethtool_ops(ndev);
> -	ndev->watchdog_timeo = 5 * HZ;
> -	ndev->max_mtu = ENETC_MAX_MTU;
> -
> -	ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM |
> -			    NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
> -			    NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK |
> -			    NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6;
> -	ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM |
> -			 NETIF_F_HW_VLAN_CTAG_TX |
> -			 NETIF_F_HW_VLAN_CTAG_RX |
> -			 NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6;
> -	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM |
> -			      NETIF_F_TSO | NETIF_F_TSO6;
> -
> -	if (si->num_rss)
> -		ndev->hw_features |= NETIF_F_RXHASH;
> -
> -	ndev->priv_flags |= IFF_UNICAST_FLT;
> -	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> -			     NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG |
> -			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> -
> -	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
> -		priv->active_offloads |= ENETC_F_QCI;
> -		ndev->features |= NETIF_F_HW_TC;
> -		ndev->hw_features |= NETIF_F_HW_TC;
> -	}
> -
> -	/* pick up primary MAC address from SI */
> -	enetc_load_primary_mac_addr(&si->hw, ndev);
> -}
> -
> -static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
> -{
> -	struct device *dev = &pf->si->pdev->dev;
> -	struct enetc_mdio_priv *mdio_priv;
> -	struct mii_bus *bus;
> -	int err;
> -
> -	bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv));
> -	if (!bus)
> -		return -ENOMEM;
> -
> -	bus->name = "Freescale ENETC MDIO Bus";
> -	bus->read = enetc_mdio_read_c22;
> -	bus->write = enetc_mdio_write_c22;
> -	bus->read_c45 = enetc_mdio_read_c45;
> -	bus->write_c45 = enetc_mdio_write_c45;
> -	bus->parent = dev;
> -	mdio_priv = bus->priv;
> -	mdio_priv->hw = &pf->si->hw;
> -	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
> -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> -
> -	err = of_mdiobus_register(bus, np);
> -	if (err)
> -		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> -
> -	pf->mdio = bus;
> -
> -	return 0;
> -}
> -
> -static void enetc_mdio_remove(struct enetc_pf *pf)
> -{
> -	if (pf->mdio)
> -		mdiobus_unregister(pf->mdio);
> -}
> -
> -static int enetc_imdio_create(struct enetc_pf *pf)
> -{
> -	struct device *dev = &pf->si->pdev->dev;
> -	struct enetc_mdio_priv *mdio_priv;
> -	struct phylink_pcs *phylink_pcs;
> -	struct mii_bus *bus;
> -	int err;
> -
> -	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
> -	if (!bus)
> -		return -ENOMEM;
> -
> -	bus->name = "Freescale ENETC internal MDIO Bus";
> -	bus->read = enetc_mdio_read_c22;
> -	bus->write = enetc_mdio_write_c22;
> -	bus->read_c45 = enetc_mdio_read_c45;
> -	bus->write_c45 = enetc_mdio_write_c45;
> -	bus->parent = dev;
> -	bus->phy_mask = ~0;
> -	mdio_priv = bus->priv;
> -	mdio_priv->hw = &pf->si->hw;
> -	mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
> -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
> -
> -	err = mdiobus_register(bus);
> -	if (err) {
> -		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);
> -		goto free_mdio_bus;
> -	}
> -
> -	phylink_pcs = lynx_pcs_create_mdiodev(bus, 0);
> -	if (IS_ERR(phylink_pcs)) {
> -		err = PTR_ERR(phylink_pcs);
> -		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
> -		goto unregister_mdiobus;
> -	}
> -
> -	pf->imdio = bus;
> -	pf->pcs = phylink_pcs;
> -
> -	return 0;
> -
> -unregister_mdiobus:
> -	mdiobus_unregister(bus);
> -free_mdio_bus:
> -	mdiobus_free(bus);
> -	return err;
> -}
> -
> -static void enetc_imdio_remove(struct enetc_pf *pf)
> -{
> -	if (pf->pcs)
> -		lynx_pcs_destroy(pf->pcs);
> -	if (pf->imdio) {
> -		mdiobus_unregister(pf->imdio);
> -		mdiobus_free(pf->imdio);
> -	}
> -}
> -
> -static bool enetc_port_has_pcs(struct enetc_pf *pf)
> -{
> -	return (pf->if_mode == PHY_INTERFACE_MODE_SGMII ||
> -		pf->if_mode == PHY_INTERFACE_MODE_1000BASEX ||
> -		pf->if_mode == PHY_INTERFACE_MODE_2500BASEX ||
> -		pf->if_mode == PHY_INTERFACE_MODE_USXGMII);
> -}
> -
> -static int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node)
> -{
> -	struct device_node *mdio_np;
> -	int err;
> -
> -	mdio_np = of_get_child_by_name(node, "mdio");
> -	if (mdio_np) {
> -		err = enetc_mdio_probe(pf, mdio_np);
> -
> -		of_node_put(mdio_np);
> -		if (err)
> -			return err;
> -	}
> -
> -	if (enetc_port_has_pcs(pf)) {
> -		err = enetc_imdio_create(pf);
> -		if (err) {
> -			enetc_mdio_remove(pf);
> -			return err;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static void enetc_mdiobus_destroy(struct enetc_pf *pf)
> -{
> -	enetc_mdio_remove(pf);
> -	enetc_imdio_remove(pf);
> -}
> -
>  static struct phylink_pcs *
>  enetc_pl_mac_select_pcs(struct phylink_config *config, phy_interface_t iface)
>  {
> @@ -1101,47 +818,6 @@ static const struct phylink_mac_ops enetc_mac_phylink_ops = {
>  	.mac_link_down = enetc_pl_mac_link_down,
>  };
>
> -static int enetc_phylink_create(struct enetc_ndev_priv *priv,
> -				struct device_node *node)
> -{
> -	struct enetc_pf *pf = enetc_si_priv(priv->si);
> -	struct phylink *phylink;
> -	int err;
> -
> -	pf->phylink_config.dev = &priv->ndev->dev;
> -	pf->phylink_config.type = PHYLINK_NETDEV;
> -	pf->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> -		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
> -
> -	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> -		  pf->phylink_config.supported_interfaces);
> -	__set_bit(PHY_INTERFACE_MODE_SGMII,
> -		  pf->phylink_config.supported_interfaces);
> -	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> -		  pf->phylink_config.supported_interfaces);
> -	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> -		  pf->phylink_config.supported_interfaces);
> -	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> -		  pf->phylink_config.supported_interfaces);
> -	phy_interface_set_rgmii(pf->phylink_config.supported_interfaces);
> -
> -	phylink = phylink_create(&pf->phylink_config, of_fwnode_handle(node),
> -				 pf->if_mode, &enetc_mac_phylink_ops);
> -	if (IS_ERR(phylink)) {
> -		err = PTR_ERR(phylink);
> -		return err;
> -	}
> -
> -	priv->phylink = phylink;
> -
> -	return 0;
> -}
> -
> -static void enetc_phylink_destroy(struct enetc_ndev_priv *priv)
> -{
> -	phylink_destroy(priv->phylink);
> -}
> -
>  /* Initialize the entire shared memory for the flow steering entries
>   * of this port (PF + VFs)
>   */
> @@ -1259,6 +935,13 @@ static void enetc_psi_destroy(struct pci_dev *pdev)
>  	enetc_pci_remove(pdev);
>  }
>
> +static const struct enetc_pf_ops enetc_pf_ops = {
> +	.set_si_primary_mac = enetc_pf_set_primary_mac_addr,
> +	.get_si_primary_mac = enetc_pf_get_primary_mac_addr,
> +	.create_pcs = enetc_pf_create_pcs,
> +	.destroy_pcs = enetc_pf_destroy_pcs,
> +};
> +

I suppose this patch should just move functions to common.c. This involve
addition code logic change. It is not easy to follow up to make sure your
change is correct.

Frank

>  static int enetc_pf_probe(struct pci_dev *pdev,
>  			  const struct pci_device_id *ent)
>  {
> @@ -1286,6 +969,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
>  	pf = enetc_si_priv(si);
>  	pf->si = si;
>  	pf->total_vfs = pci_sriov_get_totalvfs(pdev);
> +	enetc_pf_ops_register(pf, &enetc_pf_ops);
>
>  	err = enetc_setup_mac_addresses(node, pf);
>  	if (err)
> @@ -1338,7 +1022,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
>  	if (err)
>  		goto err_mdiobus_create;
>
> -	err = enetc_phylink_create(priv, node);
> +	err = enetc_phylink_create(priv, node, &enetc_mac_phylink_ops);
>  	if (err)
>  		goto err_phylink_create;
>
> @@ -1422,9 +1106,7 @@ static struct pci_driver enetc_pf_driver = {
>  	.id_table = enetc_pf_id_table,
>  	.probe = enetc_pf_probe,
>  	.remove = enetc_pf_remove,
> -#ifdef CONFIG_PCI_IOV
>  	.sriov_configure = enetc_sriov_configure,
> -#endif
>  };
>  module_pci_driver(enetc_pf_driver);
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> index c26bd66e4597..ad7dab0eb752 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> @@ -28,6 +28,15 @@ struct enetc_vf_state {
>  	enum enetc_vf_flags flags;
>  };
>
> +struct enetc_pf;
> +
> +struct enetc_pf_ops {
> +	void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr);
> +	void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr);
> +	struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus);
> +	void (*destroy_pcs)(struct phylink_pcs *pcs);
> +};
> +
>  struct enetc_pf {
>  	struct enetc_si *si;
>  	int num_vfs; /* number of active VFs, after sriov_init */
> @@ -50,6 +59,8 @@ struct enetc_pf {
>
>  	phy_interface_t if_mode;
>  	struct phylink_config phylink_config;
> +
> +	const struct enetc_pf_ops *ops;
>  };
>
>  #define phylink_to_enetc_pf(config) \
> @@ -58,3 +69,20 @@ struct enetc_pf {
>  int enetc_msg_psi_init(struct enetc_pf *pf);
>  void enetc_msg_psi_free(struct enetc_pf *pf);
>  void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16 *status);
> +
> +int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr);
> +int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf *pf);
> +void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> +			   const struct net_device_ops *ndev_ops);
> +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node);
> +void enetc_mdiobus_destroy(struct enetc_pf *pf);
> +int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node,
> +			 const struct phylink_mac_ops *pl_mac_ops);
> +void enetc_phylink_destroy(struct enetc_ndev_priv *priv);
> +int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs);
> +
> +static inline void enetc_pf_ops_register(struct enetc_pf *pf,
> +					 const struct enetc_pf_ops *ops)
> +{
> +	pf->ops = ops;
> +}
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> new file mode 100644
> index 000000000000..bbfb5c1ffd13
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Copyright 2024 NXP */
> +#include <linux/fsl/enetc_mdio.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +
> +#include "enetc_pf.h"
> +
> +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +
> +	if (pf->ops->set_si_primary_mac)
> +		pf->ops->set_si_primary_mac(hw, si, mac_addr);
> +	else
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr)
> +{
> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> +	struct sockaddr *saddr = addr;
> +	int err;
> +
> +	if (!is_valid_ether_addr(saddr->sa_data))
> +		return -EADDRNOTAVAIL;
> +
> +	err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data);
> +	if (err)
> +		return err;
> +
> +	eth_hw_addr_set(ndev, saddr->sa_data);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr);
> +
> +static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf,
> +				   int si)
> +{
> +	struct device *dev = &pf->si->pdev->dev;
> +	struct enetc_hw *hw = &pf->si->hw;
> +	u8 mac_addr[ETH_ALEN] = { 0 };
> +	int err;
> +
> +	/* (1) try to get the MAC address from the device tree */
> +	if (np) {
> +		err = of_get_mac_address(np, mac_addr);
> +		if (err == -EPROBE_DEFER)
> +			return err;
> +	}
> +
> +	/* (2) bootloader supplied MAC address */
> +	if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac)
> +		pf->ops->get_si_primary_mac(hw, si, mac_addr);
> +
> +	/* (3) choose a random one */
> +	if (is_zero_ether_addr(mac_addr)) {
> +		eth_random_addr(mac_addr);
> +		dev_info(dev, "no MAC address specified for SI%d, using %pM\n",
> +			 si, mac_addr);
> +	}
> +
> +	err = enetc_set_si_hw_addr(pf, si, mac_addr);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf *pf)
> +{
> +	int err, i;
> +
> +	/* The PF might take its MAC from the device tree */
> +	err = enetc_setup_mac_address(np, pf, 0);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < pf->total_vfs; i++) {
> +		err = enetc_setup_mac_address(NULL, pf, i + 1);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_setup_mac_addresses);
> +
> +void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> +			   const struct net_device_ops *ndev_ops)
> +{
> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +
> +	SET_NETDEV_DEV(ndev, &si->pdev->dev);
> +	priv->ndev = ndev;
> +	priv->si = si;
> +	priv->dev = &si->pdev->dev;
> +	si->ndev = ndev;
> +
> +	priv->msg_enable = (NETIF_MSG_WOL << 1) - 1;
> +	ndev->netdev_ops = ndev_ops;
> +	enetc_set_ethtool_ops(ndev);
> +	ndev->watchdog_timeo = 5 * HZ;
> +	ndev->max_mtu = ENETC_MAX_MTU;
> +
> +	ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM |
> +			    NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
> +			    NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK |
> +			    NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6 |
> +			    NETIF_F_GSO_UDP_L4;
> +	ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM |
> +			 NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
> +			 NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6 |
> +			 NETIF_F_GSO_UDP_L4;
> +	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_TSO |
> +			      NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4;
> +
> +	if (si->num_rss)
> +		ndev->hw_features |= NETIF_F_RXHASH;
> +
> +	ndev->priv_flags |= IFF_UNICAST_FLT;
> +	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> +			     NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG |
> +			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> +
> +	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
> +		priv->active_offloads |= ENETC_F_QCI;
> +		ndev->features |= NETIF_F_HW_TC;
> +		ndev->hw_features |= NETIF_F_HW_TC;
> +	}
> +
> +	/* pick up primary MAC address from SI */
> +	enetc_load_primary_mac_addr(&si->hw, ndev);
> +}
> +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup);
> +
> +static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
> +{
> +	struct device *dev = &pf->si->pdev->dev;
> +	struct enetc_mdio_priv *mdio_priv;
> +	struct mii_bus *bus;
> +	int err;
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Freescale ENETC MDIO Bus";
> +	bus->read = enetc_mdio_read_c22;
> +	bus->write = enetc_mdio_write_c22;
> +	bus->read_c45 = enetc_mdio_read_c45;
> +	bus->write_c45 = enetc_mdio_write_c45;
> +	bus->parent = dev;
> +	mdio_priv = bus->priv;
> +	mdio_priv->hw = &pf->si->hw;
> +	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +	err = of_mdiobus_register(bus, np);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> +	pf->mdio = bus;
> +
> +	return 0;
> +}
> +
> +static void enetc_mdio_remove(struct enetc_pf *pf)
> +{
> +	if (pf->mdio)
> +		mdiobus_unregister(pf->mdio);
> +}
> +
> +static bool enetc_port_has_pcs(struct enetc_pf *pf)
> +{
> +	return (pf->if_mode == PHY_INTERFACE_MODE_SGMII ||
> +		pf->if_mode == PHY_INTERFACE_MODE_1000BASEX ||
> +		pf->if_mode == PHY_INTERFACE_MODE_2500BASEX ||
> +		pf->if_mode == PHY_INTERFACE_MODE_USXGMII);
> +}
> +
> +static int enetc_imdio_create(struct enetc_pf *pf)
> +{
> +	struct device *dev = &pf->si->pdev->dev;
> +	struct enetc_mdio_priv *mdio_priv;
> +	struct phylink_pcs *phylink_pcs;
> +	struct mii_bus *bus;
> +	int err;
> +
> +	if (!pf->ops->create_pcs)
> +		return -EOPNOTSUPP;
> +
> +	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Freescale ENETC internal MDIO Bus";
> +	bus->read = enetc_mdio_read_c22;
> +	bus->write = enetc_mdio_write_c22;
> +	bus->read_c45 = enetc_mdio_read_c45;
> +	bus->write_c45 = enetc_mdio_write_c45;
> +	bus->parent = dev;
> +	bus->phy_mask = ~0;
> +	mdio_priv = bus->priv;
> +	mdio_priv->hw = &pf->si->hw;
> +	mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
> +
> +	err = mdiobus_register(bus);
> +	if (err) {
> +		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);
> +		goto free_mdio_bus;
> +	}
> +
> +	phylink_pcs = pf->ops->create_pcs(pf, bus);
> +	if (IS_ERR(phylink_pcs)) {
> +		err = PTR_ERR(phylink_pcs);
> +		dev_err(dev, "cannot create pcs (%d)\n", err);
> +		goto unregister_mdiobus;
> +	}
> +
> +	pf->imdio = bus;
> +	pf->pcs = phylink_pcs;
> +
> +	return 0;
> +
> +unregister_mdiobus:
> +	mdiobus_unregister(bus);
> +free_mdio_bus:
> +	mdiobus_free(bus);
> +	return err;
> +}
> +
> +static void enetc_imdio_remove(struct enetc_pf *pf)
> +{
> +	if (pf->pcs && pf->ops->destroy_pcs)
> +		pf->ops->destroy_pcs(pf->pcs);
> +
> +	if (pf->imdio) {
> +		mdiobus_unregister(pf->imdio);
> +		mdiobus_free(pf->imdio);
> +	}
> +}
> +
> +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node)
> +{
> +	struct device_node *mdio_np;
> +	int err;
> +
> +	mdio_np = of_get_child_by_name(node, "mdio");
> +	if (mdio_np) {
> +		err = enetc_mdio_probe(pf, mdio_np);
> +
> +		of_node_put(mdio_np);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (enetc_port_has_pcs(pf)) {
> +		err = enetc_imdio_create(pf);
> +		if (err) {
> +			enetc_mdio_remove(pf);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_mdiobus_create);
> +
> +void enetc_mdiobus_destroy(struct enetc_pf *pf)
> +{
> +	enetc_mdio_remove(pf);
> +	enetc_imdio_remove(pf);
> +}
> +EXPORT_SYMBOL_GPL(enetc_mdiobus_destroy);
> +
> +int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node,
> +			 const struct phylink_mac_ops *pl_mac_ops)
> +{
> +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> +	struct phylink *phylink;
> +	int err;
> +
> +	pf->phylink_config.dev = &priv->ndev->dev;
> +	pf->phylink_config.type = PHYLINK_NETDEV;
> +	pf->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
> +
> +	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +		  pf->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> +		  pf->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +		  pf->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +		  pf->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> +		  pf->phylink_config.supported_interfaces);
> +	phy_interface_set_rgmii(pf->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&pf->phylink_config, of_fwnode_handle(node),
> +				 pf->if_mode, pl_mac_ops);
> +	if (IS_ERR(phylink)) {
> +		err = PTR_ERR(phylink);
> +		return err;
> +	}
> +
> +	priv->phylink = phylink;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_phylink_create);
> +
> +void enetc_phylink_destroy(struct enetc_ndev_priv *priv)
> +{
> +	phylink_destroy(priv->phylink);
> +}
> +EXPORT_SYMBOL_GPL(enetc_phylink_destroy);
> +
> +int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs)
> +{
> +	struct enetc_si *si = pci_get_drvdata(pdev);
> +	struct enetc_pf *pf = enetc_si_priv(si);
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_PCI_IOV))
> +		return 0;
> +
> +	if (!num_vfs) {
> +		pci_disable_sriov(pdev);
> +		enetc_msg_psi_free(pf);
> +		kfree(pf->vf_state);
> +		pf->num_vfs = 0;
> +	} else {
> +		pf->num_vfs = num_vfs;
> +
> +		pf->vf_state = kcalloc(num_vfs, sizeof(struct enetc_vf_state),
> +				       GFP_KERNEL);
> +		if (!pf->vf_state) {
> +			pf->num_vfs = 0;
> +			return -ENOMEM;
> +		}
> +
> +		err = enetc_msg_psi_init(pf);
> +		if (err) {
> +			dev_err(&pdev->dev, "enetc_msg_psi_init (%d)\n", err);
> +			goto err_msg_psi;
> +		}
> +
> +		err = pci_enable_sriov(pdev, num_vfs);
> +		if (err) {
> +			dev_err(&pdev->dev, "pci_enable_sriov err %d\n", err);
> +			goto err_en_sriov;
> +		}
> +	}
> +
> +	return num_vfs;
> +
> +err_en_sriov:
> +	enetc_msg_psi_free(pf);
> +err_msg_psi:
> +	kfree(pf->vf_state);
> +	pf->num_vfs = 0;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(enetc_sriov_configure);
> +
> +MODULE_DESCRIPTION("NXP ENETC PF common functionality driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> --
> 2.34.1
>
Frank Li Oct. 9, 2024, 5:25 p.m. UTC | #3
On Wed, Oct 09, 2024 at 05:51:14PM +0800, Wei Fang wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
>
> There is a situation where num_tx_rings cannot be divided by
> bdr_int_num. For example, num_tx_rings is 8 and bdr_int_num
> is 3. According to the previous logic, this results in two
> tx_bdr corresponding memories not being allocated, so when
> sending packets to tx BD ring 6 or 7, wild pointers will be
> accessed. Of course, this issue does not exist for LS1028A,
> because its num_tx_rings is 8, and bdr_int_num is either 1
> or 2. So there is no situation where it cannot be divided.
> However, there is a risk for the upcoming i.MX95, so the
> allocation of tx_bdr is optimized to ensure that each tx_bdr
> can be allocated to the corresponding memory.
>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 121 ++++++++++---------
>  1 file changed, 62 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 032d8eadd003..b84c88a76762 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -2965,13 +2965,70 @@ int enetc_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>  }
>  EXPORT_SYMBOL_GPL(enetc_ioctl);
>
> +static int enetc_bdr_init(struct enetc_ndev_priv *priv, int i, int v_tx_rings)
> +{
> +	struct enetc_int_vector *v __free(kfree);
> +	struct enetc_bdr *bdr;
> +	int j, err;
> +
> +	v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> +	if (!v)
> +		return -ENOMEM;
> +
> +	bdr = &v->rx_ring;
> +	bdr->index = i;
> +	bdr->ndev = priv->ndev;
> +	bdr->dev = priv->dev;
> +	bdr->bd_count = priv->rx_bd_count;
> +	bdr->buffer_offset = ENETC_RXB_PAD;
> +	priv->rx_ring[i] = bdr;
> +
> +	err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0);
> +	if (err)
> +		return err;
> +
> +	err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq,
> +					 MEM_TYPE_PAGE_SHARED, NULL);
> +	if (err) {
> +		xdp_rxq_info_unreg(&bdr->xdp.rxq);
> +		return err;
> +	}
> +
> +	/* init defaults for adaptive IC */
> +	if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
> +		v->rx_ictt = 0x1;
> +		v->rx_dim_en = true;
> +	}
> +	INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
> +	netif_napi_add(priv->ndev, &v->napi, enetc_poll);
> +	v->count_tx_rings = v_tx_rings;
> +
> +	for (j = 0; j < v_tx_rings; j++) {
> +		int idx;
> +
> +		/* default tx ring mapping policy */
> +		idx = priv->bdr_int_num * j + i;
> +		__set_bit(idx, &v->tx_rings_map);
> +		bdr = &v->tx_ring[j];
> +		bdr->index = idx;
> +		bdr->ndev = priv->ndev;
> +		bdr->dev = priv->dev;
> +		bdr->bd_count = priv->tx_bd_count;
> +		priv->tx_ring[idx] = bdr;
> +	}
> +
> +	priv->int_vector[i] = no_free_ptr(v);
> +
> +	return 0;
> +}
> +
>  int enetc_alloc_msix(struct enetc_ndev_priv *priv)
>  {
>  	struct pci_dev *pdev = priv->si->pdev;
> +	int v_tx_rings, v_remainder;
>  	int num_stack_tx_queues;
>  	int first_xdp_tx_ring;
>  	int i, n, err, nvec;
> -	int v_tx_rings;
>
>  	nvec = ENETC_BDR_INT_BASE_IDX + priv->bdr_int_num;
>  	/* allocate MSIX for both messaging and Rx/Tx interrupts */
> @@ -2985,65 +3042,11 @@ int enetc_alloc_msix(struct enetc_ndev_priv *priv)
>
>  	/* # of tx rings per int vector */
>  	v_tx_rings = priv->num_tx_rings / priv->bdr_int_num;
> +	v_remainder = priv->num_tx_rings % priv->bdr_int_num;
>
> -	for (i = 0; i < priv->bdr_int_num; i++) {
> -		struct enetc_int_vector *v;
> -		struct enetc_bdr *bdr;
> -		int j;
> -
> -		v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> -		if (!v) {
> -			err = -ENOMEM;
> -			goto fail;
> -		}
> -
> -		priv->int_vector[i] = v;
> -
> -		bdr = &v->rx_ring;
> -		bdr->index = i;
> -		bdr->ndev = priv->ndev;
> -		bdr->dev = priv->dev;
> -		bdr->bd_count = priv->rx_bd_count;
> -		bdr->buffer_offset = ENETC_RXB_PAD;
> -		priv->rx_ring[i] = bdr;
> -
> -		err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0);
> -		if (err) {
> -			kfree(v);
> -			goto fail;
> -		}
> -
> -		err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq,
> -						 MEM_TYPE_PAGE_SHARED, NULL);
> -		if (err) {
> -			xdp_rxq_info_unreg(&bdr->xdp.rxq);
> -			kfree(v);
> -			goto fail;
> -		}
> -
> -		/* init defaults for adaptive IC */
> -		if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
> -			v->rx_ictt = 0x1;
> -			v->rx_dim_en = true;
> -		}
> -		INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
> -		netif_napi_add(priv->ndev, &v->napi, enetc_poll);
> -		v->count_tx_rings = v_tx_rings;
> -
> -		for (j = 0; j < v_tx_rings; j++) {
> -			int idx;
> -
> -			/* default tx ring mapping policy */
> -			idx = priv->bdr_int_num * j + i;
> -			__set_bit(idx, &v->tx_rings_map);
> -			bdr = &v->tx_ring[j];
> -			bdr->index = idx;
> -			bdr->ndev = priv->ndev;
> -			bdr->dev = priv->dev;
> -			bdr->bd_count = priv->tx_bd_count;
> -			priv->tx_ring[idx] = bdr;
> -		}
> -	}
> +	for (i = 0; i < priv->bdr_int_num; i++)
> +		enetc_bdr_init(priv, i,
> +			       i < v_remainder ? v_tx_rings + 1 : v_tx_rings);

suggest you create two patches, one just move to help function to
enetc_bdr_init(), the another is for real fixes.

>
>  	num_stack_tx_queues = enetc_num_stack_tx_queues(priv);
>
> --
> 2.34.1
>
Frank Li Oct. 9, 2024, 5:41 p.m. UTC | #4
On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> different from the LS1028A ENETC (revision 1.0) except for the SI
> part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
            So add new nxp-enetc4 driver for i.MX95 ENETC PF with
major revision 4.

> PF, and this driver will be used to support the ENETC PF with major
> revision 4 in the future.
>
> Currently, the nxp-enetc4 driver only supports basic transmission
> feature for i.MX95 ENETC PF, the more basic and advanced features
> will be added in the subsequent patches.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/Kconfig  |  17 +
>  drivers/net/ethernet/freescale/enetc/Makefile |   3 +
>  drivers/net/ethernet/freescale/enetc/enetc.c  |  50 +-
>  drivers/net/ethernet/freescale/enetc/enetc.h  |  19 +-
>  .../net/ethernet/freescale/enetc/enetc4_hw.h  | 153 ++++
>  .../net/ethernet/freescale/enetc/enetc4_pf.c  | 761 ++++++++++++++++++
>  .../ethernet/freescale/enetc/enetc_ethtool.c  |  68 +-
>  .../net/ethernet/freescale/enetc/enetc_hw.h   |  12 +-
>  .../net/ethernet/freescale/enetc/enetc_pf.h   |   9 +
>  .../freescale/enetc/enetc_pf_common.c         |  10 +-
>  10 files changed, 1080 insertions(+), 22 deletions(-)
>  create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_hw.h
>  create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_pf.c
>
> diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
> index 6f3306f14060..357c42efc4fb 100644
> --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> @@ -33,6 +33,23 @@ config FSL_ENETC
>
>  	  If compiled as module (M), the module name is fsl-enetc.
>
> +config NXP_ENETC4
> +	tristate "ENETC4 PF driver"
> +	depends on PCI_MSI
> +	select MDIO_DEVRES
> +	select NXP_ENETC_PF_COMMON
> +	select FSL_ENETC_CORE
> +	select FSL_ENETC_MDIO
> +	select PHYLINK
> +	select DIMLIB
> +	help
> +	  This driver supports NXP ENETC devices with major revision 4. ENETC is
> +	  as the NIC functionality in NETC, it supports virtualization/isolation
> +	  based on PCIe Single Root IO Virtualization (SR-IOV) and a full range
> +	  of TSN standards and NIC offload capabilities.
> +
> +	  If compiled as module (M), the module name is nxp-enetc4.
> +
>  config FSL_ENETC_VF
>  	tristate "ENETC VF driver"
>  	depends on PCI_MSI
> diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
> index b81ca462e358..5417815957b5 100644
> --- a/drivers/net/ethernet/freescale/enetc/Makefile
> +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> @@ -11,6 +11,9 @@ fsl-enetc-y := enetc_pf.o
>  fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
>  fsl-enetc-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o
>
> +obj-$(CONFIG_NXP_ENETC4) += nxp-enetc4.o
> +nxp-enetc4-y := enetc4_pf.o
> +
>  obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o
>  fsl-enetc-vf-y := enetc_vf.o
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index b84c88a76762..4e672089e260 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -2,6 +2,7 @@
>  /* Copyright 2017-2019 NXP */
>
>  #include "enetc.h"
> +#include <linux/clk.h>
>  #include <linux/bpf_trace.h>
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
> @@ -21,7 +22,7 @@ void enetc_port_mac_wr(struct enetc_si *si, u32 reg, u32 val)
>  {
>  	enetc_port_wr(&si->hw, reg, val);
>  	if (si->hw_features & ENETC_SI_F_QBU)
> -		enetc_port_wr(&si->hw, reg + ENETC_PMAC_OFFSET, val);
> +		enetc_port_wr(&si->hw, reg + si->pmac_offset, val);
>  }
>  EXPORT_SYMBOL_GPL(enetc_port_mac_wr);
>
> @@ -700,8 +701,10 @@ static void enetc_rx_dim_work(struct work_struct *w)
>  		net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
>  	struct enetc_int_vector	*v =
>  		container_of(dim, struct enetc_int_vector, rx_dim);
> +	struct enetc_ndev_priv *priv = netdev_priv(v->rx_ring.ndev);
> +	u64 clk_freq = priv->si->clk_freq;
>
> -	v->rx_ictt = enetc_usecs_to_cycles(moder.usec);
> +	v->rx_ictt = enetc_usecs_to_cycles(moder.usec, clk_freq);
>  	dim->state = DIM_START_MEASURE;
>  }
>
> @@ -1721,14 +1724,25 @@ void enetc_get_si_caps(struct enetc_si *si)
>  	struct enetc_hw *hw = &si->hw;
>  	u32 val;
>
> +	if (is_enetc_rev1(si))
> +		si->clk_freq = ENETC_CLK;
> +	else
> +		si->clk_freq = ENETC_CLK_333M;
> +
>  	/* find out how many of various resources we have to work with */
>  	val = enetc_rd(hw, ENETC_SICAPR0);
>  	si->num_rx_rings = (val >> 16) & 0xff;
>  	si->num_tx_rings = val & 0xff;
>
> -	val = enetc_rd(hw, ENETC_SIRFSCAPR);
> -	si->num_fs_entries = ENETC_SIRFSCAPR_GET_NUM_RFS(val);
> -	si->num_fs_entries = min(si->num_fs_entries, ENETC_MAX_RFS_SIZE);
> +	val = enetc_rd(hw, ENETC_SIPCAPR0);
> +	if (val & ENETC_SIPCAPR0_RFS) {
> +		val = enetc_rd(hw, ENETC_SIRFSCAPR);
> +		si->num_fs_entries = ENETC_SIRFSCAPR_GET_NUM_RFS(val);
> +		si->num_fs_entries = min(si->num_fs_entries, ENETC_MAX_RFS_SIZE);
> +	} else {
> +		/* ENETC which not supports RFS */
> +		si->num_fs_entries = 0;
> +	}
>
>  	si->num_rss = 0;
>  	val = enetc_rd(hw, ENETC_SIPCAPR0);
> @@ -1742,8 +1756,11 @@ void enetc_get_si_caps(struct enetc_si *si)
>  	if (val & ENETC_SIPCAPR0_QBV)
>  		si->hw_features |= ENETC_SI_F_QBV;
>
> -	if (val & ENETC_SIPCAPR0_QBU)
> +	if (val & ENETC_SIPCAPR0_QBU) {
>  		si->hw_features |= ENETC_SI_F_QBU;
> +		si->pmac_offset = is_enetc_rev1(si) ? ENETC_PMAC_OFFSET :
> +						      ENETC4_PMAC_OFFSET;
> +	}
>
>  	if (val & ENETC_SIPCAPR0_PSFP)
>  		si->hw_features |= ENETC_SI_F_PSFP;
> @@ -2056,7 +2073,7 @@ int enetc_configure_si(struct enetc_ndev_priv *priv)
>  	/* enable SI */
>  	enetc_wr(hw, ENETC_SIMR, ENETC_SIMR_EN);
>
> -	if (si->num_rss) {
> +	if (si->num_rss && is_enetc_rev1(si)) {
>  		err = enetc_setup_default_rss_table(si, priv->num_rx_rings);
>  		if (err)
>  			return err;
> @@ -2080,9 +2097,15 @@ void enetc_init_si_rings_params(struct enetc_ndev_priv *priv)
>  	 */
>  	priv->num_rx_rings = min_t(int, cpus, si->num_rx_rings);
>  	priv->num_tx_rings = si->num_tx_rings;
> -	priv->bdr_int_num = cpus;
> +	if (is_enetc_rev1(si)) {
> +		priv->bdr_int_num = cpus;
> +		priv->tx_ictt = enetc_usecs_to_cycles(600, ENETC_CLK);
> +	} else {
> +		priv->bdr_int_num = priv->num_rx_rings;
> +		priv->tx_ictt = enetc_usecs_to_cycles(500, ENETC_CLK_333M);
> +	}
> +
>  	priv->ic_mode = ENETC_IC_RX_ADAPTIVE | ENETC_IC_TX_MANUAL;
> -	priv->tx_ictt = ENETC_TXIC_TIMETHR;
>  }
>  EXPORT_SYMBOL_GPL(enetc_init_si_rings_params);
>
> @@ -2475,10 +2498,14 @@ int enetc_open(struct net_device *ndev)
>
>  	extended = !!(priv->active_offloads & ENETC_F_RX_TSTAMP);
>
> -	err = enetc_setup_irqs(priv);
> +	err = clk_prepare_enable(priv->ref_clk);
>  	if (err)
>  		return err;
>
> +	err = enetc_setup_irqs(priv);
> +	if (err)
> +		goto err_setup_irqs;
> +
>  	err = enetc_phylink_connect(ndev);
>  	if (err)
>  		goto err_phy_connect;
> @@ -2510,6 +2537,8 @@ int enetc_open(struct net_device *ndev)
>  		phylink_disconnect_phy(priv->phylink);
>  err_phy_connect:
>  	enetc_free_irqs(priv);
> +err_setup_irqs:
> +	clk_disable_unprepare(priv->ref_clk);
>
>  	return err;
>  }
> @@ -2559,6 +2588,7 @@ int enetc_close(struct net_device *ndev)
>  	enetc_assign_tx_resources(priv, NULL);
>
>  	enetc_free_irqs(priv);
> +	clk_disable_unprepare(priv->ref_clk);
>
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index 97524dfa234c..7f1ea11c33a0 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -14,6 +14,7 @@
>  #include <net/xdp.h>
>
>  #include "enetc_hw.h"
> +#include "enetc4_hw.h"
>
>  #define ENETC_MAC_MAXFRM_SIZE	9600
>  #define ENETC_MAX_MTU		(ENETC_MAC_MAXFRM_SIZE - \
> @@ -222,6 +223,7 @@ struct enetc_msg_swbd {
>  };
>
>  #define ENETC_REV1	0x1
> +#define ENETC_REV4	0x4
>  enum enetc_errata {
>  	ENETC_ERR_VLAN_ISOL	= BIT(0),
>  	ENETC_ERR_UCMCSWP	= BIT(1),
> @@ -247,10 +249,22 @@ struct enetc_si {
>  	int num_rss; /* number of RSS buckets */
>  	unsigned short pad;
>  	int hw_features;
> +	int pmac_offset; /* Only valid for PSI which supports 802.1Qbu */
> +	u64 clk_freq;
>  };
>
>  #define ENETC_SI_ALIGN	32
>
> +static inline bool is_enetc_rev1(struct enetc_si *si)
> +{
> +	return si->pdev->revision == ENETC_REV1;
> +}
> +
> +static inline bool is_enetc_rev4(struct enetc_si *si)
> +{
> +	return si->pdev->revision == ENETC_REV4;
> +}
> +

Actually, I suggest you check features, instead of check version number.

>  static inline void *enetc_si_priv(const struct enetc_si *si)
>  {
>  	return (char *)si + ALIGN(sizeof(struct enetc_si), ENETC_SI_ALIGN);
> @@ -302,7 +316,7 @@ struct enetc_cls_rule {
>  	int used;
>  };
>
> -#define ENETC_MAX_BDR_INT	2 /* fixed to max # of available cpus */
> +#define ENETC_MAX_BDR_INT	6 /* fixed to max # of available cpus */
>  struct psfp_cap {
>  	u32 max_streamid;
>  	u32 max_psfp_filter;
> @@ -340,7 +354,6 @@ enum enetc_ic_mode {
>
>  #define ENETC_RXIC_PKTTHR	min_t(u32, 256, ENETC_RX_RING_DEFAULT_SIZE / 2)
>  #define ENETC_TXIC_PKTTHR	min_t(u32, 128, ENETC_TX_RING_DEFAULT_SIZE / 2)
> -#define ENETC_TXIC_TIMETHR	enetc_usecs_to_cycles(600)
>
>  struct enetc_ndev_priv {
>  	struct net_device *ndev;
> @@ -388,6 +401,8 @@ struct enetc_ndev_priv {
>  	 * and link state updates
>  	 */
>  	struct mutex		mm_lock;
> +
> +	struct clk *ref_clk; /* RGMII/RMII reference clock */
>  };
>
>  /* Messaging */
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> new file mode 100644
> index 000000000000..0b2a35189e9d
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/*
> + * This header file defines the register offsets and bit fields
> + * of ENETC4 PF and VFs. Note that the same registers as ENETC
> + * version 1.0 are defined in the enetc_hw.h file.
> + *
> + * Copyright 2024 NXP
> + */
> +#ifndef __ENETC4_HW_H_
> +#define __ENETC4_HW_H_
> +
> +/***************************ENETC port registers**************************/
> +#define ENETC4_ECAPR0			0x0
> +#define  ECAPR0_RFS			BIT(2)
> +#define  ECAPR0_TSD			BIT(5)
> +#define  ECAPR0_RSS			BIT(8)
> +#define  ECAPR0_RSC			BIT(9)
> +#define  ECAPR0_LSO			BIT(10)
> +#define  ECAPR0_WO			BIT(13)
> +
> +#define ENETC4_ECAPR1			0x4
> +#define  ECAPR1_NUM_TCS			GENMASK(6, 4)
> +#define  ECAPR1_NUM_MCH			GENMASK(9, 8)
> +#define  ECAPR1_NUM_UCH			GENMASK(11, 10)
> +#define  ECAPR1_NUM_MSIX		GENMASK(22, 12)
> +#define  ECAPR1_NUM_VSI			GENMASK(27, 24)
> +#define  ECAPR1_NUM_IPV			BIT(31)
> +
> +#define ENETC4_ECAPR2			0x8
> +#define  ECAPR2_NUM_TX_BDR		GENMASK(9, 0)
> +#define  ECAPR2_NUM_RX_BDR		GENMASK(25, 16)
> +
> +#define ENETC4_PMR			0x10
> +#define  PMR_SI_EN(a)			BIT((16 + (a)))
> +
> +/* Port Pause ON/OFF threshold register */
> +#define ENETC4_PPAUONTR			0x108
> +#define ENETC4_PPAUOFFTR		0x10c
> +
> +/* Port Station interface promiscuous MAC mode register */
> +#define ENETC4_PSIPMMR			0x200
> +#define  PSIPMMR_SI0_MAC_UP		BIT(0)
> +#define  PSIPMMR_SI_MAC_UP		GENMASK(2, 0)
> +#define  PSIPMMR_SI0_MAC_MP		BIT(16)
> +#define  PSIPMMR_SI_MAC_MP		GENMASK(18, 16)
> +
> +/* Port Station interface promiscuous VLAN mode register */
> +#define ENETC4_PSIPVMR			0x204
> +
> +/* Port RSS key register n. n = 0,1,2,...,9 */
> +#define ENETC4_PRSSKR(n)		((n) * 0x4 + 0x250)
> +
> +/* Port station interface MAC address filtering capability register */
> +#define ENETC4_PSIMAFCAPR		0x280
> +#define  PSIMAFCAPR_NUM_MAC_AFTE	GENMASK(11, 0)
> +
> +/* Port station interface VLAN filtering capability register */
> +#define ENETC4_PSIVLANFCAPR		0x2c0
> +#define  PSIVLANFCAPR_NUM_VLAN_FTE	GENMASK(11, 0)
> +
> +/* Port station interface VLAN filtering mode register */
> +#define ENETC4_PSIVLANFMR		0x2c4
> +#define  PSIVLANFMR_VS			BIT(0)
> +
> +/* Port Station interface a primary MAC address registers */
> +#define ENETC4_PSIPMAR0(a)		((a) * 0x80 + 0x2000)
> +#define ENETC4_PSIPMAR1(a)		((a) * 0x80 + 0x2004)
> +
> +/* Port station interface a configuration register 0/2 */
> +#define ENETC4_PSICFGR0(a)		((a) * 0x80 + 0x2010)
> +#define  PSICFGR0_VASE			BIT(13)
> +#define  PSICFGR0_ASE			BIT(15)
> +#define  PSICFGR0_ANTI_SPOOFING		(PSICFGR0_VASE | PSICFGR0_ASE)
> +
> +#define ENETC4_PSICFGR2(a)		((a) * 0x80 + 0x2018)
> +
> +#define ENETC4_PMCAPR			0x4004
> +#define  PMCAPR_HD			BIT(8)
> +#define  PMCAPR_FP			GENMASK(10, 9)
> +
> +/* Port configuration register */
> +#define ENETC4_PCR			0x4010
> +#define  PCR_HDR_FMT			BIT(0)
> +#define  PCR_L2DOSE			BIT(4)
> +#define  PCR_TIMER_CS			BIT(8)
> +#define  PCR_PSPEED			GENMASK(29, 16)
> +#define  PCR_PSPEED_VAL(speed)		(((speed) / 10 - 1) << 16)
> +
> +/* Port MAC address register 0/1 */
> +#define ENETC4_PMAR0			0x4020
> +#define ENETC4_PMAR1			0x4024
> +
> +/* Port operational register */
> +#define ENETC4_POR			0x4100
> +
> +/* Port traffic class a transmit maximum SDU register */
> +#define ENETC4_PTCTMSDUR(a)		((a) * 0x20 + 0x4208)
> +#define  PTCTMSDUR_MAXSDU		GENMASK(15, 0)
> +#define  PTCTMSDUR_SDU_TYPE		GENMASK(17, 16)
> +#define   SDU_TYPE_PPDU			0
> +#define   SDU_TYPE_MPDU			1
> +#define   SDU_TYPE_MSDU			2
> +
> +#define ENETC4_PMAC_OFFSET		0x400
> +#define ENETC4_PM_CMD_CFG(mac)		(0x5008 + (mac) * 0x400)
> +#define  PM_CMD_CFG_TX_EN		BIT(0)
> +#define  PM_CMD_CFG_RX_EN		BIT(1)
> +#define  PM_CMD_CFG_PAUSE_FWD		BIT(7)
> +#define  PM_CMD_CFG_PAUSE_IGN		BIT(8)
> +#define  PM_CMD_CFG_TX_ADDR_INS		BIT(9)
> +#define  PM_CMD_CFG_LOOP_EN		BIT(10)
> +#define  PM_CMD_CFG_LPBK_MODE		GENMASK(12, 11)
> +#define   LPBCK_MODE_EXT_TX_CLK		0
> +#define   LPBCK_MODE_MAC_LEVEL		1
> +#define   LPBCK_MODE_INT_TX_CLK		2
> +#define  PM_CMD_CFG_CNT_FRM_EN		BIT(13)
> +#define  PM_CMD_CFG_TXP			BIT(15)
> +#define  PM_CMD_CFG_SEND_IDLE		BIT(16)
> +#define  PM_CMD_CFG_HD_FCEN		BIT(18)
> +#define  PM_CMD_CFG_SFD			BIT(21)
> +#define  PM_CMD_CFG_TX_FLUSH		BIT(22)
> +#define  PM_CMD_CFG_TX_LOWP_EN		BIT(23)
> +#define  PM_CMD_CFG_RX_LOWP_EMPTY	BIT(24)
> +#define  PM_CMD_CFG_SWR			BIT(26)
> +#define  PM_CMD_CFG_TS_MODE		BIT(30)
> +#define  PM_CMD_CFG_MG			BIT(31)
> +
> +/* Port MAC 0/1 Maximum Frame Length Register */
> +#define ENETC4_PM_MAXFRM(mac)		(0x5014 + (mac) * 0x400)
> +
> +/* Port MAC 0/1 Pause Quanta Register */
> +#define ENETC4_PM_PAUSE_QUANTA(mac)	(0x5054 + (mac) * 0x400)
> +
> +/* Port MAC 0/1 Pause Quanta Threshold Register */
> +#define ENETC4_PM_PAUSE_THRESH(mac)	(0x5064 + (mac) * 0x400)
> +
> +/* Port MAC 0 Interface Mode Control Register */
> +#define ENETC4_PM_IF_MODE(mac)		(0x5300 + (mac) * 0x400)
> +#define  PM_IF_MODE_IFMODE		GENMASK(2, 0)
> +#define   IFMODE_XGMII			0
> +#define   IFMODE_RMII			3
> +#define   IFMODE_RGMII			4
> +#define   IFMODE_SGMII			5
> +#define  PM_IF_MODE_REVMII		BIT(3)
> +#define  PM_IF_MODE_M10			BIT(4)
> +#define  PM_IF_MODE_HD			BIT(6)
> +#define  PM_IF_MODE_SSP			GENMASK(14, 13)
> +#define   SSP_100M			0
> +#define   SSP_10M			1
> +#define   SSP_1G			2
> +#define  PM_IF_MODE_ENA			BIT(15)
> +
> +#endif
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> new file mode 100644
> index 000000000000..e38ade76260b
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> @@ -0,0 +1,761 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Copyright 2024 NXP */
> +#include <linux/unaligned.h>
> +#include <linux/module.h>
> +#include <linux/of_net.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/fsl/netc_global.h>

sort headers.

> +
> +#include "enetc_pf.h"
> +
> +#define ENETC_SI_MAX_RING_NUM	8
> +
> +static void enetc4_get_port_caps(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +	u32 val;
> +
> +	val = enetc_port_rd(hw, ENETC4_ECAPR1);
> +	pf->caps.num_vsi = (val & ECAPR1_NUM_VSI) >> 24;
> +	pf->caps.num_msix = ((val & ECAPR1_NUM_MSIX) >> 12) + 1;
> +
> +	val = enetc_port_rd(hw, ENETC4_ECAPR2);
> +	pf->caps.num_rx_bdr = (val & ECAPR2_NUM_RX_BDR) >> 16;
> +	pf->caps.num_tx_bdr = val & ECAPR2_NUM_TX_BDR;
> +
> +	val = enetc_port_rd(hw, ENETC4_PMCAPR);
> +	pf->caps.half_duplex = (val & PMCAPR_HD) ? 1 : 0;
> +}
> +
> +static void enetc4_pf_set_si_primary_mac(struct enetc_hw *hw, int si,
> +					 const u8 *addr)
> +{
> +	u16 lower = get_unaligned_le16(addr + 4);
> +	u32 upper = get_unaligned_le32(addr);
> +
> +	if (si != 0) {
> +		__raw_writel(upper, hw->port + ENETC4_PSIPMAR0(si));
> +		__raw_writew(lower, hw->port + ENETC4_PSIPMAR1(si));
> +	} else {
> +		__raw_writel(upper, hw->port + ENETC4_PMAR0);
> +		__raw_writew(lower, hw->port + ENETC4_PMAR1);
> +	}
> +}
> +
> +static void enetc4_pf_get_si_primary_mac(struct enetc_hw *hw, int si,
> +					 u8 *addr)
> +{
> +	u32 upper;
> +	u16 lower;
> +
> +	upper = __raw_readl(hw->port + ENETC4_PSIPMAR0(si));
> +	lower = __raw_readw(hw->port + ENETC4_PSIPMAR1(si));
> +
> +	put_unaligned_le32(upper, addr);
> +	put_unaligned_le16(lower, addr + 4);
> +}
> +
> +static const struct enetc_pf_ops enetc4_pf_ops = {
> +	.set_si_primary_mac = enetc4_pf_set_si_primary_mac,
> +	.get_si_primary_mac = enetc4_pf_get_si_primary_mac,
> +};
> +
> +static int enetc4_pf_struct_init(struct enetc_si *si)
> +{
> +	struct enetc_pf *pf = enetc_si_priv(si);
> +
> +	pf->si = si;
> +	pf->total_vfs = pci_sriov_get_totalvfs(si->pdev);
> +
> +	enetc4_get_port_caps(pf);
> +	enetc_pf_ops_register(pf, &enetc4_pf_ops);
> +
> +	return 0;
> +}
> +
> +static u32 enetc4_psicfgr0_val_construct(bool is_vf, u32 num_tx_bdr, u32 num_rx_bdr)
> +{
> +	u32 val;
> +
> +	val = ENETC_PSICFGR0_SET_TXBDR(num_tx_bdr);
> +	val |= ENETC_PSICFGR0_SET_RXBDR(num_rx_bdr);
> +	val |= ENETC_PSICFGR0_SIVC(ENETC_VLAN_TYPE_C | ENETC_VLAN_TYPE_S);
> +
> +	if (is_vf)
> +		val |= ENETC_PSICFGR0_VTE | ENETC_PSICFGR0_SIVIE;
> +
> +	return val;
> +}
> +
> +static void enetc4_default_rings_allocation(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +	u32 num_rx_bdr, num_tx_bdr, val;
> +	u32 vf_tx_bdr, vf_rx_bdr;
> +	int i, rx_rem, tx_rem;
> +
> +	if (pf->caps.num_rx_bdr < ENETC_SI_MAX_RING_NUM + pf->caps.num_vsi)
> +		num_rx_bdr = pf->caps.num_rx_bdr - pf->caps.num_vsi;
> +	else
> +		num_rx_bdr = ENETC_SI_MAX_RING_NUM;
> +
> +	if (pf->caps.num_tx_bdr < ENETC_SI_MAX_RING_NUM + pf->caps.num_vsi)
> +		num_tx_bdr = pf->caps.num_tx_bdr - pf->caps.num_vsi;
> +	else
> +		num_tx_bdr = ENETC_SI_MAX_RING_NUM;
> +
> +	val = enetc4_psicfgr0_val_construct(false, num_tx_bdr, num_rx_bdr);
> +	enetc_port_wr(hw, ENETC4_PSICFGR0(0), val);
> +
> +	num_rx_bdr = pf->caps.num_rx_bdr - num_rx_bdr;
> +	rx_rem = num_rx_bdr % pf->caps.num_vsi;
> +	num_rx_bdr = num_rx_bdr / pf->caps.num_vsi;
> +
> +	num_tx_bdr = pf->caps.num_tx_bdr - num_tx_bdr;
> +	tx_rem = num_tx_bdr % pf->caps.num_vsi;
> +	num_tx_bdr = num_tx_bdr / pf->caps.num_vsi;
> +
> +	for (i = 0; i < pf->caps.num_vsi; i++) {
> +		vf_tx_bdr = (i < tx_rem) ? num_tx_bdr + 1 : num_tx_bdr;
> +		vf_rx_bdr = (i < rx_rem) ? num_rx_bdr + 1 : num_rx_bdr;
> +		val = enetc4_psicfgr0_val_construct(true, vf_tx_bdr, vf_rx_bdr);
> +		enetc_port_wr(hw, ENETC4_PSICFGR0(i + 1), val);
> +	}
> +}
> +
> +static void enetc4_allocate_si_rings(struct enetc_pf *pf)
> +{
> +	enetc4_default_rings_allocation(pf);
> +}
> +
> +static void enetc4_pf_set_si_vlan_promisc(struct enetc_hw *hw, int si, bool en)
> +{
> +	u32 val = enetc_port_rd(hw, ENETC4_PSIPVMR);
> +
> +	if (en)
> +		val |= BIT(si);
> +	else
> +		val &= ~BIT(si);
> +
> +	enetc_port_wr(hw, ENETC4_PSIPVMR, val);
> +}
> +
> +static void enetc4_set_default_si_vlan_promisc(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +	int num_si = pf->caps.num_vsi + 1;
> +	int i;
> +
> +	/* enforce VLAN promisc mode for all SIs */
> +	for (i = 0; i < num_si; i++)
> +		enetc4_pf_set_si_vlan_promisc(hw, i, true);
> +}
> +
> +/* Allocate the number of MSI-X vectors for per SI. */
> +static void enetc4_set_si_msix_num(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +	int i, num_msix, total_si;
> +	u32 val;
> +
> +	total_si = pf->caps.num_vsi + 1;
> +
> +	num_msix = pf->caps.num_msix / total_si +
> +		   pf->caps.num_msix % total_si - 1;
> +	val = num_msix & 0x3f;
> +	enetc_port_wr(hw, ENETC4_PSICFGR2(0), val);
> +
> +	num_msix = pf->caps.num_msix / total_si - 1;
> +	val = num_msix & 0x3f;
> +	for (i = 0; i < pf->caps.num_vsi; i++)
> +		enetc_port_wr(hw, ENETC4_PSICFGR2(i + 1), val);
> +}
> +
> +static void enetc4_enable_all_si(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +	int num_si = pf->caps.num_vsi + 1;
> +	u32 si_bitmap = 0;
> +	int i;
> +
> +	/* Master enable for all SIs */
> +	for (i = 0; i < num_si; i++)
> +		si_bitmap |= PMR_SI_EN(i);
> +
> +	enetc_port_wr(hw, ENETC4_PMR, si_bitmap);
> +}
> +
> +static void enetc4_configure_port_si(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +
> +	enetc4_allocate_si_rings(pf);
> +
> +	/* Outer VLAN tag will be used for VLAN filtering */
> +	enetc_port_wr(hw, ENETC4_PSIVLANFMR, PSIVLANFMR_VS);
> +
> +	enetc4_set_default_si_vlan_promisc(pf);
> +
> +	/* Disable SI MAC multicast & unicast promiscuous */
> +	enetc_port_wr(hw, ENETC4_PSIPMMR, 0);
> +
> +	enetc4_set_si_msix_num(pf);
> +
> +	enetc4_enable_all_si(pf);
> +}
> +
> +static void enetc4_pf_reset_tc_msdu(struct enetc_hw *hw)
> +{
> +	u32 val = ENETC_MAC_MAXFRM_SIZE;
> +	int tc;
> +
> +	val = u32_replace_bits(val, SDU_TYPE_MPDU, PTCTMSDUR_SDU_TYPE);
> +
> +	for (tc = 0; tc < 8; tc++)
> +		enetc_port_wr(hw, ENETC4_PTCTMSDUR(tc), val);
> +}
> +
> +static void enetc4_set_trx_frame_size(struct enetc_pf *pf)
> +{
> +	struct enetc_si *si = pf->si;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_MAXFRM(0),
> +			  ENETC_SET_MAXFRM(ENETC_MAC_MAXFRM_SIZE));
> +
> +	enetc4_pf_reset_tc_msdu(&si->hw);
> +}
> +
> +static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *bytes)
> +{
> +	int i;
> +
> +	for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++)
> +		enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)bytes)[i]);
> +}
> +
> +static void enetc4_set_default_rss_key(struct enetc_pf *pf)
> +{
> +	u8 hash_key[ENETC_RSSHASH_KEY_SIZE] = {0};
> +	struct enetc_hw *hw = &pf->si->hw;
> +
> +	/* set up hash key */
> +	get_random_bytes(hash_key, ENETC_RSSHASH_KEY_SIZE);
> +	enetc4_set_rss_key(hw, hash_key);
> +}
> +
> +static void enetc4_enable_trx(struct enetc_pf *pf)
> +{
> +	struct enetc_hw *hw = &pf->si->hw;
> +
> +	/* Enable port transmit/receive */
> +	enetc_port_wr(hw, ENETC4_POR, 0);
> +}
> +
> +static void enetc4_configure_port(struct enetc_pf *pf)
> +{
> +	enetc4_configure_port_si(pf);
> +	enetc4_set_trx_frame_size(pf);
> +	enetc4_set_default_rss_key(pf);
> +	enetc4_enable_trx(pf);
> +}
> +
> +static int enetc4_pf_init(struct enetc_pf *pf)
> +{
> +	struct device *dev = &pf->si->pdev->dev;
> +	int err;
> +
> +	/* Initialize the MAC address for PF and VFs */
> +	err = enetc_setup_mac_addresses(dev->of_node, pf);
> +	if (err) {
> +		dev_err(dev, "Failed to set MAC addresses\n");
> +		return err;
> +	}
> +
> +	enetc4_configure_port(pf);
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops enetc4_ndev_ops = {
> +	.ndo_open		= enetc_open,
> +	.ndo_stop		= enetc_close,
> +	.ndo_start_xmit		= enetc_xmit,
> +	.ndo_get_stats		= enetc_get_stats,
> +	.ndo_set_mac_address	= enetc_pf_set_mac_addr,
> +};
> +
> +static struct phylink_pcs *
> +enetc4_pl_mac_select_pcs(struct phylink_config *config, phy_interface_t iface)
> +{
> +	struct enetc_pf *pf = phylink_to_enetc_pf(config);
> +
> +	return pf->pcs;
> +}
> +
> +static void enetc4_mac_config(struct enetc_pf *pf, unsigned int mode,
> +			      phy_interface_t phy_mode)
> +{
> +	struct enetc_ndev_priv *priv = netdev_priv(pf->si->ndev);
> +	struct enetc_si *si = pf->si;
> +	u32 val;
> +
> +	val = enetc_port_mac_rd(si, ENETC4_PM_IF_MODE(0));
> +	val &= ~(PM_IF_MODE_IFMODE | PM_IF_MODE_ENA);
> +
> +	switch (phy_mode) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val |= IFMODE_RGMII;
> +		/* We need to enable auto-negotiation for the MAC
> +		 * if its RGMII interface support In-Band status.
> +		 */
> +		if (phylink_autoneg_inband(mode))
> +			val |= PM_IF_MODE_ENA;
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		val |= IFMODE_RMII;
> +		break;
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		val |= IFMODE_SGMII;
> +		break;
> +	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_XGMII:
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		val |= IFMODE_XGMII;
> +		break;
> +	default:
> +		dev_err(priv->dev,
> +			"Unsupported PHY mode:%d\n", phy_mode);
> +		return;
> +	}
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_IF_MODE(0), val);
> +}
> +
> +static void enetc4_pl_mac_config(struct phylink_config *config, unsigned int mode,
> +				 const struct phylink_link_state *state)
> +{
> +	struct enetc_pf *pf = phylink_to_enetc_pf(config);
> +
> +	enetc4_mac_config(pf, mode, state->interface);
> +}
> +
> +static void enetc4_set_port_speed(struct enetc_ndev_priv *priv, int speed)
> +{
> +	u32 old_speed = priv->speed;
> +	u32 val;
> +
> +	if (speed == old_speed)
> +		return;
> +
> +	val = enetc_port_rd(&priv->si->hw, ENETC4_PCR);
> +	val &= ~PCR_PSPEED;
> +
> +	switch (speed) {
> +	case SPEED_100:
> +	case SPEED_1000:
> +	case SPEED_2500:
> +	case SPEED_10000:
> +		val |= (PCR_PSPEED & PCR_PSPEED_VAL(speed));
> +		break;
> +	case SPEED_10:
> +	default:
> +		val |= (PCR_PSPEED & PCR_PSPEED_VAL(SPEED_10));
> +	}
> +
> +	priv->speed = speed;
> +	enetc_port_wr(&priv->si->hw, ENETC4_PCR, val);
> +}
> +
> +static void enetc4_set_rgmii_mac(struct enetc_pf *pf, int speed, int duplex)
> +{
> +	struct enetc_si *si = pf->si;
> +	u32 old_val, val;
> +
> +	old_val = enetc_port_mac_rd(si, ENETC4_PM_IF_MODE(0));
> +	val = old_val & ~(PM_IF_MODE_ENA | PM_IF_MODE_M10 | PM_IF_MODE_REVMII);
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		val = u32_replace_bits(val, SSP_1G, PM_IF_MODE_SSP);
> +		break;
> +	case SPEED_100:
> +		val = u32_replace_bits(val, SSP_100M, PM_IF_MODE_SSP);
> +		break;
> +	case SPEED_10:
> +		val = u32_replace_bits(val, SSP_10M, PM_IF_MODE_SSP);
> +	}
> +
> +	val = u32_replace_bits(val, duplex == DUPLEX_FULL ? 0 : 1,
> +			       PM_IF_MODE_HD);
> +
> +	if (val == old_val)
> +		return;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_IF_MODE(0), val);
> +}
> +
> +static void enetc4_set_rmii_mac(struct enetc_pf *pf, int speed, int duplex)
> +{
> +	struct enetc_si *si = pf->si;
> +	u32 old_val, val;
> +
> +	old_val = enetc_port_mac_rd(si, ENETC4_PM_IF_MODE(0));
> +	val = old_val & ~(PM_IF_MODE_ENA | PM_IF_MODE_SSP);
> +
> +	switch (speed) {
> +	case SPEED_100:
> +		val &= ~PM_IF_MODE_M10;
> +		break;
> +	case SPEED_10:
> +		val |= PM_IF_MODE_M10;
> +	}
> +
> +	val = u32_replace_bits(val, duplex == DUPLEX_FULL ? 0 : 1,
> +			       PM_IF_MODE_HD);
> +
> +	if (val == old_val)
> +		return;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_IF_MODE(0), val);
> +}
> +
> +static void enetc4_set_hd_flow_control(struct enetc_pf *pf, bool enable)
> +{
> +	struct enetc_si *si = pf->si;
> +	u32 old_val, val;
> +
> +	if (!pf->caps.half_duplex)
> +		return;
> +
> +	old_val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
> +	val = u32_replace_bits(old_val, enable ? 1 : 0, PM_CMD_CFG_HD_FCEN);
> +	if (val == old_val)
> +		return;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
> +}
> +
> +static void enetc4_set_rx_pause(struct enetc_pf *pf, bool rx_pause)
> +{
> +	struct enetc_si *si = pf->si;
> +	u32 old_val, val;
> +
> +	old_val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
> +	val = u32_replace_bits(old_val, rx_pause ? 0 : 1, PM_CMD_CFG_PAUSE_IGN);
> +	if (val == old_val)
> +		return;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
> +}
> +
> +static void enetc4_set_tx_pause(struct enetc_pf *pf, int num_rxbdr, bool tx_pause)
> +{
> +	u32 pause_off_thresh = 0, pause_on_thresh = 0;
> +	u32 init_quanta = 0, refresh_quanta = 0;
> +	struct enetc_hw *hw = &pf->si->hw;
> +	u32 rbmr, old_rbmr;
> +	int i;
> +
> +	for (i = 0; i < num_rxbdr; i++) {
> +		old_rbmr = enetc_rxbdr_rd(hw, i, ENETC_RBMR);
> +		rbmr = u32_replace_bits(old_rbmr, tx_pause ? 1 : 0, ENETC_RBMR_CM);
> +		if (rbmr == old_rbmr)
> +			continue;
> +
> +		enetc_rxbdr_wr(hw, i, ENETC_RBMR, rbmr);
> +	}
> +
> +	if (tx_pause) {
> +		/* When the port first enters congestion, send a PAUSE request
> +		 * with the maximum number of quanta. When the port exits
> +		 * congestion, it will automatically send a PAUSE frame with
> +		 * zero quanta.
> +		 */
> +		init_quanta = 0xffff;
> +
> +		/* Also, set up the refresh timer to send follow-up PAUSE
> +		 * frames at half the quanta value, in case the congestion
> +		 * condition persists.
> +		 */
> +		refresh_quanta = 0xffff / 2;
> +
> +		/* Start emitting PAUSE frames when 3 large frames (or more
> +		 * smaller frames) have accumulated in the FIFO waiting to be
> +		 * DMAed to the RX ring.
> +		 */
> +		pause_on_thresh = 3 * ENETC_MAC_MAXFRM_SIZE;
> +		pause_off_thresh = 1 * ENETC_MAC_MAXFRM_SIZE;
> +	}
> +
> +	enetc_port_mac_wr(pf->si, ENETC4_PM_PAUSE_QUANTA(0), init_quanta);
> +	enetc_port_mac_wr(pf->si, ENETC4_PM_PAUSE_THRESH(0), refresh_quanta);
> +	enetc_port_wr(hw, ENETC4_PPAUONTR, pause_on_thresh);
> +	enetc_port_wr(hw, ENETC4_PPAUOFFTR, pause_off_thresh);
> +}
> +
> +static void enetc4_enable_mac(struct enetc_pf *pf, bool en)
> +{
> +	struct enetc_si *si = pf->si;
> +	u32 val;
> +
> +	val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
> +	val &= ~(PM_CMD_CFG_TX_EN | PM_CMD_CFG_RX_EN);
> +	val |= en ? (PM_CMD_CFG_TX_EN | PM_CMD_CFG_RX_EN) : 0;
> +
> +	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
> +}
> +
> +static void enetc4_pl_mac_link_up(struct phylink_config *config,
> +				  struct phy_device *phy, unsigned int mode,
> +				  phy_interface_t interface, int speed,
> +				  int duplex, bool tx_pause, bool rx_pause)
> +{
> +	struct enetc_pf *pf = phylink_to_enetc_pf(config);
> +	struct enetc_si *si = pf->si;
> +	struct enetc_ndev_priv *priv;
> +	bool hd_fc = false;
> +
> +	priv = netdev_priv(si->ndev);
> +	enetc4_set_port_speed(priv, speed);
> +
> +	if (!phylink_autoneg_inband(mode) &&
> +	    phy_interface_mode_is_rgmii(interface))
> +		enetc4_set_rgmii_mac(pf, speed, duplex);
> +
> +	if (interface == PHY_INTERFACE_MODE_RMII)
> +		enetc4_set_rmii_mac(pf, speed, duplex);
> +
> +	if (duplex == DUPLEX_FULL) {
> +		/* When preemption is enabled, generation of PAUSE frames
> +		 * must be disabled, as stated in the IEEE 802.3 standard.
> +		 */
> +		if (priv->active_offloads & ENETC_F_QBU)
> +			tx_pause = false;
> +	} else { /* DUPLEX_HALF */
> +		if (tx_pause || rx_pause)
> +			hd_fc = true;
> +
> +		/* As per 802.3 annex 31B, PAUSE frames are only supported
> +		 * when the link is configured for full duplex operation.
> +		 */
> +		tx_pause = false;
> +		rx_pause = false;
> +	}
> +
> +	enetc4_set_hd_flow_control(pf, hd_fc);
> +	enetc4_set_tx_pause(pf, priv->num_rx_rings, tx_pause);
> +	enetc4_set_rx_pause(pf, rx_pause);
> +	enetc4_enable_mac(pf, true);
> +}
> +
> +static void enetc4_pl_mac_link_down(struct phylink_config *config,
> +				    unsigned int mode,
> +				    phy_interface_t interface)
> +{
> +	struct enetc_pf *pf = phylink_to_enetc_pf(config);
> +
> +	enetc4_enable_mac(pf, false);
> +}
> +
> +static const struct phylink_mac_ops enetc_pl_mac_ops = {
> +	.mac_select_pcs = enetc4_pl_mac_select_pcs,
> +	.mac_config = enetc4_pl_mac_config,
> +	.mac_link_up = enetc4_pl_mac_link_up,
> +	.mac_link_down = enetc4_pl_mac_link_down,
> +};
> +
> +static int enetc4_link_init(struct enetc_ndev_priv *priv,
> +			    struct device_node *node)
> +{
> +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> +	struct device *dev = priv->dev;
> +	int err;
> +
> +	err = of_get_phy_mode(node, &pf->if_mode);
> +	if (err) {
> +		dev_err(dev, "Failed to get PHY mode\n");
> +		return err;
> +	}
> +
> +	err = enetc_mdiobus_create(pf, node);
> +	if (err) {
> +		dev_err(dev, "Failed to create MDIO bus\n");
> +		return err;
> +	}
> +
> +	err = enetc_phylink_create(priv, node, &enetc_pl_mac_ops);
> +	if (err) {
> +		dev_err(dev, "Failed to create phylink\n");
> +		goto err_phylink_create;
> +	}
> +
> +	return 0;
> +
> +err_phylink_create:
> +	enetc_mdiobus_destroy(pf);
> +
> +	return err;
> +}
> +
> +static void enetc4_link_deinit(struct enetc_ndev_priv *priv)
> +{
> +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> +
> +	enetc_phylink_destroy(priv);
> +	enetc_mdiobus_destroy(pf);
> +}
> +
> +static int enetc4_pf_netdev_create(struct enetc_si *si)
> +{
> +	struct device *dev = &si->pdev->dev;
> +	struct enetc_ndev_priv *priv;
> +	struct net_device *ndev;
> +	int err;
> +
> +	ndev = alloc_etherdev_mqs(sizeof(struct enetc_ndev_priv),
> +				  si->num_tx_rings, si->num_rx_rings);
> +	if (!ndev)
> +		return  -ENOMEM;
> +
> +	priv = netdev_priv(ndev);
> +	priv->ref_clk = devm_clk_get_optional(dev, "enet_ref_clk");
> +	if (IS_ERR(priv->ref_clk)) {
> +		dev_err(dev, "Get enet_ref_clk failed\n");
> +		err = PTR_ERR(priv->ref_clk);
> +		goto err_clk_get;
> +	}
> +
> +	enetc_pf_netdev_setup(si, ndev, &enetc4_ndev_ops);
> +
> +	enetc_init_si_rings_params(priv);
> +
> +	err = enetc_configure_si(priv);
> +	if (err) {
> +		dev_err(dev, "Failed to configure SI\n");
> +		goto err_config_si;
> +	}
> +
> +	err = enetc_alloc_msix(priv);
> +	if (err) {
> +		dev_err(dev, "Failed to alloc MSI-X\n");
> +		goto err_alloc_msix;
> +	}
> +
> +	err = enetc4_link_init(priv, dev->of_node);
> +	if (err)
> +		goto err_link_init;
> +
> +	err = register_netdev(ndev);
> +	if (err) {
> +		dev_err(dev, "Failed to register netdev\n");
> +		goto err_reg_netdev;
> +	}
> +
> +	return 0;
> +
> +err_reg_netdev:
> +	enetc4_link_deinit(priv);
> +err_link_init:
> +	enetc_free_msix(priv);
> +err_alloc_msix:
> +err_config_si:
> +err_clk_get:
> +	mutex_destroy(&priv->mm_lock);
> +	free_netdev(ndev);
> +
> +	return err;
> +}
> +
> +static void enetc4_pf_netdev_destroy(struct enetc_si *si)
> +{
> +	struct net_device *ndev = si->ndev;
> +	struct enetc_ndev_priv *priv;
> +
> +	priv = netdev_priv(ndev);
> +	unregister_netdev(ndev);
> +	enetc_free_msix(priv);
> +	free_netdev(ndev);
> +}
> +
> +static int enetc4_pf_probe(struct pci_dev *pdev,
> +			   const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct enetc_si *si;
> +	struct enetc_pf *pf;
> +	int err;
> +
> +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> +	if (err) {
> +		dev_err(dev, "PCIe probing failed\n");
> +		return err;

use dev_err_probe()

> +	}
> +
> +	/* si is the private data. */
> +	si = pci_get_drvdata(pdev);
> +	if (!si->hw.port || !si->hw.global) {
> +		err = -ENODEV;
> +		dev_err(dev, "Couldn't map PF only space!\n");
> +		goto err_enetc_pci_probe;
> +	}
> +
> +	err = enetc4_pf_struct_init(si);
> +	if (err)
> +		goto err_pf_struct_init;
> +
> +	pf = enetc_si_priv(si);
> +	err = enetc4_pf_init(pf);
> +	if (err)
> +		goto err_pf_init;
> +
> +	pinctrl_pm_select_default_state(dev);
> +	enetc_get_si_caps(si);
> +	err = enetc4_pf_netdev_create(si);
> +	if (err)
> +		goto err_netdev_create;
> +
> +	return 0;
> +
> +err_netdev_create:
> +err_pf_init:
> +err_pf_struct_init:
> +err_enetc_pci_probe:
> +	enetc_pci_remove(pdev);

you can use devm_add_action_or_reset() to remove these goto labels.

Frank
> +
> +	return err;
> +}
> +
> +static void enetc4_pf_remove(struct pci_dev *pdev)
> +{
> +	struct enetc_si *si;
> +	struct enetc_pf *pf;
> +
> +	si = pci_get_drvdata(pdev);
> +	pf = enetc_si_priv(si);
> +
> +	enetc4_pf_netdev_destroy(si);
> +	enetc_pci_remove(pdev);
> +}
> +
> +static const struct pci_device_id enetc4_pf_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF) },
> +	{ 0, } /* End of table. */
> +};
> +MODULE_DEVICE_TABLE(pci, enetc4_pf_id_table);
> +
> +static struct pci_driver enetc4_pf_driver = {
> +	.name = KBUILD_MODNAME,
> +	.id_table = enetc4_pf_id_table,
> +	.probe = enetc4_pf_probe,
> +	.remove = enetc4_pf_remove,
> +};
> +module_pci_driver(enetc4_pf_driver);
> +
> +MODULE_DESCRIPTION("ENETC4 PF Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> index 2563eb8ac7b6..13ce771e0c43 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> @@ -44,6 +44,9 @@ static int enetc_get_reglen(struct net_device *ndev)
>  	struct enetc_hw *hw = &priv->si->hw;
>  	int len;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	len = ARRAY_SIZE(enetc_si_regs);
>  	len += ARRAY_SIZE(enetc_txbdr_regs) * priv->num_tx_rings;
>  	len += ARRAY_SIZE(enetc_rxbdr_regs) * priv->num_rx_rings;
> @@ -68,6 +71,9 @@ static void enetc_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
>  	int i, j;
>  	u32 addr;
>
> +	if (is_enetc_rev4(priv->si))
> +		return;
> +
>  	for (i = 0; i < ARRAY_SIZE(enetc_si_regs); i++) {
>  		*buf++ = enetc_si_regs[i];
>  		*buf++ = enetc_rd(hw, enetc_si_regs[i]);
> @@ -229,6 +235,9 @@ static int enetc_get_sset_count(struct net_device *ndev, int sset)
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	int len;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	if (sset != ETH_SS_STATS)
>  		return -EOPNOTSUPP;
>
> @@ -250,6 +259,9 @@ static void enetc_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
>  	u8 *p = data;
>  	int i, j;
>
> +	if (is_enetc_rev4(priv->si))
> +		return;
> +
>  	switch (stringset) {
>  	case ETH_SS_STATS:
>  		for (i = 0; i < ARRAY_SIZE(enetc_si_counters); i++) {
> @@ -290,6 +302,9 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	int i, o = 0;
>
> +	if (is_enetc_rev4(priv->si))
> +		return;
> +
>  	for (i = 0; i < ARRAY_SIZE(enetc_si_counters); i++)
>  		data[o++] = enetc_rd64(hw, enetc_si_counters[i].reg);
>
> @@ -331,6 +346,9 @@ static void enetc_get_pause_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	struct enetc_si *si = priv->si;
>
> +	if (is_enetc_rev4(si))
> +		return;
> +
>  	switch (pause_stats->src) {
>  	case ETHTOOL_MAC_STATS_SRC_EMAC:
>  		enetc_pause_stats(hw, 0, pause_stats);
> @@ -418,6 +436,9 @@ static void enetc_get_eth_mac_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	struct enetc_si *si = priv->si;
>
> +	if (is_enetc_rev4(si))
> +		return;
> +
>  	switch (mac_stats->src) {
>  	case ETHTOOL_MAC_STATS_SRC_EMAC:
>  		enetc_mac_stats(hw, 0, mac_stats);
> @@ -439,6 +460,9 @@ static void enetc_get_eth_ctrl_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	struct enetc_si *si = priv->si;
>
> +	if (is_enetc_rev4(si))
> +		return;
> +
>  	switch (ctrl_stats->src) {
>  	case ETHTOOL_MAC_STATS_SRC_EMAC:
>  		enetc_ctrl_stats(hw, 0, ctrl_stats);
> @@ -461,6 +485,9 @@ static void enetc_get_rmon_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	struct enetc_si *si = priv->si;
>
> +	if (is_enetc_rev4(si))
> +		return;
> +
>  	*ranges = enetc_rmon_ranges;
>
>  	switch (rmon_stats->src) {
> @@ -593,6 +620,9 @@ static int enetc_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc,
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	int i, j;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	switch (rxnfc->cmd) {
>  	case ETHTOOL_GRXRINGS:
>  		rxnfc->data = priv->num_rx_rings;
> @@ -643,6 +673,9 @@ static int enetc_set_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc)
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	int err;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	switch (rxnfc->cmd) {
>  	case ETHTOOL_SRXCLSRLINS:
>  		if (rxnfc->fs.location >= priv->si->num_fs_entries)
> @@ -678,6 +711,9 @@ static u32 enetc_get_rxfh_key_size(struct net_device *ndev)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>
> +	if (is_enetc_rev4(priv->si))
> +		return 0;
> +
>  	/* return the size of the RX flow hash key.  PF only */
>  	return (priv->si->hw.port) ? ENETC_RSSHASH_KEY_SIZE : 0;
>  }
> @@ -686,6 +722,9 @@ static u32 enetc_get_rxfh_indir_size(struct net_device *ndev)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>
> +	if (is_enetc_rev4(priv->si))
> +		return 0;
> +
>  	/* return the size of the RX flow hash indirection table */
>  	return priv->si->num_rss;
>  }
> @@ -697,6 +736,9 @@ static int enetc_get_rxfh(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	int err = 0, i;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	/* return hash function */
>  	rxfh->hfunc = ETH_RSS_HASH_TOP;
>
> @@ -731,6 +773,9 @@ static int enetc_set_rxfh(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	int err = 0;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	/* set hash key, if PF */
>  	if (rxfh->key && hw->port)
>  		enetc_set_rss_key(hw, rxfh->key);
> @@ -775,9 +820,10 @@ static int enetc_get_coalesce(struct net_device *ndev,
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	struct enetc_int_vector *v = priv->int_vector[0];
> +	u64 clk_freq = priv->si->clk_freq;
>
> -	ic->tx_coalesce_usecs = enetc_cycles_to_usecs(priv->tx_ictt);
> -	ic->rx_coalesce_usecs = enetc_cycles_to_usecs(v->rx_ictt);
> +	ic->tx_coalesce_usecs = enetc_cycles_to_usecs(priv->tx_ictt, clk_freq);
> +	ic->rx_coalesce_usecs = enetc_cycles_to_usecs(v->rx_ictt, clk_freq);
>
>  	ic->tx_max_coalesced_frames = ENETC_TXIC_PKTTHR;
>  	ic->rx_max_coalesced_frames = ENETC_RXIC_PKTTHR;
> @@ -793,12 +839,13 @@ static int enetc_set_coalesce(struct net_device *ndev,
>  			      struct netlink_ext_ack *extack)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	u64 clk_freq = priv->si->clk_freq;
>  	u32 rx_ictt, tx_ictt;
>  	int i, ic_mode;
>  	bool changed;
>
> -	tx_ictt = enetc_usecs_to_cycles(ic->tx_coalesce_usecs);
> -	rx_ictt = enetc_usecs_to_cycles(ic->rx_coalesce_usecs);
> +	tx_ictt = enetc_usecs_to_cycles(ic->tx_coalesce_usecs, clk_freq);
> +	rx_ictt = enetc_usecs_to_cycles(ic->rx_coalesce_usecs, clk_freq);
>
>  	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>  		return -EOPNOTSUPP;
> @@ -843,8 +890,12 @@ static int enetc_set_coalesce(struct net_device *ndev,
>  static int enetc_get_ts_info(struct net_device *ndev,
>  			     struct kernel_ethtool_ts_info *info)
>  {
> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>  	int *phc_idx;
>
> +	if (is_enetc_rev4(priv->si))
> +		return -EOPNOTSUPP;
> +
>  	phc_idx = symbol_get(enetc_phc_index);
>  	if (phc_idx) {
>  		info->phc_index = *phc_idx;
> @@ -942,6 +993,9 @@ static void enetc_get_mm_stats(struct net_device *ndev,
>  	struct enetc_hw *hw = &priv->si->hw;
>  	struct enetc_si *si = priv->si;
>
> +	if (is_enetc_rev4(si))
> +		return;
> +
>  	if (!(si->hw_features & ENETC_SI_F_QBU))
>  		return;
>
> @@ -960,6 +1014,9 @@ static int enetc_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
>  	struct enetc_hw *hw = &si->hw;
>  	u32 lafs, rafs, val;
>
> +	if (is_enetc_rev4(si))
> +		return -EOPNOTSUPP;
> +
>  	if (!(si->hw_features & ENETC_SI_F_QBU))
>  		return -EOPNOTSUPP;
>
> @@ -1090,6 +1147,9 @@ static int enetc_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>  	u32 val, add_frag_size;
>  	int err;
>
> +	if (is_enetc_rev4(si))
> +		return -EOPNOTSUPP;
> +
>  	if (!(si->hw_features & ENETC_SI_F_QBU))
>  		return -EOPNOTSUPP;
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> index 6a7b9b75d660..230e94986091 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> @@ -25,6 +25,7 @@
>  #define ENETC_SIPCAPR0_RSS	BIT(8)
>  #define ENETC_SIPCAPR0_QBV	BIT(4)
>  #define ENETC_SIPCAPR0_QBU	BIT(3)
> +#define ENETC_SIPCAPR0_RFS	BIT(2)
>  #define ENETC_SIPCAPR1	0x24
>  #define ENETC_SITGTGR	0x30
>  #define ENETC_SIRBGCR	0x38
> @@ -971,15 +972,16 @@ struct enetc_cbd {
>  	u8 status_flags;
>  };
>
> -#define ENETC_CLK  400000000ULL
> -static inline u32 enetc_cycles_to_usecs(u32 cycles)
> +#define ENETC_CLK		400000000ULL
> +#define ENETC_CLK_333M		333000000ULL
> +static inline u32 enetc_cycles_to_usecs(u32 cycles, u64 clk_freq)
>  {
> -	return (u32)div_u64(cycles * 1000000ULL, ENETC_CLK);
> +	return (u32)div_u64(cycles * 1000000ULL, clk_freq);
>  }
>
> -static inline u32 enetc_usecs_to_cycles(u32 usecs)
> +static inline u32 enetc_usecs_to_cycles(u32 usecs, u64 clk_freq)
>  {
> -	return (u32)div_u64(usecs * ENETC_CLK, 1000000ULL);
> +	return (u32)div_u64(usecs * clk_freq, 1000000ULL);
>  }
>
>  /* Port traffic class frame preemption register */
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> index ad7dab0eb752..91f2b5afb506 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> @@ -28,6 +28,14 @@ struct enetc_vf_state {
>  	enum enetc_vf_flags flags;
>  };
>
> +struct enetc_port_caps {
> +	u32 half_duplex:1;
> +	int num_vsi;
> +	int num_msix;
> +	int num_rx_bdr;
> +	int num_tx_bdr;
> +};
> +
>  struct enetc_pf;
>
>  struct enetc_pf_ops {
> @@ -60,6 +68,7 @@ struct enetc_pf {
>  	phy_interface_t if_mode;
>  	struct phylink_config phylink_config;
>
> +	struct enetc_port_caps caps;
>  	const struct enetc_pf_ops *ops;
>  };
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> index bbfb5c1ffd13..d00e1ddadb43 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> @@ -119,10 +119,17 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
>  	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_TSO |
>  			      NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4;
>
> +	ndev->priv_flags |= IFF_UNICAST_FLT;
> +
> +	/* TODO: currently, i.MX95 ENETC driver does not support advanced features */
> +	if (is_enetc_rev4(si)) {
> +		ndev->hw_features &= ~(NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK);
> +		goto end;
> +	}
> +
>  	if (si->num_rss)
>  		ndev->hw_features |= NETIF_F_RXHASH;
>
> -	ndev->priv_flags |= IFF_UNICAST_FLT;
>  	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
>  			     NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG |
>  			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> @@ -133,6 +140,7 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
>  		ndev->hw_features |= NETIF_F_HW_TC;
>  	}
>
> +end:
>  	/* pick up primary MAC address from SI */
>  	enetc_load_primary_mac_addr(&si->hw, ndev);
>  }
> --
> 2.34.1
>
Wei Fang Oct. 10, 2024, 2:31 a.m. UTC | #5
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月10日 1:02
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 04/11] net: enetc: add initial netc-blk-ctrl driver
> support
> 
> On Wed, Oct 09, 2024 at 05:51:09PM +0800, Wei Fang wrote:
> > The netc-blk-ctrl driver is used to configure Integrated Endpoint
> > Register Block (IERB) and Privileged Register Block (PRB) of NETC.
> > For i.MX platforms, it is also used to configure the NETCMIX block.
> >
> > The IERB contains registers that are used for pre-boot initialization,
> > debug, and non-customer configuration. The PRB controls global reset
> > and global error handling for NETC. The NETCMIX block is mainly used
> > to set MII protocol and PCS protocol of the links, it also contains
> > settings for some other functions.
> >
> > Note the IERB configuration registers can only be written after being
> > unlocked by PRB, otherwise, all write operations are inhibited. A warm
> > reset is performed when the IERB is unlocked, and it results in an FLR
> > to all NETC devices. Therefore, all NETC device drivers must be probed
> > or initialized after the warm reset is finished.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/enetc/Kconfig  |  14 +
> >  drivers/net/ethernet/freescale/enetc/Makefile |   3 +
> >  .../ethernet/freescale/enetc/netc_blk_ctrl.c  | 476 ++++++++++++++++++
> >  include/linux/fsl/netc_global.h               |  39 ++
> >  4 files changed, 532 insertions(+)
> >  create mode 100644 drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> >  create mode 100644 include/linux/fsl/netc_global.h
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig
> b/drivers/net/ethernet/freescale/enetc/Kconfig
> > index 4d75e6807e92..51d80ea959d4 100644
> > --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> > +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> > @@ -75,3 +75,17 @@ config FSL_ENETC_QOS
> >  	  enable/disable from user space via Qos commands(tc). In the kernel
> >  	  side, it can be loaded by Qos driver. Currently, it is only support
> >  	  taprio(802.1Qbv) and Credit Based Shaper(802.1Qbu).
> > +
> > +config NXP_NETC_BLK_CTRL
> > +	tristate "NETC blocks control driver"
> > +	help
> > +	  This driver configures Integrated Endpoint Register Block (IERB) and
> > +	  Privileged Register Block (PRB) of NETC. For i.MX platforms, it also
> > +	  includes the configuration of NETCMIX block.
> > +	  The IERB contains registers that are used for pre-boot initialization,
> > +	  debug, and non-customer configuration. The PRB controls global reset
> > +	  and global error handling for NETC. The NETCMIX block is mainly used
> > +	  to set MII protocol and PCS protocol of the links, it also contains
> > +	  settings for some other functions.
> > +
> > +	  If compiled as module (M), the module name is nxp-netc-blk-ctrl.
> > diff --git a/drivers/net/ethernet/freescale/enetc/Makefile
> b/drivers/net/ethernet/freescale/enetc/Makefile
> > index b13cbbabb2ea..5c277910d538 100644
> > --- a/drivers/net/ethernet/freescale/enetc/Makefile
> > +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> > @@ -19,3 +19,6 @@ fsl-enetc-mdio-y := enetc_pci_mdio.o enetc_mdio.o
> >
> >  obj-$(CONFIG_FSL_ENETC_PTP_CLOCK) += fsl-enetc-ptp.o
> >  fsl-enetc-ptp-y := enetc_ptp.o
> > +
> > +obj-$(CONFIG_NXP_NETC_BLK_CTRL) += nxp-netc-blk-ctrl.o
> > +nxp-netc-blk-ctrl-y := netc_blk_ctrl.o
> > \ No newline at end of file
> > diff --git a/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> > new file mode 100644
> > index 000000000000..b8eec980c199
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> > @@ -0,0 +1,476 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/*
> > + * NXP NETC Blocks Control Driver
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/fsl/netc_global.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_net.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/seq_file.h>
> > +
> > +/* NETCMIX registers */
> > +#define IMX95_CFG_LINK_IO_VAR		0x0
> > +#define  IO_VAR_16FF_16G_SERDES		0x1
> > +#define  IO_VAR(port, var)		(((var) & 0xf) << ((port) << 2))
> > +
> > +#define IMX95_CFG_LINK_MII_PROT		0x4
> > +#define CFG_LINK_MII_PORT_0		GENMASK(3, 0)
> > +#define CFG_LINK_MII_PORT_1		GENMASK(7, 4)
> 
> need #include <linux/bits.h>
> 
> > +#define  MII_PROT_MII			0x0
> > +#define  MII_PROT_RMII			0x1
> > +#define  MII_PROT_RGMII			0x2
> > +#define  MII_PROT_SERIAL		0x3
> > +#define  MII_PROT(port, prot)		(((prot) & 0xf) << ((port) << 2))
> > +
> > +#define IMX95_CFG_LINK_PCS_PROT(a)	(0x8 + (a) * 4)
> > +#define PCS_PROT_1G_SGMII		BIT(0)
> > +#define PCS_PROT_2500M_SGMII		BIT(1)
> > +#define PCS_PROT_XFI			BIT(3)
> > +#define PCS_PROT_SFI			BIT(4)
> > +#define PCS_PROT_10G_SXGMII		BIT(6)
> > +
> > +/* NETC privileged register block register */
> > +#define PRB_NETCRR			0x100
> > +#define  NETCRR_SR			BIT(0)
> > +#define  NETCRR_LOCK			BIT(1)
> > +
> > +#define PRB_NETCSR			0x104
> > +#define  NETCSR_ERROR			BIT(0)
> > +#define  NETCSR_STATE			BIT(1)
> > +
> > +/* NETC integrated endpoint register block register */
> > +#define IERB_EMDIOFAUXR			0x344
> > +#define IERB_T0FAUXR			0x444
> > +#define IERB_EFAUXR(a)			(0x3044 + 0x100 * (a))
> > +#define IERB_VFAUXR(a)			(0x4004 + 0x40 * (a))
> > +#define FAUXR_LDID			GENMASK(3, 0)
> > +
> > +/* Platform information */
> > +#define IMX95_ENETC0_BUS_DEVFN		0x0
> > +#define IMX95_ENETC1_BUS_DEVFN		0x40
> > +#define IMX95_ENETC2_BUS_DEVFN		0x80
> > +
> > +/* Flags for different platforms */
> > +#define NETC_HAS_NETCMIX		BIT(0)
> > +
> > +struct netc_devinfo {
> > +	u32 flags;
> > +	int (*netcmix_init)(struct platform_device *pdev);
> > +	int (*ierb_init)(struct platform_device *pdev);
> > +};
> > +
> > +struct netc_blk_ctrl {
> > +	void __iomem *prb;
> > +	void __iomem *ierb;
> > +	void __iomem *netcmix;
> > +	struct clk *ipg_clk;
> > +
> > +	const struct netc_devinfo *devinfo;
> > +	struct platform_device *pdev;
> > +	struct dentry *debugfs_root;
> > +};
> > +
> > +static void netc_reg_write(void __iomem *base, u32 offset, u32 val)
> > +{
> > +	netc_write(base + offset, val);
> > +}
> > +
> > +static u32 netc_reg_read(void __iomem *base, u32 offset)
> > +{
> > +	return netc_read(base + offset);
> > +}
> > +
> > +static int netc_of_pci_get_bus_devfn(struct device_node *np)
> > +{
> > +	u32 reg[5];
> > +	int error;
> > +
> > +	error = of_property_read_u32_array(np, "reg", reg, ARRAY_SIZE(reg));
> > +	if (error)
> > +		return error;
> 
> Avoid parse these common property "reg". if you need untranslate address
> you can use of_property_read_reg(), if you need bus translated cpu
> address, you can use platform_get_resource().

netc_of_pci_get_bus_devfn() is similar to of_pci_get_devfn(), just get one more
bus information.
> 
> Frank
> > +
> > +	return (reg[0] >> 8) & 0xffff;
> > +}
> > +
> > +static int netc_get_link_mii_protocol(phy_interface_t interface)
> > +{
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_MII:
> > +		return MII_PROT_MII;
> > +	case PHY_INTERFACE_MODE_RMII:
> > +		return MII_PROT_RMII;
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		return MII_PROT_RGMII;
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +	case PHY_INTERFACE_MODE_XGMII:
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		return MII_PROT_SERIAL;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int imx95_netcmix_init(struct platform_device *pdev)
> > +{
> > +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	phy_interface_t interface;
> > +	int bus_devfn, mii_proto;
> > +	u32 val;
> > +	int err;
> > +
> > +	/* Default setting of MII protocol */
> > +	val = MII_PROT(0, MII_PROT_RGMII) | MII_PROT(1, MII_PROT_RGMII) |
> > +	      MII_PROT(2, MII_PROT_SERIAL);
> > +
> > +	/* Update the link MII protocol through parsing phy-mode */
> > +	for_each_available_child_of_node_scoped(np, child) {
> > +		for_each_available_child_of_node_scoped(child, gchild) {
> > +			if (!of_device_is_compatible(gchild, "nxp,imx95-enetc"))
> > +				continue;
> > +
> > +			bus_devfn = netc_of_pci_get_bus_devfn(gchild);
> > +			if (bus_devfn < 0)
> > +				return -EINVAL;
> > +
> > +			if (bus_devfn == IMX95_ENETC2_BUS_DEVFN)
> > +				continue;
> > +
> > +			err = of_get_phy_mode(gchild, &interface);
> > +			if (err)
> > +				continue;
> > +
> > +			mii_proto = netc_get_link_mii_protocol(interface);
> > +			if (mii_proto < 0)
> > +				return -EINVAL;
> > +
> > +			switch (bus_devfn) {
> > +			case IMX95_ENETC0_BUS_DEVFN:
> > +				val = u32_replace_bits(val, mii_proto,
> > +						       CFG_LINK_MII_PORT_0);
> > +				break;
> > +			case IMX95_ENETC1_BUS_DEVFN:
> > +				val = u32_replace_bits(val, mii_proto,
> > +						       CFG_LINK_MII_PORT_1);
> > +				break;
> > +			default:
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* Configure Link I/O variant */
> > +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_IO_VAR,
> > +		       IO_VAR(2, IO_VAR_16FF_16G_SERDES));
> > +	/* Configure Link 2 PCS protocol */
> > +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_PCS_PROT(2),
> > +		       PCS_PROT_10G_SXGMII);
> > +	netc_reg_write(priv->netcmix, IMX95_CFG_LINK_MII_PROT, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static bool netc_ierb_is_locked(struct netc_blk_ctrl *priv)
> > +{
> > +	return !!(netc_reg_read(priv->prb, PRB_NETCRR) & NETCRR_LOCK);
> > +}
> > +
> > +static int netc_lock_ierb(struct netc_blk_ctrl *priv)
> > +{
> > +	u32 val;
> > +
> > +	netc_reg_write(priv->prb, PRB_NETCRR, NETCRR_LOCK);
> > +
> > +	return read_poll_timeout(netc_reg_read, val, !(val & NETCSR_STATE),
> > +				 100, 2000, false, priv->prb, PRB_NETCSR);
> > +}
> > +
> > +static int netc_unlock_ierb_with_warm_reset(struct netc_blk_ctrl *priv)
> > +{
> > +	u32 val;
> > +
> > +	netc_reg_write(priv->prb, PRB_NETCRR, 0);
> > +
> > +	return read_poll_timeout(netc_reg_read, val, !(val & NETCRR_LOCK),
> > +				 1000, 100000, true, priv->prb, PRB_NETCRR);
> > +}
> > +
> > +static int imx95_ierb_init(struct platform_device *pdev)
> > +{
> > +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> > +
> > +	/* EMDIO : No MSI-X intterupt */
> > +	netc_reg_write(priv->ierb, IERB_EMDIOFAUXR, 0);
> > +	/* ENETC0 PF */
> > +	netc_reg_write(priv->ierb, IERB_EFAUXR(0), 0);
> > +	/* ENETC0 VF0 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(0), 1);
> > +	/* ENETC0 VF1 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(1), 2);
> > +	/* ENETC1 PF */
> > +	netc_reg_write(priv->ierb, IERB_EFAUXR(1), 3);
> > +	/* ENETC1 VF0 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(2), 5);
> > +	/* ENETC1 VF1 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(3), 6);
> > +	/* ENETC2 PF */
> > +	netc_reg_write(priv->ierb, IERB_EFAUXR(2), 4);
> > +	/* ENETC2 VF0 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(4), 5);
> > +	/* ENETC2 VF1 */
> > +	netc_reg_write(priv->ierb, IERB_VFAUXR(5), 6);
> > +	/* NETC TIMER */
> > +	netc_reg_write(priv->ierb, IERB_T0FAUXR, 7);
> > +
> > +	return 0;
> > +}
> > +
> > +static int netc_ierb_init(struct platform_device *pdev)
> > +{
> > +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> > +	const struct netc_devinfo *devinfo = priv->devinfo;
> > +	int err;
> > +
> > +	if (netc_ierb_is_locked(priv)) {
> > +		err = netc_unlock_ierb_with_warm_reset(priv);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "Unlock IERB failed.\n");
> > +			return err;
> > +		}
> > +	}
> > +
> > +	if (devinfo->ierb_init) {
> > +		err = devinfo->ierb_init(pdev);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	err = netc_lock_ierb(priv);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "Lock IERB failed.\n");
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +static int netc_prb_show(struct seq_file *s, void *data)
> > +{
> > +	struct netc_blk_ctrl *priv = s->private;
> > +	u32 val;
> > +
> > +	val = netc_reg_read(priv->prb, PRB_NETCRR);
> > +	seq_printf(s, "[PRB NETCRR] Lock:%d SR:%d\n",
> > +		   (val & NETCRR_LOCK) ? 1 : 0,
> > +		   (val & NETCRR_SR) ? 1 : 0);
> > +
> > +	val = netc_reg_read(priv->prb, PRB_NETCSR);
> > +	seq_printf(s, "[PRB NETCSR] State:%d Error:%d\n",
> > +		   (val & NETCSR_STATE) ? 1 : 0,
> > +		   (val & NETCSR_ERROR) ? 1 : 0);
> > +
> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(netc_prb);
> > +
> > +static void netc_blk_ctrl_create_debugfs(struct netc_blk_ctrl *priv)
> > +{
> > +	struct dentry *root;
> > +
> > +	root = debugfs_create_dir("netc_blk_ctrl", NULL);
> > +	if (IS_ERR(root))
> > +		return;
> > +
> > +	priv->debugfs_root = root;
> > +
> > +	debugfs_create_file("prb", 0444, root, priv, &netc_prb_fops);
> > +}
> > +
> > +static void netc_blk_ctrl_remove_debugfs(struct netc_blk_ctrl *priv)
> > +{
> > +	debugfs_remove_recursive(priv->debugfs_root);
> > +	priv->debugfs_root = NULL;
> > +}
> > +
> > +#else
> > +
> > +static void netc_blk_ctrl_create_debugfs(struct netc_blk_ctrl *priv)
> > +{
> > +}
> > +
> > +static void netc_blk_ctrl_remove_debugfs(struct netc_blk_ctrl *priv)
> > +{
> > +}
> > +#endif
> > +
> > +static int netc_prb_check_error(struct netc_blk_ctrl *priv)
> > +{
> > +	u32 val;
> > +
> > +	val = netc_reg_read(priv->prb, PRB_NETCSR);
> > +	if (val & NETCSR_ERROR)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct netc_devinfo imx95_devinfo = {
> > +	.flags = NETC_HAS_NETCMIX,
> > +	.netcmix_init = imx95_netcmix_init,
> > +	.ierb_init = imx95_ierb_init,
> > +};
> > +
> > +static const struct of_device_id netc_blk_ctrl_match[] = {
> > +	{ .compatible = "nxp,imx95-netc-blk-ctrl", .data = &imx95_devinfo },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, netc_blk_ctrl_match);
> > +
> > +static int netc_blk_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *node = pdev->dev.of_node;
> > +	const struct netc_devinfo *devinfo;
> > +	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *id;
> > +	struct netc_blk_ctrl *priv;
> > +	void __iomem *regs;
> > +	int err;
> > +
> > +	if (!node || !of_device_is_available(node)) {
> > +		dev_info(dev, "Device is disabled, skipping\n");
> > +		return -ENODEV;
> > +	}
> 
> look like needn't above check.

I will check it, thanks.
> 
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->pdev = pdev;
> > +	priv->ipg_clk = devm_clk_get_optional(dev, "ipg_clk");
> > +	if (IS_ERR(priv->ipg_clk)) {
> > +		dev_err(dev, "Get ipg_clk failed\n");
> > +		err = PTR_ERR(priv->ipg_clk);
> > +		return err;
> 
> return dev_err_probe(dev, err, "...");
> same for below error path.

Okay
> 
> > +	}
> > +
> > +	err = clk_prepare_enable(priv->ipg_clk);
> > +	if (err) {
> > +		dev_err(dev, "Enable ipg_clk failed\n");
> > +		goto disable_ipg_clk;
> > +	}
> > +
> > +	id = of_match_device(netc_blk_ctrl_match, dev);
> > +	if (!id) {
> > +		dev_err(dev, "Cannot match device\n");
> > +		err = -EINVAL;
> > +		goto disable_ipg_clk;
> > +	}
> > +
> > +	devinfo = (struct netc_devinfo *)id->data;
> > +	if (!devinfo) {
> > +		dev_err(dev, "No device information\n");
> > +		err = -EINVAL;
> > +		goto disable_ipg_clk;
> > +	}
> > +	priv->devinfo = devinfo;
> > +
> > +	regs = devm_platform_ioremap_resource_byname(pdev, "ierb");
> > +	if (IS_ERR(regs)) {
> > +		err = PTR_ERR(regs);
> > +		dev_err(dev, "Missing IERB resource\n");
> > +		goto disable_ipg_clk;
> > +	}
> > +	priv->ierb = regs;
> > +
> > +	regs = devm_platform_ioremap_resource_byname(pdev, "prb");
> > +	if (IS_ERR(regs)) {
> > +		err = PTR_ERR(regs);
> > +		dev_err(dev, "Missing PRB resource\n");
> > +		goto disable_ipg_clk;
> > +	}
> > +	priv->prb = regs;
> > +
> > +	if (devinfo->flags & NETC_HAS_NETCMIX) {
> > +		regs = devm_platform_ioremap_resource_byname(pdev, "netcmix");
> > +		if (IS_ERR(regs)) {
> > +			err = PTR_ERR(regs);
> > +			dev_err(dev, "Missing NETCMIX resource\n");
> > +			goto disable_ipg_clk;
> > +		}
> > +		priv->netcmix = regs;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	if (devinfo->netcmix_init) {
> > +		err = devinfo->netcmix_init(pdev);
> > +		if (err) {
> > +			dev_err(dev, "Initializing NETCMIX failed\n");
> > +			goto disable_ipg_clk;
> > +		}
> > +	}
> > +
> > +	err = netc_ierb_init(pdev);
> > +	if (err) {
> > +		dev_err(dev, "Initializing IERB failed.\n");
> > +		goto disable_ipg_clk;
> > +	}
> > +
> > +	if (netc_prb_check_error(priv) < 0)
> > +		dev_warn(dev, "The current IERB configuration is invalid.\n");
> > +
> > +	netc_blk_ctrl_create_debugfs(priv);
> > +
> > +	err = of_platform_populate(node, NULL, NULL, dev);
> > +	if (err) {
> > +		dev_err(dev, "of_platform_populate failed\n");
> > +		goto remove_debugfs;
> > +	}
> > +
> > +	return 0;
> > +
> > +remove_debugfs:
> > +	netc_blk_ctrl_remove_debugfs(priv);
> > +disable_ipg_clk:
> > +	clk_disable_unprepare(priv->ipg_clk);
> > +
> > +	return err;
> > +}
> > +
> > +static void netc_blk_ctrl_remove(struct platform_device *pdev)
> > +{
> > +	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
> > +
> > +	of_platform_depopulate(&pdev->dev);
> > +	netc_blk_ctrl_remove_debugfs(priv);
> > +	clk_disable_unprepare(priv->ipg_clk);
> > +}
> > +
> > +static struct platform_driver netc_blk_ctrl_driver = {
> > +	.driver = {
> > +		.name = "nxp-netc-blk-ctrl",
> > +		.of_match_table = netc_blk_ctrl_match,
> > +	},
> > +	.probe = netc_blk_ctrl_probe,
> > +	.remove = netc_blk_ctrl_remove,
> > +};
> > +
> > +module_platform_driver(netc_blk_ctrl_driver);
> > +
> > +MODULE_DESCRIPTION("NXP NETC Blocks Control Driver");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
> > new file mode 100644
> > index 000000000000..f26b1b6f8813
> > --- /dev/null
> > +++ b/include/linux/fsl/netc_global.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/* Copyright 2024 NXP
> > + */
> > +#ifndef __NETC_GLOBAL_H
> > +#define __NETC_GLOBAL_H
> > +
> > +#include <linux/io.h>
> > +
> > +static inline u32 netc_read(void __iomem *reg)
> > +{
> > +	return ioread32(reg);
> > +}
> > +
> > +#ifdef ioread64
> > +static inline u64 netc_read64(void __iomem *reg)
> > +{
> > +	return ioread64(reg);
> > +}
> > +#else
> > +static inline u64 netc_read64(void __iomem *reg)
> > +{
> > +	u32 low, high;
> > +	u64 val;
> > +
> > +	low = ioread32(reg);
> > +	high = ioread32(reg + 4);
> > +
> > +	val = (u64)high << 32 | low;
> > +
> > +	return val;
> > +}
> > +#endif
> > +
> > +static inline void netc_write(void __iomem *reg, u32 val)
> > +{
> > +	iowrite32(val, reg);
> > +}
> 
> why need two layer register read/write wrap?

I just want to encapsulate ioread/iowrite into netc_read/nect_write and
provide them to the relevant NETC drivers. Also netc_read/write are inline
functions, so I don't think there will be anything wrong with this.
> 
> netc_reg_write() -> netc_write() -> iowrite32();
> 
> Frank
> > +
> > +#endif
> > --
> > 2.34.1
> >
Wei Fang Oct. 10, 2024, 3:19 a.m. UTC | #6
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月10日 1:16
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 05/11] net: enetc: add enetc-pf-common driver
> support
> 
> On Wed, Oct 09, 2024 at 05:51:10PM +0800, Wei Fang wrote:
> > The ENETC of LS1028A is revision 1.0. Now, ENETC is used on the i.MX95
> > platform and the revision is upgraded to version 4.1. The two versions
> > are incompatible except for the station interface (SI) part. Therefore,
> > we need to add a new driver for ENETC revision 4.1 and later. However,
>   ^^^^^^^^^^
>  just Add a new driver ...
> 
> > the logic of some interfaces of the two drivers is basically the same,
> > and the only difference is the hardware configuration. So in order to
> > reuse these interfaces and reduce code redundancy, we extract these
>                                                      ^^
> extract these .. in order to ...
> 
> > interfaces and compile them into a separate enetc-pf-common driver for
> > use by these two PF drivers. Note that the ENETC PF 4.1 driver will be
>                                ^^^
> Prepare to add support ENETC PF 4.1 driver in subsequent patches.
> 
> > supported in subsequent patches.
> 
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/enetc/Kconfig  |   9 +
> >  drivers/net/ethernet/freescale/enetc/Makefile |   3 +
> >  .../net/ethernet/freescale/enetc/enetc_pf.c   | 350 +---------------
> >  .../net/ethernet/freescale/enetc/enetc_pf.h   |  28 ++
> >  .../freescale/enetc/enetc_pf_common.c         | 375
> ++++++++++++++++++
> >  5 files changed, 431 insertions(+), 334 deletions(-)
> >  create mode 100644
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig
> b/drivers/net/ethernet/freescale/enetc/Kconfig
> > index 51d80ea959d4..6f3306f14060 100644
> > --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> > +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> > @@ -7,10 +7,19 @@ config FSL_ENETC_CORE
> >
> >  	  If compiled as module (M), the module name is fsl-enetc-core.
> >
> > +config NXP_ENETC_PF_COMMON
> > +	tristate "ENETC PF common functionality driver"
> > +	help
> > +	  This module supports common functionality between drivers of
> > +	  different versions of NXP ENETC PF controllers.
> > +
> > +	  If compiled as module (M), the module name is nxp-enetc-pf-common.
> > +
> >  config FSL_ENETC
> >  	tristate "ENETC PF driver"
> >  	depends on PCI_MSI
> >  	select MDIO_DEVRES
> > +	select NXP_ENETC_PF_COMMON
> >  	select FSL_ENETC_CORE
> >  	select FSL_ENETC_IERB
> >  	select FSL_ENETC_MDIO
> > diff --git a/drivers/net/ethernet/freescale/enetc/Makefile
> b/drivers/net/ethernet/freescale/enetc/Makefile
> > index 5c277910d538..b81ca462e358 100644
> > --- a/drivers/net/ethernet/freescale/enetc/Makefile
> > +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> > @@ -3,6 +3,9 @@
> >  obj-$(CONFIG_FSL_ENETC_CORE) += fsl-enetc-core.o
> >  fsl-enetc-core-y := enetc.o enetc_cbdr.o enetc_ethtool.o
> >
> > +obj-$(CONFIG_NXP_ENETC_PF_COMMON) += nxp-enetc-pf-common.o
> > +nxp-enetc-pf-common-y := enetc_pf_common.o
> > +
> 
> I am not sure why you can't link enetc_pf_common.o into enetc_pf.o?
>
If we select CONFIG_FSL_ENETC=M and CONFIG_NXP_ENETC4=Y, there will
be compilation errors due to " undefined reference".

>
> >  obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
> >  fsl-enetc-y := enetc_pf.o
> >  fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > index 8f6b0bf48139..dae8be4a1607 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > @@ -33,18 +33,15 @@ static void enetc_pf_set_primary_mac_addr(struct
> enetc_hw *hw, int si,
> >  	__raw_writew(lower, hw->port + ENETC_PSIPMAR1(si));
> >  }
> >
> > -static int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr)
> > +static struct phylink_pcs *enetc_pf_create_pcs(struct enetc_pf *pf,
> > +					       struct mii_bus *bus)
> >  {
> > -	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > -	struct sockaddr *saddr = addr;
> > -
> > -	if (!is_valid_ether_addr(saddr->sa_data))
> > -		return -EADDRNOTAVAIL;
> > -
> > -	eth_hw_addr_set(ndev, saddr->sa_data);
> > -	enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data);
> > +	return lynx_pcs_create_mdiodev(bus, 0);
> > +}
> >
> > -	return 0;
> > +static void enetc_pf_destroy_pcs(struct phylink_pcs *pcs)
> > +{
> > +	lynx_pcs_destroy(pcs);
> >  }
> >
> >  static void enetc_set_vlan_promisc(struct enetc_hw *hw, char si_map)
> > @@ -393,56 +390,6 @@ static int enetc_pf_set_vf_spoofchk(struct
> net_device *ndev, int vf, bool en)
> >  	return 0;
> >  }
> >
> > -static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf
> *pf,
> > -				   int si)
> > -{
> > -	struct device *dev = &pf->si->pdev->dev;
> > -	struct enetc_hw *hw = &pf->si->hw;
> > -	u8 mac_addr[ETH_ALEN] = { 0 };
> > -	int err;
> > -
> > -	/* (1) try to get the MAC address from the device tree */
> > -	if (np) {
> > -		err = of_get_mac_address(np, mac_addr);
> > -		if (err == -EPROBE_DEFER)
> > -			return err;
> > -	}
> > -
> > -	/* (2) bootloader supplied MAC address */
> > -	if (is_zero_ether_addr(mac_addr))
> > -		enetc_pf_get_primary_mac_addr(hw, si, mac_addr);
> > -
> > -	/* (3) choose a random one */
> > -	if (is_zero_ether_addr(mac_addr)) {
> > -		eth_random_addr(mac_addr);
> > -		dev_info(dev, "no MAC address specified for SI%d, using %pM\n",
> > -			 si, mac_addr);
> > -	}
> > -
> > -	enetc_pf_set_primary_mac_addr(hw, si, mac_addr);
> > -
> > -	return 0;
> > -}
> > -
> > -static int enetc_setup_mac_addresses(struct device_node *np,
> > -				     struct enetc_pf *pf)
> > -{
> > -	int err, i;
> > -
> > -	/* The PF might take its MAC from the device tree */
> > -	err = enetc_setup_mac_address(np, pf, 0);
> > -	if (err)
> > -		return err;
> > -
> > -	for (i = 0; i < pf->total_vfs; i++) {
> > -		err = enetc_setup_mac_address(NULL, pf, i + 1);
> > -		if (err)
> > -			return err;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  static void enetc_port_assign_rfs_entries(struct enetc_si *si)
> >  {
> >  	struct enetc_pf *pf = enetc_si_priv(si);
> > @@ -656,55 +603,6 @@ void enetc_msg_handle_rxmsg(struct enetc_pf *pf,
> int vf_id, u16 *status)
> >  	}
> >  }
> >
> > -#ifdef CONFIG_PCI_IOV
> > -static int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs)
> > -{
> > -	struct enetc_si *si = pci_get_drvdata(pdev);
> > -	struct enetc_pf *pf = enetc_si_priv(si);
> > -	int err;
> > -
> > -	if (!num_vfs) {
> > -		enetc_msg_psi_free(pf);
> > -		kfree(pf->vf_state);
> > -		pf->num_vfs = 0;
> > -		pci_disable_sriov(pdev);
> > -	} else {
> > -		pf->num_vfs = num_vfs;
> > -
> > -		pf->vf_state = kcalloc(num_vfs, sizeof(struct enetc_vf_state),
> > -				       GFP_KERNEL);
> > -		if (!pf->vf_state) {
> > -			pf->num_vfs = 0;
> > -			return -ENOMEM;
> > -		}
> > -
> > -		err = enetc_msg_psi_init(pf);
> > -		if (err) {
> > -			dev_err(&pdev->dev, "enetc_msg_psi_init (%d)\n", err);
> > -			goto err_msg_psi;
> > -		}
> > -
> > -		err = pci_enable_sriov(pdev, num_vfs);
> > -		if (err) {
> > -			dev_err(&pdev->dev, "pci_enable_sriov err %d\n", err);
> > -			goto err_en_sriov;
> > -		}
> > -	}
> > -
> > -	return num_vfs;
> > -
> > -err_en_sriov:
> > -	enetc_msg_psi_free(pf);
> > -err_msg_psi:
> > -	kfree(pf->vf_state);
> > -	pf->num_vfs = 0;
> > -
> > -	return err;
> > -}
> > -#else
> > -#define enetc_sriov_configure(pdev, num_vfs)	(void)0
> > -#endif
> > -
> >  static int enetc_pf_set_features(struct net_device *ndev,
> >  				 netdev_features_t features)
> >  {
> > @@ -775,187 +673,6 @@ static const struct net_device_ops enetc_ndev_ops
> = {
> >  	.ndo_xdp_xmit		= enetc_xdp_xmit,
> >  };
> >
> > -static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device
> *ndev,
> > -				  const struct net_device_ops *ndev_ops)
> > -{
> > -	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > -
> > -	SET_NETDEV_DEV(ndev, &si->pdev->dev);
> > -	priv->ndev = ndev;
> > -	priv->si = si;
> > -	priv->dev = &si->pdev->dev;
> > -	si->ndev = ndev;
> > -
> > -	priv->msg_enable = (NETIF_MSG_WOL << 1) - 1;
> > -	ndev->netdev_ops = ndev_ops;
> > -	enetc_set_ethtool_ops(ndev);
> > -	ndev->watchdog_timeo = 5 * HZ;
> > -	ndev->max_mtu = ENETC_MAX_MTU;
> > -
> > -	ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM |
> > -			    NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_CTAG_RX |
> > -			    NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK |
> > -			    NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6;
> > -	ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM |
> > -			 NETIF_F_HW_VLAN_CTAG_TX |
> > -			 NETIF_F_HW_VLAN_CTAG_RX |
> > -			 NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6;
> > -	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM |
> > -			      NETIF_F_TSO | NETIF_F_TSO6;
> > -
> > -	if (si->num_rss)
> > -		ndev->hw_features |= NETIF_F_RXHASH;
> > -
> > -	ndev->priv_flags |= IFF_UNICAST_FLT;
> > -	ndev->xdp_features = NETDEV_XDP_ACT_BASIC |
> NETDEV_XDP_ACT_REDIRECT |
> > -			     NETDEV_XDP_ACT_NDO_XMIT |
> NETDEV_XDP_ACT_RX_SG |
> > -			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> > -
> > -	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
> > -		priv->active_offloads |= ENETC_F_QCI;
> > -		ndev->features |= NETIF_F_HW_TC;
> > -		ndev->hw_features |= NETIF_F_HW_TC;
> > -	}
> > -
> > -	/* pick up primary MAC address from SI */
> > -	enetc_load_primary_mac_addr(&si->hw, ndev);
> > -}
> > -
> > -static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
> > -{
> > -	struct device *dev = &pf->si->pdev->dev;
> > -	struct enetc_mdio_priv *mdio_priv;
> > -	struct mii_bus *bus;
> > -	int err;
> > -
> > -	bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv));
> > -	if (!bus)
> > -		return -ENOMEM;
> > -
> > -	bus->name = "Freescale ENETC MDIO Bus";
> > -	bus->read = enetc_mdio_read_c22;
> > -	bus->write = enetc_mdio_write_c22;
> > -	bus->read_c45 = enetc_mdio_read_c45;
> > -	bus->write_c45 = enetc_mdio_write_c45;
> > -	bus->parent = dev;
> > -	mdio_priv = bus->priv;
> > -	mdio_priv->hw = &pf->si->hw;
> > -	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
> > -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > -
> > -	err = of_mdiobus_register(bus, np);
> > -	if (err)
> > -		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> > -
> > -	pf->mdio = bus;
> > -
> > -	return 0;
> > -}
> > -
> > -static void enetc_mdio_remove(struct enetc_pf *pf)
> > -{
> > -	if (pf->mdio)
> > -		mdiobus_unregister(pf->mdio);
> > -}
> > -
> > -static int enetc_imdio_create(struct enetc_pf *pf)
> > -{
> > -	struct device *dev = &pf->si->pdev->dev;
> > -	struct enetc_mdio_priv *mdio_priv;
> > -	struct phylink_pcs *phylink_pcs;
> > -	struct mii_bus *bus;
> > -	int err;
> > -
> > -	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
> > -	if (!bus)
> > -		return -ENOMEM;
> > -
> > -	bus->name = "Freescale ENETC internal MDIO Bus";
> > -	bus->read = enetc_mdio_read_c22;
> > -	bus->write = enetc_mdio_write_c22;
> > -	bus->read_c45 = enetc_mdio_read_c45;
> > -	bus->write_c45 = enetc_mdio_write_c45;
> > -	bus->parent = dev;
> > -	bus->phy_mask = ~0;
> > -	mdio_priv = bus->priv;
> > -	mdio_priv->hw = &pf->si->hw;
> > -	mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
> > -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
> > -
> > -	err = mdiobus_register(bus);
> > -	if (err) {
> > -		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);
> > -		goto free_mdio_bus;
> > -	}
> > -
> > -	phylink_pcs = lynx_pcs_create_mdiodev(bus, 0);
> > -	if (IS_ERR(phylink_pcs)) {
> > -		err = PTR_ERR(phylink_pcs);
> > -		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
> > -		goto unregister_mdiobus;
> > -	}
> > -
> > -	pf->imdio = bus;
> > -	pf->pcs = phylink_pcs;
> > -
> > -	return 0;
> > -
> > -unregister_mdiobus:
> > -	mdiobus_unregister(bus);
> > -free_mdio_bus:
> > -	mdiobus_free(bus);
> > -	return err;
> > -}
> > -
> > -static void enetc_imdio_remove(struct enetc_pf *pf)
> > -{
> > -	if (pf->pcs)
> > -		lynx_pcs_destroy(pf->pcs);
> > -	if (pf->imdio) {
> > -		mdiobus_unregister(pf->imdio);
> > -		mdiobus_free(pf->imdio);
> > -	}
> > -}
> > -
> > -static bool enetc_port_has_pcs(struct enetc_pf *pf)
> > -{
> > -	return (pf->if_mode == PHY_INTERFACE_MODE_SGMII ||
> > -		pf->if_mode == PHY_INTERFACE_MODE_1000BASEX ||
> > -		pf->if_mode == PHY_INTERFACE_MODE_2500BASEX ||
> > -		pf->if_mode == PHY_INTERFACE_MODE_USXGMII);
> > -}
> > -
> > -static int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node
> *node)
> > -{
> > -	struct device_node *mdio_np;
> > -	int err;
> > -
> > -	mdio_np = of_get_child_by_name(node, "mdio");
> > -	if (mdio_np) {
> > -		err = enetc_mdio_probe(pf, mdio_np);
> > -
> > -		of_node_put(mdio_np);
> > -		if (err)
> > -			return err;
> > -	}
> > -
> > -	if (enetc_port_has_pcs(pf)) {
> > -		err = enetc_imdio_create(pf);
> > -		if (err) {
> > -			enetc_mdio_remove(pf);
> > -			return err;
> > -		}
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -static void enetc_mdiobus_destroy(struct enetc_pf *pf)
> > -{
> > -	enetc_mdio_remove(pf);
> > -	enetc_imdio_remove(pf);
> > -}
> > -
> >  static struct phylink_pcs *
> >  enetc_pl_mac_select_pcs(struct phylink_config *config, phy_interface_t
> iface)
> >  {
> > @@ -1101,47 +818,6 @@ static const struct phylink_mac_ops
> enetc_mac_phylink_ops = {
> >  	.mac_link_down = enetc_pl_mac_link_down,
> >  };
> >
> > -static int enetc_phylink_create(struct enetc_ndev_priv *priv,
> > -				struct device_node *node)
> > -{
> > -	struct enetc_pf *pf = enetc_si_priv(priv->si);
> > -	struct phylink *phylink;
> > -	int err;
> > -
> > -	pf->phylink_config.dev = &priv->ndev->dev;
> > -	pf->phylink_config.type = PHYLINK_NETDEV;
> > -	pf->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> MAC_SYM_PAUSE |
> > -		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
> > -
> > -	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > -		  pf->phylink_config.supported_interfaces);
> > -	__set_bit(PHY_INTERFACE_MODE_SGMII,
> > -		  pf->phylink_config.supported_interfaces);
> > -	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > -		  pf->phylink_config.supported_interfaces);
> > -	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > -		  pf->phylink_config.supported_interfaces);
> > -	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> > -		  pf->phylink_config.supported_interfaces);
> > -	phy_interface_set_rgmii(pf->phylink_config.supported_interfaces);
> > -
> > -	phylink = phylink_create(&pf->phylink_config, of_fwnode_handle(node),
> > -				 pf->if_mode, &enetc_mac_phylink_ops);
> > -	if (IS_ERR(phylink)) {
> > -		err = PTR_ERR(phylink);
> > -		return err;
> > -	}
> > -
> > -	priv->phylink = phylink;
> > -
> > -	return 0;
> > -}
> > -
> > -static void enetc_phylink_destroy(struct enetc_ndev_priv *priv)
> > -{
> > -	phylink_destroy(priv->phylink);
> > -}
> > -
> >  /* Initialize the entire shared memory for the flow steering entries
> >   * of this port (PF + VFs)
> >   */
> > @@ -1259,6 +935,13 @@ static void enetc_psi_destroy(struct pci_dev *pdev)
> >  	enetc_pci_remove(pdev);
> >  }
> >
> > +static const struct enetc_pf_ops enetc_pf_ops = {
> > +	.set_si_primary_mac = enetc_pf_set_primary_mac_addr,
> > +	.get_si_primary_mac = enetc_pf_get_primary_mac_addr,
> > +	.create_pcs = enetc_pf_create_pcs,
> > +	.destroy_pcs = enetc_pf_destroy_pcs,
> > +};
> > +
> 
> I suppose this patch should just move functions to common.c. This involve
> addition code logic change. It is not easy to follow up to make sure your
> change is correct.
> 

Because these common interfaces are separated into a separate driver, some
changes are inevitable. I can split the patch into two parts, the first part moves
the functions into common.c, and the second part modifies common.c into a
separate driver.

> Frank
> 
> >  static int enetc_pf_probe(struct pci_dev *pdev,
> >  			  const struct pci_device_id *ent)
> >  {
> > @@ -1286,6 +969,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
> >  	pf = enetc_si_priv(si);
> >  	pf->si = si;
> >  	pf->total_vfs = pci_sriov_get_totalvfs(pdev);
> > +	enetc_pf_ops_register(pf, &enetc_pf_ops);
> >
> >  	err = enetc_setup_mac_addresses(node, pf);
> >  	if (err)
> > @@ -1338,7 +1022,7 @@ static int enetc_pf_probe(struct pci_dev *pdev,
> >  	if (err)
> >  		goto err_mdiobus_create;
> >
> > -	err = enetc_phylink_create(priv, node);
> > +	err = enetc_phylink_create(priv, node, &enetc_mac_phylink_ops);
> >  	if (err)
> >  		goto err_phylink_create;
> >
> > @@ -1422,9 +1106,7 @@ static struct pci_driver enetc_pf_driver = {
> >  	.id_table = enetc_pf_id_table,
> >  	.probe = enetc_pf_probe,
> >  	.remove = enetc_pf_remove,
> > -#ifdef CONFIG_PCI_IOV
> >  	.sriov_configure = enetc_sriov_configure,
> > -#endif
> >  };
> >  module_pci_driver(enetc_pf_driver);
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > index c26bd66e4597..ad7dab0eb752 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > @@ -28,6 +28,15 @@ struct enetc_vf_state {
> >  	enum enetc_vf_flags flags;
> >  };
> >
> > +struct enetc_pf;
> > +
> > +struct enetc_pf_ops {
> > +	void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr);
> > +	void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr);
> > +	struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus);
> > +	void (*destroy_pcs)(struct phylink_pcs *pcs);
> > +};
> > +
> >  struct enetc_pf {
> >  	struct enetc_si *si;
> >  	int num_vfs; /* number of active VFs, after sriov_init */
> > @@ -50,6 +59,8 @@ struct enetc_pf {
> >
> >  	phy_interface_t if_mode;
> >  	struct phylink_config phylink_config;
> > +
> > +	const struct enetc_pf_ops *ops;
> >  };
> >
> >  #define phylink_to_enetc_pf(config) \
> > @@ -58,3 +69,20 @@ struct enetc_pf {
> >  int enetc_msg_psi_init(struct enetc_pf *pf);
> >  void enetc_msg_psi_free(struct enetc_pf *pf);
> >  void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16
> *status);
> > +
> > +int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr);
> > +int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf
> *pf);
> > +void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> > +			   const struct net_device_ops *ndev_ops);
> > +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node);
> > +void enetc_mdiobus_destroy(struct enetc_pf *pf);
> > +int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node
> *node,
> > +			 const struct phylink_mac_ops *pl_mac_ops);
> > +void enetc_phylink_destroy(struct enetc_ndev_priv *priv);
> > +int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs);
> > +
> > +static inline void enetc_pf_ops_register(struct enetc_pf *pf,
> > +					 const struct enetc_pf_ops *ops)
> > +{
> > +	pf->ops = ops;
> > +}
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > new file mode 100644
> > index 000000000000..bbfb5c1ffd13
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > @@ -0,0 +1,375 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2024 NXP */
> > +#include <linux/fsl/enetc_mdio.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/of_net.h>
> > +
> > +#include "enetc_pf.h"
> > +
> > +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr)
> > +{
> > +	struct enetc_hw *hw = &pf->si->hw;
> > +
> > +	if (pf->ops->set_si_primary_mac)
> > +		pf->ops->set_si_primary_mac(hw, si, mac_addr);
> > +	else
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> > +
> > +int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> > +	struct sockaddr *saddr = addr;
> > +	int err;
> > +
> > +	if (!is_valid_ether_addr(saddr->sa_data))
> > +		return -EADDRNOTAVAIL;
> > +
> > +	err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data);
> > +	if (err)
> > +		return err;
> > +
> > +	eth_hw_addr_set(ndev, saddr->sa_data);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr);
> > +
> > +static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf
> *pf,
> > +				   int si)
> > +{
> > +	struct device *dev = &pf->si->pdev->dev;
> > +	struct enetc_hw *hw = &pf->si->hw;
> > +	u8 mac_addr[ETH_ALEN] = { 0 };
> > +	int err;
> > +
> > +	/* (1) try to get the MAC address from the device tree */
> > +	if (np) {
> > +		err = of_get_mac_address(np, mac_addr);
> > +		if (err == -EPROBE_DEFER)
> > +			return err;
> > +	}
> > +
> > +	/* (2) bootloader supplied MAC address */
> > +	if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac)
> > +		pf->ops->get_si_primary_mac(hw, si, mac_addr);
> > +
> > +	/* (3) choose a random one */
> > +	if (is_zero_ether_addr(mac_addr)) {
> > +		eth_random_addr(mac_addr);
> > +		dev_info(dev, "no MAC address specified for SI%d, using %pM\n",
> > +			 si, mac_addr);
> > +	}
> > +
> > +	err = enetc_set_si_hw_addr(pf, si, mac_addr);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf
> *pf)
> > +{
> > +	int err, i;
> > +
> > +	/* The PF might take its MAC from the device tree */
> > +	err = enetc_setup_mac_address(np, pf, 0);
> > +	if (err)
> > +		return err;
> > +
> > +	for (i = 0; i < pf->total_vfs; i++) {
> > +		err = enetc_setup_mac_address(NULL, pf, i + 1);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_setup_mac_addresses);
> > +
> > +void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> > +			   const struct net_device_ops *ndev_ops)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > +
> > +	SET_NETDEV_DEV(ndev, &si->pdev->dev);
> > +	priv->ndev = ndev;
> > +	priv->si = si;
> > +	priv->dev = &si->pdev->dev;
> > +	si->ndev = ndev;
> > +
> > +	priv->msg_enable = (NETIF_MSG_WOL << 1) - 1;
> > +	ndev->netdev_ops = ndev_ops;
> > +	enetc_set_ethtool_ops(ndev);
> > +	ndev->watchdog_timeo = 5 * HZ;
> > +	ndev->max_mtu = ENETC_MAX_MTU;
> > +
> > +	ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM |
> > +			    NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_CTAG_RX |
> > +			    NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK |
> > +			    NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6 |
> > +			    NETIF_F_GSO_UDP_L4;
> > +	ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM |
> > +			 NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX
> |
> > +			 NETIF_F_HW_CSUM | NETIF_F_TSO | NETIF_F_TSO6 |
> > +			 NETIF_F_GSO_UDP_L4;
> > +	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_TSO |
> > +			      NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4;
> > +
> > +	if (si->num_rss)
> > +		ndev->hw_features |= NETIF_F_RXHASH;
> > +
> > +	ndev->priv_flags |= IFF_UNICAST_FLT;
> > +	ndev->xdp_features = NETDEV_XDP_ACT_BASIC |
> NETDEV_XDP_ACT_REDIRECT |
> > +			     NETDEV_XDP_ACT_NDO_XMIT |
> NETDEV_XDP_ACT_RX_SG |
> > +			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> > +
> > +	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
> > +		priv->active_offloads |= ENETC_F_QCI;
> > +		ndev->features |= NETIF_F_HW_TC;
> > +		ndev->hw_features |= NETIF_F_HW_TC;
> > +	}
> > +
> > +	/* pick up primary MAC address from SI */
> > +	enetc_load_primary_mac_addr(&si->hw, ndev);
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup);
> > +
> > +static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
> > +{
> > +	struct device *dev = &pf->si->pdev->dev;
> > +	struct enetc_mdio_priv *mdio_priv;
> > +	struct mii_bus *bus;
> > +	int err;
> > +
> > +	bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "Freescale ENETC MDIO Bus";
> > +	bus->read = enetc_mdio_read_c22;
> > +	bus->write = enetc_mdio_write_c22;
> > +	bus->read_c45 = enetc_mdio_read_c45;
> > +	bus->write_c45 = enetc_mdio_write_c45;
> > +	bus->parent = dev;
> > +	mdio_priv = bus->priv;
> > +	mdio_priv->hw = &pf->si->hw;
> > +	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > +
> > +	err = of_mdiobus_register(bus, np);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> > +
> > +	pf->mdio = bus;
> > +
> > +	return 0;
> > +}
> > +
> > +static void enetc_mdio_remove(struct enetc_pf *pf)
> > +{
> > +	if (pf->mdio)
> > +		mdiobus_unregister(pf->mdio);
> > +}
> > +
> > +static bool enetc_port_has_pcs(struct enetc_pf *pf)
> > +{
> > +	return (pf->if_mode == PHY_INTERFACE_MODE_SGMII ||
> > +		pf->if_mode == PHY_INTERFACE_MODE_1000BASEX ||
> > +		pf->if_mode == PHY_INTERFACE_MODE_2500BASEX ||
> > +		pf->if_mode == PHY_INTERFACE_MODE_USXGMII);
> > +}
> > +
> > +static int enetc_imdio_create(struct enetc_pf *pf)
> > +{
> > +	struct device *dev = &pf->si->pdev->dev;
> > +	struct enetc_mdio_priv *mdio_priv;
> > +	struct phylink_pcs *phylink_pcs;
> > +	struct mii_bus *bus;
> > +	int err;
> > +
> > +	if (!pf->ops->create_pcs)
> > +		return -EOPNOTSUPP;
> > +
> > +	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "Freescale ENETC internal MDIO Bus";
> > +	bus->read = enetc_mdio_read_c22;
> > +	bus->write = enetc_mdio_write_c22;
> > +	bus->read_c45 = enetc_mdio_read_c45;
> > +	bus->write_c45 = enetc_mdio_write_c45;
> > +	bus->parent = dev;
> > +	bus->phy_mask = ~0;
> > +	mdio_priv = bus->priv;
> > +	mdio_priv->hw = &pf->si->hw;
> > +	mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
> > +
> > +	err = mdiobus_register(bus);
> > +	if (err) {
> > +		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);
> > +		goto free_mdio_bus;
> > +	}
> > +
> > +	phylink_pcs = pf->ops->create_pcs(pf, bus);
> > +	if (IS_ERR(phylink_pcs)) {
> > +		err = PTR_ERR(phylink_pcs);
> > +		dev_err(dev, "cannot create pcs (%d)\n", err);
> > +		goto unregister_mdiobus;
> > +	}
> > +
> > +	pf->imdio = bus;
> > +	pf->pcs = phylink_pcs;
> > +
> > +	return 0;
> > +
> > +unregister_mdiobus:
> > +	mdiobus_unregister(bus);
> > +free_mdio_bus:
> > +	mdiobus_free(bus);
> > +	return err;
> > +}
> > +
> > +static void enetc_imdio_remove(struct enetc_pf *pf)
> > +{
> > +	if (pf->pcs && pf->ops->destroy_pcs)
> > +		pf->ops->destroy_pcs(pf->pcs);
> > +
> > +	if (pf->imdio) {
> > +		mdiobus_unregister(pf->imdio);
> > +		mdiobus_free(pf->imdio);
> > +	}
> > +}
> > +
> > +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node)
> > +{
> > +	struct device_node *mdio_np;
> > +	int err;
> > +
> > +	mdio_np = of_get_child_by_name(node, "mdio");
> > +	if (mdio_np) {
> > +		err = enetc_mdio_probe(pf, mdio_np);
> > +
> > +		of_node_put(mdio_np);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	if (enetc_port_has_pcs(pf)) {
> > +		err = enetc_imdio_create(pf);
> > +		if (err) {
> > +			enetc_mdio_remove(pf);
> > +			return err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_mdiobus_create);
> > +
> > +void enetc_mdiobus_destroy(struct enetc_pf *pf)
> > +{
> > +	enetc_mdio_remove(pf);
> > +	enetc_imdio_remove(pf);
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_mdiobus_destroy);
> > +
> > +int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node
> *node,
> > +			 const struct phylink_mac_ops *pl_mac_ops)
> > +{
> > +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> > +	struct phylink *phylink;
> > +	int err;
> > +
> > +	pf->phylink_config.dev = &priv->ndev->dev;
> > +	pf->phylink_config.type = PHYLINK_NETDEV;
> > +	pf->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> MAC_SYM_PAUSE |
> > +		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
> > +
> > +	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > +		  pf->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +		  pf->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > +		  pf->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +		  pf->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> > +		  pf->phylink_config.supported_interfaces);
> > +	phy_interface_set_rgmii(pf->phylink_config.supported_interfaces);
> > +
> > +	phylink = phylink_create(&pf->phylink_config, of_fwnode_handle(node),
> > +				 pf->if_mode, pl_mac_ops);
> > +	if (IS_ERR(phylink)) {
> > +		err = PTR_ERR(phylink);
> > +		return err;
> > +	}
> > +
> > +	priv->phylink = phylink;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_phylink_create);
> > +
> > +void enetc_phylink_destroy(struct enetc_ndev_priv *priv)
> > +{
> > +	phylink_destroy(priv->phylink);
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_phylink_destroy);
> > +
> > +int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs)
> > +{
> > +	struct enetc_si *si = pci_get_drvdata(pdev);
> > +	struct enetc_pf *pf = enetc_si_priv(si);
> > +	int err;
> > +
> > +	if (!IS_ENABLED(CONFIG_PCI_IOV))
> > +		return 0;
> > +
> > +	if (!num_vfs) {
> > +		pci_disable_sriov(pdev);
> > +		enetc_msg_psi_free(pf);
> > +		kfree(pf->vf_state);
> > +		pf->num_vfs = 0;
> > +	} else {
> > +		pf->num_vfs = num_vfs;
> > +
> > +		pf->vf_state = kcalloc(num_vfs, sizeof(struct enetc_vf_state),
> > +				       GFP_KERNEL);
> > +		if (!pf->vf_state) {
> > +			pf->num_vfs = 0;
> > +			return -ENOMEM;
> > +		}
> > +
> > +		err = enetc_msg_psi_init(pf);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "enetc_msg_psi_init (%d)\n", err);
> > +			goto err_msg_psi;
> > +		}
> > +
> > +		err = pci_enable_sriov(pdev, num_vfs);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "pci_enable_sriov err %d\n", err);
> > +			goto err_en_sriov;
> > +		}
> > +	}
> > +
> > +	return num_vfs;
> > +
> > +err_en_sriov:
> > +	enetc_msg_psi_free(pf);
> > +err_msg_psi:
> > +	kfree(pf->vf_state);
> > +	pf->num_vfs = 0;
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(enetc_sriov_configure);
> > +
> > +MODULE_DESCRIPTION("NXP ENETC PF common functionality driver");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > --
> > 2.34.1
> >
Wei Fang Oct. 10, 2024, 3:25 a.m. UTC | #7
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月10日 1:25
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 09/11] net: enetc: optimize the allocation of
> tx_bdr
> 
> On Wed, Oct 09, 2024 at 05:51:14PM +0800, Wei Fang wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > There is a situation where num_tx_rings cannot be divided by
> > bdr_int_num. For example, num_tx_rings is 8 and bdr_int_num
> > is 3. According to the previous logic, this results in two
> > tx_bdr corresponding memories not being allocated, so when
> > sending packets to tx BD ring 6 or 7, wild pointers will be
> > accessed. Of course, this issue does not exist for LS1028A,
> > because its num_tx_rings is 8, and bdr_int_num is either 1
> > or 2. So there is no situation where it cannot be divided.
> > However, there is a risk for the upcoming i.MX95, so the
> > allocation of tx_bdr is optimized to ensure that each tx_bdr
> > can be allocated to the corresponding memory.
> >
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 121 ++++++++++---------
> >  1 file changed, 62 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 032d8eadd003..b84c88a76762 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -2965,13 +2965,70 @@ int enetc_ioctl(struct net_device *ndev, struct
> ifreq *rq, int cmd)
> >  }
> >  EXPORT_SYMBOL_GPL(enetc_ioctl);
> >
> > +static int enetc_bdr_init(struct enetc_ndev_priv *priv, int i, int v_tx_rings)
> > +{
> > +	struct enetc_int_vector *v __free(kfree);
> > +	struct enetc_bdr *bdr;
> > +	int j, err;
> > +
> > +	v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> > +	if (!v)
> > +		return -ENOMEM;
> > +
> > +	bdr = &v->rx_ring;
> > +	bdr->index = i;
> > +	bdr->ndev = priv->ndev;
> > +	bdr->dev = priv->dev;
> > +	bdr->bd_count = priv->rx_bd_count;
> > +	bdr->buffer_offset = ENETC_RXB_PAD;
> > +	priv->rx_ring[i] = bdr;
> > +
> > +	err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0);
> > +	if (err)
> > +		return err;
> > +
> > +	err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq,
> > +					 MEM_TYPE_PAGE_SHARED, NULL);
> > +	if (err) {
> > +		xdp_rxq_info_unreg(&bdr->xdp.rxq);
> > +		return err;
> > +	}
> > +
> > +	/* init defaults for adaptive IC */
> > +	if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
> > +		v->rx_ictt = 0x1;
> > +		v->rx_dim_en = true;
> > +	}
> > +	INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
> > +	netif_napi_add(priv->ndev, &v->napi, enetc_poll);
> > +	v->count_tx_rings = v_tx_rings;
> > +
> > +	for (j = 0; j < v_tx_rings; j++) {
> > +		int idx;
> > +
> > +		/* default tx ring mapping policy */
> > +		idx = priv->bdr_int_num * j + i;
> > +		__set_bit(idx, &v->tx_rings_map);
> > +		bdr = &v->tx_ring[j];
> > +		bdr->index = idx;
> > +		bdr->ndev = priv->ndev;
> > +		bdr->dev = priv->dev;
> > +		bdr->bd_count = priv->tx_bd_count;
> > +		priv->tx_ring[idx] = bdr;
> > +	}
> > +
> > +	priv->int_vector[i] = no_free_ptr(v);
> > +
> > +	return 0;
> > +}
> > +
> >  int enetc_alloc_msix(struct enetc_ndev_priv *priv)
> >  {
> >  	struct pci_dev *pdev = priv->si->pdev;
> > +	int v_tx_rings, v_remainder;
> >  	int num_stack_tx_queues;
> >  	int first_xdp_tx_ring;
> >  	int i, n, err, nvec;
> > -	int v_tx_rings;
> >
> >  	nvec = ENETC_BDR_INT_BASE_IDX + priv->bdr_int_num;
> >  	/* allocate MSIX for both messaging and Rx/Tx interrupts */
> > @@ -2985,65 +3042,11 @@ int enetc_alloc_msix(struct enetc_ndev_priv
> *priv)
> >
> >  	/* # of tx rings per int vector */
> >  	v_tx_rings = priv->num_tx_rings / priv->bdr_int_num;
> > +	v_remainder = priv->num_tx_rings % priv->bdr_int_num;
> >
> > -	for (i = 0; i < priv->bdr_int_num; i++) {
> > -		struct enetc_int_vector *v;
> > -		struct enetc_bdr *bdr;
> > -		int j;
> > -
> > -		v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL);
> > -		if (!v) {
> > -			err = -ENOMEM;
> > -			goto fail;
> > -		}
> > -
> > -		priv->int_vector[i] = v;
> > -
> > -		bdr = &v->rx_ring;
> > -		bdr->index = i;
> > -		bdr->ndev = priv->ndev;
> > -		bdr->dev = priv->dev;
> > -		bdr->bd_count = priv->rx_bd_count;
> > -		bdr->buffer_offset = ENETC_RXB_PAD;
> > -		priv->rx_ring[i] = bdr;
> > -
> > -		err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0);
> > -		if (err) {
> > -			kfree(v);
> > -			goto fail;
> > -		}
> > -
> > -		err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq,
> > -						 MEM_TYPE_PAGE_SHARED, NULL);
> > -		if (err) {
> > -			xdp_rxq_info_unreg(&bdr->xdp.rxq);
> > -			kfree(v);
> > -			goto fail;
> > -		}
> > -
> > -		/* init defaults for adaptive IC */
> > -		if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
> > -			v->rx_ictt = 0x1;
> > -			v->rx_dim_en = true;
> > -		}
> > -		INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
> > -		netif_napi_add(priv->ndev, &v->napi, enetc_poll);
> > -		v->count_tx_rings = v_tx_rings;
> > -
> > -		for (j = 0; j < v_tx_rings; j++) {
> > -			int idx;
> > -
> > -			/* default tx ring mapping policy */
> > -			idx = priv->bdr_int_num * j + i;
> > -			__set_bit(idx, &v->tx_rings_map);
> > -			bdr = &v->tx_ring[j];
> > -			bdr->index = idx;
> > -			bdr->ndev = priv->ndev;
> > -			bdr->dev = priv->dev;
> > -			bdr->bd_count = priv->tx_bd_count;
> > -			priv->tx_ring[idx] = bdr;
> > -		}
> > -	}
> > +	for (i = 0; i < priv->bdr_int_num; i++)
> > +		enetc_bdr_init(priv, i,
> > +			       i < v_remainder ? v_tx_rings + 1 : v_tx_rings);
> 
> suggest you create two patches, one just move to help function to
> enetc_bdr_init(), the another is for real fixes.

Sure, thanks

> 
> >
> >  	num_stack_tx_queues = enetc_num_stack_tx_queues(priv);
> >
> > --
> > 2.34.1
> >
Wei Fang Oct. 10, 2024, 4:59 a.m. UTC | #8
> On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > different from the LS1028A ENETC (revision 1.0) except for the SI
> > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
>             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> major revision 4.
> 
> > PF, and this driver will be used to support the ENETC PF with major
> > revision 4 in the future.
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> b/drivers/net/ethernet/freescale/enetc/enetc.h
> > index 97524dfa234c..7f1ea11c33a0 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > @@ -14,6 +14,7 @@
> >  #include <net/xdp.h>
> >
> >  #include "enetc_hw.h"
> > +#include "enetc4_hw.h"
> >
> >  #define ENETC_SI_ALIGN	32
> >
> > +static inline bool is_enetc_rev1(struct enetc_si *si)
> > +{
> > +	return si->pdev->revision == ENETC_REV1;
> > +}
> > +
> > +static inline bool is_enetc_rev4(struct enetc_si *si)
> > +{
> > +	return si->pdev->revision == ENETC_REV4;
> > +}
> > +
> 
> Actually, I suggest you check features, instead of check version number.
> 
This is mainly used to distinguish between ENETC v1 and ENETC v4 in the
general interfaces. See enetc_ethtool.c.

> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > new file mode 100644
> > index 000000000000..e38ade76260b
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > @@ -0,0 +1,761 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2024 NXP */
> > +#include <linux/unaligned.h>
> > +#include <linux/module.h>
> > +#include <linux/of_net.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/clk.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/fsl/netc_global.h>
> 
> sort headers.
> 

Sure

> > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > +			   const struct pci_device_id *ent)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct enetc_si *si;
> > +	struct enetc_pf *pf;
> > +	int err;
> > +
> > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > +	if (err) {
> > +		dev_err(dev, "PCIe probing failed\n");
> > +		return err;
> 
> use dev_err_probe()
> 

Okay

> > +	}
> > +
> > +	/* si is the private data. */
> > +	si = pci_get_drvdata(pdev);
> > +	if (!si->hw.port || !si->hw.global) {
> > +		err = -ENODEV;
> > +		dev_err(dev, "Couldn't map PF only space!\n");
> > +		goto err_enetc_pci_probe;
> > +	}
> > +
> > +	err = enetc4_pf_struct_init(si);
> > +	if (err)
> > +		goto err_pf_struct_init;
> > +
> > +	pf = enetc_si_priv(si);
> > +	err = enetc4_pf_init(pf);
> > +	if (err)
> > +		goto err_pf_init;
> > +
> > +	pinctrl_pm_select_default_state(dev);
> > +	enetc_get_si_caps(si);
> > +	err = enetc4_pf_netdev_create(si);
> > +	if (err)
> > +		goto err_netdev_create;
> > +
> > +	return 0;
> > +
> > +err_netdev_create:
> > +err_pf_init:
> > +err_pf_struct_init:
> > +err_enetc_pci_probe:
> > +	enetc_pci_remove(pdev);
> 
> you can use devm_add_action_or_reset() to remove these goto labels.
> 
Subsequent patches will have corresponding processing for these labels,
so I don't want to add too many devm_add_action_or_reset ().
Frank Li Oct. 10, 2024, 3:22 p.m. UTC | #9
On Thu, Oct 10, 2024 at 04:59:45AM +0000, Wei Fang wrote:
> > On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > > different from the LS1028A ENETC (revision 1.0) except for the SI
> > > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> >             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> > major revision 4.
> >
> > > PF, and this driver will be used to support the ENETC PF with major
> > > revision 4 in the future.
> > >
> > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > index 97524dfa234c..7f1ea11c33a0 100644
> > > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > @@ -14,6 +14,7 @@
> > >  #include <net/xdp.h>
> > >
> > >  #include "enetc_hw.h"
> > > +#include "enetc4_hw.h"
> > >
> > >  #define ENETC_SI_ALIGN	32
> > >
> > > +static inline bool is_enetc_rev1(struct enetc_si *si)
> > > +{
> > > +	return si->pdev->revision == ENETC_REV1;
> > > +}
> > > +
> > > +static inline bool is_enetc_rev4(struct enetc_si *si)
> > > +{
> > > +	return si->pdev->revision == ENETC_REV4;
> > > +}
> > > +
> >
> > Actually, I suggest you check features, instead of check version number.
> >
> This is mainly used to distinguish between ENETC v1 and ENETC v4 in the
> general interfaces. See enetc_ethtool.c.

Suggest use flags, such as, IS_SUPPORT_ETHTOOL.

otherwise, your check may become complex in future.

If use flags, you just change id table in future.

{ PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF),
  .driver_data = IS_SUPPORT_ETHTOOL | .... },

Frank
>
> > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > new file mode 100644
> > > index 000000000000..e38ade76260b
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > @@ -0,0 +1,761 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > > +/* Copyright 2024 NXP */
> > > +#include <linux/unaligned.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_net.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/pinctrl/consumer.h>
> > > +#include <linux/fsl/netc_global.h>
> >
> > sort headers.
> >
>
> Sure
>
> > > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > > +			   const struct pci_device_id *ent)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct enetc_si *si;
> > > +	struct enetc_pf *pf;
> > > +	int err;
> > > +
> > > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > > +	if (err) {
> > > +		dev_err(dev, "PCIe probing failed\n");
> > > +		return err;
> >
> > use dev_err_probe()
> >
>
> Okay
>
> > > +	}
> > > +
> > > +	/* si is the private data. */
> > > +	si = pci_get_drvdata(pdev);
> > > +	if (!si->hw.port || !si->hw.global) {
> > > +		err = -ENODEV;
> > > +		dev_err(dev, "Couldn't map PF only space!\n");
> > > +		goto err_enetc_pci_probe;
> > > +	}
> > > +
> > > +	err = enetc4_pf_struct_init(si);
> > > +	if (err)
> > > +		goto err_pf_struct_init;
> > > +
> > > +	pf = enetc_si_priv(si);
> > > +	err = enetc4_pf_init(pf);
> > > +	if (err)
> > > +		goto err_pf_init;
> > > +
> > > +	pinctrl_pm_select_default_state(dev);
> > > +	enetc_get_si_caps(si);
> > > +	err = enetc4_pf_netdev_create(si);
> > > +	if (err)
> > > +		goto err_netdev_create;
> > > +
> > > +	return 0;
> > > +
> > > +err_netdev_create:
> > > +err_pf_init:
> > > +err_pf_struct_init:
> > > +err_enetc_pci_probe:
> > > +	enetc_pci_remove(pdev);
> >
> > you can use devm_add_action_or_reset() to remove these goto labels.
> >
> Subsequent patches will have corresponding processing for these labels,
> so I don't want to add too many devm_add_action_or_reset ().
Frank Li Oct. 10, 2024, 7:20 p.m. UTC | #10
On Wed, Oct 09, 2024 at 05:51:16PM +0800, Wei Fang wrote:
> Add related YAML documentation and header files. Also, add maintainers
> from the i.MX side as ENETC starts to be used on i.MX platforms.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  MAINTAINERS | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index af635dc60cfe..355b81b642a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9015,9 +9015,18 @@ F:	drivers/dma/fsl-edma*.*
>  FREESCALE ENETC ETHERNET DRIVERS
>  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
>  M:	Vladimir Oltean <vladimir.oltean@nxp.com>
> +M:	Wei Fang <wei.fang@nxp.com>
> +M:	Clark Wang <xiaoning.wang@nxp.com>
> +L:	imx@lists.linux.dev
>  L:	netdev@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/net/fsl,enetc-ierb.yaml
> +F:	Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> +F:	Documentation/devicetree/bindings/net/fsl,enetc.yaml
> +F:	Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml

Missed enetc_pf_common.c

Frank

>  F:	drivers/net/ethernet/freescale/enetc/
> +F:	include/linux/fsl/enetc_mdio.h
> +F:	include/linux/fsl/netc_global.h
>
>  FREESCALE eTSEC ETHERNET DRIVER (GIANFAR)
>  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> --
> 2.34.1
>
Wei Fang Oct. 11, 2024, 2:02 a.m. UTC | #11
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月10日 23:22
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> i.MX95 ENETC PF
> 
> On Thu, Oct 10, 2024 at 04:59:45AM +0000, Wei Fang wrote:
> > > On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > > > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > > > different from the LS1028A ENETC (revision 1.0) except for the SI
> > > > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > > > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> > >             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> > > major revision 4.
> > >
> > > > PF, and this driver will be used to support the ENETC PF with
> > > > major revision 4 in the future.
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > index 97524dfa234c..7f1ea11c33a0 100644
> > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > @@ -14,6 +14,7 @@
> > > >  #include <net/xdp.h>
> > > >
> > > >  #include "enetc_hw.h"
> > > > +#include "enetc4_hw.h"
> > > >
> > > >  #define ENETC_SI_ALIGN	32
> > > >
> > > > +static inline bool is_enetc_rev1(struct enetc_si *si) {
> > > > +	return si->pdev->revision == ENETC_REV1; }
> > > > +
> > > > +static inline bool is_enetc_rev4(struct enetc_si *si) {
> > > > +	return si->pdev->revision == ENETC_REV4; }
> > > > +
> > >
> > > Actually, I suggest you check features, instead of check version number.
> > >
> > This is mainly used to distinguish between ENETC v1 and ENETC v4 in
> > the general interfaces. See enetc_ethtool.c.
> 
> Suggest use flags, such as, IS_SUPPORT_ETHTOOL.
> 
> otherwise, your check may become complex in future.
> 
> If use flags, you just change id table in future.

enetc_ethtool just is an example, I meant that the ENETCv4 and ENETCv1
use some common drivers, like enect_pf_common, enetc-core, so different
hardware versions have different logic, that's all.

> 
> { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF),
>   .driver_data = IS_SUPPORT_ETHTOOL | .... },
> 
> Frank
> >
> > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > new file mode 100644
> > > > index 000000000000..e38ade76260b
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > @@ -0,0 +1,761 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > > > +/* Copyright 2024 NXP */
> > > > +#include <linux/unaligned.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_net.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/pinctrl/consumer.h> #include
> > > > +<linux/fsl/netc_global.h>
> > >
> > > sort headers.
> > >
> >
> > Sure
> >
> > > > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > > > +			   const struct pci_device_id *ent) {
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct enetc_si *si;
> > > > +	struct enetc_pf *pf;
> > > > +	int err;
> > > > +
> > > > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > > > +	if (err) {
> > > > +		dev_err(dev, "PCIe probing failed\n");
> > > > +		return err;
> > >
> > > use dev_err_probe()
> > >
> >
> > Okay
> >
> > > > +	}
> > > > +
> > > > +	/* si is the private data. */
> > > > +	si = pci_get_drvdata(pdev);
> > > > +	if (!si->hw.port || !si->hw.global) {
> > > > +		err = -ENODEV;
> > > > +		dev_err(dev, "Couldn't map PF only space!\n");
> > > > +		goto err_enetc_pci_probe;
> > > > +	}
> > > > +
> > > > +	err = enetc4_pf_struct_init(si);
> > > > +	if (err)
> > > > +		goto err_pf_struct_init;
> > > > +
> > > > +	pf = enetc_si_priv(si);
> > > > +	err = enetc4_pf_init(pf);
> > > > +	if (err)
> > > > +		goto err_pf_init;
> > > > +
> > > > +	pinctrl_pm_select_default_state(dev);
> > > > +	enetc_get_si_caps(si);
> > > > +	err = enetc4_pf_netdev_create(si);
> > > > +	if (err)
> > > > +		goto err_netdev_create;
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_netdev_create:
> > > > +err_pf_init:
> > > > +err_pf_struct_init:
> > > > +err_enetc_pci_probe:
> > > > +	enetc_pci_remove(pdev);
> > >
> > > you can use devm_add_action_or_reset() to remove these goto labels.
> > >
> > Subsequent patches will have corresponding processing for these
> > labels, so I don't want to add too many devm_add_action_or_reset ().
Wei Fang Oct. 11, 2024, 2:05 a.m. UTC | #12
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月11日 3:21
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 11/11] MAINTAINERS: update ENETC driver files
> and maintainers
> 
> On Wed, Oct 09, 2024 at 05:51:16PM +0800, Wei Fang wrote:
> > Add related YAML documentation and header files. Also, add maintainers
> > from the i.MX side as ENETC starts to be used on i.MX platforms.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  MAINTAINERS | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > af635dc60cfe..355b81b642a9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9015,9 +9015,18 @@ F:	drivers/dma/fsl-edma*.*
> >  FREESCALE ENETC ETHERNET DRIVERS
> >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> >  M:	Vladimir Oltean <vladimir.oltean@nxp.com>
> > +M:	Wei Fang <wei.fang@nxp.com>
> > +M:	Clark Wang <xiaoning.wang@nxp.com>
> > +L:	imx@lists.linux.dev
> >  L:	netdev@vger.kernel.org
> >  S:	Maintained
> > +F:	Documentation/devicetree/bindings/net/fsl,enetc-ierb.yaml
> > +F:	Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> > +F:	Documentation/devicetree/bindings/net/fsl,enetc.yaml
> > +F:	Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> 
> Missed enetc_pf_common.c
> 
enetc_pf_common.c is in drivers/net/ethernet/freescale/enetc/

> 
> >  F:	drivers/net/ethernet/freescale/enetc/
> > +F:	include/linux/fsl/enetc_mdio.h
> > +F:	include/linux/fsl/netc_global.h
> >
> >  FREESCALE eTSEC ETHERNET DRIVER (GIANFAR)
> >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> > --
> > 2.34.1
> >
Frank Li Oct. 11, 2024, 4:11 a.m. UTC | #13
On Fri, Oct 11, 2024 at 02:02:03AM +0000, Wei Fang wrote:
> > -----Original Message-----
> > From: Frank Li <frank.li@nxp.com>
> > Sent: 2024年10月10日 23:22
> > To: Wei Fang <wei.fang@nxp.com>
> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> > Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> > christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> > imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> > Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> > i.MX95 ENETC PF
> >
> > On Thu, Oct 10, 2024 at 04:59:45AM +0000, Wei Fang wrote:
> > > > On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > > > > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > > > > different from the LS1028A ENETC (revision 1.0) except for the SI
> > > > > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > > > > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> > > >             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> > > > major revision 4.
> > > >
> > > > > PF, and this driver will be used to support the ENETC PF with
> > > > > major revision 4 in the future.
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > index 97524dfa234c..7f1ea11c33a0 100644
> > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > @@ -14,6 +14,7 @@
> > > > >  #include <net/xdp.h>
> > > > >
> > > > >  #include "enetc_hw.h"
> > > > > +#include "enetc4_hw.h"
> > > > >
> > > > >  #define ENETC_SI_ALIGN	32
> > > > >
> > > > > +static inline bool is_enetc_rev1(struct enetc_si *si) {
> > > > > +	return si->pdev->revision == ENETC_REV1; }
> > > > > +
> > > > > +static inline bool is_enetc_rev4(struct enetc_si *si) {
> > > > > +	return si->pdev->revision == ENETC_REV4; }
> > > > > +
> > > >
> > > > Actually, I suggest you check features, instead of check version number.
> > > >
> > > This is mainly used to distinguish between ENETC v1 and ENETC v4 in
> > > the general interfaces. See enetc_ethtool.c.
> >
> > Suggest use flags, such as, IS_SUPPORT_ETHTOOL.
> >
> > otherwise, your check may become complex in future.
> >
> > If use flags, you just change id table in future.
>
> enetc_ethtool just is an example, I meant that the ENETCv4 and ENETCv1
> use some common drivers, like enect_pf_common, enetc-core, so different
> hardware versions have different logic, that's all.

My means is that avoid use v1\v2 to distingiush it and use supported
features in difference version for example:

ENETC_FEATURE_1, ENETC_FEATURE_2, ENETC_FEATURE_3, ENETC_FEATURE_4.

{ PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF)
  .driver_data = ENETC_FEATURE_1 |  ENETC_FEATURE_2 | ENETC_FEATURE_4
  PCI_DEVICE(....)
  .driver_data = ENETC_FEATURE_1 | ENETC_FEATURE_3,
  PCI_DEVICE(...)
  .driver_data = ENETC_FEATURE_4,
)

It will be easy to know the difference between difference version. Your if
check logic will be simple.

if (driver_data & ENETC_FEATURE_1)
  ....

otherwise
   if (vers == 1 || vers == 2 || ver == 5), which distribute to difference
places in whole code.

It is real hard to know hardware differences between version in future.

You can ref drivers/misc/pci_endpoint_test.c

Frank

>
> >
> > { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF),
> >   .driver_data = IS_SUPPORT_ETHTOOL | .... },
> >
> > Frank
> > >
> > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > new file mode 100644
> > > > > index 000000000000..e38ade76260b
> > > > > --- /dev/null
> > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > @@ -0,0 +1,761 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > > > > +/* Copyright 2024 NXP */
> > > > > +#include <linux/unaligned.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_net.h>
> > > > > +#include <linux/of_platform.h>
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/pinctrl/consumer.h> #include
> > > > > +<linux/fsl/netc_global.h>
> > > >
> > > > sort headers.
> > > >
> > >
> > > Sure
> > >
> > > > > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > > > > +			   const struct pci_device_id *ent) {
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct enetc_si *si;
> > > > > +	struct enetc_pf *pf;
> > > > > +	int err;
> > > > > +
> > > > > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > > > > +	if (err) {
> > > > > +		dev_err(dev, "PCIe probing failed\n");
> > > > > +		return err;
> > > >
> > > > use dev_err_probe()
> > > >
> > >
> > > Okay
> > >
> > > > > +	}
> > > > > +
> > > > > +	/* si is the private data. */
> > > > > +	si = pci_get_drvdata(pdev);
> > > > > +	if (!si->hw.port || !si->hw.global) {
> > > > > +		err = -ENODEV;
> > > > > +		dev_err(dev, "Couldn't map PF only space!\n");
> > > > > +		goto err_enetc_pci_probe;
> > > > > +	}
> > > > > +
> > > > > +	err = enetc4_pf_struct_init(si);
> > > > > +	if (err)
> > > > > +		goto err_pf_struct_init;
> > > > > +
> > > > > +	pf = enetc_si_priv(si);
> > > > > +	err = enetc4_pf_init(pf);
> > > > > +	if (err)
> > > > > +		goto err_pf_init;
> > > > > +
> > > > > +	pinctrl_pm_select_default_state(dev);
> > > > > +	enetc_get_si_caps(si);
> > > > > +	err = enetc4_pf_netdev_create(si);
> > > > > +	if (err)
> > > > > +		goto err_netdev_create;
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err_netdev_create:
> > > > > +err_pf_init:
> > > > > +err_pf_struct_init:
> > > > > +err_enetc_pci_probe:
> > > > > +	enetc_pci_remove(pdev);
> > > >
> > > > you can use devm_add_action_or_reset() to remove these goto labels.
> > > >
> > > Subsequent patches will have corresponding processing for these
> > > labels, so I don't want to add too many devm_add_action_or_reset ().
Frank Li Oct. 11, 2024, 4:13 a.m. UTC | #14
On Fri, Oct 11, 2024 at 02:05:37AM +0000, Wei Fang wrote:
> > -----Original Message-----
> > From: Frank Li <frank.li@nxp.com>
> > Sent: 2024年10月11日 3:21
> > To: Wei Fang <wei.fang@nxp.com>
> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> > Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> > christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> > imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> > Subject: Re: [PATCH net-next 11/11] MAINTAINERS: update ENETC driver files
> > and maintainers
> >
> > On Wed, Oct 09, 2024 at 05:51:16PM +0800, Wei Fang wrote:
> > > Add related YAML documentation and header files. Also, add maintainers
> > > from the i.MX side as ENETC starts to be used on i.MX platforms.
> > >
> > > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > > ---
> > >  MAINTAINERS | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > af635dc60cfe..355b81b642a9 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9015,9 +9015,18 @@ F:	drivers/dma/fsl-edma*.*
> > >  FREESCALE ENETC ETHERNET DRIVERS
> > >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> > >  M:	Vladimir Oltean <vladimir.oltean@nxp.com>
> > > +M:	Wei Fang <wei.fang@nxp.com>
> > > +M:	Clark Wang <xiaoning.wang@nxp.com>
> > > +L:	imx@lists.linux.dev
> > >  L:	netdev@vger.kernel.org
> > >  S:	Maintained
> > > +F:	Documentation/devicetree/bindings/net/fsl,enetc-ierb.yaml
> > > +F:	Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> > > +F:	Documentation/devicetree/bindings/net/fsl,enetc.yaml
> > > +F:	Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> >
> > Missed enetc_pf_common.c
> >
> enetc_pf_common.c is in drivers/net/ethernet/freescale/enetc/

Oh, sorry, I miss it.
Maybe

Documentation/devicetree/bindings/net/fsl,enetc*.yaml will be simple.

Frank

>
> >
> > >  F:	drivers/net/ethernet/freescale/enetc/
> > > +F:	include/linux/fsl/enetc_mdio.h
> > > +F:	include/linux/fsl/netc_global.h
> > >
> > >  FREESCALE eTSEC ETHERNET DRIVER (GIANFAR)
> > >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> > > --
> > > 2.34.1
> > >
Wei Fang Oct. 11, 2024, 7:16 a.m. UTC | #15
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月11日 12:12
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> i.MX95 ENETC PF
> 
> On Fri, Oct 11, 2024 at 02:02:03AM +0000, Wei Fang wrote:
> > > -----Original Message-----
> > > From: Frank Li <frank.li@nxp.com>
> > > Sent: 2024年10月10日 23:22
> > > To: Wei Fang <wei.fang@nxp.com>
> > > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> > > conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>;
> Claudiu
> > > Manoil <claudiu.manoil@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>;
> > > christophe.leroy@csgroup.eu; linux@armlinux.org.uk;
> bhelgaas@google.com;
> > > imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> > > Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> > > i.MX95 ENETC PF
> > >
> > > On Thu, Oct 10, 2024 at 04:59:45AM +0000, Wei Fang wrote:
> > > > > On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > > > > > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > > > > > different from the LS1028A ENETC (revision 1.0) except for the SI
> > > > > > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > > > > > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> > > > >             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> > > > > major revision 4.
> > > > >
> > > > > > PF, and this driver will be used to support the ENETC PF with
> > > > > > major revision 4 in the future.
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > index 97524dfa234c..7f1ea11c33a0 100644
> > > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > @@ -14,6 +14,7 @@
> > > > > >  #include <net/xdp.h>
> > > > > >
> > > > > >  #include "enetc_hw.h"
> > > > > > +#include "enetc4_hw.h"
> > > > > >
> > > > > >  #define ENETC_SI_ALIGN	32
> > > > > >
> > > > > > +static inline bool is_enetc_rev1(struct enetc_si *si) {
> > > > > > +	return si->pdev->revision == ENETC_REV1; }
> > > > > > +
> > > > > > +static inline bool is_enetc_rev4(struct enetc_si *si) {
> > > > > > +	return si->pdev->revision == ENETC_REV4; }
> > > > > > +
> > > > >
> > > > > Actually, I suggest you check features, instead of check version number.
> > > > >
> > > > This is mainly used to distinguish between ENETC v1 and ENETC v4 in
> > > > the general interfaces. See enetc_ethtool.c.
> > >
> > > Suggest use flags, such as, IS_SUPPORT_ETHTOOL.
> > >
> > > otherwise, your check may become complex in future.
> > >
> > > If use flags, you just change id table in future.
> >
> > enetc_ethtool just is an example, I meant that the ENETCv4 and ENETCv1
> > use some common drivers, like enect_pf_common, enetc-core, so different
> > hardware versions have different logic, that's all.
> 
> My means is that avoid use v1\v2 to distingiush it and use supported
> features in difference version for example:

I think you misunderstood what I meant. This is not to distinguish different
features supported by different versions of hardware. In fact, the functions
of the two versions of enetc are basically the same, but the hardware
implementations are different, so the code logic of two hardware is different.
Take 802.1 Qbu as an example. The enetc_set_mm() interface directly returns
"unsupported" for enetc v4, but this does not mean that enetc v4 is really not
supported. It's just that for the current patch, we have not added support yet.
And we will add the Qbu support for enetc v4 in the future and add some
different configurations for enetc v4.

> 
> ENETC_FEATURE_1, ENETC_FEATURE_2, ENETC_FEATURE_3,
> ENETC_FEATURE_4.
> 
> { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF)
>   .driver_data = ENETC_FEATURE_1 |  ENETC_FEATURE_2 |
> ENETC_FEATURE_4
>   PCI_DEVICE(....)
>   .driver_data = ENETC_FEATURE_1 | ENETC_FEATURE_3,
>   PCI_DEVICE(...)
>   .driver_data = ENETC_FEATURE_4,
> )
> 
> It will be easy to know the difference between difference version. Your if
> check logic will be simple.
> 
> if (driver_data & ENETC_FEATURE_1)
>   ....
> 
> otherwise
>    if (vers == 1 || vers == 2 || ver == 5), which distribute to difference
> places in whole code.
> 
> It is real hard to know hardware differences between version in future.
> 
> You can ref drivers/misc/pci_endpoint_test.c
> 
> Frank
> 
> >
> > >
> > > { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF),
> > >   .driver_data = IS_SUPPORT_ETHTOOL | .... },
> > >
> > > Frank
> > > >
> > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..e38ade76260b
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > > @@ -0,0 +1,761 @@
> > > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > > > > > +/* Copyright 2024 NXP */
> > > > > > +#include <linux/unaligned.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of_net.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/clk.h>
> > > > > > +#include <linux/pinctrl/consumer.h> #include
> > > > > > +<linux/fsl/netc_global.h>
> > > > >
> > > > > sort headers.
> > > > >
> > > >
> > > > Sure
> > > >
> > > > > > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > > > > > +			   const struct pci_device_id *ent) {
> > > > > > +	struct device *dev = &pdev->dev;
> > > > > > +	struct enetc_si *si;
> > > > > > +	struct enetc_pf *pf;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > > > > > +	if (err) {
> > > > > > +		dev_err(dev, "PCIe probing failed\n");
> > > > > > +		return err;
> > > > >
> > > > > use dev_err_probe()
> > > > >
> > > >
> > > > Okay
> > > >
> > > > > > +	}
> > > > > > +
> > > > > > +	/* si is the private data. */
> > > > > > +	si = pci_get_drvdata(pdev);
> > > > > > +	if (!si->hw.port || !si->hw.global) {
> > > > > > +		err = -ENODEV;
> > > > > > +		dev_err(dev, "Couldn't map PF only space!\n");
> > > > > > +		goto err_enetc_pci_probe;
> > > > > > +	}
> > > > > > +
> > > > > > +	err = enetc4_pf_struct_init(si);
> > > > > > +	if (err)
> > > > > > +		goto err_pf_struct_init;
> > > > > > +
> > > > > > +	pf = enetc_si_priv(si);
> > > > > > +	err = enetc4_pf_init(pf);
> > > > > > +	if (err)
> > > > > > +		goto err_pf_init;
> > > > > > +
> > > > > > +	pinctrl_pm_select_default_state(dev);
> > > > > > +	enetc_get_si_caps(si);
> > > > > > +	err = enetc4_pf_netdev_create(si);
> > > > > > +	if (err)
> > > > > > +		goto err_netdev_create;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > +err_netdev_create:
> > > > > > +err_pf_init:
> > > > > > +err_pf_struct_init:
> > > > > > +err_enetc_pci_probe:
> > > > > > +	enetc_pci_remove(pdev);
> > > > >
> > > > > you can use devm_add_action_or_reset() to remove these goto labels.
> > > > >
> > > > Subsequent patches will have corresponding processing for these
> > > > labels, so I don't want to add too many devm_add_action_or_reset ().
Wei Fang Oct. 11, 2024, 7:29 a.m. UTC | #16
> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年10月11日 12:13
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com;
> imx@lists.linux.dev; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 11/11] MAINTAINERS: update ENETC driver files
> and maintainers
> 
> On Fri, Oct 11, 2024 at 02:05:37AM +0000, Wei Fang wrote:
> > > -----Original Message-----
> > > From: Frank Li <frank.li@nxp.com>
> > > Sent: 2024年10月11日 3:21
> > > To: Wei Fang <wei.fang@nxp.com>
> > > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> > > conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>;
> > > conor+Claudiu
> > > Manoil <claudiu.manoil@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>;
> > > christophe.leroy@csgroup.eu; linux@armlinux.org.uk;
> > > bhelgaas@google.com; imx@lists.linux.dev; netdev@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-pci@vger.kernel.org
> > > Subject: Re: [PATCH net-next 11/11] MAINTAINERS: update ENETC driver
> > > files and maintainers
> > >
> > > On Wed, Oct 09, 2024 at 05:51:16PM +0800, Wei Fang wrote:
> > > > Add related YAML documentation and header files. Also, add
> > > > maintainers from the i.MX side as ENETC starts to be used on i.MX
> platforms.
> > > >
> > > > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > > > ---
> > > >  MAINTAINERS | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > af635dc60cfe..355b81b642a9 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -9015,9 +9015,18 @@ F:	drivers/dma/fsl-edma*.*
> > > >  FREESCALE ENETC ETHERNET DRIVERS
> > > >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> > > >  M:	Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > +M:	Wei Fang <wei.fang@nxp.com>
> > > > +M:	Clark Wang <xiaoning.wang@nxp.com>
> > > > +L:	imx@lists.linux.dev
> > > >  L:	netdev@vger.kernel.org
> > > >  S:	Maintained
> > > > +F:	Documentation/devicetree/bindings/net/fsl,enetc-ierb.yaml
> > > > +F:	Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> > > > +F:	Documentation/devicetree/bindings/net/fsl,enetc.yaml
> > > > +F:	Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> > >
> > > Missed enetc_pf_common.c
> > >
> > enetc_pf_common.c is in drivers/net/ethernet/freescale/enetc/
> 
> Oh, sorry, I miss it.
> Maybe
> 
> Documentation/devicetree/bindings/net/fsl,enetc*.yaml will be simple.
> 

Yes, I will refine it, thanks

> 
> >
> > >
> > > >  F:	drivers/net/ethernet/freescale/enetc/
> > > > +F:	include/linux/fsl/enetc_mdio.h
> > > > +F:	include/linux/fsl/netc_global.h
> > > >
> > > >  FREESCALE eTSEC ETHERNET DRIVER (GIANFAR)
> > > >  M:	Claudiu Manoil <claudiu.manoil@nxp.com>
> > > > --
> > > > 2.34.1
> > > >
Simon Horman Oct. 14, 2024, 9:06 a.m. UTC | #17
On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> different from the LS1028A ENETC (revision 1.0) except for the SI
> part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> PF, and this driver will be used to support the ENETC PF with major
> revision 4 in the future.
> 
> Currently, the nxp-enetc4 driver only supports basic transmission
> feature for i.MX95 ENETC PF, the more basic and advanced features
> will be added in the subsequent patches.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>

...

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

...

> +static void enetc4_pf_remove(struct pci_dev *pdev)
> +{
> +	struct enetc_si *si;
> +	struct enetc_pf *pf;
> +
> +	si = pci_get_drvdata(pdev);
> +	pf = enetc_si_priv(si);

Hi Wei Fang,

pf is set but otherwise unused in this function.  So I think that it, and
the call to enetc_si_priv() should probably be removed. They can be added
back if and when they are needed in subsequent patches.

> +
> +	enetc4_pf_netdev_destroy(si);
> +	enetc_pci_remove(pdev);
> +}

...
Wei Fang Oct. 14, 2024, 9:13 a.m. UTC | #18
> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: 2024年10月14日 17:07
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Vladimir Oltean <vladimir.oltean@nxp.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>;
> Frank Li <frank.li@nxp.com>; christophe.leroy@csgroup.eu;
> linux@armlinux.org.uk; bhelgaas@google.com; imx@lists.linux.dev;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> i.MX95 ENETC PF
> 
> On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > different from the LS1028A ENETC (revision 1.0) except for the SI
> > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> > PF, and this driver will be used to support the ENETC PF with major
> > revision 4 in the future.
> >
> > Currently, the nxp-enetc4 driver only supports basic transmission
> > feature for i.MX95 ENETC PF, the more basic and advanced features
> > will be added in the subsequent patches.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> 
> ...
> 
> > +static void enetc4_pf_remove(struct pci_dev *pdev)
> > +{
> > +	struct enetc_si *si;
> > +	struct enetc_pf *pf;
> > +
> > +	si = pci_get_drvdata(pdev);
> > +	pf = enetc_si_priv(si);
> 
> Hi Wei Fang,
> 
> pf is set but otherwise unused in this function.  So I think that it, and
> the call to enetc_si_priv() should probably be removed. They can be added
> back if and when they are needed in subsequent patches.
>

Thanks very much, I forgot to remove it when I copied from local branch. :(

> > +
> > +	enetc4_pf_netdev_destroy(si);
> > +	enetc_pci_remove(pdev);
> > +}
> 
> ...