[twd] add twd tx register to allow continuous reading of same byte as host and [sw/bootloader] add twd option

[LukasP46]

• Add twd tx register to allow continuous reading of same byte as host.
• Add twd option to bootloader.

Now the bootloader is quite usable without UART :)

Tested only on release 1.10.9, but I created a new branch from main and squashed most commits as there were just too many and a merge would be necessary anyway. If anyone could test the functionality with the current upstream, I would appreciate it. Code-wise, though, it looks like there shouldn't be any conflicts.

[LukasP46]

I also again have a python script to upload a executable via a USB-ISS adapter but I guess it's to specific?

[LukasP46]

Damn it, the bootloader gets too big again. Should we again disable the new option as default?

[LukasP46]

I'll add more documentation about the bootloader's twd interface today.

[stnolting]

Hey @LukasP46.

Thanks for your PR!

Could you explain some details about your proposal (and maybe some practial applications)?

[LukasP46]

Could you explain some details about your proposal (and maybe some practial applications)?

Sure!

TWD TX Register

I added a new register that basically just feeds the TX fifo so that a host always reads the same byte. This is very useful as a status register and is used by the my twd bootloader feature.

TX and RX Fifo Sizes

In the same run, I enabled changing the TX and RX fifo sizes independently. I haven't had the case yet where my SoC needs to send a lot of data, but might receive quite a lot, so splitting them allows for smaller designs.

TWD Bootloader

One of my main goals is to reduce firmware and hardware size, so UART is pretty... Not ideal, especially firmware wise with all the string encoding. So I wanted to use the TWD interface, which allows uploading new firmware with only one TWI connection, which is quite lightweight. Also, I have an eeprom on the same bus and now the bootloader can either autoboot that or get it as a TWD.

[stnolting]

Thanks for the details!

In the same run, I enabled changing the TX and RX fifo sizes independently. I haven't had the case yet where my SoC needs to send a lot of data, but might receive quite a lot, so splitting them allows for smaller designs.

I think that is a good idea! 👍

So I wanted to use the TWD interface, which allows uploading new firmware with only one TWI connection, which is quite lightweight. Also, I have an eeprom on the same bus and now the bootloader can either autoboot that or get it as a TWD.

That also sounds like a great idea.

I added a new register that basically just feeds the TX fifo so that a host always reads the same byte. This is very useful as a status register and is used by the my twd bootloader feature.

Ah, now I understand. Maybe we should choose another name here... "dummy byte" or "empty response" might be more clear.

I see that you are writing this dummy byte to to the outgoing FIFO. The FIFO will therefore be completely filled with the dummy byte at some point. If the user wants to prepare the FIFO for the next transfer, an explicit deletion is required. So why don't we replace this line

neorv32/rtl/core/neorv32_twd.vhd

Line 214 in d29fe6e

engine.rdata <= tx_fifo.rdata when (tx_fifo.avail = '1') else (others => '1'); -- read ones when TX FIFO is drained

by using tx_reg as FIFO-is-empty override:

engine.rdata  <= tx_fifo.rdata when (tx_fifo.avail = '1') else tx_reg;

But wy do we need an explicit register for that? Couldn't the bootloader simply write the corresponding status byte to the TX FIFO when the data stream is ready?

[LukasP46]

Maybe we should choose another name here

I agree. I'm against "empty response" because it sounds like it would just return '1', which is the default behavior (technically we could change it to not acknowledge host reads if no data is available, but that's for another PR).
"dummy byte" sounds good :)

I see that you are writing this dummy byte to the outgoing FIFO. The FIFO will therefore be completely filled with the dummy byte at some point. If the user wants to prepare the FIFO for the next transfer, an explicit deletion is required.

Damn, you are right. I thought about that when I replaced the dummy byte itself:

neorv32/rtl/core/neorv32_twd.vhd

Line 167 in d1f1041

ctrl.clr_tx <= '1'; -- reset tx fifo for new data

But not when switching back to FIFO mode.

So why don't we replace this line

That sounds like an idea, we wouldn't need an enable bit either. It's more like a default value that can be used as a status register-like behavior :)

But why do we need an explicit register for that? Couldn't the bootloader simply write the corresponding status byte to the TX FIFO when the data stream is ready?

First, this is more robust, since if a read fails, the next read will still represent the current state of the bootloader, and second, there are application use-cases. For example, polling like behavior: a host could occasionally read if the device is finished with its operation.

[LukasP46]

I agree. I'm against "empty response" because it sounds like it would just return '1', which is the default behavior (technically we could change it to not acknowledge host reads if no data is available, but that's for another PR).
"dummy byte" sounds good :)

Now the twd does not ack the address if no data is available and dummy is disabled. The default dummy is '0xFF' so if you just enable the dummy, is it the old behavior.

[stnolting]

First, this is more robust, since if a read fails, the next read will still represent the current state of the bootloader, and second, there are application use-cases. For example, polling like behavior: a host could occasionally read if the device is finished with its operation.

Good point!

I think your changes are great so far! I just don't like that we need another memory-mapped register for the dummy byte 😅 Unfortunately, we no longer have enough empty bits in the control register...

Do we need the flexibility to provide a full-custom byte? Or would specific pattern be sufficient (all-zero, all-one, ...)? 🤔

Now the twd does not ack the address if no data is available and dummy is disabled. The default dummy is '0xFF' so if you just enable the dummy, is it the old behavior.

But this would make the TWD disappear from the bus (the external controller could not find the TWD any more), right?

Btw, could you update this branch from upstream main so we can start a (partial) review?

[LukasP46]

Do we need the flexibility to provide a full-custom byte?

Well, I do. For state like operation it is necessary. But when it gets too specific, it is what it is. I can also revert the changes in the TWD module and we just use the new bootloader without such a dummy mode, but we push the status to the fifo.

But this would make the TWD disappear from the bus (the external controller could not find the TWD any more), right?

Yes. According to chapter 3.1.6 of the I2C-bus specification and user manual, it is not so easy to say what is correct:

There are five conditions that lead to the generation of a NACK:

1. No receiver is present on the bus with the transmitted address so there is no device to
   respond with an acknowledge.
2. The receiver is unable to receive or transmit because it is performing some real-time
   function and is not ready to start communication with the controller.
3. During the transfer, the receiver gets data or commands that it does not understand.
4. During the transfer, the receiver cannot receive any more data bytes.
5. A controller-receiver must signal the end of the transfer to the target transmitter.

The 2nd point suggests that a NACK of the address is ok, but the reason is quite specific, we cannot assume that as it depends heavily on the usecase. Point 3, "commands it does not understand" Well, it's a read and I have no data, is that a "commands it does not understand"? Also it's not "During the transfer"...

I would suggest that the user can choose. Sending 0xFF has the advantage that the address will respond, but the disadvantage that 0xFF may be valid data to the host, even if it shouldn't be. NACK is the opposite.

Btw, could you update this branch from upstream main so we can start a (partial) review?
Yes, but currently I only cannot check this upstream but need to stick to release 1.10.9. This might change

[stnolting]

Well, I do. For state like operation it is necessary.

Ok that makes sense. I'm fine with this! But this additional register still bothers me a little...
How about this: if the FIFO runs empty during a read access, the last valid byte is output again and again. For me, this feels more intuitive. 🤔

Basically we just replace this line

neorv32/rtl/core/neorv32_twd.vhd

Line 214 in 8831004

engine.rdata <= tx_fifo.rdata when (tx_fifo.avail = '1') else (others => '1'); -- read ones when TX FIFO is drained

by something like this (not tested):

-- backup last TX byte in case FIFO runs empty --
tx_backup: process(rstn_i, clk_i)
begin
  if (rstn_i = '0') then
    tx_dummy <= (others => '0');
  elsif rising_edge(clk_i) then
    if (tx_fifo.avail = '1') and (engine.rd_re = '1') then
      tx_dummy <= tx_fifo.rdata;
    end if;
  end if;
end process tx_backup;
  
-- send backup byte (last pushed byte) if TX FIFO is drained --
engine.rdata <= tx_fifo.rdata when (tx_fifo.avail = '1') else tx_dummy; 

What do you think about this?

Yes. According to chapter 3.1.6 of the I2C-bus specification and user manual, it is not so easy to say what is correct:

Ok, so all choices are "legal". However, I suggest to stick with the very simple interface protocol (always send an ACK repsonse if the correct address is received):

