linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function
@ 2021-07-29 12:53 Luo Jie
  2021-07-29 12:53 ` [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq Luo Jie
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Luo Jie @ 2021-07-29 12:53 UTC (permalink / raw)
  To: andrew, hkallweit1, davem, kuba, p.zabel, agross,
	bjorn.andersson, robh+dt, robert.marko
  Cc: netdev, linux-kernel, linux-arm-msm, devicetree, sricharan, Luo Jie

Support PHY reset function and MDIO clock frequency configuration.

Signed-off-by: Luo Jie <luoj@codeaurora.org>
---
 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq
  2021-07-29 12:53 [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Luo Jie
@ 2021-07-29 12:53 ` Luo Jie
  2021-07-29 13:15   ` Andrew Lunn
  2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
  2021-07-29 13:26 ` [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Andrew Lunn
  2 siblings, 1 reply; 14+ messages in thread
From: Luo Jie @ 2021-07-29 12:53 UTC (permalink / raw)
  To: andrew, hkallweit1, davem, kuba, p.zabel, agross,
	bjorn.andersson, robh+dt, robert.marko
  Cc: netdev, linux-kernel, linux-arm-msm, devicetree, sricharan, Luo Jie

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

Signed-off-by: Luo Jie <luoj@codeaurora.org>
---
 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 12:53 [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Luo Jie
  2021-07-29 12:53 ` [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq Luo Jie
@ 2021-07-29 12:53 ` Luo Jie
  2021-07-29 13:17   ` Andrew Lunn
                     ` (2 more replies)
  2021-07-29 13:26 ` [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Andrew Lunn
  2 siblings, 3 replies; 14+ messages in thread
From: Luo Jie @ 2021-07-29 12:53 UTC (permalink / raw)
  To: andrew, hkallweit1, davem, kuba, p.zabel, agross,
	bjorn.andersson, robh+dt, robert.marko
  Cc: netdev, linux-kernel, linux-arm-msm, devicetree, sricharan, Luo Jie

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 <luoj@codeaurora.org>
---
 ...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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq
  2021-07-29 12:53 ` [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq Luo Jie
@ 2021-07-29 13:15   ` Andrew Lunn
  2021-08-02  5:56     ` luoj
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2021-07-29 13:15 UTC (permalink / raw)
  To: Luo Jie
  Cc: hkallweit1, davem, kuba, p.zabel, agross, bjorn.andersson,
	robh+dt, robert.marko, netdev, linux-kernel, linux-arm-msm,
	devicetree, sricharan

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 <luoj@codeaurora.org>
> ---
>  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.

> @@ -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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
@ 2021-07-29 13:17   ` Andrew Lunn
  2021-08-02  6:02     ` luoj
  2021-07-29 13:57   ` Rob Herring
  2021-07-29 17:29   ` Rob Herring
  2 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2021-07-29 13:17 UTC (permalink / raw)
  To: Luo Jie
  Cc: hkallweit1, davem, kuba, p.zabel, agross, bjorn.andersson,
	robh+dt, robert.marko, netdev, linux-kernel, linux-arm-msm,
	devicetree, sricharan

> @@ -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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function
  2021-07-29 12:53 [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Luo Jie
  2021-07-29 12:53 ` [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq Luo Jie
  2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
@ 2021-07-29 13:26 ` Andrew Lunn
  2021-08-02  6:46   ` luoj
  2 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2021-07-29 13:26 UTC (permalink / raw)
  To: Luo Jie
  Cc: hkallweit1, davem, kuba, p.zabel, agross, bjorn.andersson,
	robh+dt, robert.marko, netdev, linux-kernel, linux-arm-msm,
	devicetree, sricharan

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.

>  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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
  2021-07-29 13:17   ` Andrew Lunn
@ 2021-07-29 13:57   ` Rob Herring
  2021-07-29 17:29   ` Rob Herring
  2 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2021-07-29 13:57 UTC (permalink / raw)
  To: Luo Jie
  Cc: linux-arm-msm, kuba, p.zabel, hkallweit1, agross, andrew, netdev,
	davem, robert.marko, linux-kernel, sricharan, bjorn.andersson,
	devicetree, robh+dt

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 <luoj@codeaurora.org>
> ---
>  ...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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
  2021-07-29 13:17   ` Andrew Lunn
  2021-07-29 13:57   ` Rob Herring
@ 2021-07-29 17:29   ` Rob Herring
  2021-08-02  7:19     ` luoj
  2 siblings, 1 reply; 14+ messages in thread
From: Rob Herring @ 2021-07-29 17:29 UTC (permalink / raw)
  To: Luo Jie
  Cc: Andrew Lunn, Heiner Kallweit, David Miller, Jakub Kicinski,
	Philipp Zabel, Gross, Andy, Bjorn Andersson, Robert Marko,
	netdev, linux-kernel, linux-arm-msm, devicetree, Sricharan

On Thu, Jul 29, 2021 at 6:54 AM Luo Jie <luoj@codeaurora.org> 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 <luoj@codeaurora.org>
> ---
>  ...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.

>
>    "#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
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq
  2021-07-29 13:15   ` Andrew Lunn
@ 2021-08-02  5:56     ` luoj
  0 siblings, 0 replies; 14+ messages in thread
From: luoj @ 2021-08-02  5:56 UTC (permalink / raw)


On 2021-07-29 21:15, Andrew Lunn wrote:
> 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 <luoj@codeaurora.org>
>> ---
>>  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.
> 
>> -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.
> 
>> @@ -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.
> 
>> -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.
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 13:17   ` Andrew Lunn
@ 2021-08-02  6:02     ` luoj
  0 siblings, 0 replies; 14+ messages in thread
From: luoj @ 2021-08-02  6:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: hkallweit1, davem, kuba, p.zabel, agross, bjorn.andersson,
	robh+dt, robert.marko, netdev, linux-kernel, linux-arm-msm,
	devicetree, sricharan

On 2021-07-29 21:17, Andrew Lunn wrote:
>> @@ -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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function
  2021-07-29 13:26 ` [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Andrew Lunn
@ 2021-08-02  6:46   ` luoj
  0 siblings, 0 replies; 14+ messages in thread
From: luoj @ 2021-08-02  6:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: hkallweit1, davem, kuba, p.zabel, agross, bjorn.andersson,
	robh+dt, robert.marko, netdev, linux-kernel, linux-arm-msm,
	devicetree, sricharan

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.
> 
>> +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.
> 
>> +
>> +	/* 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.
> 
> 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.
> 
>>  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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-07-29 17:29   ` Rob Herring
@ 2021-08-02  7:19     ` luoj
  2021-08-02 13:39       ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: luoj @ 2021-08-02  7:19 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andrew Lunn, Heiner Kallweit, David Miller, Jakub Kicinski,
	Philipp Zabel, Gross, Andy, Bjorn Andersson, Robert Marko,
	netdev, linux-kernel, linux-arm-msm, devicetree, Sricharan

On 2021-07-30 01:29, Rob Herring wrote:
> On Thu, Jul 29, 2021 at 6:54 AM Luo Jie <luoj@codeaurora.org> 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 <luoj@codeaurora.org>
>> ---
>>  ...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.
>> 
>>    "#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.
> 
>> +
>> +  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.
> 
>> +  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.
> 
>> 
>>  required:
>>    - compatible
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-08-02  7:19     ` luoj
@ 2021-08-02 13:39       ` Andrew Lunn
  2021-08-04  2:37         ` Jie Luo
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2021-08-02 13:39 UTC (permalink / raw)
  To: luoj
  Cc: Rob Herring, Heiner Kallweit, David Miller, Jakub Kicinski,
	Philipp Zabel, Gross, Andy, Bjorn Andersson, Robert Marko,
	netdev, linux-kernel, linux-arm-msm, devicetree, Sricharan

> > 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings
  2021-08-02 13:39       ` Andrew Lunn
@ 2021-08-04  2:37         ` Jie Luo
  0 siblings, 0 replies; 14+ messages in thread
From: Jie Luo @ 2021-08-04  2:37 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rob Herring, Heiner Kallweit, David Miller, Jakub Kicinski,
	Philipp Zabel, Gross, Andy, Bjorn Andersson, Robert Marko,
	netdev, linux-kernel, linux-arm-msm, devicetree, Sricharan


On 8/2/2021 9:39 PM, Andrew Lunn wrote:
>>> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-08-04  2:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 12:53 [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Luo Jie
2021-07-29 12:53 ` [PATCH 2/3] net: mdio-ipq4019: rename mdio_ipq4019 to mdio_ipq Luo Jie
2021-07-29 13:15   ` Andrew Lunn
2021-08-02  5:56     ` luoj
2021-07-29 12:53 ` [PATCH 3/3] dt-bindings: net: rename Qualcomm IPQ MDIO bindings Luo Jie
2021-07-29 13:17   ` Andrew Lunn
2021-08-02  6:02     ` luoj
2021-07-29 13:57   ` Rob Herring
2021-07-29 17:29   ` Rob Herring
2021-08-02  7:19     ` luoj
2021-08-02 13:39       ` Andrew Lunn
2021-08-04  2:37         ` Jie Luo
2021-07-29 13:26 ` [PATCH 1/3] net: mdio-ipq4019: Add mdio reset function Andrew Lunn
2021-08-02  6:46   ` luoj

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).