2021-08-08 07:23:26

by Luo Jie

[permalink] [raw]
Subject: [PATCH v1 0/2] net: mdio: Add IPQ MDIO reset related function

This patch series add the MDIO reset features, which includes
configuring MDIO clock source frequency and indicating CMN_PLL that
ethernet LDO has been ready, this ethernet LDO is dedicated in the
IPQ5018 platform.

Specify more chipset IPQ40xx, IPQ807x, IPQ60xx and IPQ50xx supported by
this MDIO driver.

The PHY reset with GPIO and PHY reset with reset controller are covered
by the phylib code, so remove the PHY reset related code from the
initial patch series.

Luo Jie (2):
net: mdio: Add the reset function for IPQ MDIO driver
MDIO: Kconfig: Specify more IPQ chipset supported

drivers/net/mdio/Kconfig | 3 ++-
drivers/net/mdio/mdio-ipq4019.c | 48 +++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


2021-08-08 07:23:48

by Luo Jie

[permalink] [raw]
Subject: [PATCH v1 1/2] net: mdio: Add the reset function for IPQ MDIO driver

1. configure the MDIO clock source frequency.
2. the LDO resource is needed to indicate the ethernet LDO ready for CMN_PLL.

Signed-off-by: Luo Jie <[email protected]>
---
drivers/net/mdio/Kconfig | 1 +
drivers/net/mdio/mdio-ipq4019.c | 47 +++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 99a6c13a11af..a94d34cc7dc1 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 COMMON_CLK
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..b365f13c92ca 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -11,6 +11,7 @@
#include <linux/of_mdio.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/clk.h>

#define MDIO_MODE_REG 0x40
#define MDIO_ADDR_REG 0x44
@@ -31,8 +32,15 @@
#define IPQ4019_MDIO_TIMEOUT 10000
#define IPQ4019_MDIO_SLEEP 10

+/* MDIO clock source frequency is fixed to 100M */
+#define IPQ_MDIO_CLK_RATE 100000000
+
+#define IPQ_PHY_SET_DELAY_US 100000
+
struct ipq4019_mdio_data {
void __iomem *membase;
+ void __iomem *eth_ldo_rdy;
+ struct clk *mdio_clk;
};

static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
@@ -171,10 +179,41 @@ 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;
+ u32 val;
+ int ret;
+
+ /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
+ * is specified in the device tree.
+ * */
+ if (!IS_ERR(priv->eth_ldo_rdy)) {
+ val = readl(priv->eth_ldo_rdy);
+ val |= BIT(0);
+ writel(val, priv->eth_ldo_rdy);
+ fsleep(IPQ_PHY_SET_DELAY_US);
+ }
+
+ /* Configure MDIO clock source frequency if clock is specified in the device tree */
+ if (!IS_ERR_OR_NULL(priv->mdio_clk)) {
+ ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(priv->mdio_clk);
+ if (ret)
+ return ret;
+ }
+
+ 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 +221,22 @@ 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);

+ priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (res)
+ priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+
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

2021-08-08 15:31:37

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] net: mdio: Add the reset function for IPQ MDIO driver

> +static int ipq_mdio_reset(struct mii_bus *bus)
> +{
> + struct ipq4019_mdio_data *priv = bus->priv;
> + u32 val;
> + int ret;
> +
> + /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
> + * is specified in the device tree.
> + * */
> + if (!IS_ERR(priv->eth_ldo_rdy)) {
> + val = readl(priv->eth_ldo_rdy);
> + val |= BIT(0);
> + writel(val, priv->eth_ldo_rdy);
> + fsleep(IPQ_PHY_SET_DELAY_US);
> + }
> +
> + /* Configure MDIO clock source frequency if clock is specified in the device tree */
> + if (!IS_ERR_OR_NULL(priv->mdio_clk)) {
> + ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(priv->mdio_clk);
> + if (ret)
> + return ret;
> + }

These !IS_ERR() are pretty ugly. So

> @@ -182,14 +221,22 @@ 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);
>
> + priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");

If this returns an error, it is a real error. You should not ignore
it. Fail the probe returning the error. That then means when the reset
function is called priv->mdio_clk contains either a clock, or NULL,
which the clk API is happy to take. No need for an if.


> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (res)
> + priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);

platform_get_resource() returns a pointer or NULL. There is no error
code. So

> + if (!IS_ERR(priv->eth_ldo_rdy)) {

is actually wrong, should simply become

> + if (priv->eth_ldo_rdy) {

Andrew

2021-08-08 15:32:43

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] net: mdio: Add the reset function for IPQ MDIO driver

> + /* Configure MDIO clock source frequency if clock is specified in the device tree */
> + if (!IS_ERR_OR_NULL(priv->mdio_clk)) {

Please document the clock in the binding. Make it clear which devices
require the clock, and which don't.

Andrew

2021-08-09 14:14:35

by Luo Jie

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] net: mdio: Add the reset function for IPQ MDIO driver


On 8/8/2021 11:27 PM, Andrew Lunn wrote:
>> +static int ipq_mdio_reset(struct mii_bus *bus)
>> +{
>> + struct ipq4019_mdio_data *priv = bus->priv;
>> + u32 val;
>> + int ret;
>> +
>> + /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
>> + * is specified in the device tree.
>> + * */
>> + if (!IS_ERR(priv->eth_ldo_rdy)) {
>> + val = readl(priv->eth_ldo_rdy);
>> + val |= BIT(0);
>> + writel(val, priv->eth_ldo_rdy);
>> + fsleep(IPQ_PHY_SET_DELAY_US);
>> + }
>> +
>> + /* Configure MDIO clock source frequency if clock is specified in the device tree */
>> + if (!IS_ERR_OR_NULL(priv->mdio_clk)) {
>> + ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
>> + if (ret)
>> + return ret;
>> +
>> + ret = clk_prepare_enable(priv->mdio_clk);
>> + if (ret)
>> + return ret;
>> + }
> These !IS_ERR() are pretty ugly. So
>
>> @@ -182,14 +221,22 @@ 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);
>>
>> + priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
> If this returns an error, it is a real error. You should not ignore
> it. Fail the probe returning the error. That then means when the reset
> function is called priv->mdio_clk contains either a clock, or NULL,
> which the clk API is happy to take. No need for an if.
>
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (res)
>> + priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
> platform_get_resource() returns a pointer or NULL. There is no error
> code. So
>
>> + if (!IS_ERR(priv->eth_ldo_rdy)) {
> is actually wrong, should simply become
>
>> + if (priv->eth_ldo_rdy) {
> Andrew

Hi Andrew,

Thanks for the kindly review and the comments, will follow it in the
next patch set.

2021-08-09 14:23:17

by Luo Jie

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] net: mdio: Add the reset function for IPQ MDIO driver


On 8/8/2021 11:29 PM, Andrew Lunn wrote:
>> + /* Configure MDIO clock source frequency if clock is specified in the device tree */
>> + if (!IS_ERR_OR_NULL(priv->mdio_clk)) {
> Please document the clock in the binding. Make it clear which devices
> require the clock, and which don't.
>
> Andrew
sure, will document it in the next patch set, thanks Andrew.