-----Original Message-----
From: Ramesh Shanmugasundaram
Sent: 01 April 2016 13:49
To: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@xxxxxxxxxxxxxx>;
wg@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx;
mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx;
corbet@xxxxxxx
Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
can@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
geert+renesas@xxxxxxxxx; Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>
Subject: RE: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver
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
