Re: [PATCH v7 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

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

On Fri, Mar 14, 2014 at 07:16:10PM +0000, Arnd Bergmann wrote:
> On Friday 14 March 2014, Liviu Dudau wrote:
> > On Fri, Mar 14, 2014 at 06:46:23PM +0000, Arnd Bergmann wrote:
> > > On Friday 14 March 2014, Liviu Dudau wrote:
> > > 
> > > > +int of_pci_range_to_resource(struct of_pci_range *range,
> > > > +       struct device_node *np, struct resource *res)
> > > > +{
> > > > +       res->flags = range->flags;
> > > > +       res->parent = res->child = res->sibling = NULL;
> > > > +       res->name = np->full_name;
> > > > +
> > > > +       if (res->flags & IORESOURCE_IO) {
> > > > +               unsigned long port = -1;
> > > > +               int err = pci_register_io_range(range->cpu_addr, range->size);
> > > > +               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 {
> > > 
> > > This one concatenates the I/O spaces and assumes that each space starts
> > > at bus address zero, and takes little precaution to avoid filling up IO_SPACE_LIMIT
> > > if the sizes are too big.
> > 
> > Actually, you are attaching too much meaning to this one. pci_register_io_range()
> > only tries to remember the ranges, nothing else. And it *does* check that the
> > total sum of registered ranges does not exceed IO_SPACE_LIMIT. This is used before
> > we have any resource mapped for that range (actually it is used to *create* the
> > resource for the range) so there is no other helping hand.
> > 
> > It doesn't assume that space starts at bus address zero, it ignores the bus address.
> > It only handles CPU addresses for the range, to help with generating logical IO ports.
> > If you have overlapping CPU addresses with different bus addresses it will not work,
> > but then I guess you will have different problems then.
> 
> The problem is that it tries to set up a mapping so that pci_address_to_pio
> returns the actual port number, but the port that you assign to res->start
> above is assumed to match the 'start' variable below. If the value ends
> up different, the BARs that get set by the PCI bus scan are not in the
> same place that got ioremapped into the virtual I/O aperture.

Yes, after writting a reply trying to justify why it would actually work I've realised
where the fault of my logic stands (short version, two host controllers will get different
io_offsets but the ports numbers will start from zero leading to confusion about which
host controller the resource belongs to).

I will try to split your function into two parts, one that calculates the io_offset and
another that does the ioremap_page_range() side and replace my cooked function.

Best regards,
Liviu

> 
> > > >+unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr)
> > > >+{
> > > >+       unsigned long start, len, virt_start;
> > > >+       int err;
> > > >+
> > > >+       if (res->end > IO_SPACE_LIMIT)
> > > >+               return -EINVAL;
> > > >+
> > > >+       /*
> > > >+        * try finding free space for the whole size first,
> > > >+        * fall back to 64K if not available
> > > >+        */
> > > >+       len = resource_size(res);
> > > >+       start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> > > >+                               res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
> > > >+       if (start == IO_SPACE_PAGES && len > SZ_64K) {
> > > >+               len = SZ_64K;
> > > >+               start = 0;
> > > >+               start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> > > >+                                       start, len / PAGE_SIZE, 0);
> > > >+       }
> > > >+
> > > >+       /* no 64K area found */
> > > >+       if (start == IO_SPACE_PAGES)
> > > >+               return -ENOMEM;
> > > >+
> > > >+       /* ioremap physical aperture to virtual aperture */
> > > >+       virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
> > > >+       err = ioremap_page_range(virt_start, virt_start + len,
> > > >+                               phys_addr, __pgprot(PROT_DEVICE_nGnRE));
> > > >+       if (err)
> > > >+               return err;
> > > >+
> > > >+       bitmap_set(pci_iospace, start, len / PAGE_SIZE);
> > > >+
> > > >+       /* return io_offset */
> > > >+       return start * PAGE_SIZE - res->start;
> > > >+}
> 
> 	Arnd
> 

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