On 6/30/20 4:25 AM, Joakim Zhang wrote:
> I have also noticed this difference, although this could not break function,
> but IMO, using priv->can.ctrlmode should be better.
> 
> Some nitpicks:
>
> 1) Can we use uniform check for HW which supports CAN FD in the driver, using
> priv->can.ctrlmode_supported instead of quirks?>
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1392,7 +1392,7 @@ static int flexcan_chip_start(struct net_device *dev)
>                 priv->write(reg_ctrl2, &regs->ctrl2);
>         }
> 
> -       if ((priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD)) {
> +       if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {

makes sense

>                 u32 reg_fdctrl;
> 
>                 reg_fdctrl = priv->read(&regs->fdctrl);
> 
> Also delete the redundant parentheses here.
> 
> 2) Clean timing register.
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1167,6 +1167,14 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
>         struct flexcan_regs __iomem *regs = priv->regs;
>         u32 reg_cbt, reg_fdctrl;
> 
> +       reg_cbt = priv->read(&regs->cbt);
> +       reg_cbt &= ~(FLEXCAN_CBT_BTF |
> +               FIELD_PREP(FLEXCAN_CBT_EPRESDIV_MASK, 0x3ff) |
> +               FIELD_PREP(FLEXCAN_CBT_ERJW_MASK, 0x1f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPROPSEG_MASK, 0x3f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPSEG1_MASK, 0x1f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPSEG2_MASK, 0x1f));
> +

Why is this needed? The "reg_cbt &=" sets reg_cbt basically to 0, as the fields
and the BTF occupy all 32bit.

The only thing that's left over is the read()....

>         /* CBT */
>         /* CBT[EPSEG1] is 5 bit long and CBT[EPROPSEG] is 6 bit
>          * long. The can_calc_bittiming() tries to divide the tseg1
> @@ -1192,6 +1200,13 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
>         if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
>                 u32 reg_fdcbt;
> 
> +               reg_fdcbt = priv->read(&regs->fdcbt);
> +               reg_fdcbt &= ~(FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, 0x3ff) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FRJW_MASK, 0x7) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPROPSEG_MASK, 0x1f) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG1_MASK, 0x7) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG2_MASK, 0x7));
> +

Okay, I'll add this, as the fdcbt contains some reserved bits...Let's preserve them.

I've changed the setting of reg_fdcbt like this to make sense:

> -               reg_fdcbt = FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, dbt->brp - 1) |
> +               reg_fdcbt |= FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, dbt->brp - 1) |

thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |