Hi,
> -----Original Message-----
> From: Richard Cochran <richardcochran@xxxxxxxxx>
> Sent: Thursday, May 16, 2019 10:33 PM
> To: Y.b. Lu <yangbo.lu@xxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; David Miller <davem@xxxxxxxxxxxxx>; Claudiu
> Manoil <claudiu.manoil@xxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Rob
> Herring <robh+dt@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH 1/3] enetc: add hardware timestamping support
>
>
> On Thu, May 16, 2019 at 09:59:08AM +0000, Y.b. Lu wrote:
>
> > +config FSL_ENETC_HW_TIMESTAMPING
> > + bool "ENETC hardware timestamping support"
> > + depends on FSL_ENETC || FSL_ENETC_VF
> > + help
> > + Enable hardware timestamping support on the Ethernet packets
> > + using the SO_TIMESTAMPING API. Because the RX BD ring dynamic
> > + allocation hasn't been supported and it's too expensive to use
>
> s/it's/it is/
[Y.b. Lu] Will modify it. BTW, may I know what's the purpose of dropping single quote character? For searching, script checking, or something else?
If require to not use single quote character, I will also modify some other places in Kconfig messages.
>
> > + extended RX BDs if timestamping isn't used, the option was used
> > + to control hardware timestamping/extended RX BDs to be enabled
> > + or not.
>
> ..., this option enables extended RX BDs in order to support hardware
> timestamping.
[Y.b. Lu] Will rephrase it.
>
> > static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int
> > napi_budget) {
> > struct net_device *ndev = tx_ring->ndev;
> > + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > int tx_frm_cnt = 0, tx_byte_cnt = 0;
> > struct enetc_tx_swbd *tx_swbd;
> > + union enetc_tx_bd *txbd;
> > + bool do_tstamp;
> > int i, bds_to_clean;
> > + u64 tstamp = 0;
>
> Please keep in reverse Christmas tree order as much as possible:
>
> union enetc_tx_bd *txbd;
> int i, bds_to_clean;
> bool do_tstamp;
> u64 tstamp = 0;
>
> > i = tx_ring->next_to_clean;
> > tx_swbd = &tx_ring->tx_swbd[i];
> > bds_to_clean = enetc_bd_ready_count(tx_ring, i);
> >
> > + do_tstamp = false;
> > +
> > while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
> > bool is_eof = !!tx_swbd->skb;
> >
> > + if (unlikely(tx_swbd->check_wb)) {
> > + txbd = ENETC_TXBD(*tx_ring, i);
> > +
> > + if (!(txbd->flags & ENETC_TXBD_FLAGS_W))
> > + goto no_wb;
> > +
> > + if (tx_swbd->do_tstamp) {
> > + enetc_get_tx_tstamp(&priv->si->hw, txbd,
> > + &tstamp);
> > + do_tstamp = true;
> > + }
> > + }
> > +no_wb:
>
> This goto seems strange and unnecessary. How about this instead?
>
> if (txbd->flags & ENETC_TXBD_FLAGS_W &&
> tx_swbd->do_tstamp) {
> enetc_get_tx_tstamp(&priv->si->hw, txbd,
> &tstamp);
> do_tstamp = true;
> }
>
> > enetc_unmap_tx_buff(tx_ring, tx_swbd);
> > if (is_eof) {
> > + if (unlikely(do_tstamp)) {
> > + enetc_tstamp_tx(tx_swbd->skb, tstamp);
> > + do_tstamp = false;
> > + }
> > napi_consume_skb(tx_swbd->skb, napi_budget);
> > tx_swbd->skb = NULL;
> > }
> > @@ -167,6 +169,11 @@ struct enetc_cls_rule {
> >
> > #define ENETC_MAX_BDR_INT 2 /* fixed to max # of available cpus */
> >
> > +enum enetc_hw_features {
>
> This is a poor choice of name. It sounds like it describes HW capabilities, but
> you use it to track whether a feature is requested at run time.
>
> > + ENETC_F_RX_TSTAMP = BIT(0),
> > + ENETC_F_TX_TSTAMP = BIT(1),
> > +};
> > +
> > struct enetc_ndev_priv {
> > struct net_device *ndev;
> > struct device *dev; /* dma-mapping device */ @@ -178,6 +185,7
> @@
> > struct enetc_ndev_priv {
> > u16 rx_bd_count, tx_bd_count;
> >
> > u16 msg_enable;
> > + int hw_features;
>
> This is also poorly named. How about "tstamp_request" instead?
>
> >
> > struct enetc_bdr *tx_ring[16];
> > struct enetc_bdr *rx_ring[16];
>
> Thanks,
> Richard
