[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Alex Marginean]

This driver is used for MDIO muxes driven over I2C.  This is currently
used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
controlled by an on-board FPGA which in turn is configured through I2C.

Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
---

Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124

 drivers/net/Kconfig            |   8 +++
 drivers/net/Makefile           |   1 +
 drivers/net/mdio_mux_i2c_reg.c | 108 +++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 drivers/net/mdio_mux_i2c_reg.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 4d85fb1716..93535f7acb 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -595,4 +595,12 @@ config FSL_ENETC
 	  This driver supports the NXP ENETC Ethernet controller found on some
 	  of the NXP SoCs.
 
+config MDIO_MUX_I2C_REG
+	bool "MDIO MUX accessed as a register over I2C"
+	depends on DM_MDIO_MUX && DM_I2C
+	help
+	  This driver is used for MDIO muxes driven by writing to a register of
+	  an I2C chip.  The board it was developed for uses a mux controlled by
+	  on-board FPGA which in turn is accessed as a chip over I2C.
+
 endif # NETDEVICES
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 97119cec7c..9470f02dc6 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -80,3 +80,4 @@ obj-$(CONFIG_HIGMACV300_ETH) += higmacv300.o
 obj-$(CONFIG_MDIO_SANDBOX) += mdio_sandbox.o
 obj-$(CONFIG_FSL_ENETC) += fsl_enetc.o fsl_enetc_mdio.o
 obj-$(CONFIG_MDIO_MUX_SANDBOX) += mdio_mux_sandbox.o
+obj-$(CONFIG_MDIO_MUX_I2C_REG) += mdio_mux_i2c_reg.o
diff --git a/drivers/net/mdio_mux_i2c_reg.c b/drivers/net/mdio_mux_i2c_reg.c
new file mode 100644
index 0000000000..eb75c5e032
--- /dev/null
+++ b/drivers/net/mdio_mux_i2c_reg.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2019
+ * Alex Marginean, NXP
+ */
+
+#include <dm.h>
+#include <errno.h>
+#include <miiphy.h>
+#include <i2c.h>
+
+/*
+ * This driver is used for MDIO muxes driven by writing to a register of an I2C
+ * chip.  The board it was developed for uses a mux controlled by on-board FPGA
+ * which in turn is accessed as a chip over I2C.
+ */
+
+struct mmux_i2c_reg_priv {
+	struct udevice *chip;
+	int reg;
+	int mask;
+};
+
+static int mmux_i2c_reg_select(struct udevice *mux, int cur, int sel)
+{
+	struct mmux_i2c_reg_priv *priv = dev_get_priv(mux);
+	u8 val, val_old;
+
+	/* if last selection didn't change we're good to go */
+	if (cur == sel)
+		return 0;
+
+	val_old = dm_i2c_reg_read(priv->chip, priv->reg);
+	val = (val_old & ~priv->mask) | (sel & priv->mask);
+	debug("%s: chip %s, reg %x, val %x => %x\n", __func__, priv->chip->name,
+	      priv->reg, val_old, val);
+	dm_i2c_reg_write(priv->chip, priv->reg, val);
+
+	return 0;
+}
+
+static const struct mdio_mux_ops mmux_i2c_reg_ops = {
+	.select = mmux_i2c_reg_select,
+};
+
+static int mmux_i2c_reg_probe(struct udevice *dev)
+{
+	struct mmux_i2c_reg_priv *priv = dev_get_priv(dev);
+	ofnode chip_node, bus_node;
+	struct udevice *i2c_bus;
+	u32 reg_mask[2];
+	u32 chip_addr;
+	int err;
+
+	/* read the register addr/mask pair */
+	err = dev_read_u32_array(dev, "mux-reg-masks", reg_mask, 2);
+	if (err) {
+		debug("%s: error reading mux-reg-masks property\n", __func__);
+		return err;
+	}
+
+	/* parent should be an I2C chip, grandparent should be an I2C bus */
+	chip_node = ofnode_get_parent(dev->node);
+	bus_node = ofnode_get_parent(chip_node);
+
+	err = uclass_get_device_by_ofnode(UCLASS_I2C, bus_node, &i2c_bus);
+	if (err) {
+		debug("%s: can't find I2C bus for node %s\n", __func__,
+		      ofnode_get_name(bus_node));
+		return err;
+	}
+
+	err = ofnode_read_u32(chip_node, "reg", &chip_addr);
+	if (err) {
+		debug("%s: can't read chip address in %s\n", __func__,
+		      ofnode_get_name(chip_node));
+		return err;
+	}
+
+	err = i2c_get_chip(i2c_bus, (uint)chip_addr, 1, &priv->chip);
+	if (err) {
+		debug("%s: can't find i2c chip device for addr %x\n", __func__,
+		      chip_addr);
+		return err;
+	}
+
+	priv->reg = (int)reg_mask[0];
+	priv->mask = (int)reg_mask[1];
+
+	debug("%s: chip %s, reg %x, mask %x\n", __func__, priv->chip->name,
+	      priv->reg, priv->mask);
+
+	return 0;
+}
+
+static const struct udevice_id mmux_i2c_reg_ids[] = {
+	{ .compatible = "mdio-mux-i2c-reg" },
+	{ }
+};
+
+U_BOOT_DRIVER(mmux_i2c_reg) = {
+	.name		= "mdio_mux_i2c_reg",
+	.id		= UCLASS_MDIO_MUX,
+	.of_match	= mmux_i2c_reg_ids,
+	.probe		= mmux_i2c_reg_probe,
+	.ops		= &mmux_i2c_reg_ops,
+	.priv_auto_alloc_size = sizeof(struct mmux_i2c_reg_priv),
+};
-- 
2.17.1

[U-Boot] [PATCH 2/2] doc: bindings: Add binding for register driven MDIO muxes

[Alex Marginean]

This binding documents two properties that describe the registers used to
perform MUX selection.

Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
---
 doc/device-tree-bindings/net/mdio-mux-reg.txt | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 doc/device-tree-bindings/net/mdio-mux-reg.txt

diff --git a/doc/device-tree-bindings/net/mdio-mux-reg.txt b/doc/device-tree-bindings/net/mdio-mux-reg.txt
new file mode 100644
index 0000000000..13cfe9117b
--- /dev/null
+++ b/doc/device-tree-bindings/net/mdio-mux-reg.txt
@@ -0,0 +1,64 @@
+Device tree structures used by register based MDIO muxes is described here.
+This binding is based on reg-mux.txt binding in Linux and is currently used by
+mdio-mux-i2c-reg driver in U-Boot.
+
+Required properties:
+#mux-control-cells = <1> indicates how many registers are used for mux
+			selection.  mux-reg-mask property described below must
+			include this number of pairs.
+mux-reg-masks = <reg mask> describes pairs of register offset and register mask.
+			Register bits enabled in mask are set to the selection
+			value defined in reg property of child MDIOs to control
+			selection.
+Properties described in mdio-mux.txt also apply.
+
+Example structure, used on Freescale LS1028A QDS board:
+
+&i2c0 {
+	status = "okay";
+	u-boot,dm-pre-reloc;
+
+	fpga at 66 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "simple-mfd";
+		reg = <0x66>;
+
+		mux-mdio at 54 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "mdio-mux-i2c-reg";
+			reg = <0x54>;
+			#mux-control-cells = <1>;
+			mux-reg-masks = <0x54 0xf0>;
+			mdio-parent-bus = <&mdio0>;
+
+			/* on-board MDIO with a single RGMII PHY */
+			mdio at 00 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0x00>;
+
+				/* on-board 1G RGMII PHY */
+				qds_phy0: phy at 5 {
+					reg = <5>;
+				};
+			};
+			/* slot 1 */
+			mdio at 40 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0x40>;
+				/* VSC8234 1G SGMII card */
+				sgmii_port0: phy at 1c {
+					reg = <0x1c>;
+				};
+			};
+		};
+	};
+};
+
+/* Parent MDIO, defined in SoC .dtsi file, just enabled here */
+&mdio0 {
+	status = "okay";
+};
-- 
2.17.1

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Bin Meng]

Hi Alex,

