[v1,2/5] net: hpe: Add GXP UMAC MDIO
Commit Message
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
Comments
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");
> 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
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;
> +}
...
@@ -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"
@@ -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/
new file mode 100644
@@ -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
new file mode 100644
@@ -0,0 +1 @@
+obj-$(CONFIG_GXP_UMAC_MDIO) += gxp-umac-mdio.o
new file mode 100644
@@ -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");