All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] net: mdiobus: Add a function to deassert reset
@ 2023-05-09 10:44 Yan Wang
  2023-05-09 12:22 ` Andrew Lunn
  0 siblings, 1 reply; 7+ messages in thread
From: Yan Wang @ 2023-05-09 10:44 UTC (permalink / raw
  To: andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, linux
  Cc: Yan Wang

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>

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);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] net: mdiobus: Add a function to deassert reset
  2023-05-09 10:44 [PATCH v1] net: mdiobus: Add a function to deassert reset Yan Wang
@ 2023-05-09 12:22 ` Andrew Lunn
  2023-05-09 14:27   ` Yan Wang
  2023-05-10  8:02   ` [PATCH v2] " Yan Wang
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Lunn @ 2023-05-09 12:22 UTC (permalink / raw
  To: Yan Wang
  Cc: hkallweit1, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	linux

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] net: mdiobus: Add a function to deassert reset
  2023-05-09 12:22 ` Andrew Lunn
@ 2023-05-09 14:27   ` Yan Wang
  2023-05-10  8:02   ` [PATCH v2] " Yan Wang
  1 sibling, 0 replies; 7+ messages in thread
From: Yan Wang @ 2023-05-09 14:27 UTC (permalink / raw
  To: Andrew Lunn
  Cc: hkallweit1, davem, edumazet, kuba, pabeni, netdev, open list,
	linux



> 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] net: mdiobus: Add a function to deassert reset
  2023-05-09 12:22 ` Andrew Lunn
  2023-05-09 14:27   ` Yan Wang
@ 2023-05-10  8:02   ` Yan Wang
  2023-05-10 11:55     ` Alexander Stein
  1 sibling, 1 reply; 7+ messages in thread
From: Yan Wang @ 2023-05-10  8:02 UTC (permalink / raw
  To: andrew
  Cc: Yan Wang, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	open list:ETHERNET PHY LIBRARY, open list

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] net: mdiobus: Add a function to deassert reset
  2023-05-10  8:02   ` [PATCH v2] " Yan Wang
@ 2023-05-10 11:55     ` Alexander Stein
  2023-05-11  3:16       ` Yan Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Stein @ 2023-05-10 11:55 UTC (permalink / raw
  To: andrew, Yan Wang
  Cc: Yan Wang, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	open list:ETHERNET PHY LIBRARY, open list

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/



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] net: mdiobus: Add a function to deassert reset
  2023-05-10 11:55     ` Alexander Stein
@ 2023-05-11  3:16       ` Yan Wang
  2023-05-12  8:01         ` Alexander Stein
  0 siblings, 1 reply; 7+ messages in thread
From: Yan Wang @ 2023-05-11  3:16 UTC (permalink / raw
  To: Alexander Stein, andrew
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, open list:ETHERNET PHY LIBRARY,
	open list



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);
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] net: mdiobus: Add a function to deassert reset
  2023-05-11  3:16       ` Yan Wang
@ 2023-05-12  8:01         ` Alexander Stein
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Stein @ 2023-05-12  8:01 UTC (permalink / raw
  To: andrew, Yan Wang
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, open list:ETHERNET PHY LIBRARY,
	open list

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/



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-05-12  8:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-09 10:44 [PATCH v1] net: mdiobus: Add a function to deassert reset Yan Wang
2023-05-09 12:22 ` Andrew Lunn
2023-05-09 14:27   ` Yan Wang
2023-05-10  8:02   ` [PATCH v2] " Yan Wang
2023-05-10 11:55     ` Alexander Stein
2023-05-11  3:16       ` Yan Wang
2023-05-12  8:01         ` Alexander Stein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.