[PATCH v3 0/9] net: mvmdio: add xMDIO xSMI support

[PATCH v3 0/9] net: mvmdio: add xMDIO xSMI support

[Antoine Tenart]

Hello,

This series aims to add the xSMI support on the xMDIO bus to the
mvmdio driver. The xSMI interface complies with the IEEE 802.3 clause 45
and is used by 10GbE devices. On 7k and 8k (as of now), such an
interface is found and is used by Ethernet controllers.

Patches 1-3 are cosmetic cleanups.

Patches 4-6 are prerequisites to the xSMI support.

Patches 7-9 add the xSMI support to the mvmdio driver, and a node is
added both in the cp110 slave and master device trees.

This was tested on an Armada 8040 mcbin, as well as on both the
Armada 7040 DB and the Armada 8040 DB to ensure the SMI interface
was still working.

@Dave: patch 9 should go through the mvebu tree as asked by Gregory,
thanks!

Thanks,
Antoine

Since v2:
  - Brought back the marvell,xmdio compatible and updated the driver
    accordingly. The ops (smi, xsmi) are chosen based on the compatible.
  - Now return -EOPNOTSUPP when the MII_ADDR_C45 bit is wrongly set.
  - Mask dev_addr with GENMASK(4, 0).
  - Moved bit definitions under their register definition.
  - Fixed the write operation shift.
  - Added one space before the second parameter of GENMASK.

Since v1:
  - Instead of using the smi/xsmi helpers based on the compatible, now
    check if the MII_ADDR_C45 bit is set.
  - Removed the marvell,xmdio compatible addition.
  - Fixed the is_read_valid logic.
  - Updated to use static const variables for ops.
  - Added 3 Reviewed-by tags from Florian (I dropped another one as the
    patch changed in v2).

Antoine Tenart (9):
  net: mvmdio: reorder headers alphabetically
  net: mvmdio: use tabs for defines
  net: mvmdio: use GENMASK for masks
  net: mvmdio: move the read valid check into its own function
  net: mvmdio: introduce an ops structure
  net: mvmdio: put the poll intervals in the ops structure
  net: mvmdio: add xmdio xsmi support
  dt-bindings: orion-mdio: document the new xmdio compatible
  arm64: marvell: dts: add xmdio nodes for 7k/8k

 .../devicetree/bindings/net/marvell-orion-mdio.txt |   8 +-
 .../boot/dts/marvell/armada-cp110-master.dtsi      |   8 +
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |   8 +
 drivers/net/ethernet/marvell/Kconfig               |   6 +-
 drivers/net/ethernet/marvell/mvmdio.c              | 221 +++++++++++++++++----
 5 files changed, 203 insertions(+), 48 deletions(-)

-- 
2.9.4

[PATCH v3 0/9] net: mvmdio: add xMDIO xSMI support

[Antoine Tenart]

Hello,

This series aims to add the xSMI support on the xMDIO bus to the
mvmdio driver. The xSMI interface complies with the IEEE 802.3 clause 45
and is used by 10GbE devices. On 7k and 8k (as of now), such an
interface is found and is used by Ethernet controllers.

Patches 1-3 are cosmetic cleanups.

Patches 4-6 are prerequisites to the xSMI support.

Patches 7-9 add the xSMI support to the mvmdio driver, and a node is
added both in the cp110 slave and master device trees.

This was tested on an Armada 8040 mcbin, as well as on both the
Armada 7040 DB and the Armada 8040 DB to ensure the SMI interface
was still working.

@Dave: patch 9 should go through the mvebu tree as asked by Gregory,
thanks!

Thanks,
Antoine

Since v2:
  - Brought back the marvell,xmdio compatible and updated the driver
    accordingly. The ops (smi, xsmi) are chosen based on the compatible.
  - Now return -EOPNOTSUPP when the MII_ADDR_C45 bit is wrongly set.
  - Mask dev_addr with GENMASK(4, 0).
  - Moved bit definitions under their register definition.
  - Fixed the write operation shift.
  - Added one space before the second parameter of GENMASK.

Since v1:
  - Instead of using the smi/xsmi helpers based on the compatible, now
    check if the MII_ADDR_C45 bit is set.
  - Removed the marvell,xmdio compatible addition.
  - Fixed the is_read_valid logic.
  - Updated to use static const variables for ops.
  - Added 3 Reviewed-by tags from Florian (I dropped another one as the
    patch changed in v2).

Antoine Tenart (9):
  net: mvmdio: reorder headers alphabetically
  net: mvmdio: use tabs for defines
  net: mvmdio: use GENMASK for masks
  net: mvmdio: move the read valid check into its own function
  net: mvmdio: introduce an ops structure
  net: mvmdio: put the poll intervals in the ops structure
  net: mvmdio: add xmdio xsmi support
  dt-bindings: orion-mdio: document the new xmdio compatible
  arm64: marvell: dts: add xmdio nodes for 7k/8k

 .../devicetree/bindings/net/marvell-orion-mdio.txt |   8 +-
 .../boot/dts/marvell/armada-cp110-master.dtsi      |   8 +
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |   8 +
 drivers/net/ethernet/marvell/Kconfig               |   6 +-
 drivers/net/ethernet/marvell/mvmdio.c              | 221 +++++++++++++++++----
 5 files changed, 203 insertions(+), 48 deletions(-)

-- 
2.9.4

[PATCH v3 1/9] net: mvmdio: reorder headers alphabetically

[Antoine Tenart]

Cosmetic fix reordering headers alphabetically.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 90a60b98c28e..109a2bff334d 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -17,16 +17,16 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_mdio.h>
 #include <linux/phy.h>
-#include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/of_mdio.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-- 
2.9.4

[PATCH v3 1/9] net: mvmdio: reorder headers alphabetically

[Antoine Tenart]

Cosmetic fix reordering headers alphabetically.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 90a60b98c28e..109a2bff334d 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -17,16 +17,16 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_mdio.h>
 #include <linux/phy.h>
-#include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/of_mdio.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-- 
2.9.4

[PATCH v3 2/9] net: mvmdio: use tabs for defines

[Antoine Tenart]

Cosmetic patch replacing spaces by tabs for defined values.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 109a2bff334d..17b518b13ae3 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -30,25 +30,25 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-#define MVMDIO_SMI_DATA_SHIFT              0
-#define MVMDIO_SMI_PHY_ADDR_SHIFT          16
-#define MVMDIO_SMI_PHY_REG_SHIFT           21
-#define MVMDIO_SMI_READ_OPERATION          BIT(26)
-#define MVMDIO_SMI_WRITE_OPERATION         0
-#define MVMDIO_SMI_READ_VALID              BIT(27)
-#define MVMDIO_SMI_BUSY                    BIT(28)
-#define MVMDIO_ERR_INT_CAUSE		   0x007C
-#define  MVMDIO_ERR_INT_SMI_DONE	   0x00000010
-#define MVMDIO_ERR_INT_MASK		   0x0080
+#define MVMDIO_SMI_DATA_SHIFT		0
+#define MVMDIO_SMI_PHY_ADDR_SHIFT	16
+#define MVMDIO_SMI_PHY_REG_SHIFT	21
+#define MVMDIO_SMI_READ_OPERATION	BIT(26)
+#define MVMDIO_SMI_WRITE_OPERATION	0
+#define MVMDIO_SMI_READ_VALID		BIT(27)
+#define MVMDIO_SMI_BUSY			BIT(28)
+#define MVMDIO_ERR_INT_CAUSE		0x007C
+#define  MVMDIO_ERR_INT_SMI_DONE	0x00000010
+#define MVMDIO_ERR_INT_MASK		0x0080
 
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
  * - Armada 370       (Globalscale Mirabox):   41us to 43us (Polled)
  */
-#define MVMDIO_SMI_TIMEOUT		   1000 /* 1000us = 1ms */
-#define MVMDIO_SMI_POLL_INTERVAL_MIN	   45
-#define MVMDIO_SMI_POLL_INTERVAL_MAX	   55
+#define MVMDIO_SMI_TIMEOUT		1000 /* 1000us = 1ms */
+#define MVMDIO_SMI_POLL_INTERVAL_MIN	45
+#define MVMDIO_SMI_POLL_INTERVAL_MAX	55
 
 struct orion_mdio_dev {
 	struct mutex lock;
-- 
2.9.4

[PATCH v3 2/9] net: mvmdio: use tabs for defines

[Antoine Tenart]

Cosmetic patch replacing spaces by tabs for defined values.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 109a2bff334d..17b518b13ae3 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -30,25 +30,25 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-#define MVMDIO_SMI_DATA_SHIFT              0
-#define MVMDIO_SMI_PHY_ADDR_SHIFT          16
-#define MVMDIO_SMI_PHY_REG_SHIFT           21
-#define MVMDIO_SMI_READ_OPERATION          BIT(26)
-#define MVMDIO_SMI_WRITE_OPERATION         0
-#define MVMDIO_SMI_READ_VALID              BIT(27)
-#define MVMDIO_SMI_BUSY                    BIT(28)
-#define MVMDIO_ERR_INT_CAUSE		   0x007C
-#define  MVMDIO_ERR_INT_SMI_DONE	   0x00000010
-#define MVMDIO_ERR_INT_MASK		   0x0080
+#define MVMDIO_SMI_DATA_SHIFT		0
+#define MVMDIO_SMI_PHY_ADDR_SHIFT	16
+#define MVMDIO_SMI_PHY_REG_SHIFT	21
+#define MVMDIO_SMI_READ_OPERATION	BIT(26)
+#define MVMDIO_SMI_WRITE_OPERATION	0
+#define MVMDIO_SMI_READ_VALID		BIT(27)
+#define MVMDIO_SMI_BUSY			BIT(28)
+#define MVMDIO_ERR_INT_CAUSE		0x007C
+#define  MVMDIO_ERR_INT_SMI_DONE	0x00000010
+#define MVMDIO_ERR_INT_MASK		0x0080
 
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
  * - Armada 370       (Globalscale Mirabox):   41us to 43us (Polled)
  */
-#define MVMDIO_SMI_TIMEOUT		   1000 /* 1000us = 1ms */
-#define MVMDIO_SMI_POLL_INTERVAL_MIN	   45
-#define MVMDIO_SMI_POLL_INTERVAL_MAX	   55
+#define MVMDIO_SMI_TIMEOUT		1000 /* 1000us = 1ms */
+#define MVMDIO_SMI_POLL_INTERVAL_MIN	45
+#define MVMDIO_SMI_POLL_INTERVAL_MAX	55
 
 struct orion_mdio_dev {
 	struct mutex lock;
-- 
2.9.4

[PATCH v3 3/9] net: mvmdio: use GENMASK for masks

[Antoine Tenart]

Cosmetic patch to use the GENMASK helper for masks.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 17b518b13ae3..583f1c5753c2 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -138,7 +138,7 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 		goto out;
 	}
 
-	ret = val & 0xFFFF;
+	ret = val & GENMASK(15, 0);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
-- 
2.9.4

[PATCH v3 3/9] net: mvmdio: use GENMASK for masks

[Antoine Tenart]

Cosmetic patch to use the GENMASK helper for masks.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 17b518b13ae3..583f1c5753c2 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -138,7 +138,7 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 		goto out;
 	}
 
-	ret = val & 0xFFFF;
+	ret = val & GENMASK(15, 0);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
-- 
2.9.4

[PATCH v3 4/9] net: mvmdio: move the read valid check into its own function

[Antoine Tenart]

Move the read valid check in its own function. This is needed as a
requirement to factorize the driver to add the xMDIO support in the
future.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 583f1c5753c2..2ed819d25305 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -69,6 +69,11 @@ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
 }
 
+static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(struct mii_bus *bus)
@@ -113,7 +118,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	u32 val;
 	int ret;
 
 	mutex_lock(&dev->lock);
@@ -131,14 +135,13 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	if (ret < 0)
 		goto out;
 
-	val = readl(dev->regs);
-	if (!(val & MVMDIO_SMI_READ_VALID)) {
+	if (!orion_mdio_smi_is_read_valid(dev)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
 		ret = -ENODEV;
 		goto out;
 	}
 
-	ret = val & GENMASK(15, 0);
+	ret = readl(dev->regs) & GENMASK(15, 0);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
-- 
2.9.4

[PATCH v3 4/9] net: mvmdio: move the read valid check into its own function

[Antoine Tenart]

Move the read valid check in its own function. This is needed as a
requirement to factorize the driver to add the xMDIO support in the
future.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 583f1c5753c2..2ed819d25305 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -69,6 +69,11 @@ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
 }
 
+static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(struct mii_bus *bus)
@@ -113,7 +118,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	u32 val;
 	int ret;
 
 	mutex_lock(&dev->lock);
@@ -131,14 +135,13 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	if (ret < 0)
 		goto out;
 
-	val = readl(dev->regs);
-	if (!(val & MVMDIO_SMI_READ_VALID)) {
+	if (!orion_mdio_smi_is_read_valid(dev)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
 		ret = -ENODEV;
 		goto out;
 	}
 
-	ret = val & GENMASK(15, 0);
+	ret = readl(dev->regs) & GENMASK(15, 0);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
-- 
2.9.4

[PATCH v3 5/9] net: mvmdio: introduce an ops structure

[Antoine Tenart]

Introduce an ops structure to add an indirection on functions accessing
the registers. This is needed to add the xMDIO support later.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 71 ++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 2ed819d25305..7d2f1b2e1fe1 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -64,6 +64,14 @@ struct orion_mdio_dev {
 	wait_queue_head_t smi_busy_wait;
 };
 
+struct orion_mdio_ops {
+	int (*is_done)(struct orion_mdio_dev *);
+	int (*is_read_valid)(struct orion_mdio_dev *);
+	void (*start_read)(struct orion_mdio_dev *, int, int);
+	u16 (*read)(struct orion_mdio_dev *);
+	void (*write)(struct orion_mdio_dev *, int, int, u16);
+};
+
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 {
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
@@ -74,9 +82,42 @@ static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
 	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
 }
 
+static void orion_mdio_smi_start_read_op(struct orion_mdio_dev *dev, int mii_id,
+					 int regnum)
+{
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_READ_OPERATION),
+	       dev->regs);
+}
+
+static u16 orion_mdio_smi_read_op(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs) & GENMASK(15, 0);
+}
+
+static void orion_mdio_smi_write_op(struct orion_mdio_dev *dev, int mii_id,
+				    int regnum, u16 value)
+{
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_WRITE_OPERATION            |
+		(value << MVMDIO_SMI_DATA_SHIFT)),
+	       dev->regs);
+}
+
+static const struct orion_mdio_ops orion_mdio_smi_ops = {
+	.is_done = orion_mdio_smi_is_done,
+	.is_read_valid = orion_mdio_smi_is_read_valid,
+	.start_read = orion_mdio_smi_start_read_op,
+	.read = orion_mdio_smi_read_op,
+	.write = orion_mdio_smi_write_op,
+};
+
 /* Wait for the SMI unit to be ready for another operation
  */
-static int orion_mdio_wait_ready(struct mii_bus *bus)
+static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
+				 struct mii_bus *bus)
 {
 	struct orion_mdio_dev *dev = bus->priv;
 	unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
@@ -84,7 +125,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 	int timedout = 0;
 
 	while (1) {
-	        if (orion_mdio_smi_is_done(dev))
+	        if (ops->is_done(dev))
 			return 0;
 	        else if (timedout)
 			break;
@@ -103,8 +144,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 			if (timeout < 2)
 				timeout = 2;
 			wait_event_timeout(dev->smi_busy_wait,
-				           orion_mdio_smi_is_done(dev),
-				           timeout);
+				           ops->is_done(dev), timeout);
 
 			++timedout;
 	        }
@@ -118,30 +158,28 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
+	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
 	int ret;
 
 	mutex_lock(&dev->lock);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_READ_OPERATION),
-	       dev->regs);
+	ops->start_read(dev, mii_id, regnum);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	if (!orion_mdio_smi_is_read_valid(dev)) {
+	if (!ops->is_read_valid(dev)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
 		ret = -ENODEV;
 		goto out;
 	}
 
-	ret = readl(dev->regs) & GENMASK(15, 0);
+	ret = ops->read(dev);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
@@ -151,19 +189,16 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 			    int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
+	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
 	int ret;
 
 	mutex_lock(&dev->lock);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_WRITE_OPERATION            |
-		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->regs);
+	ops->write(dev, mii_id, regnum, value);
 
 out:
 	mutex_unlock(&dev->lock);
-- 
2.9.4

[PATCH v3 5/9] net: mvmdio: introduce an ops structure

[Antoine Tenart]

Introduce an ops structure to add an indirection on functions accessing
the registers. This is needed to add the xMDIO support later.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 71 ++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 2ed819d25305..7d2f1b2e1fe1 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -64,6 +64,14 @@ struct orion_mdio_dev {
 	wait_queue_head_t smi_busy_wait;
 };
 
+struct orion_mdio_ops {
+	int (*is_done)(struct orion_mdio_dev *);
+	int (*is_read_valid)(struct orion_mdio_dev *);
+	void (*start_read)(struct orion_mdio_dev *, int, int);
+	u16 (*read)(struct orion_mdio_dev *);
+	void (*write)(struct orion_mdio_dev *, int, int, u16);
+};
+
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 {
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
@@ -74,9 +82,42 @@ static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
 	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
 }
 
+static void orion_mdio_smi_start_read_op(struct orion_mdio_dev *dev, int mii_id,
+					 int regnum)
+{
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_READ_OPERATION),
+	       dev->regs);
+}
+
+static u16 orion_mdio_smi_read_op(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs) & GENMASK(15, 0);
+}
+
+static void orion_mdio_smi_write_op(struct orion_mdio_dev *dev, int mii_id,
+				    int regnum, u16 value)
+{
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_WRITE_OPERATION            |
+		(value << MVMDIO_SMI_DATA_SHIFT)),
+	       dev->regs);
+}
+
+static const struct orion_mdio_ops orion_mdio_smi_ops = {
+	.is_done = orion_mdio_smi_is_done,
+	.is_read_valid = orion_mdio_smi_is_read_valid,
+	.start_read = orion_mdio_smi_start_read_op,
+	.read = orion_mdio_smi_read_op,
+	.write = orion_mdio_smi_write_op,
+};
+
 /* Wait for the SMI unit to be ready for another operation
  */
-static int orion_mdio_wait_ready(struct mii_bus *bus)
+static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
+				 struct mii_bus *bus)
 {
 	struct orion_mdio_dev *dev = bus->priv;
 	unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
@@ -84,7 +125,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 	int timedout = 0;
 
 	while (1) {
-	        if (orion_mdio_smi_is_done(dev))
+	        if (ops->is_done(dev))
 			return 0;
 	        else if (timedout)
 			break;
@@ -103,8 +144,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 			if (timeout < 2)
 				timeout = 2;
 			wait_event_timeout(dev->smi_busy_wait,
-				           orion_mdio_smi_is_done(dev),
-				           timeout);
+				           ops->is_done(dev), timeout);
 
 			++timedout;
 	        }
@@ -118,30 +158,28 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
+	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
 	int ret;
 
 	mutex_lock(&dev->lock);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_READ_OPERATION),
-	       dev->regs);
+	ops->start_read(dev, mii_id, regnum);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	if (!orion_mdio_smi_is_read_valid(dev)) {
+	if (!ops->is_read_valid(dev)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
 		ret = -ENODEV;
 		goto out;
 	}
 
-	ret = readl(dev->regs) & GENMASK(15, 0);
+	ret = ops->read(dev);
 out:
 	mutex_unlock(&dev->lock);
 	return ret;
@@ -151,19 +189,16 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 			    int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
+	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
 	int ret;
 
 	mutex_lock(&dev->lock);
 
-	ret = orion_mdio_wait_ready(bus);
+	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
 
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_WRITE_OPERATION            |
-		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->regs);
+	ops->write(dev, mii_id, regnum, value);
 
 out:
 	mutex_unlock(&dev->lock);
-- 
2.9.4

[PATCH v3 6/9] net: mvmdio: put the poll intervals in the ops structure

[Antoine Tenart]

Put the two poll intervals (min and max) in the driver's ops
structure. This is needed to add the xmdio support later.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 7d2f1b2e1fe1..41c75d7b84fb 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -70,6 +70,9 @@ struct orion_mdio_ops {
 	void (*start_read)(struct orion_mdio_dev *, int, int);
 	u16 (*read)(struct orion_mdio_dev *);
 	void (*write)(struct orion_mdio_dev *, int, int, u16);
+
+	unsigned int poll_interval_min;
+	unsigned int poll_interval_max;
 };
 
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
@@ -112,6 +115,9 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.start_read = orion_mdio_smi_start_read_op,
 	.read = orion_mdio_smi_read_op,
 	.write = orion_mdio_smi_write_op,
+
+	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
 /* Wait for the SMI unit to be ready for another operation
@@ -131,8 +137,8 @@ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
 			break;
 
 	        if (dev->err_interrupt <= 0) {
-			usleep_range(MVMDIO_SMI_POLL_INTERVAL_MIN,
-				     MVMDIO_SMI_POLL_INTERVAL_MAX);
+			usleep_range(ops->poll_interval_min,
+				     ops->poll_interval_max);
 
 			if (time_is_before_jiffies(end))
 				++timedout;
-- 
2.9.4

[PATCH v3 6/9] net: mvmdio: put the poll intervals in the ops structure

[Antoine Tenart]

Put the two poll intervals (min and max) in the driver's ops
structure. This is needed to add the xmdio support later.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 7d2f1b2e1fe1..41c75d7b84fb 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -70,6 +70,9 @@ struct orion_mdio_ops {
 	void (*start_read)(struct orion_mdio_dev *, int, int);
 	u16 (*read)(struct orion_mdio_dev *);
 	void (*write)(struct orion_mdio_dev *, int, int, u16);
+
+	unsigned int poll_interval_min;
+	unsigned int poll_interval_max;
 };
 
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
@@ -112,6 +115,9 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.start_read = orion_mdio_smi_start_read_op,
 	.read = orion_mdio_smi_read_op,
 	.write = orion_mdio_smi_write_op,
+
+	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
 /* Wait for the SMI unit to be ready for another operation
@@ -131,8 +137,8 @@ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
 			break;
 
 	        if (dev->err_interrupt <= 0) {
-			usleep_range(MVMDIO_SMI_POLL_INTERVAL_MIN,
-				     MVMDIO_SMI_POLL_INTERVAL_MAX);
+			usleep_range(ops->poll_interval_min,
+				     ops->poll_interval_max);
 
 			if (time_is_before_jiffies(end))
 				++timedout;
-- 
2.9.4

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Antoine Tenart]

This patch adds the xmdio xsmi interface support in the mvmdio driver.
This interface is used in Ethernet controllers on Marvell 370, 7k and 8k
(as of now). The xsmi interface supported by this driver complies with
the IEEE 802.3 clause 45. The xSMI interface is used by 10GbE devices.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/Kconfig  |   6 +-
 drivers/net/ethernet/marvell/mvmdio.c | 101 +++++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index da6fb825afea..205bb7e683b7 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -35,9 +35,9 @@ config MVMDIO
 	depends on HAS_IOMEM
 	select PHYLIB
 	---help---
-	  This driver supports the MDIO interface found in the network
-	  interface units of the Marvell EBU SoCs (Kirkwood, Orion5x,
-	  Dove, Armada 370 and Armada XP).
+	  This driver supports the MDIO and xMDIO interfaces found in
+	  the network interface units of the Marvell EBU SoCs (Kirkwood,
+	  Orion5x, Dove, Armada 370, Armada XP, Armada 7k and Armada 8k).
 
 	  This driver is used by the MV643XX_ETH and MVNETA drivers.
 
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 41c75d7b84fb..c43747f0be0b 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
@@ -41,6 +42,15 @@
 #define  MVMDIO_ERR_INT_SMI_DONE	0x00000010
 #define MVMDIO_ERR_INT_MASK		0x0080
 
+#define MVMDIO_XSMI_MGNT_REG		0x0
+#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
+#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
+#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 26)
+#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
+#define  MVMDIO_XSMI_READ_VALID		BIT(29)
+#define  MVMDIO_XSMI_BUSY		BIT(30)
+#define MVMDIO_XSMI_ADDR_REG		0x8
+
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
@@ -50,6 +60,14 @@
 #define MVMDIO_SMI_POLL_INTERVAL_MIN	45
 #define MVMDIO_SMI_POLL_INTERVAL_MAX	55
 
+#define MVMDIO_XSMI_POLL_INTERVAL_MIN	150
+#define MVMDIO_XSMI_POLL_INTERVAL_MAX	160
+
+enum orion_mdio_bus_type {
+	BUS_TYPE_SMI,
+	BUS_TYPE_XSMI
+};
+
 struct orion_mdio_dev {
 	struct mutex lock;
 	void __iomem *regs;
@@ -62,6 +80,8 @@ struct orion_mdio_dev {
 	 */
 	int err_interrupt;
 	wait_queue_head_t smi_busy_wait;
