Re: [RFC PATCH] net: macb: Apply RXUBR workaround only to versions with errata

[Claudiu.Beznea]

On 23.11.2018 11:59, Harini Katakam wrote:
> The interrupt handler contains a workaround for RX hang applicable
> to Zynq and AT91 only. Subsequent versions do not need this
> workaround. This workaround unecessarily reset RX whenever RX used
> bit read is observed, which can be often under heavy traffic.Hence
> introduce an errata field and a check to enable this workaround.
> 
> Signed-off-by: Harini Katakam <harini.katakam@xxxxxxxxxx>
> ---
> Note: Enabled the errata in zynq and at91 configs only.
> Please advise if any other versions are affected by this errata.
> 
>  drivers/net/ethernet/cadence/macb.h      | 6 ++++++
>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3d45f4c..f6903d6 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -648,6 +648,9 @@
>  #define MACB_CAPS_SG_DISABLED			0x40000000
>  #define MACB_CAPS_MACB_IS_GEM			0x80000000
>  
> +/* Errata mask bits */
> +#define MACB_ERRATA_RXLOCKUP			0x00000001
> +
>  /* LSO settings */
>  #define MACB_LSO_UFO_ENABLE			0x01
>  #define MACB_LSO_TSO_ENABLE			0x02
> @@ -1085,6 +1088,7 @@ struct macb_config {
>  			    struct clk **rx_clk);
>  	int	(*init)(struct platform_device *pdev);
>  	int	jumbo_max_len;
> +	u32	errata;
>  };
>  
>  struct tsu_incr {
> @@ -1214,6 +1218,8 @@ struct macb {
>  
>  	int	rx_bd_rd_prefetch;
>  	int	tx_bd_rd_prefetch;
> +
> +	u32 errata;
>  };
>  
>  #ifdef CONFIG_MACB_USE_HWSTAMP
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 1d86b4d..f0bb8a4 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1379,7 +1379,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  		 * the at91 manual, section 41.3.1 or the Zynq manual
>  		 * section 16.7.4 for details.
>  		 */
> -		if (status & MACB_BIT(RXUBR)) {
> +		if ((bp->errata & MACB_ERRATA_RXLOCKUP) &&
> +		    (status & MACB_BIT(RXUBR))) {

Just asking, did you manage to test this on other platforms that haven't
this issue?
SAMA5D2 datasheet [1] states this:
"When in packet buffer full store and forward mode, only good received
frames are written out of the DMA, so no
 fragments will exist in the AHB buffers due to MAC receiver errors. There
is still the possibility of fragments due to
 DMA errors, for example used bit read on the second buffer of a
multibuffer frame."

But it is true that nowhere is presented that this must be special treated.

Moreover, if you do this only for MACB_ERRATA_RXLOCKUP and still have RXUBR
interrupt enabled every time it would not make sense to still enable it.
Or, if you want it enabled every time, you should clear it no matter the
MACB_ERRATA_RXLOCKUP is set or not, with something like this:

                        if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)

                                queue_writel(queue, ISR, MACB_BIT(RXUBR));


Thank you,
Claudiu Beznea

[1] http://ww1.microchip.com/downloads/en/devicedoc/ds60001476b.pdf

>  			ctrl = macb_readl(bp, NCR);
>  			macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
>  			wmb();
> @@ -3835,6 +3836,7 @@ static const struct macb_config at91sam9260_config = {
>  	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> +	.errata = MACB_ERRATA_RXLOCKUP,
>  };
>  
>  static const struct macb_config sama5d3macb_config = {
> @@ -3900,6 +3902,7 @@ static const struct macb_config zynq_config = {
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> +	.errata = MACB_ERRATA_RXLOCKUP,
>  };
>  
>  static const struct of_device_id macb_dt_ids[] = {
> @@ -4005,8 +4008,10 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->hclk = hclk;
>  	bp->tx_clk = tx_clk;
>  	bp->rx_clk = rx_clk;
> -	if (macb_config)
> +	if (macb_config) {
>  		bp->jumbo_max_len = macb_config->jumbo_max_len;
> +		bp->errata = macb_config->errata;
> +	}
>  
>  	bp->wol = 0;
>  	if (of_get_property(np, "magic-packet", NULL))
>