Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 10 Jun 2022 17:36:39 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Alvin =?utf-8?Q?=C5=A0ipraga?= <alvin@pqrs.dk>
Cc:     hauke@hauke-m.de, Linus Walleij <linus.walleij@linaro.org>,
        Alvin =?utf-8?Q?=C5=A0ipraga?= <alsi@bang-olufsen.dk>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 5/5] net: dsa: realtek: rtl8365mb: handle PHY
 interface modes correctly
Message-ID: <YqNzF7KbSI9h0tSQ@shell.armlinux.org.uk>
References: <20220610153829.446516-1-alvin@pqrs.dk>
 <20220610153829.446516-6-alvin@pqrs.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220610153829.446516-6-alvin@pqrs.dk>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Jun 10, 2022 at 05:38:29PM +0200, Alvin Šipraga wrote:
> Finally, rtl8365mb_phylink_get_caps() is fixed up to return supported
> capabilities based on the external interface properties described above.
> This allows for ports with an external interface to be treated as DSA
> user ports, and for ports with an internal PHY to be treated as DSA CPU
> ports.

I've needed to read that a few times... and I'm still not sure. You seem
to be saying that:
- ports with an internal PHY (which presumably provide baseT connections?)
  are used as DSA CPU ports.
- ports with an external interface supporting a range of RGMII, SGMII and
  HSGMII interface modes are DSA user ports.

With Marvell switches, it's the other way around - the ports with an
internal PHY are normally DSA user ports. Other ports can be a user,
inter-switch or CPU port.

So, I'm slightly confused by your description.

In any ase, looking at the get_caps() (and only that):

> @@ -953,7 +1026,13 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port)) {
> +	const struct rtl8365mb_extint *extint =
> +		rtl8365mb_get_port_extint(ds->priv, port);
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;

MAC capabilities are constant across all interfaces - okay.

> +
> +	if (!extint) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
>  
> @@ -962,12 +1041,16 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  		 */
>  		__set_bit(PHY_INTERFACE_MODE_GMII,
>  			  config->supported_interfaces);
> -	} else if (dsa_is_cpu_port(ds, port)) {
> -		phy_interface_set_rgmii(config->supported_interfaces);
> +		return;

Internal ports need to support phylib, so both internal and gmii
interface modes - okay.

>  	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
> +	/* Populate according to the modes supported by _this driver_,
> +	 * not necessarily the modes supported by the hardware, some of
> +	 * which remain unimplemented.
> +	 */
> +
> +	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII)
> +		phy_interface_set_rgmii(config->supported_interfaces);

External ports that support RGMII get all RGMII modes - also okay.

So, for the get_cops() function, I'm fine with this new code, and for
this alone:

Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

I haven't looked at the remainder of the changes.

Thanks.

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