Messages in this thread Patch in this message | | | From | <> | Subject | Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward | Date | Fri, 12 May 2023 07:57:46 +0000 |
| |
On 11.05.2023 10:12, Pranavi Somisetty wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > When the receive partial store and forward mode is activated, the > receiver will only begin to forward the packet to the external AHB > or AXI slave when enough packet data is stored in the packet buffer. > The amount of packet data required to activate the forwarding process > is programmable via watermark registers which are located at the same > address as the partial store and forward enable bits. Adding support to > read this rx-watermark value from device-tree, to program the watermark > registers and enable partial store and forwarding. > > 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> > --- > Changes v2: > > 1. Removed all the changes related to validating FCS when Rx checksum offload is disabled. > 2. Instead of using a platform dependent number (0xFFF) for the reset value of rx watermark, > derive it from designcfg_debug2 register. > 3. Added a check to see if partial s/f is supported, by reading the > designcfg_debug6 register. > --- > drivers/net/ethernet/cadence/macb.h | 14 +++++++ > drivers/net/ethernet/cadence/macb_main.c | 49 +++++++++++++++++++++++- > 2 files changed, 61 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index 14dfec4db8f9..46833662094d 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 */ > @@ -343,6 +344,11 @@ > #define GEM_ADDR64_SIZE 1 > > > +/* Bitfields in PBUFRXCUT */ > +#define GEM_WTRMRK_OFFSET 0 /* Watermark value offset */ > +#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 */ > #define MACB_NSR_LINK_SIZE 1 > @@ -509,6 +515,8 @@ > #define GEM_TX_PKT_BUFF_OFFSET 21 > #define GEM_TX_PKT_BUFF_SIZE 1 > > +#define GEM_RX_PBUF_ADDR_OFFSET 22 > +#define GEM_RX_PBUF_ADDR_SIZE 4 > > /* Bitfields in DCFG5. */ > #define GEM_TSU_OFFSET 8 > @@ -517,6 +525,8 @@ > /* Bitfields in DCFG6. */ > #define GEM_PBUF_LSO_OFFSET 27 > #define GEM_PBUF_LSO_SIZE 1 > +#define GEM_PBUF_CUTTHRU_OFFSET 26 > +#define GEM_PBUF_CUTTHRU_SIZE 1 > #define GEM_DAW64_OFFSET 23 > #define GEM_DAW64_SIZE 1 > > @@ -718,6 +728,7 @@ > #define MACB_CAPS_NEEDS_RSTONUBR 0x00000100 > #define MACB_CAPS_MIIONRGMII 0x00000200 > #define MACB_CAPS_NEED_TSUCLK 0x00000400 > +#define MACB_CAPS_PARTIAL_STORE_FORWARD 0x00000800 > #define MACB_CAPS_PCS 0x01000000 > #define MACB_CAPS_HIGH_SPEED 0x02000000 > #define MACB_CAPS_CLK_HW_CHG 0x04000000 > @@ -1283,6 +1294,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 41964fd02452..07b9964e7aa3 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2600,6 +2600,7 @@ static void macb_init_rings(struct macb *bp) > static void macb_reset_hw(struct macb *bp) > { > struct macb_queue *queue; > + u16 watermark_reset_value; > unsigned int q; > u32 ctrl = macb_readl(bp, NCR); > > @@ -2617,6 +2618,12 @@ 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) { > + watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
Is this block needed? Maybe all you need here is just to disable the rx partial store and forward?
> + gem_writel(bp, PBUFRXCUT, watermark_reset_value); > + } > + > /* Disable all interrupts */ > for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { > queue_writel(queue, IDR, -1); > @@ -2743,6 +2750,8 @@ static void macb_configure_dma(struct macb *bp) > > static void macb_init_hw(struct macb *bp) > { > + u16 watermark_reset_value; > + u16 watermark_value; > u32 config; > > macb_reset_hw(bp); > @@ -2770,6 +2779,14 @@ 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) && bp->rx_watermark) { > + watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1; > + watermark_value = bp->rx_watermark & watermark_reset_value;
You should validate the value in bp->rx_watermark in probe and here just update the PBUFRXCUT with that value.
> + gem_writel(bp, PBUFRXCUT, > + (watermark_value | GEM_BIT(ENCUTTHRU))); > + } > } > > /* The hash address register is 64 bits long and takes up two > @@ -3861,11 +3878,37 @@ static const struct net_device_ops macb_netdev_ops = { > static void macb_configure_caps(struct macb *bp, > const struct macb_config *dt_conf) > { > + u32 wtrmrk_rst_val; > + int retval; > u32 dcfg; > > 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 (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6))) { > + if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
You can get rid of MACB_CAPS_PARTIAL_STORE_FORWARD and consider it enabled or not based on device tree rx-watermark dt property. Thus you can have here only: if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6)))
and based on the validity of data passed to "rx-watermark" the bp->rx_watermark will be zero or not. You can check bp->rx_watermark all over the code to check if rx partial store and fw is enabled.
> + retval = of_property_read_u16(bp->pdev->dev.of_node, > + "rx-watermark", > + &bp->rx_watermark);
E.g. SAMA7G5 has PBUFRXCUT.watermark on 10 bits. Is it the same on Xynqmp? For compatibility with future implementations and stable DT interface it would be better to just keep rx-watermark DT property on 32 bits.
> + > + /* Disable partial store and forward in case of error or > + * invalid watermark value > + */ > + wtrmrk_rst_val = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1; > + if (retval || bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) { > + if (bp->rx_watermark > wtrmrk_rst_val) { > + dev_info(&bp->pdev->dev, "Invalid watermark value\n"); > + bp->rx_watermark = 0;
Checking this in the code should be enough. There is no need to introduce a new capability.
> + } > + 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; > > @@ -4813,7 +4856,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, > @@ -4861,7 +4905,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 >
| |