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

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.