On Fri, Jul 12, 2019 at 10:21 PM Alex Marginean
<alexandru.marginean@nxp.com> wrote:
>
> This driver is used for MDIO muxes driven over I2C.  This is currently
> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
> controlled by an on-board FPGA which in turn is configured through I2C.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>
> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124
>
>  drivers/net/Kconfig            |   8 +++
>  drivers/net/Makefile           |   1 +
>  drivers/net/mdio_mux_i2c_reg.c | 108 +++++++++++++++++++++++++++++++++
>  3 files changed, 117 insertions(+)
>  create mode 100644 drivers/net/mdio_mux_i2c_reg.c
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 4d85fb1716..93535f7acb 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -595,4 +595,12 @@ config FSL_ENETC
>           This driver supports the NXP ENETC Ethernet controller found on some
>           of the NXP SoCs.
>
> +config MDIO_MUX_I2C_REG

I don't see current Linux upstream has any I2CREG support. I believe
you will upstream the Linux support too?

So based on existing Linux kernel MMIOREG support, (see
mdio-mux-mmioreg.txt in the Linux devicetree bindings direoctory),
looks we need name this as MDIO_MUX_I2CREG for consistency.

> +       bool "MDIO MUX accessed as a register over I2C"
> +       depends on DM_MDIO_MUX && DM_I2C
> +       help
> +         This driver is used for MDIO muxes driven by writing to a register of
> +         an I2C chip.  The board it was developed for uses a mux controlled by
> +         on-board FPGA which in turn is accessed as a chip over I2C.
> +
>  endif # NETDEVICES
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 97119cec7c..9470f02dc6 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_HIGMACV300_ETH) += higmacv300.o
>  obj-$(CONFIG_MDIO_SANDBOX) += mdio_sandbox.o
>  obj-$(CONFIG_FSL_ENETC) += fsl_enetc.o fsl_enetc_mdio.o
>  obj-$(CONFIG_MDIO_MUX_SANDBOX) += mdio_mux_sandbox.o
> +obj-$(CONFIG_MDIO_MUX_I2C_REG) += mdio_mux_i2c_reg.o
> diff --git a/drivers/net/mdio_mux_i2c_reg.c b/drivers/net/mdio_mux_i2c_reg.c
> new file mode 100644
> index 0000000000..eb75c5e032
> --- /dev/null
> +++ b/drivers/net/mdio_mux_i2c_reg.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2019
> + * Alex Marginean, NXP
> + */
> +
> +#include <dm.h>
> +#include <errno.h>
> +#include <miiphy.h>
> +#include <i2c.h>
> +
> +/*
> + * This driver is used for MDIO muxes driven by writing to a register of an I2C
> + * chip.  The board it was developed for uses a mux controlled by on-board FPGA
> + * which in turn is accessed as a chip over I2C.
> + */
> +
> +struct mmux_i2c_reg_priv {
> +       struct udevice *chip;
> +       int reg;
> +       int mask;
> +};
> +
> +static int mmux_i2c_reg_select(struct udevice *mux, int cur, int sel)
> +{
> +       struct mmux_i2c_reg_priv *priv = dev_get_priv(mux);
> +       u8 val, val_old;
> +
> +       /* if last selection didn't change we're good to go */
> +       if (cur == sel)
> +               return 0;
> +
> +       val_old = dm_i2c_reg_read(priv->chip, priv->reg);
> +       val = (val_old & ~priv->mask) | (sel & priv->mask);
> +       debug("%s: chip %s, reg %x, val %x => %x\n", __func__, priv->chip->name,
> +             priv->reg, val_old, val);
> +       dm_i2c_reg_write(priv->chip, priv->reg, val);
> +
> +       return 0;
> +}
> +
> +static const struct mdio_mux_ops mmux_i2c_reg_ops = {
> +       .select = mmux_i2c_reg_select,
> +};
> +
> +static int mmux_i2c_reg_probe(struct udevice *dev)
> +{
> +       struct mmux_i2c_reg_priv *priv = dev_get_priv(dev);
> +       ofnode chip_node, bus_node;
> +       struct udevice *i2c_bus;
> +       u32 reg_mask[2];
> +       u32 chip_addr;
> +       int err;
> +
> +       /* read the register addr/mask pair */
> +       err = dev_read_u32_array(dev, "mux-reg-masks", reg_mask, 2);
> +       if (err) {
> +               debug("%s: error reading mux-reg-masks property\n", __func__);
> +               return err;
> +       }
> +
> +       /* parent should be an I2C chip, grandparent should be an I2C bus */
> +       chip_node = ofnode_get_parent(dev->node);
> +       bus_node = ofnode_get_parent(chip_node);
> +
> +       err = uclass_get_device_by_ofnode(UCLASS_I2C, bus_node, &i2c_bus);
> +       if (err) {
> +               debug("%s: can't find I2C bus for node %s\n", __func__,
> +                     ofnode_get_name(bus_node));
> +               return err;
> +       }
> +
> +       err = ofnode_read_u32(chip_node, "reg", &chip_addr);
> +       if (err) {
> +               debug("%s: can't read chip address in %s\n", __func__,
> +                     ofnode_get_name(chip_node));
> +               return err;
> +       }
> +
> +       err = i2c_get_chip(i2c_bus, (uint)chip_addr, 1, &priv->chip);
> +       if (err) {
> +               debug("%s: can't find i2c chip device for addr %x\n", __func__,
> +                     chip_addr);
> +               return err;
> +       }
> +
> +       priv->reg = (int)reg_mask[0];
> +       priv->mask = (int)reg_mask[1];
> +
> +       debug("%s: chip %s, reg %x, mask %x\n", __func__, priv->chip->name,
> +             priv->reg, priv->mask);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id mmux_i2c_reg_ids[] = {
> +       { .compatible = "mdio-mux-i2c-reg" },

We should use the exact same compatible string as Linux, although
Linux upstream does not have such support for now, but I believe it
should be "mdio-mux-i2creg" for consistency with "mdio-mux-mmioreg".

> +       { }
> +};
> +
> +U_BOOT_DRIVER(mmux_i2c_reg) = {
> +       .name           = "mdio_mux_i2c_reg",
> +       .id             = UCLASS_MDIO_MUX,
> +       .of_match       = mmux_i2c_reg_ids,
> +       .probe          = mmux_i2c_reg_probe,
> +       .ops            = &mmux_i2c_reg_ops,
> +       .priv_auto_alloc_size = sizeof(struct mmux_i2c_reg_priv),
> +};

nits: please replace all mdio_mux_i2c_reg_xxx to mdio_mux_i2creg_xxx

Regards,
Bin

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Alexandru Marginean]

