| Message ID | 20231213000249.2020835-1-justin.chen@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mdio: mdio-bcm-unimac: Delay before first poll | expand |
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!
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
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.
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.
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
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))
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(-)