[PATCH net-next 0/3] net: stmmac: Add support for coarse timestamping

[PATCH net-next 0/3] net: stmmac: Add support for coarse timestamping

[Maxime Chevallier]

Hello everyone,

This is another attempt to support the fine vs coarse timestamping modes
in stmmac.

This mode allows trading off PTP clock frequency adjustment precision
versus timestamping precision.

In coarse mode, we lose the ability to fine-tune the PTP clock
frequency, but get better timestamping precision instead. This is
especially useful when acting as a PTP Grand Master, where the PTP clock
in sync'd to a high-precision GPS clock through PPS inputs.

This has been submitted before as a dedicated ioctl() back in 2020 [1].
Since then, we now have a better representation of timestamp providers
with a dedicated qualifier (approx vs precise).

This series attempts to map these new qualifiers to stmmac's
timestamping modes, see patch 2 for details.

The main drawback IMO is that the qualifiers don't map very well to our
timestamping modes, as the "approx" qualifier actually maps to stmmac's
"coars" mode, but we actually gain in timestamping precision (while
losing frequency precision).

Patch 1 is prep work for the stmmac driver,
Patch 2 adds the mode adjustment. Most of the plumbing to compute addend
values and sub-second increment is already there in the driver.

Patch 3 makes sure our NDO for timestamping provider reconfiguration is
called upon changing the qualifier.

Let me kow what you think of this approach,

Maxime

[1] : https://lore.kernel.org/netdev/20200514102808.31163-1-olivier.dautricourt@orolia.com/

Maxime Chevallier (3):
  net: stmmac: Move subsecond increment configuration in dedicated
    helper
  net: stmmac: Allow supporting coarse adjustment mode
  net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 62 +++++++++++++------
 net/ethtool/tsconfig.c                        |  2 +-
 3 files changed, 45 insertions(+), 21 deletions(-)

-- 
2.49.0

[PATCH net-next 1/3] net: stmmac: Move subsecond increment configuration in dedicated helper

[Maxime Chevallier]

In preparation for fine/coarse support, let's move the subsecond increment
and addend configuration in a dedicated helper.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 48 +++++++++++--------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 650d75b73e0b..3f79b61d64b9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -463,6 +463,33 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
 	}
 }
 
+static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
+{
+	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
+	u32 sec_inc = 0;
+	u64 temp = 0;
+
+	stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
+
+	/* program Sub Second Increment reg */
+	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
+					   priv->plat->clk_ptp_rate,
+					   xmac, &sec_inc);
+	temp = div_u64(1000000000ULL, sec_inc);
+
+	/* Store sub second increment for later use */
+	priv->sub_second_inc = sec_inc;
+
+	/* calculate default added value:
+	 * formula is :
+	 * addend = (2^32)/freq_div_ratio;
+	 * where, freq_div_ratio = 1e9ns/sec_inc
+	 */
+	temp = (u64)(temp << 32);
+	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
+	stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
+}
+
 /**
  *  stmmac_hwtstamp_set - control hardware timestamping.
  *  @dev: device pointer.
@@ -696,10 +723,7 @@ static int stmmac_hwtstamp_get(struct net_device *dev,
 static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
 				      u32 systime_flags)
 {
-	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
 	struct timespec64 now;
-	u32 sec_inc = 0;
-	u64 temp = 0;
 
 	if (!priv->plat->clk_ptp_rate) {
 		netdev_err(priv->dev, "Invalid PTP clock rate");
@@ -709,23 +733,7 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
 	stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
 	priv->systime_flags = systime_flags;
 
-	/* program Sub Second Increment reg */
-	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
-					   priv->plat->clk_ptp_rate,
-					   xmac, &sec_inc);
-	temp = div_u64(1000000000ULL, sec_inc);
-
-	/* Store sub second increment for later use */
-	priv->sub_second_inc = sec_inc;
-
-	/* calculate default added value:
-	 * formula is :
-	 * addend = (2^32)/freq_div_ratio;
-	 * where, freq_div_ratio = 1e9ns/sec_inc
-	 */
-	temp = (u64)(temp << 32);
-	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
-	stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
+	stmmac_update_subsecond_increment(priv);
 
 	/* initialize system time */
 	ktime_get_real_ts64(&now);
-- 
2.49.0

Re: [PATCH net-next 1/3] net: stmmac: Move subsecond increment configuration in dedicated helper

[Russell King (Oracle)]

On Wed, Oct 15, 2025 at 12:27:21PM +0200, Maxime Chevallier wrote:
> +static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
> +{
> +	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;

Just to say that I have patches that get rid of these has_xxx flags for
the cores, and these changes (and the additional platform glue patches
that have been posted) will conflict with them.

Given the rate of change in stmmac, at some point we're going to have
to work out some way of stopping stmmac development to get such an
invasive cleanup change merged - but with my variability and pressures
on the time I can spend even submitting patches, I've no idea how that
will work... I was going to send them right at the start of this
cycle, but various appointments on Monday and Tuesday this week plus
work pressures prevented that happening.

So, I decided instead to send out the first stmmac PCS series... which
means I now need to wait for that to be merged before I can think about
sending out anything else stmmac-related. (and there's more PCS patches
to come beyond the 14 I sent today.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/3] net: stmmac: Move subsecond increment configuration in dedicated helper

[Maxime Chevallier]

Hi Russell,

On 15/10/2025 17:06, Russell King (Oracle) wrote:
> On Wed, Oct 15, 2025 at 12:27:21PM +0200, Maxime Chevallier wrote:
>> +static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
>> +{
>> +	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
> 
> Just to say that I have patches that get rid of these has_xxx flags for
> the cores, and these changes (and the additional platform glue patches
> that have been posted) will conflict with them.

Fair, I was in your position not so long ago :)

For this particular series, it should be straightforward to fix the
conflict, but for the pending new glue divers we'll have to
find the sweet spot for these changes.

Maybe send it as an RFC so that people can see what to expect ?

> Given the rate of change in stmmac, at some point we're going to have
> to work out some way of stopping stmmac development to get such an
> invasive cleanup change merged

Agreed.

 - but with my variability and pressures
> on the time I can spend even submitting patches, I've no idea how that
> will work... I was going to send them right at the start of this
> cycle, but various appointments on Monday and Tuesday this week plus
> work pressures prevented that happening.

To give your more visibility, that's the only work I plan to do on
stmmac for that cycle, the rest is going to be phy_port,
and probably some netdevsim-phy.

> So, I decided instead to send out the first stmmac PCS series... which
> means I now need to wait for that to be merged before I can think about
> sending out anything else stmmac-related. (and there's more PCS patches
> to come beyond the 14 I sent today.)

Do you plan to send the next round of PCS stuff next, or the cleanups
for the has_xxx flags you were mentioning ?

In any case, I'll be happy to help testing :)

Maxime

Re: [PATCH net-next 1/3] net: stmmac: Move subsecond increment configuration in dedicated helper

[Russell King (Oracle)]

On Wed, Oct 15, 2025 at 06:20:03PM +0200, Maxime Chevallier wrote:
> Hi Russell,
> 
> On 15/10/2025 17:06, Russell King (Oracle) wrote:
> > On Wed, Oct 15, 2025 at 12:27:21PM +0200, Maxime Chevallier wrote:
> >> +static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
> >> +{
> >> +	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
> > 
> > Just to say that I have patches that get rid of these has_xxx flags for
> > the cores, and these changes (and the additional platform glue patches
> > that have been posted) will conflict with them.
> 
> Fair, I was in your position not so long ago :)
> 
> For this particular series, it should be straightforward to fix the
> conflict, but for the pending new glue divers we'll have to
> find the sweet spot for these changes.
> 
> Maybe send it as an RFC so that people can see what to expect ?

My experience of sending RFCs is the engagement is sadly low to non-
existent, leading to the question of whether sending RFCs has any
use at all. I'm pretty fed up with the whole mainline kernel process,
trying to get engagement from people, that I question why I bother
most of the time.

> > Given the rate of change in stmmac, at some point we're going to have
> > to work out some way of stopping stmmac development to get such an
> > invasive cleanup change merged
> 
> Agreed.
> 
>  - but with my variability and pressures
> > on the time I can spend even submitting patches, I've no idea how that
> > will work... I was going to send them right at the start of this
> > cycle, but various appointments on Monday and Tuesday this week plus
> > work pressures prevented that happening.
> 
> To give your more visibility, that's the only work I plan to do on
> stmmac for that cycle, the rest is going to be phy_port,
> and probably some netdevsim-phy.

I'd prefer that my five patch series I've just sent out is merged
(the patches I'm talking about w.r.t. has_xxx follow immediately
after these in my queue.) However, I don't think there's any
conflicts between the five I've sent out and these changes looking
at their overall diffs. That said, pushing out loads of patches in
one day isn't helpful to the already overworked reviewers.

> Do you plan to send the next round of PCS stuff next, or the cleanups
> for the has_xxx flags you were mentioning ?

It depends whether there's any conflicting changes. I have such a
backlog that I'm trying to send out as many non-conflicting netdev
related topics as I can to get the maximum merged when I have the
opportunity, even if the topics end up touching the same files.
I just totalled up the backlog - it's around 120 stmmac related
patches (including adding the phylink core WoL support), plus about
20 for marvell PTP. Eek! :/

When one has so many patches, "what to send out next" becomes a major
decision, and really depends on previous patch set progress.

As you explicitly ask about the PCS stuff and cleanups, I've sent
the first batch (which is the bulk) of PCS stuff today, and non-
overlapping cleanup changes.  If the cleanup changes go in, then
the next tranch of cleanups will have the has_xxx change in.

If the PCS changes go in, then I'll send out the next two patches
which move the PCS interrupt control over to phylink. After that
comes the potentially regression inducing change - making stmmac's
PCS follow what phylink requests of it. I'm expecting that to cause
trouble, but I have no confidence that it'll get enough testing to
uncover issues before being merged. This change really needs testing
on those platforms that use the DWMAC integrated PCS (not xpcs).

If all goes well with the patches I've sent today, then I'm expecting
them to be merged over this coming weekend. That means the next patches
will be sent early next week.

So... if your changes can be merged before or around the time that my
5-patch cleanup gets merged, I can rebase my changes on top, but if
your patch set needs re-posting, I suggest we have another discussion
how we proceed.

As mentioned, I'm aware that there are other patches that have already
been submitted that add platform glue that reference the has_xxx stuff,
so if these get merged, a rebase is going to be required. (annoyingly,
this will be high-risk because of getting the compile coverage to
catch every reference.. unless I remember to grep for them after
rebasing.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Maxime Chevallier]

The DWMAC1000 supports 2 timestamping configurations to configure how
frequency adjustments are made to the ptp_clock, as well as the reported
timestamp values.

There was a previous attempt at upstreaming support for configuring this
mode by Olivier Dautricourt and Julien Beraud a few years back [1]

In a nutshell, the timestamping can be either set in fine mode or in
coarse mode.

In fine mode, which is the default, we use the overflow of an accumulator to
trigger frequency adjustments, but by doing so we lose precision on the
timetamps that are produced by the timestamping unit. The main drawback
is that the sub-second increment value, used to generate timestamps, can't be
set to lower than (2 / ptp_clock_freq).

The "fine" qualification comes from the frequent frequency adjustments we are
able to do, which is perfect for a PTP follower usecase.

In Coarse mode, we don't do frequency adjustments based on an
accumulator overflow. We can therefore have very fine subsecond
increment values, allowing for better timestamping precision. However
this mode works best when the ptp clock frequency is adjusted based on
an external signal, such as a PPS input produced by a GPS clock. This
mode is therefore perfect for a Grand-master usecase.

We therefore attempt to map these 2 modes with the newly introduced
hwtimestamp qualifiers (precise and approx).

Precise mode is mapped to stmmac fine mode, and is the expected default,
suitable for all cases and perfect for follower mode

Approx mode is mapped to coarse mode, suitable for Grand-master.

Changing between these modes is done using ethtool :

 - Fine mode

   ethtool --set-hwtimestamp-cfg eth0 index 0 qualifier precise

 - Coarse mode

   ethtool --set-hwtimestamp-cfg eth0 index 0 qualifier approx

[1] : https://lore.kernel.org/netdev/20200514102808.31163-1-olivier.dautricourt@orolia.com/

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |  2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 39fa1ec92f82..0594acbc0ead 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -1192,6 +1192,8 @@ static void stmmac_get_mm_stats(struct net_device *ndev,
 static const struct ethtool_ops stmmac_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
+	.supported_hwtstamp_qualifiers = BIT(HWTSTAMP_PROVIDER_QUALIFIER_PRECISE) |
+					 BIT(HWTSTAMP_PROVIDER_QUALIFIER_APPROX),
 	.get_drvinfo = stmmac_ethtool_getdrvinfo,
 	.get_msglevel = stmmac_ethtool_getmsglevel,
 	.set_msglevel = stmmac_ethtool_setmsglevel,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3f79b61d64b9..4859aba10aa3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -675,6 +675,14 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
 
 	priv->systime_flags = STMMAC_HWTS_ACTIVE;
 
+	/* This is the "coarse" mode, where we get lower frequency adjustment
+	 * precision, but better timestamping precision. This is useful when
+	 * acting as a grand-master, as we usually sync with a hgh-previcision
+	 * clock through PPS input. We default to "fine" mode.
+	 */
+	if (config->qualifier == HWTSTAMP_PROVIDER_QUALIFIER_APPROX)
+		priv->systime_flags &= ~PTP_TCR_TSCFUPDT;
+
 	if (priv->hwts_tx_en || priv->hwts_rx_en) {
 		priv->systime_flags |= tstamp_all | ptp_v2 |
 				       ptp_over_ethernet | ptp_over_ipv6_udp |
@@ -684,6 +692,12 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
 
 	stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
 
+	/* Switching between coarse/fine mode also requires updating the
+	 * subsecond increment
+	 */
+	if (priv->plat->clk_ptp_rate)
+		stmmac_update_subsecond_increment(priv);
+
 	priv->tstamp_config = *config;
 
 	return 0;
-- 
2.49.0

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Jakub Kicinski]

On Wed, 15 Oct 2025 12:27:22 +0200 Maxime Chevallier wrote:
> The DWMAC1000 supports 2 timestamping configurations to configure how
> frequency adjustments are made to the ptp_clock, as well as the reported
> timestamp values.
> 
> There was a previous attempt at upstreaming support for configuring this
> mode by Olivier Dautricourt and Julien Beraud a few years back [1]
> 
> In a nutshell, the timestamping can be either set in fine mode or in
> coarse mode.
> 
> In fine mode, which is the default, we use the overflow of an accumulator to
> trigger frequency adjustments, but by doing so we lose precision on the
> timetamps that are produced by the timestamping unit. The main drawback
> is that the sub-second increment value, used to generate timestamps, can't be
> set to lower than (2 / ptp_clock_freq).
> 
> The "fine" qualification comes from the frequent frequency adjustments we are
> able to do, which is perfect for a PTP follower usecase.
> 
> In Coarse mode, we don't do frequency adjustments based on an
> accumulator overflow. We can therefore have very fine subsecond
> increment values, allowing for better timestamping precision. However
> this mode works best when the ptp clock frequency is adjusted based on
> an external signal, such as a PPS input produced by a GPS clock. This
> mode is therefore perfect for a Grand-master usecase.
> 
> We therefore attempt to map these 2 modes with the newly introduced
> hwtimestamp qualifiers (precise and approx).
> 
> Precise mode is mapped to stmmac fine mode, and is the expected default,
> suitable for all cases and perfect for follower mode
> 
> Approx mode is mapped to coarse mode, suitable for Grand-master.

I failed to understand what this device does and what the problem is :(

What is your ptp_clock_freq? Isn't it around 50MHz typically? 
So 2 / ptp_freq is 40nsec (?), not too bad?

My recollection of the idea behind that timestamping providers
was that you can configure different filters for different providers.
IOW that you'd be able to say:
 - [precise] Rx stamp PTP packets 
 -  [approx] Rx stamp all packets
not that you'd configure precision of one piece of HW..

If the HW really needs it, just lob a devlink param at it?

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Maxime Chevallier]

Hi Jakub,

On 18/10/2025 03:23, Jakub Kicinski wrote:
> On Wed, 15 Oct 2025 12:27:22 +0200 Maxime Chevallier wrote:
>> The DWMAC1000 supports 2 timestamping configurations to configure how
>> frequency adjustments are made to the ptp_clock, as well as the reported
>> timestamp values.
>>
>> There was a previous attempt at upstreaming support for configuring this
>> mode by Olivier Dautricourt and Julien Beraud a few years back [1]
>>
>> In a nutshell, the timestamping can be either set in fine mode or in
>> coarse mode.
>>
>> In fine mode, which is the default, we use the overflow of an accumulator to
>> trigger frequency adjustments, but by doing so we lose precision on the
>> timetamps that are produced by the timestamping unit. The main drawback
>> is that the sub-second increment value, used to generate timestamps, can't be
>> set to lower than (2 / ptp_clock_freq).
>>
>> The "fine" qualification comes from the frequent frequency adjustments we are
>> able to do, which is perfect for a PTP follower usecase.
>>
>> In Coarse mode, we don't do frequency adjustments based on an
>> accumulator overflow. We can therefore have very fine subsecond
>> increment values, allowing for better timestamping precision. However
>> this mode works best when the ptp clock frequency is adjusted based on
>> an external signal, such as a PPS input produced by a GPS clock. This
>> mode is therefore perfect for a Grand-master usecase.
>>
>> We therefore attempt to map these 2 modes with the newly introduced
>> hwtimestamp qualifiers (precise and approx).
>>
>> Precise mode is mapped to stmmac fine mode, and is the expected default,
>> suitable for all cases and perfect for follower mode
>>
>> Approx mode is mapped to coarse mode, suitable for Grand-master.
> 
> I failed to understand what this device does and what the problem is :(
> 
> What is your ptp_clock_freq? Isn't it around 50MHz typically? 
> So 2 / ptp_freq is 40nsec (?), not too bad?

That's not too bad indeed, but it makes a difference when acting as
Grand Master, especially in this case because you don't need to
perform clock adjustments (it's sync'd through PPS in), so we might
as well take this opportunity to improve the TS.

> 
> My recollection of the idea behind that timestamping providers
> was that you can configure different filters for different providers.
> IOW that you'd be able to say:
>  - [precise] Rx stamp PTP packets 
>  -  [approx] Rx stamp all packets
> not that you'd configure precision of one piece of HW..

So far it looks like only one provider is enabled at a given time, my
understanding was that the qualifier would be used in case there
are multiple timestampers on the data path, to select the better one
(e.g. a PHY that supports TS, a MAC that supports TS, we use the 
best out of the two).

However I agree with your comments, that's exactly the kind of feedback
I was looking for. This work has been tried several times now each
time with a different uAPI path, I'm OK to consider that this is out
of the scope of the hwprov feature.

> If the HW really needs it, just lob a devlink param at it?

I'm totally OK with that. I'm not well versed into devlink, working mostly with
embedded devices with simple-ish NICs, most of them don't use devlink. Let me
give it a try then :)

Thanks for taking a look at this,

Maxime

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Kory Maincent]

On Sat, 18 Oct 2025 09:42:57 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi Jakub,
> 
> On 18/10/2025 03:23, Jakub Kicinski wrote:
> > On Wed, 15 Oct 2025 12:27:22 +0200 Maxime Chevallier wrote:  
> >> The DWMAC1000 supports 2 timestamping configurations to configure how
> >> frequency adjustments are made to the ptp_clock, as well as the reported
> >> timestamp values.
> >>
> >> There was a previous attempt at upstreaming support for configuring this
> >> mode by Olivier Dautricourt and Julien Beraud a few years back [1]
> >>
> >> In a nutshell, the timestamping can be either set in fine mode or in
> >> coarse mode.
> >>
> >> In fine mode, which is the default, we use the overflow of an accumulator
> >> to trigger frequency adjustments, but by doing so we lose precision on the
> >> timetamps that are produced by the timestamping unit. The main drawback
> >> is that the sub-second increment value, used to generate timestamps, can't
> >> be set to lower than (2 / ptp_clock_freq).
> >>
> >> The "fine" qualification comes from the frequent frequency adjustments we
> >> are able to do, which is perfect for a PTP follower usecase.
> >>
> >> In Coarse mode, we don't do frequency adjustments based on an
> >> accumulator overflow. We can therefore have very fine subsecond
> >> increment values, allowing for better timestamping precision. However
> >> this mode works best when the ptp clock frequency is adjusted based on
> >> an external signal, such as a PPS input produced by a GPS clock. This
> >> mode is therefore perfect for a Grand-master usecase.
> >>
> >> We therefore attempt to map these 2 modes with the newly introduced
> >> hwtimestamp qualifiers (precise and approx).
> >>
> >> Precise mode is mapped to stmmac fine mode, and is the expected default,
> >> suitable for all cases and perfect for follower mode
> >>
> >> Approx mode is mapped to coarse mode, suitable for Grand-master.  
> > 
> > I failed to understand what this device does and what the problem is :(
> > 
> > What is your ptp_clock_freq? Isn't it around 50MHz typically? 
> > So 2 / ptp_freq is 40nsec (?), not too bad?  
> 
> That's not too bad indeed, but it makes a difference when acting as
> Grand Master, especially in this case because you don't need to
> perform clock adjustments (it's sync'd through PPS in), so we might
> as well take this opportunity to improve the TS.
> 
> > 
> > My recollection of the idea behind that timestamping providers
> > was that you can configure different filters for different providers.
> > IOW that you'd be able to say:
> >  - [precise] Rx stamp PTP packets 
> >  -  [approx] Rx stamp all packets
> > not that you'd configure precision of one piece of HW..  
> 
> So far it looks like only one provider is enabled at a given time, my
> understanding was that the qualifier would be used in case there
> are multiple timestampers on the data path, to select the better one
> (e.g. a PHY that supports TS, a MAC that supports TS, we use the 
> best out of the two).

No, we do not support multiple timestampers at the same time.
For that IIUC we would have to add a an ID of the source in the packet. I
remember people were talking about modifying cmsg. 
This qualifier is indeed a first step to walk this path but I don't think
people are currently working on adding this support for now. 

> However I agree with your comments, that's exactly the kind of feedback
> I was looking for. This work has been tried several times now each
> time with a different uAPI path, I'm OK to consider that this is out
> of the scope of the hwprov feature.
> 
> > If the HW really needs it, just lob a devlink param at it?  
> 
> I'm totally OK with that. I'm not well versed into devlink, working mostly
> with embedded devices with simple-ish NICs, most of them don't use devlink.
> Let me give it a try then :)

meh, I kind of dislike using devlink here. As I said using timestamping
qualifier is a fist step for the multiple timestamping support. If one day we
will add this support, if there is other implementation it will add burden on
the development to track and change all the other implementation. Why don't we
always use this qualifier parameter even if it is not really for simultaneous
timestamping to avoid any future wrong development choice.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Maxime Chevallier]

Hi Köry,

On 20/10/2025 11:00, Kory Maincent wrote:
> On Sat, 18 Oct 2025 09:42:57 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
>> Hi Jakub,
>>
>> On 18/10/2025 03:23, Jakub Kicinski wrote:
>>> On Wed, 15 Oct 2025 12:27:22 +0200 Maxime Chevallier wrote:  
>>>> The DWMAC1000 supports 2 timestamping configurations to configure how
>>>> frequency adjustments are made to the ptp_clock, as well as the reported
>>>> timestamp values.
>>>>
>>>> There was a previous attempt at upstreaming support for configuring this
>>>> mode by Olivier Dautricourt and Julien Beraud a few years back [1]
>>>>
>>>> In a nutshell, the timestamping can be either set in fine mode or in
>>>> coarse mode.
>>>>
>>>> In fine mode, which is the default, we use the overflow of an accumulator
>>>> to trigger frequency adjustments, but by doing so we lose precision on the
>>>> timetamps that are produced by the timestamping unit. The main drawback
>>>> is that the sub-second increment value, used to generate timestamps, can't
>>>> be set to lower than (2 / ptp_clock_freq).
>>>>
>>>> The "fine" qualification comes from the frequent frequency adjustments we
>>>> are able to do, which is perfect for a PTP follower usecase.
>>>>
>>>> In Coarse mode, we don't do frequency adjustments based on an
>>>> accumulator overflow. We can therefore have very fine subsecond
>>>> increment values, allowing for better timestamping precision. However
>>>> this mode works best when the ptp clock frequency is adjusted based on
>>>> an external signal, such as a PPS input produced by a GPS clock. This
>>>> mode is therefore perfect for a Grand-master usecase.
>>>>
>>>> We therefore attempt to map these 2 modes with the newly introduced
>>>> hwtimestamp qualifiers (precise and approx).
>>>>
>>>> Precise mode is mapped to stmmac fine mode, and is the expected default,
>>>> suitable for all cases and perfect for follower mode
>>>>
>>>> Approx mode is mapped to coarse mode, suitable for Grand-master.  
>>>
>>> I failed to understand what this device does and what the problem is :(
>>>
>>> What is your ptp_clock_freq? Isn't it around 50MHz typically? 
>>> So 2 / ptp_freq is 40nsec (?), not too bad?  
>>
>> That's not too bad indeed, but it makes a difference when acting as
>> Grand Master, especially in this case because you don't need to
>> perform clock adjustments (it's sync'd through PPS in), so we might
>> as well take this opportunity to improve the TS.
>>
>>>
>>> My recollection of the idea behind that timestamping providers
>>> was that you can configure different filters for different providers.
>>> IOW that you'd be able to say:
>>>  - [precise] Rx stamp PTP packets 
>>>  -  [approx] Rx stamp all packets
>>> not that you'd configure precision of one piece of HW..  
>>
>> So far it looks like only one provider is enabled at a given time, my
>> understanding was that the qualifier would be used in case there
>> are multiple timestampers on the data path, to select the better one
>> (e.g. a PHY that supports TS, a MAC that supports TS, we use the 
>> best out of the two).
> 
> No, we do not support multiple timestampers at the same time.
> For that IIUC we would have to add a an ID of the source in the packet. I
> remember people were talking about modifying cmsg. 
> This qualifier is indeed a first step to walk this path but I don't think
> people are currently working on adding this support for now. 
> 
>> However I agree with your comments, that's exactly the kind of feedback
>> I was looking for. This work has been tried several times now each
>> time with a different uAPI path, I'm OK to consider that this is out
>> of the scope of the hwprov feature.
>>
>>> If the HW really needs it, just lob a devlink param at it?  
>>
>> I'm totally OK with that. I'm not well versed into devlink, working mostly
>> with embedded devices with simple-ish NICs, most of them don't use devlink.
>> Let me give it a try then :)
> 
> meh, I kind of dislike using devlink here. As I said using timestamping
> qualifier is a fist step for the multiple timestamping support. If one day we
> will add this support, if there is other implementation it will add burden on
> the development to track and change all the other implementation. Why don't we
> always use this qualifier parameter even if it is not really for simultaneous
> timestamping to avoid any future wrong development choice.

On my side I've implemented the devlink-based approach, and I have to say i'm not
so unhappy with it :) At least I don't have the feeling this is bending
the API to fit one specific case.

The thing is that the qualifier model doesn't fully map to the situation we
have in stmmac.

The stmmac coarse/fine adjustment doesn't only changes the timestamping
behaviour, but also the ptp_clock adjustment mode. 

So changing the qualifier here will have a side effect on the PTP clock,
do we accept that as part of the hwprov timestamping API ?

Should we use this API for coarse/fine stmmac config, I agree with your
previous comment of adding a dedicated qualifier that explicitely says
that using this qualifier comes with side effects, with the risk of
paving the way for lots of modes being added for driver-specific scenarios.

Another thing with the stmmac implem is that we don't truly have 2
timestampers (1 approx and 1 precise), but rather only one whose precision
can be adjusted. Does it really make sense here to have the qualifier
writeable for the same timestamper ?

Of course the netlink tsinfo/tsconfig is more appealing due to its generic
nature, but OTHO I don't want to introduce ill-defined behaviours in that
API with this series. The multiple timestamper work still makes a ton of
sense for MAC+PHY timestamping setups :)

I'm opened for both approached TBH :)

Maxime

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Kory Maincent]

On Mon, 20 Oct 2025 11:32:37 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi Köry,
> 
> On 20/10/2025 11:00, Kory Maincent wrote:
> > On Sat, 18 Oct 2025 09:42:57 +0200
> > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> >   
> >> Hi Jakub,
> >>
> >> On 18/10/2025 03:23, Jakub Kicinski wrote:  
> >>> On Wed, 15 Oct 2025 12:27:22 +0200 Maxime Chevallier wrote:    
> >>>> The DWMAC1000 supports 2 timestamping configurations to configure how
> >>>> frequency adjustments are made to the ptp_clock, as well as the reported
> >>>> timestamp values.
> >>>>
> >>>> There was a previous attempt at upstreaming support for configuring this
> >>>> mode by Olivier Dautricourt and Julien Beraud a few years back [1]
> >>>>
> >>>> In a nutshell, the timestamping can be either set in fine mode or in
> >>>> coarse mode.
> >>>>
> >>>> In fine mode, which is the default, we use the overflow of an accumulator
> >>>> to trigger frequency adjustments, but by doing so we lose precision on
> >>>> the timetamps that are produced by the timestamping unit. The main
> >>>> drawback is that the sub-second increment value, used to generate
> >>>> timestamps, can't be set to lower than (2 / ptp_clock_freq).
> >>>>
> >>>> The "fine" qualification comes from the frequent frequency adjustments we
> >>>> are able to do, which is perfect for a PTP follower usecase.
> >>>>
> >>>> In Coarse mode, we don't do frequency adjustments based on an
> >>>> accumulator overflow. We can therefore have very fine subsecond
> >>>> increment values, allowing for better timestamping precision. However
> >>>> this mode works best when the ptp clock frequency is adjusted based on
> >>>> an external signal, such as a PPS input produced by a GPS clock. This
> >>>> mode is therefore perfect for a Grand-master usecase.
> >>>>
> >>>> We therefore attempt to map these 2 modes with the newly introduced
> >>>> hwtimestamp qualifiers (precise and approx).
> >>>>
> >>>> Precise mode is mapped to stmmac fine mode, and is the expected default,
> >>>> suitable for all cases and perfect for follower mode
> >>>>
> >>>> Approx mode is mapped to coarse mode, suitable for Grand-master.    
> >>>
> >>> I failed to understand what this device does and what the problem is :(
> >>>
> >>> What is your ptp_clock_freq? Isn't it around 50MHz typically? 
> >>> So 2 / ptp_freq is 40nsec (?), not too bad?    
> >>
> >> That's not too bad indeed, but it makes a difference when acting as
> >> Grand Master, especially in this case because you don't need to
> >> perform clock adjustments (it's sync'd through PPS in), so we might
> >> as well take this opportunity to improve the TS.
> >>  
> >>>
> >>> My recollection of the idea behind that timestamping providers
> >>> was that you can configure different filters for different providers.
> >>> IOW that you'd be able to say:
> >>>  - [precise] Rx stamp PTP packets 
> >>>  -  [approx] Rx stamp all packets
> >>> not that you'd configure precision of one piece of HW..    
> >>
> >> So far it looks like only one provider is enabled at a given time, my
> >> understanding was that the qualifier would be used in case there
> >> are multiple timestampers on the data path, to select the better one
> >> (e.g. a PHY that supports TS, a MAC that supports TS, we use the 
> >> best out of the two).  
> > 
> > No, we do not support multiple timestampers at the same time.
> > For that IIUC we would have to add a an ID of the source in the packet. I
> > remember people were talking about modifying cmsg. 
> > This qualifier is indeed a first step to walk this path but I don't think
> > people are currently working on adding this support for now. 
> >   
> >> However I agree with your comments, that's exactly the kind of feedback
> >> I was looking for. This work has been tried several times now each
> >> time with a different uAPI path, I'm OK to consider that this is out
> >> of the scope of the hwprov feature.
> >>  
> >>> If the HW really needs it, just lob a devlink param at it?    
> >>
> >> I'm totally OK with that. I'm not well versed into devlink, working mostly
> >> with embedded devices with simple-ish NICs, most of them don't use devlink.
> >> Let me give it a try then :)  
> > 
> > meh, I kind of dislike using devlink here. As I said using timestamping
> > qualifier is a fist step for the multiple timestamping support. If one day
> > we will add this support, if there is other implementation it will add
> > burden on the development to track and change all the other implementation.
> > Why don't we always use this qualifier parameter even if it is not really
> > for simultaneous timestamping to avoid any future wrong development choice.
> >  
> 
> On my side I've implemented the devlink-based approach, and I have to say i'm
> not so unhappy with it :) At least I don't have the feeling this is bending
> the API to fit one specific case.

