[gpio] Incorrect register mask values in specification about opentitan HOT 9 CLOSED

Those are auto generated registers. Looks like there is a bug in the bit expansion.

The bogus Register enable= should not be there anymore (it isn't in the version on bubble), the fix for that went in a couple of weeks ago.

from opentitan.

Hi Mark,

Thanks for your reply. I see "Register enable=" removed after refreshing the page.
Can you please also check if mask 0x0 for gpio.DATA_IN is correct value?

Best Regards,
Gaurang

from opentitan.

@mdhayter @eunchan - Can you please clarify on mask values for INTR_STATE, INTR_ENABLE and INTR_TEST? For GPIO, all 32-bits of these register are writable, so I believe their mask values should be 0xffff_ffff.
I see that corresponding UART registers have mask values equal to 0xff as bits [7:0] are writable.

from opentitan.

Sorry for the delayed response. Could you please point the location of mask info? I cannot find in the c header, nor RTL (gpio_reg_pkg.sv)

from opentitan.

Sorry for missing your reply on this, please refer to following part in specification:

I am not able to point to the same in gpio.md file, but I can see the following in uart.md:

void uart_interrupt_routine() {
  volatile uint32 intr_state = *UART_INTR_STATE_REG;
  uint32 intr_state_mask = 0;
  char uart_ch;
  uint32 intr_enable_reg;

  // Turn off Interrupt Enable
  intr_enable_reg = *UART_INTR_ENABLE_REG;
  *UART_INTR_ENABLE_REG = intr_enable_reg & 0xFFFFFF00; // Clr bits 7:0

  if (intr_state & UART_INTR_STATE_RX_PARITY_ERR_MASK) {
    // Do something ...

    // Store Int mask
    intr_state_mask |= UART_INTR_STATE_RX_PARITY_ERR_MASK;
  }

  if (intr_state & UART_INTR_STATE_RX_BREAK_ERR_MASK) {
    // Do something ...

    // Store Int mask
    intr_state_mask |= UART_INTR_STATE_RX_BREAK_ERR_MASK;
  }

  // .. Frame Error

  // TX/RX Overflow Error

  // RX Int
  if (intr_state & UART_INTR_STATE_RX_WATERMARK_MASK) {
    while(1) {
      uart_ch = uart_rcv_char();
      if (uart_ch == 0xff) break;
      uart_buf.append(uart_ch);
    }
    // Store Int mask
    intr_state_mask |= UART_INTR_STATE_RX_WATERMARK_MASK;
  }

  // Clear Interrupt State
  *UART_INTR_STATE_REG = intr_state_mask;

  // Restore Interrupt Enable
  *UART_INTR_ENABLE_REG = intr_enable_reg;
}

I am not sure if this is relevant (I do not understand *.md files that well), but I think we are populating mask value for INTR_STATE register.
We do not have similar thing in gpio or rv_timer INTR_STATE registers and I think because of this we are seeing default value of mask which is 0x1.
In case of rv_timer, it is correct because there is only single interrupt bit that can be updated and written.
However, gpio has all bits of INTR_STATE register that can be set and cleared.
The same problem exists in INTR_TEST and INTR_ENABLE registers, too.

from opentitan.

That is due to the multibit of interrupt signal. Let me take a look at auto interrupt register generation code.

from opentitan.

@eunchan this was discussed in the GPIO V3 signoff review. Please create an new issue on reggen update, and close this. Thx.

from opentitan.

@eunchan, ping.

from opentitan.

@eunchan what is the status on this old bug?

from opentitan.