Re: [PATCH 2/2 v2] PCI: v3-semi: Add a V3 Semiconductor PCI host driver

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

 




[+cc Rob, Frank, devicetree list to make sure I got the config space
part right]

On Fri, Sep 01, 2017 at 05:49:28PM +0200, Linus Walleij wrote:
> On Tue, Aug 22, 2017 at 9:01 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> 
> > On Wed, Aug 09, 2017 at 04:14:55PM +0200, Linus Walleij wrote:
> 
> >> OF: PCI: host bridge /pciv3@62000000 ranges:
> >> OF: PCI:   No bus range found for /pciv3@62000000, using [bus 00-ff]
> >> OF: PCI:   err 0x61000000..0x61ffffff -> 0x61000000
> 
> Yeah it is called "err"
> 
> >> OF: PCI:    IO 0x60000000..0x600fffff -> 0x00000000
> >> OF: PCI:   MEM 0x40000000..0x4fffffff -> 0x00000000
> >> OF: PCI:   MEM 0x50000000..0x5fffffff -> 0x10000000
> >> pci-v3-semi 62000000.pciv3: BUS 0
> >> pci-v3-semi 62000000.pciv3: CONFIG SPACE window
> >>   start 61000000, size 01000000 (offset = 00000000, bus addr = 61000000)
> 
> But as it says here it is config space.
> 
> >> pci-v3-semi 62000000.pciv3: I/O window
> >>   start 00000000, bus addr 00000000, size 00100000
> >> pci-v3-semi 62000000.pciv3: NON-PREFETCHABLE MEM window
> >>   start 40000000, bus addr 00000000, size 10000000
> >> pci-v3-semi 62000000.pciv3: PREFETCHABLE MEM window
> >>   start 50000000, bus addr 10000000, size 10000000
> >> pci-v3-semi 62000000.pciv3: FIFO_CFG: 0000  FIFO_PRIO: 0000
> >> pci-v3-semi 62000000.pciv3: initialized PCI V3 Integrator/AP integration
> >> pci-v3-semi 62000000.pciv3: PCI host bridge to bus 0000:00
> >> pci_bus 0000:00: root bus resource [bus 00-ff]
> >> pci_bus 0000:00: root bus resource [??? 0x61000000-0x61ffffff flags 0x0]
> >
> > I think this is wrong.  Looks like this might be memory-mapped config
> > space (e.g., MMCONFIG or ECAM-like space), which is not a window and
> > shouldn't be added to the root bus resources.
> 
> So it comes from ranges, and I take it that actually the device
> tree is wrong because it looks like this:
> 
> reg = <0x62000000 0x10000>;
> ranges = <0x00000000 0 0x61000000 /* config space */
>         0x61000000 0 0x01000000 /* 16 MiB @ 61000000 */
>         0x01000000 0 0x0 /* I/O space */
>         0x60000000 0 0x00100000 /* 16 MiB @ 60000000 */
>         0x02000000 0 0x00000000 /* non-prefectable memory */
>         0x40000000 0 0x10000000 /* 256 MiB @ 40000000 */
>         0x42000000 0 0x10000000 /* prefetchable memory */
>         0x50000000 0 0x10000000>; /* 256 MiB @ 50000000 */
> 
> So what you're saying is that the config space should not be in the
> ranges but in the reg = <> property (second register range) so that
> the reg and ranges looks like this:
> 
> reg = <0x62000000 0x10000>, <0x61000000 0x01000000>;

Yes.  Documentation/devicetree/bindings/pci/designware-pcie.txt has a
little text about the typical way to do this.  Maybe this should be copied
or moved to pci.txt.

> ranges = <0x00000000 0 0x61000000 /* config space */
>         0x60000000 0 0x00100000 /* 16 MiB @ 60000000 */
>         0x02000000 0 0x00000000 /* non-prefectable memory */
>         0x40000000 0 0x10000000 /* 256 MiB @ 40000000 */
>         0x42000000 0 0x10000000 /* prefetchable memory */
>         0x50000000 0 0x10000000>; /* 256 MiB @ 50000000 */
> 
> So there is only a range for mapping the physical address
> 0x61000000 to PCI address 0x00000000.

I don't understand this; maybe a typo?  IIUC, the 16 MiB @ 61000000
region is essentially bridge register space: it is decoded by the
bridge, which then generates PCI config transactions with bus/dev/fn
determined by the register address.

> I don't mind altering the device trees, we don't have it deployed
> in flash or anything. I think we simply got this thing wrong when
> I first converted it over.

incidental: s/prefectable/prefetchable/ above

> > The driver still needs it internally, of course, and maybe could print
> > something similar to what we do for ECAM:
> >
> >   dev_info(dev, "ECAM at %pR for %pR\n", &cfg->res, &cfg->busr)
> >
> > (I don't know if this is actually identical to ECAM, so maybe there's
> > a better internally-used name for the region, but we could print the
> > memory and bus resources similarly.)
> 
> Sure, we just need to figure out if it should be in reg
> or ranges first.

> >> +     list_splice_init(&res, &host->windows);
> >> +     ret = pci_scan_root_bus_bridge(host);
> >> +     if (ret) {
> >> +             dev_err(dev, "failed to register host: %d\n", ret);
> >> +             return ret;
> >> +     }
> >> +     v3->bus = host->bus;
> >> +
> >> +     pci_bus_assign_resources(v3->bus);
> >> +     pci_bus_add_devices(v3->bus);
> >> +     pci_free_resource_list(&res);
> >
> > I think pci_free_resource_list() should be called only if an error
> > occurs, shouldn't it?
> 
> I'm not sure. I was under the impression that list_splice_init() makes
> a copy of it into host->windows and then we should discard it after
> use.

I don't think list_splice_init() does any copying.  In the non-error
case, we call pci_free_resource_list() from pci_free_host_bridge()
if/when the host bridge is removed.  The other callers of
pci_free_resource_list() are all in error handling paths.

Bjorn
--
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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux