[PATCH net-next v2 0/5] Add PCS support for Qualcomm IPQ9574 SoC

[PATCH net-next v2 0/5] Add PCS support for Qualcomm IPQ9574 SoC

[Lei Wei]

The 'UNIPHY' PCS block in the Qualcomm IPQ9574 SoC provides Ethernet
PCS and SerDes functions. It supports 1Gbps mode PCS and 10-Gigabit
mode PCS (XPCS) functions, and supports various interface modes for
the connectivity between the Ethernet MAC and the external PHYs/Switch.
There are three UNIPHY (PCS) instances in IPQ9574, supporting the six
Ethernet ports.

This patch series adds base driver support for initializing the PCS,
and PCS phylink ops for managing the PCS modes/states. Support for
SGMII/QSGMII (PCS) and USXGMII (XPCS) modes is being added initially.

The Ethernet driver which handles the MAC operations will create the
PCS instances and phylink for the MAC, by utilizing the API exported
by this driver.

While support is being added initially for IPQ9574, the driver is
expected to be easily extendable later for other SoCs in the IPQ
family such as IPQ5332.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
Changes in v2:
- dtbindings updates
  a.) Rename dt-binding header file to match binding file name.
  b.) Drop unused labels and the redundant examples.
  c.) Rename "mii_rx"/"mii_tx" clock names to "rx"/"tx".
- Rename "PCS_QCOM_IPQ" with specific name "PCS_QCOM_IPQ9574" in
  Kconfig.
- Remove interface mode check for the PCS lock.
- Use Cisco SGMII AN mode as default SGMII/QSGMII AN mode.
- Instantiate MII PCS instances in probe and export "ipq_pcs_get" and
  "ipq_pcs_put" APIs.
- Move MII RX and TX clock enable and disable to "pcs_enable" and
  "pcs_disable" methods.
- Change "dev_dbg" to "dev_dbg_ratelimited" in "pcs_get_state" method.
- Link to v1: https://lore.kernel.org/r/20241101-ipq_pcs_rc1-v1-0-fdef575620cf@quicinc.com

---
Lei Wei (5):
      dt-bindings: net: pcs: Add Ethernet PCS for Qualcomm IPQ9574 SoC
      net: pcs: Add PCS driver for Qualcomm IPQ9574 SoC
      net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations
      net: pcs: qcom-ipq9574: Add USXGMII interface mode support
      MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver

 .../bindings/net/pcs/qcom,ipq9574-pcs.yaml         | 190 +++++
 MAINTAINERS                                        |   9 +
 drivers/net/pcs/Kconfig                            |   9 +
 drivers/net/pcs/Makefile                           |   1 +
 drivers/net/pcs/pcs-qcom-ipq9574.c                 | 884 +++++++++++++++++++++
 include/dt-bindings/net/qcom,ipq9574-pcs.h         |  15 +
 include/linux/pcs/pcs-qcom-ipq9574.h               |  16 +
 7 files changed, 1124 insertions(+)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241101-ipq_pcs_rc1-26ae183c9c63

Best regards,
-- 
Lei Wei <quic_leiwei@quicinc.com>

[PATCH net-next v2 1/5] dt-bindings: net: pcs: Add Ethernet PCS for Qualcomm IPQ9574 SoC

[Lei Wei]

The 'UNIPHY' PCS block in the IPQ9574 SoC includes PCS and SerDes
functions. It supports different interface modes to enable Ethernet
MAC connections to different types of external PHYs/switch. It includes
PCS functions for 1Gbps and 2.5Gbps interface modes and XPCS functions
for 10Gbps interface modes. There are three UNIPHY (PCS) instances
in IPQ9574 SoC which provide PCS/XPCS functions to the six Ethernet
ports.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
 .../bindings/net/pcs/qcom,ipq9574-pcs.yaml         | 190 +++++++++++++++++++++
 include/dt-bindings/net/qcom,ipq9574-pcs.h         |  15 ++
 2 files changed, 205 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
new file mode 100644
index 000000000000..74573c28d6fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
@@ -0,0 +1,190 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/qcom,ipq9574-pcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ethernet PCS for Qualcomm IPQ9574 SoC
+
+maintainers:
+  - Lei Wei <quic_leiwei@quicinc.com>
+
+description:
+  The UNIPHY hardware blocks in the Qualcomm IPQ SoC include PCS and SerDes
+  functions. They enable connectivity between the Ethernet MAC inside the
+  PPE (packet processing engine) and external Ethernet PHY/switch. There are
+  three UNIPHY instances in IPQ9574 SoC which provide PCS functions to the
+  six Ethernet ports.
+
+  For SGMII (1Gbps PHY) or 2500BASE-X (2.5Gbps PHY) interface modes, the PCS
+  function is enabled by using the PCS block inside UNIPHY. For USXGMII (10Gbps
+  PHY), the XPCS block in UNIPHY is used.
+
+  The SerDes provides 125M (1Gbps mode) or 312.5M (2.5Gbps and 10Gbps modes)
+  RX and TX clocks to the NSSCC (Networking Sub System Clock Controller). The
+  NSSCC divides these clocks and generates the MII RX and TX clocks to each
+  of the MII interfaces between the PCS and MAC, as per the link speeds and
+  interface modes.
+
+  Different IPQ SoC may support different number of UNIPHYs (PCSes) since the
+  number of ports and their capabilities can be different between these SoCs
+
+  Below diagram depicts the UNIPHY (PCS) connections for an IPQ9574 SoC based
+  board. In this example, the PCS0 has four GMIIs/XGMIIs, which can connect
+  with four MACs to support QSGMII (4 x 1Gbps) or 10G_QXGMII (4 x 2.5Gbps)
+  interface modes.
+
+  -           +-------+ +---------+  +-------------------------+
+    +---------+CMN PLL| |  GCC    |  |   NSSCC (Divider)       |
+    |         +----+--+ +----+----+  +--+-------+--------------+
+    |              |         |          ^       |
+    |       31.25M |  SYS/AHB|clk  RX/TX|clk    +------------+
+    |       ref clk|         |          |       |            |
+    |              |         v          | MII RX|TX clk   MAC| RX/TX clk
+    |25/50M     +--+---------+----------+-------+---+      +-+---------+
+    |ref clk    |  |   +----------------+       |   |      | |     PPE |
+    v           |  |   |     UNIPHY0            V   |      | V         |
+  +-------+     |  v   |       +-----------+ (X)GMII|      |           |
+  |       |     |  +---+---+   |           |--------|------|-- MAC0    |
+  |       |     |  |       |   |           | (X)GMII|      |           |
+  |  Quad |     |  |SerDes |   | PCS/XPCS  |--------|------|-- MAC1    |
+  |       +<----+  |       |   |           | (X)GMII|      |           |
+  |(X)GPHY|     |  |       |   |           |--------|------|-- MAC2    |
+  |       |     |  |       |   |           | (X)GMII|      |           |
+  |       |     |  +-------+   |           |--------|------|-- MAC3    |
+  +-------+     |              |           |        |      |           |
+                |              +-----------+        |      |           |
+                +-----------------------------------+      |           |
+                +--+---------+----------+-------+---+      |           |
+  +-------+     |            UNIPHY1                |      |           |
+  |       |     |              +-----------+        |      |           |
+  |(X)GPHY|     | +-------+    |           | (X)GMII|      |           |
+  |       +<----+ |SerDes |    | PCS/XPCS  |--------|------|- MAC4     |
+  |       |     | |       |    |           |        |      |           |
+  +-------+     | +-------+    |           |        |      |           |
+                |              +-----------+        |      |           |
+                +-----------------------------------+      |           |
+                +--+---------+----------+-------+---+      |           |
+  +-------+     |           UNIPHY2                 |      |           |
+  |       |     |              +-----------+        |      |           |
+  |(X)GPHY|     | +-------+    |           | (X)GMII|      |           |
+  |       +<----+ |SerDes |    | PCS/XPCS  |--------|------|- MAC5     |
+  |       |     | |       |    |           |        |      |           |
+  +-------+     | +-------+    |           |        |      |           |
+                |              +-----------+        |      |           |
+                +-----------------------------------+      +-----------+
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq9574-pcs
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  clocks:
+    items:
+      - description: System clock
+      - description: AHB clock needed for register interface access
+
+  clock-names:
+    items:
+      - const: sys
+      - const: ahb
+
+  '#clock-cells':
+    const: 1
+    description: See include/dt-bindings/net/qcom,ipq9574-pcs.h for constants
+
+patternProperties:
+  '^pcs-mii@[0-4]$':
+    type: object
+    description: PCS MII interface.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 4
+        description: MII index
+
+      clocks:
+        items:
+          - description: PCS MII RX clock
+          - description: PCS MII TX clock
+
+      clock-names:
+        items:
+          - const: rx
+          - const: tx
+
+    required:
+      - reg
+      - clocks
+      - clock-names
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
+
+    ethernet-pcs@7a00000 {
+        compatible = "qcom,ipq9574-pcs";
+        reg = <0x7a00000 0x10000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        clocks = <&gcc GCC_UNIPHY0_SYS_CLK>,
+                 <&gcc GCC_UNIPHY0_AHB_CLK>;
+        clock-names = "sys",
+                      "ahb";
+        #clock-cells = <1>;
+
+        pcs-mii@0 {
+            reg = <0>;
+            clocks = <&nsscc 116>,
+                     <&nsscc 117>;
+            clock-names = "rx",
+                          "tx";
+        };
+
+        pcs-mii@1 {
+            reg = <1>;
+            clocks = <&nsscc 118>,
+                     <&nsscc 119>;
+            clock-names = "rx",
+                          "tx";
+        };
+
+        pcs-mii@2 {
+            reg = <2>;
+            clocks = <&nsscc 120>,
+                     <&nsscc 121>;
+            clock-names = "rx",
+                          "tx";
+        };
+
+        pcs-mii@3 {
+            reg = <3>;
+            clocks = <&nsscc 122>,
+                     <&nsscc 123>;
+            clock-names = "rx",
+                          "tx";
+        };
+    };
diff --git a/include/dt-bindings/net/qcom,ipq9574-pcs.h b/include/dt-bindings/net/qcom,ipq9574-pcs.h
new file mode 100644
index 000000000000..96bd036aaa70
--- /dev/null
+++ b/include/dt-bindings/net/qcom,ipq9574-pcs.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ * Device Tree constants for the Qualcomm IPQ9574 PCS
+ */
+
+#ifndef _DT_BINDINGS_PCS_QCOM_IPQ9574_H
+#define _DT_BINDINGS_PCS_QCOM_IPQ9574_H
+
+/* The RX and TX clocks which are provided from the SerDes to NSSCC. */
+#define PCS_RX_CLK		0
+#define PCS_TX_CLK		1
+
+#endif /* _DT_BINDINGS_PCS_QCOM_IPQ9574_H */

-- 
2.34.1

Re: [PATCH net-next v2 1/5] dt-bindings: net: pcs: Add Ethernet PCS for Qualcomm IPQ9574 SoC

[Krzysztof Kozlowski]

On Wed, Dec 04, 2024 at 10:43:53PM +0800, Lei Wei wrote:
> The 'UNIPHY' PCS block in the IPQ9574 SoC includes PCS and SerDes
> functions. It supports different interface modes to enable Ethernet
> MAC connections to different types of external PHYs/switch. It includes
> PCS functions for 1Gbps and 2.5Gbps interface modes and XPCS functions
> for 10Gbps interface modes. There are three UNIPHY (PCS) instances
> in IPQ9574 SoC which provide PCS/XPCS functions to the six Ethernet
> ports.
> 
> Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
> ---
>  .../bindings/net/pcs/qcom,ipq9574-pcs.yaml         | 190 +++++++++++++++++++++
>  include/dt-bindings/net/qcom,ipq9574-pcs.h         |  15 ++
>  2 files changed, 205 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH net-next v2 2/5] net: pcs: Add PCS driver for Qualcomm IPQ9574 SoC

[Lei Wei]

The 'UNIPHY' PCS hardware block in Qualcomm's IPQ SoC supports
different interface modes to enable Ethernet MAC connections
for different types of external PHYs/switch. Each UNIPHY block
includes a SerDes and PCS/XPCS blocks, and can operate in either
PCS or XPCS modes. It supports 1Gbps and 2.5Gbps interface modes
(Ex: SGMII) using the PCS, and 10Gbps interface modes (Ex: USXGMII)
using the XPCS. There are three UNIPHY (PCS) instances in IPQ9574
SoC which support the six Ethernet ports in the SoC.

This patch adds support for the platform driver, probe and clock
registrations for the PCS driver. The platform driver creates an
'ipq_pcs' instance for each of the UNIPHY used on the given board.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
 drivers/net/pcs/Kconfig            |   9 ++
 drivers/net/pcs/Makefile           |   1 +
 drivers/net/pcs/pcs-qcom-ipq9574.c | 245 +++++++++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+)

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index f6aa437473de..de2ec527d523 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -25,6 +25,15 @@ config PCS_MTK_LYNXI
 	  This module provides helpers to phylink for managing the LynxI PCS
 	  which is part of MediaTek's SoC and Ethernet switch ICs.
 
