netdev - Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <545A35BA.8050607@hartkopp.net>
Date:	Wed, 05 Nov 2014 15:35:38 +0100
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Dong Aisheng <b29396@...escale.com>,
	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	linux-can@...r.kernel.org, wg@...ndegger.com,
	varkabhadram@...il.com, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features



On 05.11.2014 14:46, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote:
>> On 11/05/2014 02:10 PM, 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:
>>>
>>>
>>>>> 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 bit
>>>> field. After change to the
>>>> requested operation mode the bit field is reset to “00” 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 = “01”/”10”/”11” 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
>>>> switching
>>>> 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 CAN
>>>>> FD frames.
>>>>>
>>>>> It does not configure the controller into one of these specific
>>>>> settings!
>>>>>
>>>>> 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 th
> Regards
> Dong Aisheng
>
>> Marc
>>
>> --
>> Pengutronix e.K.                  | Marc Kleine-Budde           |
>> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
>> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
>> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>>
>
>
at case that CAN mode is switched during transfer.
>>>> So the issue you concern may not happen.
>>>
>>> Yes. You are right. Having a FIFO with a size of 1 will help here :-)
>>
>> Errrr....sorry...no.
>>
>> Taking an easy route here but making it x times harder to extend the
>> driver to make use of the FIFO is not an option.
>>
>
> Hmm, this way is just following the original approach the current driver
> used. It's initial work and won't make things complicated.
>
> Extend the driver to use FIFO for TX is another story and based on
> my understanding it may be a bit complicated to do CAN FD mode switch on this
> case due to hw limitation that the revision 3.0.x does not support per-frame
> switching for FD/BRS as Oliver pointed out.
> (e.g. how to switch FD MODE for each frame on Tx FIFO?)
> Probably that's why the 3.1.x version will add the FD/BRS bit controller
> in Tx Buffer to fix this issue.
>
> Anyway, that's future work and we can discuss it when adding FIFO support
> for Tx function.
>

Yes. I have to second this opinion.

I also would like to have a TX FIFO. But due to the limitations of the 3.0.x 
M_CAN I would suggest to prefer a 'correct" CAN FD driver implementation in 
favor of having a TX FIFO which is unusable for mixed CAN frame types.

Let's try the FIFO stuff with the next M_CAN revision.

It's a bit of a SJA1000 for CAN FD :-)

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html