* [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support @ 2022-05-12 13:59 Srinivas Neeli 2022-05-13 1:24 ` Vincent Mailhol 0 siblings, 1 reply; 8+ messages in thread From: Srinivas Neeli @ 2022-05-12 13:59 UTC (permalink / raw) To: wg, mkl, davem, edumazet, appana.durga.rao, sgoud, michal.simek Cc: kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git, Srinivas Neeli Added Transmitter delay compensation (TDC) feature support. In the case of higher measured loop delay with higher baud rates, observed bit stuff errors. By enabling the TDC feature in a controller, will compensate for the measure loop delay in the receive path. TDC feature requires BRP values can be 1 or 2. The current CAN framework does not limit the brp so while using TDC, have to restrict BRP values. Ex: ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on loopback on tdco 12 tdc-mode auto Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> --- drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index e2b15d29d15e..7af518fbed02 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* Xilinx CAN device driver * - * Copyright (C) 2012 - 2014 Xilinx, Inc. + * Copyright (C) 2012 - 2022 Xilinx, Inc. * Copyright (C) 2009 PetaLogix. All rights reserved. * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy * @@ -133,6 +133,8 @@ enum xcan_reg { #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay Compensation Offset */ +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay Compensation (TDC) Enable */ #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump width */ @@ -259,7 +261,7 @@ static const struct can_bittiming_const xcan_bittiming_const_canfd2 = { .tseg2_min = 1, .tseg2_max = 128, .sjw_max = 128, - .brp_min = 2, + .brp_min = 1, .brp_max = 256, .brp_inc = 1, }; @@ -272,11 +274,21 @@ static struct can_bittiming_const xcan_data_bittiming_const_canfd2 = { .tseg2_min = 1, .tseg2_max = 16, .sjw_max = 16, - .brp_min = 2, + .brp_min = 1, .brp_max = 256, .brp_inc = 1, }; +/* Transmission Delay Compensation constants for CANFD2.0 and Versal */ +static const struct can_tdc_const xcan_tdc_const = { + .tdcv_min = 0, + .tdcv_max = 0, /* Manual mode not supported. */ + .tdco_min = 0, + .tdco_max = 64, + .tdcf_min = 0, /* Filter window not supported */ + .tdcf_max = 0, +}; + /** * xcan_write_reg_le - Write a value to the device register little endian * @priv: Driver private data structure @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct net_device *ndev) priv->devtype.cantype == XAXI_CANFD_2_0) { /* Setting Baud Rate prescalar value in F_BRPR Register */ btr0 = dbt->brp - 1; + if (can_tdc_is_enabled(&priv->can)) { + btr0 = btr0 | + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD | + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; + } /* Setting Time Segment 1 in BTR Register */ btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device *pdev) priv->can.data_bittiming_const = &xcan_data_bittiming_const_canfd; - if (devtype->cantype == XAXI_CANFD_2_0) + if (devtype->cantype == XAXI_CANFD_2_0) { priv->can.data_bittiming_const = &xcan_data_bittiming_const_canfd2; + priv->can.tdc_const = &xcan_tdc_const; + } if (devtype->cantype == XAXI_CANFD || devtype->cantype == XAXI_CANFD_2_0) - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | + CAN_CTRLMODE_TDC_AUTO; priv->reg_base = addr; priv->tx_max = tx_max; -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-12 13:59 [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli @ 2022-05-13 1:24 ` Vincent Mailhol 2022-05-13 1:37 ` Vincent Mailhol ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Vincent Mailhol @ 2022-05-13 1:24 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, mkl, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git On Fri. 13 May 2022 at 07:30, Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > Added Transmitter delay compensation (TDC) feature support. > In the case of higher measured loop delay with higher baud rates, observed > bit stuff errors. > By enabling the TDC feature in a controller, will compensate for > the measure loop delay in the receive path. > TDC feature requires BRP values can be 1 or 2. > The current CAN framework does not limit the brp so while using TDC, > have to restrict BRP values. > Ex: > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 20 > sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on > loopback on tdco 12 tdc-mode auto Did you experience some cases in which you had BRP > 2 and saw transmission errors due to the absence of delay compensation? Could you show the calculated values? Usually, you start to observe but stuff error at high bitrates (e.g. ~5MBPS), and for such bitrates, time quanta has to be small which then results in a small BRP. > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > --- > drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index e2b15d29d15e..7af518fbed02 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-or-later > /* Xilinx CAN device driver > * > - * Copyright (C) 2012 - 2014 Xilinx, Inc. > + * Copyright (C) 2012 - 2022 Xilinx, Inc. > * Copyright (C) 2009 PetaLogix. All rights reserved. > * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy > * > @@ -133,6 +133,8 @@ enum xcan_reg { > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay Compensation Offset */ Having CANFD in the name is redundant (TDC implies CANFD). #define XCAN_BRPR_TDCO_SHIFT 8 > +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay Compensation (TDC) Enable */ Why not: #define XCAN_BRPR_TDC_ENABLE BIT(16) > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump width */ > @@ -259,7 +261,7 @@ static const struct can_bittiming_const xcan_bittiming_const_canfd2 = { > .tseg2_min = 1, > .tseg2_max = 128, > .sjw_max = 128, > - .brp_min = 2, > + .brp_min = 1, Was there any reason to have brp_min = 2 in the first place? I think this change should be a different patch. If the brp_min = 2 is just a typo, you might also want to backport it to stable branches. > .brp_max = 256, > .brp_inc = 1, > }; > @@ -272,11 +274,21 @@ static struct can_bittiming_const xcan_data_bittiming_const_canfd2 = { > .tseg2_min = 1, > .tseg2_max = 16, > .sjw_max = 16, > - .brp_min = 2, > + .brp_min = 1, > .brp_max = 256, > .brp_inc = 1, > }; > > +/* Transmission Delay Compensation constants for CANFD2.0 and Versal */ > +static const struct can_tdc_const xcan_tdc_const = { > + .tdcv_min = 0, > + .tdcv_max = 0, /* Manual mode not supported. */ Right, had a look at the datasheet and xilinx indeed does not support setting TDCV. However, xilinx still has a TDCV register to report the measured transmission delay. Socket CAN's TDC framework provides can_priv::do_get_auto_tdcv() to report the measured value through the netlink interface: https://elixir.bootlin.com/linux/v5.17/source/include/linux/can/dev.h#L87 Can you implement this call back function? > + .tdco_min = 0, > + .tdco_max = 64, > + .tdcf_min = 0, /* Filter window not supported */ > + .tdcf_max = 0, > +}; > + > /** > * xcan_write_reg_le - Write a value to the device register little endian > * @priv: Driver private data structure > @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct net_device *ndev) > priv->devtype.cantype == XAXI_CANFD_2_0) { > /* Setting Baud Rate prescalar value in F_BRPR Register */ > btr0 = dbt->brp - 1; > + if (can_tdc_is_enabled(&priv->can)) { > + btr0 = btr0 | > + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD | > + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; I don't think the parenthesis around (priv->can.tdc.tdco) are needed. btr0 = btr0 | priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | XCAN_BRPR_TDC_ENABLE (c.f. above for macro names) > + } > > /* Setting Time Segment 1 in BTR Register */ > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; > @@ -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device *pdev) > priv->can.data_bittiming_const = > &xcan_data_bittiming_const_canfd; > > - if (devtype->cantype == XAXI_CANFD_2_0) > + if (devtype->cantype == XAXI_CANFD_2_0) { > priv->can.data_bittiming_const = > &xcan_data_bittiming_const_canfd2; > + priv->can.tdc_const = &xcan_tdc_const; > + } > > if (devtype->cantype == XAXI_CANFD || > devtype->cantype == XAXI_CANFD_2_0) > - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > + CAN_CTRLMODE_TDC_AUTO; > > priv->reg_base = addr; > priv->tx_max = tx_max; > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-13 1:24 ` Vincent Mailhol @ 2022-05-13 1:37 ` Vincent Mailhol 2022-05-13 11:56 ` Marc Kleine-Budde 2022-05-26 15:51 ` Srinivas Neeli 2 siblings, 0 replies; 8+ messages in thread From: Vincent Mailhol @ 2022-05-13 1:37 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, mkl, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git On Fri. 13 May 2022 at 10:24, Vincent Mailhol <vincent.mailhol@gmail.com> wrote: > On Fri. 13 May 2022 at 07:30, Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: [...] > btr0 = btr0 | > priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > XCAN_BRPR_TDC_ENABLE Actually, you can use |= operator: btr0 |= priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | XCAN_BRPR_TDC_ENABLE _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-13 1:24 ` Vincent Mailhol 2022-05-13 1:37 ` Vincent Mailhol @ 2022-05-13 11:56 ` Marc Kleine-Budde 2022-05-26 15:51 ` Srinivas Neeli 2 siblings, 0 replies; 8+ messages in thread From: Marc Kleine-Budde @ 2022-05-13 11:56 UTC (permalink / raw) To: Vincent Mailhol Cc: Srinivas Neeli, wg, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git [-- Attachment #1.1: Type: text/plain, Size: 1010 bytes --] On 13.05.2022 10:24:06, Vincent Mailhol wrote: > > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump width */ > > @@ -259,7 +261,7 @@ static const struct can_bittiming_const xcan_bittiming_const_canfd2 = { > > .tseg2_min = 1, > > .tseg2_max = 128, > > .sjw_max = 128, > > - .brp_min = 2, > > + .brp_min = 1, > > Was there any reason to have brp_min = 2 in the first place? > I think this change should be a different patch. If the brp_min = 2 > is just a typo, you might also want to backport it to stable branches. +1 Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-13 1:24 ` Vincent Mailhol 2022-05-13 1:37 ` Vincent Mailhol 2022-05-13 11:56 ` Marc Kleine-Budde @ 2022-05-26 15:51 ` Srinivas Neeli 2022-05-27 0:30 ` Vincent Mailhol 2 siblings, 1 reply; 8+ messages in thread From: Srinivas Neeli @ 2022-05-26 15:51 UTC (permalink / raw) To: Vincent Mailhol Cc: wg, mkl, davem, edumazet, Appana Durga Kedareswara Rao, Srinivas Goud, Michal Simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <vincent.mailhol@gmail.com> > Sent: Friday, May 13, 2022 6:54 AM > To: Srinivas Neeli <sneeli@xilinx.com> > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > edumazet@google.com; Appana Durga Kedareswara Rao > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal Simek > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > <git@xilinx.com> > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation > (TDC) feature support > > On Fri. 13 May 2022 at 07:30, Srinivas Neeli <srinivas.neeli@xilinx.com> > wrote: > > Added Transmitter delay compensation (TDC) feature support. > > In the case of higher measured loop delay with higher baud rates, > > observed bit stuff errors. > > By enabling the TDC feature in a controller, will compensate for the > > measure loop delay in the receive path. > > TDC feature requires BRP values can be 1 or 2. > > The current CAN framework does not limit the brp so while using TDC, > > have to restrict BRP values. > > Ex: > > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 > > 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on > > loopback on tdco 12 tdc-mode auto > > Did you experience some cases in which you had BRP > 2 and saw > transmission errors due to the absence of delay compensation? Could you > show the calculated values? > Usually, you start to observe but stuff error at high bitrates (e.g. > ~5MBPS), and for such bitrates, time quanta has to be small which then > results in a small BRP. yes, we observed errors with higher baud rates(4 and 5 MBPS). Observation: BRPA 1Mbps Sampling 75% BRPD 5MBPS Sampling 75% On NXP PHY observed a delay of 160 ns. so observing the failure. After enabling the TDC feature to work fine. > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > --- > > drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/xilinx_can.c > > b/drivers/net/can/xilinx_can.c index e2b15d29d15e..7af518fbed02 100644 > > --- a/drivers/net/can/xilinx_can.c > > +++ b/drivers/net/can/xilinx_can.c > > @@ -1,7 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0-or-later > > /* Xilinx CAN device driver > > * > > - * Copyright (C) 2012 - 2014 Xilinx, Inc. > > + * Copyright (C) 2012 - 2022 Xilinx, Inc. > > * Copyright (C) 2009 PetaLogix. All rights reserved. > > * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy > > * > > @@ -133,6 +133,8 @@ enum xcan_reg { > > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ > > > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > > +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay > Compensation Offset */ > > Having CANFD in the name is redundant (TDC implies CANFD). > #define XCAN_BRPR_TDCO_SHIFT 8 update in V2. > > > +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay > Compensation (TDC) Enable */ > > Why not: > #define XCAN_BRPR_TDC_ENABLE BIT(16) update in V2. > > > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump width > */ > > @@ -259,7 +261,7 @@ static const struct can_bittiming_const > xcan_bittiming_const_canfd2 = { > > .tseg2_min = 1, > > .tseg2_max = 128, > > .sjw_max = 128, > > - .brp_min = 2, > > + .brp_min = 1, > > Was there any reason to have brp_min = 2 in the first place? > I think this change should be a different patch. If the brp_min = 2 is just a > typo, you might also want to backport it to stable branches. On early silicon engineering samples we observed bit shrinking issue when we use brp =1 , hence we updated brp_min =2. As in production silicon this issue is fixed we are planning to revert the patch. > > > .brp_max = 256, > > .brp_inc = 1, > > }; > > @@ -272,11 +274,21 @@ static struct can_bittiming_const > xcan_data_bittiming_const_canfd2 = { > > .tseg2_min = 1, > > .tseg2_max = 16, > > .sjw_max = 16, > > - .brp_min = 2, > > + .brp_min = 1, > > .brp_max = 256, > > .brp_inc = 1, > > }; > > > > +/* Transmission Delay Compensation constants for CANFD2.0 and Versal > > +*/ static const struct can_tdc_const xcan_tdc_const = { > > + .tdcv_min = 0, > > + .tdcv_max = 0, /* Manual mode not supported. */ > > Right, had a look at the datasheet and xilinx indeed does not support setting > TDCV. > However, xilinx still has a TDCV register to report the measured transmission > delay. > > Socket CAN's TDC framework provides can_priv::do_get_auto_tdcv() to > report the measured value through the netlink interface: > https://elixir.bootlin.com/linux/v5.17/source/include/linux/can/dev.h#L87 > > Can you implement this call back function? Will implement in V2. > > > + .tdco_min = 0, > > + .tdco_max = 64, > > + .tdcf_min = 0, /* Filter window not supported */ > > + .tdcf_max = 0, > > +}; > > + > > /** > > * xcan_write_reg_le - Write a value to the device register little endian > > * @priv: Driver private data structure > > @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct net_device > *ndev) > > priv->devtype.cantype == XAXI_CANFD_2_0) { > > /* Setting Baud Rate prescalar value in F_BRPR Register */ > > btr0 = dbt->brp - 1; > > + if (can_tdc_is_enabled(&priv->can)) { > > + btr0 = btr0 | > > + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD | > > + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; > > I don't think the parenthesis around (priv->can.tdc.tdco) are needed. Yes, will update. > > btr0 = btr0 | > priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > XCAN_BRPR_TDC_ENABLE > > (c.f. above for macro names) > > > + } > > > > /* Setting Time Segment 1 in BTR Register */ > > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ > > -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device *pdev) > > priv->can.data_bittiming_const = > > &xcan_data_bittiming_const_canfd; > > > > - if (devtype->cantype == XAXI_CANFD_2_0) > > + if (devtype->cantype == XAXI_CANFD_2_0) { > > priv->can.data_bittiming_const = > > &xcan_data_bittiming_const_canfd2; > > + priv->can.tdc_const = &xcan_tdc_const; > > + } > > > > if (devtype->cantype == XAXI_CANFD || > > devtype->cantype == XAXI_CANFD_2_0) > > - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > > + CAN_CTRLMODE_TDC_AUTO; > > > > priv->reg_base = addr; > > priv->tx_max = tx_max; > > -- > > 2.25.1 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-26 15:51 ` Srinivas Neeli @ 2022-05-27 0:30 ` Vincent Mailhol 2022-05-28 0:46 ` Srinivas Neeli 0 siblings, 1 reply; 8+ messages in thread From: Vincent Mailhol @ 2022-05-27 0:30 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, mkl, davem, edumazet, Appana Durga Kedareswara Rao, Srinivas Goud, Michal Simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git On Fri. 27 May 2022 at 00:51, Srinivas Neeli <sneeli@xilinx.com> wrote: > Hi Vincent, > > > -----Original Message----- > > From: Vincent Mailhol <vincent.mailhol@gmail.com> > > Sent: Friday, May 13, 2022 6:54 AM > > To: Srinivas Neeli <sneeli@xilinx.com> > > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > > edumazet@google.com; Appana Durga Kedareswara Rao > > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal Simek > > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > > <git@xilinx.com> > > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation > > (TDC) feature support > > > > On Fri. 13 May 2022 at 07:30, Srinivas Neeli <srinivas.neeli@xilinx.com> > > wrote: > > > Added Transmitter delay compensation (TDC) feature support. > > > In the case of higher measured loop delay with higher baud rates, > > > observed bit stuff errors. > > > By enabling the TDC feature in a controller, will compensate for the > > > measure loop delay in the receive path. > > > TDC feature requires BRP values can be 1 or 2. > > > The current CAN framework does not limit the brp so while using TDC, > > > have to restrict BRP values. > > > Ex: > > > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 > > > 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on > > > loopback on tdco 12 tdc-mode auto > > > > Did you experience some cases in which you had BRP > 2 and saw > > transmission errors due to the absence of delay compensation? Could you > > show the calculated values? > > Usually, you start to observe but stuff error at high bitrates (e.g. > > ~5MBPS), and for such bitrates, time quanta has to be small which then > > results in a small BRP. > > yes, we observed errors with higher baud rates(4 and 5 MBPS). > Observation: > BRPA 1Mbps Sampling 75% > BRPD 5MBPS Sampling 75% > On NXP PHY observed a delay of 160 ns. so observing the failure. > After enabling the TDC feature to work fine. Can you also share the results of: | ip --details link show can0 for both the automatic calculation by the CAN framework and for your hand calculated values? Thank you! > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > > --- > > > drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- > > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/can/xilinx_can.c > > > b/drivers/net/can/xilinx_can.c index e2b15d29d15e..7af518fbed02 100644 > > > --- a/drivers/net/can/xilinx_can.c > > > +++ b/drivers/net/can/xilinx_can.c > > > @@ -1,7 +1,7 @@ > > > // SPDX-License-Identifier: GPL-2.0-or-later > > > /* Xilinx CAN device driver > > > * > > > - * Copyright (C) 2012 - 2014 Xilinx, Inc. > > > + * Copyright (C) 2012 - 2022 Xilinx, Inc. > > > * Copyright (C) 2009 PetaLogix. All rights reserved. > > > * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy > > > * > > > @@ -133,6 +133,8 @@ enum xcan_reg { > > > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ > > > > > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > > > +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay > > Compensation Offset */ > > > > Having CANFD in the name is redundant (TDC implies CANFD). > > #define XCAN_BRPR_TDCO_SHIFT 8 > update in V2. > > > > > > +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay > > Compensation (TDC) Enable */ > > > > Why not: > > #define XCAN_BRPR_TDC_ENABLE BIT(16) > update in V2. > > > > > > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > > > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > > > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump width > > */ > > > @@ -259,7 +261,7 @@ static const struct can_bittiming_const > > xcan_bittiming_const_canfd2 = { > > > .tseg2_min = 1, > > > .tseg2_max = 128, > > > .sjw_max = 128, > > > - .brp_min = 2, > > > + .brp_min = 1, > > > > Was there any reason to have brp_min = 2 in the first place? > > I think this change should be a different patch. If the brp_min = 2 is just a > > typo, you might also want to backport it to stable branches. > > On early silicon engineering samples we observed bit shrinking issue when we use brp =1 , hence we updated brp_min =2. > As in production silicon this issue is fixed we are planning to revert the patch. Great! > > > .brp_max = 256, > > > .brp_inc = 1, > > > }; > > > @@ -272,11 +274,21 @@ static struct can_bittiming_const > > xcan_data_bittiming_const_canfd2 = { > > > .tseg2_min = 1, > > > .tseg2_max = 16, > > > .sjw_max = 16, > > > - .brp_min = 2, > > > + .brp_min = 1, > > > .brp_max = 256, > > > .brp_inc = 1, > > > }; > > > > > > +/* Transmission Delay Compensation constants for CANFD2.0 and Versal > > > +*/ static const struct can_tdc_const xcan_tdc_const = { > > > + .tdcv_min = 0, > > > + .tdcv_max = 0, /* Manual mode not supported. */ > > > > Right, had a look at the datasheet and xilinx indeed does not support setting > > TDCV. > > However, xilinx still has a TDCV register to report the measured transmission > > delay. > > > > Socket CAN's TDC framework provides can_priv::do_get_auto_tdcv() to > > report the measured value through the netlink interface: > > https://elixir.bootlin.com/linux/v5.17/source/include/linux/can/dev.h#L87 > > > > Can you implement this call back function? > Will implement in V2. > > > > > > + .tdco_min = 0, > > > + .tdco_max = 64, > > > + .tdcf_min = 0, /* Filter window not supported */ > > > + .tdcf_max = 0, > > > +}; > > > + > > > /** > > > * xcan_write_reg_le - Write a value to the device register little endian > > > * @priv: Driver private data structure > > > @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct net_device > > *ndev) > > > priv->devtype.cantype == XAXI_CANFD_2_0) { > > > /* Setting Baud Rate prescalar value in F_BRPR Register */ > > > btr0 = dbt->brp - 1; > > > + if (can_tdc_is_enabled(&priv->can)) { > > > + btr0 = btr0 | > > > + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD | > > > + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; > > > > I don't think the parenthesis around (priv->can.tdc.tdco) are needed. > Yes, will update. > > > > btr0 = btr0 | > > priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > > XCAN_BRPR_TDC_ENABLE > > > > (c.f. above for macro names) > > > > > + } > > > > > > /* Setting Time Segment 1 in BTR Register */ > > > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ > > > -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device *pdev) > > > priv->can.data_bittiming_const = > > > &xcan_data_bittiming_const_canfd; > > > > > > - if (devtype->cantype == XAXI_CANFD_2_0) > > > + if (devtype->cantype == XAXI_CANFD_2_0) { > > > priv->can.data_bittiming_const = > > > &xcan_data_bittiming_const_canfd2; > > > + priv->can.tdc_const = &xcan_tdc_const; > > > + } > > > > > > if (devtype->cantype == XAXI_CANFD || > > > devtype->cantype == XAXI_CANFD_2_0) > > > - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > > > + CAN_CTRLMODE_TDC_AUTO; > > > > > > priv->reg_base = addr; > > > priv->tx_max = tx_max; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-27 0:30 ` Vincent Mailhol @ 2022-05-28 0:46 ` Srinivas Neeli 2022-05-29 14:25 ` Vincent Mailhol 0 siblings, 1 reply; 8+ messages in thread From: Srinivas Neeli @ 2022-05-28 0:46 UTC (permalink / raw) To: Vincent Mailhol Cc: wg, mkl, davem, edumazet, Appana Durga Kedareswara Rao, Srinivas Goud, Michal Simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <vincent.mailhol@gmail.com> > Sent: Friday, May 27, 2022 6:01 AM > To: Srinivas Neeli <sneeli@xilinx.com> > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > edumazet@google.com; Appana Durga Kedareswara Rao > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal Simek > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > <git@xilinx.com> > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation > (TDC) feature support > > On Fri. 27 May 2022 at 00:51, Srinivas Neeli <sneeli@xilinx.com> wrote: > > Hi Vincent, > > > > > -----Original Message----- > > > From: Vincent Mailhol <vincent.mailhol@gmail.com> > > > Sent: Friday, May 13, 2022 6:54 AM > > > To: Srinivas Neeli <sneeli@xilinx.com> > > > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > > > edumazet@google.com; Appana Durga Kedareswara Rao > > > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal > Simek > > > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > > > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > > > <git@xilinx.com> > > > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay > > > compensation > > > (TDC) feature support > > > > > > On Fri. 13 May 2022 at 07:30, Srinivas Neeli > > > <srinivas.neeli@xilinx.com> > > > wrote: > > > > Added Transmitter delay compensation (TDC) feature support. > > > > In the case of higher measured loop delay with higher baud rates, > > > > observed bit stuff errors. > > > > By enabling the TDC feature in a controller, will compensate for > > > > the measure loop delay in the receive path. > > > > TDC feature requires BRP values can be 1 or 2. > > > > The current CAN framework does not limit the brp so while using > > > > TDC, have to restrict BRP values. > > > > Ex: > > > > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 > > > > phase-seg2 > > > > 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd > > > > on loopback on tdco 12 tdc-mode auto > > > > > > Did you experience some cases in which you had BRP > 2 and saw > > > transmission errors due to the absence of delay compensation? Could > > > you show the calculated values? > > > Usually, you start to observe but stuff error at high bitrates (e.g. > > > ~5MBPS), and for such bitrates, time quanta has to be small which > > > then results in a small BRP. > > > > yes, we observed errors with higher baud rates(4 and 5 MBPS). > > Observation: > > BRPA 1Mbps Sampling 75% > > BRPD 5MBPS Sampling 75% > > On NXP PHY observed a delay of 160 ns. so observing the failure. > > After enabling the TDC feature to work fine. > > Can you also share the results of: > > | ip --details link show can0 > > for both the automatic calculation by the CAN framework and for your hand > calculated values? ip --details link show can6 root@xilinx-vck190-2021_1:~# ip --details link show can6 9: can6: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 link/can promiscuity 0 minmtu 0 maxmtu 0 can <FD> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0 bitrate 999999 sample-point 0.750 tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1 xilinx_can: tseg1 1..256 tseg2 1..128 sjw 1..128 brp 2..256 brp-inc 1 dbitrate 4999999 dsample-point 0.750 dtq 50 dprop-seg 1 dphase-seg1 1 dphase-seg2 1 dsjw 1 xilinx_can: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 2..256 dbrp-inc 1 clock 79999999 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 root@xilinx-vck190-2021_1:~# Hand calculated values: ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on Observations: --------------- Board VCK190 +NXP phy. Observed TDCV value generated by IP is 9clock cycles(112ns)[Choosing Falling edge of bit]. But Bit starts on rx line after 160 ns observed with scope. Exp1: BRPA 1Mbps Sampling 75% BRPD 5MBPS Sampling 75% Without TDC feature: Bit length 200ns, Sampling point 75% means 150ns. IP tries to sample at 150ns , but Bit starting at 160ns,so observed failure. With TDC feature: Bit length 200ns, Sampling point 75%, TDCO =12clock cycles(150ns). Ip tries after 112+150 = 262ns,able to sample the bit. Thank you! > > > Thank you! > > > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > > > --- > > > > drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- > > > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/can/xilinx_can.c > > > > b/drivers/net/can/xilinx_can.c index e2b15d29d15e..7af518fbed02 > > > > 100644 > > > > --- a/drivers/net/can/xilinx_can.c > > > > +++ b/drivers/net/can/xilinx_can.c > > > > @@ -1,7 +1,7 @@ > > > > // SPDX-License-Identifier: GPL-2.0-or-later > > > > /* Xilinx CAN device driver > > > > * > > > > - * Copyright (C) 2012 - 2014 Xilinx, Inc. > > > > + * Copyright (C) 2012 - 2022 Xilinx, Inc. > > > > * Copyright (C) 2009 PetaLogix. All rights reserved. > > > > * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy > > > > * > > > > @@ -133,6 +133,8 @@ enum xcan_reg { > > > > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC > */ > > > > > > > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > > > > +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay > > > Compensation Offset */ > > > > > > Having CANFD in the name is redundant (TDC implies CANFD). > > > #define XCAN_BRPR_TDCO_SHIFT 8 > > update in V2. > > > > > > > > > +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay > > > Compensation (TDC) Enable */ > > > > > > Why not: > > > #define XCAN_BRPR_TDC_ENABLE BIT(16) > > update in V2. > > > > > > > > > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > > > > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > > > > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump > width > > > */ > > > > @@ -259,7 +261,7 @@ static const struct can_bittiming_const > > > xcan_bittiming_const_canfd2 = { > > > > .tseg2_min = 1, > > > > .tseg2_max = 128, > > > > .sjw_max = 128, > > > > - .brp_min = 2, > > > > + .brp_min = 1, > > > > > > Was there any reason to have brp_min = 2 in the first place? > > > I think this change should be a different patch. If the brp_min = 2 > > > is just a typo, you might also want to backport it to stable branches. > > > > On early silicon engineering samples we observed bit shrinking issue when > we use brp =1 , hence we updated brp_min =2. > > As in production silicon this issue is fixed we are planning to revert the > patch. > > Great! > > > > > .brp_max = 256, > > > > .brp_inc = 1, > > > > }; > > > > @@ -272,11 +274,21 @@ static struct can_bittiming_const > > > xcan_data_bittiming_const_canfd2 = { > > > > .tseg2_min = 1, > > > > .tseg2_max = 16, > > > > .sjw_max = 16, > > > > - .brp_min = 2, > > > > + .brp_min = 1, > > > > .brp_max = 256, > > > > .brp_inc = 1, > > > > }; > > > > > > > > +/* Transmission Delay Compensation constants for CANFD2.0 and > > > > +Versal */ static const struct can_tdc_const xcan_tdc_const = { > > > > + .tdcv_min = 0, > > > > + .tdcv_max = 0, /* Manual mode not supported. */ > > > > > > Right, had a look at the datasheet and xilinx indeed does not > > > support setting TDCV. > > > However, xilinx still has a TDCV register to report the measured > > > transmission delay. > > > > > > Socket CAN's TDC framework provides can_priv::do_get_auto_tdcv() to > > > report the measured value through the netlink interface: > > > https://elixir.bootlin.com/linux/v5.17/source/include/linux/can/dev. > > > h#L87 > > > > > > Can you implement this call back function? > > Will implement in V2. > > > > > > > > > + .tdco_min = 0, > > > > + .tdco_max = 64, > > > > + .tdcf_min = 0, /* Filter window not supported */ > > > > + .tdcf_max = 0, > > > > +}; > > > > + > > > > /** > > > > * xcan_write_reg_le - Write a value to the device register little endian > > > > * @priv: Driver private data structure > > > > @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct > > > > net_device > > > *ndev) > > > > priv->devtype.cantype == XAXI_CANFD_2_0) { > > > > /* Setting Baud Rate prescalar value in F_BRPR Register */ > > > > btr0 = dbt->brp - 1; > > > > + if (can_tdc_is_enabled(&priv->can)) { > > > > + btr0 = btr0 | > > > > + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD > | > > > > + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; > > > > > > I don't think the parenthesis around (priv->can.tdc.tdco) are needed. > > Yes, will update. > > > > > > btr0 = btr0 | > > > priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > > > XCAN_BRPR_TDC_ENABLE > > > > > > (c.f. above for macro names) > > > > > > > + } > > > > > > > > /* Setting Time Segment 1 in BTR Register */ > > > > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ > > > > -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device > *pdev) > > > > priv->can.data_bittiming_const = > > > > &xcan_data_bittiming_const_canfd; > > > > > > > > - if (devtype->cantype == XAXI_CANFD_2_0) > > > > + if (devtype->cantype == XAXI_CANFD_2_0) { > > > > priv->can.data_bittiming_const = > > > > &xcan_data_bittiming_const_canfd2; > > > > + priv->can.tdc_const = &xcan_tdc_const; > > > > + } > > > > > > > > if (devtype->cantype == XAXI_CANFD || > > > > devtype->cantype == XAXI_CANFD_2_0) > > > > - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > > > > + > > > > + CAN_CTRLMODE_TDC_AUTO; > > > > > > > > priv->reg_base = addr; > > > > priv->tx_max = tx_max; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-05-28 0:46 ` Srinivas Neeli @ 2022-05-29 14:25 ` Vincent Mailhol 0 siblings, 0 replies; 8+ messages in thread From: Vincent Mailhol @ 2022-05-29 14:25 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, mkl, davem, edumazet, Appana Durga Kedareswara Rao, Srinivas Goud, Michal Simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git On Sat. 28 May 2022 at 09:46, Srinivas Neeli <sneeli@xilinx.com> wrote: > Hi Vincent, > > > -----Original Message----- > > From: Vincent Mailhol <vincent.mailhol@gmail.com> > > Sent: Friday, May 27, 2022 6:01 AM > > To: Srinivas Neeli <sneeli@xilinx.com> > > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > > edumazet@google.com; Appana Durga Kedareswara Rao > > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal Simek > > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > > <git@xilinx.com> > > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay compensation > > (TDC) feature support > > > > On Fri. 27 May 2022 at 00:51, Srinivas Neeli <sneeli@xilinx.com> wrote: > > > Hi Vincent, > > > > > > > -----Original Message----- > > > > From: Vincent Mailhol <vincent.mailhol@gmail.com> > > > > Sent: Friday, May 13, 2022 6:54 AM > > > > To: Srinivas Neeli <sneeli@xilinx.com> > > > > Cc: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net; > > > > edumazet@google.com; Appana Durga Kedareswara Rao > > > > <appanad@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal > > Simek > > > > <michals@xilinx.com>; kuba@kernel.org; pabeni@redhat.com; linux- > > > > can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > > > > <git@xilinx.com> > > > > Subject: Re: [PATCH] can: xilinx_can: Add Transmitter delay > > > > compensation > > > > (TDC) feature support > > > > > > > > On Fri. 13 May 2022 at 07:30, Srinivas Neeli > > > > <srinivas.neeli@xilinx.com> > > > > wrote: > > > > > Added Transmitter delay compensation (TDC) feature support. > > > > > In the case of higher measured loop delay with higher baud rates, > > > > > observed bit stuff errors. > > > > > By enabling the TDC feature in a controller, will compensate for > > > > > the measure loop delay in the receive path. > > > > > TDC feature requires BRP values can be 1 or 2. > > > > > The current CAN framework does not limit the brp so while using > > > > > TDC, have to restrict BRP values. > > > > > Ex: > > > > > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 > > > > > phase-seg2 > > > > > 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd > > > > > on loopback on tdco 12 tdc-mode auto > > > > > > > > Did you experience some cases in which you had BRP > 2 and saw > > > > transmission errors due to the absence of delay compensation? Could > > > > you show the calculated values? > > > > Usually, you start to observe but stuff error at high bitrates (e.g. > > > > ~5MBPS), and for such bitrates, time quanta has to be small which > > > > then results in a small BRP. > > > > > > yes, we observed errors with higher baud rates(4 and 5 MBPS). > > > Observation: > > > BRPA 1Mbps Sampling 75% > > > BRPD 5MBPS Sampling 75% > > > On NXP PHY observed a delay of 160 ns. so observing the failure. > > > After enabling the TDC feature to work fine. > > > > Can you also share the results of: > > > > | ip --details link show can0 > > > > for both the automatic calculation by the CAN framework and for your hand > > calculated values? > > ip --details link show can6 > root@xilinx-vck190-2021_1:~# ip --details link show can6 > 9: can6: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 > link/can promiscuity 0 minmtu 0 maxmtu 0 > can <FD> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0 > bitrate 999999 sample-point 0.750 > tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1 > xilinx_can: tseg1 1..256 tseg2 1..128 sjw 1..128 brp 2..256 brp-inc 1 > dbitrate 4999999 dsample-point 0.750 > dtq 50 dprop-seg 1 dphase-seg1 1 dphase-seg2 1 dsjw 1 > xilinx_can: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 2..256 dbrp-inc 1 > clock 79999999 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 > root@xilinx-vck190-2021_1:~# So the CAN framework calculated a DBRP of 4. Did you apply this patch from Marc? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=85d4eb2a3dfe939dda5304d61e406cb8e0852d60 It is supposed to solve those "too high BRP" issues. > Hand calculated values: > ip link set can0 type can tq 12 prop-seg 39 phase-seg1 20 phase-seg2 20 sjw 20 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 4 fd on With tq = 12 you are off by 4%, right? I get: dbrp = dtq * clock / 1000000000 = 12 * 80000000 / 1000000000 = 0.96 instead of brp = 1. Wouldn't dtq = 25 and dbrp = 2 be the best here? > Observations: > --------------- > Board VCK190 +NXP phy. > Observed TDCV value generated by IP is 9clock cycles(112ns)[Choosing Falling edge of bit]. > But Bit starts on rx line after 160 ns observed with scope. > > Exp1: > BRPA 1Mbps Sampling 75% > BRPD 5MBPS Sampling 75% > > Without TDC feature: > Bit length 200ns, Sampling point 75% means 150ns. > IP tries to sample at 150ns , but Bit starting at 160ns,so observed failure. > > With TDC feature: > Bit length 200ns, Sampling point 75%, TDCO =12clock cycles(150ns). > Ip tries after 112+150 = 262ns,able to sample the bit. That seems correct. My worries is why the framework calculates a DBRP of 4. Please check that Mark's patch was correctly applied. If not, try it and, hopefully, this will fix your issue and you will not have to enter manually calculated parameters again. > Thank you! > > > > > > Thank you! > > > > > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > > > > --- > > > > > drivers/net/can/xilinx_can.c | 30 +++++++++++++++++++++++++----- > > > > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/net/can/xilinx_can.c > > > > > b/drivers/net/can/xilinx_can.c index e2b15d29d15e..7af518fbed02 > > > > > 100644 > > > > > --- a/drivers/net/can/xilinx_can.c > > > > > +++ b/drivers/net/can/xilinx_can.c > > > > > @@ -1,7 +1,7 @@ > > > > > // SPDX-License-Identifier: GPL-2.0-or-later > > > > > /* Xilinx CAN device driver > > > > > * > > > > > - * Copyright (C) 2012 - 2014 Xilinx, Inc. > > > > > + * Copyright (C) 2012 - 2022 Xilinx, Inc. > > > > > * Copyright (C) 2009 PetaLogix. All rights reserved. > > > > > * Copyright (C) 2017 - 2018 Sandvik Mining and Construction Oy > > > > > * > > > > > @@ -133,6 +133,8 @@ enum xcan_reg { > > > > > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC > > */ > > > > > > > > > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > > > > > +#define XCAN_BRPR_TDCO_SHIFT_CANFD 8 /* Transmitter Delay > > > > Compensation Offset */ > > > > > > > > Having CANFD in the name is redundant (TDC implies CANFD). > > > > #define XCAN_BRPR_TDCO_SHIFT 8 > > > update in V2. > > > > > > > > > > > > +#define XCAN_BRPR_TDCE_SHIFT_CANFD 16 /* Transmitter Delay > > > > Compensation (TDC) Enable */ > > > > > > > > Why not: > > > > #define XCAN_BRPR_TDC_ENABLE BIT(16) > > > update in V2. > > > > > > > > > > > > #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ > > > > > #define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */ > > > > > #define XCAN_BTR_SJW_SHIFT_CANFD 16 /* Synchronous jump > > width > > > > */ > > > > > @@ -259,7 +261,7 @@ static const struct can_bittiming_const > > > > xcan_bittiming_const_canfd2 = { > > > > > .tseg2_min = 1, > > > > > .tseg2_max = 128, > > > > > .sjw_max = 128, > > > > > - .brp_min = 2, > > > > > + .brp_min = 1, > > > > > > > > Was there any reason to have brp_min = 2 in the first place? > > > > I think this change should be a different patch. If the brp_min = 2 > > > > is just a typo, you might also want to backport it to stable branches. > > > > > > On early silicon engineering samples we observed bit shrinking issue when > > we use brp =1 , hence we updated brp_min =2. > > > As in production silicon this issue is fixed we are planning to revert the > > patch. > > > > Great! > > > > > > > .brp_max = 256, > > > > > .brp_inc = 1, > > > > > }; > > > > > @@ -272,11 +274,21 @@ static struct can_bittiming_const > > > > xcan_data_bittiming_const_canfd2 = { > > > > > .tseg2_min = 1, > > > > > .tseg2_max = 16, > > > > > .sjw_max = 16, > > > > > - .brp_min = 2, > > > > > + .brp_min = 1, > > > > > .brp_max = 256, > > > > > .brp_inc = 1, > > > > > }; > > > > > > > > > > +/* Transmission Delay Compensation constants for CANFD2.0 and > > > > > +Versal */ static const struct can_tdc_const xcan_tdc_const = { > > > > > + .tdcv_min = 0, > > > > > + .tdcv_max = 0, /* Manual mode not supported. */ > > > > > > > > Right, had a look at the datasheet and xilinx indeed does not > > > > support setting TDCV. > > > > However, xilinx still has a TDCV register to report the measured > > > > transmission delay. > > > > > > > > Socket CAN's TDC framework provides can_priv::do_get_auto_tdcv() to > > > > report the measured value through the netlink interface: > > > > https://elixir.bootlin.com/linux/v5.17/source/include/linux/can/dev. > > > > h#L87 > > > > > > > > Can you implement this call back function? > > > Will implement in V2. > > > > > > > > > > > > + .tdco_min = 0, > > > > > + .tdco_max = 64, > > > > > + .tdcf_min = 0, /* Filter window not supported */ > > > > > + .tdcf_max = 0, > > > > > +}; > > > > > + > > > > > /** > > > > > * xcan_write_reg_le - Write a value to the device register little endian > > > > > * @priv: Driver private data structure > > > > > @@ -425,6 +437,11 @@ static int xcan_set_bittiming(struct > > > > > net_device > > > > *ndev) > > > > > priv->devtype.cantype == XAXI_CANFD_2_0) { > > > > > /* Setting Baud Rate prescalar value in F_BRPR Register */ > > > > > btr0 = dbt->brp - 1; > > > > > + if (can_tdc_is_enabled(&priv->can)) { > > > > > + btr0 = btr0 | > > > > > + (priv->can.tdc.tdco) << XCAN_BRPR_TDCO_SHIFT_CANFD > > | > > > > > + 1 << XCAN_BRPR_TDCE_SHIFT_CANFD; > > > > > > > > I don't think the parenthesis around (priv->can.tdc.tdco) are needed. > > > Yes, will update. > > > > > > > > btr0 = btr0 | > > > > priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > > > > XCAN_BRPR_TDC_ENABLE > > > > > > > > (c.f. above for macro names) > > > > > > > > > + } > > > > > > > > > > /* Setting Time Segment 1 in BTR Register */ > > > > > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ > > > > > -1747,13 +1764,16 @@ static int xcan_probe(struct platform_device > > *pdev) > > > > > priv->can.data_bittiming_const = > > > > > &xcan_data_bittiming_const_canfd; > > > > > > > > > > - if (devtype->cantype == XAXI_CANFD_2_0) > > > > > + if (devtype->cantype == XAXI_CANFD_2_0) { > > > > > priv->can.data_bittiming_const = > > > > > &xcan_data_bittiming_const_canfd2; > > > > > + priv->can.tdc_const = &xcan_tdc_const; > > > > > + } > > > > > > > > > > if (devtype->cantype == XAXI_CANFD || > > > > > devtype->cantype == XAXI_CANFD_2_0) > > > > > - priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD; > > > > > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > > > > > + > > > > > + CAN_CTRLMODE_TDC_AUTO; > > > > > > > > > > priv->reg_base = addr; > > > > > priv->tx_max = tx_max; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-29 14:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-12 13:59 [PATCH] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli 2022-05-13 1:24 ` Vincent Mailhol 2022-05-13 1:37 ` Vincent Mailhol 2022-05-13 11:56 ` Marc Kleine-Budde 2022-05-26 15:51 ` Srinivas Neeli 2022-05-27 0:30 ` Vincent Mailhol 2022-05-28 0:46 ` Srinivas Neeli 2022-05-29 14:25 ` Vincent Mailhol
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).