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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç