[RFC PATCH 1/3] net: phy: Add a generic phy file for TI generic PHYs

[RFC PATCH 1/3] net: phy: Add a generic phy file for TI generic PHYs

[Dan Murphy]

Add a TI Generic init file that will initialize TI PHYs that follow that
not require special handling.  These PHYs can connect with the standard
MII register set.  This generice file will register the PHY IDs and
names of the PHYs so when the command 'mdio list' is executed the PHY
name will display as opposed to 'Generic PHY'.

The DP8382X PHY series is a generic PHY that requires the generic
registration.

The DP83867 driver was updated to rename the init to a more PHY specific
init call.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 configs/am65x_evm_a53_defconfig      |   2 +-
 configs/am65x_hs_evm_a53_defconfig   |   2 +-
 configs/dra7xx_evm_defconfig         |   2 +-
 configs/dra7xx_hs_evm_defconfig      |   2 +-
 configs/dra7xx_hs_evm_usb_defconfig  |   2 +-
 configs/j721e_evm_a72_defconfig      |   2 +-
 configs/j721e_hs_evm_a72_defconfig   |   2 +-
 configs/k2g_evm_defconfig            |   2 +-
 configs/xilinx_versal_virt_defconfig |   2 +-
 configs/xilinx_zynqmp_virt_defconfig |   2 +-
 drivers/net/phy/Kconfig              |   8 ++
 drivers/net/phy/Makefile             |   3 +-
 drivers/net/phy/dp83867.c            |   3 +-
 drivers/net/phy/ti_phy_init.c        | 106 +++++++++++++++++++++++++++
 drivers/net/phy/ti_phy_init.h        |  16 ++++
 15 files changed, 144 insertions(+), 12 deletions(-)
 create mode 100644 drivers/net/phy/ti_phy_init.c
 create mode 100644 drivers/net/phy/ti_phy_init.h

diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
index 542bbd992c53..7051d6c40505 100644
--- a/configs/am65x_evm_a53_defconfig
+++ b/configs/am65x_evm_a53_defconfig
@@ -101,7 +101,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
 CONFIG_SPI_FLASH_STMICRO=y
 # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
 CONFIG_SPI_FLASH_MTD=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_FIXED=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
diff --git a/configs/am65x_hs_evm_a53_defconfig b/configs/am65x_hs_evm_a53_defconfig
index 9f43cee39611..29da3826f12a 100644
--- a/configs/am65x_hs_evm_a53_defconfig
+++ b/configs/am65x_hs_evm_a53_defconfig
@@ -103,7 +103,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
 CONFIG_SPI_FLASH_STMICRO=y
 # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
 CONFIG_SPI_FLASH_MTD=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_FIXED=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
index 4d765da4e052..19c024889155 100644
--- a/configs/dra7xx_evm_defconfig
+++ b/configs/dra7xx_evm_defconfig
@@ -86,7 +86,7 @@ CONFIG_DM_SPI_FLASH=y
 CONFIG_SF_DEFAULT_MODE=0
 CONFIG_SF_DEFAULT_SPEED=76800000
 CONFIG_SPI_FLASH_SPANSION=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_DM_ETH=y
 CONFIG_PHY_GIGE=y
 CONFIG_MII=y
diff --git a/configs/dra7xx_hs_evm_defconfig b/configs/dra7xx_hs_evm_defconfig
index c25d4ce5c142..e97f1a3ba338 100644
--- a/configs/dra7xx_hs_evm_defconfig
+++ b/configs/dra7xx_hs_evm_defconfig
@@ -89,7 +89,7 @@ CONFIG_DM_SPI_FLASH=y
 CONFIG_SF_DEFAULT_MODE=0
 CONFIG_SF_DEFAULT_SPEED=76800000
 CONFIG_SPI_FLASH_SPANSION=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_DM_ETH=y
 CONFIG_PHY_GIGE=y
 CONFIG_MII=y
diff --git a/configs/dra7xx_hs_evm_usb_defconfig b/configs/dra7xx_hs_evm_usb_defconfig
index 8e74496b2ccd..46970e31f02e 100644
--- a/configs/dra7xx_hs_evm_usb_defconfig
+++ b/configs/dra7xx_hs_evm_usb_defconfig
@@ -87,7 +87,7 @@ CONFIG_SF_DEFAULT_MODE=0
 CONFIG_SF_DEFAULT_SPEED=76800000
 CONFIG_SPI_FLASH_BAR=y
 CONFIG_SPI_FLASH_SPANSION=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_DM_ETH=y
 CONFIG_PHY_GIGE=y
 CONFIG_MII=y
diff --git a/configs/j721e_evm_a72_defconfig b/configs/j721e_evm_a72_defconfig
index e9e82bb4309d..784a6ff396c3 100644
--- a/configs/j721e_evm_a72_defconfig
+++ b/configs/j721e_evm_a72_defconfig
@@ -124,7 +124,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
 CONFIG_SPI_FLASH_STMICRO=y
 # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
 CONFIG_SPI_FLASH_MTD=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_FIXED=y
 CONFIG_DM_ETH=y
 CONFIG_TI_AM65_CPSW_NUSS=y
diff --git a/configs/j721e_hs_evm_a72_defconfig b/configs/j721e_hs_evm_a72_defconfig
index a723e2718e5e..dd93a955cefd 100644
--- a/configs/j721e_hs_evm_a72_defconfig
+++ b/configs/j721e_hs_evm_a72_defconfig
@@ -114,7 +114,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
 CONFIG_SPI_FLASH_STMICRO=y
 # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
 CONFIG_SPI_FLASH_MTD=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_FIXED=y
 CONFIG_DM_ETH=y
 CONFIG_TI_AM65_CPSW_NUSS=y
diff --git a/configs/k2g_evm_defconfig b/configs/k2g_evm_defconfig
index 5abf5faa450e..f47b1cabe9a8 100644
--- a/configs/k2g_evm_defconfig
+++ b/configs/k2g_evm_defconfig
@@ -58,7 +58,7 @@ CONFIG_PHYLIB=y
 CONFIG_PHY_MARVELL=y
 CONFIG_PHY_MICREL=y
 CONFIG_PHY_MICREL_KSZ8XXX=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_DM_ETH=y
 CONFIG_MII=y
 CONFIG_DRIVER_TI_KEYSTONE_NET=y
diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
index e8c349261207..eb2a26bc2c7d 100644
--- a/configs/xilinx_versal_virt_defconfig
+++ b/configs/xilinx_versal_virt_defconfig
@@ -61,7 +61,7 @@ CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_PHY_MARVELL=y
 CONFIG_PHY_NATSEMI=y
 CONFIG_PHY_REALTEK=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_VITESSE=y
 CONFIG_PHY_FIXED=y
 CONFIG_PHY_GIGE=y
diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
index 7b09edd78e1b..5c320f93fe5f 100644
--- a/configs/xilinx_zynqmp_virt_defconfig
+++ b/configs/xilinx_zynqmp_virt_defconfig
@@ -101,7 +101,7 @@ CONFIG_PHY_MICREL=y
 CONFIG_PHY_MICREL_KSZ90X1=y
 CONFIG_PHY_NATSEMI=y
 CONFIG_PHY_REALTEK=y
-CONFIG_PHY_TI=y
+CONFIG_PHY_DP83867=y
 CONFIG_PHY_VITESSE=y
 CONFIG_PHY_XILINX_GMII2RGMII=y
 CONFIG_PHY_FIXED=y
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index d1f049e62ab7..e366f10afc59 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -244,6 +244,14 @@ config PHY_TERANETICS
 config PHY_TI
 	bool "Texas Instruments Ethernet PHYs support"
 
+config PHY_TI_GENERIC
+	select PHY_TI
+	bool "Texas Instruments Ethernet PHYs support"
+
+config PHY_DP83867
+	select PHY_TI
+	bool "Texas Instruments Ethernet DP83867 PHY support"
+
 config PHY_VITESSE
 	bool "Vitesse Ethernet PHYs support"
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 1d81516ecd1d..9c6d31718c00 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -25,7 +25,8 @@ obj-$(CONFIG_PHY_NATSEMI) += natsemi.o
 obj-$(CONFIG_PHY_REALTEK) += realtek.o
 obj-$(CONFIG_PHY_SMSC) += smsc.o
 obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
-obj-$(CONFIG_PHY_TI) += dp83867.o
+obj-$(CONFIG_PHY_TI) += ti_phy_init.o
+obj-$(CONFIG_PHY_DP83867) += dp83867.o
 obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
 obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
 obj-$(CONFIG_PHY_VITESSE) += vitesse.o
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 50804c130efd..c9ed4a44d4db 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -12,6 +12,7 @@
 #include <dm.h>
 #include <dt-bindings/net/ti-dp83867.h>
 
+#include "ti_phy_init.h"
 
 /* TI DP83867 */
 #define DP83867_DEVADDR		0x1f
@@ -428,7 +429,7 @@ static struct phy_driver DP83867_driver = {
 	.shutdown = &genphy_shutdown,
 };
 
