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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: zl3073x: add PTP clock support

This registers a PTP clock device per DPLL channel and implements
gettimex64, settime64, adjtime, adjfine, adjphase and getmaxphase on top
of the existing ToD, TIE and output phase step helpers.  All callbacks are
serialized by the per-DPLL zldpll->lock mutex, and PTP_1588_CLOCK becomes
a hard dependency of the driver.

A few questions below.

> diff --git a/drivers/dpll/zl3073x/Kconfig b/drivers/dpll/zl3073x/Kconfig
> index 5bbca14005813..da1731ace15f0 100644
> --- a/drivers/dpll/zl3073x/Kconfig
> +++ b/drivers/dpll/zl3073x/Kconfig
> @@ -2,7 +2,7 @@
>  
>  config ZL3073X
>  	tristate "Microchip Azurite DPLL/PTP/SyncE devices" if COMPILE_TEST
> -	depends on NET
> +	depends on NET && PTP_1588_CLOCK
>  	select DPLL
>  	select NET_DEVLINK
>  	select REGMAP
> @@ -16,7 +16,7 @@ config ZL3073X
>  
>  config ZL3073X_I2C
>  	tristate "I2C bus implementation for Microchip Azurite devices"
> -	depends on I2C && NET
> +	depends on I2C && NET && PTP_1588_CLOCK
>  	select REGMAP_I2C
>  	select ZL3073X
>  	help
> @@ -28,7 +28,7 @@ config ZL3073X_I2C
>  
>  config ZL3073X_SPI
>  	tristate "SPI bus implementation for Microchip Azurite devices"
> -	depends on NET && SPI
> +	depends on NET && SPI && PTP_1588_CLOCK
>  	select REGMAP_SPI
>  	select ZL3073X

[Severity: Low]
Should PTP_1588_CLOCK be a hard dependency here?

ZL3073X is only user-visible with COMPILE_TEST and is otherwise selected by
ZL3073X_I2C/ZL3073X_SPI, so on a tree with CONFIG_PTP_1588_CLOCK=n the
ZL3073X_I2C and ZL3073X_SPI prompts disappear on oldconfig and the whole
driver is dropped, including the DPLL, devlink/flash and SyncE
functionality that does not need PTP.

The new code compiles against the !PTP stubs (only ptp_clock_register()
returns NULL, which zl3073x_dpll_ptp_register() already treats as
success), which is what PTP_1588_CLOCK_OPTIONAL in drivers/ptp/Kconfig is
for:

	config PTP_1588_CLOCK_OPTIONAL
		tristate
		default y if PTP_1588_CLOCK=n
		default PTP_1588_CLOCK

Would depends on PTP_1588_CLOCK_OPTIONAL work here instead?  If the hard
dependency is intended, could the changelog mention that PTP becomes
mandatory for existing users?

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 83bd3027dbaa1..f2e137475b40f 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -2313,45 +2315,384 @@ zl3073x_dpll_init_fine_phase_adjust(struct zl3073x_dev *zldev)

[ ... ]

> +/**
> + * zl3073x_dpll_ptp_settime64 - set ToD counters to given time
> + * @info: PTP clock info
> + * @ts: timespec with time to set
>   *
> - * Return: pointer to DPLL device on success, error pointer on error
> + * Return: 0 on success, <0 on error
>   */
> -struct zl3073x_dpll *
> -zl3073x_dpll_alloc(struct zl3073x_dev *zldev, u8 ch)
> +static int zl3073x_dpll_ptp_settime64(struct ptp_clock_info *info,
> +				      const struct timespec64 *ts)
>  {
> -	struct zl3073x_dpll *zldpll;
> +	struct zl3073x_dpll *zldpll = container_of(info, struct zl3073x_dpll,
> +						   ptp_info);
>  
> -	zldpll = kzalloc_obj(*zldpll);
> -	if (!zldpll)
> -		return ERR_PTR(-ENOMEM);
> +	guard(mutex)(&zldpll->lock);
>  
> -	zldpll->dev = zldev;
> -	zldpll->id = ch;
> -	mutex_init(&zldpll->lock);
> -	INIT_LIST_HEAD(&zldpll->pins);
> +	return zl3073x_chan_tod_write(zldpll->dev, zldpll->id, *ts);
> +}

[Severity: High]
Does settime64() need to compensate for the deferred ToD load?

zl3073x_chan_tod_write() only ever issues the next-1Hz-edge command:

drivers/dpll/zl3073x/chan.c:zl3073x_chan_tod_write() {
	...
	return zl3073x_chan_tod_ctrl(zldev, ch,
				     ZL_DPLL_TOD_CTRL_CMD_WR_NEXT_1HZ);
}

so the value written becomes the counter value at the coming 1 Hz edge,
0..1 s after the register write.  zl3073x_chan_tod_ready_wait() budgets
ZL_POLL_TOD_WR_TIMEOUT_US (1000 ms) for exactly that pending write.

zl3073x_chan_tod_adjust() in the same driver compensates for this by
basing the write on the next-1Hz read:

drivers/dpll/zl3073x/chan.c:zl3073x_chan_tod_adjust() {
	...
	rc = zl3073x_chan_tod_read(zldev, ch, true, &ts_next, NULL);
	...
	/* Apply delta to the next-Hz ToD */
	ts_next = timespec64_add(ts_next, delta);
	...
	return zl3073x_chan_tod_write(zldev, ch, ts_next);
}

Here the caller's timespec is written verbatim, so after the edge the
counter reads ts while real time has advanced by the distance from the
call to that edge.  Does this leave the clock up to a second behind the
requested time on every clock_settime()?

Also, ts.tv_nsec is written raw.  If a caller passes a non-zero tv_nsec,
does the ToD second boundary end up displaced from the DPLL 1 Hz / output
1PPS phase, i.e. the coherence that adjtime() takes care to preserve by
routing sub-second deltas through phase step or TIE write?

> +/**
> + * zl3073x_dpll_ptp_adjtime_phase_step - adjust sub-second time via phase step
> + * @zldpll: DPLL channel
> + * @delta: time adjustment in nanoseconds (must be within (-NSEC_PER_SEC,
> + *         NSEC_PER_SEC))
> + *
> + * Uses the output phase step mechanism with tod_step=1 to adjust both
> + * the output clock phase and the ToD counter simultaneously. This keeps
> + * outputs and ToD coherent. Only valid for NCO.
> + *
> + * Outputs are grouped by synthesizer since the phase step value is in
> + * synthesizer clock cycles. The first synth group with enabled outputs
> + * uses tod_step to adjust both outputs and the ToD counter. Remaining
> + * groups step outputs only. If no synth has enabled outputs, the ToD
> + * counter is stepped alone using an empty output mask (the FW uses
> + * the first enabled synth's period for the conversion).

[Severity: Low]
This isn't a bug, but the kernel-doc of
zl3073x_dpll_ptp_adjtime_phase_step() speaks of "enabled outputs" while
the code filters on the device-global step-time mask instead:

	if (!zl3073x_dev_out_is_stepped(zldev, out_id))
		continue;

drivers/dpll/zl3073x/core.h:zl3073x_dev_out_is_stepped() {
	return !!(zldev->out_step_time_mask & BIT(index));
}

out_step_time_mask comes from ZL_REG_OUTPUT_STEP_TIME_MASK and says
nothing about whether an output is enabled, so an enabled, DPLL-owned
output that is absent from that mask is excluded from the phase step and
can also trigger the !tod_stepped ToD-only fallback.  Could the wording be
changed to refer to step-time-capable outputs?

> +	/* Process each synth group */
> +	for (i = 0; i < ZL3073X_NUM_SYNTHS; i++) {

[ ... ]

> +/**
> + * zl3073x_dpll_ptp_adjtime - adjust PTP clock time

[ ... ]

> +static int zl3073x_dpll_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
>  {
> -	WARN(zldpll->dpll_dev, "DPLL device is still registered\n");
> +	struct zl3073x_dpll *zldpll = container_of(info, struct zl3073x_dpll,
> +						   ptp_info);
> +	struct zl3073x_dev *zldev = zldpll->dev;
> +	const struct zl3073x_chan *chan;
> +	bool sec_adjusted = false;
> +	struct timespec64 ts;
> +	int rc;
>  
> -	mutex_destroy(&zldpll->lock);
> -	kfree(zldpll);
> +	if (!delta)
> +		return 0;
> +
> +	guard(mutex)(&zldpll->lock);

[ ... ]

> +	if (delta >= NSEC_PER_SEC || delta <= -NSEC_PER_SEC) {
> +		s32 remainder;
> +
> +		ts.tv_sec = div_s64_rem(delta, NSEC_PER_SEC, &remainder);
> +		ts.tv_nsec = 0;
> +		delta = remainder;
> +
> +		rc = zl3073x_chan_tod_adjust(zldev, zldpll->id, ts);
> +		if (rc)
> +			return rc;
> +
> +		/* No sub-second remainder, done */
> +		if (!delta)
> +			return 0;
> +
> +		/* Wait for the ToD write to be applied at the 1 Hz edge
> +		 * before issuing phase step or TIE write, so the pending
> +		 * WR_NEXT_1HZ does not overwrite the sub-second adjustment.
> +		 */
> +		rc = zl3073x_chan_tod_ready_wait(zldev, zldpll->id);
> +		if (rc)
> +			return rc;
> +
> +		sec_adjusted = true;
> +	}

