Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521400 > unrolled thread

RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-14 09:30 +0100
    Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "liviu.dudau@arm.com" <liviu.dudau@arm.com> - 2016-11-14 12:30 +0100
      Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-18 11:20 +0100
        RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-18 13:10 +0100
          Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-18 13:30 +0100
            RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-18 14:00 +0100
              Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-18 14:50 +0100
                RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-18 17:20 +0100
                  Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-18 17:40 +0100
                    RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-18 18:10 +0100
                      Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-23 15:20 +0100
                        RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-23 16:30 +0100
                          Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-23 18:10 +0100
                            Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-24 00:30 +0100
                              Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-24 10:20 +0100
                                Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-24 11:40 +0100
                              RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-25 09:50 +0100

#1521400 — RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

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:
> > Hi Liviu
> 
> Hi Gabriele,
> 
> >
> > > -----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;
> > > 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 01:39:35PM +0000, Gabriele Paoloni wrote:
> > > > Hi Arnd
> > > >
> > > > > -----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;
> > > > > 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; liviu.dudau@arm.com;
> > > > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com
> > > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver
> implementation on
> > > > > Hip06
> > > > >
> > > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni
> > > wrote:
> > > > > >
> > > > > > Where should we get the range from? For LPC we know that it
> is
> > > going
> > > > > > Work on anything that is not used by PCI I/O space, and this
> is
> > > > > > why we use [0, PCIBIOS_MIN_IO]
> > > > >
> > > > > It should be allocated the same way we allocate PCI config
> space
> > > > > 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,
> > >
> > > >
> > > > I am not sure this would make sense...
> > > >
> > > > IMHO all the mechanism around io_range_list is needed to provide
> the
> > > > "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
> > > > 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
> > > > 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
> > > actually need another variable for "reserving" an area in the I/O
> space
> > > 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
> > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O
> range
> > > 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
> > 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).
> 
> > 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.
> 
> > i.e. I would
> > expect it to fall back into the case of I/O tokens redirection rather
> than
> > physical addresses redirection (as mentioned below from my previous
> reply).
> > 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
> 
> >
> > Thanks
> >
> > Gab
> >
> >
> > >
> > > For the general case you also have to bear in mind that
> PCIBIOS_MIN_IO
> > > could
> > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So
> it
> > > 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
> > > where I/O tokens can't be generated for.
> > >
> > > Best regards,
> > > Liviu
> > >
> > > >
> > > > >
> > > > > > > Your current version has
> > > > > > >
> > > > > > >         if (arm64_extio_ops->pfout)
> > > \
> > > > > > >                 arm64_extio_ops->pfout(arm64_extio_ops-
> > > >devpara,\
> > > > > > >                        addr, value, sizeof(type));
> > > \
> > > > > > >
> > > > > > > Instead, just subtract the start of the range from the
> logical
> > > > > > > port number to transform it back into a bus-local port
> number:
> > > > > >
> > > > > > These accessors do not operate on IO tokens:
> > > > > >
> > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end <
> addr)
> > > > > > addr is not going to be an I/O token; in fact patch 2/3
> imposes
> > > that
> > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > > > > PCIBIOS_MIN_IO
> > > > > > we have free physical addresses that the accessors can
> operate
> > > on.
> > > > >
> > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to
> refer
> > > to
> > > > > the logical I/O tokens, the purpose of that macro is really
> meant
> > > > > 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,
> > > > not I/O tokens. For them there is no way to distinguish....
> > > >
> > > > >
> > > > > Note that it's equally likely that whichever next platform
> needs
> > > > > non-mapped I/O access like this actually needs them for PCI I/O
> > > space,
> > > > > and that will use it on addresses registered to a PCI host
> bridge.
> > > >
> > > > Ok so here you are talking about a platform that has got an I/O
> range
> > > > under the PCI host controller, right?
> > > > And this I/O range cannot be directly memory mapped but needs
> special
> > > > redirections for the I/O tokens, right?
> > > >
> > > > In this scenario registering the I/O ranges with the forbidden
> range
> > > > implemented by the current patch would still allow to redirect
> I/O
> > > > 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)
> > > >
> > > > 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
> > > >
> > > > >
> > > > > 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!  /
>   ---------------
>     ¯\_(ツ)_/¯

[toc] | [next] | [standalone]

#1521560

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

[snip]

> > > >
> > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and
> > you
> > > > actually need another variable for "reserving" an area in the I/O
> > space
> > > > 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
> > > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O
> > range
> > > > 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
> > > 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
> 
> > 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).
> > 
> > > 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.
> > 
> > > i.e. I would
> > > expect it to fall back into the case of I/O tokens redirection rather
> > than
> > > physical addresses redirection (as mentioned below from my previous
> > reply).
> > > 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
> > 
> > >
> > > Thanks
> > >
> > > Gab
> > >
> > >
> > > >
> > > > For the general case you also have to bear in mind that
> > PCIBIOS_MIN_IO
> > > > could
> > > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So
> > it
> > > > 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
> > > > where I/O tokens can't be generated for.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > >
> > > > > > > > Your current version has
> > > > > > > >
> > > > > > > >         if (arm64_extio_ops->pfout)
> > > > \
> > > > > > > >                 arm64_extio_ops->pfout(arm64_extio_ops-
> > > > >devpara,\
> > > > > > > >                        addr, value, sizeof(type));
> > > > \
> > > > > > > >
> > > > > > > > Instead, just subtract the start of the range from the
> > logical
> > > > > > > > port number to transform it back into a bus-local port
> > number:
> > > > > > >
> > > > > > > These accessors do not operate on IO tokens:
> > > > > > >
> > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end <
> > addr)
> > > > > > > addr is not going to be an I/O token; in fact patch 2/3
> > imposes
> > > > that
> > > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > > > > > PCIBIOS_MIN_IO
> > > > > > > we have free physical addresses that the accessors can
> > operate
> > > > on.
> > > > > >
> > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to
> > refer
> > > > to
> > > > > > the logical I/O tokens, the purpose of that macro is really
> > meant
> > > > > > 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,
> > > > > not I/O tokens. For them there is no way to distinguish....
> > > > >
> > > > > >
> > > > > > Note that it's equally likely that whichever next platform
> > needs
> > > > > > non-mapped I/O access like this actually needs them for PCI I/O
> > > > space,
> > > > > > and that will use it on addresses registered to a PCI host
> > bridge.
> > > > >
> > > > > Ok so here you are talking about a platform that has got an I/O
> > range
> > > > > under the PCI host controller, right?
> > > > > And this I/O range cannot be directly memory mapped but needs
> > special
> > > > > redirections for the I/O tokens, right?
> > > > >
> > > > > In this scenario registering the I/O ranges with the forbidden
> > range
> > > > > implemented by the current patch would still allow to redirect
> > I/O
> > > > > 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)
> > > > >
> > > > > 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
> > > > >
> > > > > >
> > > > > > 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!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [next] | [standalone]

#1525150

On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote:
> On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:
> > > 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.).

I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you cannot
have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is different
from having an LPC master outside of PCI, as that lives in its own domain
and has a separately addressable I/O space.

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

The PCIBIOS_MIN_DIRECT_IO name still suggests having something related to
PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple
concepts here that are not the same but that are somewhat related:

a) keeping PCI devices from allocating low I/O ports on the PCI bus
   that would conflict with ISA devices behind a bridge of the
   same bus.

b) reserving the low 0x0-0xfff range of the Linux-internal I/O
   space abstraction to a particular LPC or PCI domain to make
   legacy device drivers work that hardcode a particular port
   number.

c) Redirecting inb/outb to call a domain-specific accessor function
   rather than doing the normal MMIO window for an LPC master or
   more generally any arbitrary LPC or PCI domain that has a
   nonstandard I/O space. 
   [side note: actually if we generalized this, we could avoid
    assigning an MMIO range for the I/O space on the pci-mvebu
    driver, and that would help free up some other remapping
    windows]

I think there is no need to change a) here, we have PCIBIOS_MIN_IO
today and even if we don't need it, there is no obvious downside.
I would also argue that we can ignore b) for the discussion of
the HiSilicon LPC driver, we just need to assign some range
of logical addresses to each domain.

That means solving c) is the important problem here, and it
shouldn't be so hard.  We can do this either with a single
special domain as in the v5 patch series, or by generalizing it
so that any I/O space mapping gets looked up through the device
pointer of the bus master.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1525233

Hi Arnd many thanks for your help here

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 18 November 2016 10:18
> To: liviu.dudau@arm.com
> Cc: Gabriele Paoloni; 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@go ogle.com;
> zhichang.yuan02@gmail.com; Jason Gunthorpe; Thomas Petazzoni
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote:
> > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:
> > > > 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.).
> 
> I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you
> cannot
> have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is
> different
> from having an LPC master outside of PCI, as that lives in its own
> domain
> and has a separately addressable I/O space.

Yes correct so if we go for the single domain solution arch that
have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC
unless we also redefine PCIBIOS_MIN_IO, right?

> 
> > > 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.
> 
> The PCIBIOS_MIN_DIRECT_IO name still suggests having something related
> to
> PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple
> concepts here that are not the same but that are somewhat related:
> 
> a) keeping PCI devices from allocating low I/O ports on the PCI bus
>    that would conflict with ISA devices behind a bridge of the
>    same bus.
> 
> b) reserving the low 0x0-0xfff range of the Linux-internal I/O
>    space abstraction to a particular LPC or PCI domain to make
>    legacy device drivers work that hardcode a particular port
>    number.
> 
> c) Redirecting inb/outb to call a domain-specific accessor function
>    rather than doing the normal MMIO window for an LPC master or
>    more generally any arbitrary LPC or PCI domain that has a
>    nonstandard I/O space.
>    [side note: actually if we generalized this, we could avoid
>     assigning an MMIO range for the I/O space on the pci-mvebu
>     driver, and that would help free up some other remapping
>     windows]
> 
> I think there is no need to change a) here, we have PCIBIOS_MIN_IO
> today and even if we don't need it, there is no obvious downside.
> I would also argue that we can ignore b) for the discussion of
> the HiSilicon LPC driver, we just need to assign some range
> of logical addresses to each domain.
> 
> That means solving c) is the important problem here, and it
> shouldn't be so hard.  We can do this either with a single
> special domain as in the v5 patch series, or by generalizing it
> so that any I/O space mapping gets looked up through the device
> pointer of the bus master.

I am not very on the "generalized" multi-domain solution...
Currently the IO accessors prototypes have an unsigned long addr
as input parameter. If we live in a multi-domain IO system
how can we distinguish inside the accessor which domain addr
belongs to?

Thanks

Gab

> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1525262

On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote:
> > > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:
> > > > > 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.).
> > 
> > I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you
> > cannot
> > have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is
> > different
> > from having an LPC master outside of PCI, as that lives in its own
> > domain
> > and has a separately addressable I/O space.
> 
> Yes correct so if we go for the single domain solution arch that
> have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC
> unless we also redefine PCIBIOS_MIN_IO, right?

This is what I was referring to below as the difference between
a) and b): Setting PCIBIOS_MIN_IO=0 means you cannot have LPC
behind PCI, but it shouldn't stop you from having a separate
LPC bridge.

> > The PCIBIOS_MIN_DIRECT_IO name still suggests having something related
> > to
> > PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple
> > concepts here that are not the same but that are somewhat related:
> > 
> > a) keeping PCI devices from allocating low I/O ports on the PCI bus
> >    that would conflict with ISA devices behind a bridge of the
> >    same bus.
> > 
> > b) reserving the low 0x0-0xfff range of the Linux-internal I/O
> >    space abstraction to a particular LPC or PCI domain to make
> >    legacy device drivers work that hardcode a particular port
> >    number.
> > 
> > c) Redirecting inb/outb to call a domain-specific accessor function
> >    rather than doing the normal MMIO window for an LPC master or
> >    more generally any arbitrary LPC or PCI domain that has a
> >    nonstandard I/O space.
> >    [side note: actually if we generalized this, we could avoid
> >     assigning an MMIO range for the I/O space on the pci-mvebu
> >     driver, and that would help free up some other remapping
> >     windows]
> > 
> > I think there is no need to change a) here, we have PCIBIOS_MIN_IO
> > today and even if we don't need it, there is no obvious downside.
> > I would also argue that we can ignore b) for the discussion of
> > the HiSilicon LPC driver, we just need to assign some range
> > of logical addresses to each domain.
> > 
> > That means solving c) is the important problem here, and it
> > shouldn't be so hard.  We can do this either with a single
> > special domain as in the v5 patch series, or by generalizing it
> > so that any I/O space mapping gets looked up through the device
> > pointer of the bus master.
> 
> I am not very on the "generalized" multi-domain solution...
> Currently the IO accessors prototypes have an unsigned long addr
> as input parameter. If we live in a multi-domain IO system
> how can we distinguish inside the accessor which domain addr
> belongs to?