-int phy_ti_init(void)
+int phy_dp83867_init(void)
 {
 	phy_register(&DP83867_driver);
 	return 0;
diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
new file mode 100644
index 000000000000..11c4c166b2f5
--- /dev/null
+++ b/drivers/net/phy/ti_phy_init.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI Generic PHY Init to register all TI Ethernet PHYs
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * Copyright (C) 2019 Texas Instruments Inc.
+ */
+
+#include <phy.h>
+
+#define DP83822_PHY_ID	        0x2000a240
+#define DP83825S_PHY_ID		0x2000a140
+#define DP83825I_PHY_ID		0x2000a150
+#define DP83825CM_PHY_ID	0x2000a160
+#define DP83825CS_PHY_ID	0x2000a170
+#define DP83826C_PHY_ID		0x2000a130
+#define DP83826NC_PHY_ID	0x2000a110
+
+static struct phy_driver dp83822_driver = {
+	.name = "TI DP83822",
+	.uid = DP83822_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+static struct phy_driver dp83825s_driver = {
+	.name = "TI DP83825S",
+	.uid = DP83825S_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+static struct phy_driver dp83825i_driver = {
+	.name = "TI DP83825I",
+	.uid = DP83825I_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+static struct phy_driver dp83825m_driver = {
+	.name = "TI DP83825M",
+	.uid = DP83825CM_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+static struct phy_driver dp83825cs_driver = {
+	.name = "TI DP83825CS",
+	.uid = DP83825CS_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+static struct phy_driver dp83826c_driver = {
+	.name = "TI DP83826C",
+	.uid = DP83826C_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+static struct phy_driver dp83826nc_driver = {
+	.name = "TI DP83826NC",
+	.uid = DP83826NC_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_BASIC_FEATURES,
+	.config = &genphy_config_aneg,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+int phy_ti_init(void)
+{
+#ifdef CONFIG_PHY_DP83867
+	phy_dp83867_init();
+#endif
+
+#ifdef CONFIG_PHY_TI_GENERIC
+	phy_register(&dp83822_driver);
+	phy_register(&dp83825s_driver);
+	phy_register(&dp83825i_driver);
+	phy_register(&dp83825m_driver);
+	phy_register(&dp83825cs_driver);
+	phy_register(&dp83826c_driver);
+	phy_register(&dp83826nc_driver);
+#endif
+
+	return 0;
+}
diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
new file mode 100644
index 000000000000..309da2aacccb
--- /dev/null
+++ b/drivers/net/phy/ti_phy_init.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * TI Generic Ethernet PHY
+ * Based on code in sungem_phy.c and (long-removed) gianfar_phy.c
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * Copyright (C) 2019 Texas Instruments Inc.
+ */
+
+#ifndef _TI_GEN_PHY_H
+#define _TI_GEN_PHY_H
+
+int phy_dp83867_init(void);
+
+#endif /* _TI_GEN_PHY_H */
-- 
2.25.1

[RFC PATCH 2/3] net: phy: Add helper routines to set and clear bits

[Dan Murphy]

Add phy_set/clear_bit helper routines so that ported drivers from the
kernel can use these functions.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/phy.h b/include/phy.h
index b5de14cbfc29..050c989fa537 100644
--- a/include/phy.h
+++ b/include/phy.h
@@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad,
 	return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
 }
 
+static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad,
+				   u32 regnum, u16 val)
+{
+	int value;
+	int ret;
+
+	value = phy_read_mmd(phydev, devad, regnum);
+	if (value < 0)
+		return value;
+
+	value |= val;
+
+	ret = phy_write_mmd(phydev, devad, regnum, value);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static inline int phy_clear_bits_mmd(struct phy_device *phydev, int devad,
+				     u32 regnum, u16 val)
+{
+	int value;
+	int ret;
+
+	value = phy_read_mmd(phydev, devad, regnum);
+	if (value < 0)
+		return value;
+
+	value &= ~val;
+
+	ret = phy_write_mmd(phydev, devad, regnum, value);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 #ifdef CONFIG_PHYLIB_10G
 extern struct phy_driver gen10g_driver;
 
-- 
2.25.1

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Dan Murphy]

Port the DP83869 kernel driver.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/Kconfig              |   4 +
 drivers/net/phy/Makefile             |   1 +
 drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
 drivers/net/phy/ti_phy_init.c        |   4 +
 drivers/net/phy/ti_phy_init.h        |   1 +
 include/dt-bindings/net/ti-dp83869.h |  42 +++
 6 files changed, 492 insertions(+)
 create mode 100644 drivers/net/phy/dp83869.c
 create mode 100644 include/dt-bindings/net/ti-dp83869.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index e366f10afc59..5f14bdba0a3b 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -252,6 +252,10 @@ config PHY_DP83867
 	select PHY_TI
 	bool "Texas Instruments Ethernet DP83867 PHY support"
 
+config PHY_DP83869
+	select PHY_TI
+	bool "Texas Instruments Ethernet DP83869 PHY support"
+
 config PHY_VITESSE
 	bool "Vitesse Ethernet PHYs support"
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 9c6d31718c00..f8797319154f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
 obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
 obj-$(CONFIG_PHY_TI) += ti_phy_init.o
 obj-$(CONFIG_PHY_DP83867) += dp83867.o
+obj-$(CONFIG_PHY_DP83869) += dp83869.o
 obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
 obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
 obj-$(CONFIG_PHY_VITESSE) += vitesse.o
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
new file mode 100644
index 000000000000..1eaaea20b37a
--- /dev/null
+++ b/drivers/net/phy/dp83869.c
@@ -0,0 +1,440 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for the Texas Instruments DP83869 PHY
+ * Copyright (C) 2019 Texas Instruments Inc.
+ */
+
+#include <common.h>
+#include <phy.h>
+#include <dm/devres.h>
+#include <linux/compat.h>
+#include <malloc.h>
+
+#include <dm.h>
+
+#include <dt-bindings/net/ti-dp83869.h>
+
+#include "ti_phy_init.h"
+
+#define DP83869_PHY_ID		0x2000a0f1
+#define DP83869_DEVADDR		0x1f
+
+#define MII_DP83869_PHYCTRL	0x10
+#define MII_DP83869_MICR	0x12
+#define MII_DP83869_ISR		0x13
+#define DP83869_CTRL		0x1f
+#define DP83869_CFG4		0x1e
+
+/* Extended Registers */
+#define DP83869_GEN_CFG3        0x0031
+#define DP83869_RGMIICTL	0x0032
+#define DP83869_STRAP_STS1	0x006e
+#define DP83869_RGMIIDCTL	0x0086
+#define DP83869_IO_MUX_CFG	0x0170
+#define DP83869_OP_MODE		0x01df
+#define DP83869_FX_CTRL		0x0c00
+
+#define DP83869_SW_RESET	BIT(15)
+#define DP83869_SW_RESTART	BIT(14)
+
+/* MICR Interrupt bits */
+#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
+#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
+#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
+#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
+#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
+#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
+#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
+#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
+#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
+#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
+#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
+#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
+
+#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
+					 BMCR_FULLDPLX | \
+					 BMCR_SPEED1000)
+
+#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
+					 ADVERTISE_1000XHALF | \
+					 ADVERTISE_1000XFULL | \
+					 ADVERTISE_1000XPAUSE | \
+					 ADVERTISE_1000XPSE_ASYM)
+
+/* This is the same bit mask as the BMCR so re-use the BMCR default */
+#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
+
+/* CFG1 bits */
+#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
+				 ADVERTISE_1000FULL | \
+				 CTL1000_AS_MASTER)
+
+/* RGMIICTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
+#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
+
+/* STRAP_STS1 bits */
+#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
+#define DP83869_STRAP_STS1_RESERVED		BIT(11)
+#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
+
+/* PHYCTRL bits */
+#define DP83869_RX_FIFO_SHIFT	12
+#define DP83869_TX_FIFO_SHIFT	14
+
+/* PHY_CTRL lower bytes 0x48 are declared as reserved */
+#define DP83869_PHY_CTRL_DEFAULT	0x48
+#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
+#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
+
+/* RGMIIDCTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
+
+/* IO_MUX_CFG bits */
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
+
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
+#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
+#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
+
+/* CFG3 bits */
+#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
+
+/* CFG4 bits */
+#define DP83869_INT_OE	BIT(7)
+
+/* OP MODE */
+#define DP83869_OP_MODE_MII			BIT(5)
+#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
+
+enum {
+	DP83869_PORT_MIRRORING_KEEP,
+	DP83869_PORT_MIRRORING_EN,
+	DP83869_PORT_MIRRORING_DIS,
+};
+
+struct dp83869_private {
+	int tx_fifo_depth;
+	int rx_fifo_depth;
+	int io_impedance;
+	int port_mirroring;
+	bool rxctrl_strap_quirk;
+	int clk_output_sel;
+	int mode;
+};
+
+static int dp83869_config_port_mirroring(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+
+	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
+		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+					DP83869_GEN_CFG3,
+					DP83869_CFG3_PORT_MIRROR_EN);
+	else
+		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
+					  DP83869_GEN_CFG3,
+					  DP83869_CFG3_PORT_MIRROR_EN);
+}
+
+static int dp83869_set_strapped_mode(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	u16 val;
+
+	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+	if (val < 0)
+		return val;
+
+	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
+
+	return 0;
+}
+
+#ifdef CONFIG_OF_MDIO
+static int dp83869_of_init(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	int ret;
+
+	if (!of_node)
+		return -ENODEV;
+
+	dp83869->io_impedance = -EINVAL;
+
+	/* Optional configuration */
+	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
+				   &dp83869->clk_output_sel);
+	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
+		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
+
+	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
+	if (ret == 0) {
+		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
+		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
+			return -EINVAL;
+	} else {
+		ret = dp83869_set_strapped_mode(phydev);
+		if (ret)
+			return ret;
+	}
+
+	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
+		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
+	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
+		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
+
+	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
+		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
+	} else {
+		/* If the lane swap is not in the DT then check the straps */
+		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+		if (ret < 0)
+			return ret;
+		if (ret & DP83869_STRAP_MIRROR_ENABLED)
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
+		else
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
+	}
+
+	if (of_property_read_u32(of_node, "rx-fifo-depth",
+				 &dp83869->rx_fifo_depth))
+		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
+
+	if (of_property_read_u32(of_node, "tx-fifo-depth",
+				 &dp83869->tx_fifo_depth))
+		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
+
+	return ret;
+}
+#else
+static int dp83869_of_init(struct phy_device *phydev)
+{
+	return dp83869_set_strapped_mode(phydev);
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int dp83869_configure_rgmii(struct phy_device *phydev,
+				   struct dp83869_private *dp83869)
+{
+	int ret = 0, val;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
+		if (val < 0)
+			return val;
+
+		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
+		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
+		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
+		if (ret)
+			return ret;
+	}
+
+	if (dp83869->io_impedance >= 0)
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+				       DP83869_IO_MUX_CFG,
+				       dp83869->io_impedance &
+				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
+	return ret;
+}
+
+static int dp83869_config_aneg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int err;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
+				MII_DP83869_FIBER_ADVERTISE);
+		if (err)
+			return err;
+
+		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
+				DP83869_SW_RESTART);
+		if (err)
+			return err;
+
+		return genphy_restart_aneg(phydev);
+	} else {
+		return genphy_config_aneg(phydev);
+	}
+}
+
+static int dp83869_configure_mode(struct phy_device *phydev,
+				  struct dp83869_private *dp83869)
+{
+	int phy_ctrl_val;
+	int ret;
+
+	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
+	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
+		return -EINVAL;
+
+	/* Below init sequence for each operational mode is defined in
+	 * section 9.4.8 of the datasheet.
+	 */
+	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
+			    dp83869->mode);
+	if (ret)
+		return ret;
+
+	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
+	if (ret)
+		return ret;
+
+	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
+			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
+			DP83869_PHY_CTRL_DEFAULT);
+
+	switch (dp83869->mode) {
+	case DP83869_RGMII_COPPER_ETHERNET:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
+				DP83869_CFG1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = dp83869_configure_rgmii(phydev, dp83869);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_RGMII_SGMII_BRIDGE:
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
+				     DP83869_SGMII_RGMII_BRIDGE,
+				     DP83869_SGMII_RGMII_BRIDGE);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+
+		break;
+	case DP83869_1000M_MEDIA_CONVERT:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_100M_MEDIA_CONVERT:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_SGMII_COPPER_ETHERNET:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+
+		break;
+	case DP83869_RGMII_100_BASE:
+	case DP83869_RGMII_1000_BASE:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int dp83869_phy_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
+	if (ret < 0)
+		return ret;
+
+	udelay(200);
+
+	return 0;
+}
+
+static int dp83869_config_init(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int ret;
+
+	ret = dp83869_phy_reset(phydev);
+	if (ret)
+		return ret;
+
+	ret = dp83869_configure_mode(phydev, dp83869);
+	if (ret)
+		return ret;
+
+	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
+		dp83869_config_port_mirroring(phydev);
+
+	/* Clock output selection if muxing property is set */
+	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+				       DP83869_IO_MUX_CFG,
+				       dp83869->clk_output_sel <<
+				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
+
+	dp83869_config_aneg(phydev);
+
+	return ret;
+}
+
+static int dp83869_probe(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869;
+	int ret;
+
+	dp83869 = kzalloc(sizeof(*dp83869), GFP_KERNEL);
+	if (!dp83869)
+		return -ENOMEM;
+
+	phydev->priv = dp83869;
+
+	ret = dp83869_of_init(phydev);
+	if (ret)
+		return ret;
+
+	return dp83869_config_init(phydev);
+}
+
+static struct phy_driver dp83869_driver = {
+	.name = "TI DP83869",
+	.uid = DP83869_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_GBIT_FEATURES,
+	.probe = dp83869_probe,
+	.config = &dp83869_config_init,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+int phy_dp83869_init(void)
+{
+	phy_register(&dp83869_driver);
+	return 0;
+}
diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
index 11c4c166b2f5..cb261b86b05d 100644
--- a/drivers/net/phy/ti_phy_init.c
+++ b/drivers/net/phy/ti_phy_init.c
@@ -92,6 +92,10 @@ int phy_ti_init(void)
 	phy_dp83867_init();
 #endif
 
+#ifdef CONFIG_PHY_DP83869
+	phy_dp83869_init();
+#endif
+
 #ifdef CONFIG_PHY_TI_GENERIC
 	phy_register(&dp83822_driver);
 	phy_register(&dp83825s_driver);
diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
index 309da2aacccb..f94ff6291a3f 100644
--- a/drivers/net/phy/ti_phy_init.h
+++ b/drivers/net/phy/ti_phy_init.h
@@ -12,5 +12,6 @@
 #define _TI_GEN_PHY_H
 
 int phy_dp83867_init(void);
+int phy_dp83869_init(void);
 
 #endif /* _TI_GEN_PHY_H */
diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
new file mode 100644
index 000000000000..218b1a64e975
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83869.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Device Tree constants for the Texas Instruments DP83869 PHY
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * Copyright:   (C) 2019 Texas Instruments, Inc.
+ */
+
+#ifndef _DT_BINDINGS_TI_DP83869_H
+#define _DT_BINDINGS_TI_DP83869_H
+
+/* PHY CTRL bits */
+#define DP83869_PHYCR_FIFO_DEPTH_3_B_NIB	0x00
+#define DP83869_PHYCR_FIFO_DEPTH_4_B_NIB	0x01
+#define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
+#define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
+
+/* IO_MUX_CFG - Clock output selection */
+#define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
+#define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
+#define DP83869_CLK_O_SEL_CHN_C_RCLK		0x2
+#define DP83869_CLK_O_SEL_CHN_D_RCLK		0x3
+#define DP83869_CLK_O_SEL_CHN_A_RCLK_DIV5	0x4
+#define DP83869_CLK_O_SEL_CHN_B_RCLK_DIV5	0x5
+#define DP83869_CLK_O_SEL_CHN_C_RCLK_DIV5	0x6
+#define DP83869_CLK_O_SEL_CHN_D_RCLK_DIV5	0x7
+#define DP83869_CLK_O_SEL_CHN_A_TCLK		0x8
+#define DP83869_CLK_O_SEL_CHN_B_TCLK		0x9
+#define DP83869_CLK_O_SEL_CHN_C_TCLK		0xa
+#define DP83869_CLK_O_SEL_CHN_D_TCLK		0xb
+#define DP83869_CLK_O_SEL_REF_CLK		0xc
+
+#define DP83869_RGMII_COPPER_ETHERNET		0x00
+#define DP83869_RGMII_1000_BASE			0x01
+#define DP83869_RGMII_100_BASE			0x02
+#define DP83869_RGMII_SGMII_BRIDGE		0x03
+#define DP83869_1000M_MEDIA_CONVERT		0x04
+#define DP83869_100M_MEDIA_CONVERT		0x05
+#define DP83869_SGMII_COPPER_ETHERNET		0x06
+
+#endif
-- 
2.25.1

[RFC PATCH 1/3] net: phy: Add a generic phy file for TI generic PHYs

[Michal Simek]

On 20. 04. 20 20:53, Dan Murphy wrote:
> Add a TI Generic init file that will initialize TI PHYs that follow that
> not require special handling.  These PHYs can connect with the standard
> MII register set.  This generice file will register the PHY IDs and
> names of the PHYs so when the command 'mdio list' is executed the PHY
> name will display as opposed to 'Generic PHY'.
> 
> The DP8382X PHY series is a generic PHY that requires the generic
> registration.
> 
> The DP83867 driver was updated to rename the init to a more PHY specific
> init call.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

I would personally do it with two patches.

> ---
>  configs/am65x_evm_a53_defconfig      |   2 +-
>  configs/am65x_hs_evm_a53_defconfig   |   2 +-
>  configs/dra7xx_evm_defconfig         |   2 +-
>  configs/dra7xx_hs_evm_defconfig      |   2 +-
>  configs/dra7xx_hs_evm_usb_defconfig  |   2 +-
>  configs/j721e_evm_a72_defconfig      |   2 +-
>  configs/j721e_hs_evm_a72_defconfig   |   2 +-
>  configs/k2g_evm_defconfig            |   2 +-
>  configs/xilinx_versal_virt_defconfig |   2 +-
>  configs/xilinx_zynqmp_virt_defconfig |   2 +-
>  drivers/net/phy/Kconfig              |   8 ++
>  drivers/net/phy/Makefile             |   3 +-
>  drivers/net/phy/dp83867.c            |   3 +-
>  drivers/net/phy/ti_phy_init.c        | 106 +++++++++++++++++++++++++++
>  drivers/net/phy/ti_phy_init.h        |  16 ++++
>  15 files changed, 144 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/net/phy/ti_phy_init.c
>  create mode 100644 drivers/net/phy/ti_phy_init.h
> 
> diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
> index 542bbd992c53..7051d6c40505 100644
> --- a/configs/am65x_evm_a53_defconfig
> +++ b/configs/am65x_evm_a53_defconfig
> @@ -101,7 +101,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>  CONFIG_SPI_FLASH_STMICRO=y
>  # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>  CONFIG_SPI_FLASH_MTD=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y

Don't know why this name was chosen but don't you want to label it with TI?

CONFIG_PHY_TI_DP83867 ?

Kernel is using different symbol anyway.
CONFIG_DP83867_PHY



>  CONFIG_PHY_FIXED=y
>  CONFIG_DM_ETH=y
>  CONFIG_E1000=y
> diff --git a/configs/am65x_hs_evm_a53_defconfig b/configs/am65x_hs_evm_a53_defconfig
> index 9f43cee39611..29da3826f12a 100644
> --- a/configs/am65x_hs_evm_a53_defconfig
> +++ b/configs/am65x_hs_evm_a53_defconfig
> @@ -103,7 +103,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>  CONFIG_SPI_FLASH_STMICRO=y
>  # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>  CONFIG_SPI_FLASH_MTD=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_PHY_FIXED=y
>  CONFIG_DM_ETH=y
>  CONFIG_E1000=y
> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
> index 4d765da4e052..19c024889155 100644
> --- a/configs/dra7xx_evm_defconfig
> +++ b/configs/dra7xx_evm_defconfig
> @@ -86,7 +86,7 @@ CONFIG_DM_SPI_FLASH=y
>  CONFIG_SF_DEFAULT_MODE=0
>  CONFIG_SF_DEFAULT_SPEED=76800000
>  CONFIG_SPI_FLASH_SPANSION=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_DM_ETH=y
>  CONFIG_PHY_GIGE=y
>  CONFIG_MII=y
> diff --git a/configs/dra7xx_hs_evm_defconfig b/configs/dra7xx_hs_evm_defconfig
> index c25d4ce5c142..e97f1a3ba338 100644
> --- a/configs/dra7xx_hs_evm_defconfig
> +++ b/configs/dra7xx_hs_evm_defconfig
> @@ -89,7 +89,7 @@ CONFIG_DM_SPI_FLASH=y
>  CONFIG_SF_DEFAULT_MODE=0
>  CONFIG_SF_DEFAULT_SPEED=76800000
>  CONFIG_SPI_FLASH_SPANSION=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_DM_ETH=y
>  CONFIG_PHY_GIGE=y
>  CONFIG_MII=y
> diff --git a/configs/dra7xx_hs_evm_usb_defconfig b/configs/dra7xx_hs_evm_usb_defconfig
> index 8e74496b2ccd..46970e31f02e 100644
> --- a/configs/dra7xx_hs_evm_usb_defconfig
> +++ b/configs/dra7xx_hs_evm_usb_defconfig
> @@ -87,7 +87,7 @@ CONFIG_SF_DEFAULT_MODE=0
>  CONFIG_SF_DEFAULT_SPEED=76800000
>  CONFIG_SPI_FLASH_BAR=y
>  CONFIG_SPI_FLASH_SPANSION=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_DM_ETH=y
>  CONFIG_PHY_GIGE=y
>  CONFIG_MII=y
> diff --git a/configs/j721e_evm_a72_defconfig b/configs/j721e_evm_a72_defconfig
> index e9e82bb4309d..784a6ff396c3 100644
> --- a/configs/j721e_evm_a72_defconfig
> +++ b/configs/j721e_evm_a72_defconfig
> @@ -124,7 +124,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>  CONFIG_SPI_FLASH_STMICRO=y
>  # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>  CONFIG_SPI_FLASH_MTD=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_PHY_FIXED=y
>  CONFIG_DM_ETH=y
>  CONFIG_TI_AM65_CPSW_NUSS=y
> diff --git a/configs/j721e_hs_evm_a72_defconfig b/configs/j721e_hs_evm_a72_defconfig
> index a723e2718e5e..dd93a955cefd 100644
> --- a/configs/j721e_hs_evm_a72_defconfig
> +++ b/configs/j721e_hs_evm_a72_defconfig
> @@ -114,7 +114,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>  CONFIG_SPI_FLASH_STMICRO=y
>  # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>  CONFIG_SPI_FLASH_MTD=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_PHY_FIXED=y
>  CONFIG_DM_ETH=y
>  CONFIG_TI_AM65_CPSW_NUSS=y
> diff --git a/configs/k2g_evm_defconfig b/configs/k2g_evm_defconfig
> index 5abf5faa450e..f47b1cabe9a8 100644
> --- a/configs/k2g_evm_defconfig
> +++ b/configs/k2g_evm_defconfig
> @@ -58,7 +58,7 @@ CONFIG_PHYLIB=y
>  CONFIG_PHY_MARVELL=y
>  CONFIG_PHY_MICREL=y
>  CONFIG_PHY_MICREL_KSZ8XXX=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_DM_ETH=y
>  CONFIG_MII=y
>  CONFIG_DRIVER_TI_KEYSTONE_NET=y
> diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
> index e8c349261207..eb2a26bc2c7d 100644
> --- a/configs/xilinx_versal_virt_defconfig
> +++ b/configs/xilinx_versal_virt_defconfig
> @@ -61,7 +61,7 @@ CONFIG_SPI_FLASH_WINBOND=y
>  CONFIG_PHY_MARVELL=y
>  CONFIG_PHY_NATSEMI=y
>  CONFIG_PHY_REALTEK=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_PHY_VITESSE=y
>  CONFIG_PHY_FIXED=y
>  CONFIG_PHY_GIGE=y
> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> index 7b09edd78e1b..5c320f93fe5f 100644
> --- a/configs/xilinx_zynqmp_virt_defconfig
> +++ b/configs/xilinx_zynqmp_virt_defconfig
> @@ -101,7 +101,7 @@ CONFIG_PHY_MICREL=y
>  CONFIG_PHY_MICREL_KSZ90X1=y
>  CONFIG_PHY_NATSEMI=y
>  CONFIG_PHY_REALTEK=y
> -CONFIG_PHY_TI=y
> +CONFIG_PHY_DP83867=y
>  CONFIG_PHY_VITESSE=y
>  CONFIG_PHY_XILINX_GMII2RGMII=y
>  CONFIG_PHY_FIXED=y
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index d1f049e62ab7..e366f10afc59 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -244,6 +244,14 @@ config PHY_TERANETICS
>  config PHY_TI
>  	bool "Texas Instruments Ethernet PHYs support"
>  
> +config PHY_TI_GENERIC
> +	select PHY_TI
> +	bool "Texas Instruments Ethernet PHYs support"
> +
> +config PHY_DP83867
> +	select PHY_TI
> +	bool "Texas Instruments Ethernet DP83867 PHY support"
> +
>  config PHY_VITESSE
>  	bool "Vitesse Ethernet PHYs support"
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 1d81516ecd1d..9c6d31718c00 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -25,7 +25,8 @@ obj-$(CONFIG_PHY_NATSEMI) += natsemi.o
>  obj-$(CONFIG_PHY_REALTEK) += realtek.o
>  obj-$(CONFIG_PHY_SMSC) += smsc.o
>  obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
> -obj-$(CONFIG_PHY_TI) += dp83867.o
> +obj-$(CONFIG_PHY_TI) += ti_phy_init.o
> +obj-$(CONFIG_PHY_DP83867) += dp83867.o
>  obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>  obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>  obj-$(CONFIG_PHY_VITESSE) += vitesse.o
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 50804c130efd..c9ed4a44d4db 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -12,6 +12,7 @@
>  #include <dm.h>
>  #include <dt-bindings/net/ti-dp83867.h>
>  
> +#include "ti_phy_init.h"
>  
>  /* TI DP83867 */
>  #define DP83867_DEVADDR		0x1f
> @@ -428,7 +429,7 @@ static struct phy_driver DP83867_driver = {
>  	.shutdown = &genphy_shutdown,
>  };
>  
> -int phy_ti_init(void)
> +int phy_dp83867_init(void)
>  {
>  	phy_register(&DP83867_driver);
>  	return 0;
> diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
> new file mode 100644
> index 000000000000..11c4c166b2f5
> --- /dev/null
> +++ b/drivers/net/phy/ti_phy_init.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TI Generic PHY Init to register all TI Ethernet PHYs
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * Copyright (C) 2019 Texas Instruments Inc.

2020?

> + */
> +
> +#include <phy.h>
> +
> +#define DP83822_PHY_ID	        0x2000a240
> +#define DP83825S_PHY_ID		0x2000a140
> +#define DP83825I_PHY_ID		0x2000a150
> +#define DP83825CM_PHY_ID	0x2000a160
> +#define DP83825CS_PHY_ID	0x2000a170
> +#define DP83826C_PHY_ID		0x2000a130
> +#define DP83826NC_PHY_ID	0x2000a110
> +
> +static struct phy_driver dp83822_driver = {
> +	.name = "TI DP83822",
> +	.uid = DP83822_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +static struct phy_driver dp83825s_driver = {
> +	.name = "TI DP83825S",
> +	.uid = DP83825S_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +static struct phy_driver dp83825i_driver = {
> +	.name = "TI DP83825I",
> +	.uid = DP83825I_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +static struct phy_driver dp83825m_driver = {
> +	.name = "TI DP83825M",
> +	.uid = DP83825CM_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +static struct phy_driver dp83825cs_driver = {
> +	.name = "TI DP83825CS",
> +	.uid = DP83825CS_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +static struct phy_driver dp83826c_driver = {
> +	.name = "TI DP83826C",
> +	.uid = DP83826C_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +static struct phy_driver dp83826nc_driver = {
> +	.name = "TI DP83826NC",
> +	.uid = DP83826NC_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_BASIC_FEATURES,
> +	.config = &genphy_config_aneg,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +int phy_ti_init(void)
> +{
> +#ifdef CONFIG_PHY_DP83867
> +	phy_dp83867_init();
> +#endif
> +
> +#ifdef CONFIG_PHY_TI_GENERIC
> +	phy_register(&dp83822_driver);
> +	phy_register(&dp83825s_driver);
> +	phy_register(&dp83825i_driver);
> +	phy_register(&dp83825m_driver);
> +	phy_register(&dp83825cs_driver);
> +	phy_register(&dp83826c_driver);
> +	phy_register(&dp83826nc_driver);
> +#endif

I would do it in separate patch but up2you.

> +
> +	return 0;
> +}
> diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
> new file mode 100644
> index 000000000000..309da2aacccb
> --- /dev/null
> +++ b/drivers/net/phy/ti_phy_init.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * TI Generic Ethernet PHY
> + * Based on code in sungem_phy.c and (long-removed) gianfar_phy.c
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * Copyright (C) 2019 Texas Instruments Inc.

2020

> + */
> +
> +#ifndef _TI_GEN_PHY_H
> +#define _TI_GEN_PHY_H
> +
> +int phy_dp83867_init(void);
> +
> +#endif /* _TI_GEN_PHY_H */
> 

Thanks,
Michal

[RFC PATCH 2/3] net: phy: Add helper routines to set and clear bits

[Michal Simek]

On 20. 04. 20 20:53, Dan Murphy wrote:
> Add phy_set/clear_bit helper routines so that ported drivers from the
> kernel can use these functions.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/phy.h b/include/phy.h
> index b5de14cbfc29..050c989fa537 100644
> --- a/include/phy.h
> +++ b/include/phy.h
> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad,
>  	return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
>  }
>  

kernel-doc description would be good.

> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad,
> +				   u32 regnum, u16 val)
> +{
> +	int value;
> +	int ret;

nit: on one line should be better.

> +
> +	value = phy_read_mmd(phydev, devad, regnum);
> +	if (value < 0)
> +		return value;
> +
> +	value |= val;
> +
> +	ret = phy_write_mmd(phydev, devad, regnum, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static inline int phy_clear_bits_mmd(struct phy_device *phydev, int devad,
> +				     u32 regnum, u16 val)
> +{
> +	int value;
> +	int ret;

ditto.

> +
> +	value = phy_read_mmd(phydev, devad, regnum);
> +	if (value < 0)
> +		return value;
> +
> +	value &= ~val;
> +
> +	ret = phy_write_mmd(phydev, devad, regnum, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PHYLIB_10G
>  extern struct phy_driver gen10g_driver;
>  

Thanks,
Michal

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Michal Simek]

On 20. 04. 20 20:53, Dan Murphy wrote:
> Port the DP83869 kernel driver.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/Kconfig              |   4 +
>  drivers/net/phy/Makefile             |   1 +
>  drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
>  drivers/net/phy/ti_phy_init.c        |   4 +
>  drivers/net/phy/ti_phy_init.h        |   1 +
>  include/dt-bindings/net/ti-dp83869.h |  42 +++
>  6 files changed, 492 insertions(+)
>  create mode 100644 drivers/net/phy/dp83869.c
>  create mode 100644 include/dt-bindings/net/ti-dp83869.h
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index e366f10afc59..5f14bdba0a3b 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -252,6 +252,10 @@ config PHY_DP83867
>  	select PHY_TI
>  	bool "Texas Instruments Ethernet DP83867 PHY support"
>  
> +config PHY_DP83869
> +	select PHY_TI
> +	bool "Texas Instruments Ethernet DP83869 PHY support"

isn't this a little bit short? I expect checkpatch should be reporting
issue with it.


> +
>  config PHY_VITESSE
>  	bool "Vitesse Ethernet PHYs support"
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 9c6d31718c00..f8797319154f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>  obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>  obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>  obj-$(CONFIG_PHY_DP83867) += dp83867.o
> +obj-$(CONFIG_PHY_DP83869) += dp83869.o
>  obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>  obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>  obj-$(CONFIG_PHY_VITESSE) += vitesse.o
> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> new file mode 100644
> index 000000000000..1eaaea20b37a
> --- /dev/null
> +++ b/drivers/net/phy/dp83869.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for the Texas Instruments DP83869 PHY
> + * Copyright (C) 2019 Texas Instruments Inc.

2020?

> + */
> +
> +#include <common.h>
> +#include <phy.h>
> +#include <dm/devres.h>
> +#include <linux/compat.h>
> +#include <malloc.h>
> +
> +#include <dm.h>
> +
> +#include <dt-bindings/net/ti-dp83869.h>
> +
> +#include "ti_phy_init.h"
> +
> +#define DP83869_PHY_ID		0x2000a0f1
> +#define DP83869_DEVADDR		0x1f
> +
> +#define MII_DP83869_PHYCTRL	0x10
> +#define MII_DP83869_MICR	0x12
> +#define MII_DP83869_ISR		0x13
> +#define DP83869_CTRL		0x1f
> +#define DP83869_CFG4		0x1e
> +
> +/* Extended Registers */
> +#define DP83869_GEN_CFG3        0x0031
> +#define DP83869_RGMIICTL	0x0032
> +#define DP83869_STRAP_STS1	0x006e
> +#define DP83869_RGMIIDCTL	0x0086
> +#define DP83869_IO_MUX_CFG	0x0170
> +#define DP83869_OP_MODE		0x01df
> +#define DP83869_FX_CTRL		0x0c00
> +
> +#define DP83869_SW_RESET	BIT(15)
> +#define DP83869_SW_RESTART	BIT(14)
> +
> +/* MICR Interrupt bits */
> +#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
> +#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
> +#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
> +#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
> +
> +#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
> +					 BMCR_FULLDPLX | \
> +					 BMCR_SPEED1000)
> +
> +#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
> +					 ADVERTISE_1000XHALF | \
> +					 ADVERTISE_1000XFULL | \
> +					 ADVERTISE_1000XPAUSE | \
> +					 ADVERTISE_1000XPSE_ASYM)
> +
> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
> +#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
> +
> +/* CFG1 bits */
> +#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
> +				 ADVERTISE_1000FULL | \
> +				 CTL1000_AS_MASTER)
> +
> +/* RGMIICTL bits */
> +#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
> +#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
> +
> +/* STRAP_STS1 bits */
> +#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
> +#define DP83869_STRAP_STS1_RESERVED		BIT(11)
> +#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
> +
> +/* PHYCTRL bits */
> +#define DP83869_RX_FIFO_SHIFT	12
> +#define DP83869_TX_FIFO_SHIFT	14
> +
> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
> +#define DP83869_PHY_CTRL_DEFAULT	0x48
> +#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
> +#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
> +
> +/* RGMIIDCTL bits */
> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
> +
> +/* IO_MUX_CFG bits */
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
> +
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
> +
> +/* CFG3 bits */
> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
> +
> +/* CFG4 bits */
> +#define DP83869_INT_OE	BIT(7)
> +
> +/* OP MODE */
> +#define DP83869_OP_MODE_MII			BIT(5)
> +#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
> +
> +enum {
> +	DP83869_PORT_MIRRORING_KEEP,
> +	DP83869_PORT_MIRRORING_EN,
> +	DP83869_PORT_MIRRORING_DIS,
> +};

We have met with this in the kernel. Greg was asking us to write exact
value to all enum entries.


> +
> +struct dp83869_private {
> +	int tx_fifo_depth;
> +	int rx_fifo_depth;
> +	int io_impedance;
> +	int port_mirroring;
> +	bool rxctrl_strap_quirk;
> +	int clk_output_sel;
> +	int mode;
> +};

I would prefer kernel-doc format to make reading easier.
Also tx_fifo_depth doesn't look like signed type. I would make them
unsigned.


> +
> +static int dp83869_config_port_mirroring(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +
> +	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
> +		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +					DP83869_GEN_CFG3,
> +					DP83869_CFG3_PORT_MIRROR_EN);
> +	else

you can remove this else.

> +		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
> +					  DP83869_GEN_CFG3,
> +					  DP83869_CFG3_PORT_MIRROR_EN);
> +}
> +
> +static int dp83869_set_strapped_mode(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	u16 val;
> +
> +	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
> +	if (val < 0)
> +		return val;
> +
> +	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF_MDIO

Isn't there a way to remove these?

if (IS_ENABLED(CONFIG_OF_MDIO))
...

> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;
> +	int ret;
> +
> +	if (!of_node)
> +		return -ENODEV;

didn't go deep to this but can this happen?

> +
> +	dp83869->io_impedance = -EINVAL;

I would prefer to group it together. It means move this before dt reading.

> +
> +	/* Optional configuration */
> +	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
> +				   &dp83869->clk_output_sel);
> +	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
> +		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
> +
> +	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
> +	if (ret == 0) {

!ret

> +		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
> +		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
> +			return -EINVAL;
> +	} else {
> +		ret = dp83869_set_strapped_mode(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
> +	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
> +
> +	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
> +		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
> +	} else {
> +		/* If the lane swap is not in the DT then check the straps */
> +		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
> +		if (ret < 0)
> +			return ret;

newline here please.

> +		if (ret & DP83869_STRAP_MIRROR_ENABLED)
> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
> +		else
> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
> +	}
> +
> +	if (of_property_read_u32(of_node, "rx-fifo-depth",
> +				 &dp83869->rx_fifo_depth))
> +		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
> +
> +	if (of_property_read_u32(of_node, "tx-fifo-depth",
> +				 &dp83869->tx_fifo_depth))
> +		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
> +
> +	return ret;
> +}
> +#else
> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	return dp83869_set_strapped_mode(phydev);
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
> +static int dp83869_configure_rgmii(struct phy_device *phydev,
> +				   struct dp83869_private *dp83869)
> +{
> +	int ret = 0, val;
> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
> +		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
> +		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (dp83869->io_impedance >= 0)
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +				       DP83869_IO_MUX_CFG,
> +				       dp83869->io_impedance &
> +				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
> +	return ret;
> +}
> +
> +static int dp83869_config_aneg(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	int err;
> +
> +	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
> +		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
> +				MII_DP83869_FIBER_ADVERTISE);
> +		if (err)
> +			return err;
> +
> +		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
> +				DP83869_SW_RESTART);
> +		if (err)
> +			return err;
> +
> +		return genphy_restart_aneg(phydev);
> +	} else {

you can remove this else.

> +		return genphy_config_aneg(phydev);
> +	}
> +}
> +
> +static int dp83869_configure_mode(struct phy_device *phydev,
> +				  struct dp83869_private *dp83869)
> +{
> +	int phy_ctrl_val;
> +	int ret;
> +
> +	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
> +	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
> +		return -EINVAL;
> +
> +	/* Below init sequence for each operational mode is defined in
> +	 * section 9.4.8 of the datasheet.
> +	 */
> +	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
> +			    dp83869->mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
> +			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
> +			DP83869_PHY_CTRL_DEFAULT);

() are useless.

> +
> +	switch (dp83869->mode) {
> +	case DP83869_RGMII_COPPER_ETHERNET:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
> +				DP83869_CFG1_DEFAULT);
> +		if (ret)
> +			return ret;
> +
> +		ret = dp83869_configure_rgmii(phydev, dp83869);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_RGMII_SGMII_BRIDGE:
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
> +				     DP83869_SGMII_RGMII_BRIDGE,
> +				     DP83869_SGMII_RGMII_BRIDGE);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +

nit: remove this newline - it is not consistent.

> +		break;
> +	case DP83869_1000M_MEDIA_CONVERT:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_100M_MEDIA_CONVERT:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_SGMII_COPPER_ETHERNET:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +

ditto.

> +		break;
> +	case DP83869_RGMII_100_BASE:
> +	case DP83869_RGMII_1000_BASE:

Does it mean that there is not need to anything to configure?
Comment about it would be good.


> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int dp83869_phy_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	udelay(200);

comment about this delay would be necessary. Just explain where that
200us is coming from.


> +
> +	return 0;
> +}
> +
> +static int dp83869_config_init(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	int ret;
> +
> +	ret = dp83869_phy_reset(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = dp83869_configure_mode(phydev, dp83869);
> +	if (ret)
> +		return ret;
> +
> +	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
> +		dp83869_config_port_mirroring(phydev);
> +
> +	/* Clock output selection if muxing property is set */
> +	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +				       DP83869_IO_MUX_CFG,
> +				       dp83869->clk_output_sel <<
> +				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
> +
> +	dp83869_config_aneg(phydev);


You are not checking return value from this function.




> +
> +	return ret;
> +}
> +
> +static int dp83869_probe(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869;
> +	int ret;
> +
> +	dp83869 = kzalloc(sizeof(*dp83869), GFP_KERNEL);
> +	if (!dp83869)
> +		return -ENOMEM;
> +
> +	phydev->priv = dp83869;
> +
> +	ret = dp83869_of_init(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return dp83869_config_init(phydev);
> +}
> +
> +static struct phy_driver dp83869_driver = {
> +	.name = "TI DP83869",
> +	.uid = DP83869_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_GBIT_FEATURES,
> +	.probe = dp83869_probe,
> +	.config = &dp83869_config_init,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +int phy_dp83869_init(void)
> +{
> +	phy_register(&dp83869_driver);
> +	return 0;
> +}
> diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
> index 11c4c166b2f5..cb261b86b05d 100644
> --- a/drivers/net/phy/ti_phy_init.c
> +++ b/drivers/net/phy/ti_phy_init.c
> @@ -92,6 +92,10 @@ int phy_ti_init(void)
>  	phy_dp83867_init();
>  #endif
>  
> +#ifdef CONFIG_PHY_DP83869
> +	phy_dp83869_init();
> +#endif
> +
>  #ifdef CONFIG_PHY_TI_GENERIC
>  	phy_register(&dp83822_driver);
>  	phy_register(&dp83825s_driver);
> diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
> index 309da2aacccb..f94ff6291a3f 100644
> --- a/drivers/net/phy/ti_phy_init.h
> +++ b/drivers/net/phy/ti_phy_init.h
> @@ -12,5 +12,6 @@
>  #define _TI_GEN_PHY_H
>  
>  int phy_dp83867_init(void);
> +int phy_dp83869_init(void);
>  
>  #endif /* _TI_GEN_PHY_H */
> diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
> new file mode 100644
> index 000000000000..218b1a64e975
> --- /dev/null
> +++ b/include/dt-bindings/net/ti-dp83869.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Device Tree constants for the Texas Instruments DP83869 PHY
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * Copyright:   (C) 2019 Texas Instruments, Inc.
> + */
> +
> +#ifndef _DT_BINDINGS_TI_DP83869_H
> +#define _DT_BINDINGS_TI_DP83869_H
> +
> +/* PHY CTRL bits */
> +#define DP83869_PHYCR_FIFO_DEPTH_3_B_NIB	0x00
> +#define DP83869_PHYCR_FIFO_DEPTH_4_B_NIB	0x01
> +#define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
> +#define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
> +
> +/* IO_MUX_CFG - Clock output selection */
> +#define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
> +#define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
> +#define DP83869_CLK_O_SEL_CHN_C_RCLK		0x2
> +#define DP83869_CLK_O_SEL_CHN_D_RCLK		0x3
> +#define DP83869_CLK_O_SEL_CHN_A_RCLK_DIV5	0x4
> +#define DP83869_CLK_O_SEL_CHN_B_RCLK_DIV5	0x5
> +#define DP83869_CLK_O_SEL_CHN_C_RCLK_DIV5	0x6
> +#define DP83869_CLK_O_SEL_CHN_D_RCLK_DIV5	0x7
> +#define DP83869_CLK_O_SEL_CHN_A_TCLK		0x8
> +#define DP83869_CLK_O_SEL_CHN_B_TCLK		0x9
> +#define DP83869_CLK_O_SEL_CHN_C_TCLK		0xa
> +#define DP83869_CLK_O_SEL_CHN_D_TCLK		0xb
> +#define DP83869_CLK_O_SEL_REF_CLK		0xc
> +
> +#define DP83869_RGMII_COPPER_ETHERNET		0x00
> +#define DP83869_RGMII_1000_BASE			0x01
> +#define DP83869_RGMII_100_BASE			0x02
> +#define DP83869_RGMII_SGMII_BRIDGE		0x03
> +#define DP83869_1000M_MEDIA_CONVERT		0x04
> +#define DP83869_100M_MEDIA_CONVERT		0x05
> +#define DP83869_SGMII_COPPER_ETHERNET		0x06
> +
> +#endif
> 

Thanks,
Michal

[RFC PATCH 2/3] net: phy: Add helper routines to set and clear bits

[Dan Murphy]

Michal

Thanks for the review

On 4/21/20 3:04 AM, Michal Simek wrote:
> On 20. 04. 20 20:53, Dan Murphy wrote:
>> Add phy_set/clear_bit helper routines so that ported drivers from the
>> kernel can use these functions.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/include/phy.h b/include/phy.h
>> index b5de14cbfc29..050c989fa537 100644
>> --- a/include/phy.h
>> +++ b/include/phy.h
>> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad,
>>   	return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
>>   }
>>   
> kernel-doc description would be good.

Just following the files coding practice.? No other APIs have kernel-doc.

Not that I won't add them but it is worth pointing this out.

>
>> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad,
>> +				   u32 regnum, u16 val)
>> +{
>> +	int value;
>> +	int ret;
> nit: on one line should be better.

I was under the impression that when code is side ported to uBoot it 
should be as close to the origin as possible for bug fixes to be easily 
understood and applied.

Please correct my understanding if I had the wrong impression.

Not that this specific nit is going to make a difference but it applies 
to other comments made in the other patches.

Dan

[RFC PATCH 1/3] net: phy: Add a generic phy file for TI generic PHYs

[Dan Murphy]

Michal

On 4/21/20 2:57 AM, Michal Simek wrote:
> On 20. 04. 20 20:53, Dan Murphy wrote:
>> Add a TI Generic init file that will initialize TI PHYs that follow that
>> not require special handling.  These PHYs can connect with the standard
>> MII register set.  This generice file will register the PHY IDs and
>> names of the PHYs so when the command 'mdio list' is executed the PHY
>> name will display as opposed to 'Generic PHY'.
>>
>> The DP8382X PHY series is a generic PHY that requires the generic
>> registration.
>>
>> The DP83867 driver was updated to rename the init to a more PHY specific
>> init call.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> I would personally do it with two patches.

I was going to do 2 patches but I wanted to show the viability of why 
this generic file was needed.? Without the DP8382X PHY added it looks 
like I was just moving things around for the DP83867 and changing names.

I will break it up in v2 (non-RFC if no one has serious objections to this)

>
>> ---
>>   configs/am65x_evm_a53_defconfig      |   2 +-
>>   configs/am65x_hs_evm_a53_defconfig   |   2 +-
>>   configs/dra7xx_evm_defconfig         |   2 +-
>>   configs/dra7xx_hs_evm_defconfig      |   2 +-
>>   configs/dra7xx_hs_evm_usb_defconfig  |   2 +-
>>   configs/j721e_evm_a72_defconfig      |   2 +-
>>   configs/j721e_hs_evm_a72_defconfig   |   2 +-
>>   configs/k2g_evm_defconfig            |   2 +-
>>   configs/xilinx_versal_virt_defconfig |   2 +-
>>   configs/xilinx_zynqmp_virt_defconfig |   2 +-
>>   drivers/net/phy/Kconfig              |   8 ++
>>   drivers/net/phy/Makefile             |   3 +-
>>   drivers/net/phy/dp83867.c            |   3 +-
>>   drivers/net/phy/ti_phy_init.c        | 106 +++++++++++++++++++++++++++
>>   drivers/net/phy/ti_phy_init.h        |  16 ++++
>>   15 files changed, 144 insertions(+), 12 deletions(-)
>>   create mode 100644 drivers/net/phy/ti_phy_init.c
>>   create mode 100644 drivers/net/phy/ti_phy_init.h
>>
>> diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
>> index 542bbd992c53..7051d6c40505 100644
>> --- a/configs/am65x_evm_a53_defconfig
>> +++ b/configs/am65x_evm_a53_defconfig
>> @@ -101,7 +101,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>>   CONFIG_SPI_FLASH_STMICRO=y
>>   # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>>   CONFIG_SPI_FLASH_MTD=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
> Don't know why this name was chosen but don't you want to label it with TI?
>
> CONFIG_PHY_TI_DP83867 ?
>
> Kernel is using different symbol anyway.
> CONFIG_DP83867_PHY
>
ACK.? I will make it CONFIG_PHY_TI_DP83867.? That follows the Kconfig 
uBoot standard for PHYs


>
>>   CONFIG_PHY_FIXED=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_E1000=y
>> diff --git a/configs/am65x_hs_evm_a53_defconfig b/configs/am65x_hs_evm_a53_defconfig
>> index 9f43cee39611..29da3826f12a 100644
>> --- a/configs/am65x_hs_evm_a53_defconfig
>> +++ b/configs/am65x_hs_evm_a53_defconfig
>> @@ -103,7 +103,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>>   CONFIG_SPI_FLASH_STMICRO=y
>>   # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>>   CONFIG_SPI_FLASH_MTD=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_PHY_FIXED=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_E1000=y
>> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
>> index 4d765da4e052..19c024889155 100644
>> --- a/configs/dra7xx_evm_defconfig
>> +++ b/configs/dra7xx_evm_defconfig
>> @@ -86,7 +86,7 @@ CONFIG_DM_SPI_FLASH=y
>>   CONFIG_SF_DEFAULT_MODE=0
>>   CONFIG_SF_DEFAULT_SPEED=76800000
>>   CONFIG_SPI_FLASH_SPANSION=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_PHY_GIGE=y
>>   CONFIG_MII=y
>> diff --git a/configs/dra7xx_hs_evm_defconfig b/configs/dra7xx_hs_evm_defconfig
>> index c25d4ce5c142..e97f1a3ba338 100644
>> --- a/configs/dra7xx_hs_evm_defconfig
>> +++ b/configs/dra7xx_hs_evm_defconfig
>> @@ -89,7 +89,7 @@ CONFIG_DM_SPI_FLASH=y
>>   CONFIG_SF_DEFAULT_MODE=0
>>   CONFIG_SF_DEFAULT_SPEED=76800000
>>   CONFIG_SPI_FLASH_SPANSION=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_PHY_GIGE=y
>>   CONFIG_MII=y
>> diff --git a/configs/dra7xx_hs_evm_usb_defconfig b/configs/dra7xx_hs_evm_usb_defconfig
>> index 8e74496b2ccd..46970e31f02e 100644
>> --- a/configs/dra7xx_hs_evm_usb_defconfig
>> +++ b/configs/dra7xx_hs_evm_usb_defconfig
>> @@ -87,7 +87,7 @@ CONFIG_SF_DEFAULT_MODE=0
>>   CONFIG_SF_DEFAULT_SPEED=76800000
>>   CONFIG_SPI_FLASH_BAR=y
>>   CONFIG_SPI_FLASH_SPANSION=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_PHY_GIGE=y
>>   CONFIG_MII=y
>> diff --git a/configs/j721e_evm_a72_defconfig b/configs/j721e_evm_a72_defconfig
>> index e9e82bb4309d..784a6ff396c3 100644
>> --- a/configs/j721e_evm_a72_defconfig
>> +++ b/configs/j721e_evm_a72_defconfig
>> @@ -124,7 +124,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>>   CONFIG_SPI_FLASH_STMICRO=y
>>   # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>>   CONFIG_SPI_FLASH_MTD=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_PHY_FIXED=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_TI_AM65_CPSW_NUSS=y
>> diff --git a/configs/j721e_hs_evm_a72_defconfig b/configs/j721e_hs_evm_a72_defconfig
>> index a723e2718e5e..dd93a955cefd 100644
>> --- a/configs/j721e_hs_evm_a72_defconfig
>> +++ b/configs/j721e_hs_evm_a72_defconfig
>> @@ -114,7 +114,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>>   CONFIG_SPI_FLASH_STMICRO=y
>>   # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>>   CONFIG_SPI_FLASH_MTD=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_PHY_FIXED=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_TI_AM65_CPSW_NUSS=y
>> diff --git a/configs/k2g_evm_defconfig b/configs/k2g_evm_defconfig
>> index 5abf5faa450e..f47b1cabe9a8 100644
>> --- a/configs/k2g_evm_defconfig
>> +++ b/configs/k2g_evm_defconfig
>> @@ -58,7 +58,7 @@ CONFIG_PHYLIB=y
>>   CONFIG_PHY_MARVELL=y
>>   CONFIG_PHY_MICREL=y
>>   CONFIG_PHY_MICREL_KSZ8XXX=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_DM_ETH=y
>>   CONFIG_MII=y
>>   CONFIG_DRIVER_TI_KEYSTONE_NET=y
>> diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
>> index e8c349261207..eb2a26bc2c7d 100644
>> --- a/configs/xilinx_versal_virt_defconfig
>> +++ b/configs/xilinx_versal_virt_defconfig
>> @@ -61,7 +61,7 @@ CONFIG_SPI_FLASH_WINBOND=y
>>   CONFIG_PHY_MARVELL=y
>>   CONFIG_PHY_NATSEMI=y
>>   CONFIG_PHY_REALTEK=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_PHY_VITESSE=y
>>   CONFIG_PHY_FIXED=y
>>   CONFIG_PHY_GIGE=y
>> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
>> index 7b09edd78e1b..5c320f93fe5f 100644
>> --- a/configs/xilinx_zynqmp_virt_defconfig
>> +++ b/configs/xilinx_zynqmp_virt_defconfig
>> @@ -101,7 +101,7 @@ CONFIG_PHY_MICREL=y
>>   CONFIG_PHY_MICREL_KSZ90X1=y
>>   CONFIG_PHY_NATSEMI=y
>>   CONFIG_PHY_REALTEK=y
>> -CONFIG_PHY_TI=y
>> +CONFIG_PHY_DP83867=y
>>   CONFIG_PHY_VITESSE=y
>>   CONFIG_PHY_XILINX_GMII2RGMII=y
>>   CONFIG_PHY_FIXED=y
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index d1f049e62ab7..e366f10afc59 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -244,6 +244,14 @@ config PHY_TERANETICS
>>   config PHY_TI
>>   	bool "Texas Instruments Ethernet PHYs support"
>>   
>> +config PHY_TI_GENERIC
>> +	select PHY_TI
>> +	bool "Texas Instruments Ethernet PHYs support"
>> +
>> +config PHY_DP83867
>> +	select PHY_TI
>> +	bool "Texas Instruments Ethernet DP83867 PHY support"
>> +
>>   config PHY_VITESSE
>>   	bool "Vitesse Ethernet PHYs support"
>>   
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 1d81516ecd1d..9c6d31718c00 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -25,7 +25,8 @@ obj-$(CONFIG_PHY_NATSEMI) += natsemi.o
>>   obj-$(CONFIG_PHY_REALTEK) += realtek.o
>>   obj-$(CONFIG_PHY_SMSC) += smsc.o
>>   obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>> -obj-$(CONFIG_PHY_TI) += dp83867.o
>> +obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>> +obj-$(CONFIG_PHY_DP83867) += dp83867.o
>>   obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>>   obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>>   obj-$(CONFIG_PHY_VITESSE) += vitesse.o
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index 50804c130efd..c9ed4a44d4db 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -12,6 +12,7 @@
>>   #include <dm.h>
>>   #include <dt-bindings/net/ti-dp83867.h>
>>   
>> +#include "ti_phy_init.h"
>>   
>>   /* TI DP83867 */
>>   #define DP83867_DEVADDR		0x1f
>> @@ -428,7 +429,7 @@ static struct phy_driver DP83867_driver = {
>>   	.shutdown = &genphy_shutdown,
>>   };
>>   
>> -int phy_ti_init(void)
>> +int phy_dp83867_init(void)
>>   {
>>   	phy_register(&DP83867_driver);
>>   	return 0;
>> diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
>> new file mode 100644
>> index 000000000000..11c4c166b2f5
>> --- /dev/null
>> +++ b/drivers/net/phy/ti_phy_init.c
>> @@ -0,0 +1,106 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TI Generic PHY Init to register all TI Ethernet PHYs
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
>> + *
>> + * Copyright (C) 2019 Texas Instruments Inc.
> 2020?

Ack.? I actually started this last year but had to put it aside


>
>> + */
>> +
>> +#include <phy.h>
>> +
>> +#define DP83822_PHY_ID	        0x2000a240
>> +#define DP83825S_PHY_ID		0x2000a140
>> +#define DP83825I_PHY_ID		0x2000a150
>> +#define DP83825CM_PHY_ID	0x2000a160
>> +#define DP83825CS_PHY_ID	0x2000a170
>> +#define DP83826C_PHY_ID		0x2000a130
>> +#define DP83826NC_PHY_ID	0x2000a110
>> +
>> +static struct phy_driver dp83822_driver = {
>> +	.name = "TI DP83822",
>> +	.uid = DP83822_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +static struct phy_driver dp83825s_driver = {
>> +	.name = "TI DP83825S",
>> +	.uid = DP83825S_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +static struct phy_driver dp83825i_driver = {
>> +	.name = "TI DP83825I",
>> +	.uid = DP83825I_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +static struct phy_driver dp83825m_driver = {
>> +	.name = "TI DP83825M",
>> +	.uid = DP83825CM_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +static struct phy_driver dp83825cs_driver = {
>> +	.name = "TI DP83825CS",
>> +	.uid = DP83825CS_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +static struct phy_driver dp83826c_driver = {
>> +	.name = "TI DP83826C",
>> +	.uid = DP83826C_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +static struct phy_driver dp83826nc_driver = {
>> +	.name = "TI DP83826NC",
>> +	.uid = DP83826NC_PHY_ID,
>> +	.mask = 0xfffffff0,
>> +	.features = PHY_BASIC_FEATURES,
>> +	.config = &genphy_config_aneg,
>> +	.startup = &genphy_startup,
>> +	.shutdown = &genphy_shutdown,
>> +};
>> +
>> +int phy_ti_init(void)
>> +{
>> +#ifdef CONFIG_PHY_DP83867
>> +	phy_dp83867_init();
>> +#endif
>> +
>> +#ifdef CONFIG_PHY_TI_GENERIC
>> +	phy_register(&dp83822_driver);
>> +	phy_register(&dp83825s_driver);
>> +	phy_register(&dp83825i_driver);
>> +	phy_register(&dp83825m_driver);
>> +	phy_register(&dp83825cs_driver);
>> +	phy_register(&dp83826c_driver);
>> +	phy_register(&dp83826nc_driver);
>> +#endif
> I would do it in separate patch but up2you.
Same reply as above
>
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
>> new file mode 100644
>> index 000000000000..309da2aacccb
>> --- /dev/null
>> +++ b/drivers/net/phy/ti_phy_init.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * TI Generic Ethernet PHY
>> + * Based on code in sungem_phy.c and (long-removed) gianfar_phy.c
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
>> + *
>> + * Copyright (C) 2019 Texas Instruments Inc.
> 2020


Same reply as above

Dan

[RFC PATCH 2/3] net: phy: Add helper routines to set and clear bits

[Michal Simek]

On 21. 04. 20 13:43, Dan Murphy wrote:
> Michal
> 
> Thanks for the review
> 
> On 4/21/20 3:04 AM, Michal Simek wrote:
>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>> Add phy_set/clear_bit helper routines so that ported drivers from the
>>> kernel can use these functions.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> ? include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++
>>> ? 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/include/phy.h b/include/phy.h
>>> index b5de14cbfc29..050c989fa537 100644
>>> --- a/include/phy.h
>>> +++ b/include/phy.h
>>> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct
>>> phy_device *phydev, int devad,
>>> ????? return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
>>> ? }
>>> ? 
>> kernel-doc description would be good.
> 
> Just following the files coding practice.? No other APIs have kernel-doc.
> 
> Not that I won't add them but it is worth pointing this out.

Not sure if this straight copy from Linux but from phy_init() down all
functions used that.

>>
>>> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int
>>> devad,
>>> +?????????????????? u32 regnum, u16 val)
>>> +{
>>> +??? int value;
>>> +??? int ret;
>> nit: on one line should be better.
> 
> I was under the impression that when code is side ported to uBoot it
> should be as close to the origin as possible for bug fixes to be easily
> understood and applied.
> 
> Please correct my understanding if I had the wrong impression.
> 
> Not that this specific nit is going to make a difference but it applies
> to other comments made in the other patches.

then would be good to fix it in origin source. :-) Even this fix is
quite trivial.

Thanks,
Michal

[RFC PATCH 2/3] net: phy: Add helper routines to set and clear bits

[Dan Murphy]

Michal

On 4/21/20 6:52 AM, Michal Simek wrote:
> On 21. 04. 20 13:43, Dan Murphy wrote:
>> Michal
>>
>> Thanks for the review
>>
>> On 4/21/20 3:04 AM, Michal Simek wrote:
>>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>>> Add phy_set/clear_bit helper routines so that ported drivers from the
>>>> kernel can use these functions.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>  ? include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++
>>>>  ? 1 file changed, 38 insertions(+)
>>>>
>>>> diff --git a/include/phy.h b/include/phy.h
>>>> index b5de14cbfc29..050c989fa537 100644
>>>> --- a/include/phy.h
>>>> +++ b/include/phy.h
>>>> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct
>>>> phy_device *phydev, int devad,
>>>>  ????? return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
>>>>  ? }
>>>>    
>>> kernel-doc description would be good.
>> Just following the files coding practice.? No other APIs have kernel-doc.
>>
>> Not that I won't add them but it is worth pointing this out.
> Not sure if this straight copy from Linux but from phy_init() down all
> functions used that.

Yes they do but the inline functions don't.

I will actually go one better and add the k-doc to the other in-line 
functions above for consistency.


>>>> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int
>>>> devad,
>>>> +?????????????????? u32 regnum, u16 val)
>>>> +{
>>>> +??? int value;
>>>> +??? int ret;
>>> nit: on one line should be better.
>> I was under the impression that when code is side ported to uBoot it
>> should be as close to the origin as possible for bug fixes to be easily
>> understood and applied.
>>
>> Please correct my understanding if I had the wrong impression.
>>
>> Not that this specific nit is going to make a difference but it applies
>> to other comments made in the other patches.
> then would be good to fix it in origin source. :-) Even this fix is
> quite trivial.

I am currently working on bug fixing the upstream DP83869 driver so I 
can add some of the fixes.? I will throw your Reported-by in the commit 
message if you want.

Dan

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Dan Murphy]

Michal

On 4/21/20 3:23 AM, Michal Simek wrote:
> On 20. 04. 20 20:53, Dan Murphy wrote:
>> Port the DP83869 kernel driver.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/Kconfig              |   4 +
>>   drivers/net/phy/Makefile             |   1 +
>>   drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
>>   drivers/net/phy/ti_phy_init.c        |   4 +
>>   drivers/net/phy/ti_phy_init.h        |   1 +
>>   include/dt-bindings/net/ti-dp83869.h |  42 +++
>>   6 files changed, 492 insertions(+)
>>   create mode 100644 drivers/net/phy/dp83869.c
>>   create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index e366f10afc59..5f14bdba0a3b 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>   	select PHY_TI
>>   	bool "Texas Instruments Ethernet DP83867 PHY support"
>>   
>> +config PHY_DP83869
>> +	select PHY_TI
>> +	bool "Texas Instruments Ethernet DP83869 PHY support"
> isn't this a little bit short? I expect checkpatch should be reporting
> issue with it.

Yes but I was following other config flags in this file.


>
>
>> +
>>   config PHY_VITESSE
>>   	bool "Vitesse Ethernet PHYs support"
>>   
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 9c6d31718c00..f8797319154f 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>   obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>   obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>   obj-$(CONFIG_PHY_DP83867) += dp83867.o
>> +obj-$(CONFIG_PHY_DP83869) += dp83869.o
>>   obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>>   obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>>   obj-$(CONFIG_PHY_VITESSE) += vitesse.o
>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
>> new file mode 100644
>> index 000000000000..1eaaea20b37a
>> --- /dev/null
>> +++ b/drivers/net/phy/dp83869.c
>> @@ -0,0 +1,440 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Driver for the Texas Instruments DP83869 PHY
>> + * Copyright (C) 2019 Texas Instruments Inc.
> 2020?

This driver was developed in 2019 and added to the 5.4 kernel so not 
sure we should update the copyright when side porting.

At the very least I will need to add 2019-20

>
>> + */
>> +
>> +#include <common.h>
>> +#include <phy.h>
>> +#include <dm/devres.h>
>> +#include <linux/compat.h>
>> +#include <malloc.h>
>> +
>> +#include <dm.h>
>> +
>> +#include <dt-bindings/net/ti-dp83869.h>
>> +
>> +#include "ti_phy_init.h"
>> +
>> +#define DP83869_PHY_ID		0x2000a0f1
>> +#define DP83869_DEVADDR		0x1f
>> +
>> +#define MII_DP83869_PHYCTRL	0x10
>> +#define MII_DP83869_MICR	0x12
>> +#define MII_DP83869_ISR		0x13
>> +#define DP83869_CTRL		0x1f
>> +#define DP83869_CFG4		0x1e
>> +
>> +/* Extended Registers */
>> +#define DP83869_GEN_CFG3        0x0031
>> +#define DP83869_RGMIICTL	0x0032
>> +#define DP83869_STRAP_STS1	0x006e
>> +#define DP83869_RGMIIDCTL	0x0086
>> +#define DP83869_IO_MUX_CFG	0x0170
>> +#define DP83869_OP_MODE		0x01df
>> +#define DP83869_FX_CTRL		0x0c00
>> +
>> +#define DP83869_SW_RESET	BIT(15)
>> +#define DP83869_SW_RESTART	BIT(14)
>> +
>> +/* MICR Interrupt bits */
>> +#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
>> +#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
>> +#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
>> +
>> +#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
>> +					 BMCR_FULLDPLX | \
>> +					 BMCR_SPEED1000)
>> +
>> +#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
>> +					 ADVERTISE_1000XHALF | \
>> +					 ADVERTISE_1000XFULL | \
>> +					 ADVERTISE_1000XPAUSE | \
>> +					 ADVERTISE_1000XPSE_ASYM)
>> +
>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>> +#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
>> +
>> +/* CFG1 bits */
>> +#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
>> +				 ADVERTISE_1000FULL | \
>> +				 CTL1000_AS_MASTER)
>> +
>> +/* RGMIICTL bits */
>> +#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
>> +#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
>> +
>> +/* STRAP_STS1 bits */
>> +#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
>> +#define DP83869_STRAP_STS1_RESERVED		BIT(11)
>> +#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
>> +
>> +/* PHYCTRL bits */
>> +#define DP83869_RX_FIFO_SHIFT	12
>> +#define DP83869_TX_FIFO_SHIFT	14
>> +
>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>> +#define DP83869_PHY_CTRL_DEFAULT	0x48
>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
>> +#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
>> +
>> +/* RGMIIDCTL bits */
>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
>> +
>> +/* IO_MUX_CFG bits */
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
>> +
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>> +
>> +/* CFG3 bits */
>> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
>> +
>> +/* CFG4 bits */
>> +#define DP83869_INT_OE	BIT(7)
>> +
>> +/* OP MODE */
>> +#define DP83869_OP_MODE_MII			BIT(5)
>> +#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
>> +
>> +enum {
>> +	DP83869_PORT_MIRRORING_KEEP,
>> +	DP83869_PORT_MIRRORING_EN,
>> +	DP83869_PORT_MIRRORING_DIS,
>> +};
> We have met with this in the kernel. Greg was asking us to write exact
> value to all enum entries.
>
Hmm. Can you give me a reference?? I am not doubting you but I would 
like to read that guidance.

That reference will help with an debate I am having in the kernel.


>> +
>> +struct dp83869_private {
>> +	int tx_fifo_depth;
>> +	int rx_fifo_depth;
>> +	int io_impedance;
>> +	int port_mirroring;
>> +	bool rxctrl_strap_quirk;
>> +	int clk_output_sel;
>> +	int mode;
>> +};
> I would prefer kernel-doc format to make reading easier.
> Also tx_fifo_depth doesn't look like signed type. I would make them
> unsigned.
>
>
>> +
>> +static int dp83869_config_port_mirroring(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +
>> +	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
>> +		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +					DP83869_GEN_CFG3,
>> +					DP83869_CFG3_PORT_MIRROR_EN);
>> +	else
> you can remove this else.

Ack


>
>> +		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
>> +					  DP83869_GEN_CFG3,
>> +					  DP83869_CFG3_PORT_MIRROR_EN);
>> +}
>> +
>> +static int dp83869_set_strapped_mode(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	u16 val;
>> +
>> +	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF_MDIO
> Isn't there a way to remove these?
>
> if (IS_ENABLED(CONFIG_OF_MDIO))
> ...
I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change 
this
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
>> +	int ret;
>> +
>> +	if (!of_node)
>> +		return -ENODEV;
> didn't go deep to this but can this happen?

Does every device in the uBoot tree use the DT or do some still use 
board files?

>
>> +
>> +	dp83869->io_impedance = -EINVAL;
> I would prefer to group it together. It means move this before dt reading.
>
>> +
>> +	/* Optional configuration */
>> +	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
>> +				   &dp83869->clk_output_sel);
>> +	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
>> +		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
>> +
>> +	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
>> +	if (ret == 0) {
> !ret
Ack
>
>> +		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
>> +		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
>> +			return -EINVAL;
>> +	} else {
>> +		ret = dp83869_set_strapped_mode(phydev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
>> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
>> +	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
>> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
>> +
>> +	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
>> +		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>> +	} else {
>> +		/* If the lane swap is not in the DT then check the straps */
>> +		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>> +		if (ret < 0)
>> +			return ret;
> newline here please.
Ack
>
>> +		if (ret & DP83869_STRAP_MIRROR_ENABLED)
>> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>> +		else
>> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
>> +	}
>> +
>> +	if (of_property_read_u32(of_node, "rx-fifo-depth",
>> +				 &dp83869->rx_fifo_depth))
>> +		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>> +
>> +	if (of_property_read_u32(of_node, "tx-fifo-depth",
>> +				 &dp83869->tx_fifo_depth))
>> +		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>> +
>> +	return ret;
>> +}
>> +#else
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	return dp83869_set_strapped_mode(phydev);
>> +}
>> +#endif /* CONFIG_OF_MDIO */
>> +
>> +static int dp83869_configure_rgmii(struct phy_device *phydev,
>> +				   struct dp83869_private *dp83869)
>> +{
>> +	int ret = 0, val;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
>> +		if (val < 0)
>> +			return val;
>> +
>> +		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
>> +		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
>> +		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (dp83869->io_impedance >= 0)
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +				       DP83869_IO_MUX_CFG,
>> +				       dp83869->io_impedance &
>> +				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
>> +	return ret;
>> +}
>> +
>> +static int dp83869_config_aneg(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	int err;
>> +
>> +	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
>> +		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
>> +				MII_DP83869_FIBER_ADVERTISE);
>> +		if (err)
>> +			return err;
>> +
>> +		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
>> +				DP83869_SW_RESTART);
>> +		if (err)
>> +			return err;
>> +
>> +		return genphy_restart_aneg(phydev);
>> +	} else {
> you can remove this else.
Ack
>
>> +		return genphy_config_aneg(phydev);
>> +	}
>> +}
>> +
>> +static int dp83869_configure_mode(struct phy_device *phydev,
>> +				  struct dp83869_private *dp83869)
>> +{
>> +	int phy_ctrl_val;
>> +	int ret;
>> +
>> +	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
>> +	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
>> +		return -EINVAL;
>> +
>> +	/* Below init sequence for each operational mode is defined in
>> +	 * section 9.4.8 of the datasheet.
>> +	 */
>> +	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
>> +			    dp83869->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
>> +			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
>> +			DP83869_PHY_CTRL_DEFAULT);
> () are useless.
Ack
>
>> +
>> +	switch (dp83869->mode) {
>> +	case DP83869_RGMII_COPPER_ETHERNET:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
>> +				DP83869_CFG1_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = dp83869_configure_rgmii(phydev, dp83869);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_RGMII_SGMII_BRIDGE:
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
>> +				     DP83869_SGMII_RGMII_BRIDGE,
>> +				     DP83869_SGMII_RGMII_BRIDGE);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
> nit: remove this newline - it is not consistent.
Ack
>
>> +		break;
>> +	case DP83869_1000M_MEDIA_CONVERT:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_100M_MEDIA_CONVERT:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_SGMII_COPPER_ETHERNET:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
> ditto.
Ack
>
>> +		break;
>> +	case DP83869_RGMII_100_BASE:
>> +	case DP83869_RGMII_1000_BASE:
> Does it mean that there is not need to anything to configure?
> Comment about it would be good.