[Severity: Medium]
Is this error return reachable after the seconds are already committed?

zl3073x_chan_tod_adjust() ends in zl3073x_chan_tod_write(), which arms
ZL_DPLL_TOD_CTRL_CMD_WR_NEXT_1HZ, so once it returns 0 the whole-second
step will latch at the next 1 Hz edge and cannot be revoked.  The
following zl3073x_chan_tod_ready_wait() can then fail:

drivers/dpll/zl3073x/chan.c:zl3073x_chan_tod_ready_wait() {
	...
	case ZL_DPLL_TOD_CTRL_CMD_WR_NEXT_1HZ:
		timeout = ZL_POLL_TOD_WR_TIMEOUT_US;
	...
	return rc == -ETIMEDOUT ? -EBUSY : rc;
}

with ZL_POLL_TOD_WR_TIMEOUT_US == 1000 ms while zl3073x_chan_tod_adjust()
only guarantees a 20 ms margin before the edge, so the pending write can
need nearly the full second to retire.  A plain regmap read error gives
the same exit.

That return happens before sec_adjusted = true, so this is the one error
path that is not covered by the compensating logic further down:

	if (rc && sec_adjusted) {
		dev_warn(zldev->dev,
			 "Sub-second adjustment failed after seconds applied\n");
		return 0;
	}

Does the caller then get a failure for an adjustment whose seconds part is
already applied, with the sub-second remainder dropped?  If the servo or a
clock_adjtime(ADJ_SETOFFSET) user re-issues the same delta after the edge
has passed, is the whole second applied twice?

> +
> +	/* Apply sub-second delta via phase step (NCO) or TIE write */
> +	if (zl3073x_chan_mode_is_nco(chan)) {
> +		rc = zl3073x_dpll_ptp_adjtime_phase_step(zldpll, delta);
> +		if (!rc)
> +			return 0;
> +	} else {
> +		rc = zl3073x_chan_tie_write(zldev, zldpll->id, delta);
> +		if (!rc)
> +			return 0;
> +	}

[Severity: Medium]
Does the same pending WR_NEXT_1HZ hazard exist across callback
invocations?

The comment above acknowledges the hazard within one adjtime() call, but
the drain only runs when the seconds branch was taken.  For |delta| < 1 s
sec_adjusted stays false and zl3073x_chan_tod_ready_wait() is never
called, and zl3073x_dpll_ptp_adjphase() does not drain either:

	chan = zl3073x_chan_state_get(zldev, zldpll->id);

	if (!zl3073x_chan_mode_supports_tie(chan))
		return -EOPNOTSUPP;

	return zl3073x_chan_tie_write(zldev, zldpll->id, delta);

Meanwhile zl3073x_dpll_ptp_settime64(), and adjtime() on an exact
whole-second delta ("No sub-second remainder, done"), both drop
zldpll->lock while a WR_NEXT_1HZ is still armed, because
zl3073x_chan_tod_write() only waits for a previous operation on entry:

CPU0
zl3073x_dpll_ptp_settime64()
   zl3073x_chan_tod_write()
      zl3073x_chan_tod_ctrl(..., ZL_DPLL_TOD_CTRL_CMD_WR_NEXT_1HZ)
   /* mutex released, write still pending */

