* [PATCH V2 0/2] xilinx_can: Update on xilinx can
@ 2022-06-07 8:56 Srinivas Neeli
2022-06-07 8:56 ` [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" Srinivas Neeli
2022-06-07 8:56 ` [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli
0 siblings, 2 replies; 6+ messages in thread
From: Srinivas Neeli @ 2022-06-07 8:56 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
This patch series addresses
1) Reverts the limiting CANFD brp_min to 2.
2) Adds TDC support for Xilinx can driver.
Srinivas Neeli (2):
Revert "can: xilinx_can: Limit CANFD brp to 2"
can: xilinx_can: Add Transmitter delay compensation (TDC) feature
support
drivers/net/can/xilinx_can.c | 50 ++++++++++++++++++++++++++++++++----
1 file changed, 45 insertions(+), 5 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" 2022-06-07 8:56 [PATCH V2 0/2] xilinx_can: Update on xilinx can Srinivas Neeli @ 2022-06-07 8:56 ` Srinivas Neeli 2022-06-07 9:09 ` Marc Kleine-Budde 2022-06-07 8:56 ` [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli 1 sibling, 1 reply; 6+ messages in thread From: Srinivas Neeli @ 2022-06-07 8:56 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 This reverts commit 05ca14fdb6fe65614e0652d03e44b02748d25af7. On early silicon engineering samples observed bit shrinking issue when we use brp as 1. Hence updated brp_min as 2. As in production silicon this issue is fixed,so reverting the patch. Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> --- drivers/net/can/xilinx_can.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 8a3b7b103ca4..e179d311aa28 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -258,7 +258,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, }; @@ -271,7 +271,7 @@ static const 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, }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" 2022-06-07 8:56 ` [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" Srinivas Neeli @ 2022-06-07 9:09 ` Marc Kleine-Budde 0 siblings, 0 replies; 6+ messages in thread From: Marc Kleine-Budde @ 2022-06-07 9:09 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git [-- Attachment #1: Type: text/plain, Size: 695 bytes --] On 07.06.2022 14:26:53, Srinivas Neeli wrote: > This reverts commit 05ca14fdb6fe65614e0652d03e44b02748d25af7. > > On early silicon engineering samples observed > bit shrinking issue when we use brp as 1. > Hence updated brp_min as 2. As in production > silicon this issue is fixed,so reverting the patch. > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> Should this be applied to -stable? 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-06-07 8:56 [PATCH V2 0/2] xilinx_can: Update on xilinx can Srinivas Neeli 2022-06-07 8:56 ` [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" Srinivas Neeli @ 2022-06-07 8:56 ` Srinivas Neeli 2022-06-07 9:19 ` Marc Kleine-Budde 1 sibling, 1 reply; 6+ messages in thread From: Srinivas Neeli @ 2022-06-07 8:56 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. Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> --- drivers/net/can/xilinx_can.c | 46 +++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index e179d311aa28..d0edd1bca33c 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 * @@ -99,6 +99,7 @@ enum xcan_reg { #define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */ #define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */ #define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */ +#define XCAN_SR_TDCV_MASK 0x007F0000 /* TDCV Value */ #define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full */ #define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */ #define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning */ @@ -132,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 8 /* Transmitter Delay Compensation Offset */ +#define XCAN_BRPR_TDC_ENABLE BIT(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 */ @@ -140,6 +143,7 @@ enum xcan_reg { #define XCAN_IDR_ID2_SHIFT 1 /* Extended Message Identifier */ #define XCAN_DLCR_DLC_SHIFT 28 /* Data length code */ #define XCAN_ESR_REC_SHIFT 8 /* Rx Error Count */ +#define XCAN_SR_TDCV_SHIFT 16 /* TDCV Value */ /* CAN frame length constants */ #define XCAN_FRAME_MAX_DATA_LEN 8 @@ -276,6 +280,16 @@ static const struct can_bittiming_const xcan_data_bittiming_const_canfd2 = { .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 @@ -424,6 +438,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 | + XCAN_BRPR_TDC_ENABLE; + } /* Setting Time Segment 1 in BTR Register */ btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; @@ -1483,6 +1502,23 @@ static int xcan_get_berr_counter(const struct net_device *ndev, return 0; } +/** + * xcan_get_auto_tdcv - Get Transmitter Delay Compensation Value + * @ndev: Pointer to net_device structure + * @tdcv: Pointer to TDCV value + * + * Return: 0 on success + */ +static int xcan_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv) +{ + struct xcan_priv *priv = netdev_priv(ndev); + + *tdcv = (priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_TDCV_MASK) >> + XCAN_SR_TDCV_SHIFT; + + return 0; +} + static const struct net_device_ops xcan_netdev_ops = { .ndo_open = xcan_open, .ndo_stop = xcan_close, @@ -1734,18 +1770,22 @@ static int xcan_probe(struct platform_device *pdev) priv->can.do_get_berr_counter = xcan_get_berr_counter; priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_BERR_REPORTING; + priv->can.do_get_auto_tdcv = xcan_get_auto_tdcv; if (devtype->cantype == XAXI_CANFD) 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-06-07 8:56 ` [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli @ 2022-06-07 9:19 ` Marc Kleine-Budde 2022-06-07 9:57 ` Vincent MAILHOL 0 siblings, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2022-06-07 9:19 UTC (permalink / raw) To: Srinivas Neeli Cc: wg, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git, Vincent Mailhol [-- Attachment #1: Type: text/plain, Size: 5901 bytes --] Hello Srinivas Neeli, thanks for your patch! On 07.06.2022 14:26:54, Srinivas Neeli 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. Wich controllers support TDC? XAXI_CANFD doesn't have do_get_auto_tdc assigned, but CAN_CTRLMODE_TDC_AUTO is set. > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > --- > drivers/net/can/xilinx_can.c | 46 +++++++++++++++++++++++++++++++++--- > 1 file changed, 43 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index e179d311aa28..d0edd1bca33c 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 > * > @@ -99,6 +99,7 @@ enum xcan_reg { > #define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */ > #define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */ > #define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */ > +#define XCAN_SR_TDCV_MASK 0x007F0000 /* TDCV Value */ > #define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full */ > #define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */ > #define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning */ > @@ -132,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 8 /* Transmitter Delay Compensation Offset */ > +#define XCAN_BRPR_TDC_ENABLE BIT(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 */ > @@ -140,6 +143,7 @@ enum xcan_reg { > #define XCAN_IDR_ID2_SHIFT 1 /* Extended Message Identifier */ > #define XCAN_DLCR_DLC_SHIFT 28 /* Data length code */ > #define XCAN_ESR_REC_SHIFT 8 /* Rx Error Count */ > +#define XCAN_SR_TDCV_SHIFT 16 /* TDCV Value */ > > /* CAN frame length constants */ > #define XCAN_FRAME_MAX_DATA_LEN 8 > @@ -276,6 +280,16 @@ static const struct can_bittiming_const xcan_data_bittiming_const_canfd2 = { > .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 > @@ -424,6 +438,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 | Make use of "|=" and properly indent. > + priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | Please include <linux/bitfield.h> and make use of "FIELD_PREP". > + XCAN_BRPR_TDC_ENABLE; > + } > > /* Setting Time Segment 1 in BTR Register */ > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; > @@ -1483,6 +1502,23 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > return 0; > } > > +/** > + * xcan_get_auto_tdcv - Get Transmitter Delay Compensation Value > + * @ndev: Pointer to net_device structure > + * @tdcv: Pointer to TDCV value > + * > + * Return: 0 on success > + */ > +static int xcan_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv) > +{ > + struct xcan_priv *priv = netdev_priv(ndev); > + > + *tdcv = (priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_TDCV_MASK) >> > + XCAN_SR_TDCV_SHIFT; Please use FIELD_GET. > + > + return 0; > +} > + > static const struct net_device_ops xcan_netdev_ops = { > .ndo_open = xcan_open, > .ndo_stop = xcan_close, > @@ -1734,18 +1770,22 @@ static int xcan_probe(struct platform_device *pdev) > priv->can.do_get_berr_counter = xcan_get_berr_counter; > priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | > CAN_CTRLMODE_BERR_REPORTING; > + priv->can.do_get_auto_tdcv = xcan_get_auto_tdcv; I'm not sure, if it has any side effects, if you assign do_get_auto_tdc for all controllers, even the ones that don't support it. Vincent can probably clarify this. > > if (devtype->cantype == XAXI_CANFD) > 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; regards 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support 2022-06-07 9:19 ` Marc Kleine-Budde @ 2022-06-07 9:57 ` Vincent MAILHOL 0 siblings, 0 replies; 6+ messages in thread From: Vincent MAILHOL @ 2022-06-07 9:57 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Srinivas Neeli, wg, davem, edumazet, appana.durga.rao, sgoud, michal.simek, kuba, pabeni, linux-can, netdev, linux-arm-kernel, linux-kernel, git On Tue. 7 Jun. 2022 at 18:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > Hello Srinivas Neeli, > > thanks for your patch! > > On 07.06.2022 14:26:54, Srinivas Neeli 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. > > Wich controllers support TDC? > > XAXI_CANFD doesn't have do_get_auto_tdc assigned, but > CAN_CTRLMODE_TDC_AUTO is set. > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > --- > > drivers/net/can/xilinx_can.c | 46 +++++++++++++++++++++++++++++++++--- > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > > index e179d311aa28..d0edd1bca33c 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 > > * > > @@ -99,6 +99,7 @@ enum xcan_reg { > > #define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */ > > #define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */ > > #define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */ > > +#define XCAN_SR_TDCV_MASK 0x007F0000 /* TDCV Value */ > > #define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full */ > > #define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */ > > #define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning */ > > @@ -132,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 8 /* Transmitter Delay Compensation Offset */ > > +#define XCAN_BRPR_TDC_ENABLE BIT(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 */ > > @@ -140,6 +143,7 @@ enum xcan_reg { > > #define XCAN_IDR_ID2_SHIFT 1 /* Extended Message Identifier */ > > #define XCAN_DLCR_DLC_SHIFT 28 /* Data length code */ > > #define XCAN_ESR_REC_SHIFT 8 /* Rx Error Count */ > > +#define XCAN_SR_TDCV_SHIFT 16 /* TDCV Value */ > > > > /* CAN frame length constants */ > > #define XCAN_FRAME_MAX_DATA_LEN 8 > > @@ -276,6 +280,16 @@ static const struct can_bittiming_const xcan_data_bittiming_const_canfd2 = { > > .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 > > @@ -424,6 +438,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 */ ^^^^^^^^^ Not related to this patch, but this is a typo. prescalar -> prescaler. > > btr0 = dbt->brp - 1; > > + if (can_tdc_is_enabled(&priv->can)) { > > + btr0 = btr0 | > > Make use of "|=" and properly indent. > > > + priv->can.tdc.tdco << XCAN_BRPR_TDCO_SHIFT | > > Please include <linux/bitfield.h> and make use of "FIELD_PREP". > > > + XCAN_BRPR_TDC_ENABLE; > > + } > > > > /* Setting Time Segment 1 in BTR Register */ > > btr1 = dbt->prop_seg + dbt->phase_seg1 - 1; > > @@ -1483,6 +1502,23 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > > return 0; > > } > > > > +/** > > + * xcan_get_auto_tdcv - Get Transmitter Delay Compensation Value > > + * @ndev: Pointer to net_device structure > > + * @tdcv: Pointer to TDCV value > > + * > > + * Return: 0 on success > > + */ > > +static int xcan_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv) > > +{ > > + struct xcan_priv *priv = netdev_priv(ndev); > > + > > + *tdcv = (priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_TDCV_MASK) >> > > + XCAN_SR_TDCV_SHIFT; > > Please use FIELD_GET. > > > + > > + return 0; > > +} > > + > > static const struct net_device_ops xcan_netdev_ops = { > > .ndo_open = xcan_open, > > .ndo_stop = xcan_close, > > @@ -1734,18 +1770,22 @@ static int xcan_probe(struct platform_device *pdev) > > priv->can.do_get_berr_counter = xcan_get_berr_counter; > > priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | > > CAN_CTRLMODE_BERR_REPORTING; > > + priv->can.do_get_auto_tdcv = xcan_get_auto_tdcv; > > I'm not sure, if it has any side effects, if you assign do_get_auto_tdc > for all controllers, even the ones that don't support it. Vincent can > probably clarify this. It should be fine. can_priv::do_get_auto_tdcv() gets called in one single location in can_tdc_fill_info(). c.f. https://elixir.bootlin.com/linux/v5.18/source/drivers/net/can/dev/netlink.c#L464 can_tdc_fill_info() returns if can_priv::tdc_const is not populated. https://elixir.bootlin.com/linux/v5.18/source/drivers/net/can/dev/netlink.c#L438 Below, you only assign tdc_const only for XAXI_CANFD_2_0, so this should be safe. *BUT*, I do not think this helps for the code readability so I encourage you to only populate can_priv::do_get_auto_tdcv() for the controllers which do support TDC. > > > > if (devtype->cantype == XAXI_CANFD) > > 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; Here, you only populate tdc_const for XAXI_CANFD_2_0... > > + } > > > > 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; ...but here, you set the CAN_CTRLMODE_TDC_AUTO for both XAXI_CANFD and XAXI_CANFD_2_0. Isn’t this a mismatch? I suggest to have a single if block in which you populate all the TDC fields at once: | if (devtype->cantype == XAXI_CANFD_2_0) { | priv->can.data_bittiming_const = | &xcan_data_bittiming_const_canfd2; | priv->can.tdc_const = &xcan_tdc_const; | priv->can.do_get_auto_tdcv = xcan_get_auto_tdcv; | priv->can.ctrlmode_supported |= CAN_CTRLMODE_TDC_AUTO; | } > > priv->reg_base = addr; > > priv->tx_max = tx_max; Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-07 9:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-07 8:56 [PATCH V2 0/2] xilinx_can: Update on xilinx can Srinivas Neeli 2022-06-07 8:56 ` [PATCH V2 1/2] Revert "can: xilinx_can: Limit CANFD brp to 2" Srinivas Neeli 2022-06-07 9:09 ` Marc Kleine-Budde 2022-06-07 8:56 ` [PATCH V2 2/2] can: xilinx_can: Add Transmitter delay compensation (TDC) feature support Srinivas Neeli 2022-06-07 9:19 ` Marc Kleine-Budde 2022-06-07 9:57 ` 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).