Indeed I don't think so, but my idea was to generalize the selection of
the timestamp provider source to one API even if it is only one clock for two
different qualifiers.
 
> The thing is that the qualifier model doesn't fully map to the situation we
> have in stmmac.
> 
> The stmmac coarse/fine adjustment doesn't only changes the timestamping
> behaviour, but also the ptp_clock adjustment mode. 
> 
> So changing the qualifier here will have a side effect on the PTP clock,
> do we accept that as part of the hwprov timestamping API ?

Yes, I see the timestamp source as a couple of a qualifier plus a PTP
clock index therefore if we change the timestamp source it is intended to have
side effect.

> Should we use this API for coarse/fine stmmac config, I agree with your
> previous comment of adding a dedicated qualifier that explicitely says
> that using this qualifier comes with side effects, with the risk of
> paving the way for lots of modes being added for driver-specific scenarios.

I am not really a PTP in the field user but maybe there is a limited number of
generic qualifier possible. Here we could have a qualifier for better frequency
precision and one for better timestamping precision. I don't think we will end
with tons of different qualifiers.
Maybe PTP maintainers and users like Richard or Willem have pointers on the
number of possible qualifier?

> Another thing with the stmmac implem is that we don't truly have 2
> timestampers (1 approx and 1 precise), but rather only one whose precision
> can be adjusted. Does it really make sense here to have the qualifier
> writeable for the same timestamper ?

I do think so.

> Of course the netlink tsinfo/tsconfig is more appealing due to its generic
> nature, but OTHO I don't want to introduce ill-defined behaviours in that
> API with this series. The multiple timestamper work still makes a ton of
> sense for MAC+PHY timestamping setups :)

I think that is where it would be nice to have a review from Richard or
Willem on this to give us pointers on what is existing in the PTP world and if
using a qualifier makes sense.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Jakub Kicinski]

On Sat, 18 Oct 2025 09:42:57 +0200 Maxime Chevallier wrote:
> > If the HW really needs it, just lob a devlink param at it?  
> 
> I'm totally OK with that. I'm not well versed into devlink, working mostly with
> embedded devices with simple-ish NICs, most of them don't use devlink. Let me
> give it a try then :)
> 
> Thanks for taking a look at this,

FWIW I dropped this form PW in an attempt to unblock testing of
Russell's series. I'm not convinced that the tsconfig API is correct
here but I don't get how the HW works. Could you perhaps put together
some pseudocode?

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Maxime Chevallier]

Hi Jakub,

On 21/10/2025 03:03, Jakub Kicinski wrote:
> On Sat, 18 Oct 2025 09:42:57 +0200 Maxime Chevallier wrote:
>>> If the HW really needs it, just lob a devlink param at it?  
>>
>> I'm totally OK with that. I'm not well versed into devlink, working mostly with
>> embedded devices with simple-ish NICs, most of them don't use devlink. Let me
>> give it a try then :)
>>
>> Thanks for taking a look at this,
> 
> FWIW I dropped this form PW in an attempt to unblock testing of
> Russell's series.

Yup no problem for me

> I'm not convinced that the tsconfig API is correct
> here but I don't get how the HW works. Could you perhaps put together
> some pseudocode?

I'm not fully convinced either, devlink could also work. I already have 
a patchset ready that uses devlink for that.

Let's see if I can do a better job explaining this timestamping
parameter :


- In "fine" mode (mapped to precise qualifier if we go with the
tsinfo approach), which is the mode currently used in stmmac :

  u32 accumulator
  u32 addend
  u64 subsecond_increment

  at each ref ptp clock cycle {
    
    accumulator += addend;

    /* accumulator and addend are 32 bits. addend is adjusted
     * to control the drift of the target frequency.
     */
    if (accumulator overflows)
      current_time += subsecond_increment

  }

Main advantage: We can fine tune when we update the current_time, so
basically we can multiply or divide the ref ptp clock based on when
the accumulator overflows, and use the resulting clock as the
timestamp source.

In practice for stmmac, we set the addend to a value so that it takes
2 cycles to overflow, and we adjust the subsecond increment when
updating the PHC frequency. This gives a precision of 46-ish ns.

We do this by design as the ptp ref clk may be fairly slow (down to
5MHz in some cases). If we try to rely on accumulator overflowing at
every cycle to update the time, we'd have to overflow multiple times
per cycle, and the computation of the addend value simply overflows,
cf commit 91a2559 ("net: stmmac: Fix sub-second increment") [1]

This is basic PTP follower mode, we continuously update
the clock used for timestamping.


