PCI IO resource question.

PCI IO resource question.

[Murali Karicheri]

Bjorn,

Keystone PCI h/w doesn't support IO access in the inbound direction
as stated in the hardware spec cut-n-pasted below.

• No support for IO access in inbound direction in RC or EP mode

Way back when the driver was ported, I vaguely recollect I had to
add a dummy IO range to satisfy PCI Driver core. I am assuming the 
first entry in the DTS is for this IO port access. Is it true?
  
arch/arm/boot/dts/k2e.dtsi

                       ranges = <0x81000000 0 0 0x23260000 0x4000 0x4000
                                 0x82000000 0 0x60000000 0x60000000 0 0x10000000>;

I see following boot log when I boot with 4.4.x kernel

>> keystone-pcie 21021000.pcie: error -22: failed to map resource [io 0x0000-0x400000003fff]

I need to fix the above entry to

-                       ranges = <0x81000000 0 0 0x23260000 0x4000 0x4000
+                       ranges = <0x81000000 0 0 0x23260000 0x0 0x4000
                                0x82000000 0 0x60000000 0x60000000 0 0x10000000>;


to remove this error message.

However I am wondering if it is mandatory to define this range at all. I tried
to remove the entry and I get following error then

[    0.448772] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
[    0.448783] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
[    0.448792] pci 0000:00:00.0: BAR 7: no space for [io  size 0x1000]
[    0.448800] pci 0000:00:00.0: BAR 7: failed to assign [io  size 0x1000]
[    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
[    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
[    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
[    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
[    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
[    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
[    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
[    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
[    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
[    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
[    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
[    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
[    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]


The original log is below and even with the error, I am able to have SATA
drive function as expected over this PCIe interface.


[    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
[    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
[    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
[    0.420685] Requested IO range too big, new size set to 64K
[    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
[    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
[    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
[    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
[    0.431879] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
[    0.431906] pci 0000:00:00.0: [104c:b009] type 01 class 0x060400
[    0.432173] PCI: bus0: Fast back to back transfers disabled
[    0.432334] pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
[    0.432382] pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
[    0.432402] pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
[    0.432420] pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
[    0.432439] pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
[    0.432457] pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
[    0.432476] pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
[    0.432495] pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
[    0.432561] pci 0000:01:00.0: PME# supported from D3hot
[    0.448635] PCI: bus1: Fast back to back transfers disabled
[    0.448717] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
[    0.448729] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
[    0.448738] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
[    0.448751] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
[    0.448760] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
[    0.448772] pci 0000:01:00.0: BAR 4: assigned [io  0x1000-0x100f]
[    0.448784] pci 0000:01:00.0: BAR 0: assigned [io  0x1010-0x1017]
[    0.448796] pci 0000:01:00.0: BAR 2: assigned [io  0x1018-0x101f]
[    0.448808] pci 0000:01:00.0: BAR 1: assigned [io  0x1020-0x1023]
[    0.448820] pci 0000:01:00.0: BAR 3: assigned [io  0x1024-0x1027]
[    0.448833] pci 0000:00:00.0: PCI bridge to [bus 01]
[    0.448842] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
[    0.448851] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
[    0.448860] pci 0000:00:00.0:   bridge window [mem 0x60100000-0x601fffff pref]
[    0.449077] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
[    0.449086] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
[    0.449095] pcie_pme 0000:00:00.0:pcie01: service driver pcie_pme loaded
[    0.449240] aer 0000:00:00.0:pcie02: service driver aer loaded


Thanks and regards,

-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Bjorn Helgaas]

On Wed, Mar 16, 2016 at 12:20:08PM -0400, Murali Karicheri wrote:
> Bjorn,
> 
> Keystone PCI h/w doesn't support IO access in the inbound direction
> as stated in the hardware spec cut-n-pasted below.
> 
> • No support for IO access in inbound direction in RC or EP mode
> 
> Way back when the driver was ported, I vaguely recollect I had to
> add a dummy IO range to satisfy PCI Driver core. I am assuming the 
> first entry in the DTS is for this IO port access. Is it true?

The PCI core should not require an I/O range.

> arch/arm/boot/dts/k2e.dtsi
> 
>                        ranges = <0x81000000 0 0 0x23260000 0x4000 0x4000
>                                  0x82000000 0 0x60000000 0x60000000 0 0x10000000>;
> 
> I see following boot log when I boot with 4.4.x kernel
> 
> >> keystone-pcie 21021000.pcie: error -22: failed to map resource [io 0x0000-0x400000003fff]
> 
> I need to fix the above entry to
> 
> -                       ranges = <0x81000000 0 0 0x23260000 0x4000 0x4000
> +                       ranges = <0x81000000 0 0 0x23260000 0x0 0x4000
>                                 0x82000000 0 0x60000000 0x60000000 0 0x10000000>;
> 
> 
> to remove this error message.
> 
> However I am wondering if it is mandatory to define this range at all. I tried
> to remove the entry and I get following error then
> 
> [    0.448772] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
> [    0.448783] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
> [    0.448792] pci 0000:00:00.0: BAR 7: no space for [io  size 0x1000]
> [    0.448800] pci 0000:00:00.0: BAR 7: failed to assign [io  size 0x1000]

Obviously if the host bridge doesn't support I/O port space, we will
be unable to assign space for I/O BARs, so you will see errors like
this.  

We may be able to improve the message and/or make this less noisy.
Guenter Roeck looked at a similar issue a while ago, but it's not
completely trivial:

  http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net

The PCI core should check in pci_enable_device() whether all the
device BARs have been assigned.  If not, it should fail.  But if a
driver doesn't need I/O space, it can use pci_enable_device_mem() to
indicate that it only needs the MEM BARs.  That should succeed even if
the I/O BARs aren't assigned.

Bottom line, if you omit I/O space on your host bridge:

  - You will see annoying "no space for" and "failed to assign" messages
  - Drivers that don't need I/O ports should still work
  - It's far better to have the messages than it was to pretend that
    the host bridge supported I/O space when it really didn't.

> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
> 
> 
> The original log is below and even with the error, I am able to have SATA
> drive function as expected over this PCIe interface.
> 
> 
> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
> [    0.420685] Requested IO range too big, new size set to 64K
> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]

This range is obviously bogus, since it's way too big and not a nice
round size.  I guess this is what you're fixing.

> [    0.431879] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
> [    0.431906] pci 0000:00:00.0: [104c:b009] type 01 class 0x060400
> [    0.432173] PCI: bus0: Fast back to back transfers disabled
> [    0.432334] pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
> [    0.432382] pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
> [    0.432402] pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
> [    0.432420] pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
> [    0.432439] pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
> [    0.432457] pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
> [    0.432476] pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
> [    0.432495] pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
> [    0.432561] pci 0000:01:00.0: PME# supported from D3hot
> [    0.448635] PCI: bus1: Fast back to back transfers disabled
> [    0.448717] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
> [    0.448729] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
> [    0.448738] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> [    0.448751] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> [    0.448760] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> [    0.448772] pci 0000:01:00.0: BAR 4: assigned [io  0x1000-0x100f]
> [    0.448784] pci 0000:01:00.0: BAR 0: assigned [io  0x1010-0x1017]
> [    0.448796] pci 0000:01:00.0: BAR 2: assigned [io  0x1018-0x101f]
> [    0.448808] pci 0000:01:00.0: BAR 1: assigned [io  0x1020-0x1023]
> [    0.448820] pci 0000:01:00.0: BAR 3: assigned [io  0x1024-0x1027]
> [    0.448833] pci 0000:00:00.0: PCI bridge to [bus 01]
> [    0.448842] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> [    0.448851] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
> [    0.448860] pci 0000:00:00.0:   bridge window [mem 0x60100000-0x601fffff pref]
> [    0.449077] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
> [    0.449086] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
> [    0.449095] pcie_pme 0000:00:00.0:pcie01: service driver pcie_pme loaded
> [    0.449240] aer 0000:00:00.0:pcie02: service driver aer loaded
> 
> 
> Thanks and regards,
> 
> -- 
> Murali Karicheri
> Linux Kernel, Keystone
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PCI IO resource question.

[Murali Karicheri]

Bjorn,

Thanks for your quick response! Please see below some clarification
and follow up question.

On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
0x1000]
> 
> Obviously if the host bridge doesn't support I/O port space, we will
> be unable to assign space for I/O BARs, so you will see errors like
> this.  
> 
> We may be able to improve the message and/or make this less noisy.
> Guenter Roeck looked at a similar issue a while ago, but it's not
> completely trivial:
> 
>   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
> 
> The PCI core should check in pci_enable_device() whether all the
> device BARs have been assigned.  If not, it should fail.  But if a
> driver doesn't need I/O space, it can use pci_enable_device_mem() to
> indicate that it only needs the MEM BARs.  That should succeed even if
> the I/O BARs aren't assigned.
> 
> Bottom line, if you omit I/O space on your host bridge:
> 
>   - You will see annoying "no space for" and "failed to assign" messages
>   - Drivers that don't need I/O ports should still work
>   - It's far better to have the messages than it was to pretend that
>     the host bridge supported I/O space when it really didn't.
> 
>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
>>
>>
>> The original log is below and even with the error, I am able to have SATA
>> drive function as expected over this PCIe interface.
>>
>>
>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
>> [    0.420685] Requested IO range too big, new size set to 64K
>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
> 
> This range is obviously bogus, since it's way too big and not a nice
> round size.  I guess this is what you're fixing.

Yes. But from your response, I gather it is better to remove the bogus range.
I removed the range, and did a read/write test to the hard drive connected 
to the Marvel SATA that is hooked to the PCIe interface and it still work
without issues. 
 
Another thing to worry about is the customers who are using custom
fpga pci devices connected to the pcie bus and presently using 
pci_enable_device() in their driver. So I guess if they fix their driver
to use pci_enable_device_mem() instead, it should continue to work
without issues, right? 

Regards,

Murali
> 
>> [    0.431879] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
>> [    0.431906] pci 0000:00:00.0: [104c:b009] type 01 class 0x060400
>> [    0.432173] PCI: bus0: Fast back to back transfers disabled
>> [    0.432334] pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
>> [    0.432382] pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
>> [    0.432402] pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
>> [    0.432420] pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
>> [    0.432439] pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
>> [    0.432457] pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
>> [    0.432476] pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
>> [    0.432495] pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
>> [    0.432561] pci 0000:01:00.0: PME# supported from D3hot
>> [    0.448635] PCI: bus1: Fast back to back transfers disabled
>> [    0.448717] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
>> [    0.448729] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
>> [    0.448738] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
>> [    0.448751] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>> [    0.448760] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>> [    0.448772] pci 0000:01:00.0: BAR 4: assigned [io  0x1000-0x100f]
>> [    0.448784] pci 0000:01:00.0: BAR 0: assigned [io  0x1010-0x1017]
>> [    0.448796] pci 0000:01:00.0: BAR 2: assigned [io  0x1018-0x101f]
>> [    0.448808] pci 0000:01:00.0: BAR 1: assigned [io  0x1020-0x1023]
>> [    0.448820] pci 0000:01:00.0: BAR 3: assigned [io  0x1024-0x1027]
>> [    0.448833] pci 0000:00:00.0: PCI bridge to [bus 01]
>> [    0.448842] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
>> [    0.448851] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
>> [    0.448860] pci 0000:00:00.0:   bridge window [mem 0x60100000-0x601fffff pref]
>> [    0.449077] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
>> [    0.449086] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
>> [    0.449095] pcie_pme 0000:00:00.0:pcie01: service driver pcie_pme loaded
>> [    0.449240] aer 0000:00:00.0:pcie02: service driver aer loaded
>>
>>
>> Thanks and regards,
>>
>> -- 
>> Murali Karicheri
>> Linux Kernel, Keystone
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Bjorn Helgaas]

On Wed, Mar 16, 2016 at 02:08:47PM -0400, Murali Karicheri wrote:
> Bjorn,
> 
> Thanks for your quick response! Please see below some clarification
> and follow up question.
> 
> On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
> 0x1000]
> > 
> > Obviously if the host bridge doesn't support I/O port space, we will
> > be unable to assign space for I/O BARs, so you will see errors like
> > this.  
> > 
> > We may be able to improve the message and/or make this less noisy.
> > Guenter Roeck looked at a similar issue a while ago, but it's not
> > completely trivial:
> > 
> >   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
> > 
> > The PCI core should check in pci_enable_device() whether all the
> > device BARs have been assigned.  If not, it should fail.  But if a
> > driver doesn't need I/O space, it can use pci_enable_device_mem() to
> > indicate that it only needs the MEM BARs.  That should succeed even if
> > the I/O BARs aren't assigned.
> > 
> > Bottom line, if you omit I/O space on your host bridge:
> > 
> >   - You will see annoying "no space for" and "failed to assign" messages
> >   - Drivers that don't need I/O ports should still work
> >   - It's far better to have the messages than it was to pretend that
> >     the host bridge supported I/O space when it really didn't.
> > 
> >> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> >> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> >> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
> >> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
> >> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
> >> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
> >> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
> >> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
> >> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
> >> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
> >> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
> >> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
> >> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
> >>
> >>
> >> The original log is below and even with the error, I am able to have SATA
> >> drive function as expected over this PCIe interface.
> >>
> >>
> >> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
> >> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
> >> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
> >> [    0.420685] Requested IO range too big, new size set to 64K
> >> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
> >> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
> >> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
> >> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
> >> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
> > 
> > This range is obviously bogus, since it's way too big and not a nice
> > round size.  I guess this is what you're fixing.
> 
> Yes. But from your response, I gather it is better to remove the bogus range.
> I removed the range, and did a read/write test to the hard drive connected 
> to the Marvel SATA that is hooked to the PCIe interface and it still work
> without issues. 

If your bridge doesn't support I/O space, you should definitely remove
the range.

I'm curious about this Marvell SATA device, though.  It is this
device?

  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]

If so, it looks like it uses the drivers/ata/ahci.c driver, which uses
pcim_enable_device(), which should require all BARs to be assigned.
(It doesn't look like there is a pcim_enable_device_mem() variant.)

But if you're on an arm or arm64 platform and you have PCI_PROBE_ONLY
set, pcibios_enable_device() doesn't check whether resources are
assigned, so the problem would be masked.  We're trying to remove
PCI_PROBE_ONLY, or at least remove it from paths like this, so this
might become a problem soon.

This might be a reason to add a pcim_enable_device_mem() interface
that ahci_init_one() could use.  It looks like ahci_init_one() doesn't
actually depend on the I/O BAR.

> Another thing to worry about is the customers who are using custom
> fpga pci devices connected to the pcie bus and presently using 
> pci_enable_device() in their driver. So I guess if they fix their driver
> to use pci_enable_device_mem() instead, it should continue to work
> without issues, right? 

Yes.

If the FPGA PCI devices don't have I/O BARs, it doesn't matter whether
the driver uses pci_enable_device() or pci_enable_device_mem().  If
the device has an I/O BAR, but the driver doesn't need it, the driver
should use pci_enable_device_mem().  That will make it more portable.

Bjorn

Re: PCI IO resource question.

[Murali Karicheri]

On 03/16/2016 03:29 PM, Bjorn Helgaas wrote:
> On Wed, Mar 16, 2016 at 02:08:47PM -0400, Murali Karicheri wrote:
>> Bjorn,
>>
>> Thanks for your quick response! Please see below some clarification
>> and follow up question.
>>
>> On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
>> 0x1000]
>>>
>>> Obviously if the host bridge doesn't support I/O port space, we will
>>> be unable to assign space for I/O BARs, so you will see errors like
>>> this.  
>>>
>>> We may be able to improve the message and/or make this less noisy.
>>> Guenter Roeck looked at a similar issue a while ago, but it's not
>>> completely trivial:
>>>
>>>   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
>>>
>>> The PCI core should check in pci_enable_device() whether all the
>>> device BARs have been assigned.  If not, it should fail.  But if a
>>> driver doesn't need I/O space, it can use pci_enable_device_mem() to
>>> indicate that it only needs the MEM BARs.  That should succeed even if
>>> the I/O BARs aren't assigned.
>>>
>>> Bottom line, if you omit I/O space on your host bridge:
>>>
>>>   - You will see annoying "no space for" and "failed to assign" messages
>>>   - Drivers that don't need I/O ports should still work
>>>   - It's far better to have the messages than it was to pretend that
>>>     the host bridge supported I/O space when it really didn't.
>>>
>>>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>>>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>>>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
>>>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
>>>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
>>>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
>>>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
>>>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
>>>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
>>>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
>>>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
>>>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
>>>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
>>>>
>>>>
>>>> The original log is below and even with the error, I am able to have SATA
>>>> drive function as expected over this PCIe interface.
>>>>
>>>>
>>>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
>>>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
>>>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
>>>> [    0.420685] Requested IO range too big, new size set to 64K
>>>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
>>>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
>>>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
>>>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
>>>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
>>>
>>> This range is obviously bogus, since it's way too big and not a nice
>>> round size.  I guess this is what you're fixing.
>>
>> Yes. But from your response, I gather it is better to remove the bogus range.
>> I removed the range, and did a read/write test to the hard drive connected 
>> to the Marvel SATA that is hooked to the PCIe interface and it still work
>> without issues. 
> 
> If your bridge doesn't support I/O space, you should definitely remove
> the range.
> 
Ok. Will do.

> I'm curious about this Marvell SATA device, though.  It is this
> device?
> 

Yes.

>   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]
> 
> If so, it looks like it uses the drivers/ata/ahci.c driver, which uses
> pcim_enable_device(), which should require all BARs to be assigned.
> (It doesn't look like there is a pcim_enable_device_mem() variant.)
> 
What does it mean in the context of removing the IO resource DT binding?
My AHCI SATA driver works fine with the IO DT bindings removed except
that I see the the error log for assigning IO BAR

If it expects all resources to be assigned, then it should have 
failed, right? But not.

I see following

[    1.547690] ahci 0000:01:00.0: version 3.0
[    1.551833] ahci 0000:01:00.0: limiting MRRS to 256
[    1.556822] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
[    1.564943] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 


And Then

[    1.940284] ata1: SATA link down (SStatus 0 SControl 300)
[    2.140278] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[    2.147029] ata2.00: ATA-9: WDC WD10EZEX-08M2NA0, 01.01A01, max UDMA/100
[    2.153752] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
[    2.161524] ata2.00: configured for UDMA/100
[    2.165957] bounce: isa pool size: 16 pages
[    2.170289] scsi 1:0:0:0: Direct-Access     ATA      WDC WD10EZEX-08M 1A01 PQ: 0 ANSI: 5
[    2.178967] sd 1:0:0:0: [sda] 1953525168 512-byte logical blocks: (1.00 TB/932 GiB)
[    2.186632] sd 1:0:0:0: [sda] 4096-byte physical blocks
[    2.192047] sd 1:0:0:0: [sda] Write Protect is off
[    2.196835] sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
[    2.201968] sd 1:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
[    2.226610]  sda: sda1 sda2
[    2.230300] sd 1:0:0:0: [sda] Attached SCSI removable disk


> But if you're on an arm or arm64 platform and you have PCI_PROBE_ONLY
> set, pcibios_enable_device() doesn't check whether resources are
> assigned, so the problem would be masked.  We're trying to remove
> PCI_PROBE_ONLY, or at least remove it from paths like this, so this
> might become a problem soon.
> 

Keystone is ARM v7 A15 based. The driver doesn't set PCI_PROBE_ONLY.
So am expect face problem when you remove PCI_PROBE_ONLY? I guess not.
I looked at pci_enable_resources()

	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
		if (!(mask & (1 << i)))
			continue;

		r = &dev->resource[i];

		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
			continue;
		if ((i == PCI_ROM_RESOURCE) &&
				(!(r->flags & IORESOURCE_ROM_ENABLE)))
			continue;

		if (r->flags & IORESOURCE_UNSET) {
			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
				i, r);
			return -EINVAL;
		}

		if (!r->parent) {
			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
				i, r);
			return -EINVAL;
		}

I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
depend on IO bar as you mention below or is it in a different function?

Murali
> This might be a reason to add a pcim_enable_device_mem() interface
> that ahci_init_one() could use.  It looks like ahci_init_one() doesn't
> actually depend on the I/O BAR.
> 
>> Another thing to worry about is the customers who are using custom
>> fpga pci devices connected to the pcie bus and presently using 
>> pci_enable_device() in their driver. So I guess if they fix their driver
>> to use pci_enable_device_mem() instead, it should continue to work
>> without issues, right? 
> 
> Yes.
> 
> If the FPGA PCI devices don't have I/O BARs, it doesn't matter whether
> the driver uses pci_enable_device() or pci_enable_device_mem().  If
> the device has an I/O BAR, but the driver doesn't need it, the driver
> should use pci_enable_device_mem().  That will make it more portable.
> 
> Bjorn
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Bjorn Helgaas]

On Wed, Mar 16, 2016 at 04:13:30PM -0400, Murali Karicheri wrote:
> On 03/16/2016 03:29 PM, Bjorn Helgaas wrote:
> > On Wed, Mar 16, 2016 at 02:08:47PM -0400, Murali Karicheri wrote:
> >> Bjorn,
> >>
> >> Thanks for your quick response! Please see below some clarification
> >> and follow up question.
> >>
> >> On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
> >> 0x1000]
> >>>
> >>> Obviously if the host bridge doesn't support I/O port space, we will
> >>> be unable to assign space for I/O BARs, so you will see errors like
> >>> this.  
> >>>
> >>> We may be able to improve the message and/or make this less noisy.
> >>> Guenter Roeck looked at a similar issue a while ago, but it's not
> >>> completely trivial:
> >>>
> >>>   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
> >>>
> >>> The PCI core should check in pci_enable_device() whether all the
> >>> device BARs have been assigned.  If not, it should fail.  But if a
> >>> driver doesn't need I/O space, it can use pci_enable_device_mem() to
> >>> indicate that it only needs the MEM BARs.  That should succeed even if
> >>> the I/O BARs aren't assigned.
> >>>
> >>> Bottom line, if you omit I/O space on your host bridge:
> >>>
> >>>   - You will see annoying "no space for" and "failed to assign" messages
> >>>   - Drivers that don't need I/O ports should still work
> >>>   - It's far better to have the messages than it was to pretend that
> >>>     the host bridge supported I/O space when it really didn't.
> >>>
> >>>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> >>>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> >>>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
> >>>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
> >>>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
> >>>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
> >>>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
> >>>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
> >>>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
> >>>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
> >>>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
> >>>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
> >>>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
> >>>>
> >>>>
> >>>> The original log is below and even with the error, I am able to have SATA
> >>>> drive function as expected over this PCIe interface.
> >>>>
> >>>>
> >>>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
> >>>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
> >>>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
> >>>> [    0.420685] Requested IO range too big, new size set to 64K
> >>>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
> >>>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
> >>>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
> >>>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
> >>>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
> >>>
> >>> This range is obviously bogus, since it's way too big and not a nice
> >>> round size.  I guess this is what you're fixing.
> >>
> >> Yes. But from your response, I gather it is better to remove the bogus range.
> >> I removed the range, and did a read/write test to the hard drive connected 
> >> to the Marvel SATA that is hooked to the PCIe interface and it still work
> >> without issues. 
> > 
> > If your bridge doesn't support I/O space, you should definitely remove
> > the range.
> > 
> Ok. Will do.
> 
> > I'm curious about this Marvell SATA device, though.  It is this
> > device?
> > 
> 
> Yes.
> 
> >   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]
> > 
> > If so, it looks like it uses the drivers/ata/ahci.c driver, which uses
> > pcim_enable_device(), which should require all BARs to be assigned.
> > (It doesn't look like there is a pcim_enable_device_mem() variant.)
> > 
> What does it mean in the context of removing the IO resource DT binding?
> My AHCI SATA driver works fine with the IO DT bindings removed except
> that I see the the error log for assigning IO BAR
> 
> If it expects all resources to be assigned, then it should have 
> failed, right? But not.

Right, I'm expecting the SATA driver to fail when you remove the DT I/O
resource binding.  I want to figure out why it doesn't fail.

> I see following
> 
> [    1.547690] ahci 0000:01:00.0: version 3.0
> [    1.551833] ahci 0000:01:00.0: limiting MRRS to 256
> [    1.556822] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
> [    1.564943] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> 
> 
> And Then
> 
> [    1.940284] ata1: SATA link down (SStatus 0 SControl 300)
> [    2.140278] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> [    2.147029] ata2.00: ATA-9: WDC WD10EZEX-08M2NA0, 01.01A01, max UDMA/100
> [    2.153752] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
> [    2.161524] ata2.00: configured for UDMA/100
> [    2.165957] bounce: isa pool size: 16 pages
> [    2.170289] scsi 1:0:0:0: Direct-Access     ATA      WDC WD10EZEX-08M 1A01 PQ: 0 ANSI: 5
> [    2.178967] sd 1:0:0:0: [sda] 1953525168 512-byte logical blocks: (1.00 TB/932 GiB)
> [    2.186632] sd 1:0:0:0: [sda] 4096-byte physical blocks
> [    2.192047] sd 1:0:0:0: [sda] Write Protect is off
> [    2.196835] sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
> [    2.201968] sd 1:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
> [    2.226610]  sda: sda1 sda2
> [    2.230300] sd 1:0:0:0: [sda] Attached SCSI removable disk
> 
> 
> > But if you're on an arm or arm64 platform and you have PCI_PROBE_ONLY
> > set, pcibios_enable_device() doesn't check whether resources are
> > assigned, so the problem would be masked.  We're trying to remove
> > PCI_PROBE_ONLY, or at least remove it from paths like this, so this
> > might become a problem soon.
> > 
> 
> Keystone is ARM v7 A15 based. The driver doesn't set PCI_PROBE_ONLY.
> So am expect face problem when you remove PCI_PROBE_ONLY? I guess not.

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?

> I looked at pci_enable_resources()
> 
> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> 		if (!(mask & (1 << i)))
> 			continue;
> 
> 		r = &dev->resource[i];

                dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);

> 
> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> 			continue;
> 		if ((i == PCI_ROM_RESOURCE) &&
> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
> 			continue;
> 
> 		if (r->flags & IORESOURCE_UNSET) {
> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
> 				i, r);
> 			return -EINVAL;
> 		}
> 
> 		if (!r->parent) {
> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
> 				i, r);
> 			return -EINVAL;
> 		}
> 
> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
> depend on IO bar as you mention below or is it in a different function?

Re: PCI IO resource question.

[Murali Karicheri]

On 03/16/2016 05:47 PM, Bjorn Helgaas wrote:
> On Wed, Mar 16, 2016 at 04:13:30PM -0400, Murali Karicheri wrote:
>> On 03/16/2016 03:29 PM, Bjorn Helgaas wrote:
>>> On Wed, Mar 16, 2016 at 02:08:47PM -0400, Murali Karicheri wrote:
>>>> Bjorn,
>>>>
>>>> Thanks for your quick response! Please see below some clarification
>>>> and follow up question.
>>>>
>>>> On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
>>>> 0x1000]
>>>>>
>>>>> Obviously if the host bridge doesn't support I/O port space, we will
>>>>> be unable to assign space for I/O BARs, so you will see errors like
>>>>> this.  
>>>>>
>>>>> We may be able to improve the message and/or make this less noisy.
>>>>> Guenter Roeck looked at a similar issue a while ago, but it's not
>>>>> completely trivial:
>>>>>
>>>>>   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
>>>>>
>>>>> The PCI core should check in pci_enable_device() whether all the
>>>>> device BARs have been assigned.  If not, it should fail.  But if a
>>>>> driver doesn't need I/O space, it can use pci_enable_device_mem() to
>>>>> indicate that it only needs the MEM BARs.  That should succeed even if
>>>>> the I/O BARs aren't assigned.
>>>>>
>>>>> Bottom line, if you omit I/O space on your host bridge:
>>>>>
>>>>>   - You will see annoying "no space for" and "failed to assign" messages
>>>>>   - Drivers that don't need I/O ports should still work
>>>>>   - It's far better to have the messages than it was to pretend that
>>>>>     the host bridge supported I/O space when it really didn't.
>>>>>
>>>>>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>>>>>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>>>>>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
>>>>>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
>>>>>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
>>>>>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
>>>>>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
>>>>>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
>>>>>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
>>>>>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
>>>>>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
>>>>>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
>>>>>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
>>>>>>
>>>>>>
>>>>>> The original log is below and even with the error, I am able to have SATA
>>>>>> drive function as expected over this PCIe interface.
>>>>>>
>>>>>>
>>>>>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
>>>>>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
>>>>>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
>>>>>> [    0.420685] Requested IO range too big, new size set to 64K
>>>>>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
>>>>>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
>>>>>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
>>>>>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
>>>>>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
>>>>>
>>>>> This range is obviously bogus, since it's way too big and not a nice
>>>>> round size.  I guess this is what you're fixing.
>>>>
>>>> Yes. But from your response, I gather it is better to remove the bogus range.
>>>> I removed the range, and did a read/write test to the hard drive connected 
>>>> to the Marvel SATA that is hooked to the PCIe interface and it still work
>>>> without issues. 
>>>
>>> If your bridge doesn't support I/O space, you should definitely remove
>>> the range.
>>>
>> Ok. Will do.
>>
>>> I'm curious about this Marvell SATA device, though.  It is this
>>> device?
>>>
>>
>> Yes.
>>
>>>   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]
>>>
>>> If so, it looks like it uses the drivers/ata/ahci.c driver, which uses
>>> pcim_enable_device(), which should require all BARs to be assigned.
>>> (It doesn't look like there is a pcim_enable_device_mem() variant.)
>>>
>> What does it mean in the context of removing the IO resource DT binding?
>> My AHCI SATA driver works fine with the IO DT bindings removed except
>> that I see the the error log for assigning IO BAR
>>
>> If it expects all resources to be assigned, then it should have 
>> failed, right? But not.
> 
> Right, I'm expecting the SATA driver to fail when you remove the DT I/O
> resource binding.  I want to figure out why it doesn't fail.
> 
>> I see following
>>
>> [    1.547690] ahci 0000:01:00.0: version 3.0
>> [    1.551833] ahci 0000:01:00.0: limiting MRRS to 256
>> [    1.556822] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
>> [    1.564943] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
>>
>>
>> And Then
>>
>> [    1.940284] ata1: SATA link down (SStatus 0 SControl 300)
>> [    2.140278] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>> [    2.147029] ata2.00: ATA-9: WDC WD10EZEX-08M2NA0, 01.01A01, max UDMA/100
>> [    2.153752] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
>> [    2.161524] ata2.00: configured for UDMA/100
>> [    2.165957] bounce: isa pool size: 16 pages
>> [    2.170289] scsi 1:0:0:0: Direct-Access     ATA      WDC WD10EZEX-08M 1A01 PQ: 0 ANSI: 5
>> [    2.178967] sd 1:0:0:0: [sda] 1953525168 512-byte logical blocks: (1.00 TB/932 GiB)
>> [    2.186632] sd 1:0:0:0: [sda] 4096-byte physical blocks
>> [    2.192047] sd 1:0:0:0: [sda] Write Protect is off
>> [    2.196835] sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
>> [    2.201968] sd 1:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>> [    2.226610]  sda: sda1 sda2
>> [    2.230300] sd 1:0:0:0: [sda] Attached SCSI removable disk
>>
>>
>>> But if you're on an arm or arm64 platform and you have PCI_PROBE_ONLY
>>> set, pcibios_enable_device() doesn't check whether resources are
>>> assigned, so the problem would be masked.  We're trying to remove
>>> PCI_PROBE_ONLY, or at least remove it from paths like this, so this
>>> might become a problem soon.
>>>
>>
>> Keystone is ARM v7 A15 based. The driver doesn't set PCI_PROBE_ONLY.
>> So am expect face problem when you remove PCI_PROBE_ONLY? I guess not.
> 
> 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?
> 
Will do, but need sometime. Will post once done.


Murali
>> I looked at pci_enable_resources()
>>
>> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>> 		if (!(mask & (1 << i)))
>> 			continue;
>>
>> 		r = &dev->resource[i];
> 
>                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
> 
>>
>> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
>> 			continue;
>> 		if ((i == PCI_ROM_RESOURCE) &&
>> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
>> 			continue;
>>
>> 		if (r->flags & IORESOURCE_UNSET) {
>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
>> 				i, r);
>> 			return -EINVAL;
>> 		}
>>
>> 		if (!r->parent) {
>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
>> 				i, r);
>> 			return -EINVAL;
>> 		}
>>
>> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
>> depend on IO bar as you mention below or is it in a different function?
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Murali Karicheri]

On 03/16/2016 05:47 PM, Bjorn Helgaas wrote:
> On Wed, Mar 16, 2016 at 04:13:30PM -0400, Murali Karicheri wrote:
>> On 03/16/2016 03:29 PM, Bjorn Helgaas wrote:
>>> On Wed, Mar 16, 2016 at 02:08:47PM -0400, Murali Karicheri wrote:
>>>> Bjorn,
>>>>
>>>> Thanks for your quick response! Please see below some clarification
>>>> and follow up question.
>>>>
>>>> On 03/16/2016 12:45 PM, Bjorn Helgaas wrote:
>>>> 0x1000]
>>>>>
>>>>> Obviously if the host bridge doesn't support I/O port space, we will
>>>>> be unable to assign space for I/O BARs, so you will see errors like
>>>>> this.  
>>>>>
>>>>> We may be able to improve the message and/or make this less noisy.
>>>>> Guenter Roeck looked at a similar issue a while ago, but it's not
>>>>> completely trivial:
>>>>>
>>>>>   http://lkml.kernel.org/r/20150515172836.GA27797@svl-evodev-groeck.juniper.net
>>>>>
>>>>> The PCI core should check in pci_enable_device() whether all the
>>>>> device BARs have been assigned.  If not, it should fail.  But if a
>>>>> driver doesn't need I/O space, it can use pci_enable_device_mem() to
>>>>> indicate that it only needs the MEM BARs.  That should succeed even if
>>>>> the I/O BARs aren't assigned.
>>>>>
>>>>> Bottom line, if you omit I/O space on your host bridge:
>>>>>
>>>>>   - You will see annoying "no space for" and "failed to assign" messages
>>>>>   - Drivers that don't need I/O ports should still work
>>>>>   - It's far better to have the messages than it was to pretend that
>>>>>     the host bridge supported I/O space when it really didn't.
>>>>>
>>>>>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>>>>>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>>>>>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
>>>>>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
>>>>>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
>>>>>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
>>>>>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
>>>>>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
>>>>>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
>>>>>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
>>>>>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
>>>>>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
>>>>>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
>>>>>>
>>>>>>
>>>>>> The original log is below and even with the error, I am able to have SATA
>>>>>> drive function as expected over this PCIe interface.
>>>>>>
>>>>>>
>>>>>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
>>>>>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
>>>>>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
>>>>>> [    0.420685] Requested IO range too big, new size set to 64K
>>>>>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
>>>>>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
>>>>>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
>>>>>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
>>>>>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
>>>>>
>>>>> This range is obviously bogus, since it's way too big and not a nice
>>>>> round size.  I guess this is what you're fixing.
>>>>
>>>> Yes. But from your response, I gather it is better to remove the bogus range.
>>>> I removed the range, and did a read/write test to the hard drive connected 
>>>> to the Marvel SATA that is hooked to the PCIe interface and it still work
>>>> without issues. 
>>>
>>> If your bridge doesn't support I/O space, you should definitely remove
>>> the range.
>>>
>> Ok. Will do.
>>
>>> I'm curious about this Marvell SATA device, though.  It is this
>>> device?
>>>
>>
>> Yes.
>>
>>>   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]
>>>
>>> If so, it looks like it uses the drivers/ata/ahci.c driver, which uses
>>> pcim_enable_device(), which should require all BARs to be assigned.
>>> (It doesn't look like there is a pcim_enable_device_mem() variant.)
>>>
>> What does it mean in the context of removing the IO resource DT binding?
>> My AHCI SATA driver works fine with the IO DT bindings removed except
>> that I see the the error log for assigning IO BAR
>>
>> If it expects all resources to be assigned, then it should have 
>> failed, right? But not.
> 
> Right, I'm expecting the SATA driver to fail when you remove the DT I/O
> resource binding.  I want to figure out why it doesn't fail.
> 
>> I see following
>>
>> [    1.547690] ahci 0000:01:00.0: version 3.0
>> [    1.551833] ahci 0000:01:00.0: limiting MRRS to 256
>> [    1.556822] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
>> [    1.564943] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
>>
>>
>> And Then
>>
>> [    1.940284] ata1: SATA link down (SStatus 0 SControl 300)
>> [    2.140278] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>> [    2.147029] ata2.00: ATA-9: WDC WD10EZEX-08M2NA0, 01.01A01, max UDMA/100
>> [    2.153752] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
>> [    2.161524] ata2.00: configured for UDMA/100
>> [    2.165957] bounce: isa pool size: 16 pages
>> [    2.170289] scsi 1:0:0:0: Direct-Access     ATA      WDC WD10EZEX-08M 1A01 PQ: 0 ANSI: 5
>> [    2.178967] sd 1:0:0:0: [sda] 1953525168 512-byte logical blocks: (1.00 TB/932 GiB)
>> [    2.186632] sd 1:0:0:0: [sda] 4096-byte physical blocks
>> [    2.192047] sd 1:0:0:0: [sda] Write Protect is off
>> [    2.196835] sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
>> [    2.201968] sd 1:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>> [    2.226610]  sda: sda1 sda2
>> [    2.230300] sd 1:0:0:0: [sda] Attached SCSI removable disk
>>
>>
>>> But if you're on an arm or arm64 platform and you have PCI_PROBE_ONLY
>>> set, pcibios_enable_device() doesn't check whether resources are
>>> assigned, so the problem would be masked.  We're trying to remove
>>> PCI_PROBE_ONLY, or at least remove it from paths like this, so this
>>> might become a problem soon.
>>>
>>
>> Keystone is ARM v7 A15 based. The driver doesn't set PCI_PROBE_ONLY.
>> So am expect face problem when you remove PCI_PROBE_ONLY? I guess not.
> 
> 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
[    1.583608] ahci 0000:01:00.0: limiting MRRS to 256
[    1.588595] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
[    1.596716] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
[    1.606183] scsi host0: ahci
[    1.609448] scsi host1: ahci
[    1.612636] ata1: SATA max UDMA/133 abar m512@0x60000000 port 0x60000100 irq 82
[    1.619974] ata2: SATA max UDMA/133 abar m512@0x60000000 port 0x60000180 irq 82

> 
>> I looked at pci_enable_resources()
>>
>> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>> 		if (!(mask & (1 << i)))
>> 			continue;
>>
>> 		r = &dev->resource[i];
> 
>                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
> 
>>
>> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
>> 			continue;
>> 		if ((i == PCI_ROM_RESOURCE) &&
>> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
>> 			continue;
>>
>> 		if (r->flags & IORESOURCE_UNSET) {
>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
>> 				i, r);
>> 			return -EINVAL;
>> 		}
>>
>> 		if (!r->parent) {
>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
>> 				i, r);
>> 			return -EINVAL;
>> 		}
>>
>> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
>> depend on IO bar as you mention below or is it in a different function?
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Lorenzo Pieralisi]

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).

