Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1663034 > unrolled thread

[PATCH v3 0/2] PCI: iproc: SOC specific fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/2] PCI: iproc: SOC specific fixes Oza Pawandeep <oza.oza@broadcom.com> - 2017-06-11 06:10 +0200
    [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP Oza Pawandeep <oza.oza@broadcom.com> - 2017-06-11 06:10 +0200
      Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from  EP Bjorn Helgaas <helgaas@kernel.org> - 2017-06-13 01:40 +0200
        Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP Oza Oza <oza.oza@broadcom.com> - 2017-06-13 06:30 +0200
          Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP Oza Oza <oza.oza@broadcom.com> - 2017-06-13 07:50 +0200
    [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC Oza Pawandeep <oza.oza@broadcom.com> - 2017-06-11 06:10 +0200
      Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC Bjorn Helgaas <helgaas@kernel.org> - 2017-06-13 01:50 +0200
        Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC Oza Oza <oza.oza@broadcom.com> - 2017-06-14 07:00 +0200
          Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC Bjorn Helgaas <helgaas@kernel.org> - 2017-06-15 15:50 +0200

#1663034 — [PATCH v3 0/2] PCI: iproc: SOC specific fixes

PCI: iproc: Retry request when CRS returned from EP

Above patch adds support for CRS in PCI RC driver,
otherwise if not handled at lower level, the user
space PMD (poll mode drivers) can timeout.

PCI: iproc: add device shutdown for PCI RC

This fixes the issue where certian PCI endpoints are
not getting detected on Stingray SOC after reboot.

Changes Since v3:
[re-send] 
Changes Since v2:
Fix compilation errors for pcie-iproc-platform.ko
which was caught by kbuild.

Oza Pawandeep (2):
  PCI: iproc: Retry request when CRS returned from EP
  PCI: iproc: add device shutdown for PCI RC

 drivers/pci/host/pcie-iproc-platform.c |   8 +++
 drivers/pci/host/pcie-iproc.c          | 126 ++++++++++++++++++++++++++++-----
 drivers/pci/host/pcie-iproc.h          |   1 +
 3 files changed, 116 insertions(+), 19 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1663035 — [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP

For Configuration Requests only, following reset
it is possible for a device to terminate the request
but indicate that it is temporarily unable to process
the Request, but will be able to process the Request
in the future – in this case, the Configuration Request
Retry Status 10 (CRS) Completion Status is used

SPDK user space NVMe driver reinitializes NVMe which
causes reset, while doing this some configuration requests
get NAKed by Endpoint (NVMe).
Current iproc PCI driver is agnostic about it.
PAXB will forward the NAKed response in stipulated AXI code.

NVMe spec defines this timeout in 500 ms units, and this
only happens if controller has been in reset, or with new firmware,
or in abrupt shutdown case.
Meanwhile config access could result into retry.

This patch fixes the problem, and attempts to read again in case
of PAXB forwarding the NAK.

It implements iproc_pcie_config_read which gets called for Stingray.
Otherwise it falls back to PCI generic APIs.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 0f39bd2..05a3647 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -68,6 +68,9 @@
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RETRY_STATUS             0xffff0001
+#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
+
 /* derive the enum index of the outbound/inbound mapping registers */
 #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
 
@@ -448,6 +451,47 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
 	}
 }
 
+static int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+{
+	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
+	unsigned int ret;
+
+	do {
+		ret = readl(cfg_data_p);
+		if (ret == CFG_RETRY_STATUS)
+			udelay(1);
+		else
+			return PCIBIOS_SUCCESSFUL;
+	} while (timeout--);
+
+	return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
+					       unsigned int busno,
+					       unsigned int slot,
+					       unsigned int fn,
+					       int where)
+{
+	u16 offset;
+	u32 val;
+
+	/* EP device access */
+	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
+		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
+		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
+		(where & CFG_ADDR_REG_NUM_MASK) |
+		(1 & CFG_ADDR_CFG_TYPE_MASK);
+
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
+	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
+
+	if (iproc_pcie_reg_is_invalid(offset))
+		return NULL;
+
+	return (pcie->base + offset);
+}
+
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -499,13 +543,48 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
 		return (pcie->base + offset);
 }
 
