[v2] net: mdiobus: Add a function to deassert reset

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

Posted by Yan Wang 2 years, 9 months ago

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 be scanned.

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

I found that the subsequent drive registers the reset pin into the
structure of the sub-device to prevent conflicts, so release the
reset pin.

Signed-off-by: Yan Wang <rk.code@outlook.com>
---
v2:
  - 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..6695848b8ef2 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -57,6 +57,20 @@ 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;
+
+	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
+	if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
+		return;
+
+	usleep_range(100, 200);
+	gpiod_set_value_cansleep(reset, 0);
+	/*Release the reset pin,it needs to be registered with the PHY.*/
+	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 +133,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 v2] net: mdiobus: Add a function to deassert reset

Posted by Alexander Stein 2 years, 9 months ago

Am Mittwoch, 10. Mai 2023, 10:02:52 CEST schrieb Yan Wang:
> It is possible to mount multiple sub-devices on the mido bus.

mdio 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 be scanned.
> 
> So, before adding a reset to the scan, make sure the device is in
> normal working mode.
> 
> I found that the subsequent drive registers the reset pin into the
> structure of the sub-device to prevent conflicts, so release the
> reset pin.
> 
> Signed-off-by: Yan Wang <rk.code@outlook.com>

We had similar cases where the (single) PHY was in reset during Linux boot.
Should you be able to make this work by using a "ethernet-phy-id%4x.%4x" 
compatible? See also [1]

[1] https://lkml.org/lkml/2020/10/28/1139

> ---
> v2:
>   - 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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..6695848b8ef2 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -57,6 +57,20 @@ 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;
> +
> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, 
NULL);
> +	if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)

How are you dealing with EPROBE_DEFER if the reset line is e.g. attached to an 
i2c expander, which is to be probed later on?

> +		return;
> +
> +	usleep_range(100, 200);

How do you know a PHY's reset pulse width?

> +	gpiod_set_value_cansleep(reset, 0);

What about post-reset stabilization times before MDIO access is allowed?

> +	/*Release the reset pin,it needs to be registered with the PHY.*/

/* Release [...] PHY. */

Best regards,
Alexander

> +	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 +133,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/

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

Posted by Yan Wang 2 years, 9 months ago


On 5/10/2023 7:55 PM, Alexander Stein wrote:
> Am Mittwoch, 10. Mai 2023, 10:02:52 CEST schrieb Yan Wang:
>> It is possible to mount multiple sub-devices on the mido bus.
> mdio bus
Yes, misspelled.
>> The hardware power-on does not necessarily reset these devices.
>> The device may be in an uncertain state, causing the device's ID
>> to be scanned.
>>
>> So, before adding a reset to the scan, make sure the device is in
>> normal working mode.
>>
>> I found that the subsequent drive registers the reset pin into the
>> structure of the sub-device to prevent conflicts, so release the
>> reset pin.
>>
>> Signed-off-by: Yan Wang <rk.code@outlook.com>
> We had similar cases where the (single) PHY was in reset during Linux boot.
> Should you be able to make this work by using a "ethernet-phy-id%4x.%4x"
> compatible? See also [1]
>
> [1] https://lkml.org/lkml/2020/10/28/1139
Well, I've seen the [1] before, this method may mask some issues. For 
example ,if I use
another type of phy ,I have to modify the DT, Is it very cumbersome?
>> ---
>> v2:
>>    - 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 | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
>> index 1183ef5e203e..6695848b8ef2 100644
>> --- a/drivers/net/mdio/fwnode_mdio.c
>> +++ b/drivers/net/mdio/fwnode_mdio.c
>> @@ -57,6 +57,20 @@ 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;
>> +
>> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH,
> NULL);
>> +	if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
> How are you dealing with EPROBE_DEFER if the reset line is e.g. attached to an
> i2c expander, which is to be probed later on?
Thank you ,The logic is wrong,trying to fix it.
>> +		return;
>> +
>> +	usleep_range(100, 200);
> How do you know a PHY's reset pulse width?
>
>> +	gpiod_set_value_cansleep(reset, 0);
> What about post-reset stabilization times before MDIO access is allowed?
yes,I need to get reset pulse width and post-reset stabilization times 
from reset-assert-us and  reset-deassert-us. right?
>> +	/*Release the reset pin,it needs to be registered with the PHY.*/
> /* Release [...] PHY. */
>
> Best regards,
> Alexander
Thank you for your support.
>> +	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 +133,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);
>

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

