> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Wednesday, August 1, 2018 2:56 PM > To: Pankaj Bansal <pankaj.bansal@xxxxxxx>; linux-can@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 5/7] net: can: flexcan: Add provision for variable > payload size > > On 08/01/2018 04:06 PM, Pankaj Bansal wrote: > > Till now the flexcan module supported 8 byte payload size as per CAN > > 2.0 specifications. > > But now upcoming flexcan module in NXP LX2160A SOC supports CAN FD > > protocol too.The Message buffers need to be configured to have payload > > size 64 bytes. > > > > Therefore, added provision in the driver for payload size to be 64 bytes. > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > --- > > > > Notes: > > V3: > > - Removed payload_size from devtype_data. Now change MB size > based on > > flexcan control mode being FD, which in turn supports on FD mode > being > > supported. This is controlled by quirk. > > V2: > > - Change data from u32 to __be32 in flexcan_mailbox_read > > - Added function flexcan_get_mb to get mailbox address from > > mailbox number > > > > drivers/net/can/flexcan.c | 87 ++++++++++++++++++++++++++----------- > > 1 file changed, 62 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index 04aa264cb771..232b70f371da 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -140,7 +140,6 @@ > > #define FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP 0 > > #define FLEXCAN_TX_MB_OFF_TIMESTAMP 1 > > #define FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST > (FLEXCAN_TX_MB_OFF_TIMESTAMP + 1) > > -#define FLEXCAN_RX_MB_OFF_TIMESTAMP_LAST 63 > > #define FLEXCAN_IFLAG_MB(x) BIT(x) > > #define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW BIT(7) > > #define FLEXCAN_IFLAG_RX_FIFO_WARN BIT(6) > > @@ -190,12 +189,13 @@ > > #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 { > > u32 can_ctrl; > > u32 can_id; > > - u32 data[2]; > > + u32 data[]; > > }; > > > > /* Structure of the hardware registers */ @@ -224,7 +224,7 @@ struct > > flexcan_regs { > > u32 rxfgmask; /* 0x48 */ > > u32 rxfir; /* 0x4c */ > > u32 _reserved3[12]; /* 0x50 */ > > - struct flexcan_mb mb[64]; /* 0x80 */ > > + u32 mb[256]; /* 0x80 */ > > /* FIFO-mode: > > * MB > > * 0x080...0x08f 0 RX message buffer > > @@ -353,6 +353,25 @@ static inline void flexcan_write_le(u32 val, void > __iomem *addr) > > iowrite32(val, addr); > > } > > > > +static struct flexcan_mb *flexcan_get_mb(const struct flexcan_priv > *priv, > > + u8 mb_index) > > +{ > > + u8 mb_size; > > + u8 mb_count; > > + > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > > + mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN; > > + else > > + mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN; > > + mb_count = (sizeof(priv->regs->mb) / mb_size); > > Maybe cache the mb_count value in priv. I already had this value cached in priv in patch v1. mb_count_block1 and mb_count_block2. I removed this in v2 and later because after i added flexcan_get_mb() function based on your suggestions, I felt that these variables are not required. > > > + > > + if (mb_index >= mb_count) > > + return NULL; > > Later on you're make use of this, but if you have mb_count cached, you can > write a propper for loop and this should not happen anymore. Make it with > WARN_ON() or remove it. The mb_count doesn't help in proper for loop, as we have to take care of the partition also (see patch 6) However mb_count_per_partiton, this helps. But I don't think it's necessary. > > > + > > + return (struct flexcan_mb __iomem *)((u8 *)&priv->regs->mb + > > + (mb_size * mb_index)); > > +} > > + > > static inline void flexcan_error_irq_enable(const struct flexcan_priv > > *priv) { > > struct flexcan_regs __iomem *regs = priv->regs; @@ -519,6 +538,7 > @@ > > static netdev_tx_t flexcan_start_xmit(struct sk_buff *skb, struct > net_device *de > > u32 can_id; > > u32 data; > > u32 ctrl = FLEXCAN_MB_CODE_TX_DATA | (cf->can_dlc << 16); > > + u8 can_dlc_dword, i; > > > > if (can_dropped_invalid_skb(dev, skb)) > > return NETDEV_TX_OK; > > @@ -535,13 +555,10 @@ 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 (cf->can_dlc > 0) { > > - data = be32_to_cpup((__be32 *)&cf->data[0]); > > - priv->write(data, &priv->tx_mb->data[0]); > > - } > > - if (cf->can_dlc > 4) { > > - data = be32_to_cpup((__be32 *)&cf->data[4]); > > - priv->write(data, &priv->tx_mb->data[1]); > > + can_dlc_dword = (cf->can_dlc + sizeof(u32) - 1) / sizeof(u32); > > Use DIV_ROUND_UP() here. Ok. > > > + for (i = 0; i < can_dlc_dword; i++) { > > + data = be32_to_cpup((__be32 *)&cf->data[(i * > sizeof(u32))]); > > + priv->write(data, &priv->tx_mb->data[i]); > > } > > > > can_put_echo_skb(skb, dev, 0); > > @@ -666,8 +683,12 @@ static unsigned int flexcan_mailbox_read(struct > > can_rx_offload *offload, { > > struct flexcan_priv *priv = rx_offload_to_priv(offload); > > struct flexcan_regs __iomem *regs = priv->regs; > > - struct flexcan_mb __iomem *mb = ®s->mb[n]; > > + struct flexcan_mb __iomem *mb; > > u32 reg_ctrl, reg_id, reg_iflag1; > > + __be32 data; > > move data into the for loop Ok. > > > + u8 can_dlc_dword, i; > > + > > + mb = flexcan_get_mb(priv, n); > > > > if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) { > > u32 code; > > @@ -708,8 +729,11 @@ static unsigned int flexcan_mailbox_read(struct > can_rx_offload *offload, > > cf->can_id |= CAN_RTR_FLAG; > > cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf); > > > > - *(__be32 *)(cf->data + 0) = cpu_to_be32(priv->read(&mb- > >data[0])); > > - *(__be32 *)(cf->data + 4) = cpu_to_be32(priv->read(&mb- > >data[1])); > > + can_dlc_dword = (cf->can_dlc + sizeof(u32) - 1) / sizeof(u32); > > same here, maybe add a helper function. You meant DIV_ROUND_UP() right? If yes, then ok I will. > > > + for (i = 0; i < can_dlc_dword; i++) { > > + data = cpu_to_be32(priv->read(&mb->data[i])); > > + *(__be32 *)(cf->data + (i * sizeof(u32))) = data; > > + } > > > > /* mark as read */ > > if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) { > > @@ -906,6 +930,7 @@ static int flexcan_chip_start(struct net_device > *dev) > > struct flexcan_regs __iomem *regs = priv->regs; > > u32 reg_mcr, reg_ctrl, reg_ctrl2, reg_mecr; > > int err, i; > > + struct flexcan_mb __iomem *mb; > > > > /* enable module */ > > err = flexcan_chip_enable(priv); > > @@ -987,15 +1012,19 @@ static int flexcan_chip_start(struct net_device > *dev) > > } > > > > /* clear and invalidate all mailboxes first */ > > - for (i = priv->tx_mb_idx; i < ARRAY_SIZE(regs->mb); i++) { > > - priv->write(FLEXCAN_MB_CODE_RX_INACTIVE, > > - ®s->mb[i].can_ctrl); > > - } > > + i = priv->tx_mb_idx; > > + mb = flexcan_get_mb(priv, i); > > + do { > > + priv->write(FLEXCAN_MB_CODE_RX_INACTIVE, &mb- > >can_ctrl); > > + } while ((mb = flexcan_get_mb(priv, ++i)) != NULL); > > Keep it a for loop please. Ok, I will rework this as for loop. > > > > > if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) { > > - for (i = priv->offload.mb_first; i <= priv->offload.mb_last; > i++) > > - priv->write(FLEXCAN_MB_CODE_RX_EMPTY, > > - ®s->mb[i].can_ctrl); > > + for (i = priv->offload.mb_first; > > + i <= priv->offload.mb_last; > > + i++) { > > + mb = flexcan_get_mb(priv, i); > > + priv->write(FLEXCAN_MB_CODE_RX_EMPTY, &mb- > >can_ctrl); > > + } > > } > > > > /* Errata ERR005829: mark first TX mailbox as INACTIVE */ @@ - > 1015,7 > > +1044,7 @@ static int flexcan_chip_start(struct net_device *dev) > > priv->write(0x0, ®s->rxfgmask); > > > > /* clear acceptance filters */ > > - for (i = 0; i < ARRAY_SIZE(regs->mb); i++) > > + for (i = 0; i < ARRAY_SIZE(regs->rximr); i++) > > priv->write(0, ®s->rximr[i]); > > What does the new IP say when accessing rximr[i] with i >= the mailbox > available due to CAN-FD? The IMR registers behave the same way as in CAN 2.0 mode. Their count is still the same as CAN 2.0 i.e. 64 The IMR registers greater than the MB index available would not be used in reception, however can still be accessed. > > > > > /* On Vybrid, disable memory error detection interrupts @@ - > 1099,6 > > +1128,7 @@ static int flexcan_open(struct net_device *dev) { > > struct flexcan_priv *priv = netdev_priv(dev); > > int err; > > + u8 mb_size; > > > > err = clk_prepare_enable(priv->clk_ipg); > > if (err) > > @@ -1116,14 +1146,21 @@ static int flexcan_open(struct net_device > *dev) > > if (err) > > goto out_close; > > > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > > + mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN; > > + else > > + mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN; > > + > > if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) { > > priv->tx_mb_idx = FLEXCAN_TX_MB_OFF_TIMESTAMP; > > - priv->tx_mb_reserved = ®s- > >mb[FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP]; > > + priv->tx_mb_reserved = flexcan_get_mb(priv, > > + > FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP); > > } else { > > priv->tx_mb_idx = FLEXCAN_TX_MB_OFF_FIFO; > > - priv->tx_mb_reserved = ®s- > >mb[FLEXCAN_TX_MB_RESERVED_OFF_FIFO]; > > + priv->tx_mb_reserved = flexcan_get_mb(priv, > > + > FLEXCAN_TX_MB_RESERVED_OFF_FIFO); > > } > > - priv->tx_mb = ®s->mb[priv->tx_mb_idx]; > > + priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx); > > > > priv->reg_imask1_default = FLEXCAN_IFLAG_MB(priv->tx_mb_idx); > > priv->reg_imask2_default = 0; > > @@ -1134,7 +1171,7 @@ static int flexcan_open(struct net_device *dev) > > u64 imask; > > > > priv->offload.mb_first = > FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST; > > - priv->offload.mb_last = > FLEXCAN_RX_MB_OFF_TIMESTAMP_LAST; > > + priv->offload.mb_last = (sizeof(priv->regs->mb) / mb_size) - > 1; > > > > imask = GENMASK_ULL(priv->offload.mb_last, > > priv->offload.mb_first); > > > > 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�����٥
