[PATCH 0/4] Synchronise IFI CANFD driver with real world

[PATCH 0/4] Synchronise IFI CANFD driver with real world

[Marek Vasut]

Thus far, this driver was only tested on a hardware synthesised in
the warm and safe insides of an FPGA, only against another IFI CANFD
core. The real hardware arrived now and I tested the IFI CANFD driver
against different, harsh, real-world CAN controller.

This uncovered a few bugs, so here are the fixes for those.

Marek Vasut (4):
  net: can: ifi: Fix clock generator configuration
  net: can: ifi: Fix TX DLC configuration
  net: can: ifi: Fix RX and TX ID mask
  net: can: ifi: Add obscure bit swap for EFF frame IDs

 drivers/net/can/ifi_canfd/ifi_canfd.c | 83 ++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 25 deletions(-)

Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>

-- 
2.7.0

[PATCH 1/4] net: can: ifi: Fix clock generator configuration

[Marek Vasut]

The clock generation does not match reality when using the CAN IP
core outside of the FPGA design. This patch fixes the computation
of values which are programmed into the clock generator registers.

First, there are some off-by-one errors which manifest themselves
only when communicating with different controller, so those are
fixed.

Second, the bits in the clock generator registers have different
meaning depending on whether the core is in ISO CANFD mode or any
of the other modes (BOSCH CANFD or CAN2.0). Detect the ISO CANFD
mode and fix handling of this special case of clock configuration.

Finally, the CAN clock speed is in CANCLOCK register, not SYSCLOCK
register, so fix this as well.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 43 ++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 639868b..72f5205 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -514,25 +514,25 @@ static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
 
 static const struct can_bittiming_const ifi_canfd_bittiming_const = {
 	.name		= KBUILD_MODNAME,
-	.tseg1_min	= 2,	/* Time segment 1 = prop_seg + phase_seg1 */
+	.tseg1_min	= 1,	/* Time segment 1 = prop_seg + phase_seg1 */
 	.tseg1_max	= 64,
-	.tseg2_min	= 1,	/* Time segment 2 = phase_seg2 */
-	.tseg2_max	= 16,
+	.tseg2_min	= 2,	/* Time segment 2 = phase_seg2 */
+	.tseg2_max	= 64,
 	.sjw_max	= 16,
-	.brp_min	= 1,
-	.brp_max	= 1024,
+	.brp_min	= 2,
+	.brp_max	= 256,
 	.brp_inc	= 1,
 };
 
 static const struct can_bittiming_const ifi_canfd_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,
+	.tseg1_min	= 1,	/* Time segment 1 = prop_seg + phase_seg1 */
+	.tseg1_max	= 64,
+	.tseg2_min	= 2,	/* Time segment 2 = phase_seg2 */
+	.tseg2_max	= 64,
+	.sjw_max	= 16,
+	.brp_min	= 2,
+	.brp_max	= 256,
 	.brp_inc	= 1,
 };
 
@@ -545,32 +545,33 @@ static void ifi_canfd_set_bittiming(struct net_device *ndev)
 	u32 noniso_arg = 0;
 	u32 time_off;
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO) {
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		time_off = IFI_CANFD_TIME_SJW_OFF_ISO;
+	} else {
 		noniso_arg = IFI_CANFD_TIME_SET_TIMEB_BOSCH |
 			     IFI_CANFD_TIME_SET_TIMEA_BOSCH |
 			     IFI_CANFD_TIME_SET_PRESC_BOSCH |
 			     IFI_CANFD_TIME_SET_SJW_BOSCH;
 		time_off = IFI_CANFD_TIME_SJW_OFF_BOSCH;
-	} else {
-		time_off = IFI_CANFD_TIME_SJW_OFF_ISO;
 	}
 
 	/* Configure bit timing */
-	brp = bt->brp - 1;
+	brp = bt->brp - 2;
 	sjw = bt->sjw - 1;
 	tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
