[PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Chukun Pan]

The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
This patch adds a compatible string for the required clock.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
index 42fb72b6909d..36b1e82212e7 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
@@ -68,6 +68,7 @@ properties:
         - mac_clk_rx
         - aclk_mac
         - pclk_mac
+        - pclk_xpcs
         - clk_mac_ref
         - clk_mac_refout
         - clk_mac_speed
@@ -90,6 +91,11 @@ properties:
       The phandle of the syscon node for the peripheral general register file.
     $ref: /schemas/types.yaml#/definitions/phandle
 
+  rockchip,xpcs:
+    description:
+      The phandle of the syscon node for the peripheral general register file.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
   tx_delay:
     description: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default.
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.25.1

[PATCH 2/2] ethernet: stmicro: stmmac: Add SGMII/QSGMII support for RK3568

[Chukun Pan]

From: David Wu <david.wu@rock-chips.com>

The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
This patch adds the remaining SGMII/QSGMII support.

Run-tested-on: Ariaboard Photonicat (GMAC0 SGMII)

Signed-off-by: David Wu <david.wu@rock-chips.com>
[rebase, rewrite commit message]
Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 210 +++++++++++++++++-
 1 file changed, 207 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 6656d76b6766..c65cb92bb630 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -11,6 +11,7 @@
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/phy.h>
+#include <linux/phy/phy.h>
 #include <linux/of_net.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