+
+	enum orion_mdio_bus_type bus_type;
 };
 
 struct orion_mdio_ops {
@@ -75,6 +95,7 @@ struct orion_mdio_ops {
 	unsigned int poll_interval_max;
 };
 
+/* mdio smi */
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 {
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
@@ -120,6 +141,68 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
+/* xmdio xsmi */
+static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
+}
+
+static int orion_mdio_xsmi_is_read_valid(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_READ_VALID;
+}
+
+static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
+					  int mii_id, int regnum)
+{
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_READ_OPERATION,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+}
+
+static u16 orion_mdio_xsmi_read_op(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
+}
+
+static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
+				     int regnum, u16 value)
+{
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_WRITE_OPERATION | value,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+}
+
+static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
+	.is_done = orion_mdio_xsmi_is_done,
+	.is_read_valid = orion_mdio_xsmi_is_read_valid,
+	.start_read = orion_mdio_xsmi_start_read_op,
+	.read = orion_mdio_xsmi_read_op,
+	.write = orion_mdio_xsmi_write_op,
+
+	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
+};
+
+static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
+						       int regnum)
+{
+	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
+		return &orion_mdio_xsmi_ops;
+	else if (dev->bus_type == BUS_TYPE_SMI)
+		return &orion_mdio_smi_ops;
+
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
@@ -164,9 +247,13 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
+	const struct orion_mdio_ops *ops;
 	int ret;
 
+	ops = orion_mdio_get_ops(dev, regnum);
+	if (IS_ERR(ops))
+		return PTR_ERR(ops);
+
 	mutex_lock(&dev->lock);
 
 	ret = orion_mdio_wait_ready(ops, bus);
@@ -195,9 +282,13 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 			    int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
+	const struct orion_mdio_ops *ops;
 	int ret;
 
+	ops = orion_mdio_get_ops(dev, regnum);
+	if (IS_ERR(ops))
+		return PTR_ERR(ops);
+
 	mutex_lock(&dev->lock);
 
 	ret = orion_mdio_wait_ready(ops, bus);
@@ -288,6 +379,9 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
+	dev->bus_type =
+		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+
 	mutex_init(&dev->lock);
 
 	if (pdev->dev.of_node)
@@ -338,7 +432,8 @@ static int orion_mdio_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id orion_mdio_match[] = {
-	{ .compatible = "marvell,orion-mdio" },
+	{ .compatible = "marvell,orion-mdio", .data = (void *)BUS_TYPE_SMI },
+	{ .compatible = "marvell,xmdio", .data = (void *)BUS_TYPE_XSMI },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, orion_mdio_match);
-- 
2.9.4

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Antoine Tenart]

This patch adds the xmdio xsmi interface support in the mvmdio driver.
This interface is used in Ethernet controllers on Marvell 370, 7k and 8k
(as of now). The xsmi interface supported by this driver complies with
the IEEE 802.3 clause 45. The xSMI interface is used by 10GbE devices.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/marvell/Kconfig  |   6 +-
 drivers/net/ethernet/marvell/mvmdio.c | 101 +++++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index da6fb825afea..205bb7e683b7 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -35,9 +35,9 @@ config MVMDIO
 	depends on HAS_IOMEM
 	select PHYLIB
 	---help---
-	  This driver supports the MDIO interface found in the network
-	  interface units of the Marvell EBU SoCs (Kirkwood, Orion5x,
-	  Dove, Armada 370 and Armada XP).
+	  This driver supports the MDIO and xMDIO interfaces found in
+	  the network interface units of the Marvell EBU SoCs (Kirkwood,
+	  Orion5x, Dove, Armada 370, Armada XP, Armada 7k and Armada 8k).
 
 	  This driver is used by the MV643XX_ETH and MVNETA drivers.
 
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 41c75d7b84fb..c43747f0be0b 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
@@ -41,6 +42,15 @@
 #define  MVMDIO_ERR_INT_SMI_DONE	0x00000010
 #define MVMDIO_ERR_INT_MASK		0x0080
 
+#define MVMDIO_XSMI_MGNT_REG		0x0
+#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
+#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
+#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 26)
+#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
+#define  MVMDIO_XSMI_READ_VALID		BIT(29)
+#define  MVMDIO_XSMI_BUSY		BIT(30)
+#define MVMDIO_XSMI_ADDR_REG		0x8
+
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
@@ -50,6 +60,14 @@
 #define MVMDIO_SMI_POLL_INTERVAL_MIN	45
 #define MVMDIO_SMI_POLL_INTERVAL_MAX	55
 
+#define MVMDIO_XSMI_POLL_INTERVAL_MIN	150
+#define MVMDIO_XSMI_POLL_INTERVAL_MAX	160
+
+enum orion_mdio_bus_type {
+	BUS_TYPE_SMI,
+	BUS_TYPE_XSMI
+};
+
 struct orion_mdio_dev {
 	struct mutex lock;
 	void __iomem *regs;
@@ -62,6 +80,8 @@ struct orion_mdio_dev {
 	 */
 	int err_interrupt;
 	wait_queue_head_t smi_busy_wait;
+
+	enum orion_mdio_bus_type bus_type;
 };
 
 struct orion_mdio_ops {
@@ -75,6 +95,7 @@ struct orion_mdio_ops {
 	unsigned int poll_interval_max;
 };
 
+/* mdio smi */
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 {
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
@@ -120,6 +141,68 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
+/* xmdio xsmi */
+static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
+}
+
+static int orion_mdio_xsmi_is_read_valid(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_READ_VALID;
+}
+
+static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
+					  int mii_id, int regnum)
+{
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_READ_OPERATION,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+}
+
+static u16 orion_mdio_xsmi_read_op(struct orion_mdio_dev *dev)
+{
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
+}
+
+static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
+				     int regnum, u16 value)
+{
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_WRITE_OPERATION | value,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+}
+
+static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
+	.is_done = orion_mdio_xsmi_is_done,
+	.is_read_valid = orion_mdio_xsmi_is_read_valid,
+	.start_read = orion_mdio_xsmi_start_read_op,
+	.read = orion_mdio_xsmi_read_op,
+	.write = orion_mdio_xsmi_write_op,
+
+	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
+};
+
+static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
+						       int regnum)
+{
+	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
+		return &orion_mdio_xsmi_ops;
+	else if (dev->bus_type == BUS_TYPE_SMI)
+		return &orion_mdio_smi_ops;
+
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
@@ -164,9 +247,13 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 			   int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
+	const struct orion_mdio_ops *ops;
 	int ret;
 
+	ops = orion_mdio_get_ops(dev, regnum);
+	if (IS_ERR(ops))
+		return PTR_ERR(ops);
+
 	mutex_lock(&dev->lock);
 
 	ret = orion_mdio_wait_ready(ops, bus);
@@ -195,9 +282,13 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 			    int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops = &orion_mdio_smi_ops;
+	const struct orion_mdio_ops *ops;
 	int ret;
 
+	ops = orion_mdio_get_ops(dev, regnum);
+	if (IS_ERR(ops))
+		return PTR_ERR(ops);
+
 	mutex_lock(&dev->lock);
 
 	ret = orion_mdio_wait_ready(ops, bus);
@@ -288,6 +379,9 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
+	dev->bus_type =
+		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+
 	mutex_init(&dev->lock);
 
 	if (pdev->dev.of_node)
@@ -338,7 +432,8 @@ static int orion_mdio_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id orion_mdio_match[] = {
-	{ .compatible = "marvell,orion-mdio" },
+	{ .compatible = "marvell,orion-mdio", .data = (void *)BUS_TYPE_SMI },
+	{ .compatible = "marvell,xmdio", .data = (void *)BUS_TYPE_XSMI },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, orion_mdio_match);
-- 
2.9.4

[PATCH v3 8/9] dt-bindings: orion-mdio: document the new xmdio compatible

[Antoine Tenart]

A new compatible for Marvell xMDIO interfaces was added into the Marvell
MDIO driver. Document this new compatible.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 Documentation/devicetree/bindings/net/marvell-orion-mdio.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
index ccdabdcc8618..b93a5b5a0472 100644
--- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
+++ b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
@@ -1,12 +1,12 @@
 * Marvell MDIO Ethernet Controller interface
 
 The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
-MV78xx0, Armada 370 and Armada XP have an identical unit that provides
-an interface with the MDIO bus. This driver handles this MDIO
-interface.
+MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
+identical unit that provides an interface with the MDIO bus or
+with the xMDIO bus. This driver handles these interfaces.
 
 Required properties:
-- compatible: "marvell,orion-mdio"
+- compatible: "marvell,orion-mdio" or "marvell,xmdio"
 - reg: address and length of the MDIO registers.  When an interrupt is
   not present, the length is the size of the SMI register (4 bytes)
   otherwise it must be 0x84 bytes to cover the interrupt control
-- 
2.9.4

[PATCH v3 8/9] dt-bindings: orion-mdio: document the new xmdio compatible

[Antoine Tenart]

A new compatible for Marvell xMDIO interfaces was added into the Marvell
MDIO driver. Document this new compatible.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 Documentation/devicetree/bindings/net/marvell-orion-mdio.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
index ccdabdcc8618..b93a5b5a0472 100644
--- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
+++ b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
@@ -1,12 +1,12 @@
 * Marvell MDIO Ethernet Controller interface
 
 The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
-MV78xx0, Armada 370 and Armada XP have an identical unit that provides
-an interface with the MDIO bus. This driver handles this MDIO
-interface.
+MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
+identical unit that provides an interface with the MDIO bus or
+with the xMDIO bus. This driver handles these interfaces.
 
 Required properties:
-- compatible: "marvell,orion-mdio"
+- compatible: "marvell,orion-mdio" or "marvell,xmdio"
 - reg: address and length of the MDIO registers.  When an interrupt is
   not present, the length is the size of the SMI register (4 bytes)
   otherwise it must be 0x84 bytes to cover the interrupt control
-- 
2.9.4

[PATCH v3 9/9] arm64: marvell: dts: add xmdio nodes for 7k/8k

[Antoine Tenart]

Add the description of the xMDIO bus for the Marvell Armada 7k and
Marvell Armada 8k; for both CP110 slave and master. This bus is found
on Marvell Ethernet controllers and provides an interface with the
xMDIO bus.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---

@Dave: this patch should go through the mvebu tree as asked by Gregory, thanks!

 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 8 ++++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index 037ed30d75a7..464c41d0e66a 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -98,6 +98,14 @@
 				clocks = <&cpm_syscon0 1 9>, <&cpm_syscon0 1 5>;
 			};
 
