--------------------------------------------------
From: "Kurt Van Dijck" <kurt.van.di...@eia.be>
Sent: Friday, February 11, 2011 8:50 PM
To: "Subhasish Ghosh" <subhas...@mistralsolutions.com>
Cc: <davinci-linux-open-source@linux.davincidsp.com>;
<linux-arm-ker...@lists.infradead.org>; <m-watk...@ti.com>;
<nsek...@ti.com>; <sa...@mistralsolutions.com>; "Wolfgang Grandegger"
<w...@grandegger.com>; "open list:CAN NETWORK DRIVERS"
<socketcan-c...@lists.berlios.de>; "open list:CAN NETWORK DRIVERS"
<net...@vger.kernel.org>; "open list" <linux-ker...@vger.kernel.org>
Subject: Re: [PATCH v2 09/13] can: pruss CAN driver.
Hi,
I looked a bit at the TX path:
On Fri, Feb 11, 2011 at 08:21:28PM +0530, Subhasish Ghosh wrote:
+static int omapl_pru_can_set_bittiming(struct net_device *ndev)
+{
+ struct omapl_pru_can_priv *priv = netdev_priv(ndev);
+ struct can_bittiming *bt = &priv->can.bittiming;
+ long bit_error = 0;
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) {
+ dev_warn(priv->dev, "WARN: Triple"
+ "sampling not set due to h/w limitations");
You should not have enabled CAN_CTRLMODE_3_SAMPLES in the first place?
SG - Ok Will remove.
+ }
+ if (pru_can_calc_timing(priv->dev, priv->can.clock.freq,
+ bt->bitrate) != 0)
+ return -EINVAL;
+ bit_error =
+ (((priv->timer_freq / (priv->timer_freq / bt->bitrate)) -
+ bt->bitrate) * 1000) / bt->bitrate;
+ if (bit_error) {
+ bit_error =
+ (((priv->timer_freq / (priv->timer_freq / bt->bitrate)) -
+ bt->bitrate) * 1000000) / bt->bitrate;
+ printk(KERN_INFO "\nBitrate error %ld.%ld%%\n",
+ bit_error / 10000, bit_error % 1000);
+ } else
+ printk(KERN_INFO "\nBitrate error 0.0%%\n");
+
+ return 0;
+}
I wonder how much of this code is duplicated from drivers/net/can/dev.c ?
SG - Well, I just followed ti_hecc.c :-)
+static netdev_tx_t omapl_pru_can_start_xmit(struct sk_buff *skb,
+ struct net_device *ndev)
+{
+ struct omapl_pru_can_priv *priv = netdev_priv(ndev);
+ struct can_frame *cf = (struct can_frame *)skb->data;
+ int count;
+ u8 *data = cf->data;
+ u8 dlc = cf->can_dlc;
+ u8 *ptr8data = NULL;
+
most drivers start with:
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
SG - Will do.
+ netif_stop_queue(ndev);
why would you stop when you just resumed the queue?
SG - I do not want more than one transmit request at one time. Hence, on
entering the transmit
I am using netif_stop_queue to disable tx.
+ if (cf->can_id & CAN_EFF_FLAG) /* Extended frame format */
+ *((u32 *) &priv->can_tx_hndl.strcanmailbox) =
+ (cf->can_id & CAN_EFF_MASK) | PRU_CANMID_IDE;
+ else /* Standard frame format */
+ *((u32 *) &priv->can_tx_hndl.strcanmailbox) =
+ (cf->can_id & CAN_SFF_MASK) << 18;
+
+ if (cf->can_id & CAN_RTR_FLAG) /* Remote transmission request */
+ *((u32 *) &priv->can_tx_hndl.strcanmailbox) |= CAN_RTR_FLAG;
+
+ ptr8data = &priv->can_tx_hndl.strcanmailbox.u8data7 + (dlc - 1);
+ for (count = 0; count < (u8) dlc; count++) {
+ *ptr8data-- = *data++;
+ }
+ *((u32 *) &priv->can_tx_hndl.strcanmailbox.u16datalength) = (u32) dlc;
+/*
+ * search for the next available mbx
+ * if the next mbx is busy, then try the next + 1
+ * do this until the head is reached.
+ * if still unable to tx, stop accepting any packets
+ * if able to tx and the head is reached, then reset next to tail, i.e
mbx0
+ * if head is not reached, then just point to the next mbx
+ */
+ for (; priv->tx_next <= priv->tx_head; priv->tx_next++) {
+ priv->can_tx_hndl.ecanmailboxnumber =
+ (can_mailbox_number) priv->tx_next;
+ if (-1 == pru_can_write_data_to_mailbox(priv->dev,
+ &priv->can_tx_hndl)) {
+ if (priv->tx_next == priv->tx_head) {
+ priv->tx_next = priv->tx_tail;
+ if (!netif_queue_stopped(ndev))
If you get here, the queue is not stopped. This test is therefore useless.
SG -Ok, will remove
+ netif_stop_queue(ndev); /* IF stalled */
+ dev_err(priv->dev,
+ "%s: no tx mbx available", __func__);
+ return NETDEV_TX_BUSY;
+ } else
+ continue;
+ } else {
+ /* set transmit request */
+ pru_can_tx(priv->dev, priv->tx_next, CAN_TX_PRU_1);
+ pru_can_tx_mode_set(priv->dev, false, ecanreceive);
+ pru_can_tx_mode_set(priv->dev, true, ecantransmit);
+ pru_can_start_abort_tx(priv->dev, PRU_CAN_START);
+ priv->tx_next++;
+ can_put_echo_skb(skb, ndev, 0);
+ break;
+ }
+ }
+ if (priv->tx_next > priv->tx_head) {
+ priv->tx_next = priv->tx_tail;
+ }
+ return NETDEV_TX_OK;
+}
+
+
+irqreturn_t omapl_tx_can_intr(int irq, void *dev_id)
+{
+ struct net_device *ndev = dev_id;
+ struct omapl_pru_can_priv *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &ndev->stats;
+ u32 bit_set, mbxno;
+
+ pru_can_get_intr_status(priv->dev, &priv->can_tx_hndl);
+ if ((PRU_CAN_ISR_BIT_CCI & priv->can_tx_hndl.u32interruptstatus)
+ || (PRU_CAN_ISR_BIT_SRDI & priv->can_tx_hndl.u32interruptstatus)) {
+ __can_debug("tx_int_status = 0x%X\n",
+ priv->can_tx_hndl.u32interruptstatus);
+ can_free_echo_skb(ndev, 0);
+ } else {
+ for (bit_set = 0; ((priv->can_tx_hndl.u32interruptstatus & 0xFF)
+ >> bit_set != 0); bit_set++)
+ ;
+ if (0 == bit_set) {
+ __can_err("%s: invalid mailbox number\n", __func__);
+ can_free_echo_skb(ndev, 0);
+ } else {
+ mbxno = bit_set - 1; /* mail box numbering starts from 0 */
+ if (PRU_CAN_ISR_BIT_ESI & priv->can_tx_hndl.
+ u32interruptstatus) {
+ /* read gsr and ack pru */
+ pru_can_get_global_status(priv->dev, &priv->can_tx_hndl);
+ omapl_pru_can_err(ndev,
+ priv->can_tx_hndl.
+ u32interruptstatus,
+ priv->can_tx_hndl.
+ u32globalstatus);
+ } else {
+ stats->tx_packets++;
+ /* stats->tx_bytes += dlc; */
+ /*can_get_echo_skb(ndev, 0);*/
+ }
+ }
+ }
+ if (netif_queue_stopped(ndev))
you can call netif_wake_queue(ndev) multiple times, so there is no need
for netif_queue_stopped()
SG -Ok, will remove
+ netif_wake_queue(ndev);
+
+ can_get_echo_skb(ndev, 0);
+ pru_can_tx_mode_set(priv->dev, true, ecanreceive);
+ return IRQ_HANDLED;
+}
+
+static int omapl_pru_can_open(struct net_device *ndev)
+{
+ struct omapl_pru_can_priv *priv = netdev_priv(ndev);
+ int err;
+
+ /* register interrupt handler */
+ err = request_irq(priv->trx_irq, &omapl_rx_can_intr, IRQF_SHARED,
+ "pru_can_irq", ndev);
you're doing a lot of work _in_ the irq handler. Maybe threaded irq?
SG -Ok, will do
+static int omapl_pru_can_close(struct net_device *ndev)
+{
+ struct omapl_pru_can_priv *priv = netdev_priv(ndev);
+
+ if (!netif_queue_stopped(ndev))
check is not needed.
SG -Ok, will remove
+ netif_stop_queue(ndev);
+
+ close_candev(ndev);
+
+ free_irq(priv->trx_irq, ndev);
+ return 0;
+}
+
Regards,
Kurt
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
