Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752838AbeACMzR (ORCPT + 1 other); Wed, 3 Jan 2018 07:55:17 -0500 Received: from lelnx194.ext.ti.com ([198.47.27.80]:57622 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752416AbeACMzO (ORCPT ); Wed, 3 Jan 2018 07:55:14 -0500 Subject: Re: [PATCH v6 4/6] can: m_can: Support higher speed CAN-FD bitrates To: Marc Kleine-Budde , , , CC: , , , , , , , , References: <1513949488-13026-1-git-send-email-faiz_abbas@ti.com> <1513949488-13026-5-git-send-email-faiz_abbas@ti.com> From: Faiz Abbas Message-ID: Date: Wed, 3 Jan 2018 18:25:28 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, On Tuesday 02 January 2018 07:05 PM, Marc Kleine-Budde wrote: > On 12/22/2017 02:31 PM, Faiz Abbas wrote: >> From: Franklin S Cooper Jr >> >> During test transmitting using CAN-FD at high bitrates (> 2 Mbps) >> would fail. Scoping the signals I noticed that only a single bit >> was being transmitted and with a bit more investigation realized the actual >> MCAN IP would go back to initialization mode automatically. >> >> It appears this issue is due to the MCAN needing to use the Transmitter >> Delay Compensation Mode with the correct value for the transmitter delay >> compensation offset (tdco). What impacts the tdco value isn't 100% clear >> but to calculate it you use an equation defined in the MCAN User's Guide. >> >> The user guide mentions that this register needs to be set based on clock >> values, secondary sample point and the data bitrate. One of the key >> variables that can't automatically be determined is the secondary >> sample point (ssp). This ssp is similar to the sp but is specific to this >> transmitter delay compensation mode. The guidelines for configuring >> ssp is rather vague but via some CAN test it appears for DRA76x that putting >> the value same as data sampling point works. >> >> The CAN-CIA's "Bit Time Requirements for CAN FD" paper presented at >> the International CAN Conference 2013 indicates that this TDC mode is >> only needed for data bit rates above 2.5 Mbps. Therefore, only enable >> this mode when the data bit rate is above 2.5 Mbps. >> >> Signed-off-by: Franklin S Cooper Jr >> Signed-off-by: Sekhar Nori >> Signed-off-by: Faiz Abbas >> --- >> drivers/net/can/m_can/m_can.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >> index 53e764f..371ffc1 100644 >> --- a/drivers/net/can/m_can/m_can.c >> +++ b/drivers/net/can/m_can/m_can.c >> @@ -127,6 +127,12 @@ enum m_can_mram_cfg { >> #define DBTP_DSJW_SHIFT 0 >> #define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT) >> >> +/* Transmitter Delay Compensation Register (TDCR) */ >> +#define TDCR_TDCO_SHIFT 8 >> +#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT) >> +#define TDCR_TDCF_SHIFT 0 >> +#define TDCR_TDCF_MASK (0x7F << TDCR_TDCF_SHIFT) >> + >> /* Test Register (TEST) */ >> #define TEST_LBCK BIT(4) >> >> @@ -991,7 +997,8 @@ static int m_can_set_bittiming(struct net_device *dev) >> const struct can_bittiming *bt = &priv->can.bittiming; >> const struct can_bittiming *dbt = &priv->can.data_bittiming; >> u16 brp, sjw, tseg1, tseg2; >> - u32 reg_btp; >> + u32 reg_btp, tdco, ssp; > > Please move the tdco and the ssp into "if (dbt->bitrate > 2500000)" scope. Ok. > > Initialize "reg_btp = 0", see below. > >> + bool enable_tdc = false; > > Please remove, see below. > >> >> brp = bt->brp - 1; >> sjw = bt->sjw - 1; >> @@ -1006,9 +1013,41 @@ static int m_can_set_bittiming(struct net_device *dev) >> sjw = dbt->sjw - 1; >> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1; >> tseg2 = dbt->phase_seg2 - 1; >> + >> + /* TDC is only needed for bitrates beyond 2.5 MBit/s. >> + * This is mentioned in the "Bit Time Requirements for CAN FD" >> + * paper presented at the International CAN Conference 2013 >> + */ >> + if (dbt->bitrate > 2500000) { >> + /* Use the same value of secondary sampling point >> + * as the data sampling point >> + */ >> + ssp = dbt->sample_point; >> + >> + /* Equation based on Bosch's M_CAN User Manual's >> + * Transmitter Delay Compensation Section >> + */ >> + tdco = (priv->can.clock.freq / 1000) * >> + ssp / dbt->bitrate; >> + >> + /* Max valid TDCO value is 127 */ >> + if (tdco > 127) { >> + netdev_warn(dev, "TDCO value of %u is beyond maximum limit. Disabling Transmitter Delay Compensation mode\n", > > "maximum limit"? Either "maximum" or "limit" should be enough. If the > value is above 127, does it make sense to disable the tdco completely? I guess we can put the closest possible value (127) so that the ssp calculated by the IP is as close to the chosen one as possible. > >> + tdco); >> + } else { >> + enable_tdc = true; > > Why not set "reg_btp |= DBTP_TDC;" here directly? > >> + m_can_write(priv, M_CAN_TDCR, >> + tdco << TDCR_TDCO_SHIFT); >> + } >> + } >> + >> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) | >> (tseg1 << DBTP_DTSEG1_SHIFT) | >> (tseg2 << DBTP_DTSEG2_SHIFT); > > Adjust this to "reg_btp |=". > >> + >> + if (enable_tdc) >> + reg_btp |= DBTP_TDC; > > Please remove. Sure, will remove. Thanks, Faiz