| Message ID | 20200416093126.15242-2-qiangqing.zhang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux-can-next/flexcan] can: flexcan: fix TDC feature | expand |
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
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
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 |
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, ®s->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(®s->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(®s->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(®s->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
> -----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(®s->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
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, ®s->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(®s->fdctrl) & ~FLEXCAN_FDCTRL_FDRATE; - reg_fdctrl &= ~FLEXCAN_FDCTRL_TDCEN; reg_fdctrl &= ~(FLEXCAN_FDCTRL_MBDSR1(0x3) | FLEXCAN_FDCTRL_MBDSR0(0x3)); reg_mcr = priv->read(®s->mcr) & ~FLEXCAN_MCR_FDEN; reg_ctrl2 = priv->read(®s->ctrl2) & ~FLEXCAN_CTRL2_ISOCANFDEN;
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(-)