+static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	unsigned int slot = PCI_SLOT(devfn);
+	unsigned int fn = PCI_FUNC(devfn);
+	unsigned int busno = bus->number;
+	void __iomem *cfg_data_p;
+	int ret;
+
+	/* root complex access. */
+	if (busno == 0)
+		return pci_generic_config_read32(bus, devfn, where, size, val);
+
+	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
+
+	if (!cfg_data_p)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	ret = iproc_pcie_cfg_retry(cfg_data_p);
+	if (ret)
+		return ret;
+
+	*val = readl(cfg_data_p);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *val)
 {
 	int ret;
+	struct iproc_pcie *pcie = iproc_data(bus);
 
 	iproc_pcie_apb_err_disable(bus, true);
-	ret = pci_generic_config_read32(bus, devfn, where, size, val);
+	if (pcie->type == IPROC_PCIE_PAXB_V2)
+		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
+	else
+		ret = pci_generic_config_read32(bus, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
 	return ret;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1664296 — Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP

Please wrap your changelogs to use 75 columns.  "git log" indents the
changelog by four spaces, so if your text is 75 wide, it will still
fit without wrapping.

On Sun, Jun 11, 2017 at 09:35:37AM +0530, Oza Pawandeep wrote:
> For Configuration Requests only, following reset
> it is possible for a device to terminate the request
> but indicate that it is temporarily unable to process
> the Request, but will be able to process the Request
> in the future – in this case, the Configuration Request
> Retry Status 10 (CRS) Completion Status is used

How does this relate to the CRS support we already have in the core,
e.g., pci_bus_read_dev_vendor_id()?  It looks like your root complex
already returns 0xffff0001 (CFG_RETRY_STATUS) in some cases.

Also, per spec (PCIe r3.1, sec 2.3.2), CRS Software Visibility only
affects config reads of the Vendor ID, but you call
iproc_pcie_cfg_retry() for all config offsets.

> SPDK user space NVMe driver reinitializes NVMe which
> causes reset, while doing this some configuration requests
> get NAKed by Endpoint (NVMe).

What's SPDK?  I don't know what "NAK" means in a PCIe context.  If you
can use the appropriate PCIe terminology, I think it will make more
sense to me.

> Current iproc PCI driver is agnostic about it.
> PAXB will forward the NAKed response in stipulated AXI code.

In general a native host bridge driver should not have to deal with
the CRS feature because it's supported in the PCI core.  So we need
some explanation about why iProc is special in this regard.

> NVMe spec defines this timeout in 500 ms units, and this
> only happens if controller has been in reset, or with new firmware,
> or in abrupt shutdown case.
> Meanwhile config access could result into retry.

I don't understand why NVMe is relevant here.  Is there something
special about NVMe and CRS?

> This patch fixes the problem, and attempts to read again in case
> of PAXB forwarding the NAK.
> 
> It implements iproc_pcie_config_read which gets called for Stingray.
> Otherwise it falls back to PCI generic APIs.
> 
> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 0f39bd2..05a3647 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -68,6 +68,9 @@
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> +#define CFG_RETRY_STATUS             0xffff0001
> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
> +
>  /* derive the enum index of the outbound/inbound mapping registers */
>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>  
> @@ -448,6 +451,47 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
>  	}
>  }
>  
> +static int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> +{
> +	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> +	unsigned int ret;
> +
> +	do {
> +		ret = readl(cfg_data_p);
> +		if (ret == CFG_RETRY_STATUS)
> +			udelay(1);
> +		else
> +			return PCIBIOS_SUCCESSFUL;
> +	} while (timeout--);
> +
> +	return PCIBIOS_DEVICE_NOT_FOUND;
> +}
> +
> +static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
> +					       unsigned int busno,
> +					       unsigned int slot,
> +					       unsigned int fn,
> +					       int where)
> +{
> +	u16 offset;
> +	u32 val;
> +
> +	/* EP device access */
> +	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
> +		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
> +		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
> +		(where & CFG_ADDR_REG_NUM_MASK) |
> +		(1 & CFG_ADDR_CFG_TYPE_MASK);
> +
> +	iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
> +	offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
> +
> +	if (iproc_pcie_reg_is_invalid(offset))
> +		return NULL;
> +
> +	return (pcie->base + offset);
> +}
> +
>  /**
>   * Note access to the configuration registers are protected at the higher layer
>   * by 'pci_lock' in drivers/pci/access.c
> @@ -499,13 +543,48 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>  		return (pcie->base + offset);
>  }
>  
> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	unsigned int slot = PCI_SLOT(devfn);
> +	unsigned int fn = PCI_FUNC(devfn);
> +	unsigned int busno = bus->number;
> +	void __iomem *cfg_data_p;
> +	int ret;
> +
> +	/* root complex access. */
> +	if (busno == 0)
> +		return pci_generic_config_read32(bus, devfn, where, size, val);
> +
> +	cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
> +
> +	if (!cfg_data_p)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	ret = iproc_pcie_cfg_retry(cfg_data_p);
> +	if (ret)
> +		return ret;
> +
> +	*val = readl(cfg_data_p);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>  static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  				    int where, int size, u32 *val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> -	ret = pci_generic_config_read32(bus, devfn, where, size, val);
> +	if (pcie->type == IPROC_PCIE_PAXB_V2)
> +		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
> +	else
> +		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>  	iproc_pcie_apb_err_disable(bus, false);
>  
>  	return ret;
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1664386 — Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP

On Tue, Jun 13, 2017 at 5:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Please wrap your changelogs to use 75 columns.  "git log" indents the
> changelog by four spaces, so if your text is 75 wide, it will still
> fit without wrapping.
>
> On Sun, Jun 11, 2017 at 09:35:37AM +0530, Oza Pawandeep wrote:
>> For Configuration Requests only, following reset
>> it is possible for a device to terminate the request
>> but indicate that it is temporarily unable to process
>> the Request, but will be able to process the Request
>> in the future – in this case, the Configuration Request
>> Retry Status 10 (CRS) Completion Status is used
>
> How does this relate to the CRS support we already have in the core,
> e.g., pci_bus_read_dev_vendor_id()?  It looks like your root complex
> already returns 0xffff0001 (CFG_RETRY_STATUS) in some cases.
>
> Also, per spec (PCIe r3.1, sec 2.3.2), CRS Software Visibility only
> affects config reads of the Vendor ID, but you call
> iproc_pcie_cfg_retry() for all config offsets.

