Message-ID: <f568bfdf-31af-4589-97c8-744a63a2f67c@altera.com>
Date: Fri, 18 Jul 2025 17:08:46 +0530
From: "G Thomas, Rohan" <rohan.g.thomas@...era.com>
To: Serge Semin <fancer.lancer@...il.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

Hi Serge,

Thanks for the detailed response.

On 7/17/2025 10:52 PM, Serge Semin wrote:
>>> 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)
> 

Unless there are concerns, I'll proceed with option 3 for now, as it’s
the least invasive and aligns well with the current driver structure.
I’ll declare all possible DW XGMAC MAC-capabilities in dwxgmac2_setup()
and then filter them appropriately in dwxgmac2_update_caps() based on
the dma_features flags and IP-core version.

Let me know if any concerns with this approach.

Best Regards,
Rohan

Powered by blists - more mailing lists