- In "coarse" mode :

  u32 subsecond_increment

  at each ref ptp clock cycle {
    current_time += subecond_increment
  }

We increment time at a fixed rate. The current_time can only be adjusted
by setting the entire current_time, which is jittery or even non-monotonically
increasing. However, we can use a 20ns increment (or even lower if the ref
clock is faster), and if our ref clock is externally provided by a high
precision source, this allows implementing Grand Master mode with the
highest possible precision.

[1] : https://lore.kernel.org/netdev/20200415122432.70972-2-julien.beraud@orolia.com/

Hopefully that's a bit clearer WRT what the HW can do.

Let me know if you need more clarifications on this, I'll send another
iteration once Russell's cleanup lands, using either devlink or tsconfig
depending on what we chose here.

Maxime

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Jakub Kicinski]

On Tue, 21 Oct 2025 10:02:01 +0200 Maxime Chevallier wrote:
> Let me know if you need more clarifications on this

The explanation was excellent, thank you. I wonder why it's designed
in such an odd way, instead of just having current_time with some
extra/fractional bits not visible in the timestamp. Sigh.

In any case, I don't feel strongly but it definitely seems to me like
the crucial distinction here is not the precision of the timestamp but
whether the user intends to dial the frequency.

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Maxime Chevallier]

On 22/10/2025 01:02, Jakub Kicinski wrote:
> On Tue, 21 Oct 2025 10:02:01 +0200 Maxime Chevallier wrote:
>> Let me know if you need more clarifications on this
> 
> The explanation was excellent, thank you. I wonder why it's designed
> in such an odd way, instead of just having current_time with some
> extra/fractional bits not visible in the timestamp. Sigh.
> 
> In any case, I don't feel strongly but it definitely seems to me like
> the crucial distinction here is not the precision of the timestamp but
> whether the user intends to dial the frequency.

Yes indeed. I don't have a clear view on wether this is something unique
to stmmac or if this is common enough to justify using the tsconfig API.

As we discuss this, I would tend to think devlink is the way, as this
all boils down to how this particular HW works. Moreover, if we use a
dedicated hwprov qualifier, where do we make it sit in the current
hierarchy (precise > approx) that's used for the TS source selection ?

Maxime

Re: [PATCH net-next 2/3] net: stmmac: Allow supporting coarse adjustment mode

[Kory Maincent]

On Thu, 23 Oct 2025 10:29:26 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> On 22/10/2025 01:02, Jakub Kicinski wrote:
> > On Tue, 21 Oct 2025 10:02:01 +0200 Maxime Chevallier wrote:  
> >> Let me know if you need more clarifications on this  
> > 
> > The explanation was excellent, thank you. I wonder why it's designed
> > in such an odd way, instead of just having current_time with some
> > extra/fractional bits not visible in the timestamp. Sigh.
> > 
> > In any case, I don't feel strongly but it definitely seems to me like
> > the crucial distinction here is not the precision of the timestamp but
> > whether the user intends to dial the frequency.  
> 
> Yes indeed. I don't have a clear view on wether this is something unique
> to stmmac or if this is common enough to justify using the tsconfig API.
> 
> As we discuss this, I would tend to think devlink is the way, as this
> all boils down to how this particular HW works. Moreover, if we use a
> dedicated hwprov qualifier, where do we make it sit in the current
> hierarchy (precise > approx) that's used for the TS source selection ?

That's ok to me. I was not strongly against devlink in either way, and I didn't
have real arguments. Let's go for devlink, we still can move it to tsconfig API
later if it's needed. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 3/3] net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

[Maxime Chevallier]

When a hwprov timestamping source is changed, but without updating the
timestamping parameters, we may want to reconfigure the timestamping
source to enable the new provider.

This is especially important if the same HW unit implements 2 providers,
a precise and an approx one. In this case, we need to make sure we call
the hwtstamp_set operation for the newly selected provider.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/tsconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index 169b413b31fc..e8333452926d 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -416,7 +416,7 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 			kfree_rcu(__hwprov, rcu_head);
 	}
 
-	if (config_mod) {
+	if (config_mod || hwprov_mod) {
 		ret = dev_set_hwtstamp_phylib(dev, &hwtst_config,
 					      info->extack);
 		if (ret < 0)
-- 
2.49.0

Re: [PATCH net-next 3/3] net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

[Kory Maincent]

On Wed, 15 Oct 2025 12:27:23 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> When a hwprov timestamping source is changed, but without updating the
> timestamping parameters, we may want to reconfigure the timestamping
> source to enable the new provider.
> 
> This is especially important if the same HW unit implements 2 providers,
> a precise and an approx one. In this case, we need to make sure we call
> the hwtstamp_set operation for the newly selected provider.

This is a design choice.
Do we want to preserve the hwtstamp config if only the hwtstamp source is
changed from ethtool?
If we want to configure the new source to the old source config we will also
need to remove this condition:
https://elixir.bootlin.com/linux/v6.17.1/source/net/ethtool/tsconfig.c#L339 

I do not really have a strong opinion on this, let's discuss which behavior we
prefer.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/3] net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

[Maxime Chevallier]

Hi Köry,

On 15/10/2025 14:45, Kory Maincent wrote:
> On Wed, 15 Oct 2025 12:27:23 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
>> When a hwprov timestamping source is changed, but without updating the
>> timestamping parameters, we may want to reconfigure the timestamping
>> source to enable the new provider.
>>
>> This is especially important if the same HW unit implements 2 providers,
>> a precise and an approx one. In this case, we need to make sure we call
>> the hwtstamp_set operation for the newly selected provider.
> 
> This is a design choice.
> Do we want to preserve the hwtstamp config if only the hwtstamp source is
> changed from ethtool?
> If we want to configure the new source to the old source config we will also
> need to remove this condition:
> https://elixir.bootlin.com/linux/v6.17.1/source/net/ethtool/tsconfig.c#L339

What I get from the ethtool output is that the ts config is per-source.
Re-applying the old config to the new source may not work if the new one
doesn't have the same capabilities.

> 
> I do not really have a strong opinion on this, let's discuss which behavior we
> prefer.

Well if we want to support different timestamp providers provided by the same
HW block (same MAC or even same PHY), then we need a way to notify the provider
when the timestamp provider gets selected and unselected.

Otherwise there's no way for the provider to know it has been re-enabled, unless
we perform a config change at the same time.

Maxime

Re: [PATCH net-next 3/3] net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

[Kory Maincent]

On Thu, 16 Oct 2025 10:01:53 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi Köry,
> 
> On 15/10/2025 14:45, Kory Maincent wrote:
> > On Wed, 15 Oct 2025 12:27:23 +0200
> > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> >   
> >> When a hwprov timestamping source is changed, but without updating the
> >> timestamping parameters, we may want to reconfigure the timestamping
> >> source to enable the new provider.
> >>
> >> This is especially important if the same HW unit implements 2 providers,
> >> a precise and an approx one. In this case, we need to make sure we call
> >> the hwtstamp_set operation for the newly selected provider.  
> > 
> > This is a design choice.
> > Do we want to preserve the hwtstamp config if only the hwtstamp source is
> > changed from ethtool?
> > If we want to configure the new source to the old source config we will also
> > need to remove this condition:
> > https://elixir.bootlin.com/linux/v6.17.1/source/net/ethtool/tsconfig.c#L339
> >  
> 
> What I get from the ethtool output is that the ts config is per-source.
> Re-applying the old config to the new source may not work if the new one
> doesn't have the same capabilities.
> 
> > 
> > I do not really have a strong opinion on this, let's discuss which behavior
> > we prefer.  
> 
> Well if we want to support different timestamp providers provided by the same
> HW block (same MAC or even same PHY), then we need a way to notify the
> provider when the timestamp provider gets selected and unselected.
> 
> Otherwise there's no way for the provider to know it has been re-enabled,
> unless we perform a config change at the same time.

Oh right, indeed, we need a call to ndo_hwtstamp_set to tell the provider to
change the qualifier configured. I missed that.
This could even be a fix but as it is used nowhere it won't fix anything.

Acked-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/3] net: ethtool: tsconfig: Re-configure hwtstamp upon provider change

