Date:   Tue, 28 Dec 2021 11:46:18 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Daniel Golle <daniel@makrotopia.org>, Andrew Lunn <andrew@lunn.ch>
Cc:     linux-mediatek@lists.infradead.org, netdev@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Felix Fietkau <nbd@nbd.name>, John Crispin <john@phrozen.org>,
        Sean Wang <sean.wang@mediatek.com>,
        Mark Lee <Mark-MC.Lee@mediatek.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>
Subject: Re: [PATCH v7 2/2] net: ethernet: mtk_eth_soc: implement Clause 45
 MDIO access
Message-ID: <Ycr5Cna76eg2B0An@shell.armlinux.org.uk>
References: <YcnoAscVe+2YILT8@shell.armlinux.org.uk>
 <YcpVmlb1jFavCBpS@makrotopia.org>
 <YcpVtjykiS7mgtT5@makrotopia.org>
 <YcpkXclZRXLC3XfM@makrotopia.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YcpkXclZRXLC3XfM@makrotopia.org>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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)
 {
-- 
2.30.2

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!