The easiest change compared to the v5 code would be to walk
a linked list of 'struct extio_ops' structures rather than
assuming there is only ever one of them. I think one of the
earlier versions actually did this.

Another option the IA64 approach mentioned in another subthread
today, looking up the operations based on an index from the
upper bits of the port number. If we do this, we probably
want to do that for all PIO access and replace the entire
virtual address remapping logic with that. I think Bjorn
in the past argued in favor of such an approach, while I
advocated the current scheme for simplicity based on how
every I/O space these days is just memory mapped (which now
turned out to be false, both on powerpc and arm64).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1525287

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 18 November 2016 12:24
> To: Gabriele Paoloni
> Cc: liviu.dudau@arm.com; 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@go og le.com;
> zhichang.yuan02@gmail.com; Jason Gunthorpe; Thomas Petazzoni
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com
> wrote:
> > > > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:
> > > > > > 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.).
> > >
> > > I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that
> you
> > > cannot
> > > have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is
> > > different
> > > from having an LPC master outside of PCI, as that lives in its own
> > > domain
> > > and has a separately addressable I/O space.
> >
> > Yes correct so if we go for the single domain solution arch that
> > have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC
> > unless we also redefine PCIBIOS_MIN_IO, right?
> 
> This is what I was referring to below as the difference between
> a) and b): Setting PCIBIOS_MIN_IO=0 means you cannot have LPC
> behind PCI, but it shouldn't stop you from having a separate
> LPC bridge.
> 
> > > The PCIBIOS_MIN_DIRECT_IO name still suggests having something
> related
> > > to
> > > PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple
> > > concepts here that are not the same but that are somewhat related:
> > >
> > > a) keeping PCI devices from allocating low I/O ports on the PCI bus
> > >    that would conflict with ISA devices behind a bridge of the
> > >    same bus.
> > >
> > > b) reserving the low 0x0-0xfff range of the Linux-internal I/O
> > >    space abstraction to a particular LPC or PCI domain to make
> > >    legacy device drivers work that hardcode a particular port
> > >    number.
> > >
> > > c) Redirecting inb/outb to call a domain-specific accessor function
> > >    rather than doing the normal MMIO window for an LPC master or
> > >    more generally any arbitrary LPC or PCI domain that has a
> > >    nonstandard I/O space.
> > >    [side note: actually if we generalized this, we could avoid
> > >     assigning an MMIO range for the I/O space on the pci-mvebu
> > >     driver, and that would help free up some other remapping
> > >     windows]
> > >
> > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO
> > > today and even if we don't need it, there is no obvious downside.
> > > I would also argue that we can ignore b) for the discussion of
> > > the HiSilicon LPC driver, we just need to assign some range
> > > of logical addresses to each domain.
> > >
> > > That means solving c) is the important problem here, and it
> > > shouldn't be so hard.  We can do this either with a single
> > > special domain as in the v5 patch series, or by generalizing it
> > > so that any I/O space mapping gets looked up through the device
> > > pointer of the bus master.
> >
> > I am not very on the "generalized" multi-domain solution...
> > Currently the IO accessors prototypes have an unsigned long addr
> > as input parameter. If we live in a multi-domain IO system
> > how can we distinguish inside the accessor which domain addr
> > belongs to?
> 
> The easiest change compared to the v5 code would be to walk
> a linked list of 'struct extio_ops' structures rather than
> assuming there is only ever one of them. I think one of the
> earlier versions actually did this.

Right but if my understanding is correct if we live in a multi-
domain I/O space world when you have an input addr in the I/O
accessors this addr can be duplicated (for example for the standard
PCI IO domain and for our special LPC domain).
So effectively even if you walk a linked list there is a problem
of disambiguation...am I right? 

> 
> Another option the IA64 approach mentioned in another subthread
> today, looking up the operations based on an index from the
> upper bits of the port number. If we do this, we probably
> want to do that for all PIO access and replace the entire
> virtual address remapping logic with that. I think Bjorn
> in the past argued in favor of such an approach, while I
> advocated the current scheme for simplicity based on how
> every I/O space these days is just memory mapped (which now
> turned out to be false, both on powerpc and arm64).

This seems really complex...I am a bit worried that possibly
we end up in having the maintainers saying that it is not worth
to re-invent the world just for this special LPC device...

To be honest with you I would keep things simple for this
LPC and introduce more complex reworks later if more devices
need to be introduced.

What if we stick on a single domain now where we introduce a
reserved threshold for the IO space (say INDIRECT_MAX_IO).

We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and
we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h"

So effectively this threshold can change according to the
architecture and so far we only define it for ARM64 as we need
it for ARM64...

Thoughts?

Thanks again

Gab

> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1525347

On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote:
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote:
> > > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO
> > > > today and even if we don't need it, there is no obvious downside.
> > > > I would also argue that we can ignore b) for the discussion of
> > > > the HiSilicon LPC driver, we just need to assign some range
> > > > of logical addresses to each domain.
> > > >
> > > > That means solving c) is the important problem here, and it
> > > > shouldn't be so hard.  We can do this either with a single
> > > > special domain as in the v5 patch series, or by generalizing it
> > > > so that any I/O space mapping gets looked up through the device
> > > > pointer of the bus master.
> > >
> > > I am not very on the "generalized" multi-domain solution...
> > > Currently the IO accessors prototypes have an unsigned long addr
> > > as input parameter. If we live in a multi-domain IO system
> > > how can we distinguish inside the accessor which domain addr
> > > belongs to?
> > 
> > The easiest change compared to the v5 code would be to walk
> > a linked list of 'struct extio_ops' structures rather than
> > assuming there is only ever one of them. I think one of the
> > earlier versions actually did this.
> 
> Right but if my understanding is correct if we live in a multi-
> domain I/O space world when you have an input addr in the I/O
> accessors this addr can be duplicated (for example for the standard
> PCI IO domain and for our special LPC domain).
> So effectively even if you walk a linked list there is a problem
> of disambiguation...am I right? 

No, unlike the PCI memory space, the PIO addresses are not
usually distinct, i.e. every PCI bus has its own 64K I/O
addresses starting at zero. We linearize them into the
Linux I/O space using the per-domain io_offset value.

For the ISA/LPC spaces there are only 4k of addresses, they
the bus addresses always overlap, but we can trivially
figure out the bus address from Linux I/O port number
by subtracting the start of the range.

> > Another option the IA64 approach mentioned in another subthread
> > today, looking up the operations based on an index from the
> > upper bits of the port number. If we do this, we probably
> > want to do that for all PIO access and replace the entire
> > virtual address remapping logic with that. I think Bjorn
> > in the past argued in favor of such an approach, while I
> > advocated the current scheme for simplicity based on how
> > every I/O space these days is just memory mapped (which now
> > turned out to be false, both on powerpc and arm64).
> 
> This seems really complex...I am a bit worried that possibly
> we end up in having the maintainers saying that it is not worth
> to re-invent the world just for this special LPC device...

It would clearly be a rather invasive change, but the
end-result isn't necessarily more complex than what we
have today, as we'd kill off the crazy pci_pio_to_address()
and pci_address_to_pio() hacks in address translation.

> To be honest with you I would keep things simple for this
> LPC and introduce more complex reworks later if more devices
> need to be introduced.
> 
> What if we stick on a single domain now where we introduce a
> reserved threshold for the IO space (say INDIRECT_MAX_IO).

I said having a single domain is fine, but I still don't
like the idea of reserving low port numbers for this hack,
it would mean that the numbers change for everyone else.

> We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and
> we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h"
> 
> So effectively this threshold can change according to the
> architecture and so far we only define it for ARM64 as we need
> it for ARM64...

