spi: Always delay for NSS setup time

* Fixes the lack of setup time delay of an fixed-byte command that immediately follows an N-byte command.
* Extra 50ns will be inserted in-between fixed-byte commands but should be acceptable.

Cargo.toml

@@ -32,16 +32,17 @@ nal = [
     "embedded-time", "embedded-nal", "heapless",
     "smoltcp-phy", "smoltcp/socket-tcp", "smoltcp/ethernet"
 ]
+cortex-m-cpu = ["cortex-m"]
 # Example-based features
 smoltcp-examples = [
     "smoltcp-phy", "smoltcp/socket-tcp", "smoltcp/ethernet"
 ]
 tx_stm32f407 = [
-    "stm32f4xx-hal/stm32f407", "cortex-m", "cortex-m-rtic",
+    "stm32f4xx-hal/stm32f407", "cortex-m", "cortex-m-rtic", "cortex-m-cpu",
     "panic-itm", "log"
 ]
 tcp_stm32f407 = [
-    "stm32f4xx-hal/stm32f407", "cortex-m", "cortex-m-rt", "cortex-m-rtic",
+    "stm32f4xx-hal/stm32f407", "cortex-m", "cortex-m-rt", "cortex-m-rtic", "cortex-m-cpu",
     "smoltcp-examples", "panic-itm", "log"]
 default = []
 

examples/tcp_stm32f407.rs

@@ -79,8 +79,7 @@ use stm32f4xx_hal::{
 };
 type SpiEth = enc424j600::Enc424j600<
     Spi<SPI1, (PA5<Alternate<AF5>>, PA6<Alternate<AF5>>, PA7<Alternate<AF5>>)>,
-    PA4<Output<PushPull>>,
-    fn(u32) -> ()
+    PA4<Output<PushPull>>
 >;
 
 pub struct NetStorage {
@@ -153,14 +152,12 @@ const APP: () = {
                     Hertz(enc424j600::spi::interfaces::SPI_CLOCK_FREQ),
                     clocks);
 
-                let delay_ns_fp: fn(u32) -> () = |time_ns| {
-                    cortex_m::asm::delay((time_ns*21)/125 + 1)
-                };
-                SpiEth::new(spi_eth_port, spi1_nss, delay_ns_fp)
+                SpiEth::new(spi_eth_port, spi1_nss)
+                    .cpu_freq_mhz(168)
             };
 
             // Init controller
-            match spi_eth.reset() {
+            match spi_eth.reset(&mut delay) {
                 Ok(_) => {
                     iprintln!(stim0, "Initializing Ethernet...")
                 }

examples/tx_stm32f407.rs

@@ -29,8 +29,7 @@ use stm32f4xx_hal::{
 };
 type SpiEth = enc424j600::Enc424j600<
     Spi<SPI1, (PA5<Alternate<AF5>>, PA6<Alternate<AF5>>, PA7<Alternate<AF5>>)>,
-    PA4<Output<PushPull>>,
-    fn(u32) -> ()
+    PA4<Output<PushPull>>
 >;
 
 #[rtic::app(device = stm32f4xx_hal::stm32, peripherals = true, monotonic = rtic::cyccnt::CYCCNT)]
@@ -84,14 +83,12 @@ const APP: () = {
                 Hertz(enc424j600::spi::interfaces::SPI_CLOCK_FREQ),
                 clocks);
 
-            let delay_ns: fn(u32) -> () = |time_ns| {
-                cortex_m::asm::delay((time_ns*21)/125 + 1)
-            };
-            SpiEth::new(spi_eth_port, spi1_nss, delay_ns)
+            SpiEth::new(spi_eth_port, spi1_nss)
+                .cpu_freq_mhz(168)
         };
 
         // Init
-        match spi_eth.reset() {
+        match spi_eth.reset(&mut delay) {
             Ok(_) => {
                 iprintln!(stim0, "Initializing Ethernet...")
             }

src/lib.rs

@@ -4,6 +4,7 @@ pub mod spi;
 use embedded_hal::{
     blocking::{
         spi::Transfer,
+        delay::DelayUs,
     },
     digital::v2::OutputPin,
 };
@@ -43,32 +44,36 @@ impl From<spi::Error> for Error {
 
 /// ENC424J600 controller in SPI mode
 pub struct Enc424j600<SPI: Transfer<u8>,
-                      NSS: OutputPin,
-                      F: FnMut(u32) -> ()> {
-    spi_port: spi::SpiPort<SPI, NSS, F>,
+                      NSS: OutputPin> {
+    spi_port: spi::SpiPort<SPI, NSS>,
     rx_buf: rx::RxBuffer,
-    tx_buf: tx::TxBuffer
+    tx_buf: tx::TxBuffer,
 }
 
 impl <SPI: Transfer<u8>,
-      NSS: OutputPin,
-      F: FnMut(u32) -> ()> Enc424j600<SPI, NSS, F> {
-    pub fn new(spi: SPI, nss: NSS, delay_ns: F) -> Self {
+      NSS: OutputPin> Enc424j600<SPI, NSS> {
+    pub fn new(spi: SPI, nss: NSS) -> Self {
         Enc424j600 {
-            spi_port: spi::SpiPort::new(spi, nss, delay_ns),
+            spi_port: spi::SpiPort::new(spi, nss),
             rx_buf: rx::RxBuffer::new(),
-            tx_buf: tx::TxBuffer::new()
+            tx_buf: tx::TxBuffer::new(),
         }
     }
 
-    pub fn init(&mut self) -> Result<(), Error> {
-        self.reset()?;
+    #[cfg(feature = "cortex-m-cpu")]
+    pub fn cpu_freq_mhz(mut self, freq: u32) -> Self {
+        self.spi_port = self.spi_port.cpu_freq_mhz(freq);
+        self
+    }
+
+    pub fn init(&mut self, delay: &mut impl DelayUs<u16>) -> Result<(), Error> {
+        self.reset(delay)?;
         self.init_rxbuf()?;
         self.init_txbuf()?;
         Ok(())
     }
 
-    pub fn reset(&mut self) -> Result<(), Error> {
+    pub fn reset(&mut self, delay: &mut impl DelayUs<u16>) -> Result<(), Error> {
         // Write 0x1234 to EUDAST
         self.spi_port.write_reg_16b(spi::addrs::EUDAST, 0x1234)?;
         // Verify that EUDAST is 0x1234
@@ -81,16 +86,15 @@ impl <SPI: Transfer<u8>,
             let estat = self.spi_port.read_reg_16b(spi::addrs::ESTAT)?;
             if estat & 0x1000 == 0x1000 { break }
         }
-        // Set ETHRST (ECON2<4>) to 1
-        let econ2 = self.spi_port.read_reg_8b(spi::addrs::ECON2)?;
-        self.spi_port.write_reg_8b(spi::addrs::ECON2, 0x10 | (econ2 & 0b11101111))?;
-        self.spi_port.delay_us(25);
+        // Issue system reset - set ETHRST (ECON2<4>) to 1
+        self.spi_port.send_opcode(spi::opcodes::SETETHRST)?;
+        delay.delay_us(25);
         // Verify that EUDAST is 0x0000
         eudast = self.spi_port.read_reg_16b(spi::addrs::EUDAST)?;
         if eudast != 0x0000 {
             return Err(Error::RegisterError)
         }
-        self.spi_port.delay_us(256);
+        delay.delay_us(256);
         Ok(())
     }
 
@@ -101,9 +105,8 @@ impl <SPI: Transfer<u8>,
         self.spi_port.write_reg_16b(spi::addrs::ERXTAIL, self.rx_buf.get_tail_addr())?;
         // Set MAMXFL to maximum number of bytes in each accepted packet
         self.spi_port.write_reg_16b(spi::addrs::MAMXFL, RAW_FRAME_LENGTH_MAX as u16)?;
-        // Enable RXEN (ECON1<0>)
-        let econ1 = self.spi_port.read_reg_16b(spi::addrs::ECON1)?;
-        self.spi_port.write_reg_16b(spi::addrs::ECON1, 0x1 | (econ1 & 0xfffe))?;
+        // Enable RX - set RXEN (ECON1<0>) to 1
+        self.spi_port.send_opcode(spi::opcodes::ENABLERX)?;
         Ok(())
     }
 
@@ -146,8 +149,7 @@ impl <SPI: Transfer<u8>,
 }
 
 impl <SPI: Transfer<u8>,
-      NSS: OutputPin,
-      F: FnMut(u32) -> ()> EthPhy for Enc424j600<SPI, NSS, F> {
+      NSS: OutputPin> EthPhy for Enc424j600<SPI, NSS> {
     /// Receive the next packet and return it
     /// Set is_poll to true for returning until PKTIF is set;
     /// Set is_poll to false for returning Err when PKTIF is not set
@@ -188,9 +190,8 @@ impl <SPI: Transfer<u8>,
         } else {
             self.spi_port.write_reg_16b(spi::addrs::ERXTAIL, rx::RX_MAX_ADDRESS - 1)?;
         }
-        // Set PKTDEC (ECON1<88>) to decrement PKTCNT
-        let econ1_hi = self.spi_port.read_reg_8b(spi::addrs::ECON1 + 1)?;
-        self.spi_port.write_reg_8b(spi::addrs::ECON1 + 1, 0x01 | (econ1_hi & 0xfe))?;
+        // Decrement PKTCNT - set PKTDEC (ECON1<8>)
+        self.spi_port.send_opcode(spi::opcodes::SETPKTDEC)?;
         // Return the RxPacket
         Ok(rx_packet)
     }
@@ -208,12 +209,11 @@ impl <SPI: Transfer<u8>,
         self.spi_port.write_reg_16b(spi::addrs::ETXST, self.tx_buf.get_next_addr())?;
         // Set ETXLEN to packet length
         self.spi_port.write_reg_16b(spi::addrs::ETXLEN, packet.get_frame_length() as u16)?;
-        // Set TXRTS (ECON1<1>) to start transmission
-        let mut econ1_lo = self.spi_port.read_reg_8b(spi::addrs::ECON1)?;
-        self.spi_port.write_reg_8b(spi::addrs::ECON1, 0x02 | (econ1_lo & 0xfd))?;
+        // Send packet - set TXRTS (ECON1<1>) to start transmission
+        self.spi_port.send_opcode(spi::opcodes::SETTXRTS)?;
         // Poll TXRTS (ECON1<1>) to check if it is reset
         loop {
-            econ1_lo = self.spi_port.read_reg_8b(spi::addrs::ECON1)?;
+            let econ1_lo = self.spi_port.read_reg_8b(spi::addrs::ECON1)?;
             if econ1_lo & 0x02 == 0 { break }
         }
         // TODO: Read ETXSTAT to understand Ethernet transmission status

src/nal.rs

@@ -24,7 +24,7 @@ pub enum NetworkError {
 pub type NetworkInterface<SPI, NSS> = net::iface::EthernetInterface<
     'static,
     crate::smoltcp_phy::SmoltcpDevice<
-        crate::Enc424j600<SPI, NSS, fn(u32)>
+        crate::Enc424j600<SPI, NSS>
     >,
 >;
 

src/spi.rs

@@ -15,7 +15,17 @@ pub mod interfaces {
 }
 
 pub mod opcodes {
-    /// SPI Opcodes
+    /// 1-byte Instructions
+    pub const SETETHRST: u8 = 0b1100_1010;
+    pub const SETPKTDEC: u8 = 0b1100_1100;
+    pub const SETTXRTS: u8 = 0b1101_0100;
+    pub const ENABLERX: u8 = 0b1110_1000;
+    /// 3-byte Instructions
+    pub const WRXRDPT: u8 = 0b0110_0100;    // 8-bit opcode followed by data
+    pub const RRXRDPT: u8 = 0b0110_0110;    // 8-bit opcode followed by data
+    pub const WGPWRPT: u8 = 0b0110_1100;    // 8-bit opcode followed by data
+    pub const RGPWRPT: u8 = 0b0110_1110;    // 8-bit opcode followed by data
+    /// N-byte Instructions
     pub const RCRU: u8 = 0b0010_0000;
     pub const WCRU: u8 = 0b0010_0010;
     pub const RRXDATA: u8 = 0b0010_1100;    // 8-bit opcode followed by data
@@ -52,154 +62,165 @@ pub mod addrs {
 /// Struct for SPI I/O interface on ENC424J600
 /// Note: stm32f4xx_hal::spi's pins include: SCK, MISO, MOSI
 pub struct SpiPort<SPI: Transfer<u8>,
-                   NSS: OutputPin,
-                   F: FnMut(u32) -> ()> {
+                   NSS: OutputPin> {
     spi: SPI,
     nss: NSS,
-    delay_ns: F,
+    #[cfg(feature = "cortex-m-cpu")]
+    cpu_freq_mhz: f32,
 }
 
 pub enum Error {
+    OpcodeError,
     TransferError
 }
 
 #[allow(unused_must_use)]
 impl <SPI: Transfer<u8>,
-      NSS: OutputPin,
-      F: FnMut(u32) -> ()> SpiPort<SPI, NSS, F> {
+      NSS: OutputPin> SpiPort<SPI, NSS> {
     // TODO: return as Result()
-    pub fn new(spi: SPI, mut nss: NSS, delay_ns: F) -> Self {
+    pub fn new(spi: SPI, mut nss: NSS) -> Self {
         nss.set_high();
 
         SpiPort {
             spi,
             nss,
-            delay_ns,
+            #[cfg(feature = "cortex-m-cpu")]
+            cpu_freq_mhz: 0.,
         }
     }
 
+    #[cfg(feature = "cortex-m-cpu")]
+    pub fn cpu_freq_mhz(mut self, freq: u32) -> Self {
+        self.cpu_freq_mhz = freq as f32;
+        self
+    }
+
     pub fn read_reg_8b(&mut self, addr: u8) -> Result<u8, Error> {
         // Using RCRU instruction to read using unbanked (full) address
-        let r_data = self.rw_addr_u8(opcodes::RCRU, addr, 0)?;
-        Ok(r_data)
+        let mut buf: [u8; 4] = [0; 4];
+        buf[1] = addr;
+        self.rw_n(&mut buf, opcodes::RCRU, 2)?;
+        Ok(buf[2])
     }
 
     pub fn read_reg_16b(&mut self, lo_addr: u8) -> Result<u16, Error> {
-        let r_data_lo = self.read_reg_8b(lo_addr)?;
-        let r_data_hi = self.read_reg_8b(lo_addr + 1)?;
-        // Combine top and bottom 8-bit to return 16-bit
-        Ok(((r_data_hi as u16) << 8) | r_data_lo as u16)
+        // Unless the register can be written with specific opcode,
+        // use WCRU instruction to write using unbanked (full) address
+        let mut buf: [u8; 4] = [0; 4];
+        let mut data_offset = 0;    // number of bytes separating
+                                    // actual data from opcode
+        match lo_addr {
+            addrs::ERXRDPT | addrs::EGPWRPT => { }
+            _ => {
+                buf[1] = lo_addr;
+                data_offset = 1;
+            }
+        }
+        self.rw_n(
+            &mut buf,
+            match lo_addr {
+                addrs::ERXRDPT => opcodes::RRXRDPT,
+                addrs::EGPWRPT => opcodes::RGPWRPT,
+                _ => opcodes::RCRU
+            },
+            2 + data_offset     // extra 8-bit lo_addr before data
+        )?;
+        Ok(buf[data_offset+1] as u16 | (buf[data_offset+2] as u16) << 8)
     }
 
     // Currently requires manual slicing (buf[1..]) for the data read back
     pub fn read_rxdat<'a>(&mut self, buf: &'a mut [u8], data_length: usize)
                          -> Result<(), Error> {
-        self.r_n(buf, opcodes::RRXDATA, data_length)
+        self.rw_n(buf, opcodes::RRXDATA, data_length)
     }
 
     // Currently requires actual data to be stored in buf[1..] instead of buf[0..]
     // TODO: Maybe better naming?
     pub fn write_txdat<'a>(&mut self, buf: &'a mut [u8], data_length: usize)
                           -> Result<(), Error> {
-        self.w_n(buf, opcodes::WGPDATA, data_length)
+        self.rw_n(buf, opcodes::WGPDATA, data_length)
     }
 
     pub fn write_reg_8b(&mut self, addr: u8, data: u8) -> Result<(), Error> {
-        // TODO: addr should be separated from w_data
         // Using WCRU instruction to write using unbanked (full) address
-        self.rw_addr_u8(opcodes::WCRU, addr, data)?;
-        Ok(())
+        let mut buf: [u8; 3] = [0; 3];
+        buf[1] = addr;
+        buf[2] = data;
+        self.rw_n(&mut buf, opcodes::WCRU, 2)
     }
 
     pub fn write_reg_16b(&mut self, lo_addr: u8, data: u16) -> Result<(), Error> {
-        self.write_reg_8b(lo_addr, (data & 0xff) as u8)?;
-        self.write_reg_8b(lo_addr + 1, ((data & 0xff00) >> 8) as u8)?;
-        Ok(())
-    }
-
-    pub fn delay_us(&mut self, duration: u32) {
-        (self.delay_ns)(duration * 1000)
-    }
-
-    // TODO: Generalise transfer functions
-    // TODO: (Make data read/write as reference to array)
-    // Currently requires 1-byte addr, read/write data is only 1-byte
-    fn rw_addr_u8(&mut self, opcode: u8, addr: u8, data: u8)
-                 -> Result<u8, Error> {
-        // Enable chip select
-        self.nss.set_low();
-        // Start writing to SLAVE
-        // TODO: don't just use 3 bytes
-        let mut buf: [u8; 3] = [0; 3];
-        buf[0] = opcode;
-        buf[1] = addr;
-        buf[2] = data;
-        match self.spi.transfer(&mut buf) {
-            Ok(_) => {
-                // Disable chip select
-                (self.delay_ns)(60);
-                self.nss.set_high();
-                (self.delay_ns)(30);
-                Ok(buf[2])
-            },
-            // TODO: Maybe too naive?
-            Err(_) => {
-                // Disable chip select
-                (self.delay_ns)(60);
-                self.nss.set_high();
-                (self.delay_ns)(30);
-                Err(Error::TransferError)
+        // Unless the register can be written with specific opcode,
+        // use WCRU instruction to write using unbanked (full) address
+        let mut buf: [u8; 4] = [0; 4];
+        let mut data_offset = 0;    // number of bytes separating
+                                    // actual data from opcode
+        match lo_addr {
+            addrs::ERXRDPT | addrs::EGPWRPT => { }
+            _ => {
+                buf[1] = lo_addr;
+                data_offset = 1;
             }
         }
+        buf[1+data_offset] = data as u8;
+        buf[2+data_offset] = (data >> 8) as u8;
+        self.rw_n(
+            &mut buf,
+            match lo_addr {
+                addrs::ERXRDPT => opcodes::WRXRDPT,
+                addrs::EGPWRPT => opcodes::WGPWRPT,
+                _ => opcodes::WCRU
+            },
+            2 + data_offset     // extra 8-bit lo_addr before data
+        )
+    }
+
+    pub fn send_opcode(&mut self, opcode: u8) -> Result<(), Error> {
+        match opcode {
+            opcodes::SETETHRST | opcodes::SETPKTDEC |
+            opcodes::SETTXRTS | opcodes::ENABLERX => {
+                let mut buf: [u8; 1] = [0];
+                self.rw_n(&mut buf, opcode, 0)
+            }
+            _ => Err(Error::OpcodeError)
+        }
     }
 
-    // TODO: Generalise transfer functions
-    // Currently does NOT accept addr, read data is N-byte long
-    // Returns a reference to the data returned
-    // Note: buf must be at least (data_length + 1)-byte long
-    // TODO: Check and raise error for array size < (data_length + 1)
-    fn r_n<'a>(&mut self, buf: &'a mut [u8], opcode: u8, data_length: usize)
-              -> Result<(), Error> {
-        // Enable chip select
-        self.nss.set_low();
-        // Start writing to SLAVE
-        buf[0] = opcode;
-        match self.spi.transfer(&mut buf[..data_length+1]) {
-            Ok(_) => {
-                // Disable chip select
-                self.nss.set_high();
-                Ok(())
-            },
-            // TODO: Maybe too naive?
-            Err(_) => {
-                // Disable chip select
-                self.nss.set_high();
-                Err(Error::TransferError)
-            }
-        }
-    }
-
-    // Note: buf[0] is currently reserved for opcode to overwrite
     // TODO: Actual data should start from buf[0], not buf[1]
-    fn w_n<'a>(&mut self, buf: &'a mut [u8], opcode: u8, data_length: usize)
-              -> Result<(), Error> {
+    // Completes an SPI transfer for reading data to the given buffer,
+    // or writing data from the buffer.
+    // It sends an 8-bit instruction, followed by either
+    // receiving or sending n*8-bit data.
+    // The slice of buffer provided must begin with the 8-bit instruction.
+    // If n = 0, the transfer will only involve sending the instruction.
+    fn rw_n<'a>(&mut self, buf: &'a mut [u8], opcode: u8, data_length: usize) -> Result<(), Error> {
+        assert!(buf.len() > data_length);
         // Enable chip select
         self.nss.set_low();
+        // >=50ns min. CS_n setup time
+        #[cfg(feature = "cortex-m-cpu")]
+        cortex_m::asm::delay((0.05*(self.cpu_freq_mhz+1.)) as u32);
         // Start writing to SLAVE
         buf[0] = opcode;
-        // TODO: Maybe need to copy data to buf later on
-        match self.spi.transfer(&mut buf[..data_length+1]) {
-            Ok(_) => {
+        let result = self.spi.transfer(&mut buf[..data_length+1]);
+        match opcode {
+            opcodes::RCRU | opcodes::WCRU |
+            opcodes::RRXDATA | opcodes::WGPDATA => {
                 // Disable chip select
+                // >=50ns min. CS_n hold time
+                #[cfg(feature = "cortex-m-cpu")]
+                cortex_m::asm::delay((0.05*(self.cpu_freq_mhz+1.)) as u32);
                 self.nss.set_high();
-                Ok(())
-            },
-            // TODO: Maybe too naive?
-            Err(_) => {
-                // Disable chip select
-                self.nss.set_high();
-                Err(Error::TransferError)
+                // >=20ns min. CS_n disable time
+                #[cfg(feature = "cortex-m-cpu")]
+                cortex_m::asm::delay((0.02*(self.cpu_freq_mhz+1.)) as u32);    
             }
+            _ => { }
+        }
+        match result {
+            Ok(_) => Ok(()),
+            // TODO: Maybe too naive?
+            Err(_) => Err(Error::TransferError),
         }
     }
 }