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

[PATCH net-next 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>
---
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-ipq: Add PCS create and phylink operations for IPQ9574
      net: pcs: qcom-ipq: Add USXGMII interface mode for IPQ9574
      MAINTAINERS: Add maintainer for Qualcomm IPQ PCS driver

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

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

[PATCH net-next 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         | 230 +++++++++++++++++++++
 include/dt-bindings/net/pcs-qcom-ipq.h             |  15 ++
 2 files changed, 245 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..a33873c7ad73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
@@ -0,0 +1,230 @@
+# 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 IPQ 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 first 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/pcs-qcom-ipq.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: mii_rx
+          - const: mii_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>
+
+    pcs0: 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>;
+
+        pcs0_mii0: pcs-mii@0 {
+            reg = <0>;
+            clocks = <&nsscc 116>,
+                     <&nsscc 117>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+
+        pcs0_mii1: pcs-mii@1 {
+            reg = <1>;
+            clocks = <&nsscc 118>,
+                     <&nsscc 119>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+
+        pcs0_mii2: pcs-mii@2 {
+            reg = <2>;
+            clocks = <&nsscc 120>,
+                     <&nsscc 121>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+
+        pcs0_mii3: pcs-mii@3 {
+            reg = <3>;
+            clocks = <&nsscc 122>,
+                     <&nsscc 123>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+    };
+
+    pcs1: ethernet-pcs@7a10000 {
+        compatible = "qcom,ipq9574-pcs";
+        reg = <0x7a10000 0x10000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
+                 <&gcc GCC_UNIPHY1_AHB_CLK>;
+        clock-names = "sys",
+                      "ahb";
+        #clock-cells = <1>;
+
+        pcs1_mii0: pcs-mii@0 {
+            reg = <0>;
+            clocks = <&nsscc 124>,
+                     <&nsscc 125>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+    };
+
+    pcs2: ethernet-pcs@7a20000 {
+        compatible = "qcom,ipq9574-pcs";
+        reg = <0x7a20000 0x10000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        clocks = <&gcc GCC_UNIPHY2_SYS_CLK>,
+                 <&gcc GCC_UNIPHY2_AHB_CLK>;
+        clock-names = "sys",
+                      "ahb";
+        #clock-cells = <1>;
+
+        pcs2_mii0: pcs-mii@0 {
+            reg = <0>;
+            clocks = <&nsscc 126>,
+                     <&nsscc 127>;
+            clock-names = "mii_rx",
+                          "mii_tx";
+        };
+    };
diff --git a/include/dt-bindings/net/pcs-qcom-ipq.h b/include/dt-bindings/net/pcs-qcom-ipq.h
new file mode 100644
index 000000000000..8d9124ffd75d
--- /dev/null
+++ b/include/dt-bindings/net/pcs-qcom-ipq.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 IPQ PCS
+ */
+
+#ifndef _DT_BINDINGS_PCS_QCOM_IPQ_H
+#define _DT_BINDINGS_PCS_QCOM_IPQ_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_IPQ_H */

-- 
2.34.1

[PATCH net-next 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-ipq.c | 244 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index f6aa437473de..1053326958ee 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_IPQ
+	tristate "Qualcomm IPQ PCS"
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	depends on HAS_IOMEM
+	help
+	  This module provides driver for UNIPHY PCS available on Qualcomm IPQ
+	  SoC such as IPQ9574. 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..399750c7c293 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_IPQ)	+= pcs-qcom-ipq.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-qcom-ipq.c b/drivers/net/pcs/pcs-qcom-ipq.c
new file mode 100644
index 000000000000..e065bc61cd14
--- /dev/null
+++ b/drivers/net/pcs/pcs-qcom-ipq.c
@@ -0,0 +1,244 @@
+// 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/pcs-qcom-ipq.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)))
+
+/* Private data for the PCS instance */
+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 qpcs_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 qpcs_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 = &qpcs_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 = &qpcs_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 ipq_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 ipq_pcs_of_mtable[] = {
+	{ .compatible = "qcom,ipq9574-pcs" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ipq_pcs_of_mtable);
+
+static struct platform_driver ipq_pcs_driver = {
+	.driver = {
+		.name = "ipq_pcs",
+		.of_match_table = ipq_pcs_of_mtable,
+	},
+	.probe = ipq_pcs_probe,
+};
+module_platform_driver(ipq_pcs_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm IPQ PCS driver");
+MODULE_AUTHOR("Lei Wei <quic_leiwei@quicinc.com>");

-- 
2.34.1

[PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

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

a.) PCS instance create and destroy APIs. The network driver calls
the PCS create API to create and associate the PCS instance with
the port MAC. The PCS MII RX and TX clocks are also enabled in the
PCS create API.
b.) PCS phylink operations for SGMII/QSGMII interface modes.

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

diff --git a/drivers/net/pcs/pcs-qcom-ipq.c b/drivers/net/pcs/pcs-qcom-ipq.c
index e065bc61cd14..dd432303b549 100644
--- a/drivers/net/pcs/pcs-qcom-ipq.c
+++ b/drivers/net/pcs/pcs-qcom-ipq.c
@@ -6,12 +6,49 @@
 #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-ipq.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/net/pcs-qcom-ipq.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_MODE_AN_MODE		BIT(0)
+
+#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_MII_STS_PAUSE_TX_EN		BIT(1)
+#define PCS_MII_STS_PAUSE_RX_EN		BIT(0)
+
+#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)
@@ -27,12 +64,428 @@ 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;
 };
 
+/* PCS MII clock ID */
+enum {
+	PCS_MII_RX_CLK,
+	PCS_MII_TX_CLK,
+	PCS_MII_CLK_MAX
+};
+
+/* PCS MII clock name */
+static const char *const pcs_mii_clk_name[PCS_MII_CLK_MAX] = {
+	"mii_rx",
+	"mii_tx",
+};
+
+/* Per PCS MII private data */
+struct ipq_pcs_mii {
+	struct ipq_pcs *qpcs;
+	struct phylink_pcs pcs;
+	int index;
+
+	/* Rx/Tx clocks from NSSCC to PCS MII */
+	struct clk *clk[PCS_MII_CLK_MAX];
+};
+
+#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;
+
+	if (val & PCS_MII_STS_PAUSE_TX_EN)
+		state->pause |= MLO_PAUSE_TX;
+
+	if (val & PCS_MII_STS_PAUSE_RX_EN)
+		state->pause |= MLO_PAUSE_RX;
+}
+
+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:
+		/* Select Qualcomm SGMII AN mode */
+		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
+					 PCS_MODE_SGMII);
+		if (ret)
+			return ret;
+		break;
+	case PHY_INTERFACE_MODE_QSGMII:
+		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
+					 PCS_MODE_QSGMII);
+		if (ret)
+			return ret;
+		break;
+	default:
+		dev_err(qpcs->dev,
+			"Unsupported interface %s\n", phy_modes(interface));
+		return -EOPNOTSUPP;
+	}
+
+	/* 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. This is required only for interface modes
+	 * such as QSGMII.
+	 */
+	if (interface == PHY_INTERFACE_MODE_QSGMII)
+		mutex_lock(&qpcs->config_lock);
+
+	if (qpcs->interface != interface) {
+		ret = ipq_pcs_config_mode(qpcs, interface);
+		if (ret)
+			goto err;
+	}
+
+	if (interface == PHY_INTERFACE_MODE_QSGMII)
+		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);
+
+err:
+	if (interface == PHY_INTERFACE_MODE_QSGMII)
+		mutex_unlock(&qpcs->config_lock);
+
+	return ret;
+}
+
+static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
+					int index,
+					unsigned int neg_mode,
+					int speed)
+{
+	int ret;
+
+	/* PCS speed need not be configured if in-band autoneg is enabled */
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+		goto pcs_adapter_reset;
+
+	/* PCS speed set for force mode */
+	switch (speed) {
+	case SPEED_1000:
+		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+					 PCS_MII_SPEED_MASK,
+					 PCS_MII_SPEED_1000);
+		if (ret)
+			return ret;
+		break;
+	case SPEED_100:
+		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_100);
+		if (ret)
+			return ret;
+		break;
+	case SPEED_10:
+		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_10);
+		if (ret)
+			return ret;
+		break;
+	default:
+		dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
+		return -EINVAL;
+	}
+
+pcs_adapter_reset:
+	/* 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 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(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_get_state = ipq_pcs_get_state,
+	.pcs_config = ipq_pcs_config,
+	.pcs_link_up = ipq_pcs_link_up,
+};
+
+/**
+ * ipq_pcs_create() - Create an IPQ PCS MII instance
+ * @np: Device tree node to the PCS MII
+ *
+ * Description: Create a phylink PCS instance for the given PCS MII node @np
+ * and enable the MII clocks. 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_create(struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct ipq_pcs_mii *qpcs_mii;
+	struct device_node *pcs_np;
+	struct ipq_pcs *qpcs;
+	int i, ret;
+	u32 index;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (of_property_read_u32(np, "reg", &index))
+		return ERR_PTR(-EINVAL);
+
+	if (index >= PCS_MAX_MII_NRS)
+		return ERR_PTR(-EINVAL);
+
+	pcs_np = of_get_parent(np);
+	if (!pcs_np)
+		return ERR_PTR(-ENODEV);
+
+	if (!of_device_is_available(pcs_np)) {
+		of_node_put(pcs_np);
+		return ERR_PTR(-ENODEV);
+	}
+
+	pdev = of_find_device_by_node(pcs_np);
+	of_node_put(pcs_np);
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
+	qpcs = platform_get_drvdata(pdev);
+	put_device(&pdev->dev);
+
+	/* If probe is not yet completed, return DEFER to
+	 * the dependent driver.
+	 */
+	if (!qpcs)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	qpcs_mii = kzalloc(sizeof(*qpcs_mii), GFP_KERNEL);
+	if (!qpcs_mii)
+		return ERR_PTR(-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;
+
+	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
+		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
+		if (IS_ERR(qpcs_mii->clk[i])) {
+			dev_err(qpcs->dev,
+				"Failed to get MII %d interface clock %s\n",
+				index, pcs_mii_clk_name[i]);
+			goto err_clk_get;
+		}
+
+		ret = clk_prepare_enable(qpcs_mii->clk[i]);
+		if (ret) {
+			dev_err(qpcs->dev,
+				"Failed to enable MII %d interface clock %s\n",
+				index, pcs_mii_clk_name[i]);
+			goto err_clk_en;
+		}
+	}
+
+	return &qpcs_mii->pcs;
+
+err_clk_en:
+	clk_put(qpcs_mii->clk[i]);
+err_clk_get:
+	while (i) {
+		i--;
+		clk_disable_unprepare(qpcs_mii->clk[i]);
+		clk_put(qpcs_mii->clk[i]);
+	}
+
+	kfree(qpcs_mii);
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(ipq_pcs_create);
+
+/**
+ * ipq_pcs_destroy() - Destroy the IPQ PCS MII instance
+ * @pcs: PCS instance
+ *
+ * Description: Destroy a phylink PCS instance.
+ */
+void ipq_pcs_destroy(struct phylink_pcs *pcs)
+{
+	struct ipq_pcs_mii *qpcs_mii;
+	int i;
+
+	if (!pcs)
+		return;
+
+	qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
+		clk_disable_unprepare(qpcs_mii->clk[i]);
+		clk_put(qpcs_mii->clk[i]);
+	}
+
+	kfree(qpcs_mii);
+}
+EXPORT_SYMBOL(ipq_pcs_destroy);
+
 static unsigned long ipq_pcs_clk_rate_get(struct ipq_pcs *qpcs)
 {
 	switch (qpcs->interface) {
@@ -219,6 +672,8 @@ static int ipq_pcs_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	mutex_init(&qpcs->config_lock);
+
 	platform_set_drvdata(pdev, qpcs);
 
 	return 0;
diff --git a/include/linux/pcs/pcs-qcom-ipq.h b/include/linux/pcs/pcs-qcom-ipq.h
new file mode 100644
index 000000000000..f72a895afaba
--- /dev/null
+++ b/include/linux/pcs/pcs-qcom-ipq.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_IPQ_H
+#define __LINUX_PCS_QCOM_IPQ_H
+
+struct device_node;
+struct phylink_pcs;
+
+struct phylink_pcs *ipq_pcs_create(struct device_node *np);
+void ipq_pcs_destroy(struct phylink_pcs *pcs);
+
+#endif /* __LINUX_PCS_QCOM_IPQ_H */

-- 
2.34.1

[PATCH net-next 4/5] net: pcs: qcom-ipq: Add USXGMII interface mode for IPQ9574

[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-ipq.c | 180 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 180 insertions(+)

diff --git a/drivers/net/pcs/pcs-qcom-ipq.c b/drivers/net/pcs/pcs-qcom-ipq.c
index dd432303b549..19cb995f7c87 100644
--- a/drivers/net/pcs/pcs-qcom-ipq.c
+++ b/drivers/net/pcs/pcs-qcom-ipq.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_MODE_AN_MODE		BIT(0)
 
 #define PCS_MII_CTRL(x)			(0x480 + 0x18 * (x))
@@ -57,6 +58,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
+
 /* Private data for the PCS instance */
 struct ipq_pcs {
 	struct device *dev;
@@ -144,9 +174,57 @@ static void ipq_pcs_get_state_sgmii(struct ipq_pcs *qpcs,
 		state->pause |= MLO_PAUSE_RX;
 }
 
+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;
 
@@ -167,6 +245,13 @@ static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
 		if (ret)
 			return ret;
 		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		rate = 312500000;
+		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+					 PCS_MODE_SEL_MASK, PCS_MODE_XPCS);
+		if (ret)
+			return ret;
+		break;
 	default:
 		dev_err(qpcs->dev,
 			"Unsupported interface %s\n", phy_modes(interface));
@@ -196,6 +281,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;
 }
 
@@ -240,6 +340,35 @@ static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
 	return ret;
 }
 
+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,
@@ -288,6 +417,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 void ipq_pcs_get_state(struct phylink_pcs *pcs,
 			      struct phylink_link_state *state)
 {
@@ -300,6 +472,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;
 	}
@@ -326,6 +501,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));
@@ -349,6 +526,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

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

