diff mbox series

[net-next,1/1] net: stmmac: xgmac: EST interrupts handling

Message ID 20230923031031.21434-1-rohan.g.thomas@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,1/1] net: stmmac: xgmac: EST interrupts handling | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1343 this patch: 1343
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 1363 this patch: 1363
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1366 this patch: 1366
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 137 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Rohan G Thomas Sept. 23, 2023, 3:10 a.m. UTC
Enabled the following EST related interrupts:
  1) Constant Gate Control Error (CGCE)
  2) Head-of-Line Blocking due to Scheduling (HLBS)
  3) Head-of-Line Blocking due to Frame Size (HLBF)
  4) Base Time Register error (BTRE)
  5) Switch to S/W owned list Complete (SWLC)
Also, add EST errors into the ethtool statistic.

The commit e49aa315cb01 ("net: stmmac: EST interrupts handling and
error reporting") and commit 9f298959191b ("net: stmmac: Add EST
errors into ethtool statistic") add EST interrupts handling and error
reporting support to DWMAC4 core. This patch enables the same support
for XGMAC.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    | 27 ++++++
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 89 +++++++++++++++++++
 2 files changed, 116 insertions(+)

Comments

Serge Semin Sept. 26, 2023, 11:25 a.m. UTC | #1
Hi Rohan

On Sat, Sep 23, 2023 at 11:10:31AM +0800, Rohan G Thomas wrote:
> Enabled the following EST related interrupts:
>   1) Constant Gate Control Error (CGCE)
>   2) Head-of-Line Blocking due to Scheduling (HLBS)
>   3) Head-of-Line Blocking due to Frame Size (HLBF)
>   4) Base Time Register error (BTRE)
>   5) Switch to S/W owned list Complete (SWLC)
> Also, add EST errors into the ethtool statistic.
> 
> The commit e49aa315cb01 ("net: stmmac: EST interrupts handling and
> error reporting") and commit 9f298959191b ("net: stmmac: Add EST
> errors into ethtool statistic") add EST interrupts handling and error
> reporting support to DWMAC4 core. This patch enables the same support
> for XGMAC.

So, this is basically a copy of what was done for the DW QoS Eth
IP-core (DW GMAC v4.x/v5.x). IMO it would be better to factor it out
into a separate module together with the rest of the setup methods
like it's done for TC or PTP. But since it implies some much more work
I guess we can leave it as is for now...

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    | 27 ++++++
>  .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 89 +++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 7a8f47e7b728..75782b8cdfe9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -289,6 +289,33 @@
>  #define XGMAC_PTOV_SHIFT		23
>  #define XGMAC_SSWL			BIT(1)
>  #define XGMAC_EEST			BIT(0)
> +#define XGMAC_MTL_EST_STATUS		0x00001058
> +#define XGMAC_BTRL			GENMASK(15, 8)
> +#define XGMAC_BTRL_SHIFT		8
> +#define XGMAC_BTRL_MAX			GENMASK(15, 8)
> +#define XGMAC_CGCE			BIT(4)
> +#define XGMAC_HLBS			BIT(3)
> +#define XGMAC_HLBF			BIT(2)
> +#define XGMAC_BTRE			BIT(1)
> +#define XGMAC_SWLC			BIT(0)
> +#define XGMAC_MTL_EST_SCH_ERR		0x00001060
> +#define XGMAC_MTL_EST_FRM_SZ_ERR	0x00001064
> +#define XGMAC_MTL_EST_FRM_SZ_CAP	0x00001068
> +#define XGMAC_SZ_CAP_HBFS_MASK		GENMASK(14, 0)
> +#define XGMAC_SZ_CAP_HBFQ_SHIFT		16
> +#define XGMAC_SZ_CAP_HBFQ_MASK(val)	\
> +	({					\
> +		typeof(val) _val = (val);	\
> +		(_val > 4 ? GENMASK(18, 16) :	\
> +		 _val > 2 ? GENMASK(17, 16) :	\
> +		 BIT(16));			\
> +	})
> +#define XGMAC_MTL_EST_INT_EN		0x00001070
> +#define XGMAC_IECGCE			BIT(4)
> +#define XGMAC_IEHS			BIT(3)
> +#define XGMAC_IEHF			BIT(2)
> +#define XGMAC_IEBE			BIT(1)
> +#define XGMAC_IECC			BIT(0)
>  #define XGMAC_MTL_EST_GCL_CONTROL	0x00001080
>  #define XGMAC_BTR_LOW			0x0
>  #define XGMAC_BTR_HIGH			0x1
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index f352be269deb..0af0aefa6656 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1469,9 +1469,97 @@ static int dwxgmac3_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg,
>  		ctrl &= ~XGMAC_EEST;
>  
>  	writel(ctrl, ioaddr + XGMAC_MTL_EST_CONTROL);
> +
> +	/* Configure EST interrupt */
> +	if (cfg->enable)
> +		ctrl = XGMAC_IECGCE | XGMAC_IEHS | XGMAC_IEHF | XGMAC_IEBE |
> +		       XGMAC_IECC;
> +	else
> +		ctrl = 0;
> +
> +	writel(ctrl, ioaddr + XGMAC_MTL_EST_INT_EN);
>  	return 0;
>  }
>  
> +static void dwxgmac3_est_irq_status(void __iomem *ioaddr,
> +				    struct net_device *dev,
> +				    struct stmmac_extra_stats *x, u32 txqcnt)
> +{
> +	u32 status, value, feqn, hbfq, hbfs, btrl;
> +	u32 txqcnt_mask = BIT(txqcnt) - 1;
> +
> +	status = readl(ioaddr + XGMAC_MTL_EST_STATUS);
> +
> +	value = XGMAC_CGCE | XGMAC_HLBS | XGMAC_HLBF | XGMAC_BTRE | XGMAC_SWLC;
> +
> +	/* Return if there is no error */
> +	if (!(status & value))
> +		return;
> +
> +	if (status & XGMAC_CGCE) {
> +		/* Clear Interrupt */
> +		writel(XGMAC_CGCE, ioaddr + XGMAC_MTL_EST_STATUS);
> +
> +		x->mtl_est_cgce++;
> +	}
> +
> +	if (status & XGMAC_HLBS) {
> +		value = readl(ioaddr + XGMAC_MTL_EST_SCH_ERR);
> +		value &= txqcnt_mask;
> +
> +		x->mtl_est_hlbs++;
> +
> +		/* Clear Interrupt */
> +		writel(value, ioaddr + XGMAC_MTL_EST_SCH_ERR);
> +
> +		/* Collecting info to shows all the queues that has HLBS
> +		 * issue. The only way to clear this is to clear the
> +		 * statistic.
> +		 */
> +		if (net_ratelimit())
> +			netdev_err(dev, "EST: HLB(sched) Queue 0x%x\n", value);
> +	}
> +
> +	if (status & XGMAC_HLBF) {
> +		value = readl(ioaddr + XGMAC_MTL_EST_FRM_SZ_ERR);
> +		feqn = value & txqcnt_mask;
> +
> +		value = readl(ioaddr + XGMAC_MTL_EST_FRM_SZ_CAP);
> +		hbfq = (value & XGMAC_SZ_CAP_HBFQ_MASK(txqcnt)) >>
> +			XGMAC_SZ_CAP_HBFQ_SHIFT;
> +		hbfs = value & XGMAC_SZ_CAP_HBFS_MASK;
> +
> +		x->mtl_est_hlbf++;
> +
> +		/* Clear Interrupt */
> +		writel(feqn, ioaddr + XGMAC_MTL_EST_FRM_SZ_ERR);
> +
> +		if (net_ratelimit())
> +			netdev_err(dev, "EST: HLB(size) Queue %u Size %u\n",
> +				   hbfq, hbfs);
> +	}
> +
> +	if (status & XGMAC_BTRE) {
> +		if ((status & XGMAC_BTRL) == XGMAC_BTRL_MAX)
> +			x->mtl_est_btrlm++;
> +		else
> +			x->mtl_est_btre++;
> +
> +		btrl = (status & XGMAC_BTRL) >> XGMAC_BTRL_SHIFT;
> +
> +		if (net_ratelimit())
> +			netdev_info(dev, "EST: BTR Error Loop Count %u\n",
> +				    btrl);
> +
> +		writel(XGMAC_BTRE, ioaddr + XGMAC_MTL_EST_STATUS);
> +	}
> +
> +	if (status & XGMAC_SWLC) {
> +		writel(XGMAC_SWLC, ioaddr + XGMAC_MTL_EST_STATUS);
> +		netdev_info(dev, "EST: SWOL has been switched\n");
> +	}
> +}
> +
>  static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
>  				   u32 num_rxq, bool enable)
>  {
> @@ -1541,6 +1629,7 @@ const struct stmmac_ops dwxgmac210_ops = {
>  	.config_l4_filter = dwxgmac2_config_l4_filter,
>  	.set_arp_offload = dwxgmac2_set_arp_offload,
>  	.est_configure = dwxgmac3_est_configure,
> +	.est_irq_status = dwxgmac3_est_irq_status,
>  	.fpe_configure = dwxgmac3_fpe_configure,
>  };
>  
> -- 
> 2.26.2
> 
>
Jakub Kicinski Oct. 2, 2023, 8:55 p.m. UTC | #2
On Tue, 26 Sep 2023 14:25:56 +0300 Serge Semin wrote:
> On Sat, Sep 23, 2023 at 11:10:31AM +0800, Rohan G Thomas wrote:
> > Enabled the following EST related interrupts:
> >   1) Constant Gate Control Error (CGCE)
> >   2) Head-of-Line Blocking due to Scheduling (HLBS)
> >   3) Head-of-Line Blocking due to Frame Size (HLBF)
> >   4) Base Time Register error (BTRE)
> >   5) Switch to S/W owned list Complete (SWLC)
> > Also, add EST errors into the ethtool statistic.
> > 
> > The commit e49aa315cb01 ("net: stmmac: EST interrupts handling and
> > error reporting") and commit 9f298959191b ("net: stmmac: Add EST
> > errors into ethtool statistic") add EST interrupts handling and error
> > reporting support to DWMAC4 core. This patch enables the same support
> > for XGMAC.  
> 
> So, this is basically a copy of what was done for the DW QoS Eth
> IP-core (DW GMAC v4.x/v5.x). IMO it would be better to factor it out
> into a separate module together with the rest of the setup methods
> like it's done for TC or PTP. But since it implies some much more work
> I guess we can leave it as is for now...

I think we can push back a little harder. At the very least we should
get a clear explanation why this copy'n'paste is needed, i.e. what are
the major differences. No?
Serge Semin Oct. 3, 2023, 11:12 a.m. UTC | #3
Hi Jakub

On Mon, Oct 02, 2023 at 01:55:51PM -0700, Jakub Kicinski wrote:
> On Tue, 26 Sep 2023 14:25:56 +0300 Serge Semin wrote:
> > On Sat, Sep 23, 2023 at 11:10:31AM +0800, Rohan G Thomas wrote:
> > > Enabled the following EST related interrupts:
> > >   1) Constant Gate Control Error (CGCE)
> > >   2) Head-of-Line Blocking due to Scheduling (HLBS)
> > >   3) Head-of-Line Blocking due to Frame Size (HLBF)
> > >   4) Base Time Register error (BTRE)
> > >   5) Switch to S/W owned list Complete (SWLC)
> > > Also, add EST errors into the ethtool statistic.
> > > 
> > > The commit e49aa315cb01 ("net: stmmac: EST interrupts handling and
> > > error reporting") and commit 9f298959191b ("net: stmmac: Add EST
> > > errors into ethtool statistic") add EST interrupts handling and error
> > > reporting support to DWMAC4 core. This patch enables the same support
> > > for XGMAC.  
> > 
> > So, this is basically a copy of what was done for the DW QoS Eth
> > IP-core (DW GMAC v4.x/v5.x). IMO it would be better to factor it out
> > into a separate module together with the rest of the setup methods
> > like it's done for TC or PTP. But since it implies some much more work
> > I guess we can leave it as is for now...
> 

> I think we can push back a little harder. At the very least we should
> get a clear explanation why this copy'n'paste is needed, i.e. what are
> the major differences. No?

AFAICS there are only two firm differences between the DW QoS Eth v5.x
and DW XGMAC v3.x ESTs implementations I managed to spot from the
currently available code (My DW XGMAC _v2.11a_ HW manual doesn't have
such feature described):

1. MTL_EST_CONTROL.PTOV field offset and width: QoS [31;24], XGMAC [31;23];

2. EST CSRs base addresses: QoS 0x0c50, XGMAC 0x1050.

After the patch in subject is applied the EST config method and EST
IRQ status handlers will look almost identical except the next two
things:

1. XGMAC EST-write implementation performs atomic
MTL_EST_GCL_CONTROL.SRWO flag polling. Initially added by Jose. Not
sure whether the sleepless polling is really needed because the
stmmac_est_configure() is always called within the est->lock mutex
being taken.

2. PTP time offset setup performed by means of the
MTL_EST_CONTROL.PTOV field. DW QoS Eth v5.x HW manual claims it's "The
value of PTP Clock period multiplied by 6 in nanoseconds." So either
Jose got mistaken by using _9_ for DW XGMAC v3.x or the DW XGMAC
indeed is different in that aspect.

The rest of the differences is in the macros and functions names.
Please see the attached diff-file I collected for the EST-related code
in the DW QoS Eth v5.x and DW XGMAC v3.x modules.

Getting back to the refactoring. So basically the EST part can be
factored out in a similar way as it's done for instance for TC/PTP:

1. Define EST_GMAC4_OFFSET and EST_XGMAC_OFFSET macros in the new
header file "stmmac_est.h" (I'd prefer to drop the stmmac_-prefix but
all the sub-modules except "mmc" tends to have it).

2. Add stmmac_regs_off.est field and initialize it with the respective
offsets for the QoS and XGMAC IP-cores in the stmmac_hw static array
(hwif.c).

3. Add stmmac_priv.estaddr field and initialize it with 
(ioaddr + stmmac_regs_off.est) in the stmmac_hwif_init() function (hwif.c).

4. Define stmmac_est_ops structure in "hwif.h" with two
callbacks: .est_configure and .est_irq_status. Add the
stmmac_hwif_entry.est field of the const-pointer type to that
structure instance.

5. Redefined the stmmac_est_configure() and stmmac_est_irq_status()
macro-functions to using the callbacks from the stmmac_priv->hw->est
(hwif.h).

6. Drop the stmmac_ops.est_configure and stmmac_ops.est_irq_status.
fields (hwif.h).

7. Move all the EST-related macros to the "stmmac_est.h" file. Make
sure they are defined with EST_-prefix and the CSRs are based from
zero address since they will be utilized as the offsets for the
stmmac_priv.estaddr field.

8. Move all the EST-related functions to the new "stmmac_est.c"
module. Make sure they use the new generic EST CSRs macros and the
stmmac_priv.estaddr field as the base address of the EST CSRs. Take
into account the possible differences in the MTL_EST_CONTROL.PTOV
field initialization for the DW QoS and DW XGMAC IP-cores. Add
stmmac_est.o to stmmac-objs list in the Makefile.

9. Define stmmac_est_ops structure instance (dwmac410_est_ops) in
"stmmac_est.c" and initialize its fields with the est_configure() and
est_irq_status() functions define in 8.

10. Use the stmmac_est_ops structure instance (dwmac410_est_ops) to
initialize the stmmac_hwif_entry.est field to point to that instance
for the DW Eth QoS and DW XGMAC IP-cores.

If I didn't miss some details after that we'll have a common EST
module utilized for both DW QoS Eth and DW XGMAC IP-cores.

-Serge(y)
Jakub Kicinski Oct. 4, 2023, 4:26 p.m. UTC | #4
On Tue, 3 Oct 2023 14:12:15 +0300 Serge Semin wrote:
> If I didn't miss some details after that we'll have a common EST
> module utilized for both DW QoS Eth and DW XGMAC IP-cores.

So the question now is whether we want Rohan to do this conversion
_first_, in DW QoS 5, and then add xgmac part. Or the patch should
go in as is and you'll follow up with the conversion?
Rohan G Thomas Oct. 5, 2023, 12:14 p.m. UTC | #5
On Wed, 4 Oct 2023 09:26:13 -0700 Jakub Kicinski wrote:
> On Tue, 3 Oct 2023 14:12:15 +0300 Serge Semin wrote:
> > If I didn't miss some details after that we'll have a common EST
> > module utilized for both DW QoS Eth and DW XGMAC IP-cores.
> 
> So the question now is whether we want Rohan to do this conversion _first_,
> in DW QoS 5, and then add xgmac part. Or the patch should go in as is and
> you'll follow up with the conversion?

Hi Jakub, Serge,

If agreed, this commit can go in. I can submit another patch with the
refactoring suggested by Serge.

Again, thanks Serge for the prompt response. Regarding the below point in your
earlier response,

> > 2. PTP time offset setup performed by means of the
> > MTL_EST_CONTROL.PTOV field. DW QoS Eth v5.x HW manual claims it's "The
> > value of PTP Clock period multiplied by 6 in nanoseconds." So either Jose got
> > mistaken by using _9_ for DW XGMAC v3.x or the DW XGMAC indeed is
> > different in that aspect.

This is a little confusing...
I referred databooks for DW QoS Eth v5.30a and DW XGMAC v3.10a. In both this is
mentioned as "The value of PTP Clock period multiplied by 9 in nanoseconds".

Best Regards,
Rohan
Jakub Kicinski Oct. 5, 2023, 2:05 p.m. UTC | #6
On Thu,  5 Oct 2023 20:14:41 +0800 Rohan G Thomas wrote:
> > So the question now is whether we want Rohan to do this conversion _first_,
> > in DW QoS 5, and then add xgmac part. Or the patch should go in as is and
> > you'll follow up with the conversion?  
> 
> If agreed, this commit can go in. I can submit another patch with the
> refactoring suggested by Serge.

Did you miss the emphasis I put on the word "first" in my reply?
Cleanup first, nobody will be keeping track whether your fulfilled 
your promises or not :|
Rohan G Thomas Oct. 6, 2023, 7:23 a.m. UTC | #7
On Thu, 5 Oct 2023 07:05:38 -0700 Jakub Kicinski wrote:
> On Thu, 5 Oct 2023 20:14:41 +0800 Rohan G Thomas wrote:
> > > So the question now is whether we want Rohan to do this conversion
> > > _first_, in DW QoS 5, and then add xgmac part. Or the patch should
> > > go in as is and you'll follow up with the conversion?
> >
> > If agreed, this commit can go in. I can submit another patch with the
> > refactoring suggested by Serge.
> 
> Did you miss the emphasis I put on the word "first" in my reply?
> Cleanup first, nobody will be keeping track whether your fulfilled your
> promises or not :|

Hi Jakub,

Agreed. I'll do the cleanup first.

Best Regards,
Rohan
Serge Semin Oct. 6, 2023, 10:08 a.m. UTC | #8
Hi Rohan, Jakub

On Fri, Oct 06, 2023 at 03:23:19PM +0800, Rohan G Thomas wrote:
> On Thu, 5 Oct 2023 07:05:38 -0700 Jakub Kicinski wrote:
> > On Thu, 5 Oct 2023 20:14:41 +0800 Rohan G Thomas wrote:
> > > > So the question now is whether we want Rohan to do this conversion
> > > > _first_, in DW QoS 5, and then add xgmac part. Or the patch should
> > > > go in as is and you'll follow up with the conversion?

Jakub, this was my intention if Rohan wouldn't have agreed to perform
the cleanup. Though I couldn't promise to do that in an instant.
I would have needed a month or two to find a free time-spot for that.

> > >
> > > If agreed, this commit can go in. I can submit another patch with the
> > > refactoring suggested by Serge.
> > 
> > Did you miss the emphasis I put on the word "first" in my reply?
> > Cleanup first, nobody will be keeping track whether your fulfilled your
> > promises or not :|
> 
> Hi Jakub,
> 
> Agreed. I'll do the cleanup first.

Rohan, thanks in advance. Although I don't see why it's required to be
done in the prescribed order only as long as the update comes in a
_single_ patchset. Adding EST IRS-status support to XGMAC core module
first, then factoring out both XGMAC and QoS (note both 4.x and 5.x
seems to support that) EST code would be also acceptable. Seeing you
have already done the first part, it would have taken less work in
general.

Jakub, what do you say if Rohan will just re-submit v2 with the
addition cleanup patch and let him to decided whether the cleanup
should be done first or after his XGMAC-EST IRQ update?

> > > Again, thanks Serge for the prompt response. Regarding the below point in your
> > > earlier response,

> > > > > 2. PTP time offset setup performed by means of the
> > > > > MTL_EST_CONTROL.PTOV field. DW QoS Eth v5.x HW manual claims it's "The
> > > > > value of PTP Clock period multiplied by 6 in nanoseconds." So either Jose got
> > > > > mistaken by using _9_ for DW XGMAC v3.x or the DW XGMAC indeed is
> > > > > different in that aspect.
> > > 
> > > This is a little confusing...
> > > I referred databooks for DW QoS Eth v5.30a and DW XGMAC v3.10a. In both this is
> > > mentioned as "The value of PTP Clock period multiplied by 9 in nanoseconds".

Interesting thing. My DW QoS Eth _v5.10a_ HW manual explicitly states
that it's multiplied by _6_ in nanoseconds (just rechecked). So either
there is a difference between the minor DW QoS Eth IP-core releases or
the older HW-manuals have had a typo in the MTL_EST_CONTROL.PTOV field
description. Synopsys normally describes such changes (whether it was
a mistake or a functional change) in the IP-core release notes. The
release notes document is supplied as a separate pdf file. Alas I
don't have one.( Even if I had it it would have been useless since the
change was introduced in the newer QoS IP-cores. Rohan, do you happen
to have the release notes for DW QoS Eth IP-core v5.30 at hands?
Something like DWC_ether_qos_rc_relnotes.pdf?

Also please double check that your DW QoS Eth v5.30a for sure states
that MTL_EST_CONTROL.PTOV contains value multiplied by _6_. So we
wouldn't be wasting time trying to workaround a more complex problem
than we already have.

-Serge(y)

> 
> Best Regards,
> Rohan
Jakub Kicinski Oct. 6, 2023, 1:35 p.m. UTC | #9
On Fri, 6 Oct 2023 13:08:33 +0300 Serge Semin wrote:
> Jakub, what do you say if Rohan will just re-submit v2 with the
> addition cleanup patch and let him to decided whether the cleanup
> should be done first or after his XGMAC-EST IRQ update?

Sure thing, whatever is more readable for the reviewer.
Rohan G Thomas Dec. 1, 2023, 6:49 a.m. UTC | #10
On Fri, 6 Oct 2023 13:08:33 +0300 Serge Semin wrote:
> Hi Rohan, Jakub
> ...
> Interesting thing. My DW QoS Eth _v5.10a_ HW manual explicitly states that
> it's multiplied by _6_ in nanoseconds (just rechecked). So either there is a
> difference between the minor DW QoS Eth IP-core releases or the older HW-
> manuals have had a typo in the MTL_EST_CONTROL.PTOV field description.
> Synopsys normally describes such changes (whether it was a mistake or a
> functional change) in the IP-core release notes. The release notes document
> is supplied as a separate pdf file. Alas I don't have one.( Even if I had it it
> would have been useless since the change was introduced in the newer QoS
> IP-cores. Rohan, do you happen to have the release notes for DW QoS Eth IP-
> core v5.30 at hands?
> Something like DWC_ether_qos_rc_relnotes.pdf?

Hi Serge,

Sorry for the delay. Sends out another version with the suggested changes.

Managed to get DWC_ether_qos_relnotes.pdf for v5.20a and v5.30a. But I couldn't
find anything related to this. So for refactoring, I'm keeping the logic as in
the upstream code to avoid any regression.

> 
> Also please double check that your DW QoS Eth v5.30a for sure states that
> MTL_EST_CONTROL.PTOV contains value multiplied by _6_. So we wouldn't
> be wasting time trying to workaround a more complex problem than we
> already have.

Yes, I checked this again. For DW QoS Eth v5.30a the multiplier for 
MTL_EST_CONTROL.PTOV is _9_ as per the databook.

Also noticed a similar difference for MTL_EST_Status.BTRL field length. As per
the upstream code and DW QoS Eth v5.10a databook this field covers bit 8 to bit
11. But for the xgmac IP and DW QoS Eth v5.30a databook this field covers bit 8
to bit 15. Again nothing mentioned in the release notes. Here also I'm keeping
the logic as in the upstream code to avoid any regression.

> 
> -Serge(y)
> 

Best Regards,
Rohan
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 7a8f47e7b728..75782b8cdfe9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -289,6 +289,33 @@ 
 #define XGMAC_PTOV_SHIFT		23
 #define XGMAC_SSWL			BIT(1)
 #define XGMAC_EEST			BIT(0)
+#define XGMAC_MTL_EST_STATUS		0x00001058
+#define XGMAC_BTRL			GENMASK(15, 8)
+#define XGMAC_BTRL_SHIFT		8
+#define XGMAC_BTRL_MAX			GENMASK(15, 8)
+#define XGMAC_CGCE			BIT(4)
+#define XGMAC_HLBS			BIT(3)
+#define XGMAC_HLBF			BIT(2)
+#define XGMAC_BTRE			BIT(1)
+#define XGMAC_SWLC			BIT(0)
+#define XGMAC_MTL_EST_SCH_ERR		0x00001060
+#define XGMAC_MTL_EST_FRM_SZ_ERR	0x00001064
+#define XGMAC_MTL_EST_FRM_SZ_CAP	0x00001068
+#define XGMAC_SZ_CAP_HBFS_MASK		GENMASK(14, 0)
+#define XGMAC_SZ_CAP_HBFQ_SHIFT		16
+#define XGMAC_SZ_CAP_HBFQ_MASK(val)	\
+	({					\
+		typeof(val) _val = (val);	\
+		(_val > 4 ? GENMASK(18, 16) :	\
+		 _val > 2 ? GENMASK(17, 16) :	\
+		 BIT(16));			\
+	})
+#define XGMAC_MTL_EST_INT_EN		0x00001070
+#define XGMAC_IECGCE			BIT(4)
+#define XGMAC_IEHS			BIT(3)
+#define XGMAC_IEHF			BIT(2)
+#define XGMAC_IEBE			BIT(1)
+#define XGMAC_IECC			BIT(0)
 #define XGMAC_MTL_EST_GCL_CONTROL	0x00001080
 #define XGMAC_BTR_LOW			0x0
 #define XGMAC_BTR_HIGH			0x1
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index f352be269deb..0af0aefa6656 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1469,9 +1469,97 @@  static int dwxgmac3_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg,
 		ctrl &= ~XGMAC_EEST;
 
 	writel(ctrl, ioaddr + XGMAC_MTL_EST_CONTROL);
+
+	/* Configure EST interrupt */
+	if (cfg->enable)
+		ctrl = XGMAC_IECGCE | XGMAC_IEHS | XGMAC_IEHF | XGMAC_IEBE |
+		       XGMAC_IECC;
+	else
+		ctrl = 0;
+
+	writel(ctrl, ioaddr + XGMAC_MTL_EST_INT_EN);
 	return 0;
 }
 
+static void dwxgmac3_est_irq_status(void __iomem *ioaddr,
+				    struct net_device *dev,
+				    struct stmmac_extra_stats *x, u32 txqcnt)
+{
+	u32 status, value, feqn, hbfq, hbfs, btrl;
+	u32 txqcnt_mask = BIT(txqcnt) - 1;
+
+	status = readl(ioaddr + XGMAC_MTL_EST_STATUS);
+
+	value = XGMAC_CGCE | XGMAC_HLBS | XGMAC_HLBF | XGMAC_BTRE | XGMAC_SWLC;
+
+	/* Return if there is no error */
+	if (!(status & value))
+		return;
+
+	if (status & XGMAC_CGCE) {
+		/* Clear Interrupt */
+		writel(XGMAC_CGCE, ioaddr + XGMAC_MTL_EST_STATUS);
+
+		x->mtl_est_cgce++;
+	}
+
+	if (status & XGMAC_HLBS) {
+		value = readl(ioaddr + XGMAC_MTL_EST_SCH_ERR);
+		value &= txqcnt_mask;
+
+		x->mtl_est_hlbs++;
+
+		/* Clear Interrupt */
+		writel(value, ioaddr + XGMAC_MTL_EST_SCH_ERR);
+
+		/* Collecting info to shows all the queues that has HLBS
+		 * issue. The only way to clear this is to clear the
+		 * statistic.
+		 */
+		if (net_ratelimit())
+			netdev_err(dev, "EST: HLB(sched) Queue 0x%x\n", value);
+	}
+
+	if (status & XGMAC_HLBF) {
+		value = readl(ioaddr + XGMAC_MTL_EST_FRM_SZ_ERR);
+		feqn = value & txqcnt_mask;
+
+		value = readl(ioaddr + XGMAC_MTL_EST_FRM_SZ_CAP);
+		hbfq = (value & XGMAC_SZ_CAP_HBFQ_MASK(txqcnt)) >>
+			XGMAC_SZ_CAP_HBFQ_SHIFT;
+		hbfs = value & XGMAC_SZ_CAP_HBFS_MASK;
+
+		x->mtl_est_hlbf++;
+
+		/* Clear Interrupt */
+		writel(feqn, ioaddr + XGMAC_MTL_EST_FRM_SZ_ERR);
+
+		if (net_ratelimit())
+			netdev_err(dev, "EST: HLB(size) Queue %u Size %u\n",
+				   hbfq, hbfs);
+	}
+
+	if (status & XGMAC_BTRE) {
+		if ((status & XGMAC_BTRL) == XGMAC_BTRL_MAX)
+			x->mtl_est_btrlm++;
+		else
+			x->mtl_est_btre++;
+
+		btrl = (status & XGMAC_BTRL) >> XGMAC_BTRL_SHIFT;
+
+		if (net_ratelimit())
+			netdev_info(dev, "EST: BTR Error Loop Count %u\n",
+				    btrl);
+
+		writel(XGMAC_BTRE, ioaddr + XGMAC_MTL_EST_STATUS);
+	}
+
+	if (status & XGMAC_SWLC) {
+		writel(XGMAC_SWLC, ioaddr + XGMAC_MTL_EST_STATUS);
+		netdev_info(dev, "EST: SWOL has been switched\n");
+	}
+}
+
 static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
 				   u32 num_rxq, bool enable)
 {
@@ -1541,6 +1629,7 @@  const struct stmmac_ops dwxgmac210_ops = {
 	.config_l4_filter = dwxgmac2_config_l4_filter,
 	.set_arp_offload = dwxgmac2_set_arp_offload,
 	.est_configure = dwxgmac3_est_configure,
+	.est_irq_status = dwxgmac3_est_irq_status,
 	.fpe_configure = dwxgmac3_fpe_configure,
 };