On 09/16/2015 10:29 AM, Will Deacon wrote:
Hi Lorenzo,
On Wed, Sep 16, 2015 at 12:28:52PM +0100, Lorenzo Pieralisi wrote:
On Wed, Sep 16, 2015 at 11:41:53AM +0100, Will Deacon wrote:
Here is the current code:
bus_range = pci->cfg.bus_range;
for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
u32 idx = busn - bus_range->start;
The index is offset by the bus range start...
u32 sz = 1 << pci->cfg.ops.bus_shift;
pci->cfg.win[idx] = devm_ioremap(dev,
pci->cfg.res.start + busn * sz,
sz);
But, the offset into the "reg" property is the raw bus number.
if (!pci->cfg.win[idx])
return -ENOMEM;
}
I hope that makes it more clear.
Got it. So we should be using idx instead of busn in the devm_ioremap
call above.
I think that's not what's specified in the PCI firmware specification,
at least for the MMCFG regions. For MMCFG regions (quoting the specs)
the "base address of the memory mapped configuration space always
corresponds to bus number 0 (regardless of the start bus number decoded
by the host bridge)..."
For the x86 implementation have a look at:
arch/x86/pci/mmconfig_64.c mcfg_ioremap()
static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
{
void __iomem *addr;
u64 start, size;
int num_buses;
start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
num_buses = cfg->end_bus - cfg->start_bus + 1;
size = PCI_MMCFG_BUS_OFFSET(num_buses);
addr = ioremap_nocache(start, size);
if (addr)
addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
return addr;
}
The MCFG config accessors add back the PCI_MMCFG_BUS_OFFSET(cfg->start_bus)
to the virtual address so that the proper virtual address is used when
issuing the config cycles, that's my understanding.
Ok. I think that whether the config space mapping or the config accessors
do the fixup should remain an implementation detail, but the resource
identifying config space should be dealt with consistently.
So that means the reg property should describe everything from bus 0,
but then we only map the region corresponding to the bus-range.
So IMO we have to define what "reg" represents for ECAM in DT, we can't
leave this open to interpretation (and I think makng MCFG and DT config
work the same way would be ideal).
I will update the
Documentation/devicetree/bindings/pci/host-generic-pci.txt to reflect
this interpretation.
If we define reg to cover the whole config space from bus 0 onwards,
then I think the driver should work as-is today. It's slightly odd, in
that there may be a prefix of config space that maps to god-knows-where,
but it's consistent with ACPI and doesn't require us to change the driver.
David?
I agree with this approach for two reasons:
1) My interpretation of relevant specifications agrees with Lorenzo's
2) It is less work for me, as this is how my firmware is currently
configured.
This patch 4/6 is still necessary, as the bus_max calculation is broken
for non-zero start_bus.
I anticipate sending a new version of the patch set later today (PDT).
I will add any Acked-by/Reviewed-by that I receive to the new set.
David Daney
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
