mbox series

[v14,00/21] PCI: rcar-gen4: Add R-Car Gen4 PCIe support

Message ID 20230426045557.3613826-1-yoshihiro.shimoda.uh@renesas.com
Headers show
Series PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand

Message

Yoshihiro Shimoda April 26, 2023, 4:55 a.m. UTC
Add R-Car S4-8 (R-Car Gen4) PCIe Host and Endpoint support.
To support them, modify PCIe DesignWare common codes.

Changes from v13:
https://lore.kernel.org/linux-pci/20230418122403.3178462-1-yoshihiro.shimoda.uh@renesas.com/
 - Based on next-20230424.
 - (no change, JFYI) Based on the following cleanups patches:
   [PATCH v4 00/14] PCI: dwc: Relatively simple fixes and cleanups
   https://lore.kernel.org/linux-pci/20230414021832.13167-1-Sergey.Semin@baikalelectronics.ru/
 - Fix typos in the patch 4/22 and 5/22.
 - Reviewed-by or Acked-by from Manivannan in the patch {4,5,10,16,18,21,22}/22.
 - Acked-by from Jesper in the patch 5/22.
 - Acked-by from Rob in the patch 16/22.
 - Merge the patch 8/22 to the 6/22.
 - Change the subject in the patch 9/22.
 - Fix incorrect implementation in the patch 11/22.
 - Fix issues in the patch 12/22.
 - Revise the descriptions in the patch 1[34569]/22.
 - Update copyright in the patch 1[78]/22.
 - Fix examples in the patch 1[78]/22.
 - Fix some minor issues in the patch 22/22.

Changes from v12:
https://lore.kernel.org/linux-pci/20230414061622.2930995-1-yoshihiro.shimoda.uh@renesas.com/
 - Based on next-20230414
 - (no change, JFYI) Based on the following cleanups patches:
   [PATCH v4 00/14] PCI: dwc: Relatively simple fixes and cleanups
   https://lore.kernel.org/linux-pci/20230414021832.13167-1-Sergey.Semin@baikalelectronics.ru/
 - Use PCI_HEADER_TYPE_MULTI_FUNC on probe.c and quirks.c.
 - Rename "Legacy IRQ" with "INTx".
 - Change order of patches. (INTx related patches at first.)
 - Change maxItems of reg and reg-names on the dt-bindings doc.
 - Fix minor typos.
 - Change waiting period in the INTx function and add comment.

Yoshihiro Shimoda (21):
  PCI: Add PCI_EXP_LNKCAP_MLW macros
  PCI: Add PCI_HEADER_TYPE_MULTI_FUNC
  PCI: Add INTx Mechanism Messages macros
  PCI: Rename PCI_EPC_IRQ_LEGACY to PCI_EPC_IRQ_INTX
  PCI: dwc: Rename "legacy_irq" to "INTx_irq" in DWC core
  PCI: dwc: Change arguments of dw_pcie_prog_ep_outbound_atu()
  PCI: dwc: Add members into struct dw_pcie_outbound_atu
  PCI: dwc: Add support for triggering INTx IRQs from endpoint drivers
  PCI: dwc: Add dw_pcie_link_set_max_link_width()
  PCI: dwc: Add dw_pcie_link_set_max_width()
  PCI: dwc: Add dw_pcie_link_set_max_cap_width()
  PCI: dwc: Add EDMA_UNROLL capability flag
  PCI: dwc: Expose dw_pcie_ep_exit() to module
  PCI: dwc: Introduce .ep_pre_init() and .ep_deinit()
  dt-bindings: PCI: dwc: Update maxItems of reg and reg-names
  dt-bindings: PCI: renesas: Add R-Car Gen4 PCIe Host
  dt-bindings: PCI: renesas: Add R-Car Gen4 PCIe Endpoint
  PCI: rcar-gen4: Add R-Car Gen4 PCIe Host support
  PCI: rcar-gen4-ep: Add R-Car Gen4 PCIe Endpoint support
  MAINTAINERS: Update PCI DRIVER FOR RENESAS R-CAR for R-Car Gen4
  misc: pci_endpoint_test: Add Device ID for R-Car S4-8 PCIe controller

 .../bindings/pci/rcar-gen4-pci-ep.yaml        |  98 +++++++++
 .../bindings/pci/rcar-gen4-pci-host.yaml      | 109 ++++++++++
 .../bindings/pci/snps,dw-pcie-ep.yaml         |   4 +-
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |   4 +-
 MAINTAINERS                                   |   1 +
 drivers/misc/pci_endpoint_test.c              |   4 +
 .../pci/controller/cadence/pcie-cadence-ep.c  |   2 +-
 drivers/pci/controller/dwc/Kconfig            |  18 ++
 drivers/pci/controller/dwc/Makefile           |   4 +
 drivers/pci/controller/dwc/pci-dra7xx.c       |   2 +-
 drivers/pci/controller/dwc/pci-imx6.c         |   4 +-
 drivers/pci/controller/dwc/pci-keystone.c     |   2 +-
 .../pci/controller/dwc/pci-layerscape-ep.c    |   4 +-
 drivers/pci/controller/dwc/pcie-artpec6.c     |   2 +-
 .../pci/controller/dwc/pcie-designware-ep.c   | 104 +++++++--
 .../pci/controller/dwc/pcie-designware-plat.c |   4 +-
 drivers/pci/controller/dwc/pcie-designware.c  | 202 +++++++++++-------
 drivers/pci/controller/dwc/pcie-designware.h  |  31 ++-
 drivers/pci/controller/dwc/pcie-keembay.c     |   2 +-
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |   4 +-
 .../pci/controller/dwc/pcie-rcar-gen4-ep.c    | 166 ++++++++++++++
 .../pci/controller/dwc/pcie-rcar-gen4-host.c  | 141 ++++++++++++
 drivers/pci/controller/dwc/pcie-rcar-gen4.c   | 187 ++++++++++++++++
 drivers/pci/controller/dwc/pcie-rcar-gen4.h   |  48 +++++
 drivers/pci/controller/dwc/pcie-tegra194.c    |   2 +-
 drivers/pci/controller/dwc/pcie-uniphier-ep.c |   2 +-
 drivers/pci/controller/pcie-rcar-ep.c         |   2 +-
 drivers/pci/controller/pcie-rockchip-ep.c     |   2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c |  12 +-
 drivers/pci/pci.h                             |  19 ++
 drivers/pci/probe.c                           |   2 +-
 drivers/pci/quirks.c                          |   4 +-
 include/linux/pci-epc.h                       |   4 +-
 include/uapi/linux/pci_regs.h                 |   7 +
 34 files changed, 1071 insertions(+), 132 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-ep.c
 create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4-host.c
 create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4.c
 create mode 100644 drivers/pci/controller/dwc/pcie-rcar-gen4.h

Comments

Serge Semin May 1, 2023, 5:34 p.m. UTC | #1
On Wed, Apr 26, 2023 at 01:55:39PM +0900, Yoshihiro Shimoda wrote:
> Add "Message Routing" and "INTx Mechanism Messages" macros to send
> a message by a PCIe driver.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  1 +
>  drivers/pci/pci.h                             | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f9182f8d552f..205bbcc6af27 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -9,6 +9,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  

> +#include "../../pci.h"

Unrelated change since the new macros are left unused in the framework
of this patch. Please move it to the patch which implies using the new
defines and where the included header file content is required.

>  #include "pcie-designware.h"
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2475098f6518..4be376c121a4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -11,6 +11,25 @@
>  
>  #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
>  
> +/* Message Routing */
> +#define PCI_MSG_ROUTING_RC	0
> +#define PCI_MSG_ROUTING_ADDR	1
> +#define PCI_MSG_ROUTING_ID	2
> +#define PCI_MSG_ROUTING_BC	3
> +#define PCI_MSG_ROUTING_LOCAL	4
> +#define PCI_MSG_ROUTING_GATHER	5
> +
> +/* INTx Mechanism Messages */
> +#define PCI_CODE_ASSERT_INTA	0x20
> +#define PCI_CODE_ASSERT_INTB	0x21
> +#define PCI_CODE_ASSERT_INTC	0x22
> +#define PCI_CODE_ASSERT_INTD	0x23
> +#define PCI_CODE_DEASSERT_INTA	0x24
> +#define PCI_CODE_DEASSERT_INTB	0x25
> +#define PCI_CODE_DEASSERT_INTC	0x26
> +#define PCI_CODE_DEASSERT_INTD	0x27
> +

> +

Excessive new line. Please drop it.

-Serge(y)

>  extern const unsigned char pcie_link_speed[];
>  extern bool pci_early_dump;
>  
> -- 
> 2.25.1
>
Serge Semin May 1, 2023, 6:58 p.m. UTC | #2
On Wed, Apr 26, 2023 at 01:55:42PM +0900, Yoshihiro Shimoda wrote:
> To add more arguments to the dw_pcie_prog_ep_outbound_atu() in
> the future, introduce struct dw_pcie_outbound_atu and change
> the argument. No behavior changes.

The change now looks much more coherent than before. Though it still looks
as an incomplete measure. The core driver still have two global outbound
ATU windows config methods which basically cause the same update
(performed by the same backend function), but which prototypes are
completely different. What about dropping the separate
dw_pcie_prog_outbound_atu() and dw_pcie_prog_outbound_atu() methods,
convert __dw_pcie_prog_outbound_atu() to dw_pcie_prog_outbound_atu(pci, atu)
and use it in both RP and EP drivers instead?

As a result we would have got a single outbound ATUs config method with
the next prototype:
int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, struct dw_pcie_ob_atu_cfg *atu);
Thus we would have reduced a number of the globally defined methods,
would have got a more unified outbound ATU setup interface which
by its nature would imply that the OB ATU entries setup is almost the
same for both RP and EP platforms.

Please see a few more comments below.

> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 21 ++++---
>  drivers/pci/controller/dwc/pcie-designware.c  | 63 ++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.h  | 13 +++-
>  3 files changed, 57 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index a80b9fd03638..96375b0aba82 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -183,9 +183,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
>  	return 0;
>  }
>  
> -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> -				   phys_addr_t phys_addr,
> -				   u64 pci_addr, size_t size)
> +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> +				   struct dw_pcie_outbound_atu *atu)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	u32 free_win;
> @@ -197,13 +196,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
>  		return -EINVAL;
>  	}
>  
> -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> -					   phys_addr, pci_addr, size);
> +	atu->index = free_win;
> +	ret = dw_pcie_prog_ep_outbound_atu(pci, atu);
>  	if (ret)
>  		return ret;
>  
>  	set_bit(free_win, ep->ob_window_map);
> -	ep->outbound_addr[free_win] = phys_addr;
> +	ep->outbound_addr[free_win] = atu->cpu_addr;
>  
>  	return 0;
>  }
> @@ -306,8 +305,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	int ret;
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> -
> -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> +	struct dw_pcie_outbound_atu atu = { 0 };
> +
> +	atu.func_no = func_no;
> +	atu.type = PCIE_ATU_TYPE_MEM;
> +	atu.cpu_addr = addr;
> +	atu.pci_addr = pci_addr;
> +	atu.size = size;
> +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
>  	if (ret) {
>  		dev_err(pci->dev, "Failed to enable address\n");
>  		return ret;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index ede166645289..782c4b34d0a3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -464,56 +464,55 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
>  	return val | PCIE_ATU_TD;
>  }
>  

> -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> -				       int index, int type, u64 cpu_addr,
> -				       u64 pci_addr, u64 size)
> +static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> +				       struct dw_pcie_outbound_atu *atu)
>  {
>  	u32 retries, val;
>  	u64 limit_addr;
>  
>  	if (pci->ops && pci->ops->cpu_addr_fixup)
> -		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> +		atu->cpu_addr = pci->ops->cpu_addr_fixup(pci, atu->cpu_addr);

This changes the method semantic a bit. The passed structure will be
updated meanwhile the former semantic implies the locally defined
variable modification. Please define a local var "cpu_addr" initialized
with the atu->cpu_addr field by default.

>  
> -	limit_addr = cpu_addr + size - 1;
> +	limit_addr = atu->cpu_addr + atu->size - 1;
>  
> -	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> -	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> +	if ((limit_addr & ~pci->region_limit) != (atu->cpu_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(atu->cpu_addr, pci->region_align) ||
> +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
>  		return -EINVAL;
>  	}
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> -			      lower_32_bits(cpu_addr));
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> -			      upper_32_bits(cpu_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> +			      lower_32_bits(atu->cpu_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> +			      upper_32_bits(atu->cpu_addr));
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
>  			      lower_32_bits(limit_addr));
>  	if (dw_pcie_ver_is_ge(pci, 460A))
> -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
>  				      upper_32_bits(limit_addr));
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> -			      lower_32_bits(pci_addr));
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> -			      upper_32_bits(pci_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> +			      lower_32_bits(atu->pci_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> +			      upper_32_bits(atu->pci_addr));
>  
> -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> -	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> +	if (upper_32_bits(limit_addr) > upper_32_bits(atu->cpu_addr) &&
>  	    dw_pcie_ver_is_ge(pci, 460A))
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
>  	if (dw_pcie_ver_is(pci, 490A))
>  		val = dw_pcie_enable_ecrc(val);
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
>  
>  	/*
>  	 * Make sure ATU enable takes effect before any subsequent config
>  	 * and I/O accesses.
>  	 */
>  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
>  		if (val & PCIE_ATU_ENABLE)
>  			return 0;
>  
> @@ -528,16 +527,20 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,

>  int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
>  			      u64 cpu_addr, u64 pci_addr, u64 size)
>  {
> -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> -					   cpu_addr, pci_addr, size);
> +	struct dw_pcie_outbound_atu atu = { 0 };
> +
> +	atu.index = index;
> +	atu.type = type;
> +	atu.cpu_addr = cpu_addr;
> +	atu.pci_addr = pci_addr;
> +	atu.size = size;
> +	return __dw_pcie_prog_outbound_atu(pci, &atu);
>  }
>  
> -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				 int type, u64 cpu_addr, u64 pci_addr,
> -				 u64 size)
> +int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci,
> +				 struct dw_pcie_outbound_atu *atu)
>  {
> -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> -					   cpu_addr, pci_addr, size);
> +	return __dw_pcie_prog_outbound_atu(pci, atu);
>  }

This could have been dropped if you got to implement what I suggested in
the head of the message.

>  
>  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 9acf6c40d252..81c7558a4718 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -291,6 +291,15 @@ enum dw_pcie_core_rst {
>  	DW_PCIE_NUM_CORE_RSTS
>  };
> 
 
> +struct dw_pcie_outbound_atu {

what about using the name "dw_pcie_ob_atu_cfg" instead?

> +	u64 cpu_addr;
> +	u64 pci_addr;
> +	u64 size;
> +	int index;
> +	int type;
> +	u8 func_no;

The structure will be padded by 7 bytes anyway. Let's move the "index",
"type" and "func_no" group to the head of the structure declaration.

> +};
> +
>  struct dw_pcie_host_ops {
>  	int (*host_init)(struct dw_pcie_rp *pp);
>  	void (*host_deinit)(struct dw_pcie_rp *pp);
> @@ -421,8 +430,8 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);

>  int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
>  			      u64 cpu_addr, u64 pci_addr, u64 size);
> -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> +int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci,
> +				 struct dw_pcie_outbound_atu *atu);

What about converting it to just a single:
dw_pcie_prog_outbound_atu(struct dw_pcie *pci, const struct dw_pcie_ob_atu_cfg *atu);
?

-Serge(y)

>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
>  			     u64 cpu_addr, u64 pci_addr, u64 size);
>  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -- 
> 2.25.1
>
Frank Li May 2, 2023, 5:04 p.m. UTC | #3
> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Monday, May 1, 2023 2:25 PM
> To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: jingoohan1@gmail.com; mani@kernel.org;
> gustavo.pimentel@synopsys.com; lpieralisi@kernel.org;
> robh+dt@kernel.org; kw@linux.com; bhelgaas@google.com;
> kishon@kernel.org; marek.vasut+renesas@gmail.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org
> Subject: [EXT] Re: [PATCH v14 08/21] PCI: dwc: Add support for triggering
> INTx IRQs from endpoint drivers
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Wed, Apr 26, 2023 at 01:55:44PM +0900, Yoshihiro Shimoda wrote:
> > Add support for triggering INTx IRQs by using outbound iATU.
> > Outbound iATU is utilized to send assert and de-assert INTx TLPs.
> > The message is generated based on the payloadless Msg TLP with type
> > 0x14, where 0x4 is the routing code implying the Terminate at
> > Receiver message. The message code is specified as b1000xx for
> > the INTx assertion and b1001xx for the INTx de-assertion.
> 
> [PATCH v14 08/21] PCI: dwc: Add support for triggering INTx IRQs from
> endpoint drivers
> 
> What about shortening the subject out a bit:
> "PCI: designware-ep: Add INTx IRQs support"
> ?
> 
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 71 +++++++++++++++++--
> >  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
> >  2 files changed, 69 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 96375b0aba82..b35ed2b06193 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -6,6 +6,7 @@
> >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> >   */
> >
> > +#include <linux/delay.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >
> > @@ -485,14 +486,63 @@ static const struct pci_epc_ops epc_ops = {
> >       .get_features           = dw_pcie_ep_get_features,
> >  };
> >
> > +static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8
> code,
> > +                            u8 routing)
> > +{
> > +     struct dw_pcie_outbound_atu atu = { 0 };
> > +     struct pci_epc *epc = ep->epc;
> > +     int ret;
> > +
> > +     atu.func_no = func_no;
> > +     atu.code = code;
> > +     atu.routing = routing;
> > +     atu.type = PCIE_ATU_TYPE_MSG;
> > +     atu.cpu_addr = ep->intx_mem_phys;
> > +     atu.size = epc->mem->window.page_size;
> > +
> > +     ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > +     if (ret)
> > +             return ret;
> > +
> > +     writel(0, ep->intx_mem);
> > +
> > +     dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
> > +
> > +     return 0;
> > +}
> > +
> 
> > +static int __dw_pcie_ep_raise_intx_irq(struct dw_pcie_ep *ep, u8
> func_no,
> > +                                      int intx)
> > +{
> > +     int ret;
> > +
> > +     ret = dw_pcie_ep_send_msg(ep, func_no, PCI_CODE_ASSERT_INTA +
> intx,
> > +                               PCI_MSG_ROUTING_LOCAL);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * The documents of PCIe and the controller don't mention how long
> > +      * the INTx should be asserted. If 10 usec, sometimes it failed.
> > +      * So, asserted for 50 usec.
> > +      */
> > +     usleep_range(50, 100);

It is good method to implement legacy irq support. But there is problem which need be considered. 

According to PCI spec, section 6.2.1 PCI-compatible INTx Emulation

PCI Express emulates the PCI interrupt mechanism including the Interrupt Pin and Interrupt Line registers of the PCI
Configuration Space for PCI device Functions. PCI Express non-Switch devices may optionally support these registers for
backwards compatibility. Switch devices are required to support them. Actual interrupt signaling uses in-band Messages
rather than being signaled using physical pins.
Two types of Messages are defined, Assert_INTx and Deassert_INTx, for emulation of PCI INTx signaling, where x is A, B,
C, and D for respective PCI interrupt signals. These Messages are used to provide "virtual wires" for signaling interrupts
across a Link. Switches collect these virtual wires and present a combined set at the Switch's Upstream Port. Ultimately,
the virtual wires are routed to the Root Complex which maps the virtual wires to system interrupt resources. Devices
must use assert/deassert Messages in pairs to emulate PCI interrupt **level-triggered** signaling. Actual mapping of PCI
Express INTx emulation to system interrupts is implementation specific as is mapping of physical interrupt signals in
conventional PCI.

It should be level triggered.   When call __dw_pcie_ep_raise_intx_irq, should be just assert INTx, then after PCI
Host driver's  irq handler to clear irq,  EP side can desert INTx. 

So it should be two functions, one function raise INTx,  and another one desert INTx.   But I don't know
How to avoid host side possible flood irq happen if EP can't desert INTx in time.  


> > +
> > +     return dw_pcie_ep_send_msg(ep, func_no,
> PCI_CODE_DEASSERT_INTA + intx,
> > +                                PCI_MSG_ROUTING_LOCAL);
> > +}
> 
> Why do you need the underscored version of the method? I don't see it
> being utilized anywhere but in the dw_pcie_ep_raise_intx_irq()
> function. Thus its body can be completely moved to
> dw_pcie_ep_raise_intx_irq().
> 
> -Serge(y)
> 
> > +
> >  int dw_pcie_ep_raise_intx_irq(struct dw_pcie_ep *ep, u8 func_no)
> >  {
> >       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >       struct device *dev = pci->dev;
> >
> > -     dev_err(dev, "EP cannot trigger INTx IRQs\n");
> > +     if (!ep->intx_mem) {
> > +             dev_err(dev, "INTx not supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> >
> > -     return -EINVAL;
> > +     return __dw_pcie_ep_raise_intx_irq(ep, func_no, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_raise_intx_irq);
> >
> > @@ -623,6 +673,10 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
> >
> >       dw_pcie_edma_remove(pci);
> >
> > +     if (ep->intx_mem)
> > +             pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep-
> >intx_mem,
> > +                                   epc->mem->window.page_size);
> > +
> >       pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
> >                             epc->mem->window.page_size);
> >
> > @@ -794,9 +848,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >               goto err_exit_epc_mem;
> >       }
> >
> > +     ep->intx_mem = pci_epc_mem_alloc_addr(epc, &ep-
> >intx_mem_phys,
> > +                                           epc->mem->window.page_size);
> > +     if (!ep->intx_mem)
> > +             dev_warn(dev, "Failed to reserve memory for INTx\n");
> > +
> >       ret = dw_pcie_edma_detect(pci);
> >       if (ret)
> > -             goto err_free_epc_mem;
> > +             goto err_free_epc_mem_intx;
> >
> >       if (ep->ops->get_features) {
> >               epc_features = ep->ops->get_features(ep);
> > @@ -813,7 +872,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  err_remove_edma:
> >       dw_pcie_edma_remove(pci);
> >
> > -err_free_epc_mem:
> > +err_free_epc_mem_intx:
> > +     if (ep->intx_mem)
> > +             pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep-
> >intx_mem,
> > +                                   epc->mem->window.page_size);
> > +
> >       pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
> >                             epc->mem->window.page_size);
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> > index 954d504890a1..8c08159ea08e 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -369,6 +369,8 @@ struct dw_pcie_ep {
> >       unsigned long           *ob_window_map;
> >       void __iomem            *msi_mem;
> >       phys_addr_t             msi_mem_phys;
> > +     void __iomem            *intx_mem;
> > +     phys_addr_t             intx_mem_phys;
> >       struct pci_epf_bar      *epf_bar[PCI_STD_NUM_BARS];
> >  };
> >
> > --
> > 2.25.1
> >