Hi Richard,
> -----Original Message-----
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Monday, June 4, 2018 9:49 PM
> To: Y.b. Lu <yangbo...@nxp.com>
> Cc: net...@vger.kernel.org; Madalin-cristian Bucur
> <madalin.bu...@nxp.com>; Rob Herring <robh...@kernel.org>; Shawn Guo
> <shawn...@kernel.org>; David S . Miller <da...@davemloft.net>;
> devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 09/10] dpaa_eth: add support for hardware timestamping
>
> On Mon, Jun 04, 2018 at 03:08:36PM +0800, Yangbo Lu wrote:
>
> > +if FSL_DPAA_ETH
> > +config FSL_DPAA_ETH_TS
> > + bool "DPAA hardware timestamping support"
> > + select PTP_1588_CLOCK_QORIQ
> > + default n
> > + help
> > + Enable DPAA hardware timestamping support.
> > + This option is useful for applications to get
> > + hardware time stamps on the Ethernet packets
> > + using the SO_TIMESTAMPING API.
> > +endif
>
> You should drop this #ifdef. In general, if a MAC supports time stamping and
> PHC, then the driver support should simply be compiled in.
>
> [ When time stamping incurs a large run time performance penalty to
> non-PTP users, then it might make sense to have a Kconfig option to
> disable it, but that doesn't appear to be the case here. ]
[Y.b. Lu] Actually these timestamping codes affected DPAA networking
performance in our previous performance test.
That's why we used ifdef for it.
>
> > @@ -1615,6 +1635,24 @@ static int dpaa_eth_refill_bpools(struct
> dpaa_priv *priv)
> > skbh = (struct sk_buff **)phys_to_virt(addr);
> > skb = *skbh;
> >
> > +#ifdef CONFIG_FSL_DPAA_ETH_TS
> > + if (priv->tx_tstamp &&
> > + skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
>
> This condition fits on one line easily.
[Y.b. Lu] Right. I will use one line in next version.
>
> > + struct skb_shared_hwtstamps shhwtstamps;
> > + u64 ns;
>
> Local variables belong at the top of the function.
[Y.b. Lu] Ok, will move them to the top in next verison.
>
> > + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> > +
> > + if (!dpaa_get_tstamp_ns(priv->net_dev, &ns,
> > + priv->mac_dev->port[TX],
> > + (void *)skbh)) {
> > + shhwtstamps.hwtstamp = ns_to_ktime(ns);
> > + skb_tstamp_tx(skb, &shhwtstamps);
> > + } else {
> > + dev_warn(dev, "dpaa_get_tstamp_ns failed!\n");
> > + }
> > + }
> > +#endif
> > if (unlikely(qm_fd_get_format(fd) == qm_fd_sg)) {
> > nr_frags = skb_shinfo(skb)->nr_frags;
> > dma_unmap_single(dev, addr, qm_fd_get_offset(fd) + @@ -2086,6
> > +2124,14 @@ static int dpaa_start_xmit(struct sk_buff *skb, struct
> net_device *net_dev)
> > if (unlikely(err < 0))
> > goto skb_to_fd_failed;
> >
> > +#ifdef CONFIG_FSL_DPAA_ETH_TS
> > + if (priv->tx_tstamp &&
> > + skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
>
> One line please.
[Y.b. Lu] No problem.
>
> > + fd.cmd |= FM_FD_CMD_UPD;
> > + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > + }
> > +#endif
> > +
> > if (likely(dpaa_xmit(priv, percpu_stats, queue_mapping, &fd) == 0))
> > return NETDEV_TX_OK;
> >
>
> Thanks,
> Richard