[Lei Wei]

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index c27f3190737f..6c5599c3834b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19148,6 +19148,15 @@ F:	Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
 F:	drivers/mailbox/qcom-ipcc.c
 F:	include/dt-bindings/mailbox/qcom-ipcc.h
 
+QUALCOMM IPQ 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-ipq.c
+F:	include/dt-bindings/net/pcs-qcom-ipq.h
+F:	include/linux/pcs/pcs-qcom-ipq.h
+
 QUALCOMM IPQ4019 USB PHY DRIVER
 M:	Robert Marko <robert.marko@sartura.hr>
 M:	Luka Perkov <luka.perkov@sartura.hr>

-- 
2.34.1

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

[Andrew Lunn]

> +config PCS_QCOM_IPQ
> +	tristate "Qualcomm IPQ PCS"

Will Qualcomm only ever have one PCS driver?

You probably want a more specific name so that when the next PCS
driver comes along, you have a reasonable consistent naming scheme.

	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Andrew Lunn]

> +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:
> +		/* Select Qualcomm SGMII AN mode */
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> +					 PCS_MODE_SGMII);

How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?

> +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. This is required only for interface modes
> +	 * such as QSGMII.
> +	 */
> +	if (interface == PHY_INTERFACE_MODE_QSGMII)
> +		mutex_lock(&qpcs->config_lock);

