Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann <arnd@arndb.de>]

On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > I will try to improve the error handling in the next patchset version.
> > > However I am still confused about the earlier discussion on
> > > pci_register_io_range(). Your suggestion initially was to return an
> > > error in the default weak implementation, but in your last email you
> > > are talking about returning 'port'.
> > 
> > You can do either one: 'port' should be positive or zero, while the
> > error would always be negative. We do the same thing in many interfaces
> > in the kernel.
> > 
> > > My idea when I've introduced the
> > > helper function was that it would return an error if it fails to
> > > register the IO range and zero otherwise. I agree that we can treat
> > > the default 'do nothing with the IO range' case as an error, with
> > > the caveat that will force architectures that use this code to
> > > provide their own implementation of pci_register_io_range() in order
> > > to avoid failure, even for the cases where the architecture has a 1:1
> > > mapping between IO and CPU addresses.
> > 
> > Which architectures are you thinking of? The only one I know that
> > does this is ia64, and we won't ever have to support this helper
> > on that architecture.
> 
> I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> While not an absolute indicator, with the default pci_address_to_pio()
> that means that they can use the CPU MMIO address as IO address directly.

Not really, that would only work if they also have instructions to do
raw accesses to physical memory addresses rather than virtual memory
pointers that most architectures do.

> $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff

These use a special trick where an __iomem pointer is the same as
the port number. This works most of the time, but breaks anything
that assumes that port numbers are low, such as /dev/port or
broken devices. Moreover, it means your code won't work because
it depends on passing the virtual start address of the PIO mapping
window as io_offset.

> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff

They have no MMU, and the code relies on the port number to match
both the virtual and the physical address. You could be right about
these, but I would guess that the code also needs some other
changes if we want to make it work on nommu kernels. It also depends
on whether the I/O bus address is the same as the CPU address, or
if it starts at bus address 0.

> arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL

Here, the definition is special, the token is just used to encode
a space number and an offset within the I/O space.

> arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF

no PCI here.

> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff

This looks like a mistake, it should be smaller

> arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)

I suspect it doesn't actually work. microblaze copied large parts
of this from PowerPC, but the parts that differ apparently get
it wrong for the I/O space. 

> arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

Same category as frv. We should ask David Howells whether he
thinks I/O space actually works on these.

> arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

I think this should just be 0xffff.

> arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL

Sparc actually accesses the physical addresses, so in theory
it could always work. In the 64-bit case it would however have
to check that the port number is smaller than 0xffffffff, otherwise
you couldn't set the BAR. This means you still need a custom
function.

> arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

tile seems to support only ioport_map() but not inb/outb, if I'm
reading this right.

> > I did not ask to treat 'do nothing with the IO range' as an error,
> > what I meant is that we should treat 'architecture cannot translate
> > from I/O space to memory space but DT lists a translation anyway'
> > as an error. On x86, you should never see an entry for the I/O space
> > in "ranges", so we will not call this function unless there is a
> > bug in DT.
> 
> Ok, it's just that there is no "architecture cannot translate from I/O
> space to memory space" indicator anywhere and I don't want to make x86
> a special case.

Right.

> So, my proposal is this: default weak implementation of pci_register_io_range()
> returns an error (meaning I have no idea how to translate IO addresses
> to memory space) and anyone that wants this function to return success will
> have to provide their implementation.

Another idea: make this conditional on the definition of PCI_IOBASE: If this
is defined, we can use the arm64 version that uses this number. Otherwise
we fall back to returning an error, which means that either on the
architecture we shouldn't be calling that function, or we need a custom
implementation.

	Arnd