[PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[mohammad.athari.ismail]

From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>

Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet for
10/100Mbps by default. This setting doesn`t impact pre-emption
capability for other speeds.

Signed-off-by: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
---

v2 changelog:
-Removed useless (). Comment fron Leon Romanovsky.
---
 drivers/net/pcs/pcs-xpcs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 944ba105cac1..ea33842eb0f4 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -66,6 +66,7 @@
 
 /* VR_MII_DIG_CTRL1 */
 #define DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW		BIT(9)
+#define DW_VR_MII_DIG_CTRL1_PRE_EMP		BIT(6)
 
 /* VR_MII_AN_CTRL */
 #define DW_VR_MII_AN_CTRL_TX_CONFIG_SHIFT	3
@@ -666,6 +667,10 @@ static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 	 *	 PHY about the link state change after C28 AN is completed
 	 *	 between PHY and Link Partner. There is also no need to
 	 *	 trigger AN restart for MAC-side SGMII.
+	 *
+	 * For pre-emption, the setting is :-
+	 * 1) VR_MII_DIG_CTRL1 Bit(6) [PRE_EMP] = 1b (Enable pre-emption packet
+	 *    for 10/100Mbps)
 	 */
 	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL);
 	if (ret < 0)
@@ -686,7 +691,7 @@ static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 	if (ret < 0)
 		return ret;
 
-	ret |= DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
+	ret |= DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW | DW_VR_MII_DIG_CTRL1_PRE_EMP;
 
 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
 }
-- 
2.17.1

Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Vladimir Oltean]

Hi Mohammad,

On Fri, Apr 23, 2021 at 07:06:45AM +0800, mohammad.athari.ismail@intel.com wrote:
> From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> 
> Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet for
> 10/100Mbps by default. This setting doesn`t impact pre-emption
> capability for other speeds.
> 
> Signed-off-by: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> ---

What is a "pre-emption packet"?

RE: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Ismail, Mohammad Athari]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Friday, April 23, 2021 7:53 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> 10/100Mbps
> 
> Hi Mohammad,
> 
> On Fri, Apr 23, 2021 at 07:06:45AM +0800, mohammad.athari.ismail@intel.com
> wrote:
> > From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> >
> > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet for
> > 10/100Mbps by default. This setting doesn`t impact pre-emption
> > capability for other speeds.
> >
> > Signed-off-by: Mohammad Athari Bin Ismail
> > <mohammad.athari.ismail@intel.com>
> > ---
> 
> What is a "pre-emption packet"?

In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used to differentiate between MAC Frame packet, Express Packet, Non-fragmented Normal Frame Packet, First Fragment of Preemptable Packet, Intermediate Fragment of Preemptable Packet and Last Fragment of Preemptable Packet. 

This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare Cores Ethernet PCS Databook is to allow the IP to properly receive/transmit pre-emption packets in SGMII 10M/100M Modes.

Thanks.

Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Vladimir Oltean]

On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Friday, April 23, 2021 7:53 AM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> > Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> > Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> > <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> > 10/100Mbps
> > 
> > Hi Mohammad,
> > 
> > On Fri, Apr 23, 2021 at 07:06:45AM +0800, mohammad.athari.ismail@intel.com
> > wrote:
> > > From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> > >
> > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet for
> > > 10/100Mbps by default. This setting doesn`t impact pre-emption
> > > capability for other speeds.
> > >
> > > Signed-off-by: Mohammad Athari Bin Ismail
> > > <mohammad.athari.ismail@intel.com>
> > > ---
> > 
> > What is a "pre-emption packet"?
> 
> In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used to
> differentiate between MAC Frame packet, Express Packet, Non-fragmented
> Normal Frame Packet, First Fragment of Preemptable Packet,
> Intermediate Fragment of Preemptable Packet and Last Fragment of
> Preemptable Packet. 

Citation needed, which clause are you referring to?

> 
> This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare Cores
> Ethernet PCS Databook is to allow the IP to properly receive/transmit
> pre-emption packets in SGMII 10M/100M Modes.

