Message-ID: <57D797C4.1080300@gmail.com>
Date:   Tue, 13 Sep 2016 14:08:04 +0800
From:   zhichang <zhichang.yuan02@...il.com>
To:     Arnd Bergmann <arnd@...db.de>,
        "zhichang.yuan" <yuanzhichang@...ilicon.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linuxarm@...wei.com,
        linux-kernel@...r.kernel.org, lorenzo.pieralisi@....com,
        minyard@....org, benh@...nel.crashing.org,
        gabriele.paoloni@...wei.com, john.garry@...wei.com,
        liviu.dudau@....com, zourongrong@...il.com,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH V2 1/4] ARM64 LPC: Indirect ISA port IO introduced

Hi, Arnd,


On 2016年09月08日 21:23, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 3:45:21 PM CEST zhichang.yuan wrote:
>> On 2016/9/7 23:06, Arnd Bergmann wrote:
>>> On Wednesday, September 7, 2016 9:33:50 PM CEST Zhichang Yuan wrote:
>>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>>> +
>>>> +typedef u64 (*inhook)(void *devobj, unsigned long ptaddr, void *inbuf,
>>>> +                               size_t dlen, unsigned int count);
>>>> +typedef void (*outhook)(void *devobj, unsigned long ptaddr,
>>>> +                               const void *outbuf, size_t dlen,
>>>> +                               unsigned int count);
>>>> +
>>>> +struct extio_ops {
>>>> +       inhook  pfin;
>>>> +       outhook pfout;
>>>> +       void *devpara;
>>>> +};
>>>> +
>>>> +extern struct extio_ops *arm64_simops __refdata;
>>>> +
>>>> +/*Up to now, only applied to Hip06 LPC. Define as static here.*/
>>>> +static inline void arm64_set_simops(struct extio_ops *ops)
>>>> +{
>>>> +       if (ops)
>>>> +               WRITE_ONCE(arm64_simops, ops);
>>>> +}
>>>> +
>>>> +
>>>> +#define BUILDIO(bw, type)                                              \
>>>> +static inline type in##bw(unsigned long addr)                          \
>>>> +{                                                                      \
>>>> +       if (addr >= PCIBIOS_MIN_IO)                                     \
>>>> +               return read##bw(PCI_IOBASE + addr);                     \
>>>> +       return (arm64_simops && arm64_simops->pfin) ?                   \
>>>> +               arm64_simops->pfin(arm64_simops->devpara, addr, NULL,   \
>>>> +                                       sizeof(type), 1) : -1;          \
>>>> +}                                                      \
>>>>
>>>
>>> Hmm, the way this is done, enabling CONFIG_ARM64_INDIRECT_PIO at
>>> compile time means that only the dynamically registered PIO support
>>> is possible for I/O port ranges 0-0xfff.
>> Yes. The arm64_simops is only for IO range 0-0xfff. But since only one global arm64_simops, this patch doesn't
>> support the dynamically PIO register, only one PIO range of 0-0xfff is supported. As for multiple PIO ranges
>> register, you also mention below, will discuss there.
> 
> I think having only one range is enough, but it may be best not to
> assume that this is mapped at a fixed location in the Linux PIO
> port address space.
ok. I will add the linux PIO range into struct extio_ops. With this new added PIO range, we can register a
variable linux PIO range to the global arm64_simops for our LPC.

> 
> As I understand, you list all the child devices in DT, so those
> port numbers should all be translatable from bus specific (0x0-0xfff)
> into the larger Linux range that also contains PCI devices.
> 
>>> I think the runtime check should better test if simops was defined
>>> first and fall back to normal PIO otherwise, in order to allow
>>> LPC implementations on a PCI-LPC bridge.
>> Do you mean check arm64_simops first?
>> I don't understand clearly what is the benefit about that.
>> It seems that most IO accesses are MMIO, is it the current implementation a bit efficent?
> 
> No, this is about having devices at hardcoded PIO addresses behind PCI
> on another (non-hisilicon) machine running the same kernel binary.
> 

What about defining inb like that:

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);
}

The CONFIG_ARM64_INDIRECT_PIO is still using. When indirect-IO is needed, ARM64_INDIRECT_PIO will be selected.
I don't want to define arm64_extio_ops in some build-in source file, such as setup.c; keeping
CONFIG_ARM64_INDIRECT_PIO is for the specific devices which need indirect-IO.

extio_inb is defined as weak function in c file.
All these revise will be presented in V3 patch soon.

>>> How about allowing an I/O port range to be defined along with
>>> the operations and check against that?
>>>
>>> u8 intb(unsigned long port)
>>> {
>>> 	if (arm64_simops &&
>>> 	    (port >= arm64_simops->min) && 
>>> 	    (port <= arm64_simops->max))
>>> 		return arm64_simops->pfin(arm64_simops, port, 1);
>>> 	else
>>> 		return readb(PCI_IOBASE + addr);
>>> }
>>>
>>> The other advantage of that is that you can dynamically register
>>> a translation for the LPC port range into the Linux I/O port range
>>> like PCI hosts do.
>> Yes. an IO port range along with the operations is more generic and extensible.
>> Do you want to define extio_ops like that:
>>
>> struct extio_ops {
>>         unsigned long start;
>>         unsigned long end;
>>         unsigned long ptoffset;/* port IO - linux io */
>>         inhook  pfin;
>>         outhook pfout;
>>         void *devpara;
>> };
>>
>> With this structure, we can register the PIO range we need without limit in 0-0xfff. But there is only one global struct
>> extio_ops where arm64_simops points to, we can only register one operation.
>> Actually, Hip06 LPC currently need at least two PIO ranges, 0xe4-0xe7, 0x2f8-0x2ff.
>> In this patch, we want to make the PIO differentiation in the new revised in/out() is more simpler, just reserve a bigger
>> PIO range of 0-0xfff from the whole PIO range for this indirect-IO introduced in this patch-set. I think this reservation
>> is not so safe, if there are other legacy devices which are designed to use fixable PIO range below 0x1000 through in/out,
>> the trouble will happen.
> 
> I don't think we can solve all the possible cases. When a driver asks
> for a hardcoded address, we can either route that to the first PCI bus
> that registers, or always route it to LPC, but there may always be
> corner cases that don't work.
> 
> Fortunately, it is very rare for hardcoded PIO addresses to be required
> in particular on non-x86 architectures, so it might not matter too much
> in practice:
> 
> Having the extio range live on ports 0-0x1000 by default is probably
> reasonable, as long as that range is also usable for PCI on other
> platforms. Having it registered dynamically after the PCI bus should
> also be ok.
> 
In the coming patch V3, we don't reserve the fixed PIO range of 0-0x1000. The extio range is totally depended
on the device IO resource configuration.

