[RFC PATCH 0/4] net: mvmdio: close the gaps between mv643xx_eth and mvmdio

[RFC PATCH 0/4] net: mvmdio: close the gaps between mv643xx_eth and mvmdio

[Florian Fainelli]

Hi all,

This patch series attempts to close the gaps between the SMI/MDIO driver
implemented and use by the Marvell MV643XX ethernet driver and Thomas' MVMDIO
driver. I did not convert yet mv643xx_eth to use it as it would require proper
Device Tree bindings (which is being worked on) to easily hook into it.

Note that I could not test this properly yet, thus the RFC, and I would like to
find a better way of setting/getting the PHY address than using two exports,
although I have tried to make them easy enough to use so the following logic in
Ethernet drivers can be used:

- lookup device tree handle from MDIO device tree node
- lookup corresponding struct platform_device from the device tree handle
- call the helpers

Comment welcome!

Florian Fainelli (4):
  net: mvmdio: rename base register cookie from smireg to regs
  net: mvmdio: do not assume SMI reg is at offset 0 of the resource
  net: mvmdio: enhance driver to support SMI error/done interrupts
  net: mvmdio: add getter and setter for PHY addresses

 .../devicetree/bindings/net/marvell-orion-mdio.txt |    5 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |    2 +-
 drivers/net/ethernet/marvell/mvmdio.c              |  125 +++++++++++++++++---
 include/linux/marvell_mvmdio.h                     |   10 ++
 4 files changed, 122 insertions(+), 20 deletions(-)
 create mode 100644 include/linux/marvell_mvmdio.h

-- 
1.7.10.4

[RFC PATCH 1/4] net: mvmdio: rename base register cookie from smireg to regs

[Florian Fainelli]

This patch renames the base register cookie in the mvmdio drive from
"smireg" to "regs" since a subsequent patch is going to use an ioremap()
cookie whose size is larger than a single register of 4 bytes. No
functionnal code change introduced.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/net/ethernet/marvell/mvmdio.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 74f1c15..e4a89b2 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -39,7 +39,7 @@
 
 struct orion_mdio_dev {
 	struct mutex lock;
-	void __iomem *smireg;
+	void __iomem *regs;
 };
 
 /* Wait for the SMI unit to be ready for another operation
@@ -52,7 +52,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 
 	count = 0;
 	while (1) {
-		val = readl(dev->smireg);
+		val = readl(dev->regs);
 		if (!(val & MVMDIO_SMI_BUSY))
 			break;
 
@@ -87,12 +87,12 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
 		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
 		MVMDIO_SMI_READ_OPERATION),
-	       dev->smireg);
+	       dev->regs);
 
 	/* Wait for the value to become available */
 	count = 0;
 	while (1) {
-		val = readl(dev->smireg);
+		val = readl(dev->regs);
 		if (val & MVMDIO_SMI_READ_VALID)
 			break;
 
@@ -129,7 +129,7 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
 		MVMDIO_SMI_WRITE_OPERATION            |
 		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->smireg);
+	       dev->regs);
 
 	mutex_unlock(&dev->lock);
 
@@ -173,8 +173,8 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		bus->irq[i] = PHY_POLL;
 
 	dev = bus->priv;
