On Fri, Mar 18, 2016 at 02:12:51PM -0400, Murali Karicheri wrote:
> On 03/18/2016 11:28 AM, Bjorn Helgaas wrote:
> > On Fri, Mar 18, 2016 at 11:09:27AM -0400, Murali Karicheri wrote:
> >> On 03/18/2016 07:28 AM, Lorenzo Pieralisi wrote:
> >>> On Thu, Mar 17, 2016 at 05:28:31PM -0400, Murali Karicheri wrote:
> >>>
> >>> [...]
> >>>
> >>>>> The only ways I see that PCI_PROBE_ONLY can be set on ARM are if you have
> >>>>> "linux,pci-probe-only" in your DT or you boot with "pci=firmware".
> >>>>>
> >>>>> I expect you're in this path:
> >>>>>
> >>>>> ahci_init_one
> >>>>> pcim_enable_device
> >>>>> pci_enable_device
> >>>>> pci_enable_device_flags(dev, IORESOURCE_MEM | IORESOURCE_IO)
> >>>>> # build "bars" mask
> >>>>> do_pci_enable_device(dev, bars)
> >>>>> pcibios_enable_device
> >>>>> if (pci_has_flag(PCI_PROBE_ONLY))
> >>>>> return 0;
> >>>>> pci_enable_resources
> >>>>>
> >>>>> Can you add a little debug code like this to verify that we're in this
> >>>>> path?
> >>>>
> >>>> Yes we are in the path.
> >>>>
> >>>>
> >>>> [ 1.557561] ahci_init_one
> >>>> [ 1.560214] ahci 0000:01:00.0: version 3.0
> >>>> [ 1.564302] pcim_enable_device
> >>>> [ 1.567349] pci_enable_device
> >>>> [ 1.570340] pci_enable_device_flags
> >>>> [ 1.573824] do_pci_enable_device
> >>>> [ 1.577042] pcibios_enable_device
> >>>> [ 1.580380] pci_enable_resources
> >>>
> >>> So resources are actually enabled (ie PCI_PROBE_ONLY is not set)
> >>> and that makes sense otherwise you would not be able to use the
> >>> MEM resources anyway (ie they would not be enabled).
> >>>
> >>> I suspect the PCI dev IO resources were reset in reset_resource() in
> >>> assign_requested_resource_sorted(), hence the bar mask that is built
> >>> in pci_enable_device_flags() does not contain the IO resources,
> >>> it would be helpful if you can print the bar mask passed to
> >>> pcibios_enable_device() (ie the mask parameter).
> >>
> >> Here it is
> >>
> >> [ 1.556507] ahci_init_one
> >> [ 1.559124] ahci 0000:01:00.0: version 3.0
> >> [ 1.563246] pcim_enable_device
> >> [ 1.566294] pci_enable_device
> >> [ 1.569252] pci_enable_device_flags
> >> [ 1.572766] do_pci_enable_device
> >> [ 1.575985] pcibios_enable_device 60
> >> [ 1.579551] pci_enable_resources
> >>
> >> I know that some of our customers use PCIe SATA from u-boot and would
> >> like to honor the assignment in Linux space.. I believe they use PCI_PROBE_ONLY
> >> by setting the bootarg. So Keystone PCI should work in both cases.
> >
> > We're only getting little pieces of the story here. Can you apply the
> > following patch and collect the entire dmesg log? I want to see:
> >
> > - the root bus resources (which presumably include no I/O space)
> > - all the SATA resources during enumeration (which should include an
> > I/O BAR)
> > - the reset_resource() call that clears the I/O BAR flags
> > - all the SATA resources in pci_enable_resources() (the I/O BAR
> > should be cleared out)
> > - the PCI_COMMAND register values before and after
> > pci_enable_resources()
> >
> > Bjorn
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 55641a3..83e8d42 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -211,6 +211,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
> >
> > static inline void reset_resource(struct resource *res)
> > {
> > + printk("%s: %pR\n", __func__, res);
> > res->start = 0;
> > res->end = 0;
> > res->flags = 0;
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index 66c4d8f..c2c45f9 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> > @@ -364,11 +364,14 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
> > pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > old_cmd = cmd;
> >
> > + dev_info(&dev->dev, "%s: mask %#x old_cmd %#x\n", __func__, mask, old_cmd);
> > +
> > for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > if (!(mask & (1 << i)))
> > continue;
> >
> > r = &dev->resource[i];
> > + dev_info(&dev->dev, " BAR %d %pR parent %p\n", i, r, r->parent);
> >
> > if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> > continue;
> > @@ -394,6 +397,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
> > cmd |= PCI_COMMAND_MEMORY;
> > }
> >
> > + dev_info(&dev->dev, "%s: cmd %#x\n", __func__, cmd);
> > if (cmd != old_cmd) {
> > dev_info(&dev->dev, "enabling device (%04x -> %04x)\n",
> > old_cmd, cmd);
> >
> You can see complete bootlog with above at
> http://pastebin.ubuntu.com/15416575/
Here are the interesting parts:
keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
No I/O space, as we expected.
pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
pci 0000:01:00.0: reg 0x10: [io 0x8000-0x8007]
pci 0000:01:00.0: reg 0x14: [io 0x8040-0x8043]
pci 0000:01:00.0: reg 0x18: [io 0x8100-0x8107]
pci 0000:01:00.0: reg 0x1c: [io 0x8140-0x8143]
pci 0000:01:00.0: reg 0x20: [io 0x800000-0x80000f]
pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
Several I/O BARs shown above.
pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
pci 0000:01:00.0: BAR 4: failed to assign [io size 0x0010]
reset_resource: [io size 0x0010]
pci 0000:01:00.0: BAR 0: failed to assign [io size 0x0008]
reset_resource: [io size 0x0008]
pci 0000:01:00.0: BAR 2: failed to assign [io size 0x0008]
reset_resource: [io size 0x0008]
pci 0000:01:00.0: BAR 1: failed to assign [io size 0x0004]
reset_resource: [io size 0x0004]
pci 0000:01:00.0: BAR 3: failed to assign [io size 0x0004]
reset_resource: [io size 0x0004]
reset_resource() shows "size 0x...." instead of the address because we
set the IORESOURCE_UNSET bit when we failed to assign space. That
part is fine, but then reset_resource() goes on to clear res->flags,
which is not fine.
ahci 0000:01:00.0: ahci_init_one:
ahci 0000:01:00.0: version 3.0
ahci 0000:01:00.0: pcim_enable_device:
ahci 0000:01:00.0: pci_enable_device:
ahci 0000:01:00.0: pci_enable_device_flags:
ahci 0000:01:00.0: do_pci_enable_device:
ahci 0000:01:00.0: pcibios_enable_device: 60
ahci 0000:01:00.0: pci_enable_resources: mask 0x60 old_cmd 0x143
ahci 0000:01:00.0: BAR 5 [mem 0x60000000-0x600001ff] parent eb149b10
ahci 0000:01:00.0: BAR 6 [mem 0x60100000-0x6010ffff pref] parent eb149b38
ahci 0000:01:00.0: pci_enable_resources: cmd 0x143
pci_enable_device() requests all resources of type
IORESOURCE_MEM | IORESOURCE_IO. pci_enable_device_flags() builds
"mask" (0x60 here) based on which resources match that type. For the
I/O resources, res->flags has been cleared out by reset_resource(), so
only the MMIO resources (BARs 5 & 6) match, hence we have bits 5 and 6
set in "mask".
So pci_enable_resources() only looks at the MMIO resources, which are
both fine. It thinks no IORESOURCE_IO resources are needed, so it
doesn't turn on PCI_COMMAND_IO. Somebody (maybe firmware) had
previously enabled PCI_COMMAND_IO, and we leave it enabled. This is a
potential problem because those I/O BARs are still enabled and the
device will respond if it receives an I/O access to those regions.
This isn't a problem on your particular system because there's no way
to generate I/O accesses, but it *is* a problem in general.
There are lots of things I think we should fix here. They're all in
the PCI core and in drivers, not in anything Keystone-related:
- reset_resource() shouldn't clear the IORESOURCE_TYPE_BITS. This
probably has implications in the rest of resource assignment.
- pci_enable_resources() probably should clear PCI_COMMAND_IO if any
I/O resources are unset.
- There should be a pcim_enable_device_mem().
- ahci_init_one() and similar drivers that don't need I/O space
should use pcim_enable_device_mem().
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