+config PCS_QCOM_IPQ9574
+	tristate "Qualcomm IPQ9574 PCS"
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	depends on HAS_IOMEM
+	help
+	  This module provides driver for UNIPHY PCS available on Qualcomm
+	  IPQ9574 SoC. The UNIPHY PCS supports both PCS and XPCS functions
+	  to support different interface modes for MAC to PHY connections.
+
 config PCS_RZN1_MIIC
 	tristate "Renesas RZ/N1 MII converter"
 	depends on OF && (ARCH_RZN1 || COMPILE_TEST)
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..2fa3faf8a5db 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -7,4 +7,5 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
+obj-$(CONFIG_PCS_QCOM_IPQ9574)	+= pcs-qcom-ipq9574.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
new file mode 100644
index 000000000000..ea90c1902b61
--- /dev/null
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/net/qcom,ipq9574-pcs.h>
+
+#define XPCS_INDIRECT_ADDR		0x8000
+#define XPCS_INDIRECT_AHB_ADDR		0x83fc
+#define XPCS_INDIRECT_ADDR_H		GENMASK(20, 8)
+#define XPCS_INDIRECT_ADDR_L		GENMASK(7, 0)
+#define XPCS_INDIRECT_DATA_ADDR(reg)	(FIELD_PREP(GENMASK(15, 10), 0x20) | \
+					 FIELD_PREP(GENMASK(9, 2), \
+					 FIELD_GET(XPCS_INDIRECT_ADDR_L, reg)))
+
+/* PCS private data */
+struct ipq_pcs {
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *regmap;
+	phy_interface_t interface;
+
+	/* RX clock supplied to NSSCC */
+	struct clk_hw rx_hw;
+	/* TX clock supplied to NSSCC */
+	struct clk_hw tx_hw;
+};
+
+static unsigned long ipq_pcs_clk_rate_get(struct ipq_pcs *qpcs)
+{
+	switch (qpcs->interface) {
+	case PHY_INTERFACE_MODE_USXGMII:
+		return 312500000;
+	default:
+		return 125000000;
+	}
+}
+
+/* Return clock rate for the RX clock supplied to NSSCC
+ * as per the interface mode.
+ */
+static unsigned long ipq_pcs_rx_clk_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct ipq_pcs *qpcs = container_of(hw, struct ipq_pcs, rx_hw);
+
+	return ipq_pcs_clk_rate_get(qpcs);
+}
+
+/* Return clock rate for the TX clock supplied to NSSCC
+ * as per the interface mode.
+ */
+static unsigned long ipq_pcs_tx_clk_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct ipq_pcs *qpcs = container_of(hw, struct ipq_pcs, tx_hw);
+
+	return ipq_pcs_clk_rate_get(qpcs);
+}
+
+static int ipq_pcs_clk_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	switch (req->rate) {
+	case 125000000:
+	case 312500000:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Clock ops for the RX clock supplied to NSSCC */
+static const struct clk_ops ipq_pcs_rx_clk_ops = {
+	.determine_rate = ipq_pcs_clk_determine_rate,
+	.recalc_rate = ipq_pcs_rx_clk_recalc_rate,
+};
+
+/* Clock ops for the TX clock supplied to NSSCC */
+static const struct clk_ops ipq_pcs_tx_clk_ops = {
+	.determine_rate = ipq_pcs_clk_determine_rate,
+	.recalc_rate = ipq_pcs_tx_clk_recalc_rate,
+};
+
+static struct clk_hw *ipq_pcs_clk_hw_get(struct of_phandle_args *clkspec,
+					 void *data)
+{
+	struct ipq_pcs *qpcs = data;
+
+	switch (clkspec->args[0]) {
+	case PCS_RX_CLK:
+		return &qpcs->rx_hw;
+	case PCS_TX_CLK:
+		return &qpcs->tx_hw;
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+/* Register the RX and TX clock which are output from SerDes to
+ * the NSSCC. The NSSCC driver assigns the RX and TX clock as
+ * parent, divides them to generate the MII RX and TX clock to
+ * each MII interface of the PCS as per the link speeds and
+ * interface modes.
+ */
+static int ipq_pcs_clk_register(struct ipq_pcs *qpcs)
+{
+	struct clk_init_data init = { };
+	int ret;
+
+	init.ops = &ipq_pcs_rx_clk_ops;
+	init.name = devm_kasprintf(qpcs->dev, GFP_KERNEL, "%s::rx_clk",
+				   dev_name(qpcs->dev));
+	if (!init.name)
+		return -ENOMEM;
+
+	qpcs->rx_hw.init = &init;
+	ret = devm_clk_hw_register(qpcs->dev, &qpcs->rx_hw);
+	if (ret)
+		return ret;
+
+	init.ops = &ipq_pcs_tx_clk_ops;
+	init.name = devm_kasprintf(qpcs->dev, GFP_KERNEL, "%s::tx_clk",
+				   dev_name(qpcs->dev));
+	if (!init.name)
+		return -ENOMEM;
+
+	qpcs->tx_hw.init = &init;
+	ret = devm_clk_hw_register(qpcs->dev, &qpcs->tx_hw);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(qpcs->dev, ipq_pcs_clk_hw_get, qpcs);
+}
+
+static int ipq_pcs_regmap_read(void *context, unsigned int reg,
+			       unsigned int *val)
+{
+	struct ipq_pcs *qpcs = context;
+
+	/* PCS uses direct AHB access while XPCS uses indirect AHB access */
+	if (reg >= XPCS_INDIRECT_ADDR) {
+		writel(FIELD_GET(XPCS_INDIRECT_ADDR_H, reg),
+		       qpcs->base + XPCS_INDIRECT_AHB_ADDR);
+		*val = readl(qpcs->base + XPCS_INDIRECT_DATA_ADDR(reg));
+	} else {
+		*val = readl(qpcs->base + reg);
+	}
+
+	return 0;
+}
+
+static int ipq_pcs_regmap_write(void *context, unsigned int reg,
+				unsigned int val)
+{
+	struct ipq_pcs *qpcs = context;
+
+	/* PCS uses direct AHB access while XPCS uses indirect AHB access */
+	if (reg >= XPCS_INDIRECT_ADDR) {
+		writel(FIELD_GET(XPCS_INDIRECT_ADDR_H, reg),
+		       qpcs->base + XPCS_INDIRECT_AHB_ADDR);
+		writel(val, qpcs->base + XPCS_INDIRECT_DATA_ADDR(reg));
+	} else {
+		writel(val, qpcs->base + reg);
+	}
+
+	return 0;
+}
+
+static const struct regmap_config ipq_pcs_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_read = ipq_pcs_regmap_read,
+	.reg_write = ipq_pcs_regmap_write,
+	.fast_io = true,
+};
+
+static int ipq9574_pcs_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ipq_pcs *qpcs;
+	struct clk *clk;
+	int ret;
+
+	qpcs = devm_kzalloc(dev, sizeof(*qpcs), GFP_KERNEL);
+	if (!qpcs)
+		return -ENOMEM;
+
+	qpcs->dev = dev;
+
+	qpcs->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(qpcs->base))
+		return dev_err_probe(dev, PTR_ERR(qpcs->base),
+				     "Failed to ioremap resource\n");
+
+	qpcs->regmap = devm_regmap_init(dev, NULL, qpcs, &ipq_pcs_regmap_cfg);
+	if (IS_ERR(qpcs->regmap))
+		return dev_err_probe(dev, PTR_ERR(qpcs->regmap),
+				     "Failed to allocate register map\n");
+
+	clk = devm_clk_get_enabled(dev, "sys");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Failed to enable SYS clock\n");
+
+	clk = devm_clk_get_enabled(dev, "ahb");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Failed to enable AHB clock\n");
+
+	ret = ipq_pcs_clk_register(qpcs);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, qpcs);
+
+	return 0;
+}
+
+static const struct of_device_id ipq9574_pcs_of_mtable[] = {
+	{ .compatible = "qcom,ipq9574-pcs" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ipq9574_pcs_of_mtable);
+
+static struct platform_driver ipq9574_pcs_driver = {
+	.driver = {
+		.name = "ipq9574_pcs",
+		.suppress_bind_attrs = true,
+		.of_match_table = ipq9574_pcs_of_mtable,
+	},
+	.probe = ipq9574_pcs_probe,
+};
+module_platform_driver(ipq9574_pcs_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm IPQ9574 PCS driver");
+MODULE_AUTHOR("Lei Wei <quic_leiwei@quicinc.com>");

-- 
2.34.1

[PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Lei Wei]

This patch adds the following PCS functionality for the PCS driver
for IPQ9574 SoC:

a.) Parses PCS MII DT nodes and instantiate each MII PCS instance.
b.) Exports PCS instance get and put APIs. The network driver calls
the PCS get API to get and associate the PCS instance with the port
MAC.
c.) PCS phylink operations for SGMII/QSGMII interface modes.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
 drivers/net/pcs/pcs-qcom-ipq9574.c   | 462 +++++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-qcom-ipq9574.h |  16 ++
 2 files changed, 478 insertions(+)

diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
index ea90c1902b61..3608f5506477 100644
--- a/drivers/net/pcs/pcs-qcom-ipq9574.c
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -6,12 +6,46 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pcs/pcs-qcom-ipq9574.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/net/qcom,ipq9574-pcs.h>
 
+/* Maximum number of MIIs per PCS instance. There are 5 MIIs for PSGMII. */
+#define PCS_MAX_MII_NRS			5
+
+#define PCS_CALIBRATION			0x1e0
+#define PCS_CALIBRATION_DONE		BIT(7)
+
+#define PCS_MODE_CTRL			0x46c
+#define PCS_MODE_SEL_MASK		GENMASK(12, 8)
+#define PCS_MODE_SGMII			FIELD_PREP(PCS_MODE_SEL_MASK, 0x4)
+#define PCS_MODE_QSGMII			FIELD_PREP(PCS_MODE_SEL_MASK, 0x1)
+
+#define PCS_MII_CTRL(x)			(0x480 + 0x18 * (x))
+#define PCS_MII_ADPT_RESET		BIT(11)
+#define PCS_MII_FORCE_MODE		BIT(3)
+#define PCS_MII_SPEED_MASK		GENMASK(2, 1)
+#define PCS_MII_SPEED_1000		FIELD_PREP(PCS_MII_SPEED_MASK, 0x2)
+#define PCS_MII_SPEED_100		FIELD_PREP(PCS_MII_SPEED_MASK, 0x1)
+#define PCS_MII_SPEED_10		FIELD_PREP(PCS_MII_SPEED_MASK, 0x0)
+
+#define PCS_MII_STS(x)			(0x488 + 0x18 * (x))
+#define PCS_MII_LINK_STS		BIT(7)
+#define PCS_MII_STS_DUPLEX_FULL		BIT(6)
+#define PCS_MII_STS_SPEED_MASK		GENMASK(5, 4)
+#define PCS_MII_STS_SPEED_10		0
+#define PCS_MII_STS_SPEED_100		1
+#define PCS_MII_STS_SPEED_1000		2
+
+#define PCS_PLL_RESET			0x780
+#define PCS_ANA_SW_RESET		BIT(6)
+
 #define XPCS_INDIRECT_ADDR		0x8000
 #define XPCS_INDIRECT_AHB_ADDR		0x83fc
 #define XPCS_INDIRECT_ADDR_H		GENMASK(20, 8)
@@ -20,6 +54,18 @@
 					 FIELD_PREP(GENMASK(9, 2), \
 					 FIELD_GET(XPCS_INDIRECT_ADDR_L, reg)))
 
+/* Per PCS MII private data */
+struct ipq_pcs_mii {
+	struct ipq_pcs *qpcs;
+	struct phylink_pcs pcs;
+	int index;
+
+	/* RX clock from NSSCC to PCS MII */
+	struct clk *rx_clk;
+	/* TX clock from NSSCC to PCS MII */
+	struct clk *tx_clk;
+};
+
 /* PCS private data */
 struct ipq_pcs {
 	struct device *dev;
@@ -27,12 +73,422 @@ struct ipq_pcs {
 	struct regmap *regmap;
 	phy_interface_t interface;
 
+	/* Lock to protect PCS configurations shared by multiple MII ports */
+	struct mutex config_lock;
+
 	/* RX clock supplied to NSSCC */
 	struct clk_hw rx_hw;
 	/* TX clock supplied to NSSCC */
 	struct clk_hw tx_hw;
+
+	struct ipq_pcs_mii *qpcs_mii[PCS_MAX_MII_NRS];
 };
 
+#define phylink_pcs_to_qpcs_mii(_pcs)	\
+	container_of(_pcs, struct ipq_pcs_mii, pcs)
+
+static void ipq_pcs_get_state_sgmii(struct ipq_pcs *qpcs,
+				    int index,
+				    struct phylink_link_state *state)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(qpcs->regmap, PCS_MII_STS(index), &val);
+	if (ret) {
+		state->link = 0;
+		return;
+	}
+
+	state->link = !!(val & PCS_MII_LINK_STS);
+
+	if (!state->link)
+		return;
+
+	switch (FIELD_GET(PCS_MII_STS_SPEED_MASK, val)) {
+	case PCS_MII_STS_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	case PCS_MII_STS_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case PCS_MII_STS_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	default:
+		state->link = false;
+		return;
+	}
+
+	if (val & PCS_MII_STS_DUPLEX_FULL)
+		state->duplex = DUPLEX_FULL;
+	else
+		state->duplex = DUPLEX_HALF;
+}
+
+static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
+			       phy_interface_t interface)
+{
+	unsigned int val;
+	int ret;
+
+	/* Configure PCS interface mode */
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		val = PCS_MODE_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_QSGMII:
+		val = PCS_MODE_QSGMII;
+		break;
+	default:
+		dev_err(qpcs->dev,
+			"Unsupported interface %s\n", phy_modes(interface));
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+				 PCS_MODE_SEL_MASK, val);
+	if (ret)
+		return ret;
+
+	/* PCS PLL reset */
+	ret = regmap_update_bits(qpcs->regmap, PCS_PLL_RESET,
+				 PCS_ANA_SW_RESET, 0);
+	if (ret)
+		return ret;
+
+	fsleep(1000);
+	ret = regmap_update_bits(qpcs->regmap, PCS_PLL_RESET,
+				 PCS_ANA_SW_RESET, PCS_ANA_SW_RESET);
+	if (ret)
+		return ret;
+
+	/* Wait for calibration completion */
+	ret = regmap_read_poll_timeout(qpcs->regmap, PCS_CALIBRATION,
+				       val, val & PCS_CALIBRATION_DONE,
+				       1000, 100000);
+	if (ret) {
+		dev_err(qpcs->dev, "PCS calibration timed-out\n");
+		return ret;
+	}
+
+	qpcs->interface = interface;
+
+	return 0;
+}
+
+static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
+				int index,
+				unsigned int neg_mode,
+				phy_interface_t interface)
+{
+	int ret;
+
+	/* Access to PCS registers such as PCS_MODE_CTRL which are
+	 * common to all MIIs, is lock protected and configured
+	 * only once.
+	 */
+	mutex_lock(&qpcs->config_lock);
+
+	if (qpcs->interface != interface) {
+		ret = ipq_pcs_config_mode(qpcs, interface);
+		if (ret) {
+			mutex_unlock(&qpcs->config_lock);
+			return ret;
+		}
+	}
+
+	mutex_unlock(&qpcs->config_lock);
+
+	/* Nothing to do here as in-band autoneg mode is enabled
+	 * by default for each PCS MII port.
+	 */
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+		return 0;
+
+	/* Set force speed mode */
+	return regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+				  PCS_MII_FORCE_MODE, PCS_MII_FORCE_MODE);
+}
+
+static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
+					int index,
+					unsigned int neg_mode,
+					int speed)
+{
+	unsigned int val;
+	int ret;
+
+	/* PCS speed need not be configured if in-band autoneg is enabled */
+	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		/* PCS speed set for force mode */
+		switch (speed) {
+		case SPEED_1000:
+			val = PCS_MII_SPEED_1000;
+			break;
+		case SPEED_100:
+			val = PCS_MII_SPEED_100;
+			break;
+		case SPEED_10:
+			val = PCS_MII_SPEED_10;
+			break;
+		default:
+			dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
+			return -EINVAL;
+		}
+
+		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+					 PCS_MII_SPEED_MASK, val);
+		if (ret)
+			return ret;
+	}
+
+	/* PCS adapter reset */
+	ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+				 PCS_MII_ADPT_RESET, 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+				  PCS_MII_ADPT_RESET, PCS_MII_ADPT_RESET);
+}
+
+static int ipq_pcs_enable(struct phylink_pcs *pcs)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+	int index = qpcs_mii->index;
+	int ret;
+
+	ret = clk_prepare_enable(qpcs_mii->rx_clk);
+	if (ret) {
+		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(qpcs_mii->tx_clk);
+	if (ret) {
+		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
+		clk_disable_unprepare(qpcs_mii->rx_clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ipq_pcs_disable(struct phylink_pcs *pcs)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+	if (__clk_is_enabled(qpcs_mii->rx_clk))
+		clk_disable_unprepare(qpcs_mii->rx_clk);
+
+	if (__clk_is_enabled(qpcs_mii->tx_clk))
+		clk_disable_unprepare(qpcs_mii->tx_clk);
+}
+
+static void ipq_pcs_get_state(struct phylink_pcs *pcs,
+			      struct phylink_link_state *state)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+	int index = qpcs_mii->index;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		ipq_pcs_get_state_sgmii(qpcs, index, state);
+		break;
+	default:
+		break;
+	}
+
+	dev_dbg_ratelimited(qpcs->dev,
+			    "mode=%s/%s/%s link=%u\n",
+			    phy_modes(state->interface),
+			    phy_speed_to_str(state->speed),
+			    phy_duplex_to_str(state->duplex),
+			    state->link);
+}
+
+static int ipq_pcs_config(struct phylink_pcs *pcs,
+			  unsigned int neg_mode,
+			  phy_interface_t interface,
+			  const unsigned long *advertising,
+			  bool permit)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+	int index = qpcs_mii->index;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
+	default:
+		dev_err(qpcs->dev,
+			"Unsupported interface %s\n", phy_modes(interface));
+		return -EOPNOTSUPP;
+	};
+}
+
+static void ipq_pcs_link_up(struct phylink_pcs *pcs,
+			    unsigned int neg_mode,
+			    phy_interface_t interface,
+			    int speed, int duplex)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+	int index = qpcs_mii->index;
+	int ret;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
+						   neg_mode, speed);
+		break;
+	default:
+		dev_err(qpcs->dev,
+			"Unsupported interface %s\n", phy_modes(interface));
+		return;
+	}
+
+	if (ret)
+		dev_err(qpcs->dev, "PCS link up fail for interface %s\n",
+			phy_modes(interface));
+}
+
+static const struct phylink_pcs_ops ipq_pcs_phylink_ops = {
+	.pcs_enable = ipq_pcs_enable,
+	.pcs_disable = ipq_pcs_disable,
+	.pcs_get_state = ipq_pcs_get_state,
+	.pcs_config = ipq_pcs_config,
+	.pcs_link_up = ipq_pcs_link_up,
+};
+
+/**
+ * ipq_pcs_get() - Get the IPQ PCS MII instance
+ * @np: Device tree node to the PCS MII
+ *
+ * Description: Get the phylink PCS instance for the given PCS MII node @np.
+ * This instance is associated with the specific MII of the PCS and the
+ * corresponding Ethernet netdevice.
+ *
+ * Return: A pointer to the phylink PCS instance or an error-pointer value.
+ */
+struct phylink_pcs *ipq_pcs_get(struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct ipq_pcs_mii *qpcs_mii;
+	struct ipq_pcs *qpcs;
+	u32 index;
+
+	if (of_property_read_u32(np, "reg", &index))
+		return ERR_PTR(-EINVAL);
+
+	if (index >= PCS_MAX_MII_NRS)
+		return ERR_PTR(-EINVAL);
+
+	/* Get the parent device */
+	pdev = of_find_device_by_node(np->parent);
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
+	qpcs = platform_get_drvdata(pdev);
+	if (!qpcs) {
+		put_device(&pdev->dev);
+
+		/* If probe is not yet completed, return DEFER to
+		 * the dependent driver.
+		 */
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	qpcs_mii = qpcs->qpcs_mii[index];
+	if (!qpcs_mii) {
+		put_device(&pdev->dev);
+		return ERR_PTR(-ENOENT);
+	}
+
+	return &qpcs_mii->pcs;
+}
+EXPORT_SYMBOL(ipq_pcs_get);
+
+/**
+ * ipq_pcs_put() - Release the IPQ PCS MII instance
+ * @pcs: PCS instance
+ *
+ * Description: Release a phylink PCS instance.
+ */
+void ipq_pcs_put(struct phylink_pcs *pcs)
+{
+	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+	/* Put reference taken by of_find_device_by_node() in
+	 * ipq_pcs_get().
+	 */
+	put_device(qpcs_mii->qpcs->dev);
+}
+EXPORT_SYMBOL(ipq_pcs_put);
+
+/* Parse the PCS MII DT nodes which are child nodes of the PCS node,
+ * and instantiate each MII PCS instance.
+ */
+static int ipq_pcs_create_miis(struct ipq_pcs *qpcs)
+{
+	struct device *dev = qpcs->dev;
+	struct ipq_pcs_mii *qpcs_mii;
+	struct device_node *mii_np;
+	u32 index;
+	int ret;
+
+	for_each_available_child_of_node(dev->of_node, mii_np) {
+		ret = of_property_read_u32(mii_np, "reg", &index);
+		if (ret) {
+			dev_err(dev, "Failed to read MII index\n");
+			of_node_put(mii_np);
+			return ret;
+		}
+
+		if (index >= PCS_MAX_MII_NRS) {
+			dev_err(dev, "Invalid MII index\n");
+			of_node_put(mii_np);
+			return -EINVAL;
+		}
+
+		qpcs_mii = devm_kzalloc(dev, sizeof(*qpcs_mii), GFP_KERNEL);
+		if (!qpcs_mii) {
+			of_node_put(mii_np);
+			return -ENOMEM;
+		}
+
+		qpcs_mii->qpcs = qpcs;
+		qpcs_mii->index = index;
+		qpcs_mii->pcs.ops = &ipq_pcs_phylink_ops;
+		qpcs_mii->pcs.neg_mode = true;
+		qpcs_mii->pcs.poll = true;
+
+		qpcs_mii->rx_clk = devm_get_clk_from_child(dev, mii_np, "rx");
+		if (IS_ERR(qpcs_mii->rx_clk)) {
+			dev_err(dev, "Failed to get MII %d RX clock\n", index);
+			of_node_put(mii_np);
+			return PTR_ERR(qpcs_mii->rx_clk);
+		}
+
+		qpcs_mii->tx_clk = devm_get_clk_from_child(dev, mii_np, "tx");
+		if (IS_ERR(qpcs_mii->tx_clk)) {
+			dev_err(dev, "Failed to get MII %d TX clock\n", index);
+			of_node_put(mii_np);
+			return PTR_ERR(qpcs_mii->tx_clk);
+		}
+
+		qpcs->qpcs_mii[index] = qpcs_mii;
+	}
+
+	return 0;
+}
+
 static unsigned long ipq_pcs_clk_rate_get(struct ipq_pcs *qpcs)
 {
 	switch (qpcs->interface) {
@@ -219,6 +675,12 @@ static int ipq9574_pcs_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = ipq_pcs_create_miis(qpcs);
+	if (ret)
+		return ret;
+
+	mutex_init(&qpcs->config_lock);
+
 	platform_set_drvdata(pdev, qpcs);
 
 	return 0;
diff --git a/include/linux/pcs/pcs-qcom-ipq9574.h b/include/linux/pcs/pcs-qcom-ipq9574.h
new file mode 100644
index 000000000000..5469a81b4482
--- /dev/null
+++ b/include/linux/pcs/pcs-qcom-ipq9574.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ */
+
+#ifndef __LINUX_PCS_QCOM_IPQ9574_H
+#define __LINUX_PCS_QCOM_IPQ9574_H
+
+struct device_node;
+struct phylink_pcs;
+
+struct phylink_pcs *ipq_pcs_get(struct device_node *np);
+void ipq_pcs_put(struct phylink_pcs *pcs);
+
+#endif /* __LINUX_PCS_QCOM_IPQ9574_H */

-- 
2.34.1

Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Russell King (Oracle)]

On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
> +static int ipq_pcs_enable(struct phylink_pcs *pcs)
> +{
> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> +	int index = qpcs_mii->index;
> +	int ret;
> +
> +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
> +	if (ret) {
> +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
> +	if (ret) {
> +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
> +		clk_disable_unprepare(qpcs_mii->rx_clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ipq_pcs_disable(struct phylink_pcs *pcs)
> +{
> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> +
> +	if (__clk_is_enabled(qpcs_mii->rx_clk))
> +		clk_disable_unprepare(qpcs_mii->rx_clk);
> +
> +	if (__clk_is_enabled(qpcs_mii->tx_clk))
> +		clk_disable_unprepare(qpcs_mii->tx_clk);

Why do you need the __clk_is_enabled() calls here? Phylink should be
calling pcs_enable() once when the PCS when starting to use the PCS,
and then pcs_disable() when it stops using it - it won't call
pcs_disable() without a preceeding call to pcs_enable().

Are you seeing something different?

> +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
> +			      struct phylink_link_state *state)
> +{
> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> +	int index = qpcs_mii->index;
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		ipq_pcs_get_state_sgmii(qpcs, index, state);
> +		break;
> +	default:
> +		break;
...
> +static int ipq_pcs_config(struct phylink_pcs *pcs,
> +			  unsigned int neg_mode,
> +			  phy_interface_t interface,
> +			  const unsigned long *advertising,
> +			  bool permit)
> +{
> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> +	int index = qpcs_mii->index;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
> +	default:
> +		dev_err(qpcs->dev,
> +			"Unsupported interface %s\n", phy_modes(interface));
> +		return -EOPNOTSUPP;
> +	};
> +}
> +
> +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
> +			    unsigned int neg_mode,
> +			    phy_interface_t interface,
> +			    int speed, int duplex)
> +{
> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> +	int index = qpcs_mii->index;
> +	int ret;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
> +						   neg_mode, speed);
> +		break;
> +	default:
> +		dev_err(qpcs->dev,
> +			"Unsupported interface %s\n", phy_modes(interface));
> +		return;
> +	}

So you only support SGMII and QSGMII. Rather than checking this in every
method implementation, instead provide a .pcs_validate method that
returns an error for unsupported interfaces please.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Lei Wei]

On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
> On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
>> +static int ipq_pcs_enable(struct phylink_pcs *pcs)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
>> +	if (ret) {
>> +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
>> +	if (ret) {
>> +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ipq_pcs_disable(struct phylink_pcs *pcs)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +
>> +	if (__clk_is_enabled(qpcs_mii->rx_clk))
>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>> +
>> +	if (__clk_is_enabled(qpcs_mii->tx_clk))
>> +		clk_disable_unprepare(qpcs_mii->tx_clk);
> 
> Why do you need the __clk_is_enabled() calls here? Phylink should be
> calling pcs_enable() once when the PCS when starting to use the PCS,
> and then pcs_disable() when it stops using it - it won't call
> pcs_disable() without a preceeding call to pcs_enable().
> 
> Are you seeing something different?
> 

Yes, understand that phylink won't call pcs_disable() without a 
preceeding call to pcs_enable(). However, the "clk_prepare_enable" may 
fail in the pcs_enable() method, so I added the __clk_is_enabled() check 
in pcs_disable() method. This is because the phylink_major_config() 
function today does not interpret the return value of phylink_pcs_enable().

>> +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
>> +			      struct phylink_link_state *state)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		ipq_pcs_get_state_sgmii(qpcs, index, state);
>> +		break;
>> +	default:
>> +		break;
> ...
>> +static int ipq_pcs_config(struct phylink_pcs *pcs,
>> +			  unsigned int neg_mode,
>> +			  phy_interface_t interface,
>> +			  const unsigned long *advertising,
>> +			  bool permit)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +
>> +	switch (interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
>> +	default:
>> +		dev_err(qpcs->dev,
>> +			"Unsupported interface %s\n", phy_modes(interface));
>> +		return -EOPNOTSUPP;
>> +	};
>> +}
>> +
>> +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
>> +			    unsigned int neg_mode,
>> +			    phy_interface_t interface,
>> +			    int speed, int duplex)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +	int ret;
>> +
>> +	switch (interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
>> +						   neg_mode, speed);
>> +		break;
>> +	default:
>> +		dev_err(qpcs->dev,
>> +			"Unsupported interface %s\n", phy_modes(interface));
>> +		return;
>> +	}
> 
> So you only support SGMII and QSGMII. Rather than checking this in every
> method implementation, instead provide a .pcs_validate method that
> returns an error for unsupported interfaces please.
> 

Yes, I can add the pcs_validate() method to validate the link 
configurations. This will catch invalid interface mode during the PCS 
initialization time, earlier than the pcs_config and pcs_link_up contexts.

But after of the PCS init, if at a later point the PHY interface mode 
changes, it seems phylink today is not calling the pcs_validate() op to 
validate the changed new interface mode at the time of 
"phylink_resolve". (Hope my understanding is correct).
So, In the pcs ops methods, I will keep the switch case to check and 
handle the unsupported interface modes.

> Thanks.
> 

Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Russell King (Oracle)]

On Sat, Dec 07, 2024 at 12:20:25AM +0800, Lei Wei wrote:
> On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
> > On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
> > > +static int ipq_pcs_enable(struct phylink_pcs *pcs)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > > +	int index = qpcs_mii->index;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
> > > +	if (ret) {
> > > +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
> > > +	if (ret) {
> > > +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
> > > +		clk_disable_unprepare(qpcs_mii->rx_clk);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void ipq_pcs_disable(struct phylink_pcs *pcs)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +
> > > +	if (__clk_is_enabled(qpcs_mii->rx_clk))
> > > +		clk_disable_unprepare(qpcs_mii->rx_clk);
> > > +
> > > +	if (__clk_is_enabled(qpcs_mii->tx_clk))
> > > +		clk_disable_unprepare(qpcs_mii->tx_clk);
> > 
> > Why do you need the __clk_is_enabled() calls here? Phylink should be
> > calling pcs_enable() once when the PCS when starting to use the PCS,
> > and then pcs_disable() when it stops using it - it won't call
> > pcs_disable() without a preceeding call to pcs_enable().
> > 
> > Are you seeing something different?
> 
> Yes, understand that phylink won't call pcs_disable() without a preceeding
> call to pcs_enable(). However, the "clk_prepare_enable" may fail in the
> pcs_enable() method, so I added the __clk_is_enabled() check in
> pcs_disable() method. This is because the phylink_major_config() function
> today does not interpret the return value of phylink_pcs_enable().

Right, because failure is essentially fatal in that path - we have no
context to return an error. I suppose we could stop processing at
that point, but then it brings up the question of how to unwind anything
we've already done, which is basically impossible at that point.

> > > +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
> > > +			      struct phylink_link_state *state)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > > +	int index = qpcs_mii->index;
> > > +
> > > +	switch (state->interface) {
> > > +	case PHY_INTERFACE_MODE_SGMII:
> > > +	case PHY_INTERFACE_MODE_QSGMII:
> > > +		ipq_pcs_get_state_sgmii(qpcs, index, state);
> > > +		break;
> > > +	default:
> > > +		break;
> > ...
> > > +static int ipq_pcs_config(struct phylink_pcs *pcs,
> > > +			  unsigned int neg_mode,
> > > +			  phy_interface_t interface,
> > > +			  const unsigned long *advertising,
> > > +			  bool permit)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > > +	int index = qpcs_mii->index;
> > > +
> > > +	switch (interface) {
> > > +	case PHY_INTERFACE_MODE_SGMII:
> > > +	case PHY_INTERFACE_MODE_QSGMII:
> > > +		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
> > > +	default:
> > > +		dev_err(qpcs->dev,
> > > +			"Unsupported interface %s\n", phy_modes(interface));
> > > +		return -EOPNOTSUPP;
> > > +	};
> > > +}
> > > +
> > > +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
> > > +			    unsigned int neg_mode,
> > > +			    phy_interface_t interface,
> > > +			    int speed, int duplex)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > > +	int index = qpcs_mii->index;
> > > +	int ret;
> > > +
> > > +	switch (interface) {
> > > +	case PHY_INTERFACE_MODE_SGMII:
> > > +	case PHY_INTERFACE_MODE_QSGMII:
> > > +		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
> > > +						   neg_mode, speed);
> > > +		break;
> > > +	default:
> > > +		dev_err(qpcs->dev,
> > > +			"Unsupported interface %s\n", phy_modes(interface));
> > > +		return;
> > > +	}
> > 
> > So you only support SGMII and QSGMII. Rather than checking this in every
> > method implementation, instead provide a .pcs_validate method that
> > returns an error for unsupported interfaces please.
> > 
> 
> Yes, I can add the pcs_validate() method to validate the link
> configurations. This will catch invalid interface mode during the PCS
> initialization time, earlier than the pcs_config and pcs_link_up contexts.
> 
> But after of the PCS init, if at a later point the PHY interface mode
> changes, it seems phylink today is not calling the pcs_validate() op to
> validate the changed new interface mode at the time of "phylink_resolve".

... because by that time it's way too late. Phylink will have already
looked at what the PHY can do when the PHY is attached, and eliminated
any link modes that would cause an invalid configuration (provided
phylink knows what the PHY is capable of.)

However, that assumes phylink knows what the details are of the PCS,
which is dependent on the .pcs_validate method being implemented.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Lei Wei]

On 12/7/2024 12:28 AM, Russell King (Oracle) wrote:
> On Sat, Dec 07, 2024 at 12:20:25AM +0800, Lei Wei wrote:
>> On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
>>> On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
>>>> +static int ipq_pcs_enable(struct phylink_pcs *pcs)
>>>> +{
>>>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> +	int index = qpcs_mii->index;
>>>> +	int ret;
>>>> +
>>>> +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
>>>> +	if (ret) {
>>>> +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
>>>> +	if (ret) {
>>>> +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
>>>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void ipq_pcs_disable(struct phylink_pcs *pcs)
>>>> +{
>>>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +
>>>> +	if (__clk_is_enabled(qpcs_mii->rx_clk))
>>>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>>>> +
>>>> +	if (__clk_is_enabled(qpcs_mii->tx_clk))
>>>> +		clk_disable_unprepare(qpcs_mii->tx_clk);
>>>
>>> Why do you need the __clk_is_enabled() calls here? Phylink should be
>>> calling pcs_enable() once when the PCS when starting to use the PCS,
>>> and then pcs_disable() when it stops using it - it won't call
>>> pcs_disable() without a preceeding call to pcs_enable().
>>>
>>> Are you seeing something different?
>>
>> Yes, understand that phylink won't call pcs_disable() without a preceeding
>> call to pcs_enable(). However, the "clk_prepare_enable" may fail in the
>> pcs_enable() method, so I added the __clk_is_enabled() check in
>> pcs_disable() method. This is because the phylink_major_config() function
>> today does not interpret the return value of phylink_pcs_enable().
> 
> Right, because failure is essentially fatal in that path - we have no
> context to return an error. I suppose we could stop processing at
> that point, but then it brings up the question of how to unwind anything
> we've already done, which is basically impossible at that point.
> 

Sure, understand. I will remove the checks.

>>>> +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
>>>> +			      struct phylink_link_state *state)
>>>> +{
>>>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> +	int index = qpcs_mii->index;
>>>> +
>>>> +	switch (state->interface) {
>>>> +	case PHY_INTERFACE_MODE_SGMII:
>>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>>> +		ipq_pcs_get_state_sgmii(qpcs, index, state);
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>> ...
>>>> +static int ipq_pcs_config(struct phylink_pcs *pcs,
>>>> +			  unsigned int neg_mode,
>>>> +			  phy_interface_t interface,
>>>> +			  const unsigned long *advertising,
>>>> +			  bool permit)
>>>> +{
>>>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> +	int index = qpcs_mii->index;
>>>> +
>>>> +	switch (interface) {
>>>> +	case PHY_INTERFACE_MODE_SGMII:
>>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>>> +		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
>>>> +	default:
>>>> +		dev_err(qpcs->dev,
>>>> +			"Unsupported interface %s\n", phy_modes(interface));
>>>> +		return -EOPNOTSUPP;
>>>> +	};
>>>> +}
>>>> +
>>>> +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
>>>> +			    unsigned int neg_mode,
>>>> +			    phy_interface_t interface,
>>>> +			    int speed, int duplex)
>>>> +{
>>>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> +	int index = qpcs_mii->index;
>>>> +	int ret;
>>>> +
>>>> +	switch (interface) {
>>>> +	case PHY_INTERFACE_MODE_SGMII:
>>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>>> +		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
>>>> +						   neg_mode, speed);
>>>> +		break;
>>>> +	default:
>>>> +		dev_err(qpcs->dev,
>>>> +			"Unsupported interface %s\n", phy_modes(interface));
>>>> +		return;
>>>> +	}
>>>
>>> So you only support SGMII and QSGMII. Rather than checking this in every
>>> method implementation, instead provide a .pcs_validate method that
>>> returns an error for unsupported interfaces please.
>>>
>>
>> Yes, I can add the pcs_validate() method to validate the link
>> configurations. This will catch invalid interface mode during the PCS
>> initialization time, earlier than the pcs_config and pcs_link_up contexts.
>>
>> But after of the PCS init, if at a later point the PHY interface mode
>> changes, it seems phylink today is not calling the pcs_validate() op to
>> validate the changed new interface mode at the time of "phylink_resolve".
> 
> ... because by that time it's way too late. Phylink will have already
> looked at what the PHY can do when the PHY is attached, and eliminated
> any link modes that would cause an invalid configuration (provided
> phylink knows what the PHY is capable of.)
> 
> However, that assumes phylink knows what the details are of the PCS,
> which is dependent on the .pcs_validate method being implemented.
> 

Yes, agree that pcs_validate() is necessary to be implemented and 
Phylink will validate the PHY when the PHY is attached. I will implement 
this method in the next update. Thanks for pointing to the details here.

We will also remove the debug error print for the 'default' case. 
However, I would like to retain the switch statement since we have 
different routines for SGMII/USXGMII modes, and we plan to add more 
interfaces modes later when we enhance the driver for other IPQ SoC.

[PATCH net-next v2 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support

[Lei Wei]

USXGMII mode is enabled by PCS when 10Gbps PHYs are connected, such as
Aquantia 10Gbps PHY.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
 drivers/net/pcs/pcs-qcom-ipq9574.c | 177 +++++++++++++++++++++++++++++++++++++
 1 file changed, 177 insertions(+)

diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
index 3608f5506477..ad5e9551675a 100644
--- a/drivers/net/pcs/pcs-qcom-ipq9574.c
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -26,6 +26,7 @@
 #define PCS_MODE_SEL_MASK		GENMASK(12, 8)
 #define PCS_MODE_SGMII			FIELD_PREP(PCS_MODE_SEL_MASK, 0x4)
 #define PCS_MODE_QSGMII			FIELD_PREP(PCS_MODE_SEL_MASK, 0x1)
+#define PCS_MODE_XPCS			FIELD_PREP(PCS_MODE_SEL_MASK, 0x10)
 
 #define PCS_MII_CTRL(x)			(0x480 + 0x18 * (x))
 #define PCS_MII_ADPT_RESET		BIT(11)
@@ -54,6 +55,35 @@
 					 FIELD_PREP(GENMASK(9, 2), \
 					 FIELD_GET(XPCS_INDIRECT_ADDR_L, reg)))
 
+#define XPCS_DIG_CTRL			0x38000
+#define XPCS_USXG_ADPT_RESET		BIT(10)
+#define XPCS_USXG_EN			BIT(9)
+
+#define XPCS_MII_CTRL			0x1f0000
+#define XPCS_MII_AN_EN			BIT(12)
+#define XPCS_DUPLEX_FULL		BIT(8)
+#define XPCS_SPEED_MASK			(BIT(13) | BIT(6) | BIT(5))
+#define XPCS_SPEED_10000		(BIT(13) | BIT(6))
+#define XPCS_SPEED_5000			(BIT(13) | BIT(5))
+#define XPCS_SPEED_2500			BIT(5)
+#define XPCS_SPEED_1000			BIT(6)
+#define XPCS_SPEED_100			BIT(13)
+#define XPCS_SPEED_10			0
+
+#define XPCS_MII_AN_CTRL		0x1f8001
+#define XPCS_MII_AN_8BIT		BIT(8)
+
+#define XPCS_MII_AN_INTR_STS		0x1f8002
+#define XPCS_USXG_AN_LINK_STS		BIT(14)
+#define XPCS_USXG_AN_DUPLEX_FULL	BIT(13)
+#define XPCS_USXG_AN_SPEED_MASK		GENMASK(12, 10)
+#define XPCS_USXG_AN_SPEED_10		0
+#define XPCS_USXG_AN_SPEED_100		1
+#define XPCS_USXG_AN_SPEED_1000		2
+#define XPCS_USXG_AN_SPEED_2500		4
+#define XPCS_USXG_AN_SPEED_5000		5
+#define XPCS_USXG_AN_SPEED_10000	3
+
 /* Per PCS MII private data */
 struct ipq_pcs_mii {
 	struct ipq_pcs *qpcs;
@@ -126,9 +156,57 @@ static void ipq_pcs_get_state_sgmii(struct ipq_pcs *qpcs,
 		state->duplex = DUPLEX_HALF;
 }
 
+static void ipq_pcs_get_state_usxgmii(struct ipq_pcs *qpcs,
+				      struct phylink_link_state *state)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(qpcs->regmap, XPCS_MII_AN_INTR_STS, &val);
+	if (ret) {
+		state->link = 0;
+		return;
+	}
+
+	state->link = !!(val & XPCS_USXG_AN_LINK_STS);
+
+	if (!state->link)
+		return;
+
+	switch (FIELD_GET(XPCS_USXG_AN_SPEED_MASK, val)) {
+	case XPCS_USXG_AN_SPEED_10000:
+		state->speed = SPEED_10000;
+		break;
+	case XPCS_USXG_AN_SPEED_5000:
+		state->speed = SPEED_5000;
+		break;
+	case XPCS_USXG_AN_SPEED_2500:
+		state->speed = SPEED_2500;
+		break;
+	case XPCS_USXG_AN_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	case XPCS_USXG_AN_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case XPCS_USXG_AN_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	default:
+		state->link = false;
+		return;
+	}
+
+	if (val & XPCS_USXG_AN_DUPLEX_FULL)
+		state->duplex = DUPLEX_FULL;
+	else
+		state->duplex = DUPLEX_HALF;
+}
+
 static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
 			       phy_interface_t interface)
 {
+	unsigned long rate = 125000000;
 	unsigned int val;
 	int ret;
 
@@ -140,6 +218,10 @@ static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
 	case PHY_INTERFACE_MODE_QSGMII:
 		val = PCS_MODE_QSGMII;
 		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		val = PCS_MODE_XPCS;
+		rate = 312500000;
+		break;
 	default:
 		dev_err(qpcs->dev,
 			"Unsupported interface %s\n", phy_modes(interface));
@@ -174,6 +256,21 @@ static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
 
 	qpcs->interface = interface;
 
+	/* Configure the RX and TX clock to NSSCC as 125M or 312.5M based
+	 * on current interface mode.
+	 */
+	ret = clk_set_rate(qpcs->rx_hw.clk, rate);
+	if (ret) {
+		dev_err(qpcs->dev, "Failed to set RX clock rate\n");
+		return ret;
+	}
+
+	ret = clk_set_rate(qpcs->tx_hw.clk, rate);
+	if (ret) {
+		dev_err(qpcs->dev, "Failed to set TX clock rate\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -211,6 +308,35 @@ static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
 				  PCS_MII_FORCE_MODE, PCS_MII_FORCE_MODE);
 }
 
+static int ipq_pcs_config_usxgmii(struct ipq_pcs *qpcs)
+{
+	int ret;
+
+	/* Configure the XPCS for USXGMII mode if required */
+	if (qpcs->interface != PHY_INTERFACE_MODE_USXGMII) {
+		ret = ipq_pcs_config_mode(qpcs, PHY_INTERFACE_MODE_USXGMII);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(qpcs->regmap, XPCS_DIG_CTRL,
+					 XPCS_USXG_EN, XPCS_USXG_EN);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(qpcs->regmap, XPCS_MII_AN_CTRL,
+					 XPCS_MII_AN_8BIT, XPCS_MII_AN_8BIT);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(qpcs->regmap, XPCS_MII_CTRL,
+					 XPCS_MII_AN_EN, XPCS_MII_AN_EN);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
 					int index,
 					unsigned int neg_mode,
@@ -253,6 +379,49 @@ static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
 				  PCS_MII_ADPT_RESET, PCS_MII_ADPT_RESET);
 }
 
+static int ipq_pcs_link_up_config_usxgmii(struct ipq_pcs *qpcs, int speed)
+{
+	unsigned int val;
+	int ret;
+
+	switch (speed) {
+	case SPEED_10000:
+		val = XPCS_SPEED_10000;
+		break;
+	case SPEED_5000:
+		val = XPCS_SPEED_5000;
+		break;
+	case SPEED_2500:
+		val = XPCS_SPEED_2500;
+		break;
+	case SPEED_1000:
+		val = XPCS_SPEED_1000;
+		break;
+	case SPEED_100:
+		val = XPCS_SPEED_100;
+		break;
+	case SPEED_10:
+		val = XPCS_SPEED_10;
+		break;
+	default:
+		dev_err(qpcs->dev, "Invalid USXGMII speed %d\n", speed);
+		return -EINVAL;
+	}
+
+	/* USXGMII only support full duplex mode */
+	val |= XPCS_DUPLEX_FULL;
+
+	/* Configure XPCS speed */
+	ret = regmap_update_bits(qpcs->regmap, XPCS_MII_CTRL,
+				 XPCS_SPEED_MASK | XPCS_DUPLEX_FULL, val);
+	if (ret)
+		return ret;
+
+	/* XPCS adapter reset */
+	return regmap_update_bits(qpcs->regmap, XPCS_DIG_CTRL,
+				  XPCS_USXG_ADPT_RESET, XPCS_USXG_ADPT_RESET);
+}
+
 static int ipq_pcs_enable(struct phylink_pcs *pcs)
 {
 	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
@@ -299,6 +468,9 @@ static void ipq_pcs_get_state(struct phylink_pcs *pcs,
 	case PHY_INTERFACE_MODE_QSGMII:
 		ipq_pcs_get_state_sgmii(qpcs, index, state);
 		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		ipq_pcs_get_state_usxgmii(qpcs, state);
+		break;
 	default:
 		break;
 	}
@@ -325,6 +497,8 @@ static int ipq_pcs_config(struct phylink_pcs *pcs,
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_QSGMII:
 		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
+	case PHY_INTERFACE_MODE_USXGMII:
+		return ipq_pcs_config_usxgmii(qpcs);
 	default:
 		dev_err(qpcs->dev,
 			"Unsupported interface %s\n", phy_modes(interface));
@@ -348,6 +522,9 @@ static void ipq_pcs_link_up(struct phylink_pcs *pcs,
 		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
 						   neg_mode, speed);
 		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		ret = ipq_pcs_link_up_config_usxgmii(qpcs, speed);
+		break;
 	default:
 		dev_err(qpcs->dev,
 			"Unsupported interface %s\n", phy_modes(interface));

-- 
2.34.1

Re: [PATCH net-next v2 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support

[Russell King (Oracle)]

On Wed, Dec 04, 2024 at 10:43:56PM +0800, Lei Wei wrote:
> +static int ipq_pcs_link_up_config_usxgmii(struct ipq_pcs *qpcs, int speed)
> +{
...
> +	/* USXGMII only support full duplex mode */
> +	val |= XPCS_DUPLEX_FULL;

Again... this restriction needs to be implemented in .pcs_validate() by
knocking out the half-duplex link modes when using USXGMII mode.

.pcs_validate() needs to be implemented whenever the PCS has
restrictions beyond what is standard for the PHY interface mode.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support

[Lei Wei]

On 12/4/2024 11:38 PM, Russell King (Oracle) wrote:
> On Wed, Dec 04, 2024 at 10:43:56PM +0800, Lei Wei wrote:
>> +static int ipq_pcs_link_up_config_usxgmii(struct ipq_pcs *qpcs, int speed)
>> +{
> ...
>> +	/* USXGMII only support full duplex mode */
>> +	val |= XPCS_DUPLEX_FULL;
> 
> Again... this restriction needs to be implemented in .pcs_validate() by
> knocking out the half-duplex link modes when using USXGMII mode.
> 
> .pcs_validate() needs to be implemented whenever the PCS has
> restrictions beyond what is standard for the PHY interface mode.
> 

Currently, it seems there is no phylink_validate() call in 
phylink_resolve(), to validate the resolved duplex/speed which is 
notified by phydev when the PHY is linked up. So I am thinking to add 
this duplex check in this link_up op, and return an appropriate error in 
case of half-duplex. (Kindly correct me if I am wrong).

> Thanks.
> 

Re: [PATCH net-next v2 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support

[Russell King (Oracle)]

On Sat, Dec 07, 2024 at 12:20:57AM +0800, Lei Wei wrote:
> On 12/4/2024 11:38 PM, Russell King (Oracle) wrote:
> > On Wed, Dec 04, 2024 at 10:43:56PM +0800, Lei Wei wrote:
> > > +static int ipq_pcs_link_up_config_usxgmii(struct ipq_pcs *qpcs, int speed)
> > > +{
> > ...
> > > +	/* USXGMII only support full duplex mode */
> > > +	val |= XPCS_DUPLEX_FULL;
> > 
> > Again... this restriction needs to be implemented in .pcs_validate() by
> > knocking out the half-duplex link modes when using USXGMII mode.
> > 
> > .pcs_validate() needs to be implemented whenever the PCS has
> > restrictions beyond what is standard for the PHY interface mode.
> > 
> 
> Currently, it seems there is no phylink_validate() call in
> phylink_resolve(), to validate the resolved duplex/speed which is notified
> by phydev when the PHY is linked up. So I am thinking to add this duplex
> check in this link_up op, and return an appropriate error in case of
> half-duplex. (Kindly correct me if I am wrong).

Doing validation at that point is way too late.

We don't want the PHY e.g. even advertising a half-duplex link mode if
the system as a whole can not support half-duplex modes. If the system
can't support half-duplex, then trying to trap it out at resolve time
would be way too late - the media has already negotiated a half-duplex
link, and that's that.

Instead, phylink takes the approach of restricting the media
advertisement according to the properties of the system, thereby
preventing invalid configurations _way_ before we get to autoneg
completion and calling phylink_resolve().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support

[Lei Wei]

On 12/7/2024 12:31 AM, Russell King (Oracle) wrote:
> On Sat, Dec 07, 2024 at 12:20:57AM +0800, Lei Wei wrote:
>> On 12/4/2024 11:38 PM, Russell King (Oracle) wrote:
>>> On Wed, Dec 04, 2024 at 10:43:56PM +0800, Lei Wei wrote:
>>>> +static int ipq_pcs_link_up_config_usxgmii(struct ipq_pcs *qpcs, int speed)
>>>> +{
>>> ...
>>>> +	/* USXGMII only support full duplex mode */
>>>> +	val |= XPCS_DUPLEX_FULL;
>>>
>>> Again... this restriction needs to be implemented in .pcs_validate() by
>>> knocking out the half-duplex link modes when using USXGMII mode.
>>>
>>> .pcs_validate() needs to be implemented whenever the PCS has
>>> restrictions beyond what is standard for the PHY interface mode.
>>>
>>
>> Currently, it seems there is no phylink_validate() call in
>> phylink_resolve(), to validate the resolved duplex/speed which is notified
>> by phydev when the PHY is linked up. So I am thinking to add this duplex
>> check in this link_up op, and return an appropriate error in case of
>> half-duplex. (Kindly correct me if I am wrong).
> 
> Doing validation at that point is way too late.
> 
> We don't want the PHY e.g. even advertising a half-duplex link mode if
> the system as a whole can not support half-duplex modes. If the system
> can't support half-duplex, then trying to trap it out at resolve time
> would be way too late - the media has already negotiated a half-duplex
> link, and that's that.
> 
> Instead, phylink takes the approach of restricting the media
> advertisement according to the properties of the system, thereby
> preventing invalid configurations _way_ before we get to autoneg
> completion and calling phylink_resolve().
> 

Yes, understand. I will avoid advertising half-duplex in the 
pcs_validate() method for USXGMII. Thanks for the suggestion.

[PATCH net-next v2 5/5] MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver

[Lei Wei]

Add maintainer for the Ethernet PCS driver supported for Qualcomm
IPQ9574 SoC.

Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c27f3190737f..c76348387326 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19164,6 +19164,15 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/regulator/vqmmc-ipq4019-regulator.yaml
 F:	drivers/regulator/vqmmc-ipq4019-regulator.c
 
+QUALCOMM IPQ9574 Ethernet PCS DRIVER
+M:	Lei Wei <quic_leiwei@quicinc.com>
+L:	netdev@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
+F:	drivers/net/pcs/pcs-qcom-ipq9574.c
+F:	include/dt-bindings/net/qcom,ipq9574-pcs.h
+F:	include/linux/pcs/pcs-qcom-ipq9574.h
+
 QUALCOMM NAND CONTROLLER DRIVER
 M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 L:	linux-mtd@lists.infradead.org

-- 
2.34.1