PCI: dwc ep: cache config until DBI regs available

Message ID	20181119222602.28001-1-swarren@wwwdotorg.org
State	Superseded
Delegated to:	Lorenzo Pieralisi
Headers	show Return-Path: <linux-pci-owner@vger.kernel.org> From: Stephen Warren <swarren@wwwdotorg.org> To: Jingoo Han <jingoohan1@gmail.com>, Gustavo Pimentel <gustavo.pimentel@synopsys.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org, Vidya Sagar <vidyas@nvidia.com>, Manikanta Maddireddy <mmaddireddy@nvidia.com>, Stephen Warren <swarren@nvidia.com> Subject: [PATCH] PCI: dwc ep: cache config until DBI regs available Date: Mon, 19 Nov 2018 15:26:02 -0700 Message-Id: <20181119222602.28001-1-swarren@wwwdotorg.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-pci-owner@vger.kernel.org Precedence: bulk
Series	PCI: dwc ep: cache config until DBI regs available \| expand PCI: dwc ep: cache config until DBI regs available

Stephen Warren Nov. 19, 2018, 10:26 p.m. UTC

From: Stephen Warren <swarren@nvidia.com>

Some implementations of the DWC PCIe endpoint controller do not allow
access to DBI registers until the attached host has started REFCLK,
released PERST, and the endpoint driver has initialized clocking of the
DBI registers based on that. One such system is NVIDIA's T194 SoC. The
PCIe endpoint subsystem and DWC driver currently don't work on such
hardware, since they assume that all endpoint configuration can happen
at any arbitrary time.

Enhance the DWC endpoint driver to support such systems by caching all
endpoint configuration in software, and only writing the configuration
to hardware once it's been initialized. This is implemented by splitting
all endpoint controller ops into two functions; the first which simply
records/caches the desired configuration whenever called by the
associated function driver and optionally calls the second, and the
second which actually programs the configuration into hardware, which
may be called either by the first function, or later when it's known
that the DBI registers are available.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Note: I have only compiled-tested this patch in the context of the
mainline kernel since NVIDIA Tegra PCIe EP support is not yet present.
However, I've built and tested an equivalent patch in our downstream
kernel. I did have to manually port it to mainline due to conflicts
though.

This patch was developed on top of "PCI: designware: don't hard-code
DBI/ATU offset" which I sent last week, so may rely on that for patch
context.
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 199 ++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.h  |  26 ++-
 2 files changed, 181 insertions(+), 44 deletions(-)

Vidya Sagar Nov. 20, 2018, 5:30 p.m. UTC | #1

> -----Original Message-----
> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: Tuesday, November 20, 2018 3:56 AM
> To: Jingoo Han <jingoohan1@gmail.com>; Gustavo Pimentel
> <gustavo.pimentel@synopsys.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; Manikanta
> Maddireddy <mmaddireddy@nvidia.com>; Stephen Warren
> <swarren@nvidia.com>
> Subject: [PATCH] PCI: dwc ep: cache config until DBI regs available
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> Some implementations of the DWC PCIe endpoint controller do not allow access
> to DBI registers until the attached host has started REFCLK, released PERST, and
> the endpoint driver has initialized clocking of the DBI registers based on that.
> One such system is NVIDIA's T194 SoC. The PCIe endpoint subsystem and DWC
> driver currently don't work on such hardware, since they assume that all
> endpoint configuration can happen at any arbitrary time.
> 
> Enhance the DWC endpoint driver to support such systems by caching all
> endpoint configuration in software, and only writing the configuration to
> hardware once it's been initialized. This is implemented by splitting all endpoint
> controller ops into two functions; the first which simply records/caches the
> desired configuration whenever called by the associated function driver and
> optionally calls the second, and the second which actually programs the
> configuration into hardware, which may be called either by the first function, or
> later when it's known that the DBI registers are available.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Note: I have only compiled-tested this patch in the context of the mainline
> kernel since NVIDIA Tegra PCIe EP support is not yet present.
> However, I've built and tested an equivalent patch in our downstream kernel. I
> did have to manually port it to mainline due to conflicts though.
> 
> This patch was developed on top of "PCI: designware: don't hard-code DBI/ATU
> offset" which I sent last week, so may rely on that for patch context.
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 199 ++++++++++++++----
>  drivers/pci/controller/dwc/pcie-designware.h  |  26 ++-
>  2 files changed, 181 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 880210366e71..6910b64bfdcb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -73,11 +73,10 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie
> *pci, u8 cap)
>  	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)
> +static void dw_pcie_ep_write_header_regs(struct dw_pcie_ep *ep)
>  {
> -	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct pci_epf_header *hdr = &(ep->cached_hdr);
> 
>  	dw_pcie_dbi_ro_wr_en(pci);
>  	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid); @@ -94,6
> +93,19 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8
> func_no,
>  	dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
>  			   hdr->interrupt_pin);
>  	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
> +				   struct pci_epf_header *hdr)
> +{
> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> +
> +	ep->cached_hdr = *hdr;
> +
> +	if (!ep->hw_regs_available)
> +		return 0;
> +
> +	dw_pcie_ep_write_header_regs(ep);
> 
>  	return 0;
>  }
> @@ -112,6 +124,15 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
> *ep, enum pci_barno bar,
>  		return -EINVAL;
>  	}
> 
> +	ep->cached_inbound_atus[free_win].bar = bar;
> +	ep->cached_inbound_atus[free_win].cpu_addr = cpu_addr;
> +	ep->cached_inbound_atus[free_win].as_type = as_type;
> +	ep->cached_bars[bar].atu_index = free_win;
> +	set_bit(free_win, ep->ib_window_map);
> +
> +	if (!ep->hw_regs_available)
> +		return 0;
> +
>  	ret = dw_pcie_prog_inbound_atu(pci, free_win, bar, cpu_addr,
>  				       as_type);
>  	if (ret < 0) {
> @@ -119,9 +140,6 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
> *ep, enum pci_barno bar,
>  		return ret;
>  	}
> 
> -	ep->bar_to_atu[bar] = free_win;
> -	set_bit(free_win, ep->ib_window_map);
> -
>  	return 0;
>  }
> 
> @@ -137,27 +155,66 @@ static int dw_pcie_ep_outbound_atu(struct
> dw_pcie_ep *ep, phys_addr_t phys_addr,
>  		return -EINVAL;
>  	}
> 
> +	ep->cached_outbound_atus[free_win].addr = phys_addr;
> +	ep->cached_outbound_atus[free_win].pci_addr = pci_addr;
> +	ep->cached_outbound_atus[free_win].size = size;
> +	set_bit(free_win, ep->ob_window_map);
> +
> +	if (!ep->hw_regs_available)
> +		return 0;
> +
>  	dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM,
>  				  phys_addr, pci_addr, size);
> 
> -	set_bit(free_win, ep->ob_window_map);
> -	ep->outbound_addr[free_win] = phys_addr;
> -
>  	return 0;
>  }
> 
> +static void dw_pcie_ep_clear_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
> +				      enum pci_barno bar)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	int flags = ep->cached_bars[bar].flags;
> +	u32 atu_index = ep->cached_bars[bar].atu_index;
> +
> +	__dw_pcie_ep_reset_bar(pci, bar, flags);
> +
> +	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); }
> +
>  static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
>  				 struct pci_epf_bar *epf_bar)
>  {
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> -	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	enum pci_barno bar = epf_bar->barno;
> -	u32 atu_index = ep->bar_to_atu[bar];
> +	u32 atu_index = ep->cached_bars[bar].atu_index;
> 
> -	__dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
> -
> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
>  	clear_bit(atu_index, ep->ib_window_map);
> +
> +	if (!ep->hw_regs_available)
> +		return;
> +
> +	dw_pcie_ep_clear_bar_regs(ep, func_no, bar); }
> +
> +static void dw_pcie_ep_set_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
> +				    enum pci_barno bar)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	size_t size = ep->cached_bars[bar].size;
> +	int flags = ep->cached_bars[bar].flags;
> +	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> +	dw_pcie_writel_dbi(pci, reg, flags);
> +
> +	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> +		dw_pcie_writel_dbi(pci, reg + 4, 0);
> +	}
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
>  }
> 
>  static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, @@ -165,12
> +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,  {
>  	int ret;
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> -	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	enum pci_barno bar = epf_bar->barno;
>  	size_t size = epf_bar->size;
>  	int flags = epf_bar->flags;
>  	enum dw_pcie_as_type as_type;
> -	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> 
>  	if (!(flags & PCI_BASE_ADDRESS_SPACE))
>  		as_type = DW_PCIE_AS_MEM;
> @@ -181,17 +236,13 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8
> func_no,
>  	if (ret)
>  		return ret;
> 
> -	dw_pcie_dbi_ro_wr_en(pci);
> -
> -	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> -	dw_pcie_writel_dbi(pci, reg, flags);
> +	ep->cached_bars[bar].size = size;
> +	ep->cached_bars[bar].flags = flags;
> 
> -	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> -		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> -		dw_pcie_writel_dbi(pci, reg + 4, 0);
> -	}
> +	if (!ep->hw_regs_available)
> +		return 0;
> 
> -	dw_pcie_dbi_ro_wr_dis(pci);
> +	dw_pcie_ep_set_bar_regs(ep, func_no, bar);
> 
>  	return 0;
>  }
> @@ -202,7 +253,7 @@ static int dw_pcie_find_index(struct dw_pcie_ep *ep,
> phys_addr_t addr,
>  	u32 index;
> 
>  	for (index = 0; index < ep->num_ob_windows; index++) {
> -		if (ep->outbound_addr[index] != addr)
> +		if (ep->cached_outbound_atus[index].addr != addr)
>  			continue;
>  		*atu_index = index;
>  		return 0;
> @@ -223,8 +274,12 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc
> *epc, u8 func_no,
>  	if (ret < 0)
>  		return;
> 
> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>  	clear_bit(atu_index, ep->ob_window_map);
> +
> +	if (!ep->hw_regs_available)
> +		return;
> +
> +	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>  }
> 
>  static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, @@ -254,7
> +309,10 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no)
>  		return -EINVAL;
> 
>  	reg = ep->msi_cap + PCI_MSI_FLAGS;
> -	val = dw_pcie_readw_dbi(pci, reg);
> +	if (ep->hw_regs_available)
> +		val = dw_pcie_readw_dbi(pci, reg);
> +	else
> +		val = ep->cached_msi_flags;
>  	if (!(val & PCI_MSI_FLAGS_ENABLE))
>  		return -EINVAL;
> 
> @@ -273,12 +331,19 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8
> func_no, u8 interrupts)
>  		return -EINVAL;
> 
>  	reg = ep->msi_cap + PCI_MSI_FLAGS;
> -	val = dw_pcie_readw_dbi(pci, reg);
> +	if (ep->hw_regs_available)
> +		val = dw_pcie_readw_dbi(pci, reg);
> +	else
> +		val = ep->cached_msi_flags;
>  	val &= ~PCI_MSI_FLAGS_QMASK;
>  	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> -	dw_pcie_dbi_ro_wr_en(pci);
> -	dw_pcie_writew_dbi(pci, reg, val);
> -	dw_pcie_dbi_ro_wr_dis(pci);
> +	if (ep->hw_regs_available) {
> +		dw_pcie_dbi_ro_wr_en(pci);
> +		dw_pcie_writew_dbi(pci, reg, val);
> +		dw_pcie_dbi_ro_wr_dis(pci);
> +	} else {
> +		ep->cached_msi_flags = val;
> +	}
> 
>  	return 0;
>  }
> @@ -293,7 +358,10 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc,
> u8 func_no)
>  		return -EINVAL;
> 
>  	reg = ep->msix_cap + PCI_MSIX_FLAGS;
> -	val = dw_pcie_readw_dbi(pci, reg);
> +	if (ep->hw_regs_available)
> +		val = dw_pcie_readw_dbi(pci, reg);
> +	else
> +		val = ep->cached_msix_flags;
>  	if (!(val & PCI_MSIX_FLAGS_ENABLE))
>  		return -EINVAL;
> 
> @@ -312,16 +380,49 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc,
> u8 func_no, u16 interrupts)
>  		return -EINVAL;
> 
>  	reg = ep->msix_cap + PCI_MSIX_FLAGS;
> -	val = dw_pcie_readw_dbi(pci, reg);
> +	if (ep->hw_regs_available)
> +		val = dw_pcie_readw_dbi(pci, reg);
> +	else
> +		val = ep->cached_msix_flags;
>  	val &= ~PCI_MSIX_FLAGS_QSIZE;
>  	val |= interrupts;
> -	dw_pcie_dbi_ro_wr_en(pci);
> -	dw_pcie_writew_dbi(pci, reg, val);
> -	dw_pcie_dbi_ro_wr_dis(pci);
> +	if (ep->hw_regs_available) {
> +		dw_pcie_dbi_ro_wr_en(pci);
> +		dw_pcie_writew_dbi(pci, reg, val);
> +		dw_pcie_dbi_ro_wr_dis(pci);
> +	} else {
> +		ep->cached_msix_flags = val;
> +	}
> 
>  	return 0;
>  }
> 
> +void dw_pcie_set_regs_available(struct dw_pcie *pci) {
> +	struct dw_pcie_ep *ep = &(pci->ep);
> +	int i;
> +
> +	ep->hw_regs_available = true;
> +
> +	dw_pcie_ep_write_header_regs(ep);
> +	for_each_set_bit(i, ep->ib_window_map, ep->num_ib_windows) {
> +		dw_pcie_prog_inbound_atu(pci, i,
> +			ep->cached_inbound_atus[i].bar,
> +			ep->cached_inbound_atus[i].cpu_addr,
> +			ep->cached_inbound_atus[i].as_type);
> +		dw_pcie_ep_set_bar_regs(ep, 0, ep-
> >cached_inbound_atus[i].bar);
> +	}
> +	for_each_set_bit(i, ep->ob_window_map, ep->num_ob_windows)
> +		dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> +			ep->cached_outbound_atus[i].addr,
> +			ep->cached_outbound_atus[i].pci_addr,
> +			ep->cached_outbound_atus[i].size);
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, PCI_MSI_FLAGS, ep->cached_msi_flags);
> +	dw_pcie_writew_dbi(pci, PCI_MSIX_FLAGS, ep->cached_msix_flags);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
>  static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>  				enum pci_epc_irq_type type, u16
> interrupt_num)  { @@ -330,6 +431,9 @@ static int dw_pcie_ep_raise_irq(struct
> pci_epc *epc, u8 func_no,
>  	if (!ep->ops->raise_irq)
>  		return -EINVAL;
> 
> +	if (!ep->hw_regs_available)
> +		return -EAGAIN;
> +
>  	return ep->ops->raise_irq(ep, func_no, type, interrupt_num);  }
> 
> @@ -391,6 +495,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
> u8 func_no,
>  	bool has_upper;
>  	int ret;
> 
> +	if (!ep->hw_regs_available)
> +		return -EAGAIN;
> +
>  	if (!ep->msi_cap)
>  		return -EINVAL;
> 
> @@ -436,6 +543,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep,
> u8 func_no,
>  	void __iomem *msix_tbl;
>  	int ret;
> 
> +	if (!ep->hw_regs_available)
> +		return -EAGAIN;
> +
>  	reg = ep->msix_cap + PCI_MSIX_TABLE;
>  	tbl_offset = dw_pcie_readl_dbi(pci, reg);
>  	bir = (tbl_offset & PCI_MSIX_TABLE_BIR); @@ -494,7 +604,6 @@ void
> dw_pcie_ep_exit(struct dw_pcie_ep *ep)  int dw_pcie_ep_init(struct
> dw_pcie_ep *ep)  {
>  	int ret;
> -	void *addr;
>  	struct pci_epc *epc;
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct device *dev = pci->dev;
> @@ -543,11 +652,17 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (!ep->ob_window_map)
>  		return -ENOMEM;
> 
> -	addr = devm_kcalloc(dev, ep->num_ob_windows, sizeof(phys_addr_t),
> -			    GFP_KERNEL);
> -	if (!addr)
> +	ep->cached_inbound_atus = devm_kzalloc(dev,
> +		sizeof(ep->cached_inbound_atus[0]) * ep->num_ib_windows,
> +		GFP_KERNEL);
> +	if (!ep->cached_inbound_atus)
> +		return -ENOMEM;
> +
> +	ep->cached_outbound_atus = devm_kzalloc(dev,
> +		sizeof(ep->cached_outbound_atus[0]) * ep->num_ob_windows,
> +		GFP_KERNEL);
> +	if (!ep->cached_outbound_atus)
>  		return -ENOMEM;
> -	ep->outbound_addr = addr;
> 
>  	epc = devm_pci_epc_create(dev, &epc_ops);
>  	if (IS_ERR(epc)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 02fb532c5db9..c116b289aa51 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -192,8 +192,6 @@ struct dw_pcie_ep {
>  	phys_addr_t		phys_base;
>  	size_t			addr_size;
>  	size_t			page_size;
> -	u8			bar_to_atu[6];
> -	phys_addr_t		*outbound_addr;
>  	unsigned long		*ib_window_map;
>  	unsigned long		*ob_window_map;
>  	u32			num_ib_windows;
> @@ -202,6 +200,25 @@ struct dw_pcie_ep {
>  	phys_addr_t		msi_mem_phys;
>  	u8			msi_cap;	/* MSI capability offset */
>  	u8			msix_cap;	/* MSI-X capability offset */
> +	bool			hw_regs_available;
> +	struct pci_epf_header	cached_hdr;
> +	struct {
> +		size_t			size;
> +		int			flags;
> +		u8			atu_index;
> +	}			cached_bars[6];
> +	struct {
> +		enum pci_barno		bar;
> +		dma_addr_t		cpu_addr;
> +		enum dw_pcie_as_type	as_type;
> +	}			*cached_inbound_atus;
> +	struct {
> +		phys_addr_t		addr;
> +		u64			pci_addr;
> +		size_t			size;
> +	}			*cached_outbound_atus;
> +	u32			cached_msi_flags;
> +	u32			cached_msix_flags;
>  };
> 
>  struct dw_pcie_ops {
> @@ -363,6 +380,7 @@ static inline int dw_pcie_allocate_domains(struct
> pcie_port *pp)  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);  int
> dw_pcie_ep_init(struct dw_pcie_ep *ep);  void dw_pcie_ep_exit(struct
> dw_pcie_ep *ep);
> +void dw_pcie_ep_set_regs_available(struct dw_pcie *pci);
>  int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no);  int
> dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>  			     u8 interrupt_num);
> @@ -383,6 +401,10 @@ static inline void dw_pcie_ep_exit(struct dw_pcie_ep
> *ep)  {  }
> 
> +static inline void dw_pcie_ep_set_regs_available(struct dw_pcie *pci) {
> +}
> +
>  static inline int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8
> func_no)  {
>  	return 0;
> --
> 2.19.1

What is the guidance on setting "hw_regs_available" param?
I mean, in case of hardware platforms where registers are always available to write (Ex:- pci-epf-test.c), how do they
either (a) set the param hw_regs_available to '1' or (b) call dw_pcie_set_regs_available() to commit all cached values to registers?
I see a need for APIs to establish the link among dw_pcie_ep -> pci_epc -> pci_epf_driver.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Stephen Warren Nov. 20, 2018, 5:41 p.m. UTC | #2

On 11/20/18 10:30 AM, Vidya Sagar wrote:
>> -----Original Message-----
>> From: Stephen Warren <swarren@wwwdotorg.org>
>> Sent: Tuesday, November 20, 2018 3:56 AM
>> To: Jingoo Han <jingoohan1@gmail.com>; Gustavo Pimentel
>> <gustavo.pimentel@synopsys.com>; Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>
>> Cc: linux-pci@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; Manikanta
>> Maddireddy <mmaddireddy@nvidia.com>; Stephen Warren
>> <swarren@nvidia.com>
>> Subject: [PATCH] PCI: dwc ep: cache config until DBI regs available
>>
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some implementations of the DWC PCIe endpoint controller do not allow access
>> to DBI registers until the attached host has started REFCLK, released PERST, and
>> the endpoint driver has initialized clocking of the DBI registers based on that.
>> One such system is NVIDIA's T194 SoC. The PCIe endpoint subsystem and DWC
>> driver currently don't work on such hardware, since they assume that all
>> endpoint configuration can happen at any arbitrary time.
>>
>> Enhance the DWC endpoint driver to support such systems by caching all
>> endpoint configuration in software, and only writing the configuration to
>> hardware once it's been initialized. This is implemented by splitting all endpoint
>> controller ops into two functions; the first which simply records/caches the
>> desired configuration whenever called by the associated function driver and
>> optionally calls the second, and the second which actually programs the
>> configuration into hardware, which may be called either by the first function, or
>> later when it's known that the DBI registers are available.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> Note: I have only compiled-tested this patch in the context of the mainline
>> kernel since NVIDIA Tegra PCIe EP support is not yet present.
>> However, I've built and tested an equivalent patch in our downstream kernel. I
>> did have to manually port it to mainline due to conflicts though.
>>
>> This patch was developed on top of "PCI: designware: don't hard-code DBI/ATU
>> offset" which I sent last week, so may rely on that for patch context.
>> ---
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 199 ++++++++++++++----
>>   drivers/pci/controller/dwc/pcie-designware.h  |  26 ++-
>>   2 files changed, 181 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index 880210366e71..6910b64bfdcb 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -73,11 +73,10 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie
>> *pci, u8 cap)
>>   	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)
>> +static void dw_pcie_ep_write_header_regs(struct dw_pcie_ep *ep)
>>   {
>> -	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct pci_epf_header *hdr = &(ep->cached_hdr);
>>
>>   	dw_pcie_dbi_ro_wr_en(pci);
>>   	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid); @@ -94,6
>> +93,19 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8
>> func_no,
>>   	dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
>>   			   hdr->interrupt_pin);
>>   	dw_pcie_dbi_ro_wr_dis(pci);
>> +}
>> +
>> +static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>> +				   struct pci_epf_header *hdr)
>> +{
>> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> +
>> +	ep->cached_hdr = *hdr;
>> +
>> +	if (!ep->hw_regs_available)
>> +		return 0;
>> +
>> +	dw_pcie_ep_write_header_regs(ep);
>>
>>   	return 0;
>>   }
>> @@ -112,6 +124,15 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
>> *ep, enum pci_barno bar,
>>   		return -EINVAL;
>>   	}
>>
>> +	ep->cached_inbound_atus[free_win].bar = bar;
>> +	ep->cached_inbound_atus[free_win].cpu_addr = cpu_addr;
>> +	ep->cached_inbound_atus[free_win].as_type = as_type;
>> +	ep->cached_bars[bar].atu_index = free_win;
>> +	set_bit(free_win, ep->ib_window_map);
>> +
>> +	if (!ep->hw_regs_available)
>> +		return 0;
>> +
>>   	ret = dw_pcie_prog_inbound_atu(pci, free_win, bar, cpu_addr,
>>   				       as_type);
>>   	if (ret < 0) {
>> @@ -119,9 +140,6 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
>> *ep, enum pci_barno bar,
>>   		return ret;
>>   	}
>>
>> -	ep->bar_to_atu[bar] = free_win;
>> -	set_bit(free_win, ep->ib_window_map);
>> -
>>   	return 0;
>>   }
>>
>> @@ -137,27 +155,66 @@ static int dw_pcie_ep_outbound_atu(struct
>> dw_pcie_ep *ep, phys_addr_t phys_addr,
>>   		return -EINVAL;
>>   	}
>>
>> +	ep->cached_outbound_atus[free_win].addr = phys_addr;
>> +	ep->cached_outbound_atus[free_win].pci_addr = pci_addr;
>> +	ep->cached_outbound_atus[free_win].size = size;
>> +	set_bit(free_win, ep->ob_window_map);
>> +
>> +	if (!ep->hw_regs_available)
>> +		return 0;
>> +
>>   	dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM,
>>   				  phys_addr, pci_addr, size);
>>
>> -	set_bit(free_win, ep->ob_window_map);
>> -	ep->outbound_addr[free_win] = phys_addr;
>> -
>>   	return 0;
>>   }
>>
>> +static void dw_pcie_ep_clear_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
>> +				      enum pci_barno bar)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	int flags = ep->cached_bars[bar].flags;
>> +	u32 atu_index = ep->cached_bars[bar].atu_index;
>> +
>> +	__dw_pcie_ep_reset_bar(pci, bar, flags);
>> +
>> +	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); }
>> +
>>   static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
>>   				 struct pci_epf_bar *epf_bar)
>>   {
>>   	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> -	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>   	enum pci_barno bar = epf_bar->barno;
>> -	u32 atu_index = ep->bar_to_atu[bar];
>> +	u32 atu_index = ep->cached_bars[bar].atu_index;
>>
>> -	__dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
>> -
>> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
>>   	clear_bit(atu_index, ep->ib_window_map);
>> +
>> +	if (!ep->hw_regs_available)
>> +		return;
>> +
>> +	dw_pcie_ep_clear_bar_regs(ep, func_no, bar); }
>> +
>> +static void dw_pcie_ep_set_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
>> +				    enum pci_barno bar)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	size_t size = ep->cached_bars[bar].size;
>> +	int flags = ep->cached_bars[bar].flags;
>> +	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>> +
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +
>> +	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>> +	dw_pcie_writel_dbi(pci, reg, flags);
>> +
>> +	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> +		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>> +		dw_pcie_writel_dbi(pci, reg + 4, 0);
>> +	}
>> +
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>>   }
>>
>>   static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, @@ -165,12
>> +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,  {
>>   	int ret;
>>   	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> -	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>   	enum pci_barno bar = epf_bar->barno;
>>   	size_t size = epf_bar->size;
>>   	int flags = epf_bar->flags;
>>   	enum dw_pcie_as_type as_type;
>> -	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>>
>>   	if (!(flags & PCI_BASE_ADDRESS_SPACE))
>>   		as_type = DW_PCIE_AS_MEM;
>> @@ -181,17 +236,13 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8
>> func_no,
>>   	if (ret)
>>   		return ret;
>>
>> -	dw_pcie_dbi_ro_wr_en(pci);
>> -
>> -	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>> -	dw_pcie_writel_dbi(pci, reg, flags);
>> +	ep->cached_bars[bar].size = size;
>> +	ep->cached_bars[bar].flags = flags;
>>
>> -	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> -		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>> -		dw_pcie_writel_dbi(pci, reg + 4, 0);
>> -	}
>> +	if (!ep->hw_regs_available)
>> +		return 0;
>>
>> -	dw_pcie_dbi_ro_wr_dis(pci);
>> +	dw_pcie_ep_set_bar_regs(ep, func_no, bar);
>>
>>   	return 0;
>>   }
>> @@ -202,7 +253,7 @@ static int dw_pcie_find_index(struct dw_pcie_ep *ep,
>> phys_addr_t addr,
>>   	u32 index;
>>
>>   	for (index = 0; index < ep->num_ob_windows; index++) {
>> -		if (ep->outbound_addr[index] != addr)
>> +		if (ep->cached_outbound_atus[index].addr != addr)
>>   			continue;
>>   		*atu_index = index;
>>   		return 0;
>> @@ -223,8 +274,12 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc
>> *epc, u8 func_no,
>>   	if (ret < 0)
>>   		return;
>>
>> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>   	clear_bit(atu_index, ep->ob_window_map);
>> +
>> +	if (!ep->hw_regs_available)
>> +		return;
>> +
>> +	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>   }
>>
>>   static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, @@ -254,7
>> +309,10 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no)
>>   		return -EINVAL;
>>
>>   	reg = ep->msi_cap + PCI_MSI_FLAGS;
>> -	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (ep->hw_regs_available)
>> +		val = dw_pcie_readw_dbi(pci, reg);
>> +	else
>> +		val = ep->cached_msi_flags;
>>   	if (!(val & PCI_MSI_FLAGS_ENABLE))
>>   		return -EINVAL;
>>
>> @@ -273,12 +331,19 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8
>> func_no, u8 interrupts)
>>   		return -EINVAL;
>>
>>   	reg = ep->msi_cap + PCI_MSI_FLAGS;
>> -	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (ep->hw_regs_available)
>> +		val = dw_pcie_readw_dbi(pci, reg);
>> +	else
>> +		val = ep->cached_msi_flags;
>>   	val &= ~PCI_MSI_FLAGS_QMASK;
>>   	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
>> -	dw_pcie_dbi_ro_wr_en(pci);
>> -	dw_pcie_writew_dbi(pci, reg, val);
>> -	dw_pcie_dbi_ro_wr_dis(pci);
>> +	if (ep->hw_regs_available) {
>> +		dw_pcie_dbi_ro_wr_en(pci);
>> +		dw_pcie_writew_dbi(pci, reg, val);
>> +		dw_pcie_dbi_ro_wr_dis(pci);
>> +	} else {
>> +		ep->cached_msi_flags = val;
>> +	}
>>
>>   	return 0;
>>   }
>> @@ -293,7 +358,10 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc,
>> u8 func_no)
>>   		return -EINVAL;
>>
>>   	reg = ep->msix_cap + PCI_MSIX_FLAGS;
>> -	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (ep->hw_regs_available)
>> +		val = dw_pcie_readw_dbi(pci, reg);
>> +	else
>> +		val = ep->cached_msix_flags;
>>   	if (!(val & PCI_MSIX_FLAGS_ENABLE))
>>   		return -EINVAL;
>>
>> @@ -312,16 +380,49 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc,
>> u8 func_no, u16 interrupts)
>>   		return -EINVAL;
>>
>>   	reg = ep->msix_cap + PCI_MSIX_FLAGS;
>> -	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (ep->hw_regs_available)
>> +		val = dw_pcie_readw_dbi(pci, reg);
>> +	else
>> +		val = ep->cached_msix_flags;
>>   	val &= ~PCI_MSIX_FLAGS_QSIZE;
>>   	val |= interrupts;
>> -	dw_pcie_dbi_ro_wr_en(pci);
>> -	dw_pcie_writew_dbi(pci, reg, val);
>> -	dw_pcie_dbi_ro_wr_dis(pci);
>> +	if (ep->hw_regs_available) {
>> +		dw_pcie_dbi_ro_wr_en(pci);
>> +		dw_pcie_writew_dbi(pci, reg, val);
>> +		dw_pcie_dbi_ro_wr_dis(pci);
>> +	} else {
>> +		ep->cached_msix_flags = val;
>> +	}
>>
>>   	return 0;
>>   }
>>
>> +void dw_pcie_set_regs_available(struct dw_pcie *pci) {
>> +	struct dw_pcie_ep *ep = &(pci->ep);
>> +	int i;
>> +
>> +	ep->hw_regs_available = true;
>> +
>> +	dw_pcie_ep_write_header_regs(ep);
>> +	for_each_set_bit(i, ep->ib_window_map, ep->num_ib_windows) {
>> +		dw_pcie_prog_inbound_atu(pci, i,
>> +			ep->cached_inbound_atus[i].bar,
>> +			ep->cached_inbound_atus[i].cpu_addr,
>> +			ep->cached_inbound_atus[i].as_type);
>> +		dw_pcie_ep_set_bar_regs(ep, 0, ep-
>>> cached_inbound_atus[i].bar);
>> +	}
>> +	for_each_set_bit(i, ep->ob_window_map, ep->num_ob_windows)
>> +		dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
>> +			ep->cached_outbound_atus[i].addr,
>> +			ep->cached_outbound_atus[i].pci_addr,
>> +			ep->cached_outbound_atus[i].size);
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writew_dbi(pci, PCI_MSI_FLAGS, ep->cached_msi_flags);
>> +	dw_pcie_writew_dbi(pci, PCI_MSIX_FLAGS, ep->cached_msix_flags);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +}
>> +
>>   static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>>   				enum pci_epc_irq_type type, u16
>> interrupt_num)  { @@ -330,6 +431,9 @@ static int dw_pcie_ep_raise_irq(struct
>> pci_epc *epc, u8 func_no,
>>   	if (!ep->ops->raise_irq)
>>   		return -EINVAL;
>>
>> +	if (!ep->hw_regs_available)
>> +		return -EAGAIN;
>> +
>>   	return ep->ops->raise_irq(ep, func_no, type, interrupt_num);  }
>>
>> @@ -391,6 +495,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
>> u8 func_no,
>>   	bool has_upper;
>>   	int ret;
>>
>> +	if (!ep->hw_regs_available)
>> +		return -EAGAIN;
>> +
>>   	if (!ep->msi_cap)
>>   		return -EINVAL;
>>
>> @@ -436,6 +543,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep,
>> u8 func_no,
>>   	void __iomem *msix_tbl;
>>   	int ret;
>>
>> +	if (!ep->hw_regs_available)
>> +		return -EAGAIN;
>> +
>>   	reg = ep->msix_cap + PCI_MSIX_TABLE;
>>   	tbl_offset = dw_pcie_readl_dbi(pci, reg);
>>   	bir = (tbl_offset & PCI_MSIX_TABLE_BIR); @@ -494,7 +604,6 @@ void
>> dw_pcie_ep_exit(struct dw_pcie_ep *ep)  int dw_pcie_ep_init(struct
>> dw_pcie_ep *ep)  {
>>   	int ret;
>> -	void *addr;
>>   	struct pci_epc *epc;
>>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>   	struct device *dev = pci->dev;
>> @@ -543,11 +652,17 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>   	if (!ep->ob_window_map)
>>   		return -ENOMEM;
>>
>> -	addr = devm_kcalloc(dev, ep->num_ob_windows, sizeof(phys_addr_t),
>> -			    GFP_KERNEL);
>> -	if (!addr)
>> +	ep->cached_inbound_atus = devm_kzalloc(dev,
>> +		sizeof(ep->cached_inbound_atus[0]) * ep->num_ib_windows,
>> +		GFP_KERNEL);
>> +	if (!ep->cached_inbound_atus)
>> +		return -ENOMEM;
>> +
>> +	ep->cached_outbound_atus = devm_kzalloc(dev,
>> +		sizeof(ep->cached_outbound_atus[0]) * ep->num_ob_windows,
>> +		GFP_KERNEL);
>> +	if (!ep->cached_outbound_atus)
>>   		return -ENOMEM;
>> -	ep->outbound_addr = addr;
>>
>>   	epc = devm_pci_epc_create(dev, &epc_ops);
>>   	if (IS_ERR(epc)) {
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
>> b/drivers/pci/controller/dwc/pcie-designware.h
>> index 02fb532c5db9..c116b289aa51 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -192,8 +192,6 @@ struct dw_pcie_ep {
>>   	phys_addr_t		phys_base;
>>   	size_t			addr_size;
>>   	size_t			page_size;
>> -	u8			bar_to_atu[6];
>> -	phys_addr_t		*outbound_addr;
>>   	unsigned long		*ib_window_map;
>>   	unsigned long		*ob_window_map;
>>   	u32			num_ib_windows;
>> @@ -202,6 +200,25 @@ struct dw_pcie_ep {
>>   	phys_addr_t		msi_mem_phys;
>>   	u8			msi_cap;	/* MSI capability offset */
>>   	u8			msix_cap;	/* MSI-X capability offset */
>> +	bool			hw_regs_available;
>> +	struct pci_epf_header	cached_hdr;
>> +	struct {
>> +		size_t			size;
>> +		int			flags;
>> +		u8			atu_index;
>> +	}			cached_bars[6];
>> +	struct {
>> +		enum pci_barno		bar;
>> +		dma_addr_t		cpu_addr;
>> +		enum dw_pcie_as_type	as_type;
>> +	}			*cached_inbound_atus;
>> +	struct {
>> +		phys_addr_t		addr;
>> +		u64			pci_addr;
>> +		size_t			size;
>> +	}			*cached_outbound_atus;
>> +	u32			cached_msi_flags;
>> +	u32			cached_msix_flags;
>>   };
>>
>>   struct dw_pcie_ops {
>> @@ -363,6 +380,7 @@ static inline int dw_pcie_allocate_domains(struct
>> pcie_port *pp)  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);  int
>> dw_pcie_ep_init(struct dw_pcie_ep *ep);  void dw_pcie_ep_exit(struct
>> dw_pcie_ep *ep);
>> +void dw_pcie_ep_set_regs_available(struct dw_pcie *pci);
>>   int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no);  int
>> dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>>   			     u8 interrupt_num);
>> @@ -383,6 +401,10 @@ static inline void dw_pcie_ep_exit(struct dw_pcie_ep
>> *ep)  {  }
>>
>> +static inline void dw_pcie_ep_set_regs_available(struct dw_pcie *pci) {
>> +}
>> +
>>   static inline int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8
>> func_no)  {
>>   	return 0;
>> --
>> 2.19.1
> 
> What is the guidance on setting "hw_regs_available" param?
> I mean, in case of hardware platforms where registers are always available to write (Ex:- pci-epf-test.c), how do they
> either (a) set the param hw_regs_available to '1' or (b) call dw_pcie_set_regs_available() to commit all cached values to registers?

Oh yes, I forgot to add that part when moving this patch upstream.

Right now, our downstream endpoint driver calls 
dw_pcie_set_regs_available() when it has made the registers available. 
Drivers for any hardware where the registers are always available (i.e. 
all current DWC EP drivers in mainline) should set ep->hw_regs_available 
during initialization. So, this patch needs to be amended to either:

a) Update every existing DWC EP driver to set ep->hw_regs_available 
during probe/initialization.

b) Replace ep->hw_regs_available with ep->hw_regs_unavailable, so that 
the default 0 value (assuming kzalloc is used to alloc the ep struct) 
means the registers can be accessed, with the ability for individual 
drivers (i.e. Tegra's) to set that value during probe/initialization.

Before I do that though, I'd still like general feedback on this patch 
so I can incorporate that into any updated version.

> I see a need for APIs to establish the link among dw_pcie_ep -> pci_epc -> pci_epf_driver.

I don't understand this part.

Vidya Sagar Nov. 20, 2018, 7:45 p.m. UTC | #3

On 20/11/18 11:11 PM, Stephen Warren wrote:
> On 11/20/18 10:30 AM, Vidya Sagar wrote:
>>> -----Original Message-----
>>> From: Stephen Warren <swarren@wwwdotorg.org>
>>> Sent: Tuesday, November 20, 2018 3:56 AM
>>> To: Jingoo Han <jingoohan1@gmail.com>; Gustavo Pimentel
>>> <gustavo.pimentel@synopsys.com>; Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: linux-pci@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; 
>>> Manikanta
>>> Maddireddy <mmaddireddy@nvidia.com>; Stephen Warren
>>> <swarren@nvidia.com>
>>> Subject: [PATCH] PCI: dwc ep: cache config until DBI regs available
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Some implementations of the DWC PCIe endpoint controller do not 
>>> allow access
>>> to DBI registers until the attached host has started REFCLK, 
>>> released PERST, and
>>> the endpoint driver has initialized clocking of the DBI registers 
>>> based on that.
>>> One such system is NVIDIA's T194 SoC. The PCIe endpoint subsystem 
>>> and DWC
>>> driver currently don't work on such hardware, since they assume that 
>>> all
>>> endpoint configuration can happen at any arbitrary time.
>>>
>>> Enhance the DWC endpoint driver to support such systems by caching all
>>> endpoint configuration in software, and only writing the 
>>> configuration to
>>> hardware once it's been initialized. This is implemented by 
>>> splitting all endpoint
>>> controller ops into two functions; the first which simply 
>>> records/caches the
>>> desired configuration whenever called by the associated function 
>>> driver and
>>> optionally calls the second, and the second which actually programs the
>>> configuration into hardware, which may be called either by the first 
>>> function, or
>>> later when it's known that the DBI registers are available.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>> Note: I have only compiled-tested this patch in the context of the 
>>> mainline
>>> kernel since NVIDIA Tegra PCIe EP support is not yet present.
>>> However, I've built and tested an equivalent patch in our downstream 
>>> kernel. I
>>> did have to manually port it to mainline due to conflicts though.
>>>
>>> This patch was developed on top of "PCI: designware: don't hard-code 
>>> DBI/ATU
>>> offset" which I sent last week, so may rely on that for patch context.
>>> ---
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 199 
>>> ++++++++++++++----
>>>   drivers/pci/controller/dwc/pcie-designware.h  |  26 ++-
>>>   2 files changed, 181 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> index 880210366e71..6910b64bfdcb 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> @@ -73,11 +73,10 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie
>>> *pci, u8 cap)
>>>       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)
>>> +static void dw_pcie_ep_write_header_regs(struct dw_pcie_ep *ep)
>>>   {
>>> -    struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    struct pci_epf_header *hdr = &(ep->cached_hdr);
>>>
>>>       dw_pcie_dbi_ro_wr_en(pci);
>>>       dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid); @@ -94,6
>>> +93,19 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8
>>> func_no,
>>>       dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
>>>                  hdr->interrupt_pin);
>>>       dw_pcie_dbi_ro_wr_dis(pci);
>>> +}
>>> +
>>> +static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>>> +                   struct pci_epf_header *hdr)
>>> +{
>>> +    struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> +
>>> +    ep->cached_hdr = *hdr;
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>> +    dw_pcie_ep_write_header_regs(ep);
>>>
>>>       return 0;
>>>   }
>>> @@ -112,6 +124,15 @@ static int dw_pcie_ep_inbound_atu(struct 
>>> dw_pcie_ep
>>> *ep, enum pci_barno bar,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    ep->cached_inbound_atus[free_win].bar = bar;
>>> +    ep->cached_inbound_atus[free_win].cpu_addr = cpu_addr;
>>> +    ep->cached_inbound_atus[free_win].as_type = as_type;
>>> +    ep->cached_bars[bar].atu_index = free_win;
>>> +    set_bit(free_win, ep->ib_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>>       ret = dw_pcie_prog_inbound_atu(pci, free_win, bar, cpu_addr,
>>>                          as_type);
>>>       if (ret < 0) {
>>> @@ -119,9 +140,6 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
>>> *ep, enum pci_barno bar,
>>>           return ret;
>>>       }
>>>
>>> -    ep->bar_to_atu[bar] = free_win;
>>> -    set_bit(free_win, ep->ib_window_map);
>>> -
>>>       return 0;
>>>   }
>>>
>>> @@ -137,27 +155,66 @@ static int dw_pcie_ep_outbound_atu(struct
>>> dw_pcie_ep *ep, phys_addr_t phys_addr,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    ep->cached_outbound_atus[free_win].addr = phys_addr;
>>> +    ep->cached_outbound_atus[free_win].pci_addr = pci_addr;
>>> +    ep->cached_outbound_atus[free_win].size = size;
>>> +    set_bit(free_win, ep->ob_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>>       dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM,
>>>                     phys_addr, pci_addr, size);
>>>
>>> -    set_bit(free_win, ep->ob_window_map);
>>> -    ep->outbound_addr[free_win] = phys_addr;
>>> -
>>>       return 0;
>>>   }
>>>
>>> +static void dw_pcie_ep_clear_bar_regs(struct dw_pcie_ep *ep, u8 
>>> func_no,
>>> +                      enum pci_barno bar)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    int flags = ep->cached_bars[bar].flags;
>>> +    u32 atu_index = ep->cached_bars[bar].atu_index;
>>> +
>>> +    __dw_pcie_ep_reset_bar(pci, bar, flags);
>>> +
>>> +    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); }
>>> +
>>>   static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
>>>                    struct pci_epf_bar *epf_bar)
>>>   {
>>>       struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> -    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       enum pci_barno bar = epf_bar->barno;
>>> -    u32 atu_index = ep->bar_to_atu[bar];
>>> +    u32 atu_index = ep->cached_bars[bar].atu_index;
>>>
>>> -    __dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
>>> -
>>> -    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
>>>       clear_bit(atu_index, ep->ib_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return;
>>> +
>>> +    dw_pcie_ep_clear_bar_regs(ep, func_no, bar); }
>>> +
>>> +static void dw_pcie_ep_set_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
>>> +                    enum pci_barno bar)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    size_t size = ep->cached_bars[bar].size;
>>> +    int flags = ep->cached_bars[bar].flags;
>>> +    u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>>> +
>>> +    dw_pcie_dbi_ro_wr_en(pci);
>>> +
>>> +    dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>>> +    dw_pcie_writel_dbi(pci, reg, flags);
>>> +
>>> +    if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>> +        dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>>> +        dw_pcie_writel_dbi(pci, reg + 4, 0);
>>> +    }
>>> +
>>> +    dw_pcie_dbi_ro_wr_dis(pci);
>>>   }
>>>
>>>   static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, @@ 
>>> -165,12
>>> +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 
>>> func_no,  {
>>>       int ret;
>>>       struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> -    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       enum pci_barno bar = epf_bar->barno;
>>>       size_t size = epf_bar->size;
>>>       int flags = epf_bar->flags;
>>>       enum dw_pcie_as_type as_type;
>>> -    u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>>>
>>>       if (!(flags & PCI_BASE_ADDRESS_SPACE))
>>>           as_type = DW_PCIE_AS_MEM;
>>> @@ -181,17 +236,13 @@ static int dw_pcie_ep_set_bar(struct pci_epc 
>>> *epc, u8
>>> func_no,
>>>       if (ret)
>>>           return ret;
>>>
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -
>>> -    dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>>> -    dw_pcie_writel_dbi(pci, reg, flags);
>>> +    ep->cached_bars[bar].size = size;
>>> +    ep->cached_bars[bar].flags = flags;
>>>
>>> -    if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>> -        dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>>> -        dw_pcie_writel_dbi(pci, reg + 4, 0);
>>> -    }
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>>
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    dw_pcie_ep_set_bar_regs(ep, func_no, bar);
>>>
>>>       return 0;
>>>   }
>>> @@ -202,7 +253,7 @@ static int dw_pcie_find_index(struct dw_pcie_ep 
>>> *ep,
>>> phys_addr_t addr,
>>>       u32 index;
>>>
>>>       for (index = 0; index < ep->num_ob_windows; index++) {
>>> -        if (ep->outbound_addr[index] != addr)
>>> +        if (ep->cached_outbound_atus[index].addr != addr)
>>>               continue;
>>>           *atu_index = index;
>>>           return 0;
>>> @@ -223,8 +274,12 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc
>>> *epc, u8 func_no,
>>>       if (ret < 0)
>>>           return;
>>>
>>> -    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>>       clear_bit(atu_index, ep->ob_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return;
>>> +
>>> +    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>>   }
>>>
>>>   static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, @@ 
>>> -254,7
>>> +309,10 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 
>>> func_no)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msi_cap + PCI_MSI_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msi_flags;
>>>       if (!(val & PCI_MSI_FLAGS_ENABLE))
>>>           return -EINVAL;
>>>
>>> @@ -273,12 +331,19 @@ static int dw_pcie_ep_set_msi(struct pci_epc 
>>> *epc, u8
>>> func_no, u8 interrupts)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msi_cap + PCI_MSI_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msi_flags;
>>>       val &= ~PCI_MSI_FLAGS_QMASK;
>>>       val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -    dw_pcie_writew_dbi(pci, reg, val);
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    if (ep->hw_regs_available) {
>>> +        dw_pcie_dbi_ro_wr_en(pci);
>>> +        dw_pcie_writew_dbi(pci, reg, val);
>>> +        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    } else {
>>> +        ep->cached_msi_flags = val;
>>> +    }
>>>
>>>       return 0;
>>>   }
>>> @@ -293,7 +358,10 @@ static int dw_pcie_ep_get_msix(struct pci_epc 
>>> *epc,
>>> u8 func_no)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msix_cap + PCI_MSIX_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msix_flags;
>>>       if (!(val & PCI_MSIX_FLAGS_ENABLE))
>>>           return -EINVAL;
>>>
>>> @@ -312,16 +380,49 @@ static int dw_pcie_ep_set_msix(struct pci_epc 
>>> *epc,
>>> u8 func_no, u16 interrupts)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msix_cap + PCI_MSIX_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msix_flags;
>>>       val &= ~PCI_MSIX_FLAGS_QSIZE;
>>>       val |= interrupts;
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -    dw_pcie_writew_dbi(pci, reg, val);
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    if (ep->hw_regs_available) {
>>> +        dw_pcie_dbi_ro_wr_en(pci);
>>> +        dw_pcie_writew_dbi(pci, reg, val);
>>> +        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    } else {
>>> +        ep->cached_msix_flags = val;
>>> +    }
>>>
>>>       return 0;
>>>   }
>>>
>>> +void dw_pcie_set_regs_available(struct dw_pcie *pci) {
>>> +    struct dw_pcie_ep *ep = &(pci->ep);
>>> +    int i;
>>> +
>>> +    ep->hw_regs_available = true;
>>> +
>>> +    dw_pcie_ep_write_header_regs(ep);
>>> +    for_each_set_bit(i, ep->ib_window_map, ep->num_ib_windows) {
>>> +        dw_pcie_prog_inbound_atu(pci, i,
>>> +            ep->cached_inbound_atus[i].bar,
>>> +            ep->cached_inbound_atus[i].cpu_addr,
>>> +            ep->cached_inbound_atus[i].as_type);
>>> +        dw_pcie_ep_set_bar_regs(ep, 0, ep-
>>>> cached_inbound_atus[i].bar);
>>> +    }
>>> +    for_each_set_bit(i, ep->ob_window_map, ep->num_ob_windows)
>>> +        dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
>>> +            ep->cached_outbound_atus[i].addr,
>>> +            ep->cached_outbound_atus[i].pci_addr,
>>> +            ep->cached_outbound_atus[i].size);
>>> +    dw_pcie_dbi_ro_wr_en(pci);
>>> +    dw_pcie_writew_dbi(pci, PCI_MSI_FLAGS, ep->cached_msi_flags);
>>> +    dw_pcie_writew_dbi(pci, PCI_MSIX_FLAGS, ep->cached_msix_flags);
>>> +    dw_pcie_dbi_ro_wr_dis(pci);
>>> +}
>>> +
>>>   static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>>>                   enum pci_epc_irq_type type, u16
>>> interrupt_num)  { @@ -330,6 +431,9 @@ static int 
>>> dw_pcie_ep_raise_irq(struct
>>> pci_epc *epc, u8 func_no,
>>>       if (!ep->ops->raise_irq)
>>>           return -EINVAL;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       return ep->ops->raise_irq(ep, func_no, type, interrupt_num);  }
>>>
>>> @@ -391,6 +495,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
>>> u8 func_no,
>>>       bool has_upper;
>>>       int ret;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       if (!ep->msi_cap)
>>>           return -EINVAL;
>>>
>>> @@ -436,6 +543,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep 
>>> *ep,
>>> u8 func_no,
>>>       void __iomem *msix_tbl;
>>>       int ret;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       reg = ep->msix_cap + PCI_MSIX_TABLE;
>>>       tbl_offset = dw_pcie_readl_dbi(pci, reg);
>>>       bir = (tbl_offset & PCI_MSIX_TABLE_BIR); @@ -494,7 +604,6 @@ void
>>> dw_pcie_ep_exit(struct dw_pcie_ep *ep)  int dw_pcie_ep_init(struct
>>> dw_pcie_ep *ep)  {
>>>       int ret;
>>> -    void *addr;
>>>       struct pci_epc *epc;
>>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       struct device *dev = pci->dev;
>>> @@ -543,11 +652,17 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>       if (!ep->ob_window_map)
>>>           return -ENOMEM;
>>>
>>> -    addr = devm_kcalloc(dev, ep->num_ob_windows, sizeof(phys_addr_t),
>>> -                GFP_KERNEL);
>>> -    if (!addr)
>>> +    ep->cached_inbound_atus = devm_kzalloc(dev,
>>> +        sizeof(ep->cached_inbound_atus[0]) * ep->num_ib_windows,
>>> +        GFP_KERNEL);
>>> +    if (!ep->cached_inbound_atus)
>>> +        return -ENOMEM;
>>> +
>>> +    ep->cached_outbound_atus = devm_kzalloc(dev,
>>> +        sizeof(ep->cached_outbound_atus[0]) * ep->num_ob_windows,
>>> +        GFP_KERNEL);
>>> +    if (!ep->cached_outbound_atus)
>>>           return -ENOMEM;
>>> -    ep->outbound_addr = addr;
>>>
>>>       epc = devm_pci_epc_create(dev, &epc_ops);
>>>       if (IS_ERR(epc)) {
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
>>> b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 02fb532c5db9..c116b289aa51 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -192,8 +192,6 @@ struct dw_pcie_ep {
>>>       phys_addr_t        phys_base;
>>>       size_t            addr_size;
>>>       size_t            page_size;
>>> -    u8            bar_to_atu[6];
>>> -    phys_addr_t        *outbound_addr;
>>>       unsigned long        *ib_window_map;
>>>       unsigned long        *ob_window_map;
>>>       u32            num_ib_windows;
>>> @@ -202,6 +200,25 @@ struct dw_pcie_ep {
>>>       phys_addr_t        msi_mem_phys;
>>>       u8            msi_cap;    /* MSI capability offset */
>>>       u8            msix_cap;    /* MSI-X capability offset */
>>> +    bool            hw_regs_available;
>>> +    struct pci_epf_header    cached_hdr;
>>> +    struct {
>>> +        size_t            size;
>>> +        int            flags;
>>> +        u8            atu_index;
>>> +    }            cached_bars[6];
>>> +    struct {
>>> +        enum pci_barno        bar;
>>> +        dma_addr_t        cpu_addr;
>>> +        enum dw_pcie_as_type    as_type;
>>> +    }            *cached_inbound_atus;
>>> +    struct {
>>> +        phys_addr_t        addr;
>>> +        u64            pci_addr;
>>> +        size_t            size;
>>> +    }            *cached_outbound_atus;
>>> +    u32            cached_msi_flags;
>>> +    u32            cached_msix_flags;
>>>   };
>>>
>>>   struct dw_pcie_ops {
>>> @@ -363,6 +380,7 @@ static inline int dw_pcie_allocate_domains(struct
>>> pcie_port *pp)  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);  int
>>> dw_pcie_ep_init(struct dw_pcie_ep *ep);  void dw_pcie_ep_exit(struct
>>> dw_pcie_ep *ep);
>>> +void dw_pcie_ep_set_regs_available(struct dw_pcie *pci);
>>>   int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 
>>> func_no);  int
>>> dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>                    u8 interrupt_num);
>>> @@ -383,6 +401,10 @@ static inline void dw_pcie_ep_exit(struct 
>>> dw_pcie_ep
>>> *ep)  {  }
>>>
>>> +static inline void dw_pcie_ep_set_regs_available(struct dw_pcie 
>>> *pci) {
>>> +}
>>> +
>>>   static inline int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep 
>>> *ep, u8
>>> func_no)  {
>>>       return 0;
>>> -- 
>>> 2.19.1
>>
>> What is the guidance on setting "hw_regs_available" param?
>> I mean, in case of hardware platforms where registers are always 
>> available to write (Ex:- pci-epf-test.c), how do they
>> either (a) set the param hw_regs_available to '1' or (b) call 
>> dw_pcie_set_regs_available() to commit all cached values to registers?
>
> Oh yes, I forgot to add that part when moving this patch upstream.
>
> Right now, our downstream endpoint driver calls 
> dw_pcie_set_regs_available() when it has made the registers available. 
> Drivers for any hardware where the registers are always available 
> (i.e. all current DWC EP drivers in mainline) should set 
> ep->hw_regs_available during initialization. So, this patch needs to 
> be amended to either:
>
> a) Update every existing DWC EP driver to set ep->hw_regs_available 
> during probe/initialization.
>
> b) Replace ep->hw_regs_available with ep->hw_regs_unavailable, so that 
> the default 0 value (assuming kzalloc is used to alloc the ep struct) 
> means the registers can be accessed, with the ability for individual 
> drivers (i.e. Tegra's) to set that value during probe/initialization.
>
> Before I do that though, I'd still like general feedback on this patch 
> so I can incorporate that into any updated version.
>
>> I see a need for APIs to establish the link among dw_pcie_ep -> 
>> pci_epc -> pci_epf_driver.
>
> I don't understand this part.
Nevermind. With option-(a) above, it is not required.

Vidya Sagar Nov. 20, 2018, 8 p.m. UTC | #4

On 20/11/18 11:11 PM, Stephen Warren wrote:
> On 11/20/18 10:30 AM, Vidya Sagar wrote:
>>> -----Original Message-----
>>> From: Stephen Warren <swarren@wwwdotorg.org>
>>> Sent: Tuesday, November 20, 2018 3:56 AM
>>> To: Jingoo Han <jingoohan1@gmail.com>; Gustavo Pimentel
>>> <gustavo.pimentel@synopsys.com>; Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: linux-pci@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; 
>>> Manikanta
>>> Maddireddy <mmaddireddy@nvidia.com>; Stephen Warren
>>> <swarren@nvidia.com>
>>> Subject: [PATCH] PCI: dwc ep: cache config until DBI regs available
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Some implementations of the DWC PCIe endpoint controller do not 
>>> allow access
>>> to DBI registers until the attached host has started REFCLK, 
>>> released PERST, and
>>> the endpoint driver has initialized clocking of the DBI registers 
>>> based on that.
>>> One such system is NVIDIA's T194 SoC. The PCIe endpoint subsystem 
>>> and DWC
>>> driver currently don't work on such hardware, since they assume that 
>>> all
>>> endpoint configuration can happen at any arbitrary time.
>>>
>>> Enhance the DWC endpoint driver to support such systems by caching all
>>> endpoint configuration in software, and only writing the 
>>> configuration to
>>> hardware once it's been initialized. This is implemented by 
>>> splitting all endpoint
>>> controller ops into two functions; the first which simply 
>>> records/caches the
>>> desired configuration whenever called by the associated function 
>>> driver and
>>> optionally calls the second, and the second which actually programs the
>>> configuration into hardware, which may be called either by the first 
>>> function, or
>>> later when it's known that the DBI registers are available.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>> Note: I have only compiled-tested this patch in the context of the 
>>> mainline
>>> kernel since NVIDIA Tegra PCIe EP support is not yet present.
>>> However, I've built and tested an equivalent patch in our downstream 
>>> kernel. I
>>> did have to manually port it to mainline due to conflicts though.
>>>
>>> This patch was developed on top of "PCI: designware: don't hard-code 
>>> DBI/ATU
>>> offset" which I sent last week, so may rely on that for patch context.
>>> ---
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 199 
>>> ++++++++++++++----
>>>   drivers/pci/controller/dwc/pcie-designware.h  |  26 ++-
>>>   2 files changed, 181 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> index 880210366e71..6910b64bfdcb 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> @@ -73,11 +73,10 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie
>>> *pci, u8 cap)
>>>       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)
>>> +static void dw_pcie_ep_write_header_regs(struct dw_pcie_ep *ep)
>>>   {
>>> -    struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    struct pci_epf_header *hdr = &(ep->cached_hdr);
>>>
>>>       dw_pcie_dbi_ro_wr_en(pci);
>>>       dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid); @@ -94,6
>>> +93,19 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8
>>> func_no,
>>>       dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
>>>                  hdr->interrupt_pin);
>>>       dw_pcie_dbi_ro_wr_dis(pci);
>>> +}
>>> +
>>> +static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>>> +                   struct pci_epf_header *hdr)
>>> +{
>>> +    struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> +
>>> +    ep->cached_hdr = *hdr;
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>> +    dw_pcie_ep_write_header_regs(ep);
>>>
>>>       return 0;
>>>   }
>>> @@ -112,6 +124,15 @@ static int dw_pcie_ep_inbound_atu(struct 
>>> dw_pcie_ep
>>> *ep, enum pci_barno bar,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    ep->cached_inbound_atus[free_win].bar = bar;
>>> +    ep->cached_inbound_atus[free_win].cpu_addr = cpu_addr;
>>> +    ep->cached_inbound_atus[free_win].as_type = as_type;
>>> +    ep->cached_bars[bar].atu_index = free_win;
>>> +    set_bit(free_win, ep->ib_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>>       ret = dw_pcie_prog_inbound_atu(pci, free_win, bar, cpu_addr,
>>>                          as_type);
>>>       if (ret < 0) {
>>> @@ -119,9 +140,6 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep
>>> *ep, enum pci_barno bar,
>>>           return ret;
>>>       }
>>>
>>> -    ep->bar_to_atu[bar] = free_win;
>>> -    set_bit(free_win, ep->ib_window_map);
>>> -
>>>       return 0;
>>>   }
>>>
>>> @@ -137,27 +155,66 @@ static int dw_pcie_ep_outbound_atu(struct
>>> dw_pcie_ep *ep, phys_addr_t phys_addr,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    ep->cached_outbound_atus[free_win].addr = phys_addr;
>>> +    ep->cached_outbound_atus[free_win].pci_addr = pci_addr;
>>> +    ep->cached_outbound_atus[free_win].size = size;
>>> +    set_bit(free_win, ep->ob_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>> +
>>>       dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM,
>>>                     phys_addr, pci_addr, size);
>>>
>>> -    set_bit(free_win, ep->ob_window_map);
>>> -    ep->outbound_addr[free_win] = phys_addr;
>>> -
>>>       return 0;
>>>   }
>>>
>>> +static void dw_pcie_ep_clear_bar_regs(struct dw_pcie_ep *ep, u8 
>>> func_no,
>>> +                      enum pci_barno bar)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    int flags = ep->cached_bars[bar].flags;
>>> +    u32 atu_index = ep->cached_bars[bar].atu_index;
>>> +
>>> +    __dw_pcie_ep_reset_bar(pci, bar, flags);
>>> +
>>> +    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); }
>>> +
>>>   static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
>>>                    struct pci_epf_bar *epf_bar)
>>>   {
>>>       struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> -    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       enum pci_barno bar = epf_bar->barno;
>>> -    u32 atu_index = ep->bar_to_atu[bar];
>>> +    u32 atu_index = ep->cached_bars[bar].atu_index;
>>>
>>> -    __dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
>>> -
>>> -    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
>>>       clear_bit(atu_index, ep->ib_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return;
>>> +
>>> +    dw_pcie_ep_clear_bar_regs(ep, func_no, bar); }
>>> +
>>> +static void dw_pcie_ep_set_bar_regs(struct dw_pcie_ep *ep, u8 func_no,
>>> +                    enum pci_barno bar)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    size_t size = ep->cached_bars[bar].size;
>>> +    int flags = ep->cached_bars[bar].flags;
>>> +    u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>>> +
>>> +    dw_pcie_dbi_ro_wr_en(pci);
>>> +
>>> +    dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>>> +    dw_pcie_writel_dbi(pci, reg, flags);
>>> +
>>> +    if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>> +        dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>>> +        dw_pcie_writel_dbi(pci, reg + 4, 0);
>>> +    }
>>> +
>>> +    dw_pcie_dbi_ro_wr_dis(pci);
>>>   }
>>>
>>>   static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, @@ 
>>> -165,12
>>> +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 
>>> func_no,  {
>>>       int ret;
>>>       struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>> -    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       enum pci_barno bar = epf_bar->barno;
>>>       size_t size = epf_bar->size;
>>>       int flags = epf_bar->flags;
>>>       enum dw_pcie_as_type as_type;
>>> -    u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>>>
>>>       if (!(flags & PCI_BASE_ADDRESS_SPACE))
>>>           as_type = DW_PCIE_AS_MEM;
>>> @@ -181,17 +236,13 @@ static int dw_pcie_ep_set_bar(struct pci_epc 
>>> *epc, u8
>>> func_no,
>>>       if (ret)
>>>           return ret;
>>>
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -
>>> -    dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>>> -    dw_pcie_writel_dbi(pci, reg, flags);
>>> +    ep->cached_bars[bar].size = size;
>>> +    ep->cached_bars[bar].flags = flags;
>>>
>>> -    if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>> -        dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>>> -        dw_pcie_writel_dbi(pci, reg + 4, 0);
>>> -    }
>>> +    if (!ep->hw_regs_available)
>>> +        return 0;
>>>
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    dw_pcie_ep_set_bar_regs(ep, func_no, bar);
>>>
>>>       return 0;
>>>   }
>>> @@ -202,7 +253,7 @@ static int dw_pcie_find_index(struct dw_pcie_ep 
>>> *ep,
>>> phys_addr_t addr,
>>>       u32 index;
>>>
>>>       for (index = 0; index < ep->num_ob_windows; index++) {
>>> -        if (ep->outbound_addr[index] != addr)
>>> +        if (ep->cached_outbound_atus[index].addr != addr)
>>>               continue;
>>>           *atu_index = index;
>>>           return 0;
>>> @@ -223,8 +274,12 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc
>>> *epc, u8 func_no,
>>>       if (ret < 0)
>>>           return;
>>>
>>> -    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>>       clear_bit(atu_index, ep->ob_window_map);
>>> +
>>> +    if (!ep->hw_regs_available)
>>> +        return;
>>> +
>>> +    dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
>>>   }
>>>
>>>   static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, @@ 
>>> -254,7
>>> +309,10 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 
>>> func_no)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msi_cap + PCI_MSI_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msi_flags;
>>>       if (!(val & PCI_MSI_FLAGS_ENABLE))
>>>           return -EINVAL;
>>>
>>> @@ -273,12 +331,19 @@ static int dw_pcie_ep_set_msi(struct pci_epc 
>>> *epc, u8
>>> func_no, u8 interrupts)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msi_cap + PCI_MSI_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msi_flags;
>>>       val &= ~PCI_MSI_FLAGS_QMASK;
>>>       val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -    dw_pcie_writew_dbi(pci, reg, val);
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    if (ep->hw_regs_available) {
>>> +        dw_pcie_dbi_ro_wr_en(pci);
>>> +        dw_pcie_writew_dbi(pci, reg, val);
>>> +        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    } else {
>>> +        ep->cached_msi_flags = val;
>>> +    }
>>>
>>>       return 0;
>>>   }
>>> @@ -293,7 +358,10 @@ static int dw_pcie_ep_get_msix(struct pci_epc 
>>> *epc,
>>> u8 func_no)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msix_cap + PCI_MSIX_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msix_flags;
>>>       if (!(val & PCI_MSIX_FLAGS_ENABLE))
>>>           return -EINVAL;
>>>
>>> @@ -312,16 +380,49 @@ static int dw_pcie_ep_set_msix(struct pci_epc 
>>> *epc,
>>> u8 func_no, u16 interrupts)
>>>           return -EINVAL;
>>>
>>>       reg = ep->msix_cap + PCI_MSIX_FLAGS;
>>> -    val = dw_pcie_readw_dbi(pci, reg);
>>> +    if (ep->hw_regs_available)
>>> +        val = dw_pcie_readw_dbi(pci, reg);
>>> +    else
>>> +        val = ep->cached_msix_flags;
>>>       val &= ~PCI_MSIX_FLAGS_QSIZE;
>>>       val |= interrupts;
>>> -    dw_pcie_dbi_ro_wr_en(pci);
>>> -    dw_pcie_writew_dbi(pci, reg, val);
>>> -    dw_pcie_dbi_ro_wr_dis(pci);
>>> +    if (ep->hw_regs_available) {
>>> +        dw_pcie_dbi_ro_wr_en(pci);
>>> +        dw_pcie_writew_dbi(pci, reg, val);
>>> +        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    } else {
>>> +        ep->cached_msix_flags = val;
>>> +    }
>>>
>>>       return 0;
>>>   }
>>>
>>> +void dw_pcie_set_regs_available(struct dw_pcie *pci) {
>>> +    struct dw_pcie_ep *ep = &(pci->ep);
>>> +    int i;
>>> +
>>> +    ep->hw_regs_available = true;
>>> +
>>> +    dw_pcie_ep_write_header_regs(ep);
>>> +    for_each_set_bit(i, ep->ib_window_map, ep->num_ib_windows) {
>>> +        dw_pcie_prog_inbound_atu(pci, i,
>>> +            ep->cached_inbound_atus[i].bar,
>>> +            ep->cached_inbound_atus[i].cpu_addr,
>>> +            ep->cached_inbound_atus[i].as_type);
>>> +        dw_pcie_ep_set_bar_regs(ep, 0, ep-
>>>> cached_inbound_atus[i].bar);
>>> +    }
>>> +    for_each_set_bit(i, ep->ob_window_map, ep->num_ob_windows)
>>> +        dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
>>> +            ep->cached_outbound_atus[i].addr,
>>> +            ep->cached_outbound_atus[i].pci_addr,
>>> +            ep->cached_outbound_atus[i].size);
>>> +    dw_pcie_dbi_ro_wr_en(pci);
>>> +    dw_pcie_writew_dbi(pci, PCI_MSI_FLAGS, ep->cached_msi_flags);
>>> +    dw_pcie_writew_dbi(pci, PCI_MSIX_FLAGS, ep->cached_msix_flags);
>>> +    dw_pcie_dbi_ro_wr_dis(pci);
>>> +}
>>> +
>>>   static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>>>                   enum pci_epc_irq_type type, u16
>>> interrupt_num)  { @@ -330,6 +431,9 @@ static int 
>>> dw_pcie_ep_raise_irq(struct
>>> pci_epc *epc, u8 func_no,
>>>       if (!ep->ops->raise_irq)
>>>           return -EINVAL;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       return ep->ops->raise_irq(ep, func_no, type, interrupt_num);  }
>>>
>>> @@ -391,6 +495,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
>>> u8 func_no,
>>>       bool has_upper;
>>>       int ret;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       if (!ep->msi_cap)
>>>           return -EINVAL;
>>>
>>> @@ -436,6 +543,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep 
>>> *ep,
>>> u8 func_no,
>>>       void __iomem *msix_tbl;
>>>       int ret;
>>>
>>> +    if (!ep->hw_regs_available)
>>> +        return -EAGAIN;
>>> +
>>>       reg = ep->msix_cap + PCI_MSIX_TABLE;
>>>       tbl_offset = dw_pcie_readl_dbi(pci, reg);
>>>       bir = (tbl_offset & PCI_MSIX_TABLE_BIR); @@ -494,7 +604,6 @@ void
>>> dw_pcie_ep_exit(struct dw_pcie_ep *ep)  int dw_pcie_ep_init(struct
>>> dw_pcie_ep *ep)  {
>>>       int ret;
>>> -    void *addr;
>>>       struct pci_epc *epc;
>>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       struct device *dev = pci->dev;
>>> @@ -543,11 +652,17 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>       if (!ep->ob_window_map)
>>>           return -ENOMEM;
>>>
>>> -    addr = devm_kcalloc(dev, ep->num_ob_windows, sizeof(phys_addr_t),
>>> -                GFP_KERNEL);
>>> -    if (!addr)
>>> +    ep->cached_inbound_atus = devm_kzalloc(dev,
>>> +        sizeof(ep->cached_inbound_atus[0]) * ep->num_ib_windows,
>>> +        GFP_KERNEL);
>>> +    if (!ep->cached_inbound_atus)
>>> +        return -ENOMEM;
>>> +
>>> +    ep->cached_outbound_atus = devm_kzalloc(dev,
>>> +        sizeof(ep->cached_outbound_atus[0]) * ep->num_ob_windows,
>>> +        GFP_KERNEL);
>>> +    if (!ep->cached_outbound_atus)
>>>           return -ENOMEM;
>>> -    ep->outbound_addr = addr;
>>>
>>>       epc = devm_pci_epc_create(dev, &epc_ops);
>>>       if (IS_ERR(epc)) {
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
>>> b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 02fb532c5db9..c116b289aa51 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -192,8 +192,6 @@ struct dw_pcie_ep {
>>>       phys_addr_t        phys_base;
>>>       size_t            addr_size;
>>>       size_t            page_size;
>>> -    u8            bar_to_atu[6];
>>> -    phys_addr_t        *outbound_addr;
>>>       unsigned long        *ib_window_map;
>>>       unsigned long        *ob_window_map;
>>>       u32            num_ib_windows;
>>> @@ -202,6 +200,25 @@ struct dw_pcie_ep {
>>>       phys_addr_t        msi_mem_phys;
>>>       u8            msi_cap;    /* MSI capability offset */
>>>       u8            msix_cap;    /* MSI-X capability offset */
>>> +    bool            hw_regs_available;
>>> +    struct pci_epf_header    cached_hdr;
>>> +    struct {
>>> +        size_t            size;
>>> +        int            flags;
>>> +        u8            atu_index;
>>> +    }            cached_bars[6];
>>> +    struct {
>>> +        enum pci_barno        bar;
>>> +        dma_addr_t        cpu_addr;
>>> +        enum dw_pcie_as_type    as_type;
>>> +    }            *cached_inbound_atus;
>>> +    struct {
>>> +        phys_addr_t        addr;
>>> +        u64            pci_addr;
>>> +        size_t            size;
>>> +    }            *cached_outbound_atus;
>>> +    u32            cached_msi_flags;
>>> +    u32            cached_msix_flags;
>>>   };
>>>
>>>   struct dw_pcie_ops {
>>> @@ -363,6 +380,7 @@ static inline int dw_pcie_allocate_domains(struct
>>> pcie_port *pp)  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);  int
>>> dw_pcie_ep_init(struct dw_pcie_ep *ep);  void dw_pcie_ep_exit(struct
>>> dw_pcie_ep *ep);
>>> +void dw_pcie_ep_set_regs_available(struct dw_pcie *pci);
>>>   int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 
>>> func_no);  int
>>> dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>                    u8 interrupt_num);
>>> @@ -383,6 +401,10 @@ static inline void dw_pcie_ep_exit(struct 
>>> dw_pcie_ep
>>> *ep)  {  }
>>>
>>> +static inline void dw_pcie_ep_set_regs_available(struct dw_pcie 
>>> *pci) {
>>> +}
>>> +
>>>   static inline int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep 
>>> *ep, u8
>>> func_no)  {
>>>       return 0;
>>> -- 
>>> 2.19.1
>>
>> What is the guidance on setting "hw_regs_available" param?
>> I mean, in case of hardware platforms where registers are always 
>> available to write (Ex:- pci-epf-test.c), how do they
>> either (a) set the param hw_regs_available to '1' or (b) call 
>> dw_pcie_set_regs_available() to commit all cached values to registers?
>
> Oh yes, I forgot to add that part when moving this patch upstream.
>
> Right now, our downstream endpoint driver calls 
> dw_pcie_set_regs_available() when it has made the registers available. 
> Drivers for any hardware where the registers are always available 
> (i.e. all current DWC EP drivers in mainline) should set 
> ep->hw_regs_available during initialization. So, this patch needs to 
> be amended to either:
>
> a) Update every existing DWC EP driver to set ep->hw_regs_available 
> during probe/initialization.
>
> b) Replace ep->hw_regs_available with ep->hw_regs_unavailable, so that 
> the default 0 value (assuming kzalloc is used to alloc the ep struct) 
> means the registers can be accessed, with the ability for individual 
> drivers (i.e. Tegra's) to set that value during probe/initialization.
>
> Before I do that though, I'd still like general feedback on this patch 
> so I can incorporate that into any updated version.
>
>> I see a need for APIs to establish the link among dw_pcie_ep -> 
>> pci_epc -> pci_epf_driver.
>
> I don't understand this part.
Nevermind. With Option-(a) above, it is not required anymore.

Trent Piepho Nov. 20, 2018, 8:12 p.m. UTC | #5

On Tue, 2018-11-20 at 10:41 -0700, Stephen Warren wrote:
> On 11/20/18 10:30 AM, Vidya Sagar wrote:
> > > 
> > > Some implementations of the DWC PCIe endpoint controller do not allow access
> > > to DBI registers until the attached host has started REFCLK, released PERST, and
> > > the endpoint driver has initialized clocking of the DBI registers based on that.
> > > 

> a) Update every existing DWC EP driver to set ep->hw_regs_available 
> during probe/initialization.

I believe on IMX7d, it is also the case that access to DWC PCIe
controller without PCI REFCLK causes a system hang.

We had a design with a mistake that didn't provide a REFCLK, and a hang
 starting the dwc controller is what happened.  It was fixed with an
external PCIe clock chip to provide the REFCLK input (as imx7d is
designed to expect), which, for the clock chip we used and the rest of
the external clock tree, means the REFCLK is there before Linux boots.

But there's no reason that must be the case.  Maybe the external clock
chip is I2C controlled and needs to be enabled?  And maybe I only want
to do that if PCI-e is to be used?

So I don't think if this is a specific problem for nvidia.

But I wonder, why can't you add REFCLK to the Linux clock tree and the
dwc bindings, and have the dwc driver EPROBE_DEFER if it can't
clk_prepare_enable() the refclk?

Stephen Warren Nov. 20, 2018, 8:21 p.m. UTC | #6

On 11/20/18 1:12 PM, Trent Piepho wrote:
> On Tue, 2018-11-20 at 10:41 -0700, Stephen Warren wrote:
>> On 11/20/18 10:30 AM, Vidya Sagar wrote:
>>>>
>>>> Some implementations of the DWC PCIe endpoint controller do not allow access
>>>> to DBI registers until the attached host has started REFCLK, released PERST, and
>>>> the endpoint driver has initialized clocking of the DBI registers based on that.
>>>>
> 
>> a) Update every existing DWC EP driver to set ep->hw_regs_available
>> during probe/initialization.
> 
> I believe on IMX7d, it is also the case that access to DWC PCIe
> controller without PCI REFCLK causes a system hang.
> 
> We had a design with a mistake that didn't provide a REFCLK, and a hang
>   starting the dwc controller is what happened.  It was fixed with an
> external PCIe clock chip to provide the REFCLK input (as imx7d is
> designed to expect), which, for the clock chip we used and the rest of
> the external clock tree, means the REFCLK is there before Linux boots.
> 
> But there's no reason that must be the case.  Maybe the external clock
> chip is I2C controlled and needs to be enabled?  And maybe I only want
> to do that if PCI-e is to be used?
> 
> So I don't think if this is a specific problem for nvidia.

Good to hear that this could be more generally useful.

> But I wonder, why can't you add REFCLK to the Linux clock tree and the
> dwc bindings, and have the dwc driver EPROBE_DEFER if it can't
> clk_prepare_enable() the refclk?

In our systems, we expect REFCLK to be provided by the system that 
contains the PCIe root port. There is no communication channel between 
the endpoint system and the root port system besides PCIe, so there's no 
way we could implement such clock control. The endpoint system must boot 
first, so that when the root port system boots, it is ready to be 
enumerated on the PCIe bus. To make this work, the Tegra endpoint driver 
calls dw_pcie_ep_set_regs_available() when it sees PERST release, and 
hopes to complete the DWC programming before the root port system begins 
PCIe enumeration.

PCI: dwc ep: cache config until DBI regs available

Commit Message

Comments

Patch