lkml.org 
[lkml]   [2021]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH v7 2/2] net: ethernet: mtk_eth_soc: implement Clause 45 MDIO access
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!

\
 
 \ /
  Last update: 2021-12-28 12:47    [W:0.518 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site