Fix a caching issue on the RX timestamp

Author: datdenkikniet

src/dma/cache.rs

@@ -0,0 +1,82 @@
+use crate::ptp::Timestamp;
+
+use super::PacketId;
+
+/// A cache for timestamping information,
+/// used to keep the size of Descriptors down.
+#[derive(Clone, Copy)]
+#[repr(C, packed)]
+pub struct Cache {
+    has_packet_id: bool,
+    #[cfg(feature = "ptp")]
+    has_timestamp: bool,
+
+    packet_id: PacketId,
+    #[cfg(feature = "ptp")]
+    timestamp: Timestamp,
+}
+
+impl Default for Cache {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl Cache {
+    pub const fn new() -> Self {
+        Self {
+            has_packet_id: false,
+            packet_id: PacketId(0),
+
+            #[cfg(feature = "ptp")]
+            has_timestamp: false,
+            #[cfg(feature = "ptp")]
+            timestamp: Timestamp::new_raw(0),
+        }
+    }
+
+    /// Set the packet ID of this [`Cache`]
+    ///
+    /// Removes the timestamp id if `ptp` feature is enabled.
+    pub fn set_id_and_clear_ts(&mut self, packet_id: Option<PacketId>) {
+        #[cfg(feature = "ptp")]
+        {
+            self.has_timestamp = false;
+        }
+
+        if let Some(id) = packet_id {
+            self.packet_id = id;
+            self.has_packet_id = true
+        } else {
+            self.has_packet_id = false;
+        }
+    }
+
+    pub fn id(&self) -> Option<PacketId> {
+        if self.has_packet_id {
+            Some(self.packet_id)
+        } else {
+            None
+        }
+    }
+}
+
+#[cfg(feature = "ptp")]
+impl Cache {
+    pub fn set_ts(&mut self, timestamp: Option<Timestamp>) {
+        if let Some(ts) = timestamp {
+            self.timestamp = ts;
+            self.has_timestamp = true;
+        } else {
+            self.has_timestamp = false;
+        }
+    }
+
+    pub fn ts(&self) -> Option<Timestamp> {
+        if self.has_timestamp {
+            Some(self.timestamp)
+        } else {
+            None
+        }
+    }
+}

src/dma/mod.rs

@@ -4,6 +4,9 @@ use cortex_m::peripheral::NVIC;
 
 use crate::{peripherals::ETHERNET_DMA, stm32::Interrupt};
 
+mod cache;
+pub(crate) use cache::Cache;
+
 #[cfg(feature = "smoltcp-phy")]
 mod smoltcp_phy;
 #[cfg(feature = "smoltcp-phy")]
@@ -42,6 +45,8 @@ const _TXDESC_SIZE: usize = core::mem::size_of::<TxDescriptor>();
 /// value which applies to both TX and RX descriptors.
 const _ASSERT_DESCRIPTOR_SIZES: () = assert!(_RXDESC_SIZE == _TXDESC_SIZE);
 
+const _ASSERT_DESCRIPTOR_ALIGN: () = assert!(_RXDESC_SIZE % 4 == 0);
+
 const DESC_WORD_SKIP: u8 = ((_RXDESC_SIZE / 4) - self::generic_ring::DESC_SIZE) as u8;
 
 const _ASSERT_DESC_WORD_SKIP_SIZE: () = assert!(DESC_WORD_SKIP <= 0b111);

src/dma/rx/f_series_descriptor.rs

@@ -1,4 +1,5 @@
 use crate::dma::generic_ring::RawDescriptor;
+use crate::dma::Cache;
 
 use crate::dma::{rx::RxDescriptorError, PacketId};
 
@@ -34,7 +35,7 @@ const RXDESC_1_RER: u32 = 1 << 15;
 #[derive(Clone, Copy)]
 pub struct RxDescriptor {
     desc: RawDescriptor,
-    packet_id: Option<PacketId>,
+    cache: Cache,
     last: bool,
 }
 
@@ -49,8 +50,10 @@ impl RxDescriptor {
     pub const fn new() -> Self {
         Self {
             desc: RawDescriptor::new(),
-            packet_id: None,
             last: false,
+            cache: Cache::new(),
+            #[cfg(not(feature = "ptp"))]
+            _padding: [0u8; 4],
         }
     }
 
@@ -69,8 +72,6 @@ impl RxDescriptor {
     }
 
     /// Pass ownership to the DMA engine
-    ///
-    /// Overrides old timestamp data
     pub fn set_owned(&mut self, buffer: &mut [u8]) {
         self.set_buffer(buffer);
 
@@ -121,7 +122,10 @@ impl RxDescriptor {
             core::sync::atomic::compiler_fence(core::sync::atomic::Ordering::Acquire);
 
             // Set the Packet ID for this descriptor.
-            self.packet_id = packet_id;
+            self.cache.set_id_and_clear_ts(packet_id);
+
+            #[cfg(feature = "ptp")]
+            self.cache.set_ts(self.read_timestamp());
 
             Ok(())
         } else {
@@ -152,11 +156,10 @@ impl RxDescriptor {
 #[cfg(feature = "ptp")]
 impl RxDescriptor {
     pub(super) fn has_packet_id(&self, id: &PacketId) -> bool {
-        Some(id) == self.packet_id.as_ref()
+        Some(id) == self.cache.id().as_ref()
     }
 
-    /// Get PTP timestamp if available
-    pub(super) fn timestamp(&self) -> Option<Timestamp> {
+    fn read_timestamp(&self) -> Option<Timestamp> {
         #[cfg(any(feature = "stm32f4xx-hal", feature = "stm32f7xx-hal"))]
         let (high, low) = { (self.desc.read(7), self.desc.read(6)) };
 
@@ -184,4 +187,9 @@ impl RxDescriptor {
             None
         }
     }
+
+    /// Get PTP timestamp if available
+    pub(super) fn timestamp(&self) -> Option<Timestamp> {
+        self.cache.ts()
+    }
 }

src/dma/rx/h_descriptor.rs

@@ -1,6 +1,6 @@
 use core::sync::atomic::{self, Ordering};
 
-use crate::dma::{generic_ring::RawDescriptor, PacketId};
+use crate::dma::{generic_ring::RawDescriptor, Cache, PacketId};
 
 #[cfg(feature = "ptp")]
 use crate::ptp::Timestamp;
@@ -85,9 +85,7 @@ use super::RxDescriptorError;
 /// An RX DMA Descriptor.
 pub struct RxDescriptor {
     inner_raw: RawDescriptor,
-    packet_id: Option<PacketId>,
-    #[cfg(feature = "ptp")]
-    cached_timestamp: Option<Timestamp>,
+    cache: Cache,
 }
 
 impl Default for RxDescriptor {
@@ -101,9 +99,7 @@ impl RxDescriptor {
     pub const fn new() -> Self {
         Self {
             inner_raw: RawDescriptor::new(),
-            packet_id: None,
-            #[cfg(feature = "ptp")]
-            cached_timestamp: None,
+            cache: Cache::new(),
         }
     }
 
@@ -201,9 +197,7 @@ impl RxDescriptor {
             // "Subsequent reads and writes cannot be moved ahead of preceding reads."
             atomic::compiler_fence(Ordering::Acquire);
 
-            self.packet_id = packet_id;
-            #[cfg(feature = "ptp")]
-            self.cached_timestamp.take();
+            self.cache.set_id_and_clear_ts(packet_id);
 
             Ok(())
         } else {
@@ -230,14 +224,14 @@ impl RxDescriptor {
     }
 
     pub(super) fn has_packet_id(&self, packet_id: &PacketId) -> bool {
-        self.packet_id.as_ref() == Some(packet_id)
+        self.cache.id().as_ref() == Some(packet_id)
     }
 
     pub(super) fn attach_timestamp(&mut self, timestamp: Option<Timestamp>) {
-        self.cached_timestamp = timestamp;
+        self.cache.set_ts(timestamp);
     }
 
     pub(super) fn timestamp(&self) -> Option<Timestamp> {
-        self.cached_timestamp.clone()
+        self.cache.ts()
     }
 }

src/dma/tx/f_series_descriptor.rs

@@ -1,4 +1,4 @@
-use crate::dma::{generic_ring::RawDescriptor, PacketId};
+use crate::dma::{generic_ring::RawDescriptor, Cache, PacketId};
 
 #[cfg(feature = "ptp")]
 use crate::ptp::Timestamp;
@@ -38,8 +38,8 @@ const TXDESC_1_TBS2_MASK: u32 = 0x0fff << TXDESC_1_TBS2_SHIFT;
 #[derive(Clone, Copy)]
 pub struct TxDescriptor {
     desc: RawDescriptor,
-    packet_id: Option<PacketId>,
     last: bool,
+    cache: Cache,
 }
 
 impl Default for TxDescriptor {
@@ -53,8 +53,8 @@ impl TxDescriptor {
     pub const fn new() -> Self {
         Self {
             desc: RawDescriptor::new(),
-            packet_id: None,
             last: false,
+            cache: Cache::new(),
         }
     }
 
@@ -101,7 +101,7 @@ impl TxDescriptor {
             extra_flags
         };
 
-        self.packet_id = packet_id;
+        self.cache.set_id_and_clear_ts(packet_id);
 
         // "Preceding reads and writes cannot be moved past subsequent writes."
         #[cfg(feature = "fence")]
@@ -154,9 +154,14 @@ impl TxDescriptor {
 #[cfg(feature = "ptp")]
 impl TxDescriptor {
     pub(super) fn has_packet_id(&self, packet_id: &PacketId) -> bool {
-        self.packet_id.as_ref() == Some(packet_id)
+        self.cache.id().as_ref() == Some(packet_id)
     }
 
+    /// For the TxDescriptor we ignore [`Cache::ts`] because:
+    /// * We're only really using the cache so that the size of RxDescriptor and TxDescriptor
+    ///   is the same.
+    /// * We want to be able to retrieve the timestamp immutably.
+    /// * The Timestamp in the TX descriptor is valid until we perform another transmission.
     pub(super) fn timestamp(&self) -> Option<Timestamp> {
         let tdes0 = self.desc.read(0);
 

src/dma/tx/h_descriptor.rs

@@ -1,6 +1,6 @@
 use core::sync::atomic::{self, Ordering};
 
-use crate::dma::{generic_ring::RawDescriptor, PacketId};
+use crate::dma::{generic_ring::RawDescriptor, Cache, PacketId};
 
 #[cfg(feature = "ptp")]
 use crate::ptp::Timestamp;
@@ -122,9 +122,7 @@ pub use consts::*;
 #[derive(Clone, Copy)]
 pub struct TxDescriptor {
     inner_raw: RawDescriptor,
-    packet_id: Option<PacketId>,
-    #[cfg(feature = "ptp")]
-    _padding: Option<Timestamp>,
+    cache: Cache,
 }
 
 impl Default for TxDescriptor {
@@ -138,9 +136,7 @@ impl TxDescriptor {
     pub const fn new() -> Self {
         Self {
             inner_raw: RawDescriptor::new(),
-            packet_id: None,
-            #[cfg(feature = "ptp")]
-            _padding: None,
+            cache: Cache::new(),
         }
     }
 
@@ -152,7 +148,7 @@ impl TxDescriptor {
     pub(super) fn setup(&mut self) {
         // Zero-out all fields in the descriptor
         (0..4).for_each(|n| unsafe { self.inner_raw.write(n, 0) });
-        self.packet_id.take();
+        self.cache.set_id_and_clear_ts(None);
     }
 
     pub(super) fn is_owned(&self) -> bool {
@@ -178,7 +174,7 @@ impl TxDescriptor {
             }
         }
 
-        self.packet_id = packet_id;
+        self.cache.set_id_and_clear_ts(packet_id);
 
         // "Preceding reads and writes cannot be moved past subsequent writes."
         atomic::fence(Ordering::Release);
@@ -230,9 +226,14 @@ impl TxDescriptor {
 #[cfg(feature = "ptp")]
 impl TxDescriptor {
     pub(super) fn has_packet_id(&self, packet_id: &PacketId) -> bool {
-        self.packet_id.as_ref() == Some(packet_id)
+        self.cache.id().as_ref() == Some(packet_id)
     }
 
+    /// For the TxDescriptor we ignore [`Cache::ts`] because:
+    /// * We're only really using the cache so that the size of RxDescriptor and TxDescriptor
+    ///   is the same.
+    /// * We want to be able to retrieve the timestamp immutably.
+    /// * The Timestamp in the TX descriptor is valid until we perform another transmission.
     pub(super) fn timestamp(&self) -> Option<Timestamp> {
         let contains_timestamp = (self.inner_raw.read(3) & TXDESC_3_TTSS) == TXDESC_3_TTSS;