Re: [PATCH 3/6] ptp: qcom: Add PTP driver for the Qualcomm TSC hardware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `adjfine` implementation improperly applies a one-shot phase shift and fails to clear adjustments on zero, rather than configuring continuous frequency modification.
- [High] The hardware fallback path for `adjtime` physically stops the hardware counter, causing lost ticks and non-deterministic negative phase drift.
- [High] Sequential 32-bit reads of the 64-bit hardware counter lack a retry loop, exposing severe backwards time jumps during rollover.
- [High] The `adjfine` drift calculation silently drops small negative adjustments by computing a period of zero.
- [Medium] The `PULSE_WIDTH_MASK` configuration in `qcom_tsc_settime()` is corrupted by a missing bitwise clear before applying `FIELD_PREP`.
--

--- Patch [3]: [PATCH 3/6] ptp: qcom: Add PTP driver for the Qualcomm TSC hardware ---
commit df14ad1f19420dd656de996ed9674c4b2511975f
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

ptp: qcom: Add PTP driver for the Qualcomm TSC hardware

Add a PTP Hardware Clock driver for the Qualcomm Timestamp Counter (TSC)
hardware found on Qualcomm Lemans and QDU1000 SoCs. The TSC is a free
running hardware counter used for time synchronization, clocked by an AHB
configuration clock and a global counter clock.

> diff --git a/drivers/ptp/ptp_qcom_tsc.c b/drivers/ptp/ptp_qcom_tsc.c
> new file mode 100644
> index 0000000000000..b29248150388f
> --- /dev/null
> +++ b/drivers/ptp/ptp_qcom_tsc.c

[ ... ]

