Date:   Tue, 10 Sep 2019 12:07:53 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "mkubecek@...e.cz" <mkubecek@...e.cz>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
Subject: Re: [PATCH v3 1/2] ethtool: implement Energy Detect Powerdown support
 via phy-tunable

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 <alexandru.ardelean@...log.com>

Powered by blists - more mailing lists