🐛 [twd] fix some design flaws

CHANGELOG.md

@@ -29,6 +29,7 @@ mimpid = 0x01040312 -> Version 01.04.03.12 -> v1.4.3.12
 
 | Date | Version | Comment | Ticket |
 |:----:|:-------:|:--------|:------:|
+| 16.01.2025 | 1.10.9.8 | :bug: fix several TWD design flaws | [#1161](https://github.com/stnolting/neorv32/pull/1161) |
 | 15.01.2025 | 1.10.9.7 | :sparkles: add GPIO interrupt(s); :warning: remove XIRQ controller, constrain GPIO input/output ports from 64-bit to 32-bit | [#1159](https://github.com/stnolting/neorv32/pull/1159) |
 | 13.01.2025 | 1.10.9.6 | add WDT and OCD rest outputs to top module | [#1152](https://github.com/stnolting/neorv32/pull/1152) |
 | 11.01.2025 | 1.10.9.5 | minor rtl cleanups; :bug: fix minor bug (multiple drivers on ICC nets; introduced in version 1.10.9.2) | [#1151](https://github.com/stnolting/neorv32/pull/1151) |

docs/datasheet/soc_twd.adoc

@@ -21,7 +21,8 @@
 **Overview**
 
 The NEORV32 TWD implements a I2C-compatible **device-mode** controller. Processor-external hosts can communicate
-with this module by issuing I2C transactions. The TWD is entirely passive an only reacts on those transmissions.
+with this module by issuing I2C transactions. The TWD is entirely passive an only reacts on those external
+transmissions.
 
 Key features:
 
@@ -41,19 +42,19 @@ check out the <<_two_wire_serial_interface_controller_twi>>.
 The TWD module provides two memory-mapped registers that are used for configuration & status check (`CTRL`) and
 for accessing transmission data (`DATA`). The `DATA` register is transparently buffered by separate RX and TX FIFOs.
 The size of those FIFOs can be configured by the `IO_TWD_FIFO` generic. Software can determine the FIFO size via the
-`TWD_CTRL_FIFO_*` bits.
+control register's `TWD_CTRL_FIFO_*` bits.
 
 The module is globally enabled by setting the control register's `TWD_CTRL_EN` bit. Clearing this bit will disable
 and reset the entire module also clearing the internal RX and TX FIFOs. Each FIFO can also be cleared individually at
 any time by setting `TWD_CTRL_CLR_RX` or `TWD_CTRL_CLR_TX`, respectively.
 
-The external two wire bus is sampled sampled and synchronized to processor's clock domain with a sampling frequency
-of 1/8 of the processor's main clock. To increase the resistance to glitches the sampling frequency can be lowered
-to 1/64 of the processor clock by setting the `TWD_CTRL_FSEL` bit.
+The external two wire bus is sampled sampled and synchronized into the processor's clock domain with a sampling
+frequency of 1/8 of the processor's main clock. In order to increase the resistance to glitches the sampling
+frequency can be lowered to 1/64 of the processor clock by setting the control register's `TWD_CTRL_FSEL` bit.
 
 .Current Bus State
 [TIP]
-The current state of the I²C bus lines (SCL and SDA) can be checked by software via the `TWD_CTRL_SENSE_*` control
+The current state of the I2C bus lines (SCL and SDA) can be checked by software via the `TWD_CTRL_SENSE_*` control
 register bits. Note that the TWD module needs to be enabled in order to sample the bus state.
 
 The actual 7-bit device address of the TWD is programmed by the `TWD_CTRL_DEV_ADDR` bits. Note that the TWD will
@@ -85,7 +86,7 @@ The interrupt can only trigger if the module is actually enabled (`TWD_CTRL_EN`
 
 **TWD Transmissions**
 
-Two standard I²C-compatible transaction types are supported: **read** operations and **write** operations. These
+Two standard I2C-compatible transaction types are supported: **read** operations and **write** operations. These
 two operation types are illustrated in the following figure (note that the transactions are split across two lines
 to improve readability).
 
@@ -102,17 +103,20 @@ to idle state.
 For a **write transaction** (upper timing diagram) the host can now transfer an arbitrary number of bytes (blue signals
 `D7` to `D0`, MSB-first) to the TWD module. Each byte is acknowledged by the TWD by pulling SDA low during the 9th SCL
 clock pules (**ACK**). Each received data byte is pushed to the internal RX FIFO. Data will be lost if the FIFO overflows.
-The transaction is terminated when the host issues a **STOP** condition.
+The transaction is terminated when the host issues a **STOP** condition after the TWD has acknowledged the last data
+transfer.
 
-For a **read transaction** (lower timing diagram) the cost keeps the SDA line at high state while sending the clock
+For a **read transaction** (lower timing diagram) the host keeps the SDA line at high state while sending the clock
 pulse. The TWD will read a byte from the internal TX FIFO and will transmit it MSB-first to the host (blue signals `D7`
-to `D0)`. During the 9th clock pulse the host has to acknowledged the transfer (**ACK**). If no ACK is received by the
-TWD no data is taken from the TX FIFO and the same byte can be transmitted in the next data phase. If the TX FIFO becomes
-empty while the host keeps reading data, all-one bytes are transmitted. The transaction is terminated when the host
-issues a **STOP** condition.
-
-A **repeated-START** condition can be issued at any time bringing the TWD back to the start of the address/command
-transmission phase. The control register's `TWD_CTRL_BUSY` flag remains high while a bus transaction is in progress.
+to `D0)`. During the 9th clock pulse the host has to acknowledged the transfer (**ACK**) by pulling SDA low. If no ACK
+is received by the TWD no data is taken from the TX FIFO and the same byte can be transmitted in the next data phase.
+If the TX FIFO becomes empty while the host keeps reading data, all-one bytes are transmitted. To terminate the
+transmission the host hast so send a **NACK** after receiving the last data byte by keeping SDA high. After that, the
+host has to issue a **STOP** condition.
+
+A **repeated-START** condition can be issued at any time (but after the complete transaction of a data byte and there
+according ACK/NACK) bringing the TWD back to the start of the address/command transmission phase. The control register's
+`TWD_CTRL_BUSY` flag remains high while a bus transaction is in progress.
 
 .Abort / Termination
 [TIP]

rtl/core/neorv32_package.vhd

@@ -29,7 +29,7 @@ package neorv32_package is
 
   -- Architecture Constants -----------------------------------------------------------------
   -- -------------------------------------------------------------------------------------------
-  constant hw_version_c : std_ulogic_vector(31 downto 0) := x"01100907"; -- hardware version
+  constant hw_version_c : std_ulogic_vector(31 downto 0) := x"01100908"; -- hardware version
   constant archid_c     : natural := 19; -- official RISC-V architecture ID
   constant XLEN         : natural := 32; -- native data path width
 

rtl/core/neorv32_twd.vhd

@@ -3,7 +3,7 @@
 -- -------------------------------------------------------------------------------- --
 -- The NEORV32 RISC-V Processor - https://github.com/stnolting/neorv32              --
 -- Copyright (c) NEORV32 contributors.                                              --
--- Copyright (c) 2020 - 2024 Stephan Nolting. All rights reserved.                  --
+-- Copyright (c) 2020 - 2025 Stephan Nolting. All rights reserved.                  --
 -- Licensed under the BSD-3-Clause license, see LICENSE for details.                --
 -- SPDX-License-Identifier: BSD-3-Clause                                            --
 -- ================================================================================ --
@@ -393,29 +393,33 @@ begin
           elsif (smp.start = '1') then -- start-condition
             engine.state <= S_INIT; -- restart transaction
           elsif (engine.cnt(3) = '1') and (smp.scl_fall = '1') then -- 8 bits received?
-            engine.wr_we <= '1'; -- write byte to RX FIFO
+            engine.wr_we <= not engine.cmd; -- write byte to RX FIFO (only if WRITE command)
             engine.state <= S_ACK;
           end if;
           -- sample bus on rising edge --
           if (smp.scl_rise = '1') then
             engine.sreg <= engine.sreg(6 downto 0) & smp.sda;
             engine.cnt  <= engine.cnt + 1;
           end if;
-          -- update bus on falling edge --
-          twd_sda_o <= engine.dout;
-          if (smp.scl_fall = '1') and (engine.cmd = '1') then
-            engine.dout <= engine.sreg(7);
+          -- set bus output only if READ operation --
+          if (engine.cmd = '1') then
+            twd_sda_o <= engine.dout;
+            if (smp.scl_fall = '1') then -- get next bit
+              engine.dout <= engine.sreg(7);
+            end if;
           end if;
 
         when S_ACK => -- receive/transmit ACK/NACK
         -- ------------------------------------------------------------
           engine.cnt  <= (others => '0');
           engine.sreg <= engine.rdata; -- FIFO TX data
           engine.dout <= engine.rdata(7); -- FIFO TX data (first bit)
-          if (ctrl.enable = '0') then -- disabled?
+          if (ctrl.enable = '0') or (smp.stop = '1') then -- disabled or stop-condition
             engine.state <= S_IDLE;
-          elsif (smp.scl_fall = '1') then
-            engine.state <= S_RTX;
+          elsif (smp.scl_fall = '1') then -- end of this time slot
+            if (engine.cmd = '0') or ((engine.cmd = '1') and (smp.sda = '0')) then -- WRITE or READ with ACK
+              engine.state <= S_RTX;
+            end if;
           end if;
           -- [READ] advance to next data byte if ACK is send by host --
           if (engine.cmd = '1') and (smp.scl_rise = '1') and (smp.sda = '0') then

sw/example/processor_check/main.c

@@ -1010,7 +1010,7 @@ int main() {
 
 
   // ----------------------------------------------------------
-  // Fast interrupt channel 0
+  // Fast interrupt channel 0 (TWD)
   // ----------------------------------------------------------
   neorv32_cpu_csr_write(CSR_MCAUSE, mcause_never_c);
   PRINT_STANDARD("[%i] FIRQ0 (TWD) ", cnt_test);
@@ -1038,8 +1038,10 @@ int main() {
 
     neorv32_cpu_csr_write(CSR_MIE, 0);
 
+    tmp_a = neorv32_twd_get();
     if ((neorv32_cpu_csr_read(CSR_MCAUSE) == TWD_TRAP_CODE) && // interrupt triggered
-        (neorv32_twd_get() == 0x47)) { // correct data written
+        (tmp_a == 0x47) && // correct data received by TWD
+        (neorv32_twd_rx_available() == 0)) { // no more data received by TWD
       test_ok();
     }
     else {
@@ -1320,10 +1322,10 @@ int main() {
     int twi_ack_x = neorv32_twi_get(&twi_data_y);
     neorv32_twi_get(&twi_data_y);
 
-
     if ((neorv32_cpu_csr_read(CSR_MCAUSE) == TWI_TRAP_CODE) && // interrupt triggered
         (twi_ack_x == 0x00) && // device acknowledged access
-        (twi_data_y == 0x8e)) { // correct read data
+        (twi_data_y == 0x8e) && // correct read data
+        (neorv32_twd_tx_empty())) { // no TX data left in TWD
       test_ok();
     }
     else {