Re: [PATCH net-next v2 2/2] net: stmmac: Add a devlink attribute to control timestamping mode

[Jakub Kicinski <kuba@kernel.org>]

Sorry didn't get to review this in time.

On Fri, 24 Oct 2025 09:07:18 +0200 Maxime Chevallier wrote:
> +   * - ``ts_coarse``

This is not a great name IMHO. Is "coarse" from the PRM?
It's the increment that's coarse, right? Not the timestamp
This naming confuses me greatly.

> +     - Boolean
> +     - runtime
> +     - Enable the Coarse timestamping mode. In Coarse mode, the ptp clock is
> +       expected to be updated through an external PPS input, but the subsecond

I guess the definition of "PPS input" got diluted but technically it
means Pulse Per Second, right? Here IIUC we need an actual 50MHz clock
fed in?

> +       increment used for timestamping is set to 1/ptp_clock_rate. In Fine mode
> +       (i.e. Coarse mode == false), the ptp clock frequency is adjusted more
> +       frequently, but the subsecond increment is set to 2/ptp_clock_rate.
> +       Coarse mode is suitable for PTP Grand Master operation. If unsure, leave
> +       the parameter to False.

My understanding based on your previous explanation is that basically
in one of the modes the frequency cannot be adjusted. It's only usable
if a very stable reference clock is fed into the device (or otherwise
we "trust" the clock that's fed in). So that's why Grand Master.

In the other mode we can tweak the frequency more accurately.
But it comes at a cost of the HW time incrementing 2x larger step.

If that's the case I think we should update the documentation and
rename the knob to indicate that it's the frequency adjustment that's
coarse.