Dual SJA1000 can controllers on SMP system.

Dual SJA1000 can controllers on SMP system.

[Mirza Krak]

Dear Sirs.

I am running a 3.1.10 kernel on Nvidia Tegra 2 CPU (Dual Core). Not 
vanilla kernel (Linux4Tegra).

We are using dual can controllers connected on th external data bus on 
our custom hardware. The can controllers are SJA1000.

Driver that is used is vanilla sja1000 driver (sja1000.c)

Now to the issue at hand. Will try my best to explain our scenario and 
issue:
     1. The two can controllers are connected with each other with a 
loop-back.
     2. What we are attempting is to send data on both controllers in 
two separate processes. We are using following command "cansequence can0 
-p &" and "cansequence can1 -p &". With a bit-rate configured to 1 Mbit/s.
     3. What we see when we attempt the above is that one of the 
controllers stop sending and the software buffer gets filled up. When it 
stops varies but usually after a few packets are sent.

After some digging we came to this conclusion. The two cansequence 
processes end up on different CPU cores. The process that ends up in 
CPU1 is the one that always stops transmitting.

Workarounds that we have seen that works is
     1. Disable dual core with kernel argument "maxcpus=1".
     2. Forcing the cansequence processes to CPU0 with "taskset" command.

My question to you is if anyone has tested the SJA1000 driver on an SMP 
system or anything similar.
-- 
Med Vänliga Hälsningar / Best Regards
Mirza

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
www.hostmobility.com <http://www.hostmobility.com>
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************

Re: Dual SJA1000 can controllers on SMP system.

[Wolfgang Grandegger]

On 06/13/2013 02:47 PM, Mirza Krak wrote:
> Dear Sirs.
> 
> I am running a 3.1.10 kernel on Nvidia Tegra 2 CPU (Dual Core). Not
> vanilla kernel (Linux4Tegra).
> 
> We are using dual can controllers connected on th external data bus on
> our custom hardware. The can controllers are SJA1000.

What external data bus is the CAN controller connected to?
And which CAN controller board/card?

> Driver that is used is vanilla sja1000 driver (sja1000.c)

Which bus driver is used and how do you load the module?

> Now to the issue at hand. Will try my best to explain our scenario and
> issue:
>     1. The two can controllers are connected with each other with a
> loop-back.
>     2. What we are attempting is to send data on both controllers in two
> separate processes. We are using following command "cansequence can0 -p
> &" and "cansequence can1 -p &". With a bit-rate configured to 1 Mbit/s.
>     3. What we see when we attempt the above is that one of the
> controllers stop sending and the software buffer gets filled up. When it
> stops varies but usually after a few packets are sent.
> 
> After some digging we came to this conclusion. The two cansequence
> processes end up on different CPU cores. The process that ends up in
> CPU1 is the one that always stops transmitting.
> 
> Workarounds that we have seen that works is
>     1. Disable dual core with kernel argument "maxcpus=1".
>     2. Forcing the cansequence processes to CPU0 with "taskset" command.
> 
> My question to you is if anyone has tested the SJA1000 driver on an SMP
> system or anything similar.

Well, sending from two processes concurrently is not really a typical
use case. ftrace is your friend. A function trace may help to find the
root of the problem. Let me know if you need further help on that.

Wolfgang.

Re: Dual SJA1000 can controllers on SMP system.

[mirza]

-------- Ursprungligt meddelande --------
> Från: Wolfgang Grandegger <wg@grandegger.com>
> Skickat: den 16 juni 2013 11:19
> Till: Mirza Krak <mirza.krak@hostmobility.com>
> Ämne: Re: Dual SJA1000 can controllers on SMP system.
> 
> On 06/13/2013 02:47 PM, Mirza Krak wrote:
> > Dear Sirs.
> > 
> > I am running a 3.1.10 kernel on Nvidia Tegra 2 CPU (Dual Core). Not
> > vanilla kernel (Linux4Tegra).
> > 
> > We are using dual can controllers connected on th external data bus on
> > our custom hardware. The can controllers are SJA1000.
> 
> What external data bus is the CAN controller connected to?
> And which CAN controller board/card?

