Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c03077f4-93de-d1c4-0f5d-19292553e6c8@microchip.com>
Date:   Thu, 15 Jun 2023 10:46:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH net-next v4 2/2] net: macb: Add support for partial store
 and forward
Content-Language: en-US
To:     Pranavi Somisetty <pranavi.somisetty@amd.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <claudiu.beznea@microchip.com>
CC:     <git@amd.com>, <michal.simek@amd.com>, <harini.katakam@amd.com>,
        <radhey.shyam.pandey@amd.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>
References: <20230613054340.12837-1-pranavi.somisetty@amd.com>
 <20230613054340.12837-3-pranavi.somisetty@amd.com>
From:   Nicolas Ferre <nicolas.ferre@microchip.com>
Organization: microchip
In-Reply-To: <20230613054340.12837-3-pranavi.somisetty@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 13/06/2023 at 07:43, Pranavi Somisetty wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
> 
> 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: Pranavi Somisetty <pranavi.somisetty@amd.com>

Looks good to me:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks for your patch and effort to address comments Pranavi.

Best regards,
   Nicolas

> ---
> 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.
> 
> Changes v3:
> 1. Followed reverse christmas tree pattern in declaring variables.
> 2. Return -EINVAL when an invalid watermark value is set.
> 3. Removed netdev_info when partial store and forward is not enabled.
> 4. Validating the rx-watermark value in probe itself and only write to the register
> in init.
> 5. Writing a reset value to the pbuf_cuthru register before disabing partial store
> and forward is redundant. So removing it.
> 6. Removed the platform caps flag.
> 7. Instead of reading rx-watermark from DT in macb_configure_caps,
> reading it in probe.
> 8. Changed Signed-Off-By and author names on this patch.
> 
> Changes v4:
> 1. Removed redundant code and unused variables.
> 2. When the rx-watermark value is invalid, instead of returning EINVAL,
> do not enable partial store and forward.
> 3. Change rx-watermark variable's size to u32 instead of u16.
> ---
>   drivers/net/ethernet/cadence/macb.h      | 12 +++++++++++
>   drivers/net/ethernet/cadence/macb_main.c | 27 ++++++++++++++++++++++++
>   2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 14dfec4db8f9..39d53117a8ce 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,10 @@
>   #define GEM_ADDR64_SIZE                1
> 
> 
> +/* Bitfields in PBUFRXCUT */
> +#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 +514,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 +524,8 @@
>   /* Bitfields in DCFG6. */
>   #define GEM_PBUF_LSO_OFFSET                    27
>   #define GEM_PBUF_LSO_SIZE                      1
> +#define GEM_PBUF_CUTTHRU_OFFSET                        25
> +#define GEM_PBUF_CUTTHRU_SIZE                  1
>   #define GEM_DAW64_OFFSET                       23
>   #define GEM_DAW64_SIZE                         1
> 
> @@ -1283,6 +1292,9 @@ struct macb {
> 
>          u32                     wol;
> 
> +       /* holds value of rx watermark value for pbuf_rxcutthru register */
> +       u32                     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..7d023b92b169 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2617,6 +2617,9 @@ 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 */
> +       gem_writel(bp, PBUFRXCUT, 0);
> +
>          /* Disable all interrupts */
>          for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>                  queue_writel(queue, IDR, -1);
> @@ -2770,6 +2773,10 @@ 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->rx_watermark)
> +               gem_writel(bp, PBUFRXCUT, (bp->rx_watermark | GEM_BIT(ENCUTTHRU)));
>   }
> 
>   /* The hash address register is 64 bits long and takes up two
> @@ -4923,6 +4930,7 @@ static int macb_probe(struct platform_device *pdev)
>          phy_interface_t interface;
>          struct net_device *dev;
>          struct resource *regs;
> +       u32 wtrmrk_rst_val;
>          void __iomem *mem;
>          struct macb *bp;
>          int err, val;
> @@ -4995,6 +5003,25 @@ static int macb_probe(struct platform_device *pdev)
> 
>          bp->usrio = macb_config->usrio;
> 
> +       /* 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))) {
> +               err = of_property_read_u32(bp->pdev->dev.of_node,
> +                                          "cdns,rx-watermark",
> +                                          &bp->rx_watermark);
> +
> +               if (!err) {
> +                       /* 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 (bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +                               dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +                               bp->rx_watermark = 0;
> +                       }
> +               }
> +       }
>          spin_lock_init(&bp->lock);
> 
>          /* setup capabilities */
> --
> 2.36.1
> 

-- 
Nicolas Ferre