[PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

Add the ability to query and set Energy Efficient Ethernet parameters
via ethtool for applicable devices.

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
Changes:
	v2: make routines static (Rami Rosen)

 drivers/net/ethernet/ti/cpsw.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d..c706540 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2239,6 +2239,30 @@ static int cpsw_set_channels(struct net_device *ndev,
 	return ret;
 }
 
+static int cpsw_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	struct cpsw_common *cpsw = priv->cpsw;
+	int slave_no = cpsw_slave_index(cpsw, priv);
+
+	if (cpsw->slaves[slave_no].phy)
+		return phy_ethtool_get_eee(cpsw->slaves[slave_no].phy, edata);
+	else
+		return -EOPNOTSUPP;
+}
+
+static int cpsw_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	struct cpsw_common *cpsw = priv->cpsw;
+	int slave_no = cpsw_slave_index(cpsw, priv);
+
+	if (cpsw->slaves[slave_no].phy)
+		return phy_ethtool_set_eee(cpsw->slaves[slave_no].phy, edata);
+	else
+		return -EOPNOTSUPP;
+}
+
 static const struct ethtool_ops cpsw_ethtool_ops = {
 	.get_drvinfo	= cpsw_get_drvinfo,
 	.get_msglevel	= cpsw_get_msglevel,
@@ -2262,6 +2286,8 @@ static const struct ethtool_ops cpsw_ethtool_ops = {
 	.complete	= cpsw_ethtool_op_complete,
 	.get_channels	= cpsw_get_channels,
 	.set_channels	= cpsw_set_channels,
+	.get_eee	= cpsw_get_eee,
+	.set_eee	= cpsw_set_eee,
 };
 
 static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_common *cpsw,
-- 
2.1.4

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Rami Rosen]

Acked-by: Rami Rosen <roszenrami@gmail.com>

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

On 11/23/2016 06:38 AM, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> Add the ability to query and set Energy Efficient Ethernet parameters
> via ethtool for applicable devices.

Are you sure this is enough to actually enable EEE? I don't see where
phy_init_eee() is called here, nor is the cpsw Ethernet controller part
configured to enable/disable EEE. EEE is not just a PHY thing, it
usually also needs to be configured properly at the Ethernet MAC/switch
level as well.

Just curious here.
-- 
Florian

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Yegor Yefremov]

On Wed, Nov 23, 2016 at 6:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/23/2016 06:38 AM, yegorslists@googlemail.com wrote:
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>
>> Add the ability to query and set Energy Efficient Ethernet parameters
>> via ethtool for applicable devices.
>
> Are you sure this is enough to actually enable EEE? I don't see where
> phy_init_eee() is called here, nor is the cpsw Ethernet controller part
> configured to enable/disable EEE. EEE is not just a PHY thing, it
> usually also needs to be configured properly at the Ethernet MAC/switch
> level as well.
>
> Just curious here.

I'm sure I want to disable EEE :-) So I need this patch in order to
check and disable EEE advertising.

Yegor

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

On 11/23/2016 12:08 PM, Yegor Yefremov wrote:
> On Wed, Nov 23, 2016 at 6:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 11/23/2016 06:38 AM, yegorslists@googlemail.com wrote:
>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>
>>> Add the ability to query and set Energy Efficient Ethernet parameters
>>> via ethtool for applicable devices.
>>
>> Are you sure this is enough to actually enable EEE? I don't see where
>> phy_init_eee() is called here, nor is the cpsw Ethernet controller part
>> configured to enable/disable EEE. EEE is not just a PHY thing, it
>> usually also needs to be configured properly at the Ethernet MAC/switch
>> level as well.
>>
>> Just curious here.
> 
> I'm sure I want to disable EEE :-) So I need this patch in order to
> check and disable EEE advertising.

OK, so you need this to disable EEE advertisement, which is great, but
this also allows you to enable EEE, is it enough to just advertise EEE
with your link partner for cpsw to work correctly? Just wondering, since
your commit message is more than short.
-- 
Florian

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Yegor Yefremov]

On Wed, Nov 23, 2016 at 9:15 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/23/2016 12:08 PM, Yegor Yefremov wrote:
>> On Wed, Nov 23, 2016 at 6:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 11/23/2016 06:38 AM, yegorslists@googlemail.com wrote:
>>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>>
>>>> Add the ability to query and set Energy Efficient Ethernet parameters
>>>> via ethtool for applicable devices.
>>>
>>> Are you sure this is enough to actually enable EEE? I don't see where
>>> phy_init_eee() is called here, nor is the cpsw Ethernet controller part
>>> configured to enable/disable EEE. EEE is not just a PHY thing, it
>>> usually also needs to be configured properly at the Ethernet MAC/switch
>>> level as well.
>>>
>>> Just curious here.
>>
>> I'm sure I want to disable EEE :-) So I need this patch in order to
>> check and disable EEE advertising.
>
> OK, so you need this to disable EEE advertisement, which is great, but
> this also allows you to enable EEE, is it enough to just advertise EEE
> with your link partner for cpsw to work correctly? Just wondering, since
> your commit message is more than short.

I've sent v3 with a little bit more info. Basically this is needed for
this kind of issues [1]

As for enabling advertising and correct working of cpsw do you mean it
would be better to disable EEE in any PHY on cpsw initialization as
long as cpsw doesn't provide support for EEE?

We observe some strange behavior with our gigabit PHYs and a link
partner in a EEE-capable unmanaged NetGear switch. Disabling
advertising seems to help. Though we're still investigating the issue.

[1] http://www.spinics.net/lists/netdev/msg405396.html

Yegor

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Andrew Lunn]

> As for enabling advertising and correct working of cpsw do you mean it
> would be better to disable EEE in any PHY on cpsw initialization as
> long as cpsw doesn't provide support for EEE?
> 
> We observe some strange behavior with our gigabit PHYs and a link
> partner in a EEE-capable unmanaged NetGear switch. Disabling
> advertising seems to help. Though we're still investigating the issue.

Hi Florian

Am i right in saying, a PHY should not advertise EEE until the MAC
driver calls phy_init_eee(), indicating the MAC supports EEE?

If so, it looks like we need to change a few of the PHY drivers, in
particular, the bcm-*.c.

    Andrew

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

+Peppe,

Le 24/11/2016 à 07:38, Andrew Lunn a écrit :
>> As for enabling advertising and correct working of cpsw do you mean it
>> would be better to disable EEE in any PHY on cpsw initialization as
>> long as cpsw doesn't provide support for EEE?
>>
>> We observe some strange behavior with our gigabit PHYs and a link
>> partner in a EEE-capable unmanaged NetGear switch. Disabling
>> advertising seems to help. Though we're still investigating the issue.
> 
> Hi Florian
> 
> Am i right in saying, a PHY should not advertise EEE until the MAC
> driver calls phy_init_eee(), indicating the MAC supports EEE?

You would think so, but I don't see how this could possibly work if that
was not the case already, see below.

> 
> If so, it looks like we need to change a few of the PHY drivers, in
> particular, the bcm-*.c.

The first part that bcm-phy-lib.c does is make sure that EEE is enabled
such that this gets reflected in MDIO_PCS_EEE_ABLE, without this, we
won't be able to pass the first test in phy_init_eee(). The second part
is to advertise EEE such that this gets reflected in MDIO_AN_EEE_ADV,
also to make sure that we can pass the second check in phy_init_eee().

Now, looking at phy_init_eee(), and what stmmac does (and bcmgenet,
copied after stmmac), we need to somehow, have EEE advertised for
phy_init_eee() to succeed, prepare the MAC to support EEE, and finally
conclude with a call to phy_ethtool_set_eee(), which writes to the
MDIO_AN_EEE_ADV register, and concludes the EEE auto-negotiated process.
Since we already have EEE advertised, we are essentially just checking
that the EEE advertised settings and the LP advertised settings actually
do match, so it sounds like the final call to phy_ethtool_set_eee() is
potentially useless if the resolved advertised and link partner
advertised settings already match...

So it sounds like at least, the first time you try to initialize EEE, we
should start with EEE not advertised, and then, if we have EEE enabled
at some point, and we re-negotiate the link parameters, somehow
phy_init_eee() does a right job for that.

Peppe, any thoughts on this?
-- 
Florian

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Giuseppe CAVALLARO]

Hi Florian
sorry for my delay.

On 11/24/2016 7:23 PM, Florian Fainelli wrote:
> +Peppe,
>
> Le 24/11/2016 à 07:38, Andrew Lunn a écrit :
>>> As for enabling advertising and correct working of cpsw do you mean it
>>> would be better to disable EEE in any PHY on cpsw initialization as
>>> long as cpsw doesn't provide support for EEE?
>>>
>>> We observe some strange behavior with our gigabit PHYs and a link
>>> partner in a EEE-capable unmanaged NetGear switch. Disabling
>>> advertising seems to help. Though we're still investigating the issue.
>>
>> Hi Florian
>>
>> Am i right in saying, a PHY should not advertise EEE until the MAC
>> driver calls phy_init_eee(), indicating the MAC supports EEE?
>
> You would think so, but I don't see how this could possibly work if that
> was not the case already, see below.
>
>>
>> If so, it looks like we need to change a few of the PHY drivers, in
>> particular, the bcm-*.c.
>
> The first part that bcm-phy-lib.c does is make sure that EEE is enabled
> such that this gets reflected in MDIO_PCS_EEE_ABLE, without this, we
> won't be able to pass the first test in phy_init_eee(). The second part
> is to advertise EEE such that this gets reflected in MDIO_AN_EEE_ADV,
> also to make sure that we can pass the second check in phy_init_eee().
>
> Now, looking at phy_init_eee(), and what stmmac does (and bcmgenet,
> copied after stmmac), we need to somehow, have EEE advertised for
> phy_init_eee() to succeed, prepare the MAC to support EEE, and finally
> conclude with a call to phy_ethtool_set_eee(), which writes to the
> MDIO_AN_EEE_ADV register, and concludes the EEE auto-negotiated process.
> Since we already have EEE advertised, we are essentially just checking
> that the EEE advertised settings and the LP advertised settings actually
> do match, so it sounds like the final call to phy_ethtool_set_eee() is
> potentially useless if the resolved advertised and link partner
> advertised settings already match...
>
> So it sounds like at least, the first time you try to initialize EEE, we
> should start with EEE not advertised, and then, if we have EEE enabled
> at some point, and we re-negotiate the link parameters, somehow
> phy_init_eee() does a right job for that.
>
> Peppe, any thoughts on this?

I share what you say.

In sum, the EEE management inside the stmmac is:

