[PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Lizhe]

some devices only reset when the GPIO is at a high level, but the
current function lacks support for such devices. add high-level
reset functionality to the function to support devices that require
high-level triggering for reset

Signed-off-by: Lizhe <sensor1010@163.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 836f2848dfeb..cb989e6d7eac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -458,6 +458,7 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 
 #ifdef CONFIG_OF
 	if (priv->device->of_node) {
+		int active_low = 0;
 		struct gpio_desc *reset_gpio;
 		u32 delays[3] = { 0, 0, 0 };
 
@@ -467,6 +468,9 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 		if (IS_ERR(reset_gpio))
 			return PTR_ERR(reset_gpio);
 
+		if (reset_gpio)
+			active_low = gpiod_is_active_low(reset_gpio);
+
 		device_property_read_u32_array(priv->device,
 					       "snps,reset-delays-us",
 					       delays, ARRAY_SIZE(delays));
@@ -474,11 +478,11 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 		if (delays[0])
 			msleep(DIV_ROUND_UP(delays[0], 1000));
 
-		gpiod_set_value_cansleep(reset_gpio, 1);
+		gpiod_set_value_cansleep(reset_gpio, active_low ? 1 : 0);
 		if (delays[1])
 			msleep(DIV_ROUND_UP(delays[1], 1000));
 
-		gpiod_set_value_cansleep(reset_gpio, 0);
+		gpiod_set_value_cansleep(reset_gpio, active_low ? 0 : 1);
 		if (delays[2])
 			msleep(DIV_ROUND_UP(delays[2], 1000));
 	}
-- 
2.17.1

Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Andrew Lunn]

On Tue, Jul 08, 2025 at 09:50:44AM -0700, Lizhe wrote:
> some devices only reset when the GPIO is at a high level, but the
> current function lacks support for such devices. add high-level
> reset functionality to the function to support devices that require
> high-level triggering for reset

You can probably specify this in DT:

reset-gpios = <&qe_pio_e 18 GPIO_ACTIVE_LOW>;

The gpio core will then flip the meaning of

gpiod_set_value_cansleep(reset_gpio, 1);

such that a value of 1 will become low, 0 will become high.

     Andrew

Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Tue, Jul 08, 2025 at 09:50:44AM -0700, Lizhe wrote:
> some devices only reset when the GPIO is at a high level, but the
> current function lacks support for such devices. add high-level
> reset functionality to the function to support devices that require
> high-level triggering for reset
> 
> Signed-off-by: Lizhe <sensor1010@163.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 836f2848dfeb..cb989e6d7eac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -458,6 +458,7 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  
>  #ifdef CONFIG_OF
>  	if (priv->device->of_node) {
> +		int active_low = 0;
>  		struct gpio_desc *reset_gpio;
>  		u32 delays[3] = { 0, 0, 0 };
>  
> @@ -467,6 +468,9 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  		if (IS_ERR(reset_gpio))
>  			return PTR_ERR(reset_gpio);
>  
> +		if (reset_gpio)
> +			active_low = gpiod_is_active_low(reset_gpio);
> +
>  		device_property_read_u32_array(priv->device,
>  					       "snps,reset-delays-us",
>  					       delays, ARRAY_SIZE(delays));
> @@ -474,11 +478,11 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  		if (delays[0])
>  			msleep(DIV_ROUND_UP(delays[0], 1000));
>  
> -		gpiod_set_value_cansleep(reset_gpio, 1);
> +		gpiod_set_value_cansleep(reset_gpio, active_low ? 1 : 0);
>  		if (delays[1])
>  			msleep(DIV_ROUND_UP(delays[1], 1000));
>  
> -		gpiod_set_value_cansleep(reset_gpio, 0);
> +		gpiod_set_value_cansleep(reset_gpio, active_low ? 0 : 1);
>  		if (delays[2])
>  			msleep(DIV_ROUND_UP(delays[2], 1000));
>  	}

NAK. Not required. The GPIO layer can cope with active-high and
active-low signals declared in firmware without needing driver
modification. Use the right data in the firmware and you don't
need to patch.

/* Bit 0 express polarity */
#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Wed, Jul 09, 2025 at 09:58:21AM +0800, lizhe wrote:
> Hi, 
> 
> Thx !
> 
> i conducted an experiment, and no matter whether i configured it as 
> GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH in the dts, the resulting
> GPIO pin state was 0, indicating a low level.
> 
> 
> if (delays[2])
>     msleep(DIV_ROUND_UP(DELAYS[2], 1000));
> 
> + gpio_state = gpiod_get_value_can_sleep(reset_gpio);
> + pr_info("gpio_state: %d\n", gpio_state);

Use gpiod_get_raw_value_cansleep().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Wed, Jul 09, 2025 at 09:57:50AM +0800, lizhe wrote:
> + gpio_state = gpiod_get_value_can_sleep(reset_gpio);