[Russell King (Oracle)]

On Wed, Oct 15, 2025 at 02:45:26PM +0200, Kory Maincent wrote:
> On Wed, 15 Oct 2025 12:27:23 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
> > When a hwprov timestamping source is changed, but without updating the
> > timestamping parameters, we may want to reconfigure the timestamping
> > source to enable the new provider.
> > 
> > This is especially important if the same HW unit implements 2 providers,
> > a precise and an approx one. In this case, we need to make sure we call
> > the hwtstamp_set operation for the newly selected provider.
> 
> This is a design choice.
> Do we want to preserve the hwtstamp config if only the hwtstamp source is
> changed from ethtool?

This depends what is meant by "preserve". If the hwtstamp capabilities
of the two sources being switched between are the same in terms of how
userspace configures them, then it's fine.

However, it's my understanding that the hwtstamp configuration is a
negotiation between kernel and userspace - drivers are required to
return back to userspace what they're actually doing when userspace
requests a certain configuration. If the hwtstamp capabilities are
different, it breaks this model because what the previous instance
reports back to userspace for a certain configuration could be
different to that which the new instance would report back.

This could get worse when a configuration is set on the previous
instance that isn't supported by the new instance.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/3] net: stmmac: Add support for coarse timestamping

[Kory Maincent]

On Wed, 15 Oct 2025 12:27:20 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hello everyone,
> 
> This is another attempt to support the fine vs coarse timestamping modes
> in stmmac.
> 
> This mode allows trading off PTP clock frequency adjustment precision
> versus timestamping precision.
> 
> In coarse mode, we lose the ability to fine-tune the PTP clock
> frequency, but get better timestamping precision instead. This is
> especially useful when acting as a PTP Grand Master, where the PTP clock
> in sync'd to a high-precision GPS clock through PPS inputs.
> 
> This has been submitted before as a dedicated ioctl() back in 2020 [1].
> Since then, we now have a better representation of timestamp providers
> with a dedicated qualifier (approx vs precise).
> 
> This series attempts to map these new qualifiers to stmmac's
> timestamping modes, see patch 2 for details.
> 
> The main drawback IMO is that the qualifiers don't map very well to our
> timestamping modes, as the "approx" qualifier actually maps to stmmac's
> "coars" mode, but we actually gain in timestamping precision (while
> losing frequency precision).

https://elixir.bootlin.com/linux/v6.17.1/source/include/uapi/linux/net_tstamp.h#L16
"approx" was initially added for DMA timestamp point.
Maybe we should add a new enum value here with a more suitable name.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 0/3] net: stmmac: Add support for coarse timestamping

[Maxime Chevallier]

Hi,

On 15/10/2025 14:55, Kory Maincent wrote:
> On Wed, 15 Oct 2025 12:27:20 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
>> Hello everyone,
>>
>> This is another attempt to support the fine vs coarse timestamping modes
>> in stmmac.
>>
>> This mode allows trading off PTP clock frequency adjustment precision
>> versus timestamping precision.
>>
>> In coarse mode, we lose the ability to fine-tune the PTP clock
>> frequency, but get better timestamping precision instead. This is
>> especially useful when acting as a PTP Grand Master, where the PTP clock
>> in sync'd to a high-precision GPS clock through PPS inputs.
>>
>> This has been submitted before as a dedicated ioctl() back in 2020 [1].
>> Since then, we now have a better representation of timestamp providers
>> with a dedicated qualifier (approx vs precise).
>>
>> This series attempts to map these new qualifiers to stmmac's
>> timestamping modes, see patch 2 for details.
>>
>> The main drawback IMO is that the qualifiers don't map very well to our
>> timestamping modes, as the "approx" qualifier actually maps to stmmac's
>> "coars" mode, but we actually gain in timestamping precision (while
>> losing frequency precision).
> 
> https://elixir.bootlin.com/linux/v6.17.1/source/include/uapi/linux/net_tstamp.h#L16
> "approx" was initially added for DMA timestamp point.
> Maybe we should add a new enum value here with a more suitable name.

Yeah, the terminology in stmmac of "coarse/fine" refers to frequency adjustment, while
the "fine/approx" qualifiers refer to timestamping.

I'm OK to add a new value, with the usual risk of seeing the number of qualifiers
explode if different hardware to that in different ways.

I suggest keeping "precise" for the default mode, and maybe use "enhanced" or
a similar term that would imply that the improved precision is done at the expense
of some some other aspect of the system (and therefore probably not
suitable as a default).

Maybe Richard can shed some light on that ?

> Regards,