diff mbox

[V2,1/4] can: m_can: update to support CAN FD features

Message ID 1415174326-6623-1-git-send-email-b29396@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aisheng Dong Nov. 5, 2014, 7:58 a.m. UTC
Bosch M_CAN is CAN FD capable device. This patch implements the CAN
FD features include up to 64 bytes payload and bitrate switch function.
1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
   up to 64 bytes payload. It's backward compatible with old 8 bytes
   normal CAN frame.
2) Allocate can frame or canfd frame based on EDL bit
3) Bitrate Switch function is disabled by default and will be enabled
   according to CANFD_BRS bit in cf->flags.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
Changes Since V1:
 * Allocate can frame or canfd frame based on EDL bit
 * Only check and set RTR bit for normal frame (no EDL bit set)
---
 drivers/net/can/m_can/m_can.c | 172 +++++++++++++++++++++++++++++++-----------
 1 file changed, 129 insertions(+), 43 deletions(-)

Comments

Oliver Hartkopp Nov. 5, 2014, 10:12 a.m. UTC | #1
On 05.11.2014 08:58, Dong Aisheng wrote:

> @@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
>   	m_can_write(priv, M_CAN_ILE, 0x0);
>   }
>
> -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> -			    u32 rxfs)
> +static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
>   {
> +	struct net_device_stats *stats = &dev->stats;
>   	struct m_can_priv *priv = netdev_priv(dev);
> -	u32 id, fgi;
> +	struct canfd_frame *cf;
> +	struct sk_buff *skb;
> +	u32 id, fgi, dlc;
> +	int i;
>
>   	/* calculate the fifo get index for where to read data */
>   	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> +	dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> +	if (dlc & RX_BUF_EDL)
> +		skb = alloc_canfd_skb(dev, &cf);
> +	else
> +		skb = alloc_can_skb(dev, (struct can_frame **)&cf);

Yes. That's the way it should look like ;-)

> +	if (!skb) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
>   	id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
>   	if (id & RX_BUF_XTD)
>   		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
>   	else
>   		cf->can_id = (id >> 18) & CAN_SFF_MASK;
>
> -	if (id & RX_BUF_RTR) {
> +	if (id & RX_BUF_ESI) {
> +		cf->flags |= CANFD_ESI;
> +		netdev_dbg(dev, "ESI Error\n");
> +	}
> +
> +	if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
>   		cf->can_id |= CAN_RTR_FLAG;
>   	} else {
>   		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> -		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> -		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> -							 M_CAN_FIFO_DATA(0));
> -		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> -							 M_CAN_FIFO_DATA(1));
> +		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));

if (dlc & RX_BUF_EDL)
	cf->len = can_dlc2len((id >> 16) & 0x0F);
else
	cf->len = get_can_dlc((id >> 16) & 0x0F);

(..)

> @@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev)
>   		    RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
>
>   	cccr = m_can_read(priv, M_CAN_CCCR);
> -	cccr &= ~(CCCR_TEST | CCCR_MON);
> +	cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
> +		(CCCR_CME_MASK << CCCR_CME_SHIFT));
>   	test = m_can_read(priv, M_CAN_TEST);
>   	test &= ~TEST_LBCK;
>
> @@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev)
>   		test |= TEST_LBCK;
>   	}
>
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> +		cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> +
>   	m_can_write(priv, M_CAN_CCCR, cccr);
>   	m_can_write(priv, M_CAN_TEST, test);
>

(..)

>
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +		cccr = m_can_read(priv, M_CAN_CCCR);
> +		cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> +		if (cf->flags & CANFD_BRS)
> +			cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
> +		else
> +			cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
> +		m_can_write(priv, M_CAN_CCCR, cccr);
> +	}

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 ...

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 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 of 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.

And it also addresses the intention of your patch
[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode

Regards,
Oliver
Aisheng Dong Nov. 5, 2014, 11:26 a.m. UTC | #2
On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 08:58, Dong Aisheng wrote:
> 
> >@@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
> >  	m_can_write(priv, M_CAN_ILE, 0x0);
> >  }
> >
> >-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> >-			    u32 rxfs)
> >+static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
> >  {
> >+	struct net_device_stats *stats = &dev->stats;
> >  	struct m_can_priv *priv = netdev_priv(dev);
> >-	u32 id, fgi;
> >+	struct canfd_frame *cf;
> >+	struct sk_buff *skb;
> >+	u32 id, fgi, dlc;
> >+	int i;
> >
> >  	/* calculate the fifo get index for where to read data */
> >  	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >+	dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> >+	if (dlc & RX_BUF_EDL)
> >+		skb = alloc_canfd_skb(dev, &cf);
> >+	else
> >+		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
> 
> Yes. That's the way it should look like ;-)
> 
> >+	if (!skb) {
> >+		stats->rx_dropped++;
> >+		return;
> >+	}
> >+
> >  	id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
> >  	if (id & RX_BUF_XTD)
> >  		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> >  	else
> >  		cf->can_id = (id >> 18) & CAN_SFF_MASK;
> >
> >-	if (id & RX_BUF_RTR) {
> >+	if (id & RX_BUF_ESI) {
> >+		cf->flags |= CANFD_ESI;
> >+		netdev_dbg(dev, "ESI Error\n");
> >+	}
> >+
> >+	if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
> >  		cf->can_id |= CAN_RTR_FLAG;
> >  	} else {
> >  		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> >-		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> >-		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> >-							 M_CAN_FIFO_DATA(0));
> >-		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> >-							 M_CAN_FIFO_DATA(1));
> >+		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
> 
> if (dlc & RX_BUF_EDL)
> 	cf->len = can_dlc2len((id >> 16) & 0x0F);
> else
> 	cf->len = get_can_dlc((id >> 16) & 0x0F);
> 
> (..)
> 

Correct.
Will change it.

> >@@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev)
> >  		    RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
> >
> >  	cccr = m_can_read(priv, M_CAN_CCCR);
> >-	cccr &= ~(CCCR_TEST | CCCR_MON);
> >+	cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
> >+		(CCCR_CME_MASK << CCCR_CME_SHIFT));
> >  	test = m_can_read(priv, M_CAN_TEST);
> >  	test &= ~TEST_LBCK;
> >
> >@@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev)
> >  		test |= TEST_LBCK;
> >  	}
> >
> >+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> >+		cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> >+
> >  	m_can_write(priv, M_CAN_CCCR, cccr);
> >  	m_can_write(priv, M_CAN_TEST, test);
> >
> 
> (..)
> 
> >
> >+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> >+		cccr = m_can_read(priv, M_CAN_CCCR);
> >+		cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> >+		if (cf->flags & CANFD_BRS)
> >+			cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
> >+		else
> >+			cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
> >+		m_can_write(priv, M_CAN_CCCR, cccr);
> >+	}
> 
> 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 that case that CAN mode is switched during transfer.
So the issue you concern may not happen.

Additionally it looks to me like m_can does allow set CMR bit at runtime
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 next
time the CAN protocol controller FSM reaches idle phase between CAN frames.
Upon this event CCCR.CMR is reset to “00” and the status 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."

> 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 of
> 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 operations?
Any other benefits i missed?

And the test for current code showed it seemed work well on the Mode
switch among std frame/fd frame/brs frame.

Regards
Dong Aisheng

> And it also addresses the intention of your patch
> [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
> 
> Regards,
> Oliver
>
Varka Bhadram Nov. 5, 2014, 11:35 a.m. UTC | #3
Hi Dong Aisheng,

On 11/05/2014 01:28 PM, Dong Aisheng wrote:

> Bosch M_CAN is CAN FD capable device. This patch implements the CAN
> FD features include up to 64 bytes payload and bitrate switch function.
> 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
>     up to 64 bytes payload. It's backward compatible with old 8 bytes
>     normal CAN frame.
> 2) Allocate can frame or canfd frame based on EDL bit
> 3) Bitrate Switch function is disabled by default and will be enabled
>     according to CANFD_BRS bit in cf->flags.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

