| Message ID | 1418279201-3886-1-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > } >
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); > > } > > } > > >
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>
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?
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
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
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.
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); } }
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(-)