Re: [PATCH 1/6] PCI: tegra: refactor config space mapping code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[+cc Rob, devicetree list]

On Mon, Oct 23, 2017 at 11:09:50AM +0530, Vidya Sagar wrote:
> On Saturday 21 October 2017 12:38 AM, Bjorn Helgaas wrote:
> >On Fri, Oct 13, 2017 at 12:20:06AM +0530, Vidya Sagar wrote:
> >>use only 4K space from available 1GB PCIe aperture to access
> >>end points configuration space by dynamically moving AFI_AXI_BAR
> >>base address and always making sure that the desired location
> >>to be accessed for generating required config space access falls
> >>in the 4K space reserved for this purpose. This would give more
> >>space for mapping end point device's BARs on some of Tegra platforms
> >
> >Do you now have a revlock issue between this driver change and the
> >corresponding DT changes?  I.e., what happens when you run this driver
> >change with an older DT that specifies a 256MB config aperture?  What
> >happens if you run the older driver with a new DT that only specifies
> >a 4KB config aperture?
>
> Yes. Both driver and DT changes must go together. New driver and old DT (or)
> Old driver and new DT would break config space access.

This revlock sounds like a pretty major issue.  I don't know if the DT
conventions even allow for this.  If they do, it definitely needs to
be mentioned in the changelog.

Looking for acks from Thierry and Rob.

> >>-		addr += tegra_pcie_conf_offset(devfn, where);
> >>+		addr = pcie->cfg_va_base;
> >>+		val = ((((u32)where & 0xf00) >> 8) << 24) |
> >>+		      (bus->number << 16) | (PCI_SLOT(devfn) << 11) |
> >>+		      (PCI_FUNC(devfn) << 8) | (where & 0xff);
> >I think it would be useful to keep tegra_pcie_conf_offset() (extending
> >it to take the bus number) near the comment describing the mapping.
> I'll take care of it in next patch.
> >
> >>+		addr = (val & (SZ_4K - 1)) + addr;
> >>+		val = val & ~(SZ_4K - 1);
> >   val &= ~(SZ_4K - 1);
> >
> >I *think* what you're doing is computing a config space offset and
> >mapping the 4KB window that contains it.
> >
> >"addr" is not used at all in the AFI_AXI_BAR0 programming, so this
> >would be clearer as:
> >
> >     val = ...
> >     afi_writel(...)
> >     afi_writel(...)
> >
> >     addr = pcie->cfg_va_base + (val & (SZ_4K - 1));
> but, addr is using the intermediate 'val' i.e. before 'val &=
> ~(SZ_4K - 1)' operation is performed on it.

Ah, right.  I think adding another temporary variable here would make
it clearer.

> >   }
> >
> >   return addr;
> >
> >>+		afi_writel(pcie, pcie->cs->start - val, AFI_AXI_BAR0_START);
> >>+		afi_writel(pcie, (val + SZ_4K) >> 12, AFI_AXI_BAR0_SZ);
> >>  	}
> >>  	return addr;
> >>-- 
> >>2.7.4
> >>
> 



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux