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

[PATCH] [v3] 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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/net/mdio-mux-mmioreg.txt   |   77 +++++++++
 drivers/net/phy/Kconfig                            |   13 ++
 drivers/net/phy/Makefile                           |    1 +
 drivers/net/phy/mdio-mux-mmioreg.c                 |  170 ++++++++++++++++++++
 4 files changed, 261 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..aff8fd9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
@@ -0,0 +1,77 @@
+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 eight-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.  The size field in the 'reg' property is the size of
+	register, and must therefore be 1.
+
+- mux-mask : integer, contains an eight-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 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
+		reg = <3 0 0x30>;
+		ranges = <0 3 0 0x30>;
+
+		mdio-mux-emi2 {
+			compatible = "mdio-mux-mmioreg", "mdio-mux";
+			mdio-parent-bus = <&xmdio0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <9 1>; // 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..098239a
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -0,0 +1,170 @@
+/*
+ * Simple memory-mapped device MDIO MUX driver
+ *
+ * Author: Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+ *
+ * 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;
+	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, 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;
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret) {
+		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
+			np->full_name);
+		return ret;
+	}
+	s->phys = res.start;
+
+	if (resource_size(&res) != sizeof(uint8_t)) {
+		dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+		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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Memory-mapped device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

[PATCH] [v3] 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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/net/mdio-mux-mmioreg.txt   |   77 +++++++++
 drivers/net/phy/Kconfig                            |   13 ++
 drivers/net/phy/Makefile                           |    1 +
 drivers/net/phy/mdio-mux-mmioreg.c                 |  170 ++++++++++++++++++++
 4 files changed, 261 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..aff8fd9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
@@ -0,0 +1,77 @@
+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 eight-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.  The size field in the 'reg' property is the size of
+	register, and must therefore be 1.
+
+- mux-mask : integer, contains an eight-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 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
+		reg = <3 0 0x30>;
+		ranges = <0 3 0 0x30>;
+
+		mdio-mux-emi2 {
+			compatible = "mdio-mux-mmioreg", "mdio-mux";
+			mdio-parent-bus = <&xmdio0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <9 1>; // 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..098239a
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -0,0 +1,170 @@
+/*
+ * Simple memory-mapped device MDIO MUX driver
+ *
+ * Author: Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+ *
+ * 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;
+	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, 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;
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret) {
+		dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
+			np->full_name);
+		return ret;
+	}
+	s->phys = res.start;
+
+	if (resource_size(&res) != sizeof(uint8_t)) {
+		dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+		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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Memory-mapped device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.3.4

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

[Stephen Warren]

On 08/24/2012 01:10 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).
> 
> Tested on a Freescale P5020DS board which uses the "PIXIS" FPGA attached
> to the localbus.

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

> +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 eight-bit registers.

That last sentence seems like a property of the driver, not the binding;
I could easily anticipate allowing the size to be 1 or 2 or 4, and a
driver adapter to that in the future.

Otherwise, this binding looks great now.

> +++ b/drivers/net/phy/mdio-mux-mmioreg.c

> +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, 1);
> +		uint8_t x, y;
> +
> +		if (!p)
> +			return -ENOMEM;

Why not map it during probe?

> +		x = ioread8(p);
> +		y = (x & ~s->mask) | desired_child;
> +		if (x != y) {

Isn't that always true, given if (current_child ^ desired_child) above?

> +			iowrite8((x & ~s->mask) | desired_child, p);
> +			pr_debug("%s: %02x -> %02x\n", __func__, x, y);
> +		}
> +
> +		iounmap(p);
> +	}
> +
> +	return 0;
> +}

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

[Timur Tabi]

Stephen Warren wrote:

>> +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 eight-bit registers.
> 
> That last sentence seems like a property of the driver, not the binding;
> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
> driver adapter to that in the future.

True, but I couldn't think of a better place to mention this.  Adding
support for multi-byte registers also requires handling the endianness of
those registers.  I have that problem with the mdio-mux-gpio driver.  That
driver assumes that the GPIO bits are numbered in little-endian order, so
my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
reverse order.

> Otherwise, this binding looks great now.

Do you still want me to scrub any references to the register size
requirement from the document?

>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
> 
>> +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, 1);
>> +		uint8_t x, y;
>> +
>> +		if (!p)
>> +			return -ENOMEM;
> 
> Why not map it during probe?

I thought about that, but I generally don't like mappings that exist for
all eternity even though they're rarely used.  Once the interface is up,
we don't expect any bus muxing to occur.

> 
>> +		x = ioread8(p);
>> +		y = (x & ~s->mask) | desired_child;
>> +		if (x != y) {
> 
> Isn't that always true, given if (current_child ^ desired_child) above?

If current_child == -1, but the bus is already muxed properly, then
there's no point in setting it.  Do you want me to remove the test, or add
a comment?

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Timur Tabi]

Stephen Warren wrote:

>> +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 eight-bit registers.
> 
> That last sentence seems like a property of the driver, not the binding;
> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
> driver adapter to that in the future.

True, but I couldn't think of a better place to mention this.  Adding
support for multi-byte registers also requires handling the endianness of
those registers.  I have that problem with the mdio-mux-gpio driver.  That
driver assumes that the GPIO bits are numbered in little-endian order, so
my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
reverse order.

> Otherwise, this binding looks great now.

Do you still want me to scrub any references to the register size
requirement from the document?

>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
> 
>> +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, 1);
>> +		uint8_t x, y;
>> +
>> +		if (!p)
>> +			return -ENOMEM;
> 
> Why not map it during probe?

I thought about that, but I generally don't like mappings that exist for
all eternity even though they're rarely used.  Once the interface is up,
we don't expect any bus muxing to occur.

> 
>> +		x = ioread8(p);
>> +		y = (x & ~s->mask) | desired_child;
>> +		if (x != y) {
> 
> Isn't that always true, given if (current_child ^ desired_child) above?

If current_child == -1, but the bus is already muxed properly, then
there's no point in setting it.  Do you want me to remove the test, or add
a comment?

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Stephen Warren]

On 08/24/2012 02:00 PM, Timur Tabi wrote:
> Stephen Warren wrote:
> 
>>> +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 eight-bit registers.
>>
>> That last sentence seems like a property of the driver, not the binding;
>> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
>> driver adapter to that in the future.
> 
> True, but I couldn't think of a better place to mention this.  Adding
> support for multi-byte registers also requires handling the endianness of
> those registers.  I have that problem with the mdio-mux-gpio driver.  That
> driver assumes that the GPIO bits are numbered in little-endian order, so
> my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
> reverse order.

True. One could always simply assume that the registers are native
endian by default, and then if ever they need not to be, add optional
properties to the binding.

>> Otherwise, this binding looks great now.
> 
> Do you still want me to scrub any references to the register size
> requirement from the document?

I don't feel too strongly about it. It seems cleaner to, but not a big deal.

>>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
>>
>>> +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, 1);
>>> +		uint8_t x, y;
>>> +
>>> +		if (!p)
>>> +			return -ENOMEM;
>>
>> Why not map it during probe?
> 
> I thought about that, but I generally don't like mappings that exist for
> all eternity even though they're rarely used.  Once the interface is up,
> we don't expect any bus muxing to occur.
> 
>>
>>> +		x = ioread8(p);
>>> +		y = (x & ~s->mask) | desired_child;
>>> +		if (x != y) {
>>
>> Isn't that always true, given if (current_child ^ desired_child) above?
> 
> If current_child == -1, but the bus is already muxed properly, then
> there's no point in setting it.  Do you want me to remove the test, or add
> a comment?

Ah right, I suppose that is true. It almost doesn't seem worth writing
the code to ignore that special case, since presumably the register
write is idempotent, but since it's already there you can feel free not
to rip it out!

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

[David Miller]

From: Timur Tabi <timur@freescale.com>
Date: Fri, 24 Aug 2012 14:10:53 -0500

> 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>

Applied to net-next, thanks.