[PATCH 0/1] add MDIO bus multiplexer driven by a regmap device

[PATCH 0/1] add MDIO bus multiplexer driven by a regmap device

[Pankaj Bansal]

Add support for an MDIO bus multiplexer controlled by a regmap device, like an 
FPGA.

These apis is an extension of the existing driver 
drivers/net/phy/mdio-mux-mmioreg.c.

The problem with mmioreg driver is that it can operate only on memory mapped 
devices.
but if we have a device that controls mdio muxing and that device is controlled 
using
i2c or spi, then it will not work.

Therefore, added apis that can be used by regmap device to control mdio mux.

Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA attached to the 
i2c bus.

This is my second attempt at this.
In my previous approach i wrote a separate driver for regmap apis. But then i
realized that it is not meant to control a specific device.
It is meant to control some registers of parent device. Therefore, IMO this 
should not be a
Platform driver and there should not be any "compatible" property to which this 
driver is associated.

The previous approach patches and discussion can be accessed here:
https://www.mail-archive.com/netdev@vger.kernel.org/msg252744.html

Cc: Varun Sethi <V.Sethi@nxp.com>

Pankaj Bansal (1):
  netdev/phy: add MDIO bus multiplexer driven by a regmap

 drivers/net/phy/Makefile          |   2 +-
 drivers/net/phy/mdio-mux-regmap.c | 170 ++++++++++++++++++++++++++++++
 include/linux/mdio-mux.h          |  20 ++++
 3 files changed, 191 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/mdio-mux-regmap.c

-- 
2.17.1

[PATCH 1/1] netdev/phy: add MDIO bus multiplexer driven by a regmap

[Pankaj Bansal]

Add support for an MDIO bus multiplexer controlled by a regmap
device, like an FPGA.

Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
attached to the i2c bus.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 drivers/net/phy/Makefile          |   2 +-
 drivers/net/phy/mdio-mux-regmap.c | 170 ++++++++++++++++++++++++++++
 include/linux/mdio-mux.h          |  20 ++++
 3 files changed, 191 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f41b14115fde..16145973a42f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -25,7 +25,7 @@ obj-$(CONFIG_PHYLIB)		+= libphy.o
 obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
 obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
-obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
+obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o mdio-mux-regmap.o
 obj-$(CONFIG_MDIO_BUS_MUX_BCM_IPROC)	+= mdio-mux-bcm-iproc.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-regmap.c b/drivers/net/phy/mdio-mux-regmap.c