Not saying that's what should happen, I think that's what's happening,
that's the only reason I see why you do not have pci_enable_resources()
failures when you remove the IO range from the host bridge.

Lorenzo

> [    1.583608] ahci 0000:01:00.0: limiting MRRS to 256
> [    1.588595] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
> [    1.596716] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> [    1.606183] scsi host0: ahci
> [    1.609448] scsi host1: ahci
> [    1.612636] ata1: SATA max UDMA/133 abar m512@0x60000000 port 0x60000100 irq 82
> [    1.619974] ata2: SATA max UDMA/133 abar m512@0x60000000 port 0x60000180 irq 82
> 
> > 
> >> I looked at pci_enable_resources()
> >>
> >> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> >> 		if (!(mask & (1 << i)))
> >> 			continue;
> >>
> >> 		r = &dev->resource[i];
> > 
> >                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
> > 
> >>
> >> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> >> 			continue;
> >> 		if ((i == PCI_ROM_RESOURCE) &&
> >> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
> >> 			continue;
> >>
> >> 		if (r->flags & IORESOURCE_UNSET) {
> >> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
> >> 				i, r);
> >> 			return -EINVAL;
> >> 		}
> >>
> >> 		if (!r->parent) {
> >> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
> >> 				i, r);
> >> 			return -EINVAL;
> >> 		}
> >>
> >> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
> >> depend on IO bar as you mention below or is it in a different function?
> > 
> 
> 
> -- 
> Murali Karicheri
> Linux Kernel, Keystone
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: PCI IO resource question.