CPU0 (or another task), same 1 Hz interval
zl3073x_dpll_ptp_adjphase()          /* or sub-second adjtime() */
   zl3073x_chan_tie_write()          /* no ToD semaphore poll */
   /* 1 Hz edge: pending absolute ToD write lands, sub-second
      adjustment discarded, outputs already stepped */

The mutex serializes register accesses but not the retirement of the
asynchronous device command, so does 0 get returned for an adjustment that
was silently dropped, leaving ToD and outputs incoherent for the NCO phase
step case?  Would calling zl3073x_chan_tod_ready_wait() unconditionally
before the phase step / TIE write cover this?

> +
> +	/* Phase step or TIE write failed, fall back to ToD adjust */
> +	rc = zl3073x_chan_tod_adjust(zldev, zldpll->id,
> +				     ns_to_timespec64(delta));

[ ... ]

> +/**
> + * zl3073x_dpll_ptp_adjfine - adjust PTP clock frequency
> + * @info: PTP clock info
> + * @scaled_ppm: frequency adjustment in scaled ppm (ppm * 2^16)
> + *
> + * Only supported for NCO. Writes the delta frequency offset register.
> + *
> + * Return:
> + * * %0			- success or @scaled_ppm is zero (no-op)
> + * * %-EOPNOTSUPP	- NCO pin is not connected and @scaled_ppm is non-zero
> + * * negative		- other error
> + */

[Severity: Low]
This isn't a bug, but does the documented "@scaled_ppm is zero (no-op)"
match the code?

> +	guard(mutex)(&zldpll->lock);
> +
> +	chan = zl3073x_chan_state_get(zldpll->dev, zldpll->id);
> +	if (!zl3073x_chan_mode_is_nco(chan))
> +		return scaled_ppm ? -EOPNOTSUPP : 0;
> +	if (offset == chan->df_offset)
> +		return 0;
> +
> +	return zl3073x_chan_df_offset_set(zldpll->dev, zldpll->id, offset);
> +}

With scaled_ppm == 0 on an NCO channel whose cached df_offset is non-zero,
offset becomes 0 and zl3073x_chan_df_offset_set() performs a 48-bit write
to ZL_REG_DPLL_DF_OFFSET, which changes the NCO frequency and can return a
negative errno.  Could the Return section be reworded to describe the
absolute setpoint semantics?

> @@ -2436,6 +2777,47 @@ zl3073x_dpll_ref_sync_pairs_register(struct zl3073x_dpll *zldpll)
>  	return 0;
>  }
>  
> +/**
> + * zl3073x_dpll_alloc - allocate DPLL device
> + * @zldev: pointer to zl3073x device
> + * @ch: DPLL channel number
> + *
> + * Allocates DPLL device structure for given DPLL channel.
> + *
> + * Return: pointer to DPLL device on success, error pointer on error
> + */
> +struct zl3073x_dpll *
> +zl3073x_dpll_alloc(struct zl3073x_dev *zldev, u8 ch)
> +{
> +	struct zl3073x_dpll *zldpll;
> +
> +	zldpll = kzalloc_obj(*zldpll);
> +	if (!zldpll)
> +		return ERR_PTR(-ENOMEM);
> +
> +	zldpll->dev = zldev;
> +	zldpll->id = ch;
> +	mutex_init(&zldpll->lock);
> +	INIT_LIST_HEAD(&zldpll->pins);
> +
> +	return zldpll;
> +}
> +
> +/**
> + * zl3073x_dpll_free - free DPLL device
> + * @zldpll: pointer to zl3073x_dpll structure
> + *
> + * Deallocates given DPLL device previously allocated by @zl3073x_dpll_alloc.
> + */
> +void
> +zl3073x_dpll_free(struct zl3073x_dpll *zldpll)
> +{
> +	WARN(zldpll->dpll_dev, "DPLL device is still registered\n");
> +
> +	mutex_destroy(&zldpll->lock);
> +	kfree(zldpll);
> +}
> +

[Severity: Low]
This isn't a bug, but zl3073x_dpll_alloc() and zl3073x_dpll_free() are
moved verbatim from where the new PTP helpers were inserted to this spot,
with no functional change and no mention in the changelog.  That accounts
for roughly 35 of the 439 added lines and mixes code motion with new
logic.  The new PTP helpers are all static and self-contained, so is the
move needed?  Could it be dropped or split into a separate no-functional-change
patch?

[ ... ]