Hello Marc, On 7/24/19 10:26 AM, Marc Kleine-Budde wrote: > On 4/29/19 2:03 PM, 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 <anantgole@xxxxxx> >> Cc: AnilKumar Ch <anilkumar@xxxxxx> >> Signed-off-by: Jeroen Hofstee <jhofstee@xxxxxxxxxxxxxxxxx> >> --- >> 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 <jhofstee@xxxxxxxxxxxxxxxxx> >> * >> * 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 > It's actually 0x80 > >> + >> #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); > I've changed this function to use HECC_CANMOTS. > That is correct. For completeness the HECC_CANMOTS wasn't even used in the original patch, so there is no functional difference. Thanks, Jeroen
