Re: [PATCH net-next v6 3/3] dpll: zl3073x: add PTP clock support

[Ivan Vecera <ivecera@redhat.com>]

Sashiko findings and my replies:

 > Is the hard dependency on PTP_1588_CLOCK intended here?

Yes. This was explicitly requested by Jakub in the v2 review [1].

[1] https://lore.kernel.org/netdev/20260722135848.2d401ada@kernel.org/

 > Does the requested timestamp need compensation for the 1 Hz latch
 > here? [...] gettimex64() keeps returning the old time until that
 > edge

gettimex64() does not return the old value. It calls
zl3073x_chan_tod_read() which starts with zl3073x_chan_tod_ready_wait().
If a WR_NEXT_1HZ from a previous settime64() is still pending, the
wait blocks until the 1 Hz edge applies it, and the subsequent read
returns the new value. So the written timestamp is applied accurately
at the edge with no drift to compensate for.

 > What state is the hardware left in on this path? [partial phase step
 > failure returns 0]

This is intentional. At this point the seconds and the first synth
group (with ToD) have been committed. Propagating the error would
cause the PTP servo to retry the full delta, applying seconds a
second time — a much worse outcome than a sub-second skew on some
outputs. A regmap/bus error at this stage indicates a serious
hardware problem that dev_warn() surfaces appropriately.

 > zl3073x_dpll_ptp_adjtime() documents "Return: 0 on success, <0 on
 > error", yet it returns 0 with rc != 0 when the seconds were already
 > committed [...]
 > Could these blocks spell out that 0 does not imply the requested
 > adjustment was fully applied?

Good point. Will update the kernel-doc to document the partial
success semantics.

 > Should sec_adjusted be set right after the successful
 > zl3073x_chan_tod_adjust() rather than after the wait?
 >
 > Once zl3073x_chan_tod_adjust() returns 0, the WR_NEXT_1HZ command has
 > been accepted and the firmware will latch the seconds at the next
 > 1 Hz edge; the driver cannot cancel it.

If tod_ready_wait fails, we cannot be certain that WR_NEXT_1HZ was
actually applied — the semaphore timeout means we do not know whether
the hardware committed the seconds or not. Returning the error and
letting the caller handle the retry is the safer choice.

 > Does the template need to be copied here [in alloc], or in
 > zl3073x_dpll_ptp_register() just before ptp_clock_register()?
 >
 > ptp_clock_register() writes into the driver-owned struct
 > ptp_clock_info [...] On the second registration the leftover
 > getcycles64/getcyclesx64 pointers [...]

Good catch. Will move the template assignment from
zl3073x_dpll_alloc() to zl3073x_dpll_ptp_register() so that
ptp_info is always fresh before each registration.

 > Sub-second PTP adjustments can be overwritten by pending hardware ToD
 > writes from previous API calls.

The PTP callbacks are serialized by zldpll->lock. A pending
WR_NEXT_1HZ from a previous settime64 or adjtime call is consumed
at the next 1 Hz edge. Any subsequent ToD operation goes through
tod_ready_wait which waits for the pending write to complete. For
phase step and TIE write: these operate on independent hardware
blocks and do not conflict with a pending ToD write.

Thanks,
Ivan