Message-ID: <0d02f6cee0d3a680f246e8fea40f6699@walle.cc>
Date:   Mon, 17 Feb 2020 09:48:36 +0100
From:   Michael Walle <michael@...le.cc>
To:     Shawn Guo <shawnguo@...nel.org>
Cc:     Joakim Zhang <qiangqing.zhang@....com>,
        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 Shawn,

Am 2020-02-17 08:13, schrieb Shawn Guo:
> On Fri, Feb 14, 2020 at 11:02:46AM +0100, Michael Walle wrote:
>> 
>> 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?
> 
> My opinion is that all compatibles should be defined explicitly in
> bindings doc.  In above example, the possible values of <processor>
> should be given.  This must be done anyway, as we are moving to
> json-schema bindings.

But if they are listed in the document, they also have to be in the
of_device_id table, correct? Which somehow contradicts the talk Pankaj
mentioned [1,2]. Eg.

   compatible = "fsl,ls1028ar1-flexcan","fsl,lx2160ar1-flexcan";

Doesn't make any sense, because the "fsl,ls1028ar1-flexcan" is alreay
in the driver and the fallback "fsl,lx2160ar1-flexcan" isn't needed.

OTOH the talk is already 2 to 3 years old and things might have changed
since then.

-michael

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

Powered by blists - more mailing lists