[Sival, spi_dev] Tpm mode

[engdoreis]

This PR port the spi_device_tpm_tx_rx_test for Sival.

Note: This does not work on the current Cw310 + Hyperdebug for reasons explained here.

[a-will]

The changes so far are fine, but we'll need the host part for the test to work on the FPGA. :)

[engdoreis]

I rebased the PR. Regarding the changes needed on the FPGAs, I marked this test as broken and we can merge it. This way It can be used with Silicon and manual runs on FPGAs with the MOD.

[pamaury]

Is this just coding style or is this necessary for proper synchronization of the DV test?

[pamaury]

Stupid question: I would have expected the console name and the strapping name to be related somehow. Or is SPI_TPM a strapping for all SPIs? Or maybe should there be an alias for the SPI TPM in opentitanlib?

[a-will]

This has nothing to do with the console, actually. It's a strapping for the debugger to set itself up to drive the TPM portion of spi_device on earlgrey, which has a separate GPIO for the chip select and shares all the other pins with the BOOTSTRAP SPI configuration.

The BOOTSTRAP and TPM SPI devices cannot be used simultaneously. On hyperdebug, they are actually driven by entirely different IPs: There is no one SPI IP in that MCU that can handle both use cases.

The strapping config is here:

opentitan/sw/host/opentitanlib/src/app/config/hyperdebug_cw310.json

Lines 44 to 76 in c8de39a

	"name": "SPI_TPM",
	"pins": [
	{
	"name": "SPI_DEV_SCK",
	"mode": "Input"
	},
	{
	"name": "SPI_DEV_D0",
	"mode": "Input"
	},
	{
	"name": "SPI_DEV_D1",
	"mode": "Input"
	},
	{
	"name": "SPI_TPM_SCK",
	"mode": "Alternate"
	},
	{
	"name": "SPI_TPM_MOSI",
	"mode": "Alternate"
	},
	{
	"name": "SPI_TPM_MISO",
	"mode": "Alternate"
	},
	{
	"name": "SPI_TPM_CSB",
	"mode": "PushPull"
	}
	]
	}
	],

[jwnrt]

Looks correct to me, thanks @engdoreis

[jwnrt]

Should we add something like else { CHECK(false, "unsupported test platform %d", kDeviceType) } here?

[jwnrt]

I'm not sure I understand the test properly - why does this run 10 times? Just for redundantly checking it works?

[a-will]

I believe the original test would randomize the transactions and do 10 different ones. This logic might need to be updated so it doesn't do 10x of the same transaction, hehe.

[pamaury]

Overall it looks good to me but the commits do not seem to be atomic: there are DV changes in SiVal commits, some restyling changes mixed-up with other changes. I would suggest to rewrite the commit history maybe?

[pamaury]

I have an unrelated question: is the code in atomic_wait_for_interrupt really correct?

static void atomic_wait_for_interrupt(void) {
  irq_global_ctrl(false);
  if (!header_interrupt_received) {
    wait_for_interrupt();
  }
  irq_global_ctrl(true);
}

Since interrupts are globally disabled, an interrupt will wake up the core at wait_for_interrupt but I believe that it won't be serviced. Therefore it seems to me that if the code actually enters the loop, it will get stuck. It should probably re-enabled and then re-disabled the interrupts immediately after wait_for_interrupt?

[engdoreis]

I have an unrelated question: is the code in atomic_wait_for_interrupt really correct?

static void atomic_wait_for_interrupt(void) {
  irq_global_ctrl(false);
  if (!header_interrupt_received) {
    wait_for_interrupt();
  }
  irq_global_ctrl(true);
}

Since interrupts are globally disabled, an interrupt will wake up the core at wait_for_interrupt but I believe that it won't be serviced. Therefore it seems to me that if the code actually enters the loop, it will get stuck. It should probably re-enabled and then re-disabled the interrupts immediately after wait_for_interrupt?

I think it is correct, the wfi only runs if no IRQ has fired. When it fires, the core wakes up, enables the global IRQ which allows it to be serviced and update the variable header_interrupt_received. Does that make sense?
Here's another place with an approach similar with what you suggested, but in this case undesirable IRQs can fire, but the test should not fail because of it:

opentitan/sw/device/tests/aon_timer_irq_test.c

Lines 132 to 145 in c8de39a

	// Only enter WFI loop if we haven't already seen the interrupt.
	if (peripheral != kTopEarlgreyPlicPeripheralAonTimerAon) {
	do {
	wait_for_interrupt();
	// WFI ignores global interrupt enable, so enable it now and then
	// immediately disable it. If there is an interrupt pending it will be
	// taken here between the enable and disable. This confines the interrupt
	// to a known place avoiding missed wakeup issues.
	irq_global_ctrl(true);
	irq_global_ctrl(false);
	time_elapsed = (uint32_t)irq_tick - start_tick;
	} while (peripheral != kTopEarlgreyPlicPeripheralAonTimerAon &&
	time_elapsed < sleep_range_h);
	}