I will add a comment as one of these is actually a fiber connection 
which I have not figured out how uBoot handles fiber connections yet.


>
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int dp83869_phy_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	udelay(200);
> comment about this delay would be necessary. Just explain where that
> 200us is coming from.

Ack.


>
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp83869_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	int ret;
>> +
>> +	ret = dp83869_phy_reset(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dp83869_configure_mode(phydev, dp83869);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
>> +		dp83869_config_port_mirroring(phydev);
>> +
>> +	/* Clock output selection if muxing property is set */
>> +	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +				       DP83869_IO_MUX_CFG,
>> +				       dp83869->clk_output_sel <<
>> +				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
>> +
>> +	dp83869_config_aneg(phydev);
>
> You are not checking return value from this function.

Ack


Dan

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Michal Simek]

On 21. 04. 20 14:04, Dan Murphy wrote:
> Michal
> 
> On 4/21/20 3:23 AM, Michal Simek wrote:
>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>> Port the DP83869 kernel driver.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> ? drivers/net/phy/Kconfig????????????? |?? 4 +
>>> ? drivers/net/phy/Makefile???????????? |?? 1 +
>>> ? drivers/net/phy/dp83869.c??????????? | 440 +++++++++++++++++++++++++++
>>> ? drivers/net/phy/ti_phy_init.c??????? |?? 4 +
>>> ? drivers/net/phy/ti_phy_init.h??????? |?? 1 +
>>> ? include/dt-bindings/net/ti-dp83869.h |? 42 +++
>>> ? 6 files changed, 492 insertions(+)
>>> ? create mode 100644 drivers/net/phy/dp83869.c
>>> ? create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>>
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index e366f10afc59..5f14bdba0a3b 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>> ????? select PHY_TI
>>> ????? bool "Texas Instruments Ethernet DP83867 PHY support"
>>> ? +config PHY_DP83869
>>> +??? select PHY_TI
>>> +??? bool "Texas Instruments Ethernet DP83869 PHY support"
>> isn't this a little bit short? I expect checkpatch should be reporting
>> issue with it.
> 
> Yes but I was following other config flags in this file.