I liked the idea of having it done in asm-generic/io.h (in an ifdef)
and lib/*.c under an as someone suggested earlier. There is nothing
ARM64 specific in the implementation.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1525486

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 18 November 2016 13:43
> To: linux-arm-kernel@lists.infradead.org
> Cc: Gabriele Paoloni; mark.rutland@arm.com; benh@kernel.crashing.org;
> catalin.marinas@arm.com; liviu.dudau@arm.com; Linuxarm;
> lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux-
> serial@vger.kernel.org; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John
> Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og
> le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; Thomas Petazzoni;
> linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> wrote:
> > > > > I think there is no need to change a) here, we have
> PCIBIOS_MIN_IO
> > > > > today and even if we don't need it, there is no obvious
> downside.
> > > > > I would also argue that we can ignore b) for the discussion of
> > > > > the HiSilicon LPC driver, we just need to assign some range
> > > > > of logical addresses to each domain.
> > > > >
> > > > > That means solving c) is the important problem here, and it
> > > > > shouldn't be so hard.  We can do this either with a single
> > > > > special domain as in the v5 patch series, or by generalizing it
> > > > > so that any I/O space mapping gets looked up through the device
> > > > > pointer of the bus master.
> > > >
> > > > I am not very on the "generalized" multi-domain solution...
> > > > Currently the IO accessors prototypes have an unsigned long addr
> > > > as input parameter. If we live in a multi-domain IO system
> > > > how can we distinguish inside the accessor which domain addr
> > > > belongs to?
> > >
> > > The easiest change compared to the v5 code would be to walk
> > > a linked list of 'struct extio_ops' structures rather than
> > > assuming there is only ever one of them. I think one of the
> > > earlier versions actually did this.
> >
> > Right but if my understanding is correct if we live in a multi-
> > domain I/O space world when you have an input addr in the I/O
> > accessors this addr can be duplicated (for example for the standard
> > PCI IO domain and for our special LPC domain).
> > So effectively even if you walk a linked list there is a problem
> > of disambiguation...am I right?
> 
> No, unlike the PCI memory space, the PIO addresses are not
> usually distinct, i.e. every PCI bus has its own 64K I/O
> addresses starting at zero. We linearize them into the
> Linux I/O space using the per-domain io_offset value.

I am going to summarize my understanding here below:

It seems to me that what is linearized is the virtual address
space associated to the IO address space. This address space
goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).

The I/O accessors perform rd/wr operation on this address
space using a port IO token.

Each token map into a cpu physical address range
Each cpu physical address range maps to a bus address range
if the bus controller specifies a range property.

Devices under a bus controller specify the bus addresses that
they operate on in their reg property.

So each device can use the same bus addresses under two
separate bus controllers as long as the bus controller
use the range properties to map the same bus range to different
cpu address range. 

> 
> For the ISA/LPC spaces there are only 4k of addresses, they
> the bus addresses always overlap, but we can trivially
> figure out the bus address from Linux I/O port number
> by subtracting the start of the range.

Are you saying that our LPC controller should specify a
range property to map bus addresses into a cpu address range? 

> 
> > > Another option the IA64 approach mentioned in another subthread
> > > today, looking up the operations based on an index from the
> > > upper bits of the port number. If we do this, we probably
> > > want to do that for all PIO access and replace the entire
> > > virtual address remapping logic with that. I think Bjorn
> > > in the past argued in favor of such an approach, while I
> > > advocated the current scheme for simplicity based on how
> > > every I/O space these days is just memory mapped (which now
> > > turned out to be false, both on powerpc and arm64).
> >
> > This seems really complex...I am a bit worried that possibly
> > we end up in having the maintainers saying that it is not worth
> > to re-invent the world just for this special LPC device...
> 
> It would clearly be a rather invasive change, but the
> end-result isn't necessarily more complex than what we
> have today, as we'd kill off the crazy pci_pio_to_address()
> and pci_address_to_pio() hacks in address translation.

I have to look better into this...can you provide me a reference
to the Bjorn argument in favor of this approach?

> 
> > To be honest with you I would keep things simple for this
> > LPC and introduce more complex reworks later if more devices
> > need to be introduced.
> >
> > What if we stick on a single domain now where we introduce a
> > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> 
> I said having a single domain is fine, but I still don't
> like the idea of reserving low port numbers for this hack,
> it would mean that the numbers change for everyone else.

I don't get this much...I/O tokens that are passed to the I/O
accessors are not fixed anyway and they vary depending on the order
of adding ranges to io_range_list...so I don't see a big issue
with this...

> 
> > We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and
> > we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h"
> >
> > So effectively this threshold can change according to the
> > architecture and so far we only define it for ARM64 as we need
> > it for ARM64...
> 
> I liked the idea of having it done in asm-generic/io.h (in an ifdef)
> and lib/*.c under an as someone suggested earlier. There is nothing
> ARM64 specific in the implementation.

Correct and ideally if the INDIRECT_MAX_IO approach was taken then we
should define INDIRECT_MAX_IO for any architecture that can support the
special LPC devices. We would define it for ARM64 just because LPC
is used in our case under ARM64; the idea was to leave other architecture
to define their own ones as needed...but probably this is wrong and
we should have defined this for all the architectures.

Many thanks

Gab

> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1525499

On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> > > > The easiest change compared to the v5 code would be to walk
> > > > a linked list of 'struct extio_ops' structures rather than
> > > > assuming there is only ever one of them. I think one of the
> > > > earlier versions actually did this.
> > >
> > > Right but if my understanding is correct if we live in a multi-
> > > domain I/O space world when you have an input addr in the I/O
> > > accessors this addr can be duplicated (for example for the standard
> > > PCI IO domain and for our special LPC domain).
> > > So effectively even if you walk a linked list there is a problem
> > > of disambiguation...am I right?
> > 
> > No, unlike the PCI memory space, the PIO addresses are not
> > usually distinct, i.e. every PCI bus has its own 64K I/O
> > addresses starting at zero. We linearize them into the
> > Linux I/O space using the per-domain io_offset value.
> 
> I am going to summarize my understanding here below:
> 
> It seems to me that what is linearized is the virtual address
> space associated to the IO address space. This address space
> goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).
> 
> The I/O accessors perform rd/wr operation on this address
> space using a port IO token.
> 
> Each token map into a cpu physical address range
> Each cpu physical address range maps to a bus address range
> if the bus controller specifies a range property.
> 
> Devices under a bus controller specify the bus addresses that
> they operate on in their reg property.
> 
> So each device can use the same bus addresses under two
> separate bus controllers as long as the bus controller
> use the range properties to map the same bus range to different
> cpu address range. 

Sounds all correct to me so far, yes.

> > For the ISA/LPC spaces there are only 4k of addresses, they
> > the bus addresses always overlap, but we can trivially
> > figure out the bus address from Linux I/O port number
> > by subtracting the start of the range.
> 
> Are you saying that our LPC controller should specify a
> range property to map bus addresses into a cpu address range? 

No. There is not CPU address associated with it, because it's
not memory mapped.

Instead, we need to associate a bus address with a logical
Linux port number, both in of_address_to_resource and
in inb()/outb().

> > > > Another option the IA64 approach mentioned in another subthread
> > > > today, looking up the operations based on an index from the
> > > > upper bits of the port number. If we do this, we probably
> > > > want to do that for all PIO access and replace the entire
> > > > virtual address remapping logic with that. I think Bjorn
> > > > in the past argued in favor of such an approach, while I
> > > > advocated the current scheme for simplicity based on how
> > > > every I/O space these days is just memory mapped (which now
> > > > turned out to be false, both on powerpc and arm64).
> > >
> > > This seems really complex...I am a bit worried that possibly
> > > we end up in having the maintainers saying that it is not worth
> > > to re-invent the world just for this special LPC device...
> > 
> > It would clearly be a rather invasive change, but the
> > end-result isn't necessarily more complex than what we
> > have today, as we'd kill off the crazy pci_pio_to_address()
> > and pci_address_to_pio() hacks in address translation.
> 
> I have to look better into this...can you provide me a reference
> to the Bjorn argument in favor of this approach?

The thread seems to have been pci: Introduce pci_register_io_range()
helper function, e.g. in https://lkml.org/lkml/2014/7/8/969

> > > To be honest with you I would keep things simple for this
> > > LPC and introduce more complex reworks later if more devices
> > > need to be introduced.
> > >
> > > What if we stick on a single domain now where we introduce a
> > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > 
> > I said having a single domain is fine, but I still don't
> > like the idea of reserving low port numbers for this hack,
> > it would mean that the numbers change for everyone else.
> 
> I don't get this much...I/O tokens that are passed to the I/O
> accessors are not fixed anyway and they vary depending on the order
> of adding ranges to io_range_list...so I don't see a big issue
> with this...

On machines with a legacy devices behind the PCI bridge,
there may still be a reason to have the low I/O port range
reserved for the primary bus, e.g. to get a VGA text console
to work.

On powerpc, this is called the "primary" PCI host, i.e. the
only one that is allowed to have an ISA bridge.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1525553

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 18 November 2016 16:35
> To: Gabriele Paoloni
> Cc: linux-arm-kernel@lists.infradead.org; mark.rutland@arm.com;
> benh@kernel.crashing.org; catalin.marinas@arm.com; liviu.dudau@arm.com;
> Linuxarm; lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux-
> serial@vger.kernel.org; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John
> Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og
> le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; T homas Petazzoni;
> linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni
> wrote:
> > > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> > > > > The easiest change compared to the v5 code would be to walk
> > > > > a linked list of 'struct extio_ops' structures rather than
> > > > > assuming there is only ever one of them. I think one of the
> > > > > earlier versions actually did this.
> > > >
> > > > Right but if my understanding is correct if we live in a multi-
> > > > domain I/O space world when you have an input addr in the I/O
> > > > accessors this addr can be duplicated (for example for the
> standard
> > > > PCI IO domain and for our special LPC domain).
> > > > So effectively even if you walk a linked list there is a problem
> > > > of disambiguation...am I right?
> > >
> > > No, unlike the PCI memory space, the PIO addresses are not
> > > usually distinct, i.e. every PCI bus has its own 64K I/O
> > > addresses starting at zero. We linearize them into the
> > > Linux I/O space using the per-domain io_offset value.
> >
> > I am going to summarize my understanding here below:
> >
> > It seems to me that what is linearized is the virtual address
> > space associated to the IO address space. This address space
> > goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).
> >
> > The I/O accessors perform rd/wr operation on this address
> > space using a port IO token.
> >
> > Each token map into a cpu physical address range
> > Each cpu physical address range maps to a bus address range
> > if the bus controller specifies a range property.
> >
> > Devices under a bus controller specify the bus addresses that
> > they operate on in their reg property.
> >
> > So each device can use the same bus addresses under two
> > separate bus controllers as long as the bus controller
> > use the range properties to map the same bus range to different
> > cpu address range.
> 
> Sounds all correct to me so far, yes.
> 
> > > For the ISA/LPC spaces there are only 4k of addresses, they
> > > the bus addresses always overlap, but we can trivially
> > > figure out the bus address from Linux I/O port number
> > > by subtracting the start of the range.
> >
> > Are you saying that our LPC controller should specify a
> > range property to map bus addresses into a cpu address range?
> 
> No. There is not CPU address associated with it, because it's
> not memory mapped.
> 
> Instead, we need to associate a bus address with a logical
> Linux port number, both in of_address_to_resource and
> in inb()/outb().

I think this is effectively what we are doing so far with patch 2/3.
The problem with this patch is that we are carving out a "forbidden"
IO tokens range that goes from 0 to PCIBIOS_MIN_IO.

I think that the proper solution would be to have the LPC driver to
set the carveout threshold used in pci_register_io_range(), 
pci_pio_to_address(), pci_address_to_pio(), but this would impose
a probe dependency on the LPC itself that should be probed before
the PCI controller (or before any other devices calling these
functions...) 

> 
> > > > > Another option the IA64 approach mentioned in another subthread
> > > > > today, looking up the operations based on an index from the
> > > > > upper bits of the port number. If we do this, we probably
> > > > > want to do that for all PIO access and replace the entire
> > > > > virtual address remapping logic with that. I think Bjorn
> > > > > in the past argued in favor of such an approach, while I
> > > > > advocated the current scheme for simplicity based on how
> > > > > every I/O space these days is just memory mapped (which now
> > > > > turned out to be false, both on powerpc and arm64).
> > > >
> > > > This seems really complex...I am a bit worried that possibly
> > > > we end up in having the maintainers saying that it is not worth
> > > > to re-invent the world just for this special LPC device...
> > >
> > > It would clearly be a rather invasive change, but the
> > > end-result isn't necessarily more complex than what we
> > > have today, as we'd kill off the crazy pci_pio_to_address()
> > > and pci_address_to_pio() hacks in address translation.
> >
> > I have to look better into this...can you provide me a reference
> > to the Bjorn argument in favor of this approach?
> 
> The thread seems to have been pci: Introduce pci_register_io_range()
> helper function, e.g. in https://lkml.org/lkml/2014/7/8/969

Ok many thanks I am going to look at it

> 
> > > > To be honest with you I would keep things simple for this
> > > > LPC and introduce more complex reworks later if more devices
> > > > need to be introduced.
> > > >
> > > > What if we stick on a single domain now where we introduce a
> > > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > >
> > > I said having a single domain is fine, but I still don't
> > > like the idea of reserving low port numbers for this hack,
> > > it would mean that the numbers change for everyone else.
> >
> > I don't get this much...I/O tokens that are passed to the I/O
> > accessors are not fixed anyway and they vary depending on the order
> > of adding ranges to io_range_list...so I don't see a big issue
> > with this...
> 
> On machines with a legacy devices behind the PCI bridge,
> there may still be a reason to have the low I/O port range
> reserved for the primary bus, e.g. to get a VGA text console
> to work.
> 
> On powerpc, this is called the "primary" PCI host, i.e. the
> only one that is allowed to have an ISA bridge.

Yes but
1) isn't the PCI controller range property that defines how IO bus address
   map into physical CPU addresses?
2) How can you guarantee that the cpu range associated with this
   IO bus range is the first to be registered in pci_register_io_range()?
   ( i.e. are you saying that they are just relying on the fact that it is the
     only IO range in the system and by chance the IO tokens and corresponding
     bus addresses are the same? )

Thanks

Gab

> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1528436

On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:
> > On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni
> > wrote:
> > > > For the ISA/LPC spaces there are only 4k of addresses, they
> > > > the bus addresses always overlap, but we can trivially
> > > > figure out the bus address from Linux I/O port number
> > > > by subtracting the start of the range.
> > >
> > > Are you saying that our LPC controller should specify a
> > > range property to map bus addresses into a cpu address range?
> > 
> > No. There is not CPU address associated with it, because it's
> > not memory mapped.
> > 
> > Instead, we need to associate a bus address with a logical
> > Linux port number, both in of_address_to_resource and
> > in inb()/outb().
> 
> I think this is effectively what we are doing so far with patch 2/3.
> The problem with this patch is that we are carving out a "forbidden"
> IO tokens range that goes from 0 to PCIBIOS_MIN_IO.
> 
> I think that the proper solution would be to have the LPC driver to
> set the carveout threshold used in pci_register_io_range(), 
> pci_pio_to_address(), pci_address_to_pio(), but this would impose
> a probe dependency on the LPC itself that should be probed before
> the PCI controller (or before any other devices calling these
> functions...)

Why do you think the order matters? My point was that we should
be able to register any region of logical port numbers for any
bus here.


> > > > > To be honest with you I would keep things simple for this
> > > > > LPC and introduce more complex reworks later if more devices
> > > > > need to be introduced.
> > > > >
> > > > > What if we stick on a single domain now where we introduce a
> > > > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > > >
> > > > I said having a single domain is fine, but I still don't
> > > > like the idea of reserving low port numbers for this hack,
> > > > it would mean that the numbers change for everyone else.
> > >
> > > I don't get this much...I/O tokens that are passed to the I/O
> > > accessors are not fixed anyway and they vary depending on the order
> > > of adding ranges to io_range_list...so I don't see a big issue
> > > with this...
> > 
> > On machines with a legacy devices behind the PCI bridge,
> > there may still be a reason to have the low I/O port range
> > reserved for the primary bus, e.g. to get a VGA text console
> > to work.
> > 
> > On powerpc, this is called the "primary" PCI host, i.e. the
> > only one that is allowed to have an ISA bridge.
> 
> Yes but
> 1) isn't the PCI controller range property that defines how IO bus address
>    map into physical CPU addresses?

Correct, but the DT knows nothing about logical port numbers in Linux.

> 2) How can you guarantee that the cpu range associated with this
>    IO bus range is the first to be registered in pci_register_io_range()?
>    ( i.e. are you saying that they are just relying on the fact that it is the
>      only IO range in the system and by chance the IO tokens and corresponding
>      bus addresses are the same? )

To clarify: the special properties of having the first 0x1000 logical
port numbers go to a particular physical bus are very obscure. I think
it's more important to not change the behavior for existing systems
that might rely on it than for new systems that have no such legacy.

The ipmi and uart drivers in particular will get the port numbers filled
in their platform device from the DT bus scanning, so they don't care
at all about having the same numeric value for port numbers on the bus
and logical numbers, but other drivers might rely on particular ports
to be mapped on a specific PCI host, especially when those drivers
are  used only on systems that don't have more than one PCI domain.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1528501

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 November 2016 14:16
> To: Gabriele Paoloni
> Cc: linux-arm-kernel@lists.infradead.org; mark.rutland@arm.com;
> benh@kernel.crashing.org; catalin.marinas@arm.com; liviu.dudau@arm.com;
> Linuxarm; lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux-
> serial@vger.kernel.org; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John
> Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og
> le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; T homas Petazzoni;
> linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:
> > > On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> > > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni
> > > wrote:
> > > > > For the ISA/LPC spaces there are only 4k of addresses, they
> > > > > the bus addresses always overlap, but we can trivially
> > > > > figure out the bus address from Linux I/O port number
> > > > > by subtracting the start of the range.
> > > >
> > > > Are you saying that our LPC controller should specify a
> > > > range property to map bus addresses into a cpu address range?
> > >
> > > No. There is not CPU address associated with it, because it's
> > > not memory mapped.
> > >
> > > Instead, we need to associate a bus address with a logical
> > > Linux port number, both in of_address_to_resource and
> > > in inb()/outb().
> >
> > I think this is effectively what we are doing so far with patch 2/3.
> > The problem with this patch is that we are carving out a "forbidden"
> > IO tokens range that goes from 0 to PCIBIOS_MIN_IO.
> >
> > I think that the proper solution would be to have the LPC driver to
> > set the carveout threshold used in pci_register_io_range(),
> > pci_pio_to_address(), pci_address_to_pio(), but this would impose
> > a probe dependency on the LPC itself that should be probed before
> > the PCI controller (or before any other devices calling these
> > functions...)
> 
> Why do you think the order matters? My point was that we should
> be able to register any region of logical port numbers for any
> bus here.

Maybe I have not followed well so let's roll back to your previous
comment...

"we need to associate a bus address with a logical Linux port number,
both in of_address_to_resource and in inb()/outb()"

Actually of_address_to_resource() returns the port number to used
in inb/outb(); inb() and outb() add the port number to PCI_IOBASE
to rd/wr to the right virtual address.

Our LPC cannot operate on the virtual address and it operates on
a bus address range that for LPC is also equal to the cpu address
range and goes from 0 to 0x1000.

Now as I understand it is risky and not appropriate to reserve
the logical port numbers from 0 to 0x1000 or to whatever other
upper bound because existing systems may rely on these port numbers
retrieved by __of_address_to_resource().

In this scenario I think the best thing to do would be
in the probe function of the LPC driver:
1) call pci_register_io_range() passing [0, 0x1000] (that is the
   range for LPC)
2) retrieve the logical port numbers associated to the LPC range
   by calling pci_address_to_pio() for 0 and 0x1000 and assign
   them to extio_ops_node->start and extio_ops_node->end
3) implement the LPC accessors to operate on the logical ports
   associated to the LPC range (in practice in the accessors
   implementation we will call pci_pio_to_address to retrieve
   the cpu address to operate on)

What do you think?

Thanks

Gab


> 
>
> > > > > > To be honest with you I would keep things simple for this
> > > > > > LPC and introduce more complex reworks later if more devices
> > > > > > need to be introduced.
> > > > > >
> > > > > > What if we stick on a single domain now where we introduce a
> > > > > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > > > >
> > > > > I said having a single domain is fine, but I still don't
> > > > > like the idea of reserving low port numbers for this hack,
> > > > > it would mean that the numbers change for everyone else.
> > > >
> > > > I don't get this much...I/O tokens that are passed to the I/O
> > > > accessors are not fixed anyway and they vary depending on the
> order
> > > > of adding ranges to io_range_list...so I don't see a big issue
> > > > with this...
> > >
> > > On machines with a legacy devices behind the PCI bridge,
> > > there may still be a reason to have the low I/O port range
> > > reserved for the primary bus, e.g. to get a VGA text console
> > > to work.
> > >
> > > On powerpc, this is called the "primary" PCI host, i.e. the
> > > only one that is allowed to have an ISA bridge.
> >
> > Yes but
> > 1) isn't the PCI controller range property that defines how IO bus
> address
> >    map into physical CPU addresses?
> 
> Correct, but the DT knows nothing about logical port numbers in Linux.
> 
> > 2) How can you guarantee that the cpu range associated with this
> >    IO bus range is the first to be registered in
> pci_register_io_range()?
> >    ( i.e. are you saying that they are just relying on the fact that
> it is the
> >      only IO range in the system and by chance the IO tokens and
> corresponding
> >      bus addresses are the same? )
> 
> To clarify: the special properties of having the first 0x1000 logical
> port numbers go to a particular physical bus are very obscure. I think
> it's more important to not change the behavior for existing systems
> that might rely on it than for new systems that have no such legacy.
> 
> The ipmi and uart drivers in particular will get the port numbers
> filled
> in their platform device from the DT bus scanning, so they don't care
> at all about having the same numeric value for port numbers on the bus
> and logical numbers, but other drivers might rely on particular ports
> to be mapped on a specific PCI host, especially when those drivers
> are  used only on systems that don't have more than one PCI domain.
> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1528607

On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni wrote:
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:

> > > I think this is effectively what we are doing so far with patch 2/3.
> > > The problem with this patch is that we are carving out a "forbidden"
> > > IO tokens range that goes from 0 to PCIBIOS_MIN_IO.
> > >
> > > I think that the proper solution would be to have the LPC driver to
> > > set the carveout threshold used in pci_register_io_range(),
> > > pci_pio_to_address(), pci_address_to_pio(), but this would impose
> > > a probe dependency on the LPC itself that should be probed before
> > > the PCI controller (or before any other devices calling these
> > > functions...)
> > 
> > Why do you think the order matters? My point was that we should
> > be able to register any region of logical port numbers for any
> > bus here.
> 
> Maybe I have not followed well so let's roll back to your previous
> comment...
> 
> "we need to associate a bus address with a logical Linux port number,
> both in of_address_to_resource and in inb()/outb()"
> 
> Actually of_address_to_resource() returns the port number to used
> in inb/outb(); inb() and outb() add the port number to PCI_IOBASE
> to rd/wr to the right virtual address.

Correct.

> Our LPC cannot operate on the virtual address and it operates on
> a bus address range that for LPC is also equal to the cpu address
> range and goes from 0 to 0x1000.

There is no "cpu address" here, otherwise this is correct.

> Now as I understand it is risky and not appropriate to reserve
> the logical port numbers from 0 to 0x1000 or to whatever other
> upper bound because existing systems may rely on these port numbers
> retrieved by __of_address_to_resource().

Right again.

> In this scenario I think the best thing to do would be
> in the probe function of the LPC driver:
> 1) call pci_register_io_range() passing [0, 0x1000] (that is the
>    range for LPC)

pci_register_io_range() takes a physical address, not a port number,
so that would not be appropriate as you say above. We can however
add a variant that reserves a range of port numbers in io_range_list
for an indirect access method.

> 2) retrieve the logical port numbers associated to the LPC range
>    by calling pci_address_to_pio() for 0 and 0x1000 and assign
>    them to extio_ops_node->start and extio_ops_node->end

Again, calling pci_address_to_pio() doesn't seem right here, because
we don't have a phys_addr_t address

> 3) implement the LPC accessors to operate on the logical ports
>    associated to the LPC range (in practice in the accessors
>    implementation we will call pci_pio_to_address to retrieve
>    the cpu address to operate on)

Please don't proliferate the use of
pci_pio_to_address/pci_address_to_pio here, computing the physical
address from the logical address is trivial, you just need to
subtract the start of the range that you already use when matching
the port number range.

The only thing we need here is to make of_address_to_resource()
return the correct logical port number that was registered for
a given host device when asked to translate an address that
does not have a CPU address associated with it.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1528863

On Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:
> On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:
>
> Please don't proliferate the use of
> pci_pio_to_address/pci_address_to_pio here, computing the physical
> address from the logical address is trivial, you just need to
> subtract the start of the range that you already use when matching
> the port number range.
> 
> The only thing we need here is to make of_address_to_resource()
> return the correct logical port number that was registered for
> a given host device when asked to translate an address that
> does not have a CPU address associated with it.

Ok, I admit this was a little harder than I expected, but see below
for a rough outline of how I think it can be done.

This makes it possible to translate bus specific I/O port numbers
from device nodes into Linux port numbers, and gives a way to register
them. We could take this further and completely remove pci_pio_to_address
and pci_address_to_pio if we make the I/O port translation always
go through the io_range list, looking up up the hostbridge by fwnode,
but we don't have to do that now.

The patch is completely untested and probably buggy, it just seemed
easier to put out a prototype than to keep going in circles with the
discussion.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index bf601d4df8cf..6cadf0501bb0 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct device *dev,
 	}
 }
 
-static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
+static void acpi_pci_root_remap_iospace(struct fwnode_handle *node,
+					struct resource_entry *entry)
 {
 #ifdef PCI_IOBASE
 	struct resource *res = entry->res;
@@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
 	resource_size_t length = resource_size(res);
 	unsigned long port;
 
-	if (pci_register_io_range(cpu_addr, length))
-		goto err;
-
-	port = pci_address_to_pio(cpu_addr);
-	if (port == (unsigned long)-1)
+	if (pci_register_io_range(node, cpu_addr, length, &port))
 		goto err;
 
 	res->start = port;
@@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 	else {
 		resource_list_for_each_entry_safe(entry, tmp, list) {
 			if (entry->res->flags & IORESOURCE_IO)
-				acpi_pci_root_remap_iospace(entry);
+				acpi_pci_root_remap_iospace(&device->fwnode,
+							    entry);
 
 			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index a50025a3777f..df96955a43f8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -760,8 +760,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		set_bit(NBD_RUNNING, &nbd->runtime_flags);
 		blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
 		args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
-		if (!args)
+		if (!args) {
+			error = -ENOMEM;
 			goto out_err;
+		}
 		nbd->task_recv = current;
 		mutex_unlock(&nbd->config_lock);
 
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..5decaba96eed 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -2,6 +2,7 @@
 #define pr_fmt(fmt)	"OF: " fmt
 
 #include <linux/device.h>
+#include <linux/fwnode.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
@@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range *range,
 
 	if (res->flags & IORESOURCE_IO) {
 		unsigned long port;
-		err = pci_register_io_range(range->cpu_addr, range->size);
+		err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size, &port);
 		if (err)
 			goto invalid_range;
-		port = pci_address_to_pio(range->cpu_addr);
-		if (port == (unsigned long)-1) {
-			err = -EINVAL;
-			goto invalid_range;
-		}
 		res->start = port;
 	} else {
 		if ((sizeof(resource_size_t) < 8) &&
@@ -479,7 +475,7 @@ static int of_empty_ranges_quirk(struct device_node *np)
 	return false;
 }
 
-static int of_translate_one(struct device_node *parent, struct of_bus *bus,
+static u64 of_translate_one(struct device_node *parent, struct of_bus *bus,
 			    struct of_bus *pbus, __be32 *addr,
 			    int na, int ns, int pna, const char *rprop)
 {
@@ -507,7 +503,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 	ranges = of_get_property(parent, rprop, &rlen);
 	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
 		pr_debug("no ranges; cannot translate\n");
-		return 1;
+		return OF_BAD_ADDR;
 	}
 	if (ranges == NULL || rlen == 0) {
 		offset = of_read_number(addr, na);
@@ -528,7 +524,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 	}
 	if (offset == OF_BAD_ADDR) {
 		pr_debug("not found !\n");
-		return 1;
+		return offset;
 	}
 	memcpy(addr, ranges + na, 4 * pna);
 
@@ -537,7 +533,10 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 	pr_debug("with offset: %llx\n", (unsigned long long)offset);
 
 	/* Translate it into parent bus space */
-	return pbus->translate(addr, offset, pna);
+	if (pbus->translate(addr, offset, pna))
+		return OF_BAD_ADDR;
+
+	return offset;
 }
 
 /*
@@ -549,9 +548,14 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
  * that translation is impossible (that is we are not dealing with a value
  * that can be mapped to a cpu physical address). This is not really specified
  * that way, but this is traditionally the way IBM at least do things
+ *
+ * Whenever the translation fails, the *host pointer will be set to the
+ * device that lacks a tranlation, and the return code is relative to
+ * that node.
  */
 static u64 __of_translate_address(struct device_node *dev,
-				  const __be32 *in_addr, const char *rprop)
+				  const __be32 *in_addr, const char *rprop,
+				  struct device_node **host)
 {
 	struct device_node *parent = NULL;
 	struct of_bus *bus, *pbus;
@@ -564,6 +568,7 @@ static u64 __of_translate_address(struct device_node *dev,
 	/* Increase refcount at current level */
 	of_node_get(dev);
 
+	*host = NULL;
 	/* Get parent & match bus type */
 	parent = of_get_parent(dev);
 	if (parent == NULL)
@@ -600,8 +605,9 @@ static u64 __of_translate_address(struct device_node *dev,
 		pbus = of_match_bus(parent);
 		pbus->count_cells(dev, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
-			pr_err("Bad cell count for %s\n",
-			       of_node_full_name(dev));
+			pr_debug("Bad cell count for %s\n",
+				 of_node_full_name(dev));
+			*host = of_node_get(parent);
 			break;
 		}
 
@@ -609,7 +615,9 @@ static u64 __of_translate_address(struct device_node *dev,
 		    pbus->name, pna, pns, of_node_full_name(parent));
 
 		/* Apply bus translation */
-		if (of_translate_one(dev, bus, pbus, addr, na, ns, pna, rprop))
+		result = of_translate_one(dev, bus, pbus, addr, na, ns,
+					  pna, rprop);
+		if (result == OF_BAD_ADDR)
 			break;
 
 		/* Complete the move up one level */
@@ -628,13 +636,32 @@ static u64 __of_translate_address(struct device_node *dev,
 
 u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
 {
-	return __of_translate_address(dev, in_addr, "ranges");
+	struct device_node *host;
+	u64 ret;
+
+	ret =  __of_translate_address(dev, in_addr, "ranges", &host);
+	if (host) {
+		of_node_put(host);
+		return OF_BAD_ADDR;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(of_translate_address);
 
 u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
 {
-	return __of_translate_address(dev, in_addr, "dma-ranges");
+	struct device_node *host;
+	u64 ret;
+
+	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
+
+	if (host) {
+		of_node_put(host);
+		return OF_BAD_ADDR;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(of_translate_dma_address);
 
@@ -676,29 +703,48 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+extern unsigned long extio_translate(struct fwnode_handle *node, unsigned long offset);
+
+u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr)
+{
+	u64 taddr;
+	unsigned long port;
+	struct device_node *host;
+
+	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
+	if (host) {
+		/* host specific port access */
+		port = extio_translate(&host->fwnode, taddr);
+		of_node_put(host);
+	} else {
+		/* memory mapped I/O range */
+		port = pci_address_to_pio(taddr);
+		if (port == (unsigned long)-1)
+			return OF_BAD_ADDR;
+	}
+
+	return port;
+}
+
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
 		const char *name, struct resource *r)
 {
 	u64 taddr;
 
-	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
+	if (flags & IORESOURCE_MEM)
+		taddr = of_translate_address(dev, addrp);
+	else if (flags & IORESOURCE_IO)
+		taddr = of_translate_ioport(dev, addrp);
+	else
 		return -EINVAL;
-	taddr = of_translate_address(dev, addrp);
+
 	if (taddr == OF_BAD_ADDR)
 		return -EINVAL;
 	memset(r, 0, sizeof(struct resource));
-	if (flags & IORESOURCE_IO) {
-		unsigned long port;
-		port = pci_address_to_pio(taddr);
-		if (port == (unsigned long)-1)
-			return -EINVAL;
-		r->start = port;
-		r->end = port + size - 1;
-	} else {
-		r->start = taddr;
-		r->end = taddr + size - 1;
-	}
+
+	r->start = taddr;
+	r->end = taddr + size - 1;
 	r->flags = flags;
 	r->name = name ? name : dev->full_name;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index eda6a7cf0e54..320ab9fbf6af 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3249,6 +3249,7 @@ EXPORT_SYMBOL(pci_request_regions_exclusive);
 #ifdef PCI_IOBASE
 struct io_range {
 	struct list_head list;
+	struct fwnode_handle *node;
 	phys_addr_t start;
 	resource_size_t size;
 };
@@ -3257,11 +3258,14 @@ static LIST_HEAD(io_range_list);
 static DEFINE_SPINLOCK(io_range_lock);
 #endif
 
+#define IO_RANGE_IOEXT (resource_size_t)(-1ull)
+
 /*
  * Record the PCI IO range (expressed as CPU physical address + size).
  * Return a negative value if an error has occured, zero otherwise
  */
-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
+				 resource_size_t size, unsigned long *port)
 {
 	int err = 0;
 
@@ -3272,7 +3276,12 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
 	/* check if the range hasn't been previously recorded */
 	spin_lock(&io_range_lock);
 	list_for_each_entry(range, &io_range_list, list) {
-		if (addr >= range->start && addr + size <= range->start + size) {
+		if (node == range->node)
+			goto end_register;
+
+		if (addr != IO_RANGE_IOEXT &&
+		    addr >= range->start &&
+		    addr + size <= range->start + size) {
 			/* range already registered, bail out */
 			goto end_register;
 		}
@@ -3298,6 +3307,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
 		goto end_register;
 	}
 
+	range->node = node;
 	range->start = addr;
 	range->size = size;
 
@@ -3305,11 +3315,26 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
 
 end_register:
 	spin_unlock(&io_range_lock);
+
+	*port = allocated_size;
+#else
+	/*
+	 * powerpc and microblaze have their own registration,
+	 * just look up the value here
+	 */
+	*port = pci_address_to_pio(addr);
 #endif
 
 	return err;
 }
 
+#ifdef CONFIG_IOEXT
+int ioext_register_io_range
+{
+	return pci_register_io_range(node, IO_RANGE_IOEXT, size, port);
+}
+#endif
+
 phys_addr_t pci_pio_to_address(unsigned long pio)
 {
 	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6bd94a803e8f..b7a8fa3da3ca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1192,7 +1192,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			void *alignf_data);
 
 
-int pci_register_io_range(phys_addr_t addr, resource_size_t size);
+int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
+			  resource_size_t size, unsigned long *port);
 unsigned long pci_address_to_pio(phys_addr_t addr);
 phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);

[toc] | [prev] | [next] | [standalone]

#1529092

Hi, Arnd,

Thanks you very much!

To understand your idea more clear, I have some questions on your patch sketch.
Please check it below.


On 2016/11/24 7:23, Arnd Bergmann wrote:
> On Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:
>> On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni wrote:
>>> From: Arnd Bergmann [mailto:arnd@arndb.de]
>>>> On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:
>>
>> Please don't proliferate the use of
>> pci_pio_to_address/pci_address_to_pio here, computing the physical
>> address from the logical address is trivial, you just need to
>> subtract the start of the range that you already use when matching
>> the port number range.
>>
>> The only thing we need here is to make of_address_to_resource()
>> return the correct logical port number that was registered for
>> a given host device when asked to translate an address that
>> does not have a CPU address associated with it.
> 
> Ok, I admit this was a little harder than I expected, but see below
> for a rough outline of how I think it can be done.
> 
> This makes it possible to translate bus specific I/O port numbers
> from device nodes into Linux port numbers, and gives a way to register
> them. We could take this further and completely remove pci_pio_to_address
> and pci_address_to_pio if we make the I/O port translation always
> go through the io_range list, looking up up the hostbridge by fwnode,
> but we don't have to do that now.
> 
> The patch is completely untested and probably buggy, it just seemed
> easier to put out a prototype than to keep going in circles with the
> discussion.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index bf601d4df8cf..6cadf0501bb0 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct device *dev,
>  	}
>  }
>  
> -static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node,
> +					struct resource_entry *entry)
>  {
>  #ifdef PCI_IOBASE
>  	struct resource *res = entry->res;
> @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
>  	resource_size_t length = resource_size(res);
>  	unsigned long port;
>  
> -	if (pci_register_io_range(cpu_addr, length))
> -		goto err;
> -
> -	port = pci_address_to_pio(cpu_addr);
> -	if (port == (unsigned long)-1)
> +	if (pci_register_io_range(node, cpu_addr, length, &port))
>  		goto err;
>  
>  	res->start = port;
> @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
>  	else {
>  		resource_list_for_each_entry_safe(entry, tmp, list) {
>  			if (entry->res->flags & IORESOURCE_IO)
> -				acpi_pci_root_remap_iospace(entry);
> +				acpi_pci_root_remap_iospace(&device->fwnode,
> +							    entry);
>  
>  			if (entry->res->flags & IORESOURCE_DISABLED)
>  				resource_list_destroy_entry(entry);

I think those changes in pci_root.c is only to match the new definition of
pci_register_io_range() and work for PCI I/O. It doesn't make sense for LPC, is
it right?


> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index a50025a3777f..df96955a43f8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -760,8 +760,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  		set_bit(NBD_RUNNING, &nbd->runtime_flags);
>  		blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
>  		args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
> -		if (!args)
> +		if (!args) {
> +			error = -ENOMEM;
>  			goto out_err;
> +		}
>  		nbd->task_recv = current;
>  		mutex_unlock(&nbd->config_lock);
>  
I think change here is none of the business.:)

> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..5decaba96eed 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -2,6 +2,7 @@
>  #define pr_fmt(fmt)	"OF: " fmt
>  
>  #include <linux/device.h>
> +#include <linux/fwnode.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/module.h>
> @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range *range,
>  
>  	if (res->flags & IORESOURCE_IO) {
>  		unsigned long port;
> -		err = pci_register_io_range(range->cpu_addr, range->size);
> +		err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size, &port);
>  		if (err)
>  			goto invalid_range;
> -		port = pci_address_to_pio(range->cpu_addr);
> -		if (port == (unsigned long)-1) {
> -			err = -EINVAL;
> -			goto invalid_range;
> -		}
>  		res->start = port;
>  	} else {
>  		if ((sizeof(resource_size_t) < 8) &&
> @@ -479,7 +475,7 @@ static int of_empty_ranges_quirk(struct device_node *np)
>  	return false;
>  }
>  
> -static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> +static u64 of_translate_one(struct device_node *parent, struct of_bus *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
>  {
> @@ -507,7 +503,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  	ranges = of_get_property(parent, rprop, &rlen);
>  	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>  		pr_debug("no ranges; cannot translate\n");
> -		return 1;
> +		return OF_BAD_ADDR;
>  	}
>  	if (ranges == NULL || rlen == 0) {
>  		offset = of_read_number(addr, na);
> @@ -528,7 +524,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  	}
>  	if (offset == OF_BAD_ADDR) {
>  		pr_debug("not found !\n");
> -		return 1;
> +		return offset;
>  	}
>  	memcpy(addr, ranges + na, 4 * pna);
>  
> @@ -537,7 +533,10 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  	pr_debug("with offset: %llx\n", (unsigned long long)offset);
>  
>  	/* Translate it into parent bus space */
> -	return pbus->translate(addr, offset, pna);
> +	if (pbus->translate(addr, offset, pna))
> +		return OF_BAD_ADDR;
> +
> +	return offset;
>  }
>  
>  /*
> @@ -549,9 +548,14 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>   * that translation is impossible (that is we are not dealing with a value
>   * that can be mapped to a cpu physical address). This is not really specified
>   * that way, but this is traditionally the way IBM at least do things
> + *
> + * Whenever the translation fails, the *host pointer will be set to the
> + * device that lacks a tranlation, and the return code is relative to
> + * that node.
>   */
>  static u64 __of_translate_address(struct device_node *dev,
> -				  const __be32 *in_addr, const char *rprop)
> +				  const __be32 *in_addr, const char *rprop,
> +				  struct device_node **host)
>  {
>  	struct device_node *parent = NULL;
>  	struct of_bus *bus, *pbus;
> @@ -564,6 +568,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  	/* Increase refcount at current level */
>  	of_node_get(dev);
>  
> +	*host = NULL;
>  	/* Get parent & match bus type */
>  	parent = of_get_parent(dev);
>  	if (parent == NULL)
> @@ -600,8 +605,9 @@ static u64 __of_translate_address(struct device_node *dev,
>  		pbus = of_match_bus(parent);
>  		pbus->count_cells(dev, &pna, &pns);
>  		if (!OF_CHECK_COUNTS(pna, pns)) {
> -			pr_err("Bad cell count for %s\n",
> -			       of_node_full_name(dev));
> +			pr_debug("Bad cell count for %s\n",
> +				 of_node_full_name(dev));
> +			*host = of_node_get(parent);
>  			break;
>  		}
I don't think here is the right place to fill *host. I think you want to return
the parent where the of_translate_one() failed for the 'ranges' property
missing. So, I think this seems better:

if (of_translate_one(dev, bus, pbus, addr, na, ns, pna, rprop)) {
			*host = of_node_get(dev);
			break;
}

>  
> @@ -609,7 +615,9 @@ static u64 __of_translate_address(struct device_node *dev,
>  		    pbus->name, pna, pns, of_node_full_name(parent));
>  
>  		/* Apply bus translation */
> -		if (of_translate_one(dev, bus, pbus, addr, na, ns, pna, rprop))
> +		result = of_translate_one(dev, bus, pbus, addr, na, ns,
> +					  pna, rprop);
> +		if (result == OF_BAD_ADDR)
>  			break;
>  
>  		/* Complete the move up one level */
> @@ -628,13 +636,32 @@ static u64 __of_translate_address(struct device_node *dev,
>  
>  u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret =  __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_address);
>  
>  u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "dma-ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
> +
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_dma_address);
>  
> @@ -676,29 +703,48 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +extern unsigned long extio_translate(struct fwnode_handle *node, unsigned long offset);
> +
> +u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr)
> +{
> +	u64 taddr;
> +	unsigned long port;
> +	struct device_node *host;
> +
> +	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		/* host specific port access */
> +		port = extio_translate(&host->fwnode, taddr);
> +		of_node_put(host);
> +	} else {
> +		/* memory mapped I/O range */
> +		port = pci_address_to_pio(taddr);
> +		if (port == (unsigned long)-1)
> +			return OF_BAD_ADDR;
> +	}
> +
> +	return port;
> +}
> +
>  static int __of_address_to_resource(struct device_node *dev,
>  		const __be32 *addrp, u64 size, unsigned int flags,
>  		const char *name, struct resource *r)
>  {
>  	u64 taddr;
>  
> -	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> +	if (flags & IORESOURCE_MEM)
> +		taddr = of_translate_address(dev, addrp);
> +	else if (flags & IORESOURCE_IO)
> +		taddr = of_translate_ioport(dev, addrp);
> +	else
>  		return -EINVAL;
> -	taddr = of_translate_address(dev, addrp);
> +
>  	if (taddr == OF_BAD_ADDR)
>  		return -EINVAL;
>  	memset(r, 0, sizeof(struct resource));
> -	if (flags & IORESOURCE_IO) {
> -		unsigned long port;
> -		port = pci_address_to_pio(taddr);
> -		if (port == (unsigned long)-1)
> -			return -EINVAL;
> -		r->start = port;
> -		r->end = port + size - 1;
> -	} else {
> -		r->start = taddr;
> -		r->end = taddr + size - 1;
> -	}
> +
> +	r->start = taddr;
> +	r->end = taddr + size - 1;
>  	r->flags = flags;
>  	r->name = name ? name : dev->full_name;
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eda6a7cf0e54..320ab9fbf6af 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3249,6 +3249,7 @@ EXPORT_SYMBOL(pci_request_regions_exclusive);
>  #ifdef PCI_IOBASE
>  struct io_range {
>  	struct list_head list;
> +	struct fwnode_handle *node;
>  	phys_addr_t start;
>  	resource_size_t size;
>  };
> @@ -3257,11 +3258,14 @@ static LIST_HEAD(io_range_list);
>  static DEFINE_SPINLOCK(io_range_lock);
>  #endif
>  
> +#define IO_RANGE_IOEXT (resource_size_t)(-1ull)
> +
>  /*
>   * Record the PCI IO range (expressed as CPU physical address + size).
>   * Return a negative value if an error has occured, zero otherwise
>   */
> -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
> +				 resource_size_t size, unsigned long *port)
>  {
>  	int err = 0;
>  
> @@ -3272,7 +3276,12 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>  	/* check if the range hasn't been previously recorded */
>  	spin_lock(&io_range_lock);
>  	list_for_each_entry(range, &io_range_list, list) {
> -		if (addr >= range->start && addr + size <= range->start + size) {
> +		if (node == range->node)
> +			goto end_register;
> +
I don't think it is safe to only check the node had been registered. For
PCI/PCIE, there is only one I/O windows in bridge, it seems ok. But for non-pci
devices, such as ISA/LPC, I wonder it is possible there are several disjoint I/O
'ranges' entries...

What parameters are necessary for linux PIO allocation?
1) For those bus devices which have no MMIO( that is to say, indirectIO is
using),  I think 'addr' is not needed, but 'size' is mandatory;

I am thinking for our LPC, as there is no cpu address, we should not input
'addr' for the io range register. With 'size' as parameter, we implement a new
io range register function where can assign an unique linux PIO for this
register calling. The output linux PIO can allocate from a sub-range of whole
I/O space of [0, IO_SPACE_LIMIT]. This sub-range is specific for indirectIO, I
want to define a new macro, such as EXTIO_LIMIT, to represent the upper limit of
indirect IO space.

#if defined(CONFIG_PCI) && defined(CONFIG_INDIRECT_PIO)
#define EXTIO_LIMIT	PCIBIOS_MIN_IO
#elif defined(CONFIG_INDIRECT_PIO)
#define EXTIO_LIMIT	0x1000
#else
#define EXTIO_LIMIT 	0x00
#end

We should do some checkings to ensure EXTIO_LIMIT < IO_SPACE_LIMIT.

Then when someone call pci_register_io_range() or a new function for the linux
PIO register, we can allocate linux PIO from [0, EXTIO_LIMIT) for indirectIO
bus, from [EXTIO_LIMIT, IO_SPACE_LIMIT] for MMIO;

But there are issues confused me yet. For example, how to know the IO size for
the indirectIO bus? You known, there is no 'ranges' property for those buses....



2) For PCI MMIO, I think 'addr' is needed

As for the current pci_register_io_range()/pci_address_to_pio(), I have two doubts:

2.1) If there are multiple PCI host bridges which support I/O transaction, I
wonder whether the first host bridge can access the downstream devices with bus
I/O address in [0, PCIBIOS_MIN_IO)

for the first host bridge, pci_address_to_pio() will return a linux PIO range
start from 0.
But when calling __pci_assign_resource() to allocate the linux PIO for PCI/PCIE
devices/buses which are just children of first host bus, it can not allocate
linux PIO less than PCIBIOS_MIN_IO, which means kernel can not called in/out()
with port less than PCIBIOS_MIN_IO. But we had ioremap [PCI_IOBASE + 0,
PCI_IOBASE + size) to [pci_ioadd, pci_ioadd + size) before.


static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
		int resno, resource_size_t size, resource_size_t align)
{
	struct resource *res = dev->resource + resno;
	resource_size_t min;
	int ret;

	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;


and in the later function:

static int pci_bus_alloc_from_region(struct pci_bus *bus, struct resource *res,
		resource_size_t size, resource_size_t align,
		resource_size_t min, unsigned long type_mask,
		resource_size_t (*alignf)(void *,

....
	pci_bus_for_each_resource(bus, r, i) {
		resource_size_t min_used = min;
....
		if (avail.start)
			min_used = avail.start;

		max = avail.end;

		/* Ok, try it out.. */
		ret = allocate_resource(r, res, size, min_used, max,
					align, alignf, alignf_data);

After allocate_resource(), a IO resource is allocated, but whose 'start' is not
less than min_used.( since avail.start is 0, min_used will keep the 'min'
without change to avail.start; Should be PCIBIOS_MIN_IO).

2.2) Is it possible the return linux PIO isn't page-aligned?

When calling pci_remap_iospace(const struct resource *res, phys_addr_t
phys_addr), if res->start is not page-aligned, it seems that
ioremap_page_range() will meet some issues for duplication iorempa for same
virtual page.

of-course, if we always configure the I/O ranges size as page-aligned, it will
be OK.

I found PowerPC will ensure the 'vaddress' and the 'size' are page-aligned
before ioremap, do we need to improve the current handling in
pci_register_io_range/pci_address_to_pio?


Thanks,
Zhichang
> +		if (addr != IO_RANGE_IOEXT &&
> +		    addr >= range->start &&
> +		    addr + size <= range->start + size) {
>  			/* range already registered, bail out */
>  			goto end_register;
>  		}
> @@ -3298,6 +3307,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>  		goto end_register;
>  	}
>  
> +	range->node = node;
>  	range->start = addr;
>  	range->size = size;
>  
> @@ -3305,11 +3315,26 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>  
>  end_register:
>  	spin_unlock(&io_range_lock);
> +
> +	*port = allocated_size;
> +#else
> +	/*
> +	 * powerpc and microblaze have their own registration,
> +	 * just look up the value here
> +	 */
> +	*port = pci_address_to_pio(addr);
>  #endif
>  
>  	return err;
>  }
>  
> +#ifdef CONFIG_IOEXT
> +int ioext_register_io_range
> +{
> +	return pci_register_io_range(node, IO_RANGE_IOEXT, size, port);
> +}
> +#endif
> +
>  phys_addr_t pci_pio_to_address(unsigned long pio)
>  {
>  	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6bd94a803e8f..b7a8fa3da3ca 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1192,7 +1192,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
>  			void *alignf_data);
>  
>  
> -int pci_register_io_range(phys_addr_t addr, resource_size_t size);
> +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
> +			  resource_size_t size, unsigned long *port);
>  unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1529149

On Thursday, November 24, 2016 5:12:49 PM CET zhichang.yuan wrote:
> On 2016/11/24 7:23, Arnd Bergmann wrote:
> > On Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:
> >> On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni wrote:
> >>> From: Arnd Bergmann [mailto:arnd@arndb.de]
> >>>> On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote:
> >>
> > @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> >  	else {
> >  		resource_list_for_each_entry_safe(entry, tmp, list) {
> >  			if (entry->res->flags & IORESOURCE_IO)
> > -				acpi_pci_root_remap_iospace(entry);
> > +				acpi_pci_root_remap_iospace(&device->fwnode,
> > +							    entry);
> >  
> >  			if (entry->res->flags & IORESOURCE_DISABLED)
> >  				resource_list_destroy_entry(entry);
> 
> I think those changes in pci_root.c is only to match the new definition of
> pci_register_io_range() and work for PCI I/O. It doesn't make sense for LPC, is
> it right?

Right, we wouldn't call acpi_pci_probe_root_resources() for LPC,
the change is just that we always pass the fwnode pointer to allow
matching based on that for any I/O space that does not have a
physical memory address associated with it.

I tried to keep this part general, so in theory that allows us to
have more than one I/O space without a CPU mapping, even though
we don't strictly need that for supporting your LPC controller.

> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index a50025a3777f..df96955a43f8 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -760,8 +760,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> >  		set_bit(NBD_RUNNING, &nbd->runtime_flags);
> >  		blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
> >  		args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
> > -		if (!args)
> > +		if (!args) {
> > +			error = -ENOMEM;
> >  			goto out_err;
> > +		}
> >  		nbd->task_recv = current;
> >  		mutex_unlock(&nbd->config_lock);
> >  
> I think change here is none of the business.:)

Right, sorry about that, I forgot to commit this bugfix before looking
at the I/O space stuff.

> > +	*host = NULL;
> >  	/* Get parent & match bus type */
> >  	parent = of_get_parent(dev);
> >  	if (parent == NULL)
> > @@ -600,8 +605,9 @@ static u64 __of_translate_address(struct device_node *dev,
> >  		pbus = of_match_bus(parent);
> >  		pbus->count_cells(dev, &pna, &pns);
> >  		if (!OF_CHECK_COUNTS(pna, pns)) {
> > -			pr_err("Bad cell count for %s\n",
> > -			       of_node_full_name(dev));
> > +			pr_debug("Bad cell count for %s\n",
> > +				 of_node_full_name(dev));
> > +			*host = of_node_get(parent);
> >  			break;
> >  		}
> I don't think here is the right place to fill *host. I think you want to return
> the parent where the of_translate_one() failed for the 'ranges' property
> missing. So, I think this seems better:
> 
> if (of_translate_one(dev, bus, pbus, addr, na, ns, pna, rprop)) {
> 			*host = of_node_get(dev);
> 			break;
> }

You are right, I got the wrong place. The parent node will have 
a #address-cells but won't have ranges for the I/O space.

> > @@ -3272,7 +3276,12 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> >  	/* check if the range hasn't been previously recorded */
> >  	spin_lock(&io_range_lock);
> >  	list_for_each_entry(range, &io_range_list, list) {
> > -		if (addr >= range->start && addr + size <= range->start + size) {
> > +		if (node == range->node)
> > +			goto end_register;
> > +
> I don't think it is safe to only check the node had been registered. For
> PCI/PCIE, there is only one I/O windows in bridge, it seems ok. But for non-pci
> devices, such as ISA/LPC, I wonder it is possible there are several disjoint I/O
> 'ranges' entries...

I think this part is completely safe, I can't imagine why you'd
have more than one range of I/O ports that have valid translations.

Do you have a specific example in mind where that would not be
the case, or are you just worried about the principle in general?

> What parameters are necessary for linux PIO allocation?
> 1) For those bus devices which have no MMIO( that is to say, indirectIO is
> using),  I think 'addr' is not needed, but 'size' is mandatory;

Agreed.

> I am thinking for our LPC, as there is no cpu address, we should not input
> 'addr' for the io range register. With 'size' as parameter, we implement a new
> io range register function where can assign an unique linux PIO for this
> register calling. The output linux PIO can allocate from a sub-range of whole
> I/O space of [0, IO_SPACE_LIMIT]. This sub-range is specific for indirectIO, I
> want to define a new macro, such as EXTIO_LIMIT, to represent the upper limit of
> indirect IO space.
> 
> #if defined(CONFIG_PCI) && defined(CONFIG_INDIRECT_PIO)
> #define EXTIO_LIMIT	PCIBIOS_MIN_IO
> #elif defined(CONFIG_INDIRECT_PIO)
> #define EXTIO_LIMIT	0x1000
> #else
> #define EXTIO_LIMIT 	0x00
> #end
> 
> We should do some checkings to ensure EXTIO_LIMIT < IO_SPACE_LIMIT.

I think we don't need to limit the EXTIO range at all. For your
specific case of LPC, we know it is limited, but my prototype
patch leaves this part generic enough to also allow using it
for a PCI host with indirect I/O space, and that can have a larger
size.

> Then when someone call pci_register_io_range() or a new function for the linux
> PIO register, we can allocate linux PIO from [0, EXTIO_LIMIT) for indirectIO
> bus, from [EXTIO_LIMIT, IO_SPACE_LIMIT] for MMIO;
> 
> But there are issues confused me yet. For example, how to know the IO size for
> the indirectIO bus? You known, there is no 'ranges' property for those buses....

Good point. We normally call pci_register_io_range() from
of_pci_range_to_resource and its ACPI equivalent.

When there is no ranges, we obviously won't call it, but there is
also no size associated with it. I think this is ok because
the host driver would already know the size based on the hardware
register layout, and it can just register that directly.

> 2) For PCI MMIO, I think 'addr' is needed

So far I assumed it was, but actually we can perhaps remove
the address if we manage to kill off pci_address_to_pio()
and pci_pio_to_address.

> As for the current pci_register_io_range()/pci_address_to_pio(), I have two doubts:
> 
> 2.1) If there are multiple PCI host bridges which support I/O transaction, I
> wonder whether the first host bridge can access the downstream devices with bus
> I/O address in [0, PCIBIOS_MIN_IO)
> 
> for the first host bridge, pci_address_to_pio() will return a linux PIO range
> start from 0.
> But when calling __pci_assign_resource() to allocate the linux PIO for PCI/PCIE
> devices/buses which are just children of first host bus, it can not allocate
> linux PIO less than PCIBIOS_MIN_IO, which means kernel can not called in/out()
> with port less than PCIBIOS_MIN_IO. But we had ioremap [PCI_IOBASE + 0,
> PCI_IOBASE + size) to [pci_ioadd, pci_ioadd + size) before.
> 
> 
> static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
> 		int resno, resource_size_t size, resource_size_t align)
> {
> 	struct resource *res = dev->resource + resno;
> 	resource_size_t min;
> 	int ret;
> 
> 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
> 
> 
> and in the later function:
> 
> static int pci_bus_alloc_from_region(struct pci_bus *bus, struct resource *res,
> 		resource_size_t size, resource_size_t align,
> 		resource_size_t min, unsigned long type_mask,
> 		resource_size_t (*alignf)(void *,
> 
> ....
> 	pci_bus_for_each_resource(bus, r, i) {
> 		resource_size_t min_used = min;
> ....
> 		if (avail.start)
> 			min_used = avail.start;
> 
> 		max = avail.end;
> 
> 		/* Ok, try it out.. */
> 		ret = allocate_resource(r, res, size, min_used, max,
> 					align, alignf, alignf_data);
> 
> After allocate_resource(), a IO resource is allocated, but whose 'start' is not
> less than min_used.( since avail.start is 0, min_used will keep the 'min'
> without change to avail.start; Should be PCIBIOS_MIN_IO).

I'm not completely sure I'm following here. Generally speaking, addresses
below PCIBIOS_MIN_IO are intended for PCI-ISA bridges and PCI devices with
hardcoded port numbers for ISA compatibility, while __pci_assign_resource
is meant to do dynamic assignment of I/O resources above PCIBIOS_MIN_IO
so it does not conflict with the legacy ISA ports.

Does that address your concern?

> 2.2) Is it possible the return linux PIO isn't page-aligned?
> 
> When calling pci_remap_iospace(const struct resource *res, phys_addr_t
> phys_addr), if res->start is not page-aligned, it seems that
> ioremap_page_range() will meet some issues for duplication iorempa for same
> virtual page.
> 
> of-course, if we always configure the I/O ranges size as page-aligned, it will
> be OK.
> 
> I found PowerPC will ensure the 'vaddress' and the 'size' are page-aligned
> before ioremap, do we need to improve the current handling in
> pci_register_io_range/pci_address_to_pio?

I think it would be a good idea to enforce page-alignment here, even
though everything could still work if it's not page-aligned.

The requirement for ioremap_page_range() is that the offset within
a page must be the same for the virtual and physical addresses.

Adding page-alignment to pci_register_io_range() could be an enhancement
that we can do independent of the other patches.

Thanks a lot for your detailed analysis and feedback.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1529950

Hi Arnd

Many thanks for your contribution, much appreciated

I have some comments...see inline below 

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 November 2016 23:23
> To: linux-arm-kernel@lists.infradead.org
> Cc: Gabriele Paoloni; mark.rutland@arm.com; catalin.marinas@arm.com;
> linux-pci@vger.kernel.org; liviu.dudau@arm.com; Linuxarm;
> lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; T homas
> Petazzoni; linux-serial@vger.kernel.org; benh@kernel.crashing.org;
> devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John
> Garry; olof@lixom.net; robh+dt@kernel.org; bhelgaas@go og le.com;
> kantyzc@163.com; zhichang.yuan02@gmail.com; linux-
> kernel@vger.kernel.org; Yuanzhichang; zourongrong@gmail.com
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:
> > On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni
> wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni
> wrote:
> >
> > Please don't proliferate the use of
> > pci_pio_to_address/pci_address_to_pio here, computing the physical
> > address from the logical address is trivial, you just need to
> > subtract the start of the range that you already use when matching
> > the port number range.
> >
> > The only thing we need here is to make of_address_to_resource()
> > return the correct logical port number that was registered for
> > a given host device when asked to translate an address that
> > does not have a CPU address associated with it.
> 
> Ok, I admit this was a little harder than I expected, but see below
> for a rough outline of how I think it can be done.
> 
> This makes it possible to translate bus specific I/O port numbers
> from device nodes into Linux port numbers, and gives a way to register
> them. We could take this further and completely remove
> pci_pio_to_address
> and pci_address_to_pio if we make the I/O port translation always
> go through the io_range list, looking up up the hostbridge by fwnode,
> but we don't have to do that now.
> 
> The patch is completely untested and probably buggy, it just seemed
> easier to put out a prototype than to keep going in circles with the
> discussion.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index bf601d4df8cf..6cadf0501bb0 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct
> device *dev,
>  	}
>  }
> 
> -static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node,
> +					struct resource_entry *entry)
>  {
>  #ifdef PCI_IOBASE
>  	struct resource *res = entry->res;
> @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct
> resource_entry *entry)
>  	resource_size_t length = resource_size(res);
>  	unsigned long port;
> 
> -	if (pci_register_io_range(cpu_addr, length))
> -		goto err;
> -
> -	port = pci_address_to_pio(cpu_addr);
> -	if (port == (unsigned long)-1)
> +	if (pci_register_io_range(node, cpu_addr, length, &port))
>  		goto err;
> 
>  	res->start = port;
> @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct
> acpi_pci_root_info *info)
>  	else {
>  		resource_list_for_each_entry_safe(entry, tmp, list) {
>  			if (entry->res->flags & IORESOURCE_IO)
> -				acpi_pci_root_remap_iospace(entry);
> +				acpi_pci_root_remap_iospace(&device->fwnode,
> +							    entry);
> 
>  			if (entry->res->flags & IORESOURCE_DISABLED)
>  				resource_list_destroy_entry(entry);
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index a50025a3777f..df96955a43f8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -760,8 +760,10 @@ static int __nbd_ioctl(struct block_device *bdev,
> struct nbd_device *nbd,
>  		set_bit(NBD_RUNNING, &nbd->runtime_flags);
>  		blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd-
> >num_connections);
>  		args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
> -		if (!args)
> +		if (!args) {
> +			error = -ENOMEM;
>  			goto out_err;
> +		}
>  		nbd->task_recv = current;
>  		mutex_unlock(&nbd->config_lock);
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..5decaba96eed 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -2,6 +2,7 @@
>  #define pr_fmt(fmt)	"OF: " fmt
> 
>  #include <linux/device.h>
> +#include <linux/fwnode.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/module.h>
> @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range
> *range,
> 
>  	if (res->flags & IORESOURCE_IO) {
>  		unsigned long port;
> -		err = pci_register_io_range(range->cpu_addr, range->size);
> +		err = pci_register_io_range(&np->fwnode, range->cpu_addr,
> range->size, &port);
>  		if (err)
>  			goto invalid_range;
> -		port = pci_address_to_pio(range->cpu_addr);
> -		if (port == (unsigned long)-1) {
> -			err = -EINVAL;
> -			goto invalid_range;
> -		}
>  		res->start = port;
>  	} else {
>  		if ((sizeof(resource_size_t) < 8) &&
> @@ -479,7 +475,7 @@ static int of_empty_ranges_quirk(struct device_node
> *np)
>  	return false;
>  }
> 
> -static int of_translate_one(struct device_node *parent, struct of_bus
> *bus,
> +static u64 of_translate_one(struct device_node *parent, struct of_bus
> *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
>  {
> @@ -507,7 +503,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	ranges = of_get_property(parent, rprop, &rlen);
>  	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>  		pr_debug("no ranges; cannot translate\n");
> -		return 1;
> +		return OF_BAD_ADDR;
>  	}
>  	if (ranges == NULL || rlen == 0) {
>  		offset = of_read_number(addr, na);
> @@ -528,7 +524,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	}
>  	if (offset == OF_BAD_ADDR) {
>  		pr_debug("not found !\n");
> -		return 1;
> +		return offset;
>  	}
>  	memcpy(addr, ranges + na, 4 * pna);
> 
> @@ -537,7 +533,10 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	pr_debug("with offset: %llx\n", (unsigned long long)offset);
> 
>  	/* Translate it into parent bus space */
> -	return pbus->translate(addr, offset, pna);
> +	if (pbus->translate(addr, offset, pna))
> +		return OF_BAD_ADDR;
> +
> +	return offset;
>  }
> 
>  /*
> @@ -549,9 +548,14 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>   * that translation is impossible (that is we are not dealing with a
> value
>   * that can be mapped to a cpu physical address). This is not really
> specified
>   * that way, but this is traditionally the way IBM at least do things
> + *
> + * Whenever the translation fails, the *host pointer will be set to
> the
> + * device that lacks a tranlation, and the return code is relative to
> + * that node.

This seems to be wrong to me. We are abusing of the error conditions.
So effectively if there is a buggy DT for an IO resource we end up
assuming that we are using a special IO device with unmapped addresses.

The patch at the bottom apply on top of this one and I think is a more
reasonable approach


>   */
>  static u64 __of_translate_address(struct device_node *dev,
> -				  const __be32 *in_addr, const char *rprop)
> +				  const __be32 *in_addr, const char *rprop,
> +				  struct device_node **host)
>  {
>  	struct device_node *parent = NULL;
>  	struct of_bus *bus, *pbus;
> @@ -564,6 +568,7 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  	/* Increase refcount at current level */
>  	of_node_get(dev);
> 
> +	*host = NULL;
>  	/* Get parent & match bus type */
>  	parent = of_get_parent(dev);
>  	if (parent == NULL)
> @@ -600,8 +605,9 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  		pbus = of_match_bus(parent);
>  		pbus->count_cells(dev, &pna, &pns);
>  		if (!OF_CHECK_COUNTS(pna, pns)) {
> -			pr_err("Bad cell count for %s\n",
> -			       of_node_full_name(dev));
> +			pr_debug("Bad cell count for %s\n",
> +				 of_node_full_name(dev));
> +			*host = of_node_get(parent);
>  			break;
>  		}
> 
> @@ -609,7 +615,9 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  		    pbus->name, pna, pns, of_node_full_name(parent));
> 
>  		/* Apply bus translation */
> -		if (of_translate_one(dev, bus, pbus, addr, na, ns, pna,
> rprop))
> +		result = of_translate_one(dev, bus, pbus, addr, na, ns,
> +					  pna, rprop);
> +		if (result == OF_BAD_ADDR)

It seems to me that here you missed "*host = of_node_get(parent);"..?

>  			break;
> 
>  		/* Complete the move up one level */
> @@ -628,13 +636,32 @@ static u64 __of_translate_address(struct
> device_node *dev,
> 
>  u64 of_translate_address(struct device_node *dev, const __be32
> *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret =  __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_address);
> 
>  u64 of_translate_dma_address(struct device_node *dev, const __be32
> *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "dma-ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
> +
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_dma_address);
> 
> @@ -676,29 +703,48 @@ const __be32 *of_get_address(struct device_node
> *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
> 
> +extern unsigned long extio_translate(struct fwnode_handle *node,
> unsigned long offset);
> +
> +u64 of_translate_ioport(struct device_node *dev, const __be32
> *in_addr)
> +{
> +	u64 taddr;
> +	unsigned long port;
> +	struct device_node *host;
> +
> +	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		/* host specific port access */
> +		port = extio_translate(&host->fwnode, taddr);
> +		of_node_put(host);
> +	} else {
> +		/* memory mapped I/O range */
> +		port = pci_address_to_pio(taddr);
> +		if (port == (unsigned long)-1)
> +			return OF_BAD_ADDR;
> +	}
> +
> +	return port;
> +}
> +
>  static int __of_address_to_resource(struct device_node *dev,
>  		const __be32 *addrp, u64 size, unsigned int flags,
>  		const char *name, struct resource *r)
>  {
>  	u64 taddr;
> 
> -	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> +	if (flags & IORESOURCE_MEM)
> +		taddr = of_translate_address(dev, addrp);
> +	else if (flags & IORESOURCE_IO)
> +		taddr = of_translate_ioport(dev, addrp);
> +	else
>  		return -EINVAL;
> -	taddr = of_translate_address(dev, addrp);
> +
>  	if (taddr == OF_BAD_ADDR)
>  		return -EINVAL;
>  	memset(r, 0, sizeof(struct resource));
> -	if (flags & IORESOURCE_IO) {
> -		unsigned long port;
> -		port = pci_address_to_pio(taddr);
> -		if (port == (unsigned long)-1)
> -			return -EINVAL;
> -		r->start = port;
> -		r->end = port + size - 1;
> -	} else {
> -		r->start = taddr;
> -		r->end = taddr + size - 1;
> -	}
> +
> +	r->start = taddr;
> +	r->end = taddr + size - 1;
>  	r->flags = flags;
>  	r->name = name ? name : dev->full_name;
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eda6a7cf0e54..320ab9fbf6af 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3249,6 +3249,7 @@ EXPORT_SYMBOL(pci_request_regions_exclusive);
>  #ifdef PCI_IOBASE
>  struct io_range {
>  	struct list_head list;
> +	struct fwnode_handle *node;
>  	phys_addr_t start;
>  	resource_size_t size;
>  };
> @@ -3257,11 +3258,14 @@ static LIST_HEAD(io_range_list);
>  static DEFINE_SPINLOCK(io_range_lock);
>  #endif
> 
> +#define IO_RANGE_IOEXT (resource_size_t)(-1ull)
> +
>  /*
>   * Record the PCI IO range (expressed as CPU physical address + size).
>   * Return a negative value if an error has occured, zero otherwise
>   */
> -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t
> size)
> +int __weak pci_register_io_range(struct fwnode_handle *node,
> phys_addr_t addr,
> +				 resource_size_t size, unsigned long *port)
>  {
>  	int err = 0;
> 
> @@ -3272,7 +3276,12 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
>  	/* check if the range hasn't been previously recorded */
>  	spin_lock(&io_range_lock);
>  	list_for_each_entry(range, &io_range_list, list) {
> -		if (addr >= range->start && addr + size <= range->start +
> size) {
> +		if (node == range->node)
> +			goto end_register;
> +

It seems to me that the condition above is sufficient; i.e.
we can remove the one here below...?

> +		if (addr != IO_RANGE_IOEXT &&
> +		    addr >= range->start &&
> +		    addr + size <= range->start + size) {
>  			/* range already registered, bail out */
>  			goto end_register;
>  		}
> @@ -3298,6 +3307,7 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
>  		goto end_register;
>  	}
> 
> +	range->node = node;
>  	range->start = addr;
>  	range->size = size;
> 
> @@ -3305,11 +3315,26 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
> 
>  end_register:
>  	spin_unlock(&io_range_lock);
> +
> +	*port = allocated_size;
> +#else
> +	/*
> +	 * powerpc and microblaze have their own registration,
> +	 * just look up the value here
> +	 */
> +	*port = pci_address_to_pio(addr);
>  #endif
> 
>  	return err;
>  }
> 
> +#ifdef CONFIG_IOEXT
> +int ioext_register_io_range
> +{
> +	return pci_register_io_range(node, IO_RANGE_IOEXT, size, port);
> +}
> +#endif
> +
>  phys_addr_t pci_pio_to_address(unsigned long pio)
>  {
>  	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6bd94a803e8f..b7a8fa3da3ca 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1192,7 +1192,8 @@ int __must_check pci_bus_alloc_resource(struct
> pci_bus *bus,
>  			void *alignf_data);
> 
> 
> -int pci_register_io_range(phys_addr_t addr, resource_size_t size);
> +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t
> addr,
> +			  resource_size_t size, unsigned long *port);
>  unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t
> phys_addr);

I think the patch below is a more reasonable approach to identify
a host that does not support address translation and it should 
guarantee safety against broken DTs...

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5decaba..9bfc526 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -540,6 +540,49 @@ static u64 of_translate_one(struct device_node *parent, struct of_bus *bus,
 }
 

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>

 /*
+ * of_isa_indirect_io - get the IO address from some isa reg property value.
+ *	For some isa/lpc devices, no ranges property in ancestor node.
+ *	The device addresses are described directly in their regs property.
+ *	This fixup function will be called to get the IO address of isa/lpc
+ *	devices when the normal of_translation failed.
+ *
+ * @parent:	points to the parent dts node;
+ * @bus:		points to the of_bus which can be used to parse address;
+ * @addr:	the address from reg property;
+ * @na:		the address cell counter of @addr;
+ * @presult:	store the address paresed from @addr;
+ *
+ * return 1 when successfully get the I/O address;
+ * 0 will return for some failures.
+ */
+static int of_get_isa_indirect_io(struct device_node *parent,
+				struct of_bus *bus, __be32 *addr,
+				int na, u64 *presult)
+{
+	unsigned int flags;
+	unsigned int rlen;
+
+	/* whether support indirectIO */
+	if (!indirect_io_enabled())
+		return 0;
+
+	if (!of_bus_isa_match(parent))
+		return 0;
+
+	flags = bus->get_flags(addr);
+	if (!(flags & IORESOURCE_IO))
+		return 0;
+
+	/* there is ranges property, apply the normal translation directly. */
+	if (of_get_property(parent, "ranges", &rlen))
+		return 0;
+
+	*presult = of_read_number(addr + 1, na - 1);
+	/* this fixup is only valid for specific I/O range. */
+	return addr_is_indirect_io(*presult);
+}
+
+/*
  * Translate an address from the device-tree into a CPU physical address,
  * this walks up the tree and applies the various bus mappings on the
  * way.
@@ -600,14 +643,23 @@ static u64 __of_translate_address(struct device_node *dev,
 			result = of_read_number(addr, na);
 			break;
 		}
+		/*
+		 * For indirectIO device which has no ranges property, get
+		 * the address from reg directly.
+		 */
+		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
+			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
+				of_node_full_name(dev), result);
+			*host = of_node_get(parent);
+			break;
+		}
 
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
 		pbus->count_cells(dev, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
-			pr_debug("Bad cell count for %s\n",
+			pr_err("Bad cell count for %s\n",
 				 of_node_full_name(dev));
-			*host = of_node_get(parent);
 			break;
 		}
 
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 3786473..14848d8 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -24,6 +24,23 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
+#ifndef indirect_io_enabled
+#define indirect_io_enabled indirect_io_enabled
+static inline bool indirect_io_enabled(void)
+{
+	return false;
+}
+#endif
+
+#ifndef addr_is_indirect_io
+#define addr_is_indirect_io addr_is_indirect_io
+static inline int addr_is_indirect_io(u64 taddr)
+{
+	return 0;
+}
+#endif
+
+
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web