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

[Arınç ÜNAL]

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. 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ç