Just no reason to follow bad examples. :-)



>>
>>
>>> +
>>> ? config PHY_VITESSE
>>> ????? bool "Vitesse Ethernet PHYs support"
>>> ? diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>>> index 9c6d31718c00..f8797319154f 100644
>>> --- a/drivers/net/phy/Makefile
>>> +++ b/drivers/net/phy/Makefile
>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>> ? obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>> ? obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>> ? obj-$(CONFIG_PHY_DP83867) += dp83867.o
>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.o
>>> ? obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>>> ? obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>>> ? obj-$(CONFIG_PHY_VITESSE) += vitesse.o
>>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
>>> new file mode 100644
>>> index 000000000000..1eaaea20b37a
>>> --- /dev/null
>>> +++ b/drivers/net/phy/dp83869.c
>>> @@ -0,0 +1,440 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Driver for the Texas Instruments DP83869 PHY
>>> + * Copyright (C) 2019 Texas Instruments Inc.
>> 2020?
> 
> This driver was developed in 2019 and added to the 5.4 kernel so not
> sure we should update the copyright when side porting.
> 
> At the very least I will need to add 2019-20

yup.

> 
>>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <phy.h>
>>> +#include <dm/devres.h>
>>> +#include <linux/compat.h>
>>> +#include <malloc.h>
>>> +
>>> +#include <dm.h>
>>> +
>>> +#include <dt-bindings/net/ti-dp83869.h>
>>> +
>>> +#include "ti_phy_init.h"
>>> +
>>> +#define DP83869_PHY_ID??????? 0x2000a0f1
>>> +#define DP83869_DEVADDR??????? 0x1f
>>> +
>>> +#define MII_DP83869_PHYCTRL??? 0x10
>>> +#define MII_DP83869_MICR??? 0x12
>>> +#define MII_DP83869_ISR??????? 0x13
>>> +#define DP83869_CTRL??????? 0x1f
>>> +#define DP83869_CFG4??????? 0x1e
>>> +
>>> +/* Extended Registers */
>>> +#define DP83869_GEN_CFG3??????? 0x0031
>>> +#define DP83869_RGMIICTL??? 0x0032
>>> +#define DP83869_STRAP_STS1??? 0x006e
>>> +#define DP83869_RGMIIDCTL??? 0x0086
>>> +#define DP83869_IO_MUX_CFG??? 0x0170
>>> +#define DP83869_OP_MODE??????? 0x01df
>>> +#define DP83869_FX_CTRL??????? 0x0c00
>>> +
>>> +#define DP83869_SW_RESET??? BIT(15)
>>> +#define DP83869_SW_RESTART??? BIT(14)
>>> +
>>> +/* MICR Interrupt bits */
>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN??????? BIT(15)
>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN??? BIT(14)
>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN??? BIT(13)
>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN??? BIT(12)
>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN??? BIT(11)
>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN??? BIT(10)
>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN??? BIT(8)
>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN??? BIT(4)
>>> +#define MII_DP83869_MICR_WOL_INT_EN??????? BIT(3)
>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN??? BIT(2)
>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN??? BIT(1)
>>> +#define MII_DP83869_MICR_JABBER_INT_EN??????? BIT(0)
>>> +
>>> +#define MII_DP83869_BMCR_DEFAULT??? (BMCR_ANENABLE | \
>>> +???????????????????? BMCR_FULLDPLX | \
>>> +???????????????????? BMCR_SPEED1000)
>>> +
>>> +#define MII_DP83869_FIBER_ADVERTISE??? (ADVERTISE_CSMA | \
>>> +???????????????????? ADVERTISE_1000XHALF | \
>>> +???????????????????? ADVERTISE_1000XFULL | \
>>> +???????????????????? ADVERTISE_1000XPAUSE | \
>>> +???????????????????? ADVERTISE_1000XPSE_ASYM)
>>> +
>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>>> +#define DP83869_FX_CTRL_DEFAULT??? MII_DP83869_BMCR_DEFAULT
>>> +
>>> +/* CFG1 bits */
>>> +#define DP83869_CFG1_DEFAULT??? (ADVERTISE_1000HALF | \
>>> +???????????????? ADVERTISE_1000FULL | \
>>> +???????????????? CTL1000_AS_MASTER)
>>> +
>>> +/* RGMIICTL bits */
>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN??????? BIT(1)
>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN??????? BIT(0)
>>> +
>>> +/* STRAP_STS1 bits */
>>> +#define DP83869_STRAP_OP_MODE_MASK??????? GENMASK(2, 0)
>>> +#define DP83869_STRAP_STS1_RESERVED??????? BIT(11)
>>> +#define DP83869_STRAP_MIRROR_ENABLED??????? BIT(12)
>>> +
>>> +/* PHYCTRL bits */
>>> +#define DP83869_RX_FIFO_SHIFT??? 12
>>> +#define DP83869_TX_FIFO_SHIFT??? 14
>>> +
>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>>> +#define DP83869_PHY_CTRL_DEFAULT??? 0x48
>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK??? GENMASK(15, 12)
>>> +#define DP83869_PHYCR_RESERVED_MASK??? BIT(11)
>>> +
>>> +/* RGMIIDCTL bits */
>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT??? 4
>>> +
>>> +/* IO_MUX_CFG bits */
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL??? 0x1f
>>> +
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX??? 0x0
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN??? 0x1f
>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK??? (0x1f << 8)
>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT??? 8
>>> +
>>> +/* CFG3 bits */
>>> +#define DP83869_CFG3_PORT_MIRROR_EN????????????? BIT(0)
>>> +
>>> +/* CFG4 bits */
>>> +#define DP83869_INT_OE??? BIT(7)
>>> +
>>> +/* OP MODE */
>>> +#define DP83869_OP_MODE_MII??????????? BIT(5)
>>> +#define DP83869_SGMII_RGMII_BRIDGE??????? BIT(6)
>>> +
>>> +enum {
>>> +??? DP83869_PORT_MIRRORING_KEEP,
>>> +??? DP83869_PORT_MIRRORING_EN,
>>> +??? DP83869_PORT_MIRRORING_DIS,
>>> +};
>> We have met with this in the kernel. Greg was asking us to write exact
>> value to all enum entries.
>>
> Hmm. Can you give me a reference?? I am not doubting you but I would
> like to read that guidance.
> 
> That reference will help with an debate I am having in the kernel.

