Re: [PATCH net] net: phy: micrel: populate .soft_reset for KSZ9131

[Maxime Chevallier]

Hi Claudiu,

On Fri,  5 Jan 2024 10:52:42 +0200
Claudiu <claudiu.beznea@xxxxxxxxx> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 
> The RZ/G3S SMARC Module has 2 KSZ9131 PHYs. In this setup, the KSZ9131 PHY
> is used with the ravb Ethernet driver. It has been discovered that when
> bringing the Ethernet interface down/up continuously, e.g., with the
> following sh script:
> 
> $ while :; do ifconfig eth0 down; ifconfig eth0 up; done
> 
> the link speed and duplex are wrong after interrupting the bring down/up
> operation even though the Ethernet interface is up. To recover from this
> state the following configuration sequence is necessary (executed
> manually):
> 
> $ ifconfig eth0 down
> $ ifconfig eth0 up
> 
> The behavior has been identified also on the Microchip SAMA7G5-EK board
> which runs the macb driver and uses the same PHY.
> 
> The order of PHY-related operations in ravb_open() is as follows:
> ravb_open() ->
>   ravb_phy_start() ->
>     ravb_phy_init() ->
>       of_phy_connect() ->
>         phy_connect_direct() ->
> 	  phy_attach_direct() ->
> 	    phy_init_hw() ->
> 	      phydev->drv->soft_reset()
> 	      phydev->drv->config_init()
> 	      phydev->drv->config_intr()
> 	    phy_resume()
> 	      kszphy_resume()
> 
> The order of PHY-related operations in ravb_close is as follows:
> ravb_close() ->
>   phy_stop() ->
>     phy_suspend() ->
>       kszphy_suspend() ->
>         genphy_suspend()
> 	  // set BMCR_PDOWN bit in MII_BMCR
> 
> In genphy_suspend() setting the BMCR_PDWN bit in MII_BMCR switches the PHY
> to Software Power-Down (SPD) mode (according to the KSZ9131 datasheet).
> Thus, when opening the interface after it has been  previously closed (via
> ravb_close()), the phydev->drv->config_init() and
> phydev->drv->config_intr() reach the KSZ9131 PHY driver via the
> ksz9131_config_init() and kszphy_config_intr() functions.
> 
> KSZ9131 specifies that the MII management interface remains operational
> during SPD (Software Power-Down), but (according to manual):
> - Only access to the standard registers (0 through 31) is supported.
> - Access to MMD address spaces other than MMD address space 1 is possible
>   if the spd_clock_gate_override bit is set.
> - Access to MMD address space 1 is not possible.
> 
> The spd_clock_gate_override bit is not used in the KSZ9131 driver.
> 
> ksz9131_config_init() configures RGMII delay, pad skews and LEDs by
> accessesing MMD registers other than those in address space 1.
> 
> The datasheet for the KSZ9131 does not specify what happens if registers
> from an unsupported address space are accessed while the PHY is in SPD.
> 
> To fix the issue the .soft_reset method has been instantiated for KSZ9131,
> too. This resets the PHY to the default state before doing any
> configurations to it, thus switching it out of SPD.
> 
> Fixes: bff5b4b37372 ("net: phy: micrel: add Microchip KSZ9131 initial driver")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> ---
>  drivers/net/phy/micrel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 08e3915001c3..f31f03dd87dd 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -4842,6 +4842,7 @@ static struct phy_driver ksphy_driver[] = {
>  	.flags		= PHY_POLL_CABLE_TEST,
>  	.driver_data	= &ksz9131_type,
>  	.probe		= kszphy_probe,
> +	.soft_reset	= genphy_soft_reset,
>  	.config_init	= ksz9131_config_init,
>  	.config_intr	= kszphy_config_intr,
>  	.config_aneg	= ksz9131_config_aneg,

This looks good to me. Thanks for the detailed analysis,

Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>

Maxime