RE: [PATCH net-next] net: phy: microchip_t1: SQI support for LAN887x

[Tarun.Alle]

Hi Andrew,
Thanks for your review and comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Friday, September 6, 2024 6:28 PM
> To: Tarun Alle - I68638 <Tarun.Alle@xxxxxxxxxxxxx>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; hkallweit1@xxxxxxxxx;
> linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: phy: microchip_t1: SQI support for LAN887x
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > > How long does this take?
> > >
> >
> > ~76ms
> 
> That is faster than i expected. You have a pretty efficient MDIO bus
> implementation.
> 
> > > genphy_c45_read_link() takes a few MDIO transaction, plus the two
> > > you see here. So maybe 1000 MDIO bus transactions? Which could be
> > > 3000-4000 if it needs to use C45 over C22.
> > >
> > > Do you have any data on the accuracy, with say 10, 20, 40, 80, 160 samples?
> > >
> >
> > Here number of samples are suggested by our compliance test data.
> > There is an APP Note regarding SQI samples and calculations.
> > No, the number of samples are only 200 as any other count was not
> > consistent in terms of accuracy.
> >
> > > Can the genphy_c45_read_link() be moved out of the loop? If the link
> > > is lost, is the sample totally random, or does it have a well
> > > defined value? Looking at the link status every iteration, rather
> > > than before and after collecting the samples, you are trying to
> > > protect against the link going down and back up again. If it is
> > > taking a couple of seconds to collect all the samples, i suppose that is possible,
> but if its 50ms, do you really have to worry?
> > >
> >
> >
> > Sampling data is random. If the link is down at any point during the
> > data sampling we are discarding the entire set.
> > If we check the link status before and after the data collection,
> > there could be an invalidate SQI derivation in very worst-case scenario.
> >
> > Just to improve instead of register read can I change it to use phydev->link
> variable?
> > This link variable is update by PHY state machine.
> 
> Which won't get to run because the driver is actively doing SQI. There is no
> preemption here, this code will run to completion, and then phylib will deal with
> any interrupts for link down, or do its once per second poll to check the link
> status.
> 
> With this only taking 76ms, what is the likelihood of link down and link up again
> within 76ms? For a 1000BaseT PHY, they don't report link down for 1 second, and
> it takes another 1 second to perform autoneg before the link is up again. Now this
> is an automotive PHY, so the timing is different. What does the data sheet say
> about how fast it detects and reports link down and up?
> 

For 1000M this sampling procedure will not be run rather we use SQI hardware register to read the value.
as this procedure is only for 100M and linkup time is ~100ms we can check link status before starting the sampling and after 
completing the sampling. This would ensure that link is not down before calculating SQI.

>         Andrew

Thanks,
Tarun Alle.