Take a look at this thread.
https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094 at kroah.com/

 <snip>

>>> +
>>> +#ifdef CONFIG_OF_MDIO
>> Isn't there a way to remove these?
>>
>> if (IS_ENABLED(CONFIG_OF_MDIO))
>> ...
> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change
> this

There are a lot of places which should be update/done better.


>>> +static int dp83869_of_init(struct phy_device *phydev)
>>> +{
>>> +??? struct dp83869_private *dp83869 = phydev->priv;
>>> +??? struct device *dev = &phydev->mdio.dev;
>>> +??? struct device_node *of_node = dev->of_node;
>>> +??? int ret;
>>> +
>>> +??? if (!of_node)
>>> +??????? return -ENODEV;
>> didn't go deep to this but can this happen?
> 
> Does every device in the uBoot tree use the DT or do some still use
> board files?

IIRC ethernet phys are not based on driver model that's why devices are
not created for it and I am not quite sure if platdata are supported.

I think question is what way you use. If you use just OF_MDIO/DM_ETH
then Kconfig should have dependencies. And if someone else want to run
it without it (which is IMHO unlikely) then they can update the driver self.

>>
>>> +
>>> +??? dp83869->io_impedance = -EINVAL;
>> I would prefer to group it together. It means move this before dt
>> reading.
>>

No reaction on this line that's why just commenting it that you spot it.

Thanks,
Michal

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Dan Murphy]

Michal

On 4/21/20 7:39 AM, Michal Simek wrote:
> On 21. 04. 20 14:04, Dan Murphy wrote:
>> Michal
>>
>> On 4/21/20 3:23 AM, Michal Simek wrote:
>>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>>> Port the DP83869 kernel driver.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>  ? drivers/net/phy/Kconfig????????????? |?? 4 +
>>>>  ? drivers/net/phy/Makefile???????????? |?? 1 +
>>>>  ? drivers/net/phy/dp83869.c??????????? | 440 +++++++++++++++++++++++++++
>>>>  ? drivers/net/phy/ti_phy_init.c??????? |?? 4 +
>>>>  ? drivers/net/phy/ti_phy_init.h??????? |?? 1 +
>>>>  ? include/dt-bindings/net/ti-dp83869.h |? 42 +++
>>>>  ? 6 files changed, 492 insertions(+)
>>>>  ? create mode 100644 drivers/net/phy/dp83869.c
>>>>  ? create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>>>
>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>>> index e366f10afc59..5f14bdba0a3b 100644
>>>> --- a/drivers/net/phy/Kconfig
>>>> +++ b/drivers/net/phy/Kconfig
>>>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>>>  ????? select PHY_TI
>>>>  ????? bool "Texas Instruments Ethernet DP83867 PHY support"
>>>>  ? +config PHY_DP83869
>>>> +??? select PHY_TI
>>>> +??? bool "Texas Instruments Ethernet DP83869 PHY support"
>>> isn't this a little bit short? I expect checkpatch should be reporting
>>> issue with it.
>> Yes but I was following other config flags in this file.
> Just no reason to follow bad examples. :-)