I think an I²C device should always be discoverable - no matter what its current internal state is. This also feels more straightforward (but that's just me). 🙈

[stnolting]

Damn it, the bootloader gets too big again. Should we again disable the new option as default?

Sorry, I completely overlooked this comment...
Yes, we should disable all "fancy features" by default for the time being.

By the way, the bootloader is not limited to 4kB - it can be up to 64kB (for custom applications). In the default version, 4kB is set as the upper limit so that it can also be implemented for small FPGAs.

[LukasP46]

if the FIFO runs empty during a read access, the last valid byte is output again and again. For me, this feels more intuitive. 🤔

Sounds like an idea. That way we can just use the existing infrastructure to load in a byte, I like that!

However, I suggest to stick with the very simple interface protocol (always send an ACK repsonse if the correct address is received)

I can understand that. I think it just depends on the use case what is the most useful response, so my suggestion is that

• we use a ctrl bit to set whether an "empty" read should return the last byte or 0xFF. I'm highly in favor of at least this option.
• we use another control bit to set whether an "empty" read should be acknowledged at all. Default is yes.
  Or well, if you agree with both options, lets encode the three options into one two-bit field.

What do you think? We only need two bits more and the user has all the options and it's the most transparent. You have to think about the situation "what should happen if there is a read operation and the FIFO is empty" anyways. ;)

Yes, we should disable all "fancy features" by default for the time being.

Alright :)

By the way, the bootloader is not limited to 4kB - it can be up to 64kB (for custom applications).

I know, I know. Actually I use the bootloader without UART or SPI and its only 1.3kB big ;)

[stnolting]

Great!

Can't we handle the 0xFF response via the FIFO as well?

[LukasP46]

Can't we handle the 0xFF response via the FIFO as well?

Technically yes, but I guess most users don't expect the last byte to get repeated and don't expect needing to feed an explicit 0xFF into the FIFO? On the other hand, we'll document it, so the user should know.

[stnolting]

we use a ctrl bit to set whether an "empty" read should return the last byte or 0xFF. I'm highly in favor of at least this option.

That sounds good. So let's add another control register bit to select the I²C read data:

• 0 (default): output 0xFF if TX FIFO is empty
• 1: output the very last byte pushed to the TX FIFO again and again if the TX FIFO is empty

we use another control bit to set whether an "empty" read should be acknowledged at all. Default is yes. Or well, if you agree with both options, lets encode the three options into one two-bit field.

I think I'm beginning to understand what this could be useful for. But then we should also implement it consistently. So how about this: add a new hide bit to the control register.

• hide = 0 (default):
  • the address phase of a READ-access generates an ACK if the address matches
  • the address phase of a WRITE-access generates an ACK if the address matches
• hide = 1:
  • the address phase of a READ-access generates an ACK only if the address matches and the TX FIFO is NOT EMPTY
  • the address phase of a WRITE-access generates an ACK only if the address matches and the RX FIFO is NOT FULL

I think we could implement this quite easy. Maybe we can just change some lines here:

 elsif (engine.cnt(3) = '1') and (smp.scl_fall = '1') then -- 8 bits received? 
   if (ctrl.device_addr = engine.sreg(7 downto 1)) then -- address match?
--------------------------------------------------
      if (ctrl.hide = '1') and -- hide mode: send no address ACK if ...
         (((engine.sreg(0) = '1') and (tx_fifo.avail = '0')) or -- READ and no TX DATA
          ((engine.sreg(0) = '0') and (rx_fifo.free  = '0'))) then -- WRITE and no RX SPACE
         engine.state <= S_IDLE;
      else
        engine.state <= S_RESP; -- access device
      end if;
--------------------------------------------------
   else 
     engine.state <= S_IDLE; -- no match, go back to idle 
   end if; 
 end if; 

What do you think about this?

[LukasP46]

Okay, hm, thinking about RX, there might be more changes. Here is my suggestion:

• We always ACK when the address matches (as it is now)
• We always NACK during the transfer, if the FIFO is full (not implemented)

The specs states:

There are five conditions that lead to the generation of a NACK: (...)
4. During the transfer, the receiver cannot receive any more data bytes.

And also the first byte is already a transfer, so there's just no need to send a NACK during address matching, but the host then knows that the last byte sent wasn't received.

The main difference with TX is that you as the host has no way of knowing if the data received is valid or not during the transfer, as the only ACK/NACK from the device is during address matching.