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

[John Garry <john.garry@huawei.com>]

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
>
> .
>