Message ID | 20230721062617.9810-3-boon.khai.ng@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | | expand |
On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: > From: Boon Khai Ng <boon.khai.ng@intel.com> > > Currently, VLAN tag stripping is done by software driver in > stmmac_rx_vlan(). This patch is to Add support for VLAN tag > stripping by the MAC hardware and MAC drivers to support it. > This is done by adding rx_hw_vlan() and set_hw_vlan_mode() > callbacks at stmmac_ops struct which are called from upper > software layer. ... > if (priv->dma_cap.vlhash) { > ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; > ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 23d53ea04b24..bd7f3326a44c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > plat->flags |= STMMAC_FLAG_TSO_EN; > } > > + /* Rx VLAN HW Stripping */ > + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { > + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); Why? Drop. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Friday, July 21, 2023 6:11 PM > To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng > <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>; > 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-stm32@st- > md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy > <andriy.shevchenko@intel.com>; Tham, Mun Yew > <mun.yew.tham@intel.com>; Swee, Leong Ching > <leong.ching.swee@intel.com>; G Thomas, Rohan > <rohan.g.thomas@intel.com>; Shevchenko Andriy > <andriy.shevchenko@linux.intel.com> > Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: > stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > > On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: > > From: Boon Khai Ng <boon.khai.ng@intel.com> > > > > Currently, VLAN tag stripping is done by software driver in > > stmmac_rx_vlan(). This patch is to Add support for VLAN tag stripping > > by the MAC hardware and MAC drivers to support it. > > This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks > > at stmmac_ops struct which are called from upper software layer. > ... > > > if (priv->dma_cap.vlhash) { > > ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; > > ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff -- > git > > a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > index 23d53ea04b24..bd7f3326a44c 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device > *pdev, u8 *mac) > > plat->flags |= STMMAC_FLAG_TSO_EN; > > } > > > > + /* Rx VLAN HW Stripping */ > > + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { > > + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); > > Why? Drop. > This is an dts option export to dts for user to choose whether or not they Want a Hardware stripping or a software stripping. May I know what is the reason to drop this? > > > Best regards, > Krzysztof
On 7/21/2023 8:30 AM, Ng, Boon Khai wrote: >> -----Original Message----- >> From: Krzysztof Kozlowski <krzk@kernel.org> >> Sent: Friday, July 21, 2023 6:11 PM >> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng >> <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>; >> 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-stm32@st- >> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org >> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy >> <andriy.shevchenko@intel.com>; Tham, Mun Yew >> <mun.yew.tham@intel.com>; Swee, Leong Ching >> <leong.ching.swee@intel.com>; G Thomas, Rohan >> <rohan.g.thomas@intel.com>; Shevchenko Andriy >> <andriy.shevchenko@linux.intel.com> >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping >> >> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: >>> From: Boon Khai Ng <boon.khai.ng@intel.com> >>> >>> Currently, VLAN tag stripping is done by software driver in >>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag stripping >>> by the MAC hardware and MAC drivers to support it. >>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks >>> at stmmac_ops struct which are called from upper software layer. >> ... >> >>> if (priv->dma_cap.vlhash) { >>> ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; >>> ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff -- >> git >>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> index 23d53ea04b24..bd7f3326a44c 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, u8 *mac) >>> plat->flags |= STMMAC_FLAG_TSO_EN; >>> } >>> >>> + /* Rx VLAN HW Stripping */ >>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { >>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); >> >> Why? Drop. >> > > This is an dts option export to dts for user to choose whether or not they > Want a Hardware stripping or a software stripping. > > May I know what is the reason to drop this? Because the networking stack already exposes knobs for drivers to advertise and control VLAN stripping/insertion on RX/TX using ethtool and feature bits (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX). What you are doing here is encode a policy as a Device Tree property rather than describe whether the hardware supports a given feature and this is frowned upon.
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: Friday, July 21, 2023 11:59 PM > To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski > <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; > Ng, Boon Khai <boon.khai.ng@intel.com>; Giuseppe Cavallaro > <peppe.cavallaro@st.com>; 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-stm32@st-md- > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew > <mun.yew.tham@intel.com>; Swee, Leong Ching > <leong.ching.swee@intel.com>; G Thomas, Rohan > <rohan.g.thomas@intel.com>; Shevchenko Andriy > <andriy.shevchenko@linux.intel.com> > Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: > stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > > > > On 7/21/2023 8:30 AM, Ng, Boon Khai wrote: > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@kernel.org> > >> Sent: Friday, July 21, 2023 6:11 PM > >> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng > >> <boon.khai.ng"@intel.com; Giuseppe Cavallaro > >> <peppe.cavallaro@st.com>; 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-stm32@st- md-mailman.stormreply.com; > >> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org > >> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy > >> <andriy.shevchenko@intel.com>; Tham, Mun Yew > >> <mun.yew.tham@intel.com>; Swee, Leong Ching > >> <leong.ching.swee@intel.com>; G Thomas, Rohan > >> <rohan.g.thomas@intel.com>; Shevchenko Andriy > >> <andriy.shevchenko@linux.intel.com> > >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: > >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > >> > >> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: > >>> From: Boon Khai Ng <boon.khai.ng@intel.com> > >>> > >>> Currently, VLAN tag stripping is done by software driver in > >>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag > >>> stripping by the MAC hardware and MAC drivers to support it. > >>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks > >>> at stmmac_ops struct which are called from upper software layer. > >> ... > >> > >>> if (priv->dma_cap.vlhash) { > >>> ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; > >>> ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff -- > >> git > >>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>> index 23d53ea04b24..bd7f3326a44c 100644 > >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device > >> *pdev, u8 *mac) > >>> plat->flags |= STMMAC_FLAG_TSO_EN; > >>> } > >>> > >>> + /* Rx VLAN HW Stripping */ > >>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { > >>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); > >> > >> Why? Drop. > >> > > > > This is an dts option export to dts for user to choose whether or not > > they Want a Hardware stripping or a software stripping. > > > > May I know what is the reason to drop this? > > Because the networking stack already exposes knobs for drivers to advertise and > control VLAN stripping/insertion on RX/TX using ethtool and feature bits > (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX). > Hi Florian, Understood, but how does user choose to have the default option either hardware strip or software strip, when the device just boot up? I don’t think ethool can "remember" the setting once the device get rebooted? Any other suggestion of doing it other than using the dts method? > What you are doing here is encode a policy as a Device Tree property rather > than describe whether the hardware supports a given feature and this is frowned > upon. > -- > Florian
On 21/07/2023 17:30, Ng, Boon Khai wrote: >> git >>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> index 23d53ea04b24..bd7f3326a44c 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, u8 *mac) >>> plat->flags |= STMMAC_FLAG_TSO_EN; >>> } >>> >>> + /* Rx VLAN HW Stripping */ >>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { >>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); >> >> Why? Drop. >> > > This is an dts option export to dts for user to choose whether or not they > Want a Hardware stripping or a software stripping. > > May I know what is the reason to drop this? Because we do not print simple confirmation of DT properties parsing. It's usually useless and obvious from DT. To be clear - we talk about dev_info. Best regards, Krzysztof
On 21/07/2023 17:59, Florian Fainelli wrote: >>>> + /* Rx VLAN HW Stripping */ >>>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { >>>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); >>> >>> Why? Drop. >>> >> >> This is an dts option export to dts for user to choose whether or not they >> Want a Hardware stripping or a software stripping. >> >> May I know what is the reason to drop this? > > Because the networking stack already exposes knobs for drivers to > advertise and control VLAN stripping/insertion on RX/TX using ethtool > and feature bits (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX). > > What you are doing here is encode a policy as a Device Tree property > rather than describe whether the hardware supports a given feature and > this is frowned upon. That's even better reason... Best regards, Krzysztof
On 7/21/2023 9:12 AM, Ng, Boon Khai wrote: >> -----Original Message----- >> From: Florian Fainelli <f.fainelli@gmail.com> >> Sent: Friday, July 21, 2023 11:59 PM >> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski >> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; >> Ng, Boon Khai <boon.khai.ng@intel.com>; Giuseppe Cavallaro >> <peppe.cavallaro@st.com>; 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-stm32@st-md- >> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org >> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew >> <mun.yew.tham@intel.com>; Swee, Leong Ching >> <leong.ching.swee@intel.com>; G Thomas, Rohan >> <rohan.g.thomas@intel.com>; Shevchenko Andriy >> <andriy.shevchenko@linux.intel.com> >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping >> >> >> >> On 7/21/2023 8:30 AM, Ng, Boon Khai wrote: >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzk@kernel.org> >>>> Sent: Friday, July 21, 2023 6:11 PM >>>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng >>>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro >>>> <peppe.cavallaro@st.com>; 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-stm32@st- md-mailman.stormreply.com; >>>> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org >>>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy >>>> <andriy.shevchenko@intel.com>; Tham, Mun Yew >>>> <mun.yew.tham@intel.com>; Swee, Leong Ching >>>> <leong.ching.swee@intel.com>; G Thomas, Rohan >>>> <rohan.g.thomas@intel.com>; Shevchenko Andriy >>>> <andriy.shevchenko@linux.intel.com> >>>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: >>>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping >>>> >>>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: >>>>> From: Boon Khai Ng <boon.khai.ng@intel.com> >>>>> >>>>> Currently, VLAN tag stripping is done by software driver in >>>>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag >>>>> stripping by the MAC hardware and MAC drivers to support it. >>>>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks >>>>> at stmmac_ops struct which are called from upper software layer. >>>> ... >>>> >>>>> if (priv->dma_cap.vlhash) { >>>>> ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; >>>>> ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff -- >>>> git >>>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>>> index 23d53ea04b24..bd7f3326a44c 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >>>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device >>>> *pdev, u8 *mac) >>>>> plat->flags |= STMMAC_FLAG_TSO_EN; >>>>> } >>>>> >>>>> + /* Rx VLAN HW Stripping */ >>>>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { >>>>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); >>>> >>>> Why? Drop. >>>> >>> >>> This is an dts option export to dts for user to choose whether or not >>> they Want a Hardware stripping or a software stripping. >>> >>> May I know what is the reason to drop this? >> >> Because the networking stack already exposes knobs for drivers to advertise and >> control VLAN stripping/insertion on RX/TX using ethtool and feature bits >> (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX). >> > > Hi Florian, > > Understood, but how does user choose to have the default option > either hardware strip or software strip, when the device just boot up? You need the hardware to advertise it and decide as a maintainer of that driver whether it makes sense to have one or the other behavior by default. > > I don’t think ethool can "remember" the setting once the device get rebooted? If by "device" you mean a system that incorporates a XGMAC core, then I suppose that is true, though you could have some user-space logic that does remember the various ethtool options and re-applies them as soon as the device is made available to user-space, this would not be too far fetched. > Any other suggestion of doing it other than using the dts method? Let me ask you this question: what are you trying to solve by making this configurable? HW stripping should always be more efficient, should not it, if so, what would be the reasons for not enabling that by default? If not, then leave it off and let users enable it if they feel like they want it.
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: Saturday, July 22, 2023 12:30 AM > To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski > <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; > Khai@ecsmtp.png.intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>; > 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-stm32@st- > md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew > <mun.yew.tham@intel.com>; Swee, Leong Ching > <leong.ching.swee@intel.com>; G Thomas, Rohan > <rohan.g.thomas@intel.com>; Shevchenko Andriy > <andriy.shevchenko@linux.intel.com> > Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: > stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > > > > On 7/21/2023 9:12 AM, Ng, Boon Khai wrote: > >> -----Original Message----- > >> From: Florian Fainelli <f.fainelli@gmail.com> > >> Sent: Friday, July 21, 2023 11:59 PM > >> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski > >> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; > >> Khai@ecsmtp.png.intel.com; Ng, Boon Khai <boon.khai.ng@intel.com>; > >> Giuseppe Cavallaro <peppe.cavallaro@st.com>; 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-stm32@st-md- mailman.stormreply.com; > >> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org > >> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun > Yew > >> <mun.yew.tham@intel.com>; Swee, Leong Ching > >> <leong.ching.swee@intel.com>; G Thomas, Rohan > >> <rohan.g.thomas@intel.com>; Shevchenko Andriy > >> <andriy.shevchenko@linux.intel.com> > >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: > >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > >> > >> > >> > >> On 7/21/2023 8:30 AM, Ng, Boon Khai wrote: > >>>> -----Original Message----- > >>>> From: Krzysztof Kozlowski <krzk@kernel.org> > >>>> Sent: Friday, July 21, 2023 6:11 PM > >>>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng > >>>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro > >>>> <peppe.cavallaro@st.com>; 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-stm32@st- md-mailman.stormreply.com; > >>>> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org > >>>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy > >>>> <andriy.shevchenko@intel.com>; Tham, Mun Yew > >>>> <mun.yew.tham@intel.com>; Swee, Leong Ching > >>>> <leong.ching.swee@intel.com>; G Thomas, Rohan > >>>> <rohan.g.thomas@intel.com>; Shevchenko Andriy > >>>> <andriy.shevchenko@linux.intel.com> > >>>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] > net: > >>>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping > >>>> > >>>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote: > >>>>> From: Boon Khai Ng <boon.khai.ng@intel.com> > >>>>> > >>>>> Currently, VLAN tag stripping is done by software driver in > >>>>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag > >>>>> stripping by the MAC hardware and MAC drivers to support it. > >>>>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() > >>>>> callbacks at stmmac_ops struct which are called from upper software > layer. > >>>> ... > >>>> > >>>>> if (priv->dma_cap.vlhash) { > >>>>> ndev->features |= > NETIF_F_HW_VLAN_CTAG_FILTER; > >>>>> ndev->features |= > NETIF_F_HW_VLAN_STAG_FILTER; diff -- > >>>> git > >>>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>>>> index 23d53ea04b24..bd7f3326a44c 100644 > >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > >>>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct > platform_device > >>>> *pdev, u8 *mac) > >>>>> plat->flags |= STMMAC_FLAG_TSO_EN; > >>>>> } > >>>>> > >>>>> + /* Rx VLAN HW Stripping */ > >>>>> + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { > >>>>> + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); > >>>> > >>>> Why? Drop. > >>>> > >>> > >>> This is an dts option export to dts for user to choose whether or > >>> not they Want a Hardware stripping or a software stripping. > >>> > >>> May I know what is the reason to drop this? > >> > >> Because the networking stack already exposes knobs for drivers to > >> advertise and control VLAN stripping/insertion on RX/TX using ethtool > >> and feature bits (NETIF_F_HW_VLAN_CTAG_RX, > NETIF_F_HW_VLAN_CTAG_TX). > >> > > > > Hi Florian, > > > > Understood, but how does user choose to have the default option either > > hardware strip or software strip, when the device just boot up? > > You need the hardware to advertise it and decide as a maintainer of that > driver whether it makes sense to have one or the other behavior by default. > Okay got it. > > > > I don’t think ethool can "remember" the setting once the device get > rebooted? > > If by "device" you mean a system that incorporates a XGMAC core, then I > suppose that is true, though you could have some user-space logic that does > remember the various ethtool options and re-applies them as soon as the > device is made available to user-space, this would not be too far fetched. > Okay, will try to search that, is adding the ethool command turning the Hw vlan striping at the startup script consider one way of doing it? > > Any other suggestion of doing it other than using the dts method? > > Let me ask you this question: what are you trying to solve by making this > configurable? HW stripping should always be more efficient, should not it, if > so, what would be the reasons for not enabling that by default? If not, then > leave it off and let users enable it if they feel like they want it. Okay, so seem like it is solely depends on my side whether or not to turn it on by default, Either way, if it go against the user will to have it on/off by default, they will need to write A startup script to turn the ethool on/off? > -- > Florian
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h index 153321fe42c3..2a75faf8443b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h @@ -431,6 +431,7 @@ #define XGMAC_TDES2_VTIR GENMASK(15, 14) #define XGMAC_TDES2_VTIR_SHIFT 14 #define XGMAC_TDES2_B1L GENMASK(13, 0) +#define XGMAC_TDES2_VLAN_TAG_MASK GENMASK(15, 14) #define XGMAC_TDES3_OWN BIT(31) #define XGMAC_TDES3_CTXT BIT(30) #define XGMAC_TDES3_FD BIT(29) @@ -462,6 +463,8 @@ #define XGMAC_RDES3_RSV BIT(26) #define XGMAC_RDES3_L34T GENMASK(23, 20) #define XGMAC_RDES3_L34T_SHIFT 20 +#define XGMAC_RDES3_ET_LT GENMASK(19, 16) +#define XGMAC_RDES3_ET_LT_SHIFT 16 #define XGMAC_L34T_IP4TCP 0x1 #define XGMAC_L34T_IP4UDP 0x2 #define XGMAC_L34T_IP6TCP 0x9 @@ -471,4 +474,29 @@ #define XGMAC_RDES3_TSD BIT(6) #define XGMAC_RDES3_TSA BIT(4) +/* RDES0 (write back format) */ +#define XGMAC_RDES0_VLAN_TAG_MASK GENMASK(15, 0) + +/* MAC VLAN Tag Control */ +#define XGMAC_VLAN_TAG_CTRL_OB BIT(0) +#define XGMAC_VLAN_TAG_CTRL_CT BIT(1) +#define XGMAC_VLAN_TAG_CTRL_OFS_MASK GENMASK(6, 2) +#define XGMAC_VLAN_TAG_CTRL_OFS_SHIFT 2 +#define XGMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21) +#define XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21 +#define XGMAC_VLAN_TAG_CTRL_EVLRXS BIT(24) + +#define XGMAC_VLAN_TAG_STRIP_NONE (0x0 << XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT) +#define XGMAC_VLAN_TAG_STRIP_PASS (0x1 << XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT) +#define XGMAC_VLAN_TAG_STRIP_FAIL (0x2 << XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT) +#define XGMAC_VLAN_TAG_STRIP_ALL (0x3 << XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT) + +/* Error Type or L2 Type(ET/LT) Field Number */ +#define XGMAC_ET_LT_VLAN_STAG 8 +#define XGMAC_ET_LT_VLAN_CTAG 9 +#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10 +#define XGMAC_ET_LT_DVLAN_STAG_STAG 11 +#define XGMAC_ET_LT_DVLAN_CTAG_STAG 12 +#define XGMAC_ET_LT_DVLAN_STAG_CTAG 13 + #endif /* __STMMAC_DWXGMAC2_H__ */ diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index a0c2ef8bb0ac..0f8120427d5c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -6,6 +6,7 @@ #include <linux/bitrev.h> #include <linux/crc32.h> +#include <linux/if_vlan.h> #include <linux/iopoll.h> #include "stmmac.h" #include "stmmac_ptp.h" @@ -1177,6 +1178,39 @@ static void dwxgmac2_sarc_configure(void __iomem *ioaddr, int val) writel(value, ioaddr + XGMAC_TX_CONFIG); } +static void dwxgmac2_rx_hw_vlan(struct net_device *dev, + struct mac_device_info *hw, + struct dma_desc *rx_desc, struct sk_buff *skb) +{ + if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) && + hw->desc->get_rx_vlan_valid(rx_desc)) { + u16 vid = hw->desc->get_rx_vlan_tci(rx_desc); + + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid); + } +} + +static void dwxgmac2_set_hw_vlan_mode(void __iomem *ioaddr, + netdev_features_t features) +{ + u32 val; + + val = readl(ioaddr + XGMAC_VLAN_TAG); + val &= ~XGMAC_VLAN_TAG_CTRL_EVLS_MASK; + + if (features & NETIF_F_HW_VLAN_CTAG_RX) + /* Always strip VLAN on Receive */ + val |= XGMAC_VLAN_TAG_STRIP_ALL; + else + /* Do not strip VLAN on Receive */ + val |= XGMAC_VLAN_TAG_STRIP_NONE; + + /* Enable outer VLAN Tag in Rx DMA descriptro */ + val |= XGMAC_VLAN_TAG_CTRL_EVLRXS; + + writel(val, ioaddr + XGMAC_VLAN_TAG); +} + static void dwxgmac2_enable_vlan(struct mac_device_info *hw, u32 type) { void __iomem *ioaddr = hw->pcsr; @@ -1501,6 +1535,8 @@ const struct stmmac_ops dwxgmac210_ops = { .set_arp_offload = dwxgmac2_set_arp_offload, .est_configure = dwxgmac3_est_configure, .fpe_configure = dwxgmac3_fpe_configure, + .rx_hw_vlan = dwxgmac2_rx_hw_vlan, + .set_hw_vlan_mode = dwxgmac2_set_hw_vlan_mode, }; static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, @@ -1562,6 +1598,8 @@ const struct stmmac_ops dwxlgmac2_ops = { .set_arp_offload = dwxgmac2_set_arp_offload, .est_configure = dwxgmac3_est_configure, .fpe_configure = dwxgmac3_fpe_configure, + .rx_hw_vlan = dwxgmac2_rx_hw_vlan, + .set_hw_vlan_mode = dwxgmac2_set_hw_vlan_mode, }; int dwxgmac2_setup(struct stmmac_priv *priv) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c index fc82862a612c..b8cc17c9c0db 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c @@ -67,6 +67,22 @@ static int dwxgmac2_get_tx_ls(struct dma_desc *p) return (le32_to_cpu(p->des3) & XGMAC_RDES3_LD) > 0; } +static inline int dwxgmac2_wrback_get_rx_vlan_tci(struct dma_desc *p) +{ + return le32_to_cpu(p->des0) & XGMAC_RDES0_VLAN_TAG_MASK; +} + +static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc *p) +{ + u32 et_lt; + + et_lt = (le32_to_cpu(p->des3) & XGMAC_RDES3_ET_LT) >> + XGMAC_RDES3_ET_LT_SHIFT; + + return et_lt >= XGMAC_ET_LT_VLAN_STAG && + et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG; +} + static int dwxgmac2_get_rx_frame_len(struct dma_desc *p, int rx_coe) { return (le32_to_cpu(p->des3) & XGMAC_RDES3_PL); @@ -349,6 +365,8 @@ const struct stmmac_desc_ops dwxgmac210_desc_ops = { .set_tx_owner = dwxgmac2_set_tx_owner, .set_rx_owner = dwxgmac2_set_rx_owner, .get_tx_ls = dwxgmac2_get_tx_ls, + .get_rx_vlan_tci = dwxgmac2_wrback_get_rx_vlan_tci, + .get_rx_vlan_valid = dwxgmac2_wrback_get_rx_vlan_valid, .get_rx_frame_len = dwxgmac2_get_rx_frame_len, .enable_tx_timestamp = dwxgmac2_enable_tx_timestamp, .get_tx_timestamp_status = dwxgmac2_get_tx_timestamp_status, diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 652af8f6e75f..d37fa2400f62 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -97,6 +97,10 @@ struct stmmac_desc_ops { u32 inner_type); void (*set_vlan)(struct dma_desc *p, u32 type); void (*set_tbs)(struct dma_edesc *p, u32 sec, u32 nsec); + /* get rx vlan id */ + int (*get_rx_vlan_tci)(struct dma_desc *p); + /* get rx vlan valid status */ + bool (*get_rx_vlan_valid)(struct dma_desc *p); }; #define stmmac_init_rx_desc(__priv, __args...) \ @@ -117,6 +121,10 @@ struct stmmac_desc_ops { stmmac_do_void_callback(__priv, desc, set_tx_ic, __args) #define stmmac_get_tx_ls(__priv, __args...) \ stmmac_do_callback(__priv, desc, get_tx_ls, __args) +#define stmmac_get_rx_vlan_tci(__priv, __args...) \ + stmmac_do_callback(__priv, desc, get_rx_vlan_tci, __args) +#define stmmac_get_rx_vlan_valid(__priv, __args...) \ + stmmac_do_callback(__priv, desc, get_rx_vlan_valid, __args) #define stmmac_tx_status(__priv, __args...) \ stmmac_do_callback(__priv, desc, tx_status, __args) #define stmmac_get_tx_len(__priv, __args...) \ @@ -386,6 +394,10 @@ struct stmmac_ops { void (*update_vlan_hash)(struct mac_device_info *hw, u32 hash, __le16 perfect_match, bool is_double); void (*enable_vlan)(struct mac_device_info *hw, u32 type); + void (*rx_hw_vlan)(struct net_device *dev, struct mac_device_info *hw, + struct dma_desc *rx_desc, struct sk_buff *skb); + void (*set_hw_vlan_mode)(void __iomem *ioaddr, + netdev_features_t features); int (*add_hw_vlan_rx_fltr)(struct net_device *dev, struct mac_device_info *hw, __be16 proto, u16 vid); @@ -493,6 +505,10 @@ struct stmmac_ops { stmmac_do_void_callback(__priv, mac, update_vlan_hash, __args) #define stmmac_enable_vlan(__priv, __args...) \ stmmac_do_void_callback(__priv, mac, enable_vlan, __args) +#define stmmac_rx_hw_vlan(__priv, __args...) \ + stmmac_do_void_callback(__priv, mac, rx_hw_vlan, __args) +#define stmmac_set_hw_vlan_mode(__priv, __args...) \ + stmmac_do_void_callback(__priv, mac, set_hw_vlan_mode, __args) #define stmmac_add_hw_vlan_rx_fltr(__priv, __args...) \ stmmac_do_callback(__priv, mac, add_hw_vlan_rx_fltr, __args) #define stmmac_del_hw_vlan_rx_fltr(__priv, __args...) \ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index e7ca52f0d2f2..2fe58bf3d936 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3451,6 +3451,10 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) stmmac_fpe_handshake(priv, true); } + /* Set HW VLAN Stripping mode */ + if (priv->plat->use_hw_vlan) + stmmac_set_hw_vlan_mode(priv, priv->ioaddr, dev->features); + return 0; } @@ -4596,10 +4600,8 @@ static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) __be16 vlan_proto = veth->h_vlan_proto; u16 vlanid; - if ((vlan_proto == htons(ETH_P_8021Q) && - dev->features & NETIF_F_HW_VLAN_CTAG_RX) || - (vlan_proto == htons(ETH_P_8021AD) && - dev->features & NETIF_F_HW_VLAN_STAG_RX)) { + if (vlan_proto == htons(ETH_P_8021Q) || + vlan_proto == htons(ETH_P_8021AD)) { /* pop the vlan tag */ vlanid = ntohs(veth->h_vlan_TCI); memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); @@ -5466,7 +5468,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) /* Got entire packet into SKB. Finish it. */ stmmac_get_rx_hwtstamp(priv, p, np, skb); - stmmac_rx_vlan(priv->dev, skb); + + /* Switch between rx_hw_vlan or rx_vlan */ + if (priv->plat->use_hw_vlan) + stmmac_rx_hw_vlan(priv, priv->dev, + priv->hw, p, skb); + else + stmmac_rx_vlan(priv->dev, skb); + skb->protocol = eth_type_trans(skb, priv->dev); if (unlikely(!coe)) @@ -5746,6 +5755,9 @@ static int stmmac_set_features(struct net_device *netdev, netdev_features_t features) { struct stmmac_priv *priv = netdev_priv(netdev); + netdev_features_t changed; + + changed = netdev->features ^ features; /* Keep the COE Type in case of csum is supporting */ if (features & NETIF_F_RXCSUM) @@ -5765,6 +5777,11 @@ static int stmmac_set_features(struct net_device *netdev, stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan); } + if (changed & NETIF_F_HW_VLAN_CTAG_RX) { + stmmac_set_hw_vlan_mode(priv, priv->ioaddr, features); + priv->plat->use_hw_vlan = features & NETIF_F_HW_VLAN_CTAG_RX; + } + return 0; } @@ -7418,6 +7435,8 @@ int stmmac_dvr_probe(struct device *device, #ifdef STMMAC_VLAN_TAG_USED /* Both mac100 and gmac support receive VLAN tag detection */ ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; + ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; + if (priv->dma_cap.vlhash) { ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 23d53ea04b24..bd7f3326a44c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) plat->flags |= STMMAC_FLAG_TSO_EN; } + /* Rx VLAN HW Stripping */ + if (of_property_read_bool(np, "snps,rx-vlan-offload")) { + dev_info(&pdev->dev, "RX VLAN HW Stripping\n"); + plat->use_hw_vlan = true; + } + dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg), GFP_KERNEL); if (!dma_cfg) { diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index ef67dba775d0..a989c5d82482 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -282,6 +282,7 @@ struct plat_stmmacenet_data { int rss_en; int mac_port_sel_speed; int has_xgmac; + bool use_hw_vlan; u8 vlan_fail_q; unsigned int eee_usecs_rate; struct pci_dev *pdev;