Is there a lot of contention on this lock? Why not take it for every
interface mode? It would make the code simpler.

> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +	struct ipq_pcs_mii *qpcs_mii;
> +	struct device_node *pcs_np;
> +	struct ipq_pcs *qpcs;
> +	int i, ret;
> +	u32 index;
> +
> +	if (!of_device_is_available(np))
> +		return ERR_PTR(-ENODEV);
> +
> +	if (of_property_read_u32(np, "reg", &index))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (index >= PCS_MAX_MII_NRS)
> +		return ERR_PTR(-EINVAL);
> +
> +	pcs_np = of_get_parent(np);
> +	if (!pcs_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_device_is_available(pcs_np)) {
> +		of_node_put(pcs_np);
> +		return ERR_PTR(-ENODEV);
> +	}

How have you got this far if the parent is not available?

> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> +		if (IS_ERR(qpcs_mii->clk[i])) {
> +			dev_err(qpcs->dev,
> +				"Failed to get MII %d interface clock %s\n",
> +				index, pcs_mii_clk_name[i]);
> +			goto err_clk_get;
> +		}
> +
> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
> +		if (ret) {
> +			dev_err(qpcs->dev,
> +				"Failed to enable MII %d interface clock %s\n",
> +				index, pcs_mii_clk_name[i]);
> +			goto err_clk_en;
> +		}
> +	}

Maybe devm_clk_bulk_get() etc will help you here? I've never actually
used them, so i don't know for sure.

	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Jakub Kicinski]

On Fri, 1 Nov 2024 14:21:24 +0100 Andrew Lunn wrote:
> > +	/* Access to PCS registers such as PCS_MODE_CTRL which are
> > +	 * common to all MIIs, is lock protected and configured
> > +	 * only once. This is required only for interface modes
> > +	 * such as QSGMII.
> > +	 */
> > +	if (interface == PHY_INTERFACE_MODE_QSGMII)
> > +		mutex_lock(&qpcs->config_lock);  
> 
> Is there a lot of contention on this lock? Why not take it for every
> interface mode? It would make the code simpler.

+1
-- 
pw-bot: cr

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

[Krzysztof Kozlowski]

On Fri, Nov 01, 2024 at 06:32:49PM +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         | 230 +++++++++++++++++++++
>  include/dt-bindings/net/pcs-qcom-ipq.h             |  15 ++
>  2 files changed, 245 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..a33873c7ad73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
> @@ -0,0 +1,230 @@
> +# 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 IPQ SoC

s/IPQ/IPQ9574/

> +
> +maintainers:
> +  - Lei Wei <quic_leiwei@quicinc.com>

...

> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: system clock
> +      - description: AHB clock needed for register interface access
> +
> +  clock-names:
> +    items:
> +      - const: sys
> +      - const: ahb
> +
> +  '#clock-cells':

Use consistent quotes, either ' or "

> +    const: 1
> +    description: See include/dt-bindings/net/pcs-qcom-ipq.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: mii_rx

rx

> +          - const: mii_tx

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>
> +
> +    pcs0: ethernet-pcs@7a00000 {

Drop unused labels here and further.

> +        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>;
> +
> +        pcs0_mii0: pcs-mii@0 {
> +            reg = <0>;
> +            clocks = <&nsscc 116>,
> +                     <&nsscc 117>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +
> +        pcs0_mii1: pcs-mii@1 {
> +            reg = <1>;
> +            clocks = <&nsscc 118>,
> +                     <&nsscc 119>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +
> +        pcs0_mii2: pcs-mii@2 {
> +            reg = <2>;
> +            clocks = <&nsscc 120>,
> +                     <&nsscc 121>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +
> +        pcs0_mii3: pcs-mii@3 {
> +            reg = <3>;
> +            clocks = <&nsscc 122>,
> +                     <&nsscc 123>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +    };
> +
> +    pcs1: ethernet-pcs@7a10000 {

One example is enough, drop the rest.

> +        compatible = "qcom,ipq9574-pcs";
> +        reg = <0x7a10000 0x10000>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
> +                 <&gcc GCC_UNIPHY1_AHB_CLK>;
> +        clock-names = "sys",
> +                      "ahb";
> +        #clock-cells = <1>;
> +
> +        pcs1_mii0: pcs-mii@0 {
> +            reg = <0>;
> +            clocks = <&nsscc 124>,
> +                     <&nsscc 125>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +    };
> +
> +    pcs2: ethernet-pcs@7a20000 {
> +        compatible = "qcom,ipq9574-pcs";
> +        reg = <0x7a20000 0x10000>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        clocks = <&gcc GCC_UNIPHY2_SYS_CLK>,
> +                 <&gcc GCC_UNIPHY2_AHB_CLK>;
> +        clock-names = "sys",
> +                      "ahb";
> +        #clock-cells = <1>;
> +
> +        pcs2_mii0: pcs-mii@0 {
> +            reg = <0>;
> +            clocks = <&nsscc 126>,
> +                     <&nsscc 127>;
> +            clock-names = "mii_rx",
> +                          "mii_tx";
> +        };
> +    };
> diff --git a/include/dt-bindings/net/pcs-qcom-ipq.h b/include/dt-bindings/net/pcs-qcom-ipq.h
> new file mode 100644
> index 000000000000..8d9124ffd75d
> --- /dev/null
> +++ b/include/dt-bindings/net/pcs-qcom-ipq.h

Filename matching exactly binding filename.

Best regards,
Krzysztof

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

[Lei Wei]

On 11/2/2024 9:34 PM, Krzysztof Kozlowski wrote:
> On Fri, Nov 01, 2024 at 06:32:49PM +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         | 230 +++++++++++++++++++++
>>   include/dt-bindings/net/pcs-qcom-ipq.h             |  15 ++
>>   2 files changed, 245 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..a33873c7ad73
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
>> @@ -0,0 +1,230 @@
>> +# 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 IPQ SoC
> 
> s/IPQ/IPQ9574/
> 

OK, will update.

>> +
>> +maintainers:
>> +  - Lei Wei <quic_leiwei@quicinc.com>
> 
> ...
> 
>> +    const: 0
>> +
>> +  clocks:
>> +    items:
>> +      - description: system clock
>> +      - description: AHB clock needed for register interface access
>> +
>> +  clock-names:
>> +    items:
>> +      - const: sys
>> +      - const: ahb
>> +
>> +  '#clock-cells':
> 
> Use consistent quotes, either ' or "
> 

OK, will use single quotes ' everywhere.

>> +    const: 1
>> +    description: See include/dt-bindings/net/pcs-qcom-ipq.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: mii_rx
> 
> rx
> 

OK.

>> +          - const: mii_tx
> 
> tx

OK.

> 
>> +
>> +    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>
>> +
>> +    pcs0: ethernet-pcs@7a00000 {
> 
> Drop unused labels here and further.
> 

OK, will drop the unused labels "pcs0" and "pcs0_miiX".

>> +        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>;
>> +
>> +        pcs0_mii0: pcs-mii@0 {
>> +            reg = <0>;
>> +            clocks = <&nsscc 116>,
>> +                     <&nsscc 117>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +
>> +        pcs0_mii1: pcs-mii@1 {
>> +            reg = <1>;
>> +            clocks = <&nsscc 118>,
>> +                     <&nsscc 119>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +
>> +        pcs0_mii2: pcs-mii@2 {
>> +            reg = <2>;
>> +            clocks = <&nsscc 120>,
>> +                     <&nsscc 121>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +
>> +        pcs0_mii3: pcs-mii@3 {
>> +            reg = <3>;
>> +            clocks = <&nsscc 122>,
>> +                     <&nsscc 123>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +    };
>> +
>> +    pcs1: ethernet-pcs@7a10000 {
> 
> One example is enough, drop the rest.
> 

OK.

>> +        compatible = "qcom,ipq9574-pcs";
>> +        reg = <0x7a10000 0x10000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
>> +                 <&gcc GCC_UNIPHY1_AHB_CLK>;
>> +        clock-names = "sys",
>> +                      "ahb";
>> +        #clock-cells = <1>;
>> +
>> +        pcs1_mii0: pcs-mii@0 {
>> +            reg = <0>;
>> +            clocks = <&nsscc 124>,
>> +                     <&nsscc 125>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +    };
>> +
>> +    pcs2: ethernet-pcs@7a20000 {
>> +        compatible = "qcom,ipq9574-pcs";
>> +        reg = <0x7a20000 0x10000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        clocks = <&gcc GCC_UNIPHY2_SYS_CLK>,
>> +                 <&gcc GCC_UNIPHY2_AHB_CLK>;
>> +        clock-names = "sys",
>> +                      "ahb";
>> +        #clock-cells = <1>;
>> +
>> +        pcs2_mii0: pcs-mii@0 {
>> +            reg = <0>;
>> +            clocks = <&nsscc 126>,
>> +                     <&nsscc 127>;
>> +            clock-names = "mii_rx",
>> +                          "mii_tx";
>> +        };
>> +    };
>> diff --git a/include/dt-bindings/net/pcs-qcom-ipq.h b/include/dt-bindings/net/pcs-qcom-ipq.h
>> new file mode 100644
>> index 000000000000..8d9124ffd75d
>> --- /dev/null
>> +++ b/include/dt-bindings/net/pcs-qcom-ipq.h
> 
> Filename matching exactly binding filename.
> 

OK.

> Best regards,
> Krzysztof
> 

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

[Lei Wei]

On 11/1/2024 9:00 PM, Andrew Lunn wrote:
>> +config PCS_QCOM_IPQ
>> +	tristate "Qualcomm IPQ PCS"
> 
> Will Qualcomm only ever have one PCS driver?
> 
> You probably want a more specific name so that when the next PCS
> driver comes along, you have a reasonable consistent naming scheme.
> 

We expect one PCS driver to support the 'IPQ' family of Qualcomm 
processors. While we are initially adding support for IPQ9574 SoC, this 
driver will be easily extendable later to other SoC in the IPQ family 
such as IPQ5332, IPQ5424 and others. Therefore we used the name with 
suffix '_IPQ'. Hope it is fine.

> 	Andrew

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

[Andrew Lunn]

On Mon, Nov 04, 2024 at 07:14:59PM +0800, Lei Wei wrote:
> 
> 
> On 11/1/2024 9:00 PM, Andrew Lunn wrote:
> > > +config PCS_QCOM_IPQ
> > > +	tristate "Qualcomm IPQ PCS"
> > 
> > Will Qualcomm only ever have one PCS driver?
> > 
> > You probably want a more specific name so that when the next PCS
> > driver comes along, you have a reasonable consistent naming scheme.
> > 
> 
> We expect one PCS driver to support the 'IPQ' family of Qualcomm processors.
> While we are initially adding support for IPQ9574 SoC, this driver will be
> easily extendable later to other SoC in the IPQ family such as IPQ5332,
> IPQ5424 and others. Therefore we used the name with suffix '_IPQ'. Hope it
> is fine.

So are you saying after IPQ comes IPR? And then IPS? In order to have
a new PCS design, Marketing will allow you to throw away the whole IPQ
branding?

	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

On 11/1/2024 9:21 PM, Andrew Lunn wrote:
>> +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:
>> +		/* Select Qualcomm SGMII AN mode */
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
>> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
>> +					 PCS_MODE_SGMII);
> 
> How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> 

Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding 
pause bit support in the SGMII word format. It re-uses two of the 
reserved bits 1..9 for this purpose. The PCS supports both Qualcomm 
SGMII AN and Cisco SGMII AN modes.

>> +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. This is required only for interface modes
>> +	 * such as QSGMII.
>> +	 */
>> +	if (interface == PHY_INTERFACE_MODE_QSGMII)
>> +		mutex_lock(&qpcs->config_lock);
> 
> Is there a lot of contention on this lock? Why not take it for every
> interface mode? It would make the code simpler.
> 

I agree, the contention should be minimal since the lock is common for 
all MII ports in a PCS and is used only during configuration time. I 
will remove the interface mode check.

>> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
>> +{
>> +	struct platform_device *pdev;
>> +	struct ipq_pcs_mii *qpcs_mii;
>> +	struct device_node *pcs_np;
>> +	struct ipq_pcs *qpcs;
>> +	int i, ret;
>> +	u32 index;
>> +
>> +	if (!of_device_is_available(np))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (of_property_read_u32(np, "reg", &index))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (index >= PCS_MAX_MII_NRS)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	pcs_np = of_get_parent(np);
>> +	if (!pcs_np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (!of_device_is_available(pcs_np)) {
>> +		of_node_put(pcs_np);
>> +		return ERR_PTR(-ENODEV);
>> +	}
> 
> How have you got this far if the parent is not available?
> 

This check can fail only if the parent node is disabled in the board 
DTS. I think this error situation may not be caught earlier than this 
point.
However I agree, the above check is redundant, since this check is 
immediately followed by a validity check on the 'pdev' of the parent 
node, which should be able cover any such errors as well.

>> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
>> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
>> +		if (IS_ERR(qpcs_mii->clk[i])) {
>> +			dev_err(qpcs->dev,
>> +				"Failed to get MII %d interface clock %s\n",
>> +				index, pcs_mii_clk_name[i]);
>> +			goto err_clk_get;
>> +		}
>> +
>> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
>> +		if (ret) {
>> +			dev_err(qpcs->dev,
>> +				"Failed to enable MII %d interface clock %s\n",
>> +				index, pcs_mii_clk_name[i]);
>> +			goto err_clk_en;
>> +		}
>> +	}
> 
> Maybe devm_clk_bulk_get() etc will help you here? I've never actually
> used them, so i don't know for sure.
> 

We don't have a 'device' associated with the 'np', so we could not 
consider using the "devm_clk_bulk_get()" API.

> 	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Andrew Lunn]

On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
> 
> 
> On 11/1/2024 9:21 PM, Andrew Lunn wrote:
> > > +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:
> > > +		/* Select Qualcomm SGMII AN mode */
> > > +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> > > +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> > > +					 PCS_MODE_SGMII);
> > 
> > How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> > 
> 
> Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
> bit support in the SGMII word format. It re-uses two of the reserved bits
> 1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
> SGMII AN modes.

Is Qualcomm SGMII AN actually needed? I assume it only works against a
Qualcomm PHY? What interoperability testing have you do against
non-Qualcomm PHYs?

> > > +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
> > > +{
> > > +	struct platform_device *pdev;
> > > +	struct ipq_pcs_mii *qpcs_mii;
> > > +	struct device_node *pcs_np;
> > > +	struct ipq_pcs *qpcs;
> > > +	int i, ret;
> > > +	u32 index;
> > > +
> > > +	if (!of_device_is_available(np))
> > > +		return ERR_PTR(-ENODEV);
> > > +
> > > +	if (of_property_read_u32(np, "reg", &index))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (index >= PCS_MAX_MII_NRS)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	pcs_np = of_get_parent(np);
> > > +	if (!pcs_np)
> > > +		return ERR_PTR(-ENODEV);
> > > +
> > > +	if (!of_device_is_available(pcs_np)) {
> > > +		of_node_put(pcs_np);
> > > +		return ERR_PTR(-ENODEV);
> > > +	}
> > 
> > How have you got this far if the parent is not available?
> > 
> 
> This check can fail only if the parent node is disabled in the board DTS. I
> think this error situation may not be caught earlier than this point.
> However I agree, the above check is redundant, since this check is
> immediately followed by a validity check on the 'pdev' of the parent node,
> which should be able cover any such errors as well.

This was also because the driver does not work as i expected. I was
expecting the PCS driver to walk its own DT and instantiate the PCS
devices listed. If the parent is disabled, it is clearly not going to
start its own children.  But it is in fact some other device which
walks the PCS DT blob, and as a result the child/parent relationship
is broken, a child could exist without its parent.

> > > +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> > > +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> > > +		if (IS_ERR(qpcs_mii->clk[i])) {
> > > +			dev_err(qpcs->dev,
> > > +				"Failed to get MII %d interface clock %s\n",
> > > +				index, pcs_mii_clk_name[i]);
> > > +			goto err_clk_get;
> > > +		}
> > > +
> > > +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
> > > +		if (ret) {
> > > +			dev_err(qpcs->dev,
> > > +				"Failed to enable MII %d interface clock %s\n",
> > > +				index, pcs_mii_clk_name[i]);
> > > +			goto err_clk_en;
> > > +		}
> > > +	}
> > 
> > Maybe devm_clk_bulk_get() etc will help you here? I've never actually
> > used them, so i don't know for sure.
> > 
> 
> We don't have a 'device' associated with the 'np', so we could not consider
> using the "devm_clk_bulk_get()" API.

Another artefact of not have a child-parent relationship. I wounder if
it makes sense to change the architecture. Have the PCS driver
instantiate the PCS devices as its children. They then have a device
structure for calls like clk_bulk_get(), and a more normal
consumer/provider setup.

	Andrew

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

[Lei Wei]

On 11/4/2024 9:43 PM, Andrew Lunn wrote:
> On Mon, Nov 04, 2024 at 07:14:59PM +0800, Lei Wei wrote:
>>
>>
>> On 11/1/2024 9:00 PM, Andrew Lunn wrote:
>>>> +config PCS_QCOM_IPQ
>>>> +	tristate "Qualcomm IPQ PCS"
>>>
>>> Will Qualcomm only ever have one PCS driver?
>>>
>>> You probably want a more specific name so that when the next PCS
>>> driver comes along, you have a reasonable consistent naming scheme.
>>>
>>
>> We expect one PCS driver to support the 'IPQ' family of Qualcomm processors.
>> While we are initially adding support for IPQ9574 SoC, this driver will be
>> easily extendable later to other SoC in the IPQ family such as IPQ5332,
>> IPQ5424 and others. Therefore we used the name with suffix '_IPQ'. Hope it
>> is fine.
> 
> So are you saying after IPQ comes IPR? And then IPS? In order to have
> a new PCS design, Marketing will allow you to throw away the whole IPQ
> branding?
> 

OK. We will convert the name to make it more specific to the 
architecture. 'PCS_QCOM_IPQ9574' can be used to signify this type of PCS 
within IPQ SoC family. The driver will be re-used later for other IPQ 
SoC using same architecture, with minimal driver extensions.

> 	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Russell King (Oracle)]

Hi,

On Fri, Nov 01, 2024 at 06:32:51PM +0800, Lei Wei wrote:
> +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:
> +		/* Select Qualcomm SGMII AN mode */
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> +					 PCS_MODE_SGMII);
> +		if (ret)
> +			return ret;
> +		break;
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> +					 PCS_MODE_QSGMII);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		dev_err(qpcs->dev,
> +			"Unsupported interface %s\n", phy_modes(interface));
> +		return -EOPNOTSUPP;
> +	}

I think:

	unsigned int mode;

	switch (interface) {
	case PHY_INTERFACE_MODE_SGMII:
		/* Select Qualcomm SGMII AN mode */
		mode = PCS_MODE_SGMII;
		break;

	case PHY_INTERFACE_MODE_QSGMII:
		mode = PCS_MODE_QSGMII;
		break;

	default:
		...
	}

	ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
				 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE, mode);
	if (ret)
		return ret;

might be easier to read? I notice that in patch 4, the USXGMII case
drops PCS_MODE_AN_MODE from the mask, leaving this bit set to whatever
value it previously was. Is that intentional?

> +static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
> +					int index,
> +					unsigned int neg_mode,
> +					int speed)
> +{
> +	int ret;
> +
> +	/* PCS speed need not be configured if in-band autoneg is enabled */
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +		goto pcs_adapter_reset;
> +
> +	/* PCS speed set for force mode */
> +	switch (speed) {
> +	case SPEED_1000:
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> +					 PCS_MII_SPEED_MASK,
> +					 PCS_MII_SPEED_1000);
> +		if (ret)
> +			return ret;
> +		break;
> +	case SPEED_100:
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_100);
> +		if (ret)
> +			return ret;
> +		break;
> +	case SPEED_10:
> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_10);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
> +		return -EINVAL;
> +	}

