Messages in this thread | | | Date | Mon, 02 Aug 2021 14:46:37 +0800 | From | luoj@codeauro ... | Subject | 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. > >> +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.
| |