Message-ID: <DB8PR04MB6795751DBB178E1EDF74136FE66F0@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Tue, 30 Jun 2020 02:25:56 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Michael Walle <michael@...le.cc>,
        Marc Kleine-Budde <mkl@...gutronix.de>
CC:     "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH linux-can-next/flexcan] can: flexcan: fix TDC feature


> -----Original Message-----
> From: Michael Walle <michael@...le.cc>
> Sent: 2020年6月30日 0:23
> To: Marc Kleine-Budde <mkl@...gutronix.de>
> Cc: Joakim Zhang <qiangqing.zhang@....com>; linux-can@...r.kernel.org;
> dl-linux-imx <linux-imx@....com>; netdev@...r.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

Powered by blists - more mailing lists