pciehp: only wait command complete for real hotplug control

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

System with AMD/Nvidia chipset have same problem.

Two ways to address the problem:
1. change to only wait when (EIC, PCC, PIC, AIC) bits get changed.
2. or check CMD_COMPLETE and wait before write command. Only wait when CC is set and cmd_busy is true. For chipset from intel will only have CC set for real hotplug control, so we could skip the wait for others.

This patch is using second way.

With that we could save 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

This patch address spurious "cmd completed" event problem that that Rajat met.

(quoted diff) +0 -2

-v2: use second way that is suggested by Bjorn.
-v3: rebase after
        PCI: pciehp: Acknowledge spurious "cmd completed" event

Signed-off-by Yinghai Lu <[email protected]>

Tested-by Rajat Jain <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |   28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

drivers/pci/hotplug/pciehp_hpc.c +10 -18

@@ -158,17 +158,8 @@ static void pcie_write_cmd(struct contro
        mutex_lock(&ctrl->ctrl_lock);
 
        pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-       if (slot_status & PCI_EXP_SLTSTA_CC) {
-               pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-                                          PCI_EXP_SLTSTA_CC);
-               if (!ctrl->no_cmd_complete) {
-                       /*
-                        * After 1 sec and CMD_COMPLETED still not set, just
-                        * proceed forward to issue the next command according
-                        * to spec. Just print out the error message.
-                        */
-                       ctrl_dbg(ctrl, "CMD_COMPLETED not clear after 1 sec\n");
-               } else if (!NO_CMD_CMPL(ctrl)) {
+       if (ctrl->no_cmd_complete && slot_status & PCI_EXP_SLTSTA_CC) {
+               if (!NO_CMD_CMPL(ctrl)) {
                        /*
                         * This controller seems to notify of command completed
                         * event even though it supports none of power

@@ -184,16 +175,11 @@ static void pcie_write_cmd(struct contro
        }
 
        pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-       slot_ctrl &= ~mask;
-       slot_ctrl |= (cmd & mask);
-       ctrl->cmd_busy = 1;
-       smp_mb();
-       pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
-
        /*
         * Wait for command completion.
         */
-       if (!ctrl->no_cmd_complete) {
+       if (!ctrl->no_cmd_complete && (slot_status & PCI_EXP_SLTSTA_CC) &&
+           ctrl->cmd_busy) {
                int poll = 0;
                /*
                 * if hotplug interrupt is not enabled or command

@@ -205,6 +191,12 @@ static void pcie_write_cmd(struct contro
                        poll = 1;
                 pcie_wait_cmd(ctrl, poll);
        }
+
+       slot_ctrl &= ~mask;
+       slot_ctrl |= (cmd & mask);
+       ctrl->cmd_busy = 1;
+       smp_mb();
+       pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
        mutex_unlock(&ctrl->ctrl_lock);
 }
 

Quoted 9 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Quoted 1 line

System with AMD/Nvidia chipset have same problem.

Do you have a datasheet reference for any of these parts? Do we have any way of identifying the affected parts? Is it *all* Intel parts, *all* AMD parts, *all* Nvidia parts?

I want to have a comment in the code because we basically have two strategies for dealing with Command Complete. Both are legal according to the PCIe spec, and you're changing from strategy A to strategy B because B works better on these parts. We need a hint in the code that tells us not to change from B back to A and why. Otherwise some future programmer is likely to change back to A, which will break your box again.

Quoted 12 lines · Bjorn Helgaas

Quoted 10 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

System with AMD/Nvidia chipset have same problem.

Quoted 3 lines

Do you have a datasheet reference for any of these parts? Do we have any way of identifying the affected parts? Is it *all* Intel parts, *all* AMD parts, *all* Nvidia parts?

For intel: Nehalem, westmere, sndbridge, ivybridge.

For nvidia, only according to boot log for old systems with chipset:
ck804 and mcp55.

Quoted 7 lines

I want to have a comment in the code because we basically have two strategies for dealing with Command Complete. Both are legal according to the PCIe spec, and you're changing from strategy A to strategy B because B works better on these parts. We need a hint in the code that tells us not to change from B back to A and why. Otherwise some future programmer is likely to change back to A, which will break your box again.

ok, will try to add comment about the change.

Thanks

Yinghai

Quoted 4 lines

For intel: Nehalem, westmere, sndbridge, ivybridge.

For nvidia, only according to boot log for old systems with chipset:
ck804 and mcp55.

I meant, do you have a reference to a section in the spec that talks about how this works?

So far all we have is folklore that was discovered by experimentation.
It'd be nice to see the spec that says "CC is only used when changing
these listed bits."

Quoted 5 lines · Bjorn Helgaas

Quoted 4 lines

For intel: Nehalem, westmere, sndbridge, ivybridge.

For nvidia, only according to boot log for old systems with chipset:
ck804 and mcp55.

Quoted 6 lines

I meant, do you have a reference to a section in the spec that talks about how this works?

So far all we have is folklore that was discovered by experimentation.
It'd be nice to see the spec that says "CC is only used when changing
these listed bits."

We will have doc from Intel about that. Hope it is not under NDA.

Quoted 14 lines · Yinghai Lu

Quoted 11 lines · Bjorn Helgaas

Quoted 4 lines

For intel: Nehalem, westmere, sndbridge, ivybridge.

For nvidia, only according to boot log for old systems with chipset:
ck804 and mcp55.

I meant, do you have a reference to a section in the spec that talks about how this works?

So far all we have is folklore that was discovered by experimentation.
It'd be nice to see the spec that says "CC is only used when changing
these listed bits."

Quoted 1 line

We will have doc from Intel about that. Hope it is not under NDA.

Intel published that yesterday.

http://www.intel.com/content/www/us/en/processors/xeon/xeon-e7-v2-spec-update.html
in section CF118.