> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Tuesday, August 7, 2018 8:05 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 03:10 PM, Pankaj Bansal wrote: > > > > > >> -----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. > > ok. > > >> > >>> + > >>> + 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. > > Since you have this helper function "flexcan_get_mb()", which translates > from a mailbox number to the address, the user of this function just needs > to know the total number of mailboxes. Valid values for mailbox number is > 0...n_of_mailboxes - 1. Only _this_ functions needs to know the ugly details > if there are several blocks of mailboxes, etc... That's right. That is why I did not put the mailbox count in priv data structure. As the offload.mb_last already has the number of total mailboxes available and for RX fifo case the number of Mailboxes is fixed upto six frames. > > >> > >>> + > >>> + return (struct flexcan_mb __iomem *)((u8 *)&priv->regs->mb + > >>> + (mb_size * mb_index)); > > If you make the regs->mb an u8 array, you don't need the cast here. Actually, I did this purposefully because of an errata in LX2160A. We are only able to read/write at 32bit aligned memory locations in flexcan RAM (which stores Message buffers) Because of this limitation only, I had to use can_dlc_dword in xmit as well as mailbox_read functions. > > >>> +} > >>> + > >>> 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. > > yes > > > > >> > >>> + 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. > > Just to be sure :) > > > > >> > >>> > >>> /* 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 | > > > > 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 | If there is no other review comments, then I will work on the review comments and my proposal for rx_offload and send the v4 patches. Regards, Pankaj Bansal ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
