[AMD Official Use Only - Internal Distribution Only]
Hi Pablo,
Can you please help ?
Thanks and Regards
Selwin Sebastian
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@intel.com>
Sent: Tuesday, June 9, 2020 9:34 PM
To: Sebastian, Selwin <selwin.sebast...@amd.com>; dev@dpdk.org
Cc: Somalapuram, Amaranath <amaranath.somalapu...@amd.com>; Pablo de Lara
<pablo.de.lara.gua...@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for
axgbe
[CAUTION: External Email]
On 6/9/2020 4:42 PM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Ferruh,
> Added recommended modifications and resubmitted the patch. Removed
> offloads handling part and "DEV_TX_OFFLOAD_MULTI_SEGS" Flag also as it is not
> yet supported by driver.
> Commit 0625a29f42c62998318ee3e05b2420e436318678 forces the usage of
> DEV_TX_OFFLOAD_MULTI_SEGS for using ptpclient test application. I had to
> remove this commit for my test. Any inputs on how this can be handled ?
Cc'ed Pablo.
According to the commit log "full Tx path" required for IEEE1588.
Is the DEV_TX_OFFLOAD_MULTI_SEGS requirement for IEEE1588, if so axgbe driver
needs to implement it before claiming the IEEE1588 support.
Or 'DEV_TX_OFFLOAD_MULTI_SEGS' may be used to force the underlying PMD to the
use the scalar data path. Pablo can answer this better.
>
> Regards
> Selwin
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Friday, June 5, 2020 8:34 PM
> To: Sebastian, Selwin <selwin.sebast...@amd.com>; dev@dpdk.org
> Cc: Somalapuram, Amaranath <amaranath.somalapu...@amd.com>
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP
> support for axgbe
>
> [CAUTION: External Email]
>
> On 6/1/2020 1:57 PM, selwin.sebast...@amd.com wrote:
>> From: Selwin Sebastian <selwin.sebast...@amd.com>
>>
>> Add ethdev APIs to support PTP timestamping
>
> For the patch title, "net/axgbe: " already says the change is in the 'axgbe'
> driver, no need to duplicate " .. support for axgbe".
>
> <...>
>
>> +static inline uint64_t
>> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t
>> +*remainder) {
>> + *remainder = dividend % divisor;
>> + return dividend / divisor;
>> +}
>> +
>> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
>> +{
>
> The coding convention [1] we have says return type will be on seperate line,
> as already done in some of these functions. Since this is new code, better to
> start good, can you please apply the coding convention to all fucntions, like:
>
> static inline uint64_t
> div_u64(uint64_t dividend, uint32_t divisor)
>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
> dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html&data=02%7C01%
> 7CSelwin.Sebastian%40amd.com%7Cf68e63a441694a33637c08d80c8ec76f%7C3dd8
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C637273154657245807&sdata=P6
> vU9bOzwKTBSJZFWQXEnnb6Wu%2FCViJ6kM1Cm2faeJU%3D&reserved=0
> (I definitly suggest reading it if you didn't already)
>
> <...>
>
>> @@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
>> uint16_t queue_idx,
>> struct axgbe_tx_queue *txq;
>> unsigned int tsize;
>> const struct rte_memzone *tz;
>> + struct rte_eth_dev_data *dev_data;
>>
>> tx_desc = nb_desc;
>> pdata = dev->data->dev_private; @@ -507,6 +511,7 @@ int
>> axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>> return -ENOMEM;
>> txq->pdata = pdata;
>>
>> + dev_data = pdata->eth_dev->data;
>> txq->nb_desc = tx_desc;
>> txq->free_thresh = tx_conf->tx_free_thresh ?
>> tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH; @@
>> -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
>> uint16_t queue_idx,
>> if (txq->nb_desc % txq->free_thresh != 0)
>> txq->vector_disable = 1;
>>
>> - if (tx_conf->offloads != 0)
>> + if ((tx_conf->offloads != 0) ||
>> + dev_data->dev_conf.txmode.offloads)
>> txq->vector_disable = 1;
>
>
> This change seems unrelated with the rest of the patch, and I far as I
> remember this was in the another patch too. What do you think making this
> seperate patch with the proper description it deserves?
>