True.

I will update the examples for the TI PHYs.


>
>
>
>>>
>>>> +
>>>>  ? config PHY_VITESSE
>>>>  ????? bool "Vitesse Ethernet PHYs support"
>>>>  ? diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>>>> index 9c6d31718c00..f8797319154f 100644
>>>> --- a/drivers/net/phy/Makefile
>>>> +++ b/drivers/net/phy/Makefile
>>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>>>  ? obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>>>  ? obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>>>  ? obj-$(CONFIG_PHY_DP83867) += dp83867.o
>>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.o
>>>>  ? obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>>>>  ? obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>>>>  ? obj-$(CONFIG_PHY_VITESSE) += vitesse.o
>>>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
>>>> new file mode 100644
>>>> index 000000000000..1eaaea20b37a
>>>> --- /dev/null
>>>> +++ b/drivers/net/phy/dp83869.c
>>>> @@ -0,0 +1,440 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/* Driver for the Texas Instruments DP83869 PHY
>>>> + * Copyright (C) 2019 Texas Instruments Inc.
>>> 2020?
>> This driver was developed in 2019 and added to the 5.4 kernel so not
>> sure we should update the copyright when side porting.
>>
>> At the very least I will need to add 2019-20
> yup.


Ack

>
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <phy.h>
>>>> +#include <dm/devres.h>
>>>> +#include <linux/compat.h>
>>>> +#include <malloc.h>
>>>> +
>>>> +#include <dm.h>
>>>> +
>>>> +#include <dt-bindings/net/ti-dp83869.h>
>>>> +
>>>> +#include "ti_phy_init.h"
>>>> +
>>>> +#define DP83869_PHY_ID??????? 0x2000a0f1
>>>> +#define DP83869_DEVADDR??????? 0x1f
>>>> +
>>>> +#define MII_DP83869_PHYCTRL??? 0x10
>>>> +#define MII_DP83869_MICR??? 0x12
>>>> +#define MII_DP83869_ISR??????? 0x13
>>>> +#define DP83869_CTRL??????? 0x1f
>>>> +#define DP83869_CFG4??????? 0x1e
>>>> +
>>>> +/* Extended Registers */
>>>> +#define DP83869_GEN_CFG3??????? 0x0031
>>>> +#define DP83869_RGMIICTL??? 0x0032
>>>> +#define DP83869_STRAP_STS1??? 0x006e
>>>> +#define DP83869_RGMIIDCTL??? 0x0086
>>>> +#define DP83869_IO_MUX_CFG??? 0x0170
>>>> +#define DP83869_OP_MODE??????? 0x01df
>>>> +#define DP83869_FX_CTRL??????? 0x0c00
>>>> +
>>>> +#define DP83869_SW_RESET??? BIT(15)
>>>> +#define DP83869_SW_RESTART??? BIT(14)
>>>> +
>>>> +/* MICR Interrupt bits */
>>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN??????? BIT(15)
>>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN??? BIT(14)
>>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN??? BIT(13)
>>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN??? BIT(12)
>>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN??? BIT(11)
>>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN??? BIT(10)
>>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN??? BIT(8)
>>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN??? BIT(4)
>>>> +#define MII_DP83869_MICR_WOL_INT_EN??????? BIT(3)
>>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN??? BIT(2)
>>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN??? BIT(1)
>>>> +#define MII_DP83869_MICR_JABBER_INT_EN??????? BIT(0)
>>>> +
>>>> +#define MII_DP83869_BMCR_DEFAULT??? (BMCR_ANENABLE | \
>>>> +???????????????????? BMCR_FULLDPLX | \
>>>> +???????????????????? BMCR_SPEED1000)
>>>> +
>>>> +#define MII_DP83869_FIBER_ADVERTISE??? (ADVERTISE_CSMA | \
>>>> +???????????????????? ADVERTISE_1000XHALF | \
>>>> +???????????????????? ADVERTISE_1000XFULL | \
>>>> +???????????????????? ADVERTISE_1000XPAUSE | \
>>>> +???????????????????? ADVERTISE_1000XPSE_ASYM)
>>>> +
>>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>>>> +#define DP83869_FX_CTRL_DEFAULT??? MII_DP83869_BMCR_DEFAULT
>>>> +
>>>> +/* CFG1 bits */
>>>> +#define DP83869_CFG1_DEFAULT??? (ADVERTISE_1000HALF | \
>>>> +???????????????? ADVERTISE_1000FULL | \
>>>> +???????????????? CTL1000_AS_MASTER)
>>>> +
>>>> +/* RGMIICTL bits */
>>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN??????? BIT(1)
>>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN??????? BIT(0)
>>>> +
>>>> +/* STRAP_STS1 bits */
>>>> +#define DP83869_STRAP_OP_MODE_MASK??????? GENMASK(2, 0)
>>>> +#define DP83869_STRAP_STS1_RESERVED??????? BIT(11)
>>>> +#define DP83869_STRAP_MIRROR_ENABLED??????? BIT(12)
>>>> +
>>>> +/* PHYCTRL bits */
>>>> +#define DP83869_RX_FIFO_SHIFT??? 12
>>>> +#define DP83869_TX_FIFO_SHIFT??? 14
>>>> +
>>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>>>> +#define DP83869_PHY_CTRL_DEFAULT??? 0x48
>>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK??? GENMASK(15, 12)
>>>> +#define DP83869_PHYCR_RESERVED_MASK??? BIT(11)
>>>> +
>>>> +/* RGMIIDCTL bits */
>>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT??? 4
>>>> +
>>>> +/* IO_MUX_CFG bits */
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL??? 0x1f
>>>> +
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX??? 0x0
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN??? 0x1f
>>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK??? (0x1f << 8)
>>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT??? 8
>>>> +
>>>> +/* CFG3 bits */
>>>> +#define DP83869_CFG3_PORT_MIRROR_EN????????????? BIT(0)
>>>> +
>>>> +/* CFG4 bits */
>>>> +#define DP83869_INT_OE??? BIT(7)
>>>> +
>>>> +/* OP MODE */
>>>> +#define DP83869_OP_MODE_MII??????????? BIT(5)
>>>> +#define DP83869_SGMII_RGMII_BRIDGE??????? BIT(6)
>>>> +
>>>> +enum {
>>>> +??? DP83869_PORT_MIRRORING_KEEP,
>>>> +??? DP83869_PORT_MIRRORING_EN,
>>>> +??? DP83869_PORT_MIRRORING_DIS,
>>>> +};
>>> We have met with this in the kernel. Greg was asking us to write exact
>>> value to all enum entries.
>>>
>> Hmm. Can you give me a reference?? I am not doubting you but I would
>> like to read that guidance.
>>
>> That reference will help with an debate I am having in the kernel.
> Take a look at this thread.
> https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094 at kroah.com/
Thank you
>   <snip>
>
>>>> +
>>>> +#ifdef CONFIG_OF_MDIO
>>> Isn't there a way to remove these?
>>>
>>> if (IS_ENABLED(CONFIG_OF_MDIO))
>>> ...
>> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change
>> this
> There are a lot of places which should be update/done better.
Are you inferring that this is not correct either?
>
>
>>>> +static int dp83869_of_init(struct phy_device *phydev)
>>>> +{
>>>> +??? struct dp83869_private *dp83869 = phydev->priv;
>>>> +??? struct device *dev = &phydev->mdio.dev;
>>>> +??? struct device_node *of_node = dev->of_node;
>>>> +??? int ret;
>>>> +
>>>> +??? if (!of_node)
>>>> +??????? return -ENODEV;
>>> didn't go deep to this but can this happen?
>> Does every device in the uBoot tree use the DT or do some still use
>> board files?
> IIRC ethernet phys are not based on driver model that's why devices are
> not created for it and I am not quite sure if platdata are supported.
>
> I think question is what way you use. If you use just OF_MDIO/DM_ETH
> then Kconfig should have dependencies. And if someone else want to run
> it without it (which is IMHO unlikely) then they can update the driver self.

Well technically some phys like this one may not need DT at all if 
strapped in hardware.


>>>> +
>>>> +??? dp83869->io_impedance = -EINVAL;
>>> I would prefer to group it together. It means move this before dt
>>> reading.
>>>
> No reaction on this line that's why just commenting it that you spot it.

I had to look at it in detail.? Not sure why this was set there. This 
should be removed

Dan


> Thanks,
> Michal
>
>
>

[RFC PATCH 1/3] net: phy: Add a generic phy file for TI generic PHYs

[Grygorii Strashko]

On 21/04/2020 14:48, Dan Murphy wrote:
> Michal
> 
> On 4/21/20 2:57 AM, Michal Simek wrote:
>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>> Add a TI Generic init file that will initialize TI PHYs that follow that
>>> not require special handling.? These PHYs can connect with the standard
>>> MII register set.? This generice file will register the PHY IDs and
>>> names of the PHYs so when the command 'mdio list' is executed the PHY
>>> name will display as opposed to 'Generic PHY'.
>>>
>>> The DP8382X PHY series is a generic PHY that requires the generic
>>> registration.
>>>
>>> The DP83867 driver was updated to rename the init to a more PHY specific
>>> init call.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> I would personally do it with two patches.
> 
> I was going to do 2 patches but I wanted to show the viability of why this generic file was needed.? Without the DP8382X PHY added it looks like I was just moving things around for the DP83867 and changing names.
> 
> I will break it up in v2 (non-RFC if no one has serious objections to this)
> 
>>
>>> ---
>>> ? configs/am65x_evm_a53_defconfig????? |?? 2 +-
>>> ? configs/am65x_hs_evm_a53_defconfig?? |?? 2 +-
>>> ? configs/dra7xx_evm_defconfig???????? |?? 2 +-
>>> ? configs/dra7xx_hs_evm_defconfig????? |?? 2 +-
>>> ? configs/dra7xx_hs_evm_usb_defconfig? |?? 2 +-
>>> ? configs/j721e_evm_a72_defconfig????? |?? 2 +-
>>> ? configs/j721e_hs_evm_a72_defconfig?? |?? 2 +-
>>> ? configs/k2g_evm_defconfig??????????? |?? 2 +-
>>> ? configs/xilinx_versal_virt_defconfig |?? 2 +-
>>> ? configs/xilinx_zynqmp_virt_defconfig |?? 2 +-
>>> ? drivers/net/phy/Kconfig????????????? |?? 8 ++
>>> ? drivers/net/phy/Makefile???????????? |?? 3 +-
>>> ? drivers/net/phy/dp83867.c??????????? |?? 3 +-
>>> ? drivers/net/phy/ti_phy_init.c??????? | 106 +++++++++++++++++++++++++++
>>> ? drivers/net/phy/ti_phy_init.h??????? |? 16 ++++
>>> ? 15 files changed, 144 insertions(+), 12 deletions(-)
>>> ? create mode 100644 drivers/net/phy/ti_phy_init.c
>>> ? create mode 100644 drivers/net/phy/ti_phy_init.h
>>>
>>> diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
>>> index 542bbd992c53..7051d6c40505 100644
>>> --- a/configs/am65x_evm_a53_defconfig
>>> +++ b/configs/am65x_evm_a53_defconfig
>>> @@ -101,7 +101,7 @@ CONFIG_SPI_FLASH_SFDP_SUPPORT
>>> ? CONFIG_SPI_FLASH_STMICRO=y
>>> ? # CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>>> ? CONFIG_SPI_FLASH_MTD=y
>>> -CONFIG_PHY_TI=y
>>> +CONFIG_PHY_DP83867=y
>> Don't know why this name was chosen but don't you want to label it with TI?
>>
>> CONFIG_PHY_TI_DP83867 ?
>>
>> Kernel is using different symbol anyway.
>> CONFIG_DP83867_PHY
>>
> ACK.? I will make it CONFIG_PHY_TI_DP83867.? That follows the Kconfig uBoot standard for PHYs

yep. It definitely has to be split

Cover?

-- 
Best regards,
grygorii

[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver

[Christian Gmeiner]

Hi Dan

I am interested to see the mainline support for the DP83869 phy. Is
there any progress on this patch or are there any blocker?

Am Di., 21. Apr. 2020 um 16:35 Uhr schrieb Dan Murphy <dmurphy@ti.com>:
>
> Michal
>
> On 4/21/20 7:39 AM, Michal Simek wrote:
> > On 21. 04. 20 14:04, Dan Murphy wrote:
> >> Michal
> >>
> >> On 4/21/20 3:23 AM, Michal Simek wrote:
> >>> On 20. 04. 20 20:53, Dan Murphy wrote:
> >>>> Port the DP83869 kernel driver.
> >>>>
> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >>>> ---
> >>>>    drivers/net/phy/Kconfig              |   4 +
> >>>>    drivers/net/phy/Makefile             |   1 +
> >>>>    drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
> >>>>    drivers/net/phy/ti_phy_init.c        |   4 +
> >>>>    drivers/net/phy/ti_phy_init.h        |   1 +
> >>>>    include/dt-bindings/net/ti-dp83869.h |  42 +++
> >>>>    6 files changed, 492 insertions(+)
> >>>>    create mode 100644 drivers/net/phy/dp83869.c
> >>>>    create mode 100644 include/dt-bindings/net/ti-dp83869.h
> >>>>
> >>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> >>>> index e366f10afc59..5f14bdba0a3b 100644
> >>>> --- a/drivers/net/phy/Kconfig
> >>>> +++ b/drivers/net/phy/Kconfig
> >>>> @@ -252,6 +252,10 @@ config PHY_DP83867
> >>>>        select PHY_TI
> >>>>        bool "Texas Instruments Ethernet DP83867 PHY support"
> >>>>    +config PHY_DP83869
> >>>> +    select PHY_TI
> >>>> +    bool "Texas Instruments Ethernet DP83869 PHY support"
> >>> isn't this a little bit short? I expect checkpatch should be reporting
> >>> issue with it.
> >> Yes but I was following other config flags in this file.
> > Just no reason to follow bad examples. :-)
>
> True.
>
> I will update the examples for the TI PHYs.
>
>
> >
> >
> >
> >>>
> >>>> +
> >>>>    config PHY_VITESSE
> >>>>        bool "Vitesse Ethernet PHYs support"
> >>>>    diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> >>>> index 9c6d31718c00..f8797319154f 100644
> >>>> --- a/drivers/net/phy/Makefile
> >>>> +++ b/drivers/net/phy/Makefile
> >>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
> >>>>    obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
> >>>>    obj-$(CONFIG_PHY_TI) += ti_phy_init.o
> >>>>    obj-$(CONFIG_PHY_DP83867) += dp83867.o
> >>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.o
> >>>>    obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
> >>>>    obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> >>>>    obj-$(CONFIG_PHY_VITESSE) += vitesse.o
> >>>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> >>>> new file mode 100644
> >>>> index 000000000000..1eaaea20b37a
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/phy/dp83869.c
> >>>> @@ -0,0 +1,440 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/* Driver for the Texas Instruments DP83869 PHY
> >>>> + * Copyright (C) 2019 Texas Instruments Inc.
> >>> 2020?
> >> This driver was developed in 2019 and added to the 5.4 kernel so not
> >> sure we should update the copyright when side porting.
> >>
> >> At the very least I will need to add 2019-20
> > yup.
>
>
> Ack
>
> >
> >>>> + */
> >>>> +
> >>>> +#include <common.h>
> >>>> +#include <phy.h>
> >>>> +#include <dm/devres.h>
> >>>> +#include <linux/compat.h>
> >>>> +#include <malloc.h>
> >>>> +
> >>>> +#include <dm.h>
> >>>> +
> >>>> +#include <dt-bindings/net/ti-dp83869.h>
> >>>> +
> >>>> +#include "ti_phy_init.h"
> >>>> +
> >>>> +#define DP83869_PHY_ID        0x2000a0f1
> >>>> +#define DP83869_DEVADDR        0x1f
> >>>> +
> >>>> +#define MII_DP83869_PHYCTRL    0x10
> >>>> +#define MII_DP83869_MICR    0x12
> >>>> +#define MII_DP83869_ISR        0x13
> >>>> +#define DP83869_CTRL        0x1f
> >>>> +#define DP83869_CFG4        0x1e
> >>>> +
> >>>> +/* Extended Registers */
> >>>> +#define DP83869_GEN_CFG3        0x0031
> >>>> +#define DP83869_RGMIICTL    0x0032
> >>>> +#define DP83869_STRAP_STS1    0x006e
> >>>> +#define DP83869_RGMIIDCTL    0x0086
> >>>> +#define DP83869_IO_MUX_CFG    0x0170
> >>>> +#define DP83869_OP_MODE        0x01df
> >>>> +#define DP83869_FX_CTRL        0x0c00
> >>>> +
> >>>> +#define DP83869_SW_RESET    BIT(15)
> >>>> +#define DP83869_SW_RESTART    BIT(14)
> >>>> +
> >>>> +/* MICR Interrupt bits */
> >>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN        BIT(15)
> >>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN    BIT(14)
> >>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN    BIT(13)
> >>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN    BIT(12)
> >>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN    BIT(11)
> >>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN    BIT(10)
> >>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN    BIT(8)
> >>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN    BIT(4)
> >>>> +#define MII_DP83869_MICR_WOL_INT_EN        BIT(3)
> >>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN    BIT(2)
> >>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN    BIT(1)
> >>>> +#define MII_DP83869_MICR_JABBER_INT_EN        BIT(0)
> >>>> +
> >>>> +#define MII_DP83869_BMCR_DEFAULT    (BMCR_ANENABLE | \
> >>>> +                     BMCR_FULLDPLX | \
> >>>> +                     BMCR_SPEED1000)
> >>>> +
> >>>> +#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISE_CSMA | \
> >>>> +                     ADVERTISE_1000XHALF | \
> >>>> +                     ADVERTISE_1000XFULL | \
> >>>> +                     ADVERTISE_1000XPAUSE | \
> >>>> +                     ADVERTISE_1000XPSE_ASYM)
> >>>> +
> >>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
> >>>> +#define DP83869_FX_CTRL_DEFAULT    MII_DP83869_BMCR_DEFAULT
> >>>> +
> >>>> +/* CFG1 bits */
> >>>> +#define DP83869_CFG1_DEFAULT    (ADVERTISE_1000HALF | \
> >>>> +                 ADVERTISE_1000FULL | \
> >>>> +                 CTL1000_AS_MASTER)
> >>>> +
> >>>> +/* RGMIICTL bits */
> >>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN        BIT(1)
> >>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN        BIT(0)
> >>>> +
> >>>> +/* STRAP_STS1 bits */
> >>>> +#define DP83869_STRAP_OP_MODE_MASK        GENMASK(2, 0)
> >>>> +#define DP83869_STRAP_STS1_RESERVED        BIT(11)
> >>>> +#define DP83869_STRAP_MIRROR_ENABLED        BIT(12)
> >>>> +
> >>>> +/* PHYCTRL bits */
> >>>> +#define DP83869_RX_FIFO_SHIFT    12
> >>>> +#define DP83869_TX_FIFO_SHIFT    14
> >>>> +
> >>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
> >>>> +#define DP83869_PHY_CTRL_DEFAULT    0x48
> >>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK    GENMASK(15, 12)
> >>>> +#define DP83869_PHYCR_RESERVED_MASK    BIT(11)
> >>>> +
> >>>> +/* RGMIIDCTL bits */
> >>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT    4
> >>>> +
> >>>> +/* IO_MUX_CFG bits */
> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL    0x1f
> >>>> +
> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX    0x0
> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN    0x1f
> >>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
> >>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
> >>>> +
> >>>> +/* CFG3 bits */
> >>>> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
> >>>> +
> >>>> +/* CFG4 bits */
> >>>> +#define DP83869_INT_OE    BIT(7)
> >>>> +
> >>>> +/* OP MODE */
> >>>> +#define DP83869_OP_MODE_MII            BIT(5)
> >>>> +#define DP83869_SGMII_RGMII_BRIDGE        BIT(6)
> >>>> +
> >>>> +enum {
> >>>> +    DP83869_PORT_MIRRORING_KEEP,
> >>>> +    DP83869_PORT_MIRRORING_EN,
> >>>> +    DP83869_PORT_MIRRORING_DIS,
> >>>> +};
> >>> We have met with this in the kernel. Greg was asking us to write exact
> >>> value to all enum entries.
> >>>
> >> Hmm. Can you give me a reference?  I am not doubting you but I would
> >> like to read that guidance.
> >>
> >> That reference will help with an debate I am having in the kernel.
> > Take a look at this thread.
> > https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094 at kroah.com/
> Thank you
> >   <snip>
> >
> >>>> +
> >>>> +#ifdef CONFIG_OF_MDIO
> >>> Isn't there a way to remove these?
> >>>
> >>> if (IS_ENABLED(CONFIG_OF_MDIO))
> >>> ...
> >> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change
> >> this
> > There are a lot of places which should be update/done better.
> Are you inferring that this is not correct either?
> >
> >
> >>>> +static int dp83869_of_init(struct phy_device *phydev)
> >>>> +{
> >>>> +    struct dp83869_private *dp83869 = phydev->priv;
> >>>> +    struct device *dev = &phydev->mdio.dev;
> >>>> +    struct device_node *of_node = dev->of_node;
> >>>> +    int ret;
> >>>> +
> >>>> +    if (!of_node)
> >>>> +        return -ENODEV;
> >>> didn't go deep to this but can this happen?
> >> Does every device in the uBoot tree use the DT or do some still use
> >> board files?
> > IIRC ethernet phys are not based on driver model that's why devices are
> > not created for it and I am not quite sure if platdata are supported.
> >
> > I think question is what way you use. If you use just OF_MDIO/DM_ETH
> > then Kconfig should have dependencies. And if someone else want to run
> > it without it (which is IMHO unlikely) then they can update the driver self.
>
> Well technically some phys like this one may not need DT at all if
> strapped in hardware.
>
>
> >>>> +
> >>>> +    dp83869->io_impedance = -EINVAL;
> >>> I would prefer to group it together. It means move this before dt
> >>> reading.
> >>>
> > No reaction on this line that's why just commenting it that you spot it.
>
> I had to look at it in detail.  Not sure why this was set there. This
> should be removed
>
> Dan
>
>
> > Thanks,
> > Michal
> >
> >
> >



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy