[PATCH 0/3] Add __ioread32_copy() and use it

[PATCH 0/3] Add __ioread32_copy() and use it

[Stephen Boyd]

The SMD driver is reading and writing chunks of data to iomem,
and there's an __iowrite32_copy() function for the writing part, but
no __ioread32_copy() function for the reading part. This series
adds __ioread32_copy() and uses it in two places. Andrew is on Cc in
case this should go through the -mm tree. Otherwise the target
of this patch series is SMD, so I've sent it to Andy.

Stephen Boyd (3):
  lib: iomap_copy: Add __ioread32_copy()
  soc: qcom: smd: Use __ioread32_copy() instead of open-coding it
  FIRMWARE: bcm47xx_nvram: Use __ioread32_copy() instead of open-coding

 drivers/firmware/broadcom/bcm47xx_nvram.c | 11 +++--------
 drivers/soc/qcom/smd.c                    | 13 ++++---------
 include/linux/io.h                        |  1 +
 lib/iomap_copy.c                          | 23 +++++++++++++++++++++++
 4 files changed, 31 insertions(+), 17 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] lib: iomap_copy: Add __ioread32_copy()

[Stephen Boyd]

Some drivers need to read data out of iomem areas 32-bits at a
time. Add an API to do this.

Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 include/linux/io.h |  1 +
 lib/iomap_copy.c   | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/io.h b/include/linux/io.h
index de64c1e53612..72c35e0a41d1 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -29,6 +29,7 @@ struct device;
 struct resource;
 
 __visible void __iowrite32_copy(void __iomem *to, const void *from, size_t count);
+void __ioread32_copy(void *to, const void __iomem *from, size_t count);
 void __iowrite64_copy(void __iomem *to, const void *from, size_t count);
 
 #ifdef CONFIG_MMU
diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
index 4527e751b5e0..f174d4759ef3 100644
--- a/lib/iomap_copy.c
+++ b/lib/iomap_copy.c
@@ -42,6 +42,29 @@ void __attribute__((weak)) __iowrite32_copy(void __iomem *to,
 EXPORT_SYMBOL_GPL(__iowrite32_copy);
 
 /**
+ * __ioread32_copy - copy data from MMIO space, in 32-bit units
+ * @to: destination (must be 32-bit aligned)
+ * @from: source, in MMIO space (must be 32-bit aligned)
+ * @count: number of 32-bit quantities to copy
+ *
+ * Copy data from MMIO space to kernel space, in units of 32 bits at a
+ * time.  Order of access is not guaranteed, nor is a memory barrier
+ * performed afterwards.
+ */
+void __attribute__((weak)) __ioread32_copy(void *to,
+					   const void __iomem *from,
+					   size_t count)
+{
+	u32 *dst = to;
+	const u32 __iomem *src = from;
+	const u32 __iomem *end = src + count;
+
+	while (src < end)
+		*dst++ = __raw_readl(src++);
+}
+EXPORT_SYMBOL_GPL(__ioread32_copy);
+
+/**
  * __iowrite64_copy - copy data to MMIO space, in 64-bit or 32-bit units
  * @to: destination, in MMIO space (must be 64-bit aligned)
  * @from: source (must be 64-bit aligned)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] soc: qcom: smd: Use __ioread32_copy() instead of open-coding it

[Stephen Boyd]

Now that we have a generic library function for this, replace the
open-coded instance.

Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

Note this patch relies on a previous patch on the list that
changes the readl() to __raw_readl()[1].

 drivers/soc/qcom/smd.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/qcom/smd.c b/drivers/soc/qcom/smd.c
index e2f85a714243..e81eac28b5ce 100644
--- a/drivers/soc/qcom/smd.c
+++ b/drivers/soc/qcom/smd.c
@@ -434,20 +434,15 @@ static void smd_copy_to_fifo(void __iomem *dst,
 /*
  * Copy count bytes of data using 32bit accesses, if that is required.
  */
-static void smd_copy_from_fifo(void *_dst,
-			       const void __iomem *_src,
+static void smd_copy_from_fifo(void *dst,
+			       const void __iomem *src,
 			       size_t count,
 			       bool word_aligned)
 {
-	u32 *dst = (u32 *)_dst;
-	u32 *src = (u32 *)_src;
-
 	if (word_aligned) {
-		count /= sizeof(u32);
-		while (count--)
-			*dst++ = __raw_readl(src++);
+		__ioread32_copy(dst, src, count / sizeof(u32));
 	} else {
-		memcpy_fromio(_dst, _src, count);
+		memcpy_fromio(dst, src, count);
 	}
 }
 
-- 

[1] http://lkml.kernel.org/g/1441234011-4259-7-git-send-email-sboyd@codeaurora.org

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] FIRMWARE: bcm47xx_nvram: Use __ioread32_copy() instead of open-coding

[Stephen Boyd]

Now that we have a generic library function for this, replace the
open-coded instance.

Cc: Hauke Mehrtens <hauke@hauke-m.de>
Cc: Rafał Miłecki <zajec5@gmail.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: <linux-mips@linux-mips.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index e41594510b97..8f46e6e394b1 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -56,9 +56,7 @@ static u32 find_nvram_size(void __iomem *end)
 static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
 {
 	struct nvram_header __iomem *header;
-	int i;
 	u32 off;
-	u32 *src, *dst;
 	u32 size;
 
 	if (nvram_len) {
@@ -95,10 +93,7 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
 	return -ENXIO;
 
 found:
-	src = (u32 *)header;
-	dst = (u32 *)nvram_buf;
-	for (i = 0; i < sizeof(struct nvram_header); i += 4)
-		*dst++ = __raw_readl(src++);
+	__ioread32_copy(nvram_buf, header, sizeof(*header) / 4);
 	header = (struct nvram_header *)nvram_buf;
 	nvram_len = header->len;
 	if (nvram_len > size) {
@@ -111,8 +106,8 @@ found:
 		nvram_len = NVRAM_SPACE - 1;
 	}
 	/* proceed reading data after header */
-	for (; i < nvram_len; i += 4)
-		*dst++ = readl(src++);
+	__ioread32_copy(nvram_buf + sizeof(*header), header + 1,
+			DIV_ROUND_UP(nvram_len / 4));
 	nvram_buf[NVRAM_SPACE - 1] = '\0';
 
 	return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andrew Morton]

On Tue, 15 Sep 2015 12:41:26 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:

> The SMD driver is reading and writing chunks of data to iomem,
> and there's an __iowrite32_copy() function for the writing part, but
> no __ioread32_copy() function for the reading part. This series
> adds __ioread32_copy() and uses it in two places. Andrew is on Cc in
> case this should go through the -mm tree. Otherwise the target
> of this patch series is SMD, so I've sent it to Andy.

"soc: qcom: smd: Use __ioread32_copy() instead of open-coding it" no
longer applies, because smd_copy_from_fifo() has switched to
readl_relaxed().

Let's use the __weak macro rather than open-coding it (and convert
__iowrite32_copy() while we're in there).

It's unclear why __iowrite32_copy() is a weak function - nothing
overrides it.  Perhaps we should just take that away rather than
copying it into __ioread32_copy().

__iowrite32_copy() is marked __visible.  I don't actually know what
that does and Andi's d47d5c8194579bc changelog (which sucks the big
one) didn't explain it.  Apparently it has something to do with being
implemented in assembly, but zillions of functions are implemented in
assembly, so why are only two functions marked this way?  Anyway,
__ioread32_copy() is implemented in C so I guess __visible isn't needed
there.

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Stephen Boyd]

On 09/15, Andrew Morton wrote:
> On Tue, 15 Sep 2015 12:41:26 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > The SMD driver is reading and writing chunks of data to iomem,
> > and there's an __iowrite32_copy() function for the writing part, but
> > no __ioread32_copy() function for the reading part. This series
> > adds __ioread32_copy() and uses it in two places. Andrew is on Cc in
> > case this should go through the -mm tree. Otherwise the target
> > of this patch series is SMD, so I've sent it to Andy.
> 
> "soc: qcom: smd: Use __ioread32_copy() instead of open-coding it" no
> longer applies, because smd_copy_from_fifo() has switched to
> readl_relaxed().

Yes. There are some other patches in flight on the mailing list
to this file from me[1]. Those would need to be applied first to
avoid conflicts.

> 
> Let's use the __weak macro rather than open-coding it (and convert
> __iowrite32_copy() while we're in there).

Yep, I converted the __iowrite32_copy() open-code in there too in
the patch series I mentioned above. See [2].

> 
> It's unclear why __iowrite32_copy() is a weak function - nothing
> overrides it.  Perhaps we should just take that away rather than
> copying it into __ioread32_copy().

Huh? I see that x86 has an implementation in arch/x86/lib/iomap_copy_64.S

> 
> __iowrite32_copy() is marked __visible.  I don't actually know what
> that does and Andi's d47d5c8194579bc changelog (which sucks the big
> one) didn't explain it.  Apparently it has something to do with being
> implemented in assembly, but zillions of functions are implemented in
> assembly, so why are only two functions marked this way?  Anyway,
> __ioread32_copy() is implemented in C so I guess __visible isn't needed
> there.

Yeah, I didn't add visible because there isn't an assembly
version of __ioread32_copy() so far. I can remove __weak if
desired. I left it there to match __iowrite32_copy() in case
x86 wanted to override it but we can do that later or never.

[1] http://lkml.kernel.org/g/1441234011-4259-7-git-send-email-sboyd@codeaurora.org
[2] http://lkml.kernel.org/g/1441234011-4259-5-git-send-email-sboyd@codeaurora.org

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andrew Morton]

On Tue, 15 Sep 2015 17:50:53 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:

> > 
> > __iowrite32_copy() is marked __visible.  I don't actually know what
> > that does and Andi's d47d5c8194579bc changelog (which sucks the big
> > one) didn't explain it.  Apparently it has something to do with being
> > implemented in assembly, but zillions of functions are implemented in
> > assembly, so why are only two functions marked this way?  Anyway,
> > __ioread32_copy() is implemented in C so I guess __visible isn't needed
> > there.
> 
> Yeah, I didn't add visible because there isn't an assembly
> version of __ioread32_copy() so far. I can remove __weak if
> desired. I left it there to match __iowrite32_copy() in case
> x86 wanted to override it but we can do that later or never.

OK.  I'd omit the __weak and __visible for now.  They can be added later
if someone needs them.

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andi Kleen]

> __iowrite32_copy() is marked __visible.  I don't actually know what
> that does and Andi's d47d5c8194579bc changelog (which sucks the big
> one) didn't explain it.  Apparently it has something to do with being
> implemented in assembly, but zillions of functions are implemented in
> assembly, so why are only two functions marked this way?  Anyway,
> __ioread32_copy() is implemented in C so I guess __visible isn't needed
> there.

__visible is needed for C functions that are called from assembler.
Otherwise the compiler may optimize them away.

-Andi

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andrew Morton]

On Wed, 16 Sep 2015 04:32:19 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> > __iowrite32_copy() is marked __visible.  I don't actually know what
> > that does and Andi's d47d5c8194579bc changelog (which sucks the big
> > one) didn't explain it.  Apparently it has something to do with being
> > implemented in assembly, but zillions of functions are implemented in
> > assembly, so why are only two functions marked this way?  Anyway,
> > __ioread32_copy() is implemented in C so I guess __visible isn't needed
> > there.
> 
> __visible is needed for C functions that are called from assembler.
> Otherwise the compiler may optimize them away.

Under what circumstances will the compiler (or linker?) do this? 
LTO enabled?

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andi Kleen]

> Under what circumstances will the compiler (or linker?) do this? 

Compiler.

> LTO enabled?

Yes it's for LTO.  The optimization allows the compiler to drop unused
functions, which is very popular with users (a lot use it to get smaller
kernel images)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andrew Morton]

On Wed, 16 Sep 2015 04:55:46 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> > Under what circumstances will the compiler (or linker?) do this? 
> 
> Compiler.
> 
> > LTO enabled?
> 
> Yes it's for LTO.  The optimization allows the compiler to drop unused
> functions, which is very popular with users (a lot use it to get smaller
> kernel images)
> 

Does this look truthful and complete?


--- a/include/linux/compiler-gcc.h~a
+++ a/include/linux/compiler-gcc.h
@@ -205,7 +205,10 @@
 
 #if GCC_VERSION >= 40600
 /*
- * Tell the optimizer that something else uses this function or variable.
+ * When used with Link Time Optimization, gcc can optimize away C functions or
+ * variables which are referenced only from assembly code.  __visible tells the
+ * optimizer that something else uses this function or variable, thus preventing
+ * this.
  */
 #define __visible	__attribute__((externally_visible))
 #endif
_

Re: [PATCH 0/3] Add __ioread32_copy() and use it

[Andi Kleen]

On Fri, Sep 18, 2015 at 12:19:19PM -0700, Andrew Morton wrote:
> On Wed, 16 Sep 2015 04:55:46 +0200 Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > Under what circumstances will the compiler (or linker?) do this? 
> > 
> > Compiler.
> > 
> > > LTO enabled?
> > 
> > Yes it's for LTO.  The optimization allows the compiler to drop unused
> > functions, which is very popular with users (a lot use it to get smaller
> > kernel images)
> > 
> 
> Does this look truthful and complete?
> 
> 
> --- a/include/linux/compiler-gcc.h~a
> +++ a/include/linux/compiler-gcc.h
> @@ -205,7 +205,10 @@
>  
>  #if GCC_VERSION >= 40600
>  /*
> - * Tell the optimizer that something else uses this function or variable.
> + * When used with Link Time Optimization, gcc can optimize away C functions or
> + * variables which are referenced only from assembly code.  __visible tells the
> + * optimizer that something else uses this function or variable, thus preventing
> + * this.

Yes,

In a few cases I also used it to work around LTO bugs in older gcc
releases. I don't think any of those fixes made it into mainline though,
and they are not needed anymore with 5.x

-Andi