I think it's worth having the same structure here, and with fewer lines
(and fewer long lines) maybe:

	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
		switch (speed) {
		...
		}

		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
					 PCS_MII_SPEED_MASK, ctrl);
		if (ret)
			return ret;
	}

means you don't need the pcs_adapter_reset label.

> +
> +pcs_adapter_reset:
> +	/* 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 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(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);

This will get very noisy given that in polling mode, phylink will call
this once a second - and I see you are using polling mode.

> +/**
> + * ipq_pcs_create() - Create an IPQ PCS MII instance
> + * @np: Device tree node to the PCS MII
> + *
> + * Description: Create a phylink PCS instance for the given PCS MII node @np
> + * and enable the MII clocks. 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_create(struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +	struct ipq_pcs_mii *qpcs_mii;
> +	struct device_node *pcs_np;
> +	struct ipq_pcs *qpcs;
> +	int i, ret;
> +	u32 index;
> +
> +	if (!of_device_is_available(np))
> +		return ERR_PTR(-ENODEV);
> +
> +	if (of_property_read_u32(np, "reg", &index))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (index >= PCS_MAX_MII_NRS)
> +		return ERR_PTR(-EINVAL);
> +
> +	pcs_np = of_get_parent(np);
> +	if (!pcs_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_device_is_available(pcs_np)) {
> +		of_node_put(pcs_np);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	pdev = of_find_device_by_node(pcs_np);
> +	of_node_put(pcs_np);
> +	if (!pdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	qpcs = platform_get_drvdata(pdev);
> +	put_device(&pdev->dev);
> +
> +	/* If probe is not yet completed, return DEFER to
> +	 * the dependent driver.
> +	 */
> +	if (!qpcs)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	qpcs_mii = kzalloc(sizeof(*qpcs_mii), GFP_KERNEL);
> +	if (!qpcs_mii)
> +		return ERR_PTR(-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;
> +
> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> +		if (IS_ERR(qpcs_mii->clk[i])) {
> +			dev_err(qpcs->dev,
> +				"Failed to get MII %d interface clock %s\n",
> +				index, pcs_mii_clk_name[i]);
> +			goto err_clk_get;
> +		}
> +
> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
> +		if (ret) {
> +			dev_err(qpcs->dev,
> +				"Failed to enable MII %d interface clock %s\n",
> +				index, pcs_mii_clk_name[i]);
> +			goto err_clk_en;
> +		}

Do you always need the clock prepared and enabled? If not, you could
do this in the pcs_enable() method, and turn it off in pcs_disable().

I think phylink may need a tweak to "unuse" the current PCS when
phylink_stop() is called to make that more effective at disabling the
PCS, which is something that should be done - that's a subject for a
separate patch which I may send.

-- 
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 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

On 11/6/2024 11:43 AM, Andrew Lunn wrote:
> On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
>>
>>
>> On 11/1/2024 9:21 PM, Andrew Lunn wrote:
>>>> +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:
>>>> +		/* Select Qualcomm SGMII AN mode */
>>>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
>>>> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
>>>> +					 PCS_MODE_SGMII);
>>>
>>> How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
>>>
>>
>> Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
>> bit support in the SGMII word format. It re-uses two of the reserved bits
>> 1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
>> SGMII AN modes.
> 
> Is Qualcomm SGMII AN actually needed? I assume it only works against a
> Qualcomm PHY? What interoperability testing have you do against
> non-Qualcomm PHYs?
> 