-	tseg2 = bt->phase_seg2 - 1;
+	tseg2 = bt->phase_seg2 - 2;
 	writel((tseg2 << IFI_CANFD_TIME_TIMEB_OFF) |
 	       (tseg1 << IFI_CANFD_TIME_TIMEA_OFF) |
 	       (brp << IFI_CANFD_TIME_PRESCALE_OFF) |
-	       (sjw << time_off),
+	       (sjw << time_off) |
+	       noniso_arg,
 	       priv->base + IFI_CANFD_TIME);
 
 	/* Configure data bit timing */
-	brp = dbt->brp - 1;
+	brp = dbt->brp - 2;
 	sjw = dbt->sjw - 1;
 	tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
-	tseg2 = dbt->phase_seg2 - 1;
+	tseg2 = dbt->phase_seg2 - 2;
 	writel((tseg2 << IFI_CANFD_TIME_TIMEB_OFF) |
 	       (tseg1 << IFI_CANFD_TIME_TIMEA_OFF) |
 	       (brp << IFI_CANFD_TIME_PRESCALE_OFF) |
@@ -847,7 +848,7 @@ static int ifi_canfd_plat_probe(struct platform_device *pdev)
 
 	priv->can.state = CAN_STATE_STOPPED;
 
-	priv->can.clock.freq = readl(addr + IFI_CANFD_SYSCLOCK);
+	priv->can.clock.freq = readl(addr + IFI_CANFD_CANCLOCK);
 
 	priv->can.bittiming_const	= &ifi_canfd_bittiming_const;
 	priv->can.data_bittiming_const	= &ifi_canfd_data_bittiming_const;
-- 
2.7.0

[PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Marek Vasut]

The TX DLC, the transmission length information, was not written
into the transmit configuration register. When using the CAN core
with different CAN controller, the receiving CAN controller will
receive only the ID part of the CAN frame, but no data at all.

This patch adds the TX DLC into the register to fix this issue.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 72f5205..82a33bd 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 
 	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
 		if (can_is_canfd_skb(skb)) {
+			txdlc |= can_len2dlc(cf->len);
 			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
 			if (cf->flags & CANFD_BRS)
 				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
+		} else {
+			txdlc |= cf->len;
 		}
+	} else {
+		txdlc |= cf->len;
 	}
 
 	if (cf->can_id & CAN_RTR_FLAG)
-- 
2.7.0

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Oliver Hartkopp]

On 02/29/2016 08:59 PM, Marek Vasut wrote:
> The TX DLC, the transmission length information, was not written
> into the transmit configuration register. When using the CAN core
> with different CAN controller, the receiving CAN controller will
> receive only the ID part of the CAN frame, but no data at all.
> 
> This patch adds the TX DLC into the register to fix this issue.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 72f5205..82a33bd 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>  
>  	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>  		if (can_is_canfd_skb(skb)) {
> +			txdlc |= can_len2dlc(cf->len);
>  			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>  			if (cf->flags & CANFD_BRS)
>  				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
> +		} else {
> +			txdlc |= cf->len;
>  		}
> +	} else {
> +		txdlc |= cf->len;
>  	}

Please use

	txdlc |= can_len2dlc(cf->len);

by default (it works for CAN and CAN FD).

So that it looks more like:

	txdlc |= can_len2dlc(cf->len);
	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
		if (cf->flags & CANFD_BRS)
			txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
	}

Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
This configuration bit is just for the protocol on the wire and is no
distinction for CAN / CAN FD.

Best regards,
Oliver

>  
>  	if (cf->can_id & CAN_RTR_FLAG)
> 

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Marek Vasut]

On 03/01/2016 07:11 PM, Oliver Hartkopp wrote:

Hi!

> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>> The TX DLC, the transmission length information, was not written
>> into the transmit configuration register. When using the CAN core
>> with different CAN controller, the receiving CAN controller will
>> receive only the ID part of the CAN frame, but no data at all.
>>
>> This patch adds the TX DLC into the register to fix this issue.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
>> index 72f5205..82a33bd 100644
>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>>  
>>  	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>>  		if (can_is_canfd_skb(skb)) {
>> +			txdlc |= can_len2dlc(cf->len);
>>  			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>  			if (cf->flags & CANFD_BRS)
>>  				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>> +		} else {
>> +			txdlc |= cf->len;
>>  		}
>> +	} else {
>> +		txdlc |= cf->len;
>>  	}
> 
> Please use
> 
> 	txdlc |= can_len2dlc(cf->len);
> 
> by default (it works for CAN and CAN FD).
> 
> So that it looks more like:
> 
> 	txdlc |= can_len2dlc(cf->len);

Roger.

> 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
> 		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
> 		if (cf->flags & CANFD_BRS)
> 			txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
> 	}
> 
> Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
> This configuration bit is just for the protocol on the wire and is no
> distinction for CAN / CAN FD.

So CAN_CTRLMODE_FD is always set if the system operates in CAN/FD mode.
And in addition to that, if the system operates in CAN/FD BOSCH mode,
the CAN_CTRLMODE_FD_NON_ISO is set. Do I understand it correctly ?

> Best regards,
> Oliver
> 
>>  
>>  	if (cf->can_id & CAN_RTR_FLAG)
>>


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Oliver Hartkopp]

On 03/01/2016 10:27 PM, Marek Vasut wrote:
> On 03/01/2016 07:11 PM, Oliver Hartkopp wrote:
> 
> Hi!
> 
>> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>>> The TX DLC, the transmission length information, was not written
>>> into the transmit configuration register. When using the CAN core
>>> with different CAN controller, the receiving CAN controller will
>>> receive only the ID part of the CAN frame, but no data at all.
>>>
>>> This patch adds the TX DLC into the register to fix this issue.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>>> ---
>>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> index 72f5205..82a33bd 100644
>>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>>>  
>>>  	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>>>  		if (can_is_canfd_skb(skb)) {
>>> +			txdlc |= can_len2dlc(cf->len);
>>>  			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>>  			if (cf->flags & CANFD_BRS)
>>>  				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>> +		} else {
>>> +			txdlc |= cf->len;
>>>  		}
>>> +	} else {
>>> +		txdlc |= cf->len;
>>>  	}
>>
>> Please use
>>
>> 	txdlc |= can_len2dlc(cf->len);
>>
>> by default (it works for CAN and CAN FD).
>>
>> So that it looks more like:
>>
>> 	txdlc |= can_len2dlc(cf->len);
> 
> Roger.
> 
>> 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
>> 		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>> 		if (cf->flags & CANFD_BRS)
>> 			txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>> 	}
>>
>> Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
>> This configuration bit is just for the protocol on the wire and is no
>> distinction for CAN / CAN FD.
> 
> So CAN_CTRLMODE_FD is always set if the system operates in CAN/FD mode.

Not the 'system' but this specific CAN netdevice.

> And in addition to that, if the system operates in CAN/FD BOSCH mode,
> the CAN_CTRLMODE_FD_NON_ISO is set. Do I understand it correctly ?

Yep! ('the CAN netdev')

Regards,
Oliver

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Marek Vasut]

