> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Monday, August 13, 2018 3:21 PM > To: Pankaj Bansal <pankaj.bansal@xxxxxxx>; linux-can@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v4 5/5] net: can: flexcan: can FD Format (FDF) changes > > On 08/12/2018 06:14 PM, Pankaj Bansal wrote: > > Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > --- > > > > Notes: > > V4: > > - Make FD changes part of the groundwork changes > > - Clear all the bits in can bit timings befor setting bit timings > > - use DRV_NAME in can_bittimings_const data structures > > V3: > > - No V3 > > V2: > > - Added Bit Rate switch changes > > - Added TDC changes > > - Moved CAN FD control register from flexcan_chip_start to > > flexcan_set_bittimings > > - Added data_bittimings_const structure > > > > drivers/net/can/flexcan.c | 223 ++++++++++++++++++++++++++++++---- > -- > > 1 file changed, 189 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index 52c643d07f87..3ae790a9ccfb 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -49,6 +49,7 @@ > > #define FLEXCAN_MCR_IRMQ BIT(16) > > #define FLEXCAN_MCR_LPRIO_EN BIT(13) > > #define FLEXCAN_MCR_AEN BIT(12) > > +#define FLEXCAN_MCR_FDEN BIT(11) > > /* MCR_MAXMB: maximum used MBs is MAXMB + 1 */ > > #define FLEXCAN_MCR_MAXMB(x) ((x) & 0x7f) > > #define FLEXCAN_MCR_IDAM_A (0x0 << 8) > > @@ -133,6 +134,29 @@ > > (FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \ > > FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT) > > > > +/* FLEXCAN CAN Bit Timing Register (CBT) bits */ > > +#define FLEXCAN_CBT_BTF BIT(31) > > +#define FLEXCAN_CBT_EPRESDIV(x) (((x) & 0x3ff) << 21) > > +#define FLEXCAN_CBT_ERJW(x) (((x) & 0x0f) << 16) > > +#define FLEXCAN_CBT_EPROPSEG(x) (((x) & 0x3f) << 10) > > +#define FLEXCAN_CBT_EPSEG1(x) (((x) & 0x1f) << 5) > > +#define FLEXCAN_CBT_EPSEG2(x) ((x) & 0x1f) > > + > > +/* FLEXCAN CAN FD control Register (FDCTRL) bits */ > > +#define FLEXCAN_FDCTRL_FDRATE BIT(31) > > +#define FLEXCAN_FDCTRL_MBDSR1(x) (((x) & 0x03) << 19) > > +#define FLEXCAN_FDCTRL_MBDSR0(x) (((x) & 0x03) << 16) > > +#define FLEXCAN_FDCTRL_TDCEN BIT(15) > > +#define FLEXCAN_FDCTRL_TDCFAIL BIT(14) > > +#define FLEXCAN_FDCTRL_TDCOFF(x) (((x) & 0x1f) << 8) > > + > > +/* FLEXCAN CAN FD Bit Timing Register (FDCBT) bits */ > > +#define FLEXCAN_FDCBT_FPRESDIV(x) (((x) & 0x3ff) << 20) > > +#define FLEXCAN_FDCBT_FRJW(x) (((x) & 0x03) << 16) > > +#define FLEXCAN_FDCBT_FPROPSEG(x) (((x) & 0x1f) << 10) > > +#define FLEXCAN_FDCBT_FPSEG1(x) (((x) & 0x07) << 5) > > +#define FLEXCAN_FDCBT_FPSEG2(x) ((x) & 0x07) > > + > > /* FLEXCAN interrupt flag register (IFLAG) bits */ > > /* Errata ERR005829 step7: Reserve first valid MB */ > > #define FLEXCAN_TX_MB_RESERVED_OFF_FIFO 8 > > @@ -159,6 +183,9 @@ > > #define FLEXCAN_MB_CODE_TX_DATA (0xc << 24) > > #define FLEXCAN_MB_CODE_TX_TANSWER (0xe << 24) > > > > +#define FLEXCAN_MB_CNT_EDL BIT(31) > > +#define FLEXCAN_MB_CNT_BRS BIT(30) > > +#define FLEXCAN_MB_CNT_ESI BIT(29) > > #define FLEXCAN_MB_CNT_SRR BIT(22) > > #define FLEXCAN_MB_CNT_IDE BIT(21) > > #define FLEXCAN_MB_CNT_RTR BIT(20) > > @@ -170,15 +197,16 @@ > > /* FLEXCAN hardware feature flags > > * > > * Below is some version info we got: > > - * SOC Version IP-Version Glitch- [TR]WRN_INT IRQ Err Memory err > RTR re- > > - * Filter? connected? Passive detection ception in MB > > - * MX25 FlexCAN2 03.00.00.00 no no no no no > > - * MX28 FlexCAN2 03.00.04.00 yes yes no no no > > - * MX35 FlexCAN2 03.00.00.00 no no no no no > > - * MX53 FlexCAN2 03.00.00.00 yes no no no no > > - * MX6s FlexCAN3 10.00.12.00 yes yes no no yes > > - * VF610 FlexCAN3 ? no yes no yes yes? > > - * LS1021A FlexCAN2 03.00.04.00 no yes no no yes > > + * SOC Version IP-Version Glitch- [TR]WRN_INT IRQ Err Memory err > RTR rece- FD Mode > > + * Filter? connected? Passive detection ption in MB > Supported? > > + * MX25 FlexCAN2 03.00.00.00 no no no no no > no > > + * MX28 FlexCAN2 03.00.04.00 yes yes no no no > no > > + * MX35 FlexCAN2 03.00.00.00 no no no no no > no > > + * MX53 FlexCAN2 03.00.00.00 yes no no no no > no > > + * MX6s FlexCAN3 10.00.12.00 yes yes no no yes > no > > + * VF610 FlexCAN3 ? no yes no yes yes? no > > + * LS1021A FlexCAN2 03.00.04.00 no yes no no yes > no > > + * LX2160A FlexCAN3 03.00.23.00 no yes no no yes > yes > > * > > * Some SOCs do not have the RX_WARN & TX_WARN interrupt line > connected. > > */ > > @@ -223,7 +251,8 @@ struct flexcan_regs { > > u32 crcr; /* 0x44 */ > > u32 rxfgmask; /* 0x48 */ > > u32 rxfir; /* 0x4c */ > > - u32 _reserved3[12]; /* 0x50 */ > > + u32 cbt; /* 0x50 */ > > + u32 _reserved3[11]; /* 0x54 */ > > u8 mb[2][512]; /* 0x80 */ > > /* FIFO-mode: > > * MB > > @@ -248,6 +277,10 @@ struct flexcan_regs { > > u32 rerrdr; /* 0xaf4 */ > > u32 rerrsynr; /* 0xaf8 */ > > u32 errsr; /* 0xafc */ > > + u32 _reserved7[64]; /* 0xb00 */ > > + u32 fdctrl; /* 0xc00 */ > > + u32 fdcbt; /* 0xc04 */ > > + u32 fdcrc; /* 0xc08 */ > > }; > > > > struct flexcan_devtype_data { > > @@ -309,6 +342,12 @@ static const struct flexcan_devtype_data > fsl_ls1021a_r2_devtype_data = { > > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > > }; > > > > +static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data > = { > > + .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > + FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > + FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_USE_FD, }; > > + > > static const struct can_bittiming_const flexcan_bittiming_const = { > > .name = DRV_NAME, > > .tseg1_min = 4, > > @@ -321,6 +360,30 @@ static const struct can_bittiming_const > flexcan_bittiming_const = { > > .brp_inc = 1, > > }; > > > > +static const struct can_bittiming_const > extended_flexcan_bittiming_const = { > > + .name = DRV_NAME, > > + .tseg1_min = 2, > > + .tseg1_max = 96, > > + .tseg2_min = 2, > > + .tseg2_max = 32, > > + .sjw_max = 16, > > + .brp_min = 1, > > + .brp_max = 1024, > > + .brp_inc = 1, > > +}; > > + > > +static const struct can_bittiming_const flexcan_data_bittiming_const = { > > + .name = DRV_NAME, > > + .tseg1_min = 2, > > + .tseg1_max = 39, > > + .tseg2_min = 2, > > + .tseg2_max = 8, > > + .sjw_max = 4, > > + .brp_min = 1, > > + .brp_max = 1024, > > + .brp_inc = 1, > > +}; > > + > > /* FlexCAN module is essentially modelled as a little-endian IP in most > > * SoCs, i.e the registers as well as the message buffer areas are > > * implemented in a little-endian fashion. > > @@ -557,7 +620,13 @@ static netdev_tx_t flexcan_start_xmit(struct > sk_buff *skb, struct net_device *de > > can_id = (cfd->can_id & CAN_SFF_MASK) << 18; > > } > > > > - if (likely(!can_is_canfd_skb(skb))) { > > + if (can_is_canfd_skb(skb)) { > > + ctrl |= FLEXCAN_MB_CNT_EDL; > > + if (cfd->flags & CANFD_BRS) > > + ctrl |= FLEXCAN_MB_CNT_BRS; > > + if (cfd->flags & CANFD_ESI) > > + ctrl |= FLEXCAN_MB_CNT_ESI; > > + } else { > > if (cfd->can_id & CAN_RTR_FLAG) > > ctrl |= FLEXCAN_MB_CNT_RTR; > > } > > @@ -897,34 +966,102 @@ static void flexcan_set_bittiming(struct > > net_device *dev) { > > const struct flexcan_priv *priv = netdev_priv(dev); > > const struct can_bittiming *bt = &priv->can.bittiming; > > + const struct can_bittiming *data_bt = &priv->can.data_bittiming; > > struct flexcan_regs __iomem *regs = priv->regs; > > - u32 reg; > > + u32 reg_ctrl, reg_mcr, reg_cbt, reg_fdcbt, reg_fdctrl; > > > > - reg = priv->read(®s->ctrl); > > - reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) | > > - FLEXCAN_CTRL_RJW(0x3) | > > - FLEXCAN_CTRL_PSEG1(0x7) | > > - FLEXCAN_CTRL_PSEG2(0x7) | > > - FLEXCAN_CTRL_PROPSEG(0x7) | > > - FLEXCAN_CTRL_LPB | > > - FLEXCAN_CTRL_SMP | > > - FLEXCAN_CTRL_LOM); > > - > > - reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) | > > - FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) | > > - FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) | > > - FLEXCAN_CTRL_RJW(bt->sjw - 1) | > > - FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1); > > + reg_mcr = priv->read(®s->mcr); > > + reg_ctrl = priv->read(®s->ctrl); > > + > > + reg_ctrl &= ~(FLEXCAN_CTRL_LPB | > > + FLEXCAN_CTRL_SMP | > > + FLEXCAN_CTRL_LOM); > > > > if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) > > - reg |= FLEXCAN_CTRL_LPB; > > + reg_ctrl |= FLEXCAN_CTRL_LPB; > > if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) > > - reg |= FLEXCAN_CTRL_LOM; > > + reg_ctrl |= FLEXCAN_CTRL_LOM; > > if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > > - reg |= FLEXCAN_CTRL_SMP; > > + reg_ctrl |= FLEXCAN_CTRL_SMP; > > + > > + if (reg_mcr & FLEXCAN_MCR_FDEN) { > > + reg_cbt = priv->read(®s->cbt); > > + reg_fdcbt = priv->read(®s->fdcbt); > > + reg_fdctrl = priv->read(®s->fdctrl); > > + > > + reg_cbt |= FLEXCAN_CBT_BTF; > > + reg_cbt &= ~(FLEXCAN_CBT_EPRESDIV(0x3ff) | > > + FLEXCAN_CBT_ERJW(0x0f) | > > + FLEXCAN_CBT_EPROPSEG(0x3f) | > > + FLEXCAN_CBT_EPSEG1(0x1f) | > > + FLEXCAN_CBT_EPSEG2(0x1f)); > > + > > + reg_cbt |= FLEXCAN_CBT_EPRESDIV(bt->brp - 1) | > > + FLEXCAN_CBT_ERJW(bt->sjw - 1) | > > + FLEXCAN_CBT_EPROPSEG(bt->prop_seg - 1) | > > + FLEXCAN_CBT_EPSEG1(bt->phase_seg1 - 1) | > > + FLEXCAN_CBT_EPSEG2(bt->phase_seg2 - 1); > > + > > + reg_fdcbt &= ~(FLEXCAN_FDCBT_FPRESDIV(0x3ff) | > > + FLEXCAN_FDCBT_FRJW(0x03) | > > + FLEXCAN_CBT_EPROPSEG(0x1f) | > > + FLEXCAN_FDCBT_FPSEG1(0x07) | > > + FLEXCAN_FDCBT_FPSEG2(0x07)); > > + > > + reg_fdcbt |= FLEXCAN_FDCBT_FPRESDIV(data_bt->brp - 1) | > > + FLEXCAN_FDCBT_FRJW(data_bt->sjw - 1) | > > + FLEXCAN_FDCBT_FPROPSEG(data_bt->prop_seg - > 1) | > > + FLEXCAN_FDCBT_FPSEG1(data_bt->phase_seg1 - > 1) | > > + FLEXCAN_FDCBT_FPSEG2(data_bt->phase_seg2 - > 1); > > + > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { > > + u32 tdcoff; > > + /* No TDC is needed for data bit rates up to 2.5 > MBit/s > > + * refer https://www.can- > cia.org/fileadmin/resources/documents/proceedings/2013_hartwich_v2.pd > f > > + */ > > + if (reg_ctrl & FLEXCAN_CTRL_LPB || > > + data_bt->bitrate <= 2500000) > > + reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN; > > + else > > + reg_fdctrl |= FLEXCAN_FDCTRL_TDCEN; > > + > > + reg_fdctrl |= (FLEXCAN_FDCTRL_FDRATE | > > + FLEXCAN_FDCTRL_TDCFAIL); > > + > > + /* offset should be within the can FD bit timings */ > > + tdcoff = (data_bt->phase_seg1 + data_bt- > >prop_seg) * > > + data_bt->brp; > > + > > + reg_fdctrl |= FLEXCAN_FDCTRL_TDCOFF(tdcoff); > > + reg_fdctrl |= FLEXCAN_FDCTRL_MBDSR0(3) | > > + FLEXCAN_FDCTRL_MBDSR1(3); > > + } else { > > + reg_fdctrl |= FLEXCAN_FDCTRL_MBDSR0(0) | > > + FLEXCAN_FDCTRL_MBDSR1(0); > > + reg_fdctrl &= ~FLEXCAN_FDCTRL_FDRATE; > > + } > > + netdev_dbg(dev, "writing fdctrl=0x%08x", reg_fdctrl); > > + netdev_dbg(dev, "writing cbt=0x%08x\n", reg_cbt); > > + netdev_dbg(dev, "writing fdcbt=0x%08x\n", reg_fdcbt); > > + priv->write(reg_fdctrl, ®s->fdctrl); > > + priv->write(reg_cbt, ®s->cbt); > > + priv->write(reg_fdcbt, ®s->fdcbt); > > + } else { > > + reg_ctrl &= ~(FLEXCAN_CTRL_PRESDIV(0xff) | > > + FLEXCAN_CTRL_RJW(0x3) | > > + FLEXCAN_CTRL_PSEG1(0x7) | > > + FLEXCAN_CTRL_PSEG2(0x7) | > > + FLEXCAN_CTRL_PROPSEG(0x7)); > > + > > + reg_ctrl |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) | > > + FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) | > > + FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) | > > + FLEXCAN_CTRL_RJW(bt->sjw - 1) | > > + FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1); > > + } > > > > - netdev_dbg(dev, "writing ctrl=0x%08x\n", reg); > > - priv->write(reg, ®s->ctrl); > > + netdev_dbg(dev, "writing ctrl=0x%08x\n", reg_ctrl); > > + priv->write(reg_ctrl, ®s->ctrl); > > > > /* print chip status */ > > netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__, @@ > -954,8 > > +1091,6 @@ static int flexcan_chip_start(struct net_device *dev) > > if (err) > > goto out_chip_disable; > > > > - flexcan_set_bittiming(dev); > > - > > /* MCR > > * > > * enable freeze > > @@ -967,6 +1102,7 @@ static int flexcan_chip_start(struct net_device > *dev) > > * enable individual RX masking > > * choose format C > > * set max mailbox number > > + * enable FD mode > > */ > > reg_mcr = priv->read(®s->mcr); > > reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff); @@ -981,9 +1117,13 > @@ static > > int flexcan_chip_start(struct net_device *dev) > > reg_mcr |= FLEXCAN_MCR_FEN | > > FLEXCAN_MCR_MAXMB(priv->tx_mb_idx); > > } > > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) > > + reg_mcr |= FLEXCAN_MCR_FDEN; > > What does the FLEXCAN_MCR_FDEN bit mean? > We don't want to have the controller unconditionally in CAN-FD mode. > Only if the user requests CAN-FD via ctrl-mode. To use flexcan in FD mode, FDEN bit needs to be set. This is necessary because in flexcan_probe we have provided extended_bit_timings_const as flexcan Arbitration mode timings. Now regardless of FD mode enabled or not, the CAN Arbitration phase timings (same as data phase for CAN 2.0) will be calculated based on these. We cannot set these timings in ctrl register, as ctrl register timings parameters are different. To be able to use CAN Bit timings register (regs->cbt) for CAN Arbitration phase, we need to set this bit. This is also why I have to move flexcan_set_bittiming() after MCR register is set. > > > netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr); > > priv->write(reg_mcr, ®s->mcr); > > > > + flexcan_set_bittiming(dev); > > + > > /* CTRL > > * > > * disable timer sync feature > > @@ -1348,6 +1488,7 @@ static const struct of_device_id > flexcan_of_match[] = { > > { .compatible = "fsl,p1010-flexcan", .data = > &fsl_p1010_devtype_data, }, > > { .compatible = "fsl,vf610-flexcan", .data = > &fsl_vf610_devtype_data, }, > > { .compatible = "fsl,ls1021ar2-flexcan", .data = > > &fsl_ls1021a_r2_devtype_data, }, > > + { .compatible = "fsl,lx2160ar1-flexcan", .data = > > +&fsl_lx2160a_r1_devtype_data, }, > > { /* sentinel */ }, > > }; > > MODULE_DEVICE_TABLE(of, flexcan_of_match); @@ -1437,6 +1578,14 > @@ > > static int flexcan_probe(struct platform_device *pdev) > > priv->write = flexcan_write_le; > > } > > > > + if ((devtype_data->quirks & > > + (FLEXCAN_QUIRK_USE_FD | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) == > > + FLEXCAN_QUIRK_USE_FD) { > > if ((devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) && > !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) Ok. I will fix it > > > + dev_err(&pdev->dev, "can't use FIFO with FD mode\n"); > > + err = -ENODEV; > > + goto failed_register; > > + } > > + > > priv->can.clock.freq = clock_freq; > > priv->can.bittiming_const = &flexcan_bittiming_const; > > priv->can.do_set_mode = flexcan_set_mode; @@ -1450,6 > +1599,12 @@ > > static int flexcan_probe(struct platform_device *pdev) > > priv->devtype_data = devtype_data; > > priv->reg_xceiver = reg_xceiver; > > > > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) { > > + priv->can.bittiming_const = > &extended_flexcan_bittiming_const; > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > + priv->can.data_bittiming_const = > &flexcan_data_bittiming_const; > > + } > > Does the new controller now have 3 bittiming register sets? > The old one in the reg_ctrl and the two new ones? Which one is used by the > new controller? Does it depend on the FLEXCAN_MCR_FDEN bit? As I explained above the controller has 3 registers for bit timings (ctrl, cbt, fdcbt) Cbt (for Arbitration phase timings) and fdcbt (Data phase timings) can be used if FD mode timings are to be set. For this FDEN bit needs to be set in MCR register. As we pass extended_flexcan_bittiming_const as Arbitration bit timings in probe, we have to use cbt register even if we operate in CAN 2.0 mode. That is why I have set FDEN bit based on quirk instead of ctrlmode > > > + > > err = register_flexcandev(dev); > > if (err) { > > dev_err(&pdev->dev, "registering netdev failed\n"); > > > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