I agree that using Cisco SGMII AN mode as default is more appropriate,
since is more commonly used with PHYs. I will make this change.

Qualcomm SGMII AN is an extension of top of Cisco SGMII AN (only
pause bits difference). So it is expected to be compatible with
non-Qualcomm PHYs which use Cisco SGMII AN.

>>>> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
>>>> +{
>>>> +	struct platform_device *pdev;
>>>> +	struct ipq_pcs_mii *qpcs_mii;
>>>> +	struct device_node *pcs_np;
>>>> +	struct ipq_pcs *qpcs;
>>>> +	int i, ret;
>>>> +	u32 index;
>>>> +
>>>> +	if (!of_device_is_available(np))
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	if (of_property_read_u32(np, "reg", &index))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	if (index >= PCS_MAX_MII_NRS)
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	pcs_np = of_get_parent(np);
>>>> +	if (!pcs_np)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	if (!of_device_is_available(pcs_np)) {
>>>> +		of_node_put(pcs_np);
>>>> +		return ERR_PTR(-ENODEV);
>>>> +	}
>>>
>>> How have you got this far if the parent is not available?
>>>
>>
>> This check can fail only if the parent node is disabled in the board DTS. I
>> think this error situation may not be caught earlier than this point.
>> However I agree, the above check is redundant, since this check is
>> immediately followed by a validity check on the 'pdev' of the parent node,
>> which should be able cover any such errors as well.
> 
> This was also because the driver does not work as i expected. I was
> expecting the PCS driver to walk its own DT and instantiate the PCS
> devices listed. If the parent is disabled, it is clearly not going to
> start its own children.  But it is in fact some other device which
> walks the PCS DT blob, and as a result the child/parent relationship
> is broken, a child could exist without its parent.
> 

Currently the PCS driver walks the DT and instantiates the parent PCS 
nodes during probe, where as the child PCS (the per-MII PCS instance) is 
instantiated later by the network device that is associated with the MII.

Alternatively as you mention, we could instantiate the child PCS during 
probe itself. The network driver when it comes up, can just issue a 
'get' operation on the PCS driver, to retrieve the PCS phylink given the 
PCS node associated with the MAC. Agree that this is architecturally 
simpler for instantiating the PCS nodes, and the interaction between 
network driver and PCS is simpler (single 'get_phylink_pcs' API exported 
from PCS driver instead of PCS create/destroy API exported). The PCS 
instances are freed up during platform device remove.

>>>> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
>>>> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
>>>> +		if (IS_ERR(qpcs_mii->clk[i])) {
>>>> +			dev_err(qpcs->dev,
>>>> +				"Failed to get MII %d interface clock %s\n",
>>>> +				index, pcs_mii_clk_name[i]);
>>>> +			goto err_clk_get;
>>>> +		}
>>>> +
>>>> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
>>>> +		if (ret) {
>>>> +			dev_err(qpcs->dev,
>>>> +				"Failed to enable MII %d interface clock %s\n",
>>>> +				index, pcs_mii_clk_name[i]);
>>>> +			goto err_clk_en;
>>>> +		}
>>>> +	}
>>>
>>> Maybe devm_clk_bulk_get() etc will help you here? I've never actually
>>> used them, so i don't know for sure.
>>>
>>
>> We don't have a 'device' associated with the 'np', so we could not consider
>> using the "devm_clk_bulk_get()" API.
> 
> Another artefact of not have a child-parent relationship. I wounder if
> it makes sense to change the architecture. Have the PCS driver
> instantiate the PCS devices as its children. They then have a device
> structure for calls like clk_bulk_get(), and a more normal
> consumer/provider setup.
> 