Posted by Alexander Stein 2 years, 9 months ago

Am Donnerstag, 11. Mai 2023, 05:16:52 CEST schrieb Yan Wang:
> On 5/10/2023 7:55 PM, Alexander Stein wrote:
> > Am Mittwoch, 10. Mai 2023, 10:02:52 CEST schrieb Yan Wang:
> >> It is possible to mount multiple sub-devices on the mido bus.
> > 
> > mdio bus
> 
> Yes, misspelled.
> 
> >> The hardware power-on does not necessarily reset these devices.
> >> The device may be in an uncertain state, causing the device's ID
> >> to be scanned.
> >> 
> >> So, before adding a reset to the scan, make sure the device is in
> >> normal working mode.
> >> 
> >> I found that the subsequent drive registers the reset pin into the
> >> structure of the sub-device to prevent conflicts, so release the
> >> reset pin.
> >> 
> >> Signed-off-by: Yan Wang <rk.code@outlook.com>
> > 
> > We had similar cases where the (single) PHY was in reset during Linux
> > boot.
> > Should you be able to make this work by using a "ethernet-phy-id%4x.%4x"
> > compatible? See also [1]
> > 
> > [1] https://lkml.org/lkml/2020/10/28/1139
> 
> Well, I've seen the [1] before, this method may mask some issues. For
> example ,if I use
> another type of phy ,I have to modify the DT, Is it very cumbersome?

You have to change the reset timings and possibly other properties as well if 
you change the PHY, so a DT change is necessary anyway.

Regards,
Alexander

> 
> >> ---
> >> 
> >> v2:
> >>    - fixed commit message
> >>    - Using gpiod_ replace gpio_
> >> 
> >> v1:
> >> https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR0
> >> 1M
> >> B5448.apcprd01.prod.exchangelabs.com/ - Incorrect description of commit
> >> message.
> >> 
> >>    - The gpio-api too old
> >> 
> >> ---
> >> 
> >>   drivers/net/mdio/fwnode_mdio.c | 16 ++++++++++++++++
> >>   1 file changed, 16 insertions(+)
> >> 
> >> diff --git a/drivers/net/mdio/fwnode_mdio.c
> >> b/drivers/net/mdio/fwnode_mdio.c index 1183ef5e203e..6695848b8ef2 100644
> >> --- a/drivers/net/mdio/fwnode_mdio.c
> >> +++ b/drivers/net/mdio/fwnode_mdio.c
> >> @@ -57,6 +57,20 @@ 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;
> >> +
> >> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH,
> > 
> > NULL);
> > 
> >> +	if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
> > 
> > How are you dealing with EPROBE_DEFER if the reset line is e.g. attached
> > to an i2c expander, which is to be probed later on?
> 
> Thank you ,The logic is wrong,trying to fix it.
> 
> >> +		return;
> >> +
> >> +	usleep_range(100, 200);
> > 
> > How do you know a PHY's reset pulse width?
> > 
> >> +	gpiod_set_value_cansleep(reset, 0);
> > 
> > What about post-reset stabilization times before MDIO access is allowed?
> 
> yes,I need to get reset pulse width and post-reset stabilization times
> from reset-assert-us and  reset-deassert-us. right?
> 
> >> +	/*Release the reset pin,it needs to be registered with the PHY.*/
> > 
> > /* Release [...] PHY. */
> > 
> > Best regards,
> > Alexander
> 
> Thank you for your support.
> 
> >> +	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 +133,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/