[PATCH v1 0/5] ARM: Add GXP UMAC Support

[PATCH v1 0/5] ARM: Add GXP UMAC Support

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP contains two Ethernet MACs that can be
connected externally to several physical devices. From an external
interface perspective the BMC provides two SERDES interface connections
capable of either SGMII or 1000Base-X operation. The BMC also provides
a RMII interface for sideband connections to external Ethernet controllers.

The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
SERDES interface.  The secondary MAC (umac1) can be mapped to only
the second SGMII/1000-Base X Serdes interface or it can be mapped for
RMII sideband.

The MDIO(mdio0) interface from the primary MAC (umac0) is used for
external PHY status and configuration. The MDIO(mdio1) interface from
the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
on the two SERDES interface connections.

Nick Hawkins (5):
  dt-bindings: net: Add HPE GXP UMAC MDIO
  net: hpe: Add GXP UMAC MDIO
  dt-bindings: net: Add HPE GXP UMAC
  net: hpe: Add GXP UMAC Driver
  MAINTAINERS: HPE: Add GXP UMAC Networking Files

 .../bindings/net/hpe,gxp-umac-mdio.yaml       |  51 +
 .../devicetree/bindings/net/hpe,gxp-umac.yaml | 111 +++
 MAINTAINERS                                   |   3 +
 drivers/net/ethernet/Kconfig                  |   1 +
 drivers/net/ethernet/Makefile                 |   1 +
 drivers/net/ethernet/hpe/Kconfig              |  43 +
 drivers/net/ethernet/hpe/Makefile             |   2 +
 drivers/net/ethernet/hpe/gxp-umac-mdio.c      | 158 +++
 drivers/net/ethernet/hpe/gxp-umac.c           | 911 ++++++++++++++++++
 drivers/net/ethernet/hpe/gxp-umac.h           |  89 ++
 10 files changed, 1370 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
 create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
 create mode 100644 drivers/net/ethernet/hpe/Kconfig
 create mode 100644 drivers/net/ethernet/hpe/Makefile
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac-mdio.c
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac.c
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac.h

-- 
2.17.1

[PATCH v1 1/5] dt-bindings: net: Add HPE GXP UMAC MDIO

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

Provide access to the register regions and interrupt for Universal
MAC(UMAC). The driver under the hpe,gxp-umac-mdio will provide an
interface for managing both the internal and external PHYs.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../bindings/net/hpe,gxp-umac-mdio.yaml       | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml

diff --git a/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml b/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
new file mode 100644
index 000000000000..bb0db1bb67b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hpe,gxp-umac-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP UMAC MDIO Controller
+
+maintainers:
+  - Nicholas Hawkins <nick.hawkins@hpe.com>
+
+description: |+
+  The HPE GXP Unversal MAC (UMAC) MDIO controller provides a configuration
+  path for both external PHY's and SERDES connected PHY's.
+
+allOf:
+  - $ref: mdio.yaml#
+
+properties:
+  compatible:
+    const: hpe,gxp-umac-mdio
+
+  reg:
+    maxItems: 1
+    description: The register range of the MDIO controller instance
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio0: mdio@4080 {
+      compatible = "hpe,gxp-umac-mdio";
+      reg = <0x4080 0x10>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ethphy0: ethernet-phy@0 {
+        compatible = "ethernet-phy-ieee802.3-c22";
+        phy-mode = "sgmii";
+        reg = <0>;
+      };
+    };
-- 
2.17.1

Re: [PATCH v1 1/5] dt-bindings: net: Add HPE GXP UMAC MDIO

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

On Fri, Jul 21, 2023 at 04:20:40PM -0500, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide access to the register regions and interrupt for Universal
> MAC(UMAC). The driver under the hpe,gxp-umac-mdio will provide an
> interface for managing both the internal and external PHYs.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/net/hpe,gxp-umac-mdio.yaml       | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml b/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
> new file mode 100644
> index 000000000000..bb0db1bb67b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/hpe,gxp-umac-mdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP UMAC MDIO Controller
> +
> +maintainers:
> +  - Nicholas Hawkins <nick.hawkins@hpe.com>
> +
> +description: |+

You've got no formatting to preserve, so even | on its own would be
wasted here.

> +  The HPE GXP Unversal MAC (UMAC) MDIO controller provides a configuration
> +  path for both external PHY's and SERDES connected PHY's.
> +
> +allOf:
> +  - $ref: mdio.yaml#
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-umac-mdio
> +
> +  reg:
> +    maxItems: 1

> +    description: The register range of the MDIO controller instance

This is a statement of the obvious, no?

Otherwise, looks okay to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio0: mdio@4080 {
> +      compatible = "hpe,gxp-umac-mdio";
> +      reg = <0x4080 0x10>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ethphy0: ethernet-phy@0 {
> +        compatible = "ethernet-phy-ieee802.3-c22";
> +        phy-mode = "sgmii";
> +        reg = <0>;
> +      };
> +    };
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 2/5] net: hpe: Add GXP UMAC MDIO

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP contains two Universal Ethernet MACs that can be
connected externally to several physical devices. From an external
interface perspective the BMC provides two SERDES interface connections
capable of either SGMII or 1000Base-X operation. The BMC also provides
a RMII interface for sideband connections to external Ethernet controllers.

The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
SERDES interface.  The secondary MAC (umac1) can be mapped to only
the second SGMII/1000-Base X Serdes interface or it can be mapped for
RMII sideband.

The MDIO(mdio0) interface from the primary MAC (umac0) is used for
external PHY status and configuration. The MDIO(mdio1) interface from
the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
on the two SERDES interface connections.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/net/ethernet/Kconfig             |   1 +
 drivers/net/ethernet/Makefile            |   1 +
 drivers/net/ethernet/hpe/Kconfig         |  28 ++++
 drivers/net/ethernet/hpe/Makefile        |   1 +
 drivers/net/ethernet/hpe/gxp-umac-mdio.c | 158 +++++++++++++++++++++++
 5 files changed, 189 insertions(+)
 create mode 100644 drivers/net/ethernet/hpe/Kconfig
 create mode 100644 drivers/net/ethernet/hpe/Makefile
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac-mdio.c

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 5a274b99f299..b4921b84be51 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -80,6 +80,7 @@ source "drivers/net/ethernet/fujitsu/Kconfig"
 source "drivers/net/ethernet/fungible/Kconfig"
 source "drivers/net/ethernet/google/Kconfig"
 source "drivers/net/ethernet/hisilicon/Kconfig"
+source "drivers/net/ethernet/hpe/Kconfig"
 source "drivers/net/ethernet/huawei/Kconfig"
 source "drivers/net/ethernet/i825xx/Kconfig"
 source "drivers/net/ethernet/ibm/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 0d872d4efcd1..2e3cae9dbe97 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_NET_VENDOR_FREESCALE) += freescale/
 obj-$(CONFIG_NET_VENDOR_FUJITSU) += fujitsu/
 obj-$(CONFIG_NET_VENDOR_FUNGIBLE) += fungible/
 obj-$(CONFIG_NET_VENDOR_GOOGLE) += google/
+obj-$(CONFIG_NET_VENDOR_HPE) += hpe/
 obj-$(CONFIG_NET_VENDOR_HISILICON) += hisilicon/
 obj-$(CONFIG_NET_VENDOR_HUAWEI) += huawei/
 obj-$(CONFIG_NET_VENDOR_IBM) += ibm/
