Am 2020-02-14 11:01, schrieb Pankaj Bansal:
Hi Michael,-----Original Message----- From: Michael Walle <michael@xxxxxxxx> Sent: Friday, February 14, 2020 3:03 PM To: Joakim Zhang <qiangqing.zhang@xxxxxxx> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>; wg@xxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx; Pankaj Bansal <pankaj.bansal@xxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>Subject: Re: [PATCH 0/8] can: flexcan: add CAN FD support for NXP FlexcanAm 2020-02-14 10:18, schrieb Joakim Zhang: > Best Regards, > Joakim Zhang > >> -----Original Message----- >> From: Michael Walle <michael@xxxxxxxx> >> Sent: 2020年2月14日 16:43 >> To: Joakim Zhang <qiangqing.zhang@xxxxxxx> >> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>; wg@xxxxxxxxxxxxxx; >> netdev@xxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx; Pankaj Bansal >> <pankaj.bansal@xxxxxxx> >> 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@xxxxxxxx> >> >> Sent: 2020年2月14日 3:20 >> >> To: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> >> >> Cc: Joakim Zhang <qiangqing.zhang@xxxxxxx>; wg@xxxxxxxxxxxxxx; >> >> netdev@xxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx; Pankaj Bansal >> >> <pankaj.bansal@xxxxxxx>; Michael Walle <michael@xxxxxxxx> >> >> 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&data=02%7C01%7Cqiangqing.zhang%40n >> xp.co >> > >> m%7C94dca4472a584410b3b908d7b129db27%7C686ea1d3bc2b4c6fa92cd99c >> 5c30163 >> > >> 5%7C0%7C0%7C637172665642079192&sdata=77tG6VuQCi%2FZXBKb23 >> 8%2FdNSV3 >> > NUIFrM5Y0e9yj0J3os%3D&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@xxxxxxxx> >> >> >> >> 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:
Thanks, yes that makes sense, actually I'd have done it the same way. I was just wondering about the "it has to be documented"; as Joakim pointed out, thats easy here, because there is the template here, eg. "fsl,<processor>-flexcan".
-michael
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
