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

[<Parthiban.Veerasooran@microchip.com>]

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