Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1485682
| From | zhichang <zhichang.yuan02@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced |
| Date | 2016-09-18 05:40 +0200 |
| Message-ID | <siB7r-2Pf-1@gated-at.bofh.it> (permalink) |
| References | <shh18-no-5@gated-at.bofh.it> <shhu9-PJ-9@gated-at.bofh.it> <shjcB-1Ui-29@gated-at.bofh.it> <shjmi-1XX-47@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Hi, Arnd,
On 2016年09月14日 22:23, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 10:16:28 PM CEST zhichang.yuan wrote:
>>>
>>> No need to guard includes with an #ifdef.
>> If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in
>> extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes.
>
> There is no problem with making declarations visible for functions that
> are not part of the kernel, we do that all the time.
>
>>>> +#define BUILDS_RW(bwl, type) \
>>>> +static inline void reads##bwl(const volatile void __iomem *addr, \
>>>> + void *buffer, unsigned int count) \
>>>> +{ \
>>>> + if (count) { \
>>>> + type *buf = buffer; \
>>>> + \
>>>> + do { \
>>>> + type x = __raw_read##bwl(addr); \
>>>> + *buf++ = x; \
>>>> + } while (--count); \
>>>> + } \
>>>> +} \
>>>> + \
>>>> +static inline void writes##bwl(volatile void __iomem *addr, \
>>>> + const void *buffer, unsigned int count) \
>>>> +{ \
>>>> + if (count) { \
>>>> + const type *buf = buffer; \
>>>> + \
>>>> + do { \
>>>> + __raw_write##bwl(*buf++, addr); \
>>>> + } while (--count); \
>>>> + } \
>>>> +}
>>>> +
>>>> +BUILDS_RW(b, u8)
>>>
>>> Why is this in here?
>> the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need
>> to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur.
>>
>> It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of
>> those function needed here....
>>
>> Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue.
>>
>> #ifdef CONFIG_ARM64_INDIRECT_PIO
>> #define inb inb
>> extern u8 inb(unsigned long addr);
>>
>> #define outb outb
>> extern void outb(u8 value, unsigned long addr);
>>
>> #define insb insb
>> extern void insb(unsigned long addr, void *buffer, unsigned int count);
>>
>> #define outsb outsb
>> extern void outsb(unsigned long addr, const void *buffer, unsigned int count);
>> #endif
>>
>> and definitions of all these functions are in extio.c :
>>
>> u8 inb(unsigned long addr)
>> {
>> if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>> arm64_extio_ops->end < addr)
>> return readb(PCI_IOBASE + addr);
>> else
>> return arm64_extio_ops->pfin ?
>> arm64_extio_ops->pfin(arm64_extio_ops->devpara,
>> addr + arm64_extio_ops->ptoffset, NULL,
>> sizeof(u8), 1) : -1;
>> }
>> .....
>
> Yes, sounds good.
>
>>>> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>>> #define IO_SPACE_LIMIT (PCI_IO_SIZE - 1)
>>>> #define PCI_IOBASE ((void __iomem *)PCI_IO_START)
>>>>
>>>> +
>>>> +/*
>>>> + * redefine the in(s)b/out(s)b for indirect-IO.
>>>> + */
>>>> +#define inb inb
>>>> +static inline u8 inb(unsigned long addr)
>>>> +{
>>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>>> + if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>>>> + addr <= arm64_extio_ops->end)
>>>> + return extio_inb(addr);
>>>> +#endif
>>>> + return readb(PCI_IOBASE + addr);
>>>> +}
>>>> +
>>>
>>> Looks ok, but you only seem to do this for the 8-bit
>>> accessors, when it should be done for 16-bit and 32-bit
>>> ones as well for consistency.
>> Hip06 LPC only support 8-bit I/O operations on the designated port.
>
> That is an interesting limitation. Maybe still call the extio operations
> and have them do WARN_ON_ONCE() instead?
>
> If you get a driver that calls inw/outw on the range that is owned
> by the LPC bus, you otherwise get an unhandled page fault in kernel
> space, which is not as nice.
>
Yes. It probably cause kernel panic.
Will define the extio operations for other IO length and add the corresponding WARNINGS.
Best,
Zhichang
>>>> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
>>>> new file mode 100644
>>>> index 0000000..1e7a9c5
>>>> --- /dev/null
>>>> +++ b/drivers/bus/extio.c
>>>> @@ -0,0 +1,66 @@
>>>
>>> This is in a globally visible directory
>>>
>>>> +
>>>> +struct extio_ops *arm64_extio_ops;
>>>
>>> But the identifier uses an architecture specific prefix. Either
>>> move the whole file into arch/arm64, or make the naming so that
>>> it can be used for everything.
>>
>> I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm;
>
> Ok, that simplifies it a lot, you can just do everything in asm/io.h then.
>
> Arnd
>
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH V3 0/4] ARM64 LPC: legacy ISA I/O support Zhichang Yuan <yuanzhichang@hisilicon.com> - 2016-09-14 14:00 +0200
[PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced Zhichang Yuan <yuanzhichang@hisilicon.com> - 2016-09-14 14:10 +0200
Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-09-14 14:30 +0200
Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-09-14 16:20 +0200
Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-09-14 16:30 +0200
Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced zhichang <zhichang.yuan02@gmail.com> - 2016-09-18 05:40 +0200
Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced zhichang <zhichang.yuan02@gmail.com> - 2016-09-21 11:30 +0200
[PATCH V3 4/4] ARM64 LPC: support earlycon for UART connected to LPC Zhichang Yuan <yuanzhichang@hisilicon.com> - 2016-09-14 14:10 +0200
[PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count Zhichang Yuan <yuanzhichang@hisilicon.com> - 2016-09-14 14:10 +0200
Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count Arnd Bergmann <arnd@arndb.de> - 2016-09-14 14:30 +0200
Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-09-14 17:10 +0200
Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count Arnd Bergmann <arnd@arndb.de> - 2016-09-14 23:40 +0200
Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count Arnd Bergmann <arnd@arndb.de> - 2016-09-21 21:40 +0200
csiph-web