[PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

Subsystems: can network drivers, the rest, xilinx can driver

STALE3972d

13 messages, 4 authors, 2015-10-26 · open the first message on its own page

[PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Kedareswara rao Appana <hidden>
Date: 2015-10-22 05:01:19

The driver only supports memory-mapped I/O [by ioremap()],
so readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with iowrite
Putting the barriers for each tx fifo register write fixes this issue
Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>
---
 drivers/net/can/xilinx_can.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6114214..055d6f3 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c

@@ -170,7 +170,7 @@ static const struct can_bittiming_const xcan_bittiming_const = {
 static void xcan_write_reg_le(const struct xcan_priv *priv, enum xcan_reg reg,
 			u32 val)
 {
-	iowrite32(val, priv->reg_base + reg);
+	writel(val, priv->reg_base + reg);
 }
 
 /**

@@ -183,7 +183,7 @@ static void xcan_write_reg_le(const struct xcan_priv *priv, enum xcan_reg reg,
  */
 static u32 xcan_read_reg_le(const struct xcan_priv *priv, enum xcan_reg reg)
 {
-	return ioread32(priv->reg_base + reg);
+	return readl(priv->reg_base + reg);
 }
 
 /**

-- 
2.1.2

[PATCH 2/2] can: xilinx: fix bug in bus error handling

From: Kedareswara rao Appana <hidden>
Date: 2015-10-22 04:46:03

Simply resetting the peripheral on bus off condition is not enough,
Because we also need to re-initialize the whole device.
This patch fixes this issue.

Signed-off-by: Kedareswara rao Appana <redacted>
---
 drivers/net/can/xilinx_can.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 055d6f3..9aeb85f 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c

@@ -529,6 +529,8 @@ static int xcan_rx(struct net_device *ndev)
 	return 1;
 }
 
+static void xcan_chip_stop(struct net_device *ndev);
+
 /**
  * xcan_err_interrupt - error frame Isr
  * @ndev:	net_device pointer

@@ -558,8 +560,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	if (isr & XCAN_IXR_BSOFF_MASK) {
 		priv->can.state = CAN_STATE_BUS_OFF;
 		priv->can.can_stats.bus_off++;
-		/* Leave device in Config Mode in bus-off state */
-		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
+		/* Re-initialize the whole device in bus-off state */
+		xcan_chip_stop(ndev);
+		xcan_chip_start(ndev);
 		can_bus_off(ndev);
 		if (skb)
 			cf->can_id |= CAN_ERR_BUSOFF;

-- 
2.1.2

[PATCH 2/2] can: xilinx: fix bug in bus error handling

From: Kedareswara rao Appana <hidden>
Date: 2015-10-22 04:46:03

Simply resetting the peripheral on bus off condition is not enough,
Because we also need to re-initialize the whole device.
This patch fixes this issue.

Signed-off-by: Kedareswara rao Appana <redacted>
---
 drivers/net/can/xilinx_can.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 055d6f3..9aeb85f 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c

@@ -529,6 +529,8 @@ static int xcan_rx(struct net_device *ndev)
 	return 1;
 }
 
+static void xcan_chip_stop(struct net_device *ndev);
+
 /**
  * xcan_err_interrupt - error frame Isr
  * @ndev:	net_device pointer

@@ -558,8 +560,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	if (isr & XCAN_IXR_BSOFF_MASK) {
 		priv->can.state = CAN_STATE_BUS_OFF;
 		priv->can.can_stats.bus_off++;
-		/* Leave device in Config Mode in bus-off state */
-		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
+		/* Re-initialize the whole device in bus-off state */
+		xcan_chip_stop(ndev);
+		xcan_chip_start(ndev);
 		can_bus_off(ndev);
 		if (skb)
 			cf->can_id |= CAN_ERR_BUSOFF;

-- 
2.1.2

[PATCH 2/2] can: xilinx: fix bug in bus error handling

From: Kedareswara rao Appana <hidden>
Date: 2015-10-22 05:02:24

Simply resetting the peripheral on bus off condition is not enough,
Because we also need to re-initialize the whole device.
This patch fixes this issue.

Signed-off-by: Kedareswara rao Appana <redacted>
---
 drivers/net/can/xilinx_can.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 055d6f3..9aeb85f 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c

@@ -529,6 +529,8 @@ static int xcan_rx(struct net_device *ndev)
 	return 1;
 }
 
+static void xcan_chip_stop(struct net_device *ndev);
+
 /**
  * xcan_err_interrupt - error frame Isr
  * @ndev:	net_device pointer

@@ -558,8 +560,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	if (isr & XCAN_IXR_BSOFF_MASK) {
 		priv->can.state = CAN_STATE_BUS_OFF;
 		priv->can.can_stats.bus_off++;
-		/* Leave device in Config Mode in bus-off state */
-		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
+		/* Re-initialize the whole device in bus-off state */
+		xcan_chip_stop(ndev);
+		xcan_chip_start(ndev);
 		can_bus_off(ndev);
 		if (skb)
 			cf->can_id |= CAN_ERR_BUSOFF;

-- 
2.1.2

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Arnd Bergmann <arnd@arndb.de>
Date: 2015-10-22 08:15:10

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

The driver only supports memory-mapped I/O [by ioremap()],
so readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with iowrite
Putting the barriers for each tx fifo register write fixes this issue
Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline
calling writel() on both ARM32 and ARM64. On which kernel version did you
observe the difference? It's possible that an older version used
CONFIG_GENERIC_IOMAP, which made this slightly more expensive.

If there are barriers that you want to get rid of for performance reasons,
you should use writel_relaxed(), but be careful to synchronize them correctly
with regard to DMA. It should be fine in this driver, as it does not
perform any DMA, but be aware that there is no big-endian version of
writel_relaxed() at the moment.

	Arnd

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: 2015-10-22 08:22:17

On 10/22/2015 10:14 AM, Arnd Bergmann wrote:

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()],
so readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with iowrite
Putting the barriers for each tx fifo register write fixes this issue
Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline
calling writel() on both ARM32 and ARM64. On which kernel version did you
observe the difference? It's possible that an older version used
CONFIG_GENERIC_IOMAP, which made this slightly more expensive.

