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

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

[Kedareswara rao Appana]

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 <appanad@xilinx.com>
---
 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

[Kedareswara rao Appana]

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 <appanad@xilinx.com>
---
 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

[Sören Brinkmann]

On Thu, 2015-10-22 at 10:16AM +0530, Kedareswara rao Appana wrote:
> 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 <appanad@xilinx.com>
> ---
>  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);

Isn't it possible to move the function and avoid the forward
declaration?

	S?ren

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

[Appana Durga Kedareswara Rao]

Hi Soren,

> -----Original Message-----
> From: S?ren Brinkmann [mailto:soren.brinkmann at xilinx.com]
> Sent: Thursday, October 22, 2015 10:03 PM
> To: Appana Durga Kedareswara Rao
> Cc: Anirudha Sarangi; wg at grandegger.com; mkl at pengutronix.de; Michal
> Simek; Appana Durga Kedareswara Rao; linux-can at vger.kernel.org;
> netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org
> Subject: Re: [PATCH 2/2] can: xilinx: fix bug in bus error handling
> 
> On Thu, 2015-10-22 at 10:16AM +0530, Kedareswara rao Appana wrote:
> > 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 <appanad@xilinx.com>
> > ---
> >  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);
> 
> Isn't it possible to move the function and avoid the forward declaration?

Yes it is possible to move the function will fix it in the next version of the patch.

Regards,
Kedar.

> 
> 	S?ren

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

[Arnd Bergmann]

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 <appanad@xilinx.com>

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

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

[Marc Kleine-Budde]

On 10/22/2015 10:14 AM, Arnd Bergmann wrote:
> 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 <appanad@xilinx.com>
> 
> 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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151022/b2643a57/attachment.sig>

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

[Appana Durga Kedareswara Rao]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
> Sent: Thursday, October 22, 2015 1:52 PM
> To: Arnd Bergmann; linux-arm-kernel at lists.infradead.org
> Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; wg at grandegger.com;
> Michal Simek; Soren Brinkmann; Appana Durga Kedareswara Rao;
> netdev at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
> can at 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:
> > 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 <appanad@xilinx.com>
> >
> > 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?

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   |

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

[Arnd Bergmann]

On Thursday 22 October 2015 10:21:58 Marc Kleine-Budde wrote:
> On 10/22/2015 10:14 AM, Arnd Bergmann wrote:
> > 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 <appanad@xilinx.com>
> > 
> > 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

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

[Marc Kleine-Budde]

On 10/22/2015 10:58 AM, Arnd Bergmann wrote:
>>> 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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151025/b5fa83eb/attachment.sig>

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

[Arnd Bergmann]

On Sunday 25 October 2015, Marc Kleine-Budde wrote:
> On 10/22/2015 10:58 AM, Arnd Bergmann wrote:
> >>> 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

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

[Appana Durga Kedareswara Rao]

Hi Arnd,	

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: Thursday, October 22, 2015 1:45 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; wg at grandegger.com;
> mkl at pengutronix.de; Michal Simek; Soren Brinkmann; Appana Durga
> Kedareswara Rao; netdev at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
> can at 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:
> > 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 <appanad@xilinx.com>
> 
> 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

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

[Arnd Bergmann]

On Thursday 22 October 2015 08:34:53 Appana Durga Kedareswara Rao wrote:
> > 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 <appanad@xilinx.com>
> > 
> > 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.

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

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

[Michal Simek]

On 10/22/2015 11:02 AM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 08:34:53 Appana Durga Kedareswara Rao wrote:
>>> 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 <appanad@xilinx.com>
>>>
>>> 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