From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dong Aisheng Subject: Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features Date: Wed, 5 Nov 2014 20:47:09 +0800 Message-ID: <20141105124708.GE4007@shlinux1.ap.freescale.net> References: <1415174326-6623-1-git-send-email-b29396@freescale.com> <5459F808.3020903@hartkopp.net> <20141105112639.GB4007@shlinux1.ap.freescale.net> <545A21AD.5040608@hartkopp.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , To: Oliver Hartkopp Return-path: Content-Disposition: inline In-Reply-To: <545A21AD.5040608@hartkopp.net> Sender: linux-can-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Wed, Nov 05, 2014 at 02:10:05PM +0100, Oliver Hartkopp wrote: > On 05.11.2014 12:26, Dong Aisheng wrote: > >On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: > >>On 05.11.2014 08:58, Dong Aisheng wrote: >=20 >=20 > >>Unfortunately No. Here it becomes complicated due to the fact that > >>the revision 3.0.x does not support per-frame switching for FD/BRS > >>... > > > >I'm not sure i got your point. > > From m_can spec, it allows switch CAN mode by setting CMR bit. > > > >Bits 11:10 CMR[1:0]: CAN Mode Request > >A change of the CAN operation mode is requested by writing to this b= it field. After change to the > >requested operation mode the bit field is reset to =E2=80=9C00=E2=80= =9D and the status flags FDBS and FDO are set > >accordingly. In case the requested CAN operation mode is not enabled= , the value written to CMR is > >retained until it is overwritten by the next mode change request. In= case CME =3D =E2=80=9C01=E2=80=9D/=E2=80=9D10=E2=80=9D/=E2=80=9D11=E2=80= =9D a > >change to CAN operation according to ISO 11898-1 is always possible.= Default is CAN operation > >according to ISO11898-1. > >00 unchanged > >01 Request CAN FD operation > >10 Request CAN FD operation with bit rate switching > >11 Request CAN operation according ISO11898-1 > > > >So what's the difference between this function and the per-frame swi= tching > >you mentioned? > > > >> > >>When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* > >>tells us, that the controller is _capable_ to send either CAN or CA= N > >>FD frames. > >> > >>It does not configure the controller into one of these specific set= tings! > >> > >>Additionally: AFAIK when writing to the CCCR you have to make sure > >>that there's currently no ongoing transfer. > >> > > > >I don't know about it before. > >By searching m_can user manual v302 again, i still did not find this > >limitation. Can you point me if you know where it is? > > > >BTW, since we only use one Tx Buffer for transmission currently, so = we > >should not meet that case that CAN mode is switched during transfer. > >So the issue you concern may not happen. >=20 > Yes. You are right. Having a FIFO with a size of 1 will help here :-) >=20 > > > >Additionally it looks to me like m_can does allow set CMR bit at run= time > >since the mode change will take affect when controller becomes idle. > >See below: > > > >"A mode change requested by writing to CCCR.CMR will be executed nex= t > >time the CAN protocol controller FSM reaches idle phase between CAN = frames. > >Upon this event CCCR.CMR is reset to =E2=80=9C00=E2=80=9D and the st= atus flags CCCR.FDBS > >and CCCR.FDO are set accordingly. In case the requested > >CAN operation mode is not enabled, the value written to CCCR.CMR is > >retained until it is overwritten by the next mode change request. > >Default is CAN operation according to ISO11898-1." >=20 > Ah. I assumed we have to take care to set these bits in the idle stat= e. >=20 > So when thinking about the one and only tx buffer your current > approach should work indeed. Sorry for my confusion. >=20 > > > >>I would suggest the following approach to make the revision 3.0.x > >>act like a correct CAN FD capable controller: > >> > >>- create a helper function to switch FD and BRS while taking care o= f > >>ongoing transmissions > >> > >>- create a variable that knows the current tx_mode for FD and BRS > >> > >>When we need to send a CAN frame which doesn't fit the settings in > >>the tx_mode we need to switch the CCCR and update the tx_mode > >>variable. > >> > >>This at least reduces the needed CCCR operations. > >> > > > >Yes, i can do like that. > >But what i see by doing that is only reduces the needed CCCR operati= ons? > >Any other benefits i missed? >=20 > No. I just thought about a function that also takes care of the idle > phase to switch the bits. But now you made it clear that this is not > needed - so you can stay on your current implementation: Setting the > CCCR each time before sending the frame. >=20 Okay > With the 3.1.x or 3.2.x this code will become obsolete then as the > EDL and BRS seeting will get extra bits in the Tx Buffer. > But that's a future point. >=20 Got it. Will update it in the future when the new IP doc is available. > >And the test for current code showed it seemed work well on the Mode > >switch among std frame/fd frame/brs frame. >=20 > Fine. >=20 > Btw. I would suggest to integrate patch [4/4] into [3/4]. >=20 Yes, will do it. Thanks Regards Dong Aisheng > Best regards, > Oliver >=20