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... >> >>> + >>> + 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. >>> +} >>> + >>> 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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