+			cpm_xmdio: mdio@12a600 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,xmdio";
+				reg = <0x12a600 0x10>;
+				status = "disabled";
+			};
+
 			cpm_icu: interrupt-controller@1e0000 {
 				compatible = "marvell,cp110-icu"; 
 				reg = <0x1e0000 0x10>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 2a99ff8fca2a..bbb603234f30 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -103,6 +103,14 @@
 				clocks = <&cps_syscon0 1 9>, <&cps_syscon0 1 5>;
 			};
 
+			cps_xmdio: mdio@12a600 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,xmdio";
+				reg = <0x12a600 0x10>;
+				status = "disabled";
+			};
+
 			cps_icu: interrupt-controller@1e0000 {
 				compatible = "marvell,cp110-icu";
 				reg = <0x1e0000 0x10>;
-- 
2.9.4

[PATCH v3 9/9] arm64: marvell: dts: add xmdio nodes for 7k/8k

[Antoine Tenart]

Add the description of the xMDIO bus for the Marvell Armada 7k and
Marvell Armada 8k; for both CP110 slave and master. This bus is found
on Marvell Ethernet controllers and provides an interface with the
xMDIO bus.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---

@Dave: this patch should go through the mvebu tree as asked by Gregory, thanks!

 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 8 ++++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index 037ed30d75a7..464c41d0e66a 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -98,6 +98,14 @@
 				clocks = <&cpm_syscon0 1 9>, <&cpm_syscon0 1 5>;
 			};
 
+			cpm_xmdio: mdio at 12a600 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,xmdio";
+				reg = <0x12a600 0x10>;
+				status = "disabled";
+			};
+
 			cpm_icu: interrupt-controller at 1e0000 {
 				compatible = "marvell,cp110-icu"; 
 				reg = <0x1e0000 0x10>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 2a99ff8fca2a..bbb603234f30 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -103,6 +103,14 @@
 				clocks = <&cps_syscon0 1 9>, <&cps_syscon0 1 5>;
 			};
 
+			cps_xmdio: mdio at 12a600 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,xmdio";
+				reg = <0x12a600 0x10>;
+				status = "disabled";
+			};
+
 			cps_icu: interrupt-controller at 1e0000 {
 				compatible = "marvell,cp110-icu";
 				reg = <0x1e0000 0x10>;
-- 
2.9.4

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> +static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
> +					  int mii_id, int regnum)
> +{
> +	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
> +
> +	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
> +	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +	       MVMDIO_XSMI_READ_OPERATION,
> +	       dev->regs + MVMDIO_XSMI_MGNT_REG);

So what happens if this function gets passed a Clause 22 formatted
request?  Both regnum and mii_id are in the range 0-31.  MII_ADDR_C45
in regnum is clear.

The answer is we produce two Clause 45 frames on the bus, which is
certainly not correct.  You need to trap and error out if MII_ADDR_C45
is not set (as I've already said previously.)

The SMI operations need to do the reverse - they need to fail if they
receive a regnum with MII_ADDR_C45 set, as they are unable to produce
Clause 45 frames.

> +static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
> +				     int regnum, u16 value)
> +{
> +	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
> +
> +	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
> +	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +	       MVMDIO_XSMI_WRITE_OPERATION | value,
> +	       dev->regs + MVMDIO_XSMI_MGNT_REG);

It's even more important here, as a Clause 22 write will produce a
valid Clause 45 pair of frames with dev_addr = 0.

Again, the corresponding SMI code needs to reject Clause 45 requests
as the SMI interface is unable to produce Clause 45 frames.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> +static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
> +					  int mii_id, int regnum)
> +{
> +	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
> +
> +	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
> +	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +	       MVMDIO_XSMI_READ_OPERATION,
> +	       dev->regs + MVMDIO_XSMI_MGNT_REG);

So what happens if this function gets passed a Clause 22 formatted
request?  Both regnum and mii_id are in the range 0-31.  MII_ADDR_C45
in regnum is clear.

The answer is we produce two Clause 45 frames on the bus, which is
certainly not correct.  You need to trap and error out if MII_ADDR_C45
is not set (as I've already said previously.)

The SMI operations need to do the reverse - they need to fail if they
receive a regnum with MII_ADDR_C45 set, as they are unable to produce
Clause 45 frames.

> +static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
> +				     int regnum, u16 value)
> +{
> +	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
> +
> +	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
> +	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +	       MVMDIO_XSMI_WRITE_OPERATION | value,
> +	       dev->regs + MVMDIO_XSMI_MGNT_REG);

It's even more important here, as a Clause 22 write will produce a
valid Clause 45 pair of frames with dev_addr = 0.

Again, the corresponding SMI code needs to reject Clause 45 requests
as the SMI interface is unable to produce Clause 45 frames.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> +						       int regnum)
> +{
> +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> +		return &orion_mdio_xsmi_ops;
> +	else if (dev->bus_type == BUS_TYPE_SMI)
> +		return &orion_mdio_smi_ops;
> +
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

Oh, this is where you're doing it - I'm not sure having this complexity
is really necessary - there is no dynamic choice between the two.  This
seems to be way over-engineered.

You might as well make the SMI operations fail if MII_ADDR_C45 is set,
and the XSMI operations fail if MII_ADDR_C45 is not set.

Hmm, I think this whole driver is over-engineered:

1. the mdio read/write functions implement their own locking.

   At the MDIO level, there is already locking in the form of a per-bus
   lock "bus->mdio_lock" which will be taken whenever either of these
   functions is called.  So the driver's "dev->lock" is redundant.

2. with the redundant locking removed, orion_mdio_write() becomes a
   call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
   It seems that orion_mdio_wait_ready() could be a library function
   shared between a SMI version of orion_mdio_write() and a XSMI version.

3. the same is really true of orion_mdio_read(), although that function
   is a little more complex in itself, the result would actually end up
   being simpler.

With those changes together, it elimates "struct orion_mdio_ops" entirely,
and I think makes the driver smaller, simpler, and cleaner.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> +						       int regnum)
> +{
> +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> +		return &orion_mdio_xsmi_ops;
> +	else if (dev->bus_type == BUS_TYPE_SMI)
> +		return &orion_mdio_smi_ops;
> +
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

Oh, this is where you're doing it - I'm not sure having this complexity
is really necessary - there is no dynamic choice between the two.  This
seems to be way over-engineered.

You might as well make the SMI operations fail if MII_ADDR_C45 is set,
and the XSMI operations fail if MII_ADDR_C45 is not set.

Hmm, I think this whole driver is over-engineered:

1. the mdio read/write functions implement their own locking.

   At the MDIO level, there is already locking in the form of a per-bus
   lock "bus->mdio_lock" which will be taken whenever either of these
   functions is called.  So the driver's "dev->lock" is redundant.

2. with the redundant locking removed, orion_mdio_write() becomes a
   call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
   It seems that orion_mdio_wait_ready() could be a library function
   shared between a SMI version of orion_mdio_write() and a XSMI version.

3. the same is really true of orion_mdio_read(), although that function
   is a little more complex in itself, the result would actually end up
   being simpler.

With those changes together, it elimates "struct orion_mdio_ops" entirely,
and I think makes the driver smaller, simpler, and cleaner.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> > +						       int regnum)
> > +{
> > +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> > +		return &orion_mdio_xsmi_ops;
> > +	else if (dev->bus_type == BUS_TYPE_SMI)
> > +		return &orion_mdio_smi_ops;
> > +
> > +	return ERR_PTR(-EOPNOTSUPP);
> > +}
> 
> Oh, this is where you're doing it - I'm not sure having this complexity
> is really necessary - there is no dynamic choice between the two.  This
> seems to be way over-engineered.
> 
> You might as well make the SMI operations fail if MII_ADDR_C45 is set,
> and the XSMI operations fail if MII_ADDR_C45 is not set.
> 
> Hmm, I think this whole driver is over-engineered:
> 
> 1. the mdio read/write functions implement their own locking.
> 
>    At the MDIO level, there is already locking in the form of a per-bus
>    lock "bus->mdio_lock" which will be taken whenever either of these
>    functions is called.  So the driver's "dev->lock" is redundant.
> 
> 2. with the redundant locking removed, orion_mdio_write() becomes a
>    call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
>    It seems that orion_mdio_wait_ready() could be a library function
>    shared between a SMI version of orion_mdio_write() and a XSMI version.
> 
> 3. the same is really true of orion_mdio_read(), although that function
>    is a little more complex in itself, the result would actually end up
>    being simpler.
> 
> With those changes together, it elimates "struct orion_mdio_ops" entirely,
> and I think makes the driver smaller, simpler, and cleaner.

I wasn't able to completely get rid of the ops structure, but I think
this is cleaner.  I haven't tested these two patches yet.

 drivers/net/ethernet/marvell/mvmdio.c | 276 +++++++++++++++-------------------
 1 file changed, 123 insertions(+), 153 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> > +						       int regnum)
> > +{
> > +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> > +		return &orion_mdio_xsmi_ops;
> > +	else if (dev->bus_type == BUS_TYPE_SMI)
> > +		return &orion_mdio_smi_ops;
> > +
> > +	return ERR_PTR(-EOPNOTSUPP);
> > +}
> 
> Oh, this is where you're doing it - I'm not sure having this complexity
> is really necessary - there is no dynamic choice between the two.  This
> seems to be way over-engineered.
> 
> You might as well make the SMI operations fail if MII_ADDR_C45 is set,
> and the XSMI operations fail if MII_ADDR_C45 is not set.
> 
> Hmm, I think this whole driver is over-engineered:
> 
> 1. the mdio read/write functions implement their own locking.
> 
>    At the MDIO level, there is already locking in the form of a per-bus
>    lock "bus->mdio_lock" which will be taken whenever either of these
>    functions is called.  So the driver's "dev->lock" is redundant.
> 
> 2. with the redundant locking removed, orion_mdio_write() becomes a
>    call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
>    It seems that orion_mdio_wait_ready() could be a library function
>    shared between a SMI version of orion_mdio_write() and a XSMI version.
> 
> 3. the same is really true of orion_mdio_read(), although that function
>    is a little more complex in itself, the result would actually end up
>    being simpler.
> 
> With those changes together, it elimates "struct orion_mdio_ops" entirely,
> and I think makes the driver smaller, simpler, and cleaner.

I wasn't able to completely get rid of the ops structure, but I think
this is cleaner.  I haven't tested these two patches yet.

 drivers/net/ethernet/marvell/mvmdio.c | 276 +++++++++++++++-------------------
 1 file changed, 123 insertions(+), 153 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH 1/2] net: mvmdio: remove duplicate locking

The MDIO layer already provides per-bus locking, so there's no need for
MDIO bus drivers to do their own internal locking.  Remove this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index c43747f0be0b..d6770217965e 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -23,7 +23,6 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
@@ -69,7 +68,6 @@ enum orion_mdio_bus_type {
 };
 
 struct orion_mdio_dev {
-	struct mutex lock;
 	void __iomem *regs;
 	struct clk *clk[3];
 	/*
@@ -254,8 +252,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	if (IS_ERR(ops))
 		return PTR_ERR(ops);
 
-	mutex_lock(&dev->lock);
-
 	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
@@ -274,7 +270,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 
 	ret = ops->read(dev);
 out:
-	mutex_unlock(&dev->lock);
 	return ret;
 }
 
@@ -289,8 +284,6 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 	if (IS_ERR(ops))
 		return PTR_ERR(ops);
 
-	mutex_lock(&dev->lock);
-
 	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
@@ -298,7 +291,6 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 	ops->write(dev, mii_id, regnum, value);
 
 out:
-	mutex_unlock(&dev->lock);
 	return ret;
 }
 
@@ -382,8 +374,6 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	dev->bus_type =
 		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
 
-	mutex_init(&dev->lock);
-
 	if (pdev->dev.of_node)
 		ret = of_mdiobus_register(bus, pdev->dev.of_node);
 	else
-- 
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH 1/2] net: mvmdio: remove duplicate locking

The MDIO layer already provides per-bus locking, so there's no need for
MDIO bus drivers to do their own internal locking.  Remove this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvmdio.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index c43747f0be0b..d6770217965e 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -23,7 +23,6 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
@@ -69,7 +68,6 @@ enum orion_mdio_bus_type {
 };
 
 struct orion_mdio_dev {
-	struct mutex lock;
 	void __iomem *regs;
 	struct clk *clk[3];
 	/*
@@ -254,8 +252,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	if (IS_ERR(ops))
 		return PTR_ERR(ops);
 
-	mutex_lock(&dev->lock);
-
 	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
@@ -274,7 +270,6 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 
 	ret = ops->read(dev);
 out:
-	mutex_unlock(&dev->lock);
 	return ret;
 }
 
@@ -289,8 +284,6 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 	if (IS_ERR(ops))
 		return PTR_ERR(ops);
 
-	mutex_lock(&dev->lock);
-
 	ret = orion_mdio_wait_ready(ops, bus);
 	if (ret < 0)
 		goto out;
@@ -298,7 +291,6 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 	ops->write(dev, mii_id, regnum, value);
 
 out:
-	mutex_unlock(&dev->lock);
 	return ret;
 }
 
@@ -382,8 +374,6 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	dev->bus_type =
 		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
 
-	mutex_init(&dev->lock);
-
 	if (pdev->dev.of_node)
 		ret = of_mdiobus_register(bus, pdev->dev.of_node);
 	else
-- 
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH 2/2] net: mvmdio: get rid of fine-grained ops
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"

The fine-grained ops makes the driver less readable, so let's instead
implement two pairs of read/write mii_bus ops, one for SMI and the
other for XSMI.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvmdio.c | 268 ++++++++++++++++------------------
 1 file changed, 124 insertions(+), 144 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index d6770217965e..d04022db8619 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -84,123 +84,11 @@ struct orion_mdio_dev {
 
 struct orion_mdio_ops {
 	int (*is_done)(struct orion_mdio_dev *);
-	int (*is_read_valid)(struct orion_mdio_dev *);
-	void (*start_read)(struct orion_mdio_dev *, int, int);
-	u16 (*read)(struct orion_mdio_dev *);
-	void (*write)(struct orion_mdio_dev *, int, int, u16);
 
 	unsigned int poll_interval_min;
 	unsigned int poll_interval_max;
 };
 
-/* mdio smi */
-static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
-{
-	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
-}
-
-static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
-}
-
-static void orion_mdio_smi_start_read_op(struct orion_mdio_dev *dev, int mii_id,
-					 int regnum)
-{
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_READ_OPERATION),
-	       dev->regs);
-}
-
-static u16 orion_mdio_smi_read_op(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs) & GENMASK(15, 0);
-}
-
-static void orion_mdio_smi_write_op(struct orion_mdio_dev *dev, int mii_id,
-				    int regnum, u16 value)
-{
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_WRITE_OPERATION            |
-		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->regs);
-}
-
-static const struct orion_mdio_ops orion_mdio_smi_ops = {
-	.is_done = orion_mdio_smi_is_done,
-	.is_read_valid = orion_mdio_smi_is_read_valid,
-	.start_read = orion_mdio_smi_start_read_op,
-	.read = orion_mdio_smi_read_op,
-	.write = orion_mdio_smi_write_op,
-
-	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
-};
-
-/* xmdio xsmi */
-static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
-{
-	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
-}
-
-static int orion_mdio_xsmi_is_read_valid(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_READ_VALID;
-}
-
-static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
-					  int mii_id, int regnum)
-{
-	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
-
-	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
-	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
-	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
-	       MVMDIO_XSMI_READ_OPERATION,
-	       dev->regs + MVMDIO_XSMI_MGNT_REG);
-}
-
-static u16 orion_mdio_xsmi_read_op(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
-}
-
-static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
-				     int regnum, u16 value)
-{
-	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
-
-	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
-	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
-	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
-	       MVMDIO_XSMI_WRITE_OPERATION | value,
-	       dev->regs + MVMDIO_XSMI_MGNT_REG);
-}
-
-static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
-	.is_done = orion_mdio_xsmi_is_done,
-	.is_read_valid = orion_mdio_xsmi_is_read_valid,
-	.start_read = orion_mdio_xsmi_start_read_op,
-	.read = orion_mdio_xsmi_read_op,
-	.write = orion_mdio_xsmi_write_op,
-
-	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
-};
-
-static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
-						       int regnum)
-{
-	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
-		return &orion_mdio_xsmi_ops;
-	else if (dev->bus_type == BUS_TYPE_SMI)
-		return &orion_mdio_smi_ops;
-
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
@@ -241,57 +129,138 @@ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
 	return  -ETIMEDOUT;
 }
 
-static int orion_mdio_read(struct mii_bus *bus, int mii_id,
-			   int regnum)
+/* mdio smi */
+static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
+}
+
+static const struct orion_mdio_ops orion_mdio_smi_ops = {
+	.is_done = orion_mdio_smi_is_done,
+
+	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
+};
+
+static int orion_mdio_smi_read(struct mii_bus *bus, int mii_id,
+			       int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops;
 	int ret;
 
-	ops = orion_mdio_get_ops(dev, regnum);
-	if (IS_ERR(ops))
-		return PTR_ERR(ops);
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ops->start_read(dev, mii_id, regnum);
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_READ_OPERATION),
+	       dev->regs);
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (!ops->is_read_valid(dev)) {
+	if (!(readl(dev->regs) & MVMDIO_SMI_READ_VALID)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
-		ret = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
-	ret = ops->read(dev);
-out:
-	return ret;
+	return readl(dev->regs) & GENMASK(15, 0);
 }
 
-static int orion_mdio_write(struct mii_bus *bus, int mii_id,
-			    int regnum, u16 value)
+static int orion_mdio_smi_write(struct mii_bus *bus, int mii_id,
+				int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops;
 	int ret;
 
-	ops = orion_mdio_get_ops(dev, regnum);
-	if (IS_ERR(ops))
-		return PTR_ERR(ops);
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
+
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_WRITE_OPERATION            |
+		(value << MVMDIO_SMI_DATA_SHIFT)),
+	       dev->regs);
 
-	ops->write(dev, mii_id, regnum, value);
+	return 0;
+}
 
-out:
-	return ret;
+/* xmdio xsmi */
+static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
+}
+
+static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
+	.is_done = orion_mdio_xsmi_is_done,
+
+	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
+};
+
+static int orion_mdio_xsmi_read(struct mii_bus *bus, int mii_id,
+				int regnum)
+{
+	struct orion_mdio_dev *dev = bus->priv;
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+	int ret;
+
+	if (!(regnum & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_READ_OPERATION,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	if (!(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) &
+	      MVMDIO_XSMI_READ_VALID)) {
+		dev_err(bus->parent, "XSMI bus read not valid\n");
+		return -ENODEV;
+	}
+
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
+}
+
+static int orion_mdio_xsmi_write(struct mii_bus *bus, int mii_id,
+				int regnum, u16 value)
+{
+	struct orion_mdio_dev *dev = bus->priv;
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+	int ret;
+
+	if (!(regnum & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_WRITE_OPERATION | value,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+
+	return 0;
 }
 
 static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
@@ -311,11 +280,14 @@ static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
 
 static int orion_mdio_probe(struct platform_device *pdev)
 {
+	enum orion_mdio_bus_type type;
 	struct resource *r;
 	struct mii_bus *bus;
 	struct orion_mdio_dev *dev;
 	int i, ret;
 
+	type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r) {
 		dev_err(&pdev->dev, "No SMI register address given\n");
@@ -328,8 +300,17 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	bus->name = "orion_mdio_bus";
-	bus->read = orion_mdio_read;
-	bus->write = orion_mdio_write;
+
+	switch (type) {
+	case BUS_TYPE_SMI:
+		bus->read = orion_mdio_smi_read;
+		bus->write = orion_mdio_smi_write;
+		break;
+	case BUS_TYPE_XSMI:
+		bus->read = orion_mdio_xsmi_read;
+		bus->write = orion_mdio_xsmi_write;
+		break;
+	}
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
 		 dev_name(&pdev->dev));
 	bus->parent = &pdev->dev;
@@ -371,8 +352,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
-	dev->bus_type =
-		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+	dev->bus_type = type;
 
 	if (pdev->dev.of_node)
 		ret = of_mdiobus_register(bus, pdev->dev.of_node);
-- 
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Russell King - ARM Linux]

From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH 2/2] net: mvmdio: get rid of fine-grained ops
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"

The fine-grained ops makes the driver less readable, so let's instead
implement two pairs of read/write mii_bus ops, one for SMI and the
other for XSMI.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvmdio.c | 268 ++++++++++++++++------------------
 1 file changed, 124 insertions(+), 144 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index d6770217965e..d04022db8619 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -84,123 +84,11 @@ struct orion_mdio_dev {
 
 struct orion_mdio_ops {
 	int (*is_done)(struct orion_mdio_dev *);
-	int (*is_read_valid)(struct orion_mdio_dev *);
-	void (*start_read)(struct orion_mdio_dev *, int, int);
-	u16 (*read)(struct orion_mdio_dev *);
-	void (*write)(struct orion_mdio_dev *, int, int, u16);
 
 	unsigned int poll_interval_min;
 	unsigned int poll_interval_max;
 };
 
-/* mdio smi */
-static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
-{
-	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
-}
-
-static int orion_mdio_smi_is_read_valid(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs) & MVMDIO_SMI_READ_VALID;
-}
-
-static void orion_mdio_smi_start_read_op(struct orion_mdio_dev *dev, int mii_id,
-					 int regnum)
-{
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_READ_OPERATION),
-	       dev->regs);
-}
-
-static u16 orion_mdio_smi_read_op(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs) & GENMASK(15, 0);
-}
-
-static void orion_mdio_smi_write_op(struct orion_mdio_dev *dev, int mii_id,
-				    int regnum, u16 value)
-{
-	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
-		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
-		MVMDIO_SMI_WRITE_OPERATION            |
-		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->regs);
-}
-
-static const struct orion_mdio_ops orion_mdio_smi_ops = {
-	.is_done = orion_mdio_smi_is_done,
-	.is_read_valid = orion_mdio_smi_is_read_valid,
-	.start_read = orion_mdio_smi_start_read_op,
-	.read = orion_mdio_smi_read_op,
-	.write = orion_mdio_smi_write_op,
-
-	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
-};
-
-/* xmdio xsmi */
-static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
-{
-	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
-}
-
-static int orion_mdio_xsmi_is_read_valid(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_READ_VALID;
-}
-
-static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev,
-					  int mii_id, int regnum)
-{
-	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
-
-	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
-	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
-	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
-	       MVMDIO_XSMI_READ_OPERATION,
-	       dev->regs + MVMDIO_XSMI_MGNT_REG);
-}
-
-static u16 orion_mdio_xsmi_read_op(struct orion_mdio_dev *dev)
-{
-	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
-}
-
-static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id,
-				     int regnum, u16 value)
-{
-	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
-
-	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
-	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
-	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
-	       MVMDIO_XSMI_WRITE_OPERATION | value,
-	       dev->regs + MVMDIO_XSMI_MGNT_REG);
-}
-
-static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
-	.is_done = orion_mdio_xsmi_is_done,
-	.is_read_valid = orion_mdio_xsmi_is_read_valid,
-	.start_read = orion_mdio_xsmi_start_read_op,
-	.read = orion_mdio_xsmi_read_op,
-	.write = orion_mdio_xsmi_write_op,
-
-	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
-};
-
-static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
-						       int regnum)
-{
-	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
-		return &orion_mdio_xsmi_ops;
-	else if (dev->bus_type == BUS_TYPE_SMI)
-		return &orion_mdio_smi_ops;
-
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
@@ -241,57 +129,138 @@ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
 	return  -ETIMEDOUT;
 }
 
