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

[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 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 V2 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>
---
V2: - Use can_len2dlc() unconditionally to sanitize DLC in the TX path
    - The CAN_CTRLMODE_CANFD_NON_ISO can only ever be set if the
      CANFD_CTRLMODE_CANFD is set. Use only the later for checking
      if the core transmits in CANFD mode.
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 72f5205..4bbc7d9 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -772,12 +772,11 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 		txid = cf->can_id & CAN_SFF_MASK;
 	}
 
-	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
-		if (can_is_canfd_skb(skb)) {
-			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
-			if (cf->flags & CANFD_BRS)
-				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
-		}
+	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;
 	}
 
 	if (cf->can_id & CAN_RTR_FLAG)
-- 
2.7.0

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

[Oliver Hartkopp]

Hi Marek,

sorry for picking this patch up again.

After looking around in the original source I have one more comment:

On 03/02/2016 11:42 AM, Marek Vasut wrote:

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

At the top of the function you defined

	u32 txst, txid;
	u32 txdlc = 0;

and here you use 'txdlc |= ...'.

That's weird style IMO.

Please change it to

	u32 txst, txid, txdlc;

and

	txdlc = can_len2dlc(cf->len);


Thanks,
Oliver

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

[Marek Vasut]

On 03/02/2016 07:46 PM, Oliver Hartkopp wrote:
> Hi Marek,

Hi!

> sorry for picking this patch up again.

nothing to be sorry about, thanks for the input :)

> After looking around in the original source I have one more comment:
> 
> On 03/02/2016 11:42 AM, Marek Vasut wrote:
> 
>> -	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>> -		if (can_is_canfd_skb(skb)) {
>> -			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>> -			if (cf->flags & CANFD_BRS)
>> -				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>> -		}
>> +	txdlc |= can_len2dlc(cf->len);
> 
> At the top of the function you defined
> 
> 	u32 txst, txid;
> 	u32 txdlc = 0;
> 
> and here you use 'txdlc |= ...'.
> 
> That's weird style IMO.
> 
> Please change it to
> 
> 	u32 txst, txid, txdlc;
> 
> and
> 
> 	txdlc = can_len2dlc(cf->len);

I'll send a V3 shortly.

btw. I forgot to add V2 to the 3/4 and 4/4 patches, sigh.


-- 
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>
---
V2: Use CAN_SFF_MASK instead of plain bitfield
    Use CAN_EFF_MASK for the extended frame format
---
 drivers/net/can/ifi_canfd/ifi_canfd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 4bbc7d9..238dd02 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -135,8 +135,8 @@
 
 #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_XTD_MASK		0x1fffffff
+#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
+#define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK
 #define IFI_CANFD_RXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_RXFIFO_DATA			0x70	/* 0x70..0xac */
@@ -156,8 +156,8 @@
 
 #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_XTD_MASK		0x1fffffff
+#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK		CAN_SFF_MASK
+#define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK		CAN_EFF_MASK
 #define IFI_CANFD_TXFIFO_ID_IDE			BIT(29)
 
 #define IFI_CANFD_TXFIFO_DATA			0xc0	/* 0xb0..0xfc */
-- 
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