Re: [PATCH net-next] net: microchip: lan743x: improve throughput with rx timestamp config

[<VishvambarPanth.S@microchip.com>]

On Tue, 2023-09-26 at 16:48 -0700, Jacob Keller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 9/26/2023 8:56 AM, Vishvambar Panth S wrote:
> > Currently all RX frames are timestamped which results in a
> > performance
> > penalty when timestamping is not needed.  The default is now being
> > changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but
> > support has been added to allow changing the desired RX
> > timestamping
> > mode (HWTSTAMP_FILTER_ALL -  which was the previous setting,
> > HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and
> > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using
> > SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl
> > application.
> > It is also noted that ptp4l, when started, preconfigures the device
> > to
> > timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver
> > continues
> > to work properly "out of the box".
> > 
> > Test setup:  x64 PC with LAN7430 ---> x64 PC as partner
> > 
> 
> I don't think I would bother to support
> HWTSTAMP_FILTER_PTP_V2_DELAY_REQ
> or HWTSTAMP_FILTER_PTP_V2_SYNC as these are pretty historic and only
> useful for hardware which can't do HWTSTAMP_FILTER_PTP_V2_EVENT.
> 
Hi Jacob,
Thanks for the comments. Just added these filters, as the HW supports
these individually. Since HW also supports
HWTSTAMP_FILTER_PTP_V2_EVENT, will drop the support for
individual HWTSTAMP_FILTER_PTP_V2_SYNC and
HWTSTAMP_FILTER_PTP_V2_DELAY_REQ cases in v2 patch. 

> > iperf3 with - Timestamp all incoming packets:
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval           Transfer     Bitrate         Retr
> > [  5]   0.00-5.05   sec   517 MBytes   859 Mbits/sec    0    sender
> > [  5]   0.00-5.00   sec   515 MBytes   864 Mbits/sec        
> > receiver
> > 
> > iperf Done.
> > 
> > iperf3 with - Timestamp only PTP packets:
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval           Transfer     Bitrate         Retr
> > [  5]   0.00-5.04   sec   563 MBytes   937 Mbits/sec    0    sender
> > [  5]   0.00-5.00   sec   561 MBytes   941 Mbits/sec        
> > receiver
> > 
> > 
> 
> Pretty significant cost here for the timestamping all frames. Makes
> sense to leave the default to NONE unless requested.

Thanks for the comment, will maintain as is.

> 
> Please find the earlier conversation at the link below
> > Link:
> > https://lore.kernel.org/all/20230731125418.75140-1-vishvambarpanth.s@microchip.com/
> > 
> > Signed-off-by: Vishvambar Panth S <vishvambarpanth.s@microchip.com>
> > ---
> >  .../net/ethernet/microchip/lan743x_ethtool.c  |  5 +-
> >  drivers/net/ethernet/microchip/lan743x_main.c | 58
> > ++++++++++++++++++-
> >  drivers/net/ethernet/microchip/lan743x_main.h |  8 +++
> >  drivers/net/ethernet/microchip/lan743x_ptp.c  |  9 +++
> >  4 files changed, 78 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > index 2db5949b4c7e..855844df5ea1 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > @@ -1047,7 +1047,10 @@ static int
> > lan743x_ethtool_get_ts_info(struct net_device *netdev,
> >                           BIT(HWTSTAMP_TX_ON) |
> >                           BIT(HWTSTAMP_TX_ONESTEP_SYNC);
> >       ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > -                           BIT(HWTSTAMP_FILTER_ALL);
> > +                           BIT(HWTSTAMP_FILTER_ALL) |
> > +                           BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
> > +                           BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
> > +                           BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> >       return 0;
> >  }
> > 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index f940895b14e8..0389bc7cf603 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct
> > lan743x_tx *tx)
> >               return last_head - last_tail - 1;
> >  }
> > 

> > +int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
> > +                            int rx_filter)
> > +{
> > +     int channel_number;
> > +     int index;
> > +     u32 data;
> > +
> > +     switch (rx_filter) {
> > +     case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > +                     data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > +                     data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> > +                     data |= PTP_RX_TS_CFG_SYNC_MSG_;
> > +                     lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > +                     break;
> > +     case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > +                     data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > +                     data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> > +                     data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_;
> > +                     lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > +                     break;
> > +     case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > +                     data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > +                     data |= PTP_RX_TS_CFG_EVENT_MSGS_;
> > +                     lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > +                     break;
> > +     case HWTSTAMP_FILTER_NONE:
> > +     case HWTSTAMP_FILTER_ALL:
> > +                     break;
> 
> At first this break was a bit confusing to me, since nothing is set
> here.

The idea was that in the NONE and ALL case, there need not be any
modification in this register. But will adapt to a more readable
implementation in v2 patch.

> 
> > +     default:
> > +                     netif_warn(adapter, drv, adapter->netdev,
> > +                                "rx timestamp = %d is not
> > supported\n",
> > +                                rx_filter);
> > +                     return -EINVAL;
> > +     }
> > +
> > +     for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
> > +             channel_number = adapter->rx[index].channel_number;
> > +             data = lan743x_csr_read(adapter,
> > RX_CFG_B(channel_number));
> > +             if (rx_filter == HWTSTAMP_FILTER_NONE) {
> > +                     data &= ~(RX_CFG_B_TS_ALL_RX_ |
> > +                               RX_CFG_B_TS_DESCR_EN_);
> > +             } else if (rx_filter == HWTSTAMP_FILTER_ALL) {
> > +                     data |= RX_CFG_B_TS_ALL_RX_;
> > +             } else {
> > +                     /* enable storing timestamping in extension
> > descriptor
> > +                      * instead of timestamping all the packets
> > +                      */
> > +                     data &= ~RX_CFG_B_TS_ALL_RX_;
> > +                     data |= RX_CFG_B_TS_DESCR_EN_;
> > +             }
> 
> I might have made the decision of what to program in the switch case
> above and then done the write here with "data &= ~MASK; data |=
> setting"
> rather than having two separate decision points.

Sure, will try and adapt to this.
> 
> > +             lan743x_csr_write(adapter, RX_CFG_B(channel_number),
> > +                               data);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> 
> Rest of the code seems fine, and the implementation looks ok.
> 
> I'd suggest dropping the not so useful timestamp filters for only
> sync
> or only delay request, keeping only V2_EVENT, but either way:

Thanks, will drop the not so useful filters and submit a v2 patch with
required changes.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>