-static int orion_mdio_read(struct mii_bus *bus, int mii_id,
-			   int regnum)
+/* mdio smi */
+static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
+}
+
+static const struct orion_mdio_ops orion_mdio_smi_ops = {
+	.is_done = orion_mdio_smi_is_done,
+
+	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
+};
+
+static int orion_mdio_smi_read(struct mii_bus *bus, int mii_id,
+			       int regnum)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops;
 	int ret;
 
-	ops = orion_mdio_get_ops(dev, regnum);
-	if (IS_ERR(ops))
-		return PTR_ERR(ops);
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ops->start_read(dev, mii_id, regnum);
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_READ_OPERATION),
+	       dev->regs);
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (!ops->is_read_valid(dev)) {
+	if (!(readl(dev->regs) & MVMDIO_SMI_READ_VALID)) {
 		dev_err(bus->parent, "SMI bus read not valid\n");
-		ret = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
-	ret = ops->read(dev);
-out:
-	return ret;
+	return readl(dev->regs) & GENMASK(15, 0);
 }
 
-static int orion_mdio_write(struct mii_bus *bus, int mii_id,
-			    int regnum, u16 value)
+static int orion_mdio_smi_write(struct mii_bus *bus, int mii_id,
+				int regnum, u16 value)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	const struct orion_mdio_ops *ops;
 	int ret;
 
-	ops = orion_mdio_get_ops(dev, regnum);
-	if (IS_ERR(ops))
-		return PTR_ERR(ops);
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
 
-	ret = orion_mdio_wait_ready(ops, bus);
+	ret = orion_mdio_wait_ready(&orion_mdio_smi_ops, bus);
 	if (ret < 0)
-		goto out;
+		return ret;
+
+	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
+		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
+		MVMDIO_SMI_WRITE_OPERATION            |
+		(value << MVMDIO_SMI_DATA_SHIFT)),
+	       dev->regs);
 
-	ops->write(dev, mii_id, regnum, value);
+	return 0;
+}
 
-out:
-	return ret;
+/* xmdio xsmi */
+static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY);
+}
+
+static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
+	.is_done = orion_mdio_xsmi_is_done,
+
+	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
+	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
+};
+
+static int orion_mdio_xsmi_read(struct mii_bus *bus, int mii_id,
+				int regnum)
+{
+	struct orion_mdio_dev *dev = bus->priv;
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+	int ret;
+
+	if (!(regnum & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_READ_OPERATION,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	if (!(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) &
+	      MVMDIO_XSMI_READ_VALID)) {
+		dev_err(bus->parent, "XSMI bus read not valid\n");
+		return -ENODEV;
+	}
+
+	return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
+}
+
+static int orion_mdio_xsmi_write(struct mii_bus *bus, int mii_id,
+				int regnum, u16 value)
+{
+	struct orion_mdio_dev *dev = bus->priv;
+	u16 dev_addr = (regnum >> 16) & GENMASK(4, 0);
+	int ret;
+
+	if (!(regnum & MII_ADDR_C45))
+		return -EOPNOTSUPP;
+
+	ret = orion_mdio_wait_ready(&orion_mdio_xsmi_ops, bus);
+	if (ret < 0)
+		return ret;
+
+	writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG);
+	writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) |
+	       (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) |
+	       MVMDIO_XSMI_WRITE_OPERATION | value,
+	       dev->regs + MVMDIO_XSMI_MGNT_REG);
+
+	return 0;
 }
 
 static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
@@ -311,11 +280,14 @@ static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
 
 static int orion_mdio_probe(struct platform_device *pdev)
 {
+	enum orion_mdio_bus_type type;
 	struct resource *r;
 	struct mii_bus *bus;
 	struct orion_mdio_dev *dev;
 	int i, ret;
 
+	type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r) {
 		dev_err(&pdev->dev, "No SMI register address given\n");
@@ -328,8 +300,17 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	bus->name = "orion_mdio_bus";
-	bus->read = orion_mdio_read;
-	bus->write = orion_mdio_write;
+
+	switch (type) {
+	case BUS_TYPE_SMI:
+		bus->read = orion_mdio_smi_read;
+		bus->write = orion_mdio_smi_write;
+		break;
+	case BUS_TYPE_XSMI:
+		bus->read = orion_mdio_xsmi_read;
+		bus->write = orion_mdio_xsmi_write;
+		break;
+	}
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
 		 dev_name(&pdev->dev));
 	bus->parent = &pdev->dev;
@@ -371,8 +352,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
-	dev->bus_type =
-		(enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+	dev->bus_type = type;
 
 	if (pdev->dev.of_node)
 		ret = of_mdiobus_register(bus, pdev->dev.of_node);
-- 
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 2443 bytes --]

Hi Russell,

On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> > +						       int regnum)
> > +{
> > +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> > +		return &orion_mdio_xsmi_ops;
> > +	else if (dev->bus_type == BUS_TYPE_SMI)
> > +		return &orion_mdio_smi_ops;
> > +
> > +	return ERR_PTR(-EOPNOTSUPP);
> > +}
> 
> Oh, this is where you're doing it - I'm not sure having this complexity
> is really necessary - there is no dynamic choice between the two.  This
> seems to be way over-engineered.

You're right, there is no dynamic choice between the two. The advantage
is the logic of the read/write operations are not duplicated.

> You might as well make the SMI operations fail if MII_ADDR_C45 is set,
> and the XSMI operations fail if MII_ADDR_C45 is not set.

This check is already done for xSMI operations. But this should also be
the case for SMI ones, you're right.

> 1. the mdio read/write functions implement their own locking.
> 
>    At the MDIO level, there is already locking in the form of a per-bus
>    lock "bus->mdio_lock" which will be taken whenever either of these
>    functions is called.  So the driver's "dev->lock" is redundant.

OK, that's a good rework to add in the series.

> 2. with the redundant locking removed, orion_mdio_write() becomes a
>    call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
>    It seems that orion_mdio_wait_ready() could be a library function
>    shared between a SMI version of orion_mdio_write() and a XSMI version.
> 
> 3. the same is really true of orion_mdio_read(), although that function
>    is a little more complex in itself, the result would actually end up
>    being simpler.

I'm not completely convinced as the read and write functions end up
being duplicated. One for SMI operations and one for xSMI. But I don't
really care, if you think this is better let's go for it.

Should I add your first patch in my series and squash the second one in
patch 7/9? (I'll also remove the bus_type member from the private
struct, as well as the one line it's used in the probe).

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 7/9] net: mvmdio: add xmdio xsmi support

[Antoine Tenart]

Hi Russell,

On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote:
> > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev,
> > +						       int regnum)
> > +{
> > +	if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45))
> > +		return &orion_mdio_xsmi_ops;
> > +	else if (dev->bus_type == BUS_TYPE_SMI)
> > +		return &orion_mdio_smi_ops;
> > +
> > +	return ERR_PTR(-EOPNOTSUPP);
> > +}
> 
> Oh, this is where you're doing it - I'm not sure having this complexity
> is really necessary - there is no dynamic choice between the two.  This
> seems to be way over-engineered.

You're right, there is no dynamic choice between the two. The advantage
is the logic of the read/write operations are not duplicated.

> You might as well make the SMI operations fail if MII_ADDR_C45 is set,
> and the XSMI operations fail if MII_ADDR_C45 is not set.

This check is already done for xSMI operations. But this should also be
the case for SMI ones, you're right.

> 1. the mdio read/write functions implement their own locking.
> 
>    At the MDIO level, there is already locking in the form of a per-bus
>    lock "bus->mdio_lock" which will be taken whenever either of these
>    functions is called.  So the driver's "dev->lock" is redundant.

OK, that's a good rework to add in the series.

> 2. with the redundant locking removed, orion_mdio_write() becomes a
>    call to orion_mdio_wait_ready() followed by a call to dev->ops->write.
>    It seems that orion_mdio_wait_ready() could be a library function
>    shared between a SMI version of orion_mdio_write() and a XSMI version.
> 
> 3. the same is really true of orion_mdio_read(), although that function
>    is a little more complex in itself, the result would actually end up
>    being simpler.

I'm not completely convinced as the read and write functions end up
being duplicated. One for SMI operations and one for xSMI. But I don't
really care, if you think this is better let's go for it.

Should I add your first patch in my series and squash the second one in
patch 7/9? (I'll also remove the bus_type member from the private
struct, as well as the one line it's used in the probe).

Thanks!
Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170612/9488ca0c/attachment.sig>