[PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

With a clock interval of 400 nsec and a 64 bit transactions (32 bit
preamble & 16 bit control & 16 bit data), it is reasonable to assume
the mdio transaction will take 25.6 usec. Add a 30 usec delay before
the first poll to reduce the chance of a 1000-2000 usec sleep.

Reduce the timeout from 1000ms to 100ms as it is unlikely for the bus
to take this long.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/mdio/mdio-bcm-unimac.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
index e8cd8eef319b..1f89b1bdb90f 100644
--- a/drivers/net/mdio/mdio-bcm-unimac.c
+++ b/drivers/net/mdio/mdio-bcm-unimac.c
@@ -81,7 +81,9 @@ static inline unsigned int unimac_mdio_busy(struct unimac_mdio_priv *priv)
 static int unimac_mdio_poll(void *wait_func_data)
 {
 	struct unimac_mdio_priv *priv = wait_func_data;
-	unsigned int timeout = 1000;
+	unsigned int timeout = 100;
+
+	udelay(30);
 
 	do {
 		if (!unimac_mdio_busy(priv))
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[Florian Fainelli]

On 12/12/23 16:02, Justin Chen wrote:
> With a clock interval of 400 nsec and a 64 bit transactions (32 bit
> preamble & 16 bit control & 16 bit data), it is reasonable to assume
> the mdio transaction will take 25.6 usec. Add a 30 usec delay before
> the first poll to reduce the chance of a 1000-2000 usec sleep.
> 
> Reduce the timeout from 1000ms to 100ms as it is unlikely for the bus
> to take this long.
> 
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>

Thanks!
-- 
Florian

Re: [PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[Andrew Lunn]

On Tue, Dec 12, 2023 at 04:02:49PM -0800, Justin Chen wrote:
> With a clock interval of 400 nsec and a 64 bit transactions (32 bit
> preamble & 16 bit control & 16 bit data), it is reasonable to assume
> the mdio transaction will take 25.6 usec. Add a 30 usec delay before
> the first poll to reduce the chance of a 1000-2000 usec sleep.

#define  MDIO_C45               0

suggests the hardware can do C45? The timing works out different then.
Maybe add a comment by the udelay() that is assumes C22, to give a
clue to somebody who is adding C45 support the delay needs to be
re-evaluated.

	Andrew

Re: [PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[Russell King (Oracle)]

On Wed, Dec 13, 2023 at 11:57:52AM +0100, Andrew Lunn wrote:
> On Tue, Dec 12, 2023 at 04:02:49PM -0800, Justin Chen wrote:
> > With a clock interval of 400 nsec and a 64 bit transactions (32 bit
> > preamble & 16 bit control & 16 bit data), it is reasonable to assume
> > the mdio transaction will take 25.6 usec. Add a 30 usec delay before
> > the first poll to reduce the chance of a 1000-2000 usec sleep.
> 
> #define  MDIO_C45               0
> 
> suggests the hardware can do C45? The timing works out different then.
> Maybe add a comment by the udelay() that is assumes C22, to give a
> clue to somebody who is adding C45 support the delay needs to be
> re-evaluated.

Note, however, that the driver only supports C22 operations (it only
populates the read|write functions, not the c45 variants).

However, it doesn't explicitly set the MDIO_C22 bit in the configuration
register, so what ends up being spat out on the bus would be dependent
on the boot loader configuration.

However, I'm wondering why unimac_mdio_poll() isn't written as
(based on current code):

	return read_poll_timeout(unimac_mdio_readl(priv, MDIO_CMD), val,
				 !(val & MDIO_START_BUSY), 2000,
				 2000000);

rather than open-coding the io polling.

-- 
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: mdio: mdio-bcm-unimac: Delay before first poll

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]



On 12/13/2023 7:01 AM, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 11:57:52AM +0100, Andrew Lunn wrote:
>> On Tue, Dec 12, 2023 at 04:02:49PM -0800, Justin Chen wrote:
>>> With a clock interval of 400 nsec and a 64 bit transactions (32 bit
>>> preamble & 16 bit control & 16 bit data), it is reasonable to assume
>>> the mdio transaction will take 25.6 usec. Add a 30 usec delay before
>>> the first poll to reduce the chance of a 1000-2000 usec sleep.
>>
>> #define  MDIO_C45               0
>>
>> suggests the hardware can do C45? The timing works out different then.
>> Maybe add a comment by the udelay() that is assumes C22, to give a
>> clue to somebody who is adding C45 support the delay needs to be
>> re-evaluated.

Yes the hardware supports C45 though as Russell points out, the driver 
intentionally does not support it.

> 
> Note, however, that the driver only supports C22 operations (it only
> populates the read|write functions, not the c45 variants).
> 
> However, it doesn't explicitly set the MDIO_C22 bit in the configuration
> register, so what ends up being spat out on the bus would be dependent
> on the boot loader configuration.

The hardware is guaranteed to come up with the MDIO_C22 bit being set by 
default though it would not hurt setting it explicitly, that would be 
more future proof.

> 
> However, I'm wondering why unimac_mdio_poll() isn't written as
> (based on current code):
> 
> 	return read_poll_timeout(unimac_mdio_readl(priv, MDIO_CMD), val,
> 				 !(val & MDIO_START_BUSY), 2000,
> 				 2000000);
> 
> rather than open-coding the io polling.

The driver long predates the introduction of the iopoll.h header and its 
routines. That sounds like another change that could be made.
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] net: mdio: mdio-bcm-unimac: Delay before first poll

[Andrew Lunn]

On Wed, Dec 13, 2023 at 03:01:09PM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 11:57:52AM +0100, Andrew Lunn wrote:
> > On Tue, Dec 12, 2023 at 04:02:49PM -0800, Justin Chen wrote:
> > > With a clock interval of 400 nsec and a 64 bit transactions (32 bit
> > > preamble & 16 bit control & 16 bit data), it is reasonable to assume
> > > the mdio transaction will take 25.6 usec. Add a 30 usec delay before
> > > the first poll to reduce the chance of a 1000-2000 usec sleep.
> > 
> > #define  MDIO_C45               0
> > 
> > suggests the hardware can do C45? The timing works out different then.
> > Maybe add a comment by the udelay() that is assumes C22, to give a
> > clue to somebody who is adding C45 support the delay needs to be
> > re-evaluated.
> 
> Note, however, that the driver only supports C22 operations (it only
> populates the read|write functions, not the c45 variants).

Yes, i checked that. Which is why i used the wording 'a clue to
somebody who is adding C45'. Not everybody adding such support would
figure out the relevance of 30us and that it might not be optimal for
C45. A comment might point them on the right line of thinking. That is
all i was trying to say.

    Andrew