I think you may be suggesting to drop the child node usage in the DTS, 
so that we can attach all the MII clocks to the single PCS node, to 
facilitate usage of bulk get() API to retrieve the MII clocks for that 
PCS. We reviewed this option, and believe that retaining the current 
parent-child relationship is a better option instead. This is because 
this option allows us the flexibility to enable/disable the MII channels 
(child nodes) in the board DTS as per the ethernet/MII configuration 
relevant for the board.

We would like to explain this using an example of SoC/board DTSI below.

For IPQ9574, port5 can be connected with PCS0 (PCS0: PSGMII, PCS1 not 
used) or PCS1 (PCS0: QSGMII, PCS1: USXGMII).

IPQ9574 SoC DTSI for PCS0 (5 max MII channels) and PCS1:
--------------------------------------------------------
pcs0: ethernet-pcs@7a00000 {
	clocks = <&gcc GCC_UNIPHY0_SYS_CLK>,
		 <&gcc GCC_UNIPHY0_AHB_CLK>;
	clock-names = "sys",
		      "ahb";

	status = "disabled";

	pcs0_mii0: pcs-mii@0 {
		reg = <0>;
		status = "disabled";
	};

	......

	pcs0_mii3: pcs-mii@3 {
		reg = <3>;
		status = "disabled";
	};
	pcs0_mii4: pcs-mii@3 {
		reg = <4>;
		status = "disabled";
	};
};

pcs1: ethernet-pcs@7a10000 {
	clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
		 <&gcc GCC_UNIPHY1_AHB_CLK>;
	clock-names = "sys",
		      "ahb";

	status = "disabled";

	pcs1_mii0: pcs-mii@0 {
		reg = <0>;
		status = "disabled";
	};	
};


board1 DTS (PCS0 - QSGMII (port1 - port4), PCS1 USXGMII (port 5))
-----------------------------------------------------------------
&pcs0 {
	status = "okay";
};

/* Enable only four MII channels for PCS0 for this board */
&pcs0_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

......

&pcs0_mii3 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT4_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT4_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

&pcs1 {
	status = "okay";
};

&pcs1_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

board2 DTS: (PCS0 - PSGMII (port1 - port5), PCS1 - disabled):
-------------------------------------------------------------
&pcs0 {
	status = "okay";
};

/* For PCS0, Enable all 5 MII channels for this board,
    PCS1 is disabled */
&pcs0_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

......

&pcs0_mii4 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

If we drop the child node in DTS, then all MII clocks will have to be 
combined with the SoC clocks (AHB/SYS) and added to the board DTS, which 
may not be correct. Also, I think the child-parent relationship in DTS 
will make it more clear to reflect the PCS-to--MII-channel relationship.

Kindly let us know if this approach is fine.

> 	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Russell King (Oracle)]

On Fri, Nov 08, 2024 at 07:31:31PM +0800, Lei Wei wrote:
> On 11/6/2024 11:43 AM, Andrew Lunn wrote:
> > On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
> > > On 11/1/2024 9:21 PM, Andrew Lunn wrote:
> > > > How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> > > 
> > > Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
> > > bit support in the SGMII word format. It re-uses two of the reserved bits
> > > 1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
> > > SGMII AN modes.
> > 
> > Is Qualcomm SGMII AN actually needed? I assume it only works against a
> > Qualcomm PHY? What interoperability testing have you do against
> > non-Qualcomm PHYs?
> 
> I agree that using Cisco SGMII AN mode as default is more appropriate,
> since is more commonly used with PHYs. I will make this change.
> 
> Qualcomm SGMII AN is an extension of top of Cisco SGMII AN (only
> pause bits difference). So it is expected to be compatible with
> non-Qualcomm PHYs which use Cisco SGMII AN.

I believe Marvell have similar extensions.

-- 
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 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

On 11/8/2024 3:02 AM, Russell King (Oracle) wrote:
> Hi,
> 
> On Fri, Nov 01, 2024 at 06:32:51PM +0800, Lei Wei wrote:
>> +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:
>> +		/* Select Qualcomm SGMII AN mode */
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
>> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
>> +					 PCS_MODE_SGMII);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
>> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
>> +					 PCS_MODE_QSGMII);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		dev_err(qpcs->dev,
>> +			"Unsupported interface %s\n", phy_modes(interface));
>> +		return -EOPNOTSUPP;
>> +	}
> 
> I think:
> 
> 	unsigned int mode;
> 
> 	switch (interface) {
> 	case PHY_INTERFACE_MODE_SGMII:
> 		/* Select Qualcomm SGMII AN mode */
> 		mode = PCS_MODE_SGMII;
> 		break;
> 
> 	case PHY_INTERFACE_MODE_QSGMII:
> 		mode = PCS_MODE_QSGMII;
> 		break;
> 
> 	default:
> 		...
> 	}
> 
> 	ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> 				 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE, mode);
> 	if (ret)
> 		return ret;
> 
> might be easier to read? 

Thanks for the suggestion, I will make this change.

I notice that in patch 4, the USXGMII case
> drops PCS_MODE_AN_MODE from the mask, leaving this bit set to whatever
> value it previously was. Is that intentional?
> 

Yes, this bit is used for configuring the SGMII AN mode - Cisco AN or 
Qualcomm AN. The default setting is Cisco SGMII AN mode.

Please note that as per our discussion with Andrew on his comment 
regarding the default mode to use, we would like to change the default 
setting for SGMII/QSGMII to Cisco AN Mode in the next patch update.

>> +static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
>> +					int index,
>> +					unsigned int neg_mode,
>> +					int speed)
>> +{
>> +	int ret;
>> +
>> +	/* PCS speed need not be configured if in-band autoneg is enabled */
>> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>> +		goto pcs_adapter_reset;
>> +
>> +	/* PCS speed set for force mode */
>> +	switch (speed) {
>> +	case SPEED_1000:
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
>> +					 PCS_MII_SPEED_MASK,
>> +					 PCS_MII_SPEED_1000);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case SPEED_100:
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
>> +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_100);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case SPEED_10:
>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
>> +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_10);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
>> +		return -EINVAL;
>> +	}
> 
> I think it's worth having the same structure here, and with fewer lines
> (and fewer long lines) maybe:
> 
> 	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
> 		switch (speed) {
> 		...
> 		}
> 
> 		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> 					 PCS_MII_SPEED_MASK, ctrl);
> 		if (ret)
> 			return ret;
> 	}
> 
> means you don't need the pcs_adapter_reset label.
> 

Sure, thanks for the suggestion. I will make this change.

>> +
>> +pcs_adapter_reset:
>> +	/* 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 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(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);
> 
> This will get very noisy given that in polling mode, phylink will call
> this once a second - and I see you are using polling mode.
> 

Sure, I will use dev_dbg_ratelimited() API instead.

>> +/**
>> + * ipq_pcs_create() - Create an IPQ PCS MII instance
>> + * @np: Device tree node to the PCS MII
>> + *
>> + * Description: Create a phylink PCS instance for the given PCS MII node @np
>> + * and enable the MII clocks. 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_create(struct device_node *np)
>> +{
>> +	struct platform_device *pdev;
>> +	struct ipq_pcs_mii *qpcs_mii;
>> +	struct device_node *pcs_np;
>> +	struct ipq_pcs *qpcs;
>> +	int i, ret;
>> +	u32 index;
>> +
>> +	if (!of_device_is_available(np))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (of_property_read_u32(np, "reg", &index))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (index >= PCS_MAX_MII_NRS)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	pcs_np = of_get_parent(np);
>> +	if (!pcs_np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (!of_device_is_available(pcs_np)) {
>> +		of_node_put(pcs_np);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	pdev = of_find_device_by_node(pcs_np);
>> +	of_node_put(pcs_np);
>> +	if (!pdev)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	qpcs = platform_get_drvdata(pdev);
>> +	put_device(&pdev->dev);
>> +
>> +	/* If probe is not yet completed, return DEFER to
>> +	 * the dependent driver.
>> +	 */
>> +	if (!qpcs)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	qpcs_mii = kzalloc(sizeof(*qpcs_mii), GFP_KERNEL);
>> +	if (!qpcs_mii)
>> +		return ERR_PTR(-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;
>> +
>> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
>> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
>> +		if (IS_ERR(qpcs_mii->clk[i])) {
>> +			dev_err(qpcs->dev,
>> +				"Failed to get MII %d interface clock %s\n",
>> +				index, pcs_mii_clk_name[i]);
>> +			goto err_clk_get;
>> +		}
>> +
>> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
>> +		if (ret) {
>> +			dev_err(qpcs->dev,
>> +				"Failed to enable MII %d interface clock %s\n",
>> +				index, pcs_mii_clk_name[i]);
>> +			goto err_clk_en;
>> +		}
> 
> Do you always need the clock prepared and enabled? If not, you could
> do this in the pcs_enable() method, and turn it off in pcs_disable().
> 

Yes, it can be moved to pcs_enable/pcs_disable method. I will make this 
change.

> I think phylink may need a tweak to "unuse" the current PCS when
> phylink_stop() is called to make that more effective at disabling the
> PCS, which is something that should be done - that's a subject for a
> separate patch which I may send.
> 

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Andrew Lunn]

> > Another artefact of not have a child-parent relationship. I wounder if
> > it makes sense to change the architecture. Have the PCS driver
> > instantiate the PCS devices as its children. They then have a device
> > structure for calls like clk_bulk_get(), and a more normal
> > consumer/provider setup.
> > 
> 
> I think you may be suggesting to drop the child node usage in the DTS, so
> that we can attach all the MII clocks to the single PCS node, to facilitate
> usage of bulk get() API to retrieve the MII clocks for that PCS.

I would keep the child nodes. They describe the cookie-cutter nature
of the hardware. The problem is with the clk_bulk API, not allowing
you to pass a device_node. of_clk_bulk_get() appears to do what you
want, but it is not exported. What we do have is:

/**
 * devm_get_clk_from_child - lookup and obtain a managed reference to a
 *                           clock producer from child node.
 * @dev: device for clock "consumer"
 * @np: pointer to clock consumer node
 * @con_id: clock consumer ID
 *
 * This function parses the clocks, and uses them to look up the
 * struct clk from the registered list of clock providers by using
 * @np and @con_id
 *
 * The clock will automatically be freed when the device is unbound
 * from the bus.
 */
struct clk *devm_get_clk_from_child(struct device *dev,
                                    struct device_node *np, const char *con_id);

So maybe a devm_get_clk_bulk_from_child() would be accepted?

However, it might not be worth the effort. Using the bulk API was just
a suggestion to make the code simpler, not a strong requirement.

	Andrew

Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

On 11/8/2024 9:24 PM, Andrew Lunn wrote:
>>> Another artefact of not have a child-parent relationship. I wounder if
>>> it makes sense to change the architecture. Have the PCS driver
>>> instantiate the PCS devices as its children. They then have a device
>>> structure for calls like clk_bulk_get(), and a more normal
>>> consumer/provider setup.
>>>
>>
>> I think you may be suggesting to drop the child node usage in the DTS, so
>> that we can attach all the MII clocks to the single PCS node, to facilitate
>> usage of bulk get() API to retrieve the MII clocks for that PCS.
> 
> I would keep the child nodes. They describe the cookie-cutter nature
> of the hardware. The problem is with the clk_bulk API, not allowing
> you to pass a device_node. of_clk_bulk_get() appears to do what you
> want, but it is not exported. What we do have is:
> 
> /**
>   * devm_get_clk_from_child - lookup and obtain a managed reference to a
>   *                           clock producer from child node.
>   * @dev: device for clock "consumer"
>   * @np: pointer to clock consumer node
>   * @con_id: clock consumer ID
>   *
>   * This function parses the clocks, and uses them to look up the
>   * struct clk from the registered list of clock providers by using
>   * @np and @con_id
>   *
>   * The clock will automatically be freed when the device is unbound
>   * from the bus.
>   */
> struct clk *devm_get_clk_from_child(struct device *dev,
>                                      struct device_node *np, const char *con_id);
> 
> So maybe a devm_get_clk_bulk_from_child() would be accepted?
> 
> However, it might not be worth the effort. Using the bulk API was just
> a suggestion to make the code simpler, not a strong requirement.
> 

OK, I agree.

For the PCS instantiation for child nodes, I would like to summarize the 
two options we have, and mention our chosen approach. 1.) Instantiate 
the PCS during the create API call, and export create/destroy API to the 
network driver similar to existing drivers (OR) 2.) Instantiate the 
child nodes during PCS probe and let the MAC driver access the 
'phylink_pcs' object using a ipq_pcs_get()/ipq_pcs_put() API instead of 
ipq_pcs_create()/ipq_pcs_destroy().

The other PCS drivers are following the create/destroy usage pattern 
(option 1). However we are leaning towards option 2, since it is a 
simpler design. Hope this approach is ok.

> 	Andrew