Re: [PATCH] net: phy: add qca8081 ethernet phy driver

From: Jie Luo
Date: Tue Aug 17 2021 - 09:11:04 EST



On 8/16/2021 9:56 PM, Andrew Lunn wrote:
On Mon, Aug 16, 2021 at 07:34:40PM +0800, Luo Jie wrote:
qca8081 is industry’s lowest cost and power 1-port 2.5G/1G Ethernet PHY
chip, which implements SGMII/SGMII+ for interface to SoC.
Hi Luo

No Marketing claims in the commit message please. Even if it is
correct now, it will soon be wrong with newer generations of
devices.

And what is SGMII+? Please reference a document. Is it actually
2500BaseX?

Hi Andrew,

thanks for the comments, will remove the claims in the next patch.

SGMII+ is for 2500BaseX, which is same as SGMII, but the clock frequency of SGMII+ is 2.5 times SGMII.

Signed-off-by: Luo Jie <luoj@xxxxxxxxxxxxxx>
---
drivers/net/phy/Kconfig | 6 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/qca808x.c | 573 ++++++++++++++++++++++++++++++++++++++
3 files changed, 580 insertions(+)
create mode 100644 drivers/net/phy/qca808x.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index c56f703ae998..26cb1c2ffd17 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -343,3 +343,9 @@ endif # PHYLIB
config MICREL_KS8995MA
tristate "Micrel KS8995MA 5-ports 10/100 managed Ethernet switch"
depends on SPI
+
+config QCA808X_PHY
+ tristate "Qualcomm Atheros QCA808X PHYs"
+ depends on REGULATOR
+ help
+ Currently supports the QCA8081 model
This file is sorted on the tristate text. So it should appear directly
after AT803X_PHY.
will update it in the next patch.

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 172bb193ae6a..9ef477d79588 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_STE10XP) += ste10Xp.o
obj-$(CONFIG_TERANETICS_PHY) += teranetics.o
obj-$(CONFIG_VITESSE_PHY) += vitesse.o
obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
+obj-$(CONFIG_QCA808X_PHY) += qca808x.o
And this file is sorted by CONFIG_ so should be after
CONFIG_NXP_TJA11XX_PHY.

Keeping things sorted reduces the likelyhood of a merge conflict.
will update it in the next patch.

+#include <linux/module.h>
+#include <linux/etherdevice.h>
+#include <linux/phy.h>
+#include <linux/bitfield.h>
+
+#define QCA8081_PHY_ID 0x004DD101
+
+/* MII special status */
+#define QCA808X_PHY_SPEC_STATUS 0x11
+#define QCA808X_STATUS_FULL_DUPLEX BIT(13)
+#define QCA808X_STATUS_LINK_PASS BIT(10)
+#define QCA808X_STATUS_SPEED_MASK GENMASK(9, 7)
+#define QCA808X_STATUS_SPEED_100MBS 1
+#define QCA808X_STATUS_SPEED_1000MBS 2
+#define QCA808X_STATUS_SPEED_2500MBS 4
+
+/* MII interrupt enable & status */
+#define QCA808X_PHY_INTR_MASK 0x12
+#define QCA808X_PHY_INTR_STATUS 0x13
+#define QCA808X_INTR_ENABLE_FAST_RETRAIN_FAIL BIT(15)
+#define QCA808X_INTR_ENABLE_SPEED_CHANGED BIT(14)
+#define QCA808X_INTR_ENABLE_DUPLEX_CHANGED BIT(13)
+#define QCA808X_INTR_ENABLE_PAGE_RECEIVED BIT(12)
+#define QCA808X_INTR_ENABLE_LINK_FAIL BIT(11)
+#define QCA808X_INTR_ENABLE_LINK_SUCCESS BIT(10)
+#define QCA808X_INTR_ENABLE_POE BIT(1)
+#define QCA808X_INTR_ENABLE_WOL BIT(0)
+
+/* MII DBG address & data */
+#define QCA808X_PHY_DEBUG_ADDR 0x1d
+#define QCA808X_PHY_DEBUG_DATA 0x1e
+
A lot of these registers look the same as the at803x. So i'm thinking
you should merge these two drivers. There is a lot of code which is
identical, or very similar, which you can share.

Hi Andrew,

qca8081 supports IEEE1588 feature, the IEEE1588 code may be submitted in the near future,

so it may be a good idea to keep it out from at803x code.


+static int qca808x_get_2500caps(struct phy_device *phydev)
+{
+ int phy_data;
+
+ phy_data = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_PMA_CAP_REG);
+
+ return (phy_data & QCA808X_STATUS_2500T_FD_CAPS) ? 1 : 0;
So the PHY ignores the standard and does not set bit
MDIO_PMA_NG_EXTABLE_2_5GBT in MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE ?
Please compare the registers against the standards. If they are
actually being followed, please you the linux names for these
registers, and try to use the generic code.
will update it to use the generic code in the next patch.

+static int qca808x_phy_ms_random_seed_set(struct phy_device *phydev)
+{
+ u16 seed_value = (prandom_u32() % QCA808X_MASTER_SLAVE_SEED_RANGE) << 2;
+
+ return qca808x_debug_reg_modify(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+ QCA808X_MASTER_SLAVE_SEED_CFG, seed_value);
+}
+
+static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
+{
+ u16 seed_enable = 0;
+
+ if (enable)
+ seed_enable = QCA808X_MASTER_SLAVE_SEED_ENABLE;
+
+ return qca808x_debug_reg_modify(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
+ QCA808X_MASTER_SLAVE_SEED_ENABLE, seed_enable);
+}
This is interesting. I've not seen any other PHY does this. is there
documentation? Is the datasheet available?

this piece of code is for configuring the random seed to a lower value to make the PHY linked

as the SLAVE mode for fixing some IOT issue, for master/slave auto-negotiation, please refer to

https://www.ieee802.org/3/an/public/jul04/lynskey_2_0704.pdf.


+static int qca808x_soft_reset(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = genphy_soft_reset(phydev);
+ if (ret < 0)
+ return ret;
+
+ if (phydev->autoneg == AUTONEG_DISABLE) {
+ ret = qca808x_speed_forced(phydev);
+ if (ret)
+ return ret;
That is unusual. After a reset, you would expect the config_aneg()
function to be called, and it should set things like this. Why is it
needed?

I don't see anything handling the host side. Generally, devices like
this use SGMII for 10/100/1G. When 2.5G is in use they swap their host
interface to 2500BaseX. See mv3310_update_interface() as an example.

Andrew

thanks Andrew for the comments. will remove qca808x_speed_forced from reset function,

and add the phydev interface update in the next patch.