On Mon, Sep 09, 2019 at 04:12:50PM +0300, Alexandru Ardelean wrote:
> The `phy_tunable_id` has been named `ETHTOOL_PHY_EDPD` since it looks like
> this feature is common across other PHYs (like EEE), and defining
> `ETHTOOL_PHY_ENERGY_DETECT_POWER_DOWN` seems too long.
>
> The way EDPD works, is that the RX block is put to a lower power mode,
> except for link-pulse detection circuits. The TX block is also put to low
> power mode, but the PHY wakes-up periodically to send link pulses, to avoid
> lock-ups in case the other side is also in EDPD mode.
>
> Currently, there are 2 PHY drivers that look like they could use this new
> PHY tunable feature: the `adin` && `micrel` PHYs.
>
> The ADIN's datasheet mentions that TX pulses are at intervals of 1 second
> default each, and they can be disabled. For the Micrel KSZ9031 PHY, the
> datasheet does not mention whether they can be disabled, but mentions that
> they can modified.
>
> The way this change is structured, is similar to the PHY tunable downshift
> control:
> * a `ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL` value is exposed to cover a default
> TX interval; some PHYs could specify a certain value that makes sense
> * `ETHTOOL_PHY_EDPD_NO_TX` would disable TX when EDPD is enabled
> * `ETHTOOL_PHY_EDPD_DISABLE` will disable EDPD
>
> This should allow PHYs to:
> * enable EDPD and not enable TX pulses (interval would be 0)
> * enable EDPD and configure TX pulse interval; note that TX interval units
> would be PHY specific; we could consider `seconds` as units, but it could
> happen that some PHYs would be prefer milliseconds as a unit;
> a maximum of 65533 units should be sufficient
Sorry for missing the discussion on previous version but I don't really
like the idea of leaving the choice of units to PHY. Both for manual
setting and system configuration, it would be IMHO much more convenient
to have the interpretation universal for all NICs.
Seconds as units seem too coarse and maximum of ~18 hours way too big.
Milliseconds would be more practical from granularity point of view,
would maximum of ~65 seconds be sufficient?
Michal Kubecek
> * disable EDPD
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
On Tue, 2019-09-10 at 10:00 +0200, Michal Kubecek wrote:
> [External]
>
> On Mon, Sep 09, 2019 at 04:12:50PM +0300, Alexandru Ardelean wrote:
> > The `phy_tunable_id` has been named `ETHTOOL_PHY_EDPD` since it looks like
> > this feature is common across other PHYs (like EEE), and defining
> > `ETHTOOL_PHY_ENERGY_DETECT_POWER_DOWN` seems too long.
> >
> > The way EDPD works, is that the RX block is put to a lower power mode,
> > except for link-pulse detection circuits. The TX block is also put to low
> > power mode, but the PHY wakes-up periodically to send link pulses, to avoid
> > lock-ups in case the other side is also in EDPD mode.
> >
> > Currently, there are 2 PHY drivers that look like they could use this new
> > PHY tunable feature: the `adin` && `micrel` PHYs.
> >
> > The ADIN's datasheet mentions that TX pulses are at intervals of 1 second
> > default each, and they can be disabled. For the Micrel KSZ9031 PHY, the
> > datasheet does not mention whether they can be disabled, but mentions that
> > they can modified.
> >
> > The way this change is structured, is similar to the PHY tunable downshift
> > control:
> > * a `ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL` value is exposed to cover a default
> > TX interval; some PHYs could specify a certain value that makes sense
> > * `ETHTOOL_PHY_EDPD_NO_TX` would disable TX when EDPD is enabled
> > * `ETHTOOL_PHY_EDPD_DISABLE` will disable EDPD
> >
> > This should allow PHYs to:
> > * enable EDPD and not enable TX pulses (interval would be 0)
> > * enable EDPD and configure TX pulse interval; note that TX interval units
> > would be PHY specific; we could consider `seconds` as units, but it could
> > happen that some PHYs would be prefer milliseconds as a unit;
> > a maximum of 65533 units should be sufficient
>
> Sorry for missing the discussion on previous version but I don't really
> like the idea of leaving the choice of units to PHY. Both for manual
> setting and system configuration, it would be IMHO much more convenient
> to have the interpretation universal for all NICs.
>
I was also a bit uncertain/undecided here about letting PHYs decide units.
And I also wasn't sure what to propose.
Not proposing a unit now, would have allowed us to decide later based on what PHYs implement (generally) in the future.
I know that may make me look a bit lazy [for not proposing units in ethtool], but tbh it's more of lack-of-experience
with ethtool (or these APIs) evolve over time.
> Seconds as units seem too coarse and maximum of ~18 hours way too big.
> Milliseconds would be more practical from granularity point of view,
> would maximum of ~65 seconds be sufficient?
I think 65 seconds is more than enough.
I feel that, if you plug-in an eth cable and don't have a link in a minute, then it's not great (no matter how much of a
power-saver you are).
Actually, voicing out these units makes it simpler for a decision to go in favor for milliseconds.
So: thank you :)
Alex
>
> Michal Kubecek
>
> > * disable EDPD
> >
> > Signed-off-by: Alexandru Ardelean <[email protected]>