> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Tuesday, July 31, 2018 8:15 PM > To: Pankaj Bansal <pankaj.bansal@xxxxxxx>; linux-can@xxxxxxxxxxxxxxx > Subject: Re: [RFC] net: can: flexcan: can FD Format (FDF) changes > > On 07/31/2018 07:18 PM, Pankaj Bansal wrote: > > Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > It makes no sense to have two instances to identify a chip as CAN-FD > capable. Either user .payload_size = CANFD_MAX_DLEN or a QUIRK. > > A quirk makes more sense to me. Ok. I will remove payload_size from devtype_data > > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index 01005daf93dd..16fa917d23bf 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,26 @@ > > (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) > > + > > +/* 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 +180,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) > > @@ -189,6 +213,7 @@ > > #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP BIT(5) /* Use > timestamp based offloading */ > > #define FLEXCAN_QUIRK_BROKEN_PERR_STATE BIT(6) /* No interrupt > for error passive */ > > #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN BIT(7) /* default to > BE register access */ > > +#define FLEXCAN_QUIRK_USE_FD BIT(8) /* Supports CAN FD mode */ > > > > /* Structure of the message buffer */ struct flexcan_mb { @@ -222,7 > > +247,8 @@ struct flexcan_regs { > > u32 crcr; /* 0x44 */ > > u32 rxfgmask; /* 0x48 */ > > u32 rxfir; /* 0x4c */ > > - u32 _reserved3[12]; /* 0x50 */ > > + u32 cbt; /* 0x50 */ > > + u32 _reserved3[11]; /* 0x54 */ > > u32 mb1[128]; /* 0x80 */ > > u32 mb2[128]; /* 0x280 */ > > /* FIFO-mode: > > @@ -248,6 +274,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 { > > @@ -315,6 +345,13 @@ static const struct flexcan_devtype_data > fsl_ls1021a_r2_devtype_data = { > > .payload_size = CAN_MAX_DLEN, > > }; > > > > +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, > > + .payload_size = CANFD_MAX_DLEN, > > +}; > > + > > static const struct can_bittiming_const flexcan_bittiming_const = { > > .name = DRV_NAME, > > .tseg1_min = 4, > > @@ -327,6 +364,18 @@ static const struct can_bittiming_const > flexcan_bittiming_const = { > > .brp_inc = 1, > > }; > > > > +static const struct can_bittiming_const > extended_flexcan_bittiming_const = { > > + .name = "Extended flexcan", > > Just use DRV_NAME as for the standard timing. Ok > > > + .tseg1_min = 2, > > + .tseg1_max = 96, > > + .tseg2_min = 2, > > + .tseg2_max = 32, > > + .sjw_max = 16, > > + .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. > > @@ -563,6 +612,12 @@ static netdev_tx_t flexcan_start_xmit(struct > > sk_buff *skb, struct net_device *de > > > > if (cf->can_id & CAN_RTR_FLAG) > > ctrl |= FLEXCAN_MB_CNT_RTR; > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > > + ctrl |= FLEXCAN_MB_CNT_EDL; > > + if (cf->flags & CANFD_BRS) > > + ctrl |= FLEXCAN_MB_CNT_BRS; > > + if (cf->flags & CANFD_ESI) > > + ctrl |= FLEXCAN_MB_CNT_ESI; > > > > can_dlc_dword = (cf->len + sizeof(u32) - 1) / sizeof(u32); > > for (i = 0; i < can_dlc_dword; i++) { @@ -737,7 +792,10 @@ static > > unsigned int flexcan_mailbox_read(struct can_rx_offload *offload, > > if (reg_ctrl & FLEXCAN_MB_CNT_RTR) > > cf->can_id |= CAN_RTR_FLAG; > > > > - cf->len = get_can_dlc((reg_ctrl >> 16) & 0xf); > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > > + cf->len = can_dlc2len((reg_ctrl >> 16) & 0xf); > > + else > > + cf->len = get_can_dlc((reg_ctrl >> 16) & 0xf); > > > > can_dlc_dword = (cf->len + sizeof(u32) - 1) / sizeof(u32); > > for (i = 0; i < can_dlc_dword; i++) { @@ -895,34 +953,73 @@ 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 = 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); > > + > > + if (reg_mcr & FLEXCAN_MCR_FDEN) { > > + reg_cbt = priv->read(®s->cbt); > > + reg_fdcbt = priv->read(®s->fdcbt); > > + > > + 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); > > + > > + netdev_dbg(dev, "writing cbt=0x%08x\n", reg_cbt); > > + netdev_dbg(dev, "writing fdcbt=0x%08x\n", reg_fdcbt); > > + 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); > > + } > > + > > + 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; > > > > - 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__, @@ > -938,7 > > +1035,7 @@ static int flexcan_chip_start(struct net_device *dev) { > > struct flexcan_priv *priv = netdev_priv(dev); > > struct flexcan_regs __iomem *regs = priv->regs; > > - u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr; > > + u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr, reg_fdctrl; > > int err, i; > > struct flexcan_mb __iomem *mb; > > u8 mb_size; > > @@ -955,8 +1052,6 @@ static int flexcan_chip_start(struct net_device > *dev) > > if (err) > > goto out_chip_disable; > > > > - flexcan_set_bittiming(dev); > > - > > /* MCR > > * > > * enable freeze > > @@ -982,9 +1077,14 @@ 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 that bit do? > You should only switch on CAN-FD mode if the interface is configured as > CAN-FD. This bit configures the flexcan for FD mode. This allows the MB size to be flexible. We cannot enable this bit based on FD mode enable or not, because we do the Message buffer configuration (MB count/rx offload) and bittiming info in flexcan_probe irrespective of FD mode enable or not To be able to set this bit on FD mode, we need to move message buffer configuration and bittiming info from flexcan_probe to flexcan_start. > > > netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr); > > priv->write(reg_mcr, ®s->mcr); > > > > + /* Write bit timings in freeze mode */ > > + flexcan_set_bittiming(dev); > > Why are you moving this? The device should be in freeze mode right after > soft-reset. Ok. I will revert this > > > + > > /* CTRL > > * > > * disable timer sync feature > > @@ -1018,6 +1118,25 @@ static int flexcan_chip_start(struct net_device > *dev) > > netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl); > > priv->write(reg_ctrl, ®s->ctrl); > > > > + /* FD Control */ > > + if (reg_mcr & FLEXCAN_MCR_FDEN) { > > You might want to move this to the above section, where you're already > handling the CAN-FD case. OK. > > > + u8 payload_size; > > + for (payload_size = CAN_MAX_DLEN, i = 0; > > + payload_size <= CANFD_MAX_DLEN; > > + payload_size <<= 1) { > > + if (priv->devtype_data->payload_size == > payload_size) > > + break; > > + i++; > > + } > > There's probably a kernel helper to calculate the logarithm. > As devtype_data->payload_size is currently hardcoded to 64, you can just > directly assign the right value vor "i" here and remove devtype_data- > >payload_size. > > If you want to make payload_size configureable for both mailbox banks this > has to be confgured somehow anyways (maybe via DT). So let's keep the > driver simple and just use a payload_size of 64 for CAN-FD mode. OK. I will remove the payload_size from devtype_data. > > > + reg_fdctrl = priv->read(®s->fdctrl); > > + reg_fdctrl |= FLEXCAN_FDCTRL_MBDSR0(i) | > > + FLEXCAN_FDCTRL_MBDSR1(i); > > + reg_fdctrl &= ~FLEXCAN_FDCTRL_FDRATE; > > Does the CAN controller has some sane defaults for reg_fdctrl? > If the device is configured for CAN-2.0 you should use standard 8 byte > mailboxes.... We cannot configure MB size based on FD mode enable or not, because we do the Message buffer configuration (MB count/rx offload) and bittiming info in flexcan_probe irrespective of FD mode enable or not To be able to set MB size based on FD mode, we need to move message buffer configuration and bittiming info from flexcan_probe to flexcan_start. > > > + netdev_dbg(dev, "%s: writing fdctrl=0x%08x", __func__, > > + reg_fdctrl); > > + priv->write(reg_fdctrl, ®s->fdctrl); > > + } > > + > > if ((priv->devtype_data->quirks & > FLEXCAN_QUIRK_ENABLE_EACEN_RRS)) { > > reg_ctrl2 = priv->read(®s->ctrl2); > > reg_ctrl2 |= FLEXCAN_CTRL2_EACEN | > FLEXCAN_CTRL2_RRS; @@ -1300,6 > > +1419,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); @@ -1390,7 +1510,26 > @@ > > static int flexcan_probe(struct platform_device *pdev) > > priv->write = flexcan_write_le; > > } > > > > - if (devtype_data->payload_size != CAN_MAX_DLEN) { > > + if (devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) { > > + u8 payload_size; > > + if (!(devtype_data->quirks & > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) { > > + dev_err(&pdev->dev, "can't use FIFO with FD > mode\n"); > > + err = -ENODEV; > > + goto failed_register; > > + } > > + for (payload_size = CAN_MAX_DLEN; > > + payload_size <= CANFD_MAX_DLEN; > > + payload_size <<= 1) { > > + if (devtype_data->payload_size == payload_size) > > + break; > > + } > > + if (payload_size > CANFD_MAX_DLEN) { > > + dev_err(&pdev->dev, "payload_size %d not > supported\n", > > + devtype_data->payload_size); > > + err = -ENODEV; > > + goto failed_register; > > + } > > + } else if (devtype_data->payload_size != CAN_MAX_DLEN) { > > dev_err(&pdev->dev, "payload_size %d not supported\n", > > devtype_data->payload_size); > > err = -ENODEV; > > The driver should be consistent, please remove these checks. OK. I will remove payload_size from devtype_data. > > > @@ -1445,6 +1584,12 @@ static int flexcan_probe(struct > platform_device *pdev) > > if (err) > > goto failed_offload; > > > > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) { > > + priv->can.bittiming_const = > &extended_flexcan_bittiming_const; > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > Does the hardware support ISO or NON-ISO CAN-FD? ISO > > > + priv->can.data_bittiming_const = > &extended_flexcan_bittiming_const; > > please name it flexcan_data_bittiming_const These are not data bittimings. These are arbitration phase bittimings that SHOULD be used when FD mode is to be used. > > > + } > > + > > 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 | ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