- the driver looks at own HW cap register if EEE is supported

     (indeed the user could keep disable EEE if bugged on some HW
      + Alex, Fabrice: we had some patches for this to propose where we
              called the phy_ethtool_set_eee to disable feature at phy
              level

- then the stmmac asks PHY layer to understand if transceiver and
   partners are EEE capable.

- If all matches the EEE is actually initialized.

the logic above should be respected when use ethtool, hmm, I will
check the stmmac_ethtool_op_set_eee asap.

Hoping this is useful

Regards
Peppe

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

On 12/02/2016 01:11 AM, Giuseppe CAVALLARO wrote:
> Hi Florian
> sorry for my delay.
> 
> On 11/24/2016 7:23 PM, Florian Fainelli wrote:
>> +Peppe,
>>
>> Le 24/11/2016 à 07:38, Andrew Lunn a écrit :
>>>> As for enabling advertising and correct working of cpsw do you mean it
>>>> would be better to disable EEE in any PHY on cpsw initialization as
>>>> long as cpsw doesn't provide support for EEE?
>>>>
>>>> We observe some strange behavior with our gigabit PHYs and a link
>>>> partner in a EEE-capable unmanaged NetGear switch. Disabling
>>>> advertising seems to help. Though we're still investigating the issue.
>>>
>>> Hi Florian
>>>
>>> Am i right in saying, a PHY should not advertise EEE until the MAC
>>> driver calls phy_init_eee(), indicating the MAC supports EEE?
>>
>> You would think so, but I don't see how this could possibly work if that
>> was not the case already, see below.
>>
>>>
>>> If so, it looks like we need to change a few of the PHY drivers, in
>>> particular, the bcm-*.c.
>>
>> The first part that bcm-phy-lib.c does is make sure that EEE is enabled
>> such that this gets reflected in MDIO_PCS_EEE_ABLE, without this, we
>> won't be able to pass the first test in phy_init_eee(). The second part
>> is to advertise EEE such that this gets reflected in MDIO_AN_EEE_ADV,
>> also to make sure that we can pass the second check in phy_init_eee().
>>
>> Now, looking at phy_init_eee(), and what stmmac does (and bcmgenet,
>> copied after stmmac), we need to somehow, have EEE advertised for
>> phy_init_eee() to succeed, prepare the MAC to support EEE, and finally
>> conclude with a call to phy_ethtool_set_eee(), which writes to the
>> MDIO_AN_EEE_ADV register, and concludes the EEE auto-negotiated process.
>> Since we already have EEE advertised, we are essentially just checking
>> that the EEE advertised settings and the LP advertised settings actually
>> do match, so it sounds like the final call to phy_ethtool_set_eee() is
>> potentially useless if the resolved advertised and link partner
>> advertised settings already match...
>>
>> So it sounds like at least, the first time you try to initialize EEE, we
>> should start with EEE not advertised, and then, if we have EEE enabled
>> at some point, and we re-negotiate the link parameters, somehow
>> phy_init_eee() does a right job for that.
>>
>> Peppe, any thoughts on this?
> 
> I share what you say.
> 
> In sum, the EEE management inside the stmmac is:
> 
> - the driver looks at own HW cap register if EEE is supported
> 
>     (indeed the user could keep disable EEE if bugged on some HW
>      + Alex, Fabrice: we had some patches for this to propose where we
>              called the phy_ethtool_set_eee to disable feature at phy
>              level
> 
> - then the stmmac asks PHY layer to understand if transceiver and
>   partners are EEE capable.
> 
> - If all matches the EEE is actually initialized.
> 
> the logic above should be respected when use ethtool, hmm, I will
> check the stmmac_ethtool_op_set_eee asap.
> 
> Hoping this is useful

This is definitively useful, the only part that I am struggling to
understand in phy_init_eee() is this:

                eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
                                                MDIO_MMD_AN);
                if (eee_adv <= 0)
                        goto eee_exit_err;

if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
the time we call phy_init_eee(), then we cannot complete the EEE
configuration at the PHY level, and presumably we should abort the EEE
configuration at the MAC level.

While this condition makes sense if e.g: you are re-negotiating the link
with your partner for instance and if EEE was already advertised, the
very first time this function is called, it seems to be like we should
skip the check, because phy_init_eee() should actually tell us if, as a
result of a successful check, we should be setting EEE as something we
advertise?

Do you remember what was the logic behind this check when you added it?

Thanks!
-- 
Florian

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>> Peppe, any thoughts on this?
>>
>> I share what you say.
>>
>> In sum, the EEE management inside the stmmac is:
>>
>> - the driver looks at own HW cap register if EEE is supported
>>
>>     (indeed the user could keep disable EEE if bugged on some HW
>>      + Alex, Fabrice: we had some patches for this to propose where we
>>              called the phy_ethtool_set_eee to disable feature at phy
>>              level
>>
>> - then the stmmac asks PHY layer to understand if transceiver and
>>   partners are EEE capable.
>>
>> - If all matches the EEE is actually initialized.
>>
>> the logic above should be respected when use ethtool, hmm, I will
>> check the stmmac_ethtool_op_set_eee asap.
>>
>> Hoping this is useful
> 
> This is definitively useful, the only part that I am struggling to
> understand in phy_init_eee() is this:
> 
>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>                                                 MDIO_MMD_AN);
>                 if (eee_adv <= 0)
>                         goto eee_exit_err;
> 
> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
> the time we call phy_init_eee(), then we cannot complete the EEE
> configuration at the PHY level, and presumably we should abort the EEE
> configuration at the MAC level.
> 
> While this condition makes sense if e.g: you are re-negotiating the link
> with your partner for instance and if EEE was already advertised, the
> very first time this function is called, it seems to be like we should
> skip the check, because phy_init_eee() should actually tell us if, as a
> result of a successful check, we should be setting EEE as something we
> advertise?
> 
> Do you remember what was the logic behind this check when you added it?

Peppe, can you remember why phy_init_eee() was written in a way that you
need to have already locally advertised EEE for the function to
successfully return? Thank you!
-- 
Florian

Re: Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Niklas Cassel]