> +static int qcom_tsc_read_time(struct qcom_tsc *tsc, struct timespec64 *ts)
> +{
> +	const struct qcom_tsc_regs *regs = tsc->soc->reg_layout;
> +	u32 hi, lo;
> +
> +	if (!qcom_tsc_is_enabled(tsc))
> +		return -EINVAL;
> +
> +	hi = readl_relaxed(tsc->base + regs->read_cntcv_hi);
> +	lo = readl_relaxed(tsc->base + regs->read_cntcv_lo);

[Severity: High]
Does this sequential read risk returning backward time jumps during rollover?

If the lower 32-bit register rolls over between reading the upper and
lower registers, could this return a stale upper half combined with a
wrapped lower half? Is a retry loop needed here to ensure consistency?

> +
> +	if (tsc->soc->rollover) {
> +		ts->tv_sec = hi;
> +		ts->tv_nsec = lo;
> +	} else {
> +		u64 ns = ((u64)hi << NSEC_SHIFT) | lo;
> +
> +		*ts = ns_to_timespec64(ns);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int qcom_tsc_update_offset(struct qcom_tsc *tsc, const struct timespec64 ts)
> +{
> +	const struct qcom_tsc_regs *regs = tsc->soc->reg_layout;
> +	struct timespec64 tod;
> +	u32 regval, hi, lo;
> +	int ret;
> +
> +	if (tsc->soc->offset_correction) {
> +		qcom_tsc_split_ts(tsc, ts, &hi, &lo);
> +
> +		writel_relaxed(lo, tsc->base + regs->offset_lo);
> +		writel_relaxed(hi, tsc->base + regs->offset_hi);
> +
> +		return 0;
> +	}
> +
> +	/*
> +	 * No hardware offset correction support: disable and reload the
> +	 * counter with the corrected time, as recommended by the hardware
> +	 * design team.
> +	 */
> +	ret = qcom_tsc_read_time(tsc, &tod);
> +	if (ret)
> +		return ret;
> +
> +	regval = readl_relaxed(tsc->base + regs->control_cntcr);
> +	regval &= ~BIT(0);
> +	writel_relaxed(regval, tsc->base + regs->control_cntcr);

[Severity: High]
Does disabling the counter during the update permanently lose hardware ticks?

When the software update occurs on SoCs without hardware offset correction,
the time elapsed between reading the time and disabling the counter seems to
be permanently dropped. Could this introduce negative drift and jitter on
each call to adjtime?

> +
> +	return qcom_tsc_update_counter(tsc, timespec64_add(tod, ts));
> +}

[ ... ]

> +static int qcom_tsc_settime(struct ptp_clock_info *ptp, const struct timespec64 *ts)
> +{
> +	struct qcom_tsc *tsc = container_of(ptp, struct qcom_tsc, ptp_info);
> +	const struct qcom_tsc_regs *regs = tsc->soc->reg_layout;
> +	struct timespec64 tod;
> +	u32 regval;
> +	int ret;
> +
> +	mutex_lock(&tsc->lock);
> +
> +	if (qcom_tsc_is_enabled(tsc)) {
> +		qcom_tsc_read_time(tsc, &tod);
> +		ret = qcom_tsc_update_offset(tsc, timespec64_sub(*ts, tod));
> +		mutex_unlock(&tsc->lock);
> +		return ret;
> +	}
> +
> +	/* Configure and enable the TSC counter */
> +	regval = readl_relaxed(tsc->base + regs->control_cntcr);
> +	regval |= FIELD_PREP(PULSE_WIDTH_MASK, tsc->soc->pulse_width);
> +	writel_relaxed(regval, tsc->base + regs->control_cntcr);

[Severity: Medium]
Should the PULSE_WIDTH_MASK bits be cleared before applying the bitwise OR?

If there are previous bits set in this field, could the bitwise OR without
a preceding bitwise clear leave those bits set and corrupt the pulse width
configuration?

> +
> +	if (tsc->soc->rollover)
> +		writel_relaxed(ROLLOVER_VAL, tsc->base + regs->rollover_val);
> +
> +	ret = qcom_tsc_update_counter(tsc, *ts);
> +	mutex_unlock(&tsc->lock);
> +
> +	return ret;
> +}

[ ... ]

> +static int qcom_tsc_drift_correction(struct qcom_tsc *tsc, long scaled_ppm)
> +{
> +	const struct qcom_tsc_soc_data *soc = tsc->soc;
> +	const struct qcom_tsc_regs *regs = soc->reg_layout;
> +	long ppb = scaled_ppm_to_ppb(scaled_ppm);
> +	u64 period, incval = 0;
> +	u32 regval = 0, subperiod;
> +
> +	if (ppb < 0) {
> +		ppb = -ppb;
> +		regval &= ~DRIFT_JUMP_SWALLOW;
> +
> +		period = div_u64(ppb, soc->cntr_res);
> +		if (period >= DRIFT_MAX_RES)
> +			period = DRIFT_MAX_RES;
> +	} else {

[Severity: High]
Does this calculation silently drop small negative adjustments?

When ppb is less than zero and its absolute value is smaller than
soc->cntr_res, div_u64 will evaluate to 0. 

Since the negative branch lacks the fallback to period = 1 that the positive
branch has, does this result in a period of 0 being written to hardware for
fine-grained negative frequency adjustments?

> +		regval |= DRIFT_JUMP_SWALLOW;
> +
> +		period = div_u64(ppb, soc->cntr_res);
> +		if (period >= DRIFT_MAX_RES) {
> +			period = DRIFT_MAX_RES;
> +			incval = soc->cntr_res * 2;
> +		} else if (ppb < soc->cntr_res) {
> +			incval = soc->cntr_res + ppb;
> +			period = 1;
> +		} else {
> +			incval = soc->cntr_res * 2;
> +		}
> +	}
> +
> +	subperiod = soc->pulse_width * 2;
> +	regval |= FIELD_PREP(DRIFT_SUBPERIOD_MASK, subperiod);
> +	regval |= FIELD_PREP(DRIFT_PERIOD_MASK, period);
> +
> +	/* Update jump/swallow, period, subperiod */
> +	writel_relaxed(regval, tsc->base + regs->drift_correct_duration);
> +
> +	writel_relaxed(incval, tsc->base + regs->drift_correct_incr_val);
> +
> +	/* Trigger the drift correction */
> +	regval = readl_relaxed(tsc->base + regs->drift_correct_cmd);
> +	regval |= BIT(0);
> +	writel_relaxed(regval, tsc->base + regs->drift_correct_cmd);
> +
> +	return readl_poll_timeout(tsc->base + regs->drift_correct_cmd, regval,
> +				  !(regval & BIT(1)), DRIFT_INTERVAL_US,
> +				  DRIFT_TIMEOUT_US(period, subperiod, soc->cntr_res));
> +}

[Severity: High]
Does this trigger a one-shot phase shift instead of continuous drift correction?

The ptp daemon expects adjfine to configure a continuous hardware clock rate
adjustment. Since this block triggers a correction and polls for completion,
does the hardware just return to its nominal rate afterwards?

Will the ptp synchronization servo fail to stabilize if the frequency is not
actually adjusted continuously?

> +
> +static int qcom_tsc_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	struct qcom_tsc *tsc = container_of(ptp, struct qcom_tsc, ptp_info);
> +	int ret;
> +
> +	if (scaled_ppm == 0)
> +		return 0;

[Severity: High]
Does an early exit on zero scaled_ppm leave the hardware at its previous rate?

When the ptp daemon attempts to clear adjustments by passing 0, returning
early prevents any hardware commands from halting ongoing adjustments. Should
this reset the hardware to its nominal frequency before returning?

> +
> +	mutex_lock(&tsc->lock);
> +	ret = qcom_tsc_drift_correction(tsc, scaled_ppm);
> +	mutex_unlock(&tsc->lock);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-tscss-v1-0-beed14e72510@oss.qualcomm.com?part=3