[spi_device] Improvements/fixes to SPI device constants

[rivos-eblot]

Description

While reading the documentation and the code of the SPI device IP, I've found the following potential issues:

1. In hw/ip/spi_device/dv/env/spi_device_env_pkg.sv:

   parameter uint CMD_FIFO_SIZE                   = 32;
   parameter uint ADDR_FIFO_SIZE                  = 32;

   32 does not seem to match any units used elsewhere. Both bytes (64) and depth, i.e. 32-bit words (16) are used but 32 does not seem valid.

2. In hw/ip/spi_device/rtl/spi_device_reg_pkg.sv:

   parameter int unsigned        SPI_DEVICE_INGRESS_BUFFER_SIZE   = 'h 1a0;

   however in hw/ip/spi_device/dv/env/spi_device_env_pkg.sv

   parameter uint SRAM_INGRESS_SIZE               = 256 + 64 + 64; // 96 depth

   that is 384 = 0x180 not 0x1a0

3. In hw/ip/spi_device/rtl/spi_device_pkg.sv

   SramCmdFifoDepth = 16; // 16 x 1B for Cmd
   SramAddrFifoDepth = 16; // 16 x 4B for Addr
   SramTotalDepth = SramMsgDepth + SramMailboxDepth
              + SramSfdpDepth + SramPayloadDepth
              + SramCmdFifoDepth + SramAddrFifoDepth ;

   At first glance, it seems weird to add SramCmdFifoDepth and SramAddrFifoDepth, given the comments. The comment actually means that 1-byte Cmd is first extended to a 4-byte value before being added to the 32-bit command FIFO, which is itself only reflected as a 8-bit value in UPLOAD_CMDFIFO register.

[rivos-eblot]

It also seems that EN4B/EX4B are treated as a special case, but WREN/WRDI seem to also follow the same exceptions and are not documented as such, e.g.:

• Command Upload "The command does not match to the first 11 command information entries nor EN4B/ EX4B."
  • Would the upload bit be considered for WREN/WRDI?
• Internally processed Commands "The HW can process Read Status, Read JEDEC ID, Read SFDP, Read commands accessing the mailbox region, and EN4B/EX4B." -> for WREN/WRDI, it seems the HW processes them by updating WEL.
  • Should they be handled over to SW thanks to the UPLOAD bit?
• 4B Address Management (EN4B/ EX4B) "_The two CSRs omit unnecessary fields from the CMDINFO data structure. "
  • it seems WREN/WRDI sibling fields have also been removed

• Command Parser "Other than the first 11 slots and last two slots (the last two slots are not visible to SW), the cmdparse checks the upload field and activates the upload module if the field is set."

  • it seems this applies to the last four slots. Not sure what those two last slots refer: The exposed registers show the following slots:
    • CMD_INFO_EN4B
    • CMD_INFO_EX4B
    • CMD_INFO_WREN
    • CMD_INFO_WRDI

  The last two slots are WREN and WRDI. Moreover they are visible as partially configurable slots to the SW. Or are they two extra ones that are actually hidden?

[a-will]

I think the WREN / WRDI CMD_INFO CSRs were added later, and the various ancillary comments about making EN4B / EX4B special cases didn't get updated to include those two. These are documented in https://opentitan.org/book/hw/ip/spi_device/doc/theory_of_operation.html#status-control though.

[rivos-eblot]

BTW, it is a bit "weird" that the WEL is not documented as a special bit in the FLASH_STATUS register as the BUSY bit is: IIUC, FLASH_STATUS content has not special meaning for the HW, which only allow the FW and the SPI host to exchange status for all bits but BUSY and WEL, as both are also managed by the HW, since WREN and WRDI beahavior is also controlled by the HW. "WEL bit can be controlled by SW and also by HW."

I would believe that in https://opentitan.org/book/hw/ip/spi_device/doc/registers.html#flash_status, bit 1 is also documented as a special bit the same way BUSY is, leaving status using 23:2 bits.