[PATCH 1/3] net: mdio-ipq4019: Add mdio reset function

Subsystems: ethernet phy library, networking drivers, the rest

STALE1869d

14 messages, 4 authors, 2021-08-04 · open the first message on its own page

[PATCH 1/3] net: mdio-ipq4019: Add mdio reset function

From: Luo Jie <hidden>
Date: 2021-07-29 12:55:12

Support PHY reset function and MDIO clock frequency configuration.

Signed-off-by: Luo Jie <redacted>
---
 drivers/net/mdio/Kconfig        |  1 +
 drivers/net/mdio/mdio-ipq4019.c | 71 +++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 99a6c13a11af..06a605ffb950 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -169,6 +169,7 @@ config MDIO_OCTEON
 config MDIO_IPQ4019
 	tristate "Qualcomm IPQ4019 MDIO interface support"
 	depends on HAS_IOMEM && OF_MDIO
+	depends on GPIOLIB && COMMON_CLK && RESET_CONTROLLER
 	help
 	  This driver supports the MDIO interface found in Qualcomm
 	  IPQ40xx series Soc-s.
diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 9cd71d896963..01f5b9393537 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -11,6 +11,9 @@
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/reset.h>
+#include <linux/clk.h>
 
 #define MDIO_MODE_REG				0x40
 #define MDIO_ADDR_REG				0x44
@@ -31,8 +34,16 @@
 #define IPQ4019_MDIO_TIMEOUT	10000
 #define IPQ4019_MDIO_SLEEP		10
 
+/* MDIO clock source frequency is fixed to 100M */
+#define QCA_MDIO_CLK_RATE	100000000
+
+#define QCA_PHY_SET_DELAY_US	100000
+
 struct ipq4019_mdio_data {
 	void __iomem	*membase;
+	void __iomem *eth_ldo_rdy;
+	struct reset_control *reset_ctrl;
+	struct clk *mdio_clk;
 };
 
 static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
@@ -171,10 +182,61 @@ static int ipq4019_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	return 0;
 }
 
+static int ipq_mdio_reset(struct mii_bus *bus)
+{
+	struct ipq4019_mdio_data *priv = bus->priv;
+	struct device *dev = bus->parent;
+	struct gpio_desc *reset_gpio;
+	u32 val;
+	int i, ret;
+
+	/* To indicate CMN_PLL that ethernet_ldo has been ready if needed */
+	if (!IS_ERR(priv->eth_ldo_rdy)) {
+		val = readl(priv->eth_ldo_rdy);
+		val |= BIT(0);
+		writel(val, priv->eth_ldo_rdy);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
+
+	/* Reset GEPHY if need */
+	if (!IS_ERR(priv->reset_ctrl)) {
+		reset_control_assert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		reset_control_deassert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
+
+	/* Configure MDIO clock frequency */
+	if (!IS_ERR(priv->mdio_clk)) {
+		ret = clk_set_rate(priv->mdio_clk, QCA_MDIO_CLK_RATE);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(priv->mdio_clk);
+		if (ret)
+			return ret;
+	}
+
+	/* Reset PHYs by gpio pins */
+	for (i = 0; i < gpiod_count(dev, "phy-reset"); i++) {
+		reset_gpio = gpiod_get_index_optional(dev, "phy-reset", i, GPIOD_OUT_HIGH);
+		if (IS_ERR(reset_gpio))
+			continue;
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_set_value_cansleep(reset_gpio, 1);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_put(reset_gpio);
+	}
+
+	return 0;
+}
+
 static int ipq4019_mdio_probe(struct platform_device *pdev)
 {
 	struct ipq4019_mdio_data *priv;
 	struct mii_bus *bus;
+	struct resource *res;
 	int ret;
 
 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
@@ -182,14 +244,23 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = bus->priv;
+	priv->eth_ldo_rdy = IOMEM_ERR_PTR(-EINVAL);
 
 	priv->membase = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->membase))
 		return PTR_ERR(priv->membase);
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res)
+		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+
+	priv->reset_ctrl = devm_reset_control_get_exclusive(&pdev->dev, "gephy_mdc_rst");
+	priv->mdio_clk = devm_clk_get(&pdev->dev, "gcc_mdio_ahb_clk");
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read;
 	bus->write = ipq4019_mdio_write;
+	bus->reset = ipq_mdio_reset;
 	bus->parent = &pdev->dev;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s%d", pdev->name, pdev->id);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Luo Jie <hidden>
Date: 2021-07-29 12:54:56

rename ipq4019-mdio.yaml to ipq-mdio.yaml for supporting more
ipq boards such as ipq40xx, ipq807x, ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 ...m,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} | 32 ++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/net/{qcom,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} (58%)
diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
similarity index 58%
rename from Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
rename to Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
index 0c973310ada0..5bdeb461523b 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/net/qcom,ipq4019-mdio.yaml#
+$id: http://devicetree.org/schemas/net/qcom,ipq-mdio.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Qualcomm IPQ40xx MDIO Controller Device Tree Bindings
+title: Qualcomm IPQ MDIO Controller Device Tree Bindings
 
 maintainers:
   - Robert Marko <robert.marko@sartura.hr>
@@ -14,7 +14,9 @@ allOf:
 
 properties:
   compatible:
-    const: qcom,ipq4019-mdio
+    oneOf:
+      - const: qcom,ipq4019-mdio
+      - const: qcom,ipq-mdio
 
   "#address-cells":
     const: 1
@@ -23,7 +25,29 @@ properties:
     const: 0
 
   reg:
-    maxItems: 1
+    maxItems: 2
+
+  clocks:
+    items:
+      - description: MDIO clock
+
+  clock-names:
+    items:
+      - const: gcc_mdio_ahb_clk
+
+  resets:
+    items:
+      - description: MDIO reset & GEPHY hardware reset
+
+  reset-names:
+    items:
+      - const: gephy_mdc_rst
+
+  phy-reset-gpios:
+    maxItems: 3
+    description:
+      The phandle and specifier for the GPIO that controls the RESET
+      lines of PHY devices on that MDIO bus.
 
 required:
   - compatible
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq

From: Luo Jie <hidden>
Date: 2021-07-29 12:55:01

mdio_ipq driver supports more SOCs such as ipq40xx, ipq807x,
ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 drivers/net/mdio/Kconfig                      |  6 +-
 drivers/net/mdio/Makefile                     |  2 +-
 .../net/mdio/{mdio-ipq4019.c => mdio-ipq.c}   | 66 +++++++++----------
 3 files changed, 37 insertions(+), 37 deletions(-)
 rename drivers/net/mdio/{mdio-ipq4019.c => mdio-ipq.c} (81%)
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 06a605ffb950..133c3d9cb083 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -166,13 +166,13 @@ config MDIO_OCTEON
 	  buses. It is required by the Octeon and ThunderX ethernet device
 	  drivers on some systems.
 
-config MDIO_IPQ4019
-	tristate "Qualcomm IPQ4019 MDIO interface support"
+config MDIO_IPQ
+	tristate "Qualcomm IPQ MDIO interface support"
 	depends on HAS_IOMEM && OF_MDIO
 	depends on GPIOLIB && COMMON_CLK && RESET_CONTROLLER
 	help
 	  This driver supports the MDIO interface found in Qualcomm
-	  IPQ40xx series Soc-s.
+	  IPQ40xx, IPQ60XX, IPQ807X and IPQ50XX series Soc-s.
 
 config MDIO_IPQ8064
 	tristate "Qualcomm IPQ8064 MDIO interface support"
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 15f8dc4042ce..df7afc8462de 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -13,7 +13,7 @@ obj-$(CONFIG_MDIO_CAVIUM)		+= mdio-cavium.o
 obj-$(CONFIG_MDIO_GPIO)			+= mdio-gpio.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)		+= mdio-hisi-femac.o
 obj-$(CONFIG_MDIO_I2C)			+= mdio-i2c.o
-obj-$(CONFIG_MDIO_IPQ4019)		+= mdio-ipq4019.o
+obj-$(CONFIG_MDIO_IPQ)			+= mdio-ipq.o
 obj-$(CONFIG_MDIO_IPQ8064)		+= mdio-ipq8064.o
 obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq.c
similarity index 81%
rename from drivers/net/mdio/mdio-ipq4019.c
rename to drivers/net/mdio/mdio-ipq.c
index 01f5b9393537..70e1ae05a64f 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq.c
@@ -31,38 +31,38 @@
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)
 
-#define IPQ4019_MDIO_TIMEOUT	10000
-#define IPQ4019_MDIO_SLEEP		10
+#define IPQ_MDIO_TIMEOUT	10000
+#define IPQ_MDIO_SLEEP		10
 
 /* MDIO clock source frequency is fixed to 100M */
 #define QCA_MDIO_CLK_RATE	100000000
 
 #define QCA_PHY_SET_DELAY_US	100000
 
