[RFC] PCI: add support for Immediate Readiness

Message ID 20180802113635.7097-1-felipe.balbi@linux.intel.com
State New
Delegated to: Bjorn Helgaas
Headers show
Series
  • [RFC] PCI: add support for Immediate Readiness
Related show

Commit Message

Felipe Balbi Aug. 2, 2018, 11:36 a.m.
PCIe GEN4 defines a new bit on Status Register which tells us that, if
Set, a function is immediately ready after a Reset. This means that
all delays after a Conventional or Function Reset can be skipped.

This patch reads such bit and caches its value in a flag inside struct
pci_dev to be checked later if we should delay or can skip delays
after a reset.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/pci/pci.c             | 9 ++++++++-
 include/linux/pci.h           | 1 +
 include/uapi/linux/pci_regs.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Aug. 2, 2018, 11:56 a.m. | #1
> +	unsigned int	immediate:1;	/* Supports Immediate Readiness */

That name is a bit too generic.  immediate_ready (or imm_ready if you
need it short) would be a lot more descriptive.
Felipe Balbi Aug. 2, 2018, 12:11 p.m. | #2
Hi,

Christoph Hellwig <hch@infradead.org> writes:
>> +	unsigned int	immediate:1;	/* Supports Immediate Readiness */
>
> That name is a bit too generic.  immediate_ready (or imm_ready if you
> need it short) would be a lot more descriptive.

That can be easily patched. I'll until tomorrow for more comments then
send a v2
Sinan Kaya Aug. 3, 2018, 6:14 a.m. | #3
On 8/2/2018 7:36 AM, Felipe Balbi wrote:
> PCIe GEN4 defines a new bit on Status Register which tells us that, if
> Set, a function is immediately ready after a Reset. This means that
> all delays after a Conventional or Function Reset can be skipped.

Can you give a reference to the section of the specification? or
a pointer to the ECN?
Felipe Balbi Aug. 3, 2018, 6:26 a.m. | #4
Sinan Kaya <okaya@kernel.org> writes:

> On 8/2/2018 7:36 AM, Felipe Balbi wrote:
>> PCIe GEN4 defines a new bit on Status Register which tells us that, if
>> Set, a function is immediately ready after a Reset. This means that
>> all delays after a Conventional or Function Reset can be skipped.
>
> Can you give a reference to the section of the specification? or
> a pointer to the ECN?

Section 7.5.1.1.4 of PCIe GEN4 spec. Table 7-4:

Immediate Readiness – This optional bit, when Set, indicates the
Function is guaranteed to be ready to successfully complete valid
configuration accesses at any time following any reset that the host is
capable of issuing Configuration Requests to this Function.

When this bit is Set, for accesses to this Function, software is exempt
from all requirements to delay configuration accesses following any type
of reset, including but not limited to the timing requirements defined
in Section 6.6.  How this guarantee is established is beyond the scope
of this document.

It is permitted that system software/firmware provide mechanisms that
supersede the indication provided by this bit, however such
software/firmware mechanisms are outside the scope of this
specification.
Sinan Kaya Aug. 3, 2018, 5:25 p.m. | #5
On 8/3/2018 2:26 AM, Felipe Balbi wrote:
> Sinan Kaya <okaya@kernel.org> writes:
> 
>> On 8/2/2018 7:36 AM, Felipe Balbi wrote:
>>> PCIe GEN4 defines a new bit on Status Register which tells us that, if
>>> Set, a function is immediately ready after a Reset. This means that
>>> all delays after a Conventional or Function Reset can be skipped.
>>
>> Can you give a reference to the section of the specification? or
>> a pointer to the ECN?
> 
> Section 7.5.1.1.4 of PCIe GEN4 spec. Table 7-4:
> 
> Immediate Readiness – This optional bit, when Set, indicates the
> Function is guaranteed to be ready to successfully complete valid
> configuration accesses at any time following any reset that the host is
> capable of issuing Configuration Requests to this Function.
> 
> When this bit is Set, for accesses to this Function, software is exempt
> from all requirements to delay configuration accesses following any type
> of reset, including but not limited to the timing requirements defined
> in Section 6.6.  How this guarantee is established is beyond the scope
> of this document.
> 
> It is permitted that system software/firmware provide mechanisms that
> supersede the indication provided by this bit, however such
> software/firmware mechanisms are outside the scope of this
> specification.
> 

Thanks for the spec reference.

I think the patch is touching the wrong places. pci_dev_wait() is there
to wait for CRS response to finish following reset.

Typical sequences are:
1. Do some kind of reset in another routine
2. Wait reset specific wait time (1sec for secondary bus reset as an
example and 100ms for d3-d0 transition)
3. call pci_dev_wait() after reset to see if device can accept config
transactions.

Since this applies to all resets, I think you also need to get rid of
waits following different reset types in step #2 and return immediately.
I suggest you review callers of pci_dev_wait() and tap in there.

Another thing is that this is a common functionality. Initializing
the flag in pm_init() would not be the best place.

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 316496e99da9..cf5239edd46a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -835,7 +835,7 @@  static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
 		 * because have already delayed for the bridge.
 		 */
 		if (dev->runtime_d3cold) {
-			if (dev->d3cold_delay)
+			if (dev->d3cold_delay && !dev->immediate)
 				msleep(dev->d3cold_delay);
 			/*
 			 * When powering on a bridge from D3cold, the
@@ -2442,6 +2442,7 @@  EXPORT_SYMBOL_GPL(pci_d3cold_disable);
 void pci_pm_init(struct pci_dev *dev)
 {
 	int pm;
+	u16 status;
 	u16 pmc;
 
 	pm_runtime_forbid(&dev->dev);
@@ -2504,6 +2505,9 @@  void pci_pm_init(struct pci_dev *dev)
 		/* Disable the PME# generation functionality */
 		pci_pme_active(dev, false);
 	}
+
+	pci_read_config_word(dev, PCI_STATUS, &status);
+	dev->immediate = status & PCI_STATUS_IMMEDIATE;
 }
 
 static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
@@ -4034,6 +4038,9 @@  static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 	int delay = 1;
 	u32 id;
 
+	if (dev->immediate)
+		return 0;
+
 	/*
 	 * After reset, the device should not silently discard config
 	 * requests, but it may still indicate that it needs more time by
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..d8f0a382b026 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -321,6 +321,7 @@  struct pci_dev {
 	pci_power_t	current_state;	/* Current operating state. In ACPI,
 					   this is D0-D3, D0 being fully
 					   functional, and D3 being off. */
+	unsigned int	immediate:1;	/* Supports Immediate Readiness */
 	u8		pm_cap;		/* PM capability offset */
 	unsigned int	pme_support:5;	/* Bitmask of states from which PME#
 					   can be generated */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 4da87e2ef8a8..796d12910791 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -52,6 +52,7 @@ 
 #define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
 
 #define PCI_STATUS		0x06	/* 16 bits */
+#define  PCI_STATUS_IMMEDIATE	0x01	/* Immediate Readiness */
 #define  PCI_STATUS_INTERRUPT	0x08	/* Interrupt status */
 #define  PCI_STATUS_CAP_LIST	0x10	/* Support Capability List */
 #define  PCI_STATUS_66MHZ	0x20	/* Support 66 MHz PCI 2.1 bus */