Re: [PATCH v7 09/15] can: grcan: Add saving and restoring of CAN FD baud-rate registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch claims to save and restore CAN FD baud-rate registers upon reset but fails to restore the data bit-timing register (`regs->fdbtr`), causing it to be permanently wiped.
- [Medium] The `GENMASK` definitions for the GRCANFD `NBTR` register fields overlap, creating a latent bug in the hardware interface definition.
--

commit c685fd29f8a299a3b71b10448a638f1df08b5e0d
Author: Daniel Hellstrom <daniel@gaisler.com>

can: grcan: Add saving and restoring of CAN FD baud-rate registers

While reset the GRCAN baud-rates are preserved, since GRCANFD has the
baud-rate in different registers we need to add saving of those
registers too.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 6e0a393785e86..ccf4032ed642e 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -528,6 +528,8 @@ static void grcan_reset(struct net_device *dev)
>  
>  	/* Turn off hardware filtering - regs->rxcode set to 0 by reset */
>  	grcan_write_reg(&regs->rxmask, 0);
> +
> +	priv->hwcap->set_bittiming(dev);

Does this fully restore the CAN FD baud-rate registers?

The commit message indicates that the GRCANFD baud-rate registers need
to be saved and restored, but it looks like set_bittiming() only sets the
nominal bit-timing register (regs->nbtr):

drivers/net/can/grcan.c:grcanfd_set_bittiming() {
    ...
	timing |= FIELD_PREP(GRCANFD_NBTR_SJW, sjw);
	timing |= FIELD_PREP(GRCANFD_NBTR_PS1, ps1);
	timing |= FIELD_PREP(GRCANFD_NBTR_PS2, ps2);
	timing |= FIELD_PREP(GRCANFD_NBTR_SCALER, scaler);
	netdev_dbg(dev, "setting timing=0x%x\n", timing);
	grcan_write_bits(&regs->nbtr, timing, GRCANFD_NBTR_TIMING);

	return 0;
}

Since the hardware reset clears the registers, and the fast data bit-timing
register (regs->fdbtr) is not saved before the reset or recalculated here,
will regs->fdbtr be permanently wiped?

This could cause the data baud-rate scaler to become zero and break CAN FD
data phase communication if Bit Rate Switch (BRS) is enabled.

Additionally, while looking at the register field definitions used by
set_bittiming(), do the GENMASK definitions for the GRCANFD NBTR overlap?

drivers/net/can/grcan.c:
#define GRCANFD_NBTR_SCALER     GENMASK(23, 16)
#define GRCANFD_NBTR_PS1        GENMASK(17, 10)

Bits 16 and 17 are included in both GRCANFD_NBTR_SCALER and GRCANFD_NBTR_PS1.

While it seems the current maximum phase segment limit prevents active
collisions at runtime, any future increase or use of FIELD_GET could lead
to silent data corruption because the scaler and ps1 bits bleed into
each other.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508070121.6918-1-arun.muthusamy@gaisler.com?part=9