Re: [PATCH v2 08/12] nova-core: sequencer: Add register opcodes

[Joel Fernandes <joelagnelf@nvidia.com>]

On 11/10/2025 8:50 AM, Alexandre Courbot wrote:
[...]
>> +
>> +        // First read.
>> +        sequencer.bar.try_read32(addr)?;
>> +
>> +        // Poll the requested register with requested timeout.
>> +        read_poll_timeout(
>> +            || sequencer.bar.try_read32(addr),
>> +            |current| (current & self.mask) == self.val,
>> +            Delta::ZERO,
>> +            Delta::from_micros(timeout_us),
>> +        )
>> +        .map(|_| ())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr as usize;
>> +        let _index = self.index;
>> +
>> +        let val = sequencer.bar.try_read32(addr)?;
>> +
>> +        dev_dbg!(
>> +            sequencer.dev,
>> +            "RegStore: addr=0x{:x}, index=0x{:x}, value={:?}\n",
>> +            self.addr,
>> +            self.index,
>> +            val
>> +        );
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for GspSeqCmd {
>> +    fn run(&self, seq: &GspSequencer<'_>) -> Result {
>> +        match self {
>> +            GspSeqCmd::RegWrite(cmd) => cmd.run(seq),
>> +            GspSeqCmd::RegModify(cmd) => cmd.run(seq),
>> +            GspSeqCmd::RegPoll(cmd) => cmd.run(seq),
>> +            GspSeqCmd::RegStore(cmd) => cmd.run(seq),
>> +        }
>> +    }
>> +}
> 
> This makes me wonder: do we need to store the deserialized version of
> these operands, and make a second `match` on them? How about passing the
> `bar` to the deserialization command and have it run the operand
> immediately?

I don't think a match could be avoided here because of Rust enums (cannot call a
method on the inner object of an enum without matching it first). The first
match is to construct the enum by matching on the opcode, the second match is to
run it by matching on the enum type.

Sorry if I missed something about your suggestion but I suggest lets do that
kind of refactor after the initial merge, since this code has been working and
tested for some time.

In hindsight, I probably would have tried to not use enums and my feeling is we
can probably get rid of it eventually and do this kind of function delegation
more ergonomically.

Thanks.