On 01/04/2017 03:33 PM, Florian Fainelli wrote:
> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>> Peppe, any thoughts on this?
>>>
>>> I share what you say.
>>>
>>> In sum, the EEE management inside the stmmac is:
>>>
>>> - the driver looks at own HW cap register if EEE is supported
>>>
>>>     (indeed the user could keep disable EEE if bugged on some HW
>>>      + Alex, Fabrice: we had some patches for this to propose where we
>>>              called the phy_ethtool_set_eee to disable feature at phy
>>>              level
>>>
>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>   partners are EEE capable.
>>>
>>> - If all matches the EEE is actually initialized.
>>>
>>> the logic above should be respected when use ethtool, hmm, I will
>>> check the stmmac_ethtool_op_set_eee asap.
>>>
>>> Hoping this is useful
>>
>> This is definitively useful, the only part that I am struggling to
>> understand in phy_init_eee() is this:
>>
>>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>                                                 MDIO_MMD_AN);
>>                 if (eee_adv <= 0)
>>                         goto eee_exit_err;
>>
>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>> the time we call phy_init_eee(), then we cannot complete the EEE
>> configuration at the PHY level, and presumably we should abort the EEE
>> configuration at the MAC level.
>>
>> While this condition makes sense if e.g: you are re-negotiating the link
>> with your partner for instance and if EEE was already advertised, the
>> very first time this function is called, it seems to be like we should
>> skip the check, because phy_init_eee() should actually tell us if, as a
>> result of a successful check, we should be setting EEE as something we
>> advertise?
>>
>> Do you remember what was the logic behind this check when you added it?
> 
> Peppe, can you remember why phy_init_eee() was written in a way that you
> need to have already locally advertised EEE for the function to
> successfully return? Thank you!
> 

I'm curious about this as well.

I can get EEE to work with stmmac, but to be able to turn EEE on,
I need to set eee advertise via ethtool first.
(Tested with 2 different PHYs from different vendors, with their
PHY specific driver enabled.)

Is this the same for all PHYs or are there certain PHYs/PHY drivers
that actually advertise eee by default?
(From reading this mail thread there seems to be a suggestion that
the broadcom PHY driver might advertise eee by default.)

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli]

On 04/18/2017 06:23 AM, Niklas Cassel wrote:
> On 01/04/2017 03:33 PM, Florian Fainelli wrote:
>> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>>> Peppe, any thoughts on this?
>>>>
>>>> I share what you say.
>>>>
>>>> In sum, the EEE management inside the stmmac is:
>>>>
>>>> - the driver looks at own HW cap register if EEE is supported
>>>>
>>>>     (indeed the user could keep disable EEE if bugged on some HW
>>>>      + Alex, Fabrice: we had some patches for this to propose where we
>>>>              called the phy_ethtool_set_eee to disable feature at phy
>>>>              level
>>>>
>>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>>   partners are EEE capable.
>>>>
>>>> - If all matches the EEE is actually initialized.
>>>>
>>>> the logic above should be respected when use ethtool, hmm, I will
>>>> check the stmmac_ethtool_op_set_eee asap.
>>>>
>>>> Hoping this is useful
>>>
>>> This is definitively useful, the only part that I am struggling to
>>> understand in phy_init_eee() is this:
>>>
>>>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>>                                                 MDIO_MMD_AN);
>>>                 if (eee_adv <= 0)
>>>                         goto eee_exit_err;
>>>
>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>>> the time we call phy_init_eee(), then we cannot complete the EEE
>>> configuration at the PHY level, and presumably we should abort the EEE
>>> configuration at the MAC level.
>>>
>>> While this condition makes sense if e.g: you are re-negotiating the link
>>> with your partner for instance and if EEE was already advertised, the
>>> very first time this function is called, it seems to be like we should
>>> skip the check, because phy_init_eee() should actually tell us if, as a
>>> result of a successful check, we should be setting EEE as something we
>>> advertise?
>>>
>>> Do you remember what was the logic behind this check when you added it?
>>
>> Peppe, can you remember why phy_init_eee() was written in a way that you
>> need to have already locally advertised EEE for the function to
>> successfully return? Thank you!
>>
> 
> I'm curious about this as well.
> 
> I can get EEE to work with stmmac, but to be able to turn EEE on,
> I need to set eee advertise via ethtool first.
> (Tested with 2 different PHYs from different vendors, with their
> PHY specific driver enabled.)
> 
> Is this the same for all PHYs or are there certain PHYs/PHY drivers
> that actually advertise eee by default?

It depends on whether the PHY driver takes care of the EEE advertisement
part for your or not, most drivers probably don't do that.

> (From reading this mail thread there seems to be a suggestion that
> the broadcom PHY driver might advertise eee by default.)

As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do
advertise EEE by default in order to validate the first check done in
phy_init_eee(), but that's the only reason really.

Since we have not been able to get a straight answer from Peppe about
why there is this initial check, I think the cleanest path moving
forward is the following:

- rename phy_init_eee() into something like: phy_can_do_eee() and remove
the first check on whether EEE is already advertised because that's
precisely what we are trying to determine with this function

- Ethernet MAC drivers keep calling phy_can_do_eee() (formerly
phy_init_eee()) during their adjust_link callback in order to
re-negotiate EEE with their link partner, just like they should call
phy_ethtool_set_eee() to really enable EEE the first time they want to
enable EEE with the link partner

- remove the part from phy_init_eee() that tries to stop the PHY TX
clock and provide a set of helpers: phy_can_stop_tx_clk() and
phy_set_stop_tx_clk() which will take care of that

Does that look reasonable?
-- 
Florian

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Niklas Cassel]

