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 <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
Thanks!
--
Florian
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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
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