Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock

[Pavan Chebbi]

On Wed, Nov 30, 2022 at 9:52 AM <Arun.Ramadoss@xxxxxxxxxxxxx> wrote:
>
> Hi Pavan,
> Thanks for the review comment.
>
> On Tue, 2022-11-29 at 14:13 +0530, Pavan Chebbi wrote:
> > On Mon, Nov 28, 2022 at 4:04 PM Arun Ramadoss
> > <arun.ramadoss@xxxxxxxxxxxxx> wrote:
> > > +/*  Function is pointer to the do_aux_work in the ptp_clock
> > > capability */
> > > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > > +{
> > > +       struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > > +       struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > > +       struct timespec64 ts;
> > > +
> > > +       mutex_lock(&ptp_data->lock);
> > > +       _ksz_ptp_gettime(dev, &ts);
> > > +       mutex_unlock(&ptp_data->lock);
> > > +
> > > +       spin_lock_bh(&ptp_data->clock_lock);
> > > +       ptp_data->clock_time = ts;
> > > +       spin_unlock_bh(&ptp_data->clock_lock);
> >
> > If I understand this correctly, the software clock is updated with
> > full 64b every 1s. However only 32b timestamp registers are read
> > while
> > processing packets and higher bits from this clock are used.
> > How do you ensure these higher order bits are in sync with the higher
> > order bits in the HW? IOW, what if lower 32b have wrapped around and
> > you are required to stamp a packet but you still don't have aux
> > worker
> > updated.
>
> The Ptp Hardware Clock (PHC) seconds register is 32 bit wide. To have
> register overflow it takes 4,294,967,296 seconds which is approximately
> around 136 Years. So, it is bigger value and assume that we don't need
> to care of PHC second register overflow.
> For the packet timestamping, value is read from 32 bit register. This
> register is splited into 2 bits seconds + 30 bits nanoseconds register.
> In the ksz_tstamp_reconstruct function, lower 2 bits in the ptp_data-
> >clock_time is cleared and 2 bits from the timestamp register are
> added.
>
>  spin_lock_bh(&ptp_data->clock_lock);
>  ptp_clock_time = ptp_data->clock_time;
>  spin_unlock_bh(&ptp_data->clock_lock);
>
> /* calculate full time from partial time stamp */
>  ts.tv_sec = (ptp_clock_time.tv_sec & ~3) | ts.tv_sec;
>
OK thanks. Looks like nano sec and seconds are not being stitched, if
that had happened the rollover would be very frequent. But stitching
the seconds with higher bits still has this problem.
But it should be OK since the window is so large.

> >
> > > +
> > > +       return HZ;  /* reschedule in 1 second */
> > > +}
> > > +
> > >  static int ksz_ptp_start_clock(struct ksz_device *dev)
> > >  {
> > > -       return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > > PTP_CLK_ENABLE);
> > > +       struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> > > +       int ret;
> > > +
> > > +       ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > > PTP_CLK_ENABLE);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       spin_lock_bh(&ptp_data->clock_lock);
> > > +       ptp_data->clock_time.tv_sec = 0;
> > > +       ptp_data->clock_time.tv_nsec = 0;
> > > +       spin_unlock_bh(&ptp_data->clock_lock);
> > > +
> > > +       return 0;
> > >  }
> > >
> > >
> > > --
> > > 2.36.1
> > >

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature