From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:00
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-4 and 9 are cosmetic cleanups.
Patches 5-7 are prerequisites to the xSMI support.
Patches 8 and 10-11 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 11 should go through the mvebu tree as asked by Gregory,
thanks!
Thanks,
Antoine
Since v3:
- Added a patch from Russell King removing locks, as there is already
per bus locking in the MDIO layer.
- Russell suggested another approach to add the xSMI support, by having
two different read/write functions. Reworked the series to take this
into account. (This also lead to the removal of some patches, and the
introduction of some others).
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 (10):
net: mvmdio: reorder headers alphabetically
net: mvmdio: use tabs for defines
net: mvmdio: use GENMASK for masks
net: mvmdio: introduce an ops structure
net: mvmdio: put the poll intervals in the ops structure
net: mvmdio: check the MII_ADDR_C45 bit is not set for smi operations
net: mvmdio: add xmdio xsmi support
net: mvmdio: simplify the smi read and write error paths
dt-bindings: orion-mdio: document the new xmdio compatible
arm64: marvell: dts: add xmdio nodes for 7k/8k
Russell King (1):
net: mvmdio: remove duplicate locking
.../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/mvmdio.c | 214 +++++++++++++++------
4 files changed, 178 insertions(+), 60 deletions(-)
--
2.9.4
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:04
From: Russell King <redacted>
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 <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 10 ----------
1 file changed, 10 deletions(-)
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:05
Introduce an ops structure to add an indirection on the is_done
function, as this is needed to add the xMDIO support later.
Signed-off-by: Antoine Tenart <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
@@ -62,14 +62,14 @@ struct orion_mdio_dev {wait_queue_head_tsmi_busy_wait;};-staticintorion_mdio_smi_is_done(structorion_mdio_dev*dev)-{-return!(readl(dev->regs)&MVMDIO_SMI_BUSY);-}+structorion_mdio_ops{+int(*is_done)(structorion_mdio_dev*);+};/* Wait for the SMI unit to be ready for another operation*/-staticintorion_mdio_wait_ready(structmii_bus*bus)+staticintorion_mdio_wait_ready(conststructorion_mdio_ops*ops,+structmii_bus*bus){structorion_mdio_dev*dev=bus->priv;unsignedlongtimeout=usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
@@ -77,7 +77,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)inttimedout=0;while(1){-if(orion_mdio_smi_is_done(dev))+if(ops->is_done(dev))return0;elseif(timedout)break;
@@ -96,8 +96,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;}
@@ -107,6 +106,15 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)return-ETIMEDOUT;}+staticintorion_mdio_smi_is_done(structorion_mdio_dev*dev)+{+return!(readl(dev->regs)&MVMDIO_SMI_BUSY);+}++staticconststructorion_mdio_opsorion_mdio_smi_ops={+.is_done=orion_mdio_smi_is_done,+};+staticintorion_mdio_read(structmii_bus*bus,intmii_id,intregnum){
@@ -114,7 +122,7 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,u32val;intret;-ret=orion_mdio_wait_ready(bus);+ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)gotoout;
@@ -123,7 +131,7 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,MVMDIO_SMI_READ_OPERATION),dev->regs);-ret=orion_mdio_wait_ready(bus);+ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)gotoout;
@@ -145,7 +153,7 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,structorion_mdio_dev*dev=bus->priv;intret;-ret=orion_mdio_wait_ready(bus);+ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)gotoout;
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:06
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 <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
@@ -64,6 +64,8 @@ struct orion_mdio_dev {structorion_mdio_ops{int(*is_done)(structorion_mdio_dev*);+unsignedintpoll_interval_min;+unsignedintpoll_interval_max;};/* Wait for the SMI unit to be ready for another operation
@@ -83,8 +85,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;
@@ -113,6 +115,8 @@ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)staticconststructorion_mdio_opsorion_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,};staticintorion_mdio_read(structmii_bus*bus,intmii_id,
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:07
Add a check for the read and write smi operations, to ensure the
MII_ADDR_C45 bit isn't set. This will be needed as soon as the xSMI
support is added to the mvmdio driver.
Signed-off-by: Antoine Tenart <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:08
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 <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 112 +++++++++++++++++++++++++++++++---
1 file changed, 105 insertions(+), 7 deletions(-)
@@ -154,8 +172,8 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id,returnret;}-staticintorion_mdio_write(structmii_bus*bus,intmii_id,-intregnum,u16value)+staticintorion_mdio_smi_write(structmii_bus*bus,intmii_id,+intregnum,u16value){structorion_mdio_dev*dev=bus->priv;intret;
@@ -177,6 +195,73 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id,returnret;}+staticintorion_mdio_xsmi_is_done(structorion_mdio_dev*dev)+{+return!(readl(dev->regs+MVMDIO_XSMI_MGNT_REG)&MVMDIO_XSMI_BUSY);+}++staticconststructorion_mdio_opsorion_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,+};++staticintorion_mdio_xsmi_read(structmii_bus*bus,intmii_id,+intregnum)+{+structorion_mdio_dev*dev=bus->priv;+u16dev_addr=(regnum>>16)&GENMASK(4,0);+intret;++if(!(regnum&MII_ADDR_C45))+return-EOPNOTSUPP;++ret=orion_mdio_wait_ready(&orion_mdio_xsmi_ops,bus);+if(ret<0)+returnret;++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)+returnret;++if(!(readl(dev->regs+MVMDIO_XSMI_MGNT_REG)&+MVMDIO_XSMI_READ_VALID)){+dev_err(bus->parent,"XSMI bus read not valid\n");+return-ENODEV;+}++returnreadl(dev->regs+MVMDIO_XSMI_MGNT_REG)&GENMASK(15,0);+}++staticintorion_mdio_xsmi_write(structmii_bus*bus,intmii_id,+intregnum,u16value)+{+structorion_mdio_dev*dev=bus->priv;+u16dev_addr=(regnum>>16)&GENMASK(4,0);+intret;++if(!(regnum&MII_ADDR_C45))+return-EOPNOTSUPP;++ret=orion_mdio_wait_ready(&orion_mdio_xsmi_ops,bus);+if(ret<0)+returnret;++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);++return0;+}+staticirqreturn_torion_mdio_err_irq(intirq,void*dev_id){structorion_mdio_dev*dev=dev_id;
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:09
Cosmetic patch simplifying the smi read and write error paths. It also
align their error paths with the ones of the xsmi functions.
Signed-off-by: Antoine Tenart <redacted>
---
drivers/net/ethernet/marvell/mvmdio.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
@@ -149,7 +149,7 @@ static int orion_mdio_smi_read(struct mii_bus *bus, int mii_id,ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)-gotoout;+returnret;writel(((mii_id<<MVMDIO_SMI_PHY_ADDR_SHIFT)|(regnum<<MVMDIO_SMI_PHY_REG_SHIFT)|
@@ -158,18 +158,15 @@ static int orion_mdio_smi_read(struct mii_bus *bus, int mii_id,ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)-gotoout;+returnret;val=readl(dev->regs);if(!(val&MVMDIO_SMI_READ_VALID)){dev_err(bus->parent,"SMI bus read not valid\n");-ret=-ENODEV;-gotoout;+return-ENODEV;}-ret=val&GENMASK(15,0);-out:-returnret;+returnval&GENMASK(15,0);}staticintorion_mdio_smi_write(structmii_bus*bus,intmii_id,
@@ -183,7 +180,7 @@ static int orion_mdio_smi_write(struct mii_bus *bus, int mii_id,ret=orion_mdio_wait_ready(&orion_mdio_smi_ops,bus);if(ret<0)-gotoout;+returnret;writel(((mii_id<<MVMDIO_SMI_PHY_ADDR_SHIFT)|(regnum<<MVMDIO_SMI_PHY_REG_SHIFT)|
@@ -191,8 +188,7 @@ static int orion_mdio_smi_write(struct mii_bus *bus, int mii_id,(value<<MVMDIO_SMI_DATA_SHIFT)),dev->regs);-out:-returnret;+return0;}staticintorion_mdio_xsmi_is_done(structorion_mdio_dev*dev)
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:10
A new compatible for Marvell xMDIO interfaces was added into the Marvell
MDIO driver. Document this new compatible.
Signed-off-by: Antoine Tenart <redacted>
---
Documentation/devicetree/bindings/net/marvell-orion-mdio.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -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
From: Antoine Tenart <hidden> Date: 2017-06-14 15:49:11
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 <redacted>
---
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(+)
On Wed, Jun 14, 2017 at 05:49:10PM +0200, Antoine Tenart wrote:
quoted hunk
A new compatible for Marvell xMDIO interfaces was added into the Marvell
MDIO driver. Document this new compatible.
Signed-off-by: Antoine Tenart <redacted>
---
Documentation/devicetree/bindings/net/marvell-orion-mdio.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -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.
Hi Antoine
This patches is looking good now. One nit. The description above
suggests Kirkwood, Dove, Orion5x, MV78xx0, Armada 370 and Armada XP
all have xmdio. Maybe better wording would be:
The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
identical unit that provides an interface with the MDIO bus.
Additionally, Armada 7k and Armada 8k has a second unit which
provides an interface with the xMDIO bus. This driver handles
these interfaces.
Andrew
From: Russell King <redacted>
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 <redacted>
Introduce an ops structure to add an indirection on the is_done
function, as this is needed to add the xMDIO support later.
Signed-off-by: Antoine Tenart <redacted>
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 <redacted>
Add a check for the read and write smi operations, to ensure the
MII_ADDR_C45 bit isn't set. This will be needed as soon as the xSMI
support is added to the mvmdio driver.
Signed-off-by: Antoine Tenart <redacted>
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 <redacted>
Cosmetic patch simplifying the smi read and write error paths. It also
align their error paths with the ones of the xsmi functions.
Signed-off-by: Antoine Tenart <redacted>
From: Antoine Tenart <hidden> Date: 2017-06-15 06:55:26
Hi Andrew,
On Wed, Jun 14, 2017 at 06:35:37PM +0200, Andrew Lunn wrote:
On Wed, Jun 14, 2017 at 05:49:10PM +0200, Antoine Tenart wrote:
quoted
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.
This patches is looking good now. One nit. The description above
suggests Kirkwood, Dove, Orion5x, MV78xx0, Armada 370 and Armada XP
all have xmdio. Maybe better wording would be:
The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
identical unit that provides an interface with the MDIO bus.
Additionally, Armada 7k and Armada 8k has a second unit which
provides an interface with the xMDIO bus. This driver handles
these interfaces.
From: David Laight <hidden> Date: 2017-06-19 11:57:51
From: Antoine Tenart
Sent: 14 June 2017 16:49
Cosmetic patch to use the GENMASK helper for masks.
...
- ret = val & 0xFFFF;
+ ret = val & GENMASK(15, 0);
My 2c: It isn't at all clear to me that changes like this in anyway
improve the code readability.
In some sense the '15' should be a named constant - but that just makes
it even less obvious what is going on.
David
From: David Laight <redacted>
Date: Mon, 19 Jun 2017 11:57:51 +0000
From: Antoine Tenart
quoted
Sent: 14 June 2017 16:49
Cosmetic patch to use the GENMASK helper for masks.
...
quoted
- ret = val & 0xFFFF;
+ ret = val & GENMASK(15, 0);
My 2c: It isn't at all clear to me that changes like this in anyway
improve the code readability.
In some sense the '15' should be a named constant - but that just makes
it even less obvious what is going on.
I agree, a hexidecimal mask of 0xffff is 100 times more readable and
understandable to me than "SOME_MACRO(x, y)".