Messages in this thread Patch in this message | | | From | <> | Subject | Re: [LINUX RFC PATCH] net: macb: Add support for partial store and forward | Date | Wed, 14 Dec 2022 10:09:11 +0000 |
| |
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 > #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 >
| |