Shouldn't everything be handled at the MAC merge sublayer? What business
does the PCS have in frame preemption?

Also, I know it's easy to forget, but Vinicius' patch series for
supporting frame preemption via ethtool wasn't accepted yet. How are you
testing this?

RE: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Ismail, Mohammad Athari]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Friday, April 23, 2021 8:53 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> 10/100Mbps
> 
> On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> > Hi Vladimir,
> >
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Sent: Friday, April 23, 2021 7:53 AM
> > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > <linux@armlinux.org.uk>; Ong, Boon Leong <boon.leong.ong@intel.com>;
> > > Voon, Weifeng <weifeng.voon@intel.com>; Wong, Vee Khee
> > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet
> > > for 10/100Mbps
> > >
> > > Hi Mohammad,
> > >
> > > On Fri, Apr 23, 2021 at 07:06:45AM +0800,
> > > mohammad.athari.ismail@intel.com
> > > wrote:
> > > > From: Mohammad Athari Bin Ismail
> > > > <mohammad.athari.ismail@intel.com>
> > > >
> > > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet
> > > > for 10/100Mbps by default. This setting doesn`t impact pre-emption
> > > > capability for other speeds.
> > > >
> > > > Signed-off-by: Mohammad Athari Bin Ismail
> > > > <mohammad.athari.ismail@intel.com>
> > > > ---
> > >
> > > What is a "pre-emption packet"?
> >
> > In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used to
> > differentiate between MAC Frame packet, Express Packet, Non-fragmented
> > Normal Frame Packet, First Fragment of Preemptable Packet,
> > Intermediate Fragment of Preemptable Packet and Last Fragment of
> > Preemptable Packet.
> 
> Citation needed, which clause are you referring to?

Cited from IEEE802.3-2018 Clause 99.3.

> 
> >
> > This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare Cores
> > Ethernet PCS Databook is to allow the IP to properly receive/transmit
> > pre-emption packets in SGMII 10M/100M Modes.
> 
> Shouldn't everything be handled at the MAC merge sublayer? What business
> does the PCS have in frame preemption?

There is no further detail explained in the databook w.r.t to VR_MII_DIG_CTRL1 bit-6(PRE_EMP). The only statement it mentions is "This bit should be set to 1 to allow the DWC_xpcs to properly receive/transmit pre-emption packets in SGMII 10M/100M Modes".

> 
> Also, I know it's easy to forget, but Vinicius' patch series for supporting frame
> preemption via ethtool wasn't accepted yet. How are you testing this?

For stmmac Kernel driver, frame pre-emption capability is already supported. For iproute2 (tc command), we are using custom patch based on Vinicius patch.

Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Vladimir Oltean]

On Fri, Apr 23, 2021 at 09:30:07AM +0000, Ismail, Mohammad Athari wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Friday, April 23, 2021 8:53 AM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> > Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> > Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> > <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> > 10/100Mbps
> > 
> > On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> > > Hi Vladimir,
> > >
> > > > -----Original Message-----
> > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > Sent: Friday, April 23, 2021 7:53 AM
> > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > > <linux@armlinux.org.uk>; Ong, Boon Leong <boon.leong.ong@intel.com>;
> > > > Voon, Weifeng <weifeng.voon@intel.com>; Wong, Vee Khee
> > > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet
> > > > for 10/100Mbps
> > > >
> > > > Hi Mohammad,
> > > >
> > > > On Fri, Apr 23, 2021 at 07:06:45AM +0800,
> > > > mohammad.athari.ismail@intel.com
> > > > wrote:
> > > > > From: Mohammad Athari Bin Ismail
> > > > > <mohammad.athari.ismail@intel.com>
> > > > >
> > > > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption packet
> > > > > for 10/100Mbps by default. This setting doesn`t impact pre-emption
> > > > > capability for other speeds.
> > > > >
> > > > > Signed-off-by: Mohammad Athari Bin Ismail
> > > > > <mohammad.athari.ismail@intel.com>
> > > > > ---
> > > >
> > > > What is a "pre-emption packet"?
> > >
> > > In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used to
> > > differentiate between MAC Frame packet, Express Packet, Non-fragmented
> > > Normal Frame Packet, First Fragment of Preemptable Packet,
> > > Intermediate Fragment of Preemptable Packet and Last Fragment of
> > > Preemptable Packet.
> > 
> > Citation needed, which clause are you referring to?
> 
> Cited from IEEE802.3-2018 Clause 99.3.