-	dev->smireg = of_iomap(pdev->dev.of_node, 0);
-	if (!dev->smireg) {
+	dev->regs = of_iomap(pdev->dev.of_node, 0);
+	if (!dev->regs) {
 		dev_err(&pdev->dev, "No SMI register address given in DT\n");
 		kfree(bus->irq);
 		mdiobus_free(bus);
@@ -186,7 +186,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	ret = of_mdiobus_register(bus, np);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
-		iounmap(dev->smireg);
+		iounmap(dev->regs);
 		kfree(bus->irq);
 		mdiobus_free(bus);
 		return ret;
-- 
1.7.10.4

[RFC PATCH 2/4] net: mvmdio: do not assume SMI reg is at offset 0 of the resource

[Florian Fainelli]

This patch changes the mvmdio driver not to assume that the
MVMDIO_SMI_REG is at offset 0 of the resource we are being passed via
Device Tree. This is actually required to reduce the differences between
the mv643xx_eth SMI driver and mvmdio. The only user of the "orion-mdio"
binding is updated accordingly.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 Documentation/devicetree/bindings/net/marvell-orion-mdio.txt |    2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi                         |    2 +-
 drivers/net/ethernet/marvell/mvmdio.c                        |    9 +++++----
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
index 34e7aaf..3320d5c 100644
--- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
+++ b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
@@ -19,7 +19,7 @@ mdio {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "marvell,orion-mdio";
-	reg = <0xd0072004 0x4>;
+	reg = <0xd0072000 0x8>;
 };
 
 And at the board level:
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 4c0abe8..0e7d880 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -91,7 +91,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "marvell,orion-mdio";
-			reg = <0xd0072004 0x4>;
+			reg = <0xd0072000 0x8>;
 		};
 
 		ethernet at d0070000 {
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index e4a89b2..16be140 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -29,6 +29,7 @@
 #include <linux/platform_device.h>
 #include <linux/delay.h>
 
+#define MVMDIO_SMI_REG			   0x0004
 #define MVMDIO_SMI_DATA_SHIFT              0
 #define MVMDIO_SMI_PHY_ADDR_SHIFT          16
 #define MVMDIO_SMI_PHY_REG_SHIFT           21
@@ -52,7 +53,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 
 	count = 0;
 	while (1) {
-		val = readl(dev->regs);
+		val = readl(dev->regs + MVMDIO_SMI_REG);
 		if (!(val & MVMDIO_SMI_BUSY))
 			break;
 
@@ -87,12 +88,12 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,
 	writel(((mii_id << MVMDIO_SMI_PHY_ADDR_SHIFT) |
 		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
 		MVMDIO_SMI_READ_OPERATION),
-	       dev->regs);
+	       dev->regs + MVMDIO_SMI_REG);
 
 	/* Wait for the value to become available */
 	count = 0;
 	while (1) {
-		val = readl(dev->regs);
+		val = readl(dev->regs + MVMDIO_SMI_REG);
 		if (val & MVMDIO_SMI_READ_VALID)
 			break;
 
@@ -129,7 +130,7 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,
 		(regnum << MVMDIO_SMI_PHY_REG_SHIFT)  |
 		MVMDIO_SMI_WRITE_OPERATION            |
 		(value << MVMDIO_SMI_DATA_SHIFT)),
-	       dev->regs);
+	       dev->regs + MVMDIO_SMI_REG);
 
 	mutex_unlock(&dev->lock);
 
-- 
1.7.10.4

[RFC PATCH 3/4] net: mvmdio: enhance driver to support SMI error/done interrupts

[Florian Fainelli]

This patch enhances the "mvmdio" to support a SMI error/done interrupt
line which can be used along with a wait queue instead of doing
busy-waiting on the registers. This is a feature which is available in
the mv643xx_eth SMI code and thus reduces again the gap between the two.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 .../devicetree/bindings/net/marvell-orion-mdio.txt |    3 +
 drivers/net/ethernet/marvell/mvmdio.c              |   81 +++++++++++++++++---
 2 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
index 3320d5c..1abf47a 100644
--- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
+++ b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
@@ -9,6 +9,9 @@ Required properties:
 - compatible: "marvell,orion-mdio"
 - reg: address and length of the SMI register
 
+Optional properties:
+- interrupts: interrupt line number for the SMI error/done
+
 The child nodes of the MDIO driver are the individual PHY devices
 connected to this MDIO bus. They must have a "reg" property given the
 PHY address on the MDIO bus.
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 16be140..bdd9047 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -24,10 +24,14 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/phy.h>
+#include <linux/interrupt.h>
 #include <linux/of_address.h>
 #include <linux/of_mdio.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
 
 #define MVMDIO_SMI_REG			   0x0004
 #define MVMDIO_SMI_DATA_SHIFT              0
@@ -37,12 +41,28 @@
 #define MVMDIO_SMI_WRITE_OPERATION         0
 #define MVMDIO_SMI_READ_VALID              BIT(27)
 #define MVMDIO_SMI_BUSY                    BIT(28)
