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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