diff --git a/drivers/net/ethernet/hpe/Kconfig b/drivers/net/ethernet/hpe/Kconfig
new file mode 100644
index 000000000000..461aa15ace34
--- /dev/null
+++ b/drivers/net/ethernet/hpe/Kconfig
@@ -0,0 +1,28 @@
+config NET_VENDOR_HPE
+	bool "HPE device"
+	default y
+	depends on ARCH_HPE
+	help
+	  Say y here to support the HPE network devices.
+	  The GXP contains two Ethernet MACs that can be
+	  connected externally to several physical devices.
+	  From an external interface perspective the BMC
+	  provides two SERDES interface connections capable
+	  of either SGMII or 1000Base-X operation. The BMC
+	  also provides a RMII interface for sideband
+	  connections to external Ethernet controllers.
+
+if NET_VENDOR_HPE
+
+config GXP_UMAC_MDIO
+	tristate "GXP UMAC mdio support"
+	depends on ARCH_HPE
+	help
+	  Say y here to support the GXP UMAC MDIO bus. The
+	  MDIO(mdio0) interface from the primary MAC (umac0)
+	  is used for external PHY status and configuration.
+	  The MDIO(mdio1) interface from the secondary MAC
+	  (umac1) is routed to the SGMII/100Base-X IP blocks
+	  on the two SERDES interface connections.
+
+endif
diff --git a/drivers/net/ethernet/hpe/Makefile b/drivers/net/ethernet/hpe/Makefile
new file mode 100644
index 000000000000..e84c8786ba04
--- /dev/null
+++ b/drivers/net/ethernet/hpe/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_GXP_UMAC_MDIO) += gxp-umac-mdio.o
diff --git a/drivers/net/ethernet/hpe/gxp-umac-mdio.c b/drivers/net/ethernet/hpe/gxp-umac-mdio.c
new file mode 100644
index 000000000000..763e59185409
--- /dev/null
+++ b/drivers/net/ethernet/hpe/gxp-umac-mdio.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Development Company, L.P. */
+
+#include <linux/err.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/platform_device.h>
+
+#define UMAC_MII                0x00  /* R/W MII Register */
+#define UMAC_MII_PHY_ADDR_MASK  0x001F0000
+#define UMAC_MII_PHY_ADDR_SHIFT 16
+#define UMAC_MII_MOWNER         0x00000200
+#define UMAC_MII_MRNW           0x00000100
+#define UMAC_MII_REG_ADDR_MASK  0x0000001F
+#define UMAC_MII_DATA           0x04  /* R/W MII Data Register */
+
+struct umac_mdio_priv {
+	void __iomem *base;
+};
+
+static int umac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	struct umac_mdio_priv *umac_mdio = bus->priv;
+	unsigned int value;
+	unsigned int status;
+	int ret;
+
+	status = __raw_readl(umac_mdio->base + UMAC_MII);
+
+	status &= ~(UMAC_MII_PHY_ADDR_MASK | UMAC_MII_REG_ADDR_MASK);
+	status |= ((phy_id << UMAC_MII_PHY_ADDR_SHIFT) &
+			UMAC_MII_PHY_ADDR_MASK);
+	status |= (reg & UMAC_MII_REG_ADDR_MASK);
+	status |= UMAC_MII_MRNW; /* set bit for read mode */
+	__raw_writel(status, umac_mdio->base + UMAC_MII);
+
+	status |= UMAC_MII_MOWNER; /* set bit to activate mii transfer */
+	__raw_writel(status, umac_mdio->base + UMAC_MII);
+
+	ret = readl_poll_timeout(umac_mdio->base + UMAC_MII, status,
+				 !(status & UMAC_MII_MOWNER), 1000, 100000);
+	if (ret) {
+		dev_err(bus->parent, "mdio read time out\n");
+		return -ETIMEDOUT;
+	}
+
+	value = __raw_readl(umac_mdio->base + UMAC_MII_DATA);
+	return value;
+}
+
+static int umac_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 value)
+{
+	struct umac_mdio_priv *umac_mdio = bus->priv;
+	unsigned int status;
+	int ret;
+
+	__raw_writel(value, umac_mdio->base + UMAC_MII_DATA);
+
+	status = __raw_readl(umac_mdio->base + UMAC_MII);
+
+	status &= ~(UMAC_MII_PHY_ADDR_MASK | UMAC_MII_REG_ADDR_MASK);
+	status |= ((phy_id << UMAC_MII_PHY_ADDR_SHIFT) &
+			UMAC_MII_PHY_ADDR_MASK);
+	status |= (reg & UMAC_MII_REG_ADDR_MASK);
+	status &= ~UMAC_MII_MRNW; /* clear bit for write mode */
+	__raw_writel(status, umac_mdio->base + UMAC_MII);
+
+	status |= UMAC_MII_MOWNER; /* set bit to activate mii transfer */
+	__raw_writel(status, umac_mdio->base + UMAC_MII);
+
+	ret = readl_poll_timeout(umac_mdio->base + UMAC_MII, status,
+				 !(status & UMAC_MII_MOWNER), 1000, 100000);
+	if (ret) {
+		dev_err(bus->parent, "mdio read time out\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int umac_mdio_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct mii_bus *bus;
+	struct umac_mdio_priv *umac_mdio;
+
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "fail to get resource\n");
+		return -ENODEV;
+	}
+
+	bus = devm_mdiobus_alloc_size(&pdev->dev,
+				      sizeof(struct umac_mdio_priv));
+	if (!bus) {
+		dev_err(&pdev->dev, "failed to alloc mii bus\n");
+		return -ENOMEM;
+	}
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
+
+	bus->name	= dev_name(&pdev->dev);
+	bus->read	= umac_mdio_read,
+	bus->write	= umac_mdio_write,
+	bus->parent	= &pdev->dev;
+	umac_mdio = bus->priv;
+	umac_mdio->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!umac_mdio->base) {
+		dev_err(&pdev->dev, "failed to do ioremap\n");
+		return -ENODEV;
+	}
+
+	platform_set_drvdata(pdev, umac_mdio);
+
+	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int umac_mdio_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+
+	if (bus)
+		mdiobus_unregister(bus);
+
+	return 0;
+}
+
+static const struct of_device_id umac_mdio_of_matches[] = {
+	{ .compatible = "hpe,gxp-umac-mdio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, umac_mdio_of_matches);
+
+static struct platform_driver umac_driver = {
+	.driver	= {
+		.name    = "gxp-umac-mdio",
+		.of_match_table = of_match_ptr(umac_mdio_of_matches),
+	},
+	.probe   = umac_mdio_probe,
+	.remove  = umac_mdio_remove,
+};
+
+module_platform_driver(umac_driver);
+
+MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
+MODULE_DESCRIPTION("HPE GXP UMAC MDIO driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v1 2/5] net: hpe: Add GXP UMAC MDIO

[Christophe JAILLET]

Le 21/07/2023 à 23:20, nick.hawkins@hpe.com a écrit :
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP contains two Universal Ethernet MACs that can be
> connected externally to several physical devices. From an external
> interface perspective the BMC provides two SERDES interface connections
> capable of either SGMII or 1000Base-X operation. The BMC also provides
> a RMII interface for sideband connections to external Ethernet controllers.
> 
> The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
> SERDES interface.  The secondary MAC (umac1) can be mapped to only
> the second SGMII/1000-Base X Serdes interface or it can be mapped for
> RMII sideband.
> 
> The MDIO(mdio0) interface from the primary MAC (umac0) is used for
> external PHY status and configuration. The MDIO(mdio1) interface from
> the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
> on the two SERDES interface connections.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---

[...]

> +static int umac_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mii_bus *bus;
> +	struct umac_mdio_priv *umac_mdio;
> +
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "fail to get resource\n");
> +		return -ENODEV;
> +	}
> +
> +	bus = devm_mdiobus_alloc_size(&pdev->dev,
> +				      sizeof(struct umac_mdio_priv));
> +	if (!bus) {
> +		dev_err(&pdev->dev, "failed to alloc mii bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
> +
> +	bus->name	= dev_name(&pdev->dev);
> +	bus->read	= umac_mdio_read,
> +	bus->write	= umac_mdio_write,
> +	bus->parent	= &pdev->dev;
> +	umac_mdio = bus->priv;
> +	umac_mdio->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!umac_mdio->base) {
> +		dev_err(&pdev->dev, "failed to do ioremap\n");
> +		return -ENODEV;
> +	}
> +
> +	platform_set_drvdata(pdev, umac_mdio);
> +
> +	ret = of_mdiobus_register(bus, pdev->dev.of_node);

devm_of_mdiobus_register()?

This makes the platform_set_drvdata() just above and umac_mdio_remove() 
useless.

CJ

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int umac_mdio_remove(struct platform_device *pdev)
> +{
> +	struct mii_bus *bus = platform_get_drvdata(pdev);
> +
> +	if (bus)
> +		mdiobus_unregister(bus);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id umac_mdio_of_matches[] = {
> +	{ .compatible = "hpe,gxp-umac-mdio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, umac_mdio_of_matches);
> +
> +static struct platform_driver umac_driver = {
> +	.driver	= {
> +		.name    = "gxp-umac-mdio",
> +		.of_match_table = of_match_ptr(umac_mdio_of_matches),
> +	},
> +	.probe   = umac_mdio_probe,
> +	.remove  = umac_mdio_remove,
> +};
> +
> +module_platform_driver(umac_driver);
> +
> +MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
> +MODULE_DESCRIPTION("HPE GXP UMAC MDIO driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v1 2/5] net: hpe: Add GXP UMAC MDIO

[Christophe JAILLET]

Le 21/07/2023 à 23:20, nick.hawkins@hpe.com a écrit :
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP contains two Universal Ethernet MACs that can be
> connected externally to several physical devices. From an external
> interface perspective the BMC provides two SERDES interface connections
> capable of either SGMII or 1000Base-X operation. The BMC also provides
> a RMII interface for sideband connections to external Ethernet controllers.
> 
> The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
> SERDES interface.  The secondary MAC (umac1) can be mapped to only
> the second SGMII/1000-Base X Serdes interface or it can be mapped for
> RMII sideband.
> 
> The MDIO(mdio0) interface from the primary MAC (umac0) is used for
> external PHY status and configuration. The MDIO(mdio1) interface from
> the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
> on the two SERDES interface connections.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---

[...]

> +static int umac_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mii_bus *bus;
> +	struct umac_mdio_priv *umac_mdio;
> +
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "fail to get resource\n");
> +		return -ENODEV;
> +	}
> +
> +	bus = devm_mdiobus_alloc_size(&pdev->dev,
> +				      sizeof(struct umac_mdio_priv));
> +	if (!bus) {
> +		dev_err(&pdev->dev, "failed to alloc mii bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
> +
> +	bus->name	= dev_name(&pdev->dev);
> +	bus->read	= umac_mdio_read,
> +	bus->write	= umac_mdio_write,
> +	bus->parent	= &pdev->dev;
> +	umac_mdio = bus->priv;
> +	umac_mdio->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!umac_mdio->base) {
> +		dev_err(&pdev->dev, "failed to do ioremap\n");
> +		return -ENODEV;
> +	}
> +
> +	platform_set_drvdata(pdev, umac_mdio);
> +
> +	ret = of_mdiobus_register(bus, pdev->dev.of_node);

devm_of_mdiobus_register()?

This makes the platform_set_drvdata() just above and umac_mdio_remove() 
useless.

CJ

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int umac_mdio_remove(struct platform_device *pdev)
> +{
> +	struct mii_bus *bus = platform_get_drvdata(pdev);
> +
> +	if (bus)
> +		mdiobus_unregister(bus);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id umac_mdio_of_matches[] = {
> +	{ .compatible = "hpe,gxp-umac-mdio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, umac_mdio_of_matches);
> +
> +static struct platform_driver umac_driver = {
> +	.driver	= {
> +		.name    = "gxp-umac-mdio",
> +		.of_match_table = of_match_ptr(umac_mdio_of_matches),
> +	},
> +	.probe   = umac_mdio_probe,
> +	.remove  = umac_mdio_remove,
> +};
> +
> +module_platform_driver(umac_driver);
> +
> +MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
> +MODULE_DESCRIPTION("HPE GXP UMAC MDIO driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v1 2/5] net: hpe: Add GXP UMAC MDIO

[Andrew Lunn]

>  drivers/net/ethernet/hpe/gxp-umac-mdio.c | 158 +++++++++++++++++++++++

This looks to be a standalone MDIO driver. So please move it into
drivers/net/mdio.

> +config NET_VENDOR_HPE
> +	bool "HPE device"
> +	default y
> +	depends on ARCH_HPE

Please add || COMPILE_TEST


> +	help
> +	  Say y here to support the HPE network devices.
> +	  The GXP contains two Ethernet MACs that can be
> +	  connected externally to several physical devices.
> +	  From an external interface perspective the BMC
> +	  provides two SERDES interface connections capable
> +	  of either SGMII or 1000Base-X operation. The BMC
> +	  also provides a RMII interface for sideband
> +	  connections to external Ethernet controllers.
> +
> +if NET_VENDOR_HPE
> +
> +config GXP_UMAC_MDIO
> +	tristate "GXP UMAC mdio support"
> +	depends on ARCH_HPE

You probably also need

        depends on OF_MDIO && HAS_IOMEM
        depends on MDIO_DEVRES

> +static int umac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +{
> +	struct umac_mdio_priv *umac_mdio = bus->priv;
> +	unsigned int value;
> +	unsigned int status;
> +	int ret;

Networking uses reverse christmas tree. Please sort these longest to
shorted.

...

> +	ret = readl_poll_timeout(umac_mdio->base + UMAC_MII, status,
> +				 !(status & UMAC_MII_MOWNER), 1000, 100000);
> +	if (ret) {
> +		dev_err(bus->parent, "mdio read time out\n");
> +		return -ETIMEDOUT;

return ret;

Don't transform error codes.

> +static int umac_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 value)
> +{
> +	struct umac_mdio_priv *umac_mdio = bus->priv;
> +	unsigned int status;
> +	int ret;
> +	ret = readl_poll_timeout(umac_mdio->base + UMAC_MII, status,
> +				 !(status & UMAC_MII_MOWNER), 1000, 100000);
> +	if (ret) {
> +		dev_err(bus->parent, "mdio read time out\n");
> +		return -ETIMEDOUT;
> +	}

You can simplify this, do a dev_err() inside an if, and then
unconditionally return ret;

> +static int umac_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mii_bus *bus;
> +	struct umac_mdio_priv *umac_mdio;
> +
> +	int ret;


More sorting needed.

And no blank lines please.

    Andrew

Re: [PATCH v1 2/5] net: hpe: Add GXP UMAC MDIO

[Simon Horman]

On Fri, Jul 21, 2023 at 04:20:41PM -0500, nick.hawkins@hpe.com wrote:

...

Hi Nick,

> +static int umac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +{
> +	struct umac_mdio_priv *umac_mdio = bus->priv;
> +	unsigned int value;
> +	unsigned int status;

Please use reverse xmas tree - longest line to shortest - for
local variable declarations in new Networking code.

...

> +static int umac_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mii_bus *bus;
> +	struct umac_mdio_priv *umac_mdio;

Ditto.

> +
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "fail to get resource\n");
> +		return -ENODEV;
> +	}
> +
> +	bus = devm_mdiobus_alloc_size(&pdev->dev,
> +				      sizeof(struct umac_mdio_priv));
> +	if (!bus) {
> +		dev_err(&pdev->dev, "failed to alloc mii bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(&pdev->dev));
> +
> +	bus->name	= dev_name(&pdev->dev);
> +	bus->read	= umac_mdio_read,
> +	bus->write	= umac_mdio_write,

Should the comas (',') on the two lines above be semicolons (';') ?

> +	bus->parent	= &pdev->dev;
> +	umac_mdio = bus->priv;
> +	umac_mdio->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!umac_mdio->base) {
> +		dev_err(&pdev->dev, "failed to do ioremap\n");
> +		return -ENODEV;
> +	}
> +
> +	platform_set_drvdata(pdev, umac_mdio);
> +
> +	ret = of_mdiobus_register(bus, pdev->dev.of_node);
> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

...

[PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

Provide access to the register regions and interrupt for Universal
MAC(UMAC). The driver under the hpe,gxp-umac binding will provide an
interface for sending and receiving networking data from both of the
UMACs on the system.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../devicetree/bindings/net/hpe,gxp-umac.yaml | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml

diff --git a/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml b/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
new file mode 100644
index 000000000000..c3b68c4ba7f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hpe,gxp-umac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP Unified MAC Controller
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+
+description: |
+  HPE GXP 802.3 10/100/1000T Ethernet Unifed MAC controller.
+  Device node of the controller has following properties.
+
+properties:
+  compatible:
+    const: hpe,gxp-umac
+
+  use-ncsi:
+    type: boolean
+    description: |
+      Indicates if the device should use NCSI (Network Controlled
+      Sideband Interface).
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  mac-address: true
+
+  ethernet-ports:
+    type: object
+    additionalProperties: false
+    description: Ethernet ports to PHY
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      "^port@[0-1]$":
+        type: object
+        additionalProperties: false
+        description: Port to PHY
+
+        properties:
+          reg:
+            minimum: 0
+            maximum: 1
+
+          phy-handle:
+            maxItems: 1
+
+        required:
+          - reg
+          - phy-handle
+
+  mdio:
+    $ref: mdio.yaml#
+    unevaluatedProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - ethernet-ports
+
+examples:
+  - |
+    ethernet@4000 {
+      compatible = "hpe,gxp-umac";
+      reg = <0x4000 0x80>;
+      interrupts = <22>;
+      mac-address = [00 00 00 00 00 00]; /* Filled in by U-Boot */
+      ethernet-ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+          phy-handle = <&eth_phy0>;
+        };
+
+        port@1 {
+          reg = <1>;
+          phy-handle = <&eth_phy1>;
+        };
+      };
+
+      mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        eth_phy0: ethernet-phy@0 {
+          reg = <0>;
+        };
+
+        eth_phy1: ethernet-phy@1 {
+          reg = <1>;
+        };
+      };
+    };
+...
-- 
2.17.1

Re: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Andrew Lunn]

> +examples:
> +  - |
> +    ethernet@4000 {
> +      compatible = "hpe,gxp-umac";
> +      reg = <0x4000 0x80>;
> +      interrupts = <22>;
> +      mac-address = [00 00 00 00 00 00]; /* Filled in by U-Boot */

Do both ports get the sane MAC address? 

> +      ethernet-ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          phy-handle = <&eth_phy0>;
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +          phy-handle = <&eth_phy1>;
> +        };
> +      };
> +
> +      mdio {

This seems to be wrong. You have a standalone MDIO bus driver, not
part of the MAC address space?

	Andrew

RE: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Hawkins, Nick]

Hi Andrew,

Thank you for the feedback.

> > +examples:
> > +  - |
> > +    ethernet@4000 {
> > +      compatible = "hpe,gxp-umac";
> > +      reg = <0x4000 0x80>;
> > +      interrupts = <22>;
> > +      mac-address = [00 00 00 00 00 00]; /* Filled in by U-Boot */

> Do both ports get the sane MAC address?

No they do not. The first one will get the MAC address, the second
will be an external phy we are managing via the MDIO path.

> > +      ethernet-ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +          phy-handle = <&eth_phy0>;
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +          phy-handle = <&eth_phy1>;
> > +        };
> > +      };
> > +
> > +      mdio {

> This seems to be wrong. You have a standalone MDIO bus driver, not
> part of the MAC address space?

I based this from other yaml examples I found. Is there a better way to
represent it?

Here is what I plan on having the dts/dtsi
look like:

mdio0: mdio@4080 {
	compatible = "hpe,gxp-umac-mdio";
	reg = <0x4080 0x10>;
	#address-cells = <1>;
	#size-cells = <0>;
	ext_phy0: ethernt-phy@0 {
		compatible = "marvell,88e1415","ethernet-phy-ieee802.3-c22";
		phy-mode = "sgmii";
		reg = <0>;
	};
};

mdio1: mdio@5080 {
	compatible = "hpe,gxp-umac-mdio";
	reg = <0x5080 0x10>;
	#address-cells = <1>;
	#size-cells = <0>;
	int_phy0: ethernt-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		phy-mode = "gmii";
                             reg = <0>;
	};

	int_phy1: ethernt-phy@1 {
		compatible = "ethernet-phy-ieee802.3-c22";
		phy-mode = "gmii";
		reg = <1>;
	};
};

umac0: ethernet@4000 {
	compatible = "hpe,gxp-umac";
	reg = <0x4000 0x80>;
	interrupts = <10>;
	interrupt-parent = <&vic0>;
	mac-address = [00 00 00 00 00 00];
	ethernet-ports {
		#address-cells = <1>;
		#size-cells = <0>;
		port@0 {
			reg = <0>;
			phy-handle = <&int_phy0>;
		};
		port@1 {
			reg = <1>;
			phy-handle = <&ext_phy0>;
		};
	};
};

Thank you for the help and assistance.

-Nick Hawkins

Re: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Andrew Lunn]

On Tue, Jul 25, 2023 at 01:44:30PM +0000, Hawkins, Nick wrote:
> Hi Andrew,
> 
> Thank you for the feedback.
> 
> > > +examples:
> > > +  - |
> > > +    ethernet@4000 {
> > > +      compatible = "hpe,gxp-umac";
> > > +      reg = <0x4000 0x80>;
> > > +      interrupts = <22>;
> > > +      mac-address = [00 00 00 00 00 00]; /* Filled in by U-Boot */
> 
> > Do both ports get the sane MAC address?
> 
> No they do not. The first one will get the MAC address, the second
> will be an external phy we are managing via the MDIO path.

Then please put the mac-address property in the correct place, inside
port@0.

> > > +      ethernet-ports {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        port@0 {
> > > +          reg = <0>;
> > > +          phy-handle = <&eth_phy0>;
> > > +        };

> > > +      mdio {
> 
> > This seems to be wrong. You have a standalone MDIO bus driver, not
> > part of the MAC address space?
> 
> I based this from other yaml examples I found. Is there a better way to
> represent it?

The validator when given examples does not validate phy-handle
actually points to a known node. So you can just leave the mdio bus
out all together.

> mdio0: mdio@4080 {
> 	compatible = "hpe,gxp-umac-mdio";
> 	reg = <0x4080 0x10>;
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	ext_phy0: ethernt-phy@0 {
> 		compatible = "marvell,88e1415","ethernet-phy-ieee802.3-c22";

which is wrong. Please read the binding document for PHYs.

      Andrew

Re: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Hawkins, Nick]

Note: Resend in correct format.

> > > Do both ports get the sane MAC address?
> > 
> > No they do not. The first one will get the MAC address, the second
> > will be an external phy we are managing via the MDIO path.

> Then please put the mac-address property in the correct place, inside
port@0.

Greetings Andrew,

I was mistaken, the Mac address belongs with UMAC,
not the phys. The reason ports are listed here is
because having two separate phy-handles
in one node is not allowed. The layout of the
hardware inside the GXP is unconventional.

There is a description of the layout in the cover-letter,
I see though I need to add a better description.
The internal-phy is connected to the external
phy via SGMII. To use UMAC0 we need to
configure both the internal, and external phy to enable
networking.

Ideally it would be something like this:

umac0: umac@4000 {
        compatible = "hpe, gxp-umac";
         reg = <0x4000 0x80>;
        interrupts = <10>;
        interrupt-parent = <&vic0>;
        mac-address = [94 18 82 16 04 d8];
        ext-phy-handle = <&ext_phy0>;
        int-phy-handle = <&int_phy0>;
};

Thanks,

-Nick Hawkins

Re: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Andrew Lunn]

On Wed, Jul 26, 2023 at 02:35:48PM +0000, Hawkins, Nick wrote:
> > > > Do both ports get the sane MAC address?
> > >
> > > No they do not. The first one will get the MAC address, the second
> > > will be an external phy we are managing via the MDIO path.
> 
> > Then please put the mac-address property in the correct place, inside
> port@0.
> 
> Greetings Andrew,
> 
> I was mistaken, the Mac address belongs with UMAC,
> not the phys. The reason ports are listed here is
> because having two separate phy-handles
> in one node is not allowed. The layout of the
> hardware inside the GXP is unconventional.

It is not that unconventional. See

Documentation/devicetree/bindings/net/marvell-orion-net.txt

This is an Ethernet block, with two MACs inside it. Each MAC gets its
own subblock containing MAC specific properties like the MAC address,
phy-handle, etc.

	    Andrew

Re: [PATCH v1 3/5] dt-bindings: net: Add HPE GXP UMAC

[Rob Herring]

On Fri, Jul 21, 2023 at 04:20:42PM -0500, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide access to the register regions and interrupt for Universal
> MAC(UMAC). The driver under the hpe,gxp-umac binding will provide an
> interface for sending and receiving networking data from both of the
> UMACs on the system.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../devicetree/bindings/net/hpe,gxp-umac.yaml | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml b/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
> new file mode 100644
> index 000000000000..c3b68c4ba7f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/hpe,gxp-umac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Unified MAC Controller
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +description: |

Don't need '|' if no formatting to maintain. Here and elsewhere.

> +  HPE GXP 802.3 10/100/1000T Ethernet Unifed MAC controller.
> +  Device node of the controller has following properties.
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-umac
> +
> +  use-ncsi:
> +    type: boolean
> +    description: |
> +      Indicates if the device should use NCSI (Network Controlled
> +      Sideband Interface).
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  mac-address: true
> +
> +  ethernet-ports:
> +    type: object
> +    additionalProperties: false
> +    description: Ethernet ports to PHY
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      "^port@[0-1]$":
> +        type: object
> +        additionalProperties: false
> +        description: Port to PHY
> +
> +        properties:
> +          reg:
> +            minimum: 0
> +            maximum: 1
> +
> +          phy-handle:
> +            maxItems: 1
> +
> +        required:
> +          - reg
> +          - phy-handle
> +
> +  mdio:
> +    $ref: mdio.yaml#
> +    unevaluatedProperties: false
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - ethernet-ports
> +
> +examples:
> +  - |
> +    ethernet@4000 {
> +      compatible = "hpe,gxp-umac";
> +      reg = <0x4000 0x80>;
> +      interrupts = <22>;
> +      mac-address = [00 00 00 00 00 00]; /* Filled in by U-Boot */
> +      ethernet-ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          phy-handle = <&eth_phy0>;
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +          phy-handle = <&eth_phy1>;
> +        };
> +      };
> +
> +      mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        eth_phy0: ethernet-phy@0 {
> +          reg = <0>;
> +        };
> +
> +        eth_phy1: ethernet-phy@1 {
> +          reg = <1>;
> +        };
> +      };
> +    };
> +...
> -- 
> 2.17.1
> 

[PATCH v1 4/5] net: hpe: Add GXP UMAC Driver

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP contains two Ethernet MACs that can be connected externally
to several physical devices. From an external interface perspective
the BMC provides two SERDES interface connections capable of either
SGMII or 1000Base-X operation. The BMC also provides a RMII interface
for sideband connections to external Ethernet controllers.

The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
SERDES interface.  The secondary MAC (umac1) can be mapped to only
the second SGMII/1000-Base X Serdes interface or it can be mapped for
RMII sideband.

The MDIO(mdio0) interface from the primary MAC (umac0) is used for
external PHY status and configuration. The MDIO(mdio1) interface from
the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
on the two SERDES interface connections.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/net/ethernet/hpe/Kconfig    |  15 +
 drivers/net/ethernet/hpe/Makefile   |   1 +
 drivers/net/ethernet/hpe/gxp-umac.c | 911 ++++++++++++++++++++++++++++
 drivers/net/ethernet/hpe/gxp-umac.h |  89 +++
 4 files changed, 1016 insertions(+)
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac.c
 create mode 100644 drivers/net/ethernet/hpe/gxp-umac.h

diff --git a/drivers/net/ethernet/hpe/Kconfig b/drivers/net/ethernet/hpe/Kconfig
index 461aa15ace34..cc269558b707 100644
--- a/drivers/net/ethernet/hpe/Kconfig
+++ b/drivers/net/ethernet/hpe/Kconfig
@@ -14,6 +14,21 @@ config NET_VENDOR_HPE
 
 if NET_VENDOR_HPE
 
+config GXP_UMAC
+	tristate "GXP UMAC support"
+	depends on ARCH_HPE
+	select CRC32
+	select MII
+	select PHYLIB
+	select GXP_UMAC_MDIO
+	help
+	  Say y here to support the GXP UMACs interface. The
+	  primary MAC (umac0) can be mapped to either
+	  SGMII/1000-BaseX SERDES interface. The secondary MAC
+	  (umac1) can be mapped to only the second
+	  SGMII/1000-Base X Serdes interface or it can be
+	  mapped for RMII sideband.
+
 config GXP_UMAC_MDIO
 	tristate "GXP UMAC mdio support"
 	depends on ARCH_HPE
diff --git a/drivers/net/ethernet/hpe/Makefile b/drivers/net/ethernet/hpe/Makefile
index e84c8786ba04..5c2c3bcde532 100644
--- a/drivers/net/ethernet/hpe/Makefile
+++ b/drivers/net/ethernet/hpe/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_GXP_UMAC_MDIO) += gxp-umac-mdio.o
+obj-$(CONFIG_GXP_UMAC) += gxp-umac.o
diff --git a/drivers/net/ethernet/hpe/gxp-umac.c b/drivers/net/ethernet/hpe/gxp-umac.c
new file mode 100644
index 000000000000..a4fe3eb9f68f
--- /dev/null
+++ b/drivers/net/ethernet/hpe/gxp-umac.c
@@ -0,0 +1,911 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/dma-mapping.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <net/ncsi.h>
+#include <linux/of_device.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include "gxp-umac.h"
+
+#define PHY_88E1514_COPPER_CONTROL_REG		0
+#define PHY_88E1514_PAGE_ADDRESS		22
+
+#define PHY_88E1514_GENERAL_CONTROL_REG1	20
+
+#define DRV_MODULE_NAME		"gxp-umac"
+#define DRV_MODULE_VERSION	"0.1"
+
+#define NUMBER_OF_PORTS 2
+#define EXTERNAL_PORT 1
+#define INTERNAL_PORT 0
+
+struct umac_priv {
+	void __iomem *base;
+	int irq;
+	struct platform_device *pdev;
+	struct umac_tx_descs *tx_descs;
+	struct umac_rx_descs *rx_descs;
+	dma_addr_t tx_descs_dma_addr;
+	dma_addr_t rx_descs_dma_addr;
+	unsigned int tx_cur;
+	unsigned int tx_done;
+	unsigned int rx_cur;
+	struct napi_struct napi;
+	struct net_device *ndev;
+	struct phy_device *phy_dev;
+	struct phy_device *int_phy_dev;
+	struct ncsi_dev *ncsidev;
+	bool use_ncsi;
+};
+
+static void umac_get_drvinfo(struct net_device *ndev,
+			     struct ethtool_drvinfo *info)
+{
+	strscpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
+	strscpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
+}
+
+static int umac_get_link_ksettings(struct net_device *ndev,
+				   struct ethtool_link_ksettings *cmd)
+{
+	phy_ethtool_ksettings_get(ndev->phydev, cmd);
+	return 0;
+}
+
+static int umac_set_link_ksettings(struct net_device *ndev,
+				   const struct ethtool_link_ksettings *cmd)
+{
+	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
+}
+
+static int umac_nway_reset(struct net_device *ndev)
+{
+	return genphy_restart_aneg(ndev->phydev);
+}
+
+static u32 umac_get_link(struct net_device *ndev)
+{
+	int err;
+
+	err = genphy_update_link(ndev->phydev);
+	if (err)
+		return ethtool_op_get_link(ndev);
+
+	return ndev->phydev->link;
+}
+
+static struct net_device_stats *umac_get_stats(struct net_device *ndev)
+{
+	return &ndev->stats;
+}
+
+static int umac_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
+{
+	if (!netif_running(ndev))
+		return -EINVAL;
+
+	if (!ndev->phydev)
+		return -ENODEV;
+
+	return phy_mii_ioctl(ndev->phydev, ifr, cmd);
+}
+
+static void umac_set_mac_address(struct net_device *ndev, void *p_addr)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	char *addr = (char *)p_addr;
+	unsigned int value;
+
+	/* update address to register */
+	value = addr[0] << 8 | addr[1];
+	writel(value, umac->base + UMAC_MAC_ADDR_HI);
+	value = addr[2] << 8 | addr[3];
+	writel(value, umac->base + UMAC_MAC_ADDR_MID);
+	value = addr[4] << 8 | addr[5];
+	writel(value, umac->base + UMAC_MAC_ADDR_LO);
+}
+
+static int umac_eth_mac_addr(struct net_device *ndev, void *p)
+{
+	int ret;
+	struct sockaddr *addr = p;
+
+	ret = eth_prepare_mac_addr_change(ndev, p);
+	if (ret < 0)
+		return ret;
+
+	eth_commit_mac_addr_change(ndev, p);
+	umac_set_mac_address(ndev, addr->sa_data);
+
+	return 0;
+}
+
+static void umac_channel_enable(struct umac_priv *umac)
+{
+	unsigned int value;
+
+	value = readl(umac->base + UMAC_CONFIG_STATUS);
+	value |= UMAC_CFG_TXEN | UMAC_CFG_RXEN;
+	writel(value, umac->base + UMAC_CONFIG_STATUS);
+
+	/* start processing by writing the ring prompt register */
+	writel(0, umac->base + UMAC_RING_PROMPT);
+}
+
+static void umac_channel_disable(struct umac_priv *umac)
+{
+	writel(0, umac->base + UMAC_CONFIG_STATUS);
+}
+
+static int umac_init_ring_discriptor(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	struct platform_device *pdev = umac->pdev;
+
+	struct umac_tx_desc_entry *ptxdesc;
+	struct umac_rx_desc_entry *prxdesc;
+
+	unsigned int i;
+
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) {
+		netdev_err(ndev, "No suitable DMA available\n");
+		return -ENOMEM;
+	}
+
+	umac->tx_descs = dma_alloc_coherent(&pdev->dev,
+					    sizeof(struct umac_tx_descs),
+					&umac->tx_descs_dma_addr, GFP_KERNEL);
+	if (!umac->tx_descs)
+		return -ENOMEM;
+
+	umac->rx_descs = dma_alloc_coherent(&pdev->dev,
+					    sizeof(struct umac_rx_descs),
+					&umac->rx_descs_dma_addr, GFP_KERNEL);
+	if (!umac->rx_descs) {
+		dma_free_coherent(&pdev->dev, sizeof(struct umac_tx_descs),
+				  umac->tx_descs,
+					umac->tx_descs_dma_addr);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < UMAC_MAX_TX_DESC_ENTRIES; i++) {
+		ptxdesc = &umac->tx_descs->entrylist[i];
+		ptxdesc->dmaaddress = cpu_to_le32(umac->tx_descs_dma_addr +
+						offsetof(struct umac_tx_descs,
+							 framelist[i][0]));
+	}
+
+	for (i = 0; i < UMAC_MAX_RX_DESC_ENTRIES; i++) {
+		prxdesc = &umac->rx_descs->entrylist[i];
+		prxdesc->dmaaddress = cpu_to_le32(umac->rx_descs_dma_addr +
+						offsetof(struct umac_rx_descs,
+							 framelist[i][0]));
+		prxdesc->status = UMAC_RING_ENTRY_HW_OWN;
+		prxdesc->count = UMAC_MAX_RX_FRAME_SIZE;
+	}
+
+	umac->tx_cur = 0;
+	umac->tx_done = 0;
+	umac->rx_cur = 0;
+
+	return 0;
+}
+
+static int umac_int_phy_init(struct umac_priv *umac)
+{
+	struct phy_device *phy_dev = umac->int_phy_dev;
+	unsigned int value;
+
+	value = phy_read(phy_dev, 0);
+	if (value & 0x4000)
+		pr_info("Internal PHY loopback is enabled - clearing\n");
+
+	value &= ~0x4000; /* disable loopback */
+	phy_write(phy_dev, 0, value);
+
+	value = phy_read(phy_dev, 0);
+	value |= 0x1000; /* set aneg enable */
+	value |= 0x8000; /* SW reset */
+	phy_write(phy_dev, 0, value);
+
+	do {
+		value = phy_read(phy_dev, 0);
+	} while (value & 0x8000);
+
+	return 0;
+}
+
+static int umac_phy_fixup(struct phy_device *phy_dev)
+{
+	unsigned int value;
+
+	/* set phy mode to SGMII to copper */
+	/* set page to 18 by writing 18 to register 22 */
+	phy_write(phy_dev, PHY_88E1514_PAGE_ADDRESS, 18);
+	value = phy_read(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1);
+	value &= ~0x07;
+	value |= 0x01;
+	phy_write(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1, value);
+
+	/* perform mode reset by setting bit 15 in general_control_reg1 */
+	phy_write(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1, value | 0x8000);
+
+	do {
+		value = phy_read(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1);
+	} while (value & 0x8000);
+
+	/* after setting the mode, must perform a SW reset */
+	phy_write(phy_dev, PHY_88E1514_PAGE_ADDRESS, 0); /* set page to 0 */
+
+	value = phy_read(phy_dev, PHY_88E1514_COPPER_CONTROL_REG);
+	value |= 0x8000;
+	phy_write(phy_dev, PHY_88E1514_COPPER_CONTROL_REG, value);
+
+	do {
+		value = phy_read(phy_dev, PHY_88E1514_COPPER_CONTROL_REG);
+	} while (value & 0x8000);
+
+	return 0;
+}
+
+static int umac_init_hw(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	unsigned int value;
+
+	/* initialize tx and rx rings to first entry */
+	writel(0, umac->base + UMAC_RING_PTR);
+
+	/* clear the missed bit */
+	writel(0, umac->base + UMAC_CLEAR_STATUS);
+
+	/* disable checksum generation */
+	writel(0, umac->base + UMAC_CKSUM_CONFIG);
+
+	/* write the ring size register */
+	value = ((UMAC_RING_SIZE_256 << UMAC_TX_RING_SIZE_SHIFT) &
+			UMAC_TX_RING_SIZE_MASK) |
+		((UMAC_RING_SIZE_256 << UMAC_RX_RING_SIZE_SHIFT) &
+			UMAC_RX_RING_SIZE_MASK);
+	writel(value, umac->base + UMAC_RING_SIZE);
+
+	/* write rx ring base address */
+	writel(cpu_to_le32(umac->rx_descs_dma_addr),
+	       umac->base + UMAC_RX_RING_ADDR);
+
+	/* write tx ring base address */
+	writel(cpu_to_le32(umac->tx_descs_dma_addr),
+	       umac->base + UMAC_TX_RING_ADDR);
+
+	/* write burst size */
+	writel(0x22, umac->base + UMAC_DMA_CONFIG);
+
+	umac_channel_disable(umac);
+
+	/* disable clocks and gigabit mode (leave channels disabled) */
+	value = readl(umac->base + UMAC_CONFIG_STATUS);
+	value &= 0xfffff9ff;
+	writel(value, umac->base + UMAC_CONFIG_STATUS);
+	udelay(2);
+
+	if (umac->use_ncsi) {
+		/* set correct tx clock */
+		value &= UMAC_CFG_TX_CLK_EN;
+		value &= ~UMAC_CFG_GTX_CLK_EN;
+		value &= ~UMAC_CFG_GIGABIT_MODE; /* RMII mode */
+		value |= UMAC_CFG_FULL_DUPLEX; /* full duplex */
+	} else {
+		if (ndev->phydev->duplex)
+			value |= UMAC_CFG_FULL_DUPLEX;
+		else
+			value &= ~UMAC_CFG_FULL_DUPLEX;
+
+		if (ndev->phydev->speed == SPEED_1000) {
+			value &= ~UMAC_CFG_TX_CLK_EN;
+			value |= UMAC_CFG_GTX_CLK_EN;
+			value |= UMAC_CFG_GIGABIT_MODE;
+		} else {
+			value |= UMAC_CFG_TX_CLK_EN;
+			value &= ~UMAC_CFG_GTX_CLK_EN;
+			value &= ~UMAC_CFG_GIGABIT_MODE;
+		}
+	}
+	writel(value, umac->base + UMAC_CONFIG_STATUS);
+	udelay(2);
+
+	umac_channel_enable(umac);
+
+	return 0;
+}
+
+static int umac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	struct umac_tx_desc_entry *ptxdesc;
+	u8 *pframe;
+	unsigned int length;
+
+	ptxdesc = &umac->tx_descs->entrylist[umac->tx_cur];
+	pframe = umac->tx_descs->framelist[umac->tx_cur];
+
+	length = skb->len;
+	if (length > 1514) {
+		netdev_err(ndev, "send data %d bytes > 1514, clamp it to 1514\n",
+			   skb->len);
+		length = 1514;
+	}
+
+	memset(pframe, 0, UMAC_MAX_FRAME_SIZE);
+	memcpy(pframe, skb->data, length);
+
+	if (length < ETH_ZLEN)
+		length = ETH_ZLEN; /* minimum tx byte */
+
+	ptxdesc->count = length;
+	ptxdesc->status = UMAC_RING_ENTRY_HW_OWN;
+	ptxdesc->cksumoffset = 0; /* disable checksum generation */
+
+	umac->tx_cur++;
+	if (umac->tx_cur >= UMAC_MAX_TX_DESC_ENTRIES)
+		umac->tx_cur = 0;
+
+	/* if current tx ring buffer is full, stop the queue */
+	ptxdesc = &umac->tx_descs->entrylist[umac->tx_cur];
+	if (ptxdesc->status & UMAC_RING_ENTRY_HW_OWN)
+		netif_stop_queue(ndev);
+
+	/* start processing by writing the ring prompt register */
+	writel(0, umac->base + UMAC_RING_PROMPT);
+	dev_kfree_skb(skb);
+
+	return NETDEV_TX_OK;
+}
+
+static int umac_rx(struct net_device *ndev, int budget)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+
+	struct umac_rx_desc_entry *prxdesc;
+	struct sk_buff *skb;
+
+	unsigned int rxlength;
+	int rxpktcount = 0;
+	u8 *pframe;
+	u8 *skb_buf;
+
+	prxdesc = &umac->rx_descs->entrylist[umac->rx_cur];
+	pframe = umac->rx_descs->framelist[umac->rx_cur];
+
+	while (!(prxdesc->status & UMAC_RING_ENTRY_HW_OWN)) {
+		rxlength = prxdesc->count;
+		skb = netdev_alloc_skb(ndev, rxlength);
+		if (!skb) {
+			/* run out of memory */
+			ndev->stats.rx_dropped++;
+			return rxpktcount;
+		}
+
+		/* make 16 bytes aligned for 14 bytes ethernet header */
+		skb_buf = skb_put(skb, rxlength);
+		memcpy(skb_buf, pframe, rxlength);
+
+		skb->protocol = eth_type_trans(skb, ndev);
+		netif_receive_skb(skb);
+		rxpktcount++;
+
+		prxdesc->status = UMAC_RING_ENTRY_HW_OWN;
+		prxdesc->count = UMAC_MAX_FRAME_SIZE;
+
+		ndev->stats.rx_packets++;
+		ndev->stats.rx_bytes += rxlength;
+
+		/* move to next buffer */
+		umac->rx_cur++;
+		if (umac->rx_cur >= UMAC_MAX_RX_DESC_ENTRIES)
+			umac->rx_cur = 0;
+
+		if (rxpktcount >= budget)
+			break;
+
+		prxdesc = &umac->rx_descs->entrylist[umac->rx_cur];
+		pframe = umac->rx_descs->framelist[umac->rx_cur];
+	}
+	/* start processing by writing the ring prompt register */
+	writel(0, umac->base + UMAC_RING_PROMPT);
+
+	return rxpktcount;
+}
+
+static void umac_tx_done(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+
+	unsigned int txptr;
+	unsigned int value;
+	struct umac_tx_desc_entry *ptxdesc;
+
+	value = readl(umac->base + UMAC_RING_PTR);
+	txptr = (value & UMAC_TX_RING_PTR_MASK) >> UMAC_TX_RING_PTR_SHIFT;
+
+	ptxdesc = &umac->tx_descs->entrylist[umac->tx_done];
+
+	while (!(ptxdesc->status & UMAC_RING_ENTRY_HW_OWN)) {
+		if (umac->tx_done == txptr)
+			break;
+
+		ndev->stats.tx_packets++;
+		ndev->stats.tx_bytes += ptxdesc->count;
+
+		umac->tx_done++;
+		if (umac->tx_done >= UMAC_MAX_TX_DESC_ENTRIES)
+			umac->tx_done = 0;
+		ptxdesc = &umac->tx_descs->entrylist[umac->tx_done];
+	}
+
+	/* clear tx interrupt */
+	value = readl(umac->base + UMAC_INTERRUPT);
+	value &= ~UMAC_TX_INT;
+	writel(value, umac->base + UMAC_INTERRUPT);
+
+	if (netif_queue_stopped(ndev))
+		netif_wake_queue(ndev);
+}
+
+static void umac_irq_enable(struct umac_priv *umac)
+{
+	unsigned int value;
+
+	/* enable interrupt */
+	value = readl(umac->base + UMAC_INTERRUPT);
+	value |= (UMAC_RX_INTEN | UMAC_TX_INTEN);
+	writel(value, umac->base + UMAC_INTERRUPT);
+}
+
+static void umac_irq_disable(struct umac_priv *umac)
+{
+	unsigned int value;
+
+	/* clear and disable interrupt */
+	value = readl(umac->base + UMAC_INTERRUPT);
+	value |= (UMAC_RX_INT | UMAC_TX_INT);
+	value &= ~(UMAC_RX_INTEN | UMAC_TX_INTEN);
+	writel(value, umac->base + UMAC_INTERRUPT);
+}
+
+static irqreturn_t umac_interrupt(int irq, void *p_ndev)
+{
+	struct net_device *ndev = (struct net_device *)p_ndev;
+	struct umac_priv *umac = netdev_priv(ndev);
+
+	if (umac->use_ncsi || netif_running(ndev)) {
+		umac_irq_disable(umac);
+		napi_schedule(&umac->napi);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int umac_poll(struct napi_struct *napi, int budget)
+{
+	struct umac_priv *umac = container_of(napi, struct umac_priv, napi);
+	struct net_device *ndev = umac->ndev;
+	unsigned int value;
+	int rx_done = 0;
+
+	umac_tx_done(ndev);
+
+	rx_done = umac_rx(ndev, budget);
+
+	if (rx_done < budget) {
+		napi_complete_done(napi, rx_done);
+		/* clear rx interrupt */
+		value = readl(umac->base + UMAC_INTERRUPT);
+		value &= ~UMAC_RX_INT;
+		writel(value, umac->base + UMAC_INTERRUPT);
+
+		/* enable interrupt */
+		umac_irq_enable(umac);
+	}
+
+	return rx_done;
+}
+
+static int umac_open(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	int err;
+
+	if (request_irq(ndev->irq, umac_interrupt, 0x0, ndev->name, ndev)) {
+		netdev_err(ndev, "failed to register irq\n");
+		return -EAGAIN;
+	}
+
+	umac_init_ring_discriptor(ndev);
+	umac_init_hw(ndev);
+
+	if (umac->use_ncsi)
+		netif_carrier_on(ndev);
+	else
+		phy_start(ndev->phydev);
+
+	napi_enable(&umac->napi);
+	netif_start_queue(ndev);
+	umac_irq_enable(umac);
+
+	if (umac->use_ncsi) {
+		err = ncsi_start_dev(umac->ncsidev);
+		if (err) {
+			netdev_err(ndev, "failed to start ncsi\n");
+			free_irq(ndev->irq, ndev);
+			return err;
+		}
+	}
+
+	netdev_info(ndev, "%s is OPENED\n", ndev->name);
+	return 0;
+}
+
+static int umac_stop(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	struct platform_device *pdev = umac->pdev;
+
+	dma_free_coherent(&pdev->dev, sizeof(struct umac_tx_descs),
+			  umac->tx_descs, umac->tx_descs_dma_addr);
+	dma_free_coherent(&pdev->dev, sizeof(struct umac_rx_descs),
+			  umac->rx_descs, umac->rx_descs_dma_addr);
+	netif_stop_queue(ndev);
+
+	if (umac->use_ncsi)
+		ncsi_stop_dev(umac->ncsidev);
+	else
+		phy_stop(ndev->phydev);
+	umac_irq_disable(umac);
+	umac_channel_disable(umac);
+	napi_disable(&umac->napi);
+
+	free_irq(ndev->irq, ndev);
+
+	return 0;
+}
+
+static const struct ethtool_ops umac_ethtool_ops = {
+	.get_ts_info	= ethtool_op_get_ts_info,
+	.get_link_ksettings	= umac_get_link_ksettings,
+	.set_link_ksettings	= umac_set_link_ksettings,
+	.get_drvinfo	= umac_get_drvinfo,
+	.nway_reset	= umac_nway_reset,
+	.get_link	= umac_get_link,
+};
+
+static const struct net_device_ops umac_netdev_ops = {
+	.ndo_open		= umac_open,
+	.ndo_stop		= umac_stop,
+	.ndo_start_xmit		= umac_start_xmit,
+	.ndo_get_stats		= umac_get_stats,
+	.ndo_do_ioctl		= umac_ioctl,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_set_mac_address	= umac_eth_mac_addr,
+};
+
+static int umac_init_mac_address(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	struct platform_device *pdev = umac->pdev;
+	char addr[ETH_ALEN];
+	int err;
+
+	err = of_get_mac_address(pdev->dev.of_node, addr);
+	if (err)
+		netdev_err(ndev, "Failed to get address from device-tree: %d\n",
+			   err);
+
+	if (is_valid_ether_addr(addr)) {
+		dev_addr_set(ndev, addr);
+		netdev_info(ndev,
+			    "Read MAC address %pM from DTB\n", ndev->dev_addr);
+	} else {
+		eth_hw_addr_random(ndev);
+		netdev_info(ndev, "Generated random MAC address %pM\n",
+			    ndev->dev_addr);
+	}
+
+	dev_addr_set(ndev, addr);
+	umac_set_mac_address(ndev, addr);
+
+	return 0;
+}
+
+static void umac_ncsi_handler(struct ncsi_dev *ncsidev)
+{
+	if (unlikely(ncsidev->state != ncsi_dev_state_functional))
+		return;
+
+	netdev_info(ncsidev->dev, "NCSI interface %s\n",
+		    ncsidev->link_up ? "up" : "down");
+}
+
+static void umac_adjust_link(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	int value;
+
+	if (ndev->phydev->link) {
+		/* disable both clock */
+		value = readl(umac->base + UMAC_CONFIG_STATUS);
+		value &= 0xfffff9ff;
+		writel(value, umac->base + UMAC_CONFIG_STATUS);
+		udelay(2);
+
+		if (ndev->phydev->duplex)
+			value |= UMAC_CFG_FULL_DUPLEX;
+		else
+			value &= ~UMAC_CFG_FULL_DUPLEX;
+
+		switch (ndev->phydev->speed) {
+		case SPEED_1000:
+			value &= ~UMAC_CFG_TX_CLK_EN;
+			value |= UMAC_CFG_GTX_CLK_EN;
+			value |= UMAC_CFG_GIGABIT_MODE;
+			break;
+		case SPEED_100:
+			value |= UMAC_CFG_TX_CLK_EN;
+			value &= ~UMAC_CFG_GTX_CLK_EN;
+			value &= ~UMAC_CFG_GIGABIT_MODE;
+			break;
+		}
+		/* update duplex and gigabit_mode to umac */
+		writel(value, umac->base + UMAC_CONFIG_STATUS);
+		udelay(2);
+
+		netif_carrier_on(ndev);
+	} else {
+		/* disable both clock */
+		value = readl(umac->base + UMAC_CONFIG_STATUS);
+		value &= 0xfffff9ff;
+		writel(value, umac->base + UMAC_CONFIG_STATUS);
+		udelay(2);
+
+		value &= ~UMAC_CFG_FULL_DUPLEX;
+		value &= ~UMAC_CFG_GTX_CLK_EN;
+		value &= ~UMAC_CFG_GIGABIT_MODE;
+		value |= UMAC_CFG_TX_CLK_EN;
+		writel(value, umac->base + UMAC_CONFIG_STATUS);
+		udelay(2);
+
+		netif_carrier_off(ndev);
+	}
+}
+
+static struct device_node *gxp_umac_get_eth_child_node(struct device_node *ether_np, int id)
+{
+	struct device_node *port_np;
+	int port_id;
+
+	for_each_child_of_node(ether_np, port_np) {
+		/* It is not a 'port' node, continue. */
+		if (strcmp(port_np->name, "port"))
+			continue;
+		if (of_property_read_u32(port_np, "reg", &port_id) < 0)
+			continue;
+
+		if (port_id == id)
+			return port_np;
+	}
+
+	/* Not found! */
+	return NULL;
+}
+
+static int umac_setup_phy(struct net_device *ndev)
+{
+	struct umac_priv *umac = netdev_priv(ndev);
+	struct platform_device *pdev = umac->pdev;
+	struct device_node *phy_handle;
+	phy_interface_t interface;
+	struct device_node *eth_ports_np;
+	struct device_node *port_np;
+	int ret;
+	int err;
+	int i;
+
+	/* Get child node ethernet-ports. */
+	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");
+	if (!eth_ports_np) {
+		dev_err(&pdev->dev, "No ethernet-ports child node found!\n");
+		return -ENODEV;
+	}
+
+	for (i = 0; i < NUMBER_OF_PORTS; i++) {
+		/* Get port@i of node ethernet-ports */
+		port_np = gxp_umac_get_eth_child_node(eth_ports_np, i);
+		if (!port_np)
+			break;
+
+		if (i == INTERNAL_PORT) {
+			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
+			if (phy_handle) {
+				umac->int_phy_dev = of_phy_find_device(phy_handle);
+				if (!umac->int_phy_dev)
+					return -ENODEV;
+
+				umac_int_phy_init(umac);
+			} else {
+				return dev_err_probe(&pdev->dev, PTR_ERR(phy_handle),
+						     "Failed to map phy-handle for port %d", i);
+			}
+		}
+
+		if (i == EXTERNAL_PORT) {
+			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
+			if (phy_handle) {
+				/* register the phy board fixup */
+				ret = phy_register_fixup_for_uid(0x01410dd1, 0xffffffff,
+								 umac_phy_fixup);
+				if (ret)
+					dev_err(&pdev->dev, "cannot register phy board fixup\n");
+
+				err = of_get_phy_mode(phy_handle, &interface);
+				if (err)
+					interface = PHY_INTERFACE_MODE_NA;
+
+				umac->phy_dev = of_phy_connect(ndev, phy_handle,
+							       &umac_adjust_link,
+							       0, interface);
+
+				if (!umac->phy_dev)
+					return -ENODEV;
+
+				/* If the specified phy-handle has a fixed-link declaration, use the
+				 * fixed-link properties to set the configuration for the PHY
+				 */
+				if (of_phy_is_fixed_link(phy_handle)) {
+					struct device_node *fixed_link_node =
+						of_get_child_by_name(phy_handle,
+								     "fixed-link");
+
+					if (of_property_read_u32(fixed_link_node, "speed",
+								 &umac->phy_dev->speed)) {
+						netdev_err(ndev, "Invalid fixed-link specified.\n");
+						return -EINVAL;
+					}
+					umac->phy_dev->duplex =
+						of_property_read_bool(fixed_link_node,
+								      "full-duplex");
+					umac->phy_dev->pause =
+						of_property_read_bool(fixed_link_node,
+								      "pause");
+					umac->phy_dev->asym_pause =
+						of_property_read_bool(fixed_link_node,
+								      "asym-pause");
+					umac->phy_dev->autoneg = AUTONEG_DISABLE;
+					__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+						    umac->phy_dev->advertising);
+				}
+			} else {
+				return dev_err_probe(&pdev->dev, PTR_ERR(phy_handle),
+						     "Failed to map phy-handle for port %d", i);
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int umac_probe(struct platform_device *pdev)
+{
+	struct umac_priv *umac;
+	struct net_device *ndev;
+	struct resource *res;
+	int ret = 0;
+
+	ndev = alloc_etherdev(sizeof(*umac));
+	if (!ndev)
+		return -ENOMEM;
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	umac = netdev_priv(ndev);
+	umac->pdev = pdev;
+	umac->ndev = ndev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		netdev_err(ndev, "failed to get I/O memory\n");
+		free_netdev(ndev);
+		return -ENXIO;
+	}
+
+	umac->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!umac->base) {
+		netdev_err(ndev, "failed to remap I/O memory\n");
+		free_netdev(ndev);
+		return -EBUSY;
+	}
+
+	ndev->irq = platform_get_irq(pdev, 0);
+	if (ndev->irq < 0) {
+		netdev_err(ndev, "failed to get irq\n");
+		free_netdev(ndev);
+		return -ENXIO;
+	}
+
+	platform_set_drvdata(pdev, ndev);
+
+	ndev->netdev_ops = &umac_netdev_ops;
+	ndev->ethtool_ops = &umac_ethtool_ops;
+
+	umac_init_mac_address(ndev);
+	umac_channel_disable(umac);
+	ret = umac_setup_phy(ndev);
+	if (ret != 0) {
+		netdev_err(ndev, "failed to setup phy ret=%d\n", ret);
+		return -ENODEV;
+	}
+
+	umac->use_ncsi = false;
+	if (of_get_property(pdev->dev.of_node, "use-ncsi", NULL)) {
+		if (!IS_ENABLED(CONFIG_NET_NCSI)) {
+			netdev_err(ndev, "NCSI stack not enabled\n");
+			free_netdev(ndev);
+			return 0;
+		}
+
+		dev_info(&pdev->dev, "Using NCSI interface\n");
+		umac->use_ncsi = true;
+		umac->ncsidev = ncsi_register_dev(ndev, umac_ncsi_handler);
+		if (!umac->ncsidev) {
+			free_netdev(ndev);
+			return -ENODEV;
+		}
+	}
+
+	netif_napi_add(ndev, &umac->napi, umac_poll);
+	ret = register_netdev(ndev);
+	if (ret != 0) {
+		netdev_err(ndev, "failed to register UMAC ret=%d\n", ret);
+		netif_napi_del(&umac->napi);
+		free_netdev(ndev);
+		return -ENODEV;
+	}
+
+	return ret;
+}
+
+static int umac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct umac_priv *umac = netdev_priv(ndev);
+
+	unregister_netdev(ndev);
+	iounmap(umac->base);
+	free_netdev(ndev);
+	return 0;
+}
+
+static const struct of_device_id umac_of_matches[] = {
+	{ .compatible = "hpe, gxp-umac", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, umac_of_matches);
+
+static struct platform_driver umac_driver = {
+	.driver	= {
+		.name    = "gxp-umac",
+		.of_match_table = of_match_ptr(umac_of_matches),
+	},
+	.probe   = umac_probe,
+	.remove  = umac_remove,
+};
+
+module_platform_driver(umac_driver);
+
+MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com");
+MODULE_DESCRIPTION("HPE GXP UMAC driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/hpe/gxp-umac.h b/drivers/net/ethernet/hpe/gxp-umac.h
new file mode 100644
index 000000000000..2d3c777dd5b5
--- /dev/null
+++ b/drivers/net/ethernet/hpe/gxp-umac.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2023 Hewlett-Packard Development Company, L.P. */
+
+#ifndef _UMAC_H_
+#define _UMAC_H_
+
+#define UMAC_CONFIG_STATUS	0x00        /* R/W Configuration and Status
+					     * Register I
+					     */
+#define UMAC_CFG_TXEN		0x00001000  // transmit enable
+#define UMAC_CFG_RXEN		0x00000800  // receive enable
+#define UMAC_CFG_GTX_CLK_EN	0x00000400  // gigabit clock enable
+#define UMAC_CFG_TX_CLK_EN	0x00000200  // 10/100 clock enable
+#define UMAC_CFG_GIGABIT_MODE	0x00000004  // MAC gigabit mode enable
+#define UMAC_CFG_FULL_DUPLEX	0x00000001  // enable ignoring of collisions
+#define UMAC_RING_PTR		0x04        //  R/W Ring Pointer Register
+#define UMAC_TX_RING_PTR_MASK	0x7FFF0000  // transmit ring entry pointer
+#define UMAC_TX_RING_PTR_SHIFT	16
+#define UMAC_RX_RING_PTR_MASK	0x00007FFF  // receive ring entry pointer
+#define UMAC_RX_RING_PTR_SHIFT	0
+
+#define UMAC_CLEAR_STATUS	0x0C	//  W   Clear Status Register
+#define UMAC_RING_PROMPT	0x08	//  W	Ring Prompt Register
+#define UMAC_CKSUM_CONFIG	0x10	//  R/W	Checksum Config Register
+#define UMAC_RING_SIZE		0x14	//  R/W	Ring Size Register
+#define UMAC_TX_RING_SIZE_MASK	0xFF000000
+#define UMAC_TX_RING_SIZE_SHIFT	24
+#define UMAC_RX_RING_SIZE_MASK	0x00FF0000
+#define UMAC_RX_RING_SIZE_SHIFT	16
+#define UMAC_LAST_RX_PKT_SIZE	0x0000FFFF
+#define UMAC_RING_SIZE_256	0x3F
+
+#define UMAC_MAC_ADDR_HI	0x18	//  R/W	MAC Address[47:32] Register
+#define UMAC_MAC_ADDR_MID	0x1C	//  R/W	MAC Address[31:16] Register
+#define UMAC_MAC_ADDR_LO	0x20	//  R/W	MAC Address[15:0] Register
+#define UMAC_INTERRUPT		0x30	//  R/W	MAC Interrupt Configuration
+					// and Status Register
+#define UMAC_RX_INTEN		0x00000008
+#define UMAC_RX_INT		0x00000004
+#define UMAC_TX_INTEN		0x00000002
+#define UMAC_TX_INT		0x00000001
+
+#define UMAC_RX_RING_ADDR	0x4C	//  R/W	Rx Ring Base Address Register
+#define UMAC_TX_RING_ADDR	0x50	//  R/W	Tx Ring Base Address Register
+#define UMAC_DMA_CONFIG		0x54	//  R/W	DMA Config Register
+
+#define UMAC_MAX_TX_DESC_ENTRIES	0x100	/* 256,number of ring buffer
+						 *  entries supported
+						 */
+#define UMAC_MAX_RX_DESC_ENTRIES	0x100
+#define UMAC_MAX_TX_FRAME_SIZE		0x600
+#define UMAC_MAX_RX_FRAME_SIZE		0x600
+
+// ring status masks
+#define UMAC_RING_ENTRY_HW_OWN		0x8000
+
+// maximum ethernet frame size
+#define UMAC_MIN_FRAME_SIZE		60    // excludes preable, sfd, and fcs
+#define UMAC_MAX_PAYLOAD_SIZE		1500
+#define UMAC_MAX_FRAME_SIZE		1514  // excludes preable, sfd, and fcs
+
+struct umac_rx_desc_entry {
+	u32  dmaaddress;   // Start address for DMA operationg
+	u16  status;       // Packet tx status and ownership flag
+	u16  count;        // Number of bytes received
+	u16  checksum;     // On-the-fly packet checksum
+	u16  control;      // Checksum-in-time flag
+	u32  reserved;
+} __aligned(16);
+
+struct umac_rx_descs {
+	struct umac_rx_desc_entry entrylist[UMAC_MAX_RX_DESC_ENTRIES];
+	u8 framelist[UMAC_MAX_RX_DESC_ENTRIES][UMAC_MAX_RX_FRAME_SIZE];
+} __packed;
+
+struct umac_tx_desc_entry {
+	u32  dmaaddress;   // Start address for DMA operationg
+	u16  status;       // Packet rx status, type, and ownership flag
+	u16  count;        // Number of bytes received
+	u32  cksumoffset;  // Specifies where to place packet checksum
+	u32  reserved;
+} __aligned(16);
+
+struct umac_tx_descs {
+	struct umac_tx_desc_entry entrylist[UMAC_MAX_TX_DESC_ENTRIES];
+	u8 framelist[UMAC_MAX_TX_DESC_ENTRIES][UMAC_MAX_TX_FRAME_SIZE];
+} __packed;
+
+#endif
-- 
2.17.1

Re: [PATCH v1 4/5] net: hpe: Add GXP UMAC Driver

[Christophe JAILLET]

Le 21/07/2023 à 23:20, nick.hawkins-ZPxbGqLxI0U@public.gmane.org a écrit :
> From: Nick Hawkins <nick.hawkins-ZPxbGqLxI0U@public.gmane.org>
> 
> The GXP contains two Ethernet MACs that can be connected externally
> to several physical devices. From an external interface perspective
> the BMC provides two SERDES interface connections capable of either
> SGMII or 1000Base-X operation. The BMC also provides a RMII interface
> for sideband connections to external Ethernet controllers.
> 
> The primary MAC (umac0) can be mapped to either SGMII/1000-BaseX
> SERDES interface.  The secondary MAC (umac1) can be mapped to only
> the second SGMII/1000-Base X Serdes interface or it can be mapped for
> RMII sideband.
> 
> The MDIO(mdio0) interface from the primary MAC (umac0) is used for
> external PHY status and configuration. The MDIO(mdio1) interface from
> the secondary MAC (umac1) is routed to the SGMII/100Base-X IP blocks
> on the two SERDES interface connections.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins-ZPxbGqLxI0U@public.gmane.org>
> ---

[...]

> +static int umac_poll(struct napi_struct *napi, int budget)
> +{
> +	struct umac_priv *umac = container_of(napi, struct umac_priv, napi);
> +	struct net_device *ndev = umac->ndev;
> +	unsigned int value;
> +	int rx_done = 0;

No need to init.

> +
> +	umac_tx_done(ndev);
> +
> +	rx_done = umac_rx(ndev, budget);
> +
> +	if (rx_done < budget) {
> +		napi_complete_done(napi, rx_done);
> +		/* clear rx interrupt */
> +		value = readl(umac->base + UMAC_INTERRUPT);
> +		value &= ~UMAC_RX_INT;
> +		writel(value, umac->base + UMAC_INTERRUPT);
> +
> +		/* enable interrupt */
> +		umac_irq_enable(umac);
> +	}
> +
> +	return rx_done;
> +}

[...]

> +static int umac_setup_phy(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	struct platform_device *pdev = umac->pdev;
> +	struct device_node *phy_handle;
> +	phy_interface_t interface;
> +	struct device_node *eth_ports_np;
> +	struct device_node *port_np;
> +	int ret;
> +	int err;

Could there be only one ret ou err variable?

> +	int i;
> +
> +	/* Get child node ethernet-ports. */
> +	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");
> +	if (!eth_ports_np) {
> +		dev_err(&pdev->dev, "No ethernet-ports child node found!\n");
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < NUMBER_OF_PORTS; i++) {
> +		/* Get port@i of node ethernet-ports */
> +		port_np = gxp_umac_get_eth_child_node(eth_ports_np, i);
> +		if (!port_np)
> +			break;
> +
> +		if (i == INTERNAL_PORT) {
> +			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
> +			if (phy_handle) {
> +				umac->int_phy_dev = of_phy_find_device(phy_handle);
> +				if (!umac->int_phy_dev)
> +					return -ENODEV;
> +
> +				umac_int_phy_init(umac);
> +			} else {
> +				return dev_err_probe(&pdev->dev, PTR_ERR(phy_handle),
> +						     "Failed to map phy-handle for port %d", i);
> +			}
> +		}
> +
> +		if (i == EXTERNAL_PORT) {
> +			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
> +			if (phy_handle) {
> +				/* register the phy board fixup */
> +				ret = phy_register_fixup_for_uid(0x01410dd1, 0xffffffff,
> +								 umac_phy_fixup);
> +				if (ret)
> +					dev_err(&pdev->dev, "cannot register phy board fixup\n");
> +
> +				err = of_get_phy_mode(phy_handle, &interface);
> +				if (err)
> +					interface = PHY_INTERFACE_MODE_NA;
> +
> +				umac->phy_dev = of_phy_connect(ndev, phy_handle,
> +							       &umac_adjust_link,
> +							       0, interface);
> +
> +				if (!umac->phy_dev)
> +					return -ENODEV;
> +
> +				/* If the specified phy-handle has a fixed-link declaration, use the
> +				 * fixed-link properties to set the configuration for the PHY
> +				 */
> +				if (of_phy_is_fixed_link(phy_handle)) {
> +					struct device_node *fixed_link_node =
> +						of_get_child_by_name(phy_handle,
> +								     "fixed-link");
> +
> +					if (of_property_read_u32(fixed_link_node, "speed",
> +								 &umac->phy_dev->speed)) {
> +						netdev_err(ndev, "Invalid fixed-link specified.\n");
> +						return -EINVAL;
> +					}
> +					umac->phy_dev->duplex =
> +						of_property_read_bool(fixed_link_node,
> +								      "full-duplex");
> +					umac->phy_dev->pause =
> +						of_property_read_bool(fixed_link_node,
> +								      "pause");
> +					umac->phy_dev->asym_pause =
> +						of_property_read_bool(fixed_link_node,
> +								      "asym-pause");
> +					umac->phy_dev->autoneg = AUTONEG_DISABLE;
> +					__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +						    umac->phy_dev->advertising);
> +				}
> +			} else {
> +				return dev_err_probe(&pdev->dev, PTR_ERR(phy_handle),
> +						     "Failed to map phy-handle for port %d", i);
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int umac_probe(struct platform_device *pdev)
> +{
> +	struct umac_priv *umac;
> +	struct net_device *ndev;
> +	struct resource *res;
> +	int ret = 0;
> +
> +	ndev = alloc_etherdev(sizeof(*umac));

devm_alloc_etherdev()?
This saves a lot of free_netdev() below.

> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +	umac = netdev_priv(ndev);
> +	umac->pdev = pdev;
> +	umac->ndev = ndev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		netdev_err(ndev, "failed to get I/O memory\n");
> +		free_netdev(ndev);
> +		return -ENXIO;
> +	}
> +
> +	umac->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!umac->base) {
> +		netdev_err(ndev, "failed to remap I/O memory\n");
> +		free_netdev(ndev);
> +		return -EBUSY;
> +	}
> +
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	if (ndev->irq < 0) {
> +		netdev_err(ndev, "failed to get irq\n");
> +		free_netdev(ndev);
> +		return -ENXIO;
> +	}
> +
> +	platform_set_drvdata(pdev, ndev);
> +
> +	ndev->netdev_ops = &umac_netdev_ops;
> +	ndev->ethtool_ops = &umac_ethtool_ops;
> +
> +	umac_init_mac_address(ndev);
> +	umac_channel_disable(umac);
> +	ret = umac_setup_phy(ndev);
> +	if (ret != 0) {
> +		netdev_err(ndev, "failed to setup phy ret=%d\n", ret);

free_netdev() missing. (should alloc_etherdev() be left as-is)

> +		return -ENODEV;

Does it makes sense to return ret as-is?

> +	}
> +
> +	umac->use_ncsi = false;
> +	if (of_get_property(pdev->dev.of_node, "use-ncsi", NULL)) {
> +		if (!IS_ENABLED(CONFIG_NET_NCSI)) {
> +			netdev_err(ndev, "NCSI stack not enabled\n");
> +			free_netdev(ndev);
> +			return 0;

So register_netdev() below could not be called.
In such a case unregister_netdev() would be unbalanced in the remove 
function. Using devm_register_netdev() below would avoid it.

> +		}
> +
> +		dev_info(&pdev->dev, "Using NCSI interface\n");
> +		umac->use_ncsi = true;
> +		umac->ncsidev = ncsi_register_dev(ndev, umac_ncsi_handler);
> +		if (!umac->ncsidev) {
> +			free_netdev(ndev);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	netif_napi_add(ndev, &umac->napi, umac_poll);
> +	ret = register_netdev(ndev);

devm_register_netdev()

> +	if (ret != 0) {
> +		netdev_err(ndev, "failed to register UMAC ret=%d\n", ret);
> +		netif_napi_del(&umac->napi);

(devm_)free_netdev() already call netif_napi_del().

> +		free_netdev(ndev);
> +		return -ENODEV;

Does it makes sense to return ret as-is?

> +	}
> +
> +	return ret;
> +}
> +
> +static int umac_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct umac_priv *umac = netdev_priv(ndev);
> +
> +	unregister_netdev(ndev);
> +	iounmap(umac->base);

umac->base is a managed resource.

> +	free_netdev(ndev);
> +	return 0;
> +}
> +
> +static const struct of_device_id umac_of_matches[] = {
> +	{ .compatible = "hpe, gxp-umac", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, umac_of_matches);
> +
> +static struct platform_driver umac_driver = {
> +	.driver	= {
> +		.name    = "gxp-umac",
> +		.of_match_table = of_match_ptr(umac_of_matches),
> +	},
> +	.probe   = umac_probe,
> +	.remove  = umac_remove,

With the above proposed changes, the remove function could be removed.

> +};

[...]

Re: [PATCH v1 4/5] net: hpe: Add GXP UMAC Driver

[Andrew Lunn]

> +#define PHY_88E1514_COPPER_CONTROL_REG		0
> +#define PHY_88E1514_PAGE_ADDRESS		22
> +
> +#define PHY_88E1514_GENERAL_CONTROL_REG1	20

This looks wrong. A MAC driver should never access the PHY directly.

> +
> +#define DRV_MODULE_NAME		"gxp-umac"
> +#define DRV_MODULE_VERSION	"0.1"

Versions are pointless. Please remove.

> +
> +#define NUMBER_OF_PORTS 2
> +#define EXTERNAL_PORT 1
> +#define INTERNAL_PORT 0
> +
> +struct umac_priv {
> +	void __iomem *base;
> +	int irq;
> +	struct platform_device *pdev;
> +	struct umac_tx_descs *tx_descs;
> +	struct umac_rx_descs *rx_descs;
> +	dma_addr_t tx_descs_dma_addr;
> +	dma_addr_t rx_descs_dma_addr;
> +	unsigned int tx_cur;
> +	unsigned int tx_done;
> +	unsigned int rx_cur;
> +	struct napi_struct napi;
> +	struct net_device *ndev;
> +	struct phy_device *phy_dev;
> +	struct phy_device *int_phy_dev;
> +	struct ncsi_dev *ncsidev;
> +	bool use_ncsi;
> +};
> +
> +static void umac_get_drvinfo(struct net_device *ndev,
> +			     struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
> +	strscpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));

If you leave the version empty, you get the kernel version, and i
think hash. That is actually meaningful.

> +static int umac_get_link_ksettings(struct net_device *ndev,
> +				   struct ethtool_link_ksettings *cmd)
> +{
> +	phy_ethtool_ksettings_get(ndev->phydev, cmd);
> +	return 0;

return whatever phy_ethtool_ksettings_get() returns.

phy_ethtool_get_link_ksettings() is better here. You then don't even
need a function, you can reference it directly.

> +}
> +
> +static int umac_set_link_ksettings(struct net_device *ndev,
> +				   const struct ethtool_link_ksettings *cmd)
> +{
> +	return phy_ethtool_ksettings_set(ndev->phydev, cmd);

phy_ethtool_set_link_ksettings().

> +static int umac_nway_reset(struct net_device *ndev)
> +{
> +	return genphy_restart_aneg(ndev->phydev);

You locking is broken here. Use phy_ethtool_nway_reset()

> +static u32 umac_get_link(struct net_device *ndev)
> +{
> +	int err;
> +
> +	err = genphy_update_link(ndev->phydev);
> +	if (err)
> +		return ethtool_op_get_link(ndev);
> +
> +	return ndev->phydev->link;
> +}

You have something really wrong if you are doing this.

> +static int umac_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
> +{
> +	if (!netif_running(ndev))
> +		return -EINVAL;
> +
> +	if (!ndev->phydev)
> +		return -ENODEV;
> +
> +	return phy_mii_ioctl(ndev->phydev, ifr, cmd);
> +}

> +static int umac_int_phy_init(struct umac_priv *umac)
> +{
> +	struct phy_device *phy_dev = umac->int_phy_dev;
> +	unsigned int value;
> +
> +	value = phy_read(phy_dev, 0);
> +	if (value & 0x4000)
> +		pr_info("Internal PHY loopback is enabled - clearing\n");

The MAC driver never access the PHY directly.

What is putting it into loopback? The bootloader?

> +
> +	value &= ~0x4000; /* disable loopback */
> +	phy_write(phy_dev, 0, value);
> +
> +	value = phy_read(phy_dev, 0);
> +	value |= 0x1000; /* set aneg enable */
> +	value |= 0x8000; /* SW reset */
> +	phy_write(phy_dev, 0, value);
> +
> +	do {
> +		value = phy_read(phy_dev, 0);
> +	} while (value & 0x8000);

phy_start() will do this for you.

> +static int umac_phy_fixup(struct phy_device *phy_dev)
> +{
> +	unsigned int value;
> +
> +	/* set phy mode to SGMII to copper */
> +	/* set page to 18 by writing 18 to register 22 */
> +	phy_write(phy_dev, PHY_88E1514_PAGE_ADDRESS, 18);
> +	value = phy_read(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1);
> +	value &= ~0x07;
> +	value |= 0x01;
> +	phy_write(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1, value);
> +
> +	/* perform mode reset by setting bit 15 in general_control_reg1 */
> +	phy_write(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1, value | 0x8000);
> +
> +	do {
> +		value = phy_read(phy_dev, PHY_88E1514_GENERAL_CONTROL_REG1);
> +	} while (value & 0x8000);
> +
> +	/* after setting the mode, must perform a SW reset */
> +	phy_write(phy_dev, PHY_88E1514_PAGE_ADDRESS, 0); /* set page to 0 */
> +
> +	value = phy_read(phy_dev, PHY_88E1514_COPPER_CONTROL_REG);
> +	value |= 0x8000;
> +	phy_write(phy_dev, PHY_88E1514_COPPER_CONTROL_REG, value);
> +
> +	do {
> +		value = phy_read(phy_dev, PHY_88E1514_COPPER_CONTROL_REG);
> +	} while (value & 0x8000);

Please extend the PHY driver to do this. You can pass SGMII as the
interface, and have the PHY driver act on it.

> +static int umac_init_hw(struct net_device *ndev)
> +{
> +	if (umac->use_ncsi) {
> +		/* set correct tx clock */
> +		value &= UMAC_CFG_TX_CLK_EN;
> +		value &= ~UMAC_CFG_GTX_CLK_EN;
> +		value &= ~UMAC_CFG_GIGABIT_MODE; /* RMII mode */
> +		value |= UMAC_CFG_FULL_DUPLEX; /* full duplex */
> +	} else {
> +		if (ndev->phydev->duplex)
> +			value |= UMAC_CFG_FULL_DUPLEX;
> +		else
> +			value &= ~UMAC_CFG_FULL_DUPLEX;
> +
> +		if (ndev->phydev->speed == SPEED_1000) {

The MAC driver should only access phydev members inside the
adjust_link callback. Outside of that, these members can in
inconsistent.

> +static int umac_open(struct net_device *ndev)
> +{

...

> +	netdev_info(ndev, "%s is OPENED\n", ndev->name);

Please don't spam the kernel log.

> +static int umac_init_mac_address(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	struct platform_device *pdev = umac->pdev;
> +	char addr[ETH_ALEN];
> +	int err;
> +
> +	err = of_get_mac_address(pdev->dev.of_node, addr);
> +	if (err)
> +		netdev_err(ndev, "Failed to get address from device-tree: %d\n",
> +			   err);
> +
> +	if (is_valid_ether_addr(addr)) {
> +		dev_addr_set(ndev, addr);
> +		netdev_info(ndev,
> +			    "Read MAC address %pM from DTB\n", ndev->dev_addr);

netdev_dbg()

> +static void umac_adjust_link(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	int value;
> +
> +	if (ndev->phydev->link) {
> +		/* disable both clock */
> +		value = readl(umac->base + UMAC_CONFIG_STATUS);
> +		value &= 0xfffff9ff;
> +		writel(value, umac->base + UMAC_CONFIG_STATUS);
> +		udelay(2);
> +
> +		if (ndev->phydev->duplex)
> +			value |= UMAC_CFG_FULL_DUPLEX;
> +		else
> +			value &= ~UMAC_CFG_FULL_DUPLEX;
> +
> +		switch (ndev->phydev->speed) {
> +		case SPEED_1000:
> +			value &= ~UMAC_CFG_TX_CLK_EN;
> +			value |= UMAC_CFG_GTX_CLK_EN;
> +			value |= UMAC_CFG_GIGABIT_MODE;
> +			break;
> +		case SPEED_100:
> +			value |= UMAC_CFG_TX_CLK_EN;
> +			value &= ~UMAC_CFG_GTX_CLK_EN;
> +			value &= ~UMAC_CFG_GIGABIT_MODE;
> +			break;
> +		}
> +		/* update duplex and gigabit_mode to umac */
> +		writel(value, umac->base + UMAC_CONFIG_STATUS);
> +		udelay(2);
> +
> +		netif_carrier_on(ndev);

phylib will do this for you.

> +static int umac_setup_phy(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	struct platform_device *pdev = umac->pdev;
> +	struct device_node *phy_handle;
> +	phy_interface_t interface;
> +	struct device_node *eth_ports_np;
> +	struct device_node *port_np;
> +	int ret;
> +	int err;
> +	int i;
> +
> +	/* Get child node ethernet-ports. */
> +	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");
> +	if (!eth_ports_np) {
> +		dev_err(&pdev->dev, "No ethernet-ports child node found!\n");
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < NUMBER_OF_PORTS; i++) {
> +		/* Get port@i of node ethernet-ports */
> +		port_np = gxp_umac_get_eth_child_node(eth_ports_np, i);
> +		if (!port_np)
> +			break;
> +
> +		if (i == INTERNAL_PORT) {
> +			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
> +			if (phy_handle) {
> +				umac->int_phy_dev = of_phy_find_device(phy_handle);
> +				if (!umac->int_phy_dev)
> +					return -ENODEV;
> +
> +				umac_int_phy_init(umac);
> +			} else {
> +				return dev_err_probe(&pdev->dev, PTR_ERR(phy_handle),
> +						     "Failed to map phy-handle for port %d", i);
> +			}
> +		}
> +
> +		if (i == EXTERNAL_PORT) {
> +			phy_handle = of_parse_phandle(port_np, "phy-handle", 0);
> +			if (phy_handle) {
> +				/* register the phy board fixup */
> +				ret = phy_register_fixup_for_uid(0x01410dd1, 0xffffffff,
> +								 umac_phy_fixup);
> +				if (ret)
> +					dev_err(&pdev->dev, "cannot register phy board fixup\n");
> +
> +				err = of_get_phy_mode(phy_handle, &interface);
> +				if (err)
> +					interface = PHY_INTERFACE_MODE_NA;
> +
> +				umac->phy_dev = of_phy_connect(ndev, phy_handle,
> +							       &umac_adjust_link,
> +							       0, interface);
> +
> +				if (!umac->phy_dev)
> +					return -ENODEV;

It looks like you MAC does not support 10Mbps. At some point you need
to remove the two link modes using phy_remove_link_mode().

> +
> +				/* If the specified phy-handle has a fixed-link declaration, use the
> +				 * fixed-link properties to set the configuration for the PHY

This is wrong. Look at other MAC drivers using fixed-link.

     Andrew

Re: [PATCH v1 4/5] net: hpe: Add GXP UMAC Driver

[Simon Horman]

On Fri, Jul 21, 2023 at 04:20:43PM -0500, nick.hawkins@hpe.com wrote:

...

Hi Nick,

> diff --git a/drivers/net/ethernet/hpe/gxp-umac.c b/drivers/net/ethernet/hpe/gxp-umac.c

...

> +static void umac_set_mac_address(struct net_device *ndev, void *p_addr)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	char *addr = (char *)p_addr;
> +	unsigned int value;
> +
> +	/* update address to register */
> +	value = addr[0] << 8 | addr[1];
> +	writel(value, umac->base + UMAC_MAC_ADDR_HI);
> +	value = addr[2] << 8 | addr[3];
> +	writel(value, umac->base + UMAC_MAC_ADDR_MID);
> +	value = addr[4] << 8 | addr[5];
> +	writel(value, umac->base + UMAC_MAC_ADDR_LO);
> +}
> +
> +static int umac_eth_mac_addr(struct net_device *ndev, void *p)
> +{
> +	int ret;
> +	struct sockaddr *addr = p;

Please use reverse xmas tree - longest like to shortest - for
new Networking code. Likewise in some other places in this patch/series.

This tool can be helpful in this regard.
https://github.com/ecree-solarflare/xmastree

...

> +static int umac_init_hw(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	unsigned int value;
> +
> +	/* initialize tx and rx rings to first entry */
> +	writel(0, umac->base + UMAC_RING_PTR);
> +
> +	/* clear the missed bit */
> +	writel(0, umac->base + UMAC_CLEAR_STATUS);
> +
> +	/* disable checksum generation */
> +	writel(0, umac->base + UMAC_CKSUM_CONFIG);
> +
> +	/* write the ring size register */
> +	value = ((UMAC_RING_SIZE_256 << UMAC_TX_RING_SIZE_SHIFT) &
> +			UMAC_TX_RING_SIZE_MASK) |
> +		((UMAC_RING_SIZE_256 << UMAC_RX_RING_SIZE_SHIFT) &
> +			UMAC_RX_RING_SIZE_MASK);
> +	writel(value, umac->base + UMAC_RING_SIZE);
> +
> +	/* write rx ring base address */
> +	writel(cpu_to_le32(umac->rx_descs_dma_addr),
> +	       umac->base + UMAC_RX_RING_ADDR);

It is my understanding that writel will convert the value
from host byte order to little endien. If so then pre-converting value
seems incorrect. Perhaps this should be:

	writel(umac->rx_descs_dma_addr, umac->base + UMAC_RX_RING_ADDR);

Flagged by Sparse.

> +
> +	/* write tx ring base address */
> +	writel(cpu_to_le32(umac->tx_descs_dma_addr),
> +	       umac->base + UMAC_TX_RING_ADDR);

Ditto.

> +
> +	/* write burst size */
> +	writel(0x22, umac->base + UMAC_DMA_CONFIG);
> +
> +	umac_channel_disable(umac);
> +
> +	/* disable clocks and gigabit mode (leave channels disabled) */
> +	value = readl(umac->base + UMAC_CONFIG_STATUS);
> +	value &= 0xfffff9ff;
> +	writel(value, umac->base + UMAC_CONFIG_STATUS);
> +	udelay(2);
> +
> +	if (umac->use_ncsi) {
> +		/* set correct tx clock */
> +		value &= UMAC_CFG_TX_CLK_EN;
> +		value &= ~UMAC_CFG_GTX_CLK_EN;
> +		value &= ~UMAC_CFG_GIGABIT_MODE; /* RMII mode */
> +		value |= UMAC_CFG_FULL_DUPLEX; /* full duplex */
> +	} else {
> +		if (ndev->phydev->duplex)
> +			value |= UMAC_CFG_FULL_DUPLEX;
> +		else
> +			value &= ~UMAC_CFG_FULL_DUPLEX;
> +
> +		if (ndev->phydev->speed == SPEED_1000) {
> +			value &= ~UMAC_CFG_TX_CLK_EN;
> +			value |= UMAC_CFG_GTX_CLK_EN;
> +			value |= UMAC_CFG_GIGABIT_MODE;
> +		} else {
> +			value |= UMAC_CFG_TX_CLK_EN;
> +			value &= ~UMAC_CFG_GTX_CLK_EN;
> +			value &= ~UMAC_CFG_GIGABIT_MODE;
> +		}
> +	}
> +	writel(value, umac->base + UMAC_CONFIG_STATUS);
> +	udelay(2);
> +
> +	umac_channel_enable(umac);
> +
> +	return 0;
> +}

...

> diff --git a/drivers/net/ethernet/hpe/gxp-umac.h b/drivers/net/ethernet/hpe/gxp-umac.h

...

> +struct umac_rx_desc_entry {
> +	u32  dmaaddress;   // Start address for DMA operationg

1. operationg -> operating
2. It appears that this field is used to hold __le32 values.
   Perhaps it's type should be __le32.
   As is, Sparse complains about this.

> +	u16  status;       // Packet tx status and ownership flag
> +	u16  count;        // Number of bytes received
> +	u16  checksum;     // On-the-fly packet checksum
> +	u16  control;      // Checksum-in-time flag
> +	u32  reserved;
> +} __aligned(16);
> +
> +struct umac_rx_descs {
> +	struct umac_rx_desc_entry entrylist[UMAC_MAX_RX_DESC_ENTRIES];
> +	u8 framelist[UMAC_MAX_RX_DESC_ENTRIES][UMAC_MAX_RX_FRAME_SIZE];
> +} __packed;
> +
> +struct umac_tx_desc_entry {
> +	u32  dmaaddress;   // Start address for DMA operationg

Ditto.

> +	u16  status;       // Packet rx status, type, and ownership flag
> +	u16  count;        // Number of bytes received
> +	u32  cksumoffset;  // Specifies where to place packet checksum
> +	u32  reserved;
> +} __aligned(16);
> +
> +struct umac_tx_descs {
> +	struct umac_tx_desc_entry entrylist[UMAC_MAX_TX_DESC_ENTRIES];
> +	u8 framelist[UMAC_MAX_TX_DESC_ENTRIES][UMAC_MAX_TX_FRAME_SIZE];
> +} __packed;
> +
> +#endif
> -- 
> 2.17.1
> 
> 

[PATCH v1 5/5] MAINTAINERS: HPE: Add GXP UMAC Networking Files

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

List the files added for supporting the UMAC networking on GXP.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 27ef11624748..4f1c3fa27f7f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2243,6 +2243,8 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/arm/hpe,gxp.yaml
 F:	Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
 F:	Documentation/devicetree/bindings/i2c/hpe,gxp-i2c.yaml
+F:	Documentation/devicetree/bindings/net/hpe,gxp-umac-mdio.yaml
+F:	Documentation/devicetree/bindings/net/hpe,gxp-umac.yaml
 F:	Documentation/devicetree/bindings/spi/hpe,gxp-spifi.yaml
 F:	Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
 F:	Documentation/hwmon/gxp-fan-ctrl.rst
@@ -2252,6 +2254,7 @@ F:	arch/arm/mach-hpe/
 F:	drivers/clocksource/timer-gxp.c
 F:	drivers/hwmon/gxp-fan-ctrl.c
 F:	drivers/i2c/busses/i2c-gxp.c
+F:	drivers/net/ethernet/hpe/
 F:	drivers/spi/spi-gxp.c
 F:	drivers/watchdog/gxp-wdt.c
 
-- 
2.17.1