Hi Liviu

-----Original Message-----
From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
Sent: 11 November 2016 18:16
To: Gabriele Paoloni
Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang;
mark.rutland@arm.com; devicetree@vger.kernel.org;
lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com;
zhichang.yuan02@gmail.com
Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
Hip06

On Fri, Nov 11, 2016 at 03:53:53PM +0000, Gabriele Paoloni wrote:

quoted

Hi Liviu

Hi Gabriele,

quoted

quoted

-----Original Message-----
From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
Sent: 11 November 2016 14:46
To: Gabriele Paoloni
Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org;

Yuanzhichang;

quoted

quoted

mark.rutland@arm.com; devicetree@vger.kernel.org;
lorenzo.pieralisi@arm.com; minyard@acm.org; linux-

pci@vger.kernel.org;

quoted

quoted

benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com;
zhichang.yuan02@gmail.com
Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
Hip06

On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote:

quoted

Hi Arnd

quoted

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de]
Sent: 10 November 2016 16:07
To: Gabriele Paoloni
Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
mark.rutland@arm.com; devicetree@vger.kernel.org;
lorenzo.pieralisi@arm.com; minyard@acm.org; linux-

pci@vger.kernel.org;

quoted

quoted

benh@kernel.crashing.org; John Garry; will.deacon@arm.com;

linux-

quoted

quoted

quoted

quoted

kernel@vger.kernel.org; xuwei (O); Linuxarm;

zourongrong@gmail.com;

quoted

quoted

quoted

quoted

robh+dt@kernel.org; kantyzc@163.com; linux-

serial@vger.kernel.org;

quoted

quoted

quoted

quoted

catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
bhelgaas@googl e.com; zhichang.yuan02@gmail.com
Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver

implementation on

quoted

quoted

quoted

quoted

Hip06

On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni

wrote:

quoted

quoted

quoted

Where should we get the range from? For LPC we know that it

is

quoted

quoted

going

quoted

quoted

quoted

Work on anything that is not used by PCI I/O space, and this

is

quoted

quoted

quoted

quoted

quoted

why we use [0, PCIBIOS_MIN_IO]

It should be allocated the same way we allocate PCI config

space

quoted

quoted

quoted

quoted

segments. This is currently done with the io_range list in
drivers/pci/pci.c, which isn't perfect but could be extended
if necessary. Based on what others commented here, I'd rather
make the differences between ISA/LPC and PCI I/O ranges smaller
than larger.

Gabriele,

quoted

I am not sure this would make sense...

IMHO all the mechanism around io_range_list is needed to provide

the

quoted

quoted

quoted

"mapping" between I/O tokens and physical CPU addresses.

Currently the available tokens range from 0 to IO_SPACE_LIMIT.

As you know the I/O memory accessors operate on whatever
__of_address_to_resource sets into the resource (start, end).

With this special device in place we cannot know if a resource is
assigned with an I/O token or a physical address, unless we

forbid

quoted

quoted

quoted

the I/O tokens to be in a specific range.

So this is why we are changing the offsets of all the functions
handling io_range_list (to make sure that a range is forbidden to
the tokens and is available to the physical addresses).

We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
because this is the maximum physical I/O range that a non PCI

device

quoted

quoted

quoted

can operate on and because we believe this does not impose much
restriction on the available I/O token range; that now is
[PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
So we believe that the chosen forbidden range can accommodate
any special ISA bus device with no much constraint on the rest
of I/O tokens...

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] )

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

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...

Thanks

Gab

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!  /
  ---------------
    ¯\_(ツ)_/¯

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