| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250527-sage-python-of-variation-1c7759-mkl@pengutronix.de>
Date: Tue, 27 May 2025 13:37:09 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Elaine Zhang <zhangqing@...k-chips.com>
Cc: kernel@...gutronix.de, mailhol.vincent@...adoo.fr, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, heiko@...ech.de, cl@...k-chips.com,
kever.yang@...k-chips.com, linux-can@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v6 3/4] net: can: rockchip: add can for RK3576 Soc
Hey,
here's a partial review.
On 26.05.2025 14:25:58, Elaine Zhang wrote:
> Is new controller, new register layout and Bit position definition:
> Support CAN and CANFD protocol, ISO 11898-1
> Support transmit or receive error count
> Support acceptance filter, more functional
> Support interrupt and all interrupt can be masked
> Support error code check
> Support self test\silent\loop-back mode
> Support auto retransmission mode
> Support auto bus on after bus-off state
> Support 2 transmit buffers
> Support Internal Storage Mode
> Support DMA
>
> Signed-off-by: Elaine Zhang <zhangqing@...k-chips.com>
> ---
> .../net/can/rockchip/rockchip_canfd-core.c | 453 ++++++++++++++++++
> drivers/net/can/rockchip/rockchip_canfd-rx.c | 111 +++++
> drivers/net/can/rockchip/rockchip_canfd-tx.c | 27 ++
> drivers/net/can/rockchip/rockchip_canfd.h | 267 +++++++++++
> 4 files changed, 858 insertions(+)
>
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index c21ca4c1fb9a..92e260cb2527 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -31,6 +31,8 @@ static const char *__rkcanfd_get_model_str(enum rkcanfd_model model)
> return "rk3568v2";
> case RKCANFD_MODEL_RK3568V3:
> return "rk3568v3";
> + case RKCANFD_MODEL_RK3576:
> + return "rk3576";
> }
>
> return "<unknown>";
> @@ -176,6 +178,30 @@ static void rkcanfd_get_berr_counter_corrected(struct rkcanfd_priv *priv,
> !!(reg_state & RKCANFD_REG_STATE_ERROR_WARNING_STATE));
> }
>
> +static void rk3576canfd_get_berr_counter_corrected(struct rkcanfd_priv *priv,
> + struct can_berr_counter *bec)
> +{
Is the rk3576 affected by this problem?
> + struct can_berr_counter bec_raw;
> + u32 reg_state;
> +
> + bec->rxerr = rkcanfd_read(priv, RK3576CANFD_REG_RXERRORCNT);
> + bec->txerr = rkcanfd_read(priv, RK3576CANFD_REG_TXERRORCNT);
> + bec_raw = *bec;
> +
> + if (!bec->rxerr && !bec->txerr)
> + *bec = priv->bec;
> + else
> + priv->bec = *bec;
> +
> + reg_state = rkcanfd_read(priv, RKCANFD_REG_STATE);
> + netdev_vdbg(priv->ndev,
> + "%s: Raw/Cor: txerr=%3u/%3u rxerr=%3u/%3u Bus Off=%u Warning=%u\n",
> + __func__,
> + bec_raw.txerr, bec->txerr, bec_raw.rxerr, bec->rxerr,
> + !!(reg_state & RK3576CANFD_REG_STATE_BUS_OFF_STATE),
> + !!(reg_state & RK3576CANFD_REG_STATE_ERROR_WARNING_STATE));
> +}
> +
> static int rkcanfd_get_berr_counter(const struct net_device *ndev,
> struct can_berr_counter *bec)
> {
> @@ -206,6 +232,11 @@ static void rkcanfd_chip_interrupts_disable(const struct rkcanfd_priv *priv)
> rkcanfd_write(priv, RKCANFD_REG_INT_MASK, RKCANFD_REG_INT_ALL);
> }
>
> +static void rk3576canfd_chip_interrupts_disable(const struct rkcanfd_priv *priv)
> +{
> + rkcanfd_write(priv, RK3576CANFD_REG_INT_MASK, RK3576CANFD_REG_INT_ALL);
> +}
> +
> static void rkcanfd_chip_fifo_setup(struct rkcanfd_priv *priv)
> {
> u32 reg;
> @@ -220,6 +251,72 @@ static void rkcanfd_chip_fifo_setup(struct rkcanfd_priv *priv)
> netdev_reset_queue(priv->ndev);
> }
>
> +static void rk3576canfd_chip_fifo_setup(struct rkcanfd_priv *priv)
> +{
> + u32 ism = 0, water = 0;
no need to init as 0
> +
> + ism = RK3576CANFD_REG_STR_CTL_ISM_SEL_CANFD_FIXED;
> + water = RK3576CANFD_ISM_WATERMASK_CANFD;
> +
> + /* internal sram mode */
personally I would prefer:
reg_ism = FIELD_PREP(RK3576CANFD_REG_STR_CTL_ISM_SEL,
RK3576CANFD_REG_STR_CTL_ISM_SEL_CANFD_FIXED) |
RK3576CANFD_REG_STR_CTL_STORAGE_TIMEOUT_MODE;
> + rkcanfd_write(priv, RK3576CANFD_REG_STR_CTL,
> + (FIELD_PREP(RK3576CANFD_REG_STR_CTL_ISM_SEL, ism) |
> + RK3576CANFD_REG_STR_CTL_STORAGE_TIMEOUT_MODE));
reg_water = RK3576CANFD_ISM_WATERMASK_CANFD;
> + rkcanfd_write(priv, RK3576CANFD_REG_STR_WTM, water);
> + WRITE_ONCE(priv->tx_head, 0);
> + WRITE_ONCE(priv->tx_tail, 0);
> + netdev_reset_queue(priv->ndev);
> +}
> +
> +static int rk3576canfd_atf_config(struct rkcanfd_priv *priv, int mode)
With the proposed cleanup, this will become a void function.
> +{
> + u32 id[10] = {0};
What's the use of this array?
> + u32 dlc = 0, dlc_over = 0;
> +
> + switch (mode) {
It's called with RK3576CANFD_REG_ATFM_MASK_SEL_MASK_MODE only...
> + case RK3576CANFD_REG_ATFM_MASK_SEL_MASK_MODE:
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF0, id[0]);
why not call it with 0x0?
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF1, id[1]);
create:
#define RK3576CANFD_REG_ATF(n) (0x700 + (n) << 2)
and use a for loop
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF2, id[2]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF3, id[3]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF4, id[4]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM0, RK3576CANFD_REG_ATFM_ID);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM1, RK3576CANFD_REG_ATFM_ID);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM2, RK3576CANFD_REG_ATFM_ID);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM3, RK3576CANFD_REG_ATFM_ID);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM4, RK3576CANFD_REG_ATFM_ID);
> + break;
> + case RK3576CANFD_REG_ATFM_MASK_SEL_LIST_MODE:
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF0, id[0]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF1, id[1]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF2, id[2]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF3, id[3]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF4, id[4]);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM0, id[5] | RK3576CANFD_REG_ATFM_MASK_SEL);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM1, id[6] | RK3576CANFD_REG_ATFM_MASK_SEL);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM2, id[7] | RK3576CANFD_REG_ATFM_MASK_SEL);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM3, id[8] | RK3576CANFD_REG_ATFM_MASK_SEL);
> + rkcanfd_write(priv, RK3576CANFD_REG_ATFM4, id[9] | RK3576CANFD_REG_ATFM_MASK_SEL);
> + break;
> + default:
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF_CTL, RK3576CANFD_REG_ATF_CTL_ATF_DIS_ALL);
> + return -EINVAL;
> + }
> +
> + if (dlc) {
> + if (dlc_over)
both are 0, please remove the dead code
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF_DLC,
> + dlc | RK3576CANFD_REG_ATF_DLC_ATF_DLC_EN);
> + else
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF_DLC,
> + dlc | RK3576CANFD_REG_ATF_DLC_ATF_DLC_EN |
> + RK3576CANFD_REG_ATF_DLC_ATF_DLC_MODE);
> + }
> + rkcanfd_write(priv, RK3576CANFD_REG_ATF_CTL, 0);
> +
> + return 0;
> +}
> +
> static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
> {
> u32 reg;
> @@ -285,6 +382,68 @@ static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
> rkcanfd_read(priv, RKCANFD_REG_MODE));
> }
>
> +static void rk3576canfd_chip_start(struct rkcanfd_priv *priv)
> +
> +{
> + u32 reg;
> +
> + rkcanfd_chip_set_reset_mode(priv);
> +
> + /* Receiving Filter: accept all */
> + rk3576canfd_atf_config(priv, RK3576CANFD_REG_ATFM_MASK_SEL_MASK_MODE);
> +
> + /* enable:
> + * - CAN_FD: enable CAN-FD
> + * - AUTO_RETX_MODE: auto retransmission on TX error
> + * - COVER_MODE: RX-FIFO overwrite mode, do not send OVERLOAD frames
> + * - RXSTX_MODE: Receive Self Transmit data mode
> + * - WORK_MODE: transition from reset to working mode
> + */
please adjust the comments
> + reg = rkcanfd_read(priv, RKCANFD_REG_MODE);
> + priv->reg_mode_default = reg | RKCANFD_REG_MODE_WORK_MODE;
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> + priv->reg_mode_default |= RKCANFD_REG_MODE_LBACK_MODE;
> + rkcanfd_write(priv, RK3576CANFD_REG_ERROR_MASK,
> + RK3576CANFD_REG_ERROR_MASK_ACK_ERROR);
> + }
> +
> + /* mask, i.e. ignore:
> + * - TIMESTAMP_COUNTER_OVERFLOW_INT - timestamp counter overflow interrupt
> + * - TX_ARBIT_FAIL_INT - TX arbitration fail interrupt
> + * - OVERLOAD_INT - CAN bus overload interrupt
> + * - TX_FINISH_INT - Transmit finish interrupt
> + */
> + priv->reg_int_mask_default = RK3576CANFD_REG_INT_RX_FINISH_INT;
please adjust the comments
> +
> + /* Do not mask the bus error interrupt if the bus error
> + * reporting is requested.
> + */
> + if (!(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
> + priv->reg_int_mask_default |= RKCANFD_REG_INT_ERROR_INT;
> +
> + memset(&priv->bec, 0x0, sizeof(priv->bec));
> +
> + priv->devtype_data.fifo_setup(priv);
Why do you need a callback here? You're already know you're a
rk3576canfd, here.
> +
> + rkcanfd_write(priv, RK3576CANFD_REG_AUTO_RETX_CFG,
> + RK3576CANFD_REG_AUTO_RETX_CFG_AUTO_RETX_EN);
> +
> + rkcanfd_write(priv, RK3576CANFD_REG_BRS_CFG,
> + RK3576CANFD_REG_BRS_CFG_BRS_NEGSYNC_EN |
> + RK3576CANFD_REG_BRS_CFG_BRS_POSSYNC_EN);
> +
> + rkcanfd_set_bittiming(priv);
> +
> + priv->devtype_data.interrupts_disable(priv);
> + rkcanfd_chip_set_work_mode(priv);
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + netdev_dbg(priv->ndev, "%s: reg_mode=0x%08x\n", __func__,
> + rkcanfd_read(priv, RKCANFD_REG_MODE));
> +}
> +
more later...
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists