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

  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(-)
  

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!