On 03/02/2016 07:12 AM, Oliver Hartkopp wrote:
> 
> 
> On 03/01/2016 10:27 PM, Marek Vasut wrote:
>> On 03/01/2016 07:11 PM, Oliver Hartkopp wrote:
>>
>> Hi!
>>
>>> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>>>> The TX DLC, the transmission length information, was not written
>>>> into the transmit configuration register. When using the CAN core
>>>> with different CAN controller, the receiving CAN controller will
>>>> receive only the ID part of the CAN frame, but no data at all.
>>>>
>>>> This patch adds the TX DLC into the register to fix this issue.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>>>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>>>> ---
>>>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> index 72f5205..82a33bd 100644
>>>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>>>>  
>>>>  	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>>>>  		if (can_is_canfd_skb(skb)) {
>>>> +			txdlc |= can_len2dlc(cf->len);
>>>>  			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>>>  			if (cf->flags & CANFD_BRS)
>>>>  				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>>> +		} else {
>>>> +			txdlc |= cf->len;
>>>>  		}
>>>> +	} else {
>>>> +		txdlc |= cf->len;
>>>>  	}
>>>
>>> Please use
>>>
>>> 	txdlc |= can_len2dlc(cf->len);
>>>
>>> by default (it works for CAN and CAN FD).
>>>
>>> So that it looks more like:
>>>
>>> 	txdlc |= can_len2dlc(cf->len);
>>
>> Roger.
>>
>>> 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
>>> 		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>> 		if (cf->flags & CANFD_BRS)
>>> 			txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>> 	}
>>>
>>> Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
>>> This configuration bit is just for the protocol on the wire and is no
>>> distinction for CAN / CAN FD.
>>
>> So CAN_CTRLMODE_FD is always set if the system operates in CAN/FD mode.
> 
> Not the 'system' but this specific CAN netdevice.

Ooops, of course, sorry.

>> And in addition to that, if the system operates in CAN/FD BOSCH mode,
>> the CAN_CTRLMODE_FD_NON_ISO is set. Do I understand it correctly ?
> 
> Yep! ('the CAN netdev')

Thanks for the clarification!

-- 
Best regards,
Marek Vasut

[PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Marek Vasut]

The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes
the incorrect mask, which caused the CAN IDs to miss the MSBit both
on receive and transmit.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 82a33bd..6704098 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -135,7 +135,7 @@
 
 #define IFI_CANFD_RXFIFO_ID			0x6c
 #define IFI_CANFD_RXFIFO_ID_ID_OFFSET		0
-#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x3ff
+#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
 #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff
 #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
 
@@ -156,7 +156,7 @@
 
 #define IFI_CANFD_TXFIFO_ID			0xbc
 #define IFI_CANFD_TXFIFO_ID_ID_OFFSET		0
-#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x3ff
+#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x7ff
 #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		0x1fffffff
 #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
 
-- 
2.7.0

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Oliver Hartkopp]

Hi Marek,

On 02/29/2016 08:59 PM, Marek Vasut wrote:
> The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes
> the incorrect mask, which caused the CAN IDs to miss the MSBit both
> on receive and transmit.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 82a33bd..6704098 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -135,7 +135,7 @@
>  
>  #define IFI_CANFD_RXFIFO_ID			0x6c
>  #define IFI_CANFD_RXFIFO_ID_ID_OFFSET		0
> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x3ff
> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff

You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.

You won't have trapped into this problem then :-)

>  #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
>  
> @@ -156,7 +156,7 @@
>  
>  #define IFI_CANFD_TXFIFO_ID			0xbc
>  #define IFI_CANFD_TXFIFO_ID_ID_OFFSET		0
> -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x3ff
> +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x7ff
>  #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		0x1fffffff

dito.


Regards,
Oliver

>  #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
>  
> 

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Marek Vasut]

On 03/01/2016 06:49 PM, Oliver Hartkopp wrote:
> Hi Marek,

Hi Oliver,

> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>> The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes
>> the incorrect mask, which caused the CAN IDs to miss the MSBit both
>> on receive and transmit.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
>> index 82a33bd..6704098 100644
>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>> @@ -135,7 +135,7 @@
>>  
>>  #define IFI_CANFD_RXFIFO_ID			0x6c
>>  #define IFI_CANFD_RXFIFO_ID_ID_OFFSET		0
>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x3ff
>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
>>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff
> 
> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.
> 
> You won't have trapped into this problem then :-)

These are register bitfield definitions, so should I really ?

My OCD kicks in and tells me it'd be odd and inconsistent with the rest
of the bitfields, but if you prefer it that way, I'll just send an
updated patch.

>>  #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
>>  
>> @@ -156,7 +156,7 @@
>>  
>>  #define IFI_CANFD_TXFIFO_ID			0xbc
>>  #define IFI_CANFD_TXFIFO_ID_ID_OFFSET		0
>> -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x3ff
>> +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x7ff
>>  #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		0x1fffffff
> 
> dito.
> 
> 
> Regards,
> Oliver
> 
>>  #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
>>  
>>


-- 
Best regards,
Marek Vasut

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Oliver Hartkopp]

Hi Marek,

On 03/01/2016 10:23 PM, Marek Vasut wrote:
> On 03/01/2016 06:49 PM, Oliver Hartkopp wrote:


>>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x3ff
>>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
>>>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff
>>
>> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
>> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.
>>
>> You won't have trapped into this problem then :-)
> 
> These are register bitfield definitions, so should I really ?
> 
> My OCD kicks in and tells me it'd be odd and inconsistent with the rest
> of the bitfields, but if you prefer it that way, I'll just send an
> updated patch.
> 

Your bit mask is masking the CAN ID out of a given variable.
That's what CAN_SFF_MASK and CAN_EFF_MASK is made for.

So at least it should be:

#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
#define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK

Btw. These defines are _never_ referenced in ifi_canfd.c so they should be
removed anyway.

Regards,
Oliver

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Marek Vasut]

On 03/02/2016 07:10 AM, Oliver Hartkopp wrote:
> Hi Marek,
> 
> On 03/01/2016 10:23 PM, Marek Vasut wrote:
>> On 03/01/2016 06:49 PM, Oliver Hartkopp wrote:
> 
> 
>>>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x3ff
>>>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
>>>>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff
>>>
>>> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
>>> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.
>>>
>>> You won't have trapped into this problem then :-)
>>
>> These are register bitfield definitions, so should I really ?
>>
>> My OCD kicks in and tells me it'd be odd and inconsistent with the rest
>> of the bitfields, but if you prefer it that way, I'll just send an
>> updated patch.
>>
> 
> Your bit mask is masking the CAN ID out of a given variable.
> That's what CAN_SFF_MASK and CAN_EFF_MASK is made for.
> 
> So at least it should be:
> 
> #define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
> #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK

This is good, I will do this. Thanks!

> Btw. These defines are _never_ referenced in ifi_canfd.c so they should be
> removed anyway.

The documentation for this core is not available, so if you don't mind,
I'd like to keep those.

-- 
Best regards,
Marek Vasut

[PATCH 4/4] net: can: ifi: Add obscure bit swap for EFF frame IDs

[Marek Vasut]

In case of CAN2.0 EFF frame, the controller handles frame IDs in a
rather bizzare way. The ID is split into an extended part, IDX[28:11]
and standard part, ID[10:0]. In the TX path, the core first sends the
top 11 bits of the IDX, followed by ID and finally the rest of IDX.
In the RX path, the core stores the ID the LSbit part of IDX field,
followed by the LSbit parts of real IDX. The MSbit parts of IDX are
stored in ID field of the register.

This patch implements the necessary bit shuffling to mitigate this
obscure behavior. In case two of these controllers are connected
together, the RX and TX bit swapping nullifies itself and the issue
does not manifest. The issue only manifests when talking to another
different CAN controller.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 6704098..254861b 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -136,7 +136,11 @@
 #define IFI_CANFD_RXFIFO_ID			0x6c
 #define IFI_CANFD_RXFIFO_ID_ID_OFFSET		0
 #define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		0x7ff
+#define IFI_CANFD_RXFIFO_ID_ID_STD_OFFSET	0
+#define IFI_CANFD_RXFIFO_ID_ID_STD_WIDTH	10
 #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		0x1fffffff
+#define IFI_CANFD_RXFIFO_ID_ID_XTD_OFFSET	11
+#define IFI_CANFD_RXFIFO_ID_ID_XTD_WIDTH	18
 #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_RXFIFO_DATA			0x70	/* 0x70..0xac */
@@ -157,7 +161,11 @@
 #define IFI_CANFD_TXFIFO_ID			0xbc
 #define IFI_CANFD_TXFIFO_ID_ID_OFFSET		0
 #define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		0x7ff
+#define IFI_CANFD_TXFIFO_ID_ID_STD_OFFSET	0
+#define IFI_CANFD_TXFIFO_ID_ID_STD_WIDTH	10
 #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		0x1fffffff
+#define IFI_CANFD_TXFIFO_ID_ID_XTD_OFFSET	11
+#define IFI_CANFD_TXFIFO_ID_ID_XTD_WIDTH	18
 #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_TXFIFO_DATA			0xc0	/* 0xb0..0xfc */
@@ -229,10 +237,20 @@ static void ifi_canfd_read_fifo(struct net_device *ndev)
 
 	rxid = readl(priv->base + IFI_CANFD_RXFIFO_ID);
 	id = (rxid >> IFI_CANFD_RXFIFO_ID_ID_OFFSET);
-	if (id & IFI_CANFD_RXFIFO_ID_IDE)
+	if (id & IFI_CANFD_RXFIFO_ID_IDE) {
 		id &= IFI_CANFD_RXFIFO_ID_ID_XTD_MASK;
-	else
+		/*
+		 * In case the Extended ID frame is received, the standard
+		 * and extended part of the ID are swapped in the register,
+		 * so swap them back to obtain the correct ID.
+		 */
+		id = (id >> IFI_CANFD_RXFIFO_ID_ID_XTD_OFFSET) |
+		     ((id & IFI_CANFD_RXFIFO_ID_ID_STD_MASK) <<
+		       IFI_CANFD_RXFIFO_ID_ID_XTD_WIDTH);
+		id |= CAN_EFF_FLAG;
+	} else {
 		id &= IFI_CANFD_RXFIFO_ID_ID_STD_MASK;
+	}
 	cf->can_id = id;
 
 	if (rxdlc & IFI_CANFD_RXFIFO_DLC_ESI) {
@@ -767,6 +785,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 
 	if (cf->can_id & CAN_EFF_FLAG) {
 		txid = cf->can_id & CAN_EFF_MASK;
+		/*
+		 * In case the Extended ID frame is transmitted, the
+		 * standard and extended part of the ID are swapped
+		 * in the register, so swap them back to send the
+		 * correct ID.
+		 */
+		txid = (txid >> IFI_CANFD_TXFIFO_ID_ID_XTD_WIDTH) |
+		       ((txid & IFI_CANFD_TXFIFO_ID_ID_XTD_MASK) <<
+		         IFI_CANFD_TXFIFO_ID_ID_XTD_OFFSET);
 		txid |= IFI_CANFD_TXFIFO_ID_IDE;
 	} else {
 		txid = cf->can_id & CAN_SFF_MASK;
-- 
2.7.0

[PATCH V2 0/4] Synchronise IFI CANFD driver with real world

[Marek Vasut]

Thus far, this driver was only tested on a hardware synthesised in
the warm and safe insides of an FPGA, only against another IFI CANFD
core. The real hardware arrived now and I tested the IFI CANFD driver
against different, harsh, real-world CAN controller.

This uncovered a few bugs, so here are the fixes for those.

Marek Vasut (4):
  net: can: ifi: Fix clock generator configuration
  net: can: ifi: Fix TX DLC configuration
  net: can: ifi: Fix RX and TX ID mask
  net: can: ifi: Add obscure bit swap for EFF frame IDs

 drivers/net/can/ifi_canfd/ifi_canfd.c | 93 ++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 33 deletions(-)

V2: Address comments in the ML

-- 
2.7.0

[PATCH 4/4] net: can: ifi: Add obscure bit swap for EFF frame IDs

[Marek Vasut]

In case of CAN2.0 EFF frame, the controller handles frame IDs in a
rather bizzare way. The ID is split into an extended part, IDX[28:11]
and standard part, ID[10:0]. In the TX path, the core first sends the
top 11 bits of the IDX, followed by ID and finally the rest of IDX.
In the RX path, the core stores the ID the LSbit part of IDX field,
followed by the LSbit parts of real IDX. The MSbit parts of IDX are
stored in ID field of the register.

This patch implements the necessary bit shuffling to mitigate this
obscure behavior. In case two of these controllers are connected
together, the RX and TX bit swapping nullifies itself and the issue
does not manifest. The issue only manifests when talking to another
different CAN controller.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
---
V2: Rebase on top of patch 3/4, no change to the patch
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 238dd02..a55f080 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -136,7 +136,11 @@
 #define IFI_CANFD_RXFIFO_ID			0x6c
 #define IFI_CANFD_RXFIFO_ID_ID_OFFSET		0
 #define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
+#define IFI_CANFD_RXFIFO_ID_ID_STD_OFFSET	0
+#define IFI_CANFD_RXFIFO_ID_ID_STD_WIDTH	10
 #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK
+#define IFI_CANFD_RXFIFO_ID_ID_XTD_OFFSET	11
+#define IFI_CANFD_RXFIFO_ID_ID_XTD_WIDTH	18
 #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_RXFIFO_DATA			0x70	/* 0x70..0xac */
@@ -157,7 +161,11 @@
 #define IFI_CANFD_TXFIFO_ID			0xbc
 #define IFI_CANFD_TXFIFO_ID_ID_OFFSET		0
 #define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
+#define IFI_CANFD_TXFIFO_ID_ID_STD_OFFSET	0
+#define IFI_CANFD_TXFIFO_ID_ID_STD_WIDTH	10
 #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK
+#define IFI_CANFD_TXFIFO_ID_ID_XTD_OFFSET	11
+#define IFI_CANFD_TXFIFO_ID_ID_XTD_WIDTH	18
 #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_TXFIFO_DATA			0xc0	/* 0xb0..0xfc */
@@ -229,10 +237,20 @@ static void ifi_canfd_read_fifo(struct net_device *ndev)
 
 	rxid = readl(priv->base + IFI_CANFD_RXFIFO_ID);
 	id = (rxid >> IFI_CANFD_RXFIFO_ID_ID_OFFSET);
-	if (id & IFI_CANFD_RXFIFO_ID_IDE)
+	if (id & IFI_CANFD_RXFIFO_ID_IDE) {
 		id &= IFI_CANFD_RXFIFO_ID_ID_XTD_MASK;
-	else
+		/*
+		 * In case the Extended ID frame is received, the standard
+		 * and extended part of the ID are swapped in the register,
+		 * so swap them back to obtain the correct ID.
+		 */
+		id = (id >> IFI_CANFD_RXFIFO_ID_ID_XTD_OFFSET) |
+		     ((id & IFI_CANFD_RXFIFO_ID_ID_STD_MASK) <<
+		       IFI_CANFD_RXFIFO_ID_ID_XTD_WIDTH);
+		id |= CAN_EFF_FLAG;
+	} else {
 		id &= IFI_CANFD_RXFIFO_ID_ID_STD_MASK;
+	}
 	cf->can_id = id;
 
 	if (rxdlc & IFI_CANFD_RXFIFO_DLC_ESI) {
@@ -767,6 +785,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 
 	if (cf->can_id & CAN_EFF_FLAG) {
 		txid = cf->can_id & CAN_EFF_MASK;
+		/*
+		 * In case the Extended ID frame is transmitted, the
+		 * standard and extended part of the ID are swapped
+		 * in the register, so swap them back to send the
+		 * correct ID.
+		 */
+		txid = (txid >> IFI_CANFD_TXFIFO_ID_ID_XTD_WIDTH) |
+		       ((txid & IFI_CANFD_TXFIFO_ID_ID_XTD_MASK) <<
+		         IFI_CANFD_TXFIFO_ID_ID_XTD_OFFSET);
 		txid |= IFI_CANFD_TXFIFO_ID_IDE;
 	} else {
 		txid = cf->can_id & CAN_SFF_MASK;
-- 
2.7.0