+#define MVMDIO_ERR_INT_CAUSE		   0x0080
+#define  MVMDIO_ERR_INT_SMI_DONE	   0x00000010
+#define MVMDIO_ERR_INT_MASK		   0x0084
 
 struct orion_mdio_dev {
 	struct mutex lock;
 	void __iomem *regs;
+	/*
+	 * If we have access to the error interrupt pin (which is
+	 * somewhat misnamed as it not only reflects internal errors
+	 * but also reflects SMI completion), use that to wait for
+	 * SMI access completion instead of polling the SMI busy bit.
+	 */
+	int err_interrupt;
+	wait_queue_head_t smi_busy_wait;
 };
 
+static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
+{
+	return !(readl(dev->regs + MVMDIO_SMI_REG) & MVMDIO_SMI_BUSY);
+}
+
 /* Wait for the SMI unit to be ready for another operation
  */
 static int orion_mdio_wait_ready(struct mii_bus *bus)
@@ -51,19 +71,30 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
 	int count;
 	u32 val;
 
-	count = 0;
-	while (1) {
-		val = readl(dev->regs + MVMDIO_SMI_REG);
-		if (!(val & MVMDIO_SMI_BUSY))
-			break;
-
-		if (count > 100) {
-			dev_err(bus->parent, "Timeout: SMI busy for too long\n");
-			return -ETIMEDOUT;
+	if (dev->err_interrupt == NO_IRQ) {
+		count = 0;
+		while (1) {
+			val = readl(dev->regs + MVMDIO_SMI_REG);
+			if (!(val & MVMDIO_SMI_BUSY))
+				break;
+
+			if (count > 100) {
+				dev_err(bus->parent,
+					"Timeout: SMI busy for too long\n");
+				return -ETIMEDOUT;
+			}
+
+			udelay(10);
+			count++;
 		}
+	}
 
-		udelay(10);
-		count++;
+	if (!orion_mdio_smi_is_done(dev)) {
+		wait_event_timeout(dev->smi_busy_wait,
+				orion_mdio_smi_is_done(dev),
+				msecs_to_jiffies(100));
+		if (!orion_mdio_smi_is_done(dev))
+			return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -142,6 +173,21 @@ static int orion_mdio_reset(struct mii_bus *bus)
 	return 0;
 }
 
+static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
+{
+	struct orion_mdio_dev *dev = dev_id;
+
+	if (readl(dev->regs + MVMDIO_ERR_INT_CAUSE) &
+			MVMDIO_ERR_INT_SMI_DONE) {
+		writel(~MVMDIO_ERR_INT_SMI_DONE,
+				dev->regs + MVMDIO_ERR_INT_CAUSE);
+		wake_up(&dev->smi_busy_wait);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
 static int orion_mdio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -182,6 +228,19 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	dev->err_interrupt = NO_IRQ;
+	init_waitqueue_head(&dev->smi_busy_wait);
+
+	dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (dev->err_interrupt != NO_IRQ) {
+		ret = devm_request_irq(&pdev->dev, dev->err_interrupt,
+					orion_mdio_err_irq,
+					IRQF_SHARED, pdev->name, dev);
+		if (!ret)
+			writel(MVMDIO_ERR_INT_SMI_DONE,
+					dev->regs + MVMDIO_ERR_INT_MASK);
+	}
+
 	mutex_init(&dev->lock);
 
 	ret = of_mdiobus_register(bus, np);
-- 
1.7.10.4

[RFC PATCH 4/4] net: mvmdio: add getter and setter for PHY addresses

[Florian Fainelli]

This patch adds orion_mdio_{set,get}_phy_addr(.., port_number, phy_addr)
which is a feature available in this MDIO driver to monitor a particular
PHY address. This brings mvmdio one step closer to what is used in
mv643x_eth.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/net/ethernet/marvell/mvmdio.c |   29 +++++++++++++++++++++++++++++
 include/linux/marvell_mvmdio.h        |   10 ++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 include/linux/marvell_mvmdio.h

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index bdd9047..09e2470 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -32,7 +32,9 @@
 #include <linux/delay.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
+#include <linux/marvell_mvmdio.h>
 
+#define MVMDIO_PHY_ADDR			   0x0000
 #define MVMDIO_SMI_REG			   0x0004
 #define MVMDIO_SMI_DATA_SHIFT              0
 #define MVMDIO_SMI_PHY_ADDR_SHIFT          16
@@ -188,6 +190,33 @@ static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
+void orion_mdio_phy_addr_set(struct platform_device *pdev,
+				int port_num, int phy_addr)
+{
+	struct orion_mdio_dev *dev =
+		platform_get_drvdata(pdev);
+	int addr_shift = 5 * port_num;
+	u32 data;
+
+	data = readl(dev->regs + MVMDIO_PHY_ADDR);
+	data &= ~(0x1f << addr_shift);
+	data |= (phy_addr & 0x1f) << addr_shift;
+	writel(data, dev->regs + MVMDIO_PHY_ADDR);
+}
+EXPORT_SYMBOL(orion_mdio_phy_addr_set);
+
+int orion_mdio_phy_addr_get(struct platform_device *pdev, int port_num)
+{
+	struct orion_mdio_dev *dev =
+		platform_get_drvdata(pdev);
+	unsigned int data;
+
+	data = readl(dev->regs + MVMDIO_PHY_ADDR);
+
+	return (data >> (5 * port_num)) & 0x1f;
+}
+EXPORT_SYMBOL(orion_mdio_phy_addr_get);
+
 static int orion_mdio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
diff --git a/include/linux/marvell_mvmdio.h b/include/linux/marvell_mvmdio.h
new file mode 100644
index 0000000..c2dfec4
--- /dev/null
+++ b/include/linux/marvell_mvmdio.h
@@ -0,0 +1,10 @@
+#ifndef _MARVELL_MVMDIO_H
+#define _MARVELL_MVMDIO_H
+
+#include <linux/platform_device.h>
+
+void orion_mdio_phy_addr_set(struct platform_device *pdev,
+				int port_num, int phy_addr);
+int orion_mdio_phy_addr_get(struct platform_device *pdev, int port_num);
+
+#endif /* _MARVELL_MVMDIO_H */
-- 
1.7.10.4

[RFC PATCH 4/4] net: mvmdio: add getter and setter for PHY addresses

[Jason Gunthorpe]

On Fri, Jan 25, 2013 at 11:06:49AM +0100, Florian Fainelli wrote:
> This patch adds orion_mdio_{set,get}_phy_addr(.., port_number, phy_addr)
> which is a feature available in this MDIO driver to monitor a particular
> PHY address. This brings mvmdio one step closer to what is used in
> mv643x_eth.

This seems really strange.

Are you sure this should be part of the MDIO driver? Based on my docs,
this register is part of the ethernet controller HW, which
autonomously reads from the phy via MDIO.

It seems cleaner to set this register from the ethernet controller
based on the phy it is using and keep the MDIO driver purely for MDIO
bus access.

It is acceptable to overlap the device address ranges, start the
ethernet controller at 72000 and start the MDIO at 72004, the platform
bus code automatically nests them.

Jason

[RFC PATCH 4/4] net: mvmdio: add getter and setter for PHY addresses

[Florian Fainelli]

Le 25/01/2013 19:16, Jason Gunthorpe a ?crit :
> On Fri, Jan 25, 2013 at 11:06:49AM +0100, Florian Fainelli wrote:
>> This patch adds orion_mdio_{set,get}_phy_addr(.., port_number, phy_addr)
>> which is a feature available in this MDIO driver to monitor a particular
>> PHY address. This brings mvmdio one step closer to what is used in
>> mv643x_eth.
>
> This seems really strange.
>
> Are you sure this should be part of the MDIO driver? Based on my docs,
> this register is part of the ethernet controller HW, which
> autonomously reads from the phy via MDIO.

My reading of the datasheets for 88F628x and Armada 370 says that it 
actually belongs to the MDIO part of the controller (it is just below 
the SMI registers).

>
> It seems cleaner to set this register from the ethernet controller
> based on the phy it is using and keep the MDIO driver purely for MDIO
> bus access.
>
> It is acceptable to overlap the device address ranges, start the
> ethernet controller at 72000 and start the MDIO at 72004, the platform
> bus code automatically nests them.

Ok I did not think this would work, but I kind of prefer that too.
--
Florian