Received-SPF: pass (google.com: domain of linux-kernel+bounces-189785-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 26 May 2024 17:49:48 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Byungho An <bh74.an@samsung.com>,
	Giuseppe CAVALLARO <peppe.cavallaro@st.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>, bpf@vger.kernel.org,
	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag
 based on the selected iface
Message-ID: <ZlNoLHoHjt3BsFde@shell.armlinux.org.uk>
References: <ZkDuJAx7atDXjf5m@shell.armlinux.org.uk>
 <20240524210304.9164-1-fancer.lancer@gmail.com>
 <20240524210304.9164-2-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240524210304.9164-2-fancer.lancer@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> into the DW GMAC controller. It's always done if the controller supports
> at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> interfaces support was activated during the IP-core synthesize the PCS
> block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> set. Based on that the RGMII in-band status detection procedure
> implemented in the driver hasn't been working for the devices with the
> RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> interfaces available in the device.
> 
> Fix that just by dropping the dma_cap.pcs flag check from the conditional
> statement responsible for the In-band/PCS functionality activation. If the
> RGMII interface is supported by the device then the in-band link status
> detection will be also supported automatically (it's always embedded into
> the RGMII RTL code). If the SGMII interface is supported by the device
> then the PCS block will be supported too (it's unconditionally synthesized
> into the controller). The later is also correct for the TBI/RTBI PHY
> interfaces.
> 
> Note while at it drop the netdev_dbg() calls since at the moment of the
> stmmac_check_pcs_mode() invocation the network device isn't registered. So
> the debug prints will be for the unknown/NULL device.

Thanks. As this is a fix, shouldn't it be submitted for the net tree as
it seems to be fixing a bug in the driver as it stands today?

Also, a build fix is required here:

> -	if (priv->dma_cap.pcs) {
> -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> -			priv->hw->pcs = STMMAC_PCS_RGMII;
> -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> -			priv->hw->pcs = STMMAC_PCS_SGMII;
> -		}
> -	}
> +	if (phy_interface_mode_is_rgmii(interface))
> +		priv->hw.pcs = STMMAC_PCS_RGMII;
> +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> +		priv->hw.pcs = STMMAC_PCS_SGMII;

Both of these assignments should be priv->hw->pcs not priv->hw.pcs.

I think there's also another bug that needs fixing along with this.
See stmmac_ethtool_set_link_ksettings(). Note that this denies the
ability to disable autoneg, which (a) doesn't make sense for RGMII
with an attached PHY, and (b) this code should be passing the
ethtool op to phylink for it to pass on to phylib so the PHY can
be appropriately configured for the users desired autoneg and
link mode settings.

I also don't think it makes any sense for the STMMAC_PCS_SGMII case
given that it means Cisco SGMII - which implies that there is also
a PHY (since Cisco SGMII with inband is designed to be coupled with
something that looks like a PHY to send the inband signalling
necessary to configure e.g. the SGMII link symbol replication.

In both of these cases, even if the user requests autoneg to be
disabled, that _shouldn't_ affect internal network driver links.
This ethtool op is about configuring the externally visible media
side of the network driver, not the internal links.

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