Thanks, I'll test it out on my systems today.
On Wed, Mar 19, 2014 at 11:47 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Wed, Mar 19, 2014 at 8:16 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>> On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>>> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>>
>>> I think the current pciehp design is a bit broken. Today we perform
>>> commands very synchronously:
>>>
>>> pcie_write_cmd() {
>>> write Slot Control
>>> if (Command Complete supported) {
>>> wait for Command Complete
>>> }
>>> }
>>>
>>> The typical driver pattern would be more asynchronous, like this:
>>>
>>> pcie_write_cmd() {
>>> read Slot Status
>>> if (!Command Completed) {
>>> wait for Command Complete
>>> }
>>> write Slot Control
>>> }
>>>
>>> With the asynchronous pattern, we would probably never wait at all
>>> unless we performed two commands in immediate succession. I wouldn't
>>> be surprised if doing this asynchronously would effectively cover up
>>> these hardware differences.
>>
>> I like this idea. Will give it a try.
>
> It works. Please check the patch that check CC before wait/write command.
>
> Subject: [PATCH v2] 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.
>
> -v2: use second way that is suggested by Bjorn.
>
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> @@ -158,15 +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) {
> - 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
> @@ -182,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
> @@ -203,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);
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
