[V3,00/16] Add Tegra194 PCIe support
mbox series

Message ID 20190416192730.15681-1-vidyas@nvidia.com
Headers show
Series
  • Add Tegra194 PCIe support
Related show

Message

Vidya Sagar April 16, 2019, 7:27 p.m. UTC
Tegra194 has six PCIe controllers based on Synopsys DesignWare core.
There are two Universal PHY (UPHY) blocks with each supporting 12(HSIO:
Hisg Speed IO) and 8(NVHS: NVIDIA High Speed) lanes respectively.
Controllers:0~4 use UPHY lanes from HSIO brick whereas Controller:5 uses
UPHY lanes from NVHS brick. Lane mapping in HSIO UPHY brick to each PCIe
controller (0~4) is controlled in XBAR module by BPMP-FW. Since PCIe
core has PIPE interface, a glue module called PIPE-to-UPHY (P2U) is used
to connect each UPHY lane (applicable to both HSIO and NVHS UPHY bricks)
to PCIe controller
This patch series
- Adds support for P2U PHY driver
- Adds support for PCIe host controller
- Adds device tree nodes each PCIe controllers
- Enables nodes applicable to p2972-0000 platform
- Adds helper APIs in Designware core driver to get capability regs offset
- Adds defines for new feature registers of PCIe spec revision 4
- Makes changes in DesignWare core driver to get Tegra194 PCIe working

Testing done on P2972-0000 platform
- Able to get PCIe link up with on-board Marvel eSATA controller
- Able to get PCIe link up with NVMe cards connected to M.2 Key-M slot
- Able to do data transfers with both SATA drives and NVMe cards

Note
- Enabling x8 slot on P2972-0000 platform requires pinmux driver for Tegra194.
  It is being worked on currently and hence Controller:5 (i.e. x8 slot) is
  disabled in this patch series. A future patch series would enable this.
- This series is based on top of the following series
  Jisheng's patches to add support to .remove() in Designware sub-system
  https://patchwork.kernel.org/project/linux-pci/list/?series=98559
  My patches made on top of Jisheng's patches to export various symbols
  https://patchwork.kernel.org/project/linux-pci/list/?series=101259

Changes since [v2]:
* Addressed review comments from Thierry

Changes since [v1]:
* Addressed review comments from Bjorn, Thierry, Jonathan, Rob & Kishon
* Added more patches in v2 series

Vidya Sagar (16):
  PCI: Add #defines for some of PCIe spec r4.0 features
  PCI/PME: Export pcie_pme_disable_msi() & pcie_pme_no_msi() APIs
  PCI: Export pcie_bus_config symbol
  PCI: dwc: Perform dbi regs write lock towards the end
  PCI: dwc: Move config space capability search API
  PCI: dwc: Add ext config space capability search API
  dt-bindings: PCI: designware: Add binding for CDM register check
  PCI: dwc: Add support to enable CDM register check
  Documentation/devicetree: Add PCIe supports-clkreq property
  dt-bindings: PCI: tegra: Add device tree support for T194
  dt-bindings: PHY: P2U: Add Tegra 194 P2U block
  arm64: tegra: Add P2U and PCIe controller nodes to Tegra194 DT
  arm64: tegra: Enable PCIe slots in P2972-0000 board
  phy: tegra: Add PCIe PIPE2UPHY support
  PCI: tegra: Add Tegra194 PCIe support
  arm64: Add Tegra194 PCIe driver to defconfig

 .../bindings/pci/designware-pcie.txt          |    5 +
 .../bindings/pci/nvidia,tegra194-pcie.txt     |  187 ++
 Documentation/devicetree/bindings/pci/pci.txt |    5 +
 .../bindings/phy/phy-tegra194-p2u.txt         |   28 +
 .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi |    2 +-
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   |   41 +
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  449 +++++
 arch/arm64/configs/defconfig                  |    1 +
 drivers/pci/controller/dwc/Kconfig            |   11 +
 drivers/pci/controller/dwc/Makefile           |    1 +
 .../pci/controller/dwc/pcie-designware-ep.c   |   37 +-
 .../pci/controller/dwc/pcie-designware-host.c |    3 -
 drivers/pci/controller/dwc/pcie-designware.c  |   81 +
 drivers/pci/controller/dwc/pcie-designware.h  |   12 +
 drivers/pci/controller/dwc/pcie-tegra194.c    | 1760 +++++++++++++++++
 drivers/pci/pci.c                             |    1 +
 drivers/pci/pcie/pme.c                        |   14 +-
 drivers/pci/pcie/portdrv.h                    |   16 +-
 drivers/phy/tegra/Kconfig                     |    7 +
 drivers/phy/tegra/Makefile                    |    1 +
 drivers/phy/tegra/pcie-p2u-tegra194.c         |  120 ++
 include/uapi/linux/pci_regs.h                 |   22 +-
 22 files changed, 2750 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
 create mode 100644 Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt
 create mode 100644 drivers/pci/controller/dwc/pcie-tegra194.c
 create mode 100644 drivers/phy/tegra/pcie-p2u-tegra194.c

