Received-SPF: pass (google.com: domain of linux-kernel+bounces-194249-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d246bd64-18b3-4002-bc71-eccd67bbd61f@quicinc.com>
Date: Wed, 29 May 2024 19:43:15 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for
 2.5G SGMII
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
CC: Andrew Halaney <ahalaney@redhat.com>, Vinod Koul <vkoul@kernel.org>,
        Bhupesh Sharma <bhupesh.sharma@linaro.org>,
        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>, <netdev@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <kernel@quicinc.com>
References: <20231218071118.21879-1-quic_snehshah@quicinc.com>
 <4zbf5fmijxnajk7kygcjrcusf6tdnuzsqqboh23nr6f3rb3c4g@qkfofhq7jmv6>
 <8b80ab09-8444-4c3d-83b0-c7dbf5e58658@quicinc.com>
 <wvzhz4fmtheculsiag4t2pn2kaggyle2mzhvawbs4m5isvqjto@lmaonvq3c3e7>
 <8f94489d-5f0e-4166-a14e-4959098a5c80@quicinc.com>
 <ZlNi11AsdDpKM6AM@shell.armlinux.org.uk>
Content-Language: en-US
From: Sneh Shah <quic_snehshah@quicinc.com>
In-Reply-To: <ZlNi11AsdDpKM6AM@shell.armlinux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 5/26/2024 9:57 PM, Russell King (Oracle) wrote:
> On Thu, Dec 21, 2023 at 02:23:57PM +0530, Sneh Shah wrote:
>> On 12/20/2023 9:29 PM, Andrew Halaney wrote:
>>> I'd evaluate if you can update that function to clear the ANE bit when
>>> the ane boolean is false. From the usage I see I feel that makes sense,
>>> but correct me if you think I'm wrong.
>>> At the very least let's use the defines from there, and possibly add a
>>> new function if clearing is not acceptable in dwmac_ctrl_ane().
>>>
>>> Stepping back, I was asking in general is the need to muck with ANE here
>>> is a Qualcomm specific problem, or is that a generic thing that should be
>>> handled in the core (and the phy_set_speed() bit stay here)? i.e. would
>>> any dwmac5 based IP need to do something like this for SPEED_2500?
>> I think disabling ANE for SPEED_2500 is generic not specific to qualcomm.
>> Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500.
>> Autoneg clause 37 stadard doesn't support 2500 speed. So we need to
>> disable autoneg for speed 2500
> 
> (Going back over the history of this addition)
> 
> What 802.3 Clause 37 says is utterly _irrelevant_ when discussing Cisco
> SGMII. Cisco took 802.3 1000base-X and modified it for their own
> purposes, changing the format of the 16-bit control word, adding support
> for symbol replication to support 100Mbps and 10Mbps, changing the link
> timer, etc. SGMII is *not* 802.3 Clause 37.
> 
> I guess you are getting caught up in the widespread crud where
> manufacturers stupidly abuse "SGMII" to mean maybe "Cisco SGMII" and
> maybe "802.3 1000base-X" because both are "serial gigabit MII". Yes,
> both are serial in nature, but Cisco SGMII is not 1000base-X and it
> also is not 2500base-X.
> 
> What makes this even more difficult is that 2500base-X was never
> standardised by the 802.3 committees until very late, so we've ended
> up with manufacturers doing their own thing for years. We've ended up
> with a mess of different implementations described in different ways
> many of which boil down to being 2500base-X without inband AN. For
> example, one manufacturer talks about "HS-SGMII", but doesn't permit
> the interface to operate at the x10 and x100 symbol replications that
> conventional Cisco SGMII uses for 100M and 10M speeds respectfully,
> making it in effect no different from 2500base-X.
> 
> Now through into this mess various implementations that do not support
> inband at 2.5G speeds, those that require inband at 2.5G speeds... one
> can get into the situation where one pairs a PHY that requires inband
> with a PCS that doesn't support it and the result doesn't work. This
> is particularly problematical if the PHY is on a hotpluggable module
> like a SFP.
> 
> It's a total trainwreck.


Qualcomm ethernet HW supports 2.5G speed in overclocked SGMII mode.
we internally term it as OCSGMII.

End goal of these patches is to enable SGMII with 2.5G speed support.
The patch in these series enabled up SGMII with 2.5 for cases where we
don't have external phy. ( mac-to-mac connectivity)
The new patch posted extends this for the case when the MAC has an
external phy connected. ( hence we are advertising fr 2.5G speed by adding
2500BASEX as supported interface in phylink)


> 
> I do have some work-in-progress patches that attempt to sort this out
> in phylink and identify incompatible situations.
> 
> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> 
> commits (I think)...
> 
> net: phylink: clean up phylink_resolve()
> 
> to:
> 
> net: phylink: switch to MLO_AN_PHY when PCS uses outband
> 
> and since I'm converting stmmac's hacky PCS that bypasses phylink to
> a real phylink_pcs, the ethqos code as it stands presents a blocker
> because of this issue. So, I'm intending to post a series in the next
> few days (after the bank holiday) and will definitely need to be
> tested on ethqos hardware.
> 

I am going over the list of these patches.

> However, first we need to get to the bottom of your latest patch that
> only sets PHY_INTERFACE_MODE_2500BASEX when plat_dat->flags has the
> STMMAC_FLAG_HAS_INTEGRATED_PCS flag _set_, but the stmmac code very
> oddly does _not_ use the built-in PCS if this flag is set. See:
> 
> 	stmmac_ethtool_get_link_ksettings()
> 	stmmac_ethtool_set_link_ksettings()
> 
> and their use of pcs_link / pcs_duplex / pcs_speed. Also see
> 
> 	stmmac_common_interrupt()
> 
> and its use of pcs_link to control the carrier, the dwmac1000 and
> dwmac4 code that reads the status from the GMAC, updating the
> pcs_link / pcs_duplex / pcs_speed variables.

In this version of qualcomm ethernet, PCS is not an independent HW
block. It is integrated to MAC block itself. It has very limited
configuration.Here PCS doesn't have it's own link speed/duplex
capabities. Hence we are bypassing all this PCS related functionalities.


I will update with more details on the integrated PCS block autoneg
standard.
>