If there are barriers that you want to get rid of for performance reasons,
you should use writel_relaxed(), but be careful to synchronize them correctly
with regard to DMA. It should be fine in this driver, as it does not
perform any DMA, but be aware that there is no big-endian version of
writel_relaxed() at the moment.

We don't have DMA in CAN drivers, but usually a certain write triggers
sending. Do we need a barrier before triggering the sending?

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   |

RE: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Appana Durga Kedareswara Rao <hidden>
Date: 2015-10-22 08:35:06

Hi Arnd,

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de]
Sent: Thursday, October 22, 2015 1:45 PM
To: linux-arm-kernel@lists.infradead.org
Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; wg@grandegger.com;
mkl@pengutronix.de; Michal Simek; Soren Brinkmann; Appana Durga
Kedareswara Rao; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
can@vger.kernel.org
Subject: Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()], so
readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with
iowrite Putting the barriers for each tx fifo register write fixes
this issue Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline calling
writel() on both ARM32 and ARM64. On which kernel version did you observe the
difference? It's possible that an older version used CONFIG_GENERIC_IOMAP,
which made this slightly more expensive.

I observed this issue with the 4.0.0 kernel version

If there are barriers that you want to get rid of for performance reasons, you
should use writel_relaxed(), but be careful to synchronize them correctly with
regard to DMA. It should be fine in this driver, as it does not perform any DMA,
but be aware that there is no big-endian version of
writel_relaxed() at the moment.

There is no DMA in CAN for this IP.

Regards,
Kedar.

	Arnd

RE: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Appana Durga Kedareswara Rao <hidden>
Date: 2015-10-22 08:40:08

Hi Marc,

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Thursday, October 22, 2015 1:52 PM
To: Arnd Bergmann; linux-arm-kernel@lists.infradead.org
Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; wg@grandegger.com;
Michal Simek; Soren Brinkmann; Appana Durga Kedareswara Rao;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
can@vger.kernel.org
Subject: Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

On 10/22/2015 10:14 AM, Arnd Bergmann wrote:

quoted

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()], so
readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with
iowrite Putting the barriers for each tx fifo register write fixes
this issue Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static
inline calling writel() on both ARM32 and ARM64. On which kernel
version did you observe the difference? It's possible that an older
version used CONFIG_GENERIC_IOMAP, which made this slightly more

expensive.

quoted

If there are barriers that you want to get rid of for performance
reasons, you should use writel_relaxed(), but be careful to
synchronize them correctly with regard to DMA. It should be fine in
this driver, as it does not perform any DMA, but be aware that there
is no big-endian version of
writel_relaxed() at the moment.

We don't have DMA in CAN drivers, but usually a certain write triggers sending.
Do we need a barrier before triggering the sending?

Yes During validation of this IP on ARM 64 bit processor with using iowrite32() and sending a lot of packets it requires barriers before triggering the send.
With using writel() barriers are not needed.

Regards,
Kedar.

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   |

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Arnd Bergmann <arnd@arndb.de>
Date: 2015-10-22 08:58:45

On Thursday 22 October 2015 10:21:58 Marc Kleine-Budde wrote:

On 10/22/2015 10:14 AM, Arnd Bergmann wrote:

quoted

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()],
so readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with iowrite
Putting the barriers for each tx fifo register write fixes this issue
Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline
calling writel() on both ARM32 and ARM64. On which kernel version did you
observe the difference? It's possible that an older version used
CONFIG_GENERIC_IOMAP, which made this slightly more expensive.

If there are barriers that you want to get rid of for performance reasons,
you should use writel_relaxed(), but be careful to synchronize them correctly
with regard to DMA. It should be fine in this driver, as it does not
perform any DMA, but be aware that there is no big-endian version of
writel_relaxed() at the moment.

We don't have DMA in CAN drivers, but usually a certain write triggers
sending. Do we need a barrier before triggering the sending?

No, the relaxed writes are not well-defined across architectures. On
ARM, the CPU guarantees that stores to an MMIO area are still in order
with respect to one another, the barrier is only needed for actual DMA,
so you are fine. I would expect the same to be true everywhere,
otherwise a lot of other drivers would be broken too.

To be on the safe side, that last write() could remain a writel() instead
of writel_relaxed(), and that would be guaranteed to work on all
architectures even if they end relax the ordering between MMIO writes.
If there is a measurable performance difference, just use writel_relaxed()
and add a comment.

	Arnd

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Arnd Bergmann <arnd@arndb.de>
Date: 2015-10-22 09:02:48

On Thursday 22 October 2015 08:34:53 Appana Durga Kedareswara Rao wrote:

quoted

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()], so
readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with
iowrite Putting the barriers for each tx fifo register write fixes
this issue Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline calling
writel() on both ARM32 and ARM64. On which kernel version did you observe the
difference? It's possible that an older version used CONFIG_GENERIC_IOMAP,
which made this slightly more expensive.

I observed this issue with the 4.0.0 kernel version

Is it possible that you have nonstandard patches on your kernel? If so, can
you send a diff against the mainline version?

I don't see CONFIG_GENERIC_IOMAP in 4.0.0, and writel() definitely has the
necessary barriers on arm64, the same way that iowrite() does.

quoted

If there are barriers that you want to get rid of for performance reasons, you
should use writel_relaxed(), but be careful to synchronize them correctly with
regard to DMA. It should be fine in this driver, as it does not perform any DMA,
but be aware that there is no big-endian version of
writel_relaxed() at the moment.

There is no DMA in CAN for this IP.

Ok, good.

	Arnd

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Michal Simek <hidden>
Date: 2015-10-22 10:05:07

On 10/22/2015 11:02 AM, Arnd Bergmann wrote:

On Thursday 22 October 2015 08:34:53 Appana Durga Kedareswara Rao wrote:

quoted

On Thursday 22 October 2015 10:16:02 Kedareswara rao Appana wrote:

quoted

The driver only supports memory-mapped I/O [by ioremap()], so
readl/writel is actually the right thing to do, IMO.
During the validation of this driver or IP on ARM 64-bit processor
while sending lot of packets observed that the tx packet drop with
iowrite Putting the barriers for each tx fifo register write fixes
this issue Instead of barriers using writel also fixed this issue.

Signed-off-by: Kedareswara rao Appana <redacted>

The two should really do the same thing: iowrite32() is just a static inline calling
writel() on both ARM32 and ARM64. On which kernel version did you observe the
difference? It's possible that an older version used CONFIG_GENERIC_IOMAP,
which made this slightly more expensive.

I observed this issue with the 4.0.0 kernel version

Is it possible that you have nonstandard patches on your kernel? If so, can
you send a diff against the mainline version?

Kedar: Can you please retest this on mainline kernel?
We shouldn't have any patches which should influence this.

Thanks,
Michal

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: 2015-10-25 20:32:27

On 10/22/2015 10:58 AM, Arnd Bergmann wrote:

quoted

The two should really do the same thing: iowrite32() is just a static inline
calling writel() on both ARM32 and ARM64. On which kernel version did you
observe the difference? It's possible that an older version used
CONFIG_GENERIC_IOMAP, which made this slightly more expensive.

If there are barriers that you want to get rid of for performance reasons,
you should use writel_relaxed(), but be careful to synchronize them correctly
with regard to DMA. It should be fine in this driver, as it does not
perform any DMA, but be aware that there is no big-endian version of
writel_relaxed() at the moment.

We don't have DMA in CAN drivers, but usually a certain write triggers
sending. Do we need a barrier before triggering the sending?

No, the relaxed writes are not well-defined across architectures. On
ARM, the CPU guarantees that stores to an MMIO area are still in order
with respect to one another, the barrier is only needed for actual DMA,
so you are fine. I would expect the same to be true everywhere,
otherwise a lot of other drivers would be broken too.

And the relaxed functions seem not to be available on all archs. This
driver should work on microblaze. Are __raw_writeX(), __raw_readX() an
alternative here?

To be on the safe side, that last write() could remain a writel() instead
of writel_relaxed(), and that would be guaranteed to work on all
architectures even if they end relax the ordering between MMIO writes.
If there is a measurable performance difference, just use writel_relaxed()
and add a comment.

Thanks,
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   |

Re: [PATCH 1/2] can: xilinx: use readl/writel instead of ioread/iowrite

From: Arnd Bergmann <arnd@arndb.de>
Date: 2015-10-26 01:26:13

On Sunday 25 October 2015, Marc Kleine-Budde wrote:

On 10/22/2015 10:58 AM, Arnd Bergmann wrote:

quoted

The two should really do the same thing: iowrite32() is just a static inline
calling writel() on both ARM32 and ARM64. On which kernel version did you
observe the difference? It's possible that an older version used
CONFIG_GENERIC_IOMAP, which made this slightly more expensive.

If there are barriers that you want to get rid of for performance reasons,
you should use writel_relaxed(), but be careful to synchronize them correctly
with regard to DMA. It should be fine in this driver, as it does not
perform any DMA, but be aware that there is no big-endian version of
writel_relaxed() at the moment.

We don't have DMA in CAN drivers, but usually a certain write triggers
sending. Do we need a barrier before triggering the sending?

No, the relaxed writes are not well-defined across architectures. On
ARM, the CPU guarantees that stores to an MMIO area are still in order
with respect to one another, the barrier is only needed for actual DMA,
so you are fine. I would expect the same to be true everywhere,
otherwise a lot of other drivers would be broken too.

And the relaxed functions seem not to be available on all archs. This
driver should work on microblaze. Are __raw_writeX(), __raw_readX() an
alternative here?

__raw_writeX() and __raw_readX()  are not safe to use in drivers in general.

readl_relaxed() should work on all architectures nowadays, and I've checked
that it does on microblaze.

	Arnd

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help