Comments

Gustavo Pimentel April 17, 2019, 9:27 a.m. UTC | #1
On Tue, Apr 16, 2019 at 20:27:20, Vidya Sagar <vidyas@nvidia.com> wrote:

> Add extended configuration space capability search API using struct dw_pcie *
> pointer
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> Changes from [v2]:
> * None
> 
> Changes from [v1]:
> * This is a new patch in v2 series
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 41 ++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index d68c123e409c..44c0ba078452 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -53,6 +53,47 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>  	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>  }
>  
> +static int dw_pcie_find_next_ext_capability(struct dw_pcie *pci, int start,
> +					    int cap)
> +{
> +	u32 header;
> +	int ttl;
> +	int pos = PCI_CFG_SPACE_SIZE;
> +
> +	/* minimum 8 bytes per capability */
> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> +
> +	if (start)
> +		pos = start;
> +
> +	header = dw_pcie_readl_dbi(pci, pos);
> +	/*
> +	 * If we have no capabilities, this is indicated by cap ID,
> +	 * cap version and next pointer all being 0.
> +	 */
> +	if (header == 0)
> +		return 0;
> +
> +	while (ttl-- > 0) {
> +		if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> +			return pos;
> +
> +		pos = PCI_EXT_CAP_NEXT(header);
> +		if (pos < PCI_CFG_SPACE_SIZE)
> +			break;
> +
> +		header = dw_pcie_readl_dbi(pci, pos);
> +	}
> +
> +	return 0;
> +}
> +
> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap)
> +{
> +	return dw_pcie_find_next_ext_capability(pci, 0, cap);
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
> +
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>  {
>  	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 4ccd4c706ddb..fa41d675c48f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -248,6 +248,7 @@ struct dw_pcie {
>  		container_of((endpoint), struct dw_pcie, ep)
>  
>  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap);
>  
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>  int dw_pcie_write(void __iomem *addr, int size, u32 val);
> -- 
> 2.17.1

This ext capability function is aimed to be used by the EP also?
Gustavo Pimentel April 17, 2019, 9:28 a.m. UTC | #2
On Tue, Apr 16, 2019 at 20:27:19, Vidya Sagar <vidyas@nvidia.com> wrote:

> move PCIe config space capability search API to common designware file
s/designware/DesignWare
> as this can be used by both host and ep mode codes.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> Changes from [v2]:
> * None
> 
> Changes from [v1]:
> * Removed dw_pcie_find_next_ext_capability() API from here and made a
>   separate patch for that
> 
>  .../pci/controller/dwc/pcie-designware-ep.c   | 37 +------------------
>  drivers/pci/controller/dwc/pcie-designware.c  | 33 +++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>  3 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 24f5a775ad34..b9d9c9a4ba6d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -40,39 +40,6 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>  }
>  
> -static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> -			      u8 cap)
> -{
> -	u8 cap_id, next_cap_ptr;
> -	u16 reg;
> -
> -	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> -	next_cap_ptr = (reg & 0xff00) >> 8;
> -	cap_id = (reg & 0x00ff);
> -
> -	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> -		return 0;
> -
> -	if (cap_id == cap)
> -		return cap_ptr;
> -
> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> -}
> -
> -static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> -{
> -	u8 next_cap_ptr;
> -	u16 reg;
> -
> -	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> -	next_cap_ptr = (reg & 0x00ff);
> -
> -	if (!next_cap_ptr)
> -		return 0;
> -
> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> -}
> -
>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>  				   struct pci_epf_header *hdr)
>  {
> @@ -591,9 +558,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>  		return -ENOMEM;
>  	}
> -	ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>  
> -	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>  
>  	dw_pcie_setup(pci);
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index f98e2f284ae1..d68c123e409c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -20,6 +20,39 @@
>  #define PCIE_PHY_DEBUG_R1_LINK_UP	(0x1 << 4)
>  #define PCIE_PHY_DEBUG_R1_LINK_IN_TRAINING	(0x1 << 29)
>  
> +static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> +				  u8 cap)
> +{
> +	u8 cap_id, next_cap_ptr;
> +	u16 reg;
> +
> +	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> +	next_cap_ptr = (reg & 0xff00) >> 8;
> +	cap_id = (reg & 0x00ff);
> +
> +	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> +		return 0;
> +
> +	if (cap_id == cap)
> +		return cap_ptr;
> +
> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +
> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> +{
> +	u8 next_cap_ptr;
> +	u16 reg;
> +
> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> +	next_cap_ptr = (reg & 0x00ff);
> +
> +	if (!next_cap_ptr)
> +		return 0;
> +
> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>  {
>  	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 86df36701a37..4ccd4c706ddb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -247,6 +247,8 @@ struct dw_pcie {
>  #define to_dw_pcie_from_ep(endpoint)   \
>  		container_of((endpoint), struct dw_pcie, ep)
>  
> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> +
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>  int dw_pcie_write(void __iomem *addr, int size, u32 val);
>  
> -- 
> 2.17.1

With the replacement, you have my ack:

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Thanks,
Gustavo
Vidya Sagar April 17, 2019, 9:34 a.m. UTC | #3
On 4/17/2019 2:57 PM, Gustavo Pimentel wrote:
> On Tue, Apr 16, 2019 at 20:27:20, Vidya Sagar <vidyas@nvidia.com> wrote:
> 
>> Add extended configuration space capability search API using struct dw_pcie *
>> pointer
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> Changes from [v2]:
>> * None
>>
>> Changes from [v1]:
>> * This is a new patch in v2 series
>>
>>   drivers/pci/controller/dwc/pcie-designware.c | 41 ++++++++++++++++++++
>>   drivers/pci/controller/dwc/pcie-designware.h |  1 +
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index d68c123e409c..44c0ba078452 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -53,6 +53,47 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>>   	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>>   }
>>   
>> +static int dw_pcie_find_next_ext_capability(struct dw_pcie *pci, int start,
>> +					    int cap)
>> +{
>> +	u32 header;
>> +	int ttl;
>> +	int pos = PCI_CFG_SPACE_SIZE;
>> +
>> +	/* minimum 8 bytes per capability */
>> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
>> +
>> +	if (start)
>> +		pos = start;
>> +
>> +	header = dw_pcie_readl_dbi(pci, pos);
>> +	/*
>> +	 * If we have no capabilities, this is indicated by cap ID,
>> +	 * cap version and next pointer all being 0.
>> +	 */
>> +	if (header == 0)
>> +		return 0;
>> +
>> +	while (ttl-- > 0) {
>> +		if (PCI_EXT_CAP_ID(header) == cap && pos != start)
>> +			return pos;
>> +
>> +		pos = PCI_EXT_CAP_NEXT(header);
>> +		if (pos < PCI_CFG_SPACE_SIZE)
>> +			break;
>> +
>> +		header = dw_pcie_readl_dbi(pci, pos);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap)
>> +{
>> +	return dw_pcie_find_next_ext_capability(pci, 0, cap);
>> +}
>> +EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
>> +
>>   int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>>   {
>>   	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 4ccd4c706ddb..fa41d675c48f 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -248,6 +248,7 @@ struct dw_pcie {
>>   		container_of((endpoint), struct dw_pcie, ep)
>>   
>>   u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
>> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap);
>>   
>>   int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>>   int dw_pcie_write(void __iomem *addr, int size, u32 val);
>> -- 
>> 2.17.1
> 
> This ext capability function is aimed to be used by the EP also?
> 
Yes. It can be used by EP also. Hence I added it in common files.
Gustavo Pimentel April 17, 2019, 9:39 a.m. UTC | #4
On Tue, Apr 16, 2019 at 20:27:22, Vidya Sagar <vidyas@nvidia.com> wrote:

> Add support to enable CDM (Configuration Dependent Module) register check
> for any data corruption based on the device-tree flag 'enable-cdm-check'.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> Changes since [v2]:
> * Changed code and commit description to reflect change in flag from
>   'cdm-check' to 'enable-cdm-check'
> 
> Changes since [v1]:
> * This is a new patch in v2 series
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++
>  drivers/pci/controller/dwc/pcie-designware.h | 9 +++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 44c0ba078452..5b416f483426 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -503,4 +503,11 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		break;
>  	}
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> +
> +	if (of_property_read_bool(np, "enable-cdm-check")) {
> +		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> +		val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
> +		       PCIE_PL_CHK_REG_CHK_REG_START;
> +		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> +	}
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index fa41d675c48f..7f57fe019fbf 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -83,6 +83,15 @@
>  #define PCIE_MISC_CONTROL_1_OFF		0x8BC
>  #define PCIE_DBI_RO_WR_EN		BIT(0)
>  
> +#define PCIE_PL_CHK_REG_CONTROL_STATUS			0xB20
> +#define PCIE_PL_CHK_REG_CHK_REG_START			BIT(0)
> +#define PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS		BIT(1)
> +#define PCIE_PL_CHK_REG_CHK_REG_COMPARISON_ERROR	BIT(16)
> +#define PCIE_PL_CHK_REG_CHK_REG_LOGIC_ERROR		BIT(17)
> +#define PCIE_PL_CHK_REG_CHK_REG_COMPLETE		BIT(18)
> +
> +#define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
> +
>  /*
>   * iATU Unroll-specific register definitions
>   * From 4.80 core version the address translation will be made by unroll
> -- 
> 2.17.1

Nice.

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Thanks,
Gustavo
Gustavo Pimentel April 17, 2019, 9:46 a.m. UTC | #5
On Wed, Apr 17, 2019 at 10:34:52, Vidya Sagar <vidyas@nvidia.com> wrote:

> On 4/17/2019 2:57 PM, Gustavo Pimentel wrote:
> > On Tue, Apr 16, 2019 at 20:27:20, Vidya Sagar <vidyas@nvidia.com> wrote:
> > 
> >> Add extended configuration space capability search API using struct dw_pcie *
> >> pointer
> >>
> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> >> ---
> >> Changes from [v2]:
> >> * None
> >>
> >> Changes from [v1]:
> >> * This is a new patch in v2 series
> >>
> >>   drivers/pci/controller/dwc/pcie-designware.c | 41 ++++++++++++++++++++
> >>   drivers/pci/controller/dwc/pcie-designware.h |  1 +
> >>   2 files changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> >> index d68c123e409c..44c0ba078452 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> >> @@ -53,6 +53,47 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> >>   	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
> >>   }
> >>   
> >> +static int dw_pcie_find_next_ext_capability(struct dw_pcie *pci, int start,
> >> +					    int cap)
> >> +{
> >> +	u32 header;
> >> +	int ttl;
> >> +	int pos = PCI_CFG_SPACE_SIZE;
> >> +
> >> +	/* minimum 8 bytes per capability */
> >> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> >> +
> >> +	if (start)
> >> +		pos = start;
> >> +
> >> +	header = dw_pcie_readl_dbi(pci, pos);
> >> +	/*
> >> +	 * If we have no capabilities, this is indicated by cap ID,
> >> +	 * cap version and next pointer all being 0.
> >> +	 */
> >> +	if (header == 0)
> >> +		return 0;
> >> +
> >> +	while (ttl-- > 0) {
> >> +		if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> >> +			return pos;
> >> +
> >> +		pos = PCI_EXT_CAP_NEXT(header);
> >> +		if (pos < PCI_CFG_SPACE_SIZE)
> >> +			break;
> >> +
> >> +		header = dw_pcie_readl_dbi(pci, pos);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap)
> >> +{
> >> +	return dw_pcie_find_next_ext_capability(pci, 0, cap);
> >> +}
> >> +EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
> >> +
> >>   int dw_pcie_read(void __iomem *addr, int size, u32 *val)
> >>   {
> >>   	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >> index 4ccd4c706ddb..fa41d675c48f 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >> @@ -248,6 +248,7 @@ struct dw_pcie {
> >>   		container_of((endpoint), struct dw_pcie, ep)
> >>   
> >>   u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> >> +int dw_pcie_find_ext_capability(struct dw_pcie *pci, int cap);
> >>   
> >>   int dw_pcie_read(void __iomem *addr, int size, u32 *val);
> >>   int dw_pcie_write(void __iomem *addr, int size, u32 val);
> >> -- 
> >> 2.17.1
> > 
> > This ext capability function is aimed to be used by the EP also?
> > 
> Yes. It can be used by EP also. Hence I added it in common files.


Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Gustavo Pimentel April 17, 2019, 9:56 a.m. UTC | #6
On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote:

> Remove multiple write enable and disable sequences of dbi registers as
> Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by
> DBI write-lock enable bit thereby not allowing any further writes to BAR-0
> register in config space to take place. Hence disabling write permission
> only towards the end.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> Changes since [v2]:
> * None
> 
> Changes since [v1]:
> * None
> 
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2a5332e5ccfa..c0334c92c1a6 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	val &= 0xffff00ff;
>  	val |= 0x00000100;
>  	dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
> -	dw_pcie_dbi_ro_wr_dis(pci);
>  
>  	/* Setup bus numbers */
>  	val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
> @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  
>  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
>  
> -	/* Enable write permission for the DBI read-only register */
> -	dw_pcie_dbi_ro_wr_en(pci);
>  	/* Program correct class for RC */
>  	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>  	/* Better disable write permission right after the update */
> -- 
> 2.17.1

This setup sequence was written by Jingoo Han, let's check if he did this 
by some particular reason.
Jingoo do you remember why you wrote the code like this?

Regards,
Gustavo
Jisheng Zhang April 22, 2019, 7:54 a.m. UTC | #7
On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote:

> 
> On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote:
> 
> > Remove multiple write enable and disable sequences of dbi registers as
> > Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by
> > DBI write-lock enable bit thereby not allowing any further writes to BAR-0
> > register in config space to take place. Hence disabling write permission
> > only towards the end.
> >
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > Changes since [v2]:
> > * None
> >
> > Changes since [v1]:
> > * None
> >
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 2a5332e5ccfa..c0334c92c1a6 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >       val &= 0xffff00ff;
> >       val |= 0x00000100;
> >       dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
> > -     dw_pcie_dbi_ro_wr_dis(pci);
> >
> >       /* Setup bus numbers */
> >       val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
> > @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >
> >       dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> >
> > -     /* Enable write permission for the DBI read-only register */
> > -     dw_pcie_dbi_ro_wr_en(pci);
> >       /* Program correct class for RC */
> >       dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
> >       /* Better disable write permission right after the update */
> > --
> > 2.17.1  
> 
> This setup sequence was written by Jingoo Han, let's check if he did this
> by some particular reason.
> Jingoo do you remember why you wrote the code like this?

FWICT, enabling RO writeable in the setup sequence is introduced in
commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, 
Interrupt Pin updates"). The Reason why not towards the end maybe
only enable the RO writeable when necessary.

thanks
Vidya Sagar April 23, 2019, 6:11 a.m. UTC | #8
On 4/22/2019 1:24 PM, Jisheng Zhang wrote:
> On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote:
> 
>>
>> On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote:
>>
>>> Remove multiple write enable and disable sequences of dbi registers as
>>> Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by
>>> DBI write-lock enable bit thereby not allowing any further writes to BAR-0
>>> register in config space to take place. Hence disabling write permission
>>> only towards the end.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>> Changes since [v2]:
>>> * None
>>>
>>> Changes since [v1]:
>>> * None
>>>
>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> index 2a5332e5ccfa..c0334c92c1a6 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>>        val &= 0xffff00ff;
>>>        val |= 0x00000100;
>>>        dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
>>> -     dw_pcie_dbi_ro_wr_dis(pci);
>>>
>>>        /* Setup bus numbers */
>>>        val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
>>> @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>>
>>>        dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
>>>
>>> -     /* Enable write permission for the DBI read-only register */
>>> -     dw_pcie_dbi_ro_wr_en(pci);
>>>        /* Program correct class for RC */
>>>        dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
>>>        /* Better disable write permission right after the update */
>>> --
>>> 2.17.1
>>
>> This setup sequence was written by Jingoo Han, let's check if he did this
>> by some particular reason.
>> Jingoo do you remember why you wrote the code like this?
> 
> FWICT, enabling RO writeable in the setup sequence is introduced in
> commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code,
> Interrupt Pin updates"). The Reason why not towards the end maybe
> only enable the RO writeable when necessary.
In that case, my patch shouldn't cause any issue and if any it would avoid
few writes to speed up the process. So, in my opinion, it is a good to go change.
Please let me know if you have disagree with me on this.

> 
> thanks
>
Gustavo Pimentel April 23, 2019, 9:40 a.m. UTC | #9
On Mon, Apr 22, 2019 at 8:54:32, Jisheng Zhang 
<Jisheng.Zhang@synaptics.com> wrote:

> On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote:
> 
> > 
> > On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote:
> > 
> > > Remove multiple write enable and disable sequences of dbi registers as
> > > Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by
> > > DBI write-lock enable bit thereby not allowing any further writes to BAR-0
> > > register in config space to take place. Hence disabling write permission
> > > only towards the end.
> > >
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > ---
> > > Changes since [v2]:
> > > * None
> > >
> > > Changes since [v1]:
> > > * None
> > >
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 2a5332e5ccfa..c0334c92c1a6 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> > >       val &= 0xffff00ff;
> > >       val |= 0x00000100;
> > >       dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
> > > -     dw_pcie_dbi_ro_wr_dis(pci);
> > >
> > >       /* Setup bus numbers */
> > >       val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
> > > @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> > >
> > >       dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> > >
> > > -     /* Enable write permission for the DBI read-only register */
> > > -     dw_pcie_dbi_ro_wr_en(pci);
> > >       /* Program correct class for RC */
> > >       dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
> > >       /* Better disable write permission right after the update */
> > > --
> > > 2.17.1  
> > 
> > This setup sequence was written by Jingoo Han, let's check if he did this
> > by some particular reason.
> > Jingoo do you remember why you wrote the code like this?
> 
> FWICT, enabling RO writeable in the setup sequence is introduced in
> commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, 
> Interrupt Pin updates"). The Reason why not towards the end maybe
> only enable the RO writeable when necessary.

I also share that belief, in any case, I just want to confirm with the 
original developer if this was coded like this for some good reason, 
maybe to force some additional protection. Otherwise, I think the 
presented patch is harmless.

Gustavo

> 
> thanks
Z.q. Hou April 30, 2019, 12:36 a.m. UTC | #10
> -----Original Message-----
> From: Jisheng Zhang [mailto:Jisheng.Zhang@synaptics.com]
> Sent: 2019年4月22日 15:55
> To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>; Z.q. Hou
> <zhiqiang.hou@nxp.com>
> Cc: Vidya Sagar <vidyas@nvidia.com>; bhelgaas@google.com;
> robh+dt@kernel.org; mark.rutland@arm.com; thierry.reding@gmail.com;
> jonathanh@nvidia.com; kishon@ti.com; catalin.marinas@arm.com;
> will.deacon@arm.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> devicetree@vger.kernel.org; mmaddireddy@nvidia.com; kthota@nvidia.com;
> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> mperttunen@nvidia.com; linux-tegra@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; sagar.tv@gmail.com
> Subject: [EXT] Re: [PATCH V3 04/16] PCI: dwc: Perform dbi regs write lock
> towards the end
> 
> WARNING: This email was created outside of NXP. DO NOT CLICK links or
> attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote:
> 
> >
> > On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote:
> >
> > > Remove multiple write enable and disable sequences of dbi registers
> > > as
> > > Tegra194 implements writes to BAR-0 register (offset: 0x10)
> > > controlled by DBI write-lock enable bit thereby not allowing any
> > > further writes to BAR-0 register in config space to take place.
> > > Hence disabling write permission only towards the end.
> > >
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > ---
> > > Changes since [v2]:
> > > * None
> > >
> > > Changes since [v1]:
> > > * None
> > >
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 2a5332e5ccfa..c0334c92c1a6 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> > >       val &= 0xffff00ff;
> > >       val |= 0x00000100;
> > >       dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
> > > -     dw_pcie_dbi_ro_wr_dis(pci);
> > >
> > >       /* Setup bus numbers */
> > >       val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); @@ -723,8
> > > +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> > >
> > >       dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> > >
> > > -     /* Enable write permission for the DBI read-only register */
> > > -     dw_pcie_dbi_ro_wr_en(pci);
> > >       /* Program correct class for RC */
> > >       dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2,
> PCI_CLASS_BRIDGE_PCI);
> > >       /* Better disable write permission right after the update */
> > > --
> > > 2.17.1
> >
> > This setup sequence was written by Jingoo Han, let's check if he did
> > this by some particular reason.
> > Jingoo do you remember why you wrote the code like this?
> 
> FWICT, enabling RO writeable in the setup sequence is introduced in commit
> d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, Interrupt
> Pin updates"). The Reason why not towards the end maybe only enable the
> RO writeable when necessary.
> 

Yes, you get the point, I think it's not a good choice to unlock the write permission
of the RO registers all through.

Thanks,
Zhiqiang