[net-next,v4,13/12] net: lan865x: optional hardware reset

From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
 <ramon.nordin.rodriguez@ferroamp.se>
Date: Sun, 28 Apr 2024 22:25:12 +0200
Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This commit optionally enables a hardware reset of the lan8650/1
mac-phy. These chips have a software reset that is discourage from use
in the manual since it only resets the internal phy.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 .../bindings/net/microchip,lan865x.yaml       |  4 +++
 .../net/ethernet/microchip/lan865x/lan865x.c  | 28 +++++++++++++++++++
 2 files changed, 32 insertions(+)

On Sun, Apr 28, 2024 at 11:16:32PM +0200, Ramón Nordin Rodriguez wrote:
> >From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
>  <ramon.nordin.rodriguez@ferroamp.se>
> Date: Sun, 28 Apr 2024 22:25:12 +0200
> Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

You sent this patch in an odd way. We don't normally see headers like
this. I've been using b4 recently for patch management:

https://b4.docs.kernel.org/en/latest/contributor/prep.html

Using `b4 send` is a good idea. Otherwise git format-patch; git send-email

> index 9abefa8b9d9f..bed9033574b2 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <linux/phy.h>
>  #include <linux/oa_tc6.h>
> +#include <linux/gpio/driver.h>

This is not a gpio driver, it is a gpio consumer. So you should be
using linux/gpio/consumer.h. Also, i _think_ the includes are sorted,
so it probably should go earlier.

>  
>  #define DRV_NAME			"lan865x"
>  
> @@ -33,6 +34,7 @@
>  
>  struct lan865x_priv {
>  	struct work_struct multicast_work;
> +	struct gpio_desc *reset_gpio;
>  	struct net_device *netdev;
>  	struct spi_device *spi;
>  	struct oa_tc6 *tc6;
> @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv)
>  	return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
>  }
>  
> +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv)
> +{
> +	priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset_gpio))
> +		return PTR_ERR(priv->reset_gpio);
> +
> +	return 0;
> +}
> +
> +static void lan865x_hw_reset(struct lan865x_priv *priv)
> +{
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	// section 9.6.3 RESET_N Timing specifies a minimum hold of 5us
> +	usleep_range(5, 10);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +}

Do you see a need to do a reset at any time other than probe? If not,
i would probably combine these two functions into one. Also, since you
pass GPIOD_OUT_HIGH, you have already put it into reset. So setting
the gpio to 1 is pointless.

Does the datasheet say anything about how long you should wait after
releasing the reset?

> +
>  static int lan865x_probe(struct spi_device *spi)
>  {
>  	struct net_device *netdev;
> @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi)
>  	priv->netdev = netdev;
>  	priv->spi = spi;
>  	spi_set_drvdata(spi, priv);
> +	if (lan865x_probe_reset_gpio(priv)) {
> +		dev_err(&spi->dev, "failed to probe reset pin");
> +		ret = -ENODEV;

It is normal that a function like lan865x_probe_reset_gpio() would
return an error code. You should then return that error code, rather
than replace it with ENODEV.

     Andrew

Hi Ramon,

On 29/04/24 2:46 am, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>  From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
>   <ramon.nordin.rodriguez@ferroamp.se>
> Date: Sun, 28 Apr 2024 22:25:12 +0200
> Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This commit optionally enables a hardware reset of the lan8650/1
> mac-phy. These chips have a software reset that is discourage from use
> in the manual since it only resets the internal phy.
The software reset done by the current driver is not only resetting the 
internal PHY, it resets the entire MAC-PHY including the integrated PHY.
The reset bit of the Clause 22 basic control register only will reset 
the internal PHY alone. But oa_tc6_sw_reset_macphy() function is writing 
software reset bit in the Reset Control and Status register which resets 
the entire MAC-PHY including the internal PHY.

OPEN Alliance spec says the following which is done in the 
oa_tc6_sw_reset_macphy().

9.2.4
Reset Control and Status Register (0x0003)
9.2.4.1 RSVD
Reserved for future use. Writing to these bits shall have no effect on 
the MAC-PHY.
9.2.4.2 SWRESET
MAC-PHY Software Reset. The action of writing a ‘1’ to this bit shall 
fully reset the MAC-PHY, including the integrated PHY, to an initial 
state including but not limited to resetting all state machines and 
registers to their default value. When this bit is set, the reset shall 
not occur until CSn is deasserted to allow for the control command write 
to complete. This bit is self-clearing.

LAN8650 spec says the following,

4.1.1.3 Software Reset
A software reset of the LAN8650/1 is available via the Soft Reset 
(SW_RESET) bit in the standard OA_CONFIG0 register.

Note: The SW_RESET bit of the Clause 22 Basic Control register will 
reset only the internal PHY, not the entire device. This PHY only reset 
is not recommended for use. If such a reset is detected, by reading the 
RESETC bit of the STS2 register, reset the entire device.

The above note is given in the lan8650 datasheet to let the user to know 
that clause 22 software reset will reset only internal PHY but I don't 
think they mean it for the MAC-PHY software reset done from Reset 
Control and Status register.

So in my opinion, I don't see the need of external pin reset as the 
existing oa_tc6_sw_reset_macphy() function does the software reset of 
the entire MAC-PHY.

Still if you see a need to have this external pin reset as an optional 
function then it may be needed for all the vendor specific MAC drivers. 
In that case, reset-gpios parameter value alone can be taken from the 
chip specific device tree and the remaining code for operating the reset 
gpio can be moved to oa_tc6.c and the function name can be 
oa_tc6_hw_reset_macphy().

Best regards,
Parthiban V
> 
> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> ---
>   .../bindings/net/microchip,lan865x.yaml       |  4 +++
>   .../net/ethernet/microchip/lan865x/lan865x.c  | 28 +++++++++++++++++++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> index 4fdec0ba3532..0f11f431df06 100644
> --- a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> @@ -44,6 +44,9 @@ properties:
>       minimum: 15000000
>       maximum: 25000000
> 
> +  reset-gpios:
> +    maxItems: 1
> +
>     "#address-cells":
>       const: 1
> 
> @@ -76,5 +79,6 @@ examples:
>           interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
>           local-mac-address = [04 05 06 01 02 03];
>           spi-max-frequency = <15000000>;
> +        reset-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>;
>         };
>       };
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 9abefa8b9d9f..bed9033574b2 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -9,6 +9,7 @@
>   #include <linux/kernel.h>
>   #include <linux/phy.h>
>   #include <linux/oa_tc6.h>
> +#include <linux/gpio/driver.h>
> 
>   #define DRV_NAME                       "lan865x"
> 
> @@ -33,6 +34,7 @@
> 
>   struct lan865x_priv {
>          struct work_struct multicast_work;
> +       struct gpio_desc *reset_gpio;
>          struct net_device *netdev;
>          struct spi_device *spi;
>          struct oa_tc6 *tc6;
> @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv)
>          return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
>   }
> 
> +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv)
> +{
> +       priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset",
> +                                                  GPIOD_OUT_HIGH);
> +       if (IS_ERR(priv->reset_gpio))
> +               return PTR_ERR(priv->reset_gpio);
> +
> +       return 0;
> +}
> +
> +static void lan865x_hw_reset(struct lan865x_priv *priv)
> +{
> +       gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +       // section 9.6.3 RESET_N Timing specifies a minimum hold of 5us
> +       usleep_range(5, 10);
> +       gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +}
> +
>   static int lan865x_probe(struct spi_device *spi)
>   {
>          struct net_device *netdev;
> @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi)
>          priv->netdev = netdev;
>          priv->spi = spi;
>          spi_set_drvdata(spi, priv);
> +       if (lan865x_probe_reset_gpio(priv)) {
> +               dev_err(&spi->dev, "failed to probe reset pin");
> +               ret = -ENODEV;
> +               goto free_netdev;
> +       }
> +
> +       if (priv->reset_gpio)
> +               lan865x_hw_reset(priv);
>          INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
> 
>          priv->tc6 = oa_tc6_init(spi, netdev);
> --
> 2.43.0
> 
>

