diff mbox series

[3/5] net: phy: dp83tg720: Extending support to DP83TG721 PHY

Message ID d75b772038e37452f262b6c2d87796966f92a18e.1726263095.git.a-reyes1@ti.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Extending features on DP83TG720 driver | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 3 maintainers not CCed: pabeni@redhat.com kuba@kernel.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: 'funtion' may be misspelled - perhaps 'function'? WARNING: please, no space before tabs WARNING: please, no spaces at the start of a line
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alvaro (Al-vuh-roe) Reyes Sept. 19, 2024, 9:01 p.m. UTC 
The DP83TG721 is the next revision of the DP83TG720 and will share the
same driver. Added PHY_ID and probe funtion to check which version is
being loaded. 

Signed-off-by: Alvaro (Al-vuh-roe) Reyes <a-reyes1@ti.com>
---
 drivers/net/phy/dp83tg720.c | 183 ++++++++++++++++++++++++++++--------
 1 file changed, 146 insertions(+), 37 deletions(-)

Comments

Andrew Lunn Sept. 19, 2024, 9:26 p.m. UTC | #1 
On Thu, Sep 19, 2024 at 02:01:17PM -0700, Alvaro (Al-vuh-roe) Reyes wrote:
> The DP83TG721 is the next revision of the DP83TG720 and will share the
> same driver. Added PHY_ID and probe funtion to check which version is
> being loaded. 

Please don't mix whitespace changes with real code changes. It makes
it harder to see the real changes which need reviewing.

> +enum DP83TG720_chip_type {
> +	DP83TG720_CS1_1,
> +	DP83TG721_CS1,
> +};
> +
> +struct DP83TG720_private {
> +	int chip;

I _think_ this should be enum DP83TG720_chip_type chip;

> +	bool is_master;

I think this can be removed and replaced with
phydev->master_slave_set. You probably want to mirror it into
phydev->master_slave_get.

phydev->master_slave_state normally contains the result of auto-neg,
but i expect this PHY forces it, so again, you probably want to mirror
it here as well. Test it out with ethtool and make sure it reports
what you expect.

> +	struct DP83TG720_private *DP83TG720;

Upper case is very unusual in mainline. It is generally only used for
CPP macros.

> +static struct phy_driver dp83tg720_driver[] = {
> +    DP83TG720_PHY_DRIVER(DP83TG720_CS_1_1_PHY_ID, "TI DP83TG720CS1.1"),
> +	DP83TG720_PHY_DRIVER(DP83TG721_CS_1_0_PHY_ID, "TI DP83TG721CS1.0"),
> +};

Indentation is messed up here. I expect checkpatch says something
about this?

>  module_phy_driver(dp83tg720_driver);
>  
>  static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
> -	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
> -	{ }
> +    { PHY_ID_MATCH_EXACT(DP83TG720_CS_1_1_PHY_ID) },
> +	{ PHY_ID_MATCH_EXACT(DP83TG721_CS_1_0_PHY_ID) },
> +	{ },

Here as well.

>  };
>  MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);
>  
> +// static struct phy_driver dp83tg720_driver[] = {
> +// {
> +// 	PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID),
> +// 	.name		= "TI DP83TG720",
> +
> +// 	.flags          = PHY_POLL_CABLE_TEST,
> +// 	.config_aneg	= dp83tg720_config_aneg,
> +// 	.read_status	= dp83tg720_read_status,
> +// 	.get_features	= genphy_c45_pma_read_ext_abilities,
> +// 	.config_init	= dp83tg720_config_init,
> +// 	.get_sqi	= dp83tg720_get_sqi,
> +// 	.get_sqi_max	= dp83tg720_get_sqi_max,
> +// 	.cable_test_start = dp83tg720_cable_test_start,
> +// 	.cable_test_get_status = dp83tg720_cable_test_get_status,
> +
> +// 	.suspend	= genphy_suspend,
> +// 	.resume		= genphy_resume,
> +// } };
> +// module_phy_driver(dp83tg720_driver);
> +
> +// static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
> +// 	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
> +// 	{ }
> +// };
> +// MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);

Please don't add code which is commented out.


    Andrew

