| Message ID | 20230510205306.136766-2-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp .adjphase cleanups | expand |
On Wed, May 10, 2023 at 01:52:58PM -0700, Rahul Rameshbabu wrote: > +PTP hardware clock requirements for '.adjphase' > +----------------------------------------------- > + > + The 'struct ptp_clock_info' interface has a '.adjphase' function. > + This function has a set of requirements from the PHC in order to be > + implemented. > + > + * The PHC implements a servo algorithm internally that is used to > + correct the offset passed in the '.adjphase' call. > + * When other PTP adjustment functions are called, the PHC servo > + algorithm is disabled, I disagree with this part: > and the frequency prior to the '.adjphase' > + call is restored internally in the PHC. That seems like an arbitrary rule that doesn't make much sense. Thanks, Richard
On Wed, 10 May, 2023 19:09:40 -0700 Richard Cochran <richardcochran@gmail.com> wrote: > On Wed, May 10, 2023 at 01:52:58PM -0700, Rahul Rameshbabu wrote: >> +PTP hardware clock requirements for '.adjphase' >> +----------------------------------------------- >> + >> + The 'struct ptp_clock_info' interface has a '.adjphase' function. >> + This function has a set of requirements from the PHC in order to be >> + implemented. >> + >> + * The PHC implements a servo algorithm internally that is used to >> + correct the offset passed in the '.adjphase' call. >> + * When other PTP adjustment functions are called, the PHC servo >> + algorithm is disabled, > > I disagree with this part: > >> and the frequency prior to the '.adjphase' >> + call is restored internally in the PHC. > > That seems like an arbitrary rule that doesn't make much sense. If the PHC does not restore the frequency, won't the value cached in the ptp stack in the kernel become inaccurate compared to the frequency change induced by the '.adjphase' call? This concern is why I added this clause in the documentation. Let me know if my understanding is off with regards to this. I think we had a similar conversation on this previously in the mailing list. https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/ > > Thanks, > Richard -- Rahul Rameshbabu
On Thu, May 11, 2023 at 01:20:45PM -0700, Rahul Rameshbabu wrote: > If the PHC does not restore the frequency, won't the value cached in the > ptp stack in the kernel become inaccurate compared to the frequency > change induced by the '.adjphase' call? If the HW implements a PI controller, and if it has converged, then the current frequency will be close to the remote time server's. > This concern is why I added this > clause in the documentation. Let me know if my understanding is off with > regards to this. I think we had a similar conversation on this > previously in the mailing list. > > https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/ I guess it depends on the HW algorithm and the situation. But I don't think there is a "rule" that always gets the best result. Thanks, Richard
Hi Richard, I have a v2 prepared. I have just one question left before sending that it out. On Thu, 11 May, 2023 17:51:47 -0700 Richard Cochran <richardcochran@gmail.com> wrote: > On Thu, May 11, 2023 at 01:20:45PM -0700, Rahul Rameshbabu wrote: > >> If the PHC does not restore the frequency, won't the value cached in the >> ptp stack in the kernel become inaccurate compared to the frequency >> change induced by the '.adjphase' call? > > If the HW implements a PI controller, and if it has converged, then > the current frequency will be close to the remote time server's. This point makes sense to me. However, I have a concern about the case where the linuxptp servo has not had a chance to make a single frequency adjustment (0 ppb) and .adjphase/LOCKED_STABLE state is initially called/reached. After converging, the frequency will be close to the remote time's server's frequency, but that frequency will likely not be 0 ppb. If .adjfine had been called previously, the difference between the remote time server's frequency and the cached frequency in the ptp stack would likely be significantly closer. That said, do you think it makes sense to have some kind of API that gives information about the in-HW controller such as the frequency offset it operated? Or maybe in general an API in the future for introspecting the state of this in-HW servo? > >> This concern is why I added this >> clause in the documentation. Let me know if my understanding is off with >> regards to this. I think we had a similar conversation on this >> previously in the mailing list. >> >> https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/ > > I guess it depends on the HW algorithm and the situation. But I don't > think there is a "rule" that always gets the best result. Agreed. I do not think enforcing the PHC to restore the frequency to the value before .adjphase is called would be helpful. This preserves the integrity of the cached value in the kernel stack, but that is not helpful since we can potentially see an initial growing error in the offset between the remote server's time and the PHC's time after making this frequency reversion. > > Thanks, > Richard -- Rahul Rameshbabu
On Mon, May 22, 2023 at 10:03:23AM -0700, Rahul Rameshbabu wrote: > This point makes sense to me. However, I have a concern about the case > where the linuxptp servo has not had a chance to make a single frequency > adjustment (0 ppb) and .adjphase/LOCKED_STABLE state is initially > called/reached. After converging, the frequency will be close to the > remote time's server's frequency, but that frequency will likely not be > 0 ppb. If .adjfine had been called previously, the difference between > the remote time server's frequency and the cached frequency in the ptp > stack would likely be significantly closer. That said, do you think it > makes sense to have some kind of API that gives information about the > in-HW controller such as the frequency offset it operated? Or maybe in > general an API in the future for introspecting the state of this in-HW > servo? The whole point of hardware vendors doing this is to hide their implementation from prying eyes. So they are not going to provide introspection at all. Thanks, Richard
diff --git a/Documentation/driver-api/ptp.rst b/Documentation/driver-api/ptp.rst index 664838ae7776..c6ef41cf6130 100644 --- a/Documentation/driver-api/ptp.rst +++ b/Documentation/driver-api/ptp.rst @@ -73,6 +73,23 @@ Writing clock drivers class driver, since the lock may also be needed by the clock driver's interrupt service routine. +PTP hardware clock requirements for '.adjphase' +----------------------------------------------- + + The 'struct ptp_clock_info' interface has a '.adjphase' function. + This function has a set of requirements from the PHC in order to be + implemented. + + * The PHC implements a servo algorithm internally that is used to + correct the offset passed in the '.adjphase' call. + * When other PTP adjustment functions are called, the PHC servo + algorithm is disabled, and the frequency prior to the '.adjphase' + call is restored internally in the PHC. + + **NOTE:** '.adjphase' is not a simple time adjustment functionality + that 'jumps' the PHC clock time based on the provided offset. It + should correct the offset provided using an internal algorithm. + Supported hardware ================== diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index fdffa6a98d79..f8e8443a8b35 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -77,8 +77,10 @@ struct ptp_system_timestamp { * nominal frequency in parts per million, but with a * 16 bit binary fractional field. * - * @adjphase: Adjusts the phase offset of the hardware clock. - * parameter delta: Desired change in nanoseconds. + * @adjphase: Indicates that the PHC should use an internal servo + * algorithm to correct the provided phase offset. + * parameter delta: PHC servo phase adjustment target + * in nanoseconds. * * @adjtime: Shifts the time of the hardware clock. * parameter delta: Desired change in nanoseconds.
.adjphase expects a PHC to use an internal servo algorithm to correct the provided phase offset target in the callback. Implementation of the internal servo algorithm are defined by the individual devices. Cc: Jakub Kicinski <kuba@kernel.org> Cc: Richard Cochran <richardcochran@gmail.com> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> --- Documentation/driver-api/ptp.rst | 17 +++++++++++++++++ include/linux/ptp_clock_kernel.h | 6 ++++-- 2 files changed, 21 insertions(+), 2 deletions(-)