Re: [PATCH net-next v8 11/15] dpll: sit9531x: add support to get and set esync on pins

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 3 · Low: 2

- [High] Embedded-sync frequency is implemented as the output's carrier
  frequency.
- [High] sit9531x_dpll_output_pin_esync_set() is asymmetric and destroys
  the only record of the pin's previous rate.
- [Medium] sit9531x_dpll_output_pin_esync_get() reports esync state from
  an unsynchronized software cache: it returns dpin->esync_freq and a…
- [Medium] The uAPI duty-cycle percentage constant is written verbatim
  into a hardware register field.
- [Medium] The esync enable path chains three independently committed
  hardware transactions with no rollback: sit9531x_output_freq_set()…
- [Low] sit9531x_dpll_output_pin_esync_get() reports esync->pulse = 50
  unconditionally, including when it reports esync->freq == 0 (embedded…
- [Low] The new PROG0 write is wrapped in its own
  prg_enter()/prg_commit() window even though PROG0 lives in the same…

--

dpll: sit9531x: add support to get and set esync on pins

This adds esync_get and esync_set callbacks to the sit9531x output pin ops
and a new core helper that programs the per-output ODRn_PROG0 pulse control
byte.  Only outputs flagged as esync-control accept the operations, the
rest return -EOPNOTSUPP.

No Link: tags are present in the commit header.

A few questions below.

[Severity: Medium]
Is any conversion needed between the value passed in as pulse_ctrl and the
hardware field?  The only caller passes SIT9531X_ESYNC_PULSE_DEFAULT, which
is decimal 50, and the same constant is handed to userspace as esync->pulse
in sit9531x_dpll_output_pin_esync_get().  The uAPI unit for that attribute
is percent:

Documentation/netlink/specs/dpll.yaml
        name: esync-pulse
        type: u32
        doc: |
          A ratio of high to low state of a SYNC signal pulse embedded
          into base clock frequency. Value is in percents.

The new regs.h comment in this patch describes the same register as a pulse
count byte for SYSREF / SYNCB modes and defines no field masks, so it looks
like one constant is being used both as a uAPI percentage and as a raw
register value.  Which of the two is it?

Separately, this is an unmasked full-byte write with no read-modify-write.
The neighbouring PROG6 programming in sit9531x_output_phase_adjust_set()
does preserve the upper bits:

	/* PROG6 RMW: preserve OPSTG_VCASC_BUMP in [7:5] */
	rc = sit9531x_read_u8(sitdev, SIT9531X_REG(page, base), &prog6_val);
	...
	prog6_val &= SIT9531X_OUT_PRG_OPSTG_MASK;

Can writing all eight bits of ODRn_PROG0 clear other fields in that byte
that the initial config blob programmed?

[Severity: Low]
This isn't a bug, but the comment above states PROG0 shares the PRG_CMD
programming state with the DIVO and PRG_RST_DELAY writes, so could this
byte be written inside the window sit9531x_output_freq_set() already opens?

As written, one call to sit9531x_dpll_output_pin_esync_set() runs three
separate windows:

  sit9531x_output_freq_set()        -> prg_enter()/prg_commit()
  sit9531x_output_pulse_ctrl_set()  -> prg_enter()/prg_commit()
  sit9531x_output_enable()          -> prg_enter()/prg_commit()

and sit9531x_prg_commit() issues UPDATE_NVM, then LOOP_LOCK, then an
unconditional msleep(100), so that is roughly 300 ms of multiop_lock hold
time plus three unlock/relock cycles of the output loops on a live output.

[Severity: High]
Is the embedded sync frequency here the marker rate or the carrier rate?

The dpll uAPI describes DPLL_A_PIN_ESYNC_FREQUENCY as a signal embedded
into the pin's separately configured base clock:

Documentation/netlink/specs/dpll.yaml
        name: esync-frequency
        type: u64
        doc: |
          Frequency of Embedded SYNC signal. If provided, the pin is configured
          with a SYNC signal embedded into its base clock frequency.

This range advertises 0 and 10 MHz as the acceptable esync frequencies, and
sit9531x_dpll_output_pin_esync_set() then feeds the accepted 10 MHz into
sit9531x_output_freq_set(), which programs the DIVO output divider, i.e.
the base clock of the pad.  A 1 PPS esync request is rejected with -EINVAL.

For comparison, ice_dpll_output_esync_get() treats 10 MHz as the required
base frequency and reports the marker rate separately:

	if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY) ||
	    p->freq != DPLL_PIN_FREQUENCY_10_MHZ)
		return -EOPNOTSUPP;
	...
		esync->freq = DPLL_PIN_FREQUENCY_1_HZ;

The driver's own binding example also pairs esync-control with a base rate
that esync_set would overwrite:

Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml
    pin@0 {
        reg = <0>;
        label = "clkout0";
        esync-control;
        supported-frequencies-hz = /bits/ 64 <156250000>;
    };

Enabling esync on that pin changes the pad from 156.25 MHz to 10 MHz.  Is
that intended?