On 28/04/2024 23:16, Ramón Nordin Rodriguez wrote:
> From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
>  <ramon.nordin.rodriguez@ferroamp.se>
> Date: Sun, 28 Apr 2024 22:25:12 +0200
> Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This commit optionally enables a hardware reset of the lan8650/1
> mac-phy. These chips have a software reset that is discourage from use
> in the manual since it only resets the internal phy.
> 
> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> ---
>  .../bindings/net/microchip,lan865x.yaml       |  4 +++
>  .../net/ethernet/microchip/lan865x/lan865x.c  | 28 +++++++++++++++++++

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

Best regards,
Krzysztof

> > This commit optionally enables a hardware reset of the lan8650/1
> > mac-phy. These chips have a software reset that is discourage from use
> > in the manual since it only resets the internal phy.
> The software reset done by the current driver is not only resetting the 
> internal PHY, it resets the entire MAC-PHY including the integrated PHY.
> The reset bit of the Clause 22 basic control register only will reset 
> the internal PHY alone. But oa_tc6_sw_reset_macphy() function is writing 
> software reset bit in the Reset Control and Status register which resets 
> the entire MAC-PHY including the internal PHY.

All right, I did not dig deep enough obviously.

> The above note is given in the lan8650 datasheet to let the user to know 
> that clause 22 software reset will reset only internal PHY but I don't 
> think they mean it for the MAC-PHY software reset done from Reset 
> Control and Status register.

Could still be relevant to implement the .soft_reset with -EOPNOTSUPP as
Andrew has suggested in the phy driver.

> 
> So in my opinion, I don't see the need of external pin reset as the 
> existing oa_tc6_sw_reset_macphy() function does the software reset of 
> the entire MAC-PHY.

I agree with your assesment that this invalidates the problem I was
aiming at solving.

Additionally I figured out why my setup did not work without the HW
reset, I had missed a pull resistor in the schematic that held the IC in
reset.

To me it seems more feature complete to have a driver option for the
physical capabilities of the chip, but if it doesn't actually solve a
problem it might just be bloat.

> 
> Still if you see a need to have this external pin reset as an optional 
> function then it may be needed for all the vendor specific MAC drivers. 
> In that case, reset-gpios parameter value alone can be taken from the 
> chip specific device tree and the remaining code for operating the reset 
> gpio can be moved to oa_tc6.c and the function name can be 
> oa_tc6_hw_reset_macphy().
> 

If the consensus is to keep a HW reset I do like this suggestion.

I won't push for this to be included, if it is, I'm happy to address the
feedback of patch 13.

R

> You sent this patch in an odd way. We don't normally see headers like
> this. I've been using b4 recently for patch management:

Sorry about that, I appreciate the suggestion!

You left good comments on the implementation side as well, I'll wait
with addressing these. Parthibans feedback does invalidate the core of
the problem I set out to solve with this patch.

If the patch is not dropped I'll ofc address these points!

R

> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.
> 

Very much appreciated! I'll hold off on any further submissions for this
patch until it's clear that it's desriable to include.

R

> Additionally I figured out why my setup did not work without the HW
> reset, I had missed a pull resistor in the schematic that held the IC in
> reset.

Having a reset controlled by software is a pretty common
design. Something needs to ensure the device is out of reset. It could
be the bootloader, but i don't particularly like that, hiding away
critical things where they are hard to see. So i think having it in
the Linux driver is better.

There is an open question of does the driver need to actually reset
the device, or is it sufficient to ensure it is out of reset? The
wording of the standard suggests a hardware reset cycle is probably
not required, but why did Microchip provide a reset pin?

	Andrew

> > Additionally I figured out why my setup did not work without the HW
> > reset, I had missed a pull resistor in the schematic that held the IC in
> > reset.
> 
> Having a reset controlled by software is a pretty common
> design. Something needs to ensure the device is out of reset. It could
> be the bootloader, but i don't particularly like that, hiding away
> critical things where they are hard to see. So i think having it in
> the Linux driver is better.

Wholeheartedly agree, beyond the basics of bringing up ram, cores etc.
it becomes really weird when/if the bootloaders behaviour defines kernel
functionality.

In this case the oa_tc6 module does a soft reset and waits for a status
reg to signal ready.

What seems to be missing here is that the chip signals ready/out of
reset by asserting the irq pin and setting the RESETC bit of ther OA_STATUS0
reg (which is defined behaviour in the OA spec as I understand it).

Neither the lan865x_probe or oa_tc6_init checks for the initial
condition, but I'm guessing it's a fair assumption that the chip is out
of reset by the point when the oa_tc6_sw_reset_machphy function is
invoked.

Far as I can tell no timing information is given in the datasheet.

Might be unecessary but the setup could be made more explicit/clear
with a func such as:

int oa_tc6_out_of_reset(struct oa_tc*);

Which should be invoked before any reg access/modification code in
either oa_tc6 or the mac driver.
If this fails my (opinionated) preferred style would be to do one hw
reset, recheck, and on subsequent failure bail.

Such a change would probably lead to the HW reset being invoked on
reboots (if there is enough capacitors to keep the IC powered) and
definetly as a result of kexec calls.

> 
> There is an open question of does the driver need to actually reset
> the device, or is it sufficient to ensure it is out of reset? The
> wording of the standard suggests a hardware reset cycle is probably
> not required, but why did Microchip provide a reset pin?
> 

This has me tripped up. The lan8650/1 has a configuration write
protection mechanism with an unlock sequence, described in "4.6.3
Configuration Protection" of the datasheet.
The unlocking can be bypassed/simplified with a HW reset, still does
not seem like an explanation for the functionality.

I can't define one scenario where the HW reset is definetly necessary,
but will probably do it anyways on the systems I work on.

Ramón

On 29/04/24 5:49 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Additionally I figured out why my setup did not work without the HW
>> reset, I had missed a pull resistor in the schematic that held the IC in
>> reset.
> 
> Having a reset controlled by software is a pretty common
> design. Something needs to ensure the device is out of reset. It could
> be the bootloader, but i don't particularly like that, hiding away
> critical things where they are hard to see. So i think having it in
> the Linux driver is better.
> 
> There is an open question of does the driver need to actually reset
> the device, or is it sufficient to ensure it is out of reset? The
> wording of the standard suggests a hardware reset cycle is probably
> not required, but why did Microchip provide a reset pin?
There are three resets mentioned,

1. Power ON reset - A Power-On Reset occurs when power is initially 
applied to the device. Once the reset completes, the IRQ_N pin will be 
asserted and the RESETC (Reset Completed) bit of the OA_STATUS0 Register 
will be set to 1, as specified by the Open Alliance. This indicates to 
the driver that the device has been reset and ready to configure. For a 
pre configured system this is enough to start the configuration.

2. Software reset - This reset can be done from software. In the driver, 
it is done as part of initialization. Actually it may not be needed if 
we are not going to reload the driver, as the Power ON reset does the 
same job for us. But if we compiled the driver as a loadable module and 
out of tree then it is needed for each time when we reload the driver 
without power reset.

3. Ext pin reset - A hardware reset will occur when the RESET_N pin is 
asserted. Once the RESET_N input is deasserted, the LAN8650/1 will 
restart operation. The device will indicate to the station's controller
that it has been reset and must be configured in the same way as if a 
Power-On Reset had occurred: the IRQ_N pin will be asserted and the 
RESETC (Reset Completed) bit of the OA_STATUS Register will be set to 1.

