On Mon, 2019-04-29 at 12:03 +0000, Jeroen Hofstee wrote: > As already mentioned in [1] and included in [2], there is an off by > one > issue since the high bank is already enabled when the _next_ mailbox > to > be read has index 12, so the mailbox being read was 13. The message > can > therefore go into mailbox 31 and the driver will be repolled until > the > mailbox 12 eventually receives a msg. Or the message might end up in > the > 12th mailbox, but then it would become disabled after reading it and > only > be enabled again in the next "round" after mailbox 13 was read, which > can > cause out of order messages, since the lower priority mailboxes can > accept messages in the meantime. > > As mentioned in [3] there is a hardware race condition when changing > the > CANME register while messages are being received. Even when including > a > busy poll on reception, like in [2] there are still overflows and out > of > order messages at times, but less then without the busy loop polling. > Unlike what the patch suggests, the polling time is not in the > microsecond > range, but takes as long as a current CAN bus reception needs to > finish, > so typically more in the fraction of millisecond range. Since the > timeout > is in jiffies it won't timeout. > > Even with these additional fixes the driver is still not able to > provide a > proper FIFO which doesn't drop packages. So change the driver to use > rx-offload and base order on timestamp instead of message box > numbers. As > a side affect, this also fixes [4] and [5]. > > Before this change messages with a single byte counter were dropped / > received out of order at a bitrate of 250kbit/s on an am3517. With > this > patch that no longer occurs up to and including 1Mbit/s. > > [1] > https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6 > [2] > http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2 > [3] > https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5 > [4] https://patchwork.ozlabs.org/patch/895956/ > [5] https://www.spinics.net/lists/netdev/msg494971.html > > Cc: Anant Gole <anantg...@ti.com> > Cc: AnilKumar Ch <anilku...@ti.com> > Signed-off-by: Jeroen Hofstee <jhofs...@victronenergy.com> > --- > drivers/net/can/ti_hecc.c | 189 +++++++++++++----------------------- > ---------- > 1 file changed, 53 insertions(+), 136 deletions(-) > > diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c > index db6ea93..fe7ffff 100644 > --- a/drivers/net/can/ti_hecc.c > +++ b/drivers/net/can/ti_hecc.c > @@ -5,6 +5,7 @@ > * specs for the same is available at <http://www.ti.com> > * > * Copyright (C) 2009 Texas Instruments Incorporated - > http://www.ti.com/ > + * Copyright (C) 2019 Jeroen Hofstee <jhofs...@victronenergy.com> > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License as > @@ -34,6 +35,7 @@ > #include <linux/can/dev.h> > #include <linux/can/error.h> > #include <linux/can/led.h> > +#include <linux/can/rx-offload.h> > > #define DRV_NAME "ti_hecc" > #define HECC_MODULE_VERSION "0.7" > @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION); > #define HECC_TX_PRIO_MASK (MAX_TX_PRIO << 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 (~(BIT(HECC_MAX_TX_MBOX) - 1)) > -#define HECC_DEF_NAPI_WEIGHT HECC_MAX_RX_MBOX > > /* > - * Important Note: RX mailbox configuration > - * 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 > + * RX mailbox configuration > + * The remaining mailboxes are used for reception and are delivered > based on > + * their timestamp, to avoid a hardware race when CANME is changed > while > + * CAN-bus traffix is being received. > */ > > #define HECC_MAX_RX_MBOX (HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX) > -#define HECC_RX_BUFFER_MBOX 12 /* as per table above */ > #define HECC_RX_FIRST_MBOX (HECC_MAX_MAILBOXES - 1) > -#define HECC_RX_HIGH_MBOX_MASK (~(BIT(HECC_RX_BUFFER_MBOX) - > 1)) > > /* TI HECC module registers */ > #define HECC_CANME 0x0 /* Mailbox enable */ > @@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION); > #define HECC_CANMDL 0x8 > #define HECC_CANMDH 0xC > > +#define HECC_CANMOTS 0x100 > + > #define HECC_SET_REG 0xFFFFFFFF > #define HECC_CANID_MASK 0x3FF /* 18 bits mask for > extended id's */ > #define HECC_CCE_WAIT_COUNT 100 /* Wait for ~1 sec for CCE bit > */ > @@ -193,7 +184,7 @@ static const struct can_bittiming_const > ti_hecc_bittiming_const = { > > struct ti_hecc_priv { > struct can_priv can; /* MUST be first member/field */ > - struct napi_struct napi; > + struct can_rx_offload offload; > struct net_device *ndev; > struct clk *clk; > void __iomem *base; > @@ -203,7 +194,6 @@ struct ti_hecc_priv { > spinlock_t mbx_lock; /* CANME register needs protection */ > u32 tx_head; > u32 tx_tail; > - u32 rx_next; > struct regulator *reg_xceiver; > }; > > @@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct > ti_hecc_priv *priv, int reg, u32 bit_mask) > return (hecc_read(priv, reg) & bit_mask) ? 1 : 0; > } > > +static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32 > mbxno) > +{ > + return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno); > +} > + > static int ti_hecc_set_btc(struct ti_hecc_priv *priv) > { > struct can_bittiming *bit_timing = &priv->can.bittiming; > @@ -375,7 +370,6 @@ static void ti_hecc_start(struct net_device > *ndev) > 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); > @@ -526,21 +520,17 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff > *skb, struct net_device *ndev) > return NETDEV_TX_OK; > } > > -static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno) > +static inline struct ti_hecc_priv *rx_offload_to_priv(struct > can_rx_offload *offload) > { > - struct net_device_stats *stats = &priv->ndev->stats; > - struct can_frame *cf; > - struct sk_buff *skb; > - u32 data, mbx_mask; > - unsigned long flags; > + return container_of(offload, struct ti_hecc_priv, offload); > +} > > - skb = alloc_can_skb(priv->ndev, &cf); > - if (!skb) { > - if (printk_ratelimit()) > - netdev_err(priv->ndev, > - "ti_hecc_rx_pkt: alloc_can_skb() > failed\n"); > - return -ENOMEM; > - } > +static unsigned int ti_hecc_mailbox_read(struct can_rx_offload > *offload, > + struct can_frame *cf, > + u32 *timestamp, unsigned int > mbxno) > +{ > + struct ti_hecc_priv *priv = rx_offload_to_priv(offload); > + u32 data, mbx_mask; > > mbx_mask = BIT(mbxno); > data = hecc_read_mbx(priv, mbxno, HECC_CANMID); > @@ -558,100 +548,19 @@ static int ti_hecc_rx_pkt(struct ti_hecc_priv > *priv, int mbxno) > data = hecc_read_mbx(priv, mbxno, HECC_CANMDH); > *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > } > - spin_lock_irqsave(&priv->mbx_lock, flags); > - hecc_clear_bit(priv, HECC_CANME, mbx_mask); > - hecc_write(priv, HECC_CANRMP, mbx_mask); > - /* 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, mbx_mask); > - spin_unlock_irqrestore(&priv->mbx_lock, flags); > > - stats->rx_bytes += cf->can_dlc; > - can_led_event(priv->ndev, CAN_LED_EVENT_RX); > - netif_receive_skb(skb); > - stats->rx_packets++; > + *timestamp = hecc_read_stamp(priv, mbxno); > > - 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 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->mbx_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->mbx_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 (hecc_read(priv, HECC_CANRMP) == 0) { > - napi_complete(napi); > - /* Re-enable RX mailbox interrupts */ > - mbx_mask = hecc_read(priv, HECC_CANMIM); > - mbx_mask |= HECC_TX_MBOX_MASK; > - hecc_write(priv, HECC_CANMIM, mbx_mask); > - } else { > - /* repoll is done only if whole budget is used */ > - num_pkts = quota; > - } > - > - return num_pkts; > + return 1; > } > > static int ti_hecc_error(struct net_device *ndev, int int_status, > int err_status) > { > struct ti_hecc_priv *priv = netdev_priv(ndev); > - struct net_device_stats *stats = &ndev->stats; > struct can_frame *cf; > struct sk_buff *skb; > + u32 timestamp; > > /* propagate the error condition to the can stack */ > skb = alloc_can_err_skb(ndev, &cf); > @@ -732,9 +641,8 @@ static int ti_hecc_error(struct net_device *ndev, > int int_status, > } > } > > - stats->rx_packets++; > - stats->rx_bytes += cf->can_dlc; > - netif_rx(skb); > + timestamp = hecc_read(priv, HECC_CANLNT); > + can_rx_offload_queue_sorted(&priv->offload, skb, timestamp); > > return 0; > } > @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, > void *dev_id) > struct net_device *ndev = (struct net_device *)dev_id; > struct ti_hecc_priv *priv = netdev_priv(ndev); > struct net_device_stats *stats = &ndev->stats; > - u32 mbxno, mbx_mask, int_status, err_status; > - unsigned long ack, flags; > + u32 mbxno, mbx_mask, int_status, err_status, stamp;
Reverse xmas tree.
