Re: of/pci: Fix the conversion of IO ranges into IO resources

[Liviu Dudau]

On Thu, Oct 16, 2014 at 03:55:48AM +0100, Michael Ellerman wrote:
> On Wed, 2014-10-15 at 10:02 +0100, Liviu Dudau wrote:
> > On Wed, Oct 15, 2014 at 08:35:07AM +0100, Benjamin Herrenschmidt wrote:
> > > On Thu, 2014-10-09 at 20:02 +0000, Linux Kernel Mailing List wrote:
> > > > Gitweb:     http://git.kernel.org/linus/;a=commit;h=0b0b0893d49b34201a6c4416b1a707b580b91e3d
> > > > Commit:     0b0b0893d49b34201a6c4416b1a707b580b91e3d
> > > > Author:     Liviu Dudau <Liviu.Dudau@xxxxxxx>
> > > > Committer:  Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > >
> > > >     of/pci: Fix the conversion of IO ranges into IO resources
> > > >
> > > >     The ranges property for a host bridge controller in DT describes the
> > > >     mapping between the PCI bus address and the CPU physical address.  The
> > > >     resources framework however expects that the IO resources start at a pseudo
> > > >     "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.  The
> > > >     conversion from PCI ranges to resources failed to take that into account,
> > > >     returning a CPU physical address instead of a port number.
> > > >
> > > >     Also fix all the drivers that depend on the old behaviour by fetching the
> > > >     CPU physical address based on the port number where it is being needed.
> > > 
> > > Michael just signaled me that this completely breaks IO space on powerpc ...
> > 
> > Hi Benjamin,
> > 
> > I'm sorry to hear that I've broke powerpc before I've had a chance to actually
> > change the code there. I would like to get the details of what functionality
> > get broken.
> 

Hi Michael,

> You changed code that arch/powerpc depends on, without updating it, or even
> CC'ing us on the patches. I'm not sure what you mean by "before I've had a
> chance to actually change the code there" - it's too late.

Sorry, I meant without directly changing the code in arch/powerpc. And I do appologise
for not CC-ing you on the patches. I believe (hope) Benjamin has been on CC for several
revisions, but I know he is very busy and he has told me so.

> 
> > The pci_register_io_range() function (the "allocator" for IO space) is a
> > weak function. It takes the CPU physical address of the range and its size
> > and makes sure that it can fit that area in the arch's space for PCI IO.
> > The main purpose of that function is to be a helper to pci_address_to_pio()
> > in order to help return the correct answer in that function. pci_address_to_pio()
> > is also weak and can be overwritten.
> 
> Yes, we already provide our own version of pci_address_to_pio().
> 
> The problem is it's too early to call it when we come in from
> find_and_init_phbs() -> pci_process_bridge_OF_ranges(), so it returns junk.
> 
> I think you were expecting us to hit the #ifndef PCI_IOBASE case, which looks
> like it might have worked.

That's correct. Unfortunately I lack access to a PowerPC platform with PCI(e) that I can
test my patches on, so most of my testing has been compile tests. Also, due to the
similarities in code between powerpc and microblaze in this area I hoped that focusing
on the simpler microblaze would be faster, until I run into the brick wall of not being
able to source an FPGA image for the test platform I have that contains all the relevant
bits compiled in.

> 
> For now we're just going to stop using of_pci_range_to_resource().

So you are going to continue converting IO ranges as IORESOURCE_MEM resources but with a
different flag. Is that something that powerpc depends on or is it an artifact of too
much code living around the kernel that has built in assumption of that being the fact?
I'm trying to understand the world around powerpc so as to attempt to make a useful
contribution in the future.

Best regards,
Liviu

> 
> cheers
> 
> 
> 
> 

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/