RE: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>]

Hi Marc,

Thanks for your time & review comments.

> -----Original Message-----
(...)
> > +#define RCANFD_NAPI_WEIGHT		8	/* Rx poll quota */
> > +
> > +#define RCANFD_NUM_CHANNELS		2
> > +#define RCANFD_CHANNELS_MASK		0x3	/* Two channels max */
> 
> (BIT(RCANFD_NUM_CHANNELS) - 1

OK

> 
> > +
> > +/* Rx FIFO is a global resource of the controller. There are 8 such
> FIFOs
> > + * available. Each channel gets a dedicated Rx FIFO (i.e.) the channel
(...)
> > +#define RCANFD_CMFIFO_CFDLC(x)		(((x) & 0xf) << 28)
> > +#define RCANFD_CMFIFO_CFPTR(x)		(((x) & 0xfff) << 16)
> > +#define RCANFD_CMFIFO_CFTS(x)		(((x) & 0xff) << 0)
> > +
> > +/* Global Test Config register */
> > +#define RCANFD_GTSTCFG_C0CBCE		BIT(0)
> > +#define RCANFD_GTSTCFG_C1CBCE		BIT(1)
> > +
> > +#define RCANFD_GTSTCTR_ICBCTME		BIT(0)
> > +
> > +/* AFL Rx rules registers */
> > +#define RCANFD_AFLCFG_SETRNC0(x)	(((x) & 0xff) << 24)
> > +#define RCANFD_AFLCFG_SETRNC1(x)	(((x) & 0xff) << 16)
> 
> What about something like:
> 
> #define RCANFD_AFLCFG_SETRNC(n, x)	(((x) & 0xff) << (24 - n * 8))
> 
> This will save some if()s in the code

Nice :-). Will update.

> 
> > +#define RCANFD_AFLCFG_GETRNC0(x)	(((x) >> 24) & 0xff)
> > +#define RCANFD_AFLCFG_GETRNC1(x)	(((x) >> 16) & 0xff)
> > +
> > +#define RCANFD_AFL_PAGENUM(entry)	((entry) / 16)
(...)
> > +#define rcar_canfd_read(priv, offset)			\
> > +	readl(priv->base + (offset))
> > +#define rcar_canfd_write(priv, offset, val)		\
> > +	writel(val, priv->base + (offset))
> > +#define rcar_canfd_set_bit(priv, reg, val)		\
> > +	rcar_canfd_update(val, val, priv->base + (reg))
> > +#define rcar_canfd_clear_bit(priv, reg, val)		\
> > +	rcar_canfd_update(val, 0, priv->base + (reg))
> > +#define rcar_canfd_update_bit(priv, reg, mask, val)	\
> > +	rcar_canfd_update(mask, val, priv->base + (reg))
> 
> please use static inline functions instead of defines.

OK.

> 
> > +
> > +static void rcar_canfd_get_data(struct canfd_frame *cf,
> > +				struct rcar_canfd_channel *priv, u32 off)
> 
> Please use "struct rcar_canfd_channel *priv" as first argument, struct
> canfd_frame *cf as second. Remove off, as the offset is already defined
> by the channel.

I'll re-order priv, cf as you mentioned. I'll leave "off" arg as it is because it is based on FIFO number of channel + mode (CAN only or CANFD only). Otherwise, I will have to add another check inside this function for mode.
 
> > +{
> > +	u32 i, lwords;
> > +
> > +	lwords = cf->len / sizeof(u32);
> > +	if (cf->len % sizeof(u32))
> > +		lwords++;
> 
> Use DIV_ROUND_UP() instead of open coding it.

Agreed. Thanks.

> 
> > +	for (i = 0; i < lwords; i++)
> > +		*((u32 *)cf->data + i) =
> > +			rcar_canfd_read(priv, off + (i * sizeof(u32)));
> > +}
> > +
> > +static void rcar_canfd_put_data(struct canfd_frame *cf,
> > +				struct rcar_canfd_channel *priv, u32 off)
> 
> same here

Yes (same as _get_data)
 
