Hi Anant,
I have a few more comments after a closer look...
I suggest using BIT(n) for all (1 << n), not only the macro definitions.
Also, often you use (1 << mbxno) in a sequence of expressions. I think
using
mbx_mask = BIT(mbxno);
would be more efficient. A few more comments inline...
Anant Gole wrote:
> TI HECC (High End CAN Controller) module is found on many TI devices. It
> has 32 hardware mailboxes with full implemtation of CAN protocol 2.0B
> with bus speeds up to 1Mbps. Specifications of the module are available
> at TI web <http://www.ti.com>
>
> Signed-off-by: Anant Gole <[email protected]>
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> new file mode 100644
> index 0000000..0d3cbe1
> --- /dev/null
> +++ b/drivers/net/can/ti_hecc.c
[snip]
> +
> +/* TX / RX Mailbox Configuration */
> +#define HECC_MAX_MAILBOXES 32 /* hardware mboxes - do not change */
> +#define MAX_TX_PRIO 0x3F /* hardware value - do not change */
> +
> +#define HECC_MAX_TX_MBOX 4
> +#define HECC_MAX_RX_MBOX (HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
> +
> +#if (HECC_MAX_TX_MBOX > CAN_ECHO_SKB_MAX)
> +#error "HECC: MAX TX mailboxes should be equal or less than CAN_ECHO_SKB_MAX"
> +#endif
> +
> +/* Important Note: TX mailbox configuration
> + * TX mailboxes should be restricted to the number of SKB buffers to avoid
> + * maintaining SKB buffers separately. TX mailboxes should be a power of 2
> + * for the mailbox logic to work. Top mailbox numbers are reserved for RX
> + * and lower mailboxes for TX.
> + *
> + * HECC_MAX_TX_MBOX HECC_MB_TX_SHIFT
> + * 4 (default) 2
> + * 8 3
> + * 16 4
> + */
> +#define HECC_MB_TX_SHIFT 2 /* as per table above */
I think you could get rid of this dependency if you define
HECC_MB_TX_SHIFT first (above).
#define HECC_MB_TX_SHIFT 2
#define HECC_MAX_TX_MBOX BIT(HECC_MB_TX_SHIFT)
> +#define HECC_TX_PRIO_SHIFT (HECC_MB_TX_SHIFT)
> +#define HECC_TX_PRIO_MASK (0x3f << HECC_MB_TX_SHIFT)
> +#define HECC_TX_MB_MASK (HECC_MAX_TX_MBOX - 1)
> +#define HECC_TX_MASK ((HECC_MAX_TX_MBOX - 1) | HECC_TX_PRIO_MASK)
> +#define HECC_TX_MBOX_MASK (~((1 << HECC_MAX_TX_MBOX) - 1))
> +#define HECC_DEF_NAPI_WEIGHT HECC_MAX_RX_MBOX
> +
> +/* Important Note: RX mailboxes are further logically split into two - main
> + * and buffer mailboxes. The goal is to get all packets into main mailboxes
> + * as driven by mailbox number and receive priority (higher to lower) and
> + * buffer mailboxes are used to receive pkts while main mailboxes are being
> + * processed. This ensures in-order packet reception.
> + *
> + * Here are the recommended values for buffer mailbox. Note that RX mailboxes
> + * start after TX mailboxes:
> + *
> + * HECC_MAX_RX_MBOX HECC_RX_BUFFER_MBOX No of buffer mailboxes
> + * 28 12 8
> + * 16 20 4
> + */
> +#define HECC_RX_BUFFER_MBOX 12 /* as per table above */
Note sure if this value could be calculated as well.
[snip]
> +static inline int get_tx_head_prio(struct ti_hecc_priv *priv)
> +{
> + return (priv->tx_head >> HECC_TX_PRIO_SHIFT) & 0x3F;
> +}
Please use an appropriate define for "0x3F".
[snip]
> +static void ti_hecc_start(struct net_device *ndev)
> +{
> + struct ti_hecc_priv *priv = netdev_priv(ndev);
> + int cnt, mbxno;
> +
> + /* put HECC in initialization mode and set btc */
> + ti_hecc_reset(ndev);
> +
> + priv->tx_head = priv->tx_tail = HECC_TX_MASK;
> + priv->rx_next = HECC_RX_FIRST_MBOX;
> +
> + /* Enable local and global acceptance mask registers */
> + hecc_write(priv, HECC_CANGAM, HECC_SET_REG);
> + hecc_write(priv, HECC_CANLAM0, HECC_SET_REG);
> + hecc_write(priv, HECC_CANLAM3, HECC_SET_REG);
> +
> + /* Prepare configured mailboxes to receive messages */
> + for (cnt = 0; cnt < HECC_MAX_RX_MBOX; cnt++) {
> + mbxno = (HECC_MAX_MAILBOXES - 1 - cnt);
> + hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> + hecc_write(priv, HECC_CANMID(mbxno), HECC_CANMID_AME);
> + hecc_write(priv, HECC_CANLAM(mbxno), HECC_SET_REG);
> + hecc_set_bit(priv, HECC_CANMD, (1 << mbxno));
> + hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> + hecc_set_bit(priv, HECC_CANMIM, (1 << mbxno));
mbx_mask? See general comment above.
[snip]
> +/**
> + * ti_hecc_xmit: HECC Transmit
> + *
> + * The transmit mailboxes start from 0 to HECC_MAX_TX_MBOX. In HECC the
> + * priority of the mailbox for tranmission is dependent upon priority setting
> + * field in mailbox registers. The mailbox with highest value in priority
> field
> + * is transmitted first. Only when two mailboxes have the same value in
> + * priority field the highest numbered mailbox is transmitted first.
> + *
> + * To utilize the HECC priority feature as described above we start with the
> + * highest numbered mailbox with highest priority level and move on to the
> next
> + * mailbox with the same priority level and so on. Once we loop through all
> the
> + * transmit mailboxes we choose the next priority level (lower) and so on
> + * until we reach the lowest priority level on the lowest numbered mailbox
> + * when we stop transmission until all mailboxes are transmitted and then
> + * restart at highest numbered mailbox with highest priority.
> + *
> + * Two counters (head and tail) are used to track the next mailbox to
> transmit
> + * and to track the echo buffer for already transmitted mailbox. The queue
> + * is stopped when all the mailboxes are busy or when there is a priority
> + * value roll-over happens.
> + */
> +static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct ti_hecc_priv *priv = netdev_priv(ndev);
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + u32 mbxno = 0;
Do you need to pre-set mbxno?
> + u32 data;
> + unsigned long flags;
> +
> + mbxno = get_tx_head_mb(priv);
> + spin_lock_irqsave(&priv->mbox_lock, flags);
> + if (unlikely(hecc_read(priv, HECC_CANME) & (1 << mbxno))) {
> + spin_unlock_irqrestore(&priv->mbox_lock, flags);
> + netif_stop_queue(ndev);
> + dev_err(priv->ndev->dev.parent,
> + "BUG: TX mbox not ready tx_head=%08X, tx_tail=%08X\n",
> + priv->tx_head, priv->tx_tail);
> + return NETDEV_TX_BUSY;
> + }
> + spin_unlock_irqrestore(&priv->mbox_lock, flags);
> +
> + /* Prepare mailbox for transmission */
> + data = cf->can_dlc & 0xF;
Please use min(cf->can_dlc, 8) instead.
> + if (cf->can_id & CAN_RTR_FLAG) /* Remote transmission request */
> + data |= HECC_CANMCF_RTR;
> + data |= (get_tx_head_prio(priv) << 8);
You don't need the outer brackets.
> + hecc_write(priv, HECC_CANMCF(mbxno), data);
> +
> + if (cf->can_id & CAN_EFF_FLAG) /* Extended frame format */
> + data = ((cf->can_id & CAN_EFF_MASK) | HECC_CANMID_IDE);
Ditto.
> + else /* Standard frame format */
> + data = ((cf->can_id & CAN_SFF_MASK) << 18);
Ditto.
> + hecc_write(priv, HECC_CANMID(mbxno), data);
> + hecc_write(priv, HECC_CANMDL(mbxno),
> + be32_to_cpu(*(u32 *)(cf->data + 0)));
> + if (cf->can_dlc > 4) {
> + hecc_write(priv, HECC_CANMDH(mbxno),
> + be32_to_cpu(*(u32 *)(cf->data + 4)));
> + } else {
> + *(u32 *)(cf->data + 4) = 0;
> + }
> + can_put_echo_skb(skb, ndev, mbxno);
> +
> + spin_lock_irqsave(&priv->mbox_lock, flags);
> + --priv->tx_head;
> + if ((hecc_read(priv, HECC_CANME) & (1 << get_tx_head_mb(priv))) ||
> + (priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK) {
> + netif_stop_queue(ndev);
> + }
> + hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> + spin_unlock_irqrestore(&priv->mbox_lock, flags);
> +
> + hecc_clear_bit(priv, HECC_CANMD, (1 << mbxno));
> + hecc_set_bit(priv, HECC_CANMIM, (1 << mbxno));
> + hecc_write(priv, HECC_CANTRS, (1 << mbxno));
mbx_mask?
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno)
> +{
> + struct net_device_stats *stats = &priv->ndev->stats;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + u32 data;
> + unsigned long flags;
> +
> + skb = dev_alloc_skb(sizeof(struct can_frame));
> + if (!skb) {
> + if (printk_ratelimit())
> + dev_err(priv->ndev->dev.parent,
> + "dev_alloc_skb() failed\n");
> + return -ENOMEM;
> + }
> + skb->dev = priv->ndev;
> + skb->protocol = htons(ETH_P_CAN);
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> + cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> + data = hecc_read(priv, HECC_CANMID(mbxno));
> + if (data & HECC_CANMID_IDE)
> + cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
> + else
> + cf->can_id = ((data >> 18) & CAN_SFF_MASK);
You don't need the outer brackets.
> + data = hecc_read(priv, HECC_CANMCF(mbxno));
> + if (data & HECC_CANMCF_RTR)
> + cf->can_id |= CAN_RTR_FLAG;
> + cf->can_dlc = data & 0xF;
> + data = hecc_read(priv, HECC_CANMDL(mbxno));
> + *(u32 *)(cf->data + 0) = cpu_to_be32(data);
> + if (cf->can_dlc > 4) {
> + data = hecc_read(priv, HECC_CANMDH(mbxno));
> + *(u32 *)(cf->data + 4) = cpu_to_be32(data);
> + } else {
> + *(u32 *)(cf->data + 4) = 0;
> + }
> + spin_lock_irqsave(&priv->mbox_lock, flags);
> + hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> + hecc_write(priv, HECC_CANRMP, (1 << mbxno));
> + /* enable mailbox only if it is part of rx buffer mailboxes */
> + if (priv->rx_next < HECC_RX_BUFFER_MBOX)
> + hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> + spin_unlock_irqrestore(&priv->mbox_lock, flags);
mbx_mask?
> + stats->rx_bytes += cf->can_dlc;
> + netif_receive_skb(skb);
> + stats->rx_packets++;
> +
> + return 0;
> +}
> +
> +/**
> + * ti_hecc_rx_poll - HECC receive pkts
> + *
> + * The receive mailboxes start from highest numbered mailbox till last xmit
> + * mailbox. On CAN frame reception the hardware places the data into highest
> + * numbered mailbox that matches the CAN ID filter. Since all receive
> mailboxes
> + * have same filtering (ALL CAN frames) packets will arrive in the highest
> + * available RX mailbox and we need to ensure in-order packet reception.
> + *
> + * To ensure the packets are received in the right order we logically divide
> + * the RX mailboxes into main and buffer mailboxes. Packets are received as
> per
> + * mailbox priotity (higher to lower) in the main bank and once it is full we
> + * disable further reception into main mailboxes. While the main mailboxes
> are
> + * processed in NAPI, further packets are received in buffer mailboxes.
> + *
> + * We maintain a RX next mailbox counter to process packets and once all main
> + * mailboxe packets are passed to the upper stack we enable all of them but
> + * continue to process packets received in buffer mailboxes. With each packet
> + * received from buffer mailbox we enable it immediately so as to handle the
> + * overflow from higher mailboxes.
> + */
> +static int ti_hecc_rx_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *ndev = napi->dev;
> + struct ti_hecc_priv *priv = netdev_priv(ndev);
> + u32 num_pkts = 0;
> + u32 mbxno, mbx_mask;
> + unsigned long pending_pkts, flags;
> +
> + if (!netif_running(ndev))
> + return 0;
> +
> + while ((pending_pkts = hecc_read(priv, HECC_CANRMP)) &&
> + (num_pkts < quota)) {
> + mbx_mask = BIT(priv->rx_next); /* next rx mailbox to process */
> + if (mbx_mask & pending_pkts) {
> + if (ti_hecc_rx_pkt(priv, priv->rx_next) < 0)
> + return num_pkts;
> + ++num_pkts;
> + } else if (priv->rx_next > HECC_RX_BUFFER_MBOX) {
> + break; /* pkt not received yet */
> + }
> + --priv->rx_next;
> + if (priv->rx_next == HECC_RX_BUFFER_MBOX) {
> + /* enable high bank mailboxes */
> + spin_lock_irqsave(&priv->mbox_lock, flags);
> + mbx_mask = hecc_read(priv, HECC_CANME);
> + mbx_mask |= HECC_RX_HIGH_MBOX_MASK;
> + hecc_write(priv, HECC_CANME, mbx_mask);
> + spin_unlock_irqrestore(&priv->mbox_lock, flags);
> + } else if (priv->rx_next == (HECC_MAX_TX_MBOX - 1)) {
> + priv->rx_next = HECC_RX_FIRST_MBOX;
> + break;
> + }
> + }
> +
> + /* Enable packet interrupt if all pkts are handled */
> + if (0 == hecc_read(priv, HECC_CANRMP)) {
> + napi_complete(napi);
> + /* Re-enable RX mailbox interrupts */
> + mbxno = hecc_read(priv, HECC_CANMIM);
> + mbxno |= HECC_TX_MBOX_MASK;
Wouldn't the name "mbx_mask" be more appropriate?
> + hecc_write(priv, HECC_CANMIM, mbxno);
> + }
> +
> + return num_pkts;
> +}
[snip]
> +static int ti_hecc_open(struct net_device *ndev)
> +{
> + struct ti_hecc_priv *priv = netdev_priv(ndev);
> + int err;
> +
> + err = request_irq(ndev->irq, ti_hecc_interrupt, IRQF_DISABLED,
Do you really need IRQF_DISABLED?
> + ndev->name, ndev);
> + if (err) {
> + dev_err(ndev->dev.parent, "error requesting interrupt\n");
> + return err;
> + }
> +
> + /* Open common can device */
> + err = open_candev(ndev);
> + if (err) {
> + dev_err(ndev->dev.parent, "open_candev() failed %08X\n", err);
"failed with %d" seems to be more appropriate for the error code.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