[Bjorn Helgaas]

On Fri, Mar 18, 2016 at 11:28:02AM +0000, 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).

I didn't look at the code to try to validate this theory, but it seems
plausible, and Murali could easily test it.

The very existence of reset_resource(), which clears res->flags, is a
problem because it means we're losing information about a hardware
BAR.  The fact that Linux throws away that information certainly
doesn't keep the device from responding based on the BAR contents.

If we're unable to assign any I/O BARs for a device, and we use
reset_resource() on them, pci_enable_resources() would not find any
IORESOURCE_IO BARs, so it would not turn on PCI_COMMAND_IO, so things
might appear to "work".  But pci_enable_resources() never turns
PCI_COMMAND_IO *off*, so if firmware left it enabled, we could still
have an issue.

We definitely have a problem if a device has two I/O BARs and we
assign one but call reset_resource() on the other.  Then
pci_enable_resources() won't know about the second one, so it will
turn on PCI_COMMAND_IO.  Then we have a potential conflict because the
second BAR is enabled but we don't know its contents.

> Not saying that's what should happen, I think that's what's happening,
> that's the only reason I see why you do not have pci_enable_resources()
> failures when you remove the IO range from the host bridge.

Makes a lot of sense; thanks for looking at this.

> > [    1.583608] ahci 0000:01:00.0: limiting MRRS to 256
> > [    1.588595] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
> > [    1.596716] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> > [    1.606183] scsi host0: ahci
> > [    1.609448] scsi host1: ahci
> > [    1.612636] ata1: SATA max UDMA/133 abar m512@0x60000000 port 0x60000100 irq 82
> > [    1.619974] ata2: SATA max UDMA/133 abar m512@0x60000000 port 0x60000180 irq 82
> > 
> > > 
> > >> I looked at pci_enable_resources()
> > >>
> > >> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > >> 		if (!(mask & (1 << i)))
> > >> 			continue;
> > >>
> > >> 		r = &dev->resource[i];
> > > 
> > >                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
> > > 
> > >>
> > >> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> > >> 			continue;
> > >> 		if ((i == PCI_ROM_RESOURCE) &&
> > >> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
> > >> 			continue;
> > >>
> > >> 		if (r->flags & IORESOURCE_UNSET) {
> > >> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
> > >> 				i, r);
> > >> 			return -EINVAL;
> > >> 		}
> > >>
> > >> 		if (!r->parent) {
> > >> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
> > >> 				i, r);
> > >> 			return -EINVAL;
> > >> 		}
> > >>
> > >> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
> > >> depend on IO bar as you mention below or is it in a different function?
> > > 
> > 
> > 
> > -- 
> > Murali Karicheri
> > Linux Kernel, Keystone
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PCI IO resource question.

[Murali Karicheri]

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.

Murali
> 
> Not saying that's what should happen, I think that's what's happening,
> that's the only reason I see why you do not have pci_enable_resources()
> failures when you remove the IO range from the host bridge.
> 
> Lorenzo
> 
>> [    1.583608] ahci 0000:01:00.0: limiting MRRS to 256
>> [    1.588595] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
>> [    1.596716] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
>> [    1.606183] scsi host0: ahci
>> [    1.609448] scsi host1: ahci
>> [    1.612636] ata1: SATA max UDMA/133 abar m512@0x60000000 port 0x60000100 irq 82
>> [    1.619974] ata2: SATA max UDMA/133 abar m512@0x60000000 port 0x60000180 irq 82
>>
>>>
>>>> I looked at pci_enable_resources()
>>>>
>>>> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>>> 		if (!(mask & (1 << i)))
>>>> 			continue;
>>>>
>>>> 		r = &dev->resource[i];
>>>
>>>                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
>>>
>>>>
>>>> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
>>>> 			continue;
>>>> 		if ((i == PCI_ROM_RESOURCE) &&
>>>> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
>>>> 			continue;
>>>>
>>>> 		if (r->flags & IORESOURCE_UNSET) {
>>>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
>>>> 				i, r);
>>>> 			return -EINVAL;
>>>> 		}
>>>>
>>>> 		if (!r->parent) {
>>>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
>>>> 				i, r);
>>>> 			return -EINVAL;
>>>> 		}
>>>>
>>>> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
>>>> depend on IO bar as you mention below or is it in a different function?
>>>
>>
>>
>> -- 
>> Murali Karicheri
>> Linux Kernel, Keystone
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Murali Karicheri]

On 03/18/2016 11:09 AM, 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 see only see memory resources passed in 

[    1.592835] pci_enable_resources, mask 60
[    1.596843] ahci 0000:01:00.0: res 5: [mem 0x60000000-0x600001ff]
[    1.602962] ahci 0000:01:00.0: res 6: [mem 0x60100000-0x6010ffff pref]

> 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.
> 
> Murali
>>
>> Not saying that's what should happen, I think that's what's happening,
>> that's the only reason I see why you do not have pci_enable_resources()
>> failures when you remove the IO range from the host bridge.
>>
>> Lorenzo
>>
>>> [    1.583608] ahci 0000:01:00.0: limiting MRRS to 256
>>> [    1.588595] ahci 0000:01:00.0: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl SATA mode
>>> [    1.596716] ahci 0000:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
>>> [    1.606183] scsi host0: ahci
>>> [    1.609448] scsi host1: ahci
>>> [    1.612636] ata1: SATA max UDMA/133 abar m512@0x60000000 port 0x60000100 irq 82
>>> [    1.619974] ata2: SATA max UDMA/133 abar m512@0x60000000 port 0x60000180 irq 82
>>>
>>>>
>>>>> I looked at pci_enable_resources()
>>>>>
>>>>> 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>>>> 		if (!(mask & (1 << i)))
>>>>> 			continue;
>>>>>
>>>>> 		r = &dev->resource[i];
>>>>
>>>>                 dev_info(&dev->dev, "BAR %d %pR mask %#04x parent %p\n", i, r, mask, r->parent);
>>>>
>>>>>
>>>>> 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
>>>>> 			continue;
>>>>> 		if ((i == PCI_ROM_RESOURCE) &&
>>>>> 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
>>>>> 			continue;
>>>>>
>>>>> 		if (r->flags & IORESOURCE_UNSET) {
>>>>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
>>>>> 				i, r);
>>>>> 			return -EINVAL;
>>>>> 		}
>>>>>
>>>>> 		if (!r->parent) {
>>>>> 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not claimed\n",
>>>>> 				i, r);
>>>>> 			return -EINVAL;
>>>>> 		}
>>>>>
>>>>> I don't see the error "can't enable device: BAR %d %pR not assigned" , so it doesn't
>>>>> depend on IO bar as you mention below or is it in a different function?
>>>>
>>>
>>>
>>> -- 
>>> Murali Karicheri
>>> Linux Kernel, Keystone
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Bjorn Helgaas]

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);

Re: PCI IO resource question.

[Murali Karicheri]

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/

Let me know what you find.

Murali

-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Bjorn Helgaas]

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

Re: PCI IO resource question.

[Murali Karicheri]

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

Re: PCI IO resource question.

[Lorenzo Pieralisi]

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.

Please, tell us more about it, what does "would like to honour" mean ?

Do they (ab)use pci=firmware bootarg just to keep FW PCIe SATA settings ?

There are two ways of setting PCI_PROBE_ONLY on ARM/ARM64 (unfortunately):

- DT chosen node
- pci=firmware (on arm 32-bit platforms only)

PCIe designware does not check the DT chosen node property, so you are
telling me that some customers are abusing pci=firmware command line, that
was never meant to be used on platforms other than ARM IXP2000 systems
(see Documentation/kernel-parameters.txt).

Usage of PCI_PROBE_ONLY on ARM has been controversial and is completely
ill-defined, so we are trying to get rid of its usage.

Now, commit ed00c83cd490 ("PCI: designware: Remove PCI_PROBE_ONLY handling")
removed, after mailing list initial investigation and patch review

http://www.spinics.net/lists/linux-pci/msg48248.html

PCI_PROBE_ONLY handling from PCIe designware, which means that even if
you pass pci=firmware parameter on the command line (wrongly, see above)
the kernel reassigns resources and set-up PCIe settings.

Does this trigger issues on Keystone ? Or the systems that set that
bootarg work even if the flag is ignored ? I want to understand and
I really would like to avoid asking Bjorn to revert that commit for
what looks like an abuse, I prefer finding a workaround if this is
really an issue.

For the records, I am about to send a patch to remove pci=firmware
to Russell patch system, so I really have to know what to expect
on Keystone:

http://www.spinics.net/lists/linux-pci/msg49328.html

Please let me know, I ultimately want to implement code that on ARM
carries out proper PCI resources allocation (claiming firmware and
assign resources that fails to be claimed - ie they are not configured
by FW properly), removing PCI_PROBE_ONLY handling is a stepping stone
to get there.

Thank you for your patience,
Lorenzo

Re: PCI IO resource question.

[Murali Karicheri]

On 03/18/2016 07:05 PM, Lorenzo Pieralisi 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.
> 
> Please, tell us more about it, what does "would like to honour" mean ?
> 
> Do they (ab)use pci=firmware bootarg just to keep FW PCIe SATA settings ?
> 
> There are two ways of setting PCI_PROBE_ONLY on ARM/ARM64 (unfortunately):
> 
> - DT chosen node
> - pci=firmware (on arm 32-bit platforms only)
> 
second one. pci=firmware.

> PCIe designware does not check the DT chosen node property, so you are
> telling me that some customers are abusing pci=firmware command line, that
> was never meant to be used on platforms other than ARM IXP2000 systems
> (see Documentation/kernel-parameters.txt).
> 
> Usage of PCI_PROBE_ONLY on ARM has been controversial and is completely
> ill-defined, so we are trying to get rid of its usage.
> 
> Now, commit ed00c83cd490 ("PCI: designware: Remove PCI_PROBE_ONLY handling")
> removed, after mailing list initial investigation and patch review
> 
> http://www.spinics.net/lists/linux-pci/msg48248.html
> 
> PCI_PROBE_ONLY handling from PCIe designware, which means that even if
> you pass pci=firmware parameter on the command line (wrongly, see above)
> the kernel reassigns resources and set-up PCIe settings.
> 
> Does this trigger issues on Keystone ? Or the systems that set that
> bootarg work even if the flag is ignored ? I want to understand and
> I really would like to avoid asking Bjorn to revert that commit for
> what looks like an abuse, I prefer finding a workaround if this is
> really an issue.

Unfortunately the customer uses an older version of kernel v3.13. The
Ubuntu Linux distro runs on Keystone based board using this version of
kernel. We have internal version of the PCI-keystone driver that runs on
this kernel. The requirement was like this at a high level. The U-Boot
requires the SATA hard drive to be initialized to load and run images from
hard disk. When Linux boots up, the hard drive file system is used as
rootfs.

So few changes added in Kernel PCI driver to support this. 
1. Linux PCI driver assumes, the PCI Link is initialized in u-boot. So
it just don't do re-initialization of the SerDes link. It checks the
link status and continue initialize the PCI-Keystone RC driver.
2. Uses pci=firmware to make sure the memory BARs are not re-assigned.

Without that, the Linux kernel PCI driver got some issues and I don't
have the details now as this happened almost 2 years ago.

I believe at least on K2E, this is a use case that we need to support.
K2E board has a Marvel SATA controller with a SATA port that we can hook
up a hard drive. I have a TODO item to bring up PCI and SATA on u-boot
and boot up Linux the same way as described above. So how this use case
is expected to work on ARM? I assume arch/arm/kernel/bios32.c is a
pseudo bios driver to emulate BIOS specific handling PCI initialization.

