Hi.
This patch series fixes EEE support for MT7531 and the switch on the MT7988
SoC. EEE could not be enabled on MT7531 on most boards using ethtool before
this. On MT7988 SoC switch, EEE is disabled by default but can be turned on
normally using ethtool. EEE is enabled by default on MT7530 and there's no
need to make changes on the DSA subdriver for it. This patch series
disables EEE by default on MT7531 but makes it possible to enable it using
ethtool.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
Changes in v2:
- Delegate the patch to the net tree.
- Remove patch 3, it was revealed that it doesn't fix a bug.
- Patch 1
- Disable EEE advertisement on MT7531 by default.
- Link to v1: https://lore.kernel.org/r/20240318-for-net-mt7530-fix-eee-for-mt7531-mt7988-v1-0-3f17226344e8@arinc9.com
---
Arınç ÜNAL (2):
net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
drivers/net/dsa/mt7530.c | 14 ++++++++++++++
drivers/net/dsa/mt7530.h | 7 ++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
---
base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8
Best regards,
--
Arınç ÜNAL <[email protected]>
From: Arınç ÜNAL <[email protected]>
The MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 bits let the
PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits determine the 1G/100 EEE
abilities of the MAC. If MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 are
unset, the abilities are left to be determined by PHY auto polling.
The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
made it so that the PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits are set on
mt753x_phylink_mac_link_up(). But it did not set the MT7531_FORCE_EEE1G and
MT7531_FORCE_EEE100 bits. Because of this, EEE will be enabled on the
switch MACs by polling the PHY, regardless of the result of phy_init_eee().
Define these bits and add them to MT7531_FORCE_MODE which is being used by
the subdriver. With this, EEE will be prevented from being enabled on the
switch MACs when phy_init_eee() fails.
Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 509ed5362236..5b99aeca34b4 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -299,11 +299,15 @@ enum mt7530_vlan_port_acc_frm {
#define MT7531_FORCE_DPX BIT(29)
#define MT7531_FORCE_RX_FC BIT(28)
#define MT7531_FORCE_TX_FC BIT(27)
+#define MT7531_FORCE_EEE100 BIT(26)
+#define MT7531_FORCE_EEE1G BIT(25)
#define MT7531_FORCE_MODE (MT7531_FORCE_LNK | \
MT7531_FORCE_SPD | \
MT7531_FORCE_DPX | \
MT7531_FORCE_RX_FC | \
- MT7531_FORCE_TX_FC)
+ MT7531_FORCE_TX_FC | \
+ MT7531_FORCE_EEE100 | \
+ MT7531_FORCE_EEE1G)
#define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
--
2.40.1
From: Arınç ÜNAL <[email protected]>
The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
brought EEE support but did not enable EEE on MT7531 switch MACs. EEE is
enabled on MT7531 switch MACs either by pulling the LAN2LED0 pin low on the
board (bootstrapping), or unsetting the EEE_DIS bit on the trap register.
There are existing boards that were not designed to pull the pin low.
Therefore, unset the EEE_DIS bit on the trap register.
Unlike MT7530, the modifiable trap register won't be populated identical to
the trap status register after reset. Therefore, read from the trap status
register, modify the bits, then write to the modifiable trap register.
My testing on MT7531 shows a certain amount of traffic loss when EEE is
enabled. That said, I haven't come across a board that enables EEE. So
enable EEE on the switch MACs but disable EEE advertisement on the switch
PHYs. This way, we don't change the behaviour of the majority of the boards
that have this switch.
With this change, EEE can now be enabled using ethtool.
The disable EEE bit on the trap pertains to the LAN2LED0 pin which is
usually used to control an LED. Once the bit is unset, the pin will be low.
That will make the active low LED turn on.
The pin is controlled by the switch PHY. It seems that the PHY controls the
pin in the way that it inverts the pin state. That means depending on the
wiring of the LED connected to LAN2LED0 on the board, the LED may be on
without an active link.
Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 14 ++++++++++++++
drivers/net/dsa/mt7530.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 678b51f9cea6..6aa99b590329 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2458,6 +2458,20 @@ mt7531_setup(struct dsa_switch *ds)
/* Reset the switch through internal reset */
mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_SW_RST | SYS_CTRL_REG_RST);
+ /* Allow modifying the trap and enable Energy-Efficient Ethernet (EEE).
+ */
+ val = mt7530_read(priv, MT7531_HWTRAP);
+ val |= CHG_STRAP;
+ val &= ~EEE_DIS;
+ mt7530_write(priv, MT7530_MHWTRAP, val);
+
+ /* Disable EEE advertisement on the switch PHYs. */
+ for (i = MT753X_CTRL_PHY_ADDR;
+ i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
+ mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
+ 0);
+ }
+
if (!priv->p5_sgmii) {
mt7531_pll_setup(priv);
} else {
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index a71166e0a7fc..509ed5362236 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -457,6 +457,7 @@ enum mt7531_clk_skew {
#define XTAL_FSEL_M BIT(7)
#define PHY_EN BIT(6)
#define CHG_STRAP BIT(8)
+#define EEE_DIS BIT(4)
/* Register for hw trap modification */
#define MT7530_MHWTRAP 0x7804
--
2.40.1
On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 bits let the
> PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits determine the 1G/100 EEE
> abilities of the MAC. If MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 are
> unset, the abilities are left to be determined by PHY auto polling.
>
> The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
> made it so that the PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits are set on
> mt753x_phylink_mac_link_up(). But it did not set the MT7531_FORCE_EEE1G and
> MT7531_FORCE_EEE100 bits. Because of this, EEE will be enabled on the
> switch MACs by polling the PHY, regardless of the result of phy_init_eee().
>
> Define these bits and add them to MT7531_FORCE_MODE which is being used by
> the subdriver. With this, EEE will be prevented from being enabled on the
> switch MACs when phy_init_eee() fails.
>
> Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
> Signed-off-by: Arınç ÜNAL <[email protected]>
If I read the past discussion correctly, this is a potential issue
found by code inspection and never producing problem in practice, am I
correct?
If so I think it will deserve a 3rd party tested-by tag or similar to
go in.
If nobody could provide such feedback in a little time, I suggest to
drop this patch and apply only 1/2.
Cheers,
Paolo
On 26.03.2024 12:02, Paolo Abeni wrote:
> On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> The MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 bits let the
>> PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits determine the 1G/100 EEE
>> abilities of the MAC. If MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 are
>> unset, the abilities are left to be determined by PHY auto polling.
>>
>> The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
>> made it so that the PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits are set on
>> mt753x_phylink_mac_link_up(). But it did not set the MT7531_FORCE_EEE1G and
>> MT7531_FORCE_EEE100 bits. Because of this, EEE will be enabled on the
>> switch MACs by polling the PHY, regardless of the result of phy_init_eee().
>>
>> Define these bits and add them to MT7531_FORCE_MODE which is being used by
>> the subdriver. With this, EEE will be prevented from being enabled on the
>> switch MACs when phy_init_eee() fails.
>>
>> Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>
> If I read the past discussion correctly, this is a potential issue
> found by code inspection and never producing problem in practice, am I
> correct?
>
> If so I think it will deserve a 3rd party tested-by tag or similar to
> go in.
>
> If nobody could provide such feedback in a little time, I suggest to
> drop this patch and apply only 1/2.
Whether a problem would happen in practice depends on when phy_init_eee()
fails, meaning it returns a negative non-zero code. I requested Russell to
review this patch to shed light on when phy_init_eee() would return a
negative non-zero code so we have an idea whether this patch actually fixes
a problem.
Arınç
On 26.03.2024 12:19, Arınç ÜNAL wrote:
> On 26.03.2024 12:02, Paolo Abeni wrote:
>> If I read the past discussion correctly, this is a potential issue
>> found by code inspection and never producing problem in practice, am I
>> correct?
>>
>> If so I think it will deserve a 3rd party tested-by tag or similar to
>> go in.
>>
>> If nobody could provide such feedback in a little time, I suggest to
>> drop this patch and apply only 1/2.
>
> Whether a problem would happen in practice depends on when
> phy_init_eee()
> fails, meaning it returns a negative non-zero code. I requested Russell
> to
> review this patch to shed light on when phy_init_eee() would return a
> negative non-zero code so we have an idea whether this patch actually
> fixes
> a problem.
I don't suppose Russell is going to review the patch at this point. I
will
submit this to net-next then. If someone actually reports a problem in
practice, I can always submit it to the stable trees.
Arınç
On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> From: Arınç ÜNAL <[email protected]>
>
> The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE
> features")
> brought EEE support but did not enable EEE on MT7531 switch MACs. EEE
> is
> enabled on MT7531 switch MACs either by pulling the LAN2LED0 pin low
> on the
> board (bootstrapping), or unsetting the EEE_DIS bit on the trap
> register.
>
> There are existing boards that were not designed to pull the pin low.
> Therefore, unset the EEE_DIS bit on the trap register.
>
> Unlike MT7530, the modifiable trap register won't be populated
> identical to
> the trap status register after reset. Therefore, read from the trap
> status
> register, modify the bits, then write to the modifiable trap
> register.
>
> My testing on MT7531 shows a certain amount of traffic loss when EEE
> is
> enabled. That said, I haven't come across a board that enables EEE.
> So
> enable EEE on the switch MACs but disable EEE advertisement on the
> switch
> PHYs. This way, we don't change the behaviour of the majority of the
> boards
> that have this switch.
>
> With this change, EEE can now be enabled using ethtool.
>
> The disable EEE bit on the trap pertains to the LAN2LED0 pin which is
> usually used to control an LED. Once the bit is unset, the pin will
> be low.
> That will make the active low LED turn on.
>
> The pin is controlled by the switch PHY. It seems that the PHY
> controls the
> pin in the way that it inverts the pin state. That means depending on
> the
> wiring of the LED connected to LAN2LED0 on the board, the LED may be
> on
> without an active link.
>
> Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE
> features")
> Reviewed-by: Florian Fainelli <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 14 ++++++++++++++
> drivers/net/dsa/mt7530.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 678b51f9cea6..6aa99b590329 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2458,6 +2458,20 @@ mt7531_setup(struct dsa_switch *ds)
> /* Reset the switch through internal reset */
> mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_SW_RST |
> SYS_CTRL_REG_RST);
>
> +/* Allow modifying the trap and enable Energy-Efficient Ethernet
> (EEE).
> + */
> +val = mt7530_read(priv, MT7531_HWTRAP);
> +val |= CHG_STRAP;
> +val &= ~EEE_DIS;
> +mt7530_write(priv, MT7530_MHWTRAP, val);
You may try to set bit 6 of CL45 register dev=0x1f,
reg=0x403(PLL_GROUP_CTL_REG), which is an internal EEE switch called
RG_SYSPLL_DMY2.
Read it out first with "switch command", if you have it:
root@OpenWrt:/# switch phy cl45 r 0 0x1f 0x403
Phy read dev_num=0x1f, reg=0x403, value=0x1091
And then set bit 6:
root@OpenWrt:/# $ switch phy cl45 w 0 0x1f 0x403 0x10d1
root@OpenWrt:/# ethtool --set-eee lan1 eee on tx-lpi on tx-timer 0x1e
advertise 0x28
root@OpenWrt:/# switch phy cl45 r 0 0x7 0x3c
Phy read dev_num=0x7, reg=0x3c, value=0x6
Then you should be able to enable EEE via ethtool without clearing
EEE_DIS bit of MT7530_HWTRAP.
If above CL45 command is good for you, I think this can be moved to
mtk_gephy_config_init() @ mediatek-ge.c, which will lead to cleaner
implementation.
> +
> +/* Disable EEE advertisement on the switch PHYs. */
> +for (i = MT753X_CTRL_PHY_ADDR;
> + i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
> +mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
> + 0);
> +}
Sorry, I still can't figure out why this is needed since we disable
MDIO_AN_EEE_ADV in mediatek-ge.c after reading previous threads. Would
you please provide something else (like dmesg log?) to show that
settings in mtk_gephy_config_init() may fail?
> +
> if (!priv->p5_sgmii) {
> mt7531_pll_setup(priv);
> } else {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index a71166e0a7fc..509ed5362236 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -457,6 +457,7 @@ enum mt7531_clk_skew {
> #define XTAL_FSEL_MBIT(7)
> #define PHY_ENBIT(6)
> #define CHG_STRAPBIT(8)
> +#define EEE_DISBIT(4)
>
> /* Register for hw trap modification */
> #define MT7530_MHWTRAP0x7804
>
> --
> 2.40.1
>
>
On Tue, Mar 26, 2024 at 12:19:40PM +0300, Arınç ÜNAL wrote:
> On 26.03.2024 12:02, Paolo Abeni wrote:
> > On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote:
> > > From: Arınç ÜNAL <[email protected]>
> > >
> > > The MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 bits let the
> > > PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits determine the 1G/100 EEE
> > > abilities of the MAC. If MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 are
> > > unset, the abilities are left to be determined by PHY auto polling.
> > >
> > > The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
> > > made it so that the PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits are set on
> > > mt753x_phylink_mac_link_up(). But it did not set the MT7531_FORCE_EEE1G and
> > > MT7531_FORCE_EEE100 bits. Because of this, EEE will be enabled on the
> > > switch MACs by polling the PHY, regardless of the result of phy_init_eee().
> > >
> > > Define these bits and add them to MT7531_FORCE_MODE which is being used by
> > > the subdriver. With this, EEE will be prevented from being enabled on the
> > > switch MACs when phy_init_eee() fails.
> > >
> > > Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
> > > Signed-off-by: Arınç ÜNAL <[email protected]>
> >
> > If I read the past discussion correctly, this is a potential issue
> > found by code inspection and never producing problem in practice, am I
> > correct?
> >
> > If so I think it will deserve a 3rd party tested-by tag or similar to
> > go in.
> >
> > If nobody could provide such feedback in a little time, I suggest to
> > drop this patch and apply only 1/2.
>
> Whether a problem would happen in practice depends on when phy_init_eee()
> fails, meaning it returns a negative non-zero code. I requested Russell to
> review this patch to shed light on when phy_init_eee() would return a
> negative non-zero code so we have an idea whether this patch actually fixes
> a problem.
Urgh, so I need to read the code and report back?
Well, looking at phy_init_eee(), it could return a negative vallue when:
1. phydev->drv is NULL
2. if genphy_c45_eee_is_active() returns negative
3. if genphy_c45_eee_is_active() returns zero, it returns
-EPROTONOSUPPORT
4. if phy_set_bits_mmd() fails (e.g. communication error with the PHY)
If we then look at genphy_c45_eee_is_active(), then:
genphy_c45_read_eee_adv() and genphy_c45_read_eee_lpa() propagate their
non-zero return values, otherwise this function returns zero or positive
integer.
If we then look at genphy_c45_read_eee_adv(), then a failure of
phy_read_mmd() would cause a negative value to be returned.
Looking at genphy_c45_read_eee_lpa(), the same is true.
So, it can be summarised as:
- phydev->drv is NULL
- there is a communication error accessing the PHY
- EEE is not active
otherwise, it returns zero on success.
If one wishes to determine whether an error occurred vs EEE not being
supported through negotiation for the negotiated speed, if it returns
-EPROTONOSUPPORT in the latter case. Other error codes mean either the
driver has been unloaded or communication error.
This has been expertly determined by reading the code, which only a
phylib maintainer has the capability of doing. Thank you for using this
service.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Mar 27, 2024 at 11:46:19AM +0300, [email protected] wrote:
> On 26.03.2024 12:19, Arınç ÜNAL wrote:
> > On 26.03.2024 12:02, Paolo Abeni wrote:
> > > If I read the past discussion correctly, this is a potential issue
> > > found by code inspection and never producing problem in practice, am I
> > > correct?
> > >
> > > If so I think it will deserve a 3rd party tested-by tag or similar to
> > > go in.
> > >
> > > If nobody could provide such feedback in a little time, I suggest to
> > > drop this patch and apply only 1/2.
> >
> > Whether a problem would happen in practice depends on when
> > phy_init_eee()
> > fails, meaning it returns a negative non-zero code. I requested Russell
> > to
> > review this patch to shed light on when phy_init_eee() would return a
> > negative non-zero code so we have an idea whether this patch actually
> > fixes
> > a problem.
>
> I don't suppose Russell is going to review the patch at this point. I will
> submit this to net-next then. If someone actually reports a problem in
> practice, I can always submit it to the stable trees.
So the fact that I only saw your request this morning to look at
phy_init_eee(), and to review this patch... because... I work for
Oracle, and I've been looking at backporting Arm64 KVM patches to
our kernel, been testing and debugging that effort... and the
act that less than 24 hours had passed since you made the original
request... yea, sorry, it's clearly my fault for not jumping on this
the moment you sent the email.
I get _so_ much email that incorrectly has me in the To: header. I
also get _so_ much email that fails to list me in the To: header
when the author wants me to respond. I don't have time to read every
email as it comes in. I certainly don't have time to read every
email in any case. I do the best I can, which varies considerably
with my workload.
I already find that being single, fitting everything in during the
day (paid work, chores, feeding oneself) is quite a mammoth task.
There is no one else to do the laundry. There is no one else to get
the shopping. There is no one else to do the washing up. There is no
one else to take the rubbish out. All this I do myself, and serially
because there is only one of me, and it all takes time away from
sitting here reading every damn email as it comes in.
And then when I end up doing something that _you_ very well could do
(reading the phy_init_eee() code to find out when it might return a
negative number) and then you send an email like this... yea... that
really gets my goat.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Mar 27, 2024 at 03:58:13PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 27, 2024 at 11:46:19AM +0300, [email protected] wrote:
> > On 26.03.2024 12:19, Arınç ÜNAL wrote:
> > > On 26.03.2024 12:02, Paolo Abeni wrote:
> > > > If I read the past discussion correctly, this is a potential issue
> > > > found by code inspection and never producing problem in practice, am I
> > > > correct?
> > > >
> > > > If so I think it will deserve a 3rd party tested-by tag or similar to
> > > > go in.
> > > >
> > > > If nobody could provide such feedback in a little time, I suggest to
> > > > drop this patch and apply only 1/2.
> > >
> > > Whether a problem would happen in practice depends on when
> > > phy_init_eee()
> > > fails, meaning it returns a negative non-zero code. I requested Russell
> > > to
> > > review this patch to shed light on when phy_init_eee() would return a
> > > negative non-zero code so we have an idea whether this patch actually
> > > fixes
> > > a problem.
> >
> > I don't suppose Russell is going to review the patch at this point. I will
> > submit this to net-next then. If someone actually reports a problem in
> > practice, I can always submit it to the stable trees.
>
> So the fact that I only saw your request this morning to look at
> phy_init_eee(), and to review this patch... because... I work for
> Oracle, and I've been looking at backporting Arm64 KVM patches to
> our kernel, been testing and debugging that effort... and the
> act that less than 24 hours had passed since you made the original
> request... yea, sorry, it's clearly my fault for not jumping on this
> the moment you sent the email.
>
> I get _so_ much email that incorrectly has me in the To: header. I
> also get _so_ much email that fails to list me in the To: header
> when the author wants me to respond. I don't have time to read every
> email as it comes in. I certainly don't have time to read every
> email in any case. I do the best I can, which varies considerably
> with my workload.
>
> I already find that being single, fitting everything in during the
> day (paid work, chores, feeding oneself) is quite a mammoth task.
> There is no one else to do the laundry. There is no one else to get
> the shopping. There is no one else to do the washing up. There is no
> one else to take the rubbish out. All this I do myself, and serially
> because there is only one of me, and it all takes time away from
> sitting here reading every damn email as it comes in.
>
> And then when I end up doing something that _you_ very well could do
> (reading the phy_init_eee() code to find out when it might return a
> negative number) and then you send an email like this... yea... that
> really gets my goat.
.. and now I have a 1:1 with my manager for the next 30-60 minutes.
Is it okay by you for me to be offline for that period of time while
I have a chat with him?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hello Huang.
It's great to see someone from MediaTek providing information on this
intellectual property.
On 27.03.2024 12:51, SkyLake Huang (黃啟澤) wrote:
> You may try to set bit 6 of CL45 register dev=0x1f,
> reg=0x403(PLL_GROUP_CTL_REG), which is an internal EEE switch called
> RG_SYSPLL_DMY2.
>
> Read it out first with "switch command", if you have it:
> root@OpenWrt:/# switch phy cl45 r 0 0x1f 0x403
> Phy read dev_num=0x1f, reg=0x403, value=0x1091
>
> And then set bit 6:
> root@OpenWrt:/# $ switch phy cl45 w 0 0x1f 0x403 0x10d1
> root@OpenWrt:/# ethtool --set-eee lan1 eee on tx-lpi on tx-timer 0x1e
> advertise 0x28
> root@OpenWrt:/# switch phy cl45 r 0 0x7 0x3c
> Phy read dev_num=0x7, reg=0x3c, value=0x6
>
> Then you should be able to enable EEE via ethtool without clearing
> EEE_DIS bit of MT7530_HWTRAP.
This won't interfere with the LEDs so it's better. I'll use this method,
thank you! This is the current diff [1].
I suppose bit 6 is an internal EEE switch only on MT7531? I see that bit 6
of 0x403 is unset on MT7530 yet EEE is still enabled on the switch MACs.
There's still packet loss when EEE is enabled by setting
MT7531_RG_SYSPLL_DMY2. Please let me know if MT7531 switch PHYs need
calibration for EEE like MT7986 and MT7988 SoC PHYs.
> If above CL45 command is good for you, I think this can be moved to
> mtk_gephy_config_init() @ mediatek-ge.c, which will lead to cleaner
> implementation.
I disagree. The operation concerns the switch MACs too, not just the switch
PHYs. And the operation is not per switch PHY.
>> +
>> +/* Disable EEE advertisement on the switch PHYs. */
>> +for (i = MT753X_CTRL_PHY_ADDR;
>> + i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
>> +mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
>> + 0);
>> +}
> Sorry, I still can't figure out why this is needed since we disable
> MDIO_AN_EEE_ADV in mediatek-ge.c after reading previous threads. Would
> you please provide something else (like dmesg log?) to show that
> settings in mtk_gephy_config_init() may fail?
Disabling EEE advertisement on the PHY driver doesn't fail. To confirm that
and that MDIO_AN_EEE_ADV is populated after it is unset by the PHY driver,
I've done some tests with this diff [2].
Currently:
[ 2.189149] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000
[ 2.204792] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000
[ 2.221413] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139)
[ 2.233762] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000
[ 2.242555] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000
[ 2.259011] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140)
[ 2.271052] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000
[ 2.279842] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000
[ 2.296297] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141)
[ 2.308331] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000
[ 2.317134] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000
[ 2.333597] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142)
[ 2.345653] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000
[ 2.354423] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000
[ 2.370885] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143)
# mount -t debugfs none /sys/kernel/debug
# echo go > /sys/kernel/debug/mt7530/eee_adv
[ 33.055962] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000000
[ 33.062943] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000000
[ 33.069909] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000000
[ 33.076871] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000000
[ 33.083822] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000000
# ethtool --show-eee lan0
EEE settings for lan0:
EEE status: not supported
---
If EEE_DIS is unset or MT7531_RG_SYSPLL_DMY2 is set, MDIO_AN_EEE_ADV is
populated. This also happens on the MT7530 switch:
[ 2.254291] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000006
[ 2.270015] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000
[ 2.286572] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139)
[ 2.298921] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000006
[ 2.307712] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000
[ 2.324162] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140)
[ 2.336204] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000006
[ 2.344974] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000
[ 2.361445] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141)
[ 2.373475] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000006
[ 2.382273] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000
[ 2.398728] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142)
[ 2.410780] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000006
[ 2.419571] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000
[ 2.436028] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143)
# mount -t debugfs none /sys/kernel/debug
# echo go > /sys/kernel/debug/mt7530/eee_adv
[ 8.731911] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000006
[ 8.738908] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000006
[ 8.745861] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000006
[ 8.752816] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000006
[ 8.759776] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000006
# ethtool --show-eee lan0
EEE settings for lan0:
EEE status: enabled - inactive
Tx LPI: disabled
Supported EEE link modes: 100baseT/Full
1000baseT/Full
Advertised EEE link modes: 100baseT/Full
1000baseT/Full
Link partner advertised EEE link modes: Not reported
---
If MDIO_AN_EEE_ADV is unset before the PHY driver kicks in, it stays unset:
[ 2.266561] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000
[ 2.282268] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000
[ 2.298832] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139)
[ 2.311187] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000
[ 2.319976] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000
[ 2.336443] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140)
[ 2.348488] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000
[ 2.357280] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000
[ 2.373728] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141)
[ 2.385780] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000
[ 2.394551] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000
[ 2.411011] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142)
[ 2.423047] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000
[ 2.431849] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000
[ 2.448324] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143)
# mount -t debugfs none /sys/kernel/debug
# echo go > /sys/kernel/debug/mt7530/eee_adv
[ 33.906140] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000000
[ 33.913121] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000000
[ 33.920094] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000000
[ 33.927054] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000000
[ 33.934005] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000000
# ethtool --show-eee lan0
EEE settings for lan0:
EEE status: disabled
Tx LPI: disabled
Supported EEE link modes: 100baseT/Full
1000baseT/Full
Advertised EEE link modes: Not reported
Link partner advertised EEE link modes: Not reported
[1]
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fb3d2a53bbd9..9a351d11f349 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2630,18 +2630,25 @@ mt7531_setup(struct dsa_switch *ds)
mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
MT7531_GPIO0_INTERRUPT);
- /* Enable PHY core PLL, since phy_device has not yet been created
- * provided for phy_[read,write]_mmd_indirect is called, we provide
- * our own mt7531_ind_mmd_phy_[read,write] to complete this
- * function.
+ /* Enable Energy-Efficient Ethernet (EEE) and PHY core PLL, since
+ * phy_device has not yet been created provided for
+ * phy_[read,write]_mmd_indirect is called, we provide our own
+ * mt7531_ind_mmd_phy_[read,write] to complete this function.
*/
val = mt7531_ind_c45_phy_read(priv, MT753X_CTRL_PHY_ADDR,
MDIO_MMD_VEND2, CORE_PLL_GROUP4);
- val |= MT7531_PHY_PLL_BYPASS_MODE;
+ val |= MT7531_RG_SYSPLL_DMY2 | MT7531_PHY_PLL_BYPASS_MODE;
val &= ~MT7531_PHY_PLL_OFF;
mt7531_ind_c45_phy_write(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2,
CORE_PLL_GROUP4, val);
+ /* Disable EEE advertisement on the switch PHYs. */
+ for (i = MT753X_CTRL_PHY_ADDR;
+ i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
+ mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
+ 0);
+ }
+
mt7531_setup_common(ds);
/* Setup VLAN ID 0 for VLAN-unaware bridges */
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index d17b318e6ee4..9439db495001 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -616,6 +616,7 @@ enum mt7531_clk_skew {
#define RG_SYSPLL_DDSFBK_EN BIT(12)
#define RG_SYSPLL_BIAS_EN BIT(11)
#define RG_SYSPLL_BIAS_LPF_EN BIT(10)
+#define MT7531_RG_SYSPLL_DMY2 BIT(6)
#define MT7531_PHY_PLL_OFF BIT(5)
#define MT7531_PHY_PLL_BYPASS_MODE BIT(4)
[2]
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 35906e669259..fb3d2a53bbd9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -845,6 +845,22 @@ static ssize_t reg_value_write(struct file *filp, const char __user *buffer,
return count;
}
+static ssize_t eee_adv_write(struct file *filp, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct mt7530_priv *priv = filp->private_data;
+ int i;
+
+ for (i = MT753X_CTRL_PHY_ADDR;
+ i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
+ dev_info(priv->dev, "phy%d: MDIO_AN_EEE_ADV is %08x", i,
+ mt7531_ind_c45_phy_read(priv, i, MDIO_MMD_AN,
+ MDIO_AN_EEE_ADV));
+ }
+
+ return count;
+}
+
static const struct file_operations reg_address_fops = {
.owner = THIS_MODULE,
.open = simple_open,
@@ -859,6 +875,12 @@ static const struct file_operations reg_value_fops = {
.write = reg_value_write,
};
+static const struct file_operations eee_adv_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .write = eee_adv_write,
+};
+
static void mt7530_debugfs_init(struct mt7530_priv *priv)
{
struct dentry *dir;
@@ -869,6 +891,7 @@ static void mt7530_debugfs_init(struct mt7530_priv *priv)
debugfs_create_file("reg_address", 0644, dir, priv, ®_address_fops);
debugfs_create_file("reg_value", 0644, dir, priv, ®_value_fops);
+ debugfs_create_file("eee_adv", 0644, dir, priv, &eee_adv_fops);
}
static void
diff --git a/drivers/net/phy/mediatek-ge.c b/drivers/net/phy/mediatek-ge.c
index e8822adbf3bc..aacef7091d8f 100644
--- a/drivers/net/phy/mediatek-ge.c
+++ b/drivers/net/phy/mediatek-ge.c
@@ -71,9 +71,15 @@ static int mtk_gephy_write_page(struct phy_device *phydev, int page)
static void mtk_gephy_config_init(struct phy_device *phydev)
{
+ dev_info(&phydev->mdio.dev, "MDIO_AN_EEE_ADV is %08x",
+ phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV));
+
/* Disable EEE */
phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
+ dev_info(&phydev->mdio.dev, "MDIO_AN_EEE_ADV is %08x",
+ phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV));
+
/* Enable HW auto downshift */
phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED, 0x14, 0, BIT(4));
Arınç
On 27.03.2024 18:50, Russell King (Oracle) wrote:
> On Tue, Mar 26, 2024 at 12:19:40PM +0300, Arınç ÜNAL wrote:
>> Whether a problem would happen in practice depends on when phy_init_eee()
>> fails, meaning it returns a negative non-zero code. I requested Russell to
>> review this patch to shed light on when phy_init_eee() would return a
>> negative non-zero code so we have an idea whether this patch actually fixes
>> a problem.
>
> Urgh, so I need to read the code and report back?
>
> Well, looking at phy_init_eee(), it could return a negative vallue when:
>
> 1. phydev->drv is NULL
> 2. if genphy_c45_eee_is_active() returns negative
> 3. if genphy_c45_eee_is_active() returns zero, it returns
> -EPROTONOSUPPORT
> 4. if phy_set_bits_mmd() fails (e.g. communication error with the PHY)
>
> If we then look at genphy_c45_eee_is_active(), then:
>
> genphy_c45_read_eee_adv() and genphy_c45_read_eee_lpa() propagate their
> non-zero return values, otherwise this function returns zero or positive
> integer.
>
> If we then look at genphy_c45_read_eee_adv(), then a failure of
> phy_read_mmd() would cause a negative value to be returned.
>
> Looking at genphy_c45_read_eee_lpa(), the same is true.
>
> So, it can be summarised as:
>
> - phydev->drv is NULL
> - there is a communication error accessing the PHY
> - EEE is not active
>
> otherwise, it returns zero on success.
>
> If one wishes to determine whether an error occurred vs EEE not being
> supported through negotiation for the negotiated speed, if it returns
> -EPROTONOSUPPORT in the latter case. Other error codes mean either the
> driver has been unloaded or communication error.
>
> This has been expertly determined by reading the code, which only a
> phylib maintainer has the capability of doing. Thank you for using this
> service.
Thanks for explaining it. I believe determining enabling/disabling EEE on
the switch MAC by polling the PHY, when one of the last two conditions in
your summary is true, wouldn't result in having EEE enabled. And it seems
to me that if phydev->drv is NULL, there would be bigger problems with the
device. So I think it'll be more fitting to submit this patch to net-next.
Arınç
On 27.03.2024 18:59, Russell King (Oracle) wrote:
> On Wed, Mar 27, 2024 at 03:58:13PM +0000, Russell King (Oracle) wrote:
>> On Wed, Mar 27, 2024 at 11:46:19AM +0300, [email protected] wrote:
>>> On 26.03.2024 12:19, Arınç ÜNAL wrote:
>>>> Whether a problem would happen in practice depends on when
>>>> phy_init_eee()
>>>> fails, meaning it returns a negative non-zero code. I requested Russell
>>>> to
>>>> review this patch to shed light on when phy_init_eee() would return a
>>>> negative non-zero code so we have an idea whether this patch actually
>>>> fixes
>>>> a problem.
>>>
>>> I don't suppose Russell is going to review the patch at this point. I will
>>> submit this to net-next then. If someone actually reports a problem in
>>> practice, I can always submit it to the stable trees.
>>
>> So the fact that I only saw your request this morning to look at
>> phy_init_eee(), and to review this patch... because... I work for
>> Oracle, and I've been looking at backporting Arm64 KVM patches to
>> our kernel, been testing and debugging that effort... and the
>> act that less than 24 hours had passed since you made the original
>> request... yea, sorry, it's clearly my fault for not jumping on this
>> the moment you sent the email.
>>
>> I get _so_ much email that incorrectly has me in the To: header. I
>> also get _so_ much email that fails to list me in the To: header
>> when the author wants me to respond. I don't have time to read every
>> email as it comes in. I certainly don't have time to read every
>> email in any case. I do the best I can, which varies considerably
>> with my workload.
>>
>> I already find that being single, fitting everything in during the
>> day (paid work, chores, feeding oneself) is quite a mammoth task.
>> There is no one else to do the laundry. There is no one else to get
>> the shopping. There is no one else to do the washing up. There is no
>> one else to take the rubbish out. All this I do myself, and serially
>> because there is only one of me, and it all takes time away from
>> sitting here reading every damn email as it comes in.
>>
>> And then when I end up doing something that _you_ very well could do
>> (reading the phy_init_eee() code to find out when it might return a
>> negative number) and then you send an email like this... yea... that
>> really gets my goat.
I've made the request on 21th of March. It must've been buried under the
other emails that are incorrectly sent to you as you've described. Of
course you're not in fault for not responding. I trust your expertise on
the topic so I requested your comment. You're not entitled to do that,
which is why, after waiting for about 6 days, I assumed that you're not
interested in looking at this patch, so I responded with the intention to
move on.
https://lore.kernel.org/netdev/[email protected]/
>
> ... and now I have a 1:1 with my manager for the next 30-60 minutes.
> Is it okay by you for me to be offline for that period of time while
> I have a chat with him?
That sounds exhausting. I wish things get easier for you.
Arınç