HI Marc, Gentle reminder! Are you happy with the open comment's disposition? I can send a next version of patch if we have a closure on current set of comments. Thanks, Ramesh > -----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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f