@@ -30,6 +31,8 @@ struct rk_gmac_ops {
 	void (*set_to_rgmii)(struct rk_priv_data *bsp_priv,
 			     int tx_delay, int rx_delay);
 	void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
+	void (*set_to_sgmii)(struct rk_priv_data *bsp_priv);
+	void (*set_to_qsgmii)(struct rk_priv_data *bsp_priv);
 	void (*set_rgmii_speed)(struct rk_priv_data *bsp_priv, int speed);
 	void (*set_rmii_speed)(struct rk_priv_data *bsp_priv, int speed);
 	void (*set_clock_selection)(struct rk_priv_data *bsp_priv, bool input,
@@ -60,6 +63,7 @@ struct rk_priv_data {
 	struct clk *clk_mac_speed;
 	struct clk *aclk_mac;
 	struct clk *pclk_mac;
+	struct clk *pclk_xpcs;
 	struct clk *clk_phy;
 
 	struct reset_control *phy_reset;
@@ -69,6 +73,7 @@ struct rk_priv_data {
 
 	struct regmap *grf;
 	struct regmap *php_grf;
+	struct regmap *xpcs;
 };
 
 #define HIWORD_UPDATE(val, mask, shift) \
@@ -81,6 +86,128 @@ struct rk_priv_data {
 	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
 	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
 
+/* XPCS */
+#define XPCS_APB_INCREMENT		(0x4)
+#define XPCS_APB_MASK			GENMASK_ULL(20, 0)
+
+#define SR_MII_BASE			(0x1F0000)
+#define SR_MII1_BASE			(0x1A0000)
+
+#define VR_MII_DIG_CTRL1		(0x8000)
+#define VR_MII_AN_CTRL			(0x8001)
+#define VR_MII_AN_INTR_STS		(0x8002)
+#define VR_MII_LINK_TIMER_CTRL		(0x800A)
+
+#define SR_MII_CTRL_AN_ENABLE		\
+	(BMCR_ANENABLE | BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000)
+#define MII_MAC_AUTO_SW			(0x0200)
+#define PCS_MODE_OFFSET			(0x1)
+#define MII_AN_INTR_EN			(0x1)
+#define PCS_SGMII_MODE			(0x2 << PCS_MODE_OFFSET)
+#define PCS_QSGMII_MODE			(0X3 << PCS_MODE_OFFSET)
+#define VR_MII_CTRL_SGMII_AN_EN		(PCS_SGMII_MODE | MII_AN_INTR_EN)
+#define VR_MII_CTRL_QSGMII_AN_EN	(PCS_QSGMII_MODE | MII_AN_INTR_EN)
+
+#define SR_MII_OFFSET(_x) ({		\
+	typeof(_x) (x) = (_x); \
+	(((x) == 0) ? SR_MII_BASE : (SR_MII1_BASE + ((x) - 1) * 0x10000)); \
+}) \
+
+static int xpcs_read(void *priv, int reg)
+{
+	struct rk_priv_data *bsp_priv = (struct rk_priv_data *)priv;
+	int ret, val;
+
+	ret = regmap_read(bsp_priv->xpcs,
+			  (u32)(reg * XPCS_APB_INCREMENT) & XPCS_APB_MASK,
+			  &val);
+	if (ret)
+		return ret;
+
+	return val;
+}
+
+static int xpcs_write(void *priv, int reg, u16 value)
+{
+	struct rk_priv_data *bsp_priv = (struct rk_priv_data *)priv;
+
+	return regmap_write(bsp_priv->xpcs,
+			    (reg * XPCS_APB_INCREMENT) & XPCS_APB_MASK, value);
+}
+
+static int xpcs_poll_reset(struct rk_priv_data *bsp_priv, int dev)
+{
+	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
+	unsigned int retries = 12;
+	int ret;
+
+	do {
+		msleep(50);
+		ret = xpcs_read(bsp_priv, SR_MII_OFFSET(dev) + MDIO_CTRL1);
+		if (ret < 0)
+			return ret;
+	} while (ret & MDIO_CTRL1_RESET && --retries);
+
+	return (ret & MDIO_CTRL1_RESET) ? -ETIMEDOUT : 0;
+}
+
+static int xpcs_soft_reset(struct rk_priv_data *bsp_priv, int dev)
+{
+	int ret;
+
+	ret = xpcs_write(bsp_priv, SR_MII_OFFSET(dev) + MDIO_CTRL1,
+			 MDIO_CTRL1_RESET);
+	if (ret < 0)
+		return ret;
+
+	return xpcs_poll_reset(bsp_priv, dev);
+}
+
+static int xpcs_setup(struct rk_priv_data *bsp_priv, int mode)
+{
+	int ret, i, idx = bsp_priv->id;
+	u32 val;
+
+	if (mode == PHY_INTERFACE_MODE_QSGMII && idx > 0)
+		return 0;
+
+	ret = xpcs_soft_reset(bsp_priv, idx);
+	if (ret) {
+		dev_err(&bsp_priv->pdev->dev, "xpcs_soft_reset fail %d\n", ret);
+		return ret;
+	}
+
+	xpcs_write(bsp_priv, SR_MII_OFFSET(0) + VR_MII_AN_INTR_STS, 0x0);
+	xpcs_write(bsp_priv, SR_MII_OFFSET(0) + VR_MII_LINK_TIMER_CTRL, 0x1);
+
+	if (mode == PHY_INTERFACE_MODE_SGMII)
+		xpcs_write(bsp_priv, SR_MII_OFFSET(0) + VR_MII_AN_CTRL,
+			   VR_MII_CTRL_SGMII_AN_EN);
+	else
+		xpcs_write(bsp_priv, SR_MII_OFFSET(0) + VR_MII_AN_CTRL,
+			   VR_MII_CTRL_QSGMII_AN_EN);
+
+	if (mode == PHY_INTERFACE_MODE_QSGMII) {
+		for (i = 0; i < 4; i++) {
+			val = xpcs_read(bsp_priv,
+					SR_MII_OFFSET(i) + VR_MII_DIG_CTRL1);
+			xpcs_write(bsp_priv,
+				   SR_MII_OFFSET(i) + VR_MII_DIG_CTRL1,
+				   val | MII_MAC_AUTO_SW);
+			xpcs_write(bsp_priv, SR_MII_OFFSET(i) + MII_BMCR,
+				   SR_MII_CTRL_AN_ENABLE);
+		}
+	} else {
+		val = xpcs_read(bsp_priv, SR_MII_OFFSET(idx) + VR_MII_DIG_CTRL1);
+		xpcs_write(bsp_priv, SR_MII_OFFSET(idx) + VR_MII_DIG_CTRL1,
+			   val | MII_MAC_AUTO_SW);
+		xpcs_write(bsp_priv, SR_MII_OFFSET(idx) + MII_BMCR,
+			   SR_MII_CTRL_AN_ENABLE);
+	}
+
+	return ret;
+}
+
 #define PX30_GRF_GMAC_CON1		0x0904
 
 /* PX30_GRF_GMAC_CON1 */
@@ -1008,6 +1135,7 @@ static const struct rk_gmac_ops rk3399_ops = {
 #define RK3568_GRF_GMAC1_CON1		0x038c
 
 /* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
+#define RK3568_GMAC_GMII_MODE			GRF_BIT(7)
 #define RK3568_GMAC_PHY_INTF_SEL_RGMII	\
 		(GRF_BIT(4) | GRF_CLR_BIT(5) | GRF_CLR_BIT(6))
 #define RK3568_GMAC_PHY_INTF_SEL_RMII	\
@@ -1023,6 +1151,46 @@ static const struct rk_gmac_ops rk3399_ops = {
 #define RK3568_GMAC_CLK_RX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 8)
 #define RK3568_GMAC_CLK_TX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 0)
 
+#define RK3568_PIPE_GRF_XPCS_CON0	0X0040
+
+#define RK3568_PIPE_GRF_XPCS_QGMII_MAC_SEL	GRF_BIT(0)
+#define RK3568_PIPE_GRF_XPCS_SGMII_MAC_SEL	GRF_BIT(1)
+#define RK3568_PIPE_GRF_XPCS_PHY_READY		GRF_BIT(2)
+
+static void rk3568_set_to_sgmii(struct rk_priv_data *bsp_priv)
+{
+	struct device *dev = &bsp_priv->pdev->dev;
+	u32 con1;
+
+	if (IS_ERR(bsp_priv->grf)) {
+		dev_err(dev, "Missing rockchip,grf property\n");
+		return;
+	}
+
+	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+				     RK3568_GRF_GMAC0_CON1;
+	regmap_write(bsp_priv->grf, con1, RK3568_GMAC_GMII_MODE);
+
+	xpcs_setup(bsp_priv, PHY_INTERFACE_MODE_SGMII);
+}
+
+static void rk3568_set_to_qsgmii(struct rk_priv_data *bsp_priv)
+{
+	struct device *dev = &bsp_priv->pdev->dev;
+	u32 con1;
+
+	if (IS_ERR(bsp_priv->grf)) {
+		dev_err(dev, "Missing rockchip,grf property\n");
+		return;
+	}
+
+	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+				     RK3568_GRF_GMAC0_CON1;
+	regmap_write(bsp_priv->grf, con1, RK3568_GMAC_GMII_MODE);
+
+	xpcs_setup(bsp_priv, PHY_INTERFACE_MODE_QSGMII);
+}
+
 static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
 				int tx_delay, int rx_delay)
 {
@@ -1094,6 +1262,8 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed)
 static const struct rk_gmac_ops rk3568_ops = {
 	.set_to_rgmii = rk3568_set_to_rgmii,
 	.set_to_rmii = rk3568_set_to_rmii,
+	.set_to_sgmii = rk3568_set_to_sgmii,
+	.set_to_qsgmii = rk3568_set_to_qsgmii,
 	.set_rgmii_speed = rk3568_set_gmac_speed,
 	.set_rmii_speed = rk3568_set_gmac_speed,
 	.regs_valid = true,
@@ -1517,6 +1687,12 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
 				dev_err(dev, "cannot get clock %s\n",
 					"clk_mac_refout");
 		}
+	} else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII ||
+		   bsp_priv->phy_iface == PHY_INTERFACE_MODE_QSGMII) {
+		bsp_priv->pclk_xpcs = devm_clk_get(dev, "pclk_xpcs");
+		if (IS_ERR(bsp_priv->pclk_xpcs))
+			dev_err(dev, "cannot get clock %s\n",
+				"pclk_xpcs");
 	}
 
 	bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
@@ -1572,6 +1748,9 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
 			if (!IS_ERR(bsp_priv->pclk_mac))
 				clk_prepare_enable(bsp_priv->pclk_mac);
 
+			if (!IS_ERR(bsp_priv->pclk_xpcs))
+				clk_prepare_enable(bsp_priv->pclk_xpcs);
+
 			if (!IS_ERR(bsp_priv->mac_clk_tx))
 				clk_prepare_enable(bsp_priv->mac_clk_tx);
 
@@ -1605,6 +1784,8 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
 
 			clk_disable_unprepare(bsp_priv->pclk_mac);
 
+			clk_disable_unprepare(bsp_priv->pclk_xpcs);
+
 			clk_disable_unprepare(bsp_priv->mac_clk_tx);
 
 			clk_disable_unprepare(bsp_priv->clk_mac_speed);
@@ -1623,7 +1804,7 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
 	return 0;
 }
 
-static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
+static int rk_gmac_phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
 {
 	struct regulator *ldo = bsp_priv->regulator;
 	int ret;
@@ -1728,6 +1909,18 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 							"rockchip,grf");
 	bsp_priv->php_grf = syscon_regmap_lookup_by_phandle(dev->of_node,
 							    "rockchip,php-grf");
+	bsp_priv->xpcs = syscon_regmap_lookup_by_phandle(dev->of_node,
+							 "rockchip,xpcs");
+	if (!IS_ERR(bsp_priv->xpcs)) {
+		struct phy *comphy;
+
+		comphy = devm_of_phy_get(&pdev->dev, dev->of_node, NULL);
+		if (IS_ERR(comphy))
+			dev_err(dev, "devm_of_phy_get error\n");
+		ret = phy_init(comphy);
+		if (ret)
+			dev_err(dev, "phy_init error\n");
+	}
 
 	if (plat->phy_node) {
 		bsp_priv->integrated_phy = of_property_read_bool(plat->phy_node,
@@ -1805,11 +1998,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 		dev_info(dev, "init for RMII\n");
 		bsp_priv->ops->set_to_rmii(bsp_priv);
 		break;
+	case PHY_INTERFACE_MODE_SGMII:
+		dev_info(dev, "init for SGMII\n");
+		bsp_priv->ops->set_to_sgmii(bsp_priv);
+		break;
+	case PHY_INTERFACE_MODE_QSGMII:
+		dev_info(dev, "init for QSGMII\n");
+		bsp_priv->ops->set_to_qsgmii(bsp_priv);
+		break;
 	default:
 		dev_err(dev, "NO interface defined!\n");
 	}
 
-	ret = phy_power_on(bsp_priv, true);
+	ret = rk_gmac_phy_power_on(bsp_priv, true);
 	if (ret) {
 		gmac_clk_enable(bsp_priv, false);
 		return ret;
@@ -1830,7 +2031,7 @@ static void rk_gmac_powerdown(struct rk_priv_data *gmac)
 
 	pm_runtime_put_sync(&gmac->pdev->dev);
 
-	phy_power_on(gmac, false);
+	rk_gmac_phy_power_on(gmac, false);
 	gmac_clk_enable(gmac, false);
 }
 
@@ -1851,6 +2052,9 @@ static void rk_fix_speed(void *priv, unsigned int speed)
 		if (bsp_priv->ops->set_rmii_speed)
 			bsp_priv->ops->set_rmii_speed(bsp_priv, speed);
 		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		break;
 	default:
 		dev_err(dev, "unsupported interface %d", bsp_priv->phy_iface);
 	}
-- 
2.25.1

Re: [PATCH 2/2] ethernet: stmicro: stmmac: Add SGMII/QSGMII support for RK3568

[Krzysztof Kozlowski]

On 29/11/2022 08:27, Chukun Pan wrote:
> From: David Wu <david.wu@rock-chips.com>
> 
> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> This patch adds the remaining SGMII/QSGMII support.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Run-tested-on: Ariaboard Photonicat (GMAC0 SGMII)
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> [rebase, rewrite commit message]
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 210 +++++++++++++++++-
>  1 file changed, 207 insertions(+), 3 deletions(-)
> 

>  
> -static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> +static int rk_gmac_phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>  {
>  	struct regulator *ldo = bsp_priv->regulator;
>  	int ret;
> @@ -1728,6 +1909,18 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  							"rockchip,grf");
>  	bsp_priv->php_grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>  							    "rockchip,php-grf");
> +	bsp_priv->xpcs = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							 "rockchip,xpcs");
> +	if (!IS_ERR(bsp_priv->xpcs)) {
> +		struct phy *comphy;
> +
> +		comphy = devm_of_phy_get(&pdev->dev, dev->of_node, NULL);

So instead of having PHY driver, you added a syscon and implemented PHY
driver here. No. Make a proper PHY driver.

Best regards,
Krzysztof

Re: [PATCH 2/2] ethernet: stmicro: stmmac: Add SGMII/QSGMII support for RK3568

[Andrew Lunn]

> > -static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> > +static int rk_gmac_phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
> >  {
> >  	struct regulator *ldo = bsp_priv->regulator;
> >  	int ret;
> > @@ -1728,6 +1909,18 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
> >  							"rockchip,grf");
> >  	bsp_priv->php_grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> >  							    "rockchip,php-grf");
> > +	bsp_priv->xpcs = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +							 "rockchip,xpcs");
> > +	if (!IS_ERR(bsp_priv->xpcs)) {
> > +		struct phy *comphy;
> > +
> > +		comphy = devm_of_phy_get(&pdev->dev, dev->of_node, NULL);
> 
> So instead of having PHY driver, you added a syscon and implemented PHY
> driver here. No. Make a proper PHY driver.

I'm also thinking there should be a proper pcs driver in drivers/net/pcs.

    Andrew

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Krzysztof Kozlowski]

On 29/11/2022 08:27, Chukun Pan wrote:
> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> This patch adds a compatible string for the required clock.
> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 42fb72b6909d..36b1e82212e7 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> @@ -68,6 +68,7 @@ properties:
>          - mac_clk_rx
>          - aclk_mac
>          - pclk_mac
> +        - pclk_xpcs
>          - clk_mac_ref
>          - clk_mac_refout
>          - clk_mac_speed
> @@ -90,6 +91,11 @@ properties:
>        The phandle of the syscon node for the peripheral general register file.
>      $ref: /schemas/types.yaml#/definitions/phandle
>  
> +  rockchip,xpcs:
> +    description:
> +      The phandle of the syscon node for the peripheral general register file.

You used the same description as above, so no, you cannot have two
properties which are the same. syscons for GRF are called
"rockchip,grf", aren't they?


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Krzysztof Kozlowski]

On 29/11/2022 09:49, Krzysztof Kozlowski wrote:
> On 29/11/2022 08:27, Chukun Pan wrote:
>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
>> This patch adds a compatible string for the required clock.
>>
>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>> ---
>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> index 42fb72b6909d..36b1e82212e7 100644
>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>> @@ -68,6 +68,7 @@ properties:
>>          - mac_clk_rx
>>          - aclk_mac
>>          - pclk_mac
>> +        - pclk_xpcs
>>          - clk_mac_ref
>>          - clk_mac_refout
>>          - clk_mac_speed
>> @@ -90,6 +91,11 @@ properties:
>>        The phandle of the syscon node for the peripheral general register file.
>>      $ref: /schemas/types.yaml#/definitions/phandle
>>  
>> +  rockchip,xpcs:
>> +    description:
>> +      The phandle of the syscon node for the peripheral general register file.
> 
> You used the same description as above, so no, you cannot have two
> properties which are the same. syscons for GRF are called
> "rockchip,grf", aren't they?

Also:
1. Your commit msg does not explain it at all.
2. Your driver code uses this as some phy? Don't use syscon as
workaround for missing drivers.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Heiko Stübner]

Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> On 29/11/2022 08:27, Chukun Pan wrote:
> > The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> > This patch adds a compatible string for the required clock.
> > 
> > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> > ---
> >  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > index 42fb72b6909d..36b1e82212e7 100644
> > --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > @@ -68,6 +68,7 @@ properties:
> >          - mac_clk_rx
> >          - aclk_mac
> >          - pclk_mac
> > +        - pclk_xpcs
> >          - clk_mac_ref
> >          - clk_mac_refout
> >          - clk_mac_speed
> > @@ -90,6 +91,11 @@ properties:
> >        The phandle of the syscon node for the peripheral general register file.
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >  
> > +  rockchip,xpcs:
> > +    description:
> > +      The phandle of the syscon node for the peripheral general register file.
> 
> You used the same description as above, so no, you cannot have two
> properties which are the same. syscons for GRF are called
> "rockchip,grf", aren't they?

Not necessarily :-) .

The GRF is Rockchip's way of not sorting their own invented
additional registers. (aka a bunch of registers 

While on the older models there only ever was the one GRF
as dumping ground, newer SoCs now end up with multiple ones :-)

These are still iomem areas separate from the actual device-iomem they
work with/for but SoCs like the rk3568 now have at least 13 of them.


_But_ for the patch in question I fail to see what this set does at all.
The rk3568 (only) has XPCS_CON0 and XPCS_STATUS in its PIPE_GRF syscon
(according to the TRM), but the patch2 does strange things with
offset calculations and names that do not seem to have a match in the TRM.

So definitely more explanation on what happens here would be necessary.

Heiko

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Krzysztof Kozlowski]

On 29/11/2022 10:56, Heiko Stübner wrote:
> Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
>> On 29/11/2022 08:27, Chukun Pan wrote:
>>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
>>> This patch adds a compatible string for the required clock.
>>>
>>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>>> ---
>>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> index 42fb72b6909d..36b1e82212e7 100644
>>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
>>> @@ -68,6 +68,7 @@ properties:
>>>          - mac_clk_rx
>>>          - aclk_mac
>>>          - pclk_mac
>>> +        - pclk_xpcs
>>>          - clk_mac_ref
>>>          - clk_mac_refout
>>>          - clk_mac_speed
>>> @@ -90,6 +91,11 @@ properties:
>>>        The phandle of the syscon node for the peripheral general register file.
>>>      $ref: /schemas/types.yaml#/definitions/phandle
>>>  
>>> +  rockchip,xpcs:
>>> +    description:
>>> +      The phandle of the syscon node for the peripheral general register file.
>>
>> You used the same description as above, so no, you cannot have two
>> properties which are the same. syscons for GRF are called
>> "rockchip,grf", aren't they?
> 
> Not necessarily :-) .

OK, then description should have something like "...GRF for foo bar".


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Heiko Stübner]

Am Dienstag, 29. November 2022, 10:59:34 CET schrieb Krzysztof Kozlowski:
> On 29/11/2022 10:56, Heiko Stübner wrote:
> > Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> >> On 29/11/2022 08:27, Chukun Pan wrote:
> >>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> >>> This patch adds a compatible string for the required clock.
> >>>
> >>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> index 42fb72b6909d..36b1e82212e7 100644
> >>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> >>> @@ -68,6 +68,7 @@ properties:
> >>>          - mac_clk_rx
> >>>          - aclk_mac
> >>>          - pclk_mac
> >>> +        - pclk_xpcs
> >>>          - clk_mac_ref
> >>>          - clk_mac_refout
> >>>          - clk_mac_speed
> >>> @@ -90,6 +91,11 @@ properties:
> >>>        The phandle of the syscon node for the peripheral general register file.
> >>>      $ref: /schemas/types.yaml#/definitions/phandle
> >>>  
> >>> +  rockchip,xpcs:
> >>> +    description:
> >>> +      The phandle of the syscon node for the peripheral general register file.
> >>
> >> You used the same description as above, so no, you cannot have two
> >> properties which are the same. syscons for GRF are called
> >> "rockchip,grf", aren't they?
> > 
> > Not necessarily :-) .
> 
> OK, then description should have something like "...GRF for foo bar".

Actually looking deeper in the TRM, having these registers "just" written
to from the dwmac-glue-layer feels quite a bit like a hack.

The "pcs" thingy referenced in patch2 actually looks more like a real device
with its own section in the TRM and own iomem area. This pcs device then
itself has some more settings stored in said pipe-grf.

So this looks more like it wants to be an actual phy-driver.


@Chukun Pan: plase take a look at something like
https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
on how phy-drivers for ethernets could look like.

Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
moving the xpcs_setup to a phy-driver shouldn't be too hard I think.


The qsgmii/sgmii_pcs list of registers in the TRM alone already takes up
4 A4 pages, so while using the PCS as syscon and just writing some values
into it might work now, this doesn't feel at all like a future-save handling.


Heiko

Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Rob Herring]

On Tue, Nov 29, 2022 at 11:22:28AM +0100, Heiko Stübner wrote:
> Am Dienstag, 29. November 2022, 10:59:34 CET schrieb Krzysztof Kozlowski:
> > On 29/11/2022 10:56, Heiko Stübner wrote:
> > > Am Dienstag, 29. November 2022, 09:49:08 CET schrieb Krzysztof Kozlowski:
> > >> On 29/11/2022 08:27, Chukun Pan wrote:
> > >>> The gmac of RK3568 supports RGMII/SGMII/QSGMII interface.
> > >>> This patch adds a compatible string for the required clock.
> > >>>
> > >>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml | 6 ++++++
> > >>>  1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> index 42fb72b6909d..36b1e82212e7 100644
> > >>> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> > >>> @@ -68,6 +68,7 @@ properties:
> > >>>          - mac_clk_rx
> > >>>          - aclk_mac
> > >>>          - pclk_mac
> > >>> +        - pclk_xpcs
> > >>>          - clk_mac_ref
> > >>>          - clk_mac_refout
> > >>>          - clk_mac_speed
> > >>> @@ -90,6 +91,11 @@ properties:
> > >>>        The phandle of the syscon node for the peripheral general register file.
> > >>>      $ref: /schemas/types.yaml#/definitions/phandle
> > >>>  
> > >>> +  rockchip,xpcs:
> > >>> +    description:
> > >>> +      The phandle of the syscon node for the peripheral general register file.
> > >>
> > >> You used the same description as above, so no, you cannot have two
> > >> properties which are the same. syscons for GRF are called
> > >> "rockchip,grf", aren't they?
> > > 
> > > Not necessarily :-) .
> > 
> > OK, then description should have something like "...GRF for foo bar".
> 
> Actually looking deeper in the TRM, having these registers "just" written
> to from the dwmac-glue-layer feels quite a bit like a hack.
> 
> The "pcs" thingy referenced in patch2 actually looks more like a real device
> with its own section in the TRM and own iomem area. This pcs device then
> itself has some more settings stored in said pipe-grf.
> 
> So this looks more like it wants to be an actual phy-driver.

There's a PCS binding now. Seems like it should be used if there is 
also a PHY already. PCS may be part of the PHY or separate block AIUI.

Rob

Re: Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Chukun Pan]

> Actually looking deeper in the TRM, having these registers "just" written
> to from the dwmac-glue-layer feels quite a bit like a hack.

> The "pcs" thingy referenced in patch2 actually looks more like a real device
> with its own section in the TRM and own iomem area. This pcs device then
> itself has some more settings stored in said pipe-grf.

> So this looks more like it wants to be an actual phy-driver.

> @Chukun Pan: plase take a look at something like
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
> on how phy-drivers for ethernets could look like.

> Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
> moving the xpcs_setup to a phy-driver shouldn't be too hard I think.

Thanks for pointing that out.
The patch2 is come from the sdk kernel of rockchip.
The sgmii-phy of RK3568 is designed on nanning combo phy.
In the sdk kernel, if we want to use sgmii mode, we need
to modify the device tree in the gmac section like this:

```
&gmac0 {
	power-domains = <&power RK3568_PD_PIPE>;
	phys = <&combphy1_usq PHY_TYPE_SGMII>;
	phy-handle = <&sgmii_phy>;
	phy-mode = "sgmii";
	rockchip,pipegrf = <&pipegrf>;
	rockchip,xpcs = <&xpcs>;
	status = "okay";
};
```

I'm not sure how to write this on the mainline kernel.
Any hint will be appreciated.

--
Thanks,
Chukun

-- 
2.25.1

Re: Re: [PATCH 1/2] dt-bindings: net: rockchip-dwmac: add rk3568 xpcs compatible

[Andrew Lunn]

On Sat, Dec 03, 2022 at 05:00:15PM +0800, Chukun Pan wrote:
> > Actually looking deeper in the TRM, having these registers "just" written
> > to from the dwmac-glue-layer feels quite a bit like a hack.
> 
> > The "pcs" thingy referenced in patch2 actually looks more like a real device
> > with its own section in the TRM and own iomem area. This pcs device then
> > itself has some more settings stored in said pipe-grf.
> 
> > So this looks more like it wants to be an actual phy-driver.
> 
> > @Chukun Pan: plase take a look at something like
> > https://elixir.bootlin.com/linux/latest/source/drivers/phy/mscc/phy-ocelot-serdes.c#L398
> > on how phy-drivers for ethernets could look like.
> 
> > Aquiring such a phy from the dwmac-glue and calling phy_set_mode after
> > moving the xpcs_setup to a phy-driver shouldn't be too hard I think.
> 
> Thanks for pointing that out.
> The patch2 is come from the sdk kernel of rockchip.
> The sgmii-phy of RK3568 is designed on nanning combo phy.
> In the sdk kernel, if we want to use sgmii mode, we need
> to modify the device tree in the gmac section like this:
> 
> ```
> &gmac0 {
> 	power-domains = <&power RK3568_PD_PIPE>;
> 	phys = <&combphy1_usq PHY_TYPE_SGMII>;
> 	phy-handle = <&sgmii_phy>;
> 	phy-mode = "sgmii";

phy-mode tells you you are using SGMII. You can tell the generic PHY
driver this which will call the PHY drivers .set_mode().

As said above, there are plenty of examples of this, mvneta and its
comphy, various mscc drivers etc.

	Andrew