[a-will]

I have an unrelated question: is the code in atomic_wait_for_interrupt really correct?

static void atomic_wait_for_interrupt(void) {
  irq_global_ctrl(false);
  if (!header_interrupt_received) {
    wait_for_interrupt();
  }
  irq_global_ctrl(true);
}

Since interrupts are globally disabled, an interrupt will wake up the core at wait_for_interrupt but I believe that it won't be serviced. Therefore it seems to me that if the code actually enters the loop, it will get stuck. It should probably re-enabled and then re-disabled the interrupts immediately after wait_for_interrupt?

Some clarification here: You're correct that the interrupt won't be serviced while interrupts are "globally" disabled, but this is a misnomer. The only thing that is disabled is Ibex's jump to an ISR. The PLIC has still latched the pending interrupt and is maintaining the level signaling to Ibex (irq_global_ctrl() does nothing to the PLIC's handling of interrupts).

So, if an interrupt broke us out of WFI, we will immediately jump to its ISR once irq_global_ctrl(true) happens. However, this handling isn't quite right, as it forgets to check the condition again...

[a-will]

I have an unrelated question: is the code in atomic_wait_for_interrupt really correct?

static void atomic_wait_for_interrupt(void) {
  irq_global_ctrl(false);
  if (!header_interrupt_received) {
    wait_for_interrupt();
  }
  irq_global_ctrl(true);
}

Since interrupts are globally disabled, an interrupt will wake up the core at wait_for_interrupt but I believe that it won't be serviced. Therefore it seems to me that if the code actually enters the loop, it will get stuck. It should probably re-enabled and then re-disabled the interrupts immediately after wait_for_interrupt?

I think it is correct, the wfi only runs if no IRQ has fired. When it fires, the core wakes up, enables the global IRQ which allows it to be serviced and update the variable header_interrupt_received. Does that make sense? Here's another place with an approach similar with what you suggested, but in this case undesirable IRQs can fire, but the test should not fail because of it:

opentitan/sw/device/tests/aon_timer_irq_test.c

Lines 132 to 145 in c8de39a

	// Only enter WFI loop if we haven't already seen the interrupt.
	if (peripheral != kTopEarlgreyPlicPeripheralAonTimerAon) {
	do {
	wait_for_interrupt();
	// WFI ignores global interrupt enable, so enable it now and then
	// immediately disable it. If there is an interrupt pending it will be
	// taken here between the enable and disable. This confines the interrupt
	// to a known place avoiding missed wakeup issues.
	irq_global_ctrl(true);
	irq_global_ctrl(false);
	time_elapsed = (uint32_t)irq_tick - start_tick;
	} while (peripheral != kTopEarlgreyPlicPeripheralAonTimerAon &&
	time_elapsed < sleep_range_h);
	}

It's not quite right. The condition must always be checked before proceeding. We cannot assume that exiting WFI means we got the event we were waiting for.

This test is also able to get interrupts that break out of WFI but don't touch header_interrupt_received. For example, a debugger issuing a halt will cause an ebreak, which wakes up Ibex to execute instructions in debug mode. If we don't check header_interrupt_received and loop back, the test can continue before the proper time and fail. This would make debugging issues really annoying. ;)

So, instead, it should probably look something like this:

static void atomic_wait_for_interrupt(void) {
  do {
    // Because this next check's result is to determine whether to go to WFI,
    // the check and WFI instruction must be atomic.
    irq_global_ctrl(false);
    if (!header_interrupt_received) {
      wait_for_interrupt();
    }
    irq_global_ctrl(true);
  // But don't proceed unless we actually have received a command!
  } while (!header_interrupt_received);
}

[a-will]

LGTM, modulo the CI failures. It'd probably be good to fix up that atomic_wait_for_interrupt() function, too, though.

[a-will]

This could probably be punted to another PR, but we might want randomly-generated stimulus to be provided by some PRNG that is fed by a random (but controllable) seed. Otherwise, failures can become a bit more annoying to reproduce.

[github-actions]

Backport failed for earlgrey_es_sival, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_es_sival
git worktree add -d .worktree/backport-20679-to-earlgrey_es_sival origin/earlgrey_es_sival
cd .worktree/backport-20679-to-earlgrey_es_sival
git switch --create backport-20679-to-earlgrey_es_sival
git cherry-pick -x 114700c5e43c1614d13a216c6b4d4c5cab0066e0 8ad34f8987839f7b7b2d280871c007d24bf4c20c 76061b5e3f464adae3140abf3c21d511fa781bd4 044c4671520142de47082845b28cd48fc3ff6a93 6373b9c7e69ad5993b5d613b3f880355b44211ca fa0a6d5f10f3a57714334f57ae3eda7aab7a0bfe