| Message ID | a5b04dfa8256d8302f402545a51ac4c626fdba25.1706071272.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dfa988b4c7c3a48bde7c2713308920c7741fff29 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: mt7530: fix 10M/100M speed on MT7988 switch | expand |
On Wed, Jan 24, 2024 at 05:17:25AM +0000, Daniel Golle wrote: > Setup PMCR port register for actual speed and duplex on internally > connected PHYs of the MT7988 built-in switch. This fixes links with > speeds other than 1000M. > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
On 24/01/2024 08:17, Daniel Golle wrote: > Setup PMCR port register for actual speed and duplex on internally > connected PHYs of the MT7988 built-in switch. This fixes links with > speeds other than 1000M. > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") > Signed-off-by: Daniel Golle <daniel@makrotopia.org> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> I'm wondering why we manually set speed and duplex for these interface modes in the first place. I don't how it works for PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and 802.3z interfaces, phylink should already supply proper speed and duplex. Arınç
On Thu, Jan 25, 2024 at 12:49:19PM +0300, Arınç ÜNAL wrote: > On 24/01/2024 08:17, Daniel Golle wrote: > > Setup PMCR port register for actual speed and duplex on internally > > connected PHYs of the MT7988 built-in switch. This fixes links with > > speeds other than 1000M. > > > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > I'm wondering why we manually set speed and duplex for these interface > modes in the first place. I don't how it works for > PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and > 802.3z interfaces, phylink should already supply proper speed and duplex. It's true that duplex should always be set to full-duplex already by phylink. However, speed could be 2500MBit/s (2500Base-X) or 2000MBit/s (?, TRGMII) and we yet need to program the PCR like if it was 1000MBit/s. Regarding the INTERNAL case: it was added by mistake. In case of MT7988, all ports of the switch are connected via INTERNAL links, however, the PHYs still need adjustment of the PCR register just like on all other MT753x switches and the CPU port is setup elsewhere anyway.
On 25.01.2024 19:18, Daniel Golle wrote: > On Thu, Jan 25, 2024 at 12:49:19PM +0300, Arınç ÜNAL wrote: >> On 24/01/2024 08:17, Daniel Golle wrote: >>> Setup PMCR port register for actual speed and duplex on internally >>> connected PHYs of the MT7988 built-in switch. This fixes links with >>> speeds other than 1000M. >>> >>> Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >> >> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> >> I'm wondering why we manually set speed and duplex for these interface >> modes in the first place. I don't how it works for >> PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and >> 802.3z interfaces, phylink should already supply proper speed and duplex. > > It's true that duplex should always be set to full-duplex already by > phylink. However, speed could be 2500MBit/s (2500Base-X) or 2000MBit/s > (?, TRGMII) and we yet need to program the PCR like if it was > 1000MBit/s. > > Regarding the INTERNAL case: it was added by mistake. In case of > MT7988, all ports of the switch are connected via INTERNAL links, > however, the PHYs still need adjustment of the PCR register just like > on all other MT753x switches and the CPU port is setup elsewhere > anyway. It's not necessarily PHYs needing adjustment of the port MAC control register. After reset, speed, duplex mode, etc. will be determined by polling the PHY connected to the switch MAC. We're forcing these properties on the PMCR because we're also configuring switch MACs that are not connected to PHYs, meaning the switch cannot determine these properties by polling a PHY. From what I understand, this code block is for overriding the speed and duplex variables to make the operations on the PMCR below work. It seems that this is actually only useful for PHY_INTERFACE_MODE_2500BASEX. PHY_INTERFACE_MODE_TRGMII is given SPEED_1000 by drivers/net/phy/phylink.c:phylink_interface_max_speed(). PHY_INTERFACE_MODE_2500BASEX is given SPEED_2500. Overriding the duplex variable looks unnecessary. Your patch here doesn't affect CPU ports because MT7531 and MT7988 PMCRs are configured with cpu_port_config before mt753x_phylink_mac_link_up(), and PHY_INTERFACE_MODE_INTERNAL is not used for MT7530 which, for MT7530, PMCRs will be set only on mt753x_phylink_mac_link_up(). PMCR_FORCE_SPEED_1000 is set on cpu_port_config. If someone were to get rid of cpu_port_config because of its utter uselessness, PMCR_FORCE_SPEED_1000 would not be set, causing the link between port 6 MAC and SoC MAC to break. In conclusion, I will add "case SPEED_10000:" to the operations where the speed and EEE bits are set on my patch for getting rid of cpu_port_config. Arınç
On Fri, Jan 26, 2024 at 01:44:57AM +0300, Arınç ÜNAL wrote: > On 25.01.2024 19:18, Daniel Golle wrote: > > On Thu, Jan 25, 2024 at 12:49:19PM +0300, Arınç ÜNAL wrote: > > > On 24/01/2024 08:17, Daniel Golle wrote: > > > > Setup PMCR port register for actual speed and duplex on internally > > > > connected PHYs of the MT7988 built-in switch. This fixes links with > > > > speeds other than 1000M. > > > > > > > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > > > > Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > > > > > I'm wondering why we manually set speed and duplex for these interface > > > modes in the first place. I don't how it works for > > > PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and > > > 802.3z interfaces, phylink should already supply proper speed and duplex. > > > > It's true that duplex should always be set to full-duplex already by > > phylink. However, speed could be 2500MBit/s (2500Base-X) or 2000MBit/s > > (?, TRGMII) and we yet need to program the PCR like if it was > > 1000MBit/s. > > > > Regarding the INTERNAL case: it was added by mistake. In case of > > MT7988, all ports of the switch are connected via INTERNAL links, > > however, the PHYs still need adjustment of the PCR register just like > > on all other MT753x switches and the CPU port is setup elsewhere > > anyway. > > It's not necessarily PHYs needing adjustment of the port MAC control > register. It's not the PHYs which need adjustment but the MAC PMCR register which does. > After reset, speed, duplex mode, etc. will be determined by polling > the PHY connected to the switch MAC. Yes, but as it is a DSA driver we don't use **hardware** PHY polling and keep that off. Instead, PHY interrupts or software PHY polling is used to have Linux determine the link properties. We're then forcing these properties on the MAC port of the switch. > on the PMCR because we're also configuring switch MACs that are not > connected to PHYs, meaning the switch cannot determine these properties by > polling a PHY. The switch **never** determines these properties itself when using the DSA driver. It has a facility to do so, and yes, when accessing Ethernet in U-Boot or when using the 'swconfig'-based driver then this facility is used. But, I repeat, when using DSA we do not use hardware PHY polling. We poll (or rather: react to interrupts) in software instead. > > From what I understand, this code block is for overriding the speed and > duplex variables to make the operations on the PMCR below work. It seems > that this is actually only useful for PHY_INTERFACE_MODE_2500BASEX. > PHY_INTERFACE_MODE_TRGMII is given SPEED_1000 by > drivers/net/phy/phylink.c:phylink_interface_max_speed(). > PHY_INTERFACE_MODE_2500BASEX is given SPEED_2500. Overriding the duplex > variable looks unnecessary. > > Your patch here doesn't affect CPU ports because MT7531 and MT7988 PMCRs This patch does not intend to affect the CPU port. As I've already said in my previous reply "[...] the CPU port is setup elsewhere anyway." Maybe it wasn't clear, but I meant that the CPU port settings are intentionally unaffected by this patch. It is intended to affect user ports which with phy-mode = "internal"; set in DTS -- here we **do** need to set PMCR according the external link speed and duplex. > are configured with cpu_port_config before mt753x_phylink_mac_link_up(), > and PHY_INTERFACE_MODE_INTERNAL is not used for MT7530 which, for MT7530, > PMCRs will be set only on mt753x_phylink_mac_link_up(). > > PMCR_FORCE_SPEED_1000 is set on cpu_port_config. If someone were to get rid > of cpu_port_config because of its utter uselessness, PMCR_FORCE_SPEED_1000 > would not be set, causing the link between port 6 MAC and SoC MAC to break. > > In conclusion, I will add "case SPEED_10000:" to the operations where the > speed and EEE bits are set on my patch for getting rid of cpu_port_config. PMCR needs to be set according to actual link speed for built-in TP PHYs. This is true for all mt7530 variants including MT7988. Maybe the confusion here is that on MT7988 we use 'internal' phy-mode for both, the switch CPU port's link to mtk_eth_soc gmac0 as well as the links to the 4 built-in 1GE switch PHYs. The latter were affected by wrongly overriding speed and duplex in case phy-mode is set to "internal", which should not have been put there (by me) in first place. Let's just remove it, ok?
On 26.01.2024 02:57, Daniel Golle wrote: > On Fri, Jan 26, 2024 at 01:44:57AM +0300, Arınç ÜNAL wrote: >> On 25.01.2024 19:18, Daniel Golle wrote: >>> On Thu, Jan 25, 2024 at 12:49:19PM +0300, Arınç ÜNAL wrote: >>>> On 24/01/2024 08:17, Daniel Golle wrote: >>>>> Setup PMCR port register for actual speed and duplex on internally >>>>> connected PHYs of the MT7988 built-in switch. This fixes links with >>>>> speeds other than 1000M. >>>>> >>>>> Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") >>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>>> >>>> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> >>>> >>>> I'm wondering why we manually set speed and duplex for these interface >>>> modes in the first place. I don't how it works for >>>> PHY_INTERFACE_MODE_INTERNAL but, at least for PHY_INTERFACE_MODE_TRGMII and >>>> 802.3z interfaces, phylink should already supply proper speed and duplex. >>> >>> It's true that duplex should always be set to full-duplex already by >>> phylink. However, speed could be 2500MBit/s (2500Base-X) or 2000MBit/s >>> (?, TRGMII) and we yet need to program the PCR like if it was >>> 1000MBit/s. >>> >>> Regarding the INTERNAL case: it was added by mistake. In case of >>> MT7988, all ports of the switch are connected via INTERNAL links, >>> however, the PHYs still need adjustment of the PCR register just like >>> on all other MT753x switches and the CPU port is setup elsewhere >>> anyway. >> >> It's not necessarily PHYs needing adjustment of the port MAC control >> register. > > It's not the PHYs which need adjustment but the MAC PMCR register which > does. > >> After reset, speed, duplex mode, etc. will be determined by polling >> the PHY connected to the switch MAC. > > Yes, but as it is a DSA driver we don't use **hardware** PHY polling > and keep that off. Instead, PHY interrupts or software PHY polling is > used to have Linux determine the link properties. > We're then forcing these properties on the MAC port of the switch. > >> on the PMCR because we're also configuring switch MACs that are not >> connected to PHYs, meaning the switch cannot determine these properties by >> polling a PHY. > > The switch **never** determines these properties itself when using the > DSA driver. It has a facility to do so, and yes, when accessing > Ethernet in U-Boot or when using the 'swconfig'-based driver then this > facility is used. But, I repeat, when using DSA we do not use hardware > PHY polling. We poll (or rather: react to interrupts) in software > instead. > >> >> From what I understand, this code block is for overriding the speed and >> duplex variables to make the operations on the PMCR below work. It seems >> that this is actually only useful for PHY_INTERFACE_MODE_2500BASEX. >> PHY_INTERFACE_MODE_TRGMII is given SPEED_1000 by >> drivers/net/phy/phylink.c:phylink_interface_max_speed(). >> PHY_INTERFACE_MODE_2500BASEX is given SPEED_2500. Overriding the duplex >> variable looks unnecessary. >> >> Your patch here doesn't affect CPU ports because MT7531 and MT7988 PMCRs > > This patch does not intend to affect the CPU port. As I've already > said in my previous reply "[...] the CPU port is setup elsewhere > anyway." > > Maybe it wasn't clear, but I meant that the CPU port settings are > intentionally unaffected by this patch. > > It is intended to affect user ports which with phy-mode = "internal"; > set in DTS -- here we **do** need to set PMCR according the external > link speed and duplex. > > >> are configured with cpu_port_config before mt753x_phylink_mac_link_up(), >> and PHY_INTERFACE_MODE_INTERNAL is not used for MT7530 which, for MT7530, >> PMCRs will be set only on mt753x_phylink_mac_link_up(). >> >> PMCR_FORCE_SPEED_1000 is set on cpu_port_config. If someone were to get rid >> of cpu_port_config because of its utter uselessness, PMCR_FORCE_SPEED_1000 >> would not be set, causing the link between port 6 MAC and SoC MAC to break. >> >> In conclusion, I will add "case SPEED_10000:" to the operations where the >> speed and EEE bits are set on my patch for getting rid of cpu_port_config. > > PMCR needs to be set according to actual link speed for built-in TP > PHYs. This is true for all mt7530 variants including MT7988. > > Maybe the confusion here is that on MT7988 we use 'internal' phy-mode > for both, the switch CPU port's link to mtk_eth_soc gmac0 as well as > the links to the 4 built-in 1GE switch PHYs. > > The latter were affected by wrongly overriding speed and duplex in > case phy-mode is set to "internal", which should not have been put > there (by me) in first place. > > Let's just remove it, ok? I don't see anything I disagree with on your reply. I've made my response to explain what I understand and how I will adapt my future changes accordingly with this patch so as to prevent introducing another issue. I've already acknowledged this patch! Arınç
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 24 Jan 2024 05:17:25 +0000 you wrote: > Setup PMCR port register for actual speed and duplex on internally > connected PHYs of the MT7988 built-in switch. This fixes links with > speeds other than 1000M. > > Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > [...] Here is the summary with links: - [net] net: dsa: mt7530: fix 10M/100M speed on MT7988 switch https://git.kernel.org/netdev/net/c/dfa988b4c7c3 You are awesome, thank you!
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index f8ecc354630b0..fbd52ff7cbd6e 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2843,8 +2843,7 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port, /* MT753x MAC works in 1G full duplex mode for all up-clocked * variants. */ - if (interface == PHY_INTERFACE_MODE_INTERNAL || - interface == PHY_INTERFACE_MODE_TRGMII || + if (interface == PHY_INTERFACE_MODE_TRGMII || (phy_interface_mode_is_8023z(interface))) { speed = SPEED_1000; duplex = DUPLEX_FULL;
Setup PMCR port register for actual speed and duplex on internally connected PHYs of the MT7988 built-in switch. This fixes links with speeds other than 1000M. Fixes: ("110c18bfed414 net: dsa: mt7530: introduce driver for MT7988 built-in switch") Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/dsa/mt7530.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)