Message-ID: <ZHyqI2oOI4KkvgB8@shell.armlinux.org.uk>
Date:   Sun, 4 Jun 2023 16:13:39 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Daniel Golle <daniel@...rotopia.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Richard van Schagen <richard@...terhints.com>,
        Richard van Schagen <vschagen@...com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
        erkin.bozoglu@...ont.com, mithat.guner@...ont.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 08/30] net: dsa: mt7530: change p{5,6}_interface
 to p{5,6}_configured

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@...000000/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.

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

At least that's what I gather from reading through the driver and
DSA code. It may be I've missed something, but currently, I think
that these cpu_port_config() functions aren't doing too much that
is actually useful work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists