Message-ID: <ae4b3iobvbdyyijkpqhh4xv32rnfql2nvzmlzvmfbluefecy7z@t5o42w4orpfi>
Date: Thu, 17 Jul 2025 20:22:44 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "G Thomas, Rohan" <rohan.g.thomas@...era.com>
Cc: Andrew Lunn <andrew@...n.ch>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Romain Gantois <romain.gantois@...tlin.com>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Matthew Gerlach <matthew.gerlach@...era.com>
Subject: Re: [PATCH net-next 2/3] net: stmmac: xgmac: Correct supported speed
 modes

On Thu, Jul 17, 2025 at 06:29:33PM +0530, G Thomas, Rohan wrote:
> Hi Serge,
> 
> Thanks for the review comments and the detailed explanation.
> 
> On 7/17/2025 5:17 PM, Serge Semin wrote:
> > On Tue, Jul 15, 2025 at 07:03:58PM +0530, G Thomas, Rohan wrote:

...

> > > 
> > > > 
> > > > What does XGMAC_HWFEAT_GMIISEL mean? That a SERDES style interface is
> > > > not being used? Could that be why Serge removed these speeds? He was
> > > > looking at systems with a SERDES, and they don't support slower
> > > > speeds?
> > > > 
> > > > 	Andrew
> > > As per the XGMAC databook ver 3.10a, GMIISEL bit of MAC_HW_Feature_0
> > > register indicates whether the XGMAC IP on the SOC is synthesized with
> > > DWCXG_GMII_SUPPORT. Specifically, it states:
> > > "1000/100/10 Mbps Support. This bit is set to 1 when the GMII Interface
> > > option is selected."
> > > 
> > > So yes, it’s likely that Serge was working with a SERDES interface which
> > > doesn't support 10/100Mbps speeds. Do you think it would be appropriate
> > > to add a check for this bit before enabling 10/100Mbps speeds?
> > 
> > DW XGMAC IP-core of v2.x and older don't support 10/100Mbps modes
> > neither in the XGMII nor in the GMII interfaces. That's why I dropped
> > the 10/100Mbps link capabilities retaining 1G, 2.5G and 10G speeds
> > only (the only speeds supported for DW XGMAC 1.20a/2.11a Tx in the
> > MAC_Tx_Configuration.SS register field). Although I should have
> > dropped the MAC_5000FD too since it has been supported since v3.0
> > IP-core version. My bad.(
> > 
> > Starting from DW XGMAC v3.00a IP-core the list of the supported speeds
> > has been extended to: 10/100Mbps (MII), 1G/2.5G (GMII), 2.5G/5G/10G
> > (XGMII). Thus the more appropriate fix here should take into account
> > the IP-core version. Like this:
> > 	if (dma_cap->mbps_1000 && MAC_Version.SNPSVER >= 0x30)
> > 		dma_cap->mbps_10_100 = 1;
> >  > Then you can use the mbps_1000 and mbps_10_100 flags to set the proper
> > MAC-capabilities to hw->link.caps in the dwxgmac2_setup() method. I
> > would have added the XGMII 2.5G/5G MAC-capabilities setting up to the
> > dwxgmac2_setup() method too for the v3.x IP-cores and newer.
> > 
> 
> Agreed. Will do in the next version.
> 
> Will ensure that mbps_10_100 is set only if SNPSVER >= 0x30 and will
> also conditionally enable 2.5G/5G MAC capabilities for IP versions
> v3.00a and above.
> 
> In the stmmac_dvr_probe() the setup() callback is invoked before
> hw_cap_support is populated. Given that, do you think it is sufficient
> to add these checks into the dwxgmac2_update_caps() instead?

Arrgh, I was looking at my local repo with a refactored hwif initialization
procedure which, amongst other things, implies the HW-features detection
performed in the setup methods.(
So in case of the vanilla STMMAC driver AFAICS there are three options
here:

1. Repeat what I did in my local repo and move the HW-features
detection procedure (calling the *_get_hw_feature() functions) to the
*_setup() methods. After that change you can use the retrieved
dma_cap-data to init the MAC capabilities exactly in sync to the
detected HW-features. But that must be thoroughly thought through
since there are Sun8i and Loongson MACs which provide their own HWIF
setup() methods (by means of plat_stmmacenet_data::setup()). So the
respective *_get_hw_feature() functions might need to be called in
these methods too (at least in the Loongson MACs setup() method).

2. Define new dwxgmac3_setup() method and thus a new entry in
stmmac_hw[]. Then dwxgmac2_setup() could be left with only 1G, 2.5G
and 10G MAC-capabilities declared, meanwhile dwxgmac3_setup() would
add all the DW XGMAC v3.00a MAC-capabilities. In this case you'd need
the dwxgmac2_update_caps() method defined anyway in order to filter
out the MAC-capabilities based on the
dma_features::{mbps_1000,mbps_10_100,half_duplex} flags state.

3. As you suggest indeed declare all the possible DW XGMAC
MAC-capabilities in the dwxgmac2_setup() method and then filter them
out in dwxgmac2_update_caps() based on the
dma_features::{mbps_1000,mbps_10_100,half_duplex} flags state and
IP-core version.

The later option seems the least code-invasive but implements a more
complex logic with declaring all the possible MAC-capabilities and
then filtering them out. Option two still implies filtering the
MAC-capabilities out but only from those specific for the particular
IP-core version. Finally option one is more complex to implement
implying the HWIF procedure refactoring with higher risks to break
things, but after it's done the setup() methods will turn to a more
useful procedures which could be used not only for the more exact
MAC-capabilities initialization but also for other
data/fields/callbacks setting up.

It's up to you and the maintainers to decide which solution would be
more appropriate.

-Serge(y)

> 
> > -Serge(y)
> > 
> > > 
> > > Best Regards,
> > > Rohan
> > > 
> 
> Best Regards,
> Rohan
> 

Powered by blists - more mailing lists