[PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Andreas Svensson]

A switch held in reset by default needs to wait longer until we can
reliably detect it.

An issue was observed when testing on the Marvell 88E6393X (Link Street).
The driver failed to detect the switch on some upstarts. Increasing the
wait time after reset deactivation solves this issue.

The updated wait time is now also the same as the wait time in the
mv88e6xxx_hardware_reset function.

Fixes: 7b75e49de424 ("net: dsa: mv88e6xxx: wait after reset deactivation")
Signed-off-by: Andreas Svensson <andreas.svensson@axis.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64a2f2f83735..08a46ffd53af 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -7170,7 +7170,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 		goto out;
 	}
 	if (chip->reset)
-		usleep_range(1000, 2000);
+		usleep_range(10000, 20000);
 
 	/* Detect if the device is configured in single chip addressing mode,
 	 * otherwise continue with address specific smi init/detection.
-- 
2.30.2

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Andrew Lunn]

On Tue, May 30, 2023 at 04:52:23PM +0200, Andreas Svensson wrote:
> A switch held in reset by default needs to wait longer until we can
> reliably detect it.
> 
> An issue was observed when testing on the Marvell 88E6393X (Link Street).
> The driver failed to detect the switch on some upstarts. Increasing the
> wait time after reset deactivation solves this issue.
> 
> The updated wait time is now also the same as the wait time in the
> mv88e6xxx_hardware_reset function.

Do you have an EEPROM attached and content in it?

It is not necessarily the reset itself which is the problem, but how
long it takes after the reset to read the contents of the
EEPROM. While it is doing that, is does not respond on the MDIO
bus. Which is why mv88e6xxx_hardware_reset() polls for that to
complete.

I know there are some users who want the switch to boot as fast as
possible, and don't really want the additional 9ms delay. But this is
also a legitimate change. I'm just wondering if we need to consider a
DT property here for those with EEPROM content. Or, if there is an
interrupt line, wait for the EEPROM complete interrupt. We just have
tricky chicken and egg problems. At this point in time, we don't
actually know if the devices exists or not.

	  Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Andreas Svensson]

On 5/30/23 19:28, Andrew Lunn wrote:
> On Tue, May 30, 2023 at 04:52:23PM +0200, Andreas Svensson wrote:
>> A switch held in reset by default needs to wait longer until we can
>> reliably detect it.
>>
>> An issue was observed when testing on the Marvell 88E6393X (Link Street).
>> The driver failed to detect the switch on some upstarts. Increasing the
>> wait time after reset deactivation solves this issue.
>>
>> The updated wait time is now also the same as the wait time in the
>> mv88e6xxx_hardware_reset function.
> 
> Do you have an EEPROM attached and content in it?

There's no EEPROM attached to the switch in our design.

> 
> It is not necessarily the reset itself which is the problem, but how
> long it takes after the reset to read the contents of the
> EEPROM. While it is doing that, is does not respond on the MDIO
> bus. Which is why mv88e6xxx_hardware_reset() polls for that to
> complete.

Ok, yes that makes sense. I could add the mv88e6xxx_g1_wait_eeprom_done
function after the reset deactivation.

> 
> I know there are some users who want the switch to boot as fast as
> possible, and don't really want the additional 9ms delay. But this is
> also a legitimate change. I'm just wondering if we need to consider a
> DT property here for those with EEPROM content. Or, if there is an
> interrupt line, wait for the EEPROM complete interrupt. We just have
> tricky chicken and egg problems. At this point in time, we don't
> actually know if the devices exists or not.
> 
> 	  Andrew

It just seems like we need to wait longer for the switch 88E6393X
until it responds reliably on the MDIO bus. But I'm open to adding
a new DT property if that's needed.

The datasheet for 88E6393X also states that it needs at least 10ms
before it's ready. But I suppose this varies from switch to switch.

Best Regards,
Andreas Svensson

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Paolo Abeni]

On Thu, 2023-06-01 at 11:10 +0200, Andreas Svensson wrote:
> On 5/30/23 19:28, Andrew Lunn wrote:
> > On Tue, May 30, 2023 at 04:52:23PM +0200, Andreas Svensson wrote:
> > > A switch held in reset by default needs to wait longer until we can
> > > reliably detect it.
> > > 
> > > An issue was observed when testing on the Marvell 88E6393X (Link Street).
> > > The driver failed to detect the switch on some upstarts. Increasing the
> > > wait time after reset deactivation solves this issue.
> > > 
> > > The updated wait time is now also the same as the wait time in the
> > > mv88e6xxx_hardware_reset function.
> > 
> > Do you have an EEPROM attached and content in it?
> 
> There's no EEPROM attached to the switch in our design.
> 
> > 
> > It is not necessarily the reset itself which is the problem, but how
> > long it takes after the reset to read the contents of the
> > EEPROM. While it is doing that, is does not respond on the MDIO
> > bus. Which is why mv88e6xxx_hardware_reset() polls for that to
> > complete.
> 
> Ok, yes that makes sense. I could add the mv88e6xxx_g1_wait_eeprom_done
> function after the reset deactivation.
> 
> > 
> > I know there are some users who want the switch to boot as fast as
> > possible, and don't really want the additional 9ms delay. But this is
> > also a legitimate change. I'm just wondering if we need to consider a
> > DT property here for those with EEPROM content. Or, if there is an
> > interrupt line, wait for the EEPROM complete interrupt. We just have
> > tricky chicken and egg problems. At this point in time, we don't
> > actually know if the devices exists or not.
> > 
> > 	  Andrew
> 
> It just seems like we need to wait longer for the switch 88E6393X
> until it responds reliably on the MDIO bus. But I'm open to adding
> a new DT property if that's needed.
> 
> The datasheet for 88E6393X also states that it needs at least 10ms
> before it's ready. But I suppose this varies from switch to switch.

I read the above as a new version of this fix is coming, thus not
applying this patch.

Thanks,

Paolo

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Andrew Lunn]

 On Thu, Jun 01, 2023 at 11:10:58AM +0200, Andreas Svensson wrote:
> On 5/30/23 19:28, Andrew Lunn wrote:
> > On Tue, May 30, 2023 at 04:52:23PM +0200, Andreas Svensson wrote:
> > > A switch held in reset by default needs to wait longer until we can
> > > reliably detect it.
> > > 
> > > An issue was observed when testing on the Marvell 88E6393X (Link Street).
> > > The driver failed to detect the switch on some upstarts. Increasing the
> > > wait time after reset deactivation solves this issue.
> > > 
> > > The updated wait time is now also the same as the wait time in the
> > > mv88e6xxx_hardware_reset function.
> > 
> > Do you have an EEPROM attached and content in it?
> 
> There's no EEPROM attached to the switch in our design.
> 
> > 
> > It is not necessarily the reset itself which is the problem, but how
> > long it takes after the reset to read the contents of the
> > EEPROM. While it is doing that, is does not respond on the MDIO
> > bus. Which is why mv88e6xxx_hardware_reset() polls for that to
> > complete.
> 
> Ok, yes that makes sense. I could add the mv88e6xxx_g1_wait_eeprom_done
> function after the reset deactivation.

I don't think that works, because how to talk to the switch is not
determined until after the switch has been detected.

> The datasheet for 88E6393X also states that it needs at least 10ms
> before it's ready. But I suppose this varies from switch to switch.

O.K, let go with this change and see if anybody really complains. We
can always add a DT property later.

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

You probably need to repost with my Reviewed-by added, now that Paolo
has changed the status of the patch.

    Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[Paolo Abeni]

On Thu, 2023-06-01 at 14:01 +0200, Andrew Lunn wrote:
>  On Thu, Jun 01, 2023 at 11:10:58AM +0200, Andreas Svensson wrote:
> > On 5/30/23 19:28, Andrew Lunn wrote:
> > > On Tue, May 30, 2023 at 04:52:23PM +0200, Andreas Svensson wrote:
> > > > A switch held in reset by default needs to wait longer until we can
> > > > reliably detect it.
> > > > 
> > > > An issue was observed when testing on the Marvell 88E6393X (Link Street).
> > > > The driver failed to detect the switch on some upstarts. Increasing the
> > > > wait time after reset deactivation solves this issue.
> > > > 
> > > > The updated wait time is now also the same as the wait time in the
> > > > mv88e6xxx_hardware_reset function.
> > > 
> > > Do you have an EEPROM attached and content in it?
> > 
> > There's no EEPROM attached to the switch in our design.
> > 
> > > 
> > > It is not necessarily the reset itself which is the problem, but how
> > > long it takes after the reset to read the contents of the
> > > EEPROM. While it is doing that, is does not respond on the MDIO
> > > bus. Which is why mv88e6xxx_hardware_reset() polls for that to
> > > complete.
> > 
> > Ok, yes that makes sense. I could add the mv88e6xxx_g1_wait_eeprom_done
> > function after the reset deactivation.
> 
> I don't think that works, because how to talk to the switch is not
> determined until after the switch has been detected.
> 
> > The datasheet for 88E6393X also states that it needs at least 10ms
> > before it's ready. But I suppose this varies from switch to switch.
> 
> O.K, let go with this change and see if anybody really complains. We
> can always add a DT property later.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> You probably need to repost with my Reviewed-by added, now that Paolo
> has changed the status of the patch.

Not needed. I can restore the patch in PW.

Thanks,

Paolo

Re: [PATCH net] net: dsa: mv88e6xxx: Increase wait after reset deactivation

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 30 May 2023 16:52:23 +0200 you wrote:
> A switch held in reset by default needs to wait longer until we can
> reliably detect it.
> 
> An issue was observed when testing on the Marvell 88E6393X (Link Street).
> The driver failed to detect the switch on some upstarts. Increasing the
> wait time after reset deactivation solves this issue.
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: mv88e6xxx: Increase wait after reset deactivation
    https://git.kernel.org/netdev/net/c/3c27f3d53d58

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