[PATCH v3 0/5] hisi_lpc: Improve build test coverage

[PATCH v3 0/5] hisi_lpc: Improve build test coverage

[John Garry]

This series aims to improve the build test cover of the driver by
supporting building under COMPILE_TEST.

I also included "lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops
are set at registration" as it was never picked up for 5.4.

Two new patches are also included since v1:
- clean issues detected by sparse
- build logic_pio.o into /lib library

Since v2 I limited test coverage for archs which don't define
{read, write}sb().

John Garry (5):
  lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at
    registration
  logic_pio: Define PIO_INDIRECT_SIZE for !CONFIG_INDIRECT_PIO
  bus: hisi_lpc: Clean some types
  bus: hisi_lpc: Expand build test coverage
  logic_pio: Build into a library

 drivers/bus/Kconfig       |  5 +++--
 drivers/bus/hisi_lpc.c    |  9 ++++-----
 include/linux/logic_pio.h |  4 ++--
 lib/Makefile              |  2 +-
 lib/logic_pio.c           | 14 ++++++++------
 5 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at registration

[John Garry]

Since the only LOGIC_PIO_INDIRECT host (hisi-lpc) now sets the ops prior
to registration, enforce this check for accessors ops at registration
instead of in the IO port accessors to simplify and marginally optimise
the code.

A slight misalignment is also tidied.

Also add myself as an author.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 905027574e5d..f511a99bb389 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved.
  * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
  * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: John Garry <john.garry@huawei.com>
  */
 
 #define pr_fmt(fmt)	"LOGIC PIO: " fmt
@@ -39,7 +40,8 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
 	int ret = 0;
 
-	if (!new_range || !new_range->fwnode || !new_range->size)
+	if (!new_range || !new_range->fwnode || !new_range->size ||
+	    (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops))
 		return -EINVAL;
 
 	start = new_range->hw_start;
@@ -237,7 +239,7 @@ type logic_in##bw(unsigned long addr)					\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			ret = entry->ops->in(entry->hostdata,		\
 					addr, sizeof(type));		\
 		else							\
@@ -253,7 +255,7 @@ void logic_out##bw(type value, unsigned long addr)			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->out(entry->hostdata,		\
 					addr, value, sizeof(type));	\
 		else							\
@@ -261,7 +263,7 @@ void logic_out##bw(type value, unsigned long addr)			\
 	}								\
 }									\
 									\
-void logic_ins##bw(unsigned long addr, void *buffer,		\
+void logic_ins##bw(unsigned long addr, void *buffer,			\
 		   unsigned int count)					\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
@@ -269,7 +271,7 @@ void logic_ins##bw(unsigned long addr, void *buffer,		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->ins(entry->hostdata,		\
 				addr, buffer, sizeof(type), count);	\
 		else							\
@@ -286,7 +288,7 @@ void logic_outs##bw(unsigned long addr, const void *buffer,		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->outs(entry->hostdata,		\
 				addr, buffer, sizeof(type), count);	\
 		else							\
-- 
2.17.1

[PATCH v3 2/5] logic_pio: Define PIO_INDIRECT_SIZE for !CONFIG_INDIRECT_PIO

[John Garry]

With the goal of expanding the test coverage of the HiSi LPC driver to
!ARM64, define a dummy PIO_INDIRECT_SIZE for !CONFIG_INDIRECT_PIO, which
is required by the named driver.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/logic_pio.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
index 88e1e6304a71..54945aa824b4 100644
--- a/include/linux/logic_pio.h
+++ b/include/linux/logic_pio.h
@@ -108,10 +108,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
  * area by redefining the macro below.
  */
 #define PIO_INDIRECT_SIZE 0x4000
-#define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE)
 #else
-#define MMIO_UPPER_LIMIT IO_SPACE_LIMIT
+#define PIO_INDIRECT_SIZE 0
 #endif /* CONFIG_INDIRECT_PIO */
+#define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE)
 
 struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
 unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
-- 
2.17.1

[PATCH v3 3/5] bus: hisi_lpc: Clean some types

[John Garry]

Sparse complains of these:
drivers/bus/hisi_lpc.c:82:38: warning: incorrect type in argument 1 (different address spaces)
drivers/bus/hisi_lpc.c:82:38:    expected void const volatile [noderef] <asn:2>*addr
drivers/bus/hisi_lpc.c:82:38:    got unsigned char *
drivers/bus/hisi_lpc.c:131:35: warning: incorrect type in argument 1 (different address spaces)
drivers/bus/hisi_lpc.c:131:35:    expected unsigned char *mbase
drivers/bus/hisi_lpc.c:131:35:    got void [noderef] <asn:2>*membase
drivers/bus/hisi_lpc.c:186:35: warning: incorrect type in argument 1 (different address spaces)
drivers/bus/hisi_lpc.c:186:35:    expected unsigned char *mbase
drivers/bus/hisi_lpc.c:186:35:    got void [noderef] <asn:2>*membase
drivers/bus/hisi_lpc.c:228:16: warning: cast to restricted __le32
drivers/bus/hisi_lpc.c:251:13: warning: incorrect type in assignment (different base types)
drivers/bus/hisi_lpc.c:251:13:    expected unsigned int [unsigned] [usertype] val
drivers/bus/hisi_lpc.c:251:13:    got restricted __le32 [usertype] <noident>

Clean them up.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 20c957185af2..8101df901830 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -74,7 +74,7 @@ struct hisi_lpc_dev {
 /* About 10us. This is specific for single IO operations, such as inb */
 #define LPC_PEROP_WAITCNT	100
 
-static int wait_lpc_idle(unsigned char *mbase, unsigned int waitcnt)
+static int wait_lpc_idle(void __iomem *mbase, unsigned int waitcnt)
 {
 	u32 status;
 
@@ -209,7 +209,7 @@ static u32 hisi_lpc_comm_in(void *hostdata, unsigned long pio, size_t dwidth)
 	struct hisi_lpc_dev *lpcdev = hostdata;
 	struct lpc_cycle_para iopara;
 	unsigned long addr;
-	u32 rd_data = 0;
+	__le32 rd_data = 0;
 	int ret;
 
 	if (!lpcdev || !dwidth || dwidth > LPC_MAX_DWIDTH)
@@ -244,13 +244,12 @@ static void hisi_lpc_comm_out(void *hostdata, unsigned long pio,
 	struct lpc_cycle_para iopara;
 	const unsigned char *buf;
 	unsigned long addr;
+	__le32 _val = cpu_to_le32(val);
 
 	if (!lpcdev || !dwidth || dwidth > LPC_MAX_DWIDTH)
 		return;
 
-	val = cpu_to_le32(val);
-
-	buf = (const unsigned char *)&val;
+	buf = (const unsigned char *)&_val;
 	addr = hisi_lpc_pio_to_addr(lpcdev, pio);
 
 	iopara.opflags = FG_INCRADDR_LPC;
-- 
2.17.1

[PATCH v3 4/5] bus: hisi_lpc: Expand build test coverage

[John Garry]

Currently the driver will only ever be built for ARM64 because it selects
CONFIG_INDIRECT_PIO, which itself depends on ARM64.

Expand build test coverage for the driver to other architectures by only
selecting CONFIG_INDIRECT_PIO for ARM64, when we really want it.

We don't include ALPHA, C6X, HEXAGON, and PARISC architectures as they
don't define {read, write}sb.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/Kconfig | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 6b331061d34b..70886abe008e 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -41,8 +41,9 @@ config MOXTET
 
 config HISILICON_LPC
 	bool "Support for ISA I/O space on HiSilicon Hip06/7"
-	depends on ARM64 && (ARCH_HISI || COMPILE_TEST)
-	select INDIRECT_PIO
+	depends on (ARM64 && ARCH_HISI) || (COMPILE_TEST && !ALPHA && !HEXAGON && !PARISC && !C6X)
+	depends on HAS_IOMEM
+	select INDIRECT_PIO if ARM64
 	help
 	  Driver to enable I/O access to devices attached to the Low Pin
 	  Count bus on the HiSilicon Hip06/7 SoC.
-- 
2.17.1

[PATCH v3 5/5] logic_pio: Build into a library

[John Garry]

Object file logic_pio.o is always built.

Ideally the object file should only be built when required. This is
tricky, as that would be for archs which define PCI_IOBASE, but no common
config option exists for that.

For now, continue to always build but at least ensure the symbols are not
included in the vmlinux when not referenced.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..27645143d8bb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -108,7 +108,7 @@ obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o
 obj-$(CONFIG_CHECK_SIGNATURE) += check_signature.o
 obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
 
-obj-y += logic_pio.o
+lib-y += logic_pio.o
 
 obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
 
-- 
2.17.1

Re: [PATCH v3 3/5] bus: hisi_lpc: Clean some types

[Joe Perches]

On Tue, 2019-11-05 at 01:22 +0800, John Garry wrote:
> Sparse complains of these:
> drivers/bus/hisi_lpc.c:82:38: warning: incorrect type in argument 1 (different address spaces)
> drivers/bus/hisi_lpc.c:82:38:    expected void const volatile [noderef] <asn:2>*addr
> drivers/bus/hisi_lpc.c:82:38:    got unsigned char *
> drivers/bus/hisi_lpc.c:131:35: warning: incorrect type in argument 1 (different address spaces)
> drivers/bus/hisi_lpc.c:131:35:    expected unsigned char *mbase
> drivers/bus/hisi_lpc.c:131:35:    got void [noderef] <asn:2>*membase
> drivers/bus/hisi_lpc.c:186:35: warning: incorrect type in argument 1 (different address spaces)
> drivers/bus/hisi_lpc.c:186:35:    expected unsigned char *mbase
> drivers/bus/hisi_lpc.c:186:35:    got void [noderef] <asn:2>*membase
> drivers/bus/hisi_lpc.c:228:16: warning: cast to restricted __le32
> drivers/bus/hisi_lpc.c:251:13: warning: incorrect type in assignment (different base types)
> drivers/bus/hisi_lpc.c:251:13:    expected unsigned int [unsigned] [usertype] val
> drivers/bus/hisi_lpc.c:251:13:    got restricted __le32 [usertype] <noident>
> 
> Clean them up.

OK, it might also be good to change the _in and _out functions
to use void * and not unsigned char * for buf

---
 drivers/bus/hisi_lpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 20c9571..ec2bfb 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -100,7 +100,7 @@ static int wait_lpc_idle(unsigned char *mbase, unsigned int waitcnt)
  */
 static int hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev,
 			      struct lpc_cycle_para *para, unsigned long addr,
-			      unsigned char *buf, unsigned long opcnt)
+			      void *buf, unsigned long opcnt)
 {
 	unsigned int cmd_word;
 	unsigned int waitcnt;
@@ -153,7 +153,7 @@ static int hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev,
  */
 static int hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev,
 			       struct lpc_cycle_para *para, unsigned long addr,
-			       const unsigned char *buf, unsigned long opcnt)
+			       const void *buf, unsigned long opcnt)
 {
 	unsigned int waitcnt;
 	unsigned long flags;

Re: [PATCH v3 3/5] bus: hisi_lpc: Clean some types

[John Garry]

On 04/11/2019 18:39, Joe Perches wrote:
> On Tue, 2019-11-05 at 01:22 +0800, John Garry wrote:
>> Sparse complains of these:
>> drivers/bus/hisi_lpc.c:82:38: warning: incorrect type in argument 1 (different address spaces)
>> drivers/bus/hisi_lpc.c:82:38:    expected void const volatile [noderef] <asn:2>*addr
>> drivers/bus/hisi_lpc.c:82:38:    got unsigned char *
>> drivers/bus/hisi_lpc.c:131:35: warning: incorrect type in argument 1 (different address spaces)
>> drivers/bus/hisi_lpc.c:131:35:    expected unsigned char *mbase
>> drivers/bus/hisi_lpc.c:131:35:    got void [noderef] <asn:2>*membase
>> drivers/bus/hisi_lpc.c:186:35: warning: incorrect type in argument 1 (different address spaces)
>> drivers/bus/hisi_lpc.c:186:35:    expected unsigned char *mbase
>> drivers/bus/hisi_lpc.c:186:35:    got void [noderef] <asn:2>*membase
>> drivers/bus/hisi_lpc.c:228:16: warning: cast to restricted __le32
>> drivers/bus/hisi_lpc.c:251:13: warning: incorrect type in assignment (different base types)
>> drivers/bus/hisi_lpc.c:251:13:    expected unsigned int [unsigned] [usertype] val
>> drivers/bus/hisi_lpc.c:251:13:    got restricted __le32 [usertype] <noident>
>>
>> Clean them up.
> 
> OK, it might also be good to change the _in and _out functions
> to use void * and not unsigned char * for buf

Hi Joe,

In fact, using unsigned char * is the right thing to do, and really the 
upper layer prob should not be passing void *. I'll look at this as a 
follow up.

Cheers,
John

> 
> ---
>   drivers/bus/hisi_lpc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 20c9571..ec2bfb 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -100,7 +100,7 @@ static int wait_lpc_idle(unsigned char *mbase, unsigned int waitcnt)
>    */
>   static int hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev,
>   			      struct lpc_cycle_para *para, unsigned long addr,
> -			      unsigned char *buf, unsigned long opcnt)
> +			      void *buf, unsigned long opcnt)
>   {
>   	unsigned int cmd_word;
>   	unsigned int waitcnt;
> @@ -153,7 +153,7 @@ static int hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev,
>    */
>   static int hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev,
>   			       struct lpc_cycle_para *para, unsigned long addr,
> -			       const unsigned char *buf, unsigned long opcnt)
> +			       const void *buf, unsigned long opcnt)
>   {
>   	unsigned int waitcnt;
>   	unsigned long flags;
> 
> 
> .
> 

Re: [PATCH v3 3/5] bus: hisi_lpc: Clean some types

[Joe Perches]

On Tue, 2019-11-05 at 09:50 +0000, John Garry wrote:
> On 04/11/2019 18:39, Joe Perches wrote:
> > On Tue, 2019-11-05 at 01:22 +0800, John Garry wrote:
> > > Sparse complains of these:
> > > drivers/bus/hisi_lpc.c:82:38: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/bus/hisi_lpc.c:82:38:    expected void const volatile [noderef] <asn:2>*addr
> > > drivers/bus/hisi_lpc.c:82:38:    got unsigned char *
> > > drivers/bus/hisi_lpc.c:131:35: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/bus/hisi_lpc.c:131:35:    expected unsigned char *mbase
> > > drivers/bus/hisi_lpc.c:131:35:    got void [noderef] <asn:2>*membase
> > > drivers/bus/hisi_lpc.c:186:35: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/bus/hisi_lpc.c:186:35:    expected unsigned char *mbase
> > > drivers/bus/hisi_lpc.c:186:35:    got void [noderef] <asn:2>*membase
> > > drivers/bus/hisi_lpc.c:228:16: warning: cast to restricted __le32
> > > drivers/bus/hisi_lpc.c:251:13: warning: incorrect type in assignment (different base types)
> > > drivers/bus/hisi_lpc.c:251:13:    expected unsigned int [unsigned] [usertype] val
> > > drivers/bus/hisi_lpc.c:251:13:    got restricted __le32 [usertype] <noident>
> > > 
> > > Clean them up.
> > 
> > OK, it might also be good to change the _in and _out functions
> > to use void * and not unsigned char * for buf
> 
> Hi Joe,

Hi John.

> In fact, using unsigned char * is the right thing to do, and really the 
> upper layer prob should not be passing void *. I'll look at this as a 
> follow up.

Maybe so, but it looks like the generic readsb and writesb routines
take void * so it might be a lot of other files to modify.

g'luck and cheers, Joe

Re: [PATCH v3 0/5] hisi_lpc: Improve build test coverage

[Wei Xu]

Hi John,

On 2019/11/5 1:22, John Garry wrote:
> This series aims to improve the build test cover of the driver by
> supporting building under COMPILE_TEST.
>
> I also included "lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops
> are set at registration" as it was never picked up for 5.4.
>
> Two new patches are also included since v1:
> - clean issues detected by sparse
> - build logic_pio.o into /lib library
>
> Since v2 I limited test coverage for archs which don't define
> {read, write}sb().
>
> John Garry (5):
>    lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at
>      registration
>    logic_pio: Define PIO_INDIRECT_SIZE for !CONFIG_INDIRECT_PIO
>    bus: hisi_lpc: Clean some types
>    bus: hisi_lpc: Expand build test coverage
>    logic_pio: Build into a library
>
>   drivers/bus/Kconfig       |  5 +++--
>   drivers/bus/hisi_lpc.c    |  9 ++++-----
>   include/linux/logic_pio.h |  4 ++--
>   lib/Makefile              |  2 +-
>   lib/logic_pio.c           | 14 ++++++++------
>   5 files changed, 18 insertions(+), 16 deletions(-)
>

Thanks!
Dropped the v2 and applied v3 series to the hisilicon driver tree.

Best Regards,
Wei