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@xxxxxxxx>
> > Sent: 2020年2月14日 17:33
> > 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
> > Flexcan
> >
> > Am 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?
> 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
