[PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Timur Tabi]

Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
device, like an FPGA.  The device must be memory-mapped and contain only
8-bit registers (which keeps things simple).

Tested on a Freescale P5020DS board which uses the "PIXIS" FPGA attached
to the localbus.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 .../devicetree/bindings/net/mdio-mux-mmioreg.txt   |   73 ++++++++
 drivers/net/phy/Kconfig                            |   13 ++
 drivers/net/phy/Makefile                           |    1 +
 drivers/net/phy/mdio-mux-mmioreg.c                 |  185 ++++++++++++++++++++
 4 files changed, 272 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
 create mode 100644 drivers/net/phy/mdio-mux-mmioreg.c

diff --git a/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
new file mode 100644
index 0000000..251aa10
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
@@ -0,0 +1,73 @@
+Properties for an MDIO bus multiplexer controlled by a memory-mapped device
+
+This is a special case of a MDIO bus multiplexer.  A memory-mapped device,
+like an FPGA, is used to control which child bus is connected.  The mdio-mux
+node must be a child of the memory-mapped device.  The driver currently only
+supports devices with 8-bit registers.
+
+Required properties in addition to the generic multiplexer properties:
+
+- compatible : string, must contain "mdio-mux-mmioreg"
+
+- reg : integer, contains the offset of the register that
+	controls the bus multiplexer.
+
+- mux-mask : integer, contains an 8-bit mask that specifies which
+	bits in the register control the actual bus multiplexer.  The
+	'reg' property of each child mdio-mux node must be constrained by
+	this mask.
+
+Example:
+
+The FPGA node defines a memory-mapped FPGA with a register space of 0x30 bytes.
+For the "EMI2" MDIO bus, register 9 (BRDCFG1) controls the mux on that bus.
+A bitmask of 0x6 means that bits 1 and 2 (bit 0 is lsb) are the bits on
+BRDCFG1 that control the actual mux.
+
+	/* The FPGA node */
+	fpga: board-control@3,0 {
+		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
+		reg = <3 0 0x30>;
+
+		mdio-mux-emi2 {
+			compatible = "mdio-mux-mmioreg", "mdio-mux";
+			mdio-parent-bus = <&xmdio0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <9>; // BRDCFG1
+			mux-mask = <0x6>; // EMI2
+
+			emi2_slot1: mdio@0 {	// Slot 1 XAUI (FM2)
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy_xgmii_slot1: ethernet-phy@0 {
+					compatible = "ethernet-phy-ieee802.3-c45";
+					reg = <4>;
+				};
+			};
+
+			emi2_slot2: mdio@2 {	// Slot 2 XAUI (FM1)
+				reg = <2>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy_xgmii_slot2: ethernet-phy@4 {
+					compatible = "ethernet-phy-ieee802.3-c45";
+					reg = <0>;
+				};
+			};
+		};
+	};
+
+	/* The parent MDIO bus. */
+	xmdio0: mdio@f1000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "fsl,fman-xmdio";
+		reg = <0xf1000 0x1000>;
+		interrupts = <100 1 0 0>;
+	};
+
+
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3090dc6..0268edd 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -159,6 +159,19 @@ config MDIO_BUS_MUX_GPIO
 	  several child MDIO busses to a parent bus.  Child bus
 	  selection is under the control of GPIO lines.
 
+config MDIO_BUS_MUX_MMIOREG
+	tristate "Support for MMIO device-controlled MDIO bus multiplexers"
+	depends on OF_MDIO
+	select MDIO_BUS_MUX
+	help
+	  This module provides a driver for MDIO bus multiplexers that
+	  are controlled via a simple memory-mapped device, like an FPGA.
+	  The multiplexer connects one of several child MDIO busses to a
+	  parent bus.  Child bus selection is under the control of one of
+	  the FPGA's registers.
+
+	  Currently, only 8-bit registers are supported.
+
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 6d2dc6c..426674d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
 obj-$(CONFIG_MDIO_BUS_MUX_GPIO)	+= mdio-mux-gpio.o
+obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
new file mode 100644
index 0000000..e706c695
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -0,0 +1,185 @@
+/*
+ * Simple memory-mapped device MDIO MUX driver
+ *
+ * Author: Timur Tabi <timur@freescale.com>
+ *
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/phy.h>
+#include <linux/mdio-mux.h>
+
+struct mdio_mux_mmioreg_state {
+	void *mux_handle;
+	phys_addr_t phys;
+	unsigned int offset;
+	uint8_t mask;
+};
+
+/*
+ * MDIO multiplexing switch function
+ *
+ * This function is called by the mdio-mux layer when it thinks the mdio bus
+ * multiplexer needs to switch.
+ *
+ * 'current_child' is the current value of the mux register (masked via
+ * s->mask).
+ *
+ * 'desired_child' is the value of the 'reg' property of the target child MDIO
+ * node.
+ *
+ * The first time this function is called, current_child == -1.
+ *
+ * If current_child == desired_child, then the mux is already set to the
+ * correct bus.
+ */
+static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
+				      void *data)
+{
+	struct mdio_mux_mmioreg_state *s = data;
+
+	if (current_child ^ desired_child) {
+		void *p = ioremap(s->phys + s->offset, 1);
+		uint8_t x, y;
+
+		if (!p)
+			return -ENOMEM;
+
+		x = ioread8(p);
+		y = (x & ~s->mask) | desired_child;
+		if (x != y) {
+			iowrite8((x & ~s->mask) | desired_child, p);
+			pr_debug("%s: %02x -> %02x\n", __func__, x, y);
+		}
+
+		iounmap(p);
+	}
+
+	return 0;
+}
+
+static int __devinit mdio_mux_mmioreg_probe(struct platform_device *pdev)
+{
+	struct device_node *np2, *np = pdev->dev.of_node;
+	struct mdio_mux_mmioreg_state *s;
+	struct resource res;
+	const __be32 *iprop;
+	int len, ret;
+
+	dev_dbg(&pdev->dev, "probing node %s\n", np->full_name);
+
+	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	/* The MMIO device is the parent of this device */
+	np2 = of_get_parent(np);
+	if (!np2) {
+		dev_err(&pdev->dev, "could not find parent MMIO device\n");
+		return -ENODEV;
+	}
+
+	ret = of_address_to_resource(np2, 0, &res);
+	if (ret) {
+		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
+			np2->full_name);
+		return ret;
+	}
+	s->phys = res.start;
+
+	iprop = of_get_property(np, "reg", &len);
+	if (!iprop || len != sizeof(uint32_t)) {
+		dev_err(&pdev->dev, "missing or invalid 'reg' property\n");
+		return -EINVAL;
+	}
+	s->offset = be32_to_cpup(iprop);
+	if (s->offset >= resource_size(&res)) {
+		dev_err(&pdev->dev, "'reg' property value of %u is too large\n",
+			s->offset);
+		return -EINVAL;
+	}
+
+	iprop = of_get_property(np, "mux-mask", &len);
+	if (!iprop || len != sizeof(uint32_t)) {
+		dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
+		return -ENODEV;
+	}
+	if (be32_to_cpup(iprop) > 255) {
+		dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+		return -EINVAL;
+	}
+	s->mask = be32_to_cpup(iprop);
+
+	/*
+	 * Verify that the 'reg' property of each child MDIO bus does not
+	 * set any bits outside of the 'mask'.
+	 */
+	for_each_available_child_of_node(np, np2) {
+		iprop = of_get_property(np2, "reg", &len);
+		if (!iprop || len != sizeof(uint32_t)) {
+			dev_err(&pdev->dev, "mdio-mux child node %s is "
+				"missing a 'reg' property\n", np2->full_name);
+			return -ENODEV;
+		}
+		if (be32_to_cpup(iprop) & ~s->mask) {
+			dev_err(&pdev->dev, "mdio-mux child node %s has "
+				"a 'reg' value with unmasked bits\n",
+				np2->full_name);
+			return -ENODEV;
+		}
+	}
+
+	ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
+			    &s->mux_handle, s);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register mdio-mux bus %s\n",
+			np->full_name);
+		return ret;
+	}
+
+	pdev->dev.platform_data = s;
+
+	return 0;
+}
+
+static int __devexit mdio_mux_mmioreg_remove(struct platform_device *pdev)
+{
+	struct mdio_mux_mmioreg_state *s = dev_get_platdata(&pdev->dev);
+
+	mdio_mux_uninit(s->mux_handle);
+
+	return 0;
+}
+
+static struct of_device_id mdio_mux_mmioreg_match[] = {
+	{
+		.compatible = "mdio-mux-mmioreg",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mdio_mux_mmioreg_match);
+
+static struct platform_driver mdio_mux_mmioreg_driver = {
+	.driver = {
+		.name		= "mdio-mux-mmioreg",
+		.owner		= THIS_MODULE,
+		.of_match_table = mdio_mux_mmioreg_match,
+	},
+	.probe		= mdio_mux_mmioreg_probe,
+	.remove		= __devexit_p(mdio_mux_mmioreg_remove),
+};
+
+module_platform_driver(mdio_mux_mmioreg_driver);
+
+MODULE_AUTHOR("Timur Tabi <timur@freescale.com>");
+MODULE_DESCRIPTION("Memory-mapped device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

[PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Timur Tabi]

Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
device, like an FPGA.  The device must be memory-mapped and contain only
8-bit registers (which keeps things simple).

Tested on a Freescale P5020DS board which uses the "PIXIS" FPGA attached
to the localbus.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 .../devicetree/bindings/net/mdio-mux-mmioreg.txt   |   73 ++++++++
 drivers/net/phy/Kconfig                            |   13 ++
 drivers/net/phy/Makefile                           |    1 +
 drivers/net/phy/mdio-mux-mmioreg.c                 |  185 ++++++++++++++++++++
 4 files changed, 272 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
 create mode 100644 drivers/net/phy/mdio-mux-mmioreg.c

diff --git a/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
new file mode 100644
index 0000000..251aa10
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
@@ -0,0 +1,73 @@
+Properties for an MDIO bus multiplexer controlled by a memory-mapped device
+
+This is a special case of a MDIO bus multiplexer.  A memory-mapped device,
+like an FPGA, is used to control which child bus is connected.  The mdio-mux
+node must be a child of the memory-mapped device.  The driver currently only
+supports devices with 8-bit registers.
+
+Required properties in addition to the generic multiplexer properties:
+
+- compatible : string, must contain "mdio-mux-mmioreg"
+
+- reg : integer, contains the offset of the register that
+	controls the bus multiplexer.
+
+- mux-mask : integer, contains an 8-bit mask that specifies which
+	bits in the register control the actual bus multiplexer.  The
+	'reg' property of each child mdio-mux node must be constrained by
+	this mask.
+
+Example:
+
+The FPGA node defines a memory-mapped FPGA with a register space of 0x30 bytes.
+For the "EMI2" MDIO bus, register 9 (BRDCFG1) controls the mux on that bus.
+A bitmask of 0x6 means that bits 1 and 2 (bit 0 is lsb) are the bits on
+BRDCFG1 that control the actual mux.
+
+	/* The FPGA node */
+	fpga: board-control@3,0 {
+		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
+		reg = <3 0 0x30>;
+
+		mdio-mux-emi2 {
+			compatible = "mdio-mux-mmioreg", "mdio-mux";
+			mdio-parent-bus = <&xmdio0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <9>; // BRDCFG1
+			mux-mask = <0x6>; // EMI2
+
+			emi2_slot1: mdio@0 {	// Slot 1 XAUI (FM2)
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy_xgmii_slot1: ethernet-phy@0 {
+					compatible = "ethernet-phy-ieee802.3-c45";
+					reg = <4>;
+				};
+			};
+
+			emi2_slot2: mdio@2 {	// Slot 2 XAUI (FM1)
+				reg = <2>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy_xgmii_slot2: ethernet-phy@4 {
+					compatible = "ethernet-phy-ieee802.3-c45";
+					reg = <0>;
+				};
+			};
+		};
+	};
+
+	/* The parent MDIO bus. */
+	xmdio0: mdio@f1000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "fsl,fman-xmdio";
+		reg = <0xf1000 0x1000>;
+		interrupts = <100 1 0 0>;
+	};
+
+
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3090dc6..0268edd 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -159,6 +159,19 @@ config MDIO_BUS_MUX_GPIO
 	  several child MDIO busses to a parent bus.  Child bus
 	  selection is under the control of GPIO lines.
 
+config MDIO_BUS_MUX_MMIOREG
+	tristate "Support for MMIO device-controlled MDIO bus multiplexers"
+	depends on OF_MDIO
+	select MDIO_BUS_MUX
+	help
+	  This module provides a driver for MDIO bus multiplexers that
+	  are controlled via a simple memory-mapped device, like an FPGA.
+	  The multiplexer connects one of several child MDIO busses to a
+	  parent bus.  Child bus selection is under the control of one of
+	  the FPGA's registers.
+
+	  Currently, only 8-bit registers are supported.
+
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 6d2dc6c..426674d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
 obj-$(CONFIG_MDIO_BUS_MUX_GPIO)	+= mdio-mux-gpio.o
+obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
new file mode 100644
index 0000000..e706c695
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -0,0 +1,185 @@
+/*
+ * Simple memory-mapped device MDIO MUX driver
+ *
+ * Author: Timur Tabi <timur@freescale.com>
+ *
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/phy.h>
+#include <linux/mdio-mux.h>
+
+struct mdio_mux_mmioreg_state {
+	void *mux_handle;
+	phys_addr_t phys;
+	unsigned int offset;
+	uint8_t mask;
+};
+
+/*
+ * MDIO multiplexing switch function
+ *
+ * This function is called by the mdio-mux layer when it thinks the mdio bus
+ * multiplexer needs to switch.
+ *
+ * 'current_child' is the current value of the mux register (masked via
+ * s->mask).
+ *
+ * 'desired_child' is the value of the 'reg' property of the target child MDIO
+ * node.
+ *
+ * The first time this function is called, current_child == -1.
+ *
+ * If current_child == desired_child, then the mux is already set to the
+ * correct bus.
+ */
+static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
+				      void *data)
+{
+	struct mdio_mux_mmioreg_state *s = data;
+
+	if (current_child ^ desired_child) {
+		void *p = ioremap(s->phys + s->offset, 1);
+		uint8_t x, y;
+
+		if (!p)
+			return -ENOMEM;
+
+		x = ioread8(p);
+		y = (x & ~s->mask) | desired_child;
+		if (x != y) {
+			iowrite8((x & ~s->mask) | desired_child, p);
+			pr_debug("%s: %02x -> %02x\n", __func__, x, y);
+		}
+
+		iounmap(p);
+	}
+
+	return 0;
+}
+
+static int __devinit mdio_mux_mmioreg_probe(struct platform_device *pdev)
+{
+	struct device_node *np2, *np = pdev->dev.of_node;
+	struct mdio_mux_mmioreg_state *s;
+	struct resource res;
+	const __be32 *iprop;
+	int len, ret;
+
+	dev_dbg(&pdev->dev, "probing node %s\n", np->full_name);
+
+	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	/* The MMIO device is the parent of this device */
+	np2 = of_get_parent(np);
+	if (!np2) {
+		dev_err(&pdev->dev, "could not find parent MMIO device\n");
+		return -ENODEV;
+	}
+
+	ret = of_address_to_resource(np2, 0, &res);
+	if (ret) {
+		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
+			np2->full_name);
+		return ret;
+	}
+	s->phys = res.start;
+
+	iprop = of_get_property(np, "reg", &len);
+	if (!iprop || len != sizeof(uint32_t)) {
+		dev_err(&pdev->dev, "missing or invalid 'reg' property\n");
+		return -EINVAL;
+	}
+	s->offset = be32_to_cpup(iprop);
+	if (s->offset >= resource_size(&res)) {
+		dev_err(&pdev->dev, "'reg' property value of %u is too large\n",
+			s->offset);
+		return -EINVAL;
+	}
+
+	iprop = of_get_property(np, "mux-mask", &len);
+	if (!iprop || len != sizeof(uint32_t)) {
+		dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
+		return -ENODEV;
+	}
+	if (be32_to_cpup(iprop) > 255) {
+		dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+		return -EINVAL;
+	}
+	s->mask = be32_to_cpup(iprop);
+
+	/*
+	 * Verify that the 'reg' property of each child MDIO bus does not
+	 * set any bits outside of the 'mask'.
+	 */
+	for_each_available_child_of_node(np, np2) {
+		iprop = of_get_property(np2, "reg", &len);
+		if (!iprop || len != sizeof(uint32_t)) {
+			dev_err(&pdev->dev, "mdio-mux child node %s is "
+				"missing a 'reg' property\n", np2->full_name);
+			return -ENODEV;
+		}
+		if (be32_to_cpup(iprop) & ~s->mask) {
+			dev_err(&pdev->dev, "mdio-mux child node %s has "
+				"a 'reg' value with unmasked bits\n",
+				np2->full_name);
+			return -ENODEV;
+		}
+	}
+
+	ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
+			    &s->mux_handle, s);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register mdio-mux bus %s\n",
+			np->full_name);
+		return ret;
+	}
+
+	pdev->dev.platform_data = s;
+
+	return 0;
+}
+
+static int __devexit mdio_mux_mmioreg_remove(struct platform_device *pdev)
+{
+	struct mdio_mux_mmioreg_state *s = dev_get_platdata(&pdev->dev);
+
+	mdio_mux_uninit(s->mux_handle);
+
+	return 0;
+}
+
+static struct of_device_id mdio_mux_mmioreg_match[] = {
+	{
+		.compatible = "mdio-mux-mmioreg",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mdio_mux_mmioreg_match);
+
+static struct platform_driver mdio_mux_mmioreg_driver = {
+	.driver = {
+		.name		= "mdio-mux-mmioreg",
+		.owner		= THIS_MODULE,
+		.of_match_table = mdio_mux_mmioreg_match,
+	},
+	.probe		= mdio_mux_mmioreg_probe,
+	.remove		= __devexit_p(mdio_mux_mmioreg_remove),
+};
+
+module_platform_driver(mdio_mux_mmioreg_driver);
+
+MODULE_AUTHOR("Timur Tabi <timur@freescale.com>");
+MODULE_DESCRIPTION("Memory-mapped device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Stephen Warren]

On 08/23/2012 01:44 PM, Timur Tabi wrote:
> Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
> device, like an FPGA.  The device must be memory-mapped and contain only
> 8-bit registers (which keeps things simple).

> +++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt

> +	/* The FPGA node */
> +	fpga: board-control@3,0 {
> +		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> +		reg = <3 0 0x30>;

Why not add the following here:

	#address-cells = <1>:
	#size-cells = <1>;
	ranges = <...>;

> +
> +		mdio-mux-emi2 {
> +			compatible = "mdio-mux-mmioreg", "mdio-mux";
> +			mdio-parent-bus = <&xmdio0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <9>; // BRDCFG1

Then, that'd have to be <9 1>;

That way, ...

> diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c

> +static int __devinit mdio_mux_mmioreg_probe(struct platform_device *pdev)

> +	/* The MMIO device is the parent of this device */
> +	np2 = of_get_parent(np);
> +	if (!np2) {
> +		dev_err(&pdev->dev, "could not find parent MMIO device\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_address_to_resource(np2, 0, &res);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
> +			np2->full_name);
> +		return ret;
> +	}
> +	s->phys = res.start;

You could simplify all that into just "of_iomap(np, 0)", and all the
address translation etc. happens entirely automatically, in the standard
fashion for DT.

The advantage here is that it completely decouples the mdio-mux-mmioreg
driver from any knowledge of its parent; you could just as easily use
this driver/binding as a device directly on some SoC bus, rather than
requiring there to be a parent device above it. After all, the mux
register might be some random standalone on-SoC register (although I
wonder if that case might not be better covered by a mdio-mux-pinctrl
driver instead; I guess it depends on how special-purpose the mux
register is).

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Tabi Timur-B04825]

Stephen Warren wrote:
> On 08/23/2012 01:44 PM, Timur Tabi wrote:
>> Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
>> device, like an FPGA.  The device must be memory-mapped and contain only
>> 8-bit registers (which keeps things simple).
>
>> +++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
>
>> +	/* The FPGA node */
>> +	fpga: board-control@3,0 {
>> +		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
>> +		reg = <3 0 0x30>;
>
> Why not add the following here:
>
> 	#address-cells = <1>:
> 	#size-cells = <1>;
> 	ranges = <...>;

I forgot to add them in the txt file.  They are in the real device tree.

>
>> +
>> +		mdio-mux-emi2 {
>> +			compatible = "mdio-mux-mmioreg", "mdio-mux";
>> +			mdio-parent-bus = <&xmdio0>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <9>; // BRDCFG1
>
> Then, that'd have to be <9 1>;

Actually, I had #size-cells = <0>.

>
> That way, ...
>
>> diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
>
>> +static int __devinit mdio_mux_mmioreg_probe(struct platform_device *pdev)
>
>> +	/* The MMIO device is the parent of this device */
>> +	np2 = of_get_parent(np);
>> +	if (!np2) {
>> +		dev_err(&pdev->dev, "could not find parent MMIO device\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = of_address_to_resource(np2, 0, &res);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
>> +			np2->full_name);
>> +		return ret;
>> +	}
>> +	s->phys = res.start;
>
> You could simplify all that into just "of_iomap(np, 0)", and all the
> address translation etc. happens entirely automatically, in the standard
> fashion for DT.

Ah!  You're right.  I don't need to look for the parent node.  Duh.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Stephen Warren]

On 08/23/2012 06:28 PM, Tabi Timur-B04825 wrote:
> Stephen Warren wrote:
>> On 08/23/2012 01:44 PM, Timur Tabi wrote:
>>> Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
>>> device, like an FPGA.  The device must be memory-mapped and contain only
>>> 8-bit registers (which keeps things simple).
>>
>>> +++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
>>
>>> +	/* The FPGA node */
>>> +	fpga: board-control@3,0 {
>>> +		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
>>> +		reg = <3 0 0x30>;
>>
>> Why not add the following here:
>>
>> 	#address-cells = <1>:
>> 	#size-cells = <1>;
>> 	ranges = <...>;
> 
> I forgot to add them in the txt file.  They are in the real device tree.
> 
>>
>>> +
>>> +		mdio-mux-emi2 {
>>> +			compatible = "mdio-mux-mmioreg", "mdio-mux";
>>> +			mdio-parent-bus = <&xmdio0>;
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <9>; // BRDCFG1
>>
>> Then, that'd have to be <9 1>;
> 
> Actually, I had #size-cells = <0>.

I think that if you have #size-cells=<0>, then you'll see the following
error message when attempting to translate the address into the parent's
address space:

prom_parse: Bad cell count for /board-control@3,0/mdio-mux-emi2

I fixed that error for cases where actual address translation isn't
required (i.e. coming up with the platform device name), but IIRC it'll
still trigger if you actually want to translate the address.

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Timur Tabi]

Stephen Warren wrote:
>>> >> Then, that'd have to be <9 1>;
>> > 
>> > Actually, I had #size-cells = <0>.

> I think that if you have #size-cells=<0>, then you'll see the following
> error message when attempting to translate the address into the parent's
> address space:
> 
> prom_parse: Bad cell count for /board-control@3,0/mdio-mux-emi2

It doesn't appear to be working.  Here's my tree:

/ {
	model = "fsl,P5020DS";
	compatible = "fsl,P5020DS";
	#address-cells = <2>;
	#size-cells = <2>;
	interrupt-parent = <&mpic>;

	lbc: localbus@ffe124000 {
		reg = <0xf 0xfe124000 0 0x1000>;
		ranges = <0 0 0xf 0xe8000000 0x08000000
			  2 0 0xf 0xffa00000 0x00040000
			  3 0 0xf 0xffdf0000 0x00008000>;

		fpga: board-control@3,0 {
			#address-cells = <1>;
			#size-cells = <1>;
			compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
			reg = <3 0 0x30>;

			mdio-mux-emi1 {
				compatible = "mdio-mux-mmioreg";
				mdio-parent-bus = <&mdio0>;
				#address-cells = <1>;
				#size-cells = <0>;
				reg = <9 1>; // BRDCFG1
				mux-mask = <0x78>; // EMI1

That means that the physical address that I need is fffdf0009.  However,
when I call of_address_to_resource(), the returned address I get is fe8000009.

So it's not picking up the "3" in the 'reg' property of the
board-control@3,0 node.  What am I missing?  Do I need a 'ranges' property
in the board-control@3,0 node?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Stephen Warren]

On 08/24/2012 10:27 AM, Timur Tabi wrote:
> Stephen Warren wrote:
>>>>>> Then, that'd have to be <9 1>;
>>>>
>>>> Actually, I had #size-cells = <0>.
> 
>> I think that if you have #size-cells=<0>, then you'll see the following
>> error message when attempting to translate the address into the parent's
>> address space:
>>
>> prom_parse: Bad cell count for /board-control@3,0/mdio-mux-emi2
> 
> It doesn't appear to be working.  Here's my tree:
> 
> / {
> 	model = "fsl,P5020DS";
> 	compatible = "fsl,P5020DS";
> 	#address-cells = <2>;
> 	#size-cells = <2>;
> 	interrupt-parent = <&mpic>;
> 
> 	lbc: localbus@ffe124000 {
> 		reg = <0xf 0xfe124000 0 0x1000>;
> 		ranges = <0 0 0xf 0xe8000000 0x08000000
> 			  2 0 0xf 0xffa00000 0x00040000
> 			  3 0 0xf 0xffdf0000 0x00008000>;
> 
> 		fpga: board-control@3,0 {
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> 			reg = <3 0 0x30>;
> 
> 			mdio-mux-emi1 {
> 				compatible = "mdio-mux-mmioreg";
> 				mdio-parent-bus = <&mdio0>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				reg = <9 1>; // BRDCFG1
> 				mux-mask = <0x78>; // EMI1
> 
> That means that the physical address that I need is fffdf0009.  However,
> when I call of_address_to_resource(), the returned address I get is fe8000009.
> 
> So it's not picking up the "3" in the 'reg' property of the
> board-control@3,0 node.  What am I missing?  Do I need a 'ranges' property
> in the board-control@3,0 node?

Yes.

When translating the child node's reg property into the parent's address
space, the parent's reg property shouldn't even be used at all; all the
mapping is done through the ranges property.

I thought the code error-checked for a missing ranges property, but I
guess not...

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Timur Tabi]

Stephen Warren wrote:
> When translating the child node's reg property into the parent's address
> space, the parent's reg property shouldn't even be used at all; all the
> mapping is done through the ranges property.
> 
> I thought the code error-checked for a missing ranges property, but I
> guess not...

I don't think 'ranges' is always necessary, because sometimes the child
nodes have a different address space that's not mapped to the parent.  For
instance, I2C devices have addresses that are not mapped to the I2C
controller itself.

Anyway, thanks to Scott for helping me figure this out.  I was missing a
ranges property:

	fpga: board-control@3,0 {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
		reg = <3 0 0x30>;
		ranges = <0 3 0 0x30>;

This maps the child address of 0 to the parent address of 3 0.  It seems
obvious now, but it was driving me crazy.  We've never put child devices
under our FPGA nodes, so there was no prior use case of a 'ranges'
property in any of the localbus devices that I could learn from.  Plus,
this is the first time we're probing directly on a child of a localbus device.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Scott Wood]

On 08/24/2012 01:36 PM, Timur Tabi wrote:
> Stephen Warren wrote:
>> When translating the child node's reg property into the parent's address
>> space, the parent's reg property shouldn't even be used at all; all the
>> mapping is done through the ranges property.
>>
>> I thought the code error-checked for a missing ranges property, but I
>> guess not...
> 
> I don't think 'ranges' is always necessary, because sometimes the child
> nodes have a different address space that's not mapped to the parent.  For
> instance, I2C devices have addresses that are not mapped to the I2C
> controller itself.
> 
> Anyway, thanks to Scott for helping me figure this out.  I was missing a
> ranges property:
> 
> 	fpga: board-control@3,0 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> 		reg = <3 0 0x30>;
> 		ranges = <0 3 0 0x30>;
> 
> This maps the child address of 0 to the parent address of 3 0.  It seems
> obvious now, but it was driving me crazy.  We've never put child devices
> under our FPGA nodes, so there was no prior use case of a 'ranges'
> property in any of the localbus devices that I could learn from.  Plus,
> this is the first time we're probing directly on a child of a localbus device.

There's ep8248e.dts, not that I'd have expected you to look there. :-)

-Scott

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Stephen Warren]

On 08/24/2012 12:36 PM, Timur Tabi wrote:
> Stephen Warren wrote:
>> When translating the child node's reg property into the parent's address
>> space, the parent's reg property shouldn't even be used at all; all the
>> mapping is done through the ranges property.
>>
>> I thought the code error-checked for a missing ranges property, but I
>> guess not...
> 
> I don't think 'ranges' is always necessary, because sometimes the child
> nodes have a different address space that's not mapped to the parent.  For
> instance, I2C devices have addresses that are not mapped to the I2C
> controller itself.

In the I2C case, the address spaces are disjoint, so there's never any
mapping between them, so there's no need for ranges.

Any time the child address space is intended to be part of the parent's
address space, I believe ranges is supposed to be specified, perhaps
even mandatory, even if the translation is 1:1.

> Anyway, thanks to Scott for helping me figure this out.  I was missing a
> ranges property:
> 
> 	fpga: board-control@3,0 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> 		reg = <3 0 0x30>;
> 		ranges = <0 3 0 0x30>;
> 
> This maps the child address of 0 to the parent address of 3 0.

Yes, that looks reasonable.

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[David Miller]

From: Stephen Warren <swarren@wwwdotorg.org>
Date: Fri, 24 Aug 2012 12:56:05 -0600

> In the I2C case, the address spaces are disjoint, so there's never any
> mapping between them, so there's no need for ranges.
> 
> Any time the child address space is intended to be part of the parent's
> address space, I believe ranges is supposed to be specified, perhaps
> even mandatory, even if the translation is 1:1.

Regardless, you really can't just generically translate ranges
in some universal way and expect it to work in all cases.

You need bus specific drivers to deal with various special
cases.

See the *_map() methods implemented in:

	arch/sparc/kernel/of_device_64.c

for example.

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Scott Wood]

On 08/24/2012 02:07 PM, David Miller wrote:
> From: Stephen Warren <swarren@wwwdotorg.org>
> Date: Fri, 24 Aug 2012 12:56:05 -0600
> 
>> In the I2C case, the address spaces are disjoint, so there's never any
>> mapping between them, so there's no need for ranges.
>>
>> Any time the child address space is intended to be part of the parent's
>> address space, I believe ranges is supposed to be specified, perhaps
>> even mandatory, even if the translation is 1:1.

Yes, it's mandatory (even if the kernel lets you get away without it for
the sake of some broken Apple firmware, IIRC).  If the translation is an
identity map you can use an empty "ranges;".

> Regardless, you really can't just generically translate ranges
> in some universal way and expect it to work in all cases.
> 
> You need bus specific drivers to deal with various special
> cases.
> 
> See the *_map() methods implemented in:
> 
> 	arch/sparc/kernel/of_device_64.c
> 
> for example.

We don't expect it to work in all cases.  We expect it to work if the
bus node is on the whitelist for which we create devices on
platform_bus, there's a platform driver that binds against it, and that
driver calls of_iomap() or equivalent because the binding says that reg
refers to something that is memory mapped.

-Scott

Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

[Scott Wood]

On 08/24/2012 02:07 PM, David Miller wrote:
> From: Stephen Warren <swarren@wwwdotorg.org>
> Date: Fri, 24 Aug 2012 12:56:05 -0600
> 
>> In the I2C case, the address spaces are disjoint, so there's never any
>> mapping between them, so there's no need for ranges.
>>
>> Any time the child address space is intended to be part of the parent's
>> address space, I believe ranges is supposed to be specified, perhaps
>> even mandatory, even if the translation is 1:1.

Yes, it's mandatory (even if the kernel lets you get away without it for
the sake of some broken Apple firmware, IIRC).  If the translation is an
identity map you can use an empty "ranges;".

> Regardless, you really can't just generically translate ranges
> in some universal way and expect it to work in all cases.
> 
> You need bus specific drivers to deal with various special
> cases.
> 
> See the *_map() methods implemented in:
> 
> 	arch/sparc/kernel/of_device_64.c
> 
> for example.

We don't expect it to work in all cases.  We expect it to work if the
bus node is on the whitelist for which we create devices on
platform_bus, there's a platform driver that binds against it, and that
driver calls of_iomap() or equivalent because the binding says that reg
refers to something that is memory mapped.

-Scott