| Message ID | YcpkXclZRXLC3XfM@makrotopia.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
On Tue, Dec 28, 2021 at 01:11:57AM +0000, Daniel Golle wrote: > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > index 5ef70dd8b49c6..b73d8adc9d24c 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > @@ -341,9 +341,12 @@ > /* PHY Indirect Access Control registers */ > #define MTK_PHY_IAC 0x10004 > #define PHY_IAC_ACCESS BIT(31) > +#define PHY_IAC_SET_ADDR 0 > #define PHY_IAC_READ BIT(19) > +#define PHY_IAC_READ_C45 (BIT(18) | BIT(19)) > #define PHY_IAC_WRITE BIT(18) 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)" <rmk+kernel@armlinux.org.uk> 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) <rmk+kernel@armlinux.org.uk> --- include/linux/mdio.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 5c6676d3de23..37f98fbdee49 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -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 @@ -14,6 +15,7 @@ */ #define MII_ADDR_C45 (1<<30) #define MII_DEVADDR_C45_SHIFT 16 +#define MII_DEVADDR_C45_MASK GENMASK(20, 16) #define MII_REGADDR_C45_MASK GENMASK(15, 0) struct gpio_desc; @@ -385,6 +387,16 @@ static inline u32 mdiobus_c45_addr(int devad, u16 regnum) return MII_ADDR_C45 | devad << MII_DEVADDR_C45_SHIFT | regnum; } +static inline u16 mdiobus_c45_regad(u32 regnum) +{ + return FIELD_GET(MII_REGADDR_C45_MASK, regnum); +} + +static inline u16 mdiobus_c45_devad(u32 regnum) +{ + return FIELD_GET(MII_DEVADDR_C45_MASK, regnum); +} + static inline int __mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad, u16 regnum) {
On Fri, Dec 31, 2021 at 08:52:35PM +0000, Russell King (Oracle) wrote: > 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) <rmk+kernel@armlinux.org.uk> This email has me confused. It seems to be coming directly from Russell, but is part of a patchset? Then there is a second 1/3 later in the patchset? Andrew
On Sat, Jan 01, 2022 at 06:18:54PM +0100, Andrew Lunn wrote: > On Fri, Dec 31, 2021 at 08:52:35PM +0000, Russell King (Oracle) wrote: > > 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) <rmk+kernel@armlinux.org.uk> > > This email has me confused. It seems to be coming directly from > Russell, but is part of a patchset? Then there is a second 1/3 later > in the patchset? That happened by accident when using git-send-email and hence the first time it was sent with Russell's From: in the RFC822 headers it was (rightly) rejected. I noticed that shortly after and resend the patch with my sender address set as From: in the RFC822 headers and Russell's From: in the mail body for only git, and not mailservers, to care about. Sorry for that additional noise.
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 72b3ae7b5ff8d..ed1820ba9e6ea 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -102,10 +102,30 @@ static int _mtk_mdio_write(struct mtk_eth *eth, u32 phy_addr, u32 phy_reg, write_data &= 0xffff; - mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START | PHY_IAC_WRITE | - (phy_reg << PHY_IAC_REG_SHIFT) | - (phy_addr << PHY_IAC_ADDR_SHIFT) | write_data, - MTK_PHY_IAC); + if (phy_reg & MII_ADDR_C45) { + u8 dev_num = (phy_reg >> MII_DEVADDR_C45_SHIFT) & GENMASK(4, 0); + + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START_C45 | PHY_IAC_SET_ADDR | + (dev_num << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT) | + (phy_reg & MII_REGADDR_C45_MASK), + MTK_PHY_IAC); + + if (mtk_mdio_busy_wait(eth)) + return -EBUSY; + + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START_C45 | PHY_IAC_WRITE | + (dev_num << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT) | + write_data, + MTK_PHY_IAC); + } else { + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START | PHY_IAC_WRITE | + (phy_reg << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT) | + write_data, + MTK_PHY_IAC); + } if (mtk_mdio_busy_wait(eth)) return -EBUSY; @@ -118,10 +138,28 @@ static int _mtk_mdio_read(struct mtk_eth *eth, u32 phy_addr, u32 phy_reg) if (mtk_mdio_busy_wait(eth)) return -EBUSY; - mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START | PHY_IAC_READ | - (phy_reg << PHY_IAC_REG_SHIFT) | - (phy_addr << PHY_IAC_ADDR_SHIFT), - MTK_PHY_IAC); + if (phy_reg & MII_ADDR_C45) { + u8 dev_num = (phy_reg >> MII_DEVADDR_C45_SHIFT) & GENMASK(4, 0); + + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START_C45 | PHY_IAC_SET_ADDR | + (dev_num << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT) | + (phy_reg & MII_REGADDR_C45_MASK), + MTK_PHY_IAC); + + if (mtk_mdio_busy_wait(eth)) + return -EBUSY; + + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START_C45 | PHY_IAC_READ_C45 | + (dev_num << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT), + MTK_PHY_IAC); + } else { + mtk_w32(eth, PHY_IAC_ACCESS | PHY_IAC_START | PHY_IAC_READ | + (phy_reg << PHY_IAC_REG_SHIFT) | + (phy_addr << PHY_IAC_ADDR_SHIFT), + MTK_PHY_IAC); + } if (mtk_mdio_busy_wait(eth)) return -EBUSY; @@ -493,6 +531,7 @@ static int mtk_mdio_init(struct mtk_eth *eth) eth->mii_bus->name = "mdio"; eth->mii_bus->read = mtk_mdio_read; eth->mii_bus->write = mtk_mdio_write; + eth->mii_bus->probe_capabilities = MDIOBUS_C22_C45; eth->mii_bus->priv = eth; eth->mii_bus->parent = eth->dev; diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h index 5ef70dd8b49c6..b73d8adc9d24c 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h @@ -341,9 +341,12 @@ /* PHY Indirect Access Control registers */ #define MTK_PHY_IAC 0x10004 #define PHY_IAC_ACCESS BIT(31) +#define PHY_IAC_SET_ADDR 0 #define PHY_IAC_READ BIT(19) +#define PHY_IAC_READ_C45 (BIT(18) | BIT(19)) #define PHY_IAC_WRITE BIT(18) #define PHY_IAC_START BIT(16) +#define PHY_IAC_START_C45 0 #define PHY_IAC_ADDR_SHIFT 20 #define PHY_IAC_REG_SHIFT 25 #define PHY_IAC_TIMEOUT HZ
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(-)