Yes, as per Spec, CRS Software Visibility only affects config read of
the Vendor ID.
For config write or any other config read the Root must automatically
re-issue configuration
request again as a new request, and our PCIe RC fails to do so.

>
>> SPDK user space NVMe driver reinitializes NVMe which
>> causes reset, while doing this some configuration requests
>> get NAKed by Endpoint (NVMe).
>
> What's SPDK?  I don't know what "NAK" means in a PCIe context.  If you
> can use the appropriate PCIe terminology, I think it will make more
> sense to me.

when I meant NAK, I meant CRS, will change the description, and will take
care of using appropriate PCIe terminology.

>
>> Current iproc PCI driver is agnostic about it.
>> PAXB will forward the NAKed response in stipulated AXI code.
>
> In general a native host bridge driver should not have to deal with
> the CRS feature because it's supported in the PCI core.  So we need
> some explanation about why iProc is special in this regard.
>

For config write or any other config read the Root must automatically
re-issue configuration
request again as a new request, iproc based PCIe RC does not adhere to
this, and also
our PCI-to-AXI bridge (internal), which returns code 0xffff0001 to CPU.

>> NVMe spec defines this timeout in 500 ms units, and this
>> only happens if controller has been in reset, or with new firmware,
>> or in abrupt shutdown case.
>> Meanwhile config access could result into retry.
>
> I don't understand why NVMe is relevant here.  Is there something
> special about NVMe and CRS?
>

You are right, NVMe spec is irrelevant here, but since whole
exercise was carried out with NVMe and our major use cases are NVMe,
I ended up mentioning that. I can remove that from description.

