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

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

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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@bp.renesas.com>
---
 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,
-- 
2.39.2

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

[Russell King (Oracle)]

On Fri, Jan 05, 2024 at 10:52:42AM +0200, Claudiu wrote:
> 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

Andrew,

This looks wrong to me - shouldn't we be resuming the PHY before
attempting to configure it?

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

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

[Andrew Lunn]

On Fri, Jan 05, 2024 at 09:43:22AM +0000, Russell King (Oracle) wrote:
> On Fri, Jan 05, 2024 at 10:52:42AM +0200, Claudiu wrote:
> > 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
> 
> Andrew,
> 
> This looks wrong to me - shouldn't we be resuming the PHY before
> attempting to configure it?

Hummm. The opposite of phy_stop() is phy_start(). So it would be the
logical order to perform the resume as the first action of
phy_start(), not phy_attach_direct().

In phy_connect_direct(), we don't need the PHY to be operational
yet. That happens with phy_start().

The standard says:

  22.2.4.1.5 Power down

  The PHY may be placed in a low-power consumption state by setting
  bit 0.11 to a logic one. Clearing bit 0.11 to zero allows normal
  operation. The specific behavior of a PHY in the power-down state is
  implementation specific. While in the power-down state, the PHY
  shall respond to management transactions.

So i would say this PHY is broken, its not responding to all
management transactions. So in that respect, Claudiu fix is correct.

But i also somewhat agree with you, this looks wrong, but in a
different way to how you see it. However, moving the phy_resume() to
phy_start() seems a bit risky. So i'm not sure we should actually do
that.

	Andrew

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

[Maxime Chevallier]

On Fri, 5 Jan 2024 15:36:29 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Jan 05, 2024 at 09:43:22AM +0000, Russell King (Oracle) wrote:
> > On Fri, Jan 05, 2024 at 10:52:42AM +0200, Claudiu wrote:  
> > > 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  
> > 
> > Andrew,
> > 
> > This looks wrong to me - shouldn't we be resuming the PHY before
> > attempting to configure it?  
> 
> Hummm. The opposite of phy_stop() is phy_start(). So it would be the
> logical order to perform the resume as the first action of
> phy_start(), not phy_attach_direct().
> 
> In phy_connect_direct(), we don't need the PHY to be operational
> yet. That happens with phy_start().
> 
> The standard says:
> 
>   22.2.4.1.5 Power down
> 
>   The PHY may be placed in a low-power consumption state by setting
>   bit 0.11 to a logic one. Clearing bit 0.11 to zero allows normal
>   operation. The specific behavior of a PHY in the power-down state is
>   implementation specific. While in the power-down state, the PHY
>   shall respond to management transactions.
> 
> So i would say this PHY is broken, its not responding to all
> management transactions. So in that respect, Claudiu fix is correct.
> 
> But i also somewhat agree with you, this looks wrong, but in a
> different way to how you see it. However, moving the phy_resume() to
> phy_start() seems a bit risky. So i'm not sure we should actually do
> that.

Looking at other PHYs similar to it like the 9031, the .soft_reset()
was added to fix some similar issues :

Issue :
https://lore.kernel.org/netdev/a63ca542-db96-40ed-201d-59c609f565ce@gmail.com/

Fix :
https://lore.kernel.org/netdev/6d3b1dce-7633-51a1-0556-97cd03304c2c@gmail.com/

We couldn't get a proper explanation back then. Could it be that they
suffer from the same problem, but that it was more clearly documented
for the 9131 ?

Maxime

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

[claudiu beznea]

Hi, Andrew, Russell,

On 05.01.2024 16:36, Andrew Lunn wrote:
> On Fri, Jan 05, 2024 at 09:43:22AM +0000, Russell King (Oracle) wrote:
>> On Fri, Jan 05, 2024 at 10:52:42AM +0200, Claudiu wrote:
>>> 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
>>
>> Andrew,
>>
>> This looks wrong to me - shouldn't we be resuming the PHY before
>> attempting to configure it?
> 
> Hummm. The opposite of phy_stop() is phy_start(). So it would be the
> logical order to perform the resume as the first action of
> phy_start(), not phy_attach_direct().
> 
> In phy_connect_direct(), we don't need the PHY to be operational
> yet. That happens with phy_start().
> 
> The standard says:
> 
>   22.2.4.1.5 Power down
> 
>   The PHY may be placed in a low-power consumption state by setting
>   bit 0.11 to a logic one. Clearing bit 0.11 to zero allows normal
>   operation. The specific behavior of a PHY in the power-down state is
>   implementation specific. While in the power-down state, the PHY
>   shall respond to management transactions.
> 
> So i would say this PHY is broken, its not responding to all
> management transactions. So in that respect, Claudiu fix is correct.
> 
> But i also somewhat agree with you, this looks wrong, but in a
> different way to how you see it. However, moving the phy_resume() to
> phy_start() seems a bit risky. So i'm not sure we should actually do
> that.

It's not clear to me if you both agree with this fix. Could you please let
me know?

Thank you,
Claudiu Beznea

> 
> 	Andrew

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

[Andrew Lunn]

On Wed, Jan 10, 2024 at 03:20:19PM +0200, claudiu beznea wrote:
> Hi, Andrew, Russell,
> 
> On 05.01.2024 16:36, Andrew Lunn wrote:
> > On Fri, Jan 05, 2024 at 09:43:22AM +0000, Russell King (Oracle) wrote:
> >> On Fri, Jan 05, 2024 at 10:52:42AM +0200, Claudiu wrote:
> >>> 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
> >>
> >> Andrew,
> >>
> >> This looks wrong to me - shouldn't we be resuming the PHY before
> >> attempting to configure it?
> > 
> > Hummm. The opposite of phy_stop() is phy_start(). So it would be the
> > logical order to perform the resume as the first action of
> > phy_start(), not phy_attach_direct().
> > 
> > In phy_connect_direct(), we don't need the PHY to be operational
> > yet. That happens with phy_start().
> > 
> > The standard says:
> > 
> >   22.2.4.1.5 Power down
> > 
> >   The PHY may be placed in a low-power consumption state by setting
> >   bit 0.11 to a logic one. Clearing bit 0.11 to zero allows normal
> >   operation. The specific behavior of a PHY in the power-down state is
> >   implementation specific. While in the power-down state, the PHY
> >   shall respond to management transactions.
> > 
> > So i would say this PHY is broken, its not responding to all
> > management transactions. So in that respect, Claudiu fix is correct.
> > 
> > But i also somewhat agree with you, this looks wrong, but in a
> > different way to how you see it. However, moving the phy_resume() to
> > phy_start() seems a bit risky. So i'm not sure we should actually do
> > that.
> 
> It's not clear to me if you both agree with this fix. Could you please let
> me know?

Hi Claudiu

I think this is a valid workaround for the broken hardware.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

There might be further discussion about if suspend and resume are
being done at the correct time, but i think that is orthogonal.

    Andrew

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@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> 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@bp.renesas.com>
> ---
>  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@bootlin.com>

Maxime

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri,  5 Jan 2024 10:52:42 +0200 you wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> 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:
> 
> [...]

Here is the summary with links:
  - [net] net: phy: micrel: populate .soft_reset for KSZ9131
    https://git.kernel.org/netdev/net/c/e398822c4751

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html