Message-ID: <78789949f2a9dc532ec461768fbd3a60@walle.cc>
Date:   Fri, 14 Feb 2020 11:02:46 +0100
From:   Michael Walle <michael@...le.cc>
To:     Joakim Zhang <qiangqing.zhang@....com>,
        Shawn Guo <shawnguo@...nel.org>
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, Hi Shawn,


Am 2020-02-14 10:56, schrieb Joakim Zhang:
>> -----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.

Ah thanks. So Pankaj's patch [1] seems to be correct (at least according
to the description in the device tree documentation).

Shawn, whats your opinion?

-michael

[1] https://www.spinics.net/lists/linux-can/msg01577.html

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