[PATCH net 0/2] net: stmmac: fix timestamp snapshots on dwmac1000

[PATCH net 0/2] net: stmmac: fix timestamp snapshots on dwmac1000

[Alexis Lothore]

Hello,

this small series contains two small fixes for the timestamp snapshot
feature on stmmac, especially on dwmac1000 version. Those issues have
been detected on a socfpga (Cyclone V) platform. They kind of follow the
big rework sent by Maxime at the end of last year to properly split this
feature support between different versions of the DWMAC IP.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Alexis Lothore (1):
      net: stmmac: fix dwmac1000 ptp timestamp status offset

Alexis Lothoré (1):
      net: stmmac: fix multiplication overflow when reading timestamp

 drivers/net/ethernet/stmicro/stmmac/dwmac1000.h       | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c  | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
---
base-commit: c03a49f3093a4903c8a93c8b5c9a297b5343b169
change-id: 20250422-stmmac_ts-c226a53099e2

Best regards,
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net 1/2] net: stmmac: fix dwmac1000 ptp timestamp status offset

[Alexis Lothore]

When a PTP interrupt occurs, the driver accesses the wrong offset to
learn about the number of available snapshots in the FIFO for dwmac1000:
it should be accessing bits 29..25, while it is currently reading bits
19..16 (those are bits about the auxiliary triggers which have generated
the timestamps). As a consequence, it does not compute correctly the
number of available snapshots, and so possibly do not generate the
corresponding clock events if the bogus value ends up being 0.

Fix clock events generation by reading the correct bits in the timestamp
register for dwmac1000.

Fixes: 19b93bbb20eb ("net: stmmac: Introduce dwmac1000 timestamping operations")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
index 967a16212faf008bc7b5e43031e2d85800c5c467..0c011a47d5a3e98280a98d25b8ef3614684ae78c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
@@ -320,8 +320,8 @@ enum rtc_control {
 
 /* PTP and timestamping registers */
 
-#define GMAC3_X_ATSNS       GENMASK(19, 16)
-#define GMAC3_X_ATSNS_SHIFT 16
+#define GMAC3_X_ATSNS       GENMASK(29, 25)
+#define GMAC3_X_ATSNS_SHIFT 25
 
 #define GMAC_PTP_TCR_ATSFC	BIT(24)
 #define GMAC_PTP_TCR_ATSEN0	BIT(25)

-- 
2.49.0

Re: [PATCH net 1/2] net: stmmac: fix dwmac1000 ptp timestamp status offset

[Maxime Chevallier]

Hi Alexis,

On Tue, 22 Apr 2025 17:07:22 +0200
Alexis Lothore <alexis.lothore@bootlin.com> wrote:

> When a PTP interrupt occurs, the driver accesses the wrong offset to
> learn about the number of available snapshots in the FIFO for dwmac1000:
> it should be accessing bits 29..25, while it is currently reading bits
> 19..16 (those are bits about the auxiliary triggers which have generated
> the timestamps). As a consequence, it does not compute correctly the
> number of available snapshots, and so possibly do not generate the
> corresponding clock events if the bogus value ends up being 0.
> 
> Fix clock events generation by reading the correct bits in the timestamp
> register for dwmac1000.
> 
> Fixes: 19b93bbb20eb ("net: stmmac: Introduce dwmac1000 timestamping operations")

Looks like the commit hash is wrong, should be :

477c3e1f6363 ("net: stmmac: Introduce dwmac1000 timestamping operations")

Other than that I agree with the change, these offset are the right
ones, thanks...

With the Fixes tag fixed,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net 1/2] net: stmmac: fix dwmac1000 ptp timestamp status offset

[Alexis Lothoré]

Hi Maxime,

On Tue Apr 22, 2025 at 5:49 PM CEST, Maxime Chevallier wrote:
> Hi Alexis,
>
> On Tue, 22 Apr 2025 17:07:22 +0200
> Alexis Lothore <alexis.lothore@bootlin.com> wrote:
>
>> When a PTP interrupt occurs, the driver accesses the wrong offset to
>> learn about the number of available snapshots in the FIFO for dwmac1000:
>> it should be accessing bits 29..25, while it is currently reading bits
>> 19..16 (those are bits about the auxiliary triggers which have generated
>> the timestamps). As a consequence, it does not compute correctly the
>> number of available snapshots, and so possibly do not generate the
>> corresponding clock events if the bogus value ends up being 0.
>> 
>> Fix clock events generation by reading the correct bits in the timestamp
>> register for dwmac1000.
>> 
>> Fixes: 19b93bbb20eb ("net: stmmac: Introduce dwmac1000 timestamping operations")
>
> Looks like the commit hash is wrong, should be :
>
> 477c3e1f6363 ("net: stmmac: Introduce dwmac1000 timestamping operations")

Yes, you are absolutely right, I wrongly picked this hash from a custom
branch rather than a stable branch -_- Thanks for spotting this, will be
fixed in v2.

>
> Other than that I agree with the change, these offset are the right
> ones, thanks...
>
> With the Fixes tag fixed,
>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Alexis




-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net 2/2] net: stmmac: fix multiplication overflow when reading timestamp

[Alexis Lothoré]

The current way of reading a timestamp snapshot in stmmac can lead to
integer overflow, as the computation is done on 32 bits. The issue has
been observed on a dwmac-socfpga platform returning chaotic timestamp
values due to this overflow. The corresponding multiplication is done
with a MUL instruction, which returns 32 bit values. Explicitly casting
the value to 64 bits replaced the MUL with a UMLAL, which computes and
returns the result on 64 bits, and so returns correctly the timestamps.

Prevent this overflow by explicitly casting the intermediate value to
u64 to make sure that the whole computation is made on u64. While at it,
apply the same cast on the other dwmac variant (GMAC4) method for
snapshot retrieval.

Fixes: 19b93bbb20eb ("net: stmmac: Introduce dwmac1000 timestamping operations")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c  | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index a8b901cdf5cbb395a0f6b4800ad6f06c6e870077..43b2b3377136f2d9c717f85cba6a452e7a178ad7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -553,7 +553,7 @@ void dwmac1000_get_ptptime(void __iomem *ptpaddr, u64 *ptp_time)
 	u64 ns;
 
 	ns = readl(ptpaddr + GMAC_PTP_ATNR);
-	ns += readl(ptpaddr + GMAC_PTP_ATSR) * NSEC_PER_SEC;
+	ns += (u64)(readl(ptpaddr + GMAC_PTP_ATSR)) * NSEC_PER_SEC;
 
 	*ptp_time = ns;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 0f59aa98260404bece530f505500f13d35884d0c..1950156f6af6f6f13ebdc1c04f01a862b664bc9b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -222,7 +222,7 @@ static void get_ptptime(void __iomem *ptpaddr, u64 *ptp_time)
 	u64 ns;
 
 	ns = readl(ptpaddr + PTP_ATNR);
-	ns += readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC;
+	ns += (u64)(readl(ptpaddr + PTP_ATSR)) * NSEC_PER_SEC;
 
 	*ptp_time = ns;
 }

-- 
2.49.0

Re: [PATCH net 2/2] net: stmmac: fix multiplication overflow when reading timestamp

[Russell King (Oracle)]

On Tue, Apr 22, 2025 at 05:07:23PM +0200, Alexis Lothoré wrote:
>  	ns = readl(ptpaddr + GMAC_PTP_ATNR);
> -	ns += readl(ptpaddr + GMAC_PTP_ATSR) * NSEC_PER_SEC;
> +	ns += (u64)(readl(ptpaddr + GMAC_PTP_ATSR)) * NSEC_PER_SEC;

I'm not sure what the extra parens around readl() are actually trying to
do. Please drop them if they're not useful.

-- 
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 2/2] net: stmmac: fix multiplication overflow when reading timestamp

[Alexis Lothoré]

Hello Russell,

On Tue Apr 22, 2025 at 5:32 PM CEST, Russell King (Oracle) wrote:
> On Tue, Apr 22, 2025 at 05:07:23PM +0200, Alexis Lothoré wrote:
>>  	ns = readl(ptpaddr + GMAC_PTP_ATNR);
>> -	ns += readl(ptpaddr + GMAC_PTP_ATSR) * NSEC_PER_SEC;
>> +	ns += (u64)(readl(ptpaddr + GMAC_PTP_ATSR)) * NSEC_PER_SEC;
>
> I'm not sure what the extra parens around readl() are actually trying to
> do. Please drop them if they're not useful.

They are indeed not specifically useful in this case, they will be dropped
in v2.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com