[PATCH 0/3] hisi_lpc: Improve build test coverage

[PATCH 0/3] 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.

John Garry (3):
  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: Expand build test coverage

 drivers/bus/Kconfig       |  4 ++--
 include/linux/logic_pio.h |  4 ++--
 lib/logic_pio.c           | 14 ++++++++------
 3 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH 1/3] 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 2/3] 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 3/3] 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.

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

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 6b331061d34b..44cb4b6bea18 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -41,8 +41,8 @@ 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
+	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

Re: [PATCH 3/3] bus: hisi_lpc: Expand build test coverage

[Arnd Bergmann]

On Tue, Oct 1, 2019 at 6:07 PM John Garry <john.garry@huawei.com> wrote:
>
> 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.
>
> Signed-off-by: John Garry <john.garry@huawei.com>

Good idea, but doesn't this cause a link failure against
logic_pio_register_range() when INDIRECT_PIO is disabled?

      Arnd

Re: [PATCH 3/3] bus: hisi_lpc: Expand build test coverage

[John Garry]

On 02/10/2019 16:43, Arnd Bergmann wrote:
> On Tue, Oct 1, 2019 at 6:07 PM John Garry <john.garry@huawei.com> wrote:
>>
>> 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.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>

Hi Arnd,

> Good idea, but doesn't this cause a link failure against
> logic_pio_register_range() when INDIRECT_PIO is disabled?

No, it shouldn't do. Function 
lib/logic_pio.c::logic_pio_register_range() is built always, outside any 
INDIRECT_PIO checking.

A some stage, for completeness we should probably change 
logic_pio_register_range() and friends to be under PCI_IOBASE. But then 
we would need stubs for !PCI_IOBASE, due to this above change and also 
references from the device tree code. I'd have to consider this a bit 
more. Let me know what you think.

Thanks,
John

>
>       Arnd
>
>

Re: [PATCH 3/3] bus: hisi_lpc: Expand build test coverage

[Arnd Bergmann]

On Wed, Oct 2, 2019 at 6:25 PM John Garry <john.garry@huawei.com> wrote:
> On 02/10/2019 16:43, Arnd Bergmann wrote:
> > On Tue, Oct 1, 2019 at 6:07 PM John Garry <john.garry@huawei.com> wrote:
> >>
> >> 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.
> >>
> >> Signed-off-by: John Garry <john.garry@huawei.com>
> >
>
> Hi Arnd,
>
> > Good idea, but doesn't this cause a link failure against
> > logic_pio_register_range() when INDIRECT_PIO is disabled?
>
> No, it shouldn't do. Function
> lib/logic_pio.c::logic_pio_register_range() is built always, outside any
> INDIRECT_PIO checking.

Ok, I see.

> A some stage, for completeness we should probably change
> logic_pio_register_range() and friends to be under PCI_IOBASE. But then
> we would need stubs for !PCI_IOBASE, due to this above change and also
> references from the device tree code. I'd have to consider this a bit
> more. Let me know what you think.

It's probably not to do this with the usual 'static inline' stubs in the
header files. There is no rush here, but it would be nice to not build
this code when it is not needed.

I wonder if this one-line change would take care of the !CONFIG_OF
case already (it probably doesn't):

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -107,7 +107,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

On a related note: At some point we may want to add an indirect
method for readl()/writel(), to deal with some of the weirder 32-bit
ARM platforms. We'll have to see how well this can fit into the
infrastructure we already have for indirect PIO.

     Arnd

Re: [PATCH 3/3] bus: hisi_lpc: Expand build test coverage

[John Garry]

On 02/10/2019 19:22, Arnd Bergmann wrote:
> On Wed, Oct 2, 2019 at 6:25 PM John Garry <john.garry@huawei.com> wrote:
>> On 02/10/2019 16:43, Arnd Bergmann wrote:
>>> On Tue, Oct 1, 2019 at 6:07 PM John Garry <john.garry@huawei.com> wrote:
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>
>>
>> Hi Arnd,
>>
>>> Good idea, but doesn't this cause a link failure against
>>> logic_pio_register_range() when INDIRECT_PIO is disabled?
>>
>> No, it shouldn't do. Function
>> lib/logic_pio.c::logic_pio_register_range() is built always, outside any
>> INDIRECT_PIO checking.
>
> Ok, I see.
>
>> A some stage, for completeness we should probably change
>> logic_pio_register_range() and friends to be under PCI_IOBASE. But then
>> we would need stubs for !PCI_IOBASE, due to this above change and also
>> references from the device tree code. I'd have to consider this a bit
>> more. Let me know what you think.
>
> It's probably not to do this with the usual 'static inline' stubs in the
> header files. There is no rush here, but it would be nice to not build
> this code when it is not needed.

Agreed, I'll add it to my todo list.

So we want to build logic_pio.c for archs which define PCI_IOBASE, but 
there is no specific config option for that.

>
> I wonder if this one-line change would take care of the !CONFIG_OF
> case already (it probably doesn't):
>
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -107,7 +107,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

So that means that this obj will not be included in the vmlinux file for 
!CONFIG_OF (for archs with !PCI_IOBASE or !PCI).

It is an improvement, but I still would like to make the complete change 
and not to build at all for archs which don't define PCI_IOBASE.

>
>  obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
>
> On a related note: At some point we may want to add an indirect
> method for readl()/writel(), to deal with some of the weirder 32-bit
> ARM platforms. We'll have to see how well this can fit into the
> infrastructure we already have for indirect PIO.

It should be possible. We would just need to manage 2 separate logical 
spaces instead of one: a. mmio b. pio

I'm guessing that all the code for logical <-> hw bus address 
translations would be the same.

Thanks,
John

>
>      Arnd
>
> .
>