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
Date: Tue, 28 Dec 2021 11:46:18 +0000 [thread overview]
Message-ID: <Ycr5Cna76eg2B0An@shell.armlinux.org.uk> (raw)
In-Reply-To: <YcpkXclZRXLC3XfM@makrotopia.org>
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!
next prev parent reply other threads:[~2021-12-28 11:46 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-26 21:29 [PATCH] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-26 21:51 ` Andrew Lunn
2021-12-26 21:57 ` Russell King (Oracle)
2021-12-26 22:26 ` Andrew Lunn
2021-12-26 23:23 ` [PATCH v2] " Daniel Golle
2021-12-27 16:09 ` [PATCH v3] " Daniel Golle
2021-12-27 16:21 ` Russell King (Oracle)
2021-12-27 17:51 ` [PATCH v4 0/2] net: mtk_soc_eth: " Daniel Golle
2021-12-27 17:51 ` [PATCH v4 1/2] net: mtk_eth_soc: fix return value of MDIO operations Daniel Golle
2021-12-27 17:52 ` [PATCH v4 2/2] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-27 18:30 ` [PATCH v5 0/2] " Daniel Golle
2021-12-27 18:31 ` [PATCH v5 1/2] net: ethernet: mtk_eth_soc: fix return value of MDIO ops Daniel Golle
2021-12-27 18:31 ` [PATCH v5 2/2] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-28 0:07 ` [PATCH v6 0/2] " Daniel Golle
2021-12-28 0:08 ` [PATCH v6 1/2] net: ethernet: mtk_eth_soc: fix return value of MDIO ops Daniel Golle
2021-12-28 0:09 ` [PATCH v6 2/2] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-28 1:10 ` [PATCH v7 0/2] net: ethernet: mtk_soc_eth: " Daniel Golle
2021-12-28 1:43 ` Jakub Kicinski
2021-12-28 1:10 ` [PATCH v7 1/2] net: ethernet: mtk_eth_soc: fix return value of MDIO ops Daniel Golle
2021-12-28 1:11 ` [PATCH v7 2/2] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-28 11:46 ` Russell King (Oracle) [this message]
2021-12-28 21:03 ` [PATCH v8 0/3] net: ethernet: mtk_eth_soc: refactoring and Clause 45 Daniel Golle
2021-12-28 21:05 ` [PATCH v8 1/3] net: mdio: add helpers to extract clause 45 regad and devad fields Daniel Golle
2021-12-28 21:05 ` [PATCH v8 2/3] net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops Daniel Golle
2021-12-28 21:05 ` [PATCH v8 3/3] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-31 20:52 ` [PATCH v9 0/3] net: ethernet: mtk_eth_soc: refactoring and Clause 45 Daniel Golle
2021-12-31 20:54 ` [PATCH v9 2/3] net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops Daniel Golle
2021-12-31 20:54 ` [PATCH v9 3/3] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
2021-12-31 20:57 ` [PATCH v9 1/3] net: mdio: add helpers to extract clause 45 regad and devad fields Daniel Golle
2022-01-01 17:20 ` Andrew Lunn
2022-01-02 15:47 ` [PATCH v10 0/3] net: ethernet: mtk_eth_soc: refactoring and Clause 45 Daniel Golle
2022-01-02 15:51 ` Russell King (Oracle)
2022-01-02 15:48 ` [PATCH v10 1/3] net: ethernet: mtk_eth_soc: fix return value and refactor MDIO ops Daniel Golle
2022-01-02 16:43 ` Andrew Lunn
2022-01-02 15:48 ` [PATCH v10 2/3] net: mdio: add helpers to extract clause 45 regad and devad fields Daniel Golle
2022-01-02 16:44 ` Andrew Lunn
2022-01-02 15:49 ` [PATCH v10 3/3] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access Daniel Golle
[not found] ` <Yc9tk6IZ0ldqHx4Y@makrotopia.org>
2022-01-01 17:18 ` [PATCH v9 1/3] net: mdio: add helpers to extract clause 45 regad and devad fields Andrew Lunn
2022-01-01 17:41 ` Daniel Golle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ycr5Cna76eg2B0An@shell.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=Mark-MC.Lee@mediatek.com \
--cc=andrew@lunn.ch \
--cc=daniel@makrotopia.org \
--cc=davem@davemloft.net \
--cc=john@phrozen.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=nbd@nbd.name \
--cc=netdev@vger.kernel.org \
--cc=sean.wang@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).