Re: [PATCH v4] PCI: ARM: add support for generic PCI host controller

[Liviu Dudau <liviu@dudau.co.uk>]

On Wed, Feb 26, 2014 at 05:46:59PM +0000, Will Deacon wrote:
> Hi Arnd,
> 
> On Tue, Feb 25, 2014 at 10:30:33PM +0000, Arnd Bergmann wrote:
> > On Tuesday 25 February 2014, Will Deacon wrote:
> > > I need the alignment code so I can handle being passed a CPU physical
> > > address which isn't 64K aligned. In this case, pci_ioremap_io needs a 64K
> > > aligned address so that the __io macro does the right thing.
> > 
> > I still don't see why pci_ioremap_io needs 64K alignment. For all I can tell,
> > it just needs a page-aligned address.
> 
> I misread __io as an '|' rather than a '+'. You're right, we just need page
> alignment for the mapping.
> 
> > > For example, I can tell kvmtool to place I/O space at CPU physical 0x16000,
> > > bus address 0x6000 (so ports 0x0-0x5fff are unavailable). In that case, I
> > > still need to map those lower ports for the __io macro to offset into the
> > > window correctly.
> >  
> > I don't think so. pci_ioremap_io will still try to map 64K of address space,
> > but you can map 0x16000-0x25fff to PCI_IO_VIRT_BASE. The io_offset in
> > that case will be negative (-0x6000).
> 
> Yes, and that means that all this rounding is unnecessary; I can just fail
> addresses that aren't page-aligned (which is much more reasonable).
> 
> > > 	of_pci_range_to_resource(range, dev->of_node, res);
> > > 	res->start = window + range->pci_addr;
> > > 	res->end = res->start + range->size - 1;
> > > 	*offset = window - range->pci_addr;
> > > 	return 0;
> > > }
> > 
> > The offset is good now, but the start is not: res->start refers to
> > the linux I/O space and should just be 'window' here. Adding up
> > window and pci_addr makes no sense. Think of the case where you
> > have two host bridges, one using bus address 0x0-0xffff and the other
> > one using 0x10000-0x1ffff. You really want the first one to
> > have res->start=0 and the second one res->start=0x10000, and
> > using offset=0. Instead the second one get start=0x20000,
> > which is wrong in both CPU and bus space.
> 
> Aha, so res->start is the offset into our virtual I/O space at which the
> window starts? That's what I've been getting wrong -- I thought res->start
> was a PCI bus address for some reason.

No, res->start should be start of logical I/O space. I will send today the
patch that fixes the of_pci_range_to_resource() call and then you should
no longer have to worry about offsets (other than keeping a logical io_offset
in the bridge to account for multiple host bridges being active).

> 
> > The round_down() also still gets you into trouble here, because you
> > don't adjust the start or the size.
> 
> I'll just get rid of the rounding.
> 
> > > If I do:
> > > 
> > > 	*offset = window;
> > > 
> > > then things work as expected (this is all referring back to the kvmtool example
> > > I mentioned earlier in this mail).
> > 
> > Yes, but only only because you have two bugs that cancel each other out
> > to some degree. It only works for the first bus though, as explained
> > above: if offset is >0, the Linux I/O port number that gets assigned
> > in the resource is no longer the one that maps to the one in your
> > virtual address space pointing to the right physical address.
> 
> Ok, so taking all this on board I end up with the diff below (against v4).
> It seems to work well with my kvmtool setup:
> 
>   pci_bus 0000:00: root bus resource [io  0x0000-0x9fff] (bus address [0x6000-0xffff])
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 71b0960772ec..b761f3e64a6a 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -133,30 +133,25 @@ static int gen_pci_calc_io_offset(struct device *dev,
>  {
>         static atomic_t wins = ATOMIC_INIT(0);
>         int err, idx, max_win;
> -       unsigned int io_offset;
> +       unsigned int window;
>  
> -       /* Ensure the CPU physical address is aligned to a 64K boundary */
> -       range->size += range->cpu_addr & (SZ_64K - 1);
> -       range->cpu_addr = round_down(range->cpu_addr, SZ_64K);
> -       range->pci_addr = round_down(range->pci_addr, SZ_64K);
> -
> -       if (range->size > SZ_64K)
> -               return -E2BIG;
> +       if (!PAGE_ALIGNED(range->cpu_addr))
> +               return -EINVAL;
>  
> -       max_win = IO_SPACE_LIMIT + 1 / SZ_64K;
> +       max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
>         idx = atomic_inc_return(&wins);
>         if (idx >= max_win)
>                 return -ENOSPC;
>  
> -       io_offset = (idx - 1) * SZ_64K;
> -       err = pci_ioremap_io(io_offset, range->cpu_addr);
> +       window = (idx - 1) * SZ_64K;
> +       err = pci_ioremap_io(window, range->cpu_addr);
>         if (err)
>                 return err;
>  
>         of_pci_range_to_resource(range, dev->of_node, res);
> -       res->start = io_offset + range->pci_addr;
> +       res->start = window;
>         res->end = res->start + range->size - 1;
> -       *offset = io_offset;
> +       *offset = window - range->pci_addr;
>         return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...