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

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

[Pankaj Bansal]

Hi Andrew

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, October 5, 2018 9:22 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org;
> Alexandru Marginean <alexandru.marginean@nxp.com>
> Subject: Re: [PATCH 2/2] netdev/phy: add MDIO bus multiplexer driven by a
> regmap
> 
> On Fri, Oct 05, 2018 at 01:59:26PM +0530, 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/Kconfig           |  13 ++
> >  drivers/net/phy/Makefile          |   1 +
> >  drivers/net/phy/mdio-mux-regmap.c | 177
> ++++++++++++++++++++++++++++
> >  3 files changed, 191 insertions(+)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > 82070792edbb..bb894ede7464 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
> >
> >  	  Currently, only 8/16/32 bits registers are supported.
> >
> > +config MDIO_BUS_MUX_REGMAP
> > +	tristate "REGMAP controlled MDIO bus multiplexers"
> > +	depends on OF_MDIO && HAS_IOMEM
> 
> Hi Pankaj
> 
> Doesn't it also depend on REGMAP?


I will add

> 
> > +	select MDIO_BUS_MUX
> > +	help
> > +	  This module provides a driver for MDIO bus multiplexers that
> > +	  are controlled via a regmap, 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 upto 32 bits registers are supported.
> > +
> >  config MDIO_CAVIUM
> >  	tristate
> >
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 5805c0b7d60e..33053f9f320d 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-
> mux.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
> > +obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
> >  obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
> >  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
> >  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.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..b0882273fd47
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-mux-regmap.c
> > @@ -0,0 +1,177 @@
> > +// 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.bansal@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 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 = 0;
> > +
> 
> No need to initialise ret.

Ok

> 
> > +	ret = regmap_update_bits_check(s->regmap,
> > +				       s->mux_reg,
> > +				       s->mask,
> > +				       desired_child,
> > +				       &change);
> 
> When getting the mask from DT, you use be32_to_cpup().
> When testing the reg value against the mask, you use be32_to_cpup().
> Here you do not use be32_to_cpup()?

Can you please tell me for which variable you mean I should use be32_to_cpup?
I use be32_to_cpup when reading device tree entries.
After being read, their values are stored in structure. After that no need to do be32_to_cpup

> 
> > +
> > +	if (ret)
> > +		goto out;
> 
> Just do the return here.

Ok

> 
> > +	if (change)
> > +		pr_debug("%s %d -> %d\n", __func__, current_child,
> > +			 desired_child);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int mdio_mux_regmap_probe(struct platform_device *pdev) {
> > +	struct device_node *np2, *np = pdev->dev.of_node;
> > +	struct mdio_mux_regmap_state *s;
> > +	const __be32 *iprop;
> > +	int len, ret;
> > +	u32 val;
> > +
> > +	dev_dbg(&pdev->dev, "probing node %pOF\n", np);
> > +
> > +	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
> > +	if (!s)
> > +		return -ENOMEM;
> > +
> > +	s->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +	if (IS_ERR(s->regmap)) {
> > +		dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > +		ret = PTR_ERR(s->regmap);
> > +		return ret;
> 
> return PTR_ERR(s->regmap);

Ok

> 
> > +	}
> > +
> > +	iprop = of_get_property(np, "reg", &len);
> > +	if (!iprop || len != sizeof(u32)) {
> > +		dev_err(&pdev->dev, "missing or invalid reg property\n");
> > +		return -ENODEV;
> > +	}
> > +	s->mux_reg = (u32)be32_to_cpup(iprop);
> > +
> > +	/* Test Register read write */
> > +	ret = regmap_read(s->regmap, s->mux_reg, &val);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "error while reading reg\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_write(s->regmap, s->mux_reg, val);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "error while writing reg\n");
> > +		return ret;
> > +	}
> > +
> > +	iprop = of_get_property(np, "mux-mask", &len);
> > +	if (!iprop || len != sizeof(u32)) {
> > +		dev_err(&pdev->dev, "missing or invalid mux-mask
> property\n");
> > +		return -ENODEV;
> > +	}
> > +	s->mask = (uint32_t)be32_to_cpup(iprop);
> 
> Is this cast needed?

I will remove the typecasts.

> 
> > +
> > +	/* 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(u32)) {
> > +			dev_err(&pdev->dev, "mdio-mux child node %pOF is
> missing a 'reg' property\n", np2);
> > +			of_node_put(np2);
> > +			return -ENODEV;
> > +		}
> > +		if (be32_to_cpup(iprop) & ~s->mask) {
> > +			dev_err(&pdev->dev, "mdio-mux child node %pOF
> has a 'reg' value with unmasked bits\n", np2);
> > +			of_node_put(np2);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> > +			    mdio_mux_regmap_switch_fn,
> > +			    &s->mux_handle, s, NULL);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev,
> > +				"failed to register mdio-mux bus %pOF\n",
> np);
> > +		return ret;
> > +	}
> > +
> > +	pdev->dev.platform_data = s;
> > +	return 0;
> > +}
> > +
> > +static int mdio_mux_regmap_remove(struct platform_device *pdev) {
> > +	struct mdio_mux_regmap_state *s = dev_get_platdata(&pdev-
> >dev);
> > +
> > +	mdio_mux_uninit(s->mux_handle);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mdio_mux_regmap_match[] = {
> > +	{
> > +		.compatible = "mdio-mux-regmap",
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mdio_mux_regmap_match);
> > +
> > +static struct platform_driver mdio_mux_regmap_driver = {
> > +	.driver = {
> > +		.name		= "mdio-mux-regmap",
> > +		.of_match_table = mdio_mux_regmap_match,
> > +	},
> > +	.probe		= mdio_mux_regmap_probe,
> > +	.remove		= mdio_mux_regmap_remove,
> > +};
> > +
> > +module_platform_driver(mdio_mux_regmap_driver);
> > +
> > +MODULE_AUTHOR("Pankaj Bansal <pankaj.bansal@nxp.com>");
> > +MODULE_DESCRIPTION("I2c device MDIO MUX driver");
> 
> I2C should be replaced by regmap.

Nice catch. I will replace it.

> 
>     Andrew

RE: [PATCH 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device

[Pankaj Bansal]

Hi Andrew

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, October 5, 2018 8:38 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org;
> Alexandru Marginean <alexandru.marginean@nxp.com>
> Subject: Re: [PATCH 1/2] dt-bindings: net: add MDIO bus multiplexer driven
> by a regmap device
> 
> > +i2c@2000000 {
> > +	compatible = "fsl,vf610-i2c";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	reg = <0x0 0x2000000 0x0 0x10000>;
> > +	interrupts = <0 34 0x4>; // Level high type
> > +	clock-names = "i2c";
> > +	clocks = <&clockgen 4 7>;
> > +	fsl-scl-gpio = <&gpio2 15 0>;
> > +	status = "okay";
> > +	/* The FPGA node */
> > +        fpga@66 {
> > +                compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > +		reg = <0x66>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		mdio1_mux@54 {
> > +			compatible = "mdio-mux-regmap", "mdio-mux";
> > +			mdio-parent-bus = <&emdio2>; /* MDIO bus */
> > +			reg = <0x54>;		 /* BRDCFG4 */
> > +			mux-mask = <0x07>;      /* EMI2_MDIO */
> > +
> > +			#address-cells=<1>;
> > +			#size-cells = <0>;
> > +
> > +			mdio1_ioslot1@0 {   // Slot 1
> > +				reg = <0x00>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +                                phy1@1 {
> 
> Hi Pankaj
> 
> It looks like you have tab vs space issues.

I will fix the formatting error in V2

> 
> > +                                        reg = <1>;
> > +                                        compatible = "ethernet-phy-id0210.7441";
> > +                                };
> 
> A blank line after here please.

OK

> > +                                phy1@0 {
> > +                                        reg = <0>;
> > +                                        compatible = "ethernet-phy-id0210.7441";
> > +                                };
> > +                        };
> 
> And here

Ok

> 
> > +			mdio1_ioslot2@1 {   // Slot 2
> > +				reg = <0x01>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +			};
> 
> and here.

Ok

> 
>     Andrew

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

[Andrew Lunn]

> > > +	ret = regmap_update_bits_check(s->regmap,
> > > +				       s->mux_reg,
> > > +				       s->mask,
> > > +				       desired_child,
> > > +				       &change);
> > 
> > When getting the mask from DT, you use be32_to_cpup().
> > When testing the reg value against the mask, you use be32_to_cpup().
> > Here you do not use be32_to_cpup()?
> 
> Can you please tell me for which variable you mean I should use be32_to_cpup?
> I use be32_to_cpup when reading device tree entries.
> After being read, their values are stored in structure. After that no need to do be32_to_cpup

desired_child is read from DT by the mdio-mux core.

I'm just wondering why any of this be32_to_cpup() is needed.  Why not
just use of_property_read_u32()?

     Andrew

Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device

[Florian Fainelli]

On 10/07/18 11:24, 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>
> ---
> 
> Notes:
>     V2:
>      - Fixed formatting error caused by using space instead of tab
>      - Add newline between property list and subnode
>      - Add newline between two subnodes
> 
>  .../bindings/net/mdio-mux-regmap.txt         | 95 ++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> new file mode 100644
> index 000000000000..88ebac26c1c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> @@ -0,0 +1,95 @@
> +Properties for an MDIO bus multiplexer controlled by a regmap
> +
> +This is a special case of a MDIO bus multiplexer.  A regmap device,
> +like an FPGA, is used to control which child bus is connected.  The mdio-mux
> +node must be a child of the device that is controlled by a regmap.
> +The driver currently only supports devices with upto 32-bit registers.

I would omit any sort of details about Linux constructs designed to
support specific needs (e.g: regmap) as well as putting driver
limitations into a binding document because it really ought to be
restricted to describing hardware.

> +
> +Required properties in addition to the generic multiplexer properties:
> +
> +- compatible : string, must contain "mdio-mux-regmap"
> +
> +- reg : integer, contains the offset of the register that controls the bus
> +	multiplexer. it can be 32 bit number.

Technically it could be any "reg" property size, the only requirement is
that it must be of the appropriate size with respect to what the parent
node of this "mdio-mux-regmap" node has, determined by
#address-cells/#size-cells width.

> +
> +- mux-mask : integer, contains an 32 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.

Same thing here.

Since this is a MDIO mux, I would invite you to specify what the correct
#address-cells/#size-cells values should be (1, and 0 respectively as
your example correctly shows).

> +
> +Example:
> +
> +The FPGA node defines a i2c connected FPGA with a register space of 0x30 bytes.
> +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux on that bus.
> +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the bits on
> +BRDCFG4 that control the actual mux.
> +
> +i2c@2000000 {
> +	compatible = "fsl,vf610-i2c";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x0 0x2000000 0x0 0x10000>;
> +	interrupts = <0 34 0x4>; // Level high type
> +	clock-names = "i2c";
> +	clocks = <&clockgen 4 7>;
> +	fsl-scl-gpio = <&gpio2 15 0>;
> +	status = "okay";
> +
> +	/* The FPGA node */
> +	fpga@66 {
> +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> +		reg = <0x66>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		mdio1_mux@54 {
> +			compatible = "mdio-mux-regmap", "mdio-mux";
> +			mdio-parent-bus = <&emdio2>; /* MDIO bus */
> +			reg = <0x54>;		 /* BRDCFG4 */
> +			mux-mask = <0x07>;      /* EMI2_MDIO */
> +			#address-cells=<1>;
> +			#size-cells = <0>;
> +
> +			mdio1_ioslot1@0 {   // Slot 1
> +				reg = <0x00>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				phy1@1 {
> +					reg = <1>;
> +					compatible = "ethernet-phy-id0210.7441";
> +				};
> +
> +				phy1@0 {
> +					reg = <0>;
> +					compatible = "ethernet-phy-id0210.7441";
> +				};
> +			};
> +
> +			mdio1_ioslot2@1 {   // Slot 2
> +				reg = <0x01>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +			};
> +
> +			mdio1_ioslot3@2 {   // Slot 3
> +				reg = <0x02>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +			};
> +		};
> +	};
> +};
> +
> +	/* The parent MDIO bus. */
> +	emdio2: mdio@0x8B97000 {
> +		compatible = "fsl,fman-memac-mdio";
> +		reg = <0x0 0x8B97000 0x0 0x1000>;
> +		device_type = "mdio";
> +		little-endian;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> 

-- 
Florian

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

[Florian Fainelli]

On 10/07/18 11:24, 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>
> ---
> 
> Notes:
>     V2:
>      - replaced be32_to_cpup with of_property_read_u32
>      - incorporated Andrew's comments
> 
>  drivers/net/phy/Kconfig           |  13 +++
>  drivers/net/phy/Makefile          |   1 +
>  drivers/net/phy/mdio-mux-regmap.c | 171 ++++++++++++++++++++++++++++
>  3 files changed, 185 insertions(+)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 82070792edbb..d1ac9e70cbb2 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
>  
>  	  Currently, only 8/16/32 bits registers are supported.
>  
> +config MDIO_BUS_MUX_REGMAP
> +	tristate "REGMAP controlled MDIO bus multiplexers"
> +	depends on OF_MDIO && REGMAP
> +	select MDIO_BUS_MUX
> +	help
> +	  This module provides a driver for MDIO bus multiplexers that
> +	  are controlled via a regmap device, like an FPGA connected to i2c.
> +	  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 upto 32 bits registers are supported.
> +
>  config MDIO_CAVIUM
>  	tristate
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 5805c0b7d60e..33053f9f320d 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.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
> +obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
>  obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
>  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
>  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.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..6068d05a728a
> --- /dev/null
> +++ b/drivers/net/phy/mdio-mux-regmap.c
> @@ -0,0 +1,171 @@
> +// 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.bansal@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 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)
> +		pr_debug("%s %d -> %d\n", __func__, current_child,
> +			 desired_child);

If you add a struct platform_device or struct device reference to struct
mdio_mux_regmap_state, the you can use dev_dbg() here with the correct
device, which would be helpful if you are debugging problems, and there
are more than once instance of them in the system.

> +	return ret;
> +}
> +
> +static int mdio_mux_regmap_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np2, *np = pdev->dev.of_node;

How about naming "np2", "child" instead?

Everything else looks fine to me, thanks!
-- 
Florian

[PATCH v2 0/2] 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.

This driver 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 a driver that uses regmap device to control mdio mux.

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

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


Pankaj Bansal (2):
  dt-bindings: net: add MDIO bus multiplexer driven by a regmap device
  netdev/phy: add MDIO bus multiplexer driven by a regmap

 .../bindings/net/mdio-mux-regmap.txt          |  95 ++++++++++
 drivers/net/phy/Kconfig                       |  13 ++
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/mdio-mux-regmap.c             | 171 ++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
 create mode 100644 drivers/net/phy/mdio-mux-regmap.c

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: net: 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.

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

Notes:
    V2:
     - Fixed formatting error caused by using space instead of tab
     - Add newline between property list and subnode
     - Add newline between two subnodes

 .../bindings/net/mdio-mux-regmap.txt         | 95 ++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
new file mode 100644
index 000000000000..88ebac26c1c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
@@ -0,0 +1,95 @@
+Properties for an MDIO bus multiplexer controlled by a regmap
+
+This is a special case of a MDIO bus multiplexer.  A regmap device,
+like an FPGA, is used to control which child bus is connected.  The mdio-mux
+node must be a child of the device that is controlled by a regmap.
+The driver currently only supports devices with upto 32-bit registers.
+
+Required properties in addition to the generic multiplexer properties:
+
+- compatible : string, must contain "mdio-mux-regmap"
+
+- reg : integer, contains the offset of the register that controls the bus
+	multiplexer. it can be 32 bit number.
+
+- mux-mask : integer, contains an 32 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 i2c connected FPGA with a register space of 0x30 bytes.
+For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux on that bus.
+A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the bits on
+BRDCFG4 that control the actual mux.
+
+i2c@2000000 {
+	compatible = "fsl,vf610-i2c";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x0 0x2000000 0x0 0x10000>;
+	interrupts = <0 34 0x4>; // Level high type
+	clock-names = "i2c";
+	clocks = <&clockgen 4 7>;
+	fsl-scl-gpio = <&gpio2 15 0>;
+	status = "okay";
+
+	/* The FPGA node */
+	fpga@66 {
+		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
+		reg = <0x66>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mdio1_mux@54 {
+			compatible = "mdio-mux-regmap", "mdio-mux";
+			mdio-parent-bus = <&emdio2>; /* MDIO bus */
+			reg = <0x54>;		 /* BRDCFG4 */
+			mux-mask = <0x07>;      /* EMI2_MDIO */
+			#address-cells=<1>;
+			#size-cells = <0>;
+
+			mdio1_ioslot1@0 {   // Slot 1
+				reg = <0x00>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy1@1 {
+					reg = <1>;
+					compatible = "ethernet-phy-id0210.7441";
+				};
+
+				phy1@0 {
+					reg = <0>;
+					compatible = "ethernet-phy-id0210.7441";
+				};
+			};
+
+			mdio1_ioslot2@1 {   // Slot 2
+				reg = <0x01>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+			};
+
+			mdio1_ioslot3@2 {   // Slot 3
+				reg = <0x02>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+			};
+		};
+	};
+};
+
+	/* The parent MDIO bus. */
+	emdio2: mdio@0x8B97000 {
+		compatible = "fsl,fman-memac-mdio";
+		reg = <0x0 0x8B97000 0x0 0x1000>;
+		device_type = "mdio";
+		little-endian;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
-- 
2.17.1

[PATCH v2 2/2] 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>
---

Notes:
    V2:
     - replaced be32_to_cpup with of_property_read_u32
     - incorporated Andrew's comments

 drivers/net/phy/Kconfig           |  13 +++
 drivers/net/phy/Makefile          |   1 +
 drivers/net/phy/mdio-mux-regmap.c | 171 ++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 82070792edbb..d1ac9e70cbb2 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
 
 	  Currently, only 8/16/32 bits registers are supported.
 
+config MDIO_BUS_MUX_REGMAP
+	tristate "REGMAP controlled MDIO bus multiplexers"
+	depends on OF_MDIO && REGMAP
+	select MDIO_BUS_MUX
+	help
+	  This module provides a driver for MDIO bus multiplexers that
+	  are controlled via a regmap device, like an FPGA connected to i2c.
+	  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 upto 32 bits registers are supported.
+
 config MDIO_CAVIUM
 	tristate
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 5805c0b7d60e..33053f9f320d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.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
+obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
 obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.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..6068d05a728a
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-regmap.c
@@ -0,0 +1,171 @@
+// 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.bansal@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 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)
+		pr_debug("%s %d -> %d\n", __func__, current_child,
+			 desired_child);
+	return ret;
+}
+
+static int mdio_mux_regmap_probe(struct platform_device *pdev)
+{
+	struct device_node *np2, *np = pdev->dev.of_node;
+	struct mdio_mux_regmap_state *s;
+	int ret;
+	u32 val;
+
+	dev_dbg(&pdev->dev, "probing node %pOF\n", np);
+
+	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	s->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (IS_ERR(s->regmap)) {
+		dev_err(&pdev->dev, "Failed to get parent regmap\n");
+		return PTR_ERR(s->regmap);
+	}
+
+	ret = of_property_read_u32(np, "reg", &s->mux_reg);
+	if (ret) {
+		dev_err(&pdev->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(&pdev->dev, "error while reading reg\n");
+		return ret;
+	}
+
+	ret = regmap_write(s->regmap, s->mux_reg, val);
+	if (ret) {
+		dev_err(&pdev->dev, "error while writing reg\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "mux-mask", &s->mask);
+	if (ret) {
+		dev_err(&pdev->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(np, np2) {
+		ret = of_property_read_u32(np2, "reg", &val);
+		if (ret) {
+			dev_err(&pdev->dev, "mdio-mux child node %pOF is missing a 'reg' property\n", np2);
+			of_node_put(np2);
+			return -ENODEV;
+		}
+		if (val & ~s->mask) {
+			dev_err(&pdev->dev, "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n", np2);
+			of_node_put(np2);
+			return -ENODEV;
+		}
+	}
+
+	ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
+			    mdio_mux_regmap_switch_fn,
+			    &s->mux_handle, s, NULL);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"failed to register mdio-mux bus %pOF\n", np);
+		return ret;
+	}
+
+	pdev->dev.platform_data = s;
+
+	return 0;
+}
+
+static int mdio_mux_regmap_remove(struct platform_device *pdev)
+{
+	struct mdio_mux_regmap_state *s = dev_get_platdata(&pdev->dev);
+
+	mdio_mux_uninit(s->mux_handle);
+
+	return 0;
+}
+
+static const struct of_device_id mdio_mux_regmap_match[] = {
+	{
+		.compatible = "mdio-mux-regmap",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mdio_mux_regmap_match);
+
+static struct platform_driver mdio_mux_regmap_driver = {
+	.driver = {
+		.name		= "mdio-mux-regmap",
+		.of_match_table = mdio_mux_regmap_match,
+	},
+	.probe		= mdio_mux_regmap_probe,
+	.remove		= mdio_mux_regmap_remove,
+};
+
+module_platform_driver(mdio_mux_regmap_driver);
+
+MODULE_AUTHOR("Pankaj Bansal <pankaj.bansal@nxp.com>");
+MODULE_DESCRIPTION("regmap device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

RE: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device

[Pankaj Bansal]

Hi Florian

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Sunday, October 7, 2018 11:32 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by
> a regmap device
> 
> 
> 
> On 10/07/18 11:24, 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>
> > ---
> >
> > Notes:
> >     V2:
> >      - Fixed formatting error caused by using space instead of tab
> >      - Add newline between property list and subnode
> >      - Add newline between two subnodes
> >
> >  .../bindings/net/mdio-mux-regmap.txt         | 95 ++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > new file mode 100644
> > index 000000000000..88ebac26c1c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > @@ -0,0 +1,95 @@
> > +Properties for an MDIO bus multiplexer controlled by a regmap
> > +
> > +This is a special case of a MDIO bus multiplexer.  A regmap device,
> > +like an FPGA, is used to control which child bus is connected.  The
> > +mdio-mux node must be a child of the device that is controlled by a regmap.
> > +The driver currently only supports devices with upto 32-bit registers.
> 
> I would omit any sort of details about Linux constructs designed to support
> specific needs (e.g: regmap) as well as putting driver limitations into a binding
> document because it really ought to be restricted to describing hardware.
> 

Actually the platform driver mdio-mux-regmap.c, is generalization of mdio-mux-mmioreg.c
As such, it doesn't describe any new hardware, so no new properties are described by it.
The only new property is compatible field.
I don't know how to describe this driver otherwise.  Can you please help?

> > +
> > +Required properties in addition to the generic multiplexer properties:
> > +
> > +- compatible : string, must contain "mdio-mux-regmap"
> > +
> > +- reg : integer, contains the offset of the register that controls the bus
> > +	multiplexer. it can be 32 bit number.
> 
> Technically it could be any "reg" property size, the only requirement is that it
> must be of the appropriate size with respect to what the parent node of this
> "mdio-mux-regmap" node has, determined by #address-cells/#size-cells width.

We are reading only single cell of this property using "of_propert_read_u32".
That is why I put the size in this.

> 
> > +
> > +- mux-mask : integer, contains an 32 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.
> 
> Same thing here.

We are reading only single cell of this property using "of_propert_read_u32".
That is why I put the size in this.

> 
> Since this is a MDIO mux, I would invite you to specify what the correct
> #address-cells/#size-cells values should be (1, and 0 respectively as your
> example correctly shows).
> 

I will mention #address-cells/#size-cells values

> > +
> > +Example:
> > +
> > +The FPGA node defines a i2c connected FPGA with a register space of 0x30
> bytes.
> > +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux on that
> bus.
> > +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the
> > +bits on
> > +BRDCFG4 that control the actual mux.
> > +
> > +i2c@2000000 {
> > +	compatible = "fsl,vf610-i2c";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	reg = <0x0 0x2000000 0x0 0x10000>;
> > +	interrupts = <0 34 0x4>; // Level high type
> > +	clock-names = "i2c";
> > +	clocks = <&clockgen 4 7>;
> > +	fsl-scl-gpio = <&gpio2 15 0>;
> > +	status = "okay";
> > +
> > +	/* The FPGA node */
> > +	fpga@66 {
> > +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > +		reg = <0x66>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		mdio1_mux@54 {
> > +			compatible = "mdio-mux-regmap", "mdio-mux";
> > +			mdio-parent-bus = <&emdio2>; /* MDIO bus */
> > +			reg = <0x54>;		 /* BRDCFG4 */
> > +			mux-mask = <0x07>;      /* EMI2_MDIO */
> > +			#address-cells=<1>;
> > +			#size-cells = <0>;
> > +
> > +			mdio1_ioslot1@0 {   // Slot 1
> > +				reg = <0x00>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				phy1@1 {
> > +					reg = <1>;
> > +					compatible = "ethernet-phy-
> id0210.7441";
> > +				};
> > +
> > +				phy1@0 {
> > +					reg = <0>;
> > +					compatible = "ethernet-phy-
> id0210.7441";
> > +				};
> > +			};
> > +
> > +			mdio1_ioslot2@1 {   // Slot 2
> > +				reg = <0x01>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +			};
> > +
> > +			mdio1_ioslot3@2 {   // Slot 3
> > +				reg = <0x02>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +	/* The parent MDIO bus. */
> > +	emdio2: mdio@0x8B97000 {
> > +		compatible = "fsl,fman-memac-mdio";
> > +		reg = <0x0 0x8B97000 0x0 0x1000>;
> > +		device_type = "mdio";
> > +		little-endian;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +	};
> >
> 
> --
> Florian

RE: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a regmap

[Pankaj Bansal]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Sunday, October 7, 2018 11:39 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a
> regmap
> 
> 
> 
> On 10/07/18 11:24, 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>
> > ---
> >
> > Notes:
> >     V2:
> >      - replaced be32_to_cpup with of_property_read_u32
> >      - incorporated Andrew's comments
> >
> >  drivers/net/phy/Kconfig           |  13 +++
> >  drivers/net/phy/Makefile          |   1 +
> >  drivers/net/phy/mdio-mux-regmap.c | 171 ++++++++++++++++++++++++++++
> >  3 files changed, 185 insertions(+)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > 82070792edbb..d1ac9e70cbb2 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
> >
> >  	  Currently, only 8/16/32 bits registers are supported.
> >
> > +config MDIO_BUS_MUX_REGMAP
> > +	tristate "REGMAP controlled MDIO bus multiplexers"
> > +	depends on OF_MDIO && REGMAP
> > +	select MDIO_BUS_MUX
> > +	help
> > +	  This module provides a driver for MDIO bus multiplexers that
> > +	  are controlled via a regmap device, like an FPGA connected to i2c.
> > +	  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 upto 32 bits registers are supported.
> > +
> >  config MDIO_CAVIUM
> >  	tristate
> >
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 5805c0b7d60e..33053f9f320d 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.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
> > +obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
> >  obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
> >  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
> >  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.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..6068d05a728a
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-mux-regmap.c
> > @@ -0,0 +1,171 @@
> > +// 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.bansal@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 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)
> > +		pr_debug("%s %d -> %d\n", __func__, current_child,
> > +			 desired_child);
> 
> If you add a struct platform_device or struct device reference to struct
> mdio_mux_regmap_state, the you can use dev_dbg() here with the correct
> device, which would be helpful if you are debugging problems, and there are
> more than once instance of them in the system.

Ok, I will add platform_device reference to struct.

> 
> > +	return ret;
> > +}
> > +
> > +static int mdio_mux_regmap_probe(struct platform_device *pdev) {
> > +	struct device_node *np2, *np = pdev->dev.of_node;
> 
> How about naming "np2", "child" instead?

Ok. I will rename this variable

> 
> Everything else looks fine to me, thanks!
> --
> Florian

RE: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device

[Pankaj Bansal]

> -----Original Message-----
> From: Pankaj Bansal
> Sent: Thursday, October 18, 2018 10:00 AM
> To: Florian Fainelli <f.fainelli@gmail.com>; Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by
> a regmap device
> 
> Hi Florian
> 
> > -----Original Message-----
> > From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> > Sent: Sunday, October 7, 2018 11:32 PM
> > To: Pankaj Bansal <pankaj.bansal@nxp.com>; Andrew Lunn
> > <andrew@lunn.ch>
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer
> > driven by a regmap device
> >
> >
> >
> > On 10/07/18 11:24, 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>
> > > ---
> > >
> > > Notes:
> > >     V2:
> > >      - Fixed formatting error caused by using space instead of tab
> > >      - Add newline between property list and subnode
> > >      - Add newline between two subnodes
> > >
> > >  .../bindings/net/mdio-mux-regmap.txt         | 95 ++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > new file mode 100644
> > > index 000000000000..88ebac26c1c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > @@ -0,0 +1,95 @@
> > > +Properties for an MDIO bus multiplexer controlled by a regmap
> > > +
> > > +This is a special case of a MDIO bus multiplexer.  A regmap device,
> > > +like an FPGA, is used to control which child bus is connected.  The
> > > +mdio-mux node must be a child of the device that is controlled by a
> regmap.
> > > +The driver currently only supports devices with upto 32-bit registers.
> >
> > I would omit any sort of details about Linux constructs designed to
> > support specific needs (e.g: regmap) as well as putting driver
> > limitations into a binding document because it really ought to be restricted to
> describing hardware.
> >
> 
> Actually the platform driver mdio-mux-regmap.c, is generalization of mdio-mux-
> mmioreg.c As such, it doesn't describe any new hardware, so no new properties
> are described by it.
> The only new property is compatible field.
> I don't know how to describe this driver otherwise.  Can you please help?

I further thought about it. mdio-mux-regmap.c 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.

Rather this driver should expose the APIs, which should be called by parent devices' driver.

What is your thought on this ?

> 
> > > +
> > > +Required properties in addition to the generic multiplexer properties:
> > > +
> > > +- compatible : string, must contain "mdio-mux-regmap"
> > > +
> > > +- reg : integer, contains the offset of the register that controls the bus
> > > +	multiplexer. it can be 32 bit number.
> >
> > Technically it could be any "reg" property size, the only requirement
> > is that it must be of the appropriate size with respect to what the
> > parent node of this "mdio-mux-regmap" node has, determined by #address-
> cells/#size-cells width.
> 
> We are reading only single cell of this property using "of_propert_read_u32".
> That is why I put the size in this.
> 
> >
> > > +
> > > +- mux-mask : integer, contains an 32 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.
> >
> > Same thing here.
> 
> We are reading only single cell of this property using "of_propert_read_u32".
> That is why I put the size in this.
> 
> >
> > Since this is a MDIO mux, I would invite you to specify what the
> > correct #address-cells/#size-cells values should be (1, and 0
> > respectively as your example correctly shows).
> >
> 
> I will mention #address-cells/#size-cells values
> 
> > > +
> > > +Example:
> > > +
> > > +The FPGA node defines a i2c connected FPGA with a register space of
> > > +0x30
> > bytes.
> > > +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux
> > > +on that
> > bus.
> > > +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the
> > > +bits on
> > > +BRDCFG4 that control the actual mux.
> > > +
> > > +i2c@2000000 {
> > > +	compatible = "fsl,vf610-i2c";
> > > +	#address-cells = <1>;
> > > +	#size-cells = <0>;
> > > +	reg = <0x0 0x2000000 0x0 0x10000>;
> > > +	interrupts = <0 34 0x4>; // Level high type
> > > +	clock-names = "i2c";
> > > +	clocks = <&clockgen 4 7>;
> > > +	fsl-scl-gpio = <&gpio2 15 0>;
> > > +	status = "okay";
> > > +
> > > +	/* The FPGA node */
> > > +	fpga@66 {
> > > +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > > +		reg = <0x66>;
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +
> > > +		mdio1_mux@54 {
> > > +			compatible = "mdio-mux-regmap", "mdio-mux";
> > > +			mdio-parent-bus = <&emdio2>; /* MDIO bus */
> > > +			reg = <0x54>;		 /* BRDCFG4 */
> > > +			mux-mask = <0x07>;      /* EMI2_MDIO */
> > > +			#address-cells=<1>;
> > > +			#size-cells = <0>;
> > > +
> > > +			mdio1_ioslot1@0 {   // Slot 1
> > > +				reg = <0x00>;
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +
> > > +				phy1@1 {
> > > +					reg = <1>;
> > > +					compatible = "ethernet-phy-
> > id0210.7441";
> > > +				};
> > > +
> > > +				phy1@0 {
> > > +					reg = <0>;
> > > +					compatible = "ethernet-phy-
> > id0210.7441";
> > > +				};
> > > +			};
> > > +
> > > +			mdio1_ioslot2@1 {   // Slot 2
> > > +				reg = <0x01>;
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +
> > > +			};
> > > +
> > > +			mdio1_ioslot3@2 {   // Slot 3
> > > +				reg = <0x02>;
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +
> > > +			};
> > > +		};
> > > +	};
> > > +};
> > > +
> > > +	/* The parent MDIO bus. */
> > > +	emdio2: mdio@0x8B97000 {
> > > +		compatible = "fsl,fman-memac-mdio";
> > > +		reg = <0x0 0x8B97000 0x0 0x1000>;
> > > +		device_type = "mdio";
> > > +		little-endian;
> > > +
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +	};
> > >
> >
> > --
> > Florian

Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device

[Florian Fainelli]

On 10/21/18 9:22 PM, Pankaj Bansal wrote:
> 
>> -----Original Message-----
>> From: Pankaj Bansal
>> Sent: Thursday, October 18, 2018 10:00 AM
>> To: Florian Fainelli <f.fainelli@gmail.com>; Andrew Lunn <andrew@lunn.ch>
>> Cc: netdev@vger.kernel.org
>> Subject: RE: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by
>> a regmap device
>>
>> Hi Florian
>>
>>> -----Original Message-----
>>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>>> Sent: Sunday, October 7, 2018 11:32 PM
>>> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Andrew Lunn
>>> <andrew@lunn.ch>
>>> Cc: netdev@vger.kernel.org
>>> Subject: Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer
>>> driven by a regmap device
>>>
>>>
>>>
>>> On 10/07/18 11:24, 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>
>>>> ---
>>>>
>>>> Notes:
>>>>     V2:
>>>>      - Fixed formatting error caused by using space instead of tab
>>>>      - Add newline between property list and subnode
>>>>      - Add newline between two subnodes
>>>>
>>>>  .../bindings/net/mdio-mux-regmap.txt         | 95 ++++++++++++++++++
>>>>  1 file changed, 95 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
>>>> b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
>>>> new file mode 100644
>>>> index 000000000000..88ebac26c1c5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
>>>> @@ -0,0 +1,95 @@
>>>> +Properties for an MDIO bus multiplexer controlled by a regmap
>>>> +
>>>> +This is a special case of a MDIO bus multiplexer.  A regmap device,
>>>> +like an FPGA, is used to control which child bus is connected.  The
>>>> +mdio-mux node must be a child of the device that is controlled by a
>> regmap.
>>>> +The driver currently only supports devices with upto 32-bit registers.
>>>
>>> I would omit any sort of details about Linux constructs designed to
>>> support specific needs (e.g: regmap) as well as putting driver
>>> limitations into a binding document because it really ought to be restricted to
>> describing hardware.
>>>
>>
>> Actually the platform driver mdio-mux-regmap.c, is generalization of mdio-mux-
>> mmioreg.c As such, it doesn't describe any new hardware, so no new properties
>> are described by it.
>> The only new property is compatible field.
>> I don't know how to describe this driver otherwise.  Can you please help?
> 
> I further thought about it. mdio-mux-regmap.c 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.
> 
> Rather this driver should expose the APIs, which should be called by parent devices' driver.

Which one is "this driver" in that context? If you already have a driver
that FPGA bus/piece of hardware, then that driver itself could be
offering the MDIO muxing capabilities directly. You might still want to
have some Device Tree representation to properly parent the PHY devices
to the branches of the mux.

> 
> What is your thought on this ?
> 
>>
>>>> +
>>>> +Required properties in addition to the generic multiplexer properties:
>>>> +
>>>> +- compatible : string, must contain "mdio-mux-regmap"
>>>> +
>>>> +- reg : integer, contains the offset of the register that controls the bus
>>>> +	multiplexer. it can be 32 bit number.
>>>
>>> Technically it could be any "reg" property size, the only requirement
>>> is that it must be of the appropriate size with respect to what the
>>> parent node of this "mdio-mux-regmap" node has, determined by #address-
>> cells/#size-cells width.
>>
>> We are reading only single cell of this property using "of_propert_read_u32".
>> That is why I put the size in this.
>>
>>>
>>>> +
>>>> +- mux-mask : integer, contains an 32 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.
>>>
>>> Same thing here.
>>
>> We are reading only single cell of this property using "of_propert_read_u32".
>> That is why I put the size in this.
>>
>>>
>>> Since this is a MDIO mux, I would invite you to specify what the
>>> correct #address-cells/#size-cells values should be (1, and 0
>>> respectively as your example correctly shows).
>>>
>>
>> I will mention #address-cells/#size-cells values
>>
>>>> +
>>>> +Example:
>>>> +
>>>> +The FPGA node defines a i2c connected FPGA with a register space of
>>>> +0x30
>>> bytes.
>>>> +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux
>>>> +on that
>>> bus.
>>>> +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the
>>>> +bits on
>>>> +BRDCFG4 that control the actual mux.
>>>> +
>>>> +i2c@2000000 {
>>>> +	compatible = "fsl,vf610-i2c";
>>>> +	#address-cells = <1>;
>>>> +	#size-cells = <0>;
>>>> +	reg = <0x0 0x2000000 0x0 0x10000>;
>>>> +	interrupts = <0 34 0x4>; // Level high type
>>>> +	clock-names = "i2c";
>>>> +	clocks = <&clockgen 4 7>;
>>>> +	fsl-scl-gpio = <&gpio2 15 0>;
>>>> +	status = "okay";
>>>> +
>>>> +	/* The FPGA node */
>>>> +	fpga@66 {
>>>> +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
>>>> +		reg = <0x66>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		mdio1_mux@54 {
>>>> +			compatible = "mdio-mux-regmap", "mdio-mux";
>>>> +			mdio-parent-bus = <&emdio2>; /* MDIO bus */
>>>> +			reg = <0x54>;		 /* BRDCFG4 */
>>>> +			mux-mask = <0x07>;      /* EMI2_MDIO */
>>>> +			#address-cells=<1>;
>>>> +			#size-cells = <0>;
>>>> +
>>>> +			mdio1_ioslot1@0 {   // Slot 1
>>>> +				reg = <0x00>;
>>>> +				#address-cells = <1>;
>>>> +				#size-cells = <0>;
>>>> +
>>>> +				phy1@1 {
>>>> +					reg = <1>;
>>>> +					compatible = "ethernet-phy-
>>> id0210.7441";
>>>> +				};
>>>> +
>>>> +				phy1@0 {
>>>> +					reg = <0>;
>>>> +					compatible = "ethernet-phy-
>>> id0210.7441";
>>>> +				};
>>>> +			};
>>>> +
>>>> +			mdio1_ioslot2@1 {   // Slot 2
>>>> +				reg = <0x01>;
>>>> +				#address-cells = <1>;
>>>> +				#size-cells = <0>;
>>>> +
>>>> +			};
>>>> +
>>>> +			mdio1_ioslot3@2 {   // Slot 3
>>>> +				reg = <0x02>;
>>>> +				#address-cells = <1>;
>>>> +				#size-cells = <0>;
>>>> +
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +	/* The parent MDIO bus. */
>>>> +	emdio2: mdio@0x8B97000 {
>>>> +		compatible = "fsl,fman-memac-mdio";
>>>> +		reg = <0x0 0x8B97000 0x0 0x1000>;
>>>> +		device_type = "mdio";
>>>> +		little-endian;
>>>> +
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +	};
>>>>
>>>
>>> --
>>> Florian


-- 
Florian