Re: [LINUX RFC PATCH] net: macb: Add support for partial store and forward

[<Claudiu.Beznea@microchip.com>]

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
>>
>