I am sure this is a use case that needs to be supported. Aren't there
other ARM based boards that uses SATA hard disks in u-boot to boot to
Linux? 

Murali

> 
> For the records, I am about to send a patch to remove pci=firmware
> to Russell patch system, so I really have to know what to expect
> on Keystone:
> 
> http://www.spinics.net/lists/linux-pci/msg49328.html
> 
> Please let me know, I ultimately want to implement code that on ARM
> carries out proper PCI resources allocation (claiming firmware and
> assign resources that fails to be claimed - ie they are not configured
> by FW properly), removing PCI_PROBE_ONLY handling is a stepping stone
> to get there.
> 

How the above use case is handled if you remove the PCI_PROBE_ONLY support?

Murali
> Thank you for your patience,
> Lorenzo
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Lorenzo Pieralisi]

On Mon, Mar 21, 2016 at 11:24:26AM -0400, Murali Karicheri wrote:

[...]

> >> 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.
> > 
> > Please, tell us more about it, what does "would like to honour" mean ?
> > 
> > Do they (ab)use pci=firmware bootarg just to keep FW PCIe SATA settings ?
> > 
> > There are two ways of setting PCI_PROBE_ONLY on ARM/ARM64 (unfortunately):
> > 
> > - DT chosen node
> > - pci=firmware (on arm 32-bit platforms only)
> > 
> second one. pci=firmware.

And that's an abuse that has to be stopped, I am removing that flag
straight away for this precise reason.

> > PCIe designware does not check the DT chosen node property, so you are
> > telling me that some customers are abusing pci=firmware command line, that
> > was never meant to be used on platforms other than ARM IXP2000 systems
> > (see Documentation/kernel-parameters.txt).
> > 
> > Usage of PCI_PROBE_ONLY on ARM has been controversial and is completely
> > ill-defined, so we are trying to get rid of its usage.
> > 
> > Now, commit ed00c83cd490 ("PCI: designware: Remove PCI_PROBE_ONLY handling")
> > removed, after mailing list initial investigation and patch review
> > 
> > http://www.spinics.net/lists/linux-pci/msg48248.html
> > 
> > PCI_PROBE_ONLY handling from PCIe designware, which means that even if
> > you pass pci=firmware parameter on the command line (wrongly, see above)
> > the kernel reassigns resources and set-up PCIe settings.
> > 
> > Does this trigger issues on Keystone ? Or the systems that set that
> > bootarg work even if the flag is ignored ? I want to understand and
> > I really would like to avoid asking Bjorn to revert that commit for
> > what looks like an abuse, I prefer finding a workaround if this is
> > really an issue.
> 
> Unfortunately the customer uses an older version of kernel v3.13. The
> Ubuntu Linux distro runs on Keystone based board using this version of
> kernel. We have internal version of the PCI-keystone driver that runs on
> this kernel. The requirement was like this at a high level. The U-Boot
> requires the SATA hard drive to be initialized to load and run images from
> hard disk. When Linux boots up, the hard drive file system is used as
> rootfs.
> 
> So few changes added in Kernel PCI driver to support this. 
> 1. Linux PCI driver assumes, the PCI Link is initialized in u-boot. So
> it just don't do re-initialization of the SerDes link. It checks the
> link status and continue initialize the PCI-Keystone RC driver.
> 2. Uses pci=firmware to make sure the memory BARs are not re-assigned.
> 
> Without that, the Linux kernel PCI driver got some issues and I don't
> have the details now as this happened almost 2 years ago.

Ok, it is not a mainline problem then and it is not something we have
to fix, other than removing that bootarg flag as soon as possible
since as you describe here it can be (ab/mis)used.

> I believe at least on K2E, this is a use case that we need to support.
> K2E board has a Marvel SATA controller with a SATA port that we can hook
> up a hard drive. I have a TODO item to bring up PCI and SATA on u-boot
> and boot up Linux the same way as described above. So how this use case
> is expected to work on ARM? I assume arch/arm/kernel/bios32.c is a
> pseudo bios driver to emulate BIOS specific handling PCI initialization.

PCI bios is a set of helper functions that are there as PCI arm arch
back-end and to be honest I am not entirely sure I understand what you
expect it to do, it certainly does not emulate any BIOS to the best
of my knowledge.

> I am sure this is a use case that needs to be supported. Aren't there
> other ARM based boards that uses SATA hard disks in u-boot to boot to
> Linux? 

First off, PCI_PROBE_ONLY prevents reassigning resources and
reprogramming PCIe settings for the whole PCI(e) hierarchy managed
by all host controllers, it is not about one single device (and I
do not even understand what's the problem you are facing), this
for the records so that we are on the same page (maybe that's
the only device in that system, but I thought I should mention
that).

> How the above use case is handled if you remove the PCI_PROBE_ONLY support?

U-boot loads and boots the kernel in memory, quiesces the PCIe SATA device
before kernel handover and the kernel carries out PCI enumeration, loads and
probes the required device drivers.

Can you define precisely what the problem is please ? In particular
I want to understand why, by setting the PCI_PROBE_ONLY flag you want
to make sure your PCIe SATA device BARs are not reassigned.

Now, the question related to FW/OS handover of PCI resources is a
complicated one and on ARM it was never solved/defined properly.

The question here, and I am not sure it can be answered with a simple
answer, is how we make sure the OS does not touch PCI resources that
have been initialized in FW and are not meant to be changed (whatever
that means).

To at least have some hope to sort this out, the kernel must try to
claim PCI resources as set-up by FW, if they are in a "reasonable" state
they are claimed and not touched. For resources for which claiming
fails, a reassignment policy must be implemented, that might require
re-sizing bridges and release resources so that they can be effectively
reallocated.

At present, on ALL ARM platforms out there (except for some oddballs
and possibly some arm platforms that abused the pci=firmware parameter that
I am hunting down and I am not aware of) the whole PCI resource space is
always reassigned by the kernel (which does not mean that's always the
right thing to do BTW, as it is it just works).

We must not try to "fix" this by misusing boot arguments and PCI_PROBE_ONLY,
we must come up with a PCI resources allocation policy that implements what
I mention above, my point is, PCI_PROBE_ONLY must mean the same thing
for all ARM/ARM64 platforms otherwise it becomes complicated to handle in
a sane way.

Does this help ? Please let us know.

Thanks,
Lorenzo

Re: PCI IO resource question.

[Murali Karicheri]

On 03/21/2016 02:02 PM, Lorenzo Pieralisi wrote:
> On Mon, Mar 21, 2016 at 11:24:26AM -0400, Murali Karicheri wrote:
> 
> [...]
> 
>>>> 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.
>>>
>>> Please, tell us more about it, what does "would like to honour" mean ?
>>>
>>> Do they (ab)use pci=firmware bootarg just to keep FW PCIe SATA settings ?
>>>
>>> There are two ways of setting PCI_PROBE_ONLY on ARM/ARM64 (unfortunately):
>>>
>>> - DT chosen node
>>> - pci=firmware (on arm 32-bit platforms only)
>>>
>> second one. pci=firmware.
> 
> And that's an abuse that has to be stopped, I am removing that flag
> straight away for this precise reason.
> 
Let me speak up again. Keystone usage of pci=firmware was used by our
customer to address a specific problem. I am assuming they used it 
work around the BAR re-assignment problem in Linux space in v3.13 kernel
they were working with. 

The kernel-parameters.txt states

                firmware        [ARM] Do not re-enumerate the bus but instead
                                just use the configuration from the
                                bootloader. This is currently used on
                                IXP2000 systems where the bus has to be

Probably they used it to avoid re-enumerate similar to IXP2000. But as I
said before, I can't confirm the reason as this happened almost 2 years ago.

>>> PCIe designware does not check the DT chosen node property, so you are
>>> telling me that some customers are abusing pci=firmware command line, that
>>> was never meant to be used on platforms other than ARM IXP2000 systems
>>> (see Documentation/kernel-parameters.txt).

If the problem is same why you blame using this approach on non IXP2000 system?

>>>
>>> Usage of PCI_PROBE_ONLY on ARM has been controversial and is completely
>>> ill-defined, so we are trying to get rid of its usage.

No issues. At this point, I don't have the use case I described working in
our internal TI kernel based on v4.1.x or v4.4.x or upstream. However it will
be interesting to find the ARM platforms out there that uses firmware option,
irrespective of whether they abused its usage or not. Someone hopefully respond
to your patches if it breaks these platforms.

>>>
>>> Now, commit ed00c83cd490 ("PCI: designware: Remove PCI_PROBE_ONLY handling")
>>> removed, after mailing list initial investigation and patch review
>>>
>>> http://www.spinics.net/lists/linux-pci/msg48248.html
>>>
>>> PCI_PROBE_ONLY handling from PCIe designware, which means that even if
>>> you pass pci=firmware parameter on the command line (wrongly, see above)
>>> the kernel reassigns resources and set-up PCIe settings.
>>>
>>> Does this trigger issues on Keystone ? Or the systems that set that
>>> bootarg work even if the flag is ignored ? I want to understand and
>>> I really would like to avoid asking Bjorn to revert that commit for
>>> what looks like an abuse, I prefer finding a workaround if this is
>>> really an issue.

As I have said, I don't have the use case with pci=firmware working to
help you in this.

>>
>> Unfortunately the customer uses an older version of kernel v3.13. The
>> Ubuntu Linux distro runs on Keystone based board using this version of
>> kernel. We have internal version of the PCI-keystone driver that runs on
>> this kernel. The requirement was like this at a high level. The U-Boot
>> requires the SATA hard drive to be initialized to load and run images from
>> hard disk. When Linux boots up, the hard drive file system is used as
>> rootfs.
>>
>> So few changes added in Kernel PCI driver to support this. 
>> 1. Linux PCI driver assumes, the PCI Link is initialized in u-boot. So
>> it just don't do re-initialization of the SerDes link. It checks the
>> link status and continue initialize the PCI-Keystone RC driver.
>> 2. Uses pci=firmware to make sure the memory BARs are not re-assigned.
>>
>> Without that, the Linux kernel PCI driver got some issues and I don't
>> have the details now as this happened almost 2 years ago.
> 
> Ok, it is not a mainline problem then and it is not something we have
> to fix, other than removing that bootarg flag as soon as possible
> since as you describe here it can be (ab/mis)used.
> 
>> I believe at least on K2E, this is a use case that we need to support.
>> K2E board has a Marvel SATA controller with a SATA port that we can hook
>> up a hard drive. I have a TODO item to bring up PCI and SATA on u-boot
>> and boot up Linux the same way as described above. So how this use case
>> is expected to work on ARM? I assume arch/arm/kernel/bios32.c is a
>> pseudo bios driver to emulate BIOS specific handling PCI initialization.
> 
> PCI bios is a set of helper functions that are there as PCI arm arch
> back-end and to be honest I am not entirely sure I understand what you
> expect it to do, it certainly does not emulate any BIOS to the best
> of my knowledge.
> 
>> I am sure this is a use case that needs to be supported. Aren't there
>> other ARM based boards that uses SATA hard disks in u-boot to boot to
>> Linux? 
> 
> First off, PCI_PROBE_ONLY prevents reassigning resources and
> reprogramming PCIe settings for the whole PCI(e) hierarchy managed
> by all host controllers, it is not about one single device (and I
> do not even understand what's the problem you are facing), this
> for the records so that we are on the same page (maybe that's
> the only device in that system, but I thought I should mention
> that).
> 
>> How the above use case is handled if you remove the PCI_PROBE_ONLY support?
> 
> U-boot loads and boots the kernel in memory, quiesces the PCIe SATA device
> before kernel handover and the kernel carries out PCI enumeration, loads and
> probes the required device drivers.
> 

I am with you on this. Will re-visit if I ever has to support PCI in u-boot
to work with SATA.

> Can you define precisely what the problem is please ? In particular
> I want to understand why, by setting the PCI_PROBE_ONLY flag you want
> to make sure your PCIe SATA device BARs are not reassigned.
> 

No idea. As I have said, the customer did this in their custom board.

> Now, the question related to FW/OS handover of PCI resources is a
> complicated one and on ARM it was never solved/defined properly.
> 
> The question here, and I am not sure it can be answered with a simple
> answer, is how we make sure the OS does not touch PCI resources that
> have been initialized in FW and are not meant to be changed (whatever
> that means).
> 
> To at least have some hope to sort this out, the kernel must try to
> claim PCI resources as set-up by FW, if they are in a "reasonable" state
> they are claimed and not touched. For resources for which claiming
> fails, a reassignment policy must be implemented, that might require
> re-sizing bridges and release resources so that they can be effectively
> reallocated.
> 
> At present, on ALL ARM platforms out there (except for some oddballs
> and possibly some arm platforms that abused the pci=firmware parameter that
> I am hunting down and I am not aware of) the whole PCI resource space is
> always reassigned by the kernel (which does not mean that's always the
> right thing to do BTW, as it is it just works).
> 
> We must not try to "fix" this by misusing boot arguments and PCI_PROBE_ONLY,
> we must come up with a PCI resources allocation policy that implements what
> I mention above, my point is, PCI_PROBE_ONLY must mean the same thing
> for all ARM/ARM64 platforms otherwise it becomes complicated to handle in
> a sane way.
> 

Sounds good. Personally, I have never understood the usage of pci=firmware
or PCI_PROBE_ONLY. As you have said, ideally SATA over PCI should work fine
in u-boot and then again in the linux space like any other device drivers
do. Will re-visit when I have to support this use case again on our board
with latest kernel.

Murali

> Does this help ? Please let us know.
> 
> Thanks,
> Lorenzo
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: PCI IO resource question.

[Lorenzo Pieralisi]

On Tue, Mar 22, 2016 at 03:41:59PM -0400, Murali Karicheri wrote:
> On 03/21/2016 02:02 PM, Lorenzo Pieralisi wrote:
> > On Mon, Mar 21, 2016 at 11:24:26AM -0400, Murali Karicheri wrote:
> > 
> > [...]
> > 
> >>>> 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.
> >>>
> >>> Please, tell us more about it, what does "would like to honour" mean ?
> >>>
> >>> Do they (ab)use pci=firmware bootarg just to keep FW PCIe SATA settings ?
> >>>
> >>> There are two ways of setting PCI_PROBE_ONLY on ARM/ARM64 (unfortunately):
> >>>
> >>> - DT chosen node
> >>> - pci=firmware (on arm 32-bit platforms only)
> >>>
> >> second one. pci=firmware.
> > 
> > And that's an abuse that has to be stopped, I am removing that flag
> > straight away for this precise reason.
> > 
> Let me speak up again. Keystone usage of pci=firmware was used by our
> customer to address a specific problem. I am assuming they used it 
> work around the BAR re-assignment problem in Linux space in v3.13 kernel
> they were working with. 
> 
> The kernel-parameters.txt states
> 
>                 firmware        [ARM] Do not re-enumerate the bus but instead
>                                 just use the configuration from the
>                                 bootloader. This is currently used on
>                                 IXP2000 systems where the bus has to be
> 
> Probably they used it to avoid re-enumerate similar to IXP2000. But as I
> said before, I can't confirm the reason as this happened almost 2 years ago.

Ok.

> >>> PCIe designware does not check the DT chosen node property, so you are
> >>> telling me that some customers are abusing pci=firmware command line, that
> >>> was never meant to be used on platforms other than ARM IXP2000 systems
> >>> (see Documentation/kernel-parameters.txt).
> 
> If the problem is same why you blame using this approach on non IXP2000 system?

The problem with PCI_PROBE_ONLY on ARM/ARM64 is that is not
well-defined, so I do not want people to use it to paper over
issues or to put it differently as a workaround that means
different things on different platforms.

PCI resources management must be managed at architectural level,
not with a flag that means different things on different platforms.

I understand your point, I hope I made mine clear, *currently* we
still have no clear definition of what PCI_PROBE_ONLY means on ARM,
and, as you experienced, by setting the bootarg people may already
abuse it and we are stuck with it forever.

Let's wipe the slate clean, remove the flag and if we need it
let's define its usage properly (along with FW bindings to set it),
in a documented way.

> 
> >>>
> >>> Usage of PCI_PROBE_ONLY on ARM has been controversial and is completely
> >>> ill-defined, so we are trying to get rid of its usage.
> 
> No issues. At this point, I don't have the use case I described
> working in our internal TI kernel based on v4.1.x or v4.4.x or
> upstream. However it will be interesting to find the ARM platforms out
> there that uses firmware option, irrespective of whether they abused
> its usage or not. Someone hopefully respond to your patches if it
> breaks these platforms.

I hope it has not been used as such, unfortunately removing is the
only way to find out.

> >>> Now, commit ed00c83cd490 ("PCI: designware: Remove PCI_PROBE_ONLY handling")
> >>> removed, after mailing list initial investigation and patch review
> >>>
> >>> http://www.spinics.net/lists/linux-pci/msg48248.html
> >>>
> >>> PCI_PROBE_ONLY handling from PCIe designware, which means that even if
> >>> you pass pci=firmware parameter on the command line (wrongly, see above)
> >>> the kernel reassigns resources and set-up PCIe settings.
> >>>
> >>> Does this trigger issues on Keystone ? Or the systems that set that
> >>> bootarg work even if the flag is ignored ? I want to understand and
> >>> I really would like to avoid asking Bjorn to revert that commit for
> >>> what looks like an abuse, I prefer finding a workaround if this is
> >>> really an issue.
> 
> As I have said, I don't have the use case with pci=firmware working to
> help you in this.

Great.

> >> Unfortunately the customer uses an older version of kernel v3.13. The
> >> Ubuntu Linux distro runs on Keystone based board using this version of
> >> kernel. We have internal version of the PCI-keystone driver that runs on
> >> this kernel. The requirement was like this at a high level. The U-Boot
> >> requires the SATA hard drive to be initialized to load and run images from
> >> hard disk. When Linux boots up, the hard drive file system is used as
> >> rootfs.
> >>
> >> So few changes added in Kernel PCI driver to support this. 
> >> 1. Linux PCI driver assumes, the PCI Link is initialized in u-boot. So
> >> it just don't do re-initialization of the SerDes link. It checks the
> >> link status and continue initialize the PCI-Keystone RC driver.
> >> 2. Uses pci=firmware to make sure the memory BARs are not re-assigned.
> >>
> >> Without that, the Linux kernel PCI driver got some issues and I don't
> >> have the details now as this happened almost 2 years ago.
> > 
> > Ok, it is not a mainline problem then and it is not something we have
> > to fix, other than removing that bootarg flag as soon as possible
> > since as you describe here it can be (ab/mis)used.
> > 
> >> I believe at least on K2E, this is a use case that we need to support.
> >> K2E board has a Marvel SATA controller with a SATA port that we can hook
> >> up a hard drive. I have a TODO item to bring up PCI and SATA on u-boot
> >> and boot up Linux the same way as described above. So how this use case
> >> is expected to work on ARM? I assume arch/arm/kernel/bios32.c is a
> >> pseudo bios driver to emulate BIOS specific handling PCI initialization.
> > 
> > PCI bios is a set of helper functions that are there as PCI arm arch
> > back-end and to be honest I am not entirely sure I understand what you
> > expect it to do, it certainly does not emulate any BIOS to the best
> > of my knowledge.
> > 
> >> I am sure this is a use case that needs to be supported. Aren't there
> >> other ARM based boards that uses SATA hard disks in u-boot to boot to
> >> Linux? 
> > 
> > First off, PCI_PROBE_ONLY prevents reassigning resources and
> > reprogramming PCIe settings for the whole PCI(e) hierarchy managed
> > by all host controllers, it is not about one single device (and I
> > do not even understand what's the problem you are facing), this
> > for the records so that we are on the same page (maybe that's
> > the only device in that system, but I thought I should mention
> > that).
> > 
> >> How the above use case is handled if you remove the PCI_PROBE_ONLY support?
> > 
> > U-boot loads and boots the kernel in memory, quiesces the PCIe SATA device
> > before kernel handover and the kernel carries out PCI enumeration, loads and
> > probes the required device drivers.
> > 
> 
> I am with you on this. Will re-visit if I ever has to support PCI in u-boot
> to work with SATA.

Sounds good.

> > Can you define precisely what the problem is please ? In particular
> > I want to understand why, by setting the PCI_PROBE_ONLY flag you want
> > to make sure your PCIe SATA device BARs are not reassigned.
> > 
> 
> No idea. As I have said, the customer did this in their custom board.

See above.

> > Now, the question related to FW/OS handover of PCI resources is a
> > complicated one and on ARM it was never solved/defined properly.
> > 
> > The question here, and I am not sure it can be answered with a simple
> > answer, is how we make sure the OS does not touch PCI resources that
> > have been initialized in FW and are not meant to be changed (whatever
> > that means).
> > 
> > To at least have some hope to sort this out, the kernel must try to
> > claim PCI resources as set-up by FW, if they are in a "reasonable" state
> > they are claimed and not touched. For resources for which claiming
> > fails, a reassignment policy must be implemented, that might require
> > re-sizing bridges and release resources so that they can be effectively
> > reallocated.
> > 
> > At present, on ALL ARM platforms out there (except for some oddballs
> > and possibly some arm platforms that abused the pci=firmware parameter that
> > I am hunting down and I am not aware of) the whole PCI resource space is
> > always reassigned by the kernel (which does not mean that's always the
> > right thing to do BTW, as it is it just works).
> > 
> > We must not try to "fix" this by misusing boot arguments and PCI_PROBE_ONLY,
> > we must come up with a PCI resources allocation policy that implements what
> > I mention above, my point is, PCI_PROBE_ONLY must mean the same thing
> > for all ARM/ARM64 platforms otherwise it becomes complicated to handle in
> > a sane way.
> > 
> 
> Sounds good. Personally, I have never understood the usage of pci=firmware
> or PCI_PROBE_ONLY. As you have said, ideally SATA over PCI should work fine
> in u-boot and then again in the linux space like any other device drivers
> do. Will re-visit when I have to support this use case again on our board
> with latest kernel.

That sounds like a plan, thank you.

Lorenzo

Re: PCI IO resource question.

[Lorenzo Pieralisi]

On Wed, Mar 16, 2016 at 12:20:08PM -0400, Murali Karicheri wrote:

[...]

> However I am wondering if it is mandatory to define this range at all.
> I tried to remove the entry and I get following error then

If your PCI host controller does not support IO space the DT must not
contain a range for it, can you elaborate on "dummy IO range" please ?

If you have it in DT, the host controller driver uses it to map that
range into virtual address space, depending on which CPU physical
address you pass into the IO range this may be go totally wild
with drivers poking at random (ie your dummy range) CPU physical
addresses in the worst case.

Remove the IO range if the PCI host controller does not support it,
you must not add one to make the kernel happy, that's potentially
a very dangerous thing to do.

> [    0.448772] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
> [    0.448783] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
> [    0.448792] pci 0000:00:00.0: BAR 7: no space for [io  size 0x1000]
> [    0.448800] pci 0000:00:00.0: BAR 7: failed to assign [io  size 0x1000]
> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
> 
> 
> The original log is below and even with the error, I am able to have SATA
> drive function as expected over this PCIe interface.
> 
> 
> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
> [    0.420685] Requested IO range too big, new size set to 64K
> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]

This unearthed an issue that we've got to fix. If the IO space remap
fails (pci_remap_iospace()) we must remove the IO resource from the host
bridge resources list, code below shows that we end up assigning IO space
even if the remapping of the host bridge CPU physical address corresponding
to IO space fails, which is wrong.

Do you want me to fix the host bridges concerned in one go ?

Thanks,
Lorenzo

> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
> [    0.431879] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
> [    0.431906] pci 0000:00:00.0: [104c:b009] type 01 class 0x060400
> [    0.432173] PCI: bus0: Fast back to back transfers disabled
> [    0.432334] pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
> [    0.432382] pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
> [    0.432402] pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
> [    0.432420] pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
> [    0.432439] pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
> [    0.432457] pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
> [    0.432476] pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
> [    0.432495] pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
> [    0.432561] pci 0000:01:00.0: PME# supported from D3hot
> [    0.448635] PCI: bus1: Fast back to back transfers disabled
> [    0.448717] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
> [    0.448729] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
> [    0.448738] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
> [    0.448751] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
> [    0.448760] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
> [    0.448772] pci 0000:01:00.0: BAR 4: assigned [io  0x1000-0x100f]
> [    0.448784] pci 0000:01:00.0: BAR 0: assigned [io  0x1010-0x1017]
> [    0.448796] pci 0000:01:00.0: BAR 2: assigned [io  0x1018-0x101f]
> [    0.448808] pci 0000:01:00.0: BAR 1: assigned [io  0x1020-0x1023]
> [    0.448820] pci 0000:01:00.0: BAR 3: assigned [io  0x1024-0x1027]
> [    0.448833] pci 0000:00:00.0: PCI bridge to [bus 01]
> [    0.448842] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
> [    0.448851] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
> [    0.448860] pci 0000:00:00.0:   bridge window [mem 0x60100000-0x601fffff pref]
> [    0.449077] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
> [    0.449086] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
> [    0.449095] pcie_pme 0000:00:00.0:pcie01: service driver pcie_pme loaded
> [    0.449240] aer 0000:00:00.0:pcie02: service driver aer loaded
> 
> 
> Thanks and regards,
> 
> -- 
> Murali Karicheri
> Linux Kernel, Keystone
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: PCI IO resource question.

[Bjorn Helgaas]

On Wed, Mar 16, 2016 at 06:09:36PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Mar 16, 2016 at 12:20:08PM -0400, Murali Karicheri wrote:

> > [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
> > [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
> > [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
> > [    0.420685] Requested IO range too big, new size set to 64K
> > [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
> > [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
> 
> This unearthed an issue that we've got to fix. If the IO space remap
> fails (pci_remap_iospace()) we must remove the IO resource from the host
> bridge resources list, code below shows that we end up assigning IO space
> even if the remapping of the host bridge CPU physical address corresponding
> to IO space fails, which is wrong.
> 
> Do you want me to fix the host bridges concerned in one go ?

Yes, that looks like a real problem we should fix.

Bjorn

Re: PCI IO resource question.

[Murali Karicheri]

On 03/16/2016 02:09 PM, Lorenzo Pieralisi wrote:
> On Wed, Mar 16, 2016 at 12:20:08PM -0400, Murali Karicheri wrote:
> 
> [...]
> 
>> However I am wondering if it is mandatory to define this range at all.
>> I tried to remove the entry and I get following error then
> 
> If your PCI host controller does not support IO space the DT must not
> contain a range for it, can you elaborate on "dummy IO range" please ?
> 
> If you have it in DT, the host controller driver uses it to map that
> range into virtual address space, depending on which CPU physical
> address you pass into the IO range this may be go totally wild
> with drivers poking at random (ie your dummy range) CPU physical
> addresses in the worst case.
> 
> Remove the IO range if the PCI host controller does not support it,
> you must not add one to make the kernel happy, that's potentially
> a very dangerous thing to do.
> 
Yes, that is the right thing to do. I will be posting a patch
for this.

>> [    0.448772] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
>> [    0.448783] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
>> [    0.448792] pci 0000:00:00.0: BAR 7: no space for [io  size 0x1000]
>> [    0.448800] pci 0000:00:00.0: BAR 7: failed to assign [io  size 0x1000]
>> [    0.448813] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>> [    0.448822] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>> [    0.448834] pci 0000:01:00.0: BAR 4: no space for [io  size 0x0010]
>> [    0.448841] pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
>> [    0.448848] pci 0000:01:00.0: BAR 0: no space for [io  size 0x0008]
>> [    0.448855] pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
>> [    0.448863] pci 0000:01:00.0: BAR 2: no space for [io  size 0x0008]
>> [    0.448870] pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
>> [    0.448877] pci 0000:01:00.0: BAR 1: no space for [io  size 0x0004]
>> [    0.448884] pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
>> [    0.448891] pci 0000:01:00.0: BAR 3: no space for [io  size 0x0004]
>> [    0.448898] pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
>> [    0.448907] pci 0000:00:00.0: PCI bridge to [bus 01]
>>
>>
>> The original log is below and even with the error, I am able to have SATA
>> drive function as expected over this PCIe interface.
>>
>>
>> [    0.420648] PCI host bridge /soc/pcie@21020000 ranges:
>> [    0.420659]   No bus range found for /soc/pcie@21020000, using [bus 00-ff]
>> [    0.420679]    IO 0x23260000..0x400023263fff -> 0x00000000
>> [    0.420685] Requested IO range too big, new size set to 64K
>> [    0.420702]   MEM 0x60000000..0x6fffffff -> 0x60000000
>> [    0.420713] keystone-pcie 21021000.pcie: error -22: failed to map resource [io  0x0000-0x400000003fff]
> 
> This unearthed an issue that we've got to fix. If the IO space remap
> fails (pci_remap_iospace()) we must remove the IO resource from the host
> bridge resources list, code below shows that we end up assigning IO space
> even if the remapping of the host bridge CPU physical address corresponding
> to IO space fails, which is wrong.
> 
> Do you want me to fix the host bridges concerned in one go ?

I can test your patch on Keystone and provide you the result if that helps.

Murali
> 
> Thanks,
> Lorenzo
> 
>> [    0.431849] keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
>> [    0.431861] pci_bus 0000:00: root bus resource [bus 00-ff]
>> [    0.431870] pci_bus 0000:00: root bus resource [io  0x0000-0x400000003fff]
>> [    0.431879] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
>> [    0.431906] pci 0000:00:00.0: [104c:b009] type 01 class 0x060400
>> [    0.432173] PCI: bus0: Fast back to back transfers disabled
>> [    0.432334] pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
>> [    0.432382] pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
>> [    0.432402] pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
>> [    0.432420] pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
>> [    0.432439] pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
>> [    0.432457] pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
>> [    0.432476] pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
>> [    0.432495] pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]
>> [    0.432561] pci 0000:01:00.0: PME# supported from D3hot
>> [    0.448635] PCI: bus1: Fast back to back transfers disabled
>> [    0.448717] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
>> [    0.448729] pci 0000:00:00.0: BAR 9: assigned [mem 0x60100000-0x601fffff pref]
>> [    0.448738] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
>> [    0.448751] pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
>> [    0.448760] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
>> [    0.448772] pci 0000:01:00.0: BAR 4: assigned [io  0x1000-0x100f]
>> [    0.448784] pci 0000:01:00.0: BAR 0: assigned [io  0x1010-0x1017]
>> [    0.448796] pci 0000:01:00.0: BAR 2: assigned [io  0x1018-0x101f]
>> [    0.448808] pci 0000:01:00.0: BAR 1: assigned [io  0x1020-0x1023]
>> [    0.448820] pci 0000:01:00.0: BAR 3: assigned [io  0x1024-0x1027]
>> [    0.448833] pci 0000:00:00.0: PCI bridge to [bus 01]
>> [    0.448842] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
>> [    0.448851] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
>> [    0.448860] pci 0000:00:00.0:   bridge window [mem 0x60100000-0x601fffff pref]
>> [    0.449077] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
>> [    0.449086] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
>> [    0.449095] pcie_pme 0000:00:00.0:pcie01: service driver pcie_pme loaded
>> [    0.449240] aer 0000:00:00.0:pcie02: service driver aer loaded
>>
>>
>> Thanks and regards,
>>
>> -- 
>> Murali Karicheri
>> Linux Kernel, Keystone
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>


-- 
Murali Karicheri
Linux Kernel, Keystone