Re: [PATCH net-next v14 08/12] dpll: sit9531x: add support to get and set frequency on pins

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: this patch adds frequency_get and frequency_set to
  sit9531x_dpll_output_pin_ops (drivers/dpll/sit9531x/dpll.c) and an…
- [Low] Concern: sit9531x_dpll_pin_ops_get() (dpll.c) does not special-
  case the INTSYNC pins.
- [Low] In sit9531x_output_phase_flush() (core.c), each restore write in
  the cleanup paths (GPIO_FUNC_CTRL1 trigger-source restore, target…
- [Low] A comment in sit9531x_pin_props_get() (prop.c) says the seeded
  rate is published only after the last fallible step, because 'a call…
- [Low] sit9531x_output_divo_calc() (core.c:1862-1869) justifies its
  `frequency > U32_MAX` rejection by saying the DPLL core 'validates the…
- [Low] Concern: sit9531x_prg_abort() (core.c:503) issues LOOP_LOCK after
  a failed PRG_CMD entry without the 100 ms settle that…
- [Low] The output read-back in sit9531x_pin_props_get() (prop.c:335-338)
  ignores every error from sit9531x_output_freq_get(), not just -ENODATA.
- [Low] In sit9531x_prg_enter() (core.c:534-537), if the first write
  (SIT9531X_DEBUG_UNLOCK_VAL to SIT9531X_REG_OUTSYS_DEBUG) reports an…

--

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

This adds frequency_get for sit9531x input pins, which report the rate
from firmware, and frequency_get/frequency_set for output pins.  An
output's rate is computed as Fvco / DIVO from the chip registers.  A set
programs a new DIVO inside the output programming state and then runs a
per-PLL phase flush.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index f3dbe949f219b..875a5fe783d5c 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -488,6 +488,140 @@ static int sit9531x_output_forced_hiz(struct sit9531x_dev *sitdev,

[ ... ]

> +static void sit9531x_prg_abort(struct sit9531x_dev *sitdev)
> +{
> +	int rc = -EIO;
> +	u8 attempt;
> +
> +	for (attempt = 0; attempt < SIT9531X_LOOP_LOCK_TRIES; attempt++) {
> +		rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +				       SIT9531X_LOOP_LOCK);
> +		if (!rc)
> +			break;
> +		usleep_range(1000, 2000);
> +	}
> +	if (rc)
> +		dev_err(sitdev->dev,
> +			"output loops left unlocked after a failed entry: %d\n",
> +			rc);
> +
> +	sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> +			  SIT9531X_DEBUG_LOCK_VAL);
> +}

[Severity: Low]
sit9531x_prg_abort() issues SIT9531X_LOOP_LOCK but does not wait out
the msleep(100) settling time that sit9531x_prg_commit() does.  The
commit message says:

  "issuing the commit without waiting would let the next request
  program a part that has not settled."

After a failed PRG_CMD entry, could the next request then program a
part that has not settled?

This path can't be reached at this commit, because output pins are not
registered yet.  The next patch, "dpll: sit9531x: implement output pin
state on a DPLL", removes sit9531x_prg_abort() and makes
sit9531x_prg_enter() call sit9531x_prg_commit() on a failed PRG_CMD
write.  Would it make sense to fold that change into this patch?

> +static int sit9531x_prg_enter(struct sit9531x_dev *sitdev)
> +{
> +	int rc;
> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> +			       SIT9531X_DEBUG_UNLOCK_VAL);
> +	if (rc)
> +		return rc;

[Severity: Low]
If this first write reports an error, the function returns without
writing SIT9531X_DEBUG_LOCK_VAL back.

Elsewhere the patch assumes that a transfer which reported an error may
still have reached the part.  The PRG_CMD failure path just below makes
that assumption, and so do the DIVO rollback and the phase flush
restore.  On the same reasoning, can this path leave the output-system
debug key at the unlock value?

When sit9531x_prg_enter() fails, sit9531x_output_freq_set() returns
without calling sit9531x_prg_commit().  So nothing writes the lock value
back until the next programming sequence.  This is still the case at
the end of the series.

> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +			       SIT9531X_PRG_CMD_STATE);

[ ... ]

> @@ -1342,6 +1481,675 @@ int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,

[ ... ]

