Re: [PATCH net-next v4 2/2] dpll: zl3073x: add PTP clock support

[Ivan Vecera <ivecera@redhat.com>]

Sashiko findings with replies...

 > Do these three depends on lines remove the driver from configurations 
that
 > have CONFIG_PTP_1588_CLOCK=n? [...]
 > Is the hard dependency needed? [...] Would [PTP_1588_CLOCK_OPTIONAL] be a
 > better fit here?

This was explicitly requested by Jakub in his v2 review [1]. The driver's
PTP support is a core feature, not optional functionality.

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

 > div_s64() truncates toward zero [...] does a delta smaller than one synth
 > period end up as a 0-cycle step?
 >
 > For a 100 MHz synth, a 5 ns request gives step_cycles == 0 [...]
 > Would it make sense to route the truncated residual through one of 
the finer
 > mechanisms [...]?

The synthesizer frequency range is 187.5-750 MHz, giving a step
resolution of 1.3-5.3 ns. The maximum truncation loss is therefore
one synthesizer period (~5 ns worst case), which is well below the
noise floor of any PTP servo. The servo will compensate for this
residual in the next correction cycle. Adding a secondary adjustment
path for a sub-5 ns residual would complicate the code for no
practical benefit.

 > When the phase step for the second or a later synth group fails, this 
returns
 > 0 after the warning. Does that leave the outputs of the remaining
 > synthesizers skewed from the ToD?
 >
 > [...] Would propagating the error, or retrying the remaining groups, be
 > preferable to reporting success here?

This is intentional. At this point the seconds have already been
committed via ToD and the first synth group has been stepped with
tod_step=true. Propagating the error would cause the PTP servo to
retry the full delta, applying the seconds component a second time -
a much worse outcome than leaving some outputs with a sub-second skew.
A regmap/bus error at this stage indicates a serious hardware problem
that dev_warn() surfaces appropriately. Retrying would likely hit the
same bus error.

 > Which synth period does the firmware really use for this ToD-only step?
 >
 > The doc block says "the FW uses the lowest-ID synth's period for the
 > conversion", but first_synth_freq is latched from the first synth 
that is both
 > enabled and assigned to this channel [...]

The comment is wrong, the code is right. first_synth_freq is the first
enabled synth assigned to this DPLL channel, not the globally lowest-ID
synth. I will fix the comment.

 > Is it intended that zldpll->lock is held across the multi-second hardware
 > waits that follow?
 >
 > [...] So one clock_adjtime() can park dpll_lock behind zldpll->lock 
for up
 > to seconds, which blocks dpll netlink operations [...]

This is intentional and follows the standard PTP driver pattern. The
lock serializes all DPLL and PTP operations for a given channel. The
timeouts are worst-case values - normal operations complete in
milliseconds (ToD reads ~1 ms, phase step completes within one synth
period). The tod_ready_wait for WR_NEXT_1HZ is the only one that
genuinely waits up to ~500 ms on average, but this only occurs for
multi-second adjtime calls (>= 1 s delta), which are rare one-shot
corrections. Splitting the lock would require re-validating channel
state between operations and introduce race conditions with mode
changes.

 > Should sec_adjusted be set before this tod_ready_wait() error return?
 >
 > [...] ptp_clock_adjtime() passes the error to userspace unchanged, so a
 > servo or tool that retries the same offset applies the whole-seconds
 > component a second time and the clock jumps by twice the seconds part.

The current ordering is correct. If tod_ready_wait() fails, we cannot
be certain that the 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 servo handle the
retry is the right behavior, because setting sec_adjusted = true
before confirmation would mask a genuine hardware failure and silently
report success when the adjustment may not have been applied at all.

 > Both mechanisms are asynchronous here, so can a gettimex64() right after
 > adjtime() observe a ToD that has not been stepped yet?
 >
 > [...] Should the phase step and TIE write get an equivalent wait before
 > success is returned to the PTP core?

In practice this is not a problem. PTP servos wait for the next sync
interval (typically 1-8 s) before reading the clock again. The phase
step completes within one synthesizer period (1.3-5.3 ns) and TIE
write settles within the DPLL loop bandwidth. A completion wait would
add up to 3 seconds of latency to every adjtime call for no practical
benefit - the sub-nanosecond residual from an in-flight phase step is
orders of magnitude below the servo's measurement noise.

 > Can this multiplication overflow s64 for a value the core lets through?
 >
 > [...] Since 125 is invertible modulo 2^64, a tx->freq around 10^17 can be
 > chosen so the wrapped ppb passes the max_adj test.

This is a pre-existing issue in the PTP core's scaled_ppm_to_ppb()
helper, not in this driver. For in-range values (bounded by max_adj =
10^7 ppb, |scaled_ppm| up to ~6.6e8), the product is ~4.4e16 which
is well within s64 range. Exploiting the wrap in scaled_ppm_to_ppb()
requires deliberately constructed tx->freq values around 10^17 and
CAP_SYS_TIME privilege. A driver-side guard would be papering over
a core bug.

 > The kernel-doc just above says "Return: 0 on success, -EOPNOTSUPP if 
NCO pin
 > is not connected", but this returns 0 for a non-NCO channel whenever
 > scaled_ppm == 0. [...]
 >
 > zl3073x_dpll_ptp_adjphase() has the same mismatch [...]

This is intentional - a zero adjustment is a no-op and succeeds
regardless of mode. Will update the kernel-doc to reflect this
behavior.

 > The name is built only from the per-device channel index, so two ZL3073x
 > chips in one system expose PTP clocks with identical ptp_clock_info.name
 > [...] Should dev_name(zldev->dev) be part of the PTP clock name too?

Good catch. Will include dev_name() in the PTP clock name, consistent
with the kworker naming pattern.

Thanks,
Ivan