[PATCH v4] net: mdiobus: Add a function to deassert reset

[PATCH v4] net: mdiobus: Add a function to deassert reset

[Yan Wang]

It is possible to mount multiple sub-devices on the mido bus.
The hardware power-on does not necessarily reset these devices.
The device may be in an uncertain state, causing the device's ID
to not be scanned.

So,before adding a reset to the scan, make sure the device is in
normal working mode.

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202305101702.4xW6vT72-lkp@intel.com/
Signed-off-by: Yan Wang <rk.code@outlook.com>
---
v4:
  - Get pulse width and settling time from the device tree
  - Add logic for processing (PTR_ERR(reset) == -EPROBE_DEFER)
  - included <linux/goio/consumer.h>
  - fixed commit message
v3: https://lore.kernel.org/all/KL1PR01MB5448A33A549CDAD7D68945B9E6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
  - fixed commit message
v2: https://lore.kernel.org/all/KL1PR01MB54482416A8BE0D80EA27223CE6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
  - fixed commit message
  - Using gpiod_ replace gpio_
v1: https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
  - Incorrect description of commit message.
  - The gpio-api too old
---
 drivers/net/mdio/fwnode_mdio.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..9d7df6393059 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/gpio/consumer.h>
 
 MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");
 MODULE_LICENSE("GPL");
@@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
 	return register_mii_timestamper(arg.np, arg.args[0]);
 }
 
+static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
+{
+	struct gpio_desc *reset;
+	unsigned int reset_assert_delay;
+	unsigned int reset_deassert_delay;
+
+	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
+	if (IS_ERR(reset)) {
+		if (PTR_ERR(reset) == -EPROBE_DEFER)
+			pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
+				 to_of_node(fwnode), __func__);
+		else
+			pr_err("%pOFn: %s: Can't get reset line property\n",
+			       to_of_node(fwnode), __func__);
+
+		return;
+	}
+	fwnode_property_read_u32(fwnode, "reset-assert-us",
+				 &reset_assert_delay);
+	fwnode_property_read_u32(fwnode, "reset-deassert-us",
+				 &reset_deassert_delay);
+	gpiod_set_value_cansleep(reset, 1);
+	fsleep(reset_assert_delay);
+	gpiod_set_value_cansleep(reset, 0);
+	fsleep(reset_deassert_delay);
+	/*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
+	gpiod_put(reset);
+}
+
 int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 				       struct phy_device *phy,
 				       struct fwnode_handle *child, u32 addr)
@@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	u32 phy_id;
 	int rc;
 
+	fwnode_mdiobus_pre_enable_phy(child);
+
 	psec = fwnode_find_pse_control(child);
 	if (IS_ERR(psec))
 		return PTR_ERR(psec);
-- 
2.17.1

Re: [PATCH v4] net: mdiobus: Add a function to deassert reset

[Simon Horman]

On Thu, May 11, 2023 at 02:59:09PM +0800, Yan Wang wrote:
> It is possible to mount multiple sub-devices on the mido bus.
> The hardware power-on does not necessarily reset these devices.
> The device may be in an uncertain state, causing the device's ID
> to not be scanned.
> 
> So,before adding a reset to the scan, make sure the device is in
> normal working mode.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202305101702.4xW6vT72-lkp@intel.com/
> 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..9d7df6393059 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/gpio/consumer.h>
>  
>  MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");
>  MODULE_LICENSE("GPL");
> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
>  	return register_mii_timestamper(arg.np, arg.args[0]);
>  }
>  
> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> +{
> +	struct gpio_desc *reset;
> +	unsigned int reset_assert_delay;
> +	unsigned int reset_deassert_delay;

nit: Please arrange local variables for networking code in reverse xmas
     tree order - longest line to shortest.

> +
> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
> +	if (IS_ERR(reset)) {
> +		if (PTR_ERR(reset) == -EPROBE_DEFER)
> +			pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
> +				 to_of_node(fwnode), __func__);
> +		else
> +			pr_err("%pOFn: %s: Can't get reset line property\n",
> +			       to_of_node(fwnode), __func__);
> +
> +		return;
> +	}
> +	fwnode_property_read_u32(fwnode, "reset-assert-us",
> +				 &reset_assert_delay);
> +	fwnode_property_read_u32(fwnode, "reset-deassert-us",
> +				 &reset_deassert_delay);

Does the return value of fwnode_property_read_u32() need to be
checked for errors?

> +	gpiod_set_value_cansleep(reset, 1);
> +	fsleep(reset_assert_delay);
> +	gpiod_set_value_cansleep(reset, 0);
> +	fsleep(reset_deassert_delay);
> +	/*Release phy's reset line, mdiobus_register_gpiod() need to request it*/

nit:

	/* Release phy's reset line, mdiobus_register_gpiod() needs to
	 * request it.
	 */

> +	gpiod_put(reset);
> +}
> +
>  int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>  				       struct phy_device *phy,
>  				       struct fwnode_handle *child, u32 addr)
> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  	u32 phy_id;
>  	int rc;
>  
> +	fwnode_mdiobus_pre_enable_phy(child);
> +
>  	psec = fwnode_find_pse_control(child);
>  	if (IS_ERR(psec))
>  		return PTR_ERR(psec);
> -- 
> 2.17.1
> 
> 

Re: [PATCH v4] net: mdiobus: Add a function to deassert reset

[Yan Wang]

On 5/11/2023 8:40 PM, Simon Horman wrote:
> On Thu, May 11, 2023 at 02:59:09PM +0800, Yan Wang wrote:
>> It is possible to mount multiple sub-devices on the mido bus.
>> The hardware power-on does not necessarily reset these devices.
>> The device may be in an uncertain state, causing the device's ID
>> to not be scanned.
>>
>> So,before adding a reset to the scan, make sure the device is in
>> normal working mode.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/oe-kbuild-all/202305101702.4xW6vT72-lkp@intel.com/
>> 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..9d7df6393059 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/gpio/consumer.h>
>>   
>>   MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");
>>   MODULE_LICENSE("GPL");
>> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
>>   	return register_mii_timestamper(arg.np, arg.args[0]);
>>   }
>>   
>> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
>> +{
>> +	struct gpio_desc *reset;
>> +	unsigned int reset_assert_delay;
>> +	unsigned int reset_deassert_delay;
> nit: Please arrange local variables for networking code in reverse xmas
>       tree order - longest line to shortest.
>
>> +
>> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
>> +	if (IS_ERR(reset)) {
>> +		if (PTR_ERR(reset) == -EPROBE_DEFER)
>> +			pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
>> +				 to_of_node(fwnode), __func__);
>> +		else
>> +			pr_err("%pOFn: %s: Can't get reset line property\n",
>> +			       to_of_node(fwnode), __func__);
>> +
>> +		return;
>> +	}
>> +	fwnode_property_read_u32(fwnode, "reset-assert-us",
>> +				 &reset_assert_delay);
>> +	fwnode_property_read_u32(fwnode, "reset-deassert-us",
>> +				 &reset_deassert_delay);
> Does the return value of fwnode_property_read_u32() need to be
> checked for errors?
>
>> +	gpiod_set_value_cansleep(reset, 1);
>> +	fsleep(reset_assert_delay);
>> +	gpiod_set_value_cansleep(reset, 0);
>> +	fsleep(reset_deassert_delay);
>> +	/*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
> nit:
>
> 	/* Release phy's reset line, mdiobus_register_gpiod() needs to
> 	 * request it.
> 	 */

Thank you for your suggestion.

Best Regards
>> +	gpiod_put(reset);
>> +}
>> +
>>   int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>>   				       struct phy_device *phy,
>>   				       struct fwnode_handle *child, u32 addr)
>> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>>   	u32 phy_id;
>>   	int rc;
>>   
>> +	fwnode_mdiobus_pre_enable_phy(child);
>> +
>>   	psec = fwnode_find_pse_control(child);
>>   	if (IS_ERR(psec))
>>   		return PTR_ERR(psec);
>> -- 
>> 2.17.1
>>
>>

Re: [PATCH v4] net: mdiobus: Add a function to deassert reset

[Alexander Stein]

Am Donnerstag, 11. Mai 2023, 08:59:09 CEST schrieb Yan Wang:
> It is possible to mount multiple sub-devices on the mido bus.
> The hardware power-on does not necessarily reset these devices.
> The device may be in an uncertain state, causing the device's ID
> to not be scanned.
> 
> So,before adding a reset to the scan, make sure the device is in

'So, before'. One space after ,

> normal working mode.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Link:
> https://lore.kernel.org/oe-kbuild-all/202305101702.4xW6vT72-lkp@intel.com/
> Signed-off-by: Yan Wang <rk.code@outlook.com>
> ---
> v4:
>   - Get pulse width and settling time from the device tree
>   - Add logic for processing (PTR_ERR(reset) == -EPROBE_DEFER)
>   - included <linux/goio/consumer.h>
>   - fixed commit message
> v3:
> https://lore.kernel.org/all/KL1PR01MB5448A33A549CDAD7D68945B9E6779@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - fixed commit message
> v2:
> https://lore.kernel.org/all/KL1PR01MB54482416A8BE0D80EA27223CE6779@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - fixed commit message
>   - Using gpiod_ replace gpio_
> v1:
> https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - Incorrect description of commit
> message.
>   - The gpio-api too old
> ---
>  drivers/net/mdio/fwnode_mdio.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..9d7df6393059 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/gpio/consumer.h>

Please sort alphabetically.
> 
>  MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");
>  MODULE_LICENSE("GPL");
> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
> return register_mii_timestamper(arg.np, arg.args[0]);
>  }
> 
> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> +{
> +	struct gpio_desc *reset;
> +	unsigned int reset_assert_delay;
> +	unsigned int reset_deassert_delay;
> +
> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, 
NULL);
> +	if (IS_ERR(reset)) {
> +		if (PTR_ERR(reset) == -EPROBE_DEFER)
> +			pr_debug("%pOFn: %s: GPIOs not yet available, 
retry later\n",
> +				 to_of_node(fwnode), __func__);

You are not dealing with this error at all. pr_debug is (mostly) not even 
printed in kernel log, despite the code registering the PHY ignoring the 
deferral completely.

> +		else
> +			pr_err("%pOFn: %s: Can't get reset line 
property\n",
> +			       to_of_node(fwnode), __func__);
> +

This error also gets ignored.

> +		return;
> +	}
> +	fwnode_property_read_u32(fwnode, "reset-assert-us",
> +				 &reset_assert_delay);
> +	fwnode_property_read_u32(fwnode, "reset-deassert-us",
> +				 &reset_deassert_delay);

These are currently read in fwnode_mdiobus_phy_device_register(). There is no 
need to read them twice. It should be moved to a location where it just need 
to be read once.

Regards,
Alexander

> +	gpiod_set_value_cansleep(reset, 1);
> +	fsleep(reset_assert_delay);
> +	gpiod_set_value_cansleep(reset, 0);
> +	fsleep(reset_deassert_delay);
> +	/*Release phy's reset line, mdiobus_register_gpiod() need to request 
it*/
> +	gpiod_put(reset);
> +}
> +
>  int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>  				       struct phy_device *phy,
>  				       struct fwnode_handle *child, 
u32 addr)
> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  	u32 phy_id;
>  	int rc;
> 
> +	fwnode_mdiobus_pre_enable_phy(child);
> +
>  	psec = fwnode_find_pse_control(child);
>  	if (IS_ERR(psec))
>  		return PTR_ERR(psec);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/