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

[Liviu Dudau <Liviu.Dudau@arm.com>]

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.

$ 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
arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff


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

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.

I will send an updated series.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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