netdev - Re: [PATCH v1] net: mdiobus: Add a function to deassert reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <96a1b95e-d05e-40f0-ada9-1956f43010e0@lunn.ch>
Date: Tue, 9 May 2023 14:22:49 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yan Wang <rk.code@...look.com>
Cc: hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux@...linux.org.uk
Subject: Re: [PATCH v1] net: mdiobus: Add a function to deassert reset

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@...look.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@....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