Re: [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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



signature.asc
Description: OpenPGP digital signature

RE: [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:m...@pengutronix.de]
> Sent: Thursday, October 22, 2015 1:52 PM
> To: Arnd Bergmann; linux-arm-ker...@lists.infradead.org
> Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; w...@grandegger.com;
> Michal Simek; Soren Brinkmann; Appana Durga Kedareswara Rao;
> netdev@vger.kernel.org; linux-ker...@vger.kernel.org; linux-
> c...@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 <appa...@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- |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Re: [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 
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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 

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Appana Durga Kedareswara Rao]

Hi Arnd,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 22, 2015 1:45 PM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Appana Durga Kedareswara Rao; Anirudha Sarangi; w...@grandegger.com;
> m...@pengutronix.de; Michal Simek; Soren Brinkmann; Appana Durga
> Kedareswara Rao; netdev@vger.kernel.org; linux-ker...@vger.kernel.org; linux-
> c...@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 <appa...@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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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 
> 
> 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- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



signature.asc
Description: OpenPGP digital signature

Re: [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 
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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 
>>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html