* [PATCH net-next v5 1/5] dt-bindings: net: pcs: Add Ethernet PCS for Qualcomm IPQ9574 SoC
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
@ 2025-02-07 15:53 ` Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 2/5] net: pcs: Add PCS driver " Lei Wei
` (4 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-07 15:53 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, linux-kernel, linux-arm-msm, quic_kkumarcs,
quic_suruchia, quic_pavir, quic_linchen, quic_luoj, quic_leiwei,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
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.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
.../bindings/net/pcs/qcom,ipq9574-pcs.yaml | 190 +++++++++++++++++++++
include/dt-bindings/net/qcom,ipq9574-pcs.h | 15 ++
2 files changed, 205 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
new file mode 100644
index 000000000000..74573c28d6fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
@@ -0,0 +1,190 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/qcom,ipq9574-pcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ethernet PCS for Qualcomm IPQ9574 SoC
+
+maintainers:
+ - Lei Wei <quic_leiwei@quicinc.com>
+
+description:
+ The UNIPHY hardware blocks in the Qualcomm IPQ SoC include PCS and SerDes
+ functions. They enable connectivity between the Ethernet MAC inside the
+ PPE (packet processing engine) and external Ethernet PHY/switch. There are
+ three UNIPHY instances in IPQ9574 SoC which provide PCS functions to the
+ six Ethernet ports.
+
+ For SGMII (1Gbps PHY) or 2500BASE-X (2.5Gbps PHY) interface modes, the PCS
+ function is enabled by using the PCS block inside UNIPHY. For USXGMII (10Gbps
+ PHY), the XPCS block in UNIPHY is used.
+
+ The SerDes provides 125M (1Gbps mode) or 312.5M (2.5Gbps and 10Gbps modes)
+ RX and TX clocks to the NSSCC (Networking Sub System Clock Controller). The
+ NSSCC divides these clocks and generates the MII RX and TX clocks to each
+ of the MII interfaces between the PCS and MAC, as per the link speeds and
+ interface modes.
+
+ Different IPQ SoC may support different number of UNIPHYs (PCSes) since the
+ number of ports and their capabilities can be different between these SoCs
+
+ Below diagram depicts the UNIPHY (PCS) connections for an IPQ9574 SoC based
+ board. In this example, the PCS0 has four GMIIs/XGMIIs, which can connect
+ with four MACs to support QSGMII (4 x 1Gbps) or 10G_QXGMII (4 x 2.5Gbps)
+ interface modes.
+
+ - +-------+ +---------+ +-------------------------+
+ +---------+CMN PLL| | GCC | | NSSCC (Divider) |
+ | +----+--+ +----+----+ +--+-------+--------------+
+ | | | ^ |
+ | 31.25M | SYS/AHB|clk RX/TX|clk +------------+
+ | ref clk| | | | |
+ | | v | MII RX|TX clk MAC| RX/TX clk
+ |25/50M +--+---------+----------+-------+---+ +-+---------+
+ |ref clk | | +----------------+ | | | | PPE |
+ v | | | UNIPHY0 V | | V |
+ +-------+ | v | +-----------+ (X)GMII| | |
+ | | | +---+---+ | |--------|------|-- MAC0 |
+ | | | | | | | (X)GMII| | |
+ | Quad | | |SerDes | | PCS/XPCS |--------|------|-- MAC1 |
+ | +<----+ | | | | (X)GMII| | |
+ |(X)GPHY| | | | | |--------|------|-- MAC2 |
+ | | | | | | | (X)GMII| | |
+ | | | +-------+ | |--------|------|-- MAC3 |
+ +-------+ | | | | | |
+ | +-----------+ | | |
+ +-----------------------------------+ | |
+ +--+---------+----------+-------+---+ | |
+ +-------+ | UNIPHY1 | | |
+ | | | +-----------+ | | |
+ |(X)GPHY| | +-------+ | | (X)GMII| | |
+ | +<----+ |SerDes | | PCS/XPCS |--------|------|- MAC4 |
+ | | | | | | | | | |
+ +-------+ | +-------+ | | | | |
+ | +-----------+ | | |
+ +-----------------------------------+ | |
+ +--+---------+----------+-------+---+ | |
+ +-------+ | UNIPHY2 | | |
+ | | | +-----------+ | | |
+ |(X)GPHY| | +-------+ | | (X)GMII| | |
+ | +<----+ |SerDes | | PCS/XPCS |--------|------|- MAC5 |
+ | | | | | | | | | |
+ +-------+ | +-------+ | | | | |
+ | +-----------+ | | |
+ +-----------------------------------+ +-----------+
+
+properties:
+ compatible:
+ enum:
+ - qcom,ipq9574-pcs
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ clocks:
+ items:
+ - description: System clock
+ - description: AHB clock needed for register interface access
+
+ clock-names:
+ items:
+ - const: sys
+ - const: ahb
+
+ '#clock-cells':
+ const: 1
+ description: See include/dt-bindings/net/qcom,ipq9574-pcs.h for constants
+
+patternProperties:
+ '^pcs-mii@[0-4]$':
+ type: object
+ description: PCS MII interface.
+
+ properties:
+ reg:
+ minimum: 0
+ maximum: 4
+ description: MII index
+
+ clocks:
+ items:
+ - description: PCS MII RX clock
+ - description: PCS MII TX clock
+
+ clock-names:
+ items:
+ - const: rx
+ - const: tx
+
+ required:
+ - reg
+ - clocks
+ - clock-names
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - '#address-cells'
+ - '#size-cells'
+ - clocks
+ - clock-names
+ - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
+
+ ethernet-pcs@7a00000 {
+ compatible = "qcom,ipq9574-pcs";
+ reg = <0x7a00000 0x10000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&gcc GCC_UNIPHY0_SYS_CLK>,
+ <&gcc GCC_UNIPHY0_AHB_CLK>;
+ clock-names = "sys",
+ "ahb";
+ #clock-cells = <1>;
+
+ pcs-mii@0 {
+ reg = <0>;
+ clocks = <&nsscc 116>,
+ <&nsscc 117>;
+ clock-names = "rx",
+ "tx";
+ };
+
+ pcs-mii@1 {
+ reg = <1>;
+ clocks = <&nsscc 118>,
+ <&nsscc 119>;
+ clock-names = "rx",
+ "tx";
+ };
+
+ pcs-mii@2 {
+ reg = <2>;
+ clocks = <&nsscc 120>,
+ <&nsscc 121>;
+ clock-names = "rx",
+ "tx";
+ };
+
+ pcs-mii@3 {
+ reg = <3>;
+ clocks = <&nsscc 122>,
+ <&nsscc 123>;
+ clock-names = "rx",
+ "tx";
+ };
+ };
diff --git a/include/dt-bindings/net/qcom,ipq9574-pcs.h b/include/dt-bindings/net/qcom,ipq9574-pcs.h
new file mode 100644
index 000000000000..96bd036aaa70
--- /dev/null
+++ b/include/dt-bindings/net/qcom,ipq9574-pcs.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ * Device Tree constants for the Qualcomm IPQ9574 PCS
+ */
+
+#ifndef _DT_BINDINGS_PCS_QCOM_IPQ9574_H
+#define _DT_BINDINGS_PCS_QCOM_IPQ9574_H
+
+/* The RX and TX clocks which are provided from the SerDes to NSSCC. */
+#define PCS_RX_CLK 0
+#define PCS_TX_CLK 1
+
+#endif /* _DT_BINDINGS_PCS_QCOM_IPQ9574_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v5 2/5] net: pcs: Add PCS driver for Qualcomm IPQ9574 SoC
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 1/5] dt-bindings: net: pcs: Add Ethernet PCS " Lei Wei
@ 2025-02-07 15:53 ` Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations Lei Wei
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-07 15:53 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, linux-kernel, linux-arm-msm, quic_kkumarcs,
quic_suruchia, quic_pavir, quic_linchen, quic_luoj, quic_leiwei,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john
The 'UNIPHY' PCS hardware block in Qualcomm's IPQ SoC supports
different interface modes to enable Ethernet MAC connections
for different types of external PHYs/switch. Each UNIPHY block
includes a SerDes and PCS/XPCS blocks, and can operate in either
PCS or XPCS modes. It supports 1Gbps and 2.5Gbps interface modes
(Ex: SGMII) using the PCS, and 10Gbps interface modes (Ex: USXGMII)
using the XPCS. There are three UNIPHY (PCS) instances in IPQ9574
SoC which support the six Ethernet ports in the SoC.
This patch adds support for the platform driver, probe and clock
registrations for the PCS driver. The platform driver creates an
'ipq_pcs' instance for each of the UNIPHY used on the given board.
Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
drivers/net/pcs/Kconfig | 9 ++
drivers/net/pcs/Makefile | 1 +
drivers/net/pcs/pcs-qcom-ipq9574.c | 245 +++++++++++++++++++++++++++++++++++++
3 files changed, 255 insertions(+)
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index f6aa437473de..f1f5669f501a 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -25,6 +25,15 @@ config PCS_MTK_LYNXI
This module provides helpers to phylink for managing the LynxI PCS
which is part of MediaTek's SoC and Ethernet switch ICs.
+config PCS_QCOM_IPQ9574
+ tristate "Qualcomm IPQ9574 PCS"
+ depends on OF && (ARCH_QCOM || COMPILE_TEST)
+ depends on HAS_IOMEM && COMMON_CLK
+ help
+ This module provides driver for UNIPHY PCS available on Qualcomm
+ IPQ9574 SoC. The UNIPHY PCS supports both PCS and XPCS functions
+ to support different interface modes for MAC to PHY connections.
+
config PCS_RZN1_MIIC
tristate "Renesas RZ/N1 MII converter"
depends on OF && (ARCH_RZN1 || COMPILE_TEST)
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..2fa3faf8a5db 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -7,4 +7,5 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o
obj-$(CONFIG_PCS_MTK_LYNXI) += pcs-mtk-lynxi.o
+obj-$(CONFIG_PCS_QCOM_IPQ9574) += pcs-qcom-ipq9574.o
obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
new file mode 100644
index 000000000000..ea90c1902b61
--- /dev/null
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/net/qcom,ipq9574-pcs.h>
+
+#define XPCS_INDIRECT_ADDR 0x8000
+#define XPCS_INDIRECT_AHB_ADDR 0x83fc
+#define XPCS_INDIRECT_ADDR_H GENMASK(20, 8)
+#define XPCS_INDIRECT_ADDR_L GENMASK(7, 0)
+#define XPCS_INDIRECT_DATA_ADDR(reg) (FIELD_PREP(GENMASK(15, 10), 0x20) | \
+ FIELD_PREP(GENMASK(9, 2), \
+ FIELD_GET(XPCS_INDIRECT_ADDR_L, reg)))
+
+/* PCS private data */
+struct ipq_pcs {
+ struct device *dev;
+ void __iomem *base;
+ struct regmap *regmap;
+ phy_interface_t interface;
+
+ /* RX clock supplied to NSSCC */
+ struct clk_hw rx_hw;
+ /* TX clock supplied to NSSCC */
+ struct clk_hw tx_hw;
+};
+
+static unsigned long ipq_pcs_clk_rate_get(struct ipq_pcs *qpcs)
+{
+ switch (qpcs->interface) {
+ case PHY_INTERFACE_MODE_USXGMII:
+ return 312500000;
+ default:
+ return 125000000;
+ }
+}
+
+/* Return clock rate for the RX clock supplied to NSSCC
+ * as per the interface mode.
+ */
+static unsigned long ipq_pcs_rx_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ipq_pcs *qpcs = container_of(hw, struct ipq_pcs, rx_hw);
+
+ return ipq_pcs_clk_rate_get(qpcs);
+}
+
+/* Return clock rate for the TX clock supplied to NSSCC
+ * as per the interface mode.
+ */
+static unsigned long ipq_pcs_tx_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ipq_pcs *qpcs = container_of(hw, struct ipq_pcs, tx_hw);
+
+ return ipq_pcs_clk_rate_get(qpcs);
+}
+
+static int ipq_pcs_clk_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ switch (req->rate) {
+ case 125000000:
+ case 312500000:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+/* Clock ops for the RX clock supplied to NSSCC */
+static const struct clk_ops ipq_pcs_rx_clk_ops = {
+ .determine_rate = ipq_pcs_clk_determine_rate,
+ .recalc_rate = ipq_pcs_rx_clk_recalc_rate,
+};
+
+/* Clock ops for the TX clock supplied to NSSCC */
+static const struct clk_ops ipq_pcs_tx_clk_ops = {
+ .determine_rate = ipq_pcs_clk_determine_rate,
+ .recalc_rate = ipq_pcs_tx_clk_recalc_rate,
+};
+
+static struct clk_hw *ipq_pcs_clk_hw_get(struct of_phandle_args *clkspec,
+ void *data)
+{
+ struct ipq_pcs *qpcs = data;
+
+ switch (clkspec->args[0]) {
+ case PCS_RX_CLK:
+ return &qpcs->rx_hw;
+ case PCS_TX_CLK:
+ return &qpcs->tx_hw;
+ }
+
+ return ERR_PTR(-EINVAL);
+}
+
+/* Register the RX and TX clock which are output from SerDes to
+ * the NSSCC. The NSSCC driver assigns the RX and TX clock as
+ * parent, divides them to generate the MII RX and TX clock to
+ * each MII interface of the PCS as per the link speeds and
+ * interface modes.
+ */
+static int ipq_pcs_clk_register(struct ipq_pcs *qpcs)
+{
+ struct clk_init_data init = { };
+ int ret;
+
+ init.ops = &ipq_pcs_rx_clk_ops;
+ init.name = devm_kasprintf(qpcs->dev, GFP_KERNEL, "%s::rx_clk",
+ dev_name(qpcs->dev));
+ if (!init.name)
+ return -ENOMEM;
+
+ qpcs->rx_hw.init = &init;
+ ret = devm_clk_hw_register(qpcs->dev, &qpcs->rx_hw);
+ if (ret)
+ return ret;
+
+ init.ops = &ipq_pcs_tx_clk_ops;
+ init.name = devm_kasprintf(qpcs->dev, GFP_KERNEL, "%s::tx_clk",
+ dev_name(qpcs->dev));
+ if (!init.name)
+ return -ENOMEM;
+
+ qpcs->tx_hw.init = &init;
+ ret = devm_clk_hw_register(qpcs->dev, &qpcs->tx_hw);
+ if (ret)
+ return ret;
+
+ return devm_of_clk_add_hw_provider(qpcs->dev, ipq_pcs_clk_hw_get, qpcs);
+}
+
+static int ipq_pcs_regmap_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct ipq_pcs *qpcs = context;
+
+ /* PCS uses direct AHB access while XPCS uses indirect AHB access */
+ if (reg >= XPCS_INDIRECT_ADDR) {
+ writel(FIELD_GET(XPCS_INDIRECT_ADDR_H, reg),
+ qpcs->base + XPCS_INDIRECT_AHB_ADDR);
+ *val = readl(qpcs->base + XPCS_INDIRECT_DATA_ADDR(reg));
+ } else {
+ *val = readl(qpcs->base + reg);
+ }
+
+ return 0;
+}
+
+static int ipq_pcs_regmap_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct ipq_pcs *qpcs = context;
+
+ /* PCS uses direct AHB access while XPCS uses indirect AHB access */
+ if (reg >= XPCS_INDIRECT_ADDR) {
+ writel(FIELD_GET(XPCS_INDIRECT_ADDR_H, reg),
+ qpcs->base + XPCS_INDIRECT_AHB_ADDR);
+ writel(val, qpcs->base + XPCS_INDIRECT_DATA_ADDR(reg));
+ } else {
+ writel(val, qpcs->base + reg);
+ }
+
+ return 0;
+}
+
+static const struct regmap_config ipq_pcs_regmap_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_read = ipq_pcs_regmap_read,
+ .reg_write = ipq_pcs_regmap_write,
+ .fast_io = true,
+};
+
+static int ipq9574_pcs_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ipq_pcs *qpcs;
+ struct clk *clk;
+ int ret;
+
+ qpcs = devm_kzalloc(dev, sizeof(*qpcs), GFP_KERNEL);
+ if (!qpcs)
+ return -ENOMEM;
+
+ qpcs->dev = dev;
+
+ qpcs->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(qpcs->base))
+ return dev_err_probe(dev, PTR_ERR(qpcs->base),
+ "Failed to ioremap resource\n");
+
+ qpcs->regmap = devm_regmap_init(dev, NULL, qpcs, &ipq_pcs_regmap_cfg);
+ if (IS_ERR(qpcs->regmap))
+ return dev_err_probe(dev, PTR_ERR(qpcs->regmap),
+ "Failed to allocate register map\n");
+
+ clk = devm_clk_get_enabled(dev, "sys");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk),
+ "Failed to enable SYS clock\n");
+
+ clk = devm_clk_get_enabled(dev, "ahb");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk),
+ "Failed to enable AHB clock\n");
+
+ ret = ipq_pcs_clk_register(qpcs);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, qpcs);
+
+ return 0;
+}
+
+static const struct of_device_id ipq9574_pcs_of_mtable[] = {
+ { .compatible = "qcom,ipq9574-pcs" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ipq9574_pcs_of_mtable);
+
+static struct platform_driver ipq9574_pcs_driver = {
+ .driver = {
+ .name = "ipq9574_pcs",
+ .suppress_bind_attrs = true,
+ .of_match_table = ipq9574_pcs_of_mtable,
+ },
+ .probe = ipq9574_pcs_probe,
+};
+module_platform_driver(ipq9574_pcs_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm IPQ9574 PCS driver");
+MODULE_AUTHOR("Lei Wei <quic_leiwei@quicinc.com>");
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v5 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 1/5] dt-bindings: net: pcs: Add Ethernet PCS " Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 2/5] net: pcs: Add PCS driver " Lei Wei
@ 2025-02-07 15:53 ` Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support Lei Wei
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-07 15:53 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, linux-kernel, linux-arm-msm, quic_kkumarcs,
quic_suruchia, quic_pavir, quic_linchen, quic_luoj, quic_leiwei,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john
This patch adds the following PCS functionality for the PCS driver
for IPQ9574 SoC:
a.) Parses PCS MII DT nodes and instantiate each MII PCS instance.
b.) Exports PCS instance get and put APIs. The network driver calls
the PCS get API to get and associate the PCS instance with the port
MAC.
c.) PCS phylink operations for SGMII/QSGMII interface modes.
Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
drivers/net/pcs/pcs-qcom-ipq9574.c | 469 +++++++++++++++++++++++++++++++++++
include/linux/pcs/pcs-qcom-ipq9574.h | 15 ++
2 files changed, 484 insertions(+)
diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
index ea90c1902b61..2bb55814d2fb 100644
--- a/drivers/net/pcs/pcs-qcom-ipq9574.c
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -6,12 +6,46 @@
#include <linux/clk.h>
#include <linux/clk-provider.h>
#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pcs/pcs-qcom-ipq9574.h>
#include <linux/phy.h>
+#include <linux/phylink.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <dt-bindings/net/qcom,ipq9574-pcs.h>
+/* Maximum number of MIIs per PCS instance. There are 5 MIIs for PSGMII. */
+#define PCS_MAX_MII_NRS 5
+
+#define PCS_CALIBRATION 0x1e0
+#define PCS_CALIBRATION_DONE BIT(7)
+
+#define PCS_MODE_CTRL 0x46c
+#define PCS_MODE_SEL_MASK GENMASK(12, 8)
+#define PCS_MODE_SGMII FIELD_PREP(PCS_MODE_SEL_MASK, 0x4)
+#define PCS_MODE_QSGMII FIELD_PREP(PCS_MODE_SEL_MASK, 0x1)
+
+#define PCS_MII_CTRL(x) (0x480 + 0x18 * (x))
+#define PCS_MII_ADPT_RESET BIT(11)
+#define PCS_MII_FORCE_MODE BIT(3)
+#define PCS_MII_SPEED_MASK GENMASK(2, 1)
+#define PCS_MII_SPEED_1000 FIELD_PREP(PCS_MII_SPEED_MASK, 0x2)
+#define PCS_MII_SPEED_100 FIELD_PREP(PCS_MII_SPEED_MASK, 0x1)
+#define PCS_MII_SPEED_10 FIELD_PREP(PCS_MII_SPEED_MASK, 0x0)
+
+#define PCS_MII_STS(x) (0x488 + 0x18 * (x))
+#define PCS_MII_LINK_STS BIT(7)
+#define PCS_MII_STS_DUPLEX_FULL BIT(6)
+#define PCS_MII_STS_SPEED_MASK GENMASK(5, 4)
+#define PCS_MII_STS_SPEED_10 0
+#define PCS_MII_STS_SPEED_100 1
+#define PCS_MII_STS_SPEED_1000 2
+
+#define PCS_PLL_RESET 0x780
+#define PCS_ANA_SW_RESET BIT(6)
+
#define XPCS_INDIRECT_ADDR 0x8000
#define XPCS_INDIRECT_AHB_ADDR 0x83fc
#define XPCS_INDIRECT_ADDR_H GENMASK(20, 8)
@@ -20,6 +54,18 @@
FIELD_PREP(GENMASK(9, 2), \
FIELD_GET(XPCS_INDIRECT_ADDR_L, reg)))
+/* Per PCS MII private data */
+struct ipq_pcs_mii {
+ struct ipq_pcs *qpcs;
+ struct phylink_pcs pcs;
+ int index;
+
+ /* RX clock from NSSCC to PCS MII */
+ struct clk *rx_clk;
+ /* TX clock from NSSCC to PCS MII */
+ struct clk *tx_clk;
+};
+
/* PCS private data */
struct ipq_pcs {
struct device *dev;
@@ -31,8 +77,359 @@ struct ipq_pcs {
struct clk_hw rx_hw;
/* TX clock supplied to NSSCC */
struct clk_hw tx_hw;
+
+ struct ipq_pcs_mii *qpcs_mii[PCS_MAX_MII_NRS];
};
+#define phylink_pcs_to_qpcs_mii(_pcs) \
+ container_of(_pcs, struct ipq_pcs_mii, pcs)
+
+static void ipq_pcs_get_state_sgmii(struct ipq_pcs *qpcs,
+ int index,
+ struct phylink_link_state *state)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(qpcs->regmap, PCS_MII_STS(index), &val);
+ if (ret) {
+ state->link = 0;
+ return;
+ }
+
+ state->link = !!(val & PCS_MII_LINK_STS);
+
+ if (!state->link)
+ return;
+
+ switch (FIELD_GET(PCS_MII_STS_SPEED_MASK, val)) {
+ case PCS_MII_STS_SPEED_1000:
+ state->speed = SPEED_1000;
+ break;
+ case PCS_MII_STS_SPEED_100:
+ state->speed = SPEED_100;
+ break;
+ case PCS_MII_STS_SPEED_10:
+ state->speed = SPEED_10;
+ break;
+ default:
+ state->link = false;
+ return;
+ }
+
+ if (val & PCS_MII_STS_DUPLEX_FULL)
+ state->duplex = DUPLEX_FULL;
+ else
+ state->duplex = DUPLEX_HALF;
+}
+
+static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
+ phy_interface_t interface)
+{
+ unsigned int val;
+ int ret;
+
+ /* Configure PCS interface mode */
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ val = PCS_MODE_SGMII;
+ break;
+ case PHY_INTERFACE_MODE_QSGMII:
+ val = PCS_MODE_QSGMII;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+ PCS_MODE_SEL_MASK, val);
+ if (ret)
+ return ret;
+
+ /* PCS PLL reset */
+ ret = regmap_clear_bits(qpcs->regmap, PCS_PLL_RESET, PCS_ANA_SW_RESET);
+ if (ret)
+ return ret;
+
+ fsleep(1000);
+ ret = regmap_set_bits(qpcs->regmap, PCS_PLL_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;
+
+ /* Configure the PCS mode if required */
+ if (qpcs->interface != interface) {
+ ret = ipq_pcs_config_mode(qpcs, interface);
+ if (ret)
+ return ret;
+ }
+
+ /* 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_set_bits(qpcs->regmap,
+ PCS_MII_CTRL(index), PCS_MII_FORCE_MODE);
+}
+
+static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
+ int index,
+ unsigned int neg_mode,
+ int speed)
+{
+ unsigned int val;
+ int ret;
+
+ /* PCS speed need not be configured if in-band autoneg is enabled */
+ if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
+ /* PCS speed set for force mode */
+ switch (speed) {
+ case SPEED_1000:
+ val = PCS_MII_SPEED_1000;
+ break;
+ case SPEED_100:
+ val = PCS_MII_SPEED_100;
+ break;
+ case SPEED_10:
+ val = PCS_MII_SPEED_10;
+ break;
+ default:
+ dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
+ PCS_MII_SPEED_MASK, val);
+ if (ret)
+ return ret;
+ }
+
+ /* PCS adapter reset */
+ ret = regmap_clear_bits(qpcs->regmap,
+ PCS_MII_CTRL(index), PCS_MII_ADPT_RESET);
+ if (ret)
+ return ret;
+
+ return regmap_set_bits(qpcs->regmap,
+ PCS_MII_CTRL(index), PCS_MII_ADPT_RESET);
+}
+
+static int ipq_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported,
+ const struct phylink_link_state *state)
+{
+ switch (state->interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static unsigned int ipq_pcs_inband_caps(struct phylink_pcs *pcs,
+ phy_interface_t interface)
+{
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
+ default:
+ return 0;
+ }
+}
+
+static int ipq_pcs_enable(struct phylink_pcs *pcs)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+ int ret;
+
+ ret = clk_prepare_enable(qpcs_mii->rx_clk);
+ if (ret) {
+ dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(qpcs_mii->tx_clk);
+ if (ret) {
+ /* This is a fatal event since phylink does not support unwinding
+ * the state back for this error. So, we only report the error
+ * and do not disable the clocks.
+ */
+ dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void ipq_pcs_disable(struct phylink_pcs *pcs)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+ clk_disable_unprepare(qpcs_mii->rx_clk);
+ clk_disable_unprepare(qpcs_mii->tx_clk);
+}
+
+static void ipq_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
+ struct phylink_link_state *state)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+
+ switch (state->interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ ipq_pcs_get_state_sgmii(qpcs, index, state);
+ break;
+ default:
+ break;
+ }
+
+ dev_dbg_ratelimited(qpcs->dev,
+ "mode=%s/%s/%s link=%u\n",
+ phy_modes(state->interface),
+ phy_speed_to_str(state->speed),
+ phy_duplex_to_str(state->duplex),
+ state->link);
+}
+
+static int ipq_pcs_config(struct phylink_pcs *pcs,
+ unsigned int neg_mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
+ default:
+ 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:
+ 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_validate = ipq_pcs_validate,
+ .pcs_inband_caps = ipq_pcs_inband_caps,
+ .pcs_enable = ipq_pcs_enable,
+ .pcs_disable = ipq_pcs_disable,
+ .pcs_get_state = ipq_pcs_get_state,
+ .pcs_config = ipq_pcs_config,
+ .pcs_link_up = ipq_pcs_link_up,
+};
+
+/* Parse the PCS MII DT nodes which are child nodes of the PCS node,
+ * and instantiate each MII PCS instance.
+ */
+static int ipq_pcs_create_miis(struct ipq_pcs *qpcs)
+{
+ struct device *dev = qpcs->dev;
+ struct ipq_pcs_mii *qpcs_mii;
+ struct device_node *mii_np;
+ u32 index;
+ int ret;
+
+ for_each_available_child_of_node(dev->of_node, mii_np) {
+ ret = of_property_read_u32(mii_np, "reg", &index);
+ if (ret) {
+ dev_err(dev, "Failed to read MII index\n");
+ of_node_put(mii_np);
+ return ret;
+ }
+
+ if (index >= PCS_MAX_MII_NRS) {
+ dev_err(dev, "Invalid MII index\n");
+ of_node_put(mii_np);
+ return -EINVAL;
+ }
+
+ qpcs_mii = devm_kzalloc(dev, sizeof(*qpcs_mii), GFP_KERNEL);
+ if (!qpcs_mii) {
+ of_node_put(mii_np);
+ return -ENOMEM;
+ }
+
+ qpcs_mii->qpcs = qpcs;
+ qpcs_mii->index = index;
+ qpcs_mii->pcs.ops = &ipq_pcs_phylink_ops;
+ qpcs_mii->pcs.neg_mode = true;
+ qpcs_mii->pcs.poll = true;
+
+ qpcs_mii->rx_clk = devm_get_clk_from_child(dev, mii_np, "rx");
+ if (IS_ERR(qpcs_mii->rx_clk)) {
+ of_node_put(mii_np);
+ return dev_err_probe(dev, PTR_ERR(qpcs_mii->rx_clk),
+ "Failed to get MII %d RX clock\n", index);
+ }
+
+ qpcs_mii->tx_clk = devm_get_clk_from_child(dev, mii_np, "tx");
+ if (IS_ERR(qpcs_mii->tx_clk)) {
+ of_node_put(mii_np);
+ return dev_err_probe(dev, PTR_ERR(qpcs_mii->tx_clk),
+ "Failed to get MII %d TX clock\n", index);
+ }
+
+ qpcs->qpcs_mii[index] = qpcs_mii;
+ }
+
+ return 0;
+}
+
static unsigned long ipq_pcs_clk_rate_get(struct ipq_pcs *qpcs)
{
switch (qpcs->interface) {
@@ -219,6 +616,10 @@ static int ipq9574_pcs_probe(struct platform_device *pdev)
if (ret)
return ret;
+ ret = ipq_pcs_create_miis(qpcs);
+ if (ret)
+ return ret;
+
platform_set_drvdata(pdev, qpcs);
return 0;
@@ -230,6 +631,74 @@ static const struct of_device_id ipq9574_pcs_of_mtable[] = {
};
MODULE_DEVICE_TABLE(of, ipq9574_pcs_of_mtable);
+/**
+ * ipq_pcs_get() - Get the IPQ PCS MII instance
+ * @np: Device tree node to the PCS MII
+ *
+ * Description: Get the phylink PCS instance for the given PCS MII node @np.
+ * This instance is associated with the specific MII of the PCS and the
+ * corresponding Ethernet netdevice.
+ *
+ * Return: A pointer to the phylink PCS instance or an error-pointer value.
+ */
+struct phylink_pcs *ipq_pcs_get(struct device_node *np)
+{
+ struct platform_device *pdev;
+ struct ipq_pcs_mii *qpcs_mii;
+ struct ipq_pcs *qpcs;
+ u32 index;
+
+ if (of_property_read_u32(np, "reg", &index))
+ return ERR_PTR(-EINVAL);
+
+ if (index >= PCS_MAX_MII_NRS)
+ return ERR_PTR(-EINVAL);
+
+ if (!of_match_node(ipq9574_pcs_of_mtable, np->parent))
+ return ERR_PTR(-EINVAL);
+
+ /* Get the parent device */
+ pdev = of_find_device_by_node(np->parent);
+ if (!pdev)
+ return ERR_PTR(-ENODEV);
+
+ qpcs = platform_get_drvdata(pdev);
+ if (!qpcs) {
+ put_device(&pdev->dev);
+
+ /* If probe is not yet completed, return DEFER to
+ * the dependent driver.
+ */
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ qpcs_mii = qpcs->qpcs_mii[index];
+ if (!qpcs_mii) {
+ put_device(&pdev->dev);
+ return ERR_PTR(-ENOENT);
+ }
+
+ return &qpcs_mii->pcs;
+}
+EXPORT_SYMBOL(ipq_pcs_get);
+
+/**
+ * ipq_pcs_put() - Release the IPQ PCS MII instance
+ * @pcs: PCS instance
+ *
+ * Description: Release a phylink PCS instance.
+ */
+void ipq_pcs_put(struct phylink_pcs *pcs)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+ /* Put reference taken by of_find_device_by_node() in
+ * ipq_pcs_get().
+ */
+ put_device(qpcs_mii->qpcs->dev);
+}
+EXPORT_SYMBOL(ipq_pcs_put);
+
static struct platform_driver ipq9574_pcs_driver = {
.driver = {
.name = "ipq9574_pcs",
diff --git a/include/linux/pcs/pcs-qcom-ipq9574.h b/include/linux/pcs/pcs-qcom-ipq9574.h
new file mode 100644
index 000000000000..8daff8fa5a00
--- /dev/null
+++ b/include/linux/pcs/pcs-qcom-ipq9574.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef __LINUX_PCS_QCOM_IPQ9574_H
+#define __LINUX_PCS_QCOM_IPQ9574_H
+
+struct device_node;
+struct phylink_pcs;
+
+struct phylink_pcs *ipq_pcs_get(struct device_node *np);
+void ipq_pcs_put(struct phylink_pcs *pcs);
+
+#endif /* __LINUX_PCS_QCOM_IPQ9574_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v5 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
` (2 preceding siblings ...)
2025-02-07 15:53 ` [PATCH net-next v5 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations Lei Wei
@ 2025-02-07 15:53 ` Lei Wei
2025-02-07 15:53 ` [PATCH net-next v5 5/5] MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver Lei Wei
2025-02-12 3:59 ` [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Jakub Kicinski
5 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-07 15:53 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, linux-kernel, linux-arm-msm, quic_kkumarcs,
quic_suruchia, quic_pavir, quic_linchen, quic_luoj, quic_leiwei,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john
USXGMII mode is enabled by PCS when 10Gbps PHYs are connected, such as
Aquantia 10Gbps PHY.
Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
drivers/net/pcs/pcs-qcom-ipq9574.c | 170 +++++++++++++++++++++++++++++++++++++
1 file changed, 170 insertions(+)
diff --git a/drivers/net/pcs/pcs-qcom-ipq9574.c b/drivers/net/pcs/pcs-qcom-ipq9574.c
index 2bb55814d2fb..c35281c9397b 100644
--- a/drivers/net/pcs/pcs-qcom-ipq9574.c
+++ b/drivers/net/pcs/pcs-qcom-ipq9574.c
@@ -26,6 +26,7 @@
#define PCS_MODE_SEL_MASK GENMASK(12, 8)
#define PCS_MODE_SGMII FIELD_PREP(PCS_MODE_SEL_MASK, 0x4)
#define PCS_MODE_QSGMII FIELD_PREP(PCS_MODE_SEL_MASK, 0x1)
+#define PCS_MODE_XPCS FIELD_PREP(PCS_MODE_SEL_MASK, 0x10)
#define PCS_MII_CTRL(x) (0x480 + 0x18 * (x))
#define PCS_MII_ADPT_RESET BIT(11)
@@ -54,6 +55,34 @@
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_SPEED_MASK GENMASK(12, 10)
+#define XPCS_USXG_AN_SPEED_10 0
+#define XPCS_USXG_AN_SPEED_100 1
+#define XPCS_USXG_AN_SPEED_1000 2
+#define XPCS_USXG_AN_SPEED_2500 4
+#define XPCS_USXG_AN_SPEED_5000 5
+#define XPCS_USXG_AN_SPEED_10000 3
+
/* Per PCS MII private data */
struct ipq_pcs_mii {
struct ipq_pcs *qpcs;
@@ -123,9 +152,54 @@ static void ipq_pcs_get_state_sgmii(struct ipq_pcs *qpcs,
state->duplex = DUPLEX_HALF;
}
+static void ipq_pcs_get_state_usxgmii(struct ipq_pcs *qpcs,
+ struct phylink_link_state *state)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(qpcs->regmap, XPCS_MII_AN_INTR_STS, &val);
+ if (ret) {
+ state->link = 0;
+ return;
+ }
+
+ state->link = !!(val & XPCS_USXG_AN_LINK_STS);
+
+ if (!state->link)
+ return;
+
+ switch (FIELD_GET(XPCS_USXG_AN_SPEED_MASK, val)) {
+ case XPCS_USXG_AN_SPEED_10000:
+ state->speed = SPEED_10000;
+ break;
+ case XPCS_USXG_AN_SPEED_5000:
+ state->speed = SPEED_5000;
+ break;
+ case XPCS_USXG_AN_SPEED_2500:
+ state->speed = SPEED_2500;
+ break;
+ case XPCS_USXG_AN_SPEED_1000:
+ state->speed = SPEED_1000;
+ break;
+ case XPCS_USXG_AN_SPEED_100:
+ state->speed = SPEED_100;
+ break;
+ case XPCS_USXG_AN_SPEED_10:
+ state->speed = SPEED_10;
+ break;
+ default:
+ state->link = false;
+ return;
+ }
+
+ state->duplex = DUPLEX_FULL;
+}
+
static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
phy_interface_t interface)
{
+ unsigned long rate = 125000000;
unsigned int val;
int ret;
@@ -137,6 +211,10 @@ static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
case PHY_INTERFACE_MODE_QSGMII:
val = PCS_MODE_QSGMII;
break;
+ case PHY_INTERFACE_MODE_USXGMII:
+ val = PCS_MODE_XPCS;
+ rate = 312500000;
+ break;
default:
return -EOPNOTSUPP;
}
@@ -167,6 +245,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;
}
@@ -195,6 +288,29 @@ static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
PCS_MII_CTRL(index), PCS_MII_FORCE_MODE);
}
+static int ipq_pcs_config_usxgmii(struct ipq_pcs *qpcs)
+{
+ int ret;
+
+ /* Configure the XPCS for USXGMII mode if required */
+ if (qpcs->interface == PHY_INTERFACE_MODE_USXGMII)
+ return 0;
+
+ ret = ipq_pcs_config_mode(qpcs, PHY_INTERFACE_MODE_USXGMII);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(qpcs->regmap, XPCS_DIG_CTRL, XPCS_USXG_EN);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(qpcs->regmap, XPCS_MII_AN_CTRL, XPCS_MII_AN_8BIT);
+ if (ret)
+ return ret;
+
+ return regmap_set_bits(qpcs->regmap, XPCS_MII_CTRL, XPCS_MII_AN_EN);
+}
+
static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
int index,
unsigned int neg_mode,
@@ -237,6 +353,46 @@ static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
PCS_MII_CTRL(index), 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;
+ }
+
+ /* Configure XPCS speed */
+ ret = regmap_update_bits(qpcs->regmap, XPCS_MII_CTRL,
+ XPCS_SPEED_MASK, val | XPCS_DUPLEX_FULL);
+ if (ret)
+ return ret;
+
+ /* XPCS adapter reset */
+ return regmap_set_bits(qpcs->regmap,
+ XPCS_DIG_CTRL, XPCS_USXG_ADPT_RESET);
+}
+
static int ipq_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported,
const struct phylink_link_state *state)
{
@@ -244,6 +400,11 @@ static int ipq_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported,
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_QSGMII:
return 0;
+ case PHY_INTERFACE_MODE_USXGMII:
+ /* USXGMII only supports full duplex mode */
+ phylink_clear(supported, 100baseT_Half);
+ phylink_clear(supported, 10baseT_Half);
+ return 0;
default:
return -EINVAL;
}
@@ -255,6 +416,7 @@ static unsigned int ipq_pcs_inband_caps(struct phylink_pcs *pcs,
switch (interface) {
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_USXGMII:
return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
default:
return 0;
@@ -307,6 +469,9 @@ static void ipq_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
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;
}
@@ -333,6 +498,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:
return -EOPNOTSUPP;
};
@@ -354,6 +521,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:
return;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next v5 5/5] MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
` (3 preceding siblings ...)
2025-02-07 15:53 ` [PATCH net-next v5 4/5] net: pcs: qcom-ipq9574: Add USXGMII interface mode support Lei Wei
@ 2025-02-07 15:53 ` Lei Wei
2025-02-12 3:59 ` [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Jakub Kicinski
5 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-07 15:53 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, linux-kernel, linux-arm-msm, quic_kkumarcs,
quic_suruchia, quic_pavir, quic_linchen, quic_luoj, quic_leiwei,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john
Add maintainer for the Ethernet PCS driver supported for Qualcomm
IPQ9574 SoC.
Signed-off-by: Lei Wei <quic_leiwei@quicinc.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa065..60c340a2de5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19525,6 +19525,15 @@ S: Maintained
F: Documentation/devicetree/bindings/regulator/vqmmc-ipq4019-regulator.yaml
F: drivers/regulator/vqmmc-ipq4019-regulator.c
+QUALCOMM IPQ9574 Ethernet PCS DRIVER
+M: Lei Wei <quic_leiwei@quicinc.com>
+L: netdev@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/net/pcs/qcom,ipq9574-pcs.yaml
+F: drivers/net/pcs/pcs-qcom-ipq9574.c
+F: include/dt-bindings/net/qcom,ipq9574-pcs.h
+F: include/linux/pcs/pcs-qcom-ipq9574.h
+
QUALCOMM NAND CONTROLLER DRIVER
M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
L: linux-mtd@lists.infradead.org
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-07 15:53 [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Lei Wei
` (4 preceding siblings ...)
2025-02-07 15:53 ` [PATCH net-next v5 5/5] MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver Lei Wei
@ 2025-02-12 3:59 ` Jakub Kicinski
2025-02-12 10:19 ` Russell King (Oracle)
5 siblings, 1 reply; 19+ messages in thread
From: Jakub Kicinski @ 2025-02-12 3:59 UTC (permalink / raw)
To: Lei Wei
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, Russell King, netdev, devicetree, linux-kernel,
linux-arm-msm, quic_kkumarcs, quic_suruchia, quic_pavir,
quic_linchen, quic_luoj, srinivas.kandagatla, bartosz.golaszewski,
vsmuthu, john, Krzysztof Kozlowski
On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
> 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.
Could someone with PHY, or even, dare I say, phylink expertise
take a look here?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-12 3:59 ` [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC Jakub Kicinski
@ 2025-02-12 10:19 ` Russell King (Oracle)
2025-02-19 10:46 ` Lei Wei
0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-02-12 10:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Lei Wei, Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
> > 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.
>
> Could someone with PHY, or even, dare I say, phylink expertise
> take a look here?
I've not had the time, sorry. Looking at it now, I have lots of
questions over this.
1) clocks.
- Patch 2 provides clocks from this driver which are exported to the
NSCCC block that are then used to provide the MII clocks.
- Patch 3 consumes clocks from the NSCCC block for use with each PCS.
Surely this leads to a circular dependency, where the MSCCC driver
can't get the clocks it needs until this driver has initialised, but
this driver can't get the clocks it needs for each PCS from the NSCCC
because the MSCCC driver needs this driver to initialise.
2) there's yet another open coded "_get" function for getting the
PCS given a DT node which is different from every other "_get"
function - this one checks the parent DT node has an appropriate
compatible whereas others don't. The whole poliferation of "_get"
methods that are specific to each PCS still needs solving, and I
still have the big question around what happens when the PCS driver
gets unbound - and whether that causes the kernel to oops. I'm also
not a fan of "look up the struct device and then get its driver data".
There is *no* locking over accessing the driver data.
3) doesn't populate supported_interfaces for the PCS - which would
make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.)
4)
"+ /* Nothing to do here as in-band autoneg mode is enabled
+ * by default for each PCS MII port."
"by default" doesn't matter - what if in-band is disabled and then
subsequently enabled.
5) there seems to be an open-coded decision about the clock rate but
there's also ipq_pcs_clk_rate_get() which seems to make the same
decision.
6) it seems this block has N PCS, but all PCS must operate in the same
mode (e.g. one PCS can't operate in SGMII mode, another in USXGMII
mode.) Currently, the last "config" wins over previous configs across
all interfaces. Is this the best solution? Should we be detecting
conflicting configurations? Unfortunately, pcs->supported_interfaces
can't really be changed after the PCS is being used, so I guess
any such restrictions would need to go in ipq_pcs_validate() which
should work fine - although it would mean that a MAC populating
its phylink_config->supported_interfaces using pcs->supported_interfaces
may end up with too many interface bits set.
(1), (2) and (6) are probably the major issues at the moment, and (2)
has been around for a while.
Given (1), I'm just left wondering whether this has been runtime
tested, and how the driver model's driver dependencies cope with it
if the NSCCC driver is both a clock consumer of/provider to this
driver.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-12 10:19 ` Russell King (Oracle)
@ 2025-02-19 10:46 ` Lei Wei
2025-02-28 12:05 ` Lei Wei
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-19 10:46 UTC (permalink / raw)
To: Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 2/12/2025 6:19 PM, Russell King (Oracle) wrote:
> On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
>> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
>>> 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.
>>
>> Could someone with PHY, or even, dare I say, phylink expertise
>> take a look here?
>
> I've not had the time, sorry. Looking at it now, I have lots of
> questions over this.
>
> 1) clocks.
>
> - Patch 2 provides clocks from this driver which are exported to the
> NSCCC block that are then used to provide the MII clocks.
> - Patch 3 consumes clocks from the NSCCC block for use with each PCS.
>
> Surely this leads to a circular dependency, where the MSCCC driver
> can't get the clocks it needs until this driver has initialised, but
> this driver can't get the clocks it needs for each PCS from the NSCCC
> because the MSCCC driver needs this driver to initialise.
>
Sorry for the delay in response. Below is a description of the
dependencies between the PCS/NSSCC drivers during initialization time
and how the clock relationships are set up. Based on this, there should
not any issue due to circular dependency, but please let me know if any
improvement is possible here given the hardware clock dependency. The
module loading order is as follows:
Step 1.) NSCC driver module
Step 2.) PCS driver module
Step 3.) Ethernet driver module
The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be
available at the time of registration of PCS MII clocks (NSSCC to PCS
MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered
before 'UNIPHY' PCS clock is registered, since by default the parent is
initialized to 'xo' clock. Below is the output of clock tree on the
board before the PCS driver is loaded.
xo-board-clk
nss_cc_port1_rx_clk_src
nss_cc_port1_rx_div_clk_src
nss_cc_uniphy_port1_rx_clk
nss_cc_port1_rx_clk
The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII
clock at the time when the Ethernet and PCS drivers are enabled (step3)
and the MAC links up. At link up time, the NSSCC driver sets the NSSCC
port clock rate (by configuring the divider) based on the link speed,
during which time the NSSCC port clock's parent is switched to 'UNIPHY'
PCS clock. Below is the clock tree dump after this step.
7a00000.ethernet-pcs::rx_clk
nss_cc_port1_rx_clk_src
nss_cc_port1_rx_div_clk_src
nss_cc_uniphy_port1_rx_clk
nss_cc_port1_rx_clk
> 2) there's yet another open coded "_get" function for getting the
> PCS given a DT node which is different from every other "_get"
> function - this one checks the parent DT node has an appropriate
> compatible whereas others don't. The whole poliferation of "_get"
> methods that are specific to each PCS still needs solving, and I
> still have the big question around what happens when the PCS driver
> gets unbound - and whether that causes the kernel to oops. I'm also
> not a fan of "look up the struct device and then get its driver data".
> There is *no* locking over accessing the driver data.
>
The PCS device in IPQ9574 chipset is built into the SoC chip and is not
pluggable. Also, the PCS driver module is not unloadable until the MAC
driver that depends on it is unloaded. Therefore, marking the driver
'.suppress_bind_attrs = true' to disable user unbind action may be good
enough to cover all possible scenarios of device going away for IPQ9574
PCS driver.
To avoid looking up the device and getting its driver data (which is
also seen in other PCS device drivers currently), a common
infrastructure is certainly preferable for the longer term to have a
consistent lookup. As far as I understand, the urgency for the common
infrastructure for lookup is perhaps more to resolve the issue of
hot-pluggable devices going away, and less for devices that do not
support it.
Also, the _get() API is only called once during MAC port initialization
and never later, so if the device is not pluggable and unbind is not
possible, there may not be any race concerns when accessing the driver
data using the _get() API. Please let me know if this understanding is
incorrect.
> 3) doesn't populate supported_interfaces for the PCS - which would
> make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.)
>
Agree, we will update the patch to advertise 'supported interfaces' and
use the 'pcs_validate' op only for patch4 as you pointed (for filtering
half duplex modes for USXGMII.).
[The 'pcs_validate()' was suggested by you and added in the version 3 of
this driver, and at that time, the pcs supported_interfaces is not
introduced.]
> 4)
> "+ /* Nothing to do here as in-band autoneg mode is enabled
> + * by default for each PCS MII port."
>
> "by default" doesn't matter - what if in-band is disabled and then
> subsequently enabled.
>
OK, I will fix this function to handle both in-band neg enabled and
disabled cases in next update.
> 5) there seems to be an open-coded decision about the clock rate but
> there's also ipq_pcs_clk_rate_get() which seems to make the same
> decision.
>
I think you may be referring to both ipq_pcs_config_mode() and
ipq_pcs_clk_rate_get() functions having the similar switch case to
decide the clock rate based on the interface mode. I do agree, we can
simplify this by saving the clock rate in ipq_pcs_config_mode() before
the clk_set_rate() is called, and then simply returning this clock rate
from the recalc_rate() op.
> 6) it seems this block has N PCS, but all PCS must operate in the same
> mode (e.g. one PCS can't operate in SGMII mode, another in USXGMII
> mode.) Currently, the last "config" wins over previous configs across
> all interfaces. Is this the best solution? Should we be detecting
> conflicting configurations? Unfortunately, pcs->supported_interfaces
> can't really be changed after the PCS is being used, so I guess
> any such restrictions would need to go in ipq_pcs_validate() which
> should work fine - although it would mean that a MAC populating
> its phylink_config->supported_interfaces using pcs->supported_interfaces
> may end up with too many interface bits set.
>
I would like to clarify on the hardware supported configurations for the
UNIPHY PCS hardware instances. [Note: There are three instances of
'UNIPHY PCS' in IPQ9574. However we take the example here for PCS0]
UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
Possible combinations: QSGMII (4x 1 SGMII)
PSGMII (5 x 1 SGMII),
SGMII (1 x 1 SGMII)
USXGMII (1 x 1 USXGMII)
As we can see above, different PCS channels in a 'UNIPHY' PCS block
working in different PHY interface modes is not supported by the
hardware. So, it might not be necessary to detect that conflict. If the
interface mode changes from one to another, the same interface mode is
applicable to all the PCS channels that are associated with the UNIPHY
PCS block.
Below is an example of a DTS configuration which depicts one board
configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports,
and all the PCS MII channels are in QSGMII mode. For the 'UNIPHY'
connected with single SGMII or USXGMII PHY (PCS1), only one MII channel
is enabled and connected with one PPE MAC port.
PHY:
&mdio {
ethernet-phy-package@0 {
compatible = "qcom,qca8075-package";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x10>;
qcom,package-mode = "qsgmii";
phy0: ethernet-phy@10 {
reg = <0x10>;
};
phy1: ethernet-phy@11 {
reg = <0x11>;
};
phy2: ethernet-phy@12 {
reg = <0x12>;
};
phy3: ethernet-phy@13 {
reg = <0x13>;
};
};
phy4: ethernet-phy@8 {
compatible ="ethernet-phy-ieee802.3-c45";
reg = <8>;
};
}
PCS:
pcs0: ethernet-pcs@7a00000 {
......
pcs0_mii0: pcs-mii@0 {
reg = <0>;
status = "enabled";
};
......
pcs0_mii3: pcs-mii@3 {
reg = <3>;
status = "enabled";
};
};
pcs1: ethernet-pcs@7a10000 {
......
pcs1_mii0: pcs-mii@0 {
reg = <0>;
status = "enabled";
};
};
MAC:
port@1 {
phy-mode = "qsgmii";
phy-handle = <&phy0>;
pcs-handle = <&pcs0_mii0>;
}
port@2 {
phy-mode = "qsgmii";
phy-handle = <&phy1>;
pcs-handle = <&pcs0_mii1>;
}
port@3 {
phy-mode = "qsgmii";
phy-handle = <&phy2>;
pcs-handle = <&pcs0_mii2>;
}
port@4 {
phy-mode = "qsgmii";
phy-handle = <&phy3>;
pcs-handle = <&pcs0_mii3>;
}
port@5 {
phy-mode = "usxgmii";
phy-handle = <&phy4>;
pcs-handle = <&pcs1_mii0>;
}
> (1), (2) and (6) are probably the major issues at the moment, and (2)
> has been around for a while.
>
> Given (1), I'm just left wondering whether this has been runtime
> tested, and how the driver model's driver dependencies cope with it
> if the NSCCC driver is both a clock consumer of/provider to this
> driver.
>
Yes, I have tested the PCS driver along with NSSCC driver and PPE
Ethernet driver.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-19 10:46 ` Lei Wei
@ 2025-02-28 12:05 ` Lei Wei
2025-02-28 14:22 ` Russell King (Oracle)
2025-05-12 22:56 ` mr.nuke.me
2 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-02-28 12:05 UTC (permalink / raw)
To: Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 2/19/2025 6:46 PM, Lei Wei wrote:
>
>
> On 2/12/2025 6:19 PM, Russell King (Oracle) wrote:
>> On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
>>> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
>>>> 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.
>>>
>>> Could someone with PHY, or even, dare I say, phylink expertise
>>> take a look here?
>>
>> I've not had the time, sorry. Looking at it now, I have lots of
>> questions over this.
>>
>> 1) clocks.
>>
>> - Patch 2 provides clocks from this driver which are exported to the
>> NSCCC block that are then used to provide the MII clocks.
>> - Patch 3 consumes clocks from the NSCCC block for use with each PCS.
>>
>> Surely this leads to a circular dependency, where the MSCCC driver
>> can't get the clocks it needs until this driver has initialised, but
>> this driver can't get the clocks it needs for each PCS from the NSCCC
>> because the MSCCC driver needs this driver to initialise.
>>
>
> Sorry for the delay in response. Below is a description of the
> dependencies between the PCS/NSSCC drivers during initialization time
> and how the clock relationships are set up. Based on this, there should
> not any issue due to circular dependency, but please let me know if any
> improvement is possible here given the hardware clock dependency. The
> module loading order is as follows:
>
> Step 1.) NSCC driver module
> Step 2.) PCS driver module
> Step 3.) Ethernet driver module
>
> The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be
> available at the time of registration of PCS MII clocks (NSSCC to PCS
> MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered
> before 'UNIPHY' PCS clock is registered, since by default the parent is
> initialized to 'xo' clock. Below is the output of clock tree on the
> board before the PCS driver is loaded.
>
> xo-board-clk
> nss_cc_port1_rx_clk_src
> nss_cc_port1_rx_div_clk_src
> nss_cc_uniphy_port1_rx_clk
> nss_cc_port1_rx_clk
>
> The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII
> clock at the time when the Ethernet and PCS drivers are enabled (step3)
> and the MAC links up. At link up time, the NSSCC driver sets the NSSCC
> port clock rate (by configuring the divider) based on the link speed,
> during which time the NSSCC port clock's parent is switched to 'UNIPHY'
> PCS clock. Below is the clock tree dump after this step.
>
> 7a00000.ethernet-pcs::rx_clk
> nss_cc_port1_rx_clk_src
> nss_cc_port1_rx_div_clk_src
> nss_cc_uniphy_port1_rx_clk
> nss_cc_port1_rx_clk
>
>> 2) there's yet another open coded "_get" function for getting the
>> PCS given a DT node which is different from every other "_get"
>> function - this one checks the parent DT node has an appropriate
>> compatible whereas others don't. The whole poliferation of "_get"
>> methods that are specific to each PCS still needs solving, and I
>> still have the big question around what happens when the PCS driver
>> gets unbound - and whether that causes the kernel to oops. I'm also
>> not a fan of "look up the struct device and then get its driver data".
>> There is *no* locking over accessing the driver data.
>>
>
> The PCS device in IPQ9574 chipset is built into the SoC chip and is not
> pluggable. Also, the PCS driver module is not unloadable until the MAC
> driver that depends on it is unloaded. Therefore, marking the driver
> '.suppress_bind_attrs = true' to disable user unbind action may be good
> enough to cover all possible scenarios of device going away for IPQ9574
> PCS driver.
>
> To avoid looking up the device and getting its driver data (which is
> also seen in other PCS device drivers currently), a common
> infrastructure is certainly preferable for the longer term to have a
> consistent lookup. As far as I understand, the urgency for the common
> infrastructure for lookup is perhaps more to resolve the issue of hot-
> pluggable devices going away, and less for devices that do not support it.
>
> Also, the _get() API is only called once during MAC port initialization
> and never later, so if the device is not pluggable and unbind is not
> possible, there may not be any race concerns when accessing the driver
> data using the _get() API. Please let me know if this understanding is
> incorrect.
>
>> 3) doesn't populate supported_interfaces for the PCS - which would
>> make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.)
>>
>
> Agree, we will update the patch to advertise 'supported interfaces' and
> use the 'pcs_validate' op only for patch4 as you pointed (for filtering
> half duplex modes for USXGMII.).
> [The 'pcs_validate()' was suggested by you and added in the version 3 of
> this driver, and at that time, the pcs supported_interfaces is not
> introduced.]
>
>> 4)
>> "+ /* Nothing to do here as in-band autoneg mode is enabled
>> + * by default for each PCS MII port."
>>
>> "by default" doesn't matter - what if in-band is disabled and then
>> subsequently enabled.
>>
>
> OK, I will fix this function to handle both in-band neg enabled and
> disabled cases in next update.
>
>> 5) there seems to be an open-coded decision about the clock rate but
>> there's also ipq_pcs_clk_rate_get() which seems to make the same
>> decision.
>>
>
> I think you may be referring to both ipq_pcs_config_mode() and
> ipq_pcs_clk_rate_get() functions having the similar switch case to
> decide the clock rate based on the interface mode. I do agree, we can
> simplify this by saving the clock rate in ipq_pcs_config_mode() before
> the clk_set_rate() is called, and then simply returning this clock rate
> from the recalc_rate() op.
>
>
>> 6) it seems this block has N PCS, but all PCS must operate in the same
>> mode (e.g. one PCS can't operate in SGMII mode, another in USXGMII
>> mode.) Currently, the last "config" wins over previous configs across
>> all interfaces. Is this the best solution? Should we be detecting
>> conflicting configurations? Unfortunately, pcs->supported_interfaces
>> can't really be changed after the PCS is being used, so I guess
>> any such restrictions would need to go in ipq_pcs_validate() which
>> should work fine - although it would mean that a MAC populating
>> its phylink_config->supported_interfaces using pcs->supported_interfaces
>> may end up with too many interface bits set.
>>
>
> I would like to clarify on the hardware supported configurations for the
> UNIPHY PCS hardware instances. [Note: There are three instances of
> 'UNIPHY PCS' in IPQ9574. However we take the example here for PCS0]
>
> UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
> Possible combinations: QSGMII (4x 1 SGMII)
> PSGMII (5 x 1 SGMII),
> SGMII (1 x 1 SGMII)
> USXGMII (1 x 1 USXGMII)
>
> As we can see above, different PCS channels in a 'UNIPHY' PCS block
> working in different PHY interface modes is not supported by the
> hardware. So, it might not be necessary to detect that conflict. If the
> interface mode changes from one to another, the same interface mode is
> applicable to all the PCS channels that are associated with the UNIPHY
> PCS block.
>
> Below is an example of a DTS configuration which depicts one board
> configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
> PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports,
> and all the PCS MII channels are in QSGMII mode. For the 'UNIPHY'
> connected with single SGMII or USXGMII PHY (PCS1), only one MII channel
> is enabled and connected with one PPE MAC port.
>
> PHY:
> &mdio {
> ethernet-phy-package@0 {
> compatible = "qcom,qca8075-package";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x10>;
> qcom,package-mode = "qsgmii";
>
> phy0: ethernet-phy@10 {
> reg = <0x10>;
> };
>
> phy1: ethernet-phy@11 {
> reg = <0x11>;
> };
>
> phy2: ethernet-phy@12 {
> reg = <0x12>;
> };
>
> phy3: ethernet-phy@13 {
> reg = <0x13>;
> };
> };
> phy4: ethernet-phy@8 {
> compatible ="ethernet-phy-ieee802.3-c45";
> reg = <8>;
> };
> }
>
> PCS:
> pcs0: ethernet-pcs@7a00000 {
> ......
> pcs0_mii0: pcs-mii@0 {
> reg = <0>;
> status = "enabled";
> };
>
> ......
>
> pcs0_mii3: pcs-mii@3 {
> reg = <3>;
> status = "enabled";
> };
> };
>
> pcs1: ethernet-pcs@7a10000 {
> ......
>
> pcs1_mii0: pcs-mii@0 {
> reg = <0>;
> status = "enabled";
> };
> };
>
> MAC:
> port@1 {
> phy-mode = "qsgmii";
> phy-handle = <&phy0>;
> pcs-handle = <&pcs0_mii0>;
> }
>
> port@2 {
> phy-mode = "qsgmii";
> phy-handle = <&phy1>;
> pcs-handle = <&pcs0_mii1>;
> }
> port@3 {
> phy-mode = "qsgmii";
> phy-handle = <&phy2>;
> pcs-handle = <&pcs0_mii2>;
> }
> port@4 {
> phy-mode = "qsgmii";
> phy-handle = <&phy3>;
> pcs-handle = <&pcs0_mii3>;
> }
> port@5 {
> phy-mode = "usxgmii";
> phy-handle = <&phy4>;
> pcs-handle = <&pcs1_mii0>;
> }
>
>> (1), (2) and (6) are probably the major issues at the moment, and (2)
>> has been around for a while.
>>
>> Given (1), I'm just left wondering whether this has been runtime
>> tested, and how the driver model's driver dependencies cope with it
>> if the NSCCC driver is both a clock consumer of/provider to this
>> driver.
>>
>
> Yes, I have tested the PCS driver along with NSSCC driver and PPE
> Ethernet driver.
Hi Russell,
Gentle reminder, to review my responses and provide your comments. Thank
you in advance.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-19 10:46 ` Lei Wei
2025-02-28 12:05 ` Lei Wei
@ 2025-02-28 14:22 ` Russell King (Oracle)
2025-03-06 9:12 ` Lei Wei
2025-05-12 22:56 ` mr.nuke.me
2 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-02-28 14:22 UTC (permalink / raw)
To: Lei Wei
Cc: Jakub Kicinski, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, netdev, devicetree, linux-kernel,
linux-arm-msm, quic_kkumarcs, quic_suruchia, quic_pavir,
quic_linchen, quic_luoj, srinivas.kandagatla, bartosz.golaszewski,
vsmuthu, john, Krzysztof Kozlowski
On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:
> > 2) there's yet another open coded "_get" function for getting the
> > PCS given a DT node which is different from every other "_get"
> > function - this one checks the parent DT node has an appropriate
> > compatible whereas others don't. The whole poliferation of "_get"
> > methods that are specific to each PCS still needs solving, and I
> > still have the big question around what happens when the PCS driver
> > gets unbound - and whether that causes the kernel to oops. I'm also
> > not a fan of "look up the struct device and then get its driver data".
> > There is *no* locking over accessing the driver data.
>
> The PCS device in IPQ9574 chipset is built into the SoC chip and is not
> pluggable. Also, the PCS driver module is not unloadable until the MAC
> driver that depends on it is unloaded. Therefore, marking the driver
> '.suppress_bind_attrs = true' to disable user unbind action may be good
> enough to cover all possible scenarios of device going away for IPQ9574 PCS
> driver.
What I am concerned about is the proliferation of these various PCS
specific "_get" methods. Where the PCS is looked up by firmware
reference, we should have a common way to do that, rather than all
these PCS specific ways.
I did start work on that, but I just haven't had the time to take it
forward. This is about as far as I'd got:
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..0b670fee0757 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,6 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for Linux PCS drivers
+obj-$(CONFIG_PHYLINK) += pcs-core.o
+
pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
pcs-xpcs-nxp.o pcs-xpcs-wx.o
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 976e569feb70..1c5492dab00e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
}
EXPORT_SYMBOL_GPL(phylink_pcs_change);
+/**
+ * phylink_pcs_remove() - notify phylink that a PCS is going away
+ * @pcs: PCS that is going away
+ */
+void phylink_pcs_remove(struct phylink_pcs *pcs)
+{
+
+}
+
static irqreturn_t phylink_link_handler(int irq, void *data)
{
struct phylink *pl = data;
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 071ed4683c8c..1e6b7ce0fa7a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -1,6 +1,7 @@
#ifndef NETDEV_PCS_H
#define NETDEV_PCS_H
+#include <linux/list.h>
#include <linux/phy.h>
#include <linux/spinlock.h>
#include <linux/workqueue.h>
@@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
#endif
struct phylink_pcs_ops;
+struct pcs_lookup;
/**
* struct phylink_pcs - PHYLINK PCS instance
+ * @lookup: private member for PCS core management
* @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
* are supported by this PCS.
* @ops: a pointer to the &struct phylink_pcs_ops structure
@@ -455,6 +458,7 @@ struct phylink_pcs_ops;
* the PCS driver.
*/
struct phylink_pcs {
+ struct pcs_lookup *lookup;
DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
const struct phylink_pcs_ops *ops;
struct phylink *phylink;
@@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
void phylink_mac_change(struct phylink *, bool up);
void phylink_pcs_change(struct phylink_pcs *, bool up);
+void phylink_pcs_remove(struct phylink_pcs *);
int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
@@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
void phylink_decode_usxgmii_word(struct phylink_link_state *state,
uint16_t lpa);
+
+/* PCS lookup */
+struct phylink_pcs *pcs_find(void *id);
+void pcs_remove(struct phylink_pcs *pcs);
+int pcs_add(struct phylink_pcs *pcs, void *id);
+int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
+
#endif
The idea is that you add the device using whatever identifier you decide
(the pointer value is what's matched). For example, a fwnode. You can
then find it using pcs_find().
If it returns NULL, then it's not (yet) registered - if you know that it
should exist (e.g. because the fwnode is marked as available) then you
can return -EPROBE_DEFER or fail.
There is a hook present so phylink can do something on PCS removal -
that's still to be implemented with this. I envision keeping a list
of phylink instances, and walking that list to discover if any phylink
instances are currently using the PCS. If they are, then we can take
the link down.
> I would like to clarify on the hardware supported configurations for the
> UNIPHY PCS hardware instances. [Note: There are three instances of 'UNIPHY
> PCS' in IPQ9574. However we take the example here for PCS0]
>
> UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
> Possible combinations: QSGMII (4x 1 SGMII)
> PSGMII (5 x 1 SGMII),
> SGMII (1 x 1 SGMII)
> USXGMII (1 x 1 USXGMII)
>
> As we can see above, different PCS channels in a 'UNIPHY' PCS block working
> in different PHY interface modes is not supported by the hardware. So, it
> might not be necessary to detect that conflict. If the interface mode
> changes from one to another, the same interface mode is applicable to all
> the PCS channels that are associated with the UNIPHY PCS block.
>
> Below is an example of a DTS configuration which depicts one board
> configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
> PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports, and
> all the PCS MII channels are in QSGMII mode. For the 'UNIPHY' connected with
> single SGMII or USXGMII PHY (PCS1), only one MII channel is enabled and
> connected with one PPE MAC port.
>
> PHY:
> &mdio {
> ethernet-phy-package@0 {
> compatible = "qcom,qca8075-package";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x10>;
> qcom,package-mode = "qsgmii";
>
> phy0: ethernet-phy@10 {
> reg = <0x10>;
> };
>
> phy1: ethernet-phy@11 {
> reg = <0x11>;
> };
>
> phy2: ethernet-phy@12 {
> reg = <0x12>;
> };
>
> phy3: ethernet-phy@13 {
> reg = <0x13>;
> };
> };
> phy4: ethernet-phy@8 {
> compatible ="ethernet-phy-ieee802.3-c45";
> reg = <8>;
> };
> }
>
> PCS:
> pcs0: ethernet-pcs@7a00000 {
> ......
> pcs0_mii0: pcs-mii@0 {
> reg = <0>;
> status = "enabled";
> };
>
> ......
>
> pcs0_mii3: pcs-mii@3 {
> reg = <3>;
> status = "enabled";
> };
> };
Given that this is a package of several PCS which have a global mode, I
think it would be a good idea to have a property like
"qcom,package-mode" which defines which of the four modes should be used
for all PCS.
Then the PCS driver initialises supported_interfaces for each of these
PCS to only contain that mode, thereby ensuring that unsupported
dissimilar modes can't be selected or the mode unexpectedly changed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-28 14:22 ` Russell King (Oracle)
@ 2025-03-06 9:12 ` Lei Wei
2025-03-17 15:11 ` Lei Wei
0 siblings, 1 reply; 19+ messages in thread
From: Lei Wei @ 2025-03-06 9:12 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Jakub Kicinski, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, netdev, devicetree, linux-kernel,
linux-arm-msm, quic_kkumarcs, quic_suruchia, quic_pavir,
quic_linchen, quic_luoj, srinivas.kandagatla, bartosz.golaszewski,
vsmuthu, john, Krzysztof Kozlowski
On 2/28/2025 10:22 PM, Russell King (Oracle) wrote:
> On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:
>>> 2) there's yet another open coded "_get" function for getting the
>>> PCS given a DT node which is different from every other "_get"
>>> function - this one checks the parent DT node has an appropriate
>>> compatible whereas others don't. The whole poliferation of "_get"
>>> methods that are specific to each PCS still needs solving, and I
>>> still have the big question around what happens when the PCS driver
>>> gets unbound - and whether that causes the kernel to oops. I'm also
>>> not a fan of "look up the struct device and then get its driver data".
>>> There is *no* locking over accessing the driver data.
>>
>> The PCS device in IPQ9574 chipset is built into the SoC chip and is not
>> pluggable. Also, the PCS driver module is not unloadable until the MAC
>> driver that depends on it is unloaded. Therefore, marking the driver
>> '.suppress_bind_attrs = true' to disable user unbind action may be good
>> enough to cover all possible scenarios of device going away for IPQ9574 PCS
>> driver.
>
> What I am concerned about is the proliferation of these various PCS
> specific "_get" methods. Where the PCS is looked up by firmware
> reference, we should have a common way to do that, rather than all
> these PCS specific ways.
>
> I did start work on that, but I just haven't had the time to take it
> forward. This is about as far as I'd got:
>
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b90..0b670fee0757 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,6 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for Linux PCS drivers
>
> +obj-$(CONFIG_PHYLINK) += pcs-core.o
> +
> pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
> pcs-xpcs-nxp.o pcs-xpcs-wx.o
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 976e569feb70..1c5492dab00e 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
> }
> EXPORT_SYMBOL_GPL(phylink_pcs_change);
>
> +/**
> + * phylink_pcs_remove() - notify phylink that a PCS is going away
> + * @pcs: PCS that is going away
> + */
> +void phylink_pcs_remove(struct phylink_pcs *pcs)
> +{
> +
> +}
> +
> static irqreturn_t phylink_link_handler(int irq, void *data)
> {
> struct phylink *pl = data;
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 071ed4683c8c..1e6b7ce0fa7a 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -1,6 +1,7 @@
> #ifndef NETDEV_PCS_H
> #define NETDEV_PCS_H
>
> +#include <linux/list.h>
> #include <linux/phy.h>
> #include <linux/spinlock.h>
> #include <linux/workqueue.h>
> @@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> #endif
>
> struct phylink_pcs_ops;
> +struct pcs_lookup;
>
> /**
> * struct phylink_pcs - PHYLINK PCS instance
> + * @lookup: private member for PCS core management
> * @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
> * are supported by this PCS.
> * @ops: a pointer to the &struct phylink_pcs_ops structure
> @@ -455,6 +458,7 @@ struct phylink_pcs_ops;
> * the PCS driver.
> */
> struct phylink_pcs {
> + struct pcs_lookup *lookup;
> DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
> const struct phylink_pcs_ops *ops;
> struct phylink *phylink;
> @@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
>
> void phylink_mac_change(struct phylink *, bool up);
> void phylink_pcs_change(struct phylink_pcs *, bool up);
> +void phylink_pcs_remove(struct phylink_pcs *);
>
> int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
>
> @@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
>
> void phylink_decode_usxgmii_word(struct phylink_link_state *state,
> uint16_t lpa);
> +
> +/* PCS lookup */
> +struct phylink_pcs *pcs_find(void *id);
> +void pcs_remove(struct phylink_pcs *pcs);
> +int pcs_add(struct phylink_pcs *pcs, void *id);
> +int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
> +
> #endif
>
> The idea is that you add the device using whatever identifier you decide
> (the pointer value is what's matched). For example, a fwnode. You can
> then find it using pcs_find().
>
> If it returns NULL, then it's not (yet) registered - if you know that it
> should exist (e.g. because the fwnode is marked as available) then you
> can return -EPROBE_DEFER or fail.
>
> There is a hook present so phylink can do something on PCS removal -
> that's still to be implemented with this. I envision keeping a list
> of phylink instances, and walking that list to discover if any phylink
> instances are currently using the PCS. If they are, then we can take
> the link down.
>
Thanks for sharing the details about this, the approach looks correct.
Can you suggest whether we can go ahead with the current version of the
IPQ PCS driver, and update the driver later to use the common way, once
the infrastructure method is supported? Else (preferably) if the patch
for your change can be posted, I can modify the IPQ PCS driver patch to
use the common method and rebase on top of your patch. Please suggest.
>> I would like to clarify on the hardware supported configurations for the
>> UNIPHY PCS hardware instances. [Note: There are three instances of 'UNIPHY
>> PCS' in IPQ9574. However we take the example here for PCS0]
>>
>> UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
>> Possible combinations: QSGMII (4x 1 SGMII)
>> PSGMII (5 x 1 SGMII),
>> SGMII (1 x 1 SGMII)
>> USXGMII (1 x 1 USXGMII)
>>
>> As we can see above, different PCS channels in a 'UNIPHY' PCS block working
>> in different PHY interface modes is not supported by the hardware. So, it
>> might not be necessary to detect that conflict. If the interface mode
>> changes from one to another, the same interface mode is applicable to all
>> the PCS channels that are associated with the UNIPHY PCS block.
>>
>> Below is an example of a DTS configuration which depicts one board
>> configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
>> PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports, and
>> all the PCS MII channels are in QSGMII mode. For the 'UNIPHY' connected with
>> single SGMII or USXGMII PHY (PCS1), only one MII channel is enabled and
>> connected with one PPE MAC port.
>>
>> PHY:
>> &mdio {
>> ethernet-phy-package@0 {
>> compatible = "qcom,qca8075-package";
>> #address-cells = <1>;
>> #size-cells = <0>;
>> reg = <0x10>;
>> qcom,package-mode = "qsgmii";
>>
>> phy0: ethernet-phy@10 {
>> reg = <0x10>;
>> };
>>
>> phy1: ethernet-phy@11 {
>> reg = <0x11>;
>> };
>>
>> phy2: ethernet-phy@12 {
>> reg = <0x12>;
>> };
>>
>> phy3: ethernet-phy@13 {
>> reg = <0x13>;
>> };
>> };
>> phy4: ethernet-phy@8 {
>> compatible ="ethernet-phy-ieee802.3-c45";
>> reg = <8>;
>> };
>> }
>>
>> PCS:
>> pcs0: ethernet-pcs@7a00000 {
>> ......
>> pcs0_mii0: pcs-mii@0 {
>> reg = <0>;
>> status = "enabled";
>> };
>>
>> ......
>>
>> pcs0_mii3: pcs-mii@3 {
>> reg = <3>;
>> status = "enabled";
>> };
>> };
>
> Given that this is a package of several PCS which have a global mode, I
> think it would be a good idea to have a property like
> "qcom,package-mode" which defines which of the four modes should be used
> for all PCS.
>
> Then the PCS driver initialises supported_interfaces for each of these
> PCS to only contain that mode, thereby ensuring that unsupported
> dissimilar modes can't be selected or the mode unexpectedly changed.
>
OK, I will add the "qcom,package-mode" property to restrict the
supported_interfaces for each of the MII PCS instances.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-03-06 9:12 ` Lei Wei
@ 2025-03-17 15:11 ` Lei Wei
0 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-03-17 15:11 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Jakub Kicinski, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Lunn, Heiner Kallweit, netdev, devicetree, linux-kernel,
linux-arm-msm, quic_kkumarcs, quic_suruchia, quic_pavir,
quic_linchen, quic_luoj, srinivas.kandagatla, bartosz.golaszewski,
vsmuthu, john, Krzysztof Kozlowski
Hi Russell,
Gentle reminder to review my responses and provide comments/suggestions.
Thank you.
On 3/6/2025 5:12 PM, Lei Wei wrote:
>
>
> On 2/28/2025 10:22 PM, Russell King (Oracle) wrote:
>> On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:
>>>> 2) there's yet another open coded "_get" function for getting the
>>>> PCS given a DT node which is different from every other "_get"
>>>> function - this one checks the parent DT node has an appropriate
>>>> compatible whereas others don't. The whole poliferation of "_get"
>>>> methods that are specific to each PCS still needs solving, and I
>>>> still have the big question around what happens when the PCS driver
>>>> gets unbound - and whether that causes the kernel to oops. I'm also
>>>> not a fan of "look up the struct device and then get its driver data".
>>>> There is *no* locking over accessing the driver data.
>>>
>>> The PCS device in IPQ9574 chipset is built into the SoC chip and is not
>>> pluggable. Also, the PCS driver module is not unloadable until the MAC
>>> driver that depends on it is unloaded. Therefore, marking the driver
>>> '.suppress_bind_attrs = true' to disable user unbind action may be good
>>> enough to cover all possible scenarios of device going away for
>>> IPQ9574 PCS
>>> driver.
>>
>> What I am concerned about is the proliferation of these various PCS
>> specific "_get" methods. Where the PCS is looked up by firmware
>> reference, we should have a common way to do that, rather than all
>> these PCS specific ways.
>>
>> I did start work on that, but I just haven't had the time to take it
>> forward. This is about as far as I'd got:
>>
>> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
>> index 4f7920618b90..0b670fee0757 100644
>> --- a/drivers/net/pcs/Makefile
>> +++ b/drivers/net/pcs/Makefile
>> @@ -1,6 +1,8 @@
>> # SPDX-License-Identifier: GPL-2.0
>> # Makefile for Linux PCS drivers
>> +obj-$(CONFIG_PHYLINK) += pcs-core.o
>> +
>> pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
>> pcs-xpcs-nxp.o pcs-xpcs-wx.o
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 976e569feb70..1c5492dab00e 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs
>> *pcs, bool up)
>> }
>> EXPORT_SYMBOL_GPL(phylink_pcs_change);
>> +/**
>> + * phylink_pcs_remove() - notify phylink that a PCS is going away
>> + * @pcs: PCS that is going away
>> + */
>> +void phylink_pcs_remove(struct phylink_pcs *pcs)
>> +{
>> +
>> +}
>> +
>> static irqreturn_t phylink_link_handler(int irq, void *data)
>> {
>> struct phylink *pl = data;
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 071ed4683c8c..1e6b7ce0fa7a 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -1,6 +1,7 @@
>> #ifndef NETDEV_PCS_H
>> #define NETDEV_PCS_H
>> +#include <linux/list.h>
>> #include <linux/phy.h>
>> #include <linux/spinlock.h>
>> #include <linux/workqueue.h>
>> @@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config
>> *config, u32 timer,
>> #endif
>> struct phylink_pcs_ops;
>> +struct pcs_lookup;
>> /**
>> * struct phylink_pcs - PHYLINK PCS instance
>> + * @lookup: private member for PCS core management
>> * @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
>> * are supported by this PCS.
>> * @ops: a pointer to the &struct phylink_pcs_ops structure
>> @@ -455,6 +458,7 @@ struct phylink_pcs_ops;
>> * the PCS driver.
>> */
>> struct phylink_pcs {
>> + struct pcs_lookup *lookup;
>> DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
>> const struct phylink_pcs_ops *ops;
>> struct phylink *phylink;
>> @@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
>> void phylink_mac_change(struct phylink *, bool up);
>> void phylink_pcs_change(struct phylink_pcs *, bool up);
>> +void phylink_pcs_remove(struct phylink_pcs *);
>> int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
>> @@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct
>> mdio_device *pcs,
>> void phylink_decode_usxgmii_word(struct phylink_link_state *state,
>> uint16_t lpa);
>> +
>> +/* PCS lookup */
>> +struct phylink_pcs *pcs_find(void *id);
>> +void pcs_remove(struct phylink_pcs *pcs);
>> +int pcs_add(struct phylink_pcs *pcs, void *id);
>> +int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
>> +
>> #endif
>>
>> The idea is that you add the device using whatever identifier you decide
>> (the pointer value is what's matched). For example, a fwnode. You can
>> then find it using pcs_find().
>>
>> If it returns NULL, then it's not (yet) registered - if you know that it
>> should exist (e.g. because the fwnode is marked as available) then you
>> can return -EPROBE_DEFER or fail.
>>
>> There is a hook present so phylink can do something on PCS removal -
>> that's still to be implemented with this. I envision keeping a list
>> of phylink instances, and walking that list to discover if any phylink
>> instances are currently using the PCS. If they are, then we can take
>> the link down.
>>
>
> Thanks for sharing the details about this, the approach looks correct.
>
> Can you suggest whether we can go ahead with the current version of the
> IPQ PCS driver, and update the driver later to use the common way, once
> the infrastructure method is supported? Else (preferably) if the patch
> for your change can be posted, I can modify the IPQ PCS driver patch to
> use the common method and rebase on top of your patch. Please suggest.
>
>>> I would like to clarify on the hardware supported configurations for the
>>> UNIPHY PCS hardware instances. [Note: There are three instances of
>>> 'UNIPHY
>>> PCS' in IPQ9574. However we take the example here for PCS0]
>>>
>>> UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
>>> Possible combinations: QSGMII (4x 1 SGMII)
>>> PSGMII (5 x 1 SGMII),
>>> SGMII (1 x 1 SGMII)
>>> USXGMII (1 x 1 USXGMII)
>>>
>>> As we can see above, different PCS channels in a 'UNIPHY' PCS block
>>> working
>>> in different PHY interface modes is not supported by the hardware.
>>> So, it
>>> might not be necessary to detect that conflict. If the interface mode
>>> changes from one to another, the same interface mode is applicable to
>>> all
>>> the PCS channels that are associated with the UNIPHY PCS block.
>>>
>>> Below is an example of a DTS configuration which depicts one board
>>> configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
>>> PHY, it has 4 MII channels enabled and connected with 4 PPE MAC
>>> ports, and
>>> all the PCS MII channels are in QSGMII mode. For the 'UNIPHY'
>>> connected with
>>> single SGMII or USXGMII PHY (PCS1), only one MII channel is enabled and
>>> connected with one PPE MAC port.
>>>
>>> PHY:
>>> &mdio {
>>> ethernet-phy-package@0 {
>>> compatible = "qcom,qca8075-package";
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> reg = <0x10>;
>>> qcom,package-mode = "qsgmii";
>>>
>>> phy0: ethernet-phy@10 {
>>> reg = <0x10>;
>>> };
>>>
>>> phy1: ethernet-phy@11 {
>>> reg = <0x11>;
>>> };
>>>
>>> phy2: ethernet-phy@12 {
>>> reg = <0x12>;
>>> };
>>>
>>> phy3: ethernet-phy@13 {
>>> reg = <0x13>;
>>> };
>>> };
>>> phy4: ethernet-phy@8 {
>>> compatible ="ethernet-phy-ieee802.3-c45";
>>> reg = <8>;
>>> };
>>> }
>>>
>>> PCS:
>>> pcs0: ethernet-pcs@7a00000 {
>>> ......
>>> pcs0_mii0: pcs-mii@0 {
>>> reg = <0>;
>>> status = "enabled";
>>> };
>>>
>>> ......
>>>
>>> pcs0_mii3: pcs-mii@3 {
>>> reg = <3>;
>>> status = "enabled";
>>> };
>>> };
>>
>> Given that this is a package of several PCS which have a global mode, I
>> think it would be a good idea to have a property like
>> "qcom,package-mode" which defines which of the four modes should be used
>> for all PCS.
>>
>> Then the PCS driver initialises supported_interfaces for each of these
>> PCS to only contain that mode, thereby ensuring that unsupported
>> dissimilar modes can't be selected or the mode unexpectedly changed.
>>
>
> OK, I will add the "qcom,package-mode" property to restrict the
> supported_interfaces for each of the MII PCS instances.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-02-19 10:46 ` Lei Wei
2025-02-28 12:05 ` Lei Wei
2025-02-28 14:22 ` Russell King (Oracle)
@ 2025-05-12 22:56 ` mr.nuke.me
2025-05-14 16:03 ` Lei Wei
2 siblings, 1 reply; 19+ messages in thread
From: mr.nuke.me @ 2025-05-12 22:56 UTC (permalink / raw)
To: Lei Wei, Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 2/19/25 4:46 AM, Lei Wei wrote:
>
> On 2/12/2025 6:19 PM, Russell King (Oracle) wrote:
>> On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
>>> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
>>>> 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.
>>>
>>> Could someone with PHY, or even, dare I say, phylink expertise
>>> take a look here?
>>
>> I've not had the time, sorry. Looking at it now, I have lots of
>> questions over this.
>>
>> 1) clocks.
>>
>> - Patch 2 provides clocks from this driver which are exported to the
>> NSCCC block that are then used to provide the MII clocks.
>> - Patch 3 consumes clocks from the NSCCC block for use with each PCS.
>>
>> Surely this leads to a circular dependency, where the MSCCC driver
>> can't get the clocks it needs until this driver has initialised, but
>> this driver can't get the clocks it needs for each PCS from the NSCCC
>> because the MSCCC driver needs this driver to initialise.
>>
>
> Sorry for the delay in response. Below is a description of the
> dependencies between the PCS/NSSCC drivers during initialization time
> and how the clock relationships are set up. Based on this, there should
> not any issue due to circular dependency, but please let me know if any
> improvement is possible here given the hardware clock dependency. The
> module loading order is as follows:
>
> Step 1.) NSCC driver module
> Step 2.) PCS driver module
> Step 3.) Ethernet driver module
>
> The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be
> available at the time of registration of PCS MII clocks (NSSCC to PCS
> MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered
> before 'UNIPHY' PCS clock is registered, since by default the parent is
> initialized to 'xo' clock. Below is the output of clock tree on the
> board before the PCS driver is loaded.
>
> xo-board-clk
> nss_cc_port1_rx_clk_src
> nss_cc_port1_rx_div_clk_src
> nss_cc_uniphy_port1_rx_clk
> nss_cc_port1_rx_clk
>
> The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII
> clock at the time when the Ethernet and PCS drivers are enabled (step3)
> and the MAC links up. At link up time, the NSSCC driver sets the NSSCC
> port clock rate (by configuring the divider) based on the link speed,
> during which time the NSSCC port clock's parent is switched to 'UNIPHY'
> PCS clock. Below is the clock tree dump after this step.
>
> 7a00000.ethernet-pcs::rx_clk
> nss_cc_port1_rx_clk_src
> nss_cc_port1_rx_div_clk_src
> nss_cc_uniphy_port1_rx_clk
> nss_cc_port1_rx_clk
>
I tried this PCS driver, and I am seeing a circular dependency in the
clock init. If the clock tree is:
GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
The way I understand it, the UNIPHY probe depends on the MII probe. If
MII .probe() returns -EPROBE_DEFER, then so will the UNIPHY .probe().
But the MII cannot probe until the UNIPHY is done, due to the clock
dependency. How is it supposed to work?
The way I found to resolve this is to move the probing of the MII clocks
to ipq_pcs_get().
This is the kernel log that I see:
[ 12.008754] platform 39b00000.clock-controller: deferred probe
pending: platform: supplier 7a00000.ethernet-pcs not ready
[ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
mdio_bus: supplier 7a20000.ethernet-pcs not ready
[ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
mdio_bus: supplier 90000.mdio-1:18 not ready
[ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
mdio_bus: supplier 90000.mdio-1:18 not ready
[ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
mdio_bus: supplier 90000.mdio-1:18 not ready
[ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
mdio_bus: supplier 90000.mdio-1:18 not ready
[ 12.057579] platform 7a00000.ethernet-pcs: deferred probe pending:
ipq9574_pcs: Failed to get MII 0 RX clock
[ 12.067209] platform 7a20000.ethernet-pcs: deferred probe pending:
ipq9574_pcs: Failed to get MII 0 RX clock
[ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
platform: supplier 39b00000.clock-controller not ready
PHY:
&mdio {
qca8k_nsscc: clock-controller@18 {
compatible = "qcom,qca8084-nsscc";
...
};
ethernet-phy-package@0 {
compatible = "qcom,qca8084-package";
...
qca8084_0: ethernet-phy@0 {
compatible = "ethernet-phy-id004d.d180";
reg = <0>;
clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>;
};
qca8084_1: ethernet-phy@1 {
compatible = "ethernet-phy-id004d.d180";
reg = <1>;
clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>;
};
qca8084_2: ethernet-phy@2 {
compatible = "ethernet-phy-id004d.d180";
reg = <2>;
clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>;
};
qca8084_3: ethernet-phy@3 {
compatible = "ethernet-phy-id004d.d180";
reg = <3>;
clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>;
};
};
qca8081_12: ethernet-phy@12 {
reset-gpios = <&tlmm 36 GPIO_ACTIVE_LOW>;
reg = <12>;
};
PCS:
pcs_uniphy0: ethernet-pcs@7a00000 {
compatible = "qcom,ipq9574-pcs";
...
pcsuniphy0_ch0: pcs-mii@0 {
reg = <0>;
clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
clock-names = "rx",
"tx";
};
...
MAC:
port@1 {
reg = <1>;
phy-mode = "usxgmii";
managed = "in-band-status";
phy-handle = <&qca8084_0>;
pcs-handle = <&pcsuniphy0_ch0>;
...
};
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-05-12 22:56 ` mr.nuke.me
@ 2025-05-14 16:03 ` Lei Wei
2025-05-15 2:32 ` Alex G.
0 siblings, 1 reply; 19+ messages in thread
From: Lei Wei @ 2025-05-14 16:03 UTC (permalink / raw)
To: mr.nuke.me, Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 5/13/2025 6:56 AM, mr.nuke.me@gmail.com wrote:
> On 2/19/25 4:46 AM, Lei Wei wrote:
>>
>> On 2/12/2025 6:19 PM, Russell King (Oracle) wrote:
>>> On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
>>>> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
>>>>> 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.
>>>>
>>>> Could someone with PHY, or even, dare I say, phylink expertise
>>>> take a look here?
>>>
>>> I've not had the time, sorry. Looking at it now, I have lots of
>>> questions over this.
>>>
>>> 1) clocks.
>>>
>>> - Patch 2 provides clocks from this driver which are exported to the
>>> NSCCC block that are then used to provide the MII clocks.
>>> - Patch 3 consumes clocks from the NSCCC block for use with each PCS.
>>>
>>> Surely this leads to a circular dependency, where the MSCCC driver
>>> can't get the clocks it needs until this driver has initialised, but
>>> this driver can't get the clocks it needs for each PCS from the NSCCC
>>> because the MSCCC driver needs this driver to initialise.
>>>
>>
>> Sorry for the delay in response. Below is a description of the
>> dependencies between the PCS/NSSCC drivers during initialization time
>> and how the clock relationships are set up. Based on this, there
>> should not any issue due to circular dependency, but please let me
>> know if any improvement is possible here given the hardware clock
>> dependency. The module loading order is as follows:
>>
>> Step 1.) NSCC driver module
>> Step 2.) PCS driver module
>> Step 3.) Ethernet driver module
>>
>> The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be
>> available at the time of registration of PCS MII clocks (NSSCC to PCS
>> MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered
>> before 'UNIPHY' PCS clock is registered, since by default the parent
>> is initialized to 'xo' clock. Below is the output of clock tree on the
>> board before the PCS driver is loaded.
>>
>> xo-board-clk
>> nss_cc_port1_rx_clk_src
>> nss_cc_port1_rx_div_clk_src
>> nss_cc_uniphy_port1_rx_clk
>> nss_cc_port1_rx_clk
>>
>> The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII
>> clock at the time when the Ethernet and PCS drivers are enabled
>> (step3) and the MAC links up. At link up time, the NSSCC driver sets
>> the NSSCC port clock rate (by configuring the divider) based on the
>> link speed, during which time the NSSCC port clock's parent is
>> switched to 'UNIPHY' PCS clock. Below is the clock tree dump after
>> this step.
>>
>> 7a00000.ethernet-pcs::rx_clk
>> nss_cc_port1_rx_clk_src
>> nss_cc_port1_rx_div_clk_src
>> nss_cc_uniphy_port1_rx_clk
>> nss_cc_port1_rx_clk
>>
>
> I tried this PCS driver, and I am seeing a circular dependency in the
> clock init. If the clock tree is:
> GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
>
> The way I understand it, the UNIPHY probe depends on the MII probe. If
> MII .probe() returns -EPROBE_DEFER, then so will the UNIPHY .probe().
> But the MII cannot probe until the UNIPHY is done, due to the clock
> dependency. How is it supposed to work?
>
> The way I found to resolve this is to move the probing of the MII clocks
> to ipq_pcs_get().
>
> This is the kernel log that I see:
>
> [ 12.008754] platform 39b00000.clock-controller: deferred probe
> pending: platform: supplier 7a00000.ethernet-pcs not ready
> [ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
> mdio_bus: supplier 7a20000.ethernet-pcs not ready
> [ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [ 12.057579] platform 7a00000.ethernet-pcs: deferred probe pending:
> ipq9574_pcs: Failed to get MII 0 RX clock
> [ 12.067209] platform 7a20000.ethernet-pcs: deferred probe pending:
> ipq9574_pcs: Failed to get MII 0 RX clock
> [ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
> platform: supplier 39b00000.clock-controller not ready
>
>
Hello, thanks for bringing this to our notice. Let me try to understand
the reason for the probe failure:
The merged NSSCC DTS does not reference the PCS node directly in the
"clocks" property. It uses a placeholder phandle '<0>' for the
reference. Please see below patch which is merged.
https://lore.kernel.org/all/20250313110359.242491-6-quic_mmanikan@quicinc.com/
Ideally there should be no direct dependency from NSSCC to PCS driver if
we use this version of the NSSCC DTS.
Hence it seems that you may have a modified patch here, and DTS changes
have been applied to enable all the Ethernet components including PCS
and NSSCC, and NSSCC modified to have a direct reference to PCS? However
even in this case, I think the driver probe should work if the drivers
are built as modules. Can you please confirm if the NSSCC and PCS
drivers are built-in to the kernel and not built as modules?
For the case where the drivers are built-in to kernel, and the NSSCC DTS
node has a direct reference to PCS node, we can use the below solution:
[Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
initialization/registration.]
Enable 'post-init-providers' property in the NSSCC DTS node to mark
'UNIPHY' PCS as post-initialization providers to NSSCC. This will
ensure following probe order by the kernel:
1.) NSSCC driver
2.) PCS driver.
Please let me know if the above suggestion can help.
Later once the IPQ PCS driver is merged, we are planning to push the PCS
DTS changes, along with an update of the NSSCC DTS to point to the PCS
node and mark the "post-init-providers" property. This should work for
all cases.
Also, in my view, it is not suitable to move PCS MII clocks get to
"ipq_pcs_get()" because the natural loading order for the drivers
is as below:
1) NSSCC driver
2) PCS driver
3) Ethernet driver.
Additionally, the community is currently working on an infrastructure to
provide a common pcs get method. (Christian and Sean Anderson has been
working on this). Therefore, I expect "ipq_pcs_get" to be dropped in the
future and replaced with the common pcs get method once this common
infra is merged.
This is the post-init-providers dtschma:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/post-init-providers.yaml
> PHY:
> &mdio {
> qca8k_nsscc: clock-controller@18 {
> compatible = "qcom,qca8084-nsscc";
> ...
> };
>
> ethernet-phy-package@0 {
> compatible = "qcom,qca8084-package";
> ...
>
> qca8084_0: ethernet-phy@0 {
> compatible = "ethernet-phy-id004d.d180";
> reg = <0>;
> clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
> resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>;
> };
> qca8084_1: ethernet-phy@1 {
> compatible = "ethernet-phy-id004d.d180";
> reg = <1>;
> clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
> resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>;
> };
> qca8084_2: ethernet-phy@2 {
> compatible = "ethernet-phy-id004d.d180";
> reg = <2>;
> clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
> resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>;
> };
> qca8084_3: ethernet-phy@3 {
> compatible = "ethernet-phy-id004d.d180";
> reg = <3>;
> clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
> resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>;
> };
> };
>
> qca8081_12: ethernet-phy@12 {
> reset-gpios = <&tlmm 36 GPIO_ACTIVE_LOW>;
> reg = <12>;
> };
>
> PCS:
> pcs_uniphy0: ethernet-pcs@7a00000 {
> compatible = "qcom,ipq9574-pcs";
> ...
> pcsuniphy0_ch0: pcs-mii@0 {
> reg = <0>;
> clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
> <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
> clock-names = "rx",
> "tx";
> };
> ...
>
> MAC:
> port@1 {
> reg = <1>;
> phy-mode = "usxgmii";
> managed = "in-band-status";
> phy-handle = <&qca8084_0>;
> pcs-handle = <&pcsuniphy0_ch0>;
> ...
> };
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-05-14 16:03 ` Lei Wei
@ 2025-05-15 2:32 ` Alex G.
2025-05-15 15:27 ` Lei Wei
0 siblings, 1 reply; 19+ messages in thread
From: Alex G. @ 2025-05-15 2:32 UTC (permalink / raw)
To: Lei Wei, Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 5/14/25 11:03, Lei Wei wrote:> On 5/13/2025 6:56 AM,
mr.nuke.me@gmail.com wrote:
>> On 2/19/25 4:46 AM, Lei Wei wrote:
>>
>> I tried this PCS driver, and I am seeing a circular dependency in the
>> clock init. If the clock tree is:
>> GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
>>
>> The way I understand it, the UNIPHY probe depends on the MII probe. If
>> MII .probe() returns -EPROBE_DEFER, then so will the UNIPHY .probe().
>> But the MII cannot probe until the UNIPHY is done, due to the clock
>> dependency. How is it supposed to work?
>>
>> The way I found to resolve this is to move the probing of the MII
>> clocks to ipq_pcs_get().
>>
>> This is the kernel log that I see:
>>
>> [ 12.008754] platform 39b00000.clock-controller: deferred probe
>> pending: platform: supplier 7a00000.ethernet-pcs not ready
>> [ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
>> mdio_bus: supplier 7a20000.ethernet-pcs not ready
>> [ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
>> mdio_bus: supplier 90000.mdio-1:18 not ready
>> [ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
>> mdio_bus: supplier 90000.mdio-1:18 not ready
>> [ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
>> mdio_bus: supplier 90000.mdio-1:18 not ready
>> [ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
>> mdio_bus: supplier 90000.mdio-1:18 not ready
>> [ 12.057579] platform 7a00000.ethernet-pcs: deferred probe pending:
>> ipq9574_pcs: Failed to get MII 0 RX clock
>> [ 12.067209] platform 7a20000.ethernet-pcs: deferred probe pending:
>> ipq9574_pcs: Failed to get MII 0 RX clock
>> [ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
>> platform: supplier 39b00000.clock-controller not ready
>>
>>
>
> Hello, thanks for bringing this to our notice. Let me try to understand
> the reason for the probe failure:
>
> The merged NSSCC DTS does not reference the PCS node directly in the
> "clocks" property. It uses a placeholder phandle '<0>' for the
> reference. Please see below patch which is merged.
> https://lore.kernel.org/all/20250313110359.242491-6-quic_mmanikan@quicinc.com/
>
> Ideally there should be no direct dependency from NSSCC to PCS driver if
> we use this version of the NSSCC DTS.
>
> Hence it seems that you may have a modified patch here, and DTS changes
> have been applied to enable all the Ethernet components including PCS
> and NSSCC, and NSSCC modified to have a direct reference to PCS? However
> even in this case, I think the driver probe should work if the drivers
> are built as modules. Can you please confirm if the NSSCC and PCS
> drivers are built-in to the kernel and not built as modules
The NSSCC and PCS built-in. I also added the uniphy PCS clocks to the
NSSCC in order to expose the issue.
I have a heavily patched tree with PPE driver and EDMA support. That's
the final use case in order to support ethernet, right?
> For the case where the drivers are built-in to kernel, and the NSSCC DTS
> node has a direct reference to PCS node, we can use the below solution:
> [Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
> initialization/registration.]
>
> Enable 'post-init-providers' property in the NSSCC DTS node to mark
> 'UNIPHY' PCS as post-initialization providers to NSSCC. This will
> ensure following probe order by the kernel:
>
> 1.) NSSCC driver
> 2.) PCS driver.
>
> Please let me know if the above suggestion can help.
I see. Adding the 'post-init-providers' property does fix the circular
dependency. Thank you!
I have another question. Do you have a public repository with the
unmerged IPQ9574 patches, including, PCS, PPE, EDMA, QCA8084 ?
> Later once the IPQ PCS driver is merged, we are planning to push the PCS
> DTS changes, along with an update of the NSSCC DTS to point to the PCS
> node and mark the "post-init-providers" property. This should work for
> all cases.
>
> Also, in my view, it is not suitable to move PCS MII clocks get to
> "ipq_pcs_get()" because the natural loading order for the drivers
> is as below:
>
> 1) NSSCC driver
> 2) PCS driver
> 3) Ethernet driver.
>
> Additionally, the community is currently working on an infrastructure to
> provide a common pcs get method. (Christian and Sean Anderson has been
> working on this). Therefore, I expect "ipq_pcs_get" to be dropped in the
> future and replaced with the common pcs get method once this common
> infra is merged.
That makes sense. Thank you for clarifying.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-05-15 2:32 ` Alex G.
@ 2025-05-15 15:27 ` Lei Wei
2025-05-16 1:40 ` mr.nuke.me
0 siblings, 1 reply; 19+ messages in thread
From: Lei Wei @ 2025-05-15 15:27 UTC (permalink / raw)
To: Alex G., Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 5/15/2025 10:32 AM, Alex G. wrote:
> On 5/14/25 11:03, Lei Wei wrote:> On 5/13/2025 6:56 AM,
> mr.nuke.me@gmail.com wrote:
>>> On 2/19/25 4:46 AM, Lei Wei wrote:
>>>
>>> I tried this PCS driver, and I am seeing a circular dependency in the
>>> clock init. If the clock tree is:
>>> GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
>>>
>>> The way I understand it, the UNIPHY probe depends on the MII probe.
>>> If MII .probe() returns -EPROBE_DEFER, then so will the
>>> UNIPHY .probe(). But the MII cannot probe until the UNIPHY is done,
>>> due to the clock dependency. How is it supposed to work?
>>>
>>> The way I found to resolve this is to move the probing of the MII
>>> clocks to ipq_pcs_get().
>>>
>>> This is the kernel log that I see:
>>>
>>> [ 12.008754] platform 39b00000.clock-controller: deferred probe
>>> pending: platform: supplier 7a00000.ethernet-pcs not ready
>>> [ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
>>> mdio_bus: supplier 7a20000.ethernet-pcs not ready
>>> [ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>> [ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>> [ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>> [ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>> [ 12.057579] platform 7a00000.ethernet-pcs: deferred probe pending:
>>> ipq9574_pcs: Failed to get MII 0 RX clock
>>> [ 12.067209] platform 7a20000.ethernet-pcs: deferred probe pending:
>>> ipq9574_pcs: Failed to get MII 0 RX clock
>>> [ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
>>> platform: supplier 39b00000.clock-controller not ready
>>>
>>>
>>
>> Hello, thanks for bringing this to our notice. Let me try to
>> understand the reason for the probe failure:
>>
>> The merged NSSCC DTS does not reference the PCS node directly in the
>> "clocks" property. It uses a placeholder phandle '<0>' for the
>> reference. Please see below patch which is merged.
>> https://lore.kernel.org/all/20250313110359.242491-6-
>> quic_mmanikan@quicinc.com/
>>
>> Ideally there should be no direct dependency from NSSCC to PCS driver if
>> we use this version of the NSSCC DTS.
>>
>> Hence it seems that you may have a modified patch here, and DTS
>> changes have been applied to enable all the Ethernet components
>> including PCS and NSSCC, and NSSCC modified to have a direct reference
>> to PCS? However even in this case, I think the driver probe should
>> work if the drivers are built as modules. Can you please confirm if
>> the NSSCC and PCS drivers are built-in to the kernel and not built as
>> modules
>
> The NSSCC and PCS built-in. I also added the uniphy PCS clocks to the
> NSSCC in order to expose the issue.
>
> I have a heavily patched tree with PPE driver and EDMA support. That's
> the final use case in order to support ethernet, right?
>
Yes, all the drivers are eventually for enabling the Ethernet function
on IPQ9574.
>
>> For the case where the drivers are built-in to kernel, and the NSSCC DTS
>> node has a direct reference to PCS node, we can use the below solution:
>> [Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
>> initialization/registration.]
>>
>> Enable 'post-init-providers' property in the NSSCC DTS node to mark
>> 'UNIPHY' PCS as post-initialization providers to NSSCC. This will
>> ensure following probe order by the kernel:
>>
>> 1.) NSSCC driver
>> 2.) PCS driver.
>>
>> Please let me know if the above suggestion can help.
>
> I see. Adding the 'post-init-providers' property does fix the circular
> dependency. Thank you!
>
> I have another question. Do you have a public repository with the
> unmerged IPQ9574 patches, including, PCS, PPE, EDMA, QCA8084 ?
>
May I know the source of your PPE/EDMA changes using which this issue
is seen?
The openwrt repository contains the unmerged IPQ9574 patches, Although
this version will be updated very soon with latest code(with some
fixes), the version of the code in the repo currently is also functional
and tested.
https://github.com/CodeLinaro/openwrt/tree/main/target/linux/qualcommbe/patches-6.6
>
>> Later once the IPQ PCS driver is merged, we are planning to push the
>> PCS DTS changes, along with an update of the NSSCC DTS to point to the
>> PCS node and mark the "post-init-providers" property. This should work
>> for all cases.
>>
>> Also, in my view, it is not suitable to move PCS MII clocks get to
>> "ipq_pcs_get()" because the natural loading order for the drivers
>> is as below:
>>
>> 1) NSSCC driver
>> 2) PCS driver
>> 3) Ethernet driver.
>>
>> Additionally, the community is currently working on an infrastructure to
>> provide a common pcs get method. (Christian and Sean Anderson has been
>> working on this). Therefore, I expect "ipq_pcs_get" to be dropped in
>> the future and replaced with the common pcs get method once this
>> common infra is merged.
>
> That makes sense. Thank you for clarifying.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-05-15 15:27 ` Lei Wei
@ 2025-05-16 1:40 ` mr.nuke.me
2025-05-16 11:18 ` Lei Wei
0 siblings, 1 reply; 19+ messages in thread
From: mr.nuke.me @ 2025-05-16 1:40 UTC (permalink / raw)
To: Lei Wei, Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 5/15/25 10:27 AM, Lei Wei wrote:
>
>
> On 5/15/2025 10:32 AM, Alex G. wrote:
>> On 5/14/25 11:03, Lei Wei wrote:> On 5/13/2025 6:56 AM,
>> mr.nuke.me@gmail.com wrote:
>>>> On 2/19/25 4:46 AM, Lei Wei wrote:
>>>>
>>>> I tried this PCS driver, and I am seeing a circular dependency in
>>>> the clock init. If the clock tree is:
>>>> GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
>>>>
>>>> The way I understand it, the UNIPHY probe depends on the MII probe.
>>>> If MII .probe() returns -EPROBE_DEFER, then so will the
>>>> UNIPHY .probe(). But the MII cannot probe until the UNIPHY is done,
>>>> due to the clock dependency. How is it supposed to work?
>>>>
>>>> The way I found to resolve this is to move the probing of the MII
>>>> clocks to ipq_pcs_get().
>>>>
>>>> This is the kernel log that I see:
>>>>
>>>> [ 12.008754] platform 39b00000.clock-controller: deferred probe
>>>> pending: platform: supplier 7a00000.ethernet-pcs not ready
>>>> [ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
>>>> mdio_bus: supplier 7a20000.ethernet-pcs not ready
>>>> [ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>> [ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>> [ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>> [ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>> [ 12.057579] platform 7a00000.ethernet-pcs: deferred probe
>>>> pending: ipq9574_pcs: Failed to get MII 0 RX clock
>>>> [ 12.067209] platform 7a20000.ethernet-pcs: deferred probe
>>>> pending: ipq9574_pcs: Failed to get MII 0 RX clock
>>>> [ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
>>>> platform: supplier 39b00000.clock-controller not ready
>>>>
>>>>
>>>
>>> Hello, thanks for bringing this to our notice. Let me try to
>>> understand the reason for the probe failure:
>>>
>>> The merged NSSCC DTS does not reference the PCS node directly in the
>>> "clocks" property. It uses a placeholder phandle '<0>' for the
>>> reference. Please see below patch which is merged.
>>> https://lore.kernel.org/all/20250313110359.242491-6-
>>> quic_mmanikan@quicinc.com/
>>>
>>> Ideally there should be no direct dependency from NSSCC to PCS driver if
>>> we use this version of the NSSCC DTS.
>>>
>>> Hence it seems that you may have a modified patch here, and DTS
>>> changes have been applied to enable all the Ethernet components
>>> including PCS and NSSCC, and NSSCC modified to have a direct
>>> reference to PCS? However even in this case, I think the driver probe
>>> should work if the drivers are built as modules. Can you please
>>> confirm if the NSSCC and PCS drivers are built-in to the kernel and
>>> not built as modules
>>
>> The NSSCC and PCS built-in. I also added the uniphy PCS clocks to the
>> NSSCC in order to expose the issue.
>>
>> I have a heavily patched tree with PPE driver and EDMA support. That's
>> the final use case in order to support ethernet, right?
>>
>
> Yes, all the drivers are eventually for enabling the Ethernet function
> on IPQ9574.
>
>>
>>> For the case where the drivers are built-in to kernel, and the NSSCC DTS
>>> node has a direct reference to PCS node, we can use the below solution:
>>> [Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
>>> initialization/registration.]
>>>
>>> Enable 'post-init-providers' property in the NSSCC DTS node to mark
>>> 'UNIPHY' PCS as post-initialization providers to NSSCC. This will
>>> ensure following probe order by the kernel:
>>>
>>> 1.) NSSCC driver
>>> 2.) PCS driver.
>>>
>>> Please let me know if the above suggestion can help.
>>
>> I see. Adding the 'post-init-providers' property does fix the circular
>> dependency. Thank you!
>>
>> I have another question. Do you have a public repository with the
>> unmerged IPQ9574 patches, including, PCS, PPE, EDMA, QCA8084 ?
>>
>
> May I know the source of your PPE/EDMA changes using which this issue
> is seen?
I use a mix of upstream submissions, and openwrt patches. As noted,
using 'post-init-providers' takes care of the problem.
https://github.com/mrnuke/linux/commits/ipq95xx-devel-20250515/
>
> The openwrt repository contains the unmerged IPQ9574 patches, Although
> this version will be updated very soon with latest code(with some
> fixes), the version of the code in the repo currently is also functional
> and tested.
>
> https://github.com/CodeLinaro/openwrt/tree/main/target/linux/qualcommbe/
> patches-6.6
Will you be updating a clock example with IPQ9574 + QCA8084 to the repo?
Alex
>>
>>> Later once the IPQ PCS driver is merged, we are planning to push the
>>> PCS DTS changes, along with an update of the NSSCC DTS to point to
>>> the PCS node and mark the "post-init-providers" property. This should
>>> work for all cases.
>>>
>>> Also, in my view, it is not suitable to move PCS MII clocks get to
>>> "ipq_pcs_get()" because the natural loading order for the drivers
>>> is as below:
>>>
>>> 1) NSSCC driver
>>> 2) PCS driver
>>> 3) Ethernet driver.
>>>
>>> Additionally, the community is currently working on an infrastructure to
>>> provide a common pcs get method. (Christian and Sean Anderson has
>>> been working on this). Therefore, I expect "ipq_pcs_get" to be
>>> dropped in the future and replaced with the common pcs get method
>>> once this common infra is merged.
>>
>> That makes sense. Thank you for clarifying.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
2025-05-16 1:40 ` mr.nuke.me
@ 2025-05-16 11:18 ` Lei Wei
0 siblings, 0 replies; 19+ messages in thread
From: Lei Wei @ 2025-05-16 11:18 UTC (permalink / raw)
To: mr.nuke.me, Russell King (Oracle), Jakub Kicinski
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andrew Lunn,
Heiner Kallweit, netdev, devicetree, linux-kernel, linux-arm-msm,
quic_kkumarcs, quic_suruchia, quic_pavir, quic_linchen, quic_luoj,
srinivas.kandagatla, bartosz.golaszewski, vsmuthu, john,
Krzysztof Kozlowski
On 5/16/2025 9:40 AM, mr.nuke.me@gmail.com wrote:
> On 5/15/25 10:27 AM, Lei Wei wrote:
>>
>>
>> On 5/15/2025 10:32 AM, Alex G. wrote:
>>> On 5/14/25 11:03, Lei Wei wrote:> On 5/13/2025 6:56 AM,
>>> mr.nuke.me@gmail.com wrote:
>>>>> On 2/19/25 4:46 AM, Lei Wei wrote:
>>>>>
>>>>> I tried this PCS driver, and I am seeing a circular dependency in
>>>>> the clock init. If the clock tree is:
>>>>> GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
>>>>>
>>>>> The way I understand it, the UNIPHY probe depends on the MII probe.
>>>>> If MII .probe() returns -EPROBE_DEFER, then so will the
>>>>> UNIPHY .probe(). But the MII cannot probe until the UNIPHY is done,
>>>>> due to the clock dependency. How is it supposed to work?
>>>>>
>>>>> The way I found to resolve this is to move the probing of the MII
>>>>> clocks to ipq_pcs_get().
>>>>>
>>>>> This is the kernel log that I see:
>>>>>
>>>>> [ 12.008754] platform 39b00000.clock-controller: deferred probe
>>>>> pending: platform: supplier 7a00000.ethernet-pcs not ready
>>>>> [ 12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending:
>>>>> mdio_bus: supplier 7a20000.ethernet-pcs not ready
>>>>> [ 12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending:
>>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>>> [ 12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending:
>>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>>> [ 12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending:
>>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>>> [ 12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending:
>>>>> mdio_bus: supplier 90000.mdio-1:18 not ready
>>>>> [ 12.057579] platform 7a00000.ethernet-pcs: deferred probe
>>>>> pending: ipq9574_pcs: Failed to get MII 0 RX clock
>>>>> [ 12.067209] platform 7a20000.ethernet-pcs: deferred probe
>>>>> pending: ipq9574_pcs: Failed to get MII 0 RX clock
>>>>> [ 12.077200] platform 3a000000.qcom-ppe: deferred probe pending:
>>>>> platform: supplier 39b00000.clock-controller not ready
>>>>>
>>>>>
>>>>
>>>> Hello, thanks for bringing this to our notice. Let me try to
>>>> understand the reason for the probe failure:
>>>>
>>>> The merged NSSCC DTS does not reference the PCS node directly in the
>>>> "clocks" property. It uses a placeholder phandle '<0>' for the
>>>> reference. Please see below patch which is merged.
>>>> https://lore.kernel.org/all/20250313110359.242491-6-
>>>> quic_mmanikan@quicinc.com/
>>>>
>>>> Ideally there should be no direct dependency from NSSCC to PCS
>>>> driver if
>>>> we use this version of the NSSCC DTS.
>>>>
>>>> Hence it seems that you may have a modified patch here, and DTS
>>>> changes have been applied to enable all the Ethernet components
>>>> including PCS and NSSCC, and NSSCC modified to have a direct
>>>> reference to PCS? However even in this case, I think the driver
>>>> probe should work if the drivers are built as modules. Can you
>>>> please confirm if the NSSCC and PCS drivers are built-in to the
>>>> kernel and not built as modules
>>>
>>> The NSSCC and PCS built-in. I also added the uniphy PCS clocks to the
>>> NSSCC in order to expose the issue.
>>>
>>> I have a heavily patched tree with PPE driver and EDMA support.
>>> That's the final use case in order to support ethernet, right?
>>>
>>
>> Yes, all the drivers are eventually for enabling the Ethernet function
>> on IPQ9574.
>>
>>>
>>>> For the case where the drivers are built-in to kernel, and the NSSCC
>>>> DTS
>>>> node has a direct reference to PCS node, we can use the below solution:
>>>> [Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
>>>> initialization/registration.]
>>>>
>>>> Enable 'post-init-providers' property in the NSSCC DTS node to
>>>> mark
>>>> 'UNIPHY' PCS as post-initialization providers to NSSCC. This will
>>>> ensure following probe order by the kernel:
>>>>
>>>> 1.) NSSCC driver
>>>> 2.) PCS driver.
>>>>
>>>> Please let me know if the above suggestion can help.
>>>
>>> I see. Adding the 'post-init-providers' property does fix the
>>> circular dependency. Thank you!
>>>
>>> I have another question. Do you have a public repository with the
>>> unmerged IPQ9574 patches, including, PCS, PPE, EDMA, QCA8084 ?
>>>
>>
>> May I know the source of your PPE/EDMA changes using which this issue
>> is seen?
>
> I use a mix of upstream submissions, and openwrt patches. As noted,
> using 'post-init-providers' takes care of the problem.
>
> https://github.com/mrnuke/linux/commits/ipq95xx-devel-20250515/
>
>>
>> The openwrt repository contains the unmerged IPQ9574 patches, Although
>> this version will be updated very soon with latest code(with some
>> fixes), the version of the code in the repo currently is also
>> functional and tested.
>>
>> https://github.com/CodeLinaro/openwrt/tree/main/target/linux/
>> qualcommbe/ patches-6.6
>
>
> Will you be updating a clock example with IPQ9574 + QCA8084 to the repo?
>
> Alex
>
Yes. We are planning to post the updated version of QCA8084 PHY driver
along with the QCA8084 PHY DT example in the DT bindings. This should be
available by the next review cycle.
>>>
>>>> Later once the IPQ PCS driver is merged, we are planning to push the
>>>> PCS DTS changes, along with an update of the NSSCC DTS to point to
>>>> the PCS node and mark the "post-init-providers" property. This
>>>> should work for all cases.
>>>>
>>>> Also, in my view, it is not suitable to move PCS MII clocks get to
>>>> "ipq_pcs_get()" because the natural loading order for the drivers
>>>> is as below:
>>>>
>>>> 1) NSSCC driver
>>>> 2) PCS driver
>>>> 3) Ethernet driver.
>>>>
>>>> Additionally, the community is currently working on an
>>>> infrastructure to
>>>> provide a common pcs get method. (Christian and Sean Anderson has
>>>> been working on this). Therefore, I expect "ipq_pcs_get" to be
>>>> dropped in the future and replaced with the common pcs get method
>>>> once this common infra is merged.
>>>
>>> That makes sense. Thank you for clarifying.
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread