From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 5A3E15A34 for ; Wed, 11 Nov 2015 11:45:18 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 11 Nov 2015 02:45:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,275,1444719600"; d="scan'208";a="682871775" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga003.jf.intel.com with ESMTP; 11 Nov 2015 02:45:16 -0800 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.13]) by IRSMSX152.ger.corp.intel.com ([169.254.6.143]) with mapi id 14.03.0248.002; Wed, 11 Nov 2015 10:45:15 +0000 From: "Mcnamara, John" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and functions Thread-Index: AQHRF9POx65dtt0zWkW2zONaan2V+p6VJWGAgAGES7A= Date: Wed, 11 Nov 2015 10:45:14 +0000 Message-ID: References: <1443799208-9408-1-git-send-email-danielx.t.mrzyglod@intel.com> <1446732366-10044-1-git-send-email-danielx.t.mrzyglod@intel.com> <1446732366-10044-3-git-send-email-danielx.t.mrzyglod@intel.com> <3105785.kBVynf3MPa@xps13> In-Reply-To: <3105785.kBVynf3MPa@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Nov 2015 10:45:18 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon > Sent: Tuesday, November 10, 2015 11:26 AM > To: Mrzyglod, DanielX T > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and > functions >=20 > 2015-11-05 15:06, Daniel Mrzyglod: > > This patch add common functions and structures used for PTP processing. > > > > Signed-off-by: Daniel Mrzyglod > > --- > > lib/librte_net/Makefile | 2 +- > > lib/librte_net/rte_ptp.h | 105 > > +++++++++++++++++++++++++++++++++++++++++++++++ >=20 > The library librte_net gather some structures and functions for network > headers/layers parsing. > These PTP functions looks really generic. Why not add them in EAL? > Maybe they could benefit from specific implementations in the arch/ > directory. Hi Thomas, How about the following location and new header file? lib/librte_eal/common/include/rte_time.h =20 > > +/* > > + * Structure for cyclecounter IEEE1588 functionality. > > + */ > > +struct cyclecounter { > > + uint64_t (*read)(struct rte_eth_dev *dev); >=20 > This field is not used. > Please avoid using a reference to ethdev here. Ok. We'll try rework this to be more generic. =20 > > + uint64_t mask; > > + uint32_t shift; > > +}; > > + > > +/* > > + * Structure to hold and calculate Unix epoch time. > > + */ > > +struct timecounter { > > + struct cyclecounter *cc; > > + uint64_t cycle_last; > > + uint64_t nsec; > > + uint64_t mask; > > + uint64_t frac; > > +}; >=20 > This structure is not used. >=20 > It is not clear what these structures are for, and what means each field. > When adding a new field in an API, it must be commented in Doxygen. The structure is used. We will add Doxygen comments. =20 > > +static inline uint64_t > > +timespec_to_ns(const struct timespec *ts) > [...] > > +static inline struct timespec > > +ns_to_timespec(uint64_t nsec) > [...] > > +static inline uint64_t > > +cyclecounter_cycles_to_ns(const struct cyclecounter *cc, > > + uint64_t cycles, uint64_t mask, uint64_t *frac) > [...] > > +static inline uint64_t > > +cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc, > > + uint64_t cycles, uint64_t frac) >=20 > They must be prefixed with rte_ with full doxygen comments. We can do that. But is the intention that these function are public? We rea= lly only need them internally. It is possible that they could be used somew= here else but probably not likely. How about if we document them but mark t= hem as @internal in Doxygen. Then if they are require by some other libs th= ey can be made external at a later stage. John