linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: b29396@freescale.com (Dong Aisheng)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 6/7] can: m_can: update to support CAN FD features
Date: Tue, 4 Nov 2014 15:12:08 +0800	[thread overview]
Message-ID: <20141104071146.GA17367@shlinux1.ap.freescale.net> (raw)
In-Reply-To: <5452A071.2000307@hartkopp.net>

On Thu, Oct 30, 2014 at 09:32:49PM +0100, Oliver Hartkopp wrote:
> 
> On 10/30/2014 03:42 AM, Dong Aisheng wrote:
> > On Wed, Oct 29, 2014 at 08:21:28PM +0100, Oliver Hartkopp wrote:
> 
> >> So first I would suggest to check the core release register (CREL) to be
> >> version 3.0.x and quit the driver initialization if it doesn't fit this
> >> version. I would suggest to provide a separate patch for that check. What
> >> about printing the core release version into the kernel log at driver
> >> initialization time?
> >>
> > 
> > One question is that if v3.1.0 and v3.2.0 will be backward compatible with
> > v3.0.1, if yes, how about let the driver still work for them instead of
> > simply quit?
> 
> There are several important differences between 3.0.x and 3.1.x.
> E.g. the CCCR, BTP, PSR and others are changed and a register for the
> transmitter delay compensation is added.
> 
> I assume from 3.1.x to 3.2.x the controller registers will only change in
> small details as the main update will be on the wire and not in the functionality.
> 
> > And then we can add new features according new released IP version.
> 
> Yes. We probably can wait for 3.[12].x to become available before adding the
> special code that behaves according the core release register content.
> 

Okay

> >>> @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>>  		if (rxfs & RXFS_RFL)
> >>>  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>>  
> >>> -		skb = alloc_can_skb(dev, &frame);
> >>> +		skb = alloc_canfd_skb(dev, &frame);
> >>
> >> You are *always* allocating CAN FD frames now?
> >>
> > 
> > Yes, currently it is.
> > The test seemed ok using CAN FD frames even receive normal frame.
> 
> When you put CAN frame content into a CAN FD skb it becomes a CAN FD frame -
> which is wrong.
> 
> CAN 2.0 frame (EDL is clear) -> alloc_can_skb()
> CAN FD frame (EDL is set) -> alloc_canfd_skb()
> 
> You can have a CAN FD frame with a DLC of 8, which does *not* mean that you
> have a CAN 2.0 frame.
> 
> > The issue i know is that candump seemed can not recognize remote frame reported
> > by the driver.
> 
> Do you use the latest candump from
> 

Yes, i'm using latest candump.

> 	https://gitorious.org/linux-can/can-utils/
> ??
> 
> The latest candump switches the CAN_RAW socket into the mode to accept both
> CAN *and* CAN FD frames and displays the frames correctly.
> 
> > Not sure if it's caused by canfd_frame used.
> 
> Yes. CAN FD frames do not have a RTR bit.
> 

You're right.
It's indeed caused by using the CAN FD frames to receive RTR frame.
After switch to normal frame, candump showed it well.

> > Will test and check.
> > 
> >> Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
> >> FD frame.
> >>
> >> Of course you can use the struct canfd_frame in m_can_read_fifo() as the
> >> layout of the struct can_frame is identical when filled with 'normal' CAN
> >> frame content.
> >>
> >> But you need to distinguish whether it is a CAN or CAN FD frame when
> >> allocating the skb based on the RX_BUF_EDL value.
> >>
> > 
> > Yes, i think it's good to do that.
> > One obvious benefit is it saves memory at least.
> 
> The main point is that CAN frames and CAN FD frames are separated by this
> (MTU) length information. It's not about saving memory.
> A CAN FD frame with DLC 8 still has 64 data bytes inside it's data structure.
> 

For normal can frame, the CAN_MAX_DLEN is 8 while CANFD_MAX_DLEN is 64.
So i meant using struct canfd_frame to receive normal frame is a bit waste memory.
And besides, actually it's wrong as you already indicated.
I will send out the updated patch with this changed in v2 soon.
Thanks for pointing out this.

Regards
Dong Aisheng

> Regards,
> Oliver

  reply	other threads:[~2014-11-04  7:12 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-29 10:45 [PATCH 1/7] can: m_can: fix possible sleep in napi poll Dong Aisheng
2014-10-29 10:45 ` [PATCH 2/7] can: m_can: fix the incorrect error messages Dong Aisheng
2014-11-03 16:25   ` Marc Kleine-Budde
2014-10-29 10:45 ` [PATCH 3/7] can: m_can: add .ndo_change_mtu function Dong Aisheng
2014-11-03 16:49   ` Marc Kleine-Budde
2014-10-29 10:45 ` [PATCH 4/7] can: m_can: add a bit delay after setting CCCR_INIT bit Dong Aisheng
2014-11-03 16:52   ` Marc Kleine-Budde
2014-10-29 10:45 ` [PATCH 5/7] can: clear ctrlmode when close candev Dong Aisheng
2014-11-03 16:28   ` Marc Kleine-Budde
2014-11-03 18:25     ` Oliver Hartkopp
2014-11-03 20:47   ` Marc Kleine-Budde
2014-11-04  8:27     ` Dong Aisheng
2014-10-29 10:45 ` [PATCH 6/7] can: m_can: update to support CAN FD features Dong Aisheng
2014-10-29 19:21   ` Oliver Hartkopp
2014-10-30  2:42     ` Dong Aisheng
2014-10-30 20:32       ` Oliver Hartkopp
2014-11-04  7:12         ` Dong Aisheng [this message]
2014-10-29 10:45 ` [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes Dong Aisheng
2014-11-03 16:48   ` Marc Kleine-Budde
2014-11-04  8:25     ` Dong Aisheng
2014-11-04  9:22       ` Marc Kleine-Budde
2014-11-04  9:27         ` Dong Aisheng
2014-11-04 10:33           ` Marc Kleine-Budde
2014-11-04 13:13             ` Oliver Hartkopp
2014-11-04 13:28               ` Marc Kleine-Budde
2014-11-05  2:07               ` Dong Aisheng
2014-11-05  2:03             ` Dong Aisheng
2014-11-03 16:24 ` [PATCH 1/7] can: m_can: fix possible sleep in napi poll Marc Kleine-Budde
2014-11-03 17:02 ` Marc Kleine-Budde
2014-11-03 18:37   ` Oliver Hartkopp
2014-11-03 20:49     ` Marc Kleine-Budde
2014-11-03 21:12       ` Oliver Hartkopp

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141104071146.GA17367@shlinux1.ap.freescale.net \
    --to=b29396@freescale.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).