On 7/13/2019 7:02 AM, Bin Meng wrote:
> Hi Alex,
> 
> On Fri, Jul 12, 2019 at 10:21 PM Alex Marginean
> <alexandru.marginean@nxp.com> wrote:
>>
>> This driver is used for MDIO muxes driven over I2C.  This is currently
>> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
>> controlled by an on-board FPGA which in turn is configured through I2C.
>>
>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>> ---
>>
>> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124
>>
>>   drivers/net/Kconfig            |   8 +++
>>   drivers/net/Makefile           |   1 +
>>   drivers/net/mdio_mux_i2c_reg.c | 108 +++++++++++++++++++++++++++++++++
>>   3 files changed, 117 insertions(+)
>>   create mode 100644 drivers/net/mdio_mux_i2c_reg.c
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 4d85fb1716..93535f7acb 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -595,4 +595,12 @@ config FSL_ENETC
>>            This driver supports the NXP ENETC Ethernet controller found on some
>>            of the NXP SoCs.
>>
>> +config MDIO_MUX_I2C_REG
> 
> I don't see current Linux upstream has any I2CREG support. I believe
> you will upstream the Linux support too?

Linux support is somewhat different so we won't have the same driver and
binding there.  The main difference is that Linux has the concept of a
mux controller, independent of the type of bus it muxes.  I didn't add
this to U-Boot, not yet at least.
This specific MDIO mux would be driven in Linux  by a pair of devices:
- a mux controller with bindings defined by reg-mux.txt [1], this one
implements the select function,
- MDIO mux as a client of the controller, defined by
mdio-mux-multiplexer.txt [2], which deals with the MDIO specific side of
things.

In U-Boot I picked up the relevant properties from the two bindings, the
MDIO mux device in U-Boot is supposed to implement select like a generic
Linux MUX, while the uclass code deals with MDIO.
I noticed U-Boot already has an I2C MUX class, along the lines of the
MDIO one, I'm not sure if it's useful enough to introduce a new class of
MUX controllers and then make all other kind of muxes depend on it.  For
now at least the U-Boot mux drivers are simple enough and the extra
class wouldn't help much.

The other difference is that in Linux the mux controller is driven by
the 'MMIO register bitfield-controlled multiplexer driver' [3], which is
not I2C specific.  It is built on top of regmap which I also think is a
bit too much for U-Boot.  For the U-Boot driver I just called I2C API
directly and in that context it made sense to add I2C to the driver
name, but there won't be an equivalent I2C based driver for Linux.

I'd really appreciate some feedback on this.  I think the fairly simple
approach in U-Boot is good enough, but I'm open to suggestions.


[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mux/reg-mux.txt
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio-mux-multiplexer.txt
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mux/mmio.c

> 
> So based on existing Linux kernel MMIOREG support, (see
> mdio-mux-mmioreg.txt in the Linux devicetree bindings direoctory),
> looks we need name this as MDIO_MUX_I2CREG for consistency.
> 
>> +       bool "MDIO MUX accessed as a register over I2C"
>> +       depends on DM_MDIO_MUX && DM_I2C
>> +       help
>> +         This driver is used for MDIO muxes driven by writing to a register of
>> +         an I2C chip.  The board it was developed for uses a mux controlled by
>> +         on-board FPGA which in turn is accessed as a chip over I2C.
>> +
>>   endif # NETDEVICES
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 97119cec7c..9470f02dc6 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -80,3 +80,4 @@ obj-$(CONFIG_HIGMACV300_ETH) += higmacv300.o
>>   obj-$(CONFIG_MDIO_SANDBOX) += mdio_sandbox.o
>>   obj-$(CONFIG_FSL_ENETC) += fsl_enetc.o fsl_enetc_mdio.o
>>   obj-$(CONFIG_MDIO_MUX_SANDBOX) += mdio_mux_sandbox.o
>> +obj-$(CONFIG_MDIO_MUX_I2C_REG) += mdio_mux_i2c_reg.o
>> diff --git a/drivers/net/mdio_mux_i2c_reg.c b/drivers/net/mdio_mux_i2c_reg.c
>> new file mode 100644
>> index 0000000000..eb75c5e032
>> --- /dev/null
>> +++ b/drivers/net/mdio_mux_i2c_reg.c
>> @@ -0,0 +1,108 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2019
>> + * Alex Marginean, NXP
>> + */
>> +
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <miiphy.h>
>> +#include <i2c.h>
>> +
>> +/*
>> + * This driver is used for MDIO muxes driven by writing to a register of an I2C
>> + * chip.  The board it was developed for uses a mux controlled by on-board FPGA
>> + * which in turn is accessed as a chip over I2C.
>> + */
>> +
>> +struct mmux_i2c_reg_priv {
>> +       struct udevice *chip;
>> +       int reg;
>> +       int mask;
>> +};
>> +
>> +static int mmux_i2c_reg_select(struct udevice *mux, int cur, int sel)
>> +{
>> +       struct mmux_i2c_reg_priv *priv = dev_get_priv(mux);
>> +       u8 val, val_old;
>> +
>> +       /* if last selection didn't change we're good to go */
>> +       if (cur == sel)
>> +               return 0;
>> +
>> +       val_old = dm_i2c_reg_read(priv->chip, priv->reg);
>> +       val = (val_old & ~priv->mask) | (sel & priv->mask);
>> +       debug("%s: chip %s, reg %x, val %x => %x\n", __func__, priv->chip->name,
>> +             priv->reg, val_old, val);
>> +       dm_i2c_reg_write(priv->chip, priv->reg, val);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct mdio_mux_ops mmux_i2c_reg_ops = {
>> +       .select = mmux_i2c_reg_select,
>> +};
>> +
>> +static int mmux_i2c_reg_probe(struct udevice *dev)
>> +{
>> +       struct mmux_i2c_reg_priv *priv = dev_get_priv(dev);
>> +       ofnode chip_node, bus_node;
>> +       struct udevice *i2c_bus;
>> +       u32 reg_mask[2];
>> +       u32 chip_addr;
>> +       int err;
>> +
>> +       /* read the register addr/mask pair */
>> +       err = dev_read_u32_array(dev, "mux-reg-masks", reg_mask, 2);
>> +       if (err) {
>> +               debug("%s: error reading mux-reg-masks property\n", __func__);
>> +               return err;
>> +       }
>> +
>> +       /* parent should be an I2C chip, grandparent should be an I2C bus */
>> +       chip_node = ofnode_get_parent(dev->node);
>> +       bus_node = ofnode_get_parent(chip_node);
>> +
>> +       err = uclass_get_device_by_ofnode(UCLASS_I2C, bus_node, &i2c_bus);
>> +       if (err) {
>> +               debug("%s: can't find I2C bus for node %s\n", __func__,
>> +                     ofnode_get_name(bus_node));
>> +               return err;
>> +       }
>> +
>> +       err = ofnode_read_u32(chip_node, "reg", &chip_addr);
>> +       if (err) {
>> +               debug("%s: can't read chip address in %s\n", __func__,
>> +                     ofnode_get_name(chip_node));
>> +               return err;
>> +       }
>> +
>> +       err = i2c_get_chip(i2c_bus, (uint)chip_addr, 1, &priv->chip);
>> +       if (err) {
>> +               debug("%s: can't find i2c chip device for addr %x\n", __func__,
>> +                     chip_addr);
>> +               return err;
>> +       }
>> +
>> +       priv->reg = (int)reg_mask[0];
>> +       priv->mask = (int)reg_mask[1];
>> +
>> +       debug("%s: chip %s, reg %x, mask %x\n", __func__, priv->chip->name,
>> +             priv->reg, priv->mask);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct udevice_id mmux_i2c_reg_ids[] = {
>> +       { .compatible = "mdio-mux-i2c-reg" },
> 
> We should use the exact same compatible string as Linux, although
> Linux upstream does not have such support for now, but I believe it
> should be "mdio-mux-i2creg" for consistency with "mdio-mux-mmioreg".
> 
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(mmux_i2c_reg) = {
>> +       .name           = "mdio_mux_i2c_reg",
>> +       .id             = UCLASS_MDIO_MUX,
>> +       .of_match       = mmux_i2c_reg_ids,
>> +       .probe          = mmux_i2c_reg_probe,
>> +       .ops            = &mmux_i2c_reg_ops,
>> +       .priv_auto_alloc_size = sizeof(struct mmux_i2c_reg_priv),
>> +};
> 
> nits: please replace all mdio_mux_i2c_reg_xxx to mdio_mux_i2creg_xxx

OK, I can do that.

Thank you!
Alex


> 
> Regards,
> Bin
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Joe Hershberger]

On Fri, Jul 12, 2019 at 9:21 AM Alex Marginean
<alexandru.marginean@nxp.com> wrote:
>
> This driver is used for MDIO muxes driven over I2C.  This is currently
> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
> controlled by an on-board FPGA which in turn is configured through I2C.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>
> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Alex Marginean]

Hi Joe,

On 7/15/2019 11:06 PM, Joe Hershberger wrote:
> On Fri, Jul 12, 2019 at 9:21 AM Alex Marginean
> <alexandru.marginean@nxp.com> wrote:
>>
>> This driver is used for MDIO muxes driven over I2C.  This is currently
>> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
>> controlled by an on-board FPGA which in turn is configured through I2C.
>>
>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>> ---
>>
>> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124
> 
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>


Bin suggested I should rename the APIs and config option for
consistency.  I'll send a v2 with that and I'll keep the ACK, I
assume that's OK.

Thanks!
Alex

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Bin Meng]

+Simon,

Hi Alex,

On Mon, Jul 15, 2019 at 7:45 PM Alexandru Marginean
<alexandru.marginean@nxp.com> wrote:
>
> On 7/13/2019 7:02 AM, Bin Meng wrote:
> > Hi Alex,
> >
> > On Fri, Jul 12, 2019 at 10:21 PM Alex Marginean
> > <alexandru.marginean@nxp.com> wrote:
> >>
> >> This driver is used for MDIO muxes driven over I2C.  This is currently
> >> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
> >> controlled by an on-board FPGA which in turn is configured through I2C.
> >>
> >> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> >> ---
> >>
> >> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124
> >>
> >>   drivers/net/Kconfig            |   8 +++
> >>   drivers/net/Makefile           |   1 +
> >>   drivers/net/mdio_mux_i2c_reg.c | 108 +++++++++++++++++++++++++++++++++
> >>   3 files changed, 117 insertions(+)
> >>   create mode 100644 drivers/net/mdio_mux_i2c_reg.c
> >>
> >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> >> index 4d85fb1716..93535f7acb 100644
> >> --- a/drivers/net/Kconfig
> >> +++ b/drivers/net/Kconfig
> >> @@ -595,4 +595,12 @@ config FSL_ENETC
> >>            This driver supports the NXP ENETC Ethernet controller found on some
> >>            of the NXP SoCs.
> >>
> >> +config MDIO_MUX_I2C_REG
> >
> > I don't see current Linux upstream has any I2CREG support. I believe
> > you will upstream the Linux support too?
>
> Linux support is somewhat different so we won't have the same driver and
> binding there.  The main difference is that Linux has the concept of a
> mux controller, independent of the type of bus it muxes.  I didn't add
> this to U-Boot, not yet at least.
> This specific MDIO mux would be driven in Linux  by a pair of devices:
> - a mux controller with bindings defined by reg-mux.txt [1], this one
> implements the select function,
> - MDIO mux as a client of the controller, defined by
> mdio-mux-multiplexer.txt [2], which deals with the MDIO specific side of
> things.
>

Thanks for the detailed explanations! I thought people wanted to have
exact the same DT bindings as Linux so that we can sync-up upstream
kernel DT changes to U-Boot. That's why at first I checked whether
kernel has such bindings in place.

> In U-Boot I picked up the relevant properties from the two bindings, the
> MDIO mux device in U-Boot is supposed to implement select like a generic
> Linux MUX, while the uclass code deals with MDIO.
> I noticed U-Boot already has an I2C MUX class, along the lines of the
> MDIO one, I'm not sure if it's useful enough to introduce a new class of
> MUX controllers and then make all other kind of muxes depend on it.  For
> now at least the U-Boot mux drivers are simple enough and the extra
> class wouldn't help much.
>
> The other difference is that in Linux the mux controller is driven by
> the 'MMIO register bitfield-controlled multiplexer driver' [3], which is
> not I2C specific.  It is built on top of regmap which I also think is a
> bit too much for U-Boot.  For the U-Boot driver I just called I2C API
> directly and in that context it made sense to add I2C to the driver
> name, but there won't be an equivalent I2C based driver for Linux.
>

For your current implementation I believe that's OK, at least it is
verified on LS1028. But there must be a reason that Linux kernel chose
a DT that is different. I suspect the usage of kernel DT makes us stay
future-proof.

> I'd really appreciate some feedback on this.  I think the fairly simple
> approach in U-Boot is good enough, but I'm open to suggestions.

I am open on this topic too. Agreed that for U-Boot we always want a
simple implementation for device drivers. I am adding Simon for his
opinion on this topic as well.

>
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mux/reg-mux.txt
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio-mux-multiplexer.txt
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mux/mmio.c
>

Regards,
Bin

[U-Boot] [PATCH 1/2] drivers: net: driver for MDIO muxes controlled over I2C

[Alex Marginean]

Hi Bin, Simon,

On 7/16/2019 11:21 AM, Bin Meng wrote:
> +Simon,
> 
> Hi Alex,
> 
> On Mon, Jul 15, 2019 at 7:45 PM Alexandru Marginean
> <alexandru.marginean@nxp.com> wrote:
>>
>> On 7/13/2019 7:02 AM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Fri, Jul 12, 2019 at 10:21 PM Alex Marginean
>>> <alexandru.marginean@nxp.com> wrote:
>>>>
>>>> This driver is used for MDIO muxes driven over I2C.  This is currently
>>>> used on Freescale LS1028A QDS board, on which the physical MDIO MUX is
>>>> controlled by an on-board FPGA which in turn is configured through I2C.
>>>>
>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>>>> ---
>>>>
>>>> Depends on https://patchwork.ozlabs.org/project/uboot/list/?series=119124
>>>>
>>>>    drivers/net/Kconfig            |   8 +++
>>>>    drivers/net/Makefile           |   1 +
>>>>    drivers/net/mdio_mux_i2c_reg.c | 108 +++++++++++++++++++++++++++++++++
>>>>    3 files changed, 117 insertions(+)
>>>>    create mode 100644 drivers/net/mdio_mux_i2c_reg.c
>>>>
>>>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>>> index 4d85fb1716..93535f7acb 100644
>>>> --- a/drivers/net/Kconfig
>>>> +++ b/drivers/net/Kconfig
>>>> @@ -595,4 +595,12 @@ config FSL_ENETC
>>>>             This driver supports the NXP ENETC Ethernet controller found on some
>>>>             of the NXP SoCs.
>>>>
>>>> +config MDIO_MUX_I2C_REG
>>>
>>> I don't see current Linux upstream has any I2CREG support. I believe
>>> you will upstream the Linux support too?
>>
>> Linux support is somewhat different so we won't have the same driver and
>> binding there.  The main difference is that Linux has the concept of a
>> mux controller, independent of the type of bus it muxes.  I didn't add
>> this to U-Boot, not yet at least.
>> This specific MDIO mux would be driven in Linux  by a pair of devices:
>> - a mux controller with bindings defined by reg-mux.txt [1], this one
>> implements the select function,
>> - MDIO mux as a client of the controller, defined by
>> mdio-mux-multiplexer.txt [2], which deals with the MDIO specific side of
>> things.
>>
> 
> Thanks for the detailed explanations! I thought people wanted to have
> exact the same DT bindings as Linux so that we can sync-up upstream
> kernel DT changes to U-Boot. That's why at first I checked whether
> kernel has such bindings in place.
> 
>> In U-Boot I picked up the relevant properties from the two bindings, the
>> MDIO mux device in U-Boot is supposed to implement select like a generic
>> Linux MUX, while the uclass code deals with MDIO.
>> I noticed U-Boot already has an I2C MUX class, along the lines of the
>> MDIO one, I'm not sure if it's useful enough to introduce a new class of
>> MUX controllers and then make all other kind of muxes depend on it.  For
>> now at least the U-Boot mux drivers are simple enough and the extra
>> class wouldn't help much.
>>
>> The other difference is that in Linux the mux controller is driven by
>> the 'MMIO register bitfield-controlled multiplexer driver' [3], which is
>> not I2C specific.  It is built on top of regmap which I also think is a
>> bit too much for U-Boot.  For the U-Boot driver I just called I2C API
>> directly and in that context it made sense to add I2C to the driver
>> name, but there won't be an equivalent I2C based driver for Linux.
>>
> 
> For your current implementation I believe that's OK, at least it is
> verified on LS1028. But there must be a reason that Linux kernel chose
> a DT that is different. I suspect the usage of kernel DT makes us stay
> future-proof.

Having a generic mux controller that's independent of the muxed bus has
its merit, of course.  Right now the API that a MDIO MUX must implement
in U-Boot has just a _select function, which, by itself, has not direct
relation to MDIO and could be used for other buses.  So mux controllers
could work in U-Boot, at least to keep U-Boot in sync with Linux.

I'm not sure about reg/mmio-mux.  Those work in Linux because of regmap
and layers that abstract the underlying bus which I think are a bit too
much for U-Boot.  Of course without that abstraction we can't use
"reg-mux" or "mdio-mux-multiplexer" compatibility strings for the more
specific devices and drivers in U-Boot.

> 
>> I'd really appreciate some feedback on this.  I think the fairly simple
>> approach in U-Boot is good enough, but I'm open to suggestions.
> 
> I am open on this topic too. Agreed that for U-Boot we always want a
> simple implementation for device drivers. I am adding Simon for his
> opinion on this topic as well.

Thank you, let's see what Simon thinks about this.

Alex

> 
>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mux/reg-mux.txt
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio-mux-multiplexer.txt
>> [3]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mux/mmio.c
>>
> 
> Regards,
> Bin
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>