Message-ID: <5c5xphkjbadimi3y3wzvv2hhlancxiwdtnikgl6pdv6jbbfmqu@tmfnjuxyygud>
Date: Sat, 25 May 2024 02:54:46 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Alexei Starovoitov <ast@...nel.org>, bpf@...r.kernel.org, Daniel Borkmann <daniel@...earbox.net>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, Jose Abreu <joabreu@...opsys.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-stm32@...md-mailman.stormreply.com, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

Hi Russell

On Tue, May 14, 2024 at 12:21:42AM +0100, Russell King (Oracle) wrote:
> On Tue, May 14, 2024 at 02:04:00AM +0300, Serge Semin wrote:
> > Hi Russell
> > 
> > I'll give your series a try later on this week on my DW GMAC with the
> > RGMII interface (alas I don't have an SGMII capable device, so can't
> > help with the AN-part testing).
> 
> Thanks!
> 
> > Today I've made a brief glance on it
> > and already noted a few places which may require a fix to make the
> > change working for RGMII (at least the RGSMIIIS IRQ must be got back
> > enabled). After making the patch set working for my device in what
> > form would you prefer me to submit the fixes? As incremental patches
> > in-reply to this thread?
> 
> I think it depends on where the issues are.
> 
> If they are addressing issues that are also present in the existing
> code, then it would make sense to get those patched as the driver
> stands today, because backporting them to stable would be easier.
> 
> If they are for "new" issues, given that this patch series is more
> or less experimental, I would prefer to roll them into these
> patches. As mentioned, when it comes to submitting these patches,
> the way I've split them wouldn't make much sense, but it does
> make sense for where I am with it. Hence, I'll want to resplit
> the series into something better for submission than it currently
> is. If you want to reply to this thread, that is fine.

I've just submitted the fixes for your series
https://lore.kernel.org/netdev/20240524210304.9164-1-fancer.lancer@gmail.com/
which make it working well on my hardware: DW GMAC v3.73 with RGMII
PHY interface connected to the Micrel KSZ9031RNX PHY. (For a lucky
coincident the PHY happen to support the link status sent in-band up
to the MAC.) So as long as the managed="in-band-status" property is
specified the PCS subsystem gets the link-status by means of the
pcs_get_state() callback. The status change is signaled by means of
the RGSMIIIS IRQ.  For that to work the Patch 2 of my series was
required (and of course Patch 1 which prevents the IRQs flood).

I'm sorry for submitting the series only today. First I had to dig
deeper into the way the RGMII In-band/PCS-block of the controller
works. Then I needed some time to study the STMMAC PCS-code and to
debug the problems fixed in my series. So I finished testing your
patchset on this Monday. Then I decided to spend sometime for making
the PCS implementation looking more optimized based on the knowledge I
gained during the debugging. But as it's normal for the STMMAC driver
(which sucks indeed) a few days wasn't enough for that, because due to
the driver being overwhelmed with duty hacks any more-or-less invasive
refactoring may lead to regressions here or there. So I stuck right at
the first step of getting the "snps,ps-speed" and the MAC2MAC mode
well implemented...(

Anyway here is the key points regarding the RGMII In-band and
PCS-interface implemented in the DW GMAC and DW QoS Eth
controllers/drivers:

1. Intermediate PCS for which the plat_stmmacenet_data::mac_interface
field and the "mac-mode" property was introduced isn't the case of the
PCS embedded into the DW GMAC and DW QoS Eth IP-cores by Synopsys.
That was some another PCS likely specific for the Altera SoC FPGA
platform (dwmac-socfpga.c).

2. RGMII: There is no any PCS-block utilized in case of the RGMII PHY
interface (that's why HWFEATURE.PCSSEL flag isn't set). The networking
controller provides a way to pick up the RGMII In-band status
delivered from the attached PHY. The in-band status is updated in the
GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS Eth)
registers and signalled via the RGSMIIIS MAC IRQ.

3. SGMII: The interface implementation has a PCS-block so the
HWFEATURE.PCSSEL flag is set. But the auto-negotiation procedure
complies to the SGMII specification: no ability advertisement. SGMII
PHY sends the control information back to the MAC by means of the
tx_config_Reg[15:0] register. MAC either acknowledges the update or
not. The control information retrieved from the PHY is reflected in
the GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS
Eth) registers. The only AN-related CSR available for the SGMII
interface are GMAC_AN_CTRL(x) and GMAC_AN_STATUS(x) since no
advertisement implied by the specification.

4. RGMII/SGMII/SMII: Note CSR-wise the tx_config_Reg[15:0] register
mapping is the same for all of these interfaces. It's available in the
GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS Eth)
CSRs: in case of the DW GMAC it's GMAC_RGSMIIIS[0:15] bits, but in
case of DW QoS Eth it's GMAC_PHYIF_CTRLSTATUS[16:31]. (This info can
be useful to create a common dwmac_inband_pcs_get_state() method
implementation in the stmmac_pcs.c module.)

5. TBI/RTBI: It has a traditional auto-negotiation procedure fully
complying to the IEEE 802.3z C37 specification with the link abilities
advertisement. RBI/RTBI don't imply any in-band link status detection
so the GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW
QoS Eth) CSRs aren't available for these interfaces.

6. RGMII/SGMII/SMII: In order to have the Link Speed
(GMAC_CONTROL.{PS,FES}), Duplex mode (GMAC_CONTROL.DM) and Link
Up/Down bit (GMAC_CONTROL.LUD or GMAC_PHYIF_CTRLSTATUS.LUD)
transferred from MAC to the attached PHY or to another MAC via
tx_config_Reg[15:0], the GMAC_CONTROL.TC (DW GMAC) or
GMAC_PHYIF_CTRLSTATUS.TC (DW QoS Eth) flags must be set. Just a note
seeing the current PCS implementation doesn't do that even in case of
the fixed Port-select speed setup (when snps,ps-speed property is
specified).


Based on the info above I was going to extend your stmmac_pcs.c module
with the inband link status (retrieved via the tx_config_Reg[15:0])
parsing method; create more basic PCS-ops in the framework of the
dwmac1000_core.c and dwmac4_core.c modules, and the common
phylink_pcs_ops in the stmmac_main.c module using those basic PCS-ops.
But as I mentioned before I was stuck on the fixed Port-select speed
implementation. It's activated via the "snps,ps-speed" property. If
it's specified the AN_Control.SGMRAL flag will be set which makes the
SGMII interface working with a fixed speed pre-initialized in the
MAC_CONFIG.{PS,FES} fields. First of all I wasn't sure whether the
MAC2MAC functionality it's utilized for, can be applicable for
non-SGMII interface. Secondly the port speed fixing is performed
behind the phylink back. It's possible to have the speed setting being
updated later in framework of the mac_link_up() callback. So I have
some doubts that the current "snps,ps-speed"-based fixed port speed
implementation is fully correct.

That's the stage at which I decided to stop further researches and
sent my series of fixes to you.

-Serge(y)

> 
> There's still a few netif_carrier_off()/netif_carrier_on()s left
> in the driver even after this patch series, but I think they're in
> more obscure paths, but I will also want to address those as well.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists