netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH Resend linux-can-next/testing] can: flexcan: add correctable errors correction when HW supports ECC
@ 2020-04-16  9:31 Joakim Zhang
  2020-04-16  9:31 ` [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature Joakim Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Joakim Zhang @ 2020-04-16  9:31 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: linux-imx, netdev

commit cdce844865be ("can: flexcan: add vf610 support for FlexCAN")
From above commit by Stefan Agner, the patch just disables
non-correctable errors interrupt and freeze mode. It still can correct
the correctable errors since ECC enabled by default after reset (MECR[ECCDIS]=0,
enable memory error correct) if HW supports ECC.

commit 5e269324db5a ("can: flexcan: disable completely the ECC mechanism")
From above commit by Joakim Zhang, the patch disables ECC completely (assert
MECR[ECCDIS]) according to the explanation of FLEXCAN_QUIRK_DISABLE_MECR that
disable memory error detection. This cause correctable errors cannot be
corrected even HW supports ECC.

The error correction mechanism ensures that in this 13-bit word, errors
in one bit can be corrected (correctable errors) and errors in two bits can
be detected but not corrected (non-correctable errors). Errors in more than
two bits may not be detected.

If HW supports ECC, we can use this to correct the correctable errors detected
from FlexCAN memory. Then disable non-correctable errors interrupt and freeze
mode to avoid that put FlexCAN in freeze mode.

This patch adds correctable errors correction when HW supports ECC, and
modify explanation for FLEXCAN_QUIRK_DISABLE_MECR.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 3a754355ebe6..aa871953003a 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -187,7 +187,7 @@
 #define FLEXCAN_QUIRK_BROKEN_WERR_STATE	BIT(1) /* [TR]WRN_INT not connected */
 #define FLEXCAN_QUIRK_DISABLE_RXFG	BIT(2) /* Disable RX FIFO Global mask */
 #define FLEXCAN_QUIRK_ENABLE_EACEN_RRS	BIT(3) /* Enable EACEN and RRS bit in ctrl2 */
-#define FLEXCAN_QUIRK_DISABLE_MECR	BIT(4) /* Disable Memory error detection */
+#define FLEXCAN_QUIRK_DISABLE_MECR	BIT(4) /* Disable non-correctable errors interrupt and freeze mode */
 #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp based offloading */
 #define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for error passive */
 #define FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN	BIT(7) /* default to BE register access */
@@ -1203,8 +1203,8 @@ static int flexcan_chip_start(struct net_device *dev)
 	for (i = 0; i < priv->mb_count; i++)
 		priv->write(0, &regs->rximr[i]);
 
-	/* On Vybrid, disable memory error detection interrupts
-	 * and freeze mode.
+	/* On Vybrid, disable non-correctable errors interrupt and freeze
+	 * mode. It still can correct the correctable errors when HW supports ECC.
 	 * This also works around errata e5295 which generates
 	 * false positive memory errors and put the device in
 	 * freeze mode.
@@ -1212,19 +1212,32 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_DISABLE_MECR) {
 		/* Follow the protocol as described in "Detection
 		 * and Correction of Memory Errors" to write to
-		 * MECR register
+		 * MECR register (step 1 - 5)
+		 * 1. By default, CTRL2[ECRWRE] = 0, MECR[ECRWRDIS] = 1
+		 * 2. set CTRL2[ECRWRE]
 		 */
 		reg_ctrl2 = priv->read(&regs->ctrl2);
 		reg_ctrl2 |= FLEXCAN_CTRL2_ECRWRE;
 		priv->write(reg_ctrl2, &regs->ctrl2);
 
+		/* 3. clear MECR[ECRWRDIS] */
 		reg_mecr = priv->read(&regs->mecr);
 		reg_mecr &= ~FLEXCAN_MECR_ECRWRDIS;
 		priv->write(reg_mecr, &regs->mecr);
-		reg_mecr |= FLEXCAN_MECR_ECCDIS;
+
+		/* 4. all writes to MECR must keep MECR[ECRWRDIS] cleared */
 		reg_mecr &= ~(FLEXCAN_MECR_NCEFAFRZ | FLEXCAN_MECR_HANCEI_MSK |
 			      FLEXCAN_MECR_FANCEI_MSK);
 		priv->write(reg_mecr, &regs->mecr);
+
+		/* 5. after configuration done, lock MECR by either setting
+		 * MECR[ECRWRDIS] or clearing CTRL2[ECRWRE]
+		 */
+		reg_mecr |= FLEXCAN_MECR_ECRWRDIS;
+		priv->write(reg_mecr, &regs->mecr);
+		reg_ctrl2 &= ~FLEXCAN_CTRL2_ECRWRE;
+		priv->write(reg_ctrl2, &regs->ctrl2);
+
 	}
 
 	err = flexcan_transceiver_enable(priv);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-04-16  9:31 [PATCH Resend linux-can-next/testing] can: flexcan: add correctable errors correction when HW supports ECC Joakim Zhang
@ 2020-04-16  9:31 ` Joakim Zhang
  2020-04-16  9:41   ` Joakim Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Joakim Zhang @ 2020-04-16  9:31 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: linux-imx, netdev

We enable TDC feature in flexcan_set_bittiming when loopback off, but
disable it by mistake after calling flexcan_set_bittiming.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index b16b8abc1c2c..27f4541d9400 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1202,6 +1202,8 @@ static void flexcan_set_bittiming(struct net_device *dev)
 				/* for the TDC to work reliably, the offset has to use optimal settings */
 				reg_fdctrl |= FLEXCAN_FDCTRL_TDCOFF(((dbt->phase_seg1 - 1) + dbt->prop_seg + 2) *
 								    ((dbt->brp -1) + 1));
+			} else {
+				reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN;
 			}
 			priv->write(reg_fdctrl, &regs->fdctrl);
 
@@ -1354,7 +1356,6 @@ static int flexcan_chip_start(struct net_device *dev)
 	/* FDCTRL */
 	if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
 		reg_fdctrl = priv->read(&regs->fdctrl) & ~FLEXCAN_FDCTRL_FDRATE;
-		reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN;
 		reg_fdctrl &= ~(FLEXCAN_FDCTRL_MBDSR1(0x3) | FLEXCAN_FDCTRL_MBDSR0(0x3));
 		reg_mcr = priv->read(&regs->mcr) & ~FLEXCAN_MCR_FDEN;
 		reg_ctrl2 = priv->read(&regs->ctrl2) & ~FLEXCAN_CTRL2_ISOCANFDEN;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* RE: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-04-16  9:31 ` [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature Joakim Zhang
@ 2020-04-16  9:41   ` Joakim Zhang
  2020-06-02 10:15     ` Michael Walle
  0 siblings, 1 reply; 14+ messages in thread
From: Joakim Zhang @ 2020-04-16  9:41 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: dl-linux-imx, netdev, Michael Walle


Hi Marc,

How about FlexCAN FD patch set, it is pending for a long time. Many work would base on it, we are happy to see it in upstream mainline ASAP.

Michael Walle also gives out the test-by tag:
	Tested-by: Michael Walle <michael@walle.cc>

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2020年4月16日 17:31
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
> 
> We enable TDC feature in flexcan_set_bittiming when loopback off, but disable
> it by mistake after calling flexcan_set_bittiming.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> b16b8abc1c2c..27f4541d9400 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1202,6 +1202,8 @@ static void flexcan_set_bittiming(struct net_device
> *dev)
>  				/* for the TDC to work reliably, the offset has to use
> optimal settings */
>  				reg_fdctrl |=
> FLEXCAN_FDCTRL_TDCOFF(((dbt->phase_seg1 - 1) + dbt->prop_seg + 2) *
>  								    ((dbt->brp -1) + 1));
> +			} else {
> +				reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN;
>  			}
>  			priv->write(reg_fdctrl, &regs->fdctrl);
> 
> @@ -1354,7 +1356,6 @@ static int flexcan_chip_start(struct net_device *dev)
>  	/* FDCTRL */
>  	if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
>  		reg_fdctrl = priv->read(&regs->fdctrl) & ~FLEXCAN_FDCTRL_FDRATE;
> -		reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN;
>  		reg_fdctrl &= ~(FLEXCAN_FDCTRL_MBDSR1(0x3) |
> FLEXCAN_FDCTRL_MBDSR0(0x3));
>  		reg_mcr = priv->read(&regs->mcr) & ~FLEXCAN_MCR_FDEN;
>  		reg_ctrl2 = priv->read(&regs->ctrl2) &
> ~FLEXCAN_CTRL2_ISOCANFDEN;
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-04-16  9:41   ` Joakim Zhang
@ 2020-06-02 10:15     ` Michael Walle
  2020-06-25 12:37       ` Michael Walle
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-06-02 10:15 UTC (permalink / raw)
  To: Joakim Zhang, mkl; +Cc: linux-can, dl-linux-imx, netdev

Hi Marc,

Am 2020-04-16 11:41, schrieb Joakim Zhang:
> Hi Marc,
> 
> How about FlexCAN FD patch set, it is pending for a long time. Many
> work would base on it, we are happy to see it in upstream mainline
> ASAP.
> 
> Michael Walle also gives out the test-by tag:
> 	Tested-by: Michael Walle <michael@walle.cc>

There seems to be no activity for months here. Any reason for that? Is
there anything we can do to speed things up?

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-02 10:15     ` Michael Walle
@ 2020-06-25 12:37       ` Michael Walle
  2020-06-25 12:56         ` Marc Kleine-Budde
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-06-25 12:37 UTC (permalink / raw)
  To: Joakim Zhang, mkl; +Cc: linux-can, dl-linux-imx, netdev

Am 2020-06-02 12:15, schrieb Michael Walle:
> Hi Marc,
> 
> Am 2020-04-16 11:41, schrieb Joakim Zhang:
>> Hi Marc,
>> 
>> How about FlexCAN FD patch set, it is pending for a long time. Many
>> work would base on it, we are happy to see it in upstream mainline
>> ASAP.
>> 
>> Michael Walle also gives out the test-by tag:
>> 	Tested-by: Michael Walle <michael@walle.cc>
> 
> There seems to be no activity for months here. Any reason for that? Is
> there anything we can do to speed things up?

ping.. There are no replies or anything. Sorry but this is really
annoying and frustrating.

Marc, is there anything wrong with the flexcan patches?

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-25 12:37       ` Michael Walle
@ 2020-06-25 12:56         ` Marc Kleine-Budde
  2020-06-29  5:37           ` Joakim Zhang
                             ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2020-06-25 12:56 UTC (permalink / raw)
  To: Michael Walle, Joakim Zhang; +Cc: linux-can, dl-linux-imx, netdev

On 6/25/20 2:37 PM, Michael Walle wrote:
> Am 2020-06-02 12:15, schrieb Michael Walle:
>> Hi Marc,
>>
>> Am 2020-04-16 11:41, schrieb Joakim Zhang:
>>> Hi Marc,
>>>
>>> How about FlexCAN FD patch set, it is pending for a long time. Many
>>> work would base on it, we are happy to see it in upstream mainline
>>> ASAP.
>>>
>>> Michael Walle also gives out the test-by tag:
>>> 	Tested-by: Michael Walle <michael@walle.cc>
>>
>> There seems to be no activity for months here. Any reason for that? Is
>> there anything we can do to speed things up?
> 
> ping.. There are no replies or anything. Sorry but this is really
> annoying and frustrating.
> 
> Marc, is there anything wrong with the flexcan patches?

I've cleaned up the patches a bit, can you test this branch:

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=flexcan

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 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-25 12:56         ` Marc Kleine-Budde
@ 2020-06-29  5:37           ` Joakim Zhang
  2020-06-29 16:23           ` Michael Walle
  2020-07-23 21:09           ` Michael Walle
  2 siblings, 0 replies; 14+ messages in thread
From: Joakim Zhang @ 2020-06-29  5:37 UTC (permalink / raw)
  To: Marc Kleine-Budde, Michael Walle; +Cc: linux-can, dl-linux-imx, netdev


> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年6月25日 20:57
> To: Michael Walle <michael@walle.cc>; Joakim Zhang
> <qiangqing.zhang@nxp.com>
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
> 
> On 6/25/20 2:37 PM, Michael Walle wrote:
> > Am 2020-06-02 12:15, schrieb Michael Walle:
> >> Hi Marc,
> >>
> >> Am 2020-04-16 11:41, schrieb Joakim Zhang:
> >>> Hi Marc,
> >>>
> >>> How about FlexCAN FD patch set, it is pending for a long time. Many
> >>> work would base on it, we are happy to see it in upstream mainline
> >>> ASAP.
> >>>
> >>> Michael Walle also gives out the test-by tag:
> >>> 	Tested-by: Michael Walle <michael@walle.cc>
> >>
> >> There seems to be no activity for months here. Any reason for that?
> >> Is there anything we can do to speed things up?
> >
> > ping.. There are no replies or anything. Sorry but this is really
> > annoying and frustrating.
> >
> > Marc, is there anything wrong with the flexcan patches?
> 
> I've cleaned up the patches a bit, can you test this branch:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmkl%2Flinux-can-next.git%2
> Flog%2F%3Fh%3Dflexcan&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.co
> m%7Cddbe9019fae84641164508d819073129%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637286865961564061&amp;sdata=Bm0uynxueF%2F
> WpECT8ayfqQxh%2FMYnFCLixvZLvSgUgI4%3D&amp;reserved=0

Hi Marc,

Thank you very much for your effort to clean up the code. I have test the patch set, it can work fine at my side.

But, a patch may be ignored, could you help add back it? Or you are unsatisfied with it, I can improve it according to your opinion.

commit d6ad5cc96229e28e4f2f0960277102ee142577ed
Author: Joakim Zhang <qiangqing.zhang@nxp.com>
Date:   Fri Jul 12 08:02:51 2019 +0000

    can: flexcan: add ISO CAN FD feature support

    ISO CAN FD is introduced to increase the failture detection capability
    than non-ISO CAN FD. The non-ISO CAN FD is still supported by FlexCAN so
    that it can be used mainly during an intermediate phase, for evaluation
    and development purposes.

    Therefore, it is strongly recommended to configure FlexCAN to the ISO
    CAN FD protocol by setting the ISOCANFDEN field in the CTRL2 register.

    NOTE: If you only set "fd on", driver will use ISO FD mode by default.
    You should set "fd-non-iso on" after setting "fd on" if you want to use
    NON ISO FD mode.

    Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
    Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

And, if it is okay for you, below patch could you also pick up?
can: flexcan: disable runtime PM if register flexcandev failed : https://www.spinics.net/lists/linux-can/msg03146.html


Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.com%7Cd
> dbe9019fae84641164508d819073129%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C637286865961564061&amp;sdata=U3OQykqsSE35Az9CVznq
> Ck1rl3Hc6UvERaPuh%2BVjHiw%3D&amp;reserved=0  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-25 12:56         ` Marc Kleine-Budde
  2020-06-29  5:37           ` Joakim Zhang
@ 2020-06-29 16:23           ` Michael Walle
  2020-06-30  2:25             ` Joakim Zhang
  2020-09-15 22:15             ` Marc Kleine-Budde
  2020-07-23 21:09           ` Michael Walle
  2 siblings, 2 replies; 14+ messages in thread
From: Michael Walle @ 2020-06-29 16:23 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Joakim Zhang, linux-can, dl-linux-imx, netdev

Hi Marc,

> I've cleaned up the patches a bit, can you test this branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=flexcan

This is working, but as Joakim already said, CAN-FD ISO mode is missing.
It defaults to non-ISO mode, which is even worse, IMHO.

But I've also noticed a difference between the original patch and the
one in that branch. When FD mode is enabled the original patch checks
the priv->can.controlmode [1], the patch in the branch looks at
priv->can.ctrlmode_supported instead [2], is that correct?

-michael

[1] 
https://lore.kernel.org/netdev/20190712075926.7357-4-qiangqing.zhang@nxp.com/
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/tree/drivers/net/can/flexcan.c?h=flexcan&id=5f097cd65cb2b42b88e6e1eb186f6a8f0c90559b#n1341

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-29 16:23           ` Michael Walle
@ 2020-06-30  2:25             ` Joakim Zhang
  2020-09-15 22:16               ` Marc Kleine-Budde
  2020-09-15 22:15             ` Marc Kleine-Budde
  1 sibling, 1 reply; 14+ messages in thread
From: Joakim Zhang @ 2020-06-30  2:25 UTC (permalink / raw)
  To: Michael Walle, Marc Kleine-Budde; +Cc: linux-can, dl-linux-imx, netdev


> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: 2020年6月30日 0:23
> To: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
> 
> Hi Marc,
> 
> > I've cleaned up the patches a bit, can you test this branch:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> >
> kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmkl%2Flinux-can-next.g
> >
> it%2Flog%2F%3Fh%3Dflexcan&amp;data=02%7C01%7Cqiangqing.zhang%40nx
> p.com
> > %7C7c1d0ef7d8134c1a01dd08d81c48b55c%7C686ea1d3bc2b4c6fa92cd99c5
> c301635
> > %7C0%7C1%7C637290445925151654&amp;sdata=qYaM6gUoXED%2FcdHhR
> kzZr9D1ev8t
> > fIn2bj7knAZVaVw%3D&amp;reserved=0
> 
> This is working, but as Joakim already said, CAN-FD ISO mode is missing.
> It defaults to non-ISO mode, which is even worse, IMHO.
> 
> But I've also noticed a difference between the original patch and the one in that
> branch. When FD mode is enabled the original patch checks the
> priv->can.controlmode [1], the patch in the branch looks at
> priv->can.ctrlmode_supported instead [2], is that correct?


Hi Marc, Michael,

I have also noticed this difference, although this could not break function, but IMO, using priv->can.ctrlmode should be better.

Some nitpicks:
1) Can we use uniform check for HW which supports CAN FD in the driver, using priv->can.ctrlmode_supported instead of quirks?
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1392,7 +1392,7 @@ static int flexcan_chip_start(struct net_device *dev)
                priv->write(reg_ctrl2, &regs->ctrl2);
        }

-       if ((priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD)) {
+       if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
                u32 reg_fdctrl;

                reg_fdctrl = priv->read(&regs->fdctrl);

Also delete the redundant parentheses here.

2) Clean timing register.
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1167,6 +1167,14 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
        struct flexcan_regs __iomem *regs = priv->regs;
        u32 reg_cbt, reg_fdctrl;

+       reg_cbt = priv->read(&regs->cbt);
+       reg_cbt &= ~(FLEXCAN_CBT_BTF |
+               FIELD_PREP(FLEXCAN_CBT_EPRESDIV_MASK, 0x3ff) |
+               FIELD_PREP(FLEXCAN_CBT_ERJW_MASK, 0x1f) |
+               FIELD_PREP(FLEXCAN_CBT_EPROPSEG_MASK, 0x3f) |
+               FIELD_PREP(FLEXCAN_CBT_EPSEG1_MASK, 0x1f) |
+               FIELD_PREP(FLEXCAN_CBT_EPSEG2_MASK, 0x1f));
+
        /* CBT */
        /* CBT[EPSEG1] is 5 bit long and CBT[EPROPSEG] is 6 bit
         * long. The can_calc_bittiming() tries to divide the tseg1
@@ -1192,6 +1200,13 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
        if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
                u32 reg_fdcbt;

+               reg_fdcbt = priv->read(&regs->fdcbt);
+               reg_fdcbt &= ~(FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, 0x3ff) |
+                       FIELD_PREP(FLEXCAN_FDCBT_FRJW_MASK, 0x7) |
+                       FIELD_PREP(FLEXCAN_FDCBT_FPROPSEG_MASK, 0x1f) |
+                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG1_MASK, 0x7) |
+                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG2_MASK, 0x7));
+
                if (bt->brp != dbt->brp)
                        netdev_warn(dev, "Data brp=%d and brp=%d don't match, this may result in a phase error. Consider using different bitrate and/or data bitrate.\n",
                                    dbt->brp, bt->brp);


This is just my suggestion, to see if it is reasonable.

Best Regards,
Joakim Zhang
> -michael
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker
> nel.org%2Fnetdev%2F20190712075926.7357-4-qiangqing.zhang%40nxp.com%
> 2F&amp;data=02%7C01%7Cqiangqing.zhang%40nxp.com%7C7c1d0ef7d8134c
> 1a01dd08d81c48b55c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7
> C637290445925151654&amp;sdata=hqF23aPWVEPsIuGvxiirnEdT6KHOULF%2F
> qBi7FaFY3tg%3D&amp;reserved=0
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmkl%2Flinux-can-next.git%2
> Ftree%2Fdrivers%2Fnet%2Fcan%2Fflexcan.c%3Fh%3Dflexcan%26id%3D5f097c
> d65cb2b42b88e6e1eb186f6a8f0c90559b%23n1341&amp;data=02%7C01%7Cqi
> angqing.zhang%40nxp.com%7C7c1d0ef7d8134c1a01dd08d81c48b55c%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637290445925151654&amp;
> sdata=GWA7SqDA9tSi9mudKRC4G2nrLW4FjWJGXifJe8c3V0Q%3D&amp;reserv
> ed=0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-25 12:56         ` Marc Kleine-Budde
  2020-06-29  5:37           ` Joakim Zhang
  2020-06-29 16:23           ` Michael Walle
@ 2020-07-23 21:09           ` Michael Walle
  2020-09-14 15:16             ` Michael Walle
  2 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-07-23 21:09 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Joakim Zhang, linux-can, dl-linux-imx, netdev

Am 2020-06-25 14:56, schrieb Marc Kleine-Budde:
> On 6/25/20 2:37 PM, Michael Walle wrote:
>> Am 2020-06-02 12:15, schrieb Michael Walle:
>>> Hi Marc,
>>> 
>>> Am 2020-04-16 11:41, schrieb Joakim Zhang:
>>>> Hi Marc,
>>>> 
>>>> How about FlexCAN FD patch set, it is pending for a long time. Many
>>>> work would base on it, we are happy to see it in upstream mainline
>>>> ASAP.
>>>> 
>>>> Michael Walle also gives out the test-by tag:
>>>> 	Tested-by: Michael Walle <michael@walle.cc>
>>> 
>>> There seems to be no activity for months here. Any reason for that? 
>>> Is
>>> there anything we can do to speed things up?
>> 
>> ping.. There are no replies or anything. Sorry but this is really
>> annoying and frustrating.
>> 
>> Marc, is there anything wrong with the flexcan patches?
> 
> I've cleaned up the patches a bit, can you test this branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=flexcan

Ping. Could we please try to get this into next soon?

See also
https://lore.kernel.org/netdev/20200629181809.25338-1-michael@walle.cc/

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-07-23 21:09           ` Michael Walle
@ 2020-09-14 15:16             ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2020-09-14 15:16 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Joakim Zhang, linux-can, dl-linux-imx, netdev

Hi Marc,

Am 2020-07-23 23:09, schrieb Michael Walle:
> Am 2020-06-25 14:56, schrieb Marc Kleine-Budde:
>> On 6/25/20 2:37 PM, Michael Walle wrote:
>>> Am 2020-06-02 12:15, schrieb Michael Walle:
>>>> Hi Marc,
>>>> 
>>>> Am 2020-04-16 11:41, schrieb Joakim Zhang:
>>>>> Hi Marc,
>>>>> 
>>>>> How about FlexCAN FD patch set, it is pending for a long time. Many
>>>>> work would base on it, we are happy to see it in upstream mainline
>>>>> ASAP.
>>>>> 
>>>>> Michael Walle also gives out the test-by tag:
>>>>> 	Tested-by: Michael Walle <michael@walle.cc>
>>>> 
>>>> There seems to be no activity for months here. Any reason for that? 
>>>> Is
>>>> there anything we can do to speed things up?
>>> 
>>> ping.. There are no replies or anything. Sorry but this is really
>>> annoying and frustrating.
>>> 
>>> Marc, is there anything wrong with the flexcan patches?
>> 
>> I've cleaned up the patches a bit, can you test this branch:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=flexcan
> 
> Ping. Could we please try to get this into next soon?
> 
> See also
> https://lore.kernel.org/netdev/20200629181809.25338-1-michael@walle.cc/

Ping, another 2.5 months without any activity on this :(

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-29 16:23           ` Michael Walle
  2020-06-30  2:25             ` Joakim Zhang
@ 2020-09-15 22:15             ` Marc Kleine-Budde
  1 sibling, 0 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2020-09-15 22:15 UTC (permalink / raw)
  To: Michael Walle; +Cc: Joakim Zhang, linux-can, dl-linux-imx, netdev


[-- Attachment #1.1: Type: text/plain, Size: 955 bytes --]

On 6/29/20 6:23 PM, Michael Walle wrote:
> Hi Marc,
> 
>> I've cleaned up the patches a bit, can you test this branch:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=flexcan
> 
> This is working, but as Joakim already said, CAN-FD ISO mode is missing.
> It defaults to non-ISO mode, which is even worse, IMHO.
> 
> But I've also noticed a difference between the original patch and the
> one in that branch. When FD mode is enabled the original patch checks
> the priv->can.controlmode [1], the patch in the branch looks at
> priv->can.ctrlmode_supported instead [2], is that correct?

Nope, I've corrected this.

thanks,
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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-06-30  2:25             ` Joakim Zhang
@ 2020-09-15 22:16               ` Marc Kleine-Budde
  2020-09-16  2:26                 ` Joakim Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2020-09-15 22:16 UTC (permalink / raw)
  To: Joakim Zhang, Michael Walle; +Cc: linux-can, dl-linux-imx, netdev


[-- Attachment #1.1: Type: text/plain, Size: 3138 bytes --]

On 6/30/20 4:25 AM, Joakim Zhang wrote:
> I have also noticed this difference, although this could not break function,
> but IMO, using priv->can.ctrlmode should be better.
> 
> Some nitpicks:
>
> 1) Can we use uniform check for HW which supports CAN FD in the driver, using
> priv->can.ctrlmode_supported instead of quirks?>
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1392,7 +1392,7 @@ static int flexcan_chip_start(struct net_device *dev)
>                 priv->write(reg_ctrl2, &regs->ctrl2);
>         }
> 
> -       if ((priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD)) {
> +       if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {

makes sense

>                 u32 reg_fdctrl;
> 
>                 reg_fdctrl = priv->read(&regs->fdctrl);
> 
> Also delete the redundant parentheses here.
> 
> 2) Clean timing register.
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -1167,6 +1167,14 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
>         struct flexcan_regs __iomem *regs = priv->regs;
>         u32 reg_cbt, reg_fdctrl;
> 
> +       reg_cbt = priv->read(&regs->cbt);
> +       reg_cbt &= ~(FLEXCAN_CBT_BTF |
> +               FIELD_PREP(FLEXCAN_CBT_EPRESDIV_MASK, 0x3ff) |
> +               FIELD_PREP(FLEXCAN_CBT_ERJW_MASK, 0x1f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPROPSEG_MASK, 0x3f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPSEG1_MASK, 0x1f) |
> +               FIELD_PREP(FLEXCAN_CBT_EPSEG2_MASK, 0x1f));
> +

Why is this needed? The "reg_cbt &=" sets reg_cbt basically to 0, as the fields
and the BTF occupy all 32bit.

The only thing that's left over is the read()....

>         /* CBT */
>         /* CBT[EPSEG1] is 5 bit long and CBT[EPROPSEG] is 6 bit
>          * long. The can_calc_bittiming() tries to divide the tseg1
> @@ -1192,6 +1200,13 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
>         if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
>                 u32 reg_fdcbt;
> 
> +               reg_fdcbt = priv->read(&regs->fdcbt);
> +               reg_fdcbt &= ~(FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, 0x3ff) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FRJW_MASK, 0x7) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPROPSEG_MASK, 0x1f) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG1_MASK, 0x7) |
> +                       FIELD_PREP(FLEXCAN_FDCBT_FPSEG2_MASK, 0x7));
> +

Okay, I'll add this, as the fdcbt contains some reserved bits...Let's preserve them.

I've changed the setting of reg_fdcbt like this to make sense:

> -               reg_fdcbt = FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, dbt->brp - 1) |
> +               reg_fdcbt |= FIELD_PREP(FLEXCAN_FDCBT_FPRESDIV_MASK, dbt->brp - 1) |

thanks,
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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
  2020-09-15 22:16               ` Marc Kleine-Budde
@ 2020-09-16  2:26                 ` Joakim Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Joakim Zhang @ 2020-09-16  2:26 UTC (permalink / raw)
  To: Marc Kleine-Budde, Michael Walle; +Cc: linux-can, dl-linux-imx, netdev


> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2020年9月16日 6:16
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Michael Walle
> <michael@walle.cc>
> Cc: linux-can@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature
> 
> On 6/30/20 4:25 AM, Joakim Zhang wrote:
> > I have also noticed this difference, although this could not break
> > function, but IMO, using priv->can.ctrlmode should be better.
> >
[...]

> > 2) Clean timing register.
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -1167,6 +1167,14 @@ static void flexcan_set_bittiming_cbt(const
> struct net_device *dev)
> >         struct flexcan_regs __iomem *regs = priv->regs;
> >         u32 reg_cbt, reg_fdctrl;
> >
> > +       reg_cbt = priv->read(&regs->cbt);
> > +       reg_cbt &= ~(FLEXCAN_CBT_BTF |
> > +               FIELD_PREP(FLEXCAN_CBT_EPRESDIV_MASK, 0x3ff) |
> > +               FIELD_PREP(FLEXCAN_CBT_ERJW_MASK, 0x1f) |
> > +               FIELD_PREP(FLEXCAN_CBT_EPROPSEG_MASK, 0x3f) |
> > +               FIELD_PREP(FLEXCAN_CBT_EPSEG1_MASK, 0x1f) |
> > +               FIELD_PREP(FLEXCAN_CBT_EPSEG2_MASK, 0x1f));
> > +
> 
> Why is this needed? The "reg_cbt &=" sets reg_cbt basically to 0, as the fields
> and the BTF occupy all 32bit.
> 
> The only thing that's left over is the read()....

Yes, need not, I have not noticed it has occupy the whole 32bit.

There is a small improve patch to balance the usage_count if register flexcandev failed. Could you pick up it by the way this time?
https://www.spinics.net/lists/linux-can/msg03052.html


Best Regards,
Joakim Zhang

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-09-16  2:26 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-16  9:31 [PATCH Resend linux-can-next/testing] can: flexcan: add correctable errors correction when HW supports ECC Joakim Zhang
2020-04-16  9:31 ` [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature Joakim Zhang
2020-04-16  9:41   ` Joakim Zhang
2020-06-02 10:15     ` Michael Walle
2020-06-25 12:37       ` Michael Walle
2020-06-25 12:56         ` Marc Kleine-Budde
2020-06-29  5:37           ` Joakim Zhang
2020-06-29 16:23           ` Michael Walle
2020-06-30  2:25             ` Joakim Zhang
2020-09-15 22:16               ` Marc Kleine-Budde
2020-09-16  2:26                 ` Joakim Zhang
2020-09-15 22:15             ` Marc Kleine-Budde
2020-07-23 21:09           ` Michael Walle
2020-09-14 15:16             ` Michael Walle

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