[PATCH v2 0/8] net: mvmdio: add xSMI support

[PATCH v2 0/8] net: mvmdio: add xSMI support

[Antoine Tenart]

Hello,

This series aims to add the xSMI support (also called xMDIO) 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-8 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 8 should go through the mvebu tree as asked by Gregory,
thanks!

Thanks,
Antoine

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 (8):
  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 support
  arm64: marvell: dts: add xmdio nodes for 7k/8k

 .../devicetree/bindings/net/marvell-orion-mdio.txt |   6 +-
 .../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              | 196 ++++++++++++++++-----
 5 files changed, 178 insertions(+), 46 deletions(-)

-- 
2.9.4

[PATCH v2 1/8] 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 v2 2/8] 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 v2 3/8] 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..96af8d57d9e5 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 v2 3/8] net: mvmdio: use GENMASK for masks

[Sergei Shtylyov]

Hello!

On 6/8/2017 12:26 PM, Antoine Tenart wrote:

> 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..96af8d57d9e5 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);

    Need a space after comma.

MBR, Sergei

[PATCH v2 4/8] 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 96af8d57d9e5..6cbdb221c1ab 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 v2 5/8] 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 6cbdb221c1ab..f66b474b382a 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 v2 6/8] 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 f66b474b382a..07b18f60da2a 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 v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

This patch adds the xMDIO 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 (while the SMI interface complies with the clause
22). The xSMI interface is used by 10GbE devices.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/net/marvell-orion-mdio.txt |  6 +-
 drivers/net/ethernet/marvell/Kconfig               |  6 +-
 drivers/net/ethernet/marvell/mvmdio.c              | 76 +++++++++++++++++++++-
 3 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
index ccdabdcc8618..36be4ca5dab2 100644
--- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
+++ b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
@@ -1,9 +1,9 @@
 * 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 to
+the xMDIO bus. This driver handles these interfaces.
 
 Required properties:
 - compatible: "marvell,orion-mdio"
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 07b18f60da2a..7b650ef67347 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -41,6 +41,15 @@
 #define  MVMDIO_ERR_INT_SMI_DONE	0x00000010
 #define MVMDIO_ERR_INT_MASK		0x0080
 
+#define MVMDIO_XSMI_MGNT_REG		0x0
+#define  MVMDIO_XSMI_READ_VALID		BIT(29)
+#define  MVMDIO_XSMI_BUSY		BIT(30)
+#define MVMDIO_XSMI_ADDR_REG		0x8
+#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
+#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
+#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
+#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 27)
+
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
@@ -50,6 +59,9 @@
 #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
