[PATCH 0/5] Add support for the IPQ5018 Internal GE PHY

[PATCH 0/5] Add support for the IPQ5018 Internal GE PHY

[George Moussalem via B4 Relay]

The IPQ5018 SoC contains an internal Gigabit Ethernet PHY with its
output pins that provide an MDI interface to either an external switch
in a PHY to PHY link architecture or directly to an attached RJ45
connector.

The PHY supports 10/100/1000 mbps link modes, CDT, auto-negotiation and
802.3az EEE.

The LDO controller found in the IPQ5018 SoC needs to be enabled to drive
power to the CMN Ethernet Block (CMN BLK) which the GE PHY depends on.
The LDO must be enabled in TCSR by writing to a specific register.

In a phy to phy architecture, DAC values need to be set to accommodate
for the short cable length.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
George Moussalem (5):
      dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support
      clk: qcom: gcc-ipq5018: fix GE PHY reset
      net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support
      arm64: dts: qcom: ipq5018: add MDIO buses
      arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus

 .../devicetree/bindings/net/qca,ar803x.yaml        |  23 +++
 arch/arm64/boot/dts/qcom/ipq5018.dtsi              |  51 ++++-
 drivers/clk/qcom/gcc-ipq5018.c                     |   2 +-
 drivers/net/phy/qcom/Kconfig                       |   2 +-
 drivers/net/phy/qcom/at803x.c                      | 221 ++++++++++++++++++++-
 5 files changed, 287 insertions(+), 12 deletions(-)
---
base-commit: ebfff09f63e3efb6b75b0328b3536d3ce0e26565
change-id: 20250430-ipq5018-ge-phy-db654afa4ced

Best regards,
-- 
George Moussalem <george.moussalem@outlook.com>

[PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

Document the IPQ5018 Internal Gigabit Ethernet PHY found in the IPQ5018
SoC. Its output pins provide an MDI interface to either an external
switch in a PHY to PHY link scenario or is directly attached to an RJ45
connector.

In a phy to phy architecture, DAC values need to be set to accommodate
for the short cable length. As such, add an optional property to do so.

In addition, the LDO controller found in the IPQ5018 SoC needs to be
enabled to driver low voltages to the CMN Ethernet Block (CMN BLK) which
the GE PHY depends on. The LDO must be enabled in TCSR by writing to a
specific register. So, adding a property that takes a phandle to the
TCSR node and the register offset.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 .../devicetree/bindings/net/qca,ar803x.yaml        | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644
--- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
+++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
@@ -60,6 +60,29 @@ properties:
     minimum: 1
     maximum: 255
 
+  qca,dac:
+    description:
+      Values for MDAC and EDAC to adjust amplitude, bias current settings,
+      and error detection and correction algorithm. Only set in a PHY to PHY
+      link architecture to accommodate for short cable length.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    items:
+      - items:
+          - description: value for MDAC. Expected 0x10, if set
+          - description: value for EDAC. Expected 0x10, if set
+      - maxItems: 1
+
+  qca,eth-ldo-enable:
+    description:
+      Register in TCSR to enable the LDO controller to supply
+      low voltages to the common ethernet block (CMN BLK).
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle of TCSR syscon
+          - description: offset of TCSR register to enable the LDO controller
+      - maxItems: 1
+
   vddio-supply:
     description: |
       RGMII I/O voltage regulator (see regulator/regulator.yaml).

-- 
2.49.0

[PATCH 2/5] clk: qcom: gcc-ipq5018: fix GE PHY reset

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

The MISC reset is supposed to trigger a resets across the MDC, DSP, and
RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask
of the reset definition accordingly in the GCC as per the downstream
driver.

Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 drivers/clk/qcom/gcc-ipq5018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
 	[GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 },
 	[GCC_WCSS_Q6_BCR] = { 0x18004, 0 },
 	[GCC_WCSSAON_RESET] = { 0x59010, 0},
-	[GCC_GEPHY_MISC_ARES] = { 0x56004, 0 },
+	[GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf },
 };
 
 static const struct of_device_id gcc_ipq5018_match_table[] = {

-- 
2.49.0

[PATCH 3/5] net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

The IPQ5018 SoC contains a single internal Gigabit Ethernet PHY which
provides an MDI interface directly to an RJ45 connector or an external
switch over a PHY to PHY link.

The internal LDO in the SoC itself needs to be enabled in TCSR to supply
low voltages powering the common ethernet block found in the SoC which
is required by the PHY.

Let's add support for this PHY found in the at803x driver as it falls
within the Qualcomm Atheros OUI.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 drivers/net/phy/qcom/Kconfig  |   2 +-
 drivers/net/phy/qcom/at803x.c | 221 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 214 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/qcom/Kconfig b/drivers/net/phy/qcom/Kconfig
index 570626cc8e14d3e6615f74a6377f0f7c9f723e89..84239e08a8dfa466b0a7b2a5ec724a168b692cd2 100644
--- a/drivers/net/phy/qcom/Kconfig
+++ b/drivers/net/phy/qcom/Kconfig
@@ -7,7 +7,7 @@ config AT803X_PHY
 	select QCOM_NET_PHYLIB
 	depends on REGULATOR
 	help
-	  Currently supports the AR8030, AR8031, AR8033, AR8035 model
+	  Currently supports the AR8030, AR8031, AR8033, AR8035, IPQ5018 model
 
 config QCA83XX_PHY
 	tristate "Qualcomm Atheros QCA833x PHYs"
diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 26350b962890b0321153d74758b13d817407d094..1e30ccbee74e6463be8db9a7819e5f2e7031ebab 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -7,19 +7,24 @@
  * Author: Matus Ujhelyi <ujhelyi.m@gmail.com>
  */
 
-#include <linux/phy.h>
-#include <linux/module.h>
-#include <linux/string.h>
-#include <linux/netdevice.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool_netlink.h>
-#include <linux/bitfield.h>
-#include <linux/regulator/of_regulator.h>
-#include <linux/regulator/driver.h>
-#include <linux/regulator/consumer.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
 #include <linux/of.h>
+#include <linux/phy.h>
 #include <linux/phylink.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/reset.h>
 #include <linux/sfp.h>
+#include <linux/string.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
 #include "qcom.h"
@@ -96,6 +101,8 @@
 #define ATH8035_PHY_ID				0x004dd072
 #define AT8030_PHY_ID_MASK			0xffffffef
 
+#define IPQ5018_PHY_ID				0x004dd0c0
+
 #define QCA9561_PHY_ID				0x004dd042
 
 #define AT803X_PAGE_FIBER			0
@@ -108,6 +115,46 @@
 /* disable hibernation mode */
 #define AT803X_DISABLE_HIBERNATION_MODE		BIT(2)
 
+#define IPQ5018_PHY_FIFO_CONTROL		0x19
+#define IPQ5018_PHY_FIFO_RESET			GENMASK(1, 0)
+
+#define IPQ5018_PHY_DEBUG_EDAC			0x4380
+#define IPQ5018_PHY_MMD1_MDAC			0x8100
+#define IPQ5018_PHY_DAC_MASK			GENMASK(15, 8)
+
+#define IPQ5018_PHY_MMD1_MSE_THRESH1		0x1000
+#define IPQ5018_PHY_MMD1_MSE_THRESH2		0x1001
+#define IPQ5018_PHY_MMD3_AZ_CTRL1		0x8008
+#define IPQ5018_PHY_MMD3_AZ_CTRL2		0x8009
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL3	0x8074
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL4	0x8075
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL5	0x8076
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL6	0x8077
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL7	0x8078
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL9	0x807a
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL13	0x807e
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL14	0x807f
+
+#define IPQ5018_PHY_MMD1_MSE_THRESH1_VAL	0xf1
+#define IPQ5018_PHY_MMD1_MSE_THRESH2_VAL	0x1f6
+#define IPQ5018_PHY_MMD3_AZ_CTRL1_VAL		0x7880
+#define IPQ5018_PHY_MMD3_AZ_CTRL2_VAL		0xc8
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL3_VAL	0xc040
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL4_VAL	0xa060
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL5_VAL	0xc040
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL6_VAL	0xa060
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL7_VAL	0xc24c
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL9_VAL	0xc060
+#define IPQ5018_PHY_MMD3_CDT_THRESH_CTRL13_VAL	0xb060
+#define IPQ5018_PHY_MMD3_NEAR_ECHO_THRESH_VAL	0x90b0
+
+#define IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE		0x1
+#define IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_MASK	GENMASK(7, 4)
+#define IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_DEFAULT	0x50
+#define IPQ5018_PHY_DEBUG_ANA_DAC_FILTER	0xa080
+
+#define IPQ5018_TCSR_ETH_LDO_READY		BIT(0)
+
 MODULE_DESCRIPTION("Qualcomm Atheros AR803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
@@ -133,6 +180,14 @@ struct at803x_context {
 	u16 led_control;
 };
 
+struct ipq5018_priv {
+	int num_clks;
+	struct clk_bulk_data *clks;
+	struct reset_control *rst;
+	u32 mdac;
+	u32 edac;
+};
+
 static int at803x_write_page(struct phy_device *phydev, int page)
 {
 	int mask;
@@ -987,6 +1042,142 @@ static int at8035_probe(struct phy_device *phydev)
 	return at8035_parse_dt(phydev);
 }
 
+static inline int ipq5018_cmnblk_enable(struct device *dev)
+{
+	unsigned int offset;
+	struct regmap *tcsr;
+
+	tcsr = syscon_regmap_lookup_by_phandle_args(dev->of_node, "qca,eth-ldo-ready",
+						    1, &offset);
+	if (IS_ERR(tcsr))
+		return PTR_ERR(tcsr);
+
+	return regmap_set_bits(tcsr, offset, IPQ5018_TCSR_ETH_LDO_READY);
+}
+
+static int ipq5018_cable_test_start(struct phy_device *phydev)
+{
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL3,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL3_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL4,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL4_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL5,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL5_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL6,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL6_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL7,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL7_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL9,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL9_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL13,
+		      IPQ5018_PHY_MMD3_CDT_THRESH_CTRL13_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_CDT_THRESH_CTRL3,
+		      IPQ5018_PHY_MMD3_NEAR_ECHO_THRESH_VAL);
+
+	/* we do all the (time consuming) work later */
+	return 0;
+}
+
+static int ipq5018_config_init(struct phy_device *phydev)
+{
+	struct ipq5018_priv *priv = phydev->priv;
+	u16 val = 0;
+
+	/*
+	 * set LDO efuse: first temporarily store ANA_DAC_FILTER value from
+	 * debug register as it will be reset once the ANA_LDO_EFUSE register
+	 * is written to
+	 */
+	val = at803x_debug_reg_read(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER);
+	at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE,
+			      IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_MASK,
+			      IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_DEFAULT);
+	at803x_debug_reg_write(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER, val);
+
+	/* set 8023AZ CTRL values */
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_AZ_CTRL1,
+		      IPQ5018_PHY_MMD3_AZ_CTRL1_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_AZ_CTRL2,
+		      IPQ5018_PHY_MMD3_AZ_CTRL2_VAL);
+
+	/* set MSE threshold values */
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MSE_THRESH1,
+		      IPQ5018_PHY_MMD1_MSE_THRESH1_VAL);
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MSE_THRESH2,
+		      IPQ5018_PHY_MMD1_MSE_THRESH2_VAL);
+
+	if (priv->mdac && priv->edac) {
+		/* setting MDAC (Multi-level Digital-to-Analog Converter) in MMD1 */
+		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MDAC,
+			       IPQ5018_PHY_DAC_MASK, priv->mdac);
+
+		/* setting EDAC (Error-detection and Correction) in debug register */
+		at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_EDAC,
+				      IPQ5018_PHY_DAC_MASK, priv->edac);
+	}
+
+	return 0;
+}
+
+static void ipq5018_link_change_notify(struct phy_device *phydev)
+{
+	mdiobus_modify_changed(phydev->mdio.bus, phydev->mdio.addr,
+			       IPQ5018_PHY_FIFO_CONTROL, IPQ5018_PHY_FIFO_RESET,
+			       phydev->link ? IPQ5018_PHY_FIFO_RESET : 0);
+}
+
+static int ipq5018_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct ipq5018_priv *priv;
+	u32 mdac, edac = 0;
+	int ret, cnt;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* PHY DAC values are optional and only set in a PHY to PHY link architecture */
+	cnt = of_property_count_u32_elems(dev->of_node, "qca,dac");
+	if (cnt == 2) {
+		ret = of_property_read_u32_index(dev->of_node, "qca,dac", 0, &mdac);
+		if (!ret)
+			priv->mdac = mdac;
+
+		ret = of_property_read_u32_index(dev->of_node, "qca,dac", 1, &edac);
+		if (!ret)
+			priv->edac = edac;
+	}
+
+	ret = ipq5018_cmnblk_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable LDO controller to CMN BLK");
+
+	priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks);
+	if (priv->num_clks < 0)
+		return dev_err_probe(dev, priv->num_clks,
+				     "failed to acquire clocks\n");
+
+	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to enable clocks\n");
+
+	priv->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR_OR_NULL(priv->rst))
+		return dev_err_probe(dev, PTR_ERR(priv->rst),
+				     "failed to acquire reset\n");
+
+	ret = reset_control_reset(priv->rst);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to reset\n");
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
@@ -1078,6 +1269,19 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
+}, {
+	PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID),
+	.name			= "Qualcomm Atheros IPQ5018 internal PHY",
+	.flags			= PHY_IS_INTERNAL | PHY_POLL_CABLE_TEST,
+	.probe			= ipq5018_probe,
+	.config_init		= ipq5018_config_init,
+	.link_change_notify	= ipq5018_link_change_notify,
+	.read_status		= at803x_read_status,
+	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
+	.cable_test_start	= ipq5018_cable_test_start,
+	.cable_test_get_status	= qca808x_cable_test_get_status,
+	.soft_reset		= genphy_soft_reset,
 }, {
 	/* Qualcomm Atheros QCA9561 */
 	PHY_ID_MATCH_EXACT(QCA9561_PHY_ID),
@@ -1104,6 +1308,7 @@ static const struct mdio_device_id __maybe_unused atheros_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(ATH8032_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH8035_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(ATH9331_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID) },
 	{ PHY_ID_MATCH_EXACT(QCA9561_PHY_ID) },
 	{ }
 };

-- 
2.49.0

[PATCH 4/5] arm64: dts: qcom: ipq5018: add MDIO buses

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

IPQ5018 contains two mdio buses of which one bus is used to control the
SoC's internal GE PHY, while the other bus is connected to external PHYs
or switches.

There's already support for IPQ5018 in the mdio-ipq4019 driver, so let's
simply add the mdio nodes for them.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 130360014c5e14c778e348d37e601f60325b0b14..03ebc3e305b267c98a034c41ce47a39269afce75 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -182,6 +182,30 @@ pcie0_phy: phy@86000 {
 			status = "disabled";
 		};
 
