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 <[email protected]>
---
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))) {
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))
--
2.7.4
On 11/23/2018 3:59 AM, Harini Katakam wrote:
> +/* 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;
> };
Hi Harini,
Could this be made into simpler by instead adding a caps bit, named
(perhaps) MACB_CAPS_BUGGY_RXUBR or MACB_CAPS_NEEDS_RXUBR_RESETS or
something?
That would save needing to add a new u32 field into the macb_config and
macb structs (both of which already have this caps field).
Hi Brandon,
On Thu, Nov 29, 2018 at 2:39 AM Brandon Streiff <[email protected]> wrote:
>
> On 11/23/2018 3:59 AM, Harini Katakam wrote:
> > +/* 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;
> > };
>
> Hi Harini,
>
> Could this be made into simpler by instead adding a caps bit, named
> (perhaps) MACB_CAPS_BUGGY_RXUBR or MACB_CAPS_NEEDS_RXUBR_RESETS or
> something?
>
> That would save needing to add a new u32 field into the macb_config and
> macb structs (both of which already have this caps field).
Thanks for the review. Yes, the caps field already has all that.
The only reason I separated it was that CAPS generally referred to a feature and
this is a workaround for an errata. There's no functional need, of course and
I'm ok to integrate it as well.
Regards,
Harini
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 <[email protected]>
> ---
> 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))
>
Hi Claudiu,
On Thu, Nov 29, 2018 at 3:51 PM <[email protected]> wrote:
>
>
>
> On 23.11.2018 11:59, Harini Katakam wrote:
<snip>
> > - 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.
>
Yes, I tested it on ZynqMP which does NOT have this errata - did perf and
ping flood and dint see any issues.
Just FYI, the errata in the IP version in Zynq causes an RX lock up under
high stress (when there is obviously multiple RXUBR interrupts). The RX reset
is a pre-emptive workaround to *mostly* avoid the issue.
Please see Answer record 52028 in
https://www.xilinx.com/support/documentation/errata/en247.pdf
> 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));
>
>
Thanks for pointing this out - yes i'll move the WTC code outside the check.
Regards,
Harini
On 29.11.2018 12:38, Harini Katakam wrote:
> Hi Claudiu,
> On Thu, Nov 29, 2018 at 3:51 PM <[email protected]> wrote:
>>
>>
>>
>> On 23.11.2018 11:59, Harini Katakam wrote:
> <snip>
>>> - 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.
>>
>
> Yes, I tested it on ZynqMP which does NOT have this errata - did perf and
> ping flood and dint see any issues.
Great! Thank you, Harini!
> Just FYI, the errata in the IP version in Zynq causes an RX lock up under
> high stress (when there is obviously multiple RXUBR interrupts). The RX reset
> is a pre-emptive workaround to *mostly* avoid the issue.
> Please see Answer record 52028 in
> https://www.xilinx.com/support/documentation/errata/en247.pdf
>
>> 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));
>>
>>
> Thanks for pointing this out - yes i'll move the WTC code outside the check.
>
> Regards,
> Harini
>
On 28.11.2018 23:09, Brandon Streiff wrote:
> On 11/23/2018 3:59 AM, Harini Katakam wrote:
>> +/* 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;
>> };
>
> Hi Harini,
>
> Could this be made into simpler by instead adding a caps bit, named
> (perhaps) MACB_CAPS_BUGGY_RXUBR or MACB_CAPS_NEEDS_RXUBR_RESETS or
> something?
>
> That would save needing to add a new u32 field into the macb_config and
> macb structs (both of which already have this caps field).
>
I avoid duplicating these fields from struct macb_config to struct macb I
would start to move macb_config part of struct macb as a const * member
(see [1]).
[1]
https://github.com/linux4sam/linux-at91/commit/8c588136189148d79c4078c55da2242a2ba2b8d3#diff-921c839dba439fd502a24aed0845dc9d