Several changes will be done in V3:
1) adopt the translation for all IO ranges including those from Hip06 LPC by pci_address_to_pio. Which means
that all physical IO ranges will be converted to fully different linux PIO ranges. Based on this, I think the
PCI PIO ranges can co-exist with the PIO ranges from other bus, including Hip06 LPC;
2) Since only one extio range is supported in this patch-set, we only register the linux PIO range for IPMI bt
in arm64_simops to work based on indirect-IO; In this way, ipmi bt can work well without any changes on
current ipmi driver.


>> Based on your initial idea, I have two thoughts which help to make the indirect-IO more generic:
>>
>> 1. setup a list where all indirect-IO devices' operations are linked to
>>
>>
>> struct extio_range {
>>         unsigned long start;/* inclusive, sys io addr */
>>         unsigned long end;/* inclusive, sys io addr */
>>         unsigned long ptoffset;/* port Io - system Io */
>> };
>>
>> struct extio_node {
>>         struct list_head ranlink;
>>
>>         struct extio_range iores;
>>
>>         /*pointer to the device provided services*/
>>         struct extio_ops *regops;
>> };
>>
>> when in/out is called with the input PIO parameter, check which node contains the input PIO and call the corresponding operation to
>> complete the IO.
>>
>> static inline type inb(unsigned long addr)
>> {
>>         struct extio_node *extop;
>>         unsigned long offset;
>> /* extio_range_getops() will scan the list to find the node where start <= addr <= end is satisfied*/
>>         extop = extio_range_getops(addr, &offset);
>>         if (!extop)
>>                 return read##bw(PCI_IOBASE + addr);
>>         if (extop->regops && extop->regops->pfin)
>>                 return extop->regops->pfin(extop->regops->devpara,
>>                                 addr + offset, NULL, sizeof(type), 1);
>>         return -1;
>> }
>>
>> The major disadvantage of this method is the performance. When the list is not long, it will be ok, I think.
> 
>> If support multiple PIO ranges are not needed, we don't need this list, only continue use the global arm64_simops based on the new
>> extio_ops structure. Probably this is your suggestion.
> 
> I wouldn't go this far: just assume that there is either one set of
> operations registered or none at all, but make it possible to have it
> at an arbitrary address.
> 
>> 2. extend the linux IO space to spare a fully separate PIO range for indirect-IO
>>
>> the current linux IO space on arm64 is 0 to  IO_SPACE_LIMIT:
>>
>> #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
>> #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
>>
>> current PCI_IO_SIZE is 16M.
>>
>> It seems the current linux IO space on arm64 is totally for PCI IO based on MMIO. For indirect-IO in this patch-set, we populate the linux
>> IO range from 16M to 18M, this 2M linux IO space can be divided into 32 segments with segment size is 64K. Each segment is exclusively populated
>> by one indirect-IO device. when the device is creating, a segment with unique segment ID will be allocated and the IO resource will be converted
>> to the IO range corresponding with that segment. For example, segement 2 will own the IO range 0x1020000 - 0x102ffff.
>>
>> the structure for this way is:
>>
>> #define EXTIO_VECTOR_MAX     32
>> struct extio_vector {
>>         struct mutex            seglock;
>>
>> 	/* one bit corresponds with one segment */
>>         DECLARE_BITMAP(bmap, EXTIO_VECTOR_MAX);
>>         struct extio_ops       *opsvec;
>> };
>>
>>
>> when the corresponding driver call in/out with one port address from the allocated linux IO resource, the processing like that:
>>
>> static inline type inb(unsigned long addr)
>> {
>>         if (!(addr & (0x01 << 16))) /* only check bit 16 */
>>                 return readb(PCI_IOBASE + addr);
>> /* extio_inb will directly parse the bit16 to bit 20 to get the segment ID, then get the corresponding IO operation specific to device */
>>         return extio_inb(addr);
>> }
>>
>> This method is nearly no performance lose, but is more complicated. Maybe it is not worthy to do that.
> 
> No, I don't think this is necessary either.
Ok. Will make the code simpler. One extio range is enough for hip06 LPC at this moment.

Thanks,
Zhichang
> 
>>> We may also want to move the inb/outb definitions into a .c file
>>> as they are getting rather big.
>> The current in/out is defined as inline function in asm-generic/io.h; If we move them to .c file, probably much change.....
> 
> The current implementation turns into a single CPU instruction, my idea
> was not to grow that too much but instead turn it into a branch instruction
> when your code is enabled at compile-time. When it's disabled, we should
> still use the existing behavior.
> 
> 	Arnd
> 

Powered by blists - more mailing lists