On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:

Hi Liviu

[snip]

quoted

quoted

quoted

Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and

you

quoted

quoted

actually need another variable for "reserving" an area in the I/O

space

quoted

quoted

that can be used for physical addresses rather than I/O tokens.

The one good example for using PCIBIOS_MIN_IO is when your
platform/architecture
does not support legacy ISA operations *at all*. In that case

someone

quoted

quoted

sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O

range

quoted

quoted

so that it doesn't get used. With Zhichang's patch you now start
forcing
those platforms to have a valid address below PCIBIOS_MIN_IO.

But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be

used

quoted

by PCI controllers only...

Nope, that is not what it means. It means that PCI devices can see I/O
addresses
on the bus that start from 0. There never was any usage for non-PCI
controllers

So I am a bit confused...
From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
It seems that ISA buses operate on cpu I/O address range [0, 0xFFF].
I thought that was the reason why for most architectures we have
PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers
usually use [0, PCIBIOS_MIN_IO - 1] )

First of all, cpu I/O addresses is an x86-ism. ARM architectures and others
 have no separate address space for I/O, it is all merged into one unified
address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could mean
that we don't care about ISA I/O because the platform does not support having
an ISA bus (e.g.).

For those architectures whose PCIBIOS_MIN_IO != 0x1000 probably
they are not fully compliant or they cannot fully support an ISA
controller...?

Exactly. Not fully compliant is a bit strong, as ISA is a legacy feature and
when it comes to PCI-e you are allowed to ignore it. Having PCIBIOS_MIN_IO != 0x1000
is a way to signal that you don't fully support ISA.

As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO)
to allow special ISA controllers to use that range with special
accessors.
Having a variable threshold would make life much more difficult
as there would be a probe dependency between the PCI controller and
the special ISA one (PCI to wait for the special ISA device to be
probed and set the right threshold value from DT or ACPI table).

Instead using PCIBIOS_MIN_IO is easier and should not impose much
constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to
the PCI controller for I/O tokens...

What I am suggesting is to leave PCIBIOS_MIN_IO alone which still reserves
space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will reserve
space for your direct address I/O on top of PCIBIOS_MIN_IO.

Best regards,
Liviu

Thanks

Gab

quoted

when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and
what
I think is not the right thing (and not enough anyway).

quoted

so if you have a special bus device using
an I/O range in this case should be a PCI controller...

That has always been the case. It is this series that wants to
introduce the
new meaning.

quoted

i.e. I would
expect it to fall back into the case of I/O tokens redirection rather

than

quoted

physical addresses redirection (as mentioned below from my previous

reply).

quoted

What do you think?

I think you have looked too much at the code *with* Zhichang's patches
applied.
Take a step back and look at how PCIBIOS_MIN_IO is used now, before you
apply
the patches. It is all about PCI addresses and there is no notion of
non-PCI
busses using PCI framework. Only platforms and architectures that try
to work
around some legacy standards (ISA) or HW restrictions.

Best regards,
Liviu

quoted

Thanks

Gab

quoted

For the general case you also have to bear in mind that

PCIBIOS_MIN_IO

quoted

quoted

could
be zero. In that case, what is your "forbidden" range? [0, 0) ? So

it

quoted

quoted

makes
sense to add a new #define that should only be defined by those
architectures/
platforms that want to reserve on top of PCIBIOS_MIN_IO another

region

quoted

quoted

where I/O tokens can't be generated for.

Best regards,
Liviu

quoted

quoted

quoted

quoted

Your current version has

        if (arm64_extio_ops->pfout)

\

quoted

quoted

quoted

quoted

                arm64_extio_ops->pfout(arm64_extio_ops-

devpara,\

quoted

quoted

quoted

                       addr, value, sizeof(type));

\

quoted

quoted

quoted

quoted

Instead, just subtract the start of the range from the

logical

quoted

quoted

quoted

quoted

quoted

quoted

port number to transform it back into a bus-local port

number:

quoted

quoted

quoted

quoted

quoted

These accessors do not operate on IO tokens:

If (arm64_extio_ops->start > addr || arm64_extio_ops->end <

addr)

quoted

quoted

quoted

quoted

quoted

addr is not going to be an I/O token; in fact patch 2/3

imposes

quoted

quoted

that

quoted

quoted

quoted

the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to

PCIBIOS_MIN_IO

quoted

we have free physical addresses that the accessors can

operate

quoted

quoted

on.

quoted

quoted

Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to

refer

quoted

quoted

to

quoted

quoted

the logical I/O tokens, the purpose of that macro is really

meant

quoted

quoted

quoted

quoted

for allocating PCI I/O port numbers within the address space of
one bus.

As I mentioned above, special devices operate on CPU addresses

directly,

quoted

not I/O tokens. For them there is no way to distinguish....

quoted

Note that it's equally likely that whichever next platform

needs

quoted

quoted

quoted

quoted

non-mapped I/O access like this actually needs them for PCI I/O

space,

quoted

quoted

and that will use it on addresses registered to a PCI host

bridge.

quoted

quoted

quoted

Ok so here you are talking about a platform that has got an I/O

range

quoted

quoted

quoted

under the PCI host controller, right?
And this I/O range cannot be directly memory mapped but needs

special

quoted

quoted

quoted

redirections for the I/O tokens, right?

In this scenario registering the I/O ranges with the forbidden

range

quoted

quoted

quoted

implemented by the current patch would still allow to redirect

I/O

quoted

quoted

quoted

tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO

So effectively the special PCI host controller
1) knows the physical range that needs special redirection
2) register such range
3) uses pci_pio_to_address() to retrieve the IO tokens for the
   special accessors
4) sets arm64_extio_ops->start/end to the IO tokens retrieved in

3)

quoted

quoted

quoted

So to be honest I think this patch can fit well both with
special PCI controllers that need I/O tokens redirection and with
special non-PCI controllers that need non-PCI I/O physical
address redirection...

Thanks (and sorry for the long reply but I didn't know how
to make the explanation shorter :) )

Gab

quoted

If we separate the two steps:

a) assign a range of logical I/O port numbers to a bus
b) register a set of helpers for redirecting logical I/O
   port to a helper function

then I think the code will get cleaner and more flexible.
It should actually then be able to replace the powerpc
specific implementation.

	Arnd

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