On 03/18/2016 03:34 PM, Bjorn Helgaas wrote:
> 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().
>
Ok.
Murali
> Bjorn
>
--
Murali Karicheri
Linux Kernel, Keystone
--
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