On 04/18/2017 06:40 PM, Florian Fainelli wrote:
> On 04/18/2017 06:23 AM, Niklas Cassel wrote:
>> On 01/04/2017 03:33 PM, Florian Fainelli wrote:
>>> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>>>> Peppe, any thoughts on this?
>>>>>
>>>>> I share what you say.
>>>>>
>>>>> In sum, the EEE management inside the stmmac is:
>>>>>
>>>>> - the driver looks at own HW cap register if EEE is supported
>>>>>
>>>>>     (indeed the user could keep disable EEE if bugged on some HW
>>>>>      + Alex, Fabrice: we had some patches for this to propose where we
>>>>>              called the phy_ethtool_set_eee to disable feature at phy
>>>>>              level
>>>>>
>>>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>>>   partners are EEE capable.
>>>>>
>>>>> - If all matches the EEE is actually initialized.
>>>>>
>>>>> the logic above should be respected when use ethtool, hmm, I will
>>>>> check the stmmac_ethtool_op_set_eee asap.
>>>>>
>>>>> Hoping this is useful
>>>>
>>>> This is definitively useful, the only part that I am struggling to
>>>> understand in phy_init_eee() is this:
>>>>
>>>>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>>>                                                 MDIO_MMD_AN);
>>>>                 if (eee_adv <= 0)
>>>>                         goto eee_exit_err;
>>>>
>>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>>>> the time we call phy_init_eee(), then we cannot complete the EEE
>>>> configuration at the PHY level, and presumably we should abort the EEE
>>>> configuration at the MAC level.
>>>>
>>>> While this condition makes sense if e.g: you are re-negotiating the link
>>>> with your partner for instance and if EEE was already advertised, the
>>>> very first time this function is called, it seems to be like we should
>>>> skip the check, because phy_init_eee() should actually tell us if, as a
>>>> result of a successful check, we should be setting EEE as something we
>>>> advertise?
>>>>
>>>> Do you remember what was the logic behind this check when you added it?
>>>
>>> Peppe, can you remember why phy_init_eee() was written in a way that you
>>> need to have already locally advertised EEE for the function to
>>> successfully return? Thank you!
>>>
>>
>> I'm curious about this as well.
>>
>> I can get EEE to work with stmmac, but to be able to turn EEE on,
>> I need to set eee advertise via ethtool first.
>> (Tested with 2 different PHYs from different vendors, with their
>> PHY specific driver enabled.)
>>
>> Is this the same for all PHYs or are there certain PHYs/PHY drivers
>> that actually advertise eee by default?
> 
> It depends on whether the PHY driver takes care of the EEE advertisement
> part for your or not, most drivers probably don't do that.
> 
>> (From reading this mail thread there seems to be a suggestion that
>> the broadcom PHY driver might advertise eee by default.)
> 
> As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do
> advertise EEE by default in order to validate the first check done in
> phy_init_eee(), but that's the only reason really.
> 
> Since we have not been able to get a straight answer from Peppe about
> why there is this initial check, I think the cleanest path moving
> forward is the following:
> 
> - rename phy_init_eee() into something like: phy_can_do_eee() and remove
> the first check on whether EEE is already advertised because that's
> precisely what we are trying to determine with this function
> 
> - Ethernet MAC drivers keep calling phy_can_do_eee() (formerly
> phy_init_eee()) during their adjust_link callback in order to
> re-negotiate EEE with their link partner, just like they should call
> phy_ethtool_set_eee() to really enable EEE the first time they want to
> enable EEE with the link partner
> 
> - remove the part from phy_init_eee() that tries to stop the PHY TX
> clock and provide a set of helpers: phy_can_stop_tx_clk() and
> phy_set_stop_tx_clk() which will take care of that
> 
> Does that look reasonable?


Sounds very reasonable to me.

However, if I look specifically at the stmmac driver,
stmmac_eee_init() is called from adjust_link callback.

If we replace phy_init_eee() with a phy_can_do_eee()
in stmmac_eee_init(), then the driver will enable
EEE in the IP, and setup timers etc.


If I understand you correctly, the code in the adjust_link
callback should call phy_can_do_eee() so that the PHY
re-negotiate EEE with the link partner.

You will still need to use ethtool to actually enable it in the
PHY (call the new phy_init_eee()).
(Which sounds good, since we probably do not want to suddenly
enable EEE by default in a lot of drivers.)


The issue that I see is that we probably do not want to
setup timers, etc. in the adjust_link callback before
EEE has actually been enabled, so it might not be as
easy as just replacing phy_init_eee() with phy_can_do_eee()
in some drivers.

Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Niklas Cassel]

