drivers/net/ethernet/cadence/macb.h | 10 +++ drivers/net/ethernet/cadence/macb_main.c | 79 +++++++++++++++++++++++- 2 files changed, 86 insertions(+), 3 deletions(-)
From: Maulik Jodhani <maulik.jodhani@xilinx.com>
- Validate FCS in receive interrupt handler if Rx checksum offloading
is disabled
- Get rx-watermark value from DT
Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
---
drivers/net/ethernet/cadence/macb.h | 10 +++
drivers/net/ethernet/cadence/macb_main.c | 79 +++++++++++++++++++++++-
2 files changed, 86 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 9c410f93a103..e4eebe8c8c46 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -82,6 +82,7 @@
#define GEM_NCFGR 0x0004 /* Network Config */
#define GEM_USRIO 0x000c /* User IO */
#define GEM_DMACFG 0x0010 /* DMA Configuration */
+#define GEM_PBUFRXCUT 0x0044 /* RX Partial Store and Forward */
#define GEM_JML 0x0048 /* Jumbo Max Length */
#define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */
#define GEM_HRB 0x0080 /* Hash Bottom */
@@ -342,6 +343,11 @@
#define GEM_ADDR64_OFFSET 30 /* Address bus width - 64b or 32b */
#define GEM_ADDR64_SIZE 1
+/* Bitfields in PBUFRXCUT */
+#define GEM_WTRMRK_OFFSET 0 /* Watermark value offset */
+#define GEM_WTRMRK_SIZE 12
+#define GEM_ENCUTTHRU_OFFSET 31 /* Enable RX partial store and forward */
+#define GEM_ENCUTTHRU_SIZE 1
/* Bitfields in NSR */
#define MACB_NSR_LINK_OFFSET 0 /* pcs_link_state */
@@ -720,6 +726,7 @@
#define MACB_CAPS_NEED_TSUCLK 0x00000400
#define MACB_CAPS_PCS 0x01000000
#define MACB_CAPS_HIGH_SPEED 0x02000000
+#define MACB_CAPS_PARTIAL_STORE_FORWARD 0x00000800
#define MACB_CAPS_CLK_HW_CHG 0x04000000
#define MACB_CAPS_MACB_IS_EMAC 0x08000000
#define MACB_CAPS_FIFO_MODE 0x10000000
@@ -1296,6 +1303,9 @@ struct macb {
u32 wol;
+ /* holds value of rx watermark value for pbuf_rxcutthru register */
+ u16 rx_watermark;
+
struct macb_ptp_info *ptp_info; /* macb-ptp interface */
struct phy *sgmii_phy; /* for ZynqMP SGMII mode */
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 95667b979fab..1f09fe1eec76 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -39,6 +39,7 @@
#include <linux/ptp_classify.h>
#include <linux/reset.h>
#include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/crc32.h>
#include "macb.h"
/* This structure is only used for MACB on SiFive FU540 devices */
@@ -1314,6 +1315,15 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
*/
}
+static int macb_validate_hw_csum(struct sk_buff *skb)
+{
+ u32 pkt_csum = *((u32 *)&skb->data[skb->len - ETH_FCS_LEN]);
+ u32 csum = ~crc32_le(~0, skb_mac_header(skb),
+ skb->len + ETH_HLEN - ETH_FCS_LEN);
+
+ return (pkt_csum != csum);
+}
+
static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
int budget)
{
@@ -1375,6 +1385,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
bp->rx_buffer_size, DMA_FROM_DEVICE);
skb->protocol = eth_type_trans(skb, bp->dev);
+
+ /* Validate MAC fcs if RX checsum offload disabled */
+ if (!(bp->dev->features & NETIF_F_RXCSUM)) {
+ if (macb_validate_hw_csum(skb)) {
+ netdev_err(bp->dev, "incorrect FCS\n");
+ bp->dev->stats.rx_dropped++;
+ break;
+ }
+ }
+
skb_checksum_none_assert(skb);
if (bp->dev->features & NETIF_F_RXCSUM &&
!(bp->dev->flags & IFF_PROMISC) &&
@@ -1472,6 +1492,19 @@ static int macb_rx_frame(struct macb_queue *queue, struct napi_struct *napi,
break;
}
+ /* Validate MAC fcs if RX checsum offload disabled */
+ if (!(bp->dev->features & NETIF_F_RXCSUM)) {
+ if (macb_validate_hw_csum(skb)) {
+ netdev_err(bp->dev, "incorrect FCS\n");
+ bp->dev->stats.rx_dropped++;
+
+ /* Make descriptor updates visible to hardware */
+ wmb();
+
+ return 1;
+ }
+ }
+
/* Make descriptor updates visible to hardware */
wmb();
@@ -2567,6 +2600,10 @@ static void macb_reset_hw(struct macb *bp)
macb_writel(bp, TSR, -1);
macb_writel(bp, RSR, -1);
+ /* Disable RX partial store and forward and reset watermark value */
+ if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
+ gem_writel(bp, PBUFRXCUT, 0xFFF);
+
/* Disable all interrupts */
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
queue_writel(queue, IDR, -1);
@@ -2700,7 +2737,11 @@ static void macb_init_hw(struct macb *bp)
config = macb_mdc_clk_div(bp);
config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
- config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
+
+ /* Do not discard Rx FCS if RX checsum offload disabled */
+ if (bp->dev->features & NETIF_F_RXCSUM)
+ config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
+
if (bp->caps & MACB_CAPS_JUMBO)
config |= MACB_BIT(JFRAME); /* Enable jumbo frames */
else
@@ -2720,6 +2761,15 @@ static void macb_init_hw(struct macb *bp)
bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
macb_configure_dma(bp);
+
+ /* Enable RX partial store and forward and set watermark */
+ if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
+ gem_writel(bp, PBUFRXCUT,
+ (gem_readl(bp, PBUFRXCUT) &
+ GEM_BF(WTRMRK, bp->rx_watermark)) |
+ GEM_BIT(ENCUTTHRU));
+ }
+
}
/* The hash address register is 64 bits long and takes up two
@@ -3812,10 +3862,29 @@ static void macb_configure_caps(struct macb *bp,
const struct macb_config *dt_conf)
{
u32 dcfg;
+ int retval;
if (dt_conf)
bp->caps = dt_conf->caps;
+ /* By default we set to partial store and forward mode for zynqmp.
+ * Disable if not set in devicetree.
+ */
+ if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
+ retval = of_property_read_u16(bp->pdev->dev.of_node,
+ "rx-watermark",
+ &bp->rx_watermark);
+
+ /* Disable partial store and forward in case of error or
+ * invalid watermark value
+ */
+ if (retval || bp->rx_watermark > 0xFFF) {
+ dev_info(&bp->pdev->dev,
+ "Not enabling partial store and forward\n");
+ bp->caps &= ~MACB_CAPS_PARTIAL_STORE_FORWARD;
+ }
+ }
+
if (hw_is_gem(bp->regs, bp->native_io)) {
bp->caps |= MACB_CAPS_MACB_IS_GEM;
@@ -4072,6 +4141,8 @@ static int macb_init(struct platform_device *pdev)
/* Checksum offload is only available on gem with packet buffer */
if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+ if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
+ dev->hw_features &= ~NETIF_F_RXCSUM;
if (bp->caps & MACB_CAPS_SG_DISABLED)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;
@@ -4763,7 +4834,8 @@ static const struct macb_config np4_config = {
static const struct macb_config zynqmp_config = {
.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
MACB_CAPS_JUMBO |
- MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
+ MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+ MACB_CAPS_PARTIAL_STORE_FORWARD,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = init_reset_optional,
@@ -4811,7 +4883,8 @@ static const struct macb_config sama7g5_emac_config = {
static const struct macb_config versal_config = {
.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
- MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
+ MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+ MACB_CAPS_NEED_TSUCLK | MACB_CAPS_PARTIAL_STORE_FORWARD,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = init_reset_optional,
--
2.36.1On 13.12.2022 14:12, Pranavi Somisetty wrote:
> [Some people who received this message don't often get email from pranavi.somisetty@amd.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
>
> - Validate FCS in receive interrupt handler if Rx checksum offloading
> is disabled
> - Get rx-watermark value from DT
>
> Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> ---
> drivers/net/ethernet/cadence/macb.h | 10 +++
> drivers/net/ethernet/cadence/macb_main.c | 79 +++++++++++++++++++++++-
> 2 files changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 9c410f93a103..e4eebe8c8c46 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -82,6 +82,7 @@
> #define GEM_NCFGR 0x0004 /* Network Config */
> #define GEM_USRIO 0x000c /* User IO */
> #define GEM_DMACFG 0x0010 /* DMA Configuration */
> +#define GEM_PBUFRXCUT 0x0044 /* RX Partial Store and Forward */
> #define GEM_JML 0x0048 /* Jumbo Max Length */
> #define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */
> #define GEM_HRB 0x0080 /* Hash Bottom */
> @@ -342,6 +343,11 @@
> #define GEM_ADDR64_OFFSET 30 /* Address bus width - 64b or 32b */
> #define GEM_ADDR64_SIZE 1
>
> +/* Bitfields in PBUFRXCUT */
> +#define GEM_WTRMRK_OFFSET 0 /* Watermark value offset */
> +#define GEM_WTRMRK_SIZE 12
From documentation I see watermark size depends on implementation. E.g. on
SAMA7G5 it is on 10 bits (0-9). And each platform could retrieve this value
from designcfg_debug2 register (offset 0x284 bits 22-25). Can you take that
into account? It should be less platform dependent this way.
> +#define GEM_ENCUTTHRU_OFFSET 31 /* Enable RX partial store and forward */
> +#define GEM_ENCUTTHRU_SIZE 1
>
> /* Bitfields in NSR */
> #define MACB_NSR_LINK_OFFSET 0 /* pcs_link_state */
> @@ -720,6 +726,7 @@
> #define MACB_CAPS_NEED_TSUCLK 0x00000400
> #define MACB_CAPS_PCS 0x01000000
> #define MACB_CAPS_HIGH_SPEED 0x02000000
> +#define MACB_CAPS_PARTIAL_STORE_FORWARD 0x00000800
> #define MACB_CAPS_CLK_HW_CHG 0x04000000
> #define MACB_CAPS_MACB_IS_EMAC 0x08000000
> #define MACB_CAPS_FIFO_MODE 0x10000000
> @@ -1296,6 +1303,9 @@ struct macb {
>
> u32 wol;
>
> + /* holds value of rx watermark value for pbuf_rxcutthru register */
> + u16 rx_watermark;
> +
As its value could be retrived from bits 22-25 at offset 0x284 u8 should be
enough here. Also, maybe move it at the end of structure to avoid any padding.
> struct macb_ptp_info *ptp_info; /* macb-ptp interface */
>
> struct phy *sgmii_phy; /* for ZynqMP SGMII mode */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 95667b979fab..1f09fe1eec76 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -39,6 +39,7 @@
> #include <linux/ptp_classify.h>
> #include <linux/reset.h>
> #include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/crc32.h>
> #include "macb.h"
>
> /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -1314,6 +1315,15 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
> */
> }
>
> +static int macb_validate_hw_csum(struct sk_buff *skb)
> +{
> + u32 pkt_csum = *((u32 *)&skb->data[skb->len - ETH_FCS_LEN]);
> + u32 csum = ~crc32_le(~0, skb_mac_header(skb),
> + skb->len + ETH_HLEN - ETH_FCS_LEN);
> +
> + return (pkt_csum != csum);
> +}
> +
> static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> int budget)
> {
> @@ -1375,6 +1385,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> bp->rx_buffer_size, DMA_FROM_DEVICE);
>
> skb->protocol = eth_type_trans(skb, bp->dev);
> +
> + /* Validate MAC fcs if RX checsum offload disabled */
> + if (!(bp->dev->features & NETIF_F_RXCSUM)) {
> + if (macb_validate_hw_csum(skb)) {
> + netdev_err(bp->dev, "incorrect FCS\n");
> + bp->dev->stats.rx_dropped++;
> + break;
> + }
> + }
> +
> skb_checksum_none_assert(skb);
> if (bp->dev->features & NETIF_F_RXCSUM &&
> !(bp->dev->flags & IFF_PROMISC) &&
> @@ -1472,6 +1492,19 @@ static int macb_rx_frame(struct macb_queue *queue, struct napi_struct *napi,
> break;
> }
>
> + /* Validate MAC fcs if RX checsum offload disabled */
> + if (!(bp->dev->features & NETIF_F_RXCSUM)) {
> + if (macb_validate_hw_csum(skb)) {
> + netdev_err(bp->dev, "incorrect FCS\n");
> + bp->dev->stats.rx_dropped++;
> +
> + /* Make descriptor updates visible to hardware */
> + wmb();
Or you can move your if block after the already existing wmb();
> +
> + return 1;
> + }
> + }
> +
> /* Make descriptor updates visible to hardware */
> wmb();
here
>
> @@ -2567,6 +2600,10 @@ static void macb_reset_hw(struct macb *bp)
> macb_writel(bp, TSR, -1);
> macb_writel(bp, RSR, -1);
>
> + /* Disable RX partial store and forward and reset watermark value */
> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
> + gem_writel(bp, PBUFRXCUT, 0xFFF);
Again, the 0xFFF here is platform dependent.
> +
> /* Disable all interrupts */
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> queue_writel(queue, IDR, -1);
> @@ -2700,7 +2737,11 @@ static void macb_init_hw(struct macb *bp)
>
> config = macb_mdc_clk_div(bp);
> config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
> - config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
> +
> + /* Do not discard Rx FCS if RX checsum offload disabled */
> + if (bp->dev->features & NETIF_F_RXCSUM)
> + config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
> +
> if (bp->caps & MACB_CAPS_JUMBO)
> config |= MACB_BIT(JFRAME); /* Enable jumbo frames */
> else
> @@ -2720,6 +2761,15 @@ static void macb_init_hw(struct macb *bp)
> bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
>
> macb_configure_dma(bp);
> +
> + /* Enable RX partial store and forward and set watermark */
> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> + gem_writel(bp, PBUFRXCUT,
> + (gem_readl(bp, PBUFRXCUT) &
> + GEM_BF(WTRMRK, bp->rx_watermark)) |
> + GEM_BIT(ENCUTTHRU));
> + }
> +
> }
>
> /* The hash address register is 64 bits long and takes up two
> @@ -3812,10 +3862,29 @@ static void macb_configure_caps(struct macb *bp,
> const struct macb_config *dt_conf)
> {
> u32 dcfg;
> + int retval;
>
> if (dt_conf)
> bp->caps = dt_conf->caps;
>
> + /* By default we set to partial store and forward mode for zynqmp.
> + * Disable if not set in devicetree.
> + */
> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> + retval = of_property_read_u16(bp->pdev->dev.of_node,
> + "rx-watermark",
You should also check DCFG2 to see if this feature is supported by
different implementations and set MACB_CAPS_PARTIAL_STORE_FORWARD based on
this not on platform specific data structure (struct macb_config::caps).
Also, I notice this from GEM_GXL datasheet "Note that
this option is only available when the device is configured for full
duplex operation and when not using multi buffer
frames".
> + &bp->rx_watermark);
> +
> + /* Disable partial store and forward in case of error or
> + * invalid watermark value
> + */
> + if (retval || bp->rx_watermark > 0xFFF) {
> + dev_info(&bp->pdev->dev,
> + "Not enabling partial store and forward\n");
> + bp->caps &= ~MACB_CAPS_PARTIAL_STORE_FORWARD;
> + }
> + }
> +
> if (hw_is_gem(bp->regs, bp->native_io)) {
> bp->caps |= MACB_CAPS_MACB_IS_GEM;
>
> @@ -4072,6 +4141,8 @@ static int macb_init(struct platform_device *pdev)
> /* Checksum offload is only available on gem with packet buffer */
> if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
> dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
> + dev->hw_features &= ~NETIF_F_RXCSUM;
> if (bp->caps & MACB_CAPS_SG_DISABLED)
> dev->hw_features &= ~NETIF_F_SG;
> dev->features = dev->hw_features;
> @@ -4763,7 +4834,8 @@ static const struct macb_config np4_config = {
> static const struct macb_config zynqmp_config = {
> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
> MACB_CAPS_JUMBO |
> - MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
> + MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> + MACB_CAPS_PARTIAL_STORE_FORWARD,
> .dma_burst_length = 16,
> .clk_init = macb_clk_init,
> .init = init_reset_optional,
> @@ -4811,7 +4883,8 @@ static const struct macb_config sama7g5_emac_config = {
>
> static const struct macb_config versal_config = {
> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> - MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
> + MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> + MACB_CAPS_NEED_TSUCLK | MACB_CAPS_PARTIAL_STORE_FORWARD,
> .dma_burst_length = 16,
> .clk_init = macb_clk_init,
> .init = init_reset_optional,
> --
> 2.36.1
>
On 14.12.2022 12:09, Claudiu Beznea - M18063 wrote:
> On 13.12.2022 14:12, Pranavi Somisetty wrote:
>> [Some people who received this message don't often get email from pranavi.somisetty@amd.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
>>
>> - Validate FCS in receive interrupt handler if Rx checksum offloading
>> is disabled
>> - Get rx-watermark value from DT
>>
>> Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
>> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
>> ---
>> drivers/net/ethernet/cadence/macb.h | 10 +++
>> drivers/net/ethernet/cadence/macb_main.c | 79 +++++++++++++++++++++++-
>> 2 files changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 9c410f93a103..e4eebe8c8c46 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -82,6 +82,7 @@
>> #define GEM_NCFGR 0x0004 /* Network Config */
>> #define GEM_USRIO 0x000c /* User IO */
>> #define GEM_DMACFG 0x0010 /* DMA Configuration */
>> +#define GEM_PBUFRXCUT 0x0044 /* RX Partial Store and Forward */
>> #define GEM_JML 0x0048 /* Jumbo Max Length */
>> #define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */
>> #define GEM_HRB 0x0080 /* Hash Bottom */
>> @@ -342,6 +343,11 @@
>> #define GEM_ADDR64_OFFSET 30 /* Address bus width - 64b or 32b */
>> #define GEM_ADDR64_SIZE 1
>>
>> +/* Bitfields in PBUFRXCUT */
>> +#define GEM_WTRMRK_OFFSET 0 /* Watermark value offset */
>> +#define GEM_WTRMRK_SIZE 12
>
> From documentation I see watermark size depends on implementation. E.g. on
> SAMA7G5 it is on 10 bits (0-9). And each platform could retrieve this value
> from designcfg_debug2 register (offset 0x284 bits 22-25). Can you take that
> into account? It should be less platform dependent this way.
>
>> +#define GEM_ENCUTTHRU_OFFSET 31 /* Enable RX partial store and forward */
>> +#define GEM_ENCUTTHRU_SIZE 1
>>
>> /* Bitfields in NSR */
>> #define MACB_NSR_LINK_OFFSET 0 /* pcs_link_state */
>> @@ -720,6 +726,7 @@
>> #define MACB_CAPS_NEED_TSUCLK 0x00000400
>> #define MACB_CAPS_PCS 0x01000000
>> #define MACB_CAPS_HIGH_SPEED 0x02000000
>> +#define MACB_CAPS_PARTIAL_STORE_FORWARD 0x00000800
Also, try to keep this sorted with previous caps (e.g. add it after
MACB_CAPS_NEED_TSUCLK)
>> #define MACB_CAPS_CLK_HW_CHG 0x04000000
>> #define MACB_CAPS_MACB_IS_EMAC 0x08000000
>> #define MACB_CAPS_FIFO_MODE 0x10000000
>> @@ -1296,6 +1303,9 @@ struct macb {
>>
>> u32 wol;
>>
>> + /* holds value of rx watermark value for pbuf_rxcutthru register */
>> + u16 rx_watermark;
>> +
>
> As its value could be retrived from bits 22-25 at offset 0x284 u8 should be
> enough here. Also, maybe move it at the end of structure to avoid any padding.
>
>> struct macb_ptp_info *ptp_info; /* macb-ptp interface */
>>
>> struct phy *sgmii_phy; /* for ZynqMP SGMII mode */
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 95667b979fab..1f09fe1eec76 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -39,6 +39,7 @@
>> #include <linux/ptp_classify.h>
>> #include <linux/reset.h>
>> #include <linux/firmware/xlnx-zynqmp.h>
>> +#include <linux/crc32.h>
>> #include "macb.h"
>>
>> /* This structure is only used for MACB on SiFive FU540 devices */
>> @@ -1314,6 +1315,15 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
>> */
>> }
>>
>> +static int macb_validate_hw_csum(struct sk_buff *skb)
>> +{
>> + u32 pkt_csum = *((u32 *)&skb->data[skb->len - ETH_FCS_LEN]);
>> + u32 csum = ~crc32_le(~0, skb_mac_header(skb),
>> + skb->len + ETH_HLEN - ETH_FCS_LEN);
>> +
>> + return (pkt_csum != csum);
>> +}
>> +
>> static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>> int budget)
>> {
>> @@ -1375,6 +1385,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>> bp->rx_buffer_size, DMA_FROM_DEVICE);
>>
>> skb->protocol = eth_type_trans(skb, bp->dev);
>> +
>> + /* Validate MAC fcs if RX checsum offload disabled */
>> + if (!(bp->dev->features & NETIF_F_RXCSUM)) {
>> + if (macb_validate_hw_csum(skb)) {
>> + netdev_err(bp->dev, "incorrect FCS\n");
>> + bp->dev->stats.rx_dropped++;
>> + break;
>> + }
>> + }
>> +
>> skb_checksum_none_assert(skb);
>> if (bp->dev->features & NETIF_F_RXCSUM &&
>> !(bp->dev->flags & IFF_PROMISC) &&
>> @@ -1472,6 +1492,19 @@ static int macb_rx_frame(struct macb_queue *queue, struct napi_struct *napi,
>> break;
>> }
>>
>> + /* Validate MAC fcs if RX checsum offload disabled */
>> + if (!(bp->dev->features & NETIF_F_RXCSUM)) {
>> + if (macb_validate_hw_csum(skb)) {
>> + netdev_err(bp->dev, "incorrect FCS\n");
>> + bp->dev->stats.rx_dropped++;
>> +
>> + /* Make descriptor updates visible to hardware */
>> + wmb();
>
> Or you can move your if block after the already existing wmb();
>> +
>> + return 1;
>> + }
>> + }
>> +
>> /* Make descriptor updates visible to hardware */
>> wmb();
>
> here
>
>>
>> @@ -2567,6 +2600,10 @@ static void macb_reset_hw(struct macb *bp)
>> macb_writel(bp, TSR, -1);
>> macb_writel(bp, RSR, -1);
>>
>> + /* Disable RX partial store and forward and reset watermark value */
>> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
>> + gem_writel(bp, PBUFRXCUT, 0xFFF);
>
> Again, the 0xFFF here is platform dependent.
>
>> +
>> /* Disable all interrupts */
>> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> queue_writel(queue, IDR, -1);
>> @@ -2700,7 +2737,11 @@ static void macb_init_hw(struct macb *bp)
>>
>> config = macb_mdc_clk_div(bp);
>> config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
>> - config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
>> +
>> + /* Do not discard Rx FCS if RX checsum offload disabled */
>> + if (bp->dev->features & NETIF_F_RXCSUM)
>> + config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
>> +
>> if (bp->caps & MACB_CAPS_JUMBO)
>> config |= MACB_BIT(JFRAME); /* Enable jumbo frames */
>> else
>> @@ -2720,6 +2761,15 @@ static void macb_init_hw(struct macb *bp)
>> bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
>>
>> macb_configure_dma(bp);
>> +
>> + /* Enable RX partial store and forward and set watermark */
>> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
>> + gem_writel(bp, PBUFRXCUT,
>> + (gem_readl(bp, PBUFRXCUT) &
>> + GEM_BF(WTRMRK, bp->rx_watermark)) |
>> + GEM_BIT(ENCUTTHRU));
>> + }
>> +
>> }
>>
>> /* The hash address register is 64 bits long and takes up two
>> @@ -3812,10 +3862,29 @@ static void macb_configure_caps(struct macb *bp,
>> const struct macb_config *dt_conf)
>> {
>> u32 dcfg;
>> + int retval;
>>
>> if (dt_conf)
>> bp->caps = dt_conf->caps;
>>
>> + /* By default we set to partial store and forward mode for zynqmp.
>> + * Disable if not set in devicetree.
>> + */
>> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
>> + retval = of_property_read_u16(bp->pdev->dev.of_node,
>> + "rx-watermark",
>
> You should also check DCFG2 to see if this feature is supported by
> different implementations and set MACB_CAPS_PARTIAL_STORE_FORWARD based on
> this not on platform specific data structure (struct macb_config::caps).
>
> Also, I notice this from GEM_GXL datasheet "Note that
> this option is only available when the device is configured for full
> duplex operation and when not using multi buffer
> frames".
>
>> + &bp->rx_watermark);
>> +
>> + /* Disable partial store and forward in case of error or
>> + * invalid watermark value
>> + */
>> + if (retval || bp->rx_watermark > 0xFFF) {
>> + dev_info(&bp->pdev->dev,
>> + "Not enabling partial store and forward\n");
>> + bp->caps &= ~MACB_CAPS_PARTIAL_STORE_FORWARD;
>> + }
>> + }
>> +
>> if (hw_is_gem(bp->regs, bp->native_io)) {
>> bp->caps |= MACB_CAPS_MACB_IS_GEM;
>>
>> @@ -4072,6 +4141,8 @@ static int macb_init(struct platform_device *pdev)
>> /* Checksum offload is only available on gem with packet buffer */
>> if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
>> dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD)
>> + dev->hw_features &= ~NETIF_F_RXCSUM;
>> if (bp->caps & MACB_CAPS_SG_DISABLED)
>> dev->hw_features &= ~NETIF_F_SG;
>> dev->features = dev->hw_features;
>> @@ -4763,7 +4834,8 @@ static const struct macb_config np4_config = {
>> static const struct macb_config zynqmp_config = {
>> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>> MACB_CAPS_JUMBO |
>> - MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>> + MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
>> + MACB_CAPS_PARTIAL_STORE_FORWARD,
>> .dma_burst_length = 16,
>> .clk_init = macb_clk_init,
>> .init = init_reset_optional,
>> @@ -4811,7 +4883,8 @@ static const struct macb_config sama7g5_emac_config = {
>>
>> static const struct macb_config versal_config = {
>> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>> - MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
>> + MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
>> + MACB_CAPS_NEED_TSUCLK | MACB_CAPS_PARTIAL_STORE_FORWARD,
>> .dma_burst_length = 16,
>> .clk_init = macb_clk_init,
>> .init = init_reset_optional,
>> --
>> 2.36.1
>>
>
On Tue, 13 Dec 2022 05:12:45 -0700 Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
>
> - Validate FCS in receive interrupt handler if Rx checksum offloading
> is disabled
> - Get rx-watermark value from DT
Sounds like two separate changes, please split into two patches
> @@ -1375,6 +1385,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> bp->rx_buffer_size, DMA_FROM_DEVICE);
>
> skb->protocol = eth_type_trans(skb, bp->dev);
> +
> + /* Validate MAC fcs if RX checsum offload disabled */
> + if (!(bp->dev->features & NETIF_F_RXCSUM)) {
RXCSUM is for L4 (TCP/UDP) checksums, FCS is simply assumed
to be validated by HW.
> + if (macb_validate_hw_csum(skb)) {
> + netdev_err(bp->dev, "incorrect FCS\n");
This can flood logs, and is likely unnecessary since we have a
dedicated statistics for crc errors (rx_crc_errors).
> + bp->dev->stats.rx_dropped++;
CRC errors are errors not drops see the comment above struct
rtnl_link_stats64 for more info.
> + break;
> + }
> + }
> @@ -3812,10 +3862,29 @@ static void macb_configure_caps(struct macb *bp,
> const struct macb_config *dt_conf)
> {
> u32 dcfg;
> + int retval;
>
> if (dt_conf)
> bp->caps = dt_conf->caps;
>
> + /* By default we set to partial store and forward mode for zynqmp.
> + * Disable if not set in devicetree.
> + */
> + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> + retval = of_property_read_u16(bp->pdev->dev.of_node,
> + "rx-watermark",
> + &bp->rx_watermark);
is this property documented in the bindings?
© 2016 - 2025 Red Hat, Inc.