Re: [PATCH net] net: dsa: mt7530: fix 10M/100M speed on MT7988 switch

[Arınç ÜNAL]

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@xxxxxxxxxxxxxx>
> > > >
> > > > Acked-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> > > >
> > > > 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ç