-struct ipq4019_mdio_data {
+struct ipq_mdio_data {
 	void __iomem	*membase;
 	void __iomem *eth_ldo_rdy;
 	struct reset_control *reset_ctrl;
 	struct clk *mdio_clk;
 };
 
-static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
+static int ipq_mdio_wait_busy(struct mii_bus *bus)
 {
-	struct ipq4019_mdio_data *priv = bus->priv;
+	struct ipq_mdio_data *priv = bus->priv;
 	unsigned int busy;
 
 	return readl_poll_timeout(priv->membase + MDIO_CMD_REG, busy,
 				  (busy & MDIO_CMD_ACCESS_BUSY) == 0,
-				  IPQ4019_MDIO_SLEEP, IPQ4019_MDIO_TIMEOUT);
+				  IPQ_MDIO_SLEEP, IPQ_MDIO_TIMEOUT);
 }
 
-static int ipq4019_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+static int ipq_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
-	struct ipq4019_mdio_data *priv = bus->priv;
+	struct ipq_mdio_data *priv = bus->priv;
 	unsigned int data;
 	unsigned int cmd;
 
-	if (ipq4019_mdio_wait_busy(bus))
+	if (ipq_mdio_wait_busy(bus))
 		return -ETIMEDOUT;
 
 	/* Clause 45 support */
@@ -102,7 +102,7 @@ static int ipq4019_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	writel(cmd, priv->membase + MDIO_CMD_REG);
 
 	/* Wait read complete */
-	if (ipq4019_mdio_wait_busy(bus))
+	if (ipq_mdio_wait_busy(bus))
 		return -ETIMEDOUT;
 
 	if (regnum & MII_ADDR_C45) {
@@ -110,7 +110,7 @@ static int ipq4019_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 
 		writel(cmd, priv->membase + MDIO_CMD_REG);
 
-		if (ipq4019_mdio_wait_busy(bus))
+		if (ipq_mdio_wait_busy(bus))
 			return -ETIMEDOUT;
 	}
 
@@ -118,14 +118,13 @@ static int ipq4019_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	return readl(priv->membase + MDIO_DATA_READ_REG);
 }
 
-static int ipq4019_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
-							 u16 value)
+static int ipq_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value)
 {
-	struct ipq4019_mdio_data *priv = bus->priv;
+	struct ipq_mdio_data *priv = bus->priv;
 	unsigned int data;
 	unsigned int cmd;
 
-	if (ipq4019_mdio_wait_busy(bus))
+	if (ipq_mdio_wait_busy(bus))
 		return -ETIMEDOUT;
 
 	/* Clause 45 support */
@@ -150,7 +149,7 @@ static int ipq4019_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 		writel(cmd, priv->membase + MDIO_CMD_REG);
 
-		if (ipq4019_mdio_wait_busy(bus))
+		if (ipq_mdio_wait_busy(bus))
 			return -ETIMEDOUT;
 	} else {
 		/* Enter Clause 22 mode */
@@ -176,7 +175,7 @@ static int ipq4019_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	writel(cmd, priv->membase + MDIO_CMD_REG);
 
 	/* Wait write complete */
-	if (ipq4019_mdio_wait_busy(bus))
+	if (ipq_mdio_wait_busy(bus))
 		return -ETIMEDOUT;
 
 	return 0;
@@ -184,7 +183,7 @@ static int ipq4019_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 static int ipq_mdio_reset(struct mii_bus *bus)
 {
-	struct ipq4019_mdio_data *priv = bus->priv;
+	struct ipq_mdio_data *priv = bus->priv;
 	struct device *dev = bus->parent;
 	struct gpio_desc *reset_gpio;
 	u32 val;
@@ -232,9 +231,9 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	return 0;
 }
 
-static int ipq4019_mdio_probe(struct platform_device *pdev)
+static int ipq_mdio_probe(struct platform_device *pdev)
 {
-	struct ipq4019_mdio_data *priv;
+	struct ipq_mdio_data *priv;
 	struct mii_bus *bus;
 	struct resource *res;
 	int ret;
@@ -257,9 +256,9 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	priv->reset_ctrl = devm_reset_control_get_exclusive(&pdev->dev, "gephy_mdc_rst");
 	priv->mdio_clk = devm_clk_get(&pdev->dev, "gcc_mdio_ahb_clk");
 
-	bus->name = "ipq4019_mdio";
-	bus->read = ipq4019_mdio_read;
-	bus->write = ipq4019_mdio_write;
+	bus->name = "ipq_mdio";
+	bus->read = ipq_mdio_read;
+	bus->write = ipq_mdio_write;
 	bus->reset = ipq_mdio_reset;
 	bus->parent = &pdev->dev;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s%d", pdev->name, pdev->id);
@@ -275,7 +274,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int ipq4019_mdio_remove(struct platform_device *pdev)
+static int ipq_mdio_remove(struct platform_device *pdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(pdev);
 
@@ -284,23 +283,24 @@ static int ipq4019_mdio_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id ipq4019_mdio_dt_ids[] = {
+static const struct of_device_id ipq_mdio_dt_ids[] = {
 	{ .compatible = "qcom,ipq4019-mdio" },
+	{ .compatible = "qcom,ipq-mdio" },
 	{ }
 };
-MODULE_DEVICE_TABLE(of, ipq4019_mdio_dt_ids);
+MODULE_DEVICE_TABLE(of, ipq_mdio_dt_ids);
 
-static struct platform_driver ipq4019_mdio_driver = {
-	.probe = ipq4019_mdio_probe,
-	.remove = ipq4019_mdio_remove,
+static struct platform_driver ipq_mdio_driver = {
+	.probe = ipq_mdio_probe,
+	.remove = ipq_mdio_remove,
 	.driver = {
-		.name = "ipq4019-mdio",
-		.of_match_table = ipq4019_mdio_dt_ids,
+		.name = "ipq-mdio",
+		.of_match_table = ipq_mdio_dt_ids,
 	},
 };
 
-module_platform_driver(ipq4019_mdio_driver);
+module_platform_driver(ipq_mdio_driver);
 
-MODULE_DESCRIPTION("ipq4019 MDIO interface driver");
+MODULE_DESCRIPTION("ipq MDIO interface driver");
 MODULE_AUTHOR("Qualcomm Atheros");
 MODULE_LICENSE("Dual BSD/GPL");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq

From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-07-29 13:15:46

On Thu, Jul 29, 2021 at 08:53:57PM +0800, Luo Jie wrote:
mdio_ipq driver supports more SOCs such as ipq40xx, ipq807x,
ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 drivers/net/mdio/Kconfig                      |  6 +-
 drivers/net/mdio/Makefile                     |  2 +-
 .../net/mdio/{mdio-ipq4019.c => mdio-ipq.c}   | 66 +++++++++----------
 3 files changed, 37 insertions(+), 37 deletions(-)
 rename drivers/net/mdio/{mdio-ipq4019.c => mdio-ipq.c} (81%)
Hi Luo

We don't rename files unless there is a very good reason. It makes
back porting of fixes harder in stable. There are plenty of examples
of files with device specific names, but supporting a broad range of
devices. Take for example lm75, at24.
-config MDIO_IPQ4019
-	tristate "Qualcomm IPQ4019 MDIO interface support"
+config MDIO_IPQ
+	tristate "Qualcomm IPQ MDIO interface support"
 	depends on HAS_IOMEM && OF_MDIO
 	depends on GPIOLIB && COMMON_CLK && RESET_CONTROLLER
 	help
 	  This driver supports the MDIO interface found in Qualcomm
-	  IPQ40xx series Soc-s.
+	  IPQ40xx, IPQ60XX, IPQ807X and IPQ50XX series Soc-s.
Please leave the MDIO_IPQ4019 unchanged, so we don't break backwards
compatibility, but the changes to the text are O.K.
quoted hunk
@@ -31,38 +31,38 @@
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)
 
-#define IPQ4019_MDIO_TIMEOUT	10000
-#define IPQ4019_MDIO_SLEEP		10
+#define IPQ_MDIO_TIMEOUT	10000
+#define IPQ_MDIO_SLEEP		10
This sort of mass rename will also make back porting fixes
harder. Please don't do it.
-static const struct of_device_id ipq4019_mdio_dt_ids[] = {
+static const struct of_device_id ipq_mdio_dt_ids[] = {
 	{ .compatible = "qcom,ipq4019-mdio" },
+	{ .compatible = "qcom,ipq-mdio" },
 	{ }
 };
Such a generic name is not a good idea. It appears this driver is not
compatible with the IPQ8064? It is O.K. to add more specific
compatibles. So you could add

qcom,ipq40xx, qcom,ipq60xx, qcom,ipq807x and qcom,ipq50xx.

But really, there is no need. Take for example snps,dwmac-mdio, which
is used in all sorts of devices.

   Andrew

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-07-29 13:18:21

quoted hunk
@@ -23,7 +25,29 @@ properties:
     const: 0
 
   reg:
-    maxItems: 1
+    maxItems: 2
+
+  clocks:
+    items:
+      - description: MDIO clock
+
+  clock-names:
+    items:
+      - const: gcc_mdio_ahb_clk
+
+  resets:
+    items:
+      - description: MDIO reset & GEPHY hardware reset
+
+  reset-names:
+    items:
+      - const: gephy_mdc_rst
+
+  phy-reset-gpios:
+    maxItems: 3
+    description:
+      The phandle and specifier for the GPIO that controls the RESET
+      lines of PHY devices on that MDIO bus.
This is clearly not a rename. It is great you are adding missing
properties, but please do it as a separate patch.

	    Andrew

Re: [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function

From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-07-29 13:27:06

Hi Luo

For a patchset, netdev wants to see a patch 0/X which describes the
big picture. What is the patchset as a whole doing.
+static int ipq_mdio_reset(struct mii_bus *bus)
+{
+	struct ipq4019_mdio_data *priv = bus->priv;
+	struct device *dev = bus->parent;
+	struct gpio_desc *reset_gpio;
+	u32 val;
+	int i, ret;
+
+	/* To indicate CMN_PLL that ethernet_ldo has been ready if needed */
+	if (!IS_ERR(priv->eth_ldo_rdy)) {
+		val = readl(priv->eth_ldo_rdy);
+		val |= BIT(0);
+		writel(val, priv->eth_ldo_rdy);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
+
+	/* Reset GEPHY if need */
+	if (!IS_ERR(priv->reset_ctrl)) {
+		reset_control_assert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		reset_control_deassert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
What exactly is being reset here? Which is GEPHY?

The MDIO bus master driver should not be touching any Ethernet
PHYs. All it provides is a bus, nothing more.
+
+	/* Configure MDIO clock frequency */
+	if (!IS_ERR(priv->mdio_clk)) {
+		ret = clk_set_rate(priv->mdio_clk, QCA_MDIO_CLK_RATE);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(priv->mdio_clk);
+		if (ret)
+			return ret;
+	}
+
+	/* Reset PHYs by gpio pins */
+	for (i = 0; i < gpiod_count(dev, "phy-reset"); i++) {
+		reset_gpio = gpiod_get_index_optional(dev, "phy-reset", i, GPIOD_OUT_HIGH);
+		if (IS_ERR(reset_gpio))
+			continue;
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_set_value_cansleep(reset_gpio, 1);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_put(reset_gpio);
+	}
No, there is common code in phylib to do that.
quoted hunk
 static int ipq4019_mdio_probe(struct platform_device *pdev)
 {
 	struct ipq4019_mdio_data *priv;
 	struct mii_bus *bus;
+	struct resource *res;
 	int ret;
 
 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
@@ -182,14 +244,23 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = bus->priv;
+	priv->eth_ldo_rdy = IOMEM_ERR_PTR(-EINVAL);
 
 	priv->membase = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->membase))
 		return PTR_ERR(priv->membase);
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res)
+		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+
+	priv->reset_ctrl = devm_reset_control_get_exclusive(&pdev->dev, "gephy_mdc_rst");
+	priv->mdio_clk = devm_clk_get(&pdev->dev, "gcc_mdio_ahb_clk");
You probably want to use devm_clk_get_optional().

    Andrew

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Rob Herring <robh@kernel.org>
Date: 2021-07-29 13:58:08

On Thu, 29 Jul 2021 20:53:58 +0800, Luo Jie wrote:
rename ipq4019-mdio.yaml to ipq-mdio.yaml for supporting more
ipq boards such as ipq40xx, ipq807x, ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 ...m,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} | 32 ++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/net/{qcom,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} (58%)
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/qcom,ipq-mdio.example.dt.yaml: mdio@90000: reg: [[589824, 100]] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1511253

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Rob Herring <robh+dt@kernel.org>
Date: 2021-07-29 17:29:49

On Thu, Jul 29, 2021 at 6:54 AM Luo Jie [off-list ref] wrote:
quoted hunk
rename ipq4019-mdio.yaml to ipq-mdio.yaml for supporting more
ipq boards such as ipq40xx, ipq807x, ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 ...m,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} | 32 ++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/net/{qcom,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} (58%)
diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
similarity index 58%
rename from Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
rename to Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
index 0c973310ada0..5bdeb461523b 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/net/qcom,ipq4019-mdio.yaml#
+$id: http://devicetree.org/schemas/net/qcom,ipq-mdio.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#

-title: Qualcomm IPQ40xx MDIO Controller Device Tree Bindings
+title: Qualcomm IPQ MDIO Controller Device Tree Bindings

 maintainers:
   - Robert Marko <robert.marko@sartura.hr>
@@ -14,7 +14,9 @@ allOf:

 properties:
   compatible:
-    const: qcom,ipq4019-mdio
+    oneOf:
+      - const: qcom,ipq4019-mdio
+      - const: qcom,ipq-mdio
This is more than the commit log suggests. A generic compatible by
itself is not sufficient. If other chips have the same block, just use
'qcom,ipq4019-mdio'. They should also have a compatible for the new
SoC in case it's not 'the same'.

Also, use 'enum' rather than oneOf plus const.
quoted hunk
   "#address-cells":
     const: 1
@@ -23,7 +25,29 @@ properties:
     const: 0

   reg:
-    maxItems: 1
+    maxItems: 2
This breaks compatibility because now 1 entry is not valid.
+
+  clocks:
+    items:
+      - description: MDIO clock
+
+  clock-names:
+    items:
+      - const: gcc_mdio_ahb_clk
+
+  resets:
+    items:
+      - description: MDIO reset & GEPHY hardware reset
+
+  reset-names:
+    items:
+      - const: gephy_mdc_rst
These all now apply to 'qcom,ipq4019-mdio'. The h/w had no clocks or
resets and now does?

You don't need *-names when there is only 1.
+  phy-reset-gpios:
+    maxItems: 3
+    description:
+      The phandle and specifier for the GPIO that controls the RESET
+      lines of PHY devices on that MDIO bus.
This belongs in the phy node since the reset is connected to the phy.
 required:
   - compatible
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq

From: <hidden>
Date: 2021-08-02 05:56:57

On 2021-07-29 21:15, Andrew Lunn wrote:
On Thu, Jul 29, 2021 at 08:53:57PM +0800, Luo Jie wrote:
quoted
mdio_ipq driver supports more SOCs such as ipq40xx, ipq807x,
ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 drivers/net/mdio/Kconfig                      |  6 +-
 drivers/net/mdio/Makefile                     |  2 +-
 .../net/mdio/{mdio-ipq4019.c => mdio-ipq.c}   | 66 
+++++++++----------
 3 files changed, 37 insertions(+), 37 deletions(-)
 rename drivers/net/mdio/{mdio-ipq4019.c => mdio-ipq.c} (81%)
Hi Luo

We don't rename files unless there is a very good reason. It makes
back porting of fixes harder in stable. There are plenty of examples
of files with device specific names, but supporting a broad range of
devices. Take for example lm75, at24.

Hi Andrew
Thanks for the comments, will update the patch set to keep the name 
unchanged.
quoted
-config MDIO_IPQ4019
-	tristate "Qualcomm IPQ4019 MDIO interface support"
+config MDIO_IPQ
+	tristate "Qualcomm IPQ MDIO interface support"
 	depends on HAS_IOMEM && OF_MDIO
 	depends on GPIOLIB && COMMON_CLK && RESET_CONTROLLER
 	help
 	  This driver supports the MDIO interface found in Qualcomm
-	  IPQ40xx series Soc-s.
+	  IPQ40xx, IPQ60XX, IPQ807X and IPQ50XX series Soc-s.
Please leave the MDIO_IPQ4019 unchanged, so we don't break backwards
compatibility, but the changes to the text are O.K.

will correct it in the next patch set.
quoted
@@ -31,38 +31,38 @@
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)

-#define IPQ4019_MDIO_TIMEOUT	10000
-#define IPQ4019_MDIO_SLEEP		10
+#define IPQ_MDIO_TIMEOUT	10000
+#define IPQ_MDIO_SLEEP		10
This sort of mass rename will also make back porting fixes
harder. Please don't do it.

will keep it unchanged in the next patch set.
quoted
-static const struct of_device_id ipq4019_mdio_dt_ids[] = {
+static const struct of_device_id ipq_mdio_dt_ids[] = {
 	{ .compatible = "qcom,ipq4019-mdio" },
+	{ .compatible = "qcom,ipq-mdio" },
 	{ }
 };
Such a generic name is not a good idea. It appears this driver is not
compatible with the IPQ8064? It is O.K. to add more specific
compatibles. So you could add

qcom,ipq40xx, qcom,ipq60xx, qcom,ipq807x and qcom,ipq50xx.

But really, there is no need. Take for example snps,dwmac-mdio, which
is used in all sorts of devices.

   Andrew
Hi Andrew, yes, this driver is not compatible with IPQ8064, but it is 
compatible with
the new chipset such as ipq807x, ipq60xx and ipq50xx, will take your 
suggestion in
the next patch set, thanks for the comments.

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: <hidden>
Date: 2021-08-02 06:02:24

On 2021-07-29 21:17, Andrew Lunn wrote:
quoted
@@ -23,7 +25,29 @@ properties:
     const: 0

   reg:
-    maxItems: 1
+    maxItems: 2
+
+  clocks:
+    items:
+      - description: MDIO clock
+
+  clock-names:
+    items:
+      - const: gcc_mdio_ahb_clk
+
+  resets:
+    items:
+      - description: MDIO reset & GEPHY hardware reset
+
+  reset-names:
+    items:
+      - const: gephy_mdc_rst
+
+  phy-reset-gpios:
+    maxItems: 3
+    description:
+      The phandle and specifier for the GPIO that controls the RESET
+      lines of PHY devices on that MDIO bus.
This is clearly not a rename. It is great you are adding missing
properties, but please do it as a separate patch.

	    Andrew
Hi Andrew, thanks for the comments, will separate it out in the next 
patch set.

Re: [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function

From: <hidden>
Date: 2021-08-02 06:47:10

On 2021-07-29 21:26, Andrew Lunn wrote:
Hi Luo

For a patchset, netdev wants to see a patch 0/X which describes the
big picture. What is the patchset as a whole doing.

Hi Andrew,
Thanks for reminder, will provide it in the next patch set.
quoted
+static int ipq_mdio_reset(struct mii_bus *bus)
+{
+	struct ipq4019_mdio_data *priv = bus->priv;
+	struct device *dev = bus->parent;
+	struct gpio_desc *reset_gpio;
+	u32 val;
+	int i, ret;
+
+	/* To indicate CMN_PLL that ethernet_ldo has been ready if needed */
+	if (!IS_ERR(priv->eth_ldo_rdy)) {
+		val = readl(priv->eth_ldo_rdy);
+		val |= BIT(0);
+		writel(val, priv->eth_ldo_rdy);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
+
+	/* Reset GEPHY if need */
+	if (!IS_ERR(priv->reset_ctrl)) {
+		reset_control_assert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		reset_control_deassert(priv->reset_ctrl);
+		fsleep(QCA_PHY_SET_DELAY_US);
+	}
What exactly is being reset here? Which is GEPHY?

The MDIO bus master driver should not be touching any Ethernet
PHYs. All it provides is a bus, nothing more.

The GEPHY is the embedded Giga Ethernet PHY in the chipset IPQ50xx, 
there is a dedicated MDIO bus for this internal PHY.
what the reset function does here is for resetting this dedicated MDIO 
bus and this embedded PHY DSP hardware.
because this dedicated MDIO bus is only connected with this internal 
PHY on chip set IPQ50xx, so i put this
code in the MDIO reset function.
quoted
+
+	/* Configure MDIO clock frequency */
+	if (!IS_ERR(priv->mdio_clk)) {
+		ret = clk_set_rate(priv->mdio_clk, QCA_MDIO_CLK_RATE);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(priv->mdio_clk);
+		if (ret)
+			return ret;
+	}
quoted
+
+	/* Reset PHYs by gpio pins */
+	for (i = 0; i < gpiod_count(dev, "phy-reset"); i++) {
+		reset_gpio = gpiod_get_index_optional(dev, "phy-reset", i, 
GPIOD_OUT_HIGH);
+		if (IS_ERR(reset_gpio))
+			continue;
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_set_value_cansleep(reset_gpio, 1);
+		fsleep(QCA_PHY_SET_DELAY_US);
+		gpiod_put(reset_gpio);
+	}
No, there is common code in phylib to do that.

Hi Andrew,
The common code in phylib for resetting PHY by GPIO pin is high active, 
which is not suitable for the PHY reset here.
for resetting the PHY, calling gpiod_set_value_cansleep(reset_gpio, 1), 
then gpiod_set_value_cansleep(reset_gpio, 0).
but as for resetting the PHY by GPIO pin in IPQ chipset, this is the 
opposite process(low active) from the phylib code,
which needs to set the GPIO output value to 0, then to 1 for reset as 
the code above.
quoted
 static int ipq4019_mdio_probe(struct platform_device *pdev)
 {
 	struct ipq4019_mdio_data *priv;
 	struct mii_bus *bus;
+	struct resource *res;
 	int ret;

 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
@@ -182,14 +244,23 @@ static int ipq4019_mdio_probe(struct 
platform_device *pdev)
 		return -ENOMEM;

 	priv = bus->priv;
+	priv->eth_ldo_rdy = IOMEM_ERR_PTR(-EINVAL);

 	priv->membase = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->membase))
 		return PTR_ERR(priv->membase);

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res)
+		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+
+	priv->reset_ctrl = devm_reset_control_get_exclusive(&pdev->dev, 
"gephy_mdc_rst");
+	priv->mdio_clk = devm_clk_get(&pdev->dev, "gcc_mdio_ahb_clk");
You probably want to use devm_clk_get_optional().

    Andrew

thanks for the comment, will update it in the next patch set.

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: <hidden>
Date: 2021-08-02 07:20:16

On 2021-07-30 01:29, Rob Herring wrote:
On Thu, Jul 29, 2021 at 6:54 AM Luo Jie [off-list ref] wrote:
quoted
rename ipq4019-mdio.yaml to ipq-mdio.yaml for supporting more
ipq boards such as ipq40xx, ipq807x, ipq60xx and ipq50xx.

Signed-off-by: Luo Jie <redacted>
---
 ...m,ipq4019-mdio.yaml => qcom,ipq-mdio.yaml} | 32 
++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/net/{qcom,ipq4019-mdio.yaml 
=> qcom,ipq-mdio.yaml} (58%)
diff --git 
a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml 
b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
similarity index 58%
rename from 
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
rename to Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
index 0c973310ada0..5bdeb461523b 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq-mdio.yaml
@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/net/qcom,ipq4019-mdio.yaml#
+$id: http://devicetree.org/schemas/net/qcom,ipq-mdio.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#

-title: Qualcomm IPQ40xx MDIO Controller Device Tree Bindings
+title: Qualcomm IPQ MDIO Controller Device Tree Bindings

 maintainers:
   - Robert Marko <robert.marko@sartura.hr>
@@ -14,7 +14,9 @@ allOf:

 properties:
   compatible:
-    const: qcom,ipq4019-mdio
+    oneOf:
+      - const: qcom,ipq4019-mdio
+      - const: qcom,ipq-mdio
This is more than the commit log suggests. A generic compatible by
itself is not sufficient. If other chips have the same block, just use
'qcom,ipq4019-mdio'. They should also have a compatible for the new
SoC in case it's not 'the same'.

Also, use 'enum' rather than oneOf plus const.

Hi Rob
Thanks for the comments, will keep the compatible "qcom,ipq4019-mdio" 
unchanged,
and add the new compatible name by using 'enum' in the next patch set.
the commit log will be updated in the next patch set.
quoted
   "#address-cells":
     const: 1
@@ -23,7 +25,29 @@ properties:
     const: 0

   reg:
-    maxItems: 1
+    maxItems: 2
This breaks compatibility because now 1 entry is not valid.

will update it by using "minItems: 1, maxItems: 2" in the next patch 
set.
quoted
+
+  clocks:
+    items:
+      - description: MDIO clock
+
+  clock-names:
+    items:
+      - const: gcc_mdio_ahb_clk
+
+  resets:
+    items:
+      - description: MDIO reset & GEPHY hardware reset
+
+  reset-names:
+    items:
+      - const: gephy_mdc_rst
These all now apply to 'qcom,ipq4019-mdio'. The h/w had no clocks or
resets and now does?

You don't need *-names when there is only 1.

Hi Rob
thanks for the comment, clocks is for configuring the source clock of 
MDIO bus,
which is apply to ipq4019 and the new chip set such as ipq807x, ipq50xx 
and ipq60xx,
which is not configured because of uboot configuring it, kernel should 
not depends on
the configuration of uboot, so i add it.
will remove the *-name in the next patch set.
quoted
+  phy-reset-gpios:
+    maxItems: 3
+    description:
+      The phandle and specifier for the GPIO that controls the RESET
+      lines of PHY devices on that MDIO bus.
This belongs in the phy node since the reset is connected to the phy.

since the phylib code can't satisfy resetting PHY in IPQ chipset, 
phylib resets phy by
configuring GPIO output value to 1, then to 0. however the PHY reset in 
IPQ chipset need
to configuring GPIO output value to 0, then to 1 for the PHY reset, so 
i put the phy-reset-gpios here.
quoted
 required:
   - compatible
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-08-02 13:39:36

quoted
since the phylib code can't satisfy resetting PHY in IPQ chipset, phylib
resets phy by
configuring GPIO output value to 1, then to 0. however the PHY reset in
IPQ chipset need
to configuring GPIO output value to 0, then to 1 for the PHY reset, so i
put the phy-reset-gpios here.
Look at the active low DT property of a GPIO.

     Andrew

Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings

From: Jie Luo <hidden>
Date: 2021-08-04 02:37:46

On 8/2/2021 9:39 PM, Andrew Lunn wrote:
quoted
quoted
since the phylib code can't satisfy resetting PHY in IPQ chipset, phylib
resets phy by
configuring GPIO output value to 1, then to 0. however the PHY reset in
IPQ chipset need
to configuring GPIO output value to 0, then to 1 for the PHY reset, so i
put the phy-reset-gpios here.
Look at the active low DT property of a GPIO.

      Andrew
thanks Andrew, will update it in the next patch set.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help