* [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).