+		mdio0: mdio@88000 {
+			compatible = "qcom,ipq5018-mdio";
+			reg = <0x00088000 0x64>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			clocks = <&gcc GCC_MDIO0_AHB_CLK>;
+			clock-names = "gcc_mdio_ahb_clk";
+
+			status = "disabled";
+		};
+
+		mdio1: mdio@90000 {
+			compatible = "qcom,ipq5018-mdio";
+			reg = <0x00090000 0x64>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			clocks = <&gcc GCC_MDIO1_AHB_CLK>;
+			clock-names = "gcc_mdio_ahb_clk";
+
+			status = "disabled";
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5018-tlmm";
 			reg = <0x01000000 0x300000>;

-- 
2.49.0

[PATCH 5/5] arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

The IPQ5018 SoC contains an internal GE PHY, always at phy address 7.
As such, let's add the GE PHY node to the SoC dtsi.

In addition, the GE PHY outputs both the RX and TX clocks to the GCC
which gate controls them and routes them back to the PHY itself.
So let's create two DT fixed clocks and register them in the GCC node.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 03ebc3e305b267c98a034c41ce47a39269afce75..ff2de44f9b85993fb2d426f85676f7d54c5cf637 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -16,6 +16,18 @@ / {
 	#size-cells = <2>;
 
 	clocks {
+		gephy_rx_clk: gephy-rx-clk {
+			compatible = "fixed-clock";
+			clock-frequency = <125000000>;
+			#clock-cells = <0>;
+		};
+
+		gephy_tx_clk: gephy-tx-clk {
+			compatible = "fixed-clock";
+			clock-frequency = <125000000>;
+			#clock-cells = <0>;
+		};
+
 		sleep_clk: sleep-clk {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
@@ -192,6 +204,17 @@ mdio0: mdio@88000 {
 			clock-names = "gcc_mdio_ahb_clk";
 
 			status = "disabled";
+
+			ge_phy: ethernet-phy@7 {
+				reg = <7>;
+
+				clocks = <&gcc GCC_GEPHY_RX_CLK>,
+					 <&gcc GCC_GEPHY_TX_CLK>;
+
+				resets = <&gcc GCC_GEPHY_MISC_ARES>;
+
+				qca,eth-ldo-ready = <&tcsr 0x105c4>;
+			};
 		};
 
 		mdio1: mdio@90000 {
@@ -232,8 +255,8 @@ gcc: clock-controller@1800000 {
 				 <&pcie0_phy>,
 				 <&pcie1_phy>,
 				 <0>,
-				 <0>,
-				 <0>,
+				 <&gephy_rx_clk>,
+				 <&gephy_tx_clk>,
 				 <0>,
 				 <0>;
 			#clock-cells = <1>;

-- 
2.49.0

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Andrew Lunn]

On Sun, May 25, 2025 at 09:56:04PM +0400, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> Document the IPQ5018 Internal Gigabit Ethernet PHY found in the IPQ5018
> SoC. Its output pins provide an MDI interface to either an external
> switch in a PHY to PHY link scenario or is directly attached to an RJ45
> connector.
> 
> In a phy to phy architecture, DAC values need to be set to accommodate
> for the short cable length. As such, add an optional property to do so.
> 
> In addition, the LDO controller found in the IPQ5018 SoC needs to be
> enabled to driver low voltages to the CMN Ethernet Block (CMN BLK) which
> the GE PHY depends on. The LDO must be enabled in TCSR by writing to a
> specific register. So, adding a property that takes a phandle to the
> TCSR node and the register offset.
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
>  .../devicetree/bindings/net/qca,ar803x.yaml        | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644
> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> @@ -60,6 +60,29 @@ properties:
>      minimum: 1
>      maximum: 255
>  
> +  qca,dac:
> +    description:
> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
> +      and error detection and correction algorithm. Only set in a PHY to PHY
> +      link architecture to accommodate for short cable length.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    items:
> +      - items:
> +          - description: value for MDAC. Expected 0x10, if set
> +          - description: value for EDAC. Expected 0x10, if set

DT is not a collection of magic values to be poked into registers.

A bias current should be mA, amplitude probably in mV, and error
detection as an algorithm. 

	Andrew

Re: [PATCH 3/5] net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support

[Andrew Lunn]

> +	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_AZ_CTRL1,
> +		      IPQ5018_PHY_MMD3_AZ_CTRL1_VAL);

Using MMD3 in the name is a good idea, but PCS would be better. Not
everybody knows MDIO_MMD_PCS == 3.
	
    Andrew

---
pw-bot: cr

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Krzysztof Kozlowski]

On 25/05/2025 19:56, George Moussalem via B4 Relay wrote:
> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644
> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> @@ -60,6 +60,29 @@ properties:
>      minimum: 1
>      maximum: 255
>  
> +  qca,dac:
> +    description:
> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
> +      and error detection and correction algorithm. Only set in a PHY to PHY
> +      link architecture to accommodate for short cable length.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    items:
> +      - items:
> +          - description: value for MDAC. Expected 0x10, if set
> +          - description: value for EDAC. Expected 0x10, if set

If this is fixed to 0x10, then this is fully deducible from compatible.
Drop entire property.

> +      - maxItems: 1
> +
> +  qca,eth-ldo-enable:

qcom,tcsr-syscon to match property already used.

> +    description:
> +      Register in TCSR to enable the LDO controller to supply
> +      low voltages to the common ethernet block (CMN BLK).
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle of TCSR syscon
> +          - description: offset of TCSR register to enable the LDO controller
> +      - maxItems: 1
You listed two items, but second is just one item? Drop.

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

Hi Andrew,

On 5/25/25 23:35, Andrew Lunn wrote:
> On Sun, May 25, 2025 at 09:56:04PM +0400, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@outlook.com>
>>
>> Document the IPQ5018 Internal Gigabit Ethernet PHY found in the IPQ5018
>> SoC. Its output pins provide an MDI interface to either an external
>> switch in a PHY to PHY link scenario or is directly attached to an RJ45
>> connector.
>>
>> In a phy to phy architecture, DAC values need to be set to accommodate
>> for the short cable length. As such, add an optional property to do so.
>>
>> In addition, the LDO controller found in the IPQ5018 SoC needs to be
>> enabled to driver low voltages to the CMN Ethernet Block (CMN BLK) which
>> the GE PHY depends on. The LDO must be enabled in TCSR by writing to a
>> specific register. So, adding a property that takes a phandle to the
>> TCSR node and the register offset.
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>>   .../devicetree/bindings/net/qca,ar803x.yaml        | 23 ++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644
>> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> @@ -60,6 +60,29 @@ properties:
>>       minimum: 1
>>       maximum: 255
>>   
>> +  qca,dac:
>> +    description:
>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>> +      link architecture to accommodate for short cable length.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    items:
>> +      - items:
>> +          - description: value for MDAC. Expected 0x10, if set
>> +          - description: value for EDAC. Expected 0x10, if set
> 
> DT is not a collection of magic values to be poked into registers.
> 
> A bias current should be mA, amplitude probably in mV, and error
> detection as an algorithm.

I couldn't agree more but I just don't know what these values are 
exactly as they aren't documented anywhere. I'm working off the 
downstream QCA-SSDK codelinaro repo. My *best guess* for the MDAC value 
is that it halves the amplitude and current for short cable length, but 
I don't know what algorithm is used for error detection and correction.

What I do know is that values must be written in a phy to phy link 
architecture as the 'cable length' is short, a few cm at most. Without 
setting these values, the link doesn't work.

With the lack of proper documentation, I could move the values to the 
driver itself and convert it to a boolean property such as 
qca,phy-to-phy-dac or something..

Any suggestions?

> 
> 	Andrew

Best regards,
George

Re: [PATCH 3/5] net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support

[George Moussalem]

Hi Andrew,

On 5/25/25 23:42, Andrew Lunn wrote:
>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_MMD3_AZ_CTRL1,
>> +		      IPQ5018_PHY_MMD3_AZ_CTRL1_VAL);
> 
> Using MMD3 in the name is a good idea, but PCS would be better. Not
> everybody knows MDIO_MMD_PCS == 3.

Will rename all IPQ5018_PHY_MMD3* macros to IPQ5018_PHY_PCS* in next 
version.

> 	
>      Andrew
> 
> ---
> pw-bot: cr

Thanks,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

On 5/26/25 08:17, Krzysztof Kozlowski wrote:
> On 25/05/2025 19:56, George Moussalem via B4 Relay wrote:
>> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644
>> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
>> @@ -60,6 +60,29 @@ properties:
>>       minimum: 1
>>       maximum: 255
>>   
>> +  qca,dac:
>> +    description:
>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>> +      link architecture to accommodate for short cable length.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    items:
>> +      - items:
>> +          - description: value for MDAC. Expected 0x10, if set
>> +          - description: value for EDAC. Expected 0x10, if set
> 
> If this is fixed to 0x10, then this is fully deducible from compatible.
> Drop entire property.

as mentioned to Andrew, I can move the required values to the driver 
itself, but a property would still be required to indicate that this PHY 
is connected to an external PHY (ex. qca8337 switch). In that case, the 
values need to be set. Otherwise, not..

Would qcom,phy-to-phy-dac (boolean) do?

> 
>> +      - maxItems: 1
>> +
>> +  qca,eth-ldo-enable:
> 
> qcom,tcsr-syscon to match property already used.

to make sure I understand correctly, rename it to qcom,tcsr-syscon?

> 
>> +    description:
>> +      Register in TCSR to enable the LDO controller to supply
>> +      low voltages to the common ethernet block (CMN BLK).
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle of TCSR syscon
>> +          - description: offset of TCSR register to enable the LDO controller
>> +      - maxItems: 1
> You listed two items, but second is just one item? Drop.

What is expected is one item that has two values, in this case: <&tcsr 
0x019475c4>

I could move the offset to the driver itself as it's a fixed offset, so 
ultimately the property would become:

qcom,tcsr-syscon = <&tscr>;

agreed?

> 
> Best regards,
> Krzysztof
> 

Best regards,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Krzysztof Kozlowski]

On 26/05/2025 08:43, George Moussalem wrote:
>>> +  qca,dac:
>>> +    description:
>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>> +      link architecture to accommodate for short cable length.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    items:
>>> +      - items:
>>> +          - description: value for MDAC. Expected 0x10, if set
>>> +          - description: value for EDAC. Expected 0x10, if set
>>
>> If this is fixed to 0x10, then this is fully deducible from compatible.
>> Drop entire property.
> 
> as mentioned to Andrew, I can move the required values to the driver 
> itself, but a property would still be required to indicate that this PHY 
> is connected to an external PHY (ex. qca8337 switch). In that case, the 
> values need to be set. Otherwise, not..
> 
> Would qcom,phy-to-phy-dac (boolean) do?

Seems fine to me.

> 
>>
>>> +      - maxItems: 1
>>> +
>>> +  qca,eth-ldo-enable:
>>
>> qcom,tcsr-syscon to match property already used.
> 
> to make sure I understand correctly, rename it to qcom,tcsr-syscon?

Yes

> 
>>
>>> +    description:
>>> +      Register in TCSR to enable the LDO controller to supply
>>> +      low voltages to the common ethernet block (CMN BLK).
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    items:
>>> +      - items:
>>> +          - description: phandle of TCSR syscon
>>> +          - description: offset of TCSR register to enable the LDO controller
>>> +      - maxItems: 1
>> You listed two items, but second is just one item? Drop.
> 
> What is expected is one item that has two values, in this case: <&tcsr 
> 0x019475c4>

I know.

> 
> I could move the offset to the driver itself as it's a fixed offset, so 
> ultimately the property would become:
> 
> qcom,tcsr-syscon = <&tscr>;
> 
> agreed?
No. Just fix the syntax.

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Andrew Lunn]

> I couldn't agree more but I just don't know what these values are exactly as
> they aren't documented anywhere. I'm working off the downstream QCA-SSDK
> codelinaro repo. My *best guess* for the MDAC value is that it halves the
> amplitude and current for short cable length, but I don't know what
> algorithm is used for error detection and correction.
> 
> What I do know is that values must be written in a phy to phy link
> architecture as the 'cable length' is short, a few cm at most. Without
> setting these values, the link doesn't work.
> 
> With the lack of proper documentation, I could move the values to the driver
> itself and convert it to a boolean property such as qca,phy-to-phy-dac or
> something..

Making it a boolean property is good. Please include in the binding
the behaviour when the bool is not present.

What you are really describing here is a sort cable, not phy-to-phy,
since a PHY is always connected to another PHY. So i think the
property name should be about the sort cable/distance.

	Andrew

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

On 5/26/25 17:34, Andrew Lunn wrote:
>> I couldn't agree more but I just don't know what these values are exactly as
>> they aren't documented anywhere. I'm working off the downstream QCA-SSDK
>> codelinaro repo. My *best guess* for the MDAC value is that it halves the
>> amplitude and current for short cable length, but I don't know what
>> algorithm is used for error detection and correction.
>>
>> What I do know is that values must be written in a phy to phy link
>> architecture as the 'cable length' is short, a few cm at most. Without
>> setting these values, the link doesn't work.
>>
>> With the lack of proper documentation, I could move the values to the driver
>> itself and convert it to a boolean property such as qca,phy-to-phy-dac or
>> something..
> 
> Making it a boolean property is good. Please include in the binding
> the behaviour when the bool is not present.

Thanks, will do.

> 
> What you are really describing here is a sort cable, not phy-to-phy,
> since a PHY is always connected to another PHY. So i think the
> property name should be about the sort cable/distance.

The two common archictures across IPQ5018 boards are:
1. IPQ5018 PHY --> MDI --> RJ45 connector
2. IPQ5018 PHY --> MDI --> External PHY (ex. a PHY of a qca8337 switch)
Only in scenario 2 (phy to phy architecture), DAC values need to be set 
to accommodate for the short cable length. Nothing needs to be set in 
scenario 1 when connected directly to an RJ45 connector.

if not, qcom,phy-to-phy-dac, perhaps qcom,dac-short-cable or any other 
suggestions?

> 
> 	Andrew

Best regards,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Konrad Dybcio]

On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
> On 26/05/2025 08:43, George Moussalem wrote:
>>>> +  qca,dac:
>>>> +    description:
>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>> +      link architecture to accommodate for short cable length.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    items:
>>>> +      - items:
>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>
>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>> Drop entire property.
>>
>> as mentioned to Andrew, I can move the required values to the driver 
>> itself, but a property would still be required to indicate that this PHY 
>> is connected to an external PHY (ex. qca8337 switch). In that case, the 
>> values need to be set. Otherwise, not..
>>
>> Would qcom,phy-to-phy-dac (boolean) do?
> 
> Seems fine to me.

Can the driver instead check for a phy reference?

Konrad

Re: [PATCH 2/5] clk: qcom: gcc-ipq5018: fix GE PHY reset

[Konrad Dybcio]

On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> The MISC reset is supposed to trigger a resets across the MDC, DSP, and
> RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask
> of the reset definition accordingly in the GCC as per the downstream
> driver.
> 
> Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
>  drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644
> --- a/drivers/clk/qcom/gcc-ipq5018.c
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
>  	[GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 },
>  	[GCC_WCSS_Q6_BCR] = { 0x18004, 0 },
>  	[GCC_WCSSAON_RESET] = { 0x59010, 0},
> -	[GCC_GEPHY_MISC_ARES] = { 0x56004, 0 },
> +	[GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf },

The computer tells me there aren't any bits beyond this mask..

Does this actually fix anything?

Konrad

Re: [PATCH 4/5] arm64: dts: qcom: ipq5018: add MDIO buses

[Konrad Dybcio]

On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> IPQ5018 contains two mdio buses of which one bus is used to control the
> SoC's internal GE PHY, while the other bus is connected to external PHYs
> or switches.
> 
> There's already support for IPQ5018 in the mdio-ipq4019 driver, so let's
> simply add the mdio nodes for them.
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 130360014c5e14c778e348d37e601f60325b0b14..03ebc3e305b267c98a034c41ce47a39269afce75 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -182,6 +182,30 @@ pcie0_phy: phy@86000 {
>  			status = "disabled";
>  		};
>  
> +		mdio0: mdio@88000 {
> +			compatible = "qcom,ipq5018-mdio";
> +			reg = <0x00088000 0x64>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			clocks = <&gcc GCC_MDIO0_AHB_CLK>;
> +			clock-names = "gcc_mdio_ahb_clk";

I see there's resets named GCC_MDIO[01]_BCR - are they related to
these hosts?

fwiw the addressses look good

Konrad

Re: [PATCH 2/5] clk: qcom: gcc-ipq5018: fix GE PHY reset

[George Moussalem]

Hi Konrad,

On 5/27/25 15:00, Konrad Dybcio wrote:
> On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@outlook.com>
>>
>> The MISC reset is supposed to trigger a resets across the MDC, DSP, and
>> RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask
>> of the reset definition accordingly in the GCC as per the downstream
>> driver.
>>
>> Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>>   drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
>> index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644
>> --- a/drivers/clk/qcom/gcc-ipq5018.c
>> +++ b/drivers/clk/qcom/gcc-ipq5018.c
>> @@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
>>   	[GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 },
>>   	[GCC_WCSS_Q6_BCR] = { 0x18004, 0 },
>>   	[GCC_WCSSAON_RESET] = { 0x59010, 0},
>> -	[GCC_GEPHY_MISC_ARES] = { 0x56004, 0 },
>> +	[GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf },
> 
> The computer tells me there aren't any bits beyond this mask..
> 
> Does this actually fix anything?

The mask is documented in the referenced downstream driver and allows 
for consolidating:

resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
	 <&gcc GCC_GEPHY_DSP_HW_ARES>,
	 <&gcc GCC_GEPHY_RX_ARES>,
	 <&gcc GCC_GEPHY_TX_ARES>;
to:

resets = <&gcc GCC_MISC_ARES>;

to conform to this bindings restriction in ethernet-phy.yaml

   resets:
     maxItems: 1

Effectively, there's no functional change. So we can also list all the 
resets in the device tree, whatever is preferred.

> 
> Konrad

Thanks,
George

Re: [PATCH 2/5] clk: qcom: gcc-ipq5018: fix GE PHY reset

[Konrad Dybcio]

On 5/27/25 1:14 PM, George Moussalem wrote:
> Hi Konrad,
> 
> On 5/27/25 15:00, Konrad Dybcio wrote:
>> On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
>>> From: George Moussalem <george.moussalem@outlook.com>
>>>
>>> The MISC reset is supposed to trigger a resets across the MDC, DSP, and
>>> RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask
>>> of the reset definition accordingly in the GCC as per the downstream
>>> driver.
>>>
>>> Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932
>>>
>>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>>> ---
>>>   drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
>>> index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644
>>> --- a/drivers/clk/qcom/gcc-ipq5018.c
>>> +++ b/drivers/clk/qcom/gcc-ipq5018.c
>>> @@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
>>>       [GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 },
>>>       [GCC_WCSS_Q6_BCR] = { 0x18004, 0 },
>>>       [GCC_WCSSAON_RESET] = { 0x59010, 0},
>>> -    [GCC_GEPHY_MISC_ARES] = { 0x56004, 0 },
>>> +    [GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf },
>>
>> The computer tells me there aren't any bits beyond this mask..
>>
>> Does this actually fix anything?
> 
> The mask is documented in the referenced downstream driver and allows for consolidating:
> 
> resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
>      <&gcc GCC_GEPHY_DSP_HW_ARES>,
>      <&gcc GCC_GEPHY_RX_ARES>,
>      <&gcc GCC_GEPHY_TX_ARES>;
> to:
> 
> resets = <&gcc GCC_MISC_ARES>;
> 
> to conform to this bindings restriction in ethernet-phy.yaml
> 
>   resets:
>     maxItems: 1
> 
> Effectively, there's no functional change. So we can also list all the resets in the device tree, whatever is preferred.

+ Kathiravan

are there any recommendations from the hw team on which one to use?
As far as I can tell, the _MISC one simply pulls all the aforementioned
resets, like George described.. it seems weird that it would be designed
like this

Konrad

Re: [PATCH 4/5] arm64: dts: qcom: ipq5018: add MDIO buses

[George Moussalem]

Hi Konrad

On 5/27/25 15:07, Konrad Dybcio wrote:
> On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@outlook.com>
>>
>> IPQ5018 contains two mdio buses of which one bus is used to control the
>> SoC's internal GE PHY, while the other bus is connected to external PHYs
>> or switches.
>>
>> There's already support for IPQ5018 in the mdio-ipq4019 driver, so let's
>> simply add the mdio nodes for them.
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> index 130360014c5e14c778e348d37e601f60325b0b14..03ebc3e305b267c98a034c41ce47a39269afce75 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> @@ -182,6 +182,30 @@ pcie0_phy: phy@86000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		mdio0: mdio@88000 {
>> +			compatible = "qcom,ipq5018-mdio";
>> +			reg = <0x00088000 0x64>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			clocks = <&gcc GCC_MDIO0_AHB_CLK>;
>> +			clock-names = "gcc_mdio_ahb_clk";
> 
> I see there's resets named GCC_MDIO[01]_BCR - are they related to
> these hosts?

Yes, they are specific to these mdio buses, yet not required for 
operation, just like ipq8074 and ipq6018 don't list this reset in their 
respective nodes in the dtsi.

> 
> fwiw the addressses look good
> 
> Konrad

Best regards,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

Hi Konrad,

On 5/27/25 14:59, Konrad Dybcio wrote:
> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
>> On 26/05/2025 08:43, George Moussalem wrote:
>>>>> +  qca,dac:
>>>>> +    description:
>>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>>> +      link architecture to accommodate for short cable length.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>> +    items:
>>>>> +      - items:
>>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>>
>>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>>> Drop entire property.
>>>
>>> as mentioned to Andrew, I can move the required values to the driver
>>> itself, but a property would still be required to indicate that this PHY
>>> is connected to an external PHY (ex. qca8337 switch). In that case, the
>>> values need to be set. Otherwise, not..
>>>
>>> Would qcom,phy-to-phy-dac (boolean) do?
>>
>> Seems fine to me.
> 
> Can the driver instead check for a phy reference?

Do you mean using the existing phy-handle DT property or create a new DT 
property called 'qcom,phy-reference'? Either way, can add it for v2.

> 
> Konrad

Best regards,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Konrad Dybcio]

On 5/27/25 1:28 PM, George Moussalem wrote:
> Hi Konrad,
> 
> On 5/27/25 14:59, Konrad Dybcio wrote:
>> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
>>> On 26/05/2025 08:43, George Moussalem wrote:
>>>>>> +  qca,dac:
>>>>>> +    description:
>>>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>>>> +      link architecture to accommodate for short cable length.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>> +    items:
>>>>>> +      - items:
>>>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>>>
>>>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>>>> Drop entire property.
>>>>
>>>> as mentioned to Andrew, I can move the required values to the driver
>>>> itself, but a property would still be required to indicate that this PHY
>>>> is connected to an external PHY (ex. qca8337 switch). In that case, the
>>>> values need to be set. Otherwise, not..
>>>>
>>>> Would qcom,phy-to-phy-dac (boolean) do?
>>>
>>> Seems fine to me.
>>
>> Can the driver instead check for a phy reference?
> 
> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.

I'm not sure how this is all wired up. Do you have an example of a DT
with both configurations you described in your reply to Andrew?

Konrad

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

On 5/27/25 15:31, Konrad Dybcio wrote:
> On 5/27/25 1:28 PM, George Moussalem wrote:
>> Hi Konrad,
>>
>> On 5/27/25 14:59, Konrad Dybcio wrote:
>>> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
>>>> On 26/05/2025 08:43, George Moussalem wrote:
>>>>>>> +  qca,dac:
>>>>>>> +    description:
>>>>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>>>>> +      link architecture to accommodate for short cable length.
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>>> +    items:
>>>>>>> +      - items:
>>>>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>>>>
>>>>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>>>>> Drop entire property.
>>>>>
>>>>> as mentioned to Andrew, I can move the required values to the driver
>>>>> itself, but a property would still be required to indicate that this PHY
>>>>> is connected to an external PHY (ex. qca8337 switch). In that case, the
>>>>> values need to be set. Otherwise, not..
>>>>>
>>>>> Would qcom,phy-to-phy-dac (boolean) do?
>>>>
>>>> Seems fine to me.
>>>
>>> Can the driver instead check for a phy reference?
>>
>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.
> 
> I'm not sure how this is all wired up. Do you have an example of a DT
> with both configurations you described in your reply to Andrew?

Sure, for IPQ5018 GE PHY connected to a QCA8337 switch (phy to phy):
Link: 
https://github.com/openwrt/openwrt/blob/main/target/linux/qualcommax/files/arch/arm64/boot/dts/qcom/ipq5018-spnmx56.dts
In this scenario, the IPQ5018 single UNIPHY is freed up and can be used 
with an external PHY such as QCA8081 to offer up to 2.5 gbps 
connectivity, see diagram below:

* =================================================================
*     _______________________             _______________________
*    |        IPQ5018        |           |        QCA8337        |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    | | MAC0 |---| GE Phy |-+--- MDI ---+ | Phy4   |---| MAC5 | |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    |                       |           |_______________________|
*    |                       |            _______________________
*    |                       |           |        QCA8081        |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    |_______________________|           |_______________________|
*
* =================================================================

The other use case is when an external switch or PHY, if any, is 
connected to the IPQ5018 UNIPHY over SGMII(+), freeing up the GE PHY 
which can optionally be connected to an RJ45 connector. I haven't worked 
on such board yet where the GE PHY is directly connected to RJ45, but I 
believe the Linksys MX6200 has this architecture (which I'll look into 
soon).

* =================================================================
*     _______________________             ____________
*    |        IPQ5018        |           |            |
*    | +------+   +--------+ |           | +--------+ |
*    | | MAC0 |---| GE Phy |-+--- MDI ---+ | RJ45   | +
*    | +------+   +--------+ |           | +--------+ |
*    |                       |           |____________|
*    |                       |            _______________________
*    |                       |           |      QCA8081 Phy      |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
*    | +------+   +--------+ |           | +--------+   +------+ |
*    |_______________________|           |_______________________|
*
* =================================================================
> 
> Konrad

Best regards,
George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Konrad Dybcio]

On 5/27/25 2:13 PM, George Moussalem wrote:
> 
> 
> On 5/27/25 15:31, Konrad Dybcio wrote:
>> On 5/27/25 1:28 PM, George Moussalem wrote:
>>> Hi Konrad,
>>>
>>> On 5/27/25 14:59, Konrad Dybcio wrote:
>>>> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
>>>>> On 26/05/2025 08:43, George Moussalem wrote:
>>>>>>>> +  qca,dac:
>>>>>>>> +    description:
>>>>>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>>>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>>>>>> +      link architecture to accommodate for short cable length.
>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>>>> +    items:
>>>>>>>> +      - items:
>>>>>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>>>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>>>>>
>>>>>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>>>>>> Drop entire property.
>>>>>>
>>>>>> as mentioned to Andrew, I can move the required values to the driver
>>>>>> itself, but a property would still be required to indicate that this PHY
>>>>>> is connected to an external PHY (ex. qca8337 switch). In that case, the
>>>>>> values need to be set. Otherwise, not..
>>>>>>
>>>>>> Would qcom,phy-to-phy-dac (boolean) do?
>>>>>
>>>>> Seems fine to me.
>>>>
>>>> Can the driver instead check for a phy reference?
>>>
>>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.
>>
>> I'm not sure how this is all wired up. Do you have an example of a DT
>> with both configurations you described in your reply to Andrew?
> 
> Sure, for IPQ5018 GE PHY connected to a QCA8337 switch (phy to phy):
> Link: https://github.com/openwrt/openwrt/blob/main/target/linux/qualcommax/files/arch/arm64/boot/dts/qcom/ipq5018-spnmx56.dts
> In this scenario, the IPQ5018 single UNIPHY is freed up and can be used with an external PHY such as QCA8081 to offer up to 2.5 gbps connectivity, see diagram below:
> 
> * =================================================================
> *     _______________________             _______________________
> *    |        IPQ5018        |           |        QCA8337        |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    | | MAC0 |---| GE Phy |-+--- MDI ---+ | Phy4   |---| MAC5 | |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    |                       |           |_______________________|
> *    |                       |            _______________________
> *    |                       |           |        QCA8081        |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    |_______________________|           |_______________________|
> *
> * =================================================================
> 
> The other use case is when an external switch or PHY, if any, is connected to the IPQ5018 UNIPHY over SGMII(+), freeing up the GE PHY which can optionally be connected to an RJ45 connector. I haven't worked on such board yet where the GE PHY is directly connected to RJ45, but I believe the Linksys MX6200 has this architecture (which I'll look into soon).
> 
> * =================================================================
> *     _______________________             ____________
> *    |        IPQ5018        |           |            |
> *    | +------+   +--------+ |           | +--------+ |
> *    | | MAC0 |---| GE Phy |-+--- MDI ---+ | RJ45   | +
> *    | +------+   +--------+ |           | +--------+ |
> *    |                       |           |____________|
> *    |                       |            _______________________
> *    |                       |           |      QCA8081 Phy      |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
> *    | +------+   +--------+ |           | +--------+   +------+ |
> *    |_______________________|           |_______________________|
> *
> * =================================================================

So - with keeping in mind that I'm not a big networking guy - can we test
for whether there's an ethernet-switch present under the MDIO host and
decide based on that?

Konrad

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[George Moussalem]

On 5/27/25 17:00, Konrad Dybcio wrote:
> On 5/27/25 2:13 PM, George Moussalem wrote:
>>
>>
>> On 5/27/25 15:31, Konrad Dybcio wrote:
>>> On 5/27/25 1:28 PM, George Moussalem wrote:
>>>> Hi Konrad,
>>>>
>>>> On 5/27/25 14:59, Konrad Dybcio wrote:
>>>>> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote:
>>>>>> On 26/05/2025 08:43, George Moussalem wrote:
>>>>>>>>> +  qca,dac:
>>>>>>>>> +    description:
>>>>>>>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>>>>>>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>>>>>>>> +      link architecture to accommodate for short cable length.
>>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>>>>> +    items:
>>>>>>>>> +      - items:
>>>>>>>>> +          - description: value for MDAC. Expected 0x10, if set
>>>>>>>>> +          - description: value for EDAC. Expected 0x10, if set
>>>>>>>>
>>>>>>>> If this is fixed to 0x10, then this is fully deducible from compatible.
>>>>>>>> Drop entire property.
>>>>>>>
>>>>>>> as mentioned to Andrew, I can move the required values to the driver
>>>>>>> itself, but a property would still be required to indicate that this PHY
>>>>>>> is connected to an external PHY (ex. qca8337 switch). In that case, the
>>>>>>> values need to be set. Otherwise, not..
>>>>>>>
>>>>>>> Would qcom,phy-to-phy-dac (boolean) do?
>>>>>>
>>>>>> Seems fine to me.
>>>>>
>>>>> Can the driver instead check for a phy reference?
>>>>
>>>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.
>>>
>>> I'm not sure how this is all wired up. Do you have an example of a DT
>>> with both configurations you described in your reply to Andrew?
>>
>> Sure, for IPQ5018 GE PHY connected to a QCA8337 switch (phy to phy):
>> Link: https://github.com/openwrt/openwrt/blob/main/target/linux/qualcommax/files/arch/arm64/boot/dts/qcom/ipq5018-spnmx56.dts
>> In this scenario, the IPQ5018 single UNIPHY is freed up and can be used with an external PHY such as QCA8081 to offer up to 2.5 gbps connectivity, see diagram below:
>>
>> * =================================================================
>> *     _______________________             _______________________
>> *    |        IPQ5018        |           |        QCA8337        |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    | | MAC0 |---| GE Phy |-+--- MDI ---+ | Phy4   |---| MAC5 | |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    |                       |           |_______________________|
>> *    |                       |            _______________________
>> *    |                       |           |        QCA8081        |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    |_______________________|           |_______________________|
>> *
>> * =================================================================
>>
>> The other use case is when an external switch or PHY, if any, is connected to the IPQ5018 UNIPHY over SGMII(+), freeing up the GE PHY which can optionally be connected to an RJ45 connector. I haven't worked on such board yet where the GE PHY is directly connected to RJ45, but I believe the Linksys MX6200 has this architecture (which I'll look into soon).
>>
>> * =================================================================
>> *     _______________________             ____________
>> *    |        IPQ5018        |           |            |
>> *    | +------+   +--------+ |           | +--------+ |
>> *    | | MAC0 |---| GE Phy |-+--- MDI ---+ | RJ45   | +
>> *    | +------+   +--------+ |           | +--------+ |
>> *    |                       |           |____________|
>> *    |                       |            _______________________
>> *    |                       |           |      QCA8081 Phy      |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy    |---| RJ45 | |
>> *    | +------+   +--------+ |           | +--------+   +------+ |
>> *    |_______________________|           |_______________________|
>> *
>> * =================================================================
> 
> So - with keeping in mind that I'm not a big networking guy - can we test
> for whether there's an ethernet-switch present under the MDIO host and
> decide based on that?

AFAIK and unless I stand corrected by others, we can detect the presence 
of a phy or switch, but we can't detect how it's wired up. It could be 
connected to the GE PHY or the UNIPHY. Hence, the need for a property.
> 
> Konrad

George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Andrew Lunn]

> > > > > > Would qcom,phy-to-phy-dac (boolean) do?
> > > > > 
> > > > > Seems fine to me.
> > > > 
> > > > Can the driver instead check for a phy reference?
> > > 
> > > Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.
> > 
> > I'm not sure how this is all wired up. Do you have an example of a DT
> > with both configurations you described in your reply to Andrew?

When a SoC interface is connected to a switch, the SoC interface has
no real knowledge it is actually connected to a switch. All it knows
is it has a link peer, and it does not know if that peer is 1cm away
or 100m. It does nothing different.

The switch has a phandle to the SoC interface, so it knows a bit more,
but it would be a bit around about for the SoC interface to search the
whole device tree to see if there is a switch with a phandle pointing
to it. So for me, a bool property indicating a short 'cable' is the
better solution.

	Andrew

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Konrad Dybcio]

On 5/27/25 3:08 PM, Andrew Lunn wrote:
>>>>>>> Would qcom,phy-to-phy-dac (boolean) do?
>>>>>>
>>>>>> Seems fine to me.
>>>>>
>>>>> Can the driver instead check for a phy reference?
>>>>
>>>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2.
>>>
>>> I'm not sure how this is all wired up. Do you have an example of a DT
>>> with both configurations you described in your reply to Andrew?
> 
> When a SoC interface is connected to a switch, the SoC interface has
> no real knowledge it is actually connected to a switch. All it knows
> is it has a link peer, and it does not know if that peer is 1cm away
> or 100m. It does nothing different.
> 
> The switch has a phandle to the SoC interface, so it knows a bit more,
> but it would be a bit around about for the SoC interface to search the
> whole device tree to see if there is a switch with a phandle pointing
> to it. So for me, a bool property indicating a short 'cable' is the
> better solution.

OK

does this sound like a generic enough problem to contemplate something
common, or should we go with something like qcom,dac-preset-short-cable

Konrad

Re: [PATCH 5/5] arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus

[Konrad Dybcio]

On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> The IPQ5018 SoC contains an internal GE PHY, always at phy address 7.
> As such, let's add the GE PHY node to the SoC dtsi.
> 
> In addition, the GE PHY outputs both the RX and TX clocks to the GCC
> which gate controls them and routes them back to the PHY itself.
> So let's create two DT fixed clocks and register them in the GCC node.
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 03ebc3e305b267c98a034c41ce47a39269afce75..ff2de44f9b85993fb2d426f85676f7d54c5cf637 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -16,6 +16,18 @@ / {
>  	#size-cells = <2>;
>  
>  	clocks {
> +		gephy_rx_clk: gephy-rx-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <125000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		gephy_tx_clk: gephy-tx-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <125000000>;
> +			#clock-cells = <0>;
> +		};
> +
>  		sleep_clk: sleep-clk {
>  			compatible = "fixed-clock";
>  			#clock-cells = <0>;
> @@ -192,6 +204,17 @@ mdio0: mdio@88000 {
>  			clock-names = "gcc_mdio_ahb_clk";
>  
>  			status = "disabled";
> +
> +			ge_phy: ethernet-phy@7 {

drop the label unless it needs to be passed somewhere

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 5/5] arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus

[George Moussalem]

On 5/27/25 17:34, Konrad Dybcio wrote:
> On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@outlook.com>
>>
>> The IPQ5018 SoC contains an internal GE PHY, always at phy address 7.
>> As such, let's add the GE PHY node to the SoC dtsi.
>>
>> In addition, the GE PHY outputs both the RX and TX clocks to the GCC
>> which gate controls them and routes them back to the PHY itself.
>> So let's create two DT fixed clocks and register them in the GCC node.
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> index 03ebc3e305b267c98a034c41ce47a39269afce75..ff2de44f9b85993fb2d426f85676f7d54c5cf637 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> @@ -16,6 +16,18 @@ / {
>>   	#size-cells = <2>;
>>   
>>   	clocks {
>> +		gephy_rx_clk: gephy-rx-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <125000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		gephy_tx_clk: gephy-tx-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <125000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>>   		sleep_clk: sleep-clk {
>>   			compatible = "fixed-clock";
>>   			#clock-cells = <0>;
>> @@ -192,6 +204,17 @@ mdio0: mdio@88000 {
>>   			clock-names = "gcc_mdio_ahb_clk";
>>   
>>   			status = "disabled";
>> +
>> +			ge_phy: ethernet-phy@7 {
> 
> drop the label unless it needs to be passed somewhere

it is needed for boards where the qcom,dac-preset-short-cable property 
needs to be set. Thanks for the quick review!

> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Konrad

George

Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Andrew Lunn]

> does this sound like a generic enough problem to contemplate something
> common, or should we go with something like qcom,dac-preset-short-cable

I've seen a few other boards with back to back PHYs like this, and
they did not need any properties.

It could be this PHY does not conform to the standard, does not have
the needed dynamic range, and is getting saturated.

What we do have is:

  tx-amplitude-100base-tx-percent:
    description:
      Transmit amplitude gain applied for 100BASE-TX. 100% matches 2V
      peak-to-peak specified in ANSI X3.263. When omitted, the PHYs default
      will be left as is.

This is intended for actually boosting the amplitude, to deal with
losses between the PHY and the RJ-45 connector. So this is the
opposite.

The description of what the magic value does on this PHY suggests it
does more, and it cannot be represented as a percent. So i think a
vendor property is O.K.

   Andrew

Re: [PATCH 0/5] Add support for the IPQ5018 Internal GE PHY

[Rob Herring (Arm)]

On Sun, 25 May 2025 21:56:03 +0400, George Moussalem wrote:
> The IPQ5018 SoC contains an internal Gigabit Ethernet PHY with its
> output pins that provide an MDI interface to either an external switch
> in a PHY to PHY link architecture or directly to an attached RJ45
> connector.
> 
> The PHY supports 10/100/1000 mbps link modes, CDT, auto-negotiation and
> 802.3az EEE.
> 
> The LDO controller found in the IPQ5018 SoC needs to be enabled to drive
> power to the CMN Ethernet Block (CMN BLK) which the GE PHY depends on.
> The LDO must be enabled in TCSR by writing to a specific register.
> 
> In a phy to phy architecture, DAC values need to be set to accommodate
> for the short cable length.
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
> George Moussalem (5):
>       dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support
>       clk: qcom: gcc-ipq5018: fix GE PHY reset
>       net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support
>       arm64: dts: qcom: ipq5018: add MDIO buses
>       arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus
> 
>  .../devicetree/bindings/net/qca,ar803x.yaml        |  23 +++
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi              |  51 ++++-
>  drivers/clk/qcom/gcc-ipq5018.c                     |   2 +-
>  drivers/net/phy/qcom/Kconfig                       |   2 +-
>  drivers/net/phy/qcom/at803x.c                      | 221 ++++++++++++++++++++-
>  5 files changed, 287 insertions(+), 12 deletions(-)
> ---
> base-commit: ebfff09f63e3efb6b75b0328b3536d3ce0e26565
> change-id: 20250430-ipq5018-ge-phy-db654afa4ced
> 
> Best regards,
> --
> George Moussalem <george.moussalem@outlook.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


This patch series was applied (using b4) to base:
 Base: base-commit ebfff09f63e3efb6b75b0328b3536d3ce0e26565 not known, ignoring
 Base: attempting to guess base-commit...
 Base: remotes/arm-soc/qcom/dt64-11-g43fefd6c7129 (exact match)

If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)

New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250525-ipq5018-ge-phy-v1-0-ddab8854e253@outlook.com:

arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
arch/arm64/boot/dts/qcom/ipq5018-tplink-archer-ax55-v1.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#

Re: [PATCH 0/5] Add support for the IPQ5018 Internal GE PHY

[George Moussalem]

Hi Rob,

On 5/27/25 21:56, Rob Herring (Arm) wrote:
> 
> On Sun, 25 May 2025 21:56:03 +0400, George Moussalem wrote:
>> The IPQ5018 SoC contains an internal Gigabit Ethernet PHY with its
>> output pins that provide an MDI interface to either an external switch
>> in a PHY to PHY link architecture or directly to an attached RJ45
>> connector.
>>
>> The PHY supports 10/100/1000 mbps link modes, CDT, auto-negotiation and
>> 802.3az EEE.
>>
>> The LDO controller found in the IPQ5018 SoC needs to be enabled to drive
>> power to the CMN Ethernet Block (CMN BLK) which the GE PHY depends on.
>> The LDO must be enabled in TCSR by writing to a specific register.
>>
>> In a phy to phy architecture, DAC values need to be set to accommodate
>> for the short cable length.
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>> George Moussalem (5):
>>        dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support
>>        clk: qcom: gcc-ipq5018: fix GE PHY reset
>>        net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support
>>        arm64: dts: qcom: ipq5018: add MDIO buses
>>        arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus
>>
>>   .../devicetree/bindings/net/qca,ar803x.yaml        |  23 +++
>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi              |  51 ++++-
>>   drivers/clk/qcom/gcc-ipq5018.c                     |   2 +-
>>   drivers/net/phy/qcom/Kconfig                       |   2 +-
>>   drivers/net/phy/qcom/at803x.c                      | 221 ++++++++++++++++++++-
>>   5 files changed, 287 insertions(+), 12 deletions(-)
>> ---
>> base-commit: ebfff09f63e3efb6b75b0328b3536d3ce0e26565
>> change-id: 20250430-ipq5018-ge-phy-db654afa4ced
>>
>> Best regards,
>> --
>> George Moussalem <george.moussalem@outlook.com>
>>
>>
>>
> 
> 
> My bot found new DTB warnings on the .dts files added or changed in this
> series.
> 
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
> 
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
> 
>    pip3 install dtschema --upgrade
> 
> 
> This patch series was applied (using b4) to base:
>   Base: base-commit ebfff09f63e3efb6b75b0328b3536d3ce0e26565 not known, ignoring
>   Base: attempting to guess base-commit...
>   Base: remotes/arm-soc/qcom/dt64-11-g43fefd6c7129 (exact match)
> 
> If this is not the correct base, please add 'base-commit' tag
> (or use b4 which does this automatically)
> 
> New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250525-ipq5018-ge-phy-v1-0-ddab8854e253@outlook.com:
> 
> arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
> 	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
> arch/arm64/boot/dts/qcom/ipq5018-tplink-archer-ax55-v1.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
> 	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
> 

These pop up as the phy needs to enable 2 clocks (RX and TX) during 
probe which conflicts with the restriction set in ethernet-phy.yaml 
which says:

   clocks:
     maxItems: 1

Would you like me to add a condition in qca,ar803x.yaml on the 
compatible (PHY ID) to override it and set it to two?

Likewise on resets, right now we I've got 1 reset (a bitmask that 
actually triggers 4 resets) to conform to the bindings. If, as per 
ongoing discussion, I need to list all resets, it will also conflict 
with the restriction on resets of max 1 item.

> 
> 
> 
> 

Thanks,
Georg

Re: [PATCH 0/5] Add support for the IPQ5018 Internal GE PHY

[Andrew Lunn]

> > arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
> > 	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
> > arch/arm64/boot/dts/qcom/ipq5018-tplink-archer-ax55-v1.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long
> > 	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
> > 
> 
> These pop up as the phy needs to enable 2 clocks (RX and TX) during probe
> which conflicts with the restriction set in ethernet-phy.yaml which says:
> 
>   clocks:
>     maxItems: 1
> 
> Would you like me to add a condition in qca,ar803x.yaml on the compatible
> (PHY ID) to override it and set it to two?
> 
> Likewise on resets, right now we I've got 1 reset (a bitmask that actually
> triggers 4 resets) to conform to the bindings. If, as per ongoing
> discussion, I need to list all resets, it will also conflict with the
> restriction on resets of max 1 item.

You should describe the hardware. If the hardware has more than one
reset or clock, describe them all, and please fixup the binding to
suit.

	Andrew