| Message ID | KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01MB5448.apcprd01.prod.exchangelabs.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] net: mdiobus: Add a function to deassert reset | expand |
On Tue, May 09, 2023 at 06:44:02PM +0800, Yan Wang wrote: > Every PHY chip has a reset pin. Hi Yan Experience has shown that very few PHYs have controllable resets. So i would not say every. the state isn't > sure of the PHY before scanning. > > It is resetting, Scanning phy ID will fail, so > deassert reset for the chip ,normal operation. Please look at your white space in both the commit message and the patch. No space before , but after. Spaces between words etc. More blank lines are common in code to break up logical sections etc. "While in resetting, scanning of the PHY ID will fail. So deassert the reset for the chip to ensure normal operation." What you are missing is a delay afterwards. Look at the DT binding, it lists optional properties to control the delay. And if there is no delay specified, the code which will later take the GPIO inserts a delay. > > Release the reset pin, because it needs to be > registered to the created phy device. > > Signed-off-by: Yan Wang <rk.code@outlook.com> > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > index 1183ef5e203e..8fdf1293f447 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -11,6 +11,7 @@ > #include <linux/of.h> > #include <linux/phy.h> > #include <linux/pse-pd/pse.h> > +#include <linux/of_gpio.h> These includes appear to be sorted. > > MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); > MODULE_LICENSE("GPL"); > @@ -57,6 +58,32 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) > return register_mii_timestamper(arg.np, arg.args[0]); > } > > +static void fwnode_mdiobus_deassert_reset_phy(struct fwnode_handle *fwnode) > +{ > + struct device_node *np; > + int reset; > + int rc; > + > + np = to_of_node(fwnode); > + if (!np) > + return; > + reset = of_get_named_gpio(np, "reset-gpios", 0); > + if (gpio_is_valid(reset)) { > + rc = gpio_request(reset, NULL); > + if (rc < 0) { > + pr_err("The currunt state of the reset pin is %s ", > + rc == -EBUSY ? "busy" : "error"); Please correctly handle -EPROBE_DEFFER. The GPIO driver might not of probed yet. The gpio maintainers are also trying to remove the gpio_ API and replace it with gpiod_. > + } else { > + gpio_direction_output(reset, 0); > + usleep_range(1000, 2000); > + gpio_direction_output(reset, 1); This is actually putting it into reset first, and then taking it out of reset. We want to avoid that. it forces a new auto-neg cycles which takes a little over 1 second. Phylib will try to avoid forcing an auto-neg so you get link faster. If the PHY does not need to be reconfigured it won't be and the result of the auto neg can be used. Andrew
> On May 9, 2023, at 20:22, Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, May 09, 2023 at 06:44:02PM +0800, Yan Wang wrote: >> Every PHY chip has a reset pin. > > Hi Yan > > Experience has shown that very few PHYs have controllable resets. So i > would not say every. > > the state isn't >> sure of the PHY before scanning. >> >> It is resetting, Scanning phy ID will fail, so >> deassert reset for the chip ,normal operation. > > Please look at your white space in both the commit message and the > patch. No space before , but after. Spaces between words etc. More > blank lines are common in code to break up logical sections etc. > > "While in resetting, scanning of the PHY ID will fail. So deassert the > reset for the chip to ensure normal operation." > > What you are missing is a delay afterwards. Look at the DT binding, it > lists optional properties to control the delay. And if there is no > delay specified, the code which will later take the GPIO inserts a > delay. > Incorrect description caused your misunderstanding. On my customized board, multiple phy devices are mounted on the mido bus, and there are multiple pins on the hardware that control these phy devices. These pins default to low level, so these phy devices are in a reset state, so they cannot scan IDs.Therefore, I need to raise these control reset pins to make the phy device work properly. Can I resend a v2 patch? Think you. >> >> Release the reset pin, because it needs to be >> registered to the created phy device. >> >> Signed-off-by: Yan Wang <rk.code@outlook.com> >> >> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c >> index 1183ef5e203e..8fdf1293f447 100644 >> --- a/drivers/net/mdio/fwnode_mdio.c >> +++ b/drivers/net/mdio/fwnode_mdio.c >> @@ -11,6 +11,7 @@ >> #include <linux/of.h> >> #include <linux/phy.h> >> #include <linux/pse-pd/pse.h> >> +#include <linux/of_gpio.h> > > These includes appear to be sorted. OK, I will modify it. > >> >> MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); >> MODULE_LICENSE("GPL"); >> @@ -57,6 +58,32 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) >> return register_mii_timestamper(arg.np, arg.args[0]); >> } >> >> +static void fwnode_mdiobus_deassert_reset_phy(struct fwnode_handle *fwnode) >> +{ >> + struct device_node *np; >> + int reset; >> + int rc; >> + >> + np = to_of_node(fwnode); >> + if (!np) >> + return; >> + reset = of_get_named_gpio(np, "reset-gpios", 0); >> + if (gpio_is_valid(reset)) { >> + rc = gpio_request(reset, NULL); >> + if (rc < 0) { >> + pr_err("The currunt state of the reset pin is %s ", >> + rc == -EBUSY ? "busy" : "error"); > > Please correctly handle -EPROBE_DEFFER. The GPIO driver might not of > probed yet. The gpio maintainers are also trying to remove the gpio_ > API and replace it with gpiod_. Ok,I will modify it. > >> + } else { >> + gpio_direction_output(reset, 0); >> + usleep_range(1000, 2000); >> + gpio_direction_output(reset, 1); > > This is actually putting it into reset first, and then taking it out > of reset. We want to avoid that. it forces a new auto-neg cycles which > takes a little over 1 second. Phylib will try to avoid forcing an > auto-neg so you get link faster. If the PHY does not need to be > reconfigured it won't be and the result of the auto neg can be used. Is the delay too long? The phy reset pin is got and pulled up in mdiobus_register_device(). Auto-neg is meaningless! Phy does not have a matching driver. > Andrew
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index 1183ef5e203e..8fdf1293f447 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -11,6 +11,7 @@ #include <linux/of.h> #include <linux/phy.h> #include <linux/pse-pd/pse.h> +#include <linux/of_gpio.h> MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); MODULE_LICENSE("GPL"); @@ -57,6 +58,32 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) return register_mii_timestamper(arg.np, arg.args[0]); } +static void fwnode_mdiobus_deassert_reset_phy(struct fwnode_handle *fwnode) +{ + struct device_node *np; + int reset; + int rc; + + np = to_of_node(fwnode); + if (!np) + return; + reset = of_get_named_gpio(np, "reset-gpios", 0); + if (gpio_is_valid(reset)) { + rc = gpio_request(reset, NULL); + if (rc < 0) { + pr_err("The currunt state of the reset pin is %s ", + rc == -EBUSY ? "busy" : "error"); + } else { + gpio_direction_output(reset, 0); + usleep_range(1000, 2000); + gpio_direction_output(reset, 1); + /*Release the reset pin,it needs to be registeredwith the PHY.*/ + gpio_free(reset); + } + } + +} + int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy, struct fwnode_handle *child, u32 addr) @@ -129,6 +156,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, goto clean_pse; } + fwnode_mdiobus_deassert_reset_phy(child); + is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); if (is_c45 || fwnode_get_phy_id(child, &phy_id)) phy = get_phy_device(bus, addr, is_c45);
Every PHY chip has a reset pin. the state isn't sure of the PHY before scanning. It is resetting, Scanning phy ID will fail, so deassert reset for the chip ,normal operation. Release the reset pin, because it needs to be registered to the created phy device. Signed-off-by: Yan Wang <rk.code@outlook.com>