new file mode 100644
index 000000000000..ca63a44da25f
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-regmap.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/* Simple regmap based MDIO MUX driver
+ *
+ * Copyright 2018 NXP
+ *
+ * Based on mdio-mux-mmioreg.c by Timur Tabi
+ *
+ * Author:
+ *     Pankaj Bansal <pankaj.ban...@nxp.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/mdio-mux.h>
+#include <linux/regmap.h>
+
+struct mdio_mux_regmap_state {
+	void		*mux_handle;
+	struct device	*dev;
+	struct regmap	*regmap;
+	u32		mux_reg;
+	u32		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_regmap_switch_fn(int current_child, int desired_child,
+				     void *data)
+{
+	struct mdio_mux_regmap_state *s = data;
+	bool change;
+	int ret;
+
+	ret = regmap_update_bits_check(s->regmap,
+				       s->mux_reg,
+				       s->mask,
+				       desired_child,
+				       &change);
+
+	if (ret)
+		return ret;
+	if (change)
+		dev_dbg(s->dev, "%s %d -> %d\n", __func__, current_child,
+			desired_child);
+	return ret;
+}
+
+/**
+ * mdio_mux_regmap_init - control MDIO bus muxing using regmap constructs.
+ * @dev: device with which regmap construct is associated.
+ * @mux_node: mdio bus mux node that contains parent mdio bus phandle.
+ *	      This node also contains sub nodes, where each subnode denotes
+ *	      a child mdio bus. All the child mdio buses are muxed, i.e. at a
+ *	      time only one of the child mdio buses can be used.
+ * @data: to store the address of data allocated by this function
+ */
+int mdio_mux_regmap_init(struct device *dev,
+			 struct device_node *mux_node,
+			 void **data)
+{
+	struct device_node *child;
+	struct mdio_mux_regmap_state *s;
+	int ret;
+	u32 val;
+
+	dev_dbg(dev, "probing node %pOF\n", mux_node);
+
+	s = devm_kzalloc(dev, sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	s->regmap = dev_get_regmap(dev, NULL);
+	if (IS_ERR(s->regmap)) {
+		dev_err(dev, "Failed to get parent regmap\n");
+		return PTR_ERR(s->regmap);
+	}
+
+	ret = of_property_read_u32(mux_node, "reg", &s->mux_reg);
+	if (ret) {
+		dev_err(dev, "missing or invalid reg property\n");
+		return -ENODEV;
+	}
+
+	/* Test Register read write */
+	ret = regmap_read(s->regmap, s->mux_reg, &val);
+	if (ret) {
+		dev_err(dev, "error while reading reg\n");
+		return ret;
+	}
+
+	ret = regmap_write(s->regmap, s->mux_reg, val);
+	if (ret) {
+		dev_err(dev, "error while writing reg\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(mux_node, "mux-mask", &s->mask);
+	if (ret) {
+		dev_err(dev, "missing or invalid mux-mask property\n");
+		return -ENODEV;
+	}
+
+	/* 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(mux_node, child) {
+		ret = of_property_read_u32(child, "reg", &val);
+		if (ret) {
+			dev_err(dev, "mdio-mux child node %pOF is missing a 'reg' property\n", child);
+			of_node_put(child);
+			return -ENODEV;
+		}
+		if (val & ~s->mask) {
+			dev_err(dev, "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n", child);
+			of_node_put(child);
+			return -ENODEV;
+		}
+	}
+
+	ret = mdio_mux_init(dev, mux_node, mdio_mux_regmap_switch_fn,
+			    &s->mux_handle, s, NULL);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to register mdio-mux bus %pOF\n", mux_node);
+		return ret;
+	}
+
+	*data = s;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mdio_mux_regmap_init);
+
+/**
+ * mdio_mux_regmap_uninit - relinquish the control of MDIO bus muxing using
+ *			    regmap constructs.
+ * @data: address of data allocated by mdio_mux_regmap_init
+ */
+int mdio_mux_regmap_uninit(void *data)
+{
+	struct mdio_mux_regmap_state *s = data;
+
+	mdio_mux_uninit(s->mux_handle);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mdio_mux_regmap_uninit);
+
+MODULE_AUTHOR("Pankaj Bansal <pankaj.bansal@nxp.com>");
+MODULE_DESCRIPTION("regmap based MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
index a5d58f221939..231cfa3ba429 100644
--- a/include/linux/mdio-mux.h
+++ b/include/linux/mdio-mux.h
@@ -29,4 +29,24 @@ int mdio_mux_init(struct device *dev,
 
 void mdio_mux_uninit(void *mux_handle);
 
+/**
+ * mdio_mux_regmap_init - control MDIO bus muxing using regmap constructs.
+ * @dev: device with which regmap construct is associated.
+ * @mux_node: mdio bus mux node that contains parent mdio bus phandle.
+ *	      This node also contains sub nodes, where each subnode denotes
+ *	      a child mdio bus. All the child mdio buses are muxed, i.e. at a
+ *	      time only one of the child mdio buses can be used.
+ * @data: to store the address of data allocated by this function
+ */
+int mdio_mux_regmap_init(struct device *dev,
+			 struct device_node *mux_node,
+			 void **data);
+
+/**
+ * mdio_mux_regmap_uninit - relinquish the control of MDIO bus muxing using
+ *			    regmap constructs.
+ * @data: address of data allocated by mdio_mux_regmap_init
+ */
+int mdio_mux_regmap_uninit(void *data);
+
 #endif /* __LINUX_MDIO_MUX_H */
-- 
2.17.1

Re: [PATCH 0/1] add MDIO bus multiplexer driven by a regmap device

[Andrew Lunn]

On Wed, Jan 30, 2019 at 11:21:57AM +0000, Pankaj Bansal wrote:
> Add support for an MDIO bus multiplexer controlled by a regmap device, like an 
> FPGA.
> 
> These apis is an extension of the existing driver 
> drivers/net/phy/mdio-mux-mmioreg.c.
> 
> The problem with mmioreg driver is that it can operate only on memory mapped 
> devices.
> but if we have a device that controls mdio muxing and that device is controlled 
> using
> i2c or spi, then it will not work.
> 
> Therefore, added apis that can be used by regmap device to control mdio mux.
> 
> Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA attached to the 
> i2c bus.
> 
> This is my second attempt at this.
> In my previous approach i wrote a separate driver for regmap apis. But then i
> realized that it is not meant to control a specific device.
> It is meant to control some registers of parent device. Therefore, IMO this 
> should not be a
> Platform driver and there should not be any "compatible" property to which this 
> driver is associated.

Hi Pankaj

It is not clear to me how you actually use this. You also need to
document the device tree binding. It could be when you write that
documentation it then becomes clear how it should be used.

Do you have patches adding support for this to the LX2160AQDS?  Seeing
that would also help.

Thanks
	Andrew

Re: [PATCH 1/1] netdev/phy: add MDIO bus multiplexer driven by a regmap

[Andrew Lunn]

> +++ b/drivers/net/phy/mdio-mux-regmap.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0+

..

> +MODULE_LICENSE("GPL v2");

These are not consistent.

      Andrew

Re: [PATCH 1/1] netdev/phy: add MDIO bus multiplexer driven by a regmap

[Andrew Lunn]

On Wed, Jan 30, 2019 at 11:22:00AM +0000, Pankaj Bansal wrote:
> Add support for an MDIO bus multiplexer controlled by a regmap
> device, like an FPGA.
> 
> Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
> attached to the i2c bus.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  drivers/net/phy/Makefile          |   2 +-
>  drivers/net/phy/mdio-mux-regmap.c | 170 ++++++++++++++++++++++++++++
>  include/linux/mdio-mux.h          |  20 ++++
>  3 files changed, 191 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index f41b14115fde..16145973a42f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -25,7 +25,7 @@ obj-$(CONFIG_PHYLIB)		+= libphy.o
>  obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
>  obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
>  obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
> -obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
> +obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o mdio-mux-regmap.o

Please add a new KCONFIG symbol for it. And think about the
depend/select statement, since you need regmap.

> +/* 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.
> + */

Please use kerneldoc formatting for this function documentation.

> +int mdio_mux_regmap_init(struct device *dev,
> +			 struct device_node *mux_node,
> +			 void **data)
> +{

> +	/* 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(mux_node, child) {
> +		ret = of_property_read_u32(child, "reg", &val);
> +		if (ret) {
> +			dev_err(dev, "mdio-mux child node %pOF is missing a 'reg' property\n", child);

You can probably remove "mdio-mux child node " making the line < 80,
but still retain the meaning. The child node name should be sufficient
to identify it.


> +			of_node_put(child);
> +			return -ENODEV;
> +		}
> +		if (val & ~s->mask) {
> +			dev_err(dev, "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n", child);

Same here.

     Andrew

RE: [PATCH 0/1] add MDIO bus multiplexer driven by a regmap device

[Pankaj Bansal]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Wednesday, 30 January, 2019 08:53 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org; Varun Sethi
> <V.Sethi@nxp.com>
> Subject: Re: [PATCH 0/1] add MDIO bus multiplexer driven by a regmap device
> 
> On Wed, Jan 30, 2019 at 11:21:57AM +0000, Pankaj Bansal wrote:
> > Add support for an MDIO bus multiplexer controlled by a regmap device,
> > like an FPGA.
> >
> > These apis is an extension of the existing driver
> > drivers/net/phy/mdio-mux-mmioreg.c.
> >
> > The problem with mmioreg driver is that it can operate only on memory
> > mapped devices.
> > but if we have a device that controls mdio muxing and that device is
> > controlled using i2c or spi, then it will not work.
> >
> > Therefore, added apis that can be used by regmap device to control mdio mux.
> >
> > Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA attached
> > to the i2c bus.
> >
> > This is my second attempt at this.
> > In my previous approach i wrote a separate driver for regmap apis. But
> > then i realized that it is not meant to control a specific device.
> > It is meant to control some registers of parent device. Therefore, IMO
> > this should not be a Platform driver and there should not be any
> > "compatible" property to which this driver is associated.
> 
> Hi Pankaj
> 
> It is not clear to me how you actually use this. You also need to document the
> device tree binding. It could be when you write that documentation it then
> becomes clear how it should be used.
> 

This is not a new driver for a device. Which is why there is no compatible field that I have used in these APIs.
Should I create a new binding document for it ?

> Do you have patches adding support for this to the LX2160AQDS?  Seeing that
> would also help.

DTS representation: https://patchwork.ozlabs.org/patch/998278/
Usage Patch : https://patchwork.kernel.org/patch/10788345/

> 
> Thanks
> 	Andrew

Re: [PATCH 0/1] add MDIO bus multiplexer driven by a regmap device

[Andrew Lunn]

> This is not a new driver for a device. Which is why there is no compatible field that I have used in these APIs.
> Should I create a new binding document for it ?

Yes, you always need to document the binding.

     Andrew