Re: [PATCH v6 2/4] can: rockchip: add RK3588 CAN support

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] The rockchip_canfd driver unconditionally enables CAN-FD mode in t=
he hardware, even if the user only requests Classic CAN mode. However, the =
data bit timing registers are only initialized if the user explicitly reque=
sts CAN-FD. This leaves the hardware configured to process CAN-FD frames bu=
t with uninitialized data bit timing, causing it to lose synchronization an=
d transmit error frames, which disrupts valid CAN-FD traffic on the bus.
--

--- Patch [2]: [PATCH v6 2/4] can: rockchip: add RK3588 CAN support ---
commit c9bd120b397e60634c9f6a44ebe7ae2f872b4274
Author: Cunhao Lu <[email protected]>

can: rockchip: add RK3588 CAN support

Add support for the RK3588 CAN controller by introducing a dedicated
model ID and OF match entry. Enable existing erratum 5 and 6 workarounds
and enable CAN-FD for RK3588.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net=
/can/rockchip/rockchip_canfd-core.c
> index 29de0c01e4edc..37c1c22c40c9f 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
[ ... ]
> @@ -148,6 +156,12 @@ static int rkcanfd_set_bittiming(struct rkcanfd_priv=
 *priv)
> =20
>  	rkcanfd_write(priv, RKCANFD_REG_FD_DATA_BITTIMING, reg_dbt);
> =20
> +	/* RK3588 CAN-FD BRS works with TDC disabled. */
> +	if (priv->devtype_data.model =3D=3D RKCANFD_MODEL_RK3588) {
> +		rkcanfd_write(priv, RKCANFD_REG_TRANSMIT_DELAY_COMPENSATION, 0);
> +		return 0;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but it appears there is a
pre-existing issue with how CAN-FD is enabled that affects both RK3568 and
the newly added RK3588 controllers.

In drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_start(), the
hardware is unconditionally configured to enable CAN-FD mode by setting
RKCANFD_REG_MODE_CAN_FD_MODE_ENABLE.

However, in rkcanfd_set_bittiming() (just above the changes in this hunk),
if the user configures the interface in Classic CAN mode, the initialization
of data bit timing registers is skipped:

	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD))
		return 0;

Does this leave the hardware configured to process CAN-FD frames but with
uninitialized data bit timing?

If a valid CAN-FD frame with BRS is sent by another node on the bus, could
this controller attempt to switch to the uninitialized data bit timing, fai=
l,
and actively transmit an error frame, disrupting valid CAN-FD traffic?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260730-master-v6-=
[email protected]?part=3D2