[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
are heavy L2 cache maintenance operations at the same time.

The driver does not perform DMA, so it's safe to use the relaxed version.
>From another side, the relaxed io accessor macros are available on all
architectures now, so we can use the relaxed versions instead.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 23628b7..e279948 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -170,10 +170,10 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 	u32 value;
 
 	if (dev->accessor_flags & ACCESS_16BIT)
-		value = readw(dev->base + offset) |
-			(readw(dev->base + offset + 2) << 16);
+		value = readw_relaxed(dev->base + offset) |
+			(readw_relaxed(dev->base + offset + 2) << 16);
 	else
-		value = readl(dev->base + offset);
+		value = readl_relaxed(dev->base + offset);
 
 	if (dev->accessor_flags & ACCESS_SWAP)
 		return swab32(value);
@@ -187,10 +187,10 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 		b = swab32(b);
 
 	if (dev->accessor_flags & ACCESS_16BIT) {
-		writew((u16)b, dev->base + offset);
-		writew((u16)(b >> 16), dev->base + offset + 2);
+		writew_relaxed((u16)b, dev->base + offset);
+		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
 	} else {
-		writel(b, dev->base + offset);
+		writel_relaxed(b, dev->base + offset);
 	}
 }
 
-- 
2.1.3

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

Dear all,

Is there any issue I need to resolve so that the patch can be merged?

Thanks in advance,
Jisheng

On Wed, 10 Dec 2014 22:26:41 -0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> are heavy L2 cache maintenance operations at the same time.
> 
> The driver does not perform DMA, so it's safe to use the relaxed version.
> From another side, the relaxed io accessor macros are available on all
> architectures now, so we can use the relaxed versions instead.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c index 23628b7..e279948 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -170,10 +170,10 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
>  	u32 value;
>  
>  	if (dev->accessor_flags & ACCESS_16BIT)
> -		value = readw(dev->base + offset) |
> -			(readw(dev->base + offset + 2) << 16);
> +		value = readw_relaxed(dev->base + offset) |
> +			(readw_relaxed(dev->base + offset + 2) << 16);
>  	else
> -		value = readl(dev->base + offset);
> +		value = readl_relaxed(dev->base + offset);
>  
>  	if (dev->accessor_flags & ACCESS_SWAP)
>  		return swab32(value);
> @@ -187,10 +187,10 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int
> offset) b = swab32(b);
>  
>  	if (dev->accessor_flags & ACCESS_16BIT) {
> -		writew((u16)b, dev->base + offset);
> -		writew((u16)(b >> 16), dev->base + offset + 2);
> +		writew_relaxed((u16)b, dev->base + offset);
> +		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
>  	} else {
> -		writel(b, dev->base + offset);
> +		writel_relaxed(b, dev->base + offset);
>  	}
>  }
>  

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Wolfram Sang]

On Fri, Dec 19, 2014 at 10:43:15AM +0800, Jisheng Zhang wrote:
> Dear all,
> 
> Is there any issue I need to resolve so that the patch can be merged?

Adding Mika to the loop. He uses the driver a lot (and knows other
people who do)...


> On Wed, 10 Dec 2014 22:26:41 -0800
> Jisheng Zhang <jszhang@marvell.com> wrote:
> 
> > readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> > This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> > are heavy L2 cache maintenance operations at the same time.
> > 
> > The driver does not perform DMA, so it's safe to use the relaxed version.
> > From another side, the relaxed io accessor macros are available on all
> > architectures now, so we can use the relaxed versions instead.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c index 23628b7..e279948 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -170,10 +170,10 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> >  	u32 value;
> >  
> >  	if (dev->accessor_flags & ACCESS_16BIT)
> > -		value = readw(dev->base + offset) |
> > -			(readw(dev->base + offset + 2) << 16);
> > +		value = readw_relaxed(dev->base + offset) |
> > +			(readw_relaxed(dev->base + offset + 2) << 16);
> >  	else
> > -		value = readl(dev->base + offset);
> > +		value = readl_relaxed(dev->base + offset);
> >  
> >  	if (dev->accessor_flags & ACCESS_SWAP)
> >  		return swab32(value);
> > @@ -187,10 +187,10 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int
> > offset) b = swab32(b);
> >  
> >  	if (dev->accessor_flags & ACCESS_16BIT) {
> > -		writew((u16)b, dev->base + offset);
> > -		writew((u16)(b >> 16), dev->base + offset + 2);
> > +		writew_relaxed((u16)b, dev->base + offset);
> > +		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
> >  	} else {
> > -		writel(b, dev->base + offset);
> > +		writel_relaxed(b, dev->base + offset);
> >  	}
> >  }
> >  
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150113/e21dd833/attachment.sig>

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Mika Westerberg]

On Tue, Jan 13, 2015 at 12:52:05PM +0100, Wolfram Sang wrote:
> On Fri, Dec 19, 2014 at 10:43:15AM +0800, Jisheng Zhang wrote:
> > Dear all,
> > 
> > Is there any issue I need to resolve so that the patch can be merged?
> 
> Adding Mika to the loop. He uses the driver a lot (and knows other
> people who do)...

I don't see problems with this patch.

On x86 we have readl_relaxed() the same as readl() so this patch does
not change anything there.

Feel free to add,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Wolfram Sang]

On Thu, Dec 11, 2014 at 02:26:41PM +0800, Jisheng Zhang wrote:
> readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> are heavy L2 cache maintenance operations at the same time.

Reading this again, I got a question:

Really read/write errors? I would think that there is a performance
penalty because of the memory barriers. But errors?

> The driver does not perform DMA, so it's safe to use the relaxed version.
> From another side, the relaxed io accessor macros are available on all
> architectures now, so we can use the relaxed versions instead.

Can the designware core make use of DMA in theory?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150113/471af9f5/attachment.sig>

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Baruch Siach]

Hi Wolfram,

On Tue, Jan 13, 2015 at 03:36:54PM +0100, Wolfram Sang wrote:
> > The driver does not perform DMA, so it's safe to use the relaxed version.
> > From another side, the relaxed io accessor macros are available on all
> > architectures now, so we can use the relaxed versions instead.
> 
> Can the designware core make use of DMA in theory?

According to the "DesignWare DW_apb_i2c Databook" version I have it can 
(optionally).

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

Dear Wolfram,

On Tue, 13 Jan 2015 06:36:54 -0800
Wolfram Sang <wsa@the-dreams.de> wrote:

> 
> On Thu, Dec 11, 2014 at 02:26:41PM +0800, Jisheng Zhang wrote:
> > readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> > This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> > are heavy L2 cache maintenance operations at the same time.
> 
> Reading this again, I got a question:
> 
> Really read/write errors? I would think that there is a performance
> penalty because of the memory barriers. But errors?

I dunno whether I can call the issue as error. The situation is one i2c client
has a bit strict timing requirement. Without the patch, if there are heavy L2 cache
maintenance operations at the same time, there may be long delay between each
DW_IC_DATA_CMD write opeartions in i2c_dw_xfer_msg() in the DW_IC_INTR_TX_EMPTY isr.

Thus about 1/300 i2c transactions may be ignored by the i2c client per my test.

> 
> > The driver does not perform DMA, so it's safe to use the relaxed version.
> > From another side, the relaxed io accessor macros are available on all
> > architectures now, so we can use the relaxed versions instead.
> 
> Can the designware core make use of DMA in theory?
> 

the IP can do DMA in theory. But currently, there's no DMA support in the driver.

Thanks for your review,
Jisheng

[PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel

[Wolfram Sang]

On Thu, Dec 11, 2014 at 02:26:41PM +0800, Jisheng Zhang wrote:
> readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> are heavy L2 cache maintenance operations at the same time.
> 
> The driver does not perform DMA, so it's safe to use the relaxed version.
> From another side, the relaxed io accessor macros are available on all
> architectures now, so we can use the relaxed versions instead.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Applied to for-next, thanks! Just replaced "errors" with "delays" in the
description.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150123/54ca148c/attachment.sig>