Date:   Fri, 14 Feb 2020 10:01:52 +0000
From:   Pankaj Bansal <pankaj.bansal@....com>
To:     Michael Walle <michael@...le.cc>,
        Joakim Zhang <qiangqing.zhang@....com>
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>,
        Shawn Guo <shawnguo@...nel.org>
Subject: RE: [PATCH 0/8] can: flexcan: add CAN FD support for NXP Flexcan

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@...le.cc>
> Sent: Friday, February 14, 2020 3:03 PM
> 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?
> 

This is done to ensure that device tree bindings are stable.
See this talk for more information for stable device tree ABI:

https://elinux.org/images/0/0e/OSELAS.Presentation-ELCE2017-DT.pdf
https://www.youtube.com/watch?v=6iguKSJJfxo

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