[PATCH] net: phy: add support for Motorcomm yt8531C phy

[PATCH] net: phy: add support for Motorcomm yt8531C phy

[Furkan Kardame]

From: Peter Geis <pgwipeout@gmail.com>

This patch adds support for Motorcomm YT8531C which is
used in OrangePi 3 LTS, OrangePi 4 LTS and OrangePi 800
Currently being used by Manjaro Arm kernel

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
Signed-off-by: Furkan Kardame <f.kardame@manjaro.org>
---
 drivers/net/phy/motorcomm.c | 90 +++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 7e6ac2c5e..cbc8ef15d 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -10,6 +10,7 @@
 #include <linux/phy.h>
 
 #define PHY_ID_YT8511		0x0000010a
+#define PHY_ID_YT8531		0x4f51e91b
 
 #define YT8511_PAGE_SELECT	0x1e
 #define YT8511_PAGE		0x1f
@@ -38,6 +39,38 @@
 #define YT8511_DELAY_FE_TX_EN	(0xf << 12)
 #define YT8511_DELAY_FE_TX_DIS	(0x2 << 12)
 
+#define YT8531_RGMII_CONFIG1	0xa003
+
+/* TX Gig-E Delay is bits 3:0, default 0x1
+ * TX Fast-E Delay is bits 7:4, default 0xf
+ * RX Delay is bits 13:10, default 0x0
+ * Delay = 150ps * N
+ * On = 2000ps, off = 50ps
+ */
+#define YT8531_DELAY_GE_TX_EN	(0xd << 0)
+#define YT8531_DELAY_GE_TX_DIS	(0x0 << 0)
+#define YT8531_DELAY_FE_TX_EN	(0xd << 4)
+#define YT8531_DELAY_FE_TX_DIS	(0x0 << 4)
+#define YT8531_DELAY_RX_EN	(0xd << 10)
+#define YT8531_DELAY_RX_DIS	(0x0 << 10)
+#define YT8531_DELAY_MASK	(GENMASK(13, 10) | GENMASK(7, 0))
+
+#define YT8531_SYNCE_CFG	0xa012
+
+/* Clk src config is bits 3:1
+ * 3b000 src from pll
+ * 3b001 src from rx_clk
+ * 3b010 src from serdes
+ * 3b011 src from ptp_in
+ * 3b100 src from 25mhz refclk *default*
+ * 3b101 src from 25mhz ssc
+ * Clk rate select is bit 4
+ * 1b0 25mhz clk output *default*
+ * 1b1 125mhz clk output
+ * Clkout enable is bit 6
+ */
+#define YT8531_CLKCFG_125M	(BIT(6) | BIT(4) | (0x0 < 1))
+
 static int yt8511_read_page(struct phy_device *phydev)
 {
 	return __phy_read(phydev, YT8511_PAGE_SELECT);
@@ -111,6 +145,51 @@ static int yt8511_config_init(struct phy_device *phydev)
 	return phy_restore_page(phydev, oldpage, ret);
 }
 
+static int yt8531_config_init(struct phy_device *phydev)
+{
+	int oldpage, ret = 0;
+	unsigned int val;
+
+	oldpage = phy_select_page(phydev, YT8531_RGMII_CONFIG1);
+	if (oldpage < 0)
+		goto err_restore_page;
+
+	/* set rgmii delay mode */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		val = YT8531_DELAY_RX_DIS | YT8531_DELAY_GE_TX_DIS | YT8531_DELAY_FE_TX_DIS;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = YT8531_DELAY_RX_EN | YT8531_DELAY_GE_TX_DIS | YT8531_DELAY_FE_TX_DIS;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = YT8531_DELAY_RX_DIS | YT8531_DELAY_GE_TX_EN | YT8531_DELAY_FE_TX_EN;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val = YT8531_DELAY_RX_EN | YT8531_DELAY_GE_TX_EN | YT8531_DELAY_FE_TX_EN;
+		break;
+	default: /* do not support other modes */
+		ret = -EOPNOTSUPP;
+		goto err_restore_page;
+	}
+
+	ret = __phy_modify(phydev, YT8511_PAGE, YT8531_DELAY_MASK, val);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* set clock mode to 125mhz */
+	ret = __phy_write(phydev, YT8511_PAGE_SELECT, YT8531_SYNCE_CFG);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = __phy_write(phydev, YT8511_PAGE, YT8531_CLKCFG_125M);
+	if (ret < 0)
+		goto err_restore_page;
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static struct phy_driver motorcomm_phy_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
@@ -120,7 +200,16 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= yt8511_read_page,
 		.write_page	= yt8511_write_page,
+	}, {
+		PHY_ID_MATCH_EXACT(PHY_ID_YT8531),
+		.name		= "YT8531 Gigabit Ethernet",
+		.config_init	= yt8531_config_init,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.read_page	= yt8511_read_page,
+		.write_page	= yt8511_write_page,
 	},
+
 };
 
 module_phy_driver(motorcomm_phy_drvs);
@@ -131,6 +220,7 @@ MODULE_LICENSE("GPL");
 
 static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531) },
 	{ /* sentinal */ }
 };
 
-- 
2.37.3

Re: [PATCH] net: phy: add support for Motorcomm yt8531C phy

[Jakub Kicinski]

On Sun,  9 Oct 2022 22:24:05 +0300 Furkan Kardame wrote:
> From: Peter Geis <pgwipeout@gmail.com>
> 
> This patch adds support for Motorcomm YT8531C which is
> used in OrangePi 3 LTS, OrangePi 4 LTS and OrangePi 800
> Currently being used by Manjaro Arm kernel

# Form letter - net-next is closed

We have already sent the networking pull request for 6.1
and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after 6.1-rc1 is cut.

RFC patches sent for review only are obviously welcome at any time.

Re: [PATCH] net: phy: add support for Motorcomm yt8531C phy

[Andrew Lunn]

> +#define YT8531_RGMII_CONFIG1	0xa003
> +
> +/* TX Gig-E Delay is bits 3:0, default 0x1
> + * TX Fast-E Delay is bits 7:4, default 0xf
> + * RX Delay is bits 13:10, default 0x0
> + * Delay = 150ps * N
> + * On = 2000ps, off = 50ps
> + */
> +#define YT8531_DELAY_GE_TX_EN	(0xd << 0)
> +#define YT8531_DELAY_GE_TX_DIS	(0x0 << 0)

The comments above and the value here don't correspond.  These seem to
be enable/disable, which is usually a single bit. Here you have 3 bits
set? And what about the default 0x1?

0xd is 13. 13*150 = 1950, which is about 2000ps?

So YT8531_DELAY_GE_TX_EN is not really enable, it is
YT8531_DELAY_GE_TX_1950_PS, and YT8531_DELAY_GE_TX_DIS should be
YT8531_DELAY_GE_TX_0_PS.

> +#define YT8531_DELAY_FE_TX_EN	(0xd << 4)
> +#define YT8531_DELAY_FE_TX_DIS	(0x0 << 4)

Default is 0xf?

> +#define YT8531_DELAY_RX_EN	(0xd << 10)
> +#define YT8531_DELAY_RX_DIS	(0x0 << 10)
> +#define YT8531_DELAY_MASK	(GENMASK(13, 10) | GENMASK(7, 0))

Please rework these.

> +	ret = __phy_write(phydev, YT8511_PAGE, YT8531_CLKCFG_125M);
> +	if (ret < 0)
> +		goto err_restore_page;

This if statement is pointless.

> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +