[RFC] pciehp: Add archdata setting

[RFC] pciehp: Add archdata setting

[松本博郎]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

Hi


I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug
Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp.
e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
<snip>
[   65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[   65.508555] pcieport 0000:02:01.0:   bridge window [io
0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem
0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform
[   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
[   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip>  // There is no network interface for 82572EI

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
without archdata.
So e1000e driver doesn't return error.

I think that archdata should be set before driver probe (e.g pciehp).
I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
driver..., so please give better way.


Regards.

Hiroo MATSUMOTO (FUJITSU COMPUTER TECHNOLOGIES LIMITED)
matsumoto.hiroo@jp.fujitsu.com



[-- Attachment #2: pciehp-add-archdata.patch --]
[-- Type: text/plain, Size: 475 bytes --]

Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index a4031df..a021fbc 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
 			pciehp_add_bridge(dev);
 		}
 		pci_dev_put(dev);
+		dev->dev.archdata = bridge->dev.archdata;
 	}
 
 	pci_assign_unassigned_bridge_resources(bridge);

Re: [RFC] pciehp: Add archdata setting

[Bjorn Helgaas]

On Mon, Apr 2, 2012 at 1:42 AM, 松本博郎 <matsumoto.hiroo@jp.fujitsu.com> wrote:
> I'm trying to use pciehp on powerpc platform.
> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
> Capability.
>
> There is problem when poweron PCI Express HotPlug slot with pciehp.
> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
> set (dma_ops is NULL) when probe from pciehp.
> Then e1000e driver returns error as below.
>
> -sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> [   65.493662] pci 0000:03:00.0: BAR 2: set to [io
> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
> [   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
> [   65.508555] pcieport 0000:02:01.0:   bridge window [io
> 0xff7ee000-0xff7eefff]
> [   65.515785] pcieport 0000:02:01.0:   bridge window [mem
> 0xa0100000-0xa01fffff]
> [   65.523015] pcieport 0000:02:01.0:   bridge window [mem
> 0xa0200000-0xa02fffff 64bit pref]
> [   65.531238] pci 0000:03:00.0: no hotplug settings from platform
> [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
> [   65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
> [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
> -sh-3.2# lspci
> <snip>
> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
> Ethernet Controller (Copper) (rev 06)
> <snip>
> -sh-3.2# ifconfig -a
> <snip>  // There is no network interface for 82572EI
>
> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
> without archdata.
> So e1000e driver doesn't return error.
>
> I think that archdata should be set before driver probe (e.g pciehp).
> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
> driver..., so please give better way.

Interesting.  It would be nice if we could set archdata the same way
for both devices present at boot and those hot-added later.  Can you
figure out where archdata is set for devices present at boot?

Bjorn

> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index a4031df..a021fbc 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
>                        pciehp_add_bridge(dev);
>                }
>                pci_dev_put(dev);
> +               dev->dev.archdata = bridge->dev.archdata;
>        }
>
>        pci_assign_unassigned_bridge_resources(bridge);
>

Re: [RFC] pciehp: Add archdata setting

[松本博郎]

Hi


Thanks for your comment.

I have mistake.
archdata.dma_ops is always NULL on x86 platform.
On x86 platform, probe with pciehp is same as probe with initcall.
I'm so sorry.

But archdata on powerpc platform is set with initcall.
Call stack is as below.
   1. pcibios_init
   2. pcibios_scan_phb
   3. pci_scan_child_bus
   4. pci_scan_bridge
   5. pci_scan_child_bus
   6. pci_scan_bridge
   7. pci_scan_child_bus
   8. pcibios_setup_bus_devices

In pcibios_setup_bus_devices, archdata is set with set_dma_ops and 
set_dma_offset.
But set_dma_ops and set_dma_offset are defined only at powerpc and 
microblaze (Why...).

Can architecture code, like arch/powerpc/kernel/pci.c, be called from 
pciehp ?


Regards.

Hiroo MATSUMOTO (FUJITSU COMPUTER TECHNOLOGIES LIMITED)
matsumoto.hiroo@jp.fujitsu.com

>> I'm trying to use pciehp on powerpc platform.
>> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
>> Capability.
>>
>> There is problem when poweron PCI Express HotPlug slot with pciehp.
>> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
>> set (dma_ops is NULL) when probe from pciehp.
>> Then e1000e driver returns error as below.
>>
>> -sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
>> <snip>
>> [   65.493662] pci 0000:03:00.0: BAR 2: set to [io
>> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
>> [   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>> [   65.508555] pcieport 0000:02:01.0:   bridge window [io
>> 0xff7ee000-0xff7eefff]
>> [   65.515785] pcieport 0000:02:01.0:   bridge window [mem
>> 0xa0100000-0xa01fffff]
>> [   65.523015] pcieport 0000:02:01.0:   bridge window [mem
>> 0xa0200000-0xa02fffff 64bit pref]
>> [   65.531238] pci 0000:03:00.0: no hotplug settings from platform
>> [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
>> [   65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
>> [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
>> -sh-3.2# lspci
>> <snip>
>> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
>> Ethernet Controller (Copper) (rev 06)
>> <snip>
>> -sh-3.2# ifconfig -a
>> <snip>  // There is no network interface for 82572EI
>>
>> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
>> without archdata.
>> So e1000e driver doesn't return error.
>>
>> I think that archdata should be set before driver probe (e.g pciehp).
>> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
>> driver..., so please give better way.
> 
> Interesting.  It would be nice if we could set archdata the same way
> for both devices present at boot and those hot-added later.  Can you
> figure out where archdata is set for devices present at boot?
> 
> Bjorn
> 
>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
>> index a4031df..a021fbc 100644
>> --- a/drivers/pci/hotplug/pciehp_pci.c
>> +++ b/drivers/pci/hotplug/pciehp_pci.c
>> @@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
>>                        pciehp_add_bridge(dev);
>>                }
>>                pci_dev_put(dev);
>> +               dev->dev.archdata = bridge->dev.archdata;
>>        }
>>
>>        pci_assign_unassigned_bridge_resources(bridge);
>>
> 

Re: [RFC] pciehp: Add archdata setting

[Kenji Kaneshige]

(2012/04/02 23:13), Bjorn Helgaas wrote:
> On Mon, Apr 2, 2012 at 1:42 AM, 松本博郎<matsumoto.hiroo@jp.fujitsu.com>  wrote:
>> I'm trying to use pciehp on powerpc platform.
>> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
>> Capability.
>>
>> There is problem when poweron PCI Express HotPlug slot with pciehp.
>> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
>> set (dma_ops is NULL) when probe from pciehp.
>> Then e1000e driver returns error as below.
>>
>> -sh-3.2# echo 1>  /sys/bus/pci/slots/1/power
>> <snip>
>> [   65.493662] pci 0000:03:00.0: BAR 2: set to [io
>> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
>> [   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>> [   65.508555] pcieport 0000:02:01.0:   bridge window [io
>> 0xff7ee000-0xff7eefff]
>> [   65.515785] pcieport 0000:02:01.0:   bridge window [mem
>> 0xa0100000-0xa01fffff]
>> [   65.523015] pcieport 0000:02:01.0:   bridge window [mem
>> 0xa0200000-0xa02fffff 64bit pref]
>> [   65.531238] pci 0000:03:00.0: no hotplug settings from platform
>> [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
>> [   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
>> [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
>> [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
>> -sh-3.2# lspci
>> <snip>
>> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
>> Ethernet Controller (Copper) (rev 06)
>> <snip>
>> -sh-3.2# ifconfig -a
>> <snip>    // There is no network interface for 82572EI
>>
>> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
>> without archdata.
>> So e1000e driver doesn't return error.
>>
>> I think that archdata should be set before driver probe (e.g pciehp).
>> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
>> driver..., so please give better way.
>
> Interesting.  It would be nice if we could set archdata the same way
> for both devices present at boot and those hot-added later.

Yes. But it might be not so easy...

>                                                              Can you
> figure out where archdata is set for devices present at boot?

I read some x86 and powerpc code to setup archdata. x86 and powerpc
have different way to setup archdata. And it seems archdata is not
set in one location, each member in the archdata is setup in the
different locations.

On x86, archdata.dma_ops is set in the pci_iommu_init() code path, which
is called through rootfs_initcall(). On powerpc, archdata.dma_ops is set
in pcibios_fixup_bus() code path.

Here are my notes.

x86 (calgary iommu)
===================
rootfs_initcall()
  => pci_iommu_init()
      => x86_init.iommu.iommu_init() (== calgary_iommu_init())
          => calgary_init()

calgary_init()
{
         for_each_pci_dev() {
                         dev->dev.archdata.dma_ops = &calgary_dma_ops;
         }
}

x86 (amd iommu)
===============
rootfs_initcall()
  => pci_iommu_init()
      => x86_init.iommu.iommu_init() (== amd_iommu_init())
          => amd_iommu_init_dma_ops()
              => device_dma_ops_init()

device_dma_ops_init()
{
	for_each_pci_dev() {
		pdev->dev.archdata.dma_ops = &amd_iommu_dma_ops;
}

PowerPC
=======
pci_scan_child_bus()
  => pcibios_fixup_bus()
      => pcibios_setup_bus_devices()
          => set_dma_ops()              # for each device on the bus (default)
          => ppc_md.pci_dma_dev_setup() # for each device on the bus (machine depend)

static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
{
         dev->archdata.dma_ops = ops;
}


One possible idea is that to setup archdata in the pcibios_xxx() hook.
(for example, use existing pcibios_enable_device(), or provide new
hook like pcibios_setup_device() which is called by pci_setup_device).

Another idea is that to add support for hot-plug to each iommu (dma)
code. Maybe it can be implemented by using bus notifier.

Regards,
Kenji Kaneshige

Re: [RFC] pciehp: Add archdata setting

[Kenji Kaneshige]

(2012/04/03 15:26), 松本博郎 wrote:
> Hi
> 
> 
> Thanks for your comment.
> 
> I have mistake.
> archdata.dma_ops is always NULL on x86 platform.
> On x86 platform, probe with pciehp is same as probe with initcall.
> I'm so sorry.
> 
> But archdata on powerpc platform is set with initcall.
> Call stack is as below.
> 1. pcibios_init
> 2. pcibios_scan_phb
> 3. pci_scan_child_bus
> 4. pci_scan_bridge
> 5. pci_scan_child_bus
> 6. pci_scan_bridge
> 7. pci_scan_child_bus
> 8. pcibios_setup_bus_devices
> 
> In pcibios_setup_bus_devices, archdata is set with set_dma_ops and set_dma_offset.
> But set_dma_ops and set_dma_offset are defined only at powerpc and microblaze (Why...).
> 
> Can architecture code, like arch/powerpc/kernel/pci.c, be called from pciehp ?

Hello Hiroo,

Pcidhp can call the arch specific code, but I don't think it's a good idea.
It's not pciehp specific problem. So it should be done in more generic way.
As Bjorn said, it would be nice if we could use the same code for both
devices present at boot and hot-added devices.

Regards,
Kenji Kaneshige


> 
> 
> Regards.
> 
> Hiroo MATSUMOTO (FUJITSU COMPUTER TECHNOLOGIES LIMITED)
> matsumoto.hiroo@jp.fujitsu.com
> 
>>> I'm trying to use pciehp on powerpc platform.
>>> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
>>> Capability.
>>>
>>> There is problem when poweron PCI Express HotPlug slot with pciehp.
>>> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
>>> set (dma_ops is NULL) when probe from pciehp.
>>> Then e1000e driver returns error as below.
>>>
>>> -sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> [ 65.493662] pci 0000:03:00.0: BAR 2: set to [io
>>> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
>>> [ 65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>>> [ 65.508555] pcieport 0000:02:01.0: bridge window [io
>>> 0xff7ee000-0xff7eefff]
>>> [ 65.515785] pcieport 0000:02:01.0: bridge window [mem
>>> 0xa0100000-0xa01fffff]
>>> [ 65.523015] pcieport 0000:02:01.0: bridge window [mem
>>> 0xa0200000-0xa02fffff 64bit pref]
>>> [ 65.531238] pci 0000:03:00.0: no hotplug settings from platform
>>> [ 65.538361] e1000e 0000:03:00.0: Disabling ASPM L1
>>> [ 65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> [ 65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
>>> [ 65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
>>> -sh-3.2# lspci
>>> <snip>
>>> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
>>> Ethernet Controller (Copper) (rev 06)
>>> <snip>
>>> -sh-3.2# ifconfig -a
>>> <snip> // There is no network interface for 82572EI
>>>
>>> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
>>> without archdata.
>>> So e1000e driver doesn't return error.
>>>
>>> I think that archdata should be set before driver probe (e.g pciehp).
>>> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
>>> driver..., so please give better way.
>>
>> Interesting. It would be nice if we could set archdata the same way
>> for both devices present at boot and those hot-added later. Can you
>> figure out where archdata is set for devices present at boot?
>>
>> Bjorn
>>
>>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>>> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
>>> index a4031df..a021fbc 100644
>>> --- a/drivers/pci/hotplug/pciehp_pci.c
>>> +++ b/drivers/pci/hotplug/pciehp_pci.c
>>> @@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
>>> pciehp_add_bridge(dev);
>>> }
>>> pci_dev_put(dev);
>>> + dev->dev.archdata = bridge->dev.archdata;
>>> }
>>>
>>> pci_assign_unassigned_bridge_resources(bridge);
>>>
>>
> 
> 

Re: [RFC] pciehp: Add archdata setting

[松本博郎]

Hi, Bjorn and Kaneshige


Thanks for your comments and proposal.
I will rethink a way of setting archdata with pcibios_xxx() hook or bus 
notifier.


Regards.

Hiroo MATSUMOTO

>>> I'm trying to use pciehp on powerpc platform.
>>> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
>>> Capability.
>>>
>>> There is problem when poweron PCI Express HotPlug slot with pciehp.
>>> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
>>> set (dma_ops is NULL) when probe from pciehp.
>>> Then e1000e driver returns error as below.
>>>
>>> -sh-3.2# echo 1>  /sys/bus/pci/slots/1/power
>>> <snip>
>>> [   65.493662] pci 0000:03:00.0: BAR 2: set to [io
>>> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
>>> [   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>>> [   65.508555] pcieport 0000:02:01.0:   bridge window [io
>>> 0xff7ee000-0xff7eefff]
>>> [   65.515785] pcieport 0000:02:01.0:   bridge window [mem
>>> 0xa0100000-0xa01fffff]
>>> [   65.523015] pcieport 0000:02:01.0:   bridge window [mem
>>> 0xa0200000-0xa02fffff 64bit pref]
>>> [   65.531238] pci 0000:03:00.0: no hotplug settings from platform
>>> [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
>>> [   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
>>> [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, 
>>> aborting
>>> [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
>>> -sh-3.2# lspci
>>> <snip>
>>> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
>>> Ethernet Controller (Copper) (rev 06)
>>> <snip>
>>> -sh-3.2# ifconfig -a
>>> <snip>    // There is no network interface for 82572EI
>>>
>>> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
>>> without archdata.
>>> So e1000e driver doesn't return error.
>>>
>>> I think that archdata should be set before driver probe (e.g pciehp).
>>> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
>>> driver..., so please give better way.
>>
>> Interesting.  It would be nice if we could set archdata the same way
>> for both devices present at boot and those hot-added later.
> 
> Yes. But it might be not so easy...
> 
>>                                                              Can you
>> figure out where archdata is set for devices present at boot?
> 
> I read some x86 and powerpc code to setup archdata. x86 and powerpc
> have different way to setup archdata. And it seems archdata is not
> set in one location, each member in the archdata is setup in the
> different locations.
> 
> On x86, archdata.dma_ops is set in the pci_iommu_init() code path, which
> is called through rootfs_initcall(). On powerpc, archdata.dma_ops is set
> in pcibios_fixup_bus() code path.
> 
> Here are my notes.
> 
> x86 (calgary iommu)
> ===================
> rootfs_initcall()
>  => pci_iommu_init()
>      => x86_init.iommu.iommu_init() (== calgary_iommu_init())
>          => calgary_init()
> 
> calgary_init()
> {
>         for_each_pci_dev() {
>                         dev->dev.archdata.dma_ops = &calgary_dma_ops;
>         }
> }
> 
> x86 (amd iommu)
> ===============
> rootfs_initcall()
>  => pci_iommu_init()
>      => x86_init.iommu.iommu_init() (== amd_iommu_init())
>          => amd_iommu_init_dma_ops()
>              => device_dma_ops_init()
> 
> device_dma_ops_init()
> {
>     for_each_pci_dev() {
>         pdev->dev.archdata.dma_ops = &amd_iommu_dma_ops;
> }
> 
> PowerPC
> =======
> pci_scan_child_bus()
>  => pcibios_fixup_bus()
>      => pcibios_setup_bus_devices()
>          => set_dma_ops()              # for each device on the bus 
> (default)
>          => ppc_md.pci_dma_dev_setup() # for each device on the bus 
> (machine depend)
> 
> static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
> {
>         dev->archdata.dma_ops = ops;
> }
> 
> 
> One possible idea is that to setup archdata in the pcibios_xxx() hook.
> (for example, use existing pcibios_enable_device(), or provide new
> hook like pcibios_setup_device() which is called by pci_setup_device).
> 
> Another idea is that to add support for hot-plug to each iommu (dma)
> code. Maybe it can be implemented by using bus notifier.
> 
> Regards,
> Kenji Kaneshige
> 

Re: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

Hi, Kaneshige


You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is 
called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in
arch/powerpc/kernel/pci-common.c.
It works good.

I will send this code to arch/powerpc guys.
Thanks for your great proposal.


Regards.

Hiroo Matsumoto




> Hi, Bjorn and Kaneshige
> 
> 
> Thanks for your comments and proposal.
> I will rethink a way of setting archdata with pcibios_xxx() hook or bus 
> notifier.
> 
> 
> Regards.
> 
> Hiroo MATSUMOTO
> 
>>>> I'm trying to use pciehp on powerpc platform.
>>>> I set e1000e card to PCI Express bridge that has PCI Express HotPlug
>>>> Capability.
>>>>
>>>> There is problem when poweron PCI Express HotPlug slot with pciehp.
>>>> e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't
>>>> set (dma_ops is NULL) when probe from pciehp.
>>>> Then e1000e driver returns error as below.
>>>>
>>>> -sh-3.2# echo 1>  /sys/bus/pci/slots/1/power
>>>> <snip>
>>>> [   65.493662] pci 0000:03:00.0: BAR 2: set to [io
>>>> 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
>>>> [   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>>>> [   65.508555] pcieport 0000:02:01.0:   bridge window [io
>>>> 0xff7ee000-0xff7eefff]
>>>> [   65.515785] pcieport 0000:02:01.0:   bridge window [mem
>>>> 0xa0100000-0xa01fffff]
>>>> [   65.523015] pcieport 0000:02:01.0:   bridge window [mem
>>>> 0xa0200000-0xa02fffff 64bit pref]
>>>> [   65.531238] pci 0000:03:00.0: no hotplug settings from platform
>>>> [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
>>>> [   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
>>>> [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, 
>>>> aborting
>>>> [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5
>>>> -sh-3.2# lspci
>>>> <snip>
>>>> 0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit
>>>> Ethernet Controller (Copper) (rev 06)
>>>> <snip>
>>>> -sh-3.2# ifconfig -a
>>>> <snip>    // There is no network interface for 82572EI
>>>>
>>>> If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops
>>>> without archdata.
>>>> So e1000e driver doesn't return error.
>>>>
>>>> I think that archdata should be set before driver probe (e.g pciehp).
>>>> I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI
>>>> driver..., so please give better way.
>>>
>>> Interesting.  It would be nice if we could set archdata the same way
>>> for both devices present at boot and those hot-added later.
>>
>> Yes. But it might be not so easy...
>>
>>>                                                              Can you
>>> figure out where archdata is set for devices present at boot?
>>
>> I read some x86 and powerpc code to setup archdata. x86 and powerpc
>> have different way to setup archdata. And it seems archdata is not
>> set in one location, each member in the archdata is setup in the
>> different locations.
>>
>> On x86, archdata.dma_ops is set in the pci_iommu_init() code path, which
>> is called through rootfs_initcall(). On powerpc, archdata.dma_ops is set
>> in pcibios_fixup_bus() code path.
>>
>> Here are my notes.
>>
>> x86 (calgary iommu)
>> ===================
>> rootfs_initcall()
>>  => pci_iommu_init()
>>      => x86_init.iommu.iommu_init() (== calgary_iommu_init())
>>          => calgary_init()
>>
>> calgary_init()
>> {
>>         for_each_pci_dev() {
>>                         dev->dev.archdata.dma_ops = &calgary_dma_ops;
>>         }
>> }
>>
>> x86 (amd iommu)
>> ===============
>> rootfs_initcall()
>>  => pci_iommu_init()
>>      => x86_init.iommu.iommu_init() (== amd_iommu_init())
>>          => amd_iommu_init_dma_ops()
>>              => device_dma_ops_init()
>>
>> device_dma_ops_init()
>> {
>>     for_each_pci_dev() {
>>         pdev->dev.archdata.dma_ops = &amd_iommu_dma_ops;
>> }
>>
>> PowerPC
>> =======
>> pci_scan_child_bus()
>>  => pcibios_fixup_bus()
>>      => pcibios_setup_bus_devices()
>>          => set_dma_ops()              # for each device on the bus 
>> (default)
>>          => ppc_md.pci_dma_dev_setup() # for each device on the bus 
>> (machine depend)
>>
>> static inline void set_dma_ops(struct device *dev, struct dma_map_ops 
>> *ops)
>> {
>>         dev->archdata.dma_ops = ops;
>> }
>>
>>
>> One possible idea is that to setup archdata in the pcibios_xxx() hook.
>> (for example, use existing pcibios_enable_device(), or provide new
>> hook like pcibios_setup_device() which is called by pci_setup_device).
>>
>> Another idea is that to add support for hot-plug to each iommu (dma)
>> code. Maybe it can be implemented by using bus notifier.
>>
>> Regards,
>> Kenji Kaneshige
>>
> 

Re: [RFC] pciehp: Add archdata setting

[Bjorn Helgaas]

On Wed, Apr 11, 2012 at 11:00 PM, Hiroo Matsumoto
<matsumoto.hiroo@jp.fujitsu.com> wrote:
> Hi, Kaneshige
>
>
> You are right!
> Setting dma_ops in pcibios_enable_device is a nice way.
> In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is
> called before checking archdata.dma_ops.
>
> I added code of checking and setting dma_ops to pcibios_enable_device in
> arch/powerpc/kernel/pci-common.c.
> It works good.

When I researched this, I thought the best route was to use the
bus_register_notifier() path, as amd_iommu_init_notifier() does.

We're filling in a struct device field, not a PCI field, and
bus_register_notifier() seems like a more generic path than relying on
pcibios_enable_device().

Bjorn

Re: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

Hi, Bjorn


Thanks for your research and comment.

As you said, a way of registering code with bus_register_notifier(), 
which will be called in device_add(), is better one than 
pcibios_enable_device().
I will try to write code with bus_register_notifier().


Regards.

Hiroo MATSUMOTO

> On Wed, Apr 11, 2012 at 11:00 PM, Hiroo Matsumoto
> <matsumoto.hiroo@jp.fujitsu.com> wrote:
>> Hi, Kaneshige
>>
>>
>> You are right!
>> Setting dma_ops in pcibios_enable_device is a nice way.
>> In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is
>> called before checking archdata.dma_ops.
>>
>> I added code of checking and setting dma_ops to pcibios_enable_device in
>> arch/powerpc/kernel/pci-common.c.
>> It works good.
> 
> When I researched this, I thought the best route was to use the
> bus_register_notifier() path, as amd_iommu_init_notifier() does.
> 
> We're filling in a struct device field, not a PCI field, and
> bus_register_notifier() seems like a more generic path than relying on
> pcibios_enable_device().
> 
> Bjorn
> 
> 

Re: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

Hi, Bjorn


I wrote a code with bus_register_notifier().
A function that calls bus_register_notifer() is called from 
rootfs_initcall as well as amd_iommu is.

Please review this.


Regards.

Hiroo MATSUMOTO

> Hi, Bjorn
> 
> 
> Thanks for your research and comment.
> 
> As you said, a way of registering code with bus_register_notifier(), 
> which will be called in device_add(), is better one than 
> pcibios_enable_device().
> I will try to write code with bus_register_notifier().
> 
> 
> Regards.
> 
> Hiroo MATSUMOTO
> 
>> On Wed, Apr 11, 2012 at 11:00 PM, Hiroo Matsumoto
>> <matsumoto.hiroo@jp.fujitsu.com> wrote:
>>> Hi, Kaneshige
>>>
>>>
>>> You are right!
>>> Setting dma_ops in pcibios_enable_device is a nice way.
>>> In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is
>>> called before checking archdata.dma_ops.
>>>
>>> I added code of checking and setting dma_ops to pcibios_enable_device in
>>> arch/powerpc/kernel/pci-common.c.
>>> It works good.
>>
>> When I researched this, I thought the best route was to use the
>> bus_register_notifier() path, as amd_iommu_init_notifier() does.
>>
>> We're filling in a struct device field, not a PCI field, and
>> bus_register_notifier() seems like a more generic path than relying on
>> pcibios_enable_device().
>>
>> Bjorn
>>
>>
> 

[-- Attachment #2: ppc-pcibios-bus_notifier-for-hp.patch --]
[-- Type: text/plain, Size: 2136 bytes --]

Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 32656f1..1fe1e25 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1083,6 +1083,17 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
 		ppc_md.pci_dma_bus_setup(bus);
 }
 
+static inline void pcibios_set_dma_ops(struct pci_dev *dev)
+{
+	/* Hook up default DMA ops */
+	set_dma_ops(&dev->dev, pci_dma_ops);
+	set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+	/* Additional platform DMA/iommu setup */
+	if (ppc_md.pci_dma_dev_setup)
+		ppc_md.pci_dma_dev_setup(dev);
+}
+
 void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
@@ -1102,13 +1113,7 @@ void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
 		 */
 		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
 
-		/* Hook up default DMA ops */
-		set_dma_ops(&dev->dev, pci_dma_ops);
-		set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
-
-		/* Additional platform DMA/iommu setup */
-		if (ppc_md.pci_dma_dev_setup)
-			ppc_md.pci_dma_dev_setup(dev);
+		pcibios_set_dma_ops(dev);
 
 		/* Read default IRQs and fixup if necessary */
 		pci_read_irq_line(dev);
@@ -1749,3 +1754,33 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
+
+static int pcibios_device_change_notifier(struct notifier_block *nb,
+					  unsigned long action, void *data)
+{
+	struct pci_dev *dev = to_pci_dev(data);
+
+	if (!dev)
+		return 0;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (!get_dma_ops(&dev->dev))
+			pcibios_set_dma_ops(dev);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block device_nb = {
+	.notifier_call = pcibios_device_change_notifier,
+};
+
+static int pcibios_bus_notifier_init(void)
+{
+	return bus_register_notifier(&pci_bus_type, &device_nb);
+}
+rootfs_initcall(pcibios_bus_notifier_init);

Re: [RFC] pciehp: Add archdata setting

[Bjorn Helgaas]

On Mon, Apr 16, 2012 at 12:32 AM, Hiroo Matsumoto
<matsumoto.hiroo@jp.fujitsu.com> wrote:
> Hi, Bjorn
>
>
> I wrote a code with bus_register_notifier().
> A function that calls bus_register_notifer() is called from rootfs_initcall
> as well as amd_iommu is.
>
> Please review this.
>
>
> Regards.
>
> Hiroo MATSUMOTO
>
>
>> Hi, Bjorn
>>
>>
>> Thanks for your research and comment.
>>
>> As you said, a way of registering code with bus_register_notifier(), which
>> will be called in device_add(), is better one than pcibios_enable_device().
>> I will try to write code with bus_register_notifier().
>>
>>
>> Regards.
>>
>> Hiroo MATSUMOTO
>>
>>> On Wed, Apr 11, 2012 at 11:00 PM, Hiroo Matsumoto
>>> <matsumoto.hiroo@jp.fujitsu.com> wrote:
>>>>
>>>> Hi, Kaneshige
>>>>
>>>>
>>>> You are right!
>>>> Setting dma_ops in pcibios_enable_device is a nice way.
>>>> In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is
>>>> called before checking archdata.dma_ops.
>>>>
>>>> I added code of checking and setting dma_ops to pcibios_enable_device in
>>>> arch/powerpc/kernel/pci-common.c.
>>>> It works good.
>>>
>>>
>>> When I researched this, I thought the best route was to use the
>>> bus_register_notifier() path, as amd_iommu_init_notifier() does.
>>>
>>> We're filling in a struct device field, not a PCI field, and
>>> bus_register_notifier() seems like a more generic path than relying on
>>> pcibios_enable_device().
>>>
>>> Bjorn
>>>
>>>
>>
>
> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
> diff --git a/arch/powerpc/kernel/pci-common.c
> b/arch/powerpc/kernel/pci-common.c
> index 32656f1..1fe1e25 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1083,6 +1083,17 @@ void __devinit pcibios_setup_bus_self(struct pci_bus
> *bus)
>                ppc_md.pci_dma_bus_setup(bus);
>  }
>
> +static inline void pcibios_set_dma_ops(struct pci_dev *dev)
> +{
> +       /* Hook up default DMA ops */
> +       set_dma_ops(&dev->dev, pci_dma_ops);
> +       set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> +
> +       /* Additional platform DMA/iommu setup */
> +       if (ppc_md.pci_dma_dev_setup)
> +               ppc_md.pci_dma_dev_setup(dev);
> +}
> +
>  void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>  {
>        struct pci_dev *dev;
> @@ -1102,13 +1113,7 @@ void __devinit pcibios_setup_bus_devices(struct
> pci_bus *bus)
>                 */
>                set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>
> -               /* Hook up default DMA ops */
> -               set_dma_ops(&dev->dev, pci_dma_ops);
> -               set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> -
> -               /* Additional platform DMA/iommu setup */
> -               if (ppc_md.pci_dma_dev_setup)
> -                       ppc_md.pci_dma_dev_setup(dev);
> +               pcibios_set_dma_ops(dev);
>
>                /* Read default IRQs and fixup if necessary */
>                pci_read_irq_line(dev);
> @@ -1749,3 +1754,33 @@ static void fixup_hide_host_resource_fsl(struct
> pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID,
> fixup_hide_host_resource_fsl);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID,
> fixup_hide_host_resource_fsl);
> +
> +static int pcibios_device_change_notifier(struct notifier_block *nb,
> +                                         unsigned long action, void *data)
> +{
> +       struct pci_dev *dev = to_pci_dev(data);
> +
> +       if (!dev)
> +               return 0;

I don't think you need this check.

> +
> +       switch (action) {
> +       case BUS_NOTIFY_ADD_DEVICE:
> +               if (!get_dma_ops(&dev->dev))
> +                       pcibios_set_dma_ops(dev);
> +               break;
> +       default:
> +               break;

The "default:" case is superfluous.

> +       }
> +
> +       return 0;
> +}
> +
> +static struct notifier_block device_nb = {
> +       .notifier_call = pcibios_device_change_notifier,
> +};
> +
> +static int pcibios_bus_notifier_init(void)
> +{
> +       return bus_register_notifier(&pci_bus_type, &device_nb);
> +}
> +rootfs_initcall(pcibios_bus_notifier_init);

Instead of making this a rootfs_initcall(), can you call
bus_register_notifier() explicitly in pcibios_init()?  That way
pcibios_device_change_notifier() should get called for *all* new PCI
devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the
pcibios_setup_bus_devices() code into the
pcibios_device_change_notifier() path, including the NUMA node and IRQ
fixups.

Your patch will also need a changelog.

Bjorn

Re: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

Hi, Bjorn


Thanks a lot for your review and comment.
I will rethink again.


Regards.

Hiroo MATSUMOTO

> On Mon, Apr 16, 2012 at 12:32 AM, Hiroo Matsumoto
> <matsumoto.hiroo@jp.fujitsu.com> wrote:
>> Hi, Bjorn
>>
>>
>> I wrote a code with bus_register_notifier().
>> A function that calls bus_register_notifer() is called from rootfs_initcall
>> as well as amd_iommu is.
>>
>> Please review this.
>>
>>
>> Regards.
>>
>> Hiroo MATSUMOTO
>>
>>
>>> Hi, Bjorn
>>>
>>>
>>> Thanks for your research and comment.
>>>
>>> As you said, a way of registering code with bus_register_notifier(), which
>>> will be called in device_add(), is better one than pcibios_enable_device().
>>> I will try to write code with bus_register_notifier().
>>>
>>>
>>> Regards.
>>>
>>> Hiroo MATSUMOTO
>>>
>>>> On Wed, Apr 11, 2012 at 11:00 PM, Hiroo Matsumoto
>>>> <matsumoto.hiroo@jp.fujitsu.com> wrote:
>>>>> Hi, Kaneshige
>>>>>
>>>>>
>>>>> You are right!
>>>>> Setting dma_ops in pcibios_enable_device is a nice way.
>>>>> In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is
>>>>> called before checking archdata.dma_ops.
>>>>>
>>>>> I added code of checking and setting dma_ops to pcibios_enable_device in
>>>>> arch/powerpc/kernel/pci-common.c.
>>>>> It works good.
>>>>
>>>> When I researched this, I thought the best route was to use the
>>>> bus_register_notifier() path, as amd_iommu_init_notifier() does.
>>>>
>>>> We're filling in a struct device field, not a PCI field, and
>>>> bus_register_notifier() seems like a more generic path than relying on
>>>> pcibios_enable_device().
>>>>
>>>> Bjorn
>>>>
>>>>
>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>> diff --git a/arch/powerpc/kernel/pci-common.c
>> b/arch/powerpc/kernel/pci-common.c
>> index 32656f1..1fe1e25 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1083,6 +1083,17 @@ void __devinit pcibios_setup_bus_self(struct pci_bus
>> *bus)
>>                ppc_md.pci_dma_bus_setup(bus);
>>  }
>>
>> +static inline void pcibios_set_dma_ops(struct pci_dev *dev)
>> +{
>> +       /* Hook up default DMA ops */
>> +       set_dma_ops(&dev->dev, pci_dma_ops);
>> +       set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> +       /* Additional platform DMA/iommu setup */
>> +       if (ppc_md.pci_dma_dev_setup)
>> +               ppc_md.pci_dma_dev_setup(dev);
>> +}
>> +
>>  void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>>  {
>>        struct pci_dev *dev;
>> @@ -1102,13 +1113,7 @@ void __devinit pcibios_setup_bus_devices(struct
>> pci_bus *bus)
>>                 */
>>                set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>
>> -               /* Hook up default DMA ops */
>> -               set_dma_ops(&dev->dev, pci_dma_ops);
>> -               set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> -
>> -               /* Additional platform DMA/iommu setup */
>> -               if (ppc_md.pci_dma_dev_setup)
>> -                       ppc_md.pci_dma_dev_setup(dev);
>> +               pcibios_set_dma_ops(dev);
>>
>>                /* Read default IRQs and fixup if necessary */
>>                pci_read_irq_line(dev);
>> @@ -1749,3 +1754,33 @@ static void fixup_hide_host_resource_fsl(struct
>> pci_dev *dev)
>>  }
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID,
>> fixup_hide_host_resource_fsl);
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID,
>> fixup_hide_host_resource_fsl);
>> +
>> +static int pcibios_device_change_notifier(struct notifier_block *nb,
>> +                                         unsigned long action, void *data)
>> +{
>> +       struct pci_dev *dev = to_pci_dev(data);
>> +
>> +       if (!dev)
>> +               return 0;
> 
> I don't think you need this check.
> 
>> +
>> +       switch (action) {
>> +       case BUS_NOTIFY_ADD_DEVICE:
>> +               if (!get_dma_ops(&dev->dev))
>> +                       pcibios_set_dma_ops(dev);
>> +               break;
>> +       default:
>> +               break;
> 
> The "default:" case is superfluous.
> 
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct notifier_block device_nb = {
>> +       .notifier_call = pcibios_device_change_notifier,
>> +};
>> +
>> +static int pcibios_bus_notifier_init(void)
>> +{
>> +       return bus_register_notifier(&pci_bus_type, &device_nb);
>> +}
>> +rootfs_initcall(pcibios_bus_notifier_init);
> 
> Instead of making this a rootfs_initcall(), can you call
> bus_register_notifier() explicitly in pcibios_init()?  That way
> pcibios_device_change_notifier() should get called for *all* new PCI
> devices, whether we find them at boot-time or they're hot-added later.
> 
> If you do that, I think you can move all the
> pcibios_setup_bus_devices() code into the
> pcibios_device_change_notifier() path, including the NUMA node and IRQ
> fixups.
> 
> Your patch will also need a changelog.
> 
> Bjorn
> 
> 

Re: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

[-- Attachment #1: Type: text/plain, Size: 4252 bytes --]

Hi, Bjorn


Refer to your review and comment, I updated bus notifier code.
This works good. Thanks for your comments.
And I think this way can be applied to microblaze.
Please review this patch.


* Changes
   1. Moving pcibios_setup_bus_devices code to 
pcibios_device_change_notifier so that boot and hotplug use same code. 
This will change boot message on powerpc platform but there is no 
difference. Please see "Not patched boot message" and "Patched message".
   2. Calling bus_register_notifier in pcibios_init.
   3. Using bus notifier not only on powerpc platform but also on 
microblaze platform because microblaze pcibios_setup_bus_devices is a 
similer way with powerpc.
   4. Removing caller and callee of pcibios_setup_bus_devices because 
bus notifier works instead of pcibios_setup_bus_devices.

* Not patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0:   bridge window [io  0x0000-0x0000] (disabled)
pci 0000:00:00.0:   bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0:   bridge window [mem 0x10000000-0x000fffff pref] 
(disabled)
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
<snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0:   bridge window [io  0x1000-0x1fff]
pci 0000:01:00.0:   bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0:   bridge window [mem 0x10000000-0x000fffff pref] 
(disabled)
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0:   bridge window [io  0x0000-0x0000] (disabled)
pci 0000:00:00.0:   bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0:   bridge window [mem 0x10000000-0x000fffff pref] 
(disabled)
<snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0:   bridge window [io  0x1000-0x1fff]
pci 0000:01:00.0:   bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0:   bridge window [mem 0x10000000-0x000fffff pref] 
(disabled)
<snip>
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Not patched pciehp message on powerpc platform
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0:   bridge window [io  0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0:   bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0:   bridge window [mem 0xa0200000-0xa02fffff 64bit 
pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM  L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5

* Patched pciehp message on powerpc platform
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0:   bridge window [io  0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0:   bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0:   bridge window [mem 0xa0200000-0xa02fffff 64bit 
pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM  L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003


Regards.

Hiroo MATSUMOTO


> Instead of making this a rootfs_initcall(), can you call
> bus_register_notifier() explicitly in pcibios_init()?  That way
> pcibios_device_change_notifier() should get called for *all* new PCI
> devices, whether we find them at boot-time or they're hot-added later.
> 
> If you do that, I think you can move all the
> pcibios_setup_bus_devices() code into the
> pcibios_device_change_notifier() path, including the NUMA node and IRQ
> fixups.
> 
> Your patch will also need a changelog.
> 
> Bjorn
> 
> 

[-- Attachment #2: add-pcibios_device_change_notifier.patch --]
[-- Type: text/plain, Size: 9266 bytes --]

Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
diff --git a/arch/microblaze/include/asm/pci.h b/arch/microblaze/include/asm/pci.h
index 0331376..8d11277 100644
--- a/arch/microblaze/include/asm/pci.h
+++ b/arch/microblaze/include/asm/pci.h
@@ -149,8 +149,8 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
 				 const struct resource *rsrc,
 				 resource_size_t *start, resource_size_t *end);
 
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
 extern void pcibios_setup_bus_self(struct pci_bus *bus);
+extern void pcibios_setup_bus_notifier(void);
 
 /* This part of code was originally in xilinx-pci.h */
 #ifdef CONFIG_PCI_XILINX
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 85f2ac1..bb28ede 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -1063,31 +1063,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
 		pcibios_fixup_bridge(bus);
 }
 
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
-	struct pci_dev *dev;
-
-	pr_debug("PCI: Fixup bus devices %d (%s)\n",
-		 bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		/* Setup OF node pointer in archdata */
-		dev->dev.of_node = pci_device_to_OF_node(dev);
-
-		/* Fixup NUMA node as it may not be setup yet by the generic
-		 * code and is needed by the DMA init
-		 */
-		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
-		/* Hook up default DMA ops */
-		set_dma_ops(&dev->dev, pci_dma_ops);
-		dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
-
-		/* Read default IRQs and fixup if necessary */
-		pci_read_irq_line(dev);
-	}
-}
-
 void __devinit pcibios_fixup_bus(struct pci_bus *bus)
 {
 	/* When called from the generic PCI probe, read PCI<->PCI bridge
@@ -1099,9 +1074,6 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
 
 	/* Now fixup the bus bus */
 	pcibios_setup_bus_self(bus);
-
-	/* Now fixup devices on that bus */
-	pcibios_setup_bus_devices(bus);
 }
 EXPORT_SYMBOL(pcibios_fixup_bus);
 
@@ -1604,6 +1576,43 @@ static void __devinit pcibios_scan_phb(struct pci_controller *hose)
 	hose->last_busno = bus->subordinate;
 }
 
+static int pcibios_device_change_notifier(struct notifier_block *nb,
+					  unsigned long action, void *data)
+{
+	struct pci_dev *dev = to_pci_dev(data);
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		/* Setup OF node pointer in archdata */
+		dev->dev.of_node = pci_device_to_OF_node(dev);
+
+		/* Fixup NUMA node as it may not be setup yet by the generic
+		 * code and is needed by the DMA init
+		 */
+		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+		/* Hook up default DMA ops */
+		set_dma_ops(&dev->dev, pci_dma_ops);
+		dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
+
+		/* Read default IRQs and fixup if necessary */
+		pci_read_irq_line(dev);
+
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block device_nb = {
+	.notifier_call = pcibios_device_change_notifier,
+};
+
+void __devinit pcibios_setup_bus_notifier(void)
+{
+	bus_register_notifier(&pci_bus_type, &device_nb);
+}
+
 static int __init pcibios_init(void)
 {
 	struct pci_controller *hose, *tmp;
@@ -1611,6 +1620,8 @@ static int __init pcibios_init(void)
 
 	printk(KERN_INFO "PCI: Probing PCI hardware\n");
 
+	pcibios_setup_bus_notifier();
+
 	/* Scan all of the recorded PCI controllers.  */
 	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
 		hose->last_busno = 0xff;
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index f54b3d2..7b4ca5a 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -190,10 +190,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
 				 const struct resource *rsrc,
 				 resource_size_t *start, resource_size_t *end);
 
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
 extern void pcibios_setup_bus_self(struct pci_bus *bus);
 extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
 extern void pcibios_scan_phb(struct pci_controller *hose);
+extern void pcibios_setup_bus_notifier(void);
 
 #endif	/* __KERNEL__ */
 #endif /* __ASM_POWERPC_PCI_H */
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index cce98d7..78ac583 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1097,40 +1097,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
 		ppc_md.pci_dma_bus_setup(bus);
 }
 
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
-	struct pci_dev *dev;
-
-	pr_debug("PCI: Fixup bus devices %d (%s)\n",
-		 bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		/* Cardbus can call us to add new devices to a bus, so ignore
-		 * those who are already fully discovered
-		 */
-		if (dev->is_added)
-			continue;
-
-		/* Fixup NUMA node as it may not be setup yet by the generic
-		 * code and is needed by the DMA init
-		 */
-		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
-		/* Hook up default DMA ops */
-		set_dma_ops(&dev->dev, pci_dma_ops);
-		set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
-
-		/* Additional platform DMA/iommu setup */
-		if (ppc_md.pci_dma_dev_setup)
-			ppc_md.pci_dma_dev_setup(dev);
-
-		/* Read default IRQs and fixup if necessary */
-		pci_read_irq_line(dev);
-		if (ppc_md.pci_irq_fixup)
-			ppc_md.pci_irq_fixup(dev);
-	}
-}
-
 void pcibios_set_master(struct pci_dev *dev)
 {
 	/* No special bus mastering setup handling */
@@ -1147,16 +1113,11 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
 
 	/* Now fixup the bus bus */
 	pcibios_setup_bus_self(bus);
-
-	/* Now fixup devices on that bus */
-	pcibios_setup_bus_devices(bus);
 }
 EXPORT_SYMBOL(pcibios_fixup_bus);
 
 void __devinit pci_fixup_cardbus(struct pci_bus *bus)
 {
-	/* Now fixup devices on that bus */
-	pcibios_setup_bus_devices(bus);
 }
 
 
@@ -1763,6 +1724,55 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
 	}
 }
 
+static int pcibios_device_change_notifier(struct notifier_block *nb,
+					  unsigned long action, void *data)
+{
+	struct pci_dev *dev = to_pci_dev(data);
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		/* Cardbus can call us to add new devices to a bus, so ignore
+		 * those who are already fully discovered
+		 */
+		if (dev->is_added)
+			break;
+
+		/* Setup OF node pointer in the device */
+		dev->dev.of_node = pci_device_to_OF_node(dev);
+
+		/* Fixup NUMA node as it may not be setup yet by the generic
+		 * code and is needed by the DMA init
+		 */
+		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+		/* Hook up default DMA ops */
+		set_dma_ops(&dev->dev, pci_dma_ops);
+		set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+		/* Additional platform DMA/iommu setup */
+		if (ppc_md.pci_dma_dev_setup)
+			ppc_md.pci_dma_dev_setup(dev);
+
+		/* Read default IRQs and fixup if necessary */
+		pci_read_irq_line(dev);
+		if (ppc_md.pci_irq_fixup)
+			ppc_md.pci_irq_fixup(dev);
+
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block device_nb = {
+	.notifier_call = pcibios_device_change_notifier,
+};
+
+void __devinit pcibios_setup_bus_notifier(void)
+{
+	bus_register_notifier(&pci_bus_type, &device_nb);
+}
+
 static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
 {
 	int i, class = dev->class >> 8;
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index fdd1a3d..5a30cec 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -231,6 +231,8 @@ static int __init pcibios_init(void)
 
 	printk(KERN_INFO "PCI: Probing PCI hardware\n");
 
+	pcibios_setup_bus_notifier();
+
 	if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
 		pci_assign_all_buses = 1;
 
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 3318d39..d66c9dc 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -50,6 +50,8 @@ static int __init pcibios_init(void)
 
 	printk(KERN_INFO "PCI: Probing PCI hardware\n");
 
+	pcibios_setup_bus_notifier();
+
 	/* For now, override phys_mem_access_prot. If we need it,g
 	 * later, we may move that initialization to each ppc_md
 	 */
diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index b37d0b5..1e29642 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -329,7 +329,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
 	 */
 	if (!rescan_existing)
 		pcibios_setup_bus_self(bus);
-	pcibios_setup_bus_devices(bus);
 
 	/* Now scan child busses */
 	list_for_each_entry(dev, &bus->devices, bus_list) {
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
index 55d4ec1..fdb8b64 100644
--- a/arch/powerpc/platforms/pseries/pci_dlpar.c
+++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
@@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
 		num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
 		if (!num)
 			return;
-		pcibios_setup_bus_devices(bus);
 		max = bus->secondary;
 		for (pass=0; pass < 2; pass++)
 			list_for_each_entry(dev, &bus->devices, bus_list) {

Re: [RFC] pciehp: Add archdata setting

[Kenji Kaneshige]

Hi,

I have some comments.

- I think the patch should be split into two, for microblaze and
  for powerpc.

- The following code looks redundant. I think dev->is_added is
  always false at the BUS_NOTIFY_ADD_DEVICE time.

+		/* Cardbus can call us to add new devices to a bus, so ignore
+		 * those who are already fully discovered
+		 */
+		if (dev->is_added)
+			break;

- You need to add patch description.

Regards,
Kenji Kaneshige


(2012/05/10 19:38), Hiroo Matsumoto wrote:
> Hi, Bjorn
> 
> 
> Refer to your review and comment, I updated bus notifier code.
> This works good. Thanks for your comments.
> And I think this way can be applied to microblaze.
> Please review this patch.
> 
> 
> * Changes
> 1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
> 2. Calling bus_register_notifier in pcibios_init.
> 3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
> 4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.
> 
> * Not patched boot message
> PCI: Probing PCI hardware
> <snip>
> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
> <snip>
> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
> 
> * Patched boot message
> PCI: Probing PCI hardware
> <snip>
> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
> <snip>
> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
> <snip>
> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
> 
> * Not patched pciehp message on powerpc platform
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
> pci 0000:03:00.0: no hotplug settings from platform
> e1000e 0000:03:00.0: Disabling ASPM L1
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> e1000e 0000:03:00.0: No usable DMA configuration, aborting
> e1000e: probe of 0000:03:00.0 failed with error -5
> 
> * Patched pciehp message on powerpc platform
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
> pci 0000:03:00.0: no hotplug settings from platform
> e1000e 0000:03:00.0: Disabling ASPM L1
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
> 
> 
> Regards.
> 
> Hiroo MATSUMOTO
> 
> 
>> Instead of making this a rootfs_initcall(), can you call
>> bus_register_notifier() explicitly in pcibios_init()? That way
>> pcibios_device_change_notifier() should get called for *all* new PCI
>> devices, whether we find them at boot-time or they're hot-added later.
>>
>> If you do that, I think you can move all the
>> pcibios_setup_bus_devices() code into the
>> pcibios_device_change_notifier() path, including the NUMA node and IRQ
>> fixups.
>>
>> Your patch will also need a changelog.
>>
>> Bjorn
>>
>>

Re: [RFC] pciehp: Add archdata setting

[Bjorn Helgaas]

On Thu, May 10, 2012 at 6:09 AM, Kenji Kaneshige
<kaneshige.kenji@jp.fujitsu.com> wrote:
> Hi,
>
> I have some comments.
>
> - I think the patch should be split into two, for microblaze and
>  for powerpc.
>
> - The following code looks redundant. I think dev->is_added is
>  always false at the BUS_NOTIFY_ADD_DEVICE time.
>
> +               /* Cardbus can call us to add new devices to a bus, so ignore
> +                * those who are already fully discovered
> +                */
> +               if (dev->is_added)
> +                       break;
>
> - You need to add patch description.

Thanks for doing this; I think the code looks great.  Please address
Kenji-san's dev->is_added comment.

He's also right that we need this in two patches.  Add cc: Benjamin
Herrenschmidt <benh@kernel.crashing.org> when you repost it so he can
ack/nack the powerpc part.

> (2012/05/10 19:38), Hiroo Matsumoto wrote:
>> Hi, Bjorn
>>
>>
>> Refer to your review and comment, I updated bus notifier code.
>> This works good. Thanks for your comments.
>> And I think this way can be applied to microblaze.
>> Please review this patch.
>>
>>
>> * Changes
>> 1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
>> 2. Calling bus_register_notifier in pcibios_init.
>> 3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
>> 4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.
>>
>> * Not patched boot message
>> PCI: Probing PCI hardware
>> <snip>
>> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
>> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
>> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
>> <snip>
>> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
>> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
>> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
>> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
>> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
>>
>> * Patched boot message
>> PCI: Probing PCI hardware
>> <snip>
>> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
>> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
>> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>> <snip>
>> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
>> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
>> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
>> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>> <snip>
>> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
>> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
>> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
>>
>> * Not patched pciehp message on powerpc platform
>> # echo 1 > /sys/bus/pci/slots/1/power
>> <snip>
>> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
>> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
>> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
>> pci 0000:03:00.0: no hotplug settings from platform
>> e1000e 0000:03:00.0: Disabling ASPM L1
>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> e1000e 0000:03:00.0: No usable DMA configuration, aborting
>> e1000e: probe of 0000:03:00.0 failed with error -5
>>
>> * Patched pciehp message on powerpc platform
>> # echo 1 > /sys/bus/pci/slots/1/power
>> <snip>
>> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
>> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
>> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
>> pci 0000:03:00.0: no hotplug settings from platform
>> e1000e 0000:03:00.0: Disabling ASPM L1
>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>
>>
>> Regards.
>>
>> Hiroo MATSUMOTO
>>
>>
>>> Instead of making this a rootfs_initcall(), can you call
>>> bus_register_notifier() explicitly in pcibios_init()? That way
>>> pcibios_device_change_notifier() should get called for *all* new PCI
>>> devices, whether we find them at boot-time or they're hot-added later.
>>>
>>> If you do that, I think you can move all the
>>> pcibios_setup_bus_devices() code into the
>>> pcibios_device_change_notifier() path, including the NUMA node and IRQ
>>> fixups.
>>>
>>> Your patch will also need a changelog.
>>>
>>> Bjorn
>>>
>>>
>
> --
> 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: [RFC] pciehp: Add archdata setting

[Hiroo Matsumoto]

Hi, Bjorn, Kaneshige


Thanks for your comments.

As Kaneshige said, if dev->is_added is set, bus notifier will not be called.
I will modify it and split this patch to powerpc and microblaze, then repost
it with adding cc:.


Regards.

Hiroo MATSUMOTO

> On Thu, May 10, 2012 at 6:09 AM, Kenji Kaneshige
> <kaneshige.kenji@jp.fujitsu.com> wrote:
>> Hi,
>>
>> I have some comments.
>>
>> - I think the patch should be split into two, for microblaze and
>>  for powerpc.
>>
>> - The following code looks redundant. I think dev->is_added is
>>  always false at the BUS_NOTIFY_ADD_DEVICE time.
>>
>> +               /* Cardbus can call us to add new devices to a bus, so ignore
>> +                * those who are already fully discovered
>> +                */
>> +               if (dev->is_added)
>> +                       break;
>>
>> - You need to add patch description.
> 
> Thanks for doing this; I think the code looks great.  Please address
> Kenji-san's dev->is_added comment.
> 
> He's also right that we need this in two patches.  Add cc: Benjamin
> Herrenschmidt <benh@kernel.crashing.org> when you repost it so he can
> ack/nack the powerpc part.
> 
>> (2012/05/10 19:38), Hiroo Matsumoto wrote:
>>> Hi, Bjorn
>>>
>>>
>>> Refer to your review and comment, I updated bus notifier code.
>>> This works good. Thanks for your comments.
>>> And I think this way can be applied to microblaze.
>>> Please review this patch.
>>>
>>>
>>> * Changes
>>> 1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
>>> 2. Calling bus_register_notifier in pcibios_init.
>>> 3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
>>> 4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.
>>>
>>> * Not patched boot message
>>> PCI: Probing PCI hardware
>>> <snip>
>>> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>>> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
>>> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
>>> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>>> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
>>> <snip>
>>> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
>>> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
>>> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
>>> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>>> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
>>> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
>>>
>>> * Patched boot message
>>> PCI: Probing PCI hardware
>>> <snip>
>>> pci 0000:00:00.0: PCI bridge to [bus 01-ff]
>>> pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled)
>>> pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
>>> pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>>> <snip>
>>> pci 0000:01:00.0: PCI bridge to [bus 02-ff]
>>> pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
>>> pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
>>> pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
>>> <snip>
>>> irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
>>> irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
>>> irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18
>>>
>>> * Not patched pciehp message on powerpc platform
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>>> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
>>> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
>>> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
>>> pci 0000:03:00.0: no hotplug settings from platform
>>> e1000e 0000:03:00.0: Disabling ASPM L1
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> e1000e 0000:03:00.0: No usable DMA configuration, aborting
>>> e1000e: probe of 0000:03:00.0 failed with error -5
>>>
>>> * Patched pciehp message on powerpc platform
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
>>> pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
>>> pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
>>> pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
>>> pci 0000:03:00.0: no hotplug settings from platform
>>> e1000e 0000:03:00.0: Disabling ASPM L1
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>>> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>>> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>>> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>>
>>>
>>> Regards.
>>>
>>> Hiroo MATSUMOTO
>>>
>>>
>>>> Instead of making this a rootfs_initcall(), can you call
>>>> bus_register_notifier() explicitly in pcibios_init()? That way
>>>> pcibios_device_change_notifier() should get called for *all* new PCI
>>>> devices, whether we find them at boot-time or they're hot-added later.
>>>>
>>>> If you do that, I think you can move all the
>>>> pcibios_setup_bus_devices() code into the
>>>> pcibios_device_change_notifier() path, including the NUMA node and IRQ
>>>> fixups.
>>>>
>>>> Your patch will also need a changelog.
>>>>
>>>> Bjorn
>>>>
>>>>
>> --
>> 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
> 
>