Re: [PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced

[Rongrong Zou <zourongrong@huawei.com>]

Hi Arnd,

Thanks for your comment.

在 2015/12/29 21:47, Arnd Bergmann 写道:
> On Tuesday 29 December 2015 21:33:50 Rongrong Zou wrote:
>> Indirect ISA port I/O accessing introduced, vendors can hook
>> their own in/out function to general inb/outb. Drivers can access
>> legacy ISA I/O port by inb/outb as it is done in x86 platform.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>
> Looks correct to me, but I have a few style comments
>
>> ---
>>   arch/arm64/Kconfig.platforms |  5 ++-
>>   arch/arm64/include/asm/io.h  | 78 ++++++++++++++++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/setup.c    |  5 +++
>>   3 files changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 4043c35..98ae206 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -127,5 +127,8 @@ config ARCH_ZYNQMP
>>   	bool "Xilinx ZynqMP Family"
>>   	help
>>   	  This enables support for Xilinx ZynqMP Family
>> -
>> +config ARM64_INDIRECT_PIO
>> +	bool "ARM64 Indirect port I/O"
>> +	help
>> +	  This enables support for ARM64 indirect port I/O
>>   endmenu
>
> The option should probably go into arch/arm64/Kconfig. Possibly you can make
> it a silent option that just gets selected whenever a driver is enabled
> that might set the callbacks.

I try to bind it with HISILICON platform.

>
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 44be1e0..0041f3b 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -193,6 +193,84 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
>>    */
>>   #define xlate_dev_kmem_ptr(p)	p
>>
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +#define DEF_PCI_HOOK_pio(x)   x
>> +#else
>> +#define DEF_PCI_HOOK_pio(x)   NULL
>> +#endif
>
> Maybe just put the entire definition block inside #ifdef and
> fall back to the default inb/outb definitions otherwise.
>
>> +/*
>> + * This value is equal to PCIBIOS_MIN_IO
>> + */
>> +#define LEGACY_ISA_PORT_MAX 0x1000
>
> I would just use PCIBIOS_MIN_IO instead of defining another macro.

Because PCIBIOS_MIN_IO is defined in asm/pci.h, and asm/io.h
is included by asm/pci.h. so do you mean i define PCIBIOS_MIN_IO here,
and in asm/pci.h just use include asm/io.h ?

>
>> +extern struct arm64_isa_io {
>> +	u8 (*inb)(unsigned long port);
>> +	u16 (*inw)(unsigned long port);
>> +	u32 (*inl)(unsigned long port);
>> +	void (*outb)(u8 value, unsigned long port);
>> +	void (*outw)(u16 value, unsigned long port);
>> +	void (*outl)(u32 value, unsigned long port);
>> +} arm64_isa_io;
>
> Maybe make this a single function pointer like
>
> void (*arm64_indirect_pio)(unsigned long port, bool write, int size, void *data);
>
> I'm guessing that this would result in smaller object code at the call sites,
> but you'd have to try.

OK, i try it.

>
> 	Arnd
>
> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>