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

[Rongrong Zou]

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@xxxxxxxxx>
>
> 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@xxxxxxxxxx
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/