Aha, you know that what you just said is not what's in the "MAC Merge
sublayer" clause, right? There is no such thing as "pre-emption packet"
in the standard, this is a made-up name, maybe preemptable packets, but
the definition of preemptable packets is not that, hence my question.

> > 
> > >
> > > This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare Cores
> > > Ethernet PCS Databook is to allow the IP to properly receive/transmit
> > > pre-emption packets in SGMII 10M/100M Modes.
> > 
> > Shouldn't everything be handled at the MAC merge sublayer? What business
> > does the PCS have in frame preemption?
> 
> There is no further detail explained in the databook w.r.t to
> VR_MII_DIG_CTRL1 bit-6(PRE_EMP). The only statement it mentions is
> "This bit should be set to 1 to allow the DWC_xpcs to properly
> receive/transmit pre-emption packets in SGMII 10M/100M Modes".

Correct, I see this too. I asked our hardware design team, and at least
on NXP LS1028A (no Synopsys PCS), the PCS layer has nothing to do with
frame preemption, as mentioned.

But indeed, I do see this obscure bit in the Digital Control 1 register
too, I've no idea what it does. I'll ask around. Odd anyway. If you have
to set it, you have to set it, I guess. But it is interesting to see why
is it even a configurable bit, why it is not enabled by default, what is
the drawback of enabling it?!

> > 
> > Also, I know it's easy to forget, but Vinicius' patch series for supporting frame
> > preemption via ethtool wasn't accepted yet. How are you testing this?
> 
> For stmmac Kernel driver, frame pre-emption capability is already
> supported. For iproute2 (tc command), we are using custom patch based
> on Vinicius patch.

Don't you want to help contributing the ethtool netlink support to the
mainline kernel though? :)

RE: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Ismail, Mohammad Athari]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Saturday, April 24, 2021 2:12 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> 10/100Mbps
> 
> On Fri, Apr 23, 2021 at 09:30:07AM +0000, Ismail, Mohammad Athari wrote:
> > Hi Vladimir,
> >
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Sent: Friday, April 23, 2021 8:53 AM
> > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > <linux@armlinux.org.uk>; Ong, Boon Leong <boon.leong.ong@intel.com>;
> > > Voon, Weifeng <weifeng.voon@intel.com>; Wong, Vee Khee
> > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet
> > > for 10/100Mbps
> > >
> > > On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> > > > Hi Vladimir,
> > > >
> > > > > -----Original Message-----
> > > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > > Sent: Friday, April 23, 2021 7:53 AM
> > > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > > > <linux@armlinux.org.uk>; Ong, Boon Leong
> > > > > <boon.leong.ong@intel.com>; Voon, Weifeng
> > > > > <weifeng.voon@intel.com>; Wong, Vee Khee
> > > > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption
> > > > > packet for 10/100Mbps
> > > > >
> > > > > Hi Mohammad,
> > > > >
> > > > > On Fri, Apr 23, 2021 at 07:06:45AM +0800,
> > > > > mohammad.athari.ismail@intel.com
> > > > > wrote:
> > > > > > From: Mohammad Athari Bin Ismail
> > > > > > <mohammad.athari.ismail@intel.com>
> > > > > >
> > > > > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption
> > > > > > packet for 10/100Mbps by default. This setting doesn`t impact
> > > > > > pre-emption capability for other speeds.
> > > > > >
> > > > > > Signed-off-by: Mohammad Athari Bin Ismail
> > > > > > <mohammad.athari.ismail@intel.com>
> > > > > > ---
> > > > >
> > > > > What is a "pre-emption packet"?
> > > >
> > > > In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used
> > > > to differentiate between MAC Frame packet, Express Packet,
> > > > Non-fragmented Normal Frame Packet, First Fragment of Preemptable
> > > > Packet, Intermediate Fragment of Preemptable Packet and Last
> > > > Fragment of Preemptable Packet.
> > >
> > > Citation needed, which clause are you referring to?
> >
> > Cited from IEEE802.3-2018 Clause 99.3.
> 
> Aha, you know that what you just said is not what's in the "MAC Merge sublayer"
> clause, right? There is no such thing as "pre-emption packet"
> in the standard, this is a made-up name, maybe preemptable packets, but the
> definition of preemptable packets is not that, hence my question.
> 

Thank you for the knowledge sharing. My guess, this "pre-emption packet" might be referring to "preamble" byte in Ethernet frame. 

> > >
> > > >
> > > > This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare
> > > > Cores Ethernet PCS Databook is to allow the IP to properly
> > > > receive/transmit pre-emption packets in SGMII 10M/100M Modes.
> > >
> > > Shouldn't everything be handled at the MAC merge sublayer? What
> > > business does the PCS have in frame preemption?
> >
> > There is no further detail explained in the databook w.r.t to
> > VR_MII_DIG_CTRL1 bit-6(PRE_EMP). The only statement it mentions is
> > "This bit should be set to 1 to allow the DWC_xpcs to properly
> > receive/transmit pre-emption packets in SGMII 10M/100M Modes".
> 
> Correct, I see this too. I asked our hardware design team, and at least on NXP
> LS1028A (no Synopsys PCS), the PCS layer has nothing to do with frame
> preemption, as mentioned.
> 
> But indeed, I do see this obscure bit in the Digital Control 1 register too, I've no
> idea what it does. I'll ask around. Odd anyway. If you have to set it, you have to
> set it, I guess. But it is interesting to see why is it even a configurable bit, why it
> is not enabled by default, what is the drawback of enabling it?!

The databook states that the default value is 0. We don`t see any drawback of enabling it. As the databook mentions that, enabling the bit will allow SGMII 10/100M to receive/transmit preamble properly, so I think it is recommended to enable it for IP that support SGMII 10/100M speed.

> 
> > >
> > > Also, I know it's easy to forget, but Vinicius' patch series for
> > > supporting frame preemption via ethtool wasn't accepted yet. How are you
> testing this?
> >
> > For stmmac Kernel driver, frame pre-emption capability is already
> > supported. For iproute2 (tc command), we are using custom patch based
> > on Vinicius patch.
> 
> Don't you want to help contributing the ethtool netlink support to the mainline
> kernel though? :)

We are working with Vinicius to have ethtool support for frame pre-emption. 

Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Vladimir Oltean]

Hi Ismail,

