On 10/20/25 11:55 AM, Joel Fernandes wrote: > Add support for managing GSP falcon interrupts. These are required for > GSP message queue interrupt handling. > > Also rename clear_swgen0_intr() to enable_msq_interrupt() for > readability.
Hi Joel, I have a few comments below, including one that doesn't apply to you, but to Alex Courbot. Also, other than some trivia below, I can't find any problems with this patch, other than possibly the above commit message wording, so regardless of what we do with the .alter() method, please feel free to add: Reviewed-by: John Hubbard <[email protected]> > > Signed-off-by: Joel Fernandes <[email protected]> > --- > drivers/gpu/nova-core/falcon/gsp.rs | 26 +++++++++++++++++++++++--- > drivers/gpu/nova-core/gpu.rs | 2 +- > drivers/gpu/nova-core/regs.rs | 10 ++++++++++ > 3 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/nova-core/falcon/gsp.rs > b/drivers/gpu/nova-core/falcon/gsp.rs > index f17599cb49fa..6da63823996b 100644 > --- a/drivers/gpu/nova-core/falcon/gsp.rs > +++ b/drivers/gpu/nova-core/falcon/gsp.rs > @@ -22,11 +22,31 @@ impl FalconEngine for Gsp { > } > > impl Falcon<Gsp> { > - /// Clears the SWGEN0 bit in the Falcon's IRQ status clear register to > - /// allow GSP to signal CPU for processing new messages in message queue. > - pub(crate) fn clear_swgen0_intr(&self, bar: &Bar0) { > + /// Enable the GSP Falcon message queue interrupt (SWGEN0 interrupt). > + #[expect(dead_code)] > + pub(crate) fn enable_msgq_interrupt(&self, bar: &Bar0) { > + regs::NV_PFALCON_FALCON_IRQMASK::alter(bar, &Gsp::ID, |r| > r.set_swgen0(true)); > + } Alex, this ".alter" method is misnamed, IMHO. Because for registers, The One True Way (or so I claim, haha) is to have the following methods: .read .modify, also known as RMW (read-modify-write) .write "alter" never shows up in this naming scheme. I'm going to claim that this is a bit jarring for old hardware/kernel programmers. But it's not too late: these are only used in a very few places, and entirely within nova-core, too. Can I *please* send a patch to rename "alter" to "modify", perhaps? > + > + /// Check if the message queue interrupt is pending. > + #[expect(dead_code)] > + pub(crate) fn has_msgq_interrupt(&self, bar: &Bar0) -> bool { > + regs::NV_PFALCON_FALCON_IRQSTAT::read(bar, &Gsp::ID).swgen0() > + } Joel: I am guessing that there is never a situation in which we would *disable* these interrupts, right? Just thought I'd ask. > + > + /// Clears the message queue interrupt to allow GSP to signal CPU > + /// for processing new messages. > + pub(crate) fn clear_msgq_interrupt(&self, bar: &Bar0) { > regs::NV_PFALCON_FALCON_IRQSCLR::default() > .set_swgen0(true) > .write(bar, &Gsp::ID); > } > + > + /// Acknowledge all pending GSP interrupts. > + #[expect(dead_code)] > + pub(crate) fn ack_all_interrupts(&self, bar: &Bar0) { > + // Read status and write the raw value to IRQSCLR to clear all > pending interrupts. > + let status = regs::NV_PFALCON_FALCON_IRQSTAT::read(bar, &Gsp::ID); > + regs::NV_PFALCON_FALCON_IRQSCLR::from(u32::from(status)).write(bar, > &Gsp::ID); > + } > } > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs > index af20e2daea24..fb120cf7b15d 100644 > --- a/drivers/gpu/nova-core/gpu.rs > +++ b/drivers/gpu/nova-core/gpu.rs > @@ -216,7 +216,7 @@ pub(crate) fn new<'a>( > bar, > spec.chipset > Chipset::GA100, > ) > - .inspect(|falcon| falcon.clear_swgen0_intr(bar))?, > + .inspect(|falcon| falcon.clear_msgq_interrupt(bar))?, > > sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset, bar, > true)?, > > diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs > index 206dab2e1335..a3836a01996b 100644 > --- a/drivers/gpu/nova-core/regs.rs > +++ b/drivers/gpu/nova-core/regs.rs > @@ -198,6 +198,16 @@ pub(crate) fn vga_workspace_addr(self) -> Option<u64> { > > // PFALCON > > +register!(NV_PFALCON_FALCON_IRQMASK @ PFalconBase[0x00000014] { > + 4:4 halt as bool; > + 6:6 swgen0 as bool; > +}); > + > +register!(NV_PFALCON_FALCON_IRQSTAT @ PFalconBase[0x00000008] { > + 4:4 halt as bool; > + 6:6 swgen0 as bool; > +}); > + > register!(NV_PFALCON_FALCON_IRQSCLR @ PFalconBase[0x00000004] { > 4:4 halt as bool; > 6:6 swgen0 as bool; thanks, -- John Hubbard
