| Message ID | 20210729125358.5227-1-luoj@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] net: mdio-ipq4019: Add mdio reset function | expand |
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 <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. >
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.
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);
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(+)