> +static int sit9531x_output_phase_flush(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{

[ ... ]

> +	ret = sit9531x_write_u8(sitdev, SIT9531X_REG_GPIO_FUNC_CTRL1, orig);
> +	if (ret && !rc)
> +		rc = ret;
> +
> +disarm:

[ ... ]

> +	if (!(armed_mask & BIT(pll_idx))) {
> +		ret = sit9531x_update_pll_u8(sitdev, pll_idx,
> +					     SIT9531X_PLL_REG_PHFL_CTRL,
> +					     SIT9531X_PLL_PHFL_ON_DEMAND_EN, 0);
> +		if (!ret)
> +			ret = sit9531x_pll_small_update(sitdev, pll_idx);
> +		if (ret && !rc)
> +			rc = ret;
> +	}
> +
> +unpark:
> +	for (i = 0; i < SIT9531X_NUM_PLLS; i++) {
> +		if (!(parked & BIT(i)))
> +			continue;
> +		ret = sit9531x_update_pll_u8(sitdev, i,
> +					     SIT9531X_PLL_REG_PHFL_CTRL,
> +					     SIT9531X_PLL_PHFL_ON_DEMAND_EN,
> +					     SIT9531X_PLL_PHFL_ON_DEMAND_EN);
> +		if (!ret)
> +			ret = sit9531x_pll_small_update(sitdev, i);
> +		if (ret && !rc)
> +			rc = ret;
> +	}
> +
> +	return rc;
> +}

[Severity: Low]
The commit message says "The flush's own error paths restore what they
changed".  However, each restore write here is tried only once:

  - the SIT9531X_REG_GPIO_FUNC_CTRL1 trigger-source restore
  - the disarm of pll_idx
  - the unpark of each sibling

armed_mask and parked are locals, so they are lost when the function
returns.

Take a sibling that the profile armed.  If it is parked successfully and
then the re-arm write in the unpark loop fails, does that sibling stay
with SIT9531X_PLL_PHFL_ON_DEMAND_EN cleared?  A later flush re-reads
PHFL_CTRL from the hardware, sees the sibling as not armed, and never
restores it.

The other two cases are similar.  A failed disarm leaves pll_idx
answering later trigger pulses.  A failed trigger-source restore leaves
the chip-global source pinned to the in-register trigger.

sit9531x_output_freq_set() then reports any of these as success:

	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
	if (rc) {
		dev_warn(sitdev->dev, ...);
		rc = 0;
	}

This is unchanged at the end of the series.  The later flush_pending
retry re-reads the enables, which have already been changed.

Could these restores be retried, the way sit9531x_prg_commit() retries
the loop lock?  Alternatively, could the original enables be kept
somewhere that outlives the call?

[ ... ]

> +static int sit9531x_output_divo_calc(struct sit9531x_dev *sitdev, u8 out_idx,
> +				     u8 pll_idx, u64 frequency, u64 *fvco_out,
> +				     u64 *divo_out)
> +{

[ ... ]

> +	/*
> +	 * The core validates the request against the supported ranges with
> +	 * the value narrowed to u32 but hands the full u64 down, so a value
> +	 * like U32_MAX + 1 Hz validates as 1 Hz.  Reject anything that does
> +	 * not fit the narrowed width the validation actually covered.
> +	 */
> +	if (frequency > U32_MAX)
> +		return -EINVAL;

[Severity: Low]
Is this comment accurate?  dpll_pin_freq_set() in
drivers/dpll/dpll_netlink.c does not narrow the value:

	u64 freq = nla_get_u64(a), old_freq;
	...
	if (!dpll_pin_is_freq_supported(pin, freq)) {

dpll_pin_is_freq_supported() takes a u64 and compares the full value
against freq_supported[].min and .max.

The check itself is harmless, since the advertised ranges stop at
1 GHz.  Could the comment be corrected so that it doesn't misdescribe
the core's validation?  The comment is unchanged at the end of the
series.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 0400444d91a55..aa021b374c45c 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> @@ -719,8 +763,78 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,

[ ... ]

>  static const struct dpll_pin_ops sit9531x_dpll_output_pin_ops = {
>  	.direction_get		= sit9531x_dpll_output_pin_direction_get,
> +	.frequency_get		= sit9531x_dpll_output_pin_frequency_get,
> +	.frequency_set		= sit9531x_dpll_output_pin_frequency_set,
>  };

[Severity: Low]
This isn't a bug, but at this commit sit9531x_dpll_pin_is_registrable()
in core.c still rejects every output:

	if (dir != DPLL_PIN_DIRECTION_INPUT)
		return false;

So neither these output frequency callbacks nor the output read-back in
sit9531x_pin_props_get() can run yet.  Outputs are first registered in
the next patch, "dpll: sit9531x: implement output pin state on a DPLL",
and that patch's commit message explains this.

Could this commit message also say that the output side does nothing
until then?

>  const struct dpll_pin_ops *
> @@ -730,6 +844,12 @@ sit9531x_dpll_pin_ops_get(const struct sit9531x_dpll_pin *pin)
>  		return &sit9531x_dpll_output_pin_ops;
>  	if (sit9531x_dpll_is_xo_pin(pin))
>  		return &sit9531x_dpll_xo_pin_ops;
> +	/*
> +	 * Seeded from the firmware node before the pin is registered and
> +	 * never written afterwards, so register and unregister agree.
> +	 */
> +	if (!sit9531x_ref_state_get(pin->dpll->dev, pin->id)->freq)
> +		return &sit9531x_dpll_input_pin_norate_ops;
>  	return &sit9531x_dpll_input_pin_ops;
>  }

[Severity: Low]
There is no INTSYNC case here.

For the INTSYNC destination input (SIT9531X_INTSYNC_PIN_ID, index 9),
wouldn't this read sitdev->ref[9].freq?  That is past the end of ref[]
and aliases out[0].freq, so register and unregister could pick
different ops tables.

The INTSYNC source output (SIT9531X_INTSYNC_OUT_PIN_ID) would get
sit9531x_dpll_output_pin_ops.  Its frequency_get would return -EINVAL
for that index and abort the pin dump.

Neither pin is reachable anywhere in the series:

  - At this commit, sit9531x_input_pin_is_registrable() returns false
    for index >= num_inputs, and no output is registered.
  - In "dpll: sit9531x: implement output pin state on a DPLL", both
    INTSYNC ids are still rejected.
  - "dpll: sit9531x: model the inter-PLL sync net as a pair of pins"
    adds sit9531x_dpll_intsync_src_pin_ops and
    sit9531x_dpll_intsync_dst_pin_ops, and returns them before any
    ref[] lookup.

Is it still worth guarding the lookup here, so this function is correct
without relying on the later patches?

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> index 16643f25f6b28..4ca92f4f17d15 100644
> --- a/drivers/dpll/sit9531x/prop.c
> +++ b/drivers/dpll/sit9531x/prop.c

[ ... ]

> @@ -313,6 +314,30 @@ sit9531x_pin_props_get(struct sit9531x_dev *sitdev,
>  	    index == SIT9531X_INTSYNC_OUT_PIN_ID)
>  		return props;
>  
> +	/*

[ ... ]

> +	 * that rate is published ahead of the fallback range below.  An
> +	 * output the configuration does not route has no rate to read,
> +	 * which is not an error.
> +	 */
> +	if (dir == DPLL_PIN_DIRECTION_OUTPUT &&
> +	    index < sitdev->info->num_outputs) {
> +		u64 hw_freq;
> +
> +		mutex_lock(&sitdev->multiop_lock);
> +		rc = sit9531x_output_freq_get(sitdev, index, &hw_freq);
> +		mutex_unlock(&sitdev->multiop_lock);
> +		if (!rc)
> +			curr_freq = hw_freq;
> +	}

[Severity: Low]
sit9531x_output_freq_get() updates the cache itself:

	*frequency = div64_u64(fvco, divo);
	sitdev->out[out_idx].freq = *frequency;

Further down in sit9531x_pin_props_get(), the existing comment says the
seeded rate is published only after the ranges kcalloc(), because "a
call that reports failure must not leave the device's cached rate
changed behind it."

If that kcalloc() fails, doesn't the function return -ENOMEM with
out[index].freq already changed?

The value written is the real hardware rate, so the effect is small.
Still, the comment and the code now disagree, and that is unchanged at
the end of the series.

[Severity: Low]
The comment above only explains why the unrouted case is ignored.  The
"if (!rc)" check, though, also drops I2C errors from
sit9531x_output_freq_get().

If the read-back fails during registration, the pin is built with only
the firmware list in freq_supported, or the 1 Hz to 1 GHz fallback when
there is no list.  The rate the output is actually running at is
missing.

The pin properties are never rebuilt.  dpll_pin_freq_set() checks
dpll_pin_is_freq_supported() before it calls the driver.  If the
firmware list doesn't include the running rate, would the core then
reject a later request for that rate, such as one restoring it after a
change?

Should only -ENODATA be ignored here?  This is unchanged at the end of
the series.

[ ... ]

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