Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading
From: Jeroen Hofstee
Date: Wed Jul 24 2019 - 05:13:39 EST
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