> 
> > +{
> > +	u32 i, j, lwords, leftover;
> > +	u32 data = 0;
> > +
> > +	lwords = cf->len / sizeof(u32);
> > +	leftover = cf->len % sizeof(u32);
> > +	for (i = 0; i < lwords; i++)
> > +		rcar_canfd_write(priv, off + (i * sizeof(u32)),
> > +				 *((u32 *)cf->data + i));
> 
> Here you don't convert the endianess...

Yes

> > +
> > +	if (leftover) {
> > +		u8 *p = (u8 *)((u32 *)cf->data + i);
> > +
> > +		for (j = 0; j < leftover; j++)
> > +			data |= p[j] << (j * 8);
> 
> ...here you do an implicit endianess conversion. "data" is little
> endian, while p[j] is big endian.

Not sure I got the question correctly.
Controller expectation of data bytes in 32bit register is bits[7:0] = byte0, bits[15:8] = byte1 and so on - little endian.
For e.g. if cf->data points to byte stream H'112233445566 (cf->data[0] = 0x11), first rcar_canfd_write will write 0x44332211 value to register. Yes the host cpu is assumed little endian. In leftover case, data will be 0x00006655 - again little endian. 
I think I should remove this leftover logic and just mask the unused bits to zero as cf->data is pre-allocated for max length anyway.

p[j] is a byte?? Am I missing something?

> 
> > +		rcar_canfd_write(priv, off + (i * sizeof(u32)), data);
> > +	}
> 
> Have you tested to send CAN frames with len != n*4 against a different
> controller?

Yes, with Vector VN1630A.

> > +}
> > +
> > +static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
> > +{
> > +	u32 i;
> > +
> > +	for (i = 0; i < RCANFD_FIFO_DEPTH; i++)
> > +		can_free_echo_skb(ndev, i);
> > +}
> > +
(...)
> > +static void rcar_canfd_tx_done(struct net_device *ndev)
> > +{
> > +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> > +	struct net_device_stats *stats = &ndev->stats;
> > +	u32 sts;
> > +	unsigned long flags;
> > +	u32 ch = priv->channel;
> > +
> > +	do {
> 
> You should iterare over all pending CAN frames:
> 
> > 	for (/* nix */; (priv->tx_head - priv->tx_tail) > 0; priv-
> >tx_tail++) {
> 

Yes, current code does iterate over pending tx'ed frames. If we use this for loop semantics, we may have to protect the whole loop with no real benefit. 

> 
> > +		u8 unsent, sent;
> > +
> > +		sent = priv->tx_tail % RCANFD_FIFO_DEPTH;
> 
> and check here, if that packet has really been tramsitted. Exit the loop
> otherweise.

We are here because of tx_done and hence no need to check tx done again for the first iteration. Hence the do-while loop. Checks further down takes care of the need for more iterations/pending tx'ed frames.

> 
> > +		stats->tx_packets++;
> > +		stats->tx_bytes += priv->tx_len[sent];
> > +		priv->tx_len[sent] = 0;
> > +		can_get_echo_skb(ndev, sent);
> > +
> > +		spin_lock_irqsave(&priv->tx_lock, flags);
> 
> What does the tx_lock protect? The tx path is per channel, isn't it?

You are right - tx path is per channel. In tx path, head & tail are also used to determine when to stop/wakeup netif queue. They are incremented & compared in different contexts to make this decision. Per channel tx_lock protects this critical section.

> 
> > +		priv->tx_tail++;
> > +		sts = rcar_canfd_read(priv,
> > +				      RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX));
> > +		unsent = RCANFD_CMFIFO_CFMC(sts);
> > +
> > +		/* Wake producer only when there is room */
> > +		if (unsent != RCANFD_FIFO_DEPTH)
> > +			netif_wake_queue(ndev);
> 
> Move the netif_wake_queue() out of the loop.

With the tx logic mentioned above, I think keeping it in the loop seems better. For e.g. say cpu1 pumps 8 frames from an app loop and cpu0 handles tx_done interrupt handling: cpu1 would have stopped the queue because FIFO is full -> cpu0 gets tx_done interrupt and by the time it checks "unsent" there could be one or more frames transmitted by device (i.e.) there would be more space in fifo. It is better to wakeup the netif queue then and there so that app running on cpu1 can pump more. If we move it out of the loop we wake up the queue only in the end. Have I missed anything?

> 
> > +
> > +		if (priv->tx_head - priv->tx_tail <= unsent) {
> > +			spin_unlock_irqrestore(&priv->tx_lock, flags);
> > +			break;
> > +		}
> > +		spin_unlock_irqrestore(&priv->tx_lock, flags);
> > +
> > +	} while (1);
> > +
> > +	/* Clear interrupt */
> > +	rcar_canfd_write(priv, RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX),
> > +			 sts & ~RCANFD_CMFIFO_CFTXIF);
> > +	can_led_event(ndev, CAN_LED_EVENT_TX);
> > +}
> > +
> > +	if (cf->can_id & CAN_RTR_FLAG)
> > +		id |= RCANFD_CMFIFO_CFRTR;
> > +
> > +	rcar_canfd_write(priv, RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX),
> > +			 id);
> > +	ptr = RCANFD_CMFIFO_CFDLC(can_len2dlc(cf->len));
> 
> ptr usually means pointer, better call it dlc.

I used the register name "ptr". OK, will change it do dlc.

> 
> > +	rcar_canfd_write(priv, RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX),
> > +			 ptr);
> > +
(...)
> > +	can_put_echo_skb(skb, ndev, priv->tx_head % RCANFD_FIFO_DEPTH);
> > +
> > +	spin_lock_irqsave(&priv->tx_lock, flags);
> > +	priv->tx_head++;
> > +
> > +	/* Start Tx: Write 0xff to CFPC to increment the CPU-side
> > +	 * pointer for the Common FIFO
> > +	 */
> > +	rcar_canfd_write(priv, RCANFD_CFPCTR(ch, RCANFD_CFFIFO_IDX), 0xff);
> > +
> > +	/* Stop the queue if we've filled all FIFO entries */
> > +	if (priv->tx_head - priv->tx_tail >= RCANFD_FIFO_DEPTH)
> > +		netif_stop_queue(ndev);
> 
> Please move the check of stop_queue, before starting the send.

OK. 

> 
> > +
> > +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> > +	return NETDEV_TX_OK;
> > +}
> > +
(...)
> > +{
> > +	struct rcar_canfd_channel *priv =
> > +		container_of(napi, struct rcar_canfd_channel, napi);
> > +	int num_pkts;
> > +	u32 sts;
> > +	u32 ch = priv->channel;
> > +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> > +
> > +	for (num_pkts = 0; num_pkts < quota; num_pkts++) {
> > +		sts = rcar_canfd_read(priv, RCANFD_RFSTS(ridx));
> > +		/* Clear interrupt bit */
> > +		if (sts & RCANFD_RFFIFO_RFIF)
> > +			rcar_canfd_write(priv, RCANFD_RFSTS(ridx),
> > +					 sts & ~RCANFD_RFFIFO_RFIF);
> > +
> > +		/* Check FIFO empty condition */
> > +		if (sts & RCANFD_RFFIFO_RFEMP)
> > +			break;
> > +
> > +		rcar_canfd_rx_pkt(priv);
> 
> This sequence looks strange. You first conditionally ack the interrupt
> then you check for empty fifo, then read the CAN frame. I would assume
> that you first check if there's a CAN frame, read it and then clear the
> interrupt.

Yes, I shall re-arrange the sequence as you mentioned.
 
> 
> > +	}
> > +
> > +	/* All packets processed */
> > +	if (num_pkts < quota) {
> > +		napi_complete(napi);
> > +		/* Enable Rx FIFO interrupts */
> > +		rcar_canfd_set_bit(priv, RCANFD_RFCC(ridx),
> RCANFD_RFFIFO_RFIE);
(...)
> > +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> > +		err = rcar_canfd_channel_probe(gpriv, ch);
> > +		if (err)
> > +			goto fail_channel;
> > +	}
> 
> Should the CAN IP core be clocked the whole time? What about shuting
> down the clock and enabling it on ifup?

The fCAN clock is enabled only on ifup of one of the channels. However, the peripheral clock is enabled during probe to bring the controller to Global Operational mode. This clock cannot be turned off with the register values & mode retained.

> > +
> > +	platform_set_drvdata(pdev, gpriv);
> > +	dev_info(&pdev->dev, "global operational state (clk %d)\n",
> > +		 gpriv->clock_select);
(...)
> > +	rcar_canfd_reset_controller(gpriv);
> > +	rcar_canfd_disable_global_interrupts(gpriv);
> > +
> > +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> > +		priv = gpriv->ch[ch];
> > +		if (priv) {
> 
> This should always be true.

I agree. I thought I cleaned this up but it's in my local commits :-(

> 
> > +			rcar_canfd_disable_channel_interrupts(priv);
> > +			unregister_candev(priv->ndev);
> > +			netif_napi_del(&priv->napi);
> > +			free_candev(priv->ndev);
> 
> Please make use of rcar_canfd_channel_remove(), as you already have the
> function.

Yes.

Thanks,
Ramesh