Support PHY reset function and MDIO clock frequency configuration.
Signed-off-by: Luo Jie <[email protected]>
---
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
mdio_ipq driver supports more SOCs such as ipq40xx, ipq807x,
ipq60xx and ipq50xx.
Signed-off-by: Luo Jie <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
>> ---
>> 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.
>
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.