>> This patch fixes the problem, and attempts to read again in case
>> of PAXB forwarding the NAK.
>>
>> It implements iproc_pcie_config_read which gets called for Stingray.
>> Otherwise it falls back to PCI generic APIs.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 0f39bd2..05a3647 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -68,6 +68,9 @@
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> +#define CFG_RETRY_STATUS             0xffff0001
>> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>> +
>>  /* derive the enum index of the outbound/inbound mapping registers */
>>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>>
>> @@ -448,6 +451,47 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
>>       }
>>  }
>>
>> +static int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>> +{
>> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>> +     unsigned int ret;
>> +
>> +     do {
>> +             ret = readl(cfg_data_p);
>> +             if (ret == CFG_RETRY_STATUS)
>> +                     udelay(1);
>> +             else
>> +                     return PCIBIOS_SUCCESSFUL;
>> +     } while (timeout--);
>> +
>> +     return PCIBIOS_DEVICE_NOT_FOUND;
>> +}
>> +
>> +static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>> +                                            unsigned int busno,
>> +                                            unsigned int slot,
>> +                                            unsigned int fn,
>> +                                            int where)
>> +{
>> +     u16 offset;
>> +     u32 val;
>> +
>> +     /* EP device access */
>> +     val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
>> +             (slot << CFG_ADDR_DEV_NUM_SHIFT) |
>> +             (fn << CFG_ADDR_FUNC_NUM_SHIFT) |
>> +             (where & CFG_ADDR_REG_NUM_MASK) |
>> +             (1 & CFG_ADDR_CFG_TYPE_MASK);
>> +
>> +     iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
>> +     offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
>> +
>> +     if (iproc_pcie_reg_is_invalid(offset))
>> +             return NULL;
>> +
>> +     return (pcie->base + offset);
>> +}
>> +
>>  /**
>>   * Note access to the configuration registers are protected at the higher layer
>>   * by 'pci_lock' in drivers/pci/access.c
>> @@ -499,13 +543,48 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>>               return (pcie->base + offset);
>>  }
>>
>> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>> +                                 int where, int size, u32 *val)
>> +{
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>> +     unsigned int slot = PCI_SLOT(devfn);
>> +     unsigned int fn = PCI_FUNC(devfn);
>> +     unsigned int busno = bus->number;
>> +     void __iomem *cfg_data_p;
>> +     int ret;
>> +
>> +     /* root complex access. */
>> +     if (busno == 0)
>> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>> +
>> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>> +
>> +     if (!cfg_data_p)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +     ret = iproc_pcie_cfg_retry(cfg_data_p);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *val = readl(cfg_data_p);
>> +
>> +     if (size <= 2)
>> +             *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>>                                   int where, int size, u32 *val)
>>  {
>>       int ret;
>> +     struct iproc_pcie *pcie = iproc_data(bus);
>>
>>       iproc_pcie_apb_err_disable(bus, true);
>> -     ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>> +     else
>> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>       iproc_pcie_apb_err_disable(bus, false);
>>
>>       return ret;
>> --
>> 1.9.1
>>

[toc] | [prev] | [next] | [standalone]

#1664449 — Re: [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP

On Tue, Jun 13, 2017 at 9:58 AM, Oza Oza <oza.oza@broadcom.com> wrote:
> On Tue, Jun 13, 2017 at 5:00 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> Please wrap your changelogs to use 75 columns.  "git log" indents the
>> changelog by four spaces, so if your text is 75 wide, it will still
>> fit without wrapping.
>>
>> On Sun, Jun 11, 2017 at 09:35:37AM +0530, Oza Pawandeep wrote:
>>> For Configuration Requests only, following reset
>>> it is possible for a device to terminate the request
>>> but indicate that it is temporarily unable to process
>>> the Request, but will be able to process the Request
>>> in the future – in this case, the Configuration Request
>>> Retry Status 10 (CRS) Completion Status is used
>>
>> How does this relate to the CRS support we already have in the core,
>> e.g., pci_bus_read_dev_vendor_id()?  It looks like your root complex
>> already returns 0xffff0001 (CFG_RETRY_STATUS) in some cases.
>>
>> Also, per spec (PCIe r3.1, sec 2.3.2), CRS Software Visibility only
>> affects config reads of the Vendor ID, but you call
>> iproc_pcie_cfg_retry() for all config offsets.
>
> Yes, as per Spec, CRS Software Visibility only affects config read of
> the Vendor ID.
> For config write or any other config read the Root must automatically
> re-issue configuration
> request again as a new request, and our PCIe RC fails to do so.
>
>>
>>> SPDK user space NVMe driver reinitializes NVMe which
>>> causes reset, while doing this some configuration requests
>>> get NAKed by Endpoint (NVMe).
>>
>> What's SPDK?  I don't know what "NAK" means in a PCIe context.  If you
>> can use the appropriate PCIe terminology, I think it will make more
>> sense to me.

SPDK supports user space poll mode driver, and along with DPDK
interface with vfio
to directly map PCIe resources to user space.
the reason I mentioned SPDK, because it exposes this bug in our PCIe RC.

>
> when I meant NAK, I meant CRS, will change the description, and will take
> care of using appropriate PCIe terminology.
>
>>
>>> Current iproc PCI driver is agnostic about it.
>>> PAXB will forward the NAKed response in stipulated AXI code.
>>
>> In general a native host bridge driver should not have to deal with
>> the CRS feature because it's supported in the PCI core.  So we need
>> some explanation about why iProc is special in this regard.
>>
>
> For config write or any other config read the Root must automatically
> re-issue configuration
> request again as a new request, iproc based PCIe RC does not adhere to
> this, and also
> our PCI-to-AXI bridge (internal), which returns code 0xffff0001 to CPU.
>
>>> NVMe spec defines this timeout in 500 ms units, and this
>>> only happens if controller has been in reset, or with new firmware,
>>> or in abrupt shutdown case.
>>> Meanwhile config access could result into retry.
>>
>> I don't understand why NVMe is relevant here.  Is there something
>> special about NVMe and CRS?
>>
>
> You are right, NVMe spec is irrelevant here, but since whole
> exercise was carried out with NVMe and our major use cases are NVMe,
> I ended up mentioning that. I can remove that from description.
>
>>> This patch fixes the problem, and attempts to read again in case
>>> of PAXB forwarding the NAK.
>>>
>>> It implements iproc_pcie_config_read which gets called for Stingray.
>>> Otherwise it falls back to PCI generic APIs.
>>>
>>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>> index 0f39bd2..05a3647 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -68,6 +68,9 @@
>>>  #define APB_ERR_EN_SHIFT             0
>>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>>
>>> +#define CFG_RETRY_STATUS             0xffff0001
>>> +#define CFG_RETRY_STATUS_TIMEOUT_US  500000 /* 500 milli-seconds. */
>>> +
>>>  /* derive the enum index of the outbound/inbound mapping registers */
>>>  #define MAP_REG(base_reg, index)      ((base_reg) + (index) * 2)
>>>
>>> @@ -448,6 +451,47 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
>>>       }
>>>  }
>>>
>>> +static int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
>>> +{
>>> +     int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
>>> +     unsigned int ret;
>>> +
>>> +     do {
>>> +             ret = readl(cfg_data_p);
>>> +             if (ret == CFG_RETRY_STATUS)
>>> +                     udelay(1);
>>> +             else
>>> +                     return PCIBIOS_SUCCESSFUL;
>>> +     } while (timeout--);
>>> +
>>> +     return PCIBIOS_DEVICE_NOT_FOUND;
>>> +}
>>> +
>>> +static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
>>> +                                            unsigned int busno,
>>> +                                            unsigned int slot,
>>> +                                            unsigned int fn,
>>> +                                            int where)
>>> +{
>>> +     u16 offset;
>>> +     u32 val;
>>> +
>>> +     /* EP device access */
>>> +     val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
>>> +             (slot << CFG_ADDR_DEV_NUM_SHIFT) |
>>> +             (fn << CFG_ADDR_FUNC_NUM_SHIFT) |
>>> +             (where & CFG_ADDR_REG_NUM_MASK) |
>>> +             (1 & CFG_ADDR_CFG_TYPE_MASK);
>>> +
>>> +     iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
>>> +     offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
>>> +
>>> +     if (iproc_pcie_reg_is_invalid(offset))
>>> +             return NULL;
>>> +
>>> +     return (pcie->base + offset);
>>> +}
>>> +
>>>  /**
>>>   * Note access to the configuration registers are protected at the higher layer
>>>   * by 'pci_lock' in drivers/pci/access.c
>>> @@ -499,13 +543,48 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>>>               return (pcie->base + offset);
>>>  }
>>>
>>> +static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>>> +                                 int where, int size, u32 *val)
>>> +{
>>> +     struct iproc_pcie *pcie = iproc_data(bus);
>>> +     unsigned int slot = PCI_SLOT(devfn);
>>> +     unsigned int fn = PCI_FUNC(devfn);
>>> +     unsigned int busno = bus->number;
>>> +     void __iomem *cfg_data_p;
>>> +     int ret;
>>> +
>>> +     /* root complex access. */
>>> +     if (busno == 0)
>>> +             return pci_generic_config_read32(bus, devfn, where, size, val);
>>> +
>>> +     cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn, where);
>>> +
>>> +     if (!cfg_data_p)
>>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>>> +
>>> +     ret = iproc_pcie_cfg_retry(cfg_data_p);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     *val = readl(cfg_data_p);
>>> +
>>> +     if (size <= 2)
>>> +             *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>>> +
>>> +     return PCIBIOS_SUCCESSFUL;
>>> +}
>>> +
>>>  static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>>>                                   int where, int size, u32 *val)
>>>  {
>>>       int ret;
>>> +     struct iproc_pcie *pcie = iproc_data(bus);
>>>
>>>       iproc_pcie_apb_err_disable(bus, true);
>>> -     ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>> +     if (pcie->type == IPROC_PCIE_PAXB_V2)
>>> +             ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>>> +     else
>>> +             ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>>       iproc_pcie_apb_err_disable(bus, false);
>>>
>>>       return ret;
>>> --
>>> 1.9.1
>>>

[toc] | [prev] | [next] | [standalone]

#1663036 — [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC

PERST# must be asserted around ~500ms before
the reboot is applied.

During soft reset (e.g., "reboot" from Linux) on some iProc based SoCs
LCPLL clock and PERST both goes off simultaneously.
This will cause certain Endpoints Intel NVMe not get detected, upon
next boot sequence.

This happens because; Endpoint is expecting the clock for some amount of
time after PERST is asserted, which is not happening in our case
(Compare to Intel X86 boards, will have clocks running).
this cause NVMe to behave in undefined way.

Essentially clock will remain alive for 500ms with PERST# = 0
before reboot.

This patch adds platform shutdown where it should be
called in device_shutdown while reboot command is issued.

So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
followed by RC shutdown is called, which issues safe PERST
assertion.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index 90d2bdd..9512960 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -131,6 +131,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	return iproc_pcie_remove(pcie);
 }
 
+static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
+{
+	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
+
+	iproc_pcie_shutdown(pcie);
+}
+
 static struct platform_driver iproc_pcie_pltfm_driver = {
 	.driver = {
 		.name = "iproc-pcie",
@@ -138,6 +145,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
 	},
 	.probe = iproc_pcie_pltfm_probe,
 	.remove = iproc_pcie_pltfm_remove,
+	.shutdown = iproc_pcie_pltfm_shutdown,
 };
 module_platform_driver(iproc_pcie_pltfm_driver);
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 05a3647..bb61376 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -608,32 +608,40 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
 	.write = iproc_pcie_config_write32,
 };
 
-static void iproc_pcie_reset(struct iproc_pcie *pcie)
+static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
 {
 	u32 val;
 
 	/*
-	 * PAXC and the internal emulated endpoint device downstream should not
-	 * be reset.  If firmware has been loaded on the endpoint device at an
-	 * earlier boot stage, reset here causes issues.
+	 * The internal emulated endpoints (such as PAXC) device downstream
+	 * should not be reset. If firmware has been loaded on the endpoint
+	 * device at an earlier boot stage, reset here causes issues.
 	 */
 	if (pcie->ep_is_internal)
 		return;
 
-	/*
-	 * Select perst_b signal as reset source. Put the device into reset,
-	 * and then bring it out of reset
-	 */
-	val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
-	val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
-		~RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	udelay(250);
-
-	val |= RC_PCIE_RST_OUTPUT;
-	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
-	msleep(100);
+	if (assert) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
+			~RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		udelay(250);
+	} else {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+		val |= RC_PCIE_RST_OUTPUT;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+		msleep(100);
+	}
+}
+
+int iproc_pcie_shutdown(struct iproc_pcie *pcie)
+{
+	iproc_pcie_perst_ctrl(pcie, true);
+	msleep(500);
+
+	return 0;
 }
+EXPORT_SYMBOL(iproc_pcie_shutdown);
 
 static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
 {
@@ -1310,7 +1318,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
-	iproc_pcie_reset(pcie);
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
 
 	if (pcie->need_ob_cfg) {
 		ret = iproc_pcie_map_ranges(pcie, res);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 0bbe2ea..a6b55ce 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -110,6 +110,7 @@ struct iproc_pcie {
 
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
+int iproc_pcie_shutdown(struct iproc_pcie *pcie);
 
 #ifdef CONFIG_PCIE_IPROC_MSI
 int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1664301 — Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC

On Sun, Jun 11, 2017 at 09:35:38AM +0530, Oza Pawandeep wrote:
> PERST# must be asserted around ~500ms before
> the reboot is applied.
> 
> During soft reset (e.g., "reboot" from Linux) on some iProc based SoCs
> LCPLL clock and PERST both goes off simultaneously.
> This will cause certain Endpoints Intel NVMe not get detected, upon
> next boot sequence.
> 
> This happens because; Endpoint is expecting the clock for some amount of
> time after PERST is asserted, which is not happening in our case
> (Compare to Intel X86 boards, will have clocks running).
> this cause NVMe to behave in undefined way.

This doesn't smell right.  The description makes this sound like a
band-aid that happens to "solve" a problem with one particular device.
It doesn't sound like this is making iProc adhere to something in the
PCIe spec.

Is there some PCIe spec timing requirement that tells you about this
500ms number?  Please include the reference if so.

> Essentially clock will remain alive for 500ms with PERST# = 0
> before reboot.

> This patch adds platform shutdown where it should be
> called in device_shutdown while reboot command is issued.
> 
> So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
> followed by RC shutdown is called, which issues safe PERST
> assertion.
> 
> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index 90d2bdd..9512960 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -131,6 +131,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
>  	return iproc_pcie_remove(pcie);
>  }
>  
> +static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
> +{
> +	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	iproc_pcie_shutdown(pcie);
> +}
> +
>  static struct platform_driver iproc_pcie_pltfm_driver = {
>  	.driver = {
>  		.name = "iproc-pcie",
> @@ -138,6 +145,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
>  	},
>  	.probe = iproc_pcie_pltfm_probe,
>  	.remove = iproc_pcie_pltfm_remove,
> +	.shutdown = iproc_pcie_pltfm_shutdown,
>  };
>  module_platform_driver(iproc_pcie_pltfm_driver);
>  
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 05a3647..bb61376 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -608,32 +608,40 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>  	.write = iproc_pcie_config_write32,
>  };
>  
> -static void iproc_pcie_reset(struct iproc_pcie *pcie)
> +static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
>  {
>  	u32 val;
>  
>  	/*
> -	 * PAXC and the internal emulated endpoint device downstream should not
> -	 * be reset.  If firmware has been loaded on the endpoint device at an
> -	 * earlier boot stage, reset here causes issues.
> +	 * The internal emulated endpoints (such as PAXC) device downstream
> +	 * should not be reset. If firmware has been loaded on the endpoint
> +	 * device at an earlier boot stage, reset here causes issues.
>  	 */
>  	if (pcie->ep_is_internal)
>  		return;
>  
> -	/*
> -	 * Select perst_b signal as reset source. Put the device into reset,
> -	 * and then bring it out of reset
> -	 */
> -	val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> -	val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
> -		~RC_PCIE_RST_OUTPUT;
> -	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> -	udelay(250);
> -
> -	val |= RC_PCIE_RST_OUTPUT;
> -	iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> -	msleep(100);
> +	if (assert) {
> +		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> +		val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
> +			~RC_PCIE_RST_OUTPUT;
> +		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> +		udelay(250);
> +	} else {
> +		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> +		val |= RC_PCIE_RST_OUTPUT;
> +		iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> +		msleep(100);
> +	}
> +}
> +
> +int iproc_pcie_shutdown(struct iproc_pcie *pcie)
> +{
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	msleep(500);
> +
> +	return 0;
>  }
> +EXPORT_SYMBOL(iproc_pcie_shutdown);
>  
>  static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>  {
> @@ -1310,7 +1318,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		goto err_exit_phy;
>  	}
>  
> -	iproc_pcie_reset(pcie);
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	iproc_pcie_perst_ctrl(pcie, false);
>  
>  	if (pcie->need_ob_cfg) {
>  		ret = iproc_pcie_map_ranges(pcie, res);
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index 0bbe2ea..a6b55ce 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -110,6 +110,7 @@ struct iproc_pcie {
>  
>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
>  int iproc_pcie_remove(struct iproc_pcie *pcie);
> +int iproc_pcie_shutdown(struct iproc_pcie *pcie);
>  
>  #ifdef CONFIG_PCIE_IPROC_MSI
>  int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1665420 — Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC

On Tue, Jun 13, 2017 at 5:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sun, Jun 11, 2017 at 09:35:38AM +0530, Oza Pawandeep wrote:
>> PERST# must be asserted around ~500ms before
>> the reboot is applied.
>>
>> During soft reset (e.g., "reboot" from Linux) on some iProc based SoCs
>> LCPLL clock and PERST both goes off simultaneously.
>> This will cause certain Endpoints Intel NVMe not get detected, upon
>> next boot sequence.
>>
>> This happens because; Endpoint is expecting the clock for some amount of
>> time after PERST is asserted, which is not happening in our case
>> (Compare to Intel X86 boards, will have clocks running).
>> this cause NVMe to behave in undefined way.
>
> This doesn't smell right.  The description makes this sound like a
> band-aid that happens to "solve" a problem with one particular device.
> It doesn't sound like this is making iProc adhere to something in the
> PCIe spec.
>
> Is there some PCIe spec timing requirement that tells you about this
> 500ms number?  Please include the reference if so.

On chip PLL which sources the reference clock to the device gets reset
when soft reset is applied; the soft reset also asserts PERST#.
This simultaneous assertion of reset and clock being lost is a problem
with some NVME cards.
The delay is added to keep clock alive while PERST gets asserted.
The time delay number can be set to a number that will allow the NVME
device to go into reset state.
500 ms delay is picked for that reason only, which is long enough to
get EP into reset correctly.

>
>> Essentially clock will remain alive for 500ms with PERST# = 0
>> before reboot.
>
>> This patch adds platform shutdown where it should be
>> called in device_shutdown while reboot command is issued.
>>
>> So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
>> followed by RC shutdown is called, which issues safe PERST
>> assertion.
>>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>> index 90d2bdd..9512960 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -131,6 +131,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
>>       return iproc_pcie_remove(pcie);
>>  }
>>
>> +static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
>> +{
>> +     struct iproc_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +     iproc_pcie_shutdown(pcie);
>> +}
>> +
>>  static struct platform_driver iproc_pcie_pltfm_driver = {
>>       .driver = {
>>               .name = "iproc-pcie",
>> @@ -138,6 +145,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
>>       },
>>       .probe = iproc_pcie_pltfm_probe,
>>       .remove = iproc_pcie_pltfm_remove,
>> +     .shutdown = iproc_pcie_pltfm_shutdown,
>>  };
>>  module_platform_driver(iproc_pcie_pltfm_driver);
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 05a3647..bb61376 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -608,32 +608,40 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>>       .write = iproc_pcie_config_write32,
>>  };
>>
>> -static void iproc_pcie_reset(struct iproc_pcie *pcie)
>> +static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
>>  {
>>       u32 val;
>>
>>       /*
>> -      * PAXC and the internal emulated endpoint device downstream should not
>> -      * be reset.  If firmware has been loaded on the endpoint device at an
>> -      * earlier boot stage, reset here causes issues.
>> +      * The internal emulated endpoints (such as PAXC) device downstream
>> +      * should not be reset. If firmware has been loaded on the endpoint
>> +      * device at an earlier boot stage, reset here causes issues.
>>        */
>>       if (pcie->ep_is_internal)
>>               return;
>>
>> -     /*
>> -      * Select perst_b signal as reset source. Put the device into reset,
>> -      * and then bring it out of reset
>> -      */
>> -     val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
>> -     val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
>> -             ~RC_PCIE_RST_OUTPUT;
>> -     iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
>> -     udelay(250);
>> -
>> -     val |= RC_PCIE_RST_OUTPUT;
>> -     iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
>> -     msleep(100);
>> +     if (assert) {
>> +             val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
>> +             val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
>> +                     ~RC_PCIE_RST_OUTPUT;
>> +             iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
>> +             udelay(250);
>> +     } else {
>> +             val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
>> +             val |= RC_PCIE_RST_OUTPUT;
>> +             iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
>> +             msleep(100);
>> +     }
>> +}
>> +
>> +int iproc_pcie_shutdown(struct iproc_pcie *pcie)
>> +{
>> +     iproc_pcie_perst_ctrl(pcie, true);
>> +     msleep(500);
>> +
>> +     return 0;
>>  }
>> +EXPORT_SYMBOL(iproc_pcie_shutdown);
>>
>>  static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>>  {
>> @@ -1310,7 +1318,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>               goto err_exit_phy;
>>       }
>>
>> -     iproc_pcie_reset(pcie);
>> +     iproc_pcie_perst_ctrl(pcie, true);
>> +     iproc_pcie_perst_ctrl(pcie, false);
>>
>>       if (pcie->need_ob_cfg) {
>>               ret = iproc_pcie_map_ranges(pcie, res);
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> index 0bbe2ea..a6b55ce 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -110,6 +110,7 @@ struct iproc_pcie {
>>
>>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
>>  int iproc_pcie_remove(struct iproc_pcie *pcie);
>> +int iproc_pcie_shutdown(struct iproc_pcie *pcie);
>>
>>  #ifdef CONFIG_PCIE_IPROC_MSI
>>  int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
>> --
>> 1.9.1
>>

[toc] | [prev] | [next] | [standalone]

#1666736 — Re: [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC

On Wed, Jun 14, 2017 at 10:24:11AM +0530, Oza Oza wrote:
> On Tue, Jun 13, 2017 at 5:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Jun 11, 2017 at 09:35:38AM +0530, Oza Pawandeep wrote:
> >> PERST# must be asserted around ~500ms before
> >> the reboot is applied.
> >>
> >> During soft reset (e.g., "reboot" from Linux) on some iProc based SoCs
> >> LCPLL clock and PERST both goes off simultaneously.
> >> This will cause certain Endpoints Intel NVMe not get detected, upon
> >> next boot sequence.
> >>
> >> This happens because; Endpoint is expecting the clock for some amount of
> >> time after PERST is asserted, which is not happening in our case
> >> (Compare to Intel X86 boards, will have clocks running).
> >> this cause NVMe to behave in undefined way.
> >
> > This doesn't smell right.  The description makes this sound like a
> > band-aid that happens to "solve" a problem with one particular device.
> > It doesn't sound like this is making iProc adhere to something in the
> > PCIe spec.
> >
> > Is there some PCIe spec timing requirement that tells you about this
> > 500ms number?  Please include the reference if so.
> 
> On chip PLL which sources the reference clock to the device gets reset
> when soft reset is applied; the soft reset also asserts PERST#.
> This simultaneous assertion of reset and clock being lost is a problem
> with some NVME cards.
> The delay is added to keep clock alive while PERST gets asserted.
> The time delay number can be set to a number that will allow the NVME
> device to go into reset state.
> 500 ms delay is picked for that reason only, which is long enough to
> get EP into reset correctly.

This doesn't really tell me anything new.

We're talking about the protocol on the link between the RC and the
endpoint.  That *should* be completely specified by the PCIe spec.  If
there's some requirement that the clock keep running after PERST is
asserted, that should be in the spec.

If it's not in the spec, and an endpoint relies on it, the endpoint is
non-compliant, and we'll likely see similar issues with that endpoint
on other platforms.

If we need a workaround for a specific endpoint, that might be OK; the
world isn't perfect.  But if that's the case, we should include more
specifics about the device, e.g., vendor/device IDs and "lspci -vv"
output.

> >> Essentially clock will remain alive for 500ms with PERST# = 0
> >> before reboot.
> >
> >> This patch adds platform shutdown where it should be
> >> called in device_shutdown while reboot command is issued.
> >>
> >> So in sequence first Endpoint Shutdown (e.g. nvme_shutdown)
> >> followed by RC shutdown is called, which issues safe PERST
> >> assertion.
> >>
> >> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> >> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> >> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> >>
> >> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> >> index 90d2bdd..9512960 100644
> >> --- a/drivers/pci/host/pcie-iproc-platform.c
> >> +++ b/drivers/pci/host/pcie-iproc-platform.c
> >> @@ -131,6 +131,13 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
> >>       return iproc_pcie_remove(pcie);
> >>  }
> >>
> >> +static void iproc_pcie_pltfm_shutdown(struct platform_device *pdev)
> >> +{
> >> +     struct iproc_pcie *pcie = platform_get_drvdata(pdev);
> >> +
> >> +     iproc_pcie_shutdown(pcie);
> >> +}
> >> +
> >>  static struct platform_driver iproc_pcie_pltfm_driver = {
> >>       .driver = {
> >>               .name = "iproc-pcie",
> >> @@ -138,6 +145,7 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
> >>       },
> >>       .probe = iproc_pcie_pltfm_probe,
> >>       .remove = iproc_pcie_pltfm_remove,
> >> +     .shutdown = iproc_pcie_pltfm_shutdown,
> >>  };
> >>  module_platform_driver(iproc_pcie_pltfm_driver);
> >>
> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> >> index 05a3647..bb61376 100644
> >> --- a/drivers/pci/host/pcie-iproc.c
> >> +++ b/drivers/pci/host/pcie-iproc.c
> >> @@ -608,32 +608,40 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
> >>       .write = iproc_pcie_config_write32,
> >>  };
> >>
> >> -static void iproc_pcie_reset(struct iproc_pcie *pcie)
> >> +static void iproc_pcie_perst_ctrl(struct iproc_pcie *pcie, bool assert)
> >>  {
> >>       u32 val;
> >>
> >>       /*
> >> -      * PAXC and the internal emulated endpoint device downstream should not
> >> -      * be reset.  If firmware has been loaded on the endpoint device at an
> >> -      * earlier boot stage, reset here causes issues.
> >> +      * The internal emulated endpoints (such as PAXC) device downstream
> >> +      * should not be reset. If firmware has been loaded on the endpoint
> >> +      * device at an earlier boot stage, reset here causes issues.
> >>        */
> >>       if (pcie->ep_is_internal)
> >>               return;
> >>
> >> -     /*
> >> -      * Select perst_b signal as reset source. Put the device into reset,
> >> -      * and then bring it out of reset
> >> -      */
> >> -     val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> >> -     val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
> >> -             ~RC_PCIE_RST_OUTPUT;
> >> -     iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> >> -     udelay(250);
> >> -
> >> -     val |= RC_PCIE_RST_OUTPUT;
> >> -     iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> >> -     msleep(100);
> >> +     if (assert) {
> >> +             val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> >> +             val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
> >> +                     ~RC_PCIE_RST_OUTPUT;
> >> +             iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> >> +             udelay(250);
> >> +     } else {
> >> +             val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
> >> +             val |= RC_PCIE_RST_OUTPUT;
> >> +             iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
> >> +             msleep(100);
> >> +     }
> >> +}
> >> +
> >> +int iproc_pcie_shutdown(struct iproc_pcie *pcie)
> >> +{
> >> +     iproc_pcie_perst_ctrl(pcie, true);
> >> +     msleep(500);
> >> +
> >> +     return 0;
> >>  }
> >> +EXPORT_SYMBOL(iproc_pcie_shutdown);
> >>
> >>  static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
> >>  {
> >> @@ -1310,7 +1318,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >>               goto err_exit_phy;
> >>       }
> >>
> >> -     iproc_pcie_reset(pcie);
> >> +     iproc_pcie_perst_ctrl(pcie, true);
> >> +     iproc_pcie_perst_ctrl(pcie, false);
> >>
> >>       if (pcie->need_ob_cfg) {
> >>               ret = iproc_pcie_map_ranges(pcie, res);
> >> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> >> index 0bbe2ea..a6b55ce 100644
> >> --- a/drivers/pci/host/pcie-iproc.h
> >> +++ b/drivers/pci/host/pcie-iproc.h
> >> @@ -110,6 +110,7 @@ struct iproc_pcie {
> >>
> >>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
> >>  int iproc_pcie_remove(struct iproc_pcie *pcie);
> >> +int iproc_pcie_shutdown(struct iproc_pcie *pcie);
> >>
> >>  #ifdef CONFIG_PCIE_IPROC_MSI
> >>  int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
> >> --
> >> 1.9.1
> >>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web