---
pw-bot: cr
Oleksij Rempel Sept. 23, 2024, 6:42 a.m. UTC | #2 
On Thu, Sep 19, 2024 at 11:26:49PM +0200, Andrew Lunn wrote:
> On Thu, Sep 19, 2024 at 02:01:17PM -0700, Alvaro (Al-vuh-roe) Reyes wrote:
> > The DP83TG721 is the next revision of the DP83TG720 and will share the
> > same driver. Added PHY_ID and probe funtion to check which version is
> > being loaded. 
> 
> Please don't mix whitespace changes with real code changes. It makes
> it harder to see the real changes which need reviewing.
> 
> > +enum DP83TG720_chip_type {
> > +	DP83TG720_CS1_1,
> > +	DP83TG721_CS1,
> > +};
> > +
> > +struct DP83TG720_private {
> > +	int chip;
> 
> I _think_ this should be enum DP83TG720_chip_type chip;
> 
> > +	bool is_master;
> 
> I think this can be removed and replaced with
> phydev->master_slave_set. You probably want to mirror it into
> phydev->master_slave_get.
> 
> phydev->master_slave_state normally contains the result of auto-neg,
> but i expect this PHY forces it, so again, you probably want to mirror
> it here as well. Test it out with ethtool and make sure it reports
> what you expect.

And we will have device-tree based overwrites for the timing role soon,
so there is no reason to store this values in the priv.

Same for RGMII and SGMII modes, DT is already overwriting it with the
phy-mode property.

> > +	struct DP83TG720_private *DP83TG720;
> 
> Upper case is very unusual in mainline. It is generally only used for
> CPP macros.
> 
> > +static struct phy_driver dp83tg720_driver[] = {
> > +    DP83TG720_PHY_DRIVER(DP83TG720_CS_1_1_PHY_ID, "TI DP83TG720CS1.1"),
> > +	DP83TG720_PHY_DRIVER(DP83TG721_CS_1_0_PHY_ID, "TI DP83TG721CS1.0"),
> > +};

I would prefer not to have DP83TG720_PHY_DRIVER() macros. This devices
are not identical. For example: DP83TG721 have HW time stamping,
DP83TG720 don't. As soon as some one will start implementing it, this
macro will be obsolete.

About names: TI DP83TG720CS1.1 and TI DP83TG721CS1.0 are not known
anywhere outside of TI. If you really won't to use this names, please
add comment describing which final products use core variants
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83tg720.c b/drivers/net/phy/dp83tg720.c
index a6f90293aa61..b70802818f3c 100644
--- a/drivers/net/phy/dp83tg720.c
+++ b/drivers/net/phy/dp83tg720.c
@@ -10,8 +10,8 @@ 
 
 #include "open_alliance_helpers.h"
 
-#define DP83TG720_PHY_ID			0x2000a284
-
+#define DP83TG720_CS_1_1_PHY_ID				0x2000a284
+#define DP83TG721_CS_1_0_PHY_ID			0x2000a290
 #define MMD1F							0x1f
 #define MMD1							0x1
 
@@ -21,41 +21,41 @@ 
 #define DP83TG720_LINK_STATUS			BIT(0)
 
 /* TDR Configuration Register (0x1E) */
-#define DP83TG720_TDR_CFG			0x1e
+#define DP83TG720_TDR_CFG				0x1e
 /* 1b = TDR start, 0b = No TDR */
-#define DP83TG720_TDR_START			BIT(15)
+#define DP83TG720_TDR_START				BIT(15)
 /* 1b = TDR auto on link down, 0b = Manual TDR start */
 #define DP83TG720_CFG_TDR_AUTO_RUN		BIT(14)
 /* 1b = TDR done, 0b = TDR in progress */
-#define DP83TG720_TDR_DONE			BIT(1)
+#define DP83TG720_TDR_DONE				BIT(1)
 /* 1b = TDR fail, 0b = TDR success */
-#define DP83TG720_TDR_FAIL			BIT(0)
+#define DP83TG720_TDR_FAIL				BIT(0)
 
-#define DP83TG720_PHY_RESET			0x1f
-#define DP83TG720_HW_RESET			BIT(15)
+#define DP83TG720_PHY_RESET				0x1f
+#define DP83TG720_HW_RESET				BIT(15)
 
-#define DP83TG720_LPS_CFG3			0x18c
+#define DP83TG720_LPS_CFG3				0x18c
 /* Power modes are documented as bit fields but used as values */
 /* Power Mode 0 is Normal mode */
-#define DP83TG720_LPS_CFG3_PWR_MODE_0		BIT(0)
+#define DP83TG720_LPS_CFG3_PWR_MODE_0	BIT(0)
 
 /* Open Aliance 1000BaseT1 compatible HDD.TDR Fault Status Register */
 #define DP83TG720_TDR_FAULT_STATUS		0x30f
 
 /* Register 0x0301: TDR Configuration 2 */
-#define DP83TG720_TDR_CFG2			0x301
+#define DP83TG720_TDR_CFG2				0x301
 
 /* Register 0x0303: TDR Configuration 3 */
-#define DP83TG720_TDR_CFG3			0x303
+#define DP83TG720_TDR_CFG3				0x303
 
 /* Register 0x0304: TDR Configuration 4 */
-#define DP83TG720_TDR_CFG4			0x304
+#define DP83TG720_TDR_CFG4				0x304
 
 /* Register 0x0405: Unknown Register */
 #define DP83TG720_UNKNOWN_0405			0x405
 
 /* Register 0x0576: TDR Master Link Down Control */
-#define DP83TG720_TDR_MASTER_LINK_DOWN		0x576
+#define DP83TG720_TDR_MASTER_LINK_DOWN	0x576
 
 #define DP83TG720_RGMII_DELAY_CTRL		0x602
 /* In RGMII mode, Enable or disable the internal delay for RXD */
@@ -66,11 +66,11 @@ 
 /* Register 0x083F: Unknown Register */
 #define DP83TG720_UNKNOWN_083F			0x83f
 
-#define DP83TG720_SQI_REG_1			0x871
-#define DP83TG720_SQI_OUT_WORST		GENMASK(7, 5)
-#define DP83TG720_SQI_OUT			GENMASK(3, 1)
+#define DP83TG720_SQI_REG_1				0x871
+#define DP83TG720_SQI_OUT_WORST			GENMASK(7, 5)
+#define DP83TG720_SQI_OUT				GENMASK(3, 1)
 
-#define DP83TG720_SQI_MAX			7
+#define DP83TG720_SQI_MAX				7
 
 /* SGMII CTRL Registers/bits */
 #define DP83TG720_SGMII_CTRL			0x0608
@@ -78,6 +78,54 @@ 
 #define DP83TG720_SGMII_AUTO_NEG_EN		BIT(0)
 #define DP83TG720_SGMII_EN				BIT(9)
 
+/* Strap Register/bits */
+#define DP83TG720_STRAP					0x045d
+#define DP83TG720_MASTER_MODE			BIT(5)
+#define DP83TG720_RGMII_IS_EN			BIT(12)
+#define DP83TG720_SGMII_IS_EN			BIT(13)
+#define DP83TG720_RX_SHIFT_EN			BIT(14)
+#define DP83TG720_TX_SHIFT_EN			BIT(15)
+
+enum DP83TG720_chip_type {
+	DP83TG720_CS1_1,
+	DP83TG721_CS1,
+};
+
+struct DP83TG720_private {
+	int chip;
+	bool is_master;
+	bool is_rgmii;
+	bool is_sgmii;
+	bool rx_shift;
+	bool tx_shift;
+};
+
+static int dp83tg720_read_straps(struct phy_device *phydev)
+{
+	struct DP83TG720_private *DP83TG720 = phydev->priv;
+	int strap;
+
+	strap = phy_read_mmd(phydev, MMD1F, DP83TG720_STRAP);
+	if (strap < 0)
+		return strap;
+
+	if (strap & DP83TG720_MASTER_MODE)
+		DP83TG720->is_master = true;
+
+	if (strap & DP83TG720_RGMII_IS_EN)
+		DP83TG720->is_rgmii = true;
+
+	if (strap & DP83TG720_SGMII_IS_EN)
+		DP83TG720->is_sgmii = true;
+
+	if (strap & DP83TG720_RX_SHIFT_EN)
+		DP83TG720->rx_shift = true;
+
+	if (strap & DP83TG720_TX_SHIFT_EN)
+		DP83TG720->tx_shift = true;
+
+	return 0;
+};
 
 /**
  * dp83tg720_cable_test_start - Start the cable test for the DP83TG720 PHY.
@@ -364,32 +412,93 @@  static int dp83tg720_config_init(struct phy_device *phydev)
 	return genphy_c45_pma_baset1_read_master_slave(phydev);
 }
 
-static struct phy_driver dp83tg720_driver[] = {
+static int dp83tg720_probe(struct phy_device *phydev)
 {
-	PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID),
-	.name		= "TI DP83TG720",
-
-	.flags          = PHY_POLL_CABLE_TEST,
-	.config_aneg	= dp83tg720_config_aneg,
-	.read_status	= dp83tg720_read_status,
-	.get_features	= genphy_c45_pma_read_ext_abilities,
-	.config_init	= dp83tg720_config_init,
-	.get_sqi	= dp83tg720_get_sqi,
-	.get_sqi_max	= dp83tg720_get_sqi_max,
-	.cable_test_start = dp83tg720_cable_test_start,
-	.cable_test_get_status = dp83tg720_cable_test_get_status,
-
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
-} };
+	struct DP83TG720_private *DP83TG720;
+	int ret;
+
+	DP83TG720 = devm_kzalloc(&phydev->mdio.dev, sizeof(*DP83TG720),
+			       GFP_KERNEL);
+	if (!DP83TG720)
+		return -ENOMEM;
+
+	phydev->priv = DP83TG720;
+
+	ret = dp83tg720_read_straps(phydev);
+	if (ret)
+		return ret;
+
+	switch (phydev->phy_id) {
+	case DP83TG720_CS_1_1_PHY_ID:
+		DP83TG720->chip = DP83TG720_CS1_1;
+		break;
+	case DP83TG721_CS_1_0_PHY_ID:
+		DP83TG720->chip = DP83TG721_CS1;
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	return dp83tg720_config_init(phydev);
+}
+
+#define DP83TG720_PHY_DRIVER(_id, _name)                                \
+{                                                                       \
+    PHY_ID_MATCH_EXACT(_id),                                            \
+    .name                   = (_name),                                  \
+    .probe                  = dp83tg720_probe,                          \
+    .flags                  = PHY_POLL_CABLE_TEST,                      \
+    .config_aneg            = dp83tg720_config_aneg,                    \
+    .read_status            = dp83tg720_read_status,                    \
+    .get_features           = genphy_c45_pma_read_ext_abilities,        \
+    .config_init            = dp83tg720_config_init,                    \
+    .get_sqi                = dp83tg720_get_sqi,                        \
+    .get_sqi_max            = dp83tg720_get_sqi_max,                    \
+    .cable_test_start       = dp83tg720_cable_test_start,               \
+    .cable_test_get_status  = dp83tg720_cable_test_get_status,          \
+    .suspend                = genphy_suspend,                           \
+    .resume                 = genphy_resume,                            \
+}
+
+static struct phy_driver dp83tg720_driver[] = {
+    DP83TG720_PHY_DRIVER(DP83TG720_CS_1_1_PHY_ID, "TI DP83TG720CS1.1"),
+	DP83TG720_PHY_DRIVER(DP83TG721_CS_1_0_PHY_ID, "TI DP83TG721CS1.0"),
+};
 module_phy_driver(dp83tg720_driver);
 
 static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
-	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
-	{ }
+    { PHY_ID_MATCH_EXACT(DP83TG720_CS_1_1_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(DP83TG721_CS_1_0_PHY_ID) },
+	{ },
 };
 MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);
 
+// static struct phy_driver dp83tg720_driver[] = {
+// {
+// 	PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID),
+// 	.name		= "TI DP83TG720",
+
+// 	.flags          = PHY_POLL_CABLE_TEST,
+// 	.config_aneg	= dp83tg720_config_aneg,
+// 	.read_status	= dp83tg720_read_status,
+// 	.get_features	= genphy_c45_pma_read_ext_abilities,
+// 	.config_init	= dp83tg720_config_init,
+// 	.get_sqi	= dp83tg720_get_sqi,
+// 	.get_sqi_max	= dp83tg720_get_sqi_max,
+// 	.cable_test_start = dp83tg720_cable_test_start,
+// 	.cable_test_get_status = dp83tg720_cable_test_get_status,
+
+// 	.suspend	= genphy_suspend,
+// 	.resume		= genphy_resume,
+// } };
+// module_phy_driver(dp83tg720_driver);
+
+// static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
+// 	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
+// 	{ }
+// };
+// MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);
+
 MODULE_DESCRIPTION("Texas Instruments DP83TG720 PHY driver");
 MODULE_AUTHOR("Oleksij Rempel <kernel@pengutronix.de>");
 MODULE_LICENSE("GPL");