On Fri, Apr 23, 2021 at 10:03:58PM +0000, Ismail, Mohammad Athari wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Saturday, April 24, 2021 2:12 AM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Jakub
> > Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Ong, Boon
> > Leong <boon.leong.ong@intel.com>; Voon, Weifeng
> > <weifeng.voon@intel.com>; Wong, Vee Khee <vee.khee.wong@intel.com>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> > 10/100Mbps
> > 
> > On Fri, Apr 23, 2021 at 09:30:07AM +0000, Ismail, Mohammad Athari wrote:
> > > Hi Vladimir,
> > >
> > > > -----Original Message-----
> > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > Sent: Friday, April 23, 2021 8:53 AM
> > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > > <linux@armlinux.org.uk>; Ong, Boon Leong <boon.leong.ong@intel.com>;
> > > > Voon, Weifeng <weifeng.voon@intel.com>; Wong, Vee Khee
> > > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet
> > > > for 10/100Mbps
> > > >
> > > > On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> > > > > Hi Vladimir,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > > > Sent: Friday, April 23, 2021 7:53 AM
> > > > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > > > > Cc: Alexandre Torgue <alexandre.torgue@st.com>; Jose Abreu
> > > > > > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> > > > > > Jakub Kicinski <kuba@kernel.org>; Andrew Lunn <andrew@lunn.ch>;
> > > > > > Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > > > > > <linux@armlinux.org.uk>; Ong, Boon Leong
> > > > > > <boon.leong.ong@intel.com>; Voon, Weifeng
> > > > > > <weifeng.voon@intel.com>; Wong, Vee Khee
> > > > > > <vee.khee.wong@intel.com>; netdev@vger.kernel.org;
> > > > > > linux-kernel@vger.kernel.org
> > > > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption
> > > > > > packet for 10/100Mbps
> > > > > >
> > > > > > Hi Mohammad,
> > > > > >
> > > > > > On Fri, Apr 23, 2021 at 07:06:45AM +0800,
> > > > > > mohammad.athari.ismail@intel.com
> > > > > > wrote:
> > > > > > > From: Mohammad Athari Bin Ismail
> > > > > > > <mohammad.athari.ismail@intel.com>
> > > > > > >
> > > > > > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption
> > > > > > > packet for 10/100Mbps by default. This setting doesn`t impact
> > > > > > > pre-emption capability for other speeds.
> > > > > > >
> > > > > > > Signed-off-by: Mohammad Athari Bin Ismail
> > > > > > > <mohammad.athari.ismail@intel.com>
> > > > > > > ---
> > > > > >
> > > > > > What is a "pre-emption packet"?
> > > > >
> > > > > In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used
> > > > > to differentiate between MAC Frame packet, Express Packet,
> > > > > Non-fragmented Normal Frame Packet, First Fragment of Preemptable
> > > > > Packet, Intermediate Fragment of Preemptable Packet and Last
> > > > > Fragment of Preemptable Packet.
> > > >
> > > > Citation needed, which clause are you referring to?
> > >
> > > Cited from IEEE802.3-2018 Clause 99.3.
> > 
> > Aha, you know that what you just said is not what's in the "MAC Merge sublayer"
> > clause, right? There is no such thing as "pre-emption packet"
> > in the standard, this is a made-up name, maybe preemptable packets, but the
> > definition of preemptable packets is not that, hence my question.
> > 
> 
> Thank you for the knowledge sharing. My guess, this "pre-emption
> packet" might be referring to "preamble" byte in Ethernet frame. 
> 
> > > >
> > > > >
> > > > > This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare
> > > > > Cores Ethernet PCS Databook is to allow the IP to properly
> > > > > receive/transmit pre-emption packets in SGMII 10M/100M Modes.
> > > >
> > > > Shouldn't everything be handled at the MAC merge sublayer? What
> > > > business does the PCS have in frame preemption?
> > >
> > > There is no further detail explained in the databook w.r.t to
> > > VR_MII_DIG_CTRL1 bit-6(PRE_EMP). The only statement it mentions is
> > > "This bit should be set to 1 to allow the DWC_xpcs to properly
> > > receive/transmit pre-emption packets in SGMII 10M/100M Modes".
> > 
> > Correct, I see this too. I asked our hardware design team, and at least on NXP
> > LS1028A (no Synopsys PCS), the PCS layer has nothing to do with frame
> > preemption, as mentioned.
> > 
> > But indeed, I do see this obscure bit in the Digital Control 1 register too, I've no
> > idea what it does. I'll ask around. Odd anyway. If you have to set it, you have to
> > set it, I guess. But it is interesting to see why is it even a configurable bit, why it
> > is not enabled by default, what is the drawback of enabling it?!
> 
> The databook states that the default value is 0. We don`t see any
> drawback of enabling it. As the databook mentions that, enabling the
> bit will allow SGMII 10/100M to receive/transmit preamble properly, so
> I think it is recommended to enable it for IP that support SGMII
> 10/100M speed.

Why do you need this patch, exactly? Is there anything that doesn't work
if you don't make the change? For example, if you leave the PRE_EMP bit
in the PCS set to zero, you set the link to 100 Mbps, configure all
queues to go to the pMAC and stress the interface with some iperf3
traffic for a while, do you see any issues at all?