Date:   Fri, 14 Feb 2020 09:56:18 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Michael Walle <michael@...le.cc>
CC:     Marc Kleine-Budde <mkl@...gutronix.de>,
        "wg@...ndegger.com" <wg@...ndegger.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        Pankaj Bansal <pankaj.bansal@....com>,
        Shawn Guo <shawnguo@...nel.org>
Subject: RE: [PATCH 0/8] can: flexcan: add CAN FD support for NXP Flexcan


> -----Original Message-----
> From: Michael Walle <michael@...le.cc>
> Sent: 2020年2月14日 17:33
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: Marc Kleine-Budde <mkl@...gutronix.de>; wg@...ndegger.com;
> netdev@...r.kernel.org; linux-can@...r.kernel.org; Pankaj Bansal
> <pankaj.bansal@....com>; Shawn Guo <shawnguo@...nel.org>
> Subject: Re: [PATCH 0/8] can: flexcan: add CAN FD support for NXP Flexcan
> 
> Am 2020-02-14 10:18, schrieb Joakim Zhang:
> > Best Regards,
> > Joakim Zhang
> >
> >> -----Original Message-----
> >> From: Michael Walle <michael@...le.cc>
> >> Sent: 2020年2月14日 16:43
> >> To: Joakim Zhang <qiangqing.zhang@....com>
> >> Cc: Marc Kleine-Budde <mkl@...gutronix.de>; wg@...ndegger.com;
> >> netdev@...r.kernel.org; linux-can@...r.kernel.org; Pankaj Bansal
> >> <pankaj.bansal@....com>
> >> Subject: Re: [PATCH 0/8] can: flexcan: add CAN FD support for NXP
> >> Flexcan
> >>
> >> Hi Joakim,
> >>
> >> Am 2020-02-14 02:55, schrieb Joakim Zhang:
> >> > Hi Michal,
> >> >
> >> >> -----Original Message-----
> >> >> From: Michael Walle <michael@...le.cc>
> >> >> Sent: 2020年2月14日 3:20
> >> >> To: Marc Kleine-Budde <mkl@...gutronix.de>
> >> >> Cc: Joakim Zhang <qiangqing.zhang@....com>; wg@...ndegger.com;
> >> >> netdev@...r.kernel.org; linux-can@...r.kernel.org; Pankaj Bansal
> >> >> <pankaj.bansal@....com>; Michael Walle <michael@...le.cc>
> >> >> Subject: Re: [PATCH 0/8] can: flexcan: add CAN FD support for NXP
> >> >> Flexcan
> >> >>
> >> >> Hi,
> >> >>
> >> >> >>> Are you prepared to add back these patches as they are
> >> >> >>> necessary for Flexcan CAN FD? And this Flexcan CAN FD patch
> >> >> >>> set is based on these patches.
> >> >> >>
> >> >> >> Yes, these patches will be added back.
> >> >> >
> >> >> >I've cleaned up the first patch a bit, and pushed everything to
> >> >> >the testing branch. Can you give it a test.
> >> >>
> >> >> What happend to that branch? FWIW I've just tried the patches on a
> >> >> custom board with a LS1028A SoC. Both CAN and CAN-FD are working.
> >> >> I've tested against a Peaktech USB CAN adapter. I'd love to see
> >> >> these patches upstream, because our board also offers CAN and
> >> >> basic support for it just made it upstream [1].
> >> > The FlexCAN CAN FD related patches have stayed in
> >> > linux-can-next/flexcan branch for a long time, I still don't know
> >> > why Marc doesn't merge them into Linux mainline.
> >> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> >> >
> >>
> kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmkl%2Flinux-can-next.
> >> g
> >> >
> >>
> it%2Ftree%2F%3Fh%3Dflexcan&amp;data=02%7C01%7Cqiangqing.zhang%40n
> >> xp.co
> >> >
> >>
> m%7C94dca4472a584410b3b908d7b129db27%7C686ea1d3bc2b4c6fa92cd99c
> >> 5c30163
> >> >
> >>
> 5%7C0%7C0%7C637172665642079192&amp;sdata=77tG6VuQCi%2FZXBKb23
> >> 8%2FdNSV3
> >> > NUIFrM5Y0e9yj0J3os%3D&amp;reserved=0
> >> > Also must hope that this patch set can be upstreamed soon. :-)
> >>
> >> I've took them from this branch and applied them to the latest linux
> >> master.
> >>
> >> Thus,
> >>
> >> Tested-by: Michael Walle <michael@...le.cc>
> >>
> >>
> >> >> If these patches are upstream, only the device tree nodes seems to
> >> >> be missing.
> >> >> I don't know what has happened to [2]. But the patch doesn't seem
> >> >> to be necessary.
> >> > Yes, this patch is unnecessary. I have NACKed this patch for that,
> >> > according to FlexCAN Integrated Guide, CTRL1[CLKSRC]=0 select
> >> > oscillator clock and CTRL1[CLKSRC]=1 select peripheral clock.
> >> > But it is actually decided by SoC integration, for i.MX, the design
> >> > is different.
> >>
> >> ok thanks for clarifying.
> >>
> >> > I have not upstream i.MX FlexCAN device tree nodes, since it's
> >> > dependency have not upstreamed yet.
> >> >
> >> >> Pankaj already send a patch to add the device node to the LS1028A [3].
> >> >> Thats basically the same I've used, only that mine didn't had the
> >> >> "fsl,ls1028ar1-flexcan" compatiblity string, but only the
> >> >> "lx2160ar1-flexcan"
> >> >> which is the correct way to use it, right?
> >> > You can see below table from FlexCAN driver, "fsl,lx2160ar1-flexcan"
> >> > supports CAN FD, you can use this compatible string.
> >>
> >> correct. I've already a patch that does exactly this ;) Who would
> >> take the patch for adding the LS1028A can device tree nodes to
> >> ls1028a.dtsi? You or Shawn Guo?
> > Sorry, I missed the link[3], we usually write it this way:
> > 			compatible = "fsl,ls1028ar1-flexcan","fsl,lx2160ar1-flexcan";
> > Please send patch to Shawn Guo, he will review the device tree.
> 
> As far as I know, there should be no undocumented binding. Eg. the
> ls1028ar1-flexcan is neither in the source nor in the device tree binding
> documentation, thus wouldn't be accepted.
> 
> Thus either there should be another ls1028ar1-flexcan in the flexcan_of_match
> table and the node should only contain that string or the node should only
> contain fsl,lx2160ar1-flexcan. Is there any advantage of the first option?
From the FlexCAN binding(Documentation/devicetree/bindings/net/can/fsl-flexcan.txt)
- compatible : Should be "fsl,<processor>-flexcan"

  An implementation should also claim any of the following compatibles
  that it is fully backwards compatible with:

  - fsl,p1010-flexcan

You also can check imx6ul.dtsi imx7s.dtsi etc.

Sorry :-(, I also don't know the advantage, it's just that we're used to writing it that way. You can check nodes of other devices.
It's unnecessary to add compatible string for each SoCs since they may share the same IP. And dts had batter have a SoC specific compatible string. It's just my understanding.

Joakim

> -michael
> 
> 
> >
> >> > static const struct of_device_id flexcan_of_match[] = {
> >> > 	{ .compatible = "fsl,imx8qm-flexcan", .data =
> >> > &fsl_imx8qm_devtype_data, },
> >> > 	{ .compatible = "fsl,imx6q-flexcan", .data =
> >> > &fsl_imx6q_devtype_data, },
> >> > 	{ .compatible = "fsl,imx28-flexcan", .data =
> >> > &fsl_imx28_devtype_data, },
> >> > 	{ .compatible = "fsl,imx53-flexcan", .data =
> >> > &fsl_imx25_devtype_data, },
> >> > 	{ .compatible = "fsl,imx35-flexcan", .data =
> >> > &fsl_imx25_devtype_data, },
> >> > 	{ .compatible = "fsl,imx25-flexcan", .data =
> >> > &fsl_imx25_devtype_data, },
> >> > 	{ .compatible = "fsl,p1010-flexcan", .data =
> >> > &fsl_p1010_devtype_data, },
> >> > 	{ .compatible = "fsl,vf610-flexcan", .data =
> >> > &fsl_vf610_devtype_data, },
> >> > 	{ .compatible = "fsl,ls1021ar2-flexcan", .data =
> >> > &fsl_ls1021a_r2_devtype_data, },
> >> > 	{ .compatible = "fsl,lx2160ar1-flexcan", .data =
> >> > &fsl_lx2160a_r1_devtype_data, },
> >> > 	{ /* sentinel */ },
> >> > };
> >> >
> >>
> >> -michael

Powered by blists - more mailing lists