SJA1000 register accesses

SJA1000 register accesses

[Nikita Edward Baruzdin]

Hello,

We use CAN at 1 Mbit/s with sometimes pretty high bus load (our client wants to
do some strange things over CAN). So my colleagues were complaining that CAN
consumes too much CPU on our machines, which negatively affects the work of
other network interfaces.

While investigating if we can do anything about this, I have learned that
sja1000 interrupt handler wastes 90 % of the time on register accesses.
Particularly, when the driver deals with the transmit/receive buffer, it needs
to access up to 13 consecutive bytes (frame information byte + 2/4 CAN ID bytes
+ 0-8 payload bytes). At the moment, this is done with 13 ioread8()/iowrite8()
calls. Considering that PCI bus transactions are at least 32 bits wide, I
believe we can do much better.

Thus I made the patch. I tested it on the machine with Intel Core 2 Duo E4700
CPU (2.60 GHz) and ELCUS CAN-200PCI (SJA1000-based) card under the maximum load
(cangen -ig 0) and got the following results:

> ==========================================================================================================
>			CPU load: irq/can0, irq/can1	Bus load		Frames per second (+/- 100)
> ----------------------------------------------------------------------------------------------------------
> Traffic\Variant	Current		Patched		Current	Patched		Current	Patched
> ----------------------------------------------------------------------------------------------------------
> STD random		25 %, 26 %	22 %, 23 %	88 %	92 %		7900	8200
> EXT random		23 %, 24 %	20 %, 20 %	91 %	94 %		6600	6900
> STD RTR random	37 %, 31 %	34 %, 31 %	73 %	77 %		13400	14000
> EXT RTR random	31 %, 27 %	28 %, 25 %	80 %	85 %		10000	10600
> STD 8 bytes payload	23 %, 25 %	21 %, 21 %	90 %	94 %		6600	7000
> EXT 8 bytes payload	22 %, 24 %	19 %, 19 %	91 %	96 %		5700	6000
> STD 5 bytes payload	26 %, 27 %	24 %, 23 %	88 %	91 %		8400	8600
> STD 1 byte payload	30 %, 31 %	29 %, 30 %	82 %	83 %		12600	12800

These results are obtained with the help of cangen, htop and canbusload. can0
was the sender and can1 — the receiver. As you can see, in all cases the CPU
load decreased while the number of transmitted frames and the bus load went up.

[PATCH] can: sja1000: Optimise register accesses

[Nikita Edward Baruzdin]

Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
instead of consecutive ioread8()/iowrite8() calls seems reasonable.

Thus, this patch introduces plx_pci_read_reg_rep() and
plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
register accesses as much as possible. The functions are then used to
read/write CAN ID and payload data to improve driver performance.

Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
 drivers/net/can/sja1000/plx_pci.c | 42 +++++++++++++++++++++++++++++++++++++++
 drivers/net/can/sja1000/sja1000.c | 30 +++++++++++-----------------
 drivers/net/can/sja1000/sja1000.h |  4 ++++
 3 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..7aaf6b9 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -371,6 +371,46 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
 	iowrite8(val, priv->reg_base + port);
 }
 
+static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
+                                 void *buffer, unsigned long count)
+{
+	u8 *p = buffer;
+
+	while (count >= 4) {
+		*(u32 *)p = ioread32(priv->reg_base + reg);
+		p += 4;
+		reg += 4;
+		count -= 4;
+	}
+	if (count & 2) {
+		*(u16 *)p = ioread16(priv->reg_base + reg);
+		p += 2;
+		reg += 2;
+	}
+	if (count & 1)
+		*p = ioread8(priv->reg_base + reg);
+}
+
+static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
+                                  const void *buffer, unsigned long count)
+{
+	const u8 *p = buffer;
+
+	while (count >= 4) {
+		iowrite32(*(u32 *)p, priv->reg_base + reg);
+		p += 4;
+		reg += 4;
+		count -= 4;
+	}
+	if (count & 2) {
+		iowrite16(*(u16 *)p, priv->reg_base + reg);
+		p += 2;
+		reg += 2;
+	}
+	if (count & 1)
+		iowrite8(*p, priv->reg_base + reg);
+}
+
 /*
  * Check if a CAN controller is present at the specified location
  * by trying to switch 'em from the Basic mode into the PeliCAN mode.
@@ -626,6 +666,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
 		priv->reg_base = addr + cm->offset;
 		priv->read_reg = plx_pci_read_reg;
 		priv->write_reg = plx_pci_write_reg;
+		priv->read_reg_rep = plx_pci_read_reg_rep;
+		priv->write_reg_rep = plx_pci_write_reg_rep;
 
 		/* Check if channel is present */
 		if (plx_pci_check_sja1000(priv)) {
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 9f10779..b26173e 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -288,7 +288,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 	canid_t id;
 	uint8_t dreg;
 	u8 cmd_reg_val = 0x00;
-	int i;
+	u8 id_buf[4];
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
@@ -305,19 +305,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 		fi |= SJA1000_FI_FF;
 		dreg = SJA1000_EFF_BUF;
 		priv->write_reg(priv, SJA1000_FI, fi);
-		priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
-		priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
-		priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
-		priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+		*(u32 *)id_buf = cpu_to_be32(id << 3);
+		priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
 	} else {
 		dreg = SJA1000_SFF_BUF;
 		priv->write_reg(priv, SJA1000_FI, fi);
-		priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
-		priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+		*(u16 *)id_buf = cpu_to_be16(id << 5);
+		priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
 	}
 
-	for (i = 0; i < dlc; i++)
-		priv->write_reg(priv, dreg++, cf->data[i]);
+	priv->write_reg_rep(priv, dreg, cf->data, dlc);
 
 	can_put_echo_skb(skb, dev, 0);
 
@@ -343,7 +340,7 @@ static void sja1000_rx(struct net_device *dev)
 	uint8_t fi;
 	uint8_t dreg;
 	canid_t id;
-	int i;
+	u8 id_buf[4];
 
 	/* create zero'ed CAN frame buffer */
 	skb = alloc_can_skb(dev, &cf);
@@ -355,24 +352,21 @@ static void sja1000_rx(struct net_device *dev)
 	if (fi & SJA1000_FI_FF) {
 		/* extended frame format (EFF) */
 		dreg = SJA1000_EFF_BUF;
-		id = (priv->read_reg(priv, SJA1000_ID1) << 21)
-		    | (priv->read_reg(priv, SJA1000_ID2) << 13)
-		    | (priv->read_reg(priv, SJA1000_ID3) << 5)
-		    | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+		priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+		id = be32_to_cpu(*(u32 *)id_buf) >> 3;
 		id |= CAN_EFF_FLAG;
 	} else {
 		/* standard frame format (SFF) */
 		dreg = SJA1000_SFF_BUF;
-		id = (priv->read_reg(priv, SJA1000_ID1) << 3)
-		    | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+		priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+		id = be16_to_cpu(*(u16 *)id_buf) >> 5;
 	}
 
 	cf->can_dlc = get_can_dlc(fi & 0x0F);
 	if (fi & SJA1000_FI_RTR) {
 		id |= CAN_RTR_FLAG;
 	} else {
-		for (i = 0; i < cf->can_dlc; i++)
-			cf->data[i] = priv->read_reg(priv, dreg++);
+		priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
 	}
 
 	cf->can_id = id;
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..887045a 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,6 +157,10 @@ struct sja1000_priv {
 	/* the lower-layer is responsible for appropriate locking */
 	u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
 	void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
+	void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
+	                      void *buffer, unsigned long count);
+	void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
+	                       const void *buffer, unsigned long count);
 	void (*pre_irq) (const struct sja1000_priv *priv);
 	void (*post_irq) (const struct sja1000_priv *priv);
 
-- 
2.9.3

Re: [PATCH] can: sja1000: Optimise register accesses

[Oliver Hartkopp]

Hello Nikita,

On 09/05/2016 07:58 PM, Nikita Edward Baruzdin wrote:
> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
> instead of consecutive ioread8()/iowrite8() calls seems reasonable.

I definitely like this patch.

> Thus, this patch introduces plx_pci_read_reg_rep() and
> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
> register accesses as much as possible. The functions are then used to
> read/write CAN ID and payload data to improve driver performance.

But there is something more to do as you currently brake all sja1000 
drivers except plx_pci ;-)

You need to take care of drivers that do not support 
priv->[read|write]_reg_rep().

So that other (pci) drivers can follow at will after testing.

> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
> ---
>  drivers/net/can/sja1000/plx_pci.c | 42 +++++++++++++++++++++++++++++++++++++++
>  drivers/net/can/sja1000/sja1000.c | 30 +++++++++++-----------------
>  drivers/net/can/sja1000/sja1000.h |  4 ++++
>  3 files changed, 58 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
> index 3eb7430..7aaf6b9 100644
> --- a/drivers/net/can/sja1000/plx_pci.c
> +++ b/drivers/net/can/sja1000/plx_pci.c
> @@ -371,6 +371,46 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
>  	iowrite8(val, priv->reg_base + port);
>  }
>
> +static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
> +                                 void *buffer, unsigned long count)
> +{
> +	u8 *p = buffer;
> +
> +	while (count >= 4) {
> +		*(u32 *)p = ioread32(priv->reg_base + reg);
> +		p += 4;
> +		reg += 4;
> +		count -= 4;
> +	}
> +	if (count & 2) {
> +		*(u16 *)p = ioread16(priv->reg_base + reg);
> +		p += 2;
> +		reg += 2;
> +	}
> +	if (count & 1)
> +		*p = ioread8(priv->reg_base + reg);
> +}
> +
> +static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
> +                                  const void *buffer, unsigned long count)
> +{
> +	const u8 *p = buffer;
> +
> +	while (count >= 4) {
> +		iowrite32(*(u32 *)p, priv->reg_base + reg);
> +		p += 4;
> +		reg += 4;
> +		count -= 4;
> +	}
> +	if (count & 2) {
> +		iowrite16(*(u16 *)p, priv->reg_base + reg);
> +		p += 2;
> +		reg += 2;
> +	}
> +	if (count & 1)
> +		iowrite8(*p, priv->reg_base + reg);
> +}
> +
>  /*
>   * Check if a CAN controller is present at the specified location
>   * by trying to switch 'em from the Basic mode into the PeliCAN mode.
> @@ -626,6 +666,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
>  		priv->reg_base = addr + cm->offset;
>  		priv->read_reg = plx_pci_read_reg;
>  		priv->write_reg = plx_pci_write_reg;
> +		priv->read_reg_rep = plx_pci_read_reg_rep;
> +		priv->write_reg_rep = plx_pci_write_reg_rep;
>
>  		/* Check if channel is present */
>  		if (plx_pci_check_sja1000(priv)) {
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 9f10779..b26173e 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -288,7 +288,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  	canid_t id;
>  	uint8_t dreg;
>  	u8 cmd_reg_val = 0x00;
> -	int i;

This 'i' needs to be preserved, see below.

> +	u8 id_buf[4];
>
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
> @@ -305,19 +305,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  		fi |= SJA1000_FI_FF;
>  		dreg = SJA1000_EFF_BUF;
>  		priv->write_reg(priv, SJA1000_FI, fi);
> -		priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
> -		priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
> -		priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
> -		priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);

Has to be

if (priv->write_reg_rep) {
	*(u32 *)id_buf = cpu_to_be32(id << 3);
	priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
} else {
	priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
	priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
	priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
	priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
}

at all occurrences of priv->[read|write]_reg_rep().


>  	} else {
>  		dreg = SJA1000_SFF_BUF;
>  		priv->write_reg(priv, SJA1000_FI, fi);
> -		priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
> -		priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
> +		*(u16 *)id_buf = cpu_to_be16(id << 5);
> +		priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);

Like here.

>  	}
>
> -	for (i = 0; i < dlc; i++)
> -		priv->write_reg(priv, dreg++, cf->data[i]);
> +	priv->write_reg_rep(priv, dreg, cf->data, dlc);

dito

>
>  	can_put_echo_skb(skb, dev, 0);
>
> @@ -343,7 +340,7 @@ static void sja1000_rx(struct net_device *dev)
>  	uint8_t fi;
>  	uint8_t dreg;
>  	canid_t id;
> -	int i;
> +	u8 id_buf[4];
>
>  	/* create zero'ed CAN frame buffer */
>  	skb = alloc_can_skb(dev, &cf);
> @@ -355,24 +352,21 @@ static void sja1000_rx(struct net_device *dev)
>  	if (fi & SJA1000_FI_FF) {
>  		/* extended frame format (EFF) */
>  		dreg = SJA1000_EFF_BUF;
> -		id = (priv->read_reg(priv, SJA1000_ID1) << 21)
> -		    | (priv->read_reg(priv, SJA1000_ID2) << 13)
> -		    | (priv->read_reg(priv, SJA1000_ID3) << 5)
> -		    | (priv->read_reg(priv, SJA1000_ID4) >> 3);
> +		priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
> +		id = be32_to_cpu(*(u32 *)id_buf) >> 3;

here

>  		id |= CAN_EFF_FLAG;
>  	} else {
>  		/* standard frame format (SFF) */
>  		dreg = SJA1000_SFF_BUF;
> -		id = (priv->read_reg(priv, SJA1000_ID1) << 3)
> -		    | (priv->read_reg(priv, SJA1000_ID2) >> 5);
> +		priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
> +		id = be16_to_cpu(*(u16 *)id_buf) >> 5;

here

>  	}
>
>  	cf->can_dlc = get_can_dlc(fi & 0x0F);
>  	if (fi & SJA1000_FI_RTR) {
>  		id |= CAN_RTR_FLAG;
>  	} else {
> -		for (i = 0; i < cf->can_dlc; i++)
> -			cf->data[i] = priv->read_reg(priv, dreg++);
> +		priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);

and here.

Additionally:

You always take care of the CPU endian when reading writing CAN 
identifiers. Why is it ok to use read_reg_rep() for the CAN data content 
without any beXX_to_cpu() conversion?

Regards,
Oliver


>  	}
>
>  	cf->can_id = id;
> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> index 9d46398..887045a 100644
> --- a/drivers/net/can/sja1000/sja1000.h
> +++ b/drivers/net/can/sja1000/sja1000.h
> @@ -157,6 +157,10 @@ struct sja1000_priv {
>  	/* the lower-layer is responsible for appropriate locking */
>  	u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
>  	void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
> +	void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
> +	                      void *buffer, unsigned long count);
> +	void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
> +	                       const void *buffer, unsigned long count);
>  	void (*pre_irq) (const struct sja1000_priv *priv);
>  	void (*post_irq) (const struct sja1000_priv *priv);
>
>

Re: [PATCH] can: sja1000: Optimise register accesses

[Nikita Edward Baruzdin]

On Mon, Sep 5, 2016 at 10:56 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hello Nikita,
>
> On 09/05/2016 07:58 PM, Nikita Edward Baruzdin wrote:
>>
>> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
>> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>
>
> I definitely like this patch.
>
>> Thus, this patch introduces plx_pci_read_reg_rep() and
>> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
>> register accesses as much as possible. The functions are then used to
>> read/write CAN ID and payload data to improve driver performance.
>
>
> But there is something more to do as you currently brake all sja1000 drivers
> except plx_pci ;-)
>
> You need to take care of drivers that do not support
> priv->[read|write]_reg_rep().
>
> So that other (pci) drivers can follow at will after testing.

Thanks! I'm sorry for being so sloppy :/
I will resend the patch.

> Additionally:
>
> You always take care of the CPU endian when reading writing CAN identifiers.
> Why is it ok to use read_reg_rep() for the CAN data content without any
> beXX_to_cpu() conversion?

beXX_to_cpu() is there just because CAN ID is stored in device registers in the
big-endian manner. The old code actually does the same byte reordering manually
with shifts.

However, I've now realised that current plx_pci_read/write_reg_rep()
implementations won't work with big-endian CPUs as ioreadXX()/iowriteXX()
assume the driver data needs to be cpu-endian while it really needs to be
little-endian since it is treated as a byte array.

Will it be ok, if I add cpu_to_leXX()/leXX_to_cpu() and change functions like
this? I've checked this is still correct with Intel CPU.

>static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
>                                 void *buffer, unsigned long count)
>{
>    u8 *p = buffer;
>
>    while (count >= 4) {
>        *(u32 *)p = cpu_to_le32(ioread32(priv->reg_base + reg));
>        p += 4;
>        reg += 4;
>        count -= 4;
>    }
>    if (count & 2) {
>        *(u16 *)p = cpu_to_le16(ioread16(priv->reg_base + reg));
>        p += 2;
>        reg += 2;
>    }
>    if (count & 1)
>        *p = ioread8(priv->reg_base + reg);
>}
>
>static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
>                                  const void *buffer, unsigned long count)
>{
>    const u8 *p = buffer;
>
>    while (count >= 4) {
>        iowrite32(le32_to_cpu(*(u32 *)p), priv->reg_base + reg);
>        p += 4;
>        reg += 4;
>        count -= 4;
>    }
>    if (count & 2) {
>        iowrite16(le16_to_cpu(*(u16 *)p), priv->reg_base + reg);
>        p += 2;
>        reg += 2;
>    }
>    if (count & 1)
>        iowrite8(*p, priv->reg_base + reg);
>}

Re: [PATCH] can: sja1000: Optimise register accesses

[Oliver Hartkopp]

On 09/06/2016 06:22 PM, Nikita Edward Baruzdin wrote:
> On Mon, Sep 5, 2016 at 10:56 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>> You always take care of the CPU endian when reading writing CAN identifiers.
>> Why is it ok to use read_reg_rep() for the CAN data content without any
>> beXX_to_cpu() conversion?
>
> beXX_to_cpu() is there just because CAN ID is stored in device registers in the
> big-endian manner. The old code actually does the same byte reordering manually
> with shifts.
>
> However, I've now realised that current plx_pci_read/write_reg_rep()
> implementations won't work with big-endian CPUs as ioreadXX()/iowriteXX()
> assume the driver data needs to be cpu-endian while it really needs to be
> little-endian since it is treated as a byte array.
>
> Will it be ok, if I add cpu_to_leXX()/leXX_to_cpu() and change functions like
> this? I've checked this is still correct with Intel CPU.

I assume this will be ok :-)
Please send an updated version, so that Marc can take a look too.

Best regards,
Oliver