From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-28 01:12:09
Implement read and write access to IEEE 802.3 Clause 45 Ethernet
phy registers.
Tested on the Ubiquiti UniFi 6 LR access point featuring
MediaTek MT7622BV WiSoC with Aquantia AQR112C.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
---
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 55 ++++++++++++++++++---
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 ++
2 files changed, 50 insertions(+), 8 deletions(-)
You probably want at some point to clean the "operation code" (OP)
definition up here. IEEE 802.3 defines this as a two bit field:
OP Clause 22 Clause 45
00 Undefined Address
01 Write Write
10 Read Post-read-increment-address
11 Undefined Read
What I'm getting at is, this isn't a couple of independent bits, and my
feeling is that such fields should not be defined using BIT() unless
they truely are independent bits.
Maybe:
#define PHY_IAC_OP(x) ((x) << 18)
#define PHY_IAC_OP_C45_ADDR PHY_IAC_OP(0)
#define PHY_IAC_OP_WRITE PHY_IAC_OP(1)
#define PHY_IAC_OP_C22_READ PHY_IAC_OP(2)
#define PHY_IAC_OP_C45_READ PHY_IAC_OP(3)
Or if you prefer GENMASK / FIELD_FIT way of doing things:
#define PHY_IAC_OP_MASK GENMASK(17, 16)
#define PHY_IAC_OP_C45_ADDR FIELD_FIT(PHY_IAC_OP_MASK, 0)
#define PHY_IAC_OP_WRITE FIELD_FIT(PHY_IAC_OP_MASK, 1)
#define PHY_IAC_OP_C22_READ FIELD_FIT(PHY_IAC_OP_MASK, 2)
#define PHY_IAC_OP_C45_READ FIELD_FIT(PHY_IAC_OP_MASK, 3)
I'm also wondering about the PHY_IAC_START* definitions. IEEE 802.3
gives us:
ST
00 Clause 45
01 Clause 22
I'm wondering whether bit 17 is part of the start field in your device,
making bit 16 and 17 a two-bit field that defines the ST bits sent on
the bus.
Also, as I mentioned previously, I would like to see helpers to extract
the devad and regad fields, so here's a patch that adds a couple of
helpers (but is completely untested!) If you update your patches to use
this (please wait a bit longer before doing so in case there's other
comments) you will need to include my patch along with your other two.
Thanks.
8<====
From: "Russell King (Oracle)" <redacted>
Subject: [PATCH] net: mdio: add helpers to extract clause 45 regad and devad
fields
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Signed-off-by: Russell King (Oracle) <redacted>
---
include/linux/mdio.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -7,6 +7,7 @@#define __LINUX_MDIO_H__#include<uapi/linux/mdio.h>+#include<linux/bitfield.h>#include<linux/mod_devicetable.h>/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-28 21:04:08
As it turned out some clean-up would be needed, first address return
value and type of mdio read and write functions in mtk_eth_soc and
generally clean up and unify both functions.
Also refactor IAC register definitions to use bitfield helper macros
to get rid of constants previously used as mask or shift values.
Then add support to access Clause 45 phy registers, using the newly
introcuced helper macros suggested by Russell King to access the
Clause 45 device and register address encoded in the 32-bit parameter.
This series is tested on MediaTek MT7622AV based Bananapi BPi-R64 board
having MediaTek MT7531BE DSA gigE switch using Clause 22 MDIO and
MediaTek MT7622BV based Ubiquiti UniFi 6 LR access point having
Aquantia AQR112C PHY using Clause 45 MDIO.
v8: add patch from Russel King, switch to bitfield helper macros
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
Daniel Golle (2):
net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops
net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access
Russell King (Oracle) (1):
net: mdio: add helpers to extract clause 45 regad and devad fields
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 73 ++++++++++++++-------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 19 ++++--
include/linux/mdio.h | 12 ++++
3 files changed, 77 insertions(+), 27 deletions(-)
--
2.34.1
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-28 21:05:14
From: "Russell King (Oracle)" <redacted>
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Tested-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Russell King (Oracle) <redacted>
---
v8: First inclusing upon comment on mailing list
include/linux/mdio.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -7,6 +7,7 @@#define __LINUX_MDIO_H__#include<uapi/linux/mdio.h>+#include<linux/bitfield.h>#include<linux/mod_devicetable.h>/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-28 21:05:41
Instead of returning -1 (-EPERM) when MDIO bus is stuck busy
while writing or 0xffff if it happens while reading, return the
appropriate -EBUSY. Also fix return type to int instead of u32.
Refactor functions to use bitfield helpers instead of having various
masking and shifting constants in the code.
Fixes: 656e705243fd0 ("net-next: mediatek: add support for MT7623 ethernet")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v8: switch to bitfield helper macros
v7: remove unneeded variables
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 35 +++++++++------------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 16 +++++++---
2 files changed, 26 insertions(+), 25 deletions(-)
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-28 21:06:07
Implement read and write access to IEEE 802.3 Clause 45 Ethernet
phy registers while making use of new mdiobus_c45_regad and
mdiobus_c45_devad helpers.
Improve readability by using bitfield helper macros which makes the
register definitions in the header file resemble the exact values
stated in the Reference Manual.
Tested on the Ubiquiti UniFi 6 LR access point featuring
MediaTek MT7622BV WiSoC with Aquantia AQR112C.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v8: switch to bitfield helper macros, incl. newly introduced ones
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 52 +++++++++++++++++----
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 ++
2 files changed, 46 insertions(+), 9 deletions(-)
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-31 20:52:32
Rework value and type of mdio read and write functions in mtk_eth_soc
and generally clean up and unify both functions.
Then add support to access Clause 45 phy registers.
All three commits are tested on the Bananapi BPi-R64 board having
MediaTek MT7531BE DSA gigE switch using clause 22 MDIO and
Ubiquiti UniFi 6 LR access point having Aquantia AQR112C PHY using
clause 45 MDIO.
v9: improved formatting and Cc missing maintainer
v8: add patch from Russel King, switch to bitfield helper macros
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
Daniel Golle (2):
net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops
net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access
Russell King (Oracle) (1):
net: mdio: add helpers to extract clause 45 regad and devad fields
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 85 +++++++++++++++------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 19 +++--
include/linux/mdio.h | 12 +++
3 files changed, 89 insertions(+), 27 deletions(-)
--
2.34.1
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-31 20:54:20
Instead of returning -1 (-EPERM) when MDIO bus is stuck busy
while writing or 0xffff if it happens while reading, return the
appropriate -EBUSY. Also fix return type to int instead of u32.
Refactor functions to use bitfield helpers instead of having various
masking and shifting constants in the code.
This allows the register definitions in the header file to closely
resemble the values stated in the Reference Manual.
Fixes: 656e705243fd0 ("net-next: mediatek: add support for MT7623 ethernet")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v9: improved formatting and Cc missing maintainer
v8: switch to bitfield helper macros
v7: remove unneeded variables
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 39 ++++++++++-----------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 16 ++++++---
2 files changed, 30 insertions(+), 25 deletions(-)
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-31 20:54:58
Implement read and write access to IEEE 802.3 Clause 45 Ethernet
phy registers while making use of new mdiobus_c45_regad and
mdiobus_c45_devad helpers.
Tested on the Ubiquiti UniFi 6 LR access point featuring
MediaTek MT7622BV WiSoC with Aquantia AQR112C.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v9: improved formatting and Cc missing maintainer
v8: switch to bitfield helper macros, incl. newly introduced ones
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 68 +++++++++++++++++----
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 +
2 files changed, 58 insertions(+), 13 deletions(-)
From: Daniel Golle <daniel@makrotopia.org> Date: 2021-12-31 20:58:00
From: "Russell King (Oracle)" <redacted>
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Tested-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Russell King (Oracle) <redacted>
---
v9: unchanged
v8: First inclusing upon comment on mailing list
include/linux/mdio.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -7,6 +7,7 @@#define __LINUX_MDIO_H__#include<uapi/linux/mdio.h>+#include<linux/bitfield.h>#include<linux/mod_devicetable.h>/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
From: Andrew Lunn <andrew@lunn.ch> Date: 2022-01-01 17:20:46
On Fri, Dec 31, 2021 at 08:57:43PM +0000, Daniel Golle wrote:
From: "Russell King (Oracle)" <redacted>
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Tested-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Russell King (Oracle) <redacted>
Hi Daniel
Since you are submitted this, your Signed-off-by: needs to be last.
Andrew
From: Daniel Golle <daniel@makrotopia.org> Date: 2022-01-02 15:49:02
Rework value and type of mdio read and write functions in mtk_eth_soc
and generally clean up and unify both functions.
Then add support to access Clause 45 phy registers, using newly
introduced helper macros added by a patch Russell King has suggested
in a reply to an earlier version of this series [1].
All three commits are tested on the Bananapi BPi-R64 board having
MediaTek MT7531BE DSA gigE switch using clause 22 MDIO and
Ubiquiti UniFi 6 LR access point having Aquantia AQR112C PHY using
clause 45 MDIO.
[1]: https://lore.kernel.org/netdev/Ycr5Cna76eg2B0An@shell.armlinux.org.uk/
v10: correct order of SoB lines in 2/3, change patch order in series
v9: improved formatting and Cc missing maintainer
v8: add patch from Russel King, switch to bitfield helper macros
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
Daniel Golle (2):
net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops
net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access
Russell King (Oracle) (1):
net: mdio: add helpers to extract clause 45 regad and devad fields
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 85 +++++++++++++++------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 19 +++--
include/linux/mdio.h | 12 +++
3 files changed, 89 insertions(+), 27 deletions(-)
--
2.34.1
From: Daniel Golle <daniel@makrotopia.org> Date: 2022-01-02 15:49:03
Instead of returning -1 (-EPERM) when MDIO bus is stuck busy
while writing or 0xffff if it happens while reading, return the
appropriate -EBUSY. Also fix return type to int instead of u32.
Refactor functions to use bitfield helpers instead of having various
masking and shifting constants in the code.
Fixes: 656e705243fd0 ("net-next: mediatek: add support for MT7623 ethernet")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v10: unchanged
v9: improved formatting and Cc missing maintainer
v8: switch to bitfield helper macros
v7: remove unneeded variables
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 39 ++++++++++-----------
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 16 ++++++---
2 files changed, 30 insertions(+), 25 deletions(-)
From: Daniel Golle <daniel@makrotopia.org> Date: 2022-01-02 15:49:03
From: "Russell King (Oracle)" <redacted>
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Tested-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Russell King (Oracle) <redacted>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v10: correct order of SoB lines
v9: unchanged
v8: First inclusing upon comment on mailing list
include/linux/mdio.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -7,6 +7,7 @@#define __LINUX_MDIO_H__#include<uapi/linux/mdio.h>+#include<linux/bitfield.h>#include<linux/mod_devicetable.h>/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
From: Daniel Golle <daniel@makrotopia.org> Date: 2022-01-02 15:50:28
Implement read and write access to IEEE 802.3 Clause 45 Ethernet
phy registers while making use of new mdiobus_c45_regad and
mdiobus_c45_devad helpers.
Improve readability by using bitfield helper macros which makes the
register definitions in the header file resemble the exact values
stated in the Reference Manual.
Tested on the Ubiquiti UniFi 6 LR access point featuring
MediaTek MT7622BV WiSoC with Aquantia AQR112C.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v10: unchanged
v9: improved formatting and Cc missing maintainer
v8: switch to bitfield helper macros, incl. newly introduced ones
v7: remove unneeded variables and order OR-ed call parameters
v6: further clean up functions and more cleanly separate patches
v5: fix wrong variable name in first patch covered by follow-up patch
v4: clean-up return values and types, split into two commits
v3: return -1 instead of 0xffff on error in _mtk_mdio_write
v2: use MII_DEVADDR_C45_SHIFT and MII_REGADDR_C45_MASK to extract
device id and register address. Unify read and write functions to
have identical types and parameter names where possible as we are
anyway already replacing both function bodies.
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 68 +++++++++++++++++----
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 +
2 files changed, 58 insertions(+), 13 deletions(-)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2022-01-02 15:51:32
On Sun, Jan 02, 2022 at 03:47:52PM +0000, Daniel Golle wrote:
Rework value and type of mdio read and write functions in mtk_eth_soc
and generally clean up and unify both functions.
Then add support to access Clause 45 phy registers, using newly
introduced helper macros added by a patch Russell King has suggested
in a reply to an earlier version of this series [1].
Can you please stop threading each re-post to the previous posting of
the series. It's getting rather annoying after ten iterations to have
the subject lines disappearing off the right hand side of the field.
This is not a request to re-post, it is a request for the next posting.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
-ETIMEDOUT would be more normal.
I would probably also change mtk_mdio_busy_wait() so that it either
returned 0, or -ETIMEDOUT. That is the general pattern in Linux,
return 0 on success, or a negative error code. Returning -1 is an
invitation for trouble.
The code would then become
ret = mtk_mdio_busy_wait(eth);
if (ret < 0)
return ret;
which is a very common pattern in Linux.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2022-01-02 16:44:46
On Sun, Jan 02, 2022 at 03:48:50PM +0000, Daniel Golle wrote:
From: "Russell King (Oracle)" <redacted>
Add a couple of helpers and definitions to extract the clause 45 regad
and devad fields from the regnum passed into MDIO drivers.
Tested-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Russell King (Oracle) <redacted>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>