In these four patches two of them are V1 and other two are V2.

Is there any reason..?

How will you be able to generate like this by git format-patch..?
Aisheng Dong Nov. 5, 2014, 11:36 a.m. UTC | #4
On Wed, Nov 05, 2014 at 05:05:11PM +0530, Varka Bhadram wrote:
> Hi Dong Aisheng,
> 
> On 11/05/2014 01:28 PM, Dong Aisheng wrote:
> 
> >Bosch M_CAN is CAN FD capable device. This patch implements the CAN
> >FD features include up to 64 bytes payload and bitrate switch function.
> >1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
> >    up to 64 bytes payload. It's backward compatible with old 8 bytes
> >    normal CAN frame.
> >2) Allocate can frame or canfd frame based on EDL bit
> >3) Bitrate Switch function is disabled by default and will be enabled
> >    according to CANFD_BRS bit in cf->flags.
> >
> >Signed-off-by: Dong Aisheng <b29396@freescale.com>
> 
> In these four patches two of them are V1 and other two are V2.
> 
> Is there any reason..?
> 

Sorry for confusion.
Because some patches are already picked by Macr on the first round.
See below:
http://www.spinics.net/lists/netdev/msg302133.html

> How will you be able to generate like this by git format-patch..?
> 

Generated as v2 and i manually changed the later two to v1 since they
are new.
Not sure if it's suitable to do like that. :-)

Regards
Dong Aisheng

> -- 
> Thanks and Regards,
> Varka Bhadram.
>
Aisheng Dong Nov. 5, 2014, 12:47 p.m. UTC | #5
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:
> 
> 
> >>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 that 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 :-)
> 
> >
> >Additionally it looks to me like m_can does allow set CMR bit at runtime
> >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 next
> >time the CAN protocol controller FSM reaches idle phase between CAN frames.
> >Upon this event CCCR.CMR is reset to “00” and the status 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."
> 
> Ah. I assumed we have to take care to set these bits in the idle state.
> 
> So when thinking about the one and only tx buffer your current
> approach should work indeed. Sorry for my confusion.
> 
> >
> >>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 of
> >>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 operations?
> >Any other benefits i missed?
> 
> 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.
> 

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.
> 

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.
> 
> Fine.
> 
> Btw. I would suggest to integrate patch [4/4] into [3/4].
> 

Yes, will do it.
Thanks

Regards
Dong Aisheng

> Best regards,
> Oliver
>
Aisheng Dong Nov. 5, 2014, 12:47 p.m. UTC | #6
On Wed, Nov 05, 2014 at 02:15:27PM +0100, Oliver Hartkopp wrote:
> Typo
> 
> On 05.11.2014 14:10, Oliver Hartkopp wrote:
> 
> >Btw. I would suggest to integrate patch [4/4] into [3/4].
> 
> into [1/4] of course

Understand! :-)

Regards
Dong Aisheng
Oliver Hartkopp Nov. 5, 2014, 1:10 p.m. UTC | #7
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 that 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 :-)

>
> Additionally it looks to me like m_can does allow set CMR bit at runtime
> 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 next
> time the CAN protocol controller FSM reaches idle phase between CAN frames.
> Upon this event CCCR.CMR is reset to “00” and the status 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."

Ah. I assumed we have to take care to set these bits in the idle state.

So when thinking about the one and only tx buffer your current approach should 
work indeed. Sorry for my confusion.

>
>> 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 of
>> 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 operations?
> Any other benefits i missed?

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.

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.

> And the test for current code showed it seemed work well on the Mode
> switch among std frame/fd frame/brs frame.

Fine.

Btw. I would suggest to integrate patch [4/4] into [3/4].

Best regards,
Oliver
Oliver Hartkopp Nov. 5, 2014, 1:15 p.m. UTC | #8
Typo

On 05.11.2014 14:10, Oliver Hartkopp wrote:

> Btw. I would suggest to integrate patch [4/4] into [3/4].

into [1/4] of course
Marc Kleine-Budde Nov. 5, 2014, 1:19 p.m. UTC | #9
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 that 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.

Marc
Aisheng Dong Nov. 5, 2014, 1:46 p.m. UTC | #10
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 that 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.

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   |
>
Oliver Hartkopp Nov. 5, 2014, 2:35 p.m. UTC | #11
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
diff mbox

Patch

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6160b9c..664fe30 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -105,14 +105,36 @@  enum m_can_mram_cfg {
 	MRAM_CFG_NUM,
 };
 
+/* Fast Bit Timing & Prescaler Register (FBTP) */
+#define FBTR_FBRP_MASK		0x1f
+#define FBTR_FBRP_SHIFT		16
+#define FBTR_FTSEG1_SHIFT	8
+#define FBTR_FTSEG1_MASK	(0xf << FBTR_FTSEG1_SHIFT)
+#define FBTR_FTSEG2_SHIFT	4
+#define FBTR_FTSEG2_MASK	(0x7 << FBTR_FTSEG2_SHIFT)
+#define FBTR_FSJW_SHIFT		0
+#define FBTR_FSJW_MASK		0x3
+
 /* Test Register (TEST) */
 #define TEST_LBCK	BIT(4)
 
 /* CC Control Register(CCCR) */
-#define CCCR_TEST	BIT(7)
-#define CCCR_MON	BIT(5)
-#define CCCR_CCE	BIT(1)
-#define CCCR_INIT	BIT(0)
+#define CCCR_TEST		BIT(7)
+#define CCCR_CMR_MASK		0x3
+#define CCCR_CMR_SHIFT		10
+#define CCCR_CMR_CANFD		0x1
+#define CCCR_CMR_CANFD_BRS	0x2
+#define CCCR_CMR_CAN		0x3
+#define CCCR_CME_MASK		0x3
+#define CCCR_CME_SHIFT		8
+#define CCCR_CME_CAN		0
+#define CCCR_CME_CANFD		0x1
+#define CCCR_CME_CANFD_BRS	0x2
+#define CCCR_TEST		BIT(7)
+#define CCCR_MON		BIT(5)
+#define CCCR_CCE		BIT(1)
+#define CCCR_INIT		BIT(0)
+#define CCCR_CANFD		0x10
 
 /* Bit Timing & Prescaler Register (BTP) */
 #define BTR_BRP_MASK		0x3ff
@@ -204,6 +226,7 @@  enum m_can_mram_cfg {
 
 /* Rx Buffer / FIFO Element Size Configuration (RXESC) */
 #define M_CAN_RXESC_8BYTES	0x0
+#define M_CAN_RXESC_64BYTES	0x777
 
 /* Tx Buffer Configuration(TXBC) */
 #define TXBC_NDTB_OFF		16
@@ -211,6 +234,7 @@  enum m_can_mram_cfg {
 
 /* Tx Buffer Element Size Configuration(TXESC) */
 #define TXESC_TBDS_8BYTES	0x0
+#define TXESC_TBDS_64BYTES	0x7
 
 /* Tx Event FIFO Con.guration (TXEFC) */
 #define TXEFC_EFS_OFF		16
@@ -219,11 +243,11 @@  enum m_can_mram_cfg {
 /* Message RAM Configuration (in bytes) */
 #define SIDF_ELEMENT_SIZE	4
 #define XIDF_ELEMENT_SIZE	8
-#define RXF0_ELEMENT_SIZE	16
-#define RXF1_ELEMENT_SIZE	16
+#define RXF0_ELEMENT_SIZE	72
+#define RXF1_ELEMENT_SIZE	72
 #define RXB_ELEMENT_SIZE	16
 #define TXE_ELEMENT_SIZE	8
-#define TXB_ELEMENT_SIZE	16
+#define TXB_ELEMENT_SIZE	72
 
 /* Message RAM Elements */
 #define M_CAN_FIFO_ID		0x0
@@ -231,11 +255,17 @@  enum m_can_mram_cfg {
 #define M_CAN_FIFO_DATA(n)	(0x8 + ((n) << 2))
 
 /* Rx Buffer Element */
+/* R0 */
 #define RX_BUF_ESI		BIT(31)
 #define RX_BUF_XTD		BIT(30)
 #define RX_BUF_RTR		BIT(29)
+/* R1 */
+#define RX_BUF_ANMF		BIT(31)
+#define RX_BUF_EDL		BIT(21)
+#define RX_BUF_BRS		BIT(20)
 
 /* Tx Buffer Element */
+/* R0 */
 #define TX_BUF_XTD		BIT(30)
 #define TX_BUF_RTR		BIT(29)
 
@@ -327,41 +357,65 @@  static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
 	m_can_write(priv, M_CAN_ILE, 0x0);
 }
 
-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
-			    u32 rxfs)
+static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 {
+	struct net_device_stats *stats = &dev->stats;
 	struct m_can_priv *priv = netdev_priv(dev);
-	u32 id, fgi;
+	struct canfd_frame *cf;
+	struct sk_buff *skb;
+	u32 id, fgi, dlc;
+	int i;
 
 	/* calculate the fifo get index for where to read data */
 	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
+	dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
+	if (dlc & RX_BUF_EDL)
+		skb = alloc_canfd_skb(dev, &cf);
+	else
+		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
+	if (!skb) {
+		stats->rx_dropped++;
+		return;
+	}
+
 	id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
 	if (id & RX_BUF_XTD)
 		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
 		cf->can_id = (id >> 18) & CAN_SFF_MASK;
 
-	if (id & RX_BUF_RTR) {
+	if (id & RX_BUF_ESI) {
+		cf->flags |= CANFD_ESI;
+		netdev_dbg(dev, "ESI Error\n");
+	}
+
+	if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
 		cf->can_id |= CAN_RTR_FLAG;
 	} else {
 		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
-		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
-		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
-							 M_CAN_FIFO_DATA(0));
-		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
-							 M_CAN_FIFO_DATA(1));
+		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
+
+		if (id & RX_BUF_BRS)
+			cf->flags |= CANFD_BRS;
+
+		for (i = 0; i < cf->len; i += 4)
+			*(u32 *)(cf->data + i) =
+				m_can_fifo_read(priv, fgi,
+						M_CAN_FIFO_DATA(i / 4));
 	}
 
 	/* acknowledge rx fifo 0 */
 	m_can_write(priv, M_CAN_RXF0A, fgi);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->len;
+
+	netif_receive_skb(skb);
 }
 
 static int m_can_do_rx_poll(struct net_device *dev, int quota)
 {
 	struct m_can_priv *priv = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
-	struct sk_buff *skb;
-	struct can_frame *frame;
 	u32 pkts = 0;
 	u32 rxfs;
 
@@ -375,18 +429,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);
-		if (!skb) {
-			stats->rx_dropped++;
-			return pkts;
-		}
-
-		m_can_read_fifo(dev, frame, rxfs);
-
-		stats->rx_packets++;
-		stats->rx_bytes += frame->can_dlc;
-
-		netif_receive_skb(skb);
+		m_can_read_fifo(dev, rxfs);
 
 		quota--;
 		pkts++;
@@ -744,10 +787,23 @@  static const struct can_bittiming_const m_can_bittiming_const = {
 	.brp_inc = 1,
 };
 
+static const struct can_bittiming_const m_can_data_bittiming_const = {
+	.name = KBUILD_MODNAME,
+	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
+	.tseg1_max = 16,
+	.tseg2_min = 1,		/* Time segment 2 = phase_seg2 */
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 32,
+	.brp_inc = 1,
+};
+
 static int m_can_set_bittiming(struct net_device *dev)
 {
 	struct m_can_priv *priv = netdev_priv(dev);
 	const struct can_bittiming *bt = &priv->can.bittiming;
+	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	u16 brp, sjw, tseg1, tseg2;
 	u32 reg_btp;
 
@@ -758,7 +814,17 @@  static int m_can_set_bittiming(struct net_device *dev)
 	reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
 			(tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
 	m_can_write(priv, M_CAN_BTP, reg_btp);
-	netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		brp = dbt->brp - 1;
+		sjw = dbt->sjw - 1;
+		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
+		tseg2 = dbt->phase_seg2 - 1;
+		reg_btp = (brp << FBTR_FBRP_SHIFT) | (sjw << FBTR_FSJW_SHIFT) |
+				(tseg1 << FBTR_FTSEG1_SHIFT) |
+				(tseg2 << FBTR_FTSEG2_SHIFT);
+		m_can_write(priv, M_CAN_FBTP, reg_btp);
+	}
 
 	return 0;
 }
@@ -778,8 +844,8 @@  static void m_can_chip_config(struct net_device *dev)
 
 	m_can_config_endisable(priv, true);
 
-	/* RX Buffer/FIFO Element Size 8 bytes data field */
-	m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
+	/* RX Buffer/FIFO Element Size 64 bytes data field */
+	m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_64BYTES);
 
 	/* Accept Non-matching Frames Into FIFO 0 */
 	m_can_write(priv, M_CAN_GFC, 0x0);
@@ -788,8 +854,8 @@  static void m_can_chip_config(struct net_device *dev)
 	m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
 		    priv->mcfg[MRAM_TXB].off);
 
-	/* only support 8 bytes firstly */
-	m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
+	/* support 64 bytes payload */
+	m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_64BYTES);
 
 	m_can_write(priv, M_CAN_TXEFC, (1 << TXEFC_EFS_OFF) |
 		    priv->mcfg[MRAM_TXE].off);
@@ -804,7 +870,8 @@  static void m_can_chip_config(struct net_device *dev)
 		    RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
 
 	cccr = m_can_read(priv, M_CAN_CCCR);
-	cccr &= ~(CCCR_TEST | CCCR_MON);
+	cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
+		(CCCR_CME_MASK << CCCR_CME_SHIFT));
 	test = m_can_read(priv, M_CAN_TEST);
 	test &= ~TEST_LBCK;
 
@@ -816,6 +883,9 @@  static void m_can_chip_config(struct net_device *dev)
 		test |= TEST_LBCK;
 	}
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
+		cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
+
 	m_can_write(priv, M_CAN_CCCR, cccr);
 	m_can_write(priv, M_CAN_TEST, test);
 
@@ -880,11 +950,13 @@  static struct net_device *alloc_m_can_dev(void)
 
 	priv->dev = dev;
 	priv->can.bittiming_const = &m_can_bittiming_const;
+	priv->can.data_bittiming_const = &m_can_data_bittiming_const;
 	priv->can.do_set_mode = m_can_set_mode;
 	priv->can.do_get_berr_counter = m_can_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
 					CAN_CTRLMODE_LISTENONLY |
-					CAN_CTRLMODE_BERR_REPORTING;
+					CAN_CTRLMODE_BERR_REPORTING |
+					CAN_CTRLMODE_FD;
 
 	return dev;
 }
@@ -967,8 +1039,9 @@  static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
 	struct m_can_priv *priv = netdev_priv(dev);
-	struct can_frame *cf = (struct can_frame *)skb->data;
-	u32 id;
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	u32 id, cccr;
+	int i;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
@@ -987,11 +1060,24 @@  static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 
 	/* message ram configuration */
 	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, cf->can_dlc << 16);
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
+	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
+
+	for (i = 0; i < cf->len; i += 4)
+		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
+				 *(u32 *)(cf->data + i));
+
 	can_put_echo_skb(skb, dev, 0);
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		cccr = m_can_read(priv, M_CAN_CCCR);
+		cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
+		if (cf->flags & CANFD_BRS)
+			cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
+		else
+			cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
+		m_can_write(priv, M_CAN_CCCR, cccr);
+	}
+
 	/* enable first TX buffer to start transfer  */
 	m_can_write(priv, M_CAN_TXBTIE, 0x1);
 	m_can_write(priv, M_CAN_TXBAR, 0x1);