The changelog says:

  "On this device that is the same divider programming as a frequency
  change plus the per-output pulse control, so enabling it sets the output
  to 1 Hz and disabling it restores the requested rate."

That does not seem to match the code: the only accepted non-zero value is
10 MHz, not 1 Hz, and the freq == 0 path only calls
sit9531x_output_disable() rather than restoring a rate.  Could the
changelog be adjusted to describe what the code does?

[Severity: Medium]
Can dpin->esync_freq go stale here?  Unlike
sit9531x_dpll_output_pin_frequency_get(), this getter does no DIVO or
PROG0 readback, and dpin->esync_freq is written only by esync_set().

Two writers of the same divider leave it inconsistent:

  sit9531x_dpll_output_pin_frequency_set()
     -> sit9531x_output_freq_set(sitdev, dpin->id, actual_pll, frequency)

  sit9531x_dpll_output_pin_state_on_dpll_set(DISCONNECTED)
     -> sit9531x_output_disable(sitdev, dpin->id)

Neither clears dpin->esync_freq, so after pin_set(esync-frequency=10000000)
followed by pin_set(frequency=25000000) a pin dump still advertises an
embedded sync configuration the device is no longer producing.  Note that
dpll_pin_set_from_nlattr() handles DPLL_A_PIN_FREQUENCY and
DPLL_A_PIN_ESYNC_FREQUENCY independently in message order, so both can
arrive in one request.

Userspace also cannot repair the stale value, because dpll_pin_esync_set()
consults the getter first:

drivers/dpll/dpll_netlink.c:dpll_pin_esync_set() {
	...
	if (freq == esync.freq)
		return 0;
	...
}

so a repeat request for the cached value is acked without the driver
callback running.

Also, sit9531x_dpll_pin_register() copies only the capability flag:

	pin->esync_control = props->esync_control;

so an output that the efuse/NVM blob already programmed for esync reads
back as disabled.  Should the getter read the hardware instead?

[Severity: Low]
Should esync->pulse be reported as 0 when esync->freq is 0?  As written a
pin dump on a never-configured esync-control output reports
DPLL_A_PIN_ESYNC_FREQUENCY = 0 together with DPLL_A_PIN_ESYNC_PULSE = 50,
i.e. a 50 percent pulse ratio for a signal that is not present.  Both
ice_dpll_output_esync_get() and zl3073x_dpll_output_pin_esync_get() zero
both fields in the disabled case:

	} else {
		esync->freq = 0;
		esync->pulse = 0;
	}

[Severity: High]
Where is the pin's previous output rate saved so this path can restore it?

The enable path below calls sit9531x_output_freq_set(), which reprograms
DIVO and also replaces the cached rate:

drivers/dpll/sit9531x/core.c:sit9531x_output_freq_set() {
	...
	sitdev->out[out_idx].freq = (u32)div64_u64(fvco, divo);
}

and sit9531x_output_disable() touches only the Hi-Z bits:

	/* Take control (MASK=1) and mute (STATE=0) on both DIFF and SE */

Neither struct sit9531x_dpll_pin nor struct sit9531x_out holds the
pre-esync frequency or the previous PROG0 byte, and
sit9531x_output_pulse_ctrl_set() never reads PROG0 back, so after an
enable/disable cycle the rate userspace originally configured appears to be
gone from both hardware and driver state.

Does a following pin_set(state=connected) then un-mute the pad at 10 MHz
with the esync pulse still armed?

drivers/dpll/sit9531x/dpll.c:sit9531x_dpll_output_pin_state_on_dpll_set() {
	case DPLL_PIN_STATE_CONNECTED:
		rc = sit9531x_output_enable(sitdev, dpin->id);
	...
}

while sit9531x_dpll_output_pin_esync_get() reports esync->freq == 0.

[Severity: Medium]
What happens if the second or third step here fails?

Each of the three calls commits independently: sit9531x_output_freq_set()
writes DIVO and runs prg_commit() with UPDATE_NVM plus LOOP_LOCK, and
sit9531x_output_pulse_ctrl_set() opens its own PRG_CMD window, so this is
not one transaction.  If sit9531x_output_pulse_ctrl_set() or
sit9531x_output_enable() returns an I2C or regmap error, the divider change
stays committed and nothing is unwound.

Since dpin->esync_freq is only assigned when all three steps return 0, the
driver then reports esync as disabled on a pin whose carrier has already
been changed to 10 MHz.  Is that state recoverable through this attribute?
dpll_pin_esync_set() short-circuits on the cached value:

	if (freq == esync.freq)
		return 0;

so a follow-up request for freq == 0 matches the stale cached 0 and never
reaches the driver.

There is also a window between the frequency commit and the PROG0 commit
where an already-enabled output emits the new rate with no marker.

 static const struct dpll_pin_ops sit9531x_dpll_output_pin_ops = {

[ ... ]

The comment says this byte applies in SYSREF / SYNCB modes.  The new esync
path does not select either mode, so does the write take effect at all in
the mode the output is left in?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902214030.20955-1-arouhi%40sitime.com