This is also basically does the same job but it may be useful when the 
chip gets into a situation where it can't perform SPI anymore to do a 
software reset. This may be needed in the initial phase testing. But 
once the development is completed and the chip is ready to use then 
there will not be any possibility to get into the above situation unless 
there is a permanent hardware failure where this reset also will not 
make any sense.

OPEN Alliance spec says the following in the section 8.2 Variables

reset
	This variable reflects the logical-OR of all reset sources of the 
MAC-PHY and is TRUE when any of the reset sources are asserted. Reset 
sources include power-on reset (POR), software reset (see Section 
9.2.4.2), and an external RESET pin (if implemented).

In the spec, external RESET pin is mentioned as "if implemented", in my 
understanding it is MAC-PHY vendors choice of implementing it where 
Microchip is implemented it. Using this reset, can be a application 
requirement/decision. It can be controlled from an external application 
where it is not needed SPI to operate.

Best regards,
Parthiban V
> 
>          Andrew

> In the spec, external RESET pin is mentioned as "if implemented", in my 
> understanding it is MAC-PHY vendors choice of implementing it where 
> Microchip is implemented it. Using this reset, can be a application 
> requirement/decision. It can be controlled from an external application 
> where it is not needed SPI to operate.

Since it is optional, controlling the reset pin is clearly not
something for the TC6 core.

However, i doubt having an external application controlling the reset
is a good idea. You don't want to reset during operation. So to me,
this reset should be controlled by the driver. I tend to agree, that
actually performing a reset is optional, but i would expect the driver
to ensure the device is taken out of reset during probe, if the power
on default of the board is to hold it in reset.

   Andrew

Hi Andrew,

On 30/04/24 7:44 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> In the spec, external RESET pin is mentioned as "if implemented", in my
>> understanding it is MAC-PHY vendors choice of implementing it where
>> Microchip is implemented it. Using this reset, can be a application
>> requirement/decision. It can be controlled from an external application
>> where it is not needed SPI to operate.
> 
> Since it is optional, controlling the reset pin is clearly not
> something for the TC6 core.
> 
> However, i doubt having an external application controlling the reset
> is a good idea. You don't want to reset during operation. So to me,
> this reset should be controlled by the driver. I tend to agree, that
> actually performing a reset is optional, but i would expect the driver
> to ensure the device is taken out of reset during probe, if the power
> on default of the board is to hold it in reset.
POR - Power ON Reset.

If I understand you correctly, we need to ensure that whether the 
MAC-PHY is out of POR before doing any SPI operation right? so you 
expect oa_tc6_sw_reset_macphy() also may fail as the SWRESET bit write 
may fail if the device is still in POR.

Basically oa_tc6_sw_reset_macphy() does the following,
  - Writes 1 to SWRESET bit in the Reset Control and Status Register to 
trigger software reset.
  - Polls for software reset complete for every 1ms until 1s timeout.
  - Clears reset status complete status.

Can we rename the function like oa_tc6_check_power_on_reset_complete()
and the definition like below?
  - Poll for software reset complete for every 1ms until 1s timeout. The 
return value of oa_tc6_read_register() may not be needed to check for 
error as it might fail if the POR is taking place right?
  - Clear reset complete status.

But this case will work fine with initial power ON but after that if we 
are only reloading the driver for next time without powering OFF and ON 
then it will fail.

I am just thinking from TC6 framework point of view. Controlling from 
ext reset pin will not be an option as it is mentioned like an optional 
implementation in the OPEN Alliance. So some MAC-PHY vendors might not 
implement this.

Just an open question, after powering ON the Linux system, there are 
different stages are involved in the booting sequence of a system before 
kernel driver loads. A device like MAC-PHY which takes hardly around 5 
to 10us will complete the POR before that? OR is my understanding here 
is wrong?

Best regards,
Parthiban V
> 
>     Andrew