Use gpiod_get_raw_value_cansleep(). The normal get/set return the
active/inactive state, whereas the _raw get/set return the physical
state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Wed, Jul 09, 2025 at 03:48:25PM +0800, lizhe wrote:
> Hi, 
> 
> after replacing with this function, the function returns 0, meaning the gpio is
> still at a low voltage level.
> 
> +              int gpio_state = -1;
> 
> 
>                 if (delays[2])
>                         msleep(DIV_ROUND_UP(delays[2], 1000));
> +
> +               gpio_state = gpiod_get_raw_value_cansleep(reset_gpio);
> +               pr_info("gpio_state: %d\n", gpio_state);
> +               pr_info("gpio_state: %d\n", gpio_state);
> 
>  gpio-111 (                    |snps,reset          ) out lo
> 
> 
> [    3.899319] gpio_state: 0
> [    3.899324] gpio_state: 0

How have you declared the snps,reset-gpio property in DT?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Andrew Lunn]

On Wed, Jul 09, 2025 at 09:57:50AM +0800, lizhe wrote:
> Hi， Andrew
> 
> Thx,
> 
> 
> i conducted an experiment, and no matter whether i configured it as 
> 
> GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH in the dts, the resulting
> 
> GPIO pin state was 0, indicating a low level.

You need to keep digging and understand why.

Scatter some printk() in the gpio core code. Does it recognise the
option in DT? Is GPIO_ACTIVE_LOW, FLAG_ACTIVE_LOW being set?

Put some prints into the actual GPIO driver, what is passed to it.

There is something interesting in gpio.txt:

Most controllers are specifying a generic flag bitfield in the last cell, so
for these, use the macros defined in
include/dt-bindings/gpio/gpio.h whenever possible:

It says 'Most'. Is the GPIO controller you are using not actually
doing this?

	Andrew

Re: Re: Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Wed, Jul 09, 2025 at 07:42:55PM +0800, lizhe wrote:
> Hi,
>     i have already declared it in the DTS.
> 
>     &gmac {
>             snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_HIGH>;
>     };

Active high means that the reset is in effect when the output is at the
logic '1' state. So, gpiod_get_value*() will return the same as
gpiod_get_raw_value*().

Active low means that the reset is in effect when the output is at the
logic '0' state, and in that case the return value of
gpiod_get_value*() will return true when the reset is active (at logic
'0' state) whereas gpiod_get_raw_value*() will return zero.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Re: Re: Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Andrew Lunn]

On Thu, Jul 10, 2025 at 12:05:05AM +0800, lizhe wrote:
> Hi，

Please don't top post.

>     snps, reset-gpio = <&gpioX RK_PXX GPIO_ACTIVE_HIGH>;   
>     All of them correctly parse the GPIO pin's state are described in the DTS
> 
> 
> 
> Thx !
> 
> Lizhe
> 
> 
> 
> 
> At 2025-07-09 22:29:46, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >On Wed, Jul 09, 2025 at 07:42:55PM +0800, lizhe wrote:
> >> Hi,
> >>     i have already declared it in the DTS.
> >>
> >>     &gmac {
> >>             snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_HIGH>;
> >>     };
> >
> >Active high means that the reset is in effect when the output is at the
> >logic '1' state. So, gpiod_get_value*() will return the same as
> >gpiod_get_raw_value*().
> >
> >Active low means that the reset is in effect when the output is at the
> >logic '0' state, and in that case the return value of
> >gpiod_get_value*() will return true when the reset is active (at logic
> >'0' state) whereas gpiod_get_raw_value*() will return zero.

Did you read what Russell said? If so, why are you using
GPIO_ACTIVE_HIGH?

	Andrew

Re: Re: Re: Re: Re: [PATCH] net: stmmac: Support gpio high-level reset for devices requiring it

[Russell King (Oracle)]

On Thu, Jul 10, 2025 at 12:05:05AM +0800, lizhe wrote:
> Hi，
> 
>      
> 
>   if i add the following code to this function, the gpio outputs a high level
> 
>   without this code, it outputs a low level, 
>   the function currently drivers the reset GPIO to a low state, failing to account 
> 
>   for devices requiring an active-high reset.
> 
> 
> 
> 
>   i invited our hardware engineer to  measure the voltage level on this GPIO pin,
> 
>   without  adding this code, the voltage at this GPIO pin remains at 0V
> 
> 
> 
> 
>   +             int current_value;
> 
> 
> 
> 
>   +             keep_high = device_property_read_bool(priv->device,
> 
>   +                                                             "snps,reset-keep-high");
> 
>   +              if (keep_high) {
> 
>   +                     gpiod_set_value_cansleep(reset_gpio, 1);
> 
>   +                      current_value = gpiod_get_value_cansleep(reset_gpio);
> 
>   +                      pr_info("current_value: %d\n", current_value);
> 
>   +              }
> 
>    in the RK3588 system, i am using ,there are many DTS node configured link this:
> 
>     snps, reset-gpio = <&gpioX RK_PXX GPIO_ACTIVE_HIGH>;   
>     All of them correctly parse the GPIO pin's state are described in the DTS
> 

I'm wondering at this point whether the problem here is one of
mis-understanding the engineering terminology. Look at the below
using a fixed-width font:

Active-high reset: _____/^^^^^^^^\____

Active-low reset:  ^^^^^\________/^^^^

                        | reset  |
			|asserted|

So, an active high reset needs to be logic low in order for the
device to function. An active low reset needs to be logic high
for the device to function.

You seem to be wanting to tell the kernel that you have an
active high reset, and expect it to be logic high when you
want it to be active. That is *not* an active high reset.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!