As I mentioned the CAN controllers are SJA1000 and the SJA1000 chip is integrated in our board. No external card/board for SJA1000 chip.

It is connected to the Nvidia Tegra 2 externa data bus (a.k.a SNOR). 

> > Driver that is used is vanilla sja1000 driver (sja1000.c)
> 
> Which bus driver is used and how do you load the module?

The bus driver is tegra_snor and is linked in statically in the kernel.  

> 
> > Now to the issue at hand. Will try my best to explain our scenario and
> > issue:
> >     1. The two can controllers are connected with each other with a
> > loop-back.
> >     2. What we are attempting is to send data on both controllers in two
> > separate processes. We are using following command "cansequence can0 -p
> > &" and "cansequence can1 -p &". With a bit-rate configured to 1 Mbit/s.
> >     3. What we see when we attempt the above is that one of the
> > controllers stop sending and the software buffer gets filled up. When it
> > stops varies but usually after a few packets are sent.
> > 
> > After some digging we came to this conclusion. The two cansequence
> > processes end up on different CPU cores. The process that ends up in
> > CPU1 is the one that always stops transmitting.
> > 
> > Workarounds that we have seen that works is
> >     1. Disable dual core with kernel argument "maxcpus=1".
> >     2. Forcing the cansequence processes to CPU0 with "taskset" command.
> > 
> > My question to you is if anyone has tested the SJA1000 driver on an SMP
> > system or anything similar.
> 
> Well, sending from two processes concurrently is not really a typical
> use case. ftrace is your friend. A function trace may help to find the
> root of the problem. Let me know if you need further help on that.

I agree that this is not the typical use case but I still don´t see why this would not work. Any tip on where to start tracing?

> 
> Wolfgang.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 

Re: Dual SJA1000 can controllers on SMP system.

[Wolfgang Grandegger]

On 06/16/2013 01:20 PM, mirza@hostmobility.com wrote:
> 
> 
> -------- Ursprungligt meddelande --------
>> Från: Wolfgang Grandegger <wg@grandegger.com>
>> Skickat: den 16 juni 2013 11:19
>> Till: Mirza Krak <mirza.krak@hostmobility.com>
>> Ämne: Re: Dual SJA1000 can controllers on SMP system.
>>
>> On 06/13/2013 02:47 PM, Mirza Krak wrote:
>>> Dear Sirs.
>>>
>>> I am running a 3.1.10 kernel on Nvidia Tegra 2 CPU (Dual Core). Not
>>> vanilla kernel (Linux4Tegra).
>>>
>>> We are using dual can controllers connected on th external data bus on
>>> our custom hardware. The can controllers are SJA1000.
>>
>> What external data bus is the CAN controller connected to?
>> And which CAN controller board/card?
> 
> As I mentioned the CAN controllers are SJA1000 and the SJA1000 chip is integrated in our board. No external card/board for SJA1000 chip.
> 
> It is connected to the Nvidia Tegra 2 externa data bus (a.k.a SNOR). 
> 
>>> Driver that is used is vanilla sja1000 driver (sja1000.c)
>>
>> Which bus driver is used and how do you load the module?
> 
> The bus driver is tegra_snor and is linked in statically in the kernel.  

The low-level device initialization and access is not done in sja1000.c
but by another driver module, e.g., sja1000_isa, sja1000_platform,
ems_pci.c. What module are you using? What functions are you using to
access the SNOR bus?

Wolfgang.

Re: Dual SJA1000 can controllers on SMP system.

[Mirza Krak]

On 06/16/2013 02:39 PM, Wolfgang Grandegger wrote:
> On 06/16/2013 01:20 PM, mirza@hostmobility.com wrote:
>>
>>
>> -------- Ursprungligt meddelande --------
>>> Från: Wolfgang Grandegger <wg@grandegger.com>
>>> Skickat: den 16 juni 2013 11:19
>>> Till: Mirza Krak <mirza.krak@hostmobility.com>
>>> Ämne: Re: Dual SJA1000 can controllers on SMP system.
>>>
>>> On 06/13/2013 02:47 PM, Mirza Krak wrote:
>>>> Dear Sirs.
>>>>
>>>> I am running a 3.1.10 kernel on Nvidia Tegra 2 CPU (Dual Core). Not
>>>> vanilla kernel (Linux4Tegra).
>>>>
>>>> We are using dual can controllers connected on th external data bus on
>>>> our custom hardware. The can controllers are SJA1000.
>>>
>>> What external data bus is the CAN controller connected to?
>>> And which CAN controller board/card?
>>
>> As I mentioned the CAN controllers are SJA1000 and the SJA1000 chip is integrated in our board. No external card/board for SJA1000 chip.
>>
>> It is connected to the Nvidia Tegra 2 externa data bus (a.k.a SNOR).
>>
>>>> Driver that is used is vanilla sja1000 driver (sja1000.c)
>>>
>>> Which bus driver is used and how do you load the module?
>>
>> The bus driver is tegra_snor and is linked in statically in the kernel.
>
> The low-level device initialization and access is not done in sja1000.c
> but by another driver module, e.g., sja1000_isa, sja1000_platform,
> ems_pci.c. What module are you using? What functions are you using to
> access the SNOR bus?

Aah off course. The low-level device driver is sja1000_platform.c. The 
functions I am using are sp_read_reg8 and sp_write_reg8 which I have 
slightly modified. Since I don't use multiplexed bus functionality I 
write the address first.

   static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
{
	iowrite8(reg, priv->reg_base);
	return ioread8(priv->reg_base + 0x20);
}

static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
{
	iowrite8(reg, priv->reg_base);
	iowrite8(val, (priv->reg_base + 0x20));
}

>
> Wolfgang.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Dual SJA1000 can controllers on SMP system.

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On 06/17/2013 08:10 AM, Mirza Krak wrote:
> Aah off course. The low-level device driver is sja1000_platform.c. The
> functions I am using are sp_read_reg8 and sp_write_reg8 which I have
> slightly modified. Since I don't use multiplexed bus functionality I
> write the address first.
> 
>   static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
> {
>     iowrite8(reg, priv->reg_base);
      ^^^^^^^^
>     return ioread8(priv->reg_base + 0x20);
> }
> 
> static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
> {
>     iowrite8(reg, priv->reg_base);
      ^^^^^^^^
>     iowrite8(val, (priv->reg_base + 0x20));
> }

Can you post a proper patch, so that we can see what you have changed?

You probably have a nice race condition here. The two iowrites have to
be atomic.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: Dual SJA1000 can controllers on SMP system.

[Mirza Krak]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

My modifications to sja1000_platform.c are attached as a patch.

Wolfgang: Using spinlock_irqsave() and spinlock_irqrestore() was also my 
initial idea on the io functions but I tried this without it solving the 
problem. I could try this again to be sure.

Med Vänliga Hälsningar / Best Regards
Mirza

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
www.hostmobility.com <http://www.hostmobility.com>
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************


On 06/17/2013 09:18 AM, Marc Kleine-Budde wrote:
> On 06/17/2013 08:10 AM, Mirza Krak wrote:
>> Aah off course. The low-level device driver is sja1000_platform.c. The
>> functions I am using are sp_read_reg8 and sp_write_reg8 which I have
>> slightly modified. Since I don't use multiplexed bus functionality I
>> write the address first.
>>
>>    static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
>> {
>>      iowrite8(reg, priv->reg_base);
>        ^^^^^^^^
>>      return ioread8(priv->reg_base + 0x20);
>> }
>>
>> static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
>> {
>>      iowrite8(reg, priv->reg_base);
>        ^^^^^^^^
>>      iowrite8(val, (priv->reg_base + 0x20));
>> }
>
> Can you post a proper patch, so that we can see what you have changed?
>
> You probably have a nice race condition here. The two iowrites have to
> be atomic.
>
> Marc
>

[-- Attachment #2: sja1000_platform.patch --]
[-- Type: text/x-patch, Size: 646 bytes --]

Index: sja1000_platform.c
===================================================================
--- sja1000_platform.c	(revision 160)
+++ sja1000_platform.c	(working copy)
@@ -38,12 +38,14 @@
 
 static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
 {
-	return ioread8(priv->reg_base + reg);
+	iowrite8(reg, priv->reg_base);
+	return ioread8(priv->reg_base + 0x20);
 }
 
 static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
 {
-	iowrite8(val, priv->reg_base + reg);
+	iowrite8(reg, priv->reg_base);
+	iowrite8(val, (priv->reg_base + 0x20));
 }
 
 static u8 sp_read_reg16(const struct sja1000_priv *priv, int reg)

Re: Dual SJA1000 can controllers on SMP system.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 700 bytes --]

On 06/17/2013 03:46 PM, Mirza Krak wrote:
> My modifications to sja1000_platform.c are attached as a patch.
> 
> Wolfgang: Using spinlock_irqsave() and spinlock_irqrestore() was also my
> initial idea on the io functions but I tried this without it solving the
> problem. I could try this again to be sure.

Without problem locking it not work anyway. Try the attached patch (it's
compile time tested only).

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   |

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-HACK-can-sja1000_platform-add-support-for-indirect-r.patch --]
[-- Type: text/x-diff; name="0001-HACK-can-sja1000_platform-add-support-for-indirect-r.patch", Size: 2793 bytes --]

From bfebee863f12bee39dee9b00bfe9e3d8c1017d16 Mon Sep 17 00:00:00 2001
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Mon, 17 Jun 2013 16:09:13 +0200
Subject: [PATCH] HACK: can: sja1000_platform: add support for indirect
 register access
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on a privious work by Vänliga Hälsningar.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/sja1000/sja1000_platform.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 8e259c5..35f279c 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -37,14 +37,33 @@ MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
 MODULE_ALIAS("platform:" DRV_NAME);
 MODULE_LICENSE("GPL v2");
 
+struct sp_priv {
+	spinlock_t ind_lock;
+};
+
 static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
 {
-	return ioread8(priv->reg_base + reg);
+	struct sp_priv *sp = priv->priv;
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&sp->ind_lock, flags);
+	iowrite8(reg, priv->reg_base);
+	val = ioread8(priv->reg_base + 0x20);
+	spin_unlock_irqrestore(&sp->ind_lock, flags);
+
+	return val;
 }
 
 static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
 {
-	iowrite8(val, priv->reg_base + reg);
+	struct sp_priv *sp = priv->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sp->ind_lock, flags);
+	iowrite8(reg, priv->reg_base);
+	iowrite8(val, (priv->reg_base + 0x20));
+	spin_unlock_irqrestore(&sp->ind_lock, flags);
 }
 
 static u8 sp_read_reg16(const struct sja1000_priv *priv, int reg)
@@ -73,6 +92,7 @@ static int sp_probe(struct platform_device *pdev)
 	void __iomem *addr;
 	struct net_device *dev;
 	struct sja1000_priv *priv;
+	struct sp_priv *sp;
 	struct resource *res_mem, *res_irq;
 	struct sja1000_platform_data *pdata;
 
@@ -102,12 +122,13 @@ static int sp_probe(struct platform_device *pdev)
 		goto exit_release;
 	}
 
-	dev = alloc_sja1000dev(0);
+	dev = alloc_sja1000dev(sizeof(struct sp_priv));
 	if (!dev) {
 		err = -ENOMEM;
 		goto exit_iounmap;
 	}
 	priv = netdev_priv(dev);
+	sp = priv->priv;
 
 	dev->irq = res_irq->start;
 	priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
@@ -118,6 +139,7 @@ static int sp_probe(struct platform_device *pdev)
 	priv->can.clock.freq = pdata->osc_freq / 2;
 	priv->ocr = pdata->ocr;
 	priv->cdr = pdata->cdr;
+	spin_lock_init(&sp->ind_lock);
 
 	switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
 	case IORESOURCE_MEM_32BIT:
-- 
1.8.3.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: Dual SJA1000 can controllers on SMP system.

[Mirza Krak]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

Thank you Marc for your patch. We  applied it but it did not solve our 
problem.

Wolfgang suggested defining a lock common for all devices:

  static DEFINE_SPINLOCK(snor_bus_lock);

Which I applied and now it seems to work perfectly. Thank you for your help.

Should I blame the hardware driver? :)

Attaching the patch for reference .

Med Vänliga Hälsningar / Best Regards
Mirza

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
www.hostmobility.com <http://www.hostmobility.com>
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************


On 06/17/2013 04:12 PM, Marc Kleine-Budde wrote:
> On 06/17/2013 03:46 PM, Mirza Krak wrote:
>> My modifications to sja1000_platform.c are attached as a patch.
>>
>> Wolfgang: Using spinlock_irqsave() and spinlock_irqrestore() was also my
>> initial idea on the io functions but I tried this without it solving the
>> problem. I could try this again to be sure.
>
> Without problem locking it not work anyway. Try the attached patch (it's
> compile time tested only).
>
> Marc
>

[-- Attachment #2: sja1000_platform.c.patch --]
[-- Type: text/x-diff, Size: 1261 bytes --]

Index: drivers/net/can/sja1000/sja1000_platform.c
===================================================================
--- drivers/net/can/sja1000/sja1000_platform.c	(revision 519)
+++ drivers/net/can/sja1000/sja1000_platform.c	(working copy)
@@ -27,6 +27,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/platform/sja1000.h>
 #include <linux/io.h>
+#include <linux/spinlock_types.h>
 
 #include "sja1000.h"
 
@@ -36,16 +37,29 @@
 MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
 MODULE_LICENSE("GPL v2");
 
+static DEFINE_SPINLOCK(snor_bus_lock);
+
 static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
 {
+	u8 value;
+	unsigned long flags;
+
+	spin_lock_irqsave(&snor_bus_lock, flags);
 	iowrite8(reg, priv->reg_base);
-	return ioread8(priv->reg_base + 0x20);
+	value = ioread8(priv->reg_base + 0x20);
+	spin_unlock_irqrestore(&snor_bus_lock, flags);
+
+	return value;
 }
 
 static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&snor_bus_lock, flags);
 	iowrite8(reg, priv->reg_base);
 	iowrite8(val, (priv->reg_base + 0x20));
+	spin_unlock_irqrestore(&snor_bus_lock, flags);
 }
 
 static u8 sp_read_reg16(const struct sja1000_priv *priv, int reg)

Re: Dual SJA1000 can controllers on SMP system.

[Wolfgang Grandegger]

On 06/17/2013 08:49 PM, Mirza Krak wrote:
> Thank you Marc for your patch. We  applied it but it did not solve our
> problem.
> 
> Wolfgang suggested defining a lock common for all devices:
> 
>  static DEFINE_SPINLOCK(snor_bus_lock);
> 
> Which I applied and now it seems to work perfectly. Thank you for your
> help.
> 
> Should I blame the hardware driver? :)

Even if there is a separate set of registers for each device, they can
obviously not be accessed concurrently. Well, you can say it's a
hardware feature, but a stupid and inefficient one.

Wolfgang.