Re: [net-next PATCH v2] octeontx2-pf: Use PTP HW timestamp counter atomic update feature

[Sai Krishna Gajula]

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Thursday, August 10, 2023 4:20 AM
> To: Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri
> Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula
> <gakula@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>;
> richardcochran@xxxxxxxxx; Naveen Mamindlapalli
> <naveenm@xxxxxxxxxxx>
> Subject: Re: [net-next PATCH v2] octeontx2-pf: Use PTP HW timestamp
> counter atomic update feature
> 
> On Mon, 7 Aug 2023 19:35:35 +0530 Sai Krishna wrote:
> > Some of the newer silicon versions in CN10K series supports a feature
> > where in the current PTP timestamp in HW can be updated atomically
> > without losing any cpu cycles unlike read/modify/write register.
> > This patch uses this feature so that PTP accuracy can be improved
> > while adjusting the master offset in HW. There is no need for SW
> > timecounter when using this feature. So removed references to SW
> > timecounter wherever appropriate.
> 
> > -#include "ptp.h"
> >  #include "mbox.h"
> >  #include "rvu.h"
> > +#include "ptp.h"
> 
> If you reorder the includes - maybe put them in alphabetical order?

There are some structure definitions in rvu.h which are required in ptp.h. So, re-ordering in alphabetical order will give compilation issue. 

> >  static bool cn10k_ptp_errata(struct ptp *ptp)  {
> > -	if (ptp->pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CN10K_A_PTP ||
> > -	    ptp->pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CNF10K_A_PTP)
> > +	if ((is_ptp_dev_cn10ka(ptp) &&
> > +	     ((ptp->pdev->revision & 0x0F) == 0x0 || (ptp->pdev->revision &
> 0x0F) == 0x1)) ||
> > +	    (is_ptp_dev_cnf10ka(ptp) &&
> > +	     ((ptp->pdev->revision & 0x0F) == 0x0 || (ptp->pdev->revision &
> > +0x0F) == 0x1)))
> 
> Please refactor the revision check to avoid these long lines repeating the
> same logic

Sai:  Ack, will submit refactor changes in V3 patch.

> 
> >  		return true;
> > +
> >  	return false;
> >  }
> >
> > -static bool is_ptp_tsfmt_sec_nsec(struct ptp *ptp)
> > +static bool is_tstmp_atomic_update_supported(struct rvu *rvu)
> >  {
> > -	if (ptp->pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CN10K_A_PTP ||
> > -	    ptp->pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CNF10K_A_PTP)
> > -		return true;
> > -	return false;
> > +	struct ptp *ptp = rvu->ptp;
> > +	struct pci_dev *pdev;
> > +
> > +	if (is_rvu_otx2(rvu))
> > +		return false;
> > +
> > +	pdev = ptp->pdev;
> > +
> > +	/* On older silicon variants of CN10K, atomic update feature
> > +	 * is not available.
> > +	 */
> > +	if ((pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_A_PTP
> &&
> > +	     (pdev->revision & 0x0F) == 0x0) ||
> > +	     (pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_A_PTP
> &&
> > +	     (pdev->revision & 0x0F) == 0x1) ||
> > +	     (pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CNF10K_A_PTP &&
> > +	     (pdev->revision & 0x0F) == 0x0) ||
> > +	     (pdev->subsystem_device ==
> PCI_SUBSYS_DEVID_CNF10K_A_PTP &&
> > +	     (pdev->revision & 0x0F) == 0x1))
> 
> why are you not using cn10k_ptp_errata() here?

Ack, will submit the changes in V3 patch.

> 
> > +		return false;
> > +
> > +	return true;
> >  }
> 
> > -static int otx2_ptp_adjtime(struct ptp_clock_info *ptp_info, s64
> > delta)
> > +static int otx2_ptp_tc_adjtime(struct ptp_clock_info *ptp_info, s64
> > +delta)
> >  {
> >  	struct otx2_ptp *ptp = container_of(ptp_info, struct otx2_ptp,
> >  					    ptp_info);
> >  	struct otx2_nic *pfvf = ptp->nic;
> >
> > +	if (!ptp->nic)
> > +		return -ENODEV;
> 
> Is this check related to the rest of the patch?

I will remove this check and submit V3 patch. Will submit this check as a separate patch, if required.

Thanks,
Sai
> 
> >  	mutex_lock(&pfvf->mbox.lock);
> >  	timecounter_adjtime(&ptp->time_counter, delta);
> >  	mutex_unlock(&pfvf->mbox.lock);
> --
> pw-bot: cr