Re: [PATCH net-next 08/30] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

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

On 4.06.2023 18:13, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
>> On 4.06.2023 16:07, Russell King (Oracle) wrote:
>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
>>>>> I don't remember whether Vladimir's firmware validator will fail for
>>>>> mt753x if CPU ports are not fully described, but that would be well
>>>>> worth checking. If it does, then we can be confident that phylink
>>>>> will always be used, and those bypassing calls should not be necessary.
>>>>
>>>> It does, I've just retested this:
>>>>
>>>> [    8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
>>>> [    8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>>>> [    8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
>>>
>>> ... which isn't listed in dsa_switches_apply_workarounds[], and
>>> neither is mt753x. Thanks.
>>>
>>> So, that should be sufficient to know that the CPU port will always
>>> properly described, and thus bypassing phylink in mt753x for the CPU
>>> port should not be necessary.
>>
>> Perfect! If I understand correctly, there's this code - specific to MT7531
>> and MT7988 ports being used as CPU ports - which runs in addition to what's
>> in mt753x_phylink_mac_config():
>>
>> 	mt7530_write(priv, MT7530_PMCR_P(port),
>> 		     PMCR_CPU_PORT_SETTING(priv->id));
>>
>> This should be put on mt753x_phylink_mac_config(), under priv->id ==
>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
> 
> Please remember that I have very little knowledge of MT753x, so in
> order to answer this question, I've read through the mt7530 driver
> code.
> 
> Looking at mt7530.h:
> 
> #define  PMCR_CPU_PORT_SETTING(id)      (PMCR_FORCE_MODE_ID((id)) | \
>                                           PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>                                           PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>                                           PMCR_TX_EN | PMCR_RX_EN | \
>                                           PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
>                                           PMCR_FORCE_SPEED_1000 | \
>                                           PMCR_FORCE_FDX | PMCR_FORCE_LNK)
> 
> This seems to be some kind of port control register that sets amongst
> other things parameters such as whether flow control is enabled, the
> port speed, the duplex setting, whether link is forced up, etc.
> 
> Looking at what mt753x_phylink_mac_link_up() does:
> 
> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
>     an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
> 
> So, provided this is called with the appropriate parameters, for a
> fixed link, that will leave the following:
> 
> 	PMCR_FORCE_MODE_ID(id)
> 	PMCR_IFG_XMIT(1)
> 	PMCR_MAC_MODE
> 	PMCR_BACKOFF_EN
> 	PMCR_BACKPR_EN
> 
> If we now look at mt753x_phylink_mac_config(), this sets
> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
> PMCR_CPU_PORT_SETTING(priv->id) is doing.
> 
> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
> with pause cause phylink to call both mt753x_phylink_mac_config() and
> mt753x_phylink_mac_link_up() with appropriate arguments to set all
> of these parameters in PMCR?
> 
> Now, I'm going to analyse something else. mt7531_cpu_port_config()
> is called from mt753x_cpu_port_enable(), which is itself called from
> mt7531_setup_common(). That is ultimately called from the DSA switch
> ops .setup() method.
> 
> This method is called from dsa_switch_setup() for each switch in the
> DSA tree. dsa_tree_setup_switches() calls this, and is called from
> dsa_tree_setup().  Once dsa_tree_setup_switches() finishes
> successfully, dsa_tree_setup_ports() will be called. This will then
> setup DSA and CPU ports, which will then setup a phylink instance
> for these ports. phylink will parse the firmware description for
> the port. DSA will then call dsa_port_enable().
> 
> dsa_port_enable() will then call any port_enable() method in the
> mt7530.c driver, which will be mt7530_port_enable(). This then...
> 
>          mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
> 
> which is:
> 
> #define  PMCR_LINK_SETTINGS_MASK        (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
>                                           PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
>                                           PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
>                                           PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
>                                           PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
> 
> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
> performed - undoing *everything* below that switch() statement in
> mt7531_cpu_port_config()!
> 
> Once the port_enable() method returns, DSA will then call
> phylink_start(), which will trigger phylink to bring up the link
> according to the settings it has, which will mean phylink calls
> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
> with the appropriate parameters for the firmware described link.

I'm slowly learning how DSA and phylink works, this is the full code
path I could make up for the MT7530 DSA subdriver:

mt7530_probe() & mt7988_probe()
    -> mt7530_probe_common()
    -> dsa_register_switch()
       -> dsa_switch_probe()
          -> dsa_tree_setup()
             -> dsa_tree_setup_switches()
                -> dsa_switch_setup()
                   -> ds->ops->setup(): mt753x_setup()
                      -> mt7530_setup()
                         -> mt753x_cpu_port_enable()
                      -> mt7531_setup()
                         -> mt7531_setup_common()
                            -> mt753x_cpu_port_enable()
                               -> priv->info->cpu_port_config():
                                  mt7531_cpu_port_config()
                      -> mt7988_setup()
                         -> mt7531_setup_common()
                            -> mt753x_cpu_port_enable()
                               -> priv->info->cpu_port_config():
                                  mt7988_cpu_port_config()
             -> dsa_tree_setup_ports()
                -> dsa_port_setup()
                   -> dsa_shared_port_link_register_of()
                      -> dsa_shared_port_link_register_of()
                         -> dsa_shared_port_phylink_register()
                            -> dsa_port_phylink_create()
                               -> ds->ops->phylink_get_caps():
                                  mt753x_phylink_get_caps()
                               -> phylink_create()
                                  -> INIT_WORK(&pl->resolve, phylink_resolve)
                   -> dsa_port_enable()
                      -> dsa_port_enable_rt()
                         -> ds->ops->port_enable():
                            mt7530_port_enable()
                         -> phylink_start()
                            -> phylink_mac_initial_config()
                               -> phylink_major_config()
                                  -> phylink_mac_config()
                                     -> pl->mac_ops->mac_config():
                                        dsa_port_phylink_mac_config()
                                        -> ds->ops->phylink_mac_config():
                                           mt753x_phylink_mac_config()
                                  -> pl->pcs->ops->pcs_config():
                                     mt753x_pcs_config()
                            -> phylink_enable_and_run_resolve()
                               -> phylink_run_resolve()
                                  -> queue_work(system_power_efficient_wq, &pl->resolve)
                                     -> phylink_link_up()
                                        -> pl->pcs->ops->pcs_link_up():
                                           mtk_pcs_lynxi_link_up()
                                        -> pl->mac_ops->mac_link_up():
                                           dsa_port_phylink_mac_link_up()
                                           -> ds->ops->phylink_mac_link_up():
                                              mt753x_phylink_mac_link_up()

> 
> So I think I have the answer to my initial thought: do the calls in
> mt7531_cpu_port_config() to the phylink methods have any use what so
> ever? The answer is no, they are entirely useless. The same goes for
> the other cpu_port_config() methods that do something similar. The
> same goes for the PMCR register write that's changing any bits
> included in PMCR_LINK_SETTINGS_MASK.
> 
> What that means is that mt7988_cpu_port_config() can be entirely
> removed, it serves no useful purpose what so ever. For
> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
> which, as far as I can see, probably only avoids mac_config() doing
> any pad setup (that's a guess.)

This is what I also believe and the reason why I made this patch to
simplify it. Looks like I'll just remove priv->info->cpu_port_config()
instead.

Arınç