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

[Ivan Vecera <ivecera@redhat.com>]

Replies to Sashiko's findings:

 > Is a hard dependency on PTP_1588_CLOCK required here?
 > ...
 > Would "depends on NET && PTP_1588_CLOCK_OPTIONAL" work instead?

This was changed from OPTIONAL to a hard dependency at Jakub's
explicit request in v2 review [1]. His rationale is that OPTIONAL is
meant for NICs where PTP is truly a side-feature, while this is a
dedicated DPLL/PTP device. Non-PTP chip variants simply skip PTP
clock registration.

[1] https://lore.kernel.org/netdev/20260727125921.2e16bed9@kernel.org/

 > does the kernel-doc match the implementation?
 > The body has a third gating condition ... zl3073x_chan_is_out_stepped()
 >
 > Can two perout channels end up aliasing the same physical output?
 > ...
 > Related: for single-ended N-divided formats, is the P-pin channel
 > usable at all? ... out.esync_n_period ... truncates to 0
 >
 > Is reporting success for a disable request that does nothing the
 > intended behaviour?
 >
 > How are the two userspace interfaces that can now program this output
 > pin meant to coordinate?

Will drop perout from this series entirely. The current pin-based
design has the P/N aliasing and N-divided truncation issues you
identified, and the disable path needs rework to use the hardware's
glitchless stop mechanism (output_ctrl stop bit). Will rework perout
as a follow-up with an output-based design: one perout channel per
physical output, eligibility checks in the enable callback, proper
stop/start via output_ctrl, and output divisor restore on disable.

 > Does settime64 need to compensate for the next-1 Hz load latency?

This is a known hardware characteristic. settime64 is used for large
absolute time sets (boot, initial synchronization) where sub-second
precision is not critical. The PTP servo uses adjtime/adjphase for
fine control, both of which handle the 1 Hz boundary correctly. This
matches the behavior of other I2C/SPI-attached PHC drivers (e.g.
ptp_clockmatrix) that have the same HW constraint.

 > Can delta * synth_freq overflow s64 here?
 > For delta == S64_MIN the negation wraps, so abs() returns S64_MIN

Real bug. abs(S64_MIN) is undefined behavior and wraps to S64_MIN
on two's complement, bypassing the >= NSEC_PER_SEC split. Will add
an explicit guard at the top of adjtime.

 > Should a partial phase step really report success?
 > ...
 > There, clock_adjtime(ADJ_SETOFFSET) reports success while up to
 > ~999 ms of the requested step was never applied.

This is intentional. The seconds part is already committed via ToD
read-modify-write and cannot be rolled back. Reporting an error
would cause the PTP servo to retry the full delta, applying the
seconds adjustment again. The sub-second residual self-corrects in
the next servo cycle. A dev_warn is emitted so the condition is not
silent.

The same reasoning applies to the partial phase step case: the ToD
counter and some output groups are already stepped, so reporting
success with a warning is the lesser evil compared to a retry that
doubles the seconds adjustment.

 > Does first_synth_freq match what the comment describes?
 > ...
 > On a multi-channel device those can be different synths

The code is correct - first_synth_freq is the lowest-ID synth
assigned to this DPLL channel, which is the one the firmware uses
for the ToD-only conversion. Will clarify the comment.

Regarding the empty output mask with tod_step: this is a documented
firmware feature. When output mask is 0 and tod_step is set, the
firmware steps only the ToD counter using the first assigned synth's
clock period for the cycle-to-time conversion.

Thanks,
Ivan