On 04/18/2017 06:40 PM, Florian Fainelli wrote:
> On 04/18/2017 06:23 AM, Niklas Cassel wrote:
>> On 01/04/2017 03:33 PM, Florian Fainelli wrote:
>>> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>>>> Peppe, any thoughts on this?
>>>>>
>>>>> I share what you say.
>>>>>
>>>>> In sum, the EEE management inside the stmmac is:
>>>>>
>>>>> - the driver looks at own HW cap register if EEE is supported
>>>>>
>>>>>     (indeed the user could keep disable EEE if bugged on some HW
>>>>>      + Alex, Fabrice: we had some patches for this to propose where we
>>>>>              called the phy_ethtool_set_eee to disable feature at phy
>>>>>              level
>>>>>
>>>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>>>   partners are EEE capable.
>>>>>
>>>>> - If all matches the EEE is actually initialized.
>>>>>
>>>>> the logic above should be respected when use ethtool, hmm, I will
>>>>> check the stmmac_ethtool_op_set_eee asap.
>>>>>
>>>>> Hoping this is useful
>>>>
>>>> This is definitively useful, the only part that I am struggling to
>>>> understand in phy_init_eee() is this:
>>>>
>>>>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>>>                                                 MDIO_MMD_AN);
>>>>                 if (eee_adv <= 0)
>>>>                         goto eee_exit_err;
>>>>
>>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>>>> the time we call phy_init_eee(), then we cannot complete the EEE
>>>> configuration at the PHY level, and presumably we should abort the EEE
>>>> configuration at the MAC level.
>>>>
>>>> While this condition makes sense if e.g: you are re-negotiating the link
>>>> with your partner for instance and if EEE was already advertised, the
>>>> very first time this function is called, it seems to be like we should
>>>> skip the check, because phy_init_eee() should actually tell us if, as a
>>>> result of a successful check, we should be setting EEE as something we
>>>> advertise?
>>>>
>>>> Do you remember what was the logic behind this check when you added it?
>>>
>>> Peppe, can you remember why phy_init_eee() was written in a way that you
>>> need to have already locally advertised EEE for the function to
>>> successfully return? Thank you!
>>>
>>
>> I'm curious about this as well.
>>
>> I can get EEE to work with stmmac, but to be able to turn EEE on,
>> I need to set eee advertise via ethtool first.
>> (Tested with 2 different PHYs from different vendors, with their
>> PHY specific driver enabled.)
>>
>> Is this the same for all PHYs or are there certain PHYs/PHY drivers
>> that actually advertise eee by default?
> 
> It depends on whether the PHY driver takes care of the EEE advertisement
> part for your or not, most drivers probably don't do that.
> 
>> (From reading this mail thread there seems to be a suggestion that
>> the broadcom PHY driver might advertise eee by default.)
> 
> As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do
> advertise EEE by default in order to validate the first check done in
> phy_init_eee(), but that's the only reason really.
> 
> Since we have not been able to get a straight answer from Peppe about
> why there is this initial check, I think the cleanest path moving
> forward is the following:
> 
> - rename phy_init_eee() into something like: phy_can_do_eee() and remove
> the first check on whether EEE is already advertised because that's
> precisely what we are trying to determine with this function
> 
> - Ethernet MAC drivers keep calling phy_can_do_eee() (formerly
> phy_init_eee()) during their adjust_link callback in order to
> re-negotiate EEE with their link partner, just like they should call
> phy_ethtool_set_eee() to really enable EEE the first time they want to
> enable EEE with the link partner
> 
> - remove the part from phy_init_eee() that tries to stop the PHY TX
> clock and provide a set of helpers: phy_can_stop_tx_clk() and
> phy_set_stop_tx_clk() which will take care of that
> 
> Does that look reasonable?


Sounds very reasonable to me.

However, if I look specifically at the stmmac driver,
stmmac_eee_init() is called from adjust_link callback.

If we replace phy_init_eee() with a phy_can_do_eee()
in stmmac_eee_init(), then the driver will enable
EEE in the IP, and setup timers etc.


If I understand you correctly, the code in the adjust_link
callback should call phy_can_do_eee() so that the PHY
re-negotiate EEE with the link partner.

You will still need to use ethtool to actually enable it in the
PHY (call the new phy_init_eee()).
(Which sounds good, since we probably do not want to suddenly
enable EEE by default in a lot of drivers.)


The issue that I see is that we probably do not want to
setup timers, etc. in the adjust_link callback before
EEE has actually been enabled, so it might not be as
easy as just replacing phy_init_eee() with phy_can_do_eee()
in some drivers.