+
 struct orion_mdio_dev {
 	struct mutex lock;
 	void __iomem *regs;
@@ -75,6 +87,7 @@ struct orion_mdio_ops {
 	unsigned int poll_interval_max;
 };
 
+/* smi */
 static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 {
 	return !(readl(dev->regs) & MVMDIO_SMI_BUSY);
@@ -120,6 +133,65 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
+/* 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;
+
+	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;
+
+	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(int regnum)
+{
+	if (regnum & MII_ADDR_C45)
+		return &orion_mdio_xsmi_ops;
+
+	return &orion_mdio_smi_ops;
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
@@ -164,7 +236,7 @@ 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 = orion_mdio_get_ops(regnum);
 	int ret;
 
 	mutex_lock(&dev->lock);
@@ -195,7 +267,7 @@ 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 = orion_mdio_get_ops(regnum);
 	int ret;
 
 	mutex_lock(&dev->lock);
-- 
2.9.4

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Andrew Lunn]

On Thu, Jun 08, 2017 at 11:26:52AM +0200, Antoine Tenart wrote:
> +#define MVMDIO_XSMI_MGNT_REG		0x0
> +#define  MVMDIO_XSMI_READ_VALID		BIT(29)
> +#define  MVMDIO_XSMI_BUSY		BIT(30)
> +#define MVMDIO_XSMI_ADDR_REG		0x8
> +#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
> +#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
> +#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
> +#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 27)

Hi Antoine

These two operations seem odd. Generally ops have the same shift.

      Andrew

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

Hi Andrew,

On Thu, Jun 08, 2017 at 06:03:31PM +0200, Andrew Lunn wrote:
> On Thu, Jun 08, 2017 at 11:26:52AM +0200, Antoine Tenart wrote:
> > +#define MVMDIO_XSMI_MGNT_REG		0x0
> > +#define  MVMDIO_XSMI_READ_VALID		BIT(29)
> > +#define  MVMDIO_XSMI_BUSY		BIT(30)
> > +#define MVMDIO_XSMI_ADDR_REG		0x8
> > +#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
> > +#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
> > +#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
> > +#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 27)
> 
> These two operations seem odd. Generally ops have the same shift.

Indeed, this is odd. I'll have a look at this.

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/20170609/d6fc7877/attachment.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Russell King - ARM Linux]

On Fri, Jun 09, 2017 at 08:40:19AM +0200, Antoine Tenart wrote:
> Hi Andrew,
> 
> On Thu, Jun 08, 2017 at 06:03:31PM +0200, Andrew Lunn wrote:
> > On Thu, Jun 08, 2017 at 11:26:52AM +0200, Antoine Tenart wrote:
> > > +#define MVMDIO_XSMI_MGNT_REG		0x0
> > > +#define  MVMDIO_XSMI_READ_VALID		BIT(29)
> > > +#define  MVMDIO_XSMI_BUSY		BIT(30)
> > > +#define MVMDIO_XSMI_ADDR_REG		0x8
> > > +#define  MVMDIO_XSMI_PHYADDR_SHIFT	16
> > > +#define  MVMDIO_XSMI_DEVADDR_SHIFT	21
> > > +#define  MVMDIO_XSMI_READ_OPERATION	(0x7 << 26)
> > > +#define  MVMDIO_XSMI_WRITE_OPERATION	(0x5 << 27)
> > 
> > These two operations seem odd. Generally ops have the same shift.
> 
> Indeed, this is odd. I'll have a look at this.

The Marvell driver uses 5 << 26:

+#define XOPCODE_OFFS           26
+#define XOPCODE_ADDR_READ      (7 << XOPCODE_OFFS)
+#define XOPCODE_ADDR_WRITE     (5 << XOPCODE_OFFS)

What this means is that with the incorrect shift in your driver,
although writes appeared to work, they actually resulted in a
post-read-increment-address frame (and hence no error.)

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

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Florian Fainelli]

On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> This patch adds the xMDIO 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 (while the SMI interface complies with the clause
> 22). The xSMI interface is used by 10GbE devices.

In the previous version you were properly defining a new compatibles
strings for xmdio, but now you don't and instead you runtime select the
operations based on whether MII_ADDR_C45 is set in the register which is
fine from a functional perspective.

If I get this right, the xMDIO controller is actually a superset of the
MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
preform C45 accesses?

If that is the case (and looking at patch 8 that seems to be the case),
you probably still need to define a new compatible string for that
block, because it has a different register layout than its predecessor.

[snip]
>  static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
> @@ -164,7 +236,7 @@ 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 = orion_mdio_get_ops(regnum);
>  	int ret;
>  
>  	mutex_lock(&dev->lock);
> @@ -195,7 +267,7 @@ 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 = orion_mdio_get_ops(regnum);
>  	int ret;

ok, that seems to work since you get the operation based on
MII_ADDR_C45. Thanks!
--
Florian

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Andrew Lunn]

On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> > This patch adds the xMDIO 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 (while the SMI interface complies with the clause
> > 22). The xSMI interface is used by 10GbE devices.
> 
> In the previous version you were properly defining a new compatibles
> strings for xmdio, but now you don't and instead you runtime select the
> operations based on whether MII_ADDR_C45 is set in the register which is
> fine from a functional perspective.
> 
> If I get this right, the xMDIO controller is actually a superset of the
> MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> preform C45 accesses?
> 
> If that is the case (and looking at patch 8 that seems to be the case),
> you probably still need to define a new compatible string for that
> block, because it has a different register layout than its predecessor.

Yes, i think you need the compatible string to return -EOPNOSUP when
somebody tries to do a C45 access on the older IP which only has C22.

	 Andrew

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

Hello Florian, Andrew,

On Thu, Jun 08, 2017 at 06:55:46PM +0200, Andrew Lunn wrote:
> On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> > On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> > > This patch adds the xMDIO 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 (while the SMI interface complies with the clause
> > > 22). The xSMI interface is used by 10GbE devices.
> > 
> > In the previous version you were properly defining a new compatibles
> > strings for xmdio, but now you don't and instead you runtime select the
> > operations based on whether MII_ADDR_C45 is set in the register which is
> > fine from a functional perspective.
> > 
> > If I get this right, the xMDIO controller is actually a superset of the
> > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> > preform C45 accesses?
> > 
> > If that is the case (and looking at patch 8 that seems to be the case),
> > you probably still need to define a new compatible string for that
> > block, because it has a different register layout than its predecessor.
> 
> Yes, i think you need the compatible string to return -EOPNOSUP when
> somebody tries to do a C45 access on the older IP which only has C22.

That's a very good point. I'll update the series to fix this.

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/20170609/02ae2b79/attachment.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> > This patch adds the xMDIO 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 (while the SMI interface complies with the clause
> > 22). The xSMI interface is used by 10GbE devices.
> 
> In the previous version you were properly defining a new compatibles
> strings for xmdio, but now you don't and instead you runtime select the
> operations based on whether MII_ADDR_C45 is set in the register which is
> fine from a functional perspective.
> 
> If I get this right, the xMDIO controller is actually a superset of the
> MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> preform C45 accesses?

This is also a mistake. It's not a superset as the register offsets are
different. So we can't use the same smi operations in both cases. We
would need dedicated xmdio smi operations, but I don't think there is a
board where a c22 PHY is connected to the xMDIO interface.

I'll add smi and xsmi flags and return -ENOTSUPP based on the
MII_ADDR_C45 bit and flags combination. If the need to support smi
operations on the xMDIO bus arise it will be fairly easy to implement in
the future. I'll rename the ops functions as well to differentiate mdio
and xmdio operations.

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/20170609/4782d8b0/attachment-0001.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Andrew Lunn]

On Fri, Jun 09, 2017 at 10:25:41AM +0200, Antoine Tenart wrote:
> On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> > On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> > > This patch adds the xMDIO 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 (while the SMI interface complies with the clause
> > > 22). The xSMI interface is used by 10GbE devices.
> > 
> > In the previous version you were properly defining a new compatibles
> > strings for xmdio, but now you don't and instead you runtime select the
> > operations based on whether MII_ADDR_C45 is set in the register which is
> > fine from a functional perspective.
> > 
> > If I get this right, the xMDIO controller is actually a superset of the
> > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> > preform C45 accesses?
> 
> This is also a mistake. It's not a superset as the register offsets are
> different. So we can't use the same smi operations in both cases. We
> would need dedicated xmdio smi operations, but I don't think there is a
> board where a c22 PHY is connected to the xMDIO interface.

Hi Antoine

I don't have the datasheet...

Is it described as one device, which can do c22 via one set of
registers, and c45 by another set of registers?

Or are there two separate devices. In particular, does each device
have there own MDC and MDIO pins? Do we have an C22 only device/bus,
and a C45 only device/bus?

    Andrew

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

Hi Andrew,

On Fri, Jun 09, 2017 at 03:26:24PM +0200, Andrew Lunn wrote:
> On Fri, Jun 09, 2017 at 10:25:41AM +0200, Antoine Tenart wrote:
> > On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> > > 
> > > If I get this right, the xMDIO controller is actually a superset of the
> > > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> > > preform C45 accesses?
> > 
> > This is also a mistake. It's not a superset as the register offsets are
> > different. So we can't use the same smi operations in both cases. We
> > would need dedicated xmdio smi operations, but I don't think there is a
> > board where a c22 PHY is connected to the xMDIO interface.
> 
> Is it described as one device, which can do c22 via one set of
> registers, and c45 by another set of registers?
> 
> Or are there two separate devices. In particular, does each device
> have there own MDC and MDIO pins? Do we have an C22 only device/bus,
> and a C45 only device/bus?

The MDIO/xMDIO registers are embedded into the network controller. The
mvmdio driver was created at first to abstract this functionality
outside the network controller driver because it is shared between all
ports and used in different IPs. So it's not really devices per say.

Looking at the datasheet/schematics there are two hardware buses, one
for c22 and one for c45. So we should keep two separate nodes to
describe the two interfaces. From what I read c45 is backward
compatible with c22 so the xSMI interface should be capable to speak to
c22 PHYs as well. And by looking at the datasheet this seems possible,
although it's not completely clear. But anyway this wouldn't impact the
dt bindings.

What I'll propose in v3 is two separate nodes, with their own
compatibles. The driver will have smi ops for the marvell,orion-mdio and
xsmi ops for the marvell,xmdio. I won't implement for now the smi ops
for marvell,xmdio as I can't test them and can't be 100% sure it would
work. (But again this won't impact the dt and can be updated in the
driver later).

Does that sound right?

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/20170609/cbd68fcc/attachment-0001.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Andrew Lunn]

On Fri, Jun 09, 2017 at 04:09:22PM +0200, Antoine Tenart wrote:
> Hi Andrew,
> 
> On Fri, Jun 09, 2017 at 03:26:24PM +0200, Andrew Lunn wrote:
> > On Fri, Jun 09, 2017 at 10:25:41AM +0200, Antoine Tenart wrote:
> > > On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> > > > 
> > > > If I get this right, the xMDIO controller is actually a superset of the
> > > > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> > > > preform C45 accesses?
> > > 
> > > This is also a mistake. It's not a superset as the register offsets are
> > > different. So we can't use the same smi operations in both cases. We
> > > would need dedicated xmdio smi operations, but I don't think there is a
> > > board where a c22 PHY is connected to the xMDIO interface.
> > 
> > Is it described as one device, which can do c22 via one set of
> > registers, and c45 by another set of registers?
> > 
> > Or are there two separate devices. In particular, does each device
> > have there own MDC and MDIO pins? Do we have an C22 only device/bus,
> > and a C45 only device/bus?
> 
> The MDIO/xMDIO registers are embedded into the network controller. The
> mvmdio driver was created at first to abstract this functionality
> outside the network controller driver because it is shared between all
> ports and used in different IPs. So it's not really devices per say.
> 
> Looking at the datasheet/schematics there are two hardware buses, one
> for c22 and one for c45. So we should keep two separate nodes to
> describe the two interfaces. From what I read c45 is backward
> compatible with c22 so the xSMI interface should be capable to speak to
> c22 PHYs as well.

The on the wire protocol of c45 is backwards compatible with c22, in
that a c22 device will not get confused by a c45 transaction on the
bus. A c22 device will just ignore it. You cannot talk to a c22 device
using c45.

>From what you are saying, you have a hardware block generating c45
transactions and a hardware block which generates c22 transactions.
What is not clear to me is if these two hardware blocks are using the
same physical MDC/MDIO pins, i.e. there is just one MDIO bus to the
outside world, or are there two busses?

> And by looking at the datasheet this seems possible, although it's
> not completely clear. But anyway this wouldn't impact the dt
> bindings.

What i'm worried about is there being one set of MDC/MDIO lines. You
should not expose that to linux as two mdio busses. It is one bus.

The phylib will poll each phy on the bus once per second to check its
state. The phylib serialises the read/writes at a bus level. So if you
have one physical bus registered as two logical bus, at some point you
are going to get simultaneous read/writes, and you are going to need a
mutex low down to serialise this for physical bus access.

And in the end, this would affect the dt binding. If it is one
physical bus, you want one binding representing both c22 and c45
transactions.

      Andrew

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

Hi Andrew,

On Fri, Jun 09, 2017 at 04:49:36PM +0200, Andrew Lunn wrote:
> On Fri, Jun 09, 2017 at 04:09:22PM +0200, Antoine Tenart wrote:
> > 
> > The MDIO/xMDIO registers are embedded into the network controller. The
> > mvmdio driver was created at first to abstract this functionality
> > outside the network controller driver because it is shared between all
> > ports and used in different IPs. So it's not really devices per say.
> > 
> > Looking at the datasheet/schematics there are two hardware buses, one
> > for c22 and one for c45. So we should keep two separate nodes to
> > describe the two interfaces. From what I read c45 is backward
> > compatible with c22 so the xSMI interface should be capable to speak to
> > c22 PHYs as well.
> 
> The on the wire protocol of c45 is backwards compatible with c22, in
> that a c22 device will not get confused by a c45 transaction on the
> bus. A c22 device will just ignore it. You cannot talk to a c22 device
> using c45.

I see.

> From what you are saying, you have a hardware block generating c45
> transactions and a hardware block which generates c22 transactions.
> What is not clear to me is if these two hardware blocks are using the
> same physical MDC/MDIO pins, i.e. there is just one MDIO bus to the
> outside world, or are there two busses?

There are two busses, one generating c22 transactions and one generating
c45 transactions. Each bus has its own MDC/MDIO pins.

> > And by looking at the datasheet this seems possible, although it's
> > not completely clear. But anyway this wouldn't impact the dt
> > bindings.
> 
> What i'm worried about is there being one set of MDC/MDIO lines. You
> should not expose that to linux as two mdio busses. It is one bus.
> 
> The phylib will poll each phy on the bus once per second to check its
> state. The phylib serialises the read/writes at a bus level. So if you
> have one physical bus registered as two logical bus, at some point you
> are going to get simultaneous read/writes, and you are going to need a
> mutex low down to serialise this for physical bus access.
> 
> And in the end, this would affect the dt binding. If it is one
> physical bus, you want one binding representing both c22 and c45
> transactions.

Of course. See above.

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/20170609/a03cf96d/attachment.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Andrew Lunn]

> There are two busses, one generating c22 transactions and one generating
> c45 transactions. Each bus has its own MDC/MDIO pins.

O.K. That is what i wanted to know. So we want two completely separate
device tree bindings, busses registered with Linux, etc.

Thanks for clarification.

       Andrew

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Antoine Tenart]

On Fri, Jun 09, 2017 at 05:03:40PM +0200, Andrew Lunn wrote:
> > There are two busses, one generating c22 transactions and one generating
> > c45 transactions. Each bus has its own MDC/MDIO pins.
> 
> O.K. That is what i wanted to know. So we want two completely separate
> device tree bindings, busses registered with Linux, etc.
> 
> Thanks for clarification.

So in the end I need one change in v3: to bind the xSMI usage to
marvell,xmdio and the SMI one to marvell,orion-mdio. (Plus the GENMASK
and offset comments).

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/20170609/312316eb/attachment.sig>

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Russell King - ARM Linux]

On Fri, Jun 09, 2017 at 06:22:16PM +0200, Antoine Tenart wrote:
> On Fri, Jun 09, 2017 at 05:03:40PM +0200, Andrew Lunn wrote:
> > > There are two busses, one generating c22 transactions and one generating
> > > c45 transactions. Each bus has its own MDC/MDIO pins.
> > 
> > O.K. That is what i wanted to know. So we want two completely separate
> > device tree bindings, busses registered with Linux, etc.
> > 
> > Thanks for clarification.
> 
> So in the end I need one change in v3: to bind the xSMI usage to
> marvell,xmdio and the SMI one to marvell,orion-mdio. (Plus the GENMASK
> and offset comments).

Also, you need to
1. trap out on incorrect MII_ADDR_C45 in regnum for the interface.
2. mask the dev_addr with GENMASK(4, 0) (as merely shifting will leave
   the MII_ADDR_C45 bit set.)
3. moving MVMDIO_XSMI_PHYADDR_SHIFT / MVMDIO_XSMI_DEVADDR_SHIFT /
   MVMDIO_XSMI_READ_OPERATION / MVMDIO_XSMI_WRITE_OPERATION under the
   MVMDIO_XSMI_MGNT_REG reg - these definitions are nothing to do with
   MVMDIO_XSMI_ADDR_REG.
4. fixing MVMDIO_XSMI_WRITE_OPERATION to be 5 << 26, not 5 << 27.

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

[PATCH v2 7/8] net: mvmdio: add xmdio support

[Russell King - ARM Linux]

On Fri, Jun 09, 2017 at 04:49:36PM +0200, Andrew Lunn wrote:
> On Fri, Jun 09, 2017 at 04:09:22PM +0200, Antoine Tenart wrote:
> > The MDIO/xMDIO registers are embedded into the network controller. The
> > mvmdio driver was created at first to abstract this functionality
> > outside the network controller driver because it is shared between all
> > ports and used in different IPs. So it's not really devices per say.
> > 
> > Looking at the datasheet/schematics there are two hardware buses, one
> > for c22 and one for c45. So we should keep two separate nodes to
> > describe the two interfaces. From what I read c45 is backward
> > compatible with c22 so the xSMI interface should be capable to speak to
> > c22 PHYs as well.
> 
> The on the wire protocol of c45 is backwards compatible with c22, in
> that a c22 device will not get confused by a c45 transaction on the
> bus. A c22 device will just ignore it. You cannot talk to a c22 device
> using c45.

>From what I can tell, having 'scoped the MDIO line and tried writing
several different values to the XSMI registers, it is not possible
for the XSMI block to generate C22 frame structures - the "start"
bits are always "00", and I can't make them the required "01" for C22.

However, I can confirm that bits 26 and 27 of the XSMI register are used
directly for the OP field (so 0 << 26 produces a C45 address frame.)  I
suspect, although I haven't delved that deeply (yet) that bit 28 sets
whether XSMI produces an address cycle itself along with the data cycle.

Bit 31 appears to be writable, but has no effect on the frame structure.

> What i'm worried about is there being one set of MDC/MDIO lines. You
> should not expose that to linux as two mdio busses. It is one bus.

We're independent - the SMI and XSMI blocks are two entirely separate
interfaces with entirely separate hardware MDC/MDIO lines.  The XSMI
MDC/MDIO lines remain at logic '1' while phylib polls the PHY via
the SMI interface.

-- 
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 v2 8/8] 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: patch 8 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..ab728aacdfae 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,orion-mdio";
+				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..89af7a31e622 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,orion-mdio";
+				reg = <0x12a600 0x10>;
+				status = "disabled";
+			};
+
 			cps_icu: interrupt-controller at 1e0000 {
 				compatible = "marvell,cp110-icu";
 				reg = <0x1e0000 0x10>;
-- 
2.9.4