Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

From: Claudiu.Beznea
Date: Fri May 12 2023 - 03:58:24 EST

Next message: Claudiu.Beznea: "Re: [PATCH v3 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml"
Previous message: Linus Walleij: "Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()"
In reply to: Andrew Lunn: "Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward"
Next in thread: Somisetty, Pranavi: "RE: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> Signed-off-by: Harini Katakam <harini.katakam@xxxxxxxxxx>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxxxxx>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@xxxxxxx>
> ---
> 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
>

Next message: Claudiu.Beznea: "Re: [PATCH v3 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml"
Previous message: Linus Walleij: "Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()"
In reply to: Andrew Lunn: "Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward"
Next in thread: Somisetty, Pranavi: "RE: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]