[PATCH v3 0/6] PCI: dwc: Add common pme_turn_off message by using outbound iATU

[PATCH v3 0/6] PCI: dwc: Add common pme_turn_off message by using outbound iATU

[Frank Li]

Involve an new and common mathod to send pme_turn_off() message. Previously
pme_turn_off() implement by platform related special register to trigge    
it.                                                                        
                                                                           
But Yoshihiro give good idea by using iATU to send out message. Previously 
Yoshihiro provide patches to raise INTx message by dummy write to outbound 
iATU.                                                                      
                                                                           
Use similar mathod to send out pme_turn_off message.                       
                                                                           
Previous two patches is picked from Yoshihiro' big patch serialise.        
 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()                 
 PCI: Add INTx Mechanism Messages macros                                   
                                                                           
PCI: Add PME_TURN_OFF message macro                                        
dt-bindings: PCI: dwc: Add 'msg" register region, Add "msg" region to use  
to map PCI msg.                                                            
                                                                           
PCI: dwc: Add common pme_turn_off message method                           
Using common pme_turn_off() message if platform have not define their.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v3:
- fix 'MSG"
- Add pcie spec ref in head file
- using function name dw_pci_pme_turn_off()
- Using PCIE_ prefix macro
- Link to v2: https://lore.kernel.org/r/20240201-pme_msg-v2-0-6767052fe6a4@nxp.com

Changes in v2:
  - Add my sign off at PCI: dwc: Add outbound MSG TLPs support
  - Add Bjorn review tag at  Add INTx Mechanism Messages macros
  - using PME_Turn_Off match PCIe spec
  - ref to pcie spec v6.1
  - using section number.

- Link to v1: https://lore.kernel.org/r/20240130-pme_msg-v1-0-d52b0add5c7c@nxp.com

---
Frank Li (3):
      PCI: Add PCIE_MSG_CODE_PME_TURN_OFF message macro
      dt-bindings: PCI: dwc: Add 'msg' register region
      PCI: dwc: Add common send PME_Turn_Off message method

Yoshihiro Shimoda (3):
      PCI: Add INTx Mechanism Messages macros
      PCI: dwc: Consolidate args of dw_pcie_prog_outbound_atu() into a structure
      PCI: dwc: Add outbound MSG TLPs support

 .../devicetree/bindings/pci/snps,dw-pcie.yaml      |   4 +
 drivers/pci/controller/dwc/pcie-designware-ep.c    |  21 +++--
 drivers/pci/controller/dwc/pcie-designware-host.c  | 104 +++++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.c       |  62 ++++++------
 drivers/pci/controller/dwc/pcie-designware.h       |  22 ++++-
 drivers/pci/pci.h                                  |  20 ++++
 6 files changed, 169 insertions(+), 64 deletions(-)
---
base-commit: e08fc59eee9991afa467d406d684d46d543299a9
change-id: 20240130-pme_msg-dd2d81ee9886

Best regards,
-- 
Frank Li <Frank.Li@nxp.com>

[PATCH v3 1/6] PCI: Add INTx Mechanism Messages macros

[Frank Li]

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Add "Message Routing" and "INTx Mechanism Messages" macros to enable
a PCIe driver to send messages for INTx Interrupt Signaling.

The "Message Routing" is in the section 2.2.8, and the "INTx Mechanism
Messages" is in the section 2.2.8.1 on the PCI Express Base Specification,
Rev 6.1.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/pci.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2336a8d1edab2..ffd066c15f3bb 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -22,6 +22,24 @@
  */
 #define PCIE_PME_TO_L2_TIMEOUT_US	10000
 
+/* Message Routing (r[2:0]) See: PCIe r6.0, sec 2.2.8 */
+#define PCIE_MSG_TYPE_R_RC	0
+#define PCIE_MSG_TYPE_R_ADDR	1
+#define PCIE_MSG_TYPE_R_ID	2
+#define PCIE_MSG_TYPE_R_BC	3
+#define PCIE_MSG_TYPE_R_LOCAL	4
+#define PCIE_MSG_TYPE_R_GATHER	5
+
+/* INTx Mechanism Messages See: PCIe r6.0, sec 2.2.8.1 */
+#define PCIE_MSG_CODE_ASSERT_INTA	0x20
+#define PCIE_MSG_CODE_ASSERT_INTB	0x21
+#define PCIE_MSG_CODE_ASSERT_INTC	0x22
+#define PCIE_MSG_CODE_ASSERT_INTD	0x23
+#define PCIE_MSG_CODE_DEASSERT_INTA	0x24
+#define PCIE_MSG_CODE_DEASSERT_INTB	0x25
+#define PCIE_MSG_CODE_DEASSERT_INTC	0x26
+#define PCIE_MSG_CODE_DEASSERT_INTD	0x27
+
 extern const unsigned char pcie_link_speed[];
 extern bool pci_early_dump;
 

-- 
2.34.1

[PATCH v3 2/6] PCI: dwc: Consolidate args of dw_pcie_prog_outbound_atu() into a structure

[Frank Li]

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

This is a preparation before adding the Msg-type outbound iATU
mapping. The respective update will require two more arguments added
to __dw_pcie_prog_outbound_atu(). That will make the already
complicated function prototype even more hard to comprehend accepting
_eight_ arguments. In order to prevent that and keep the code
more-or-less readable all the outbound iATU-related arguments are
moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
is supposed to be locally defined and populated with the outbound iATU
settings implied by the caller context.

As a result of the denoted change there is no longer need in having
the two distinctive methods for the Host and End-point outbound iATU
setups since the corresponding code can directly call the
dw_pcie_prog_outbound_atu() method with the config-structure
populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 21 +++++----
 drivers/pci/controller/dwc/pcie-designware-host.c | 52 ++++++++++++++++-------
 drivers/pci/controller/dwc/pcie-designware.c      | 49 ++++++++-------------
 drivers/pci/controller/dwc/pcie-designware.h      | 15 +++++--
 4 files changed, 77 insertions(+), 60 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 5befed2dc02b7..27956b2a73be7 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -159,9 +159,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_ob_atu_cfg *atu)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	u32 free_win;
@@ -173,13 +172,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_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;
 }
@@ -279,8 +278,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_ob_atu_cfg 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-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d5fc31f8345f7..267687ab33cbc 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int type, ret;
 	u32 busdev;
 
@@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 	else
 		type = PCIE_ATU_TYPE_CFG1;
 
-	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
-					pp->cfg0_size);
+	atu.type = type;
+	atu.cpu_addr = pp->cfg0_base;
+	atu.pci_addr = busdev;
+	atu.size = pp->cfg0_size;
+
+	ret = dw_pcie_prog_outbound_atu(pci, &atu);
 	if (ret)
 		return NULL;
 
@@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int ret;
 
 	ret = pci_generic_config_read(bus, devfn, where, size, val);
@@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
 		return ret;
 
 	if (pp->cfg0_io_shared) {
-		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
-						pp->io_base, pp->io_bus_addr,
-						pp->io_size);
+		atu.type = PCIE_ATU_TYPE_IO;
+		atu.cpu_addr = pp->io_base;
+		atu.pci_addr = pp->io_bus_addr;
+		atu.size = pp->io_size;
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret)
 			return PCIBIOS_SET_FAILED;
 	}
@@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int ret;
 
 	ret = pci_generic_config_write(bus, devfn, where, size, val);
@@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
 		return ret;
 
 	if (pp->cfg0_io_shared) {
-		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
-						pp->io_base, pp->io_bus_addr,
-						pp->io_size);
+		atu.type = PCIE_ATU_TYPE_IO;
+		atu.cpu_addr = pp->io_base;
+		atu.pci_addr = pp->io_bus_addr;
+		atu.size = pp->io_size;
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret)
 			return PCIBIOS_SET_FAILED;
 	}
@@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
 static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	struct resource_entry *entry;
 	int i, ret;
 
@@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		if (pci->num_ob_windows <= ++i)
 			break;
 
-		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
-						entry->res->start,
-						entry->res->start - entry->offset,
-						resource_size(entry->res));
+		atu.index = i;
+		atu.type = PCIE_ATU_TYPE_MEM;
+		atu.cpu_addr = entry->res->start;
+		atu.pci_addr = entry->res->start - entry->offset;
+		atu.size = resource_size(entry->res);
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret) {
 			dev_err(pci->dev, "Failed to set MEM range %pr\n",
 				entry->res);
@@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 
 	if (pp->io_size) {
 		if (pci->num_ob_windows > ++i) {
-			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
-							pp->io_base,
-							pp->io_bus_addr,
-							pp->io_size);
+			atu.index = i;
+			atu.type = PCIE_ATU_TYPE_IO;
+			atu.cpu_addr = pp->io_base;
+			atu.pci_addr = pp->io_bus_addr;
+			atu.size = pp->io_size;
+
+			ret = dw_pcie_prog_outbound_atu(pci, &atu);
 			if (ret) {
 				dev_err(pci->dev, "Failed to set IO range %pr\n",
 					entry->res);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 250cf7f40b858..df2575ec5f44c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -465,56 +465,56 @@ 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)
+int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
+			      const struct dw_pcie_ob_atu_cfg *atu)
 {
+	u64 cpu_addr = atu->cpu_addr;
 	u32 retries, val;
 	u64 limit_addr;
 
 	if (pci->ops && pci->ops->cpu_addr_fixup)
 		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
 
-	limit_addr = cpu_addr + size - 1;
+	limit_addr = 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) {
+	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
 		return -EINVAL;
 	}
 
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
 			      lower_32_bits(cpu_addr));
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
 			      upper_32_bits(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);
+	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(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;
 
@@ -526,21 +526,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 	return -ETIMEDOUT;
 }
 
-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);
-}
-
-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)
-{
-	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
-					   cpu_addr, pci_addr, size);
-}
-
 static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
 {
 	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 26dae48374627..d21db82e586d5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -299,6 +299,15 @@ enum dw_pcie_ltssm {
 	DW_PCIE_LTSSM_UNKNOWN = 0xFFFFFFFF,
 };
 
+struct dw_pcie_ob_atu_cfg {
+	int index;
+	int type;
+	u8 func_no;
+	u64 cpu_addr;
+	u64 pci_addr;
+	u64 size;
+};
+
 struct dw_pcie_host_ops {
 	int (*init)(struct dw_pcie_rp *pp);
 	void (*deinit)(struct dw_pcie_rp *pp);
@@ -434,10 +443,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
 int dw_pcie_link_up(struct dw_pcie *pci);
 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_outbound_atu(struct dw_pcie *pci,
+			      const struct dw_pcie_ob_atu_cfg *atu);
 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.34.1

[PATCH v3 3/6] PCI: dwc: Add outbound MSG TLPs support

[Frank Li]

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Add "code" and "routing" into struct dw_pcie_ob_atu_cfg for triggering
INTx IRQs by iATU in the PCIe endpoint mode in near the future.
PCIE_ATU_INHIBIT_PAYLOAD is set to issue TLP type of Msg instead of
MsgD. So, this implementation supports the data-less messages only
for now.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 9 +++++++--
 drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index df2575ec5f44c..ba909fade9db1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -499,7 +499,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
 			      upper_32_bits(atu->pci_addr));
 
-	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
+	val = atu->type | atu->routing | PCIE_ATU_FUNC_NUM(atu->func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
 	    dw_pcie_ver_is_ge(pci, 460A))
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
@@ -507,7 +507,12 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 		val = dw_pcie_enable_ecrc(val);
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
 
-	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
+	val = PCIE_ATU_ENABLE;
+	if (atu->type == PCIE_ATU_TYPE_MSG) {
+		/* The data-less messages only for now */
+		val |= PCIE_ATU_INHIBIT_PAYLOAD | atu->code;
+	}
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, val);
 
 	/*
 	 * Make sure ATU enable takes effect before any subsequent config
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index d21db82e586d5..703b50bc5e0f1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -148,11 +148,13 @@
 #define PCIE_ATU_TYPE_IO		0x2
 #define PCIE_ATU_TYPE_CFG0		0x4
 #define PCIE_ATU_TYPE_CFG1		0x5
+#define PCIE_ATU_TYPE_MSG		0x10
 #define PCIE_ATU_TD			BIT(8)
 #define PCIE_ATU_FUNC_NUM(pf)           ((pf) << 20)
 #define PCIE_ATU_REGION_CTRL2		0x004
 #define PCIE_ATU_ENABLE			BIT(31)
 #define PCIE_ATU_BAR_MODE_ENABLE	BIT(30)
+#define PCIE_ATU_INHIBIT_PAYLOAD	BIT(22)
 #define PCIE_ATU_FUNC_NUM_MATCH_EN      BIT(19)
 #define PCIE_ATU_LOWER_BASE		0x008
 #define PCIE_ATU_UPPER_BASE		0x00C
@@ -303,6 +305,8 @@ struct dw_pcie_ob_atu_cfg {
 	int index;
 	int type;
 	u8 func_no;
+	u8 code;
+	u8 routing;
 	u64 cpu_addr;
 	u64 pci_addr;
 	u64 size;

-- 
2.34.1

[PATCH v3 4/6] PCI: Add PCIE_MSG_CODE_PME_TURN_OFF message macro

[Frank Li]

Add PCIE_MSG_CODE_PME_TURN_OFF macros to enable a PCIe host driver to send
PME_Turn_Off messages.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ffd066c15f3bb..989681a0d6057 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -40,6 +40,8 @@
 #define PCIE_MSG_CODE_DEASSERT_INTC	0x26
 #define PCIE_MSG_CODE_DEASSERT_INTD	0x27
 
+#define PCIE_MSG_CODE_PME_TURN_OFF	0x19
+
 extern const unsigned char pcie_link_speed[];
 extern bool pci_early_dump;
 

-- 
2.34.1

[PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

Add an outbound iATU-capable memory-region which will be used to send PCIe
message (such as PME_Turn_Off) to peripheral. So all platforms can use
common method to send out PME_Turn_Off message by using one outbound iATU.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index 022055edbf9e6..25a5420a9ce1e 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -101,6 +101,10 @@ properties:
             Outbound iATU-capable memory-region which will be used to access
             the peripheral PCIe devices configuration space.
           const: config
+        - description:
+            Outbound iATU-capable memory-region which will be used to send
+            PCIe message (such as PME_Turn_Off) to peripheral.
+          const: msg
         - description:
             Vendor-specific CSR names. Consider using the generic names above
             for new bindings.

-- 
2.34.1

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> Add an outbound iATU-capable memory-region which will be used to send PCIe
> message (such as PME_Turn_Off) to peripheral. So all platforms can use
> common method to send out PME_Turn_Off message by using one outbound iATU.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 022055edbf9e6..25a5420a9ce1e 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -101,6 +101,10 @@ properties:

>              Outbound iATU-capable memory-region which will be used to access
>              the peripheral PCIe devices configuration space.
>            const: config
> +        - description:
> +            Outbound iATU-capable memory-region which will be used to send
> +            PCIe message (such as PME_Turn_Off) to peripheral.
> +          const: msg

Note there is a good chance Rob won't like this change. AFAIR he
already expressed a concern regarding having the "config" reg-name
describing a memory space within the outbound iATU memory which is
normally defined by the "ranges" property. Adding a new reg-entry with
similar semantics I guess won't receive warm welcome.

-Serge(y)


>          - description:
>              Vendor-specific CSR names. Consider using the generic names above
>              for new bindings.
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > common method to send out PME_Turn_Off message by using one outbound iATU.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > index 022055edbf9e6..25a5420a9ce1e 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > @@ -101,6 +101,10 @@ properties:
> 
> >              Outbound iATU-capable memory-region which will be used to access
> >              the peripheral PCIe devices configuration space.
> >            const: config
> > +        - description:
> > +            Outbound iATU-capable memory-region which will be used to send
> > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > +          const: msg
> 
> Note there is a good chance Rob won't like this change. AFAIR he
> already expressed a concern regarding having the "config" reg-name
> describing a memory space within the outbound iATU memory which is
> normally defined by the "ranges" property. Adding a new reg-entry with
> similar semantics I guess won't receive warm welcome.
> 
> -Serge(y)

Okay, Anyone from device-tree give comments about this?

@rob, krzysztof kozlowski

Frank
> 
> 
> >          - description:
> >              Vendor-specific CSR names. Consider using the generic names above
> >              for new bindings.
> > 
> > -- 
> > 2.34.1
> > 
> > 

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Rob Herring]

On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > common method to send out PME_Turn_Off message by using one outbound iATU.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > index 022055edbf9e6..25a5420a9ce1e 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > @@ -101,6 +101,10 @@ properties:
> 
> >              Outbound iATU-capable memory-region which will be used to access
> >              the peripheral PCIe devices configuration space.
> >            const: config
> > +        - description:
> > +            Outbound iATU-capable memory-region which will be used to send
> > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > +          const: msg
> 
> Note there is a good chance Rob won't like this change. AFAIR he
> already expressed a concern regarding having the "config" reg-name
> describing a memory space within the outbound iATU memory which is
> normally defined by the "ranges" property. Adding a new reg-entry with
> similar semantics I guess won't receive warm welcome.

I do think it is a bit questionable. Ideally, the driver could 
just configure this on its own. However, since we don't describe all of 
the CPU address space (that's input to the iATU) already, that's not 
going to be possible. I suppose we could fix that, but then config space 
would have to be handled differently too.

Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > @@ -101,6 +101,10 @@ properties:
> > 
> > >              Outbound iATU-capable memory-region which will be used to access
> > >              the peripheral PCIe devices configuration space.
> > >            const: config
> > > +        - description:
> > > +            Outbound iATU-capable memory-region which will be used to send
> > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > +          const: msg
> > 
> > Note there is a good chance Rob won't like this change. AFAIR he
> > already expressed a concern regarding having the "config" reg-name
> > describing a memory space within the outbound iATU memory which is
> > normally defined by the "ranges" property. Adding a new reg-entry with
> > similar semantics I guess won't receive warm welcome.
> 
> I do think it is a bit questionable. Ideally, the driver could 
> just configure this on its own. However, since we don't describe all of 
> the CPU address space (that's input to the iATU) already, that's not 
> going to be possible. I suppose we could fix that, but then config space 
> would have to be handled differently too.

Sorry, I have not understand what your means. Do you means, you want
a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 

Then allocated some space to 'config', 'io', 'memory' and this 'msg'.

Frank

> 
> Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > @@ -101,6 +101,10 @@ properties:
> > > 
> > > >              Outbound iATU-capable memory-region which will be used to access
> > > >              the peripheral PCIe devices configuration space.
> > > >            const: config
> > > > +        - description:
> > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > +          const: msg
> > > 
> > > Note there is a good chance Rob won't like this change. AFAIR he
> > > already expressed a concern regarding having the "config" reg-name
> > > describing a memory space within the outbound iATU memory which is
> > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > similar semantics I guess won't receive warm welcome.
> > 
> > I do think it is a bit questionable. Ideally, the driver could 
> > just configure this on its own. However, since we don't describe all of 
> > the CPU address space (that's input to the iATU) already, that's not 
> > going to be possible. I suppose we could fix that, but then config space 
> > would have to be handled differently too.
> 
> Sorry, I have not understand what your means. Do you means, you want
> a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> 
> Then allocated some space to 'config', 'io', 'memory' and this 'msg'.

@rob:

    So far, I think "msg" is feasilbe solution. Or give me some little
detail direction?

Frank

> 
> Frank
> 
> > 
> > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > 
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > @@ -101,6 +101,10 @@ properties:
> > > > 
> > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > >              the peripheral PCIe devices configuration space.
> > > > >            const: config
> > > > > +        - description:
> > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > +          const: msg
> > > > 
> > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > already expressed a concern regarding having the "config" reg-name
> > > > describing a memory space within the outbound iATU memory which is
> > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > similar semantics I guess won't receive warm welcome.
> > > 
> > > I do think it is a bit questionable. Ideally, the driver could 
> > > just configure this on its own. However, since we don't describe all of 
> > > the CPU address space (that's input to the iATU) already, that's not 
> > > going to be possible. I suppose we could fix that, but then config space 
> > > would have to be handled differently too.
> > 
> > Sorry, I have not understand what your means. Do you means, you want
> > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > 
> > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> 
> @rob:
> 
>     So far, I think "msg" is feasilbe solution. Or give me some little
> detail direction?

Found the Rob' note about the iATU-space chunks utilized in the reg
property:
https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/

So basically Rob meant back then that
either originally we should have defined a new reg-name like "atu-out"
with the entire outbound iATU CPU-space specified and unpin the
regions like "config"/"ecam"/"msg"/etc from there in the driver
or, well, stick to the chunking further. The later path was chosen
after the patch with the "ecam" reg-name was accepted (see the link
above).

Really ECAM/config space access, custom TLP messages, legacy interrupt
TLPs, etc are all application-specific features. Each of them is
implemented based on a bit specific but basically the same outbound
iATU engine setup. Thus from the "DT is a hardware description" point
of view it would have been enough to describe the entire outbound iATU
CPU address space and then let the software do the space
reconfiguration in runtime based on it' application needs.

* Rob, correct me if am wrong.

On the other hand it's possible to have more than one disjoint CPU
address region handled by the outbound iATU (especially if there is no
AXI-bridge enabled, see XALI - application transmit client interfaces
in HW manual). Thus having a single reg-property might get to be
inapplicable in some cases. Thinking about that got me to an idea.
What about just extending the PCIe "ranges" property flags
(IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
use the flag to define a custom memory range for the TLP messages
generation. At some point it can be also utilized for the config-space
mapping. What do you think?

-Serge(y)

> 
> Frank
> 
> > 
> > Frank
> > 
> > > 
> > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > 
> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > 
> > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > >              the peripheral PCIe devices configuration space.
> > > > > >            const: config
> > > > > > +        - description:
> > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > +          const: msg
> > > > > 
> > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > already expressed a concern regarding having the "config" reg-name
> > > > > describing a memory space within the outbound iATU memory which is
> > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > similar semantics I guess won't receive warm welcome.
> > > > 
> > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > just configure this on its own. However, since we don't describe all of 
> > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > going to be possible. I suppose we could fix that, but then config space 
> > > > would have to be handled differently too.
> > > 
> > > Sorry, I have not understand what your means. Do you means, you want
> > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > 
> > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > 
> > @rob:
> > 
> >     So far, I think "msg" is feasilbe solution. Or give me some little
> > detail direction?
> 
> Found the Rob' note about the iATU-space chunks utilized in the reg
> property:
> https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> 
> So basically Rob meant back then that
> either originally we should have defined a new reg-name like "atu-out"
> with the entire outbound iATU CPU-space specified and unpin the
> regions like "config"/"ecam"/"msg"/etc from there in the driver
> or, well, stick to the chunking further. The later path was chosen
> after the patch with the "ecam" reg-name was accepted (see the link
> above).
> 
> Really ECAM/config space access, custom TLP messages, legacy interrupt
> TLPs, etc are all application-specific features. Each of them is
> implemented based on a bit specific but basically the same outbound
> iATU engine setup. Thus from the "DT is a hardware description" point
> of view it would have been enough to describe the entire outbound iATU
> CPU address space and then let the software do the space
> reconfiguration in runtime based on it' application needs.

There are "addr_space" in EP mode, which useful map out outbound iatu
region. We can reuse this name.

To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
(default) for msg, 64K( for IO if not IO region in 'ranges'), left is
mem region. We can config each region size by module parameter or drvdata.

So we can deprecate 'config', even 'ranges'

> 
> * Rob, correct me if am wrong.
> 
> On the other hand it's possible to have more than one disjoint CPU
> address region handled by the outbound iATU (especially if there is no
> AXI-bridge enabled, see XALI - application transmit client interfaces
> in HW manual). Thus having a single reg-property might get to be
> inapplicable in some cases. Thinking about that got me to an idea.
> What about just extending the PCIe "ranges" property flags
> (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> use the flag to define a custom memory range for the TLP messages
> generation. At some point it can be also utilized for the config-space
> mapping. What do you think?

IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
all IORESOURCE_* bit need move. And it is actual MEMORY regain. 

Or we can use IORESOURCE_BITS (0xff)

/* PCI ROM control bits (IORESOURCE_BITS) */
#define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
#define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */

/* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
#define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
#define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */

we can add

IORESOURCE_PRIV_WINDOWS			(1<<6)

I think previous method was more extendable. How do you think?

> 
> -Serge(y)
> 
> > 
> > Frank
> > 
> > > 
> > > Frank
> > > 
> > > > 
> > > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > 
> > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > 
> > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > >            const: config
> > > > > > > +        - description:
> > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > +          const: msg
> > > > > > 
> > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > similar semantics I guess won't receive warm welcome.
> > > > > 
> > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > just configure this on its own. However, since we don't describe all of 
> > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > would have to be handled differently too.
> > > > 
> > > > Sorry, I have not understand what your means. Do you means, you want
> > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > 
> > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > 
> > > @rob:
> > > 
> > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > detail direction?
> > 
> > Found the Rob' note about the iATU-space chunks utilized in the reg
> > property:
> > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > 
> > So basically Rob meant back then that
> > either originally we should have defined a new reg-name like "atu-out"
> > with the entire outbound iATU CPU-space specified and unpin the
> > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > or, well, stick to the chunking further. The later path was chosen
> > after the patch with the "ecam" reg-name was accepted (see the link
> > above).
> > 
> > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > TLPs, etc are all application-specific features. Each of them is
> > implemented based on a bit specific but basically the same outbound
> > iATU engine setup. Thus from the "DT is a hardware description" point
> > of view it would have been enough to describe the entire outbound iATU
> > CPU address space and then let the software do the space
> > reconfiguration in runtime based on it' application needs.
> 
> There are "addr_space" in EP mode, which useful map out outbound iatu
> region. We can reuse this name.
> 
> To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> mem region. We can config each region size by module parameter or drvdata.
> 
> So we can deprecate 'config', even 'ranges'

Not sure I fully understand what you mean. In anyway the "config" reg
name is highly utilized by the DW PCIe IP-core instances. We can't
deprecate it that easily. At least the backwards compatibility must be
preserved. Moreover "addr_space" is also just a single value reg which
won't solve a problem with the disjoint DW PCIe outbound iATU memory
regions.

The "ranges" property is a part of the DT specification.  The
PCI-specific way of the property-based mapping is de-facto a standard
too. So this can't be deprecated.

> 
> > 
> > * Rob, correct me if am wrong.
> > 
> > On the other hand it's possible to have more than one disjoint CPU
> > address region handled by the outbound iATU (especially if there is no
> > AXI-bridge enabled, see XALI - application transmit client interfaces
> > in HW manual). Thus having a single reg-property might get to be
> > inapplicable in some cases. Thinking about that got me to an idea.
> > What about just extending the PCIe "ranges" property flags
> > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > use the flag to define a custom memory range for the TLP messages
> > generation. At some point it can be also utilized for the config-space
> > mapping. What do you think?
> 

> IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> all IORESOURCE_* bit need move. And it is actual MEMORY regain. 

No. The lowest four bits aren't flags but the actual value. They are
retrieved from the PCI-specific memory ranges mapping:
https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
Currently only first four out of _sixteen_ values have been defined so
far. So we can freely use some of the free values for custom TLPs,
etc. Note the config-space is already defined by the ranges property
having the 0x0 space code (see the first link above), but it isn't
currently supported by the PCI subsystem. So at least that option can
be considered as a ready-to-implement replacement for the "config"
reg-name.

> 
> Or we can use IORESOURCE_BITS (0xff)
> 
> /* PCI ROM control bits (IORESOURCE_BITS) */
> #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> 
> /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> 
> we can add
> 
> IORESOURCE_PRIV_WINDOWS			(1<<6)
> 
> I think previous method was more extendable. How do you think?

IMO extending the PCIe "ranges" property semantics looks more
promising, more flexible and more portable across various PCIe
controllers. But the most importantly is what Rob and Bjorn think
about that, not me.

-Serge(y)

> 
> > 
> > -Serge(y)
> > 
> > > 
> > > Frank
> > > 
> > > > 
> > > > Frank
> > > > 
> > > > > 
> > > > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > 
> > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > 
> > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > >            const: config
> > > > > > > > +        - description:
> > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > +          const: msg
> > > > > > > 
> > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > 
> > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > would have to be handled differently too.
> > > > > 
> > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > 
> > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > 
> > > > @rob:
> > > > 
> > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > detail direction?
> > > 
> > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > property:
> > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > 
> > > So basically Rob meant back then that
> > > either originally we should have defined a new reg-name like "atu-out"
> > > with the entire outbound iATU CPU-space specified and unpin the
> > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > or, well, stick to the chunking further. The later path was chosen
> > > after the patch with the "ecam" reg-name was accepted (see the link
> > > above).
> > > 
> > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > TLPs, etc are all application-specific features. Each of them is
> > > implemented based on a bit specific but basically the same outbound
> > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > of view it would have been enough to describe the entire outbound iATU
> > > CPU address space and then let the software do the space
> > > reconfiguration in runtime based on it' application needs.
> > 
> > There are "addr_space" in EP mode, which useful map out outbound iatu
> > region. We can reuse this name.
> > 
> > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > mem region. We can config each region size by module parameter or drvdata.
> > 
> > So we can deprecate 'config', even 'ranges'
> 
> Not sure I fully understand what you mean. In anyway the "config" reg
> name is highly utilized by the DW PCIe IP-core instances. We can't
> deprecate it that easily. At least the backwards compatibility must be
> preserved. Moreover "addr_space" is also just a single value reg which
> won't solve a problem with the disjoint DW PCIe outbound iATU memory
> regions.
> 
> The "ranges" property is a part of the DT specification.  The
> PCI-specific way of the property-based mapping is de-facto a standard
> too. So this can't be deprecated.
> 
> > 
> > > 
> > > * Rob, correct me if am wrong.
> > > 
> > > On the other hand it's possible to have more than one disjoint CPU
> > > address region handled by the outbound iATU (especially if there is no
> > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > in HW manual). Thus having a single reg-property might get to be
> > > inapplicable in some cases. Thinking about that got me to an idea.
> > > What about just extending the PCIe "ranges" property flags
> > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > use the flag to define a custom memory range for the TLP messages
> > > generation. At some point it can be also utilized for the config-space
> > > mapping. What do you think?
> > 
> 
> > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> 
> No. The lowest four bits aren't flags but the actual value. They are
> retrieved from the PCI-specific memory ranges mapping:
> https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78

In dt: phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr

of_bus_pci_get_flags() will parser (phys.hi) to resource flags. Even there
are "000" in dt, we can use, but it need convert IORESOURCE_*, which have
not reserve bit can be used for TLP.

we may call reserve_region_with_split() to split 4k region in mmio windows
in dw_pcie_host_init(). 

So needn't change any dts file. 

Frank

> Currently only first four out of _sixteen_ values have been defined so
> far. So we can freely use some of the free values for custom TLPs,
> etc. Note the config-space is already defined by the ranges property
> having the 0x0 space code (see the first link above), but it isn't
> currently supported by the PCI subsystem. So at least that option can
> be considered as a ready-to-implement replacement for the "config"
> reg-name.
> 
> > 
> > Or we can use IORESOURCE_BITS (0xff)
> > 
> > /* PCI ROM control bits (IORESOURCE_BITS) */
> > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > 
> > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > 
> > we can add
> > 
> > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > 
> > I think previous method was more extendable. How do you think?
> 
> IMO extending the PCIe "ranges" property semantics looks more
> promising, more flexible and more portable across various PCIe
> controllers. But the most importantly is what Rob and Bjorn think
> about that, not me.
> 
> -Serge(y)
> 
> > 
> > > 
> > > -Serge(y)
> > > 
> > > > 
> > > > Frank
> > > > 
> > > > > 
> > > > > Frank
> > > > > 
> > > > > > 
> > > > > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Mon, Feb 12, 2024 at 05:24:01PM -0500, Frank Li wrote:
> On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > 
> > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > >            const: config
> > > > > > > > > +        - description:
> > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > +          const: msg
> > > > > > > > 
> > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > 
> > > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > > would have to be handled differently too.
> > > > > > 
> > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > > 
> > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > 
> > > > > @rob:
> > > > > 
> > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > detail direction?
> > > > 
> > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > property:
> > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > 
> > > > So basically Rob meant back then that
> > > > either originally we should have defined a new reg-name like "atu-out"
> > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > or, well, stick to the chunking further. The later path was chosen
> > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > above).
> > > > 
> > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > TLPs, etc are all application-specific features. Each of them is
> > > > implemented based on a bit specific but basically the same outbound
> > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > of view it would have been enough to describe the entire outbound iATU
> > > > CPU address space and then let the software do the space
> > > > reconfiguration in runtime based on it' application needs.
> > > 
> > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > region. We can reuse this name.
> > > 
> > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > mem region. We can config each region size by module parameter or drvdata.
> > > 
> > > So we can deprecate 'config', even 'ranges'
> > 
> > Not sure I fully understand what you mean. In anyway the "config" reg
> > name is highly utilized by the DW PCIe IP-core instances. We can't
> > deprecate it that easily. At least the backwards compatibility must be
> > preserved. Moreover "addr_space" is also just a single value reg which
> > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > regions.
> > 
> > The "ranges" property is a part of the DT specification.  The
> > PCI-specific way of the property-based mapping is de-facto a standard
> > too. So this can't be deprecated.
> > 
> > > 
> > > > 
> > > > * Rob, correct me if am wrong.
> > > > 
> > > > On the other hand it's possible to have more than one disjoint CPU
> > > > address region handled by the outbound iATU (especially if there is no
> > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > in HW manual). Thus having a single reg-property might get to be
> > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > What about just extending the PCIe "ranges" property flags
> > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > use the flag to define a custom memory range for the TLP messages
> > > > generation. At some point it can be also utilized for the config-space
> > > > mapping. What do you think?
> > > 
> > 
> > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> > 
> > No. The lowest four bits aren't flags but the actual value. They are
> > retrieved from the PCI-specific memory ranges mapping:
> > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> 
> In dt: phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> 
> of_bus_pci_get_flags() will parser (phys.hi) to resource flags. Even there
> are "000" in dt, we can use, but it need convert IORESOURCE_*, which have
> not reserve bit can be used for TLP.
> 
> we may call reserve_region_with_split() to split 4k region in mmio windows
> in dw_pcie_host_init(). 

By using resource_request() to reserve a region from IOMEMORY space. So
Needn't change dt binding. All changes are in dwc drivers.

If you have time, please check
https://lore.kernel.org/imx/20240213-pme_msg-v4-0-e2acd4d7a292@nxp.com/T/#t

Frank

> 
> So needn't change any dts file. 
> 
> Frank
> 
> > Currently only first four out of _sixteen_ values have been defined so
> > far. So we can freely use some of the free values for custom TLPs,
> > etc. Note the config-space is already defined by the ranges property
> > having the 0x0 space code (see the first link above), but it isn't
> > currently supported by the PCI subsystem. So at least that option can
> > be considered as a ready-to-implement replacement for the "config"
> > reg-name.
> > 
> > > 
> > > Or we can use IORESOURCE_BITS (0xff)
> > > 
> > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > > 
> > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > > 
> > > we can add
> > > 
> > > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > > 
> > > I think previous method was more extendable. How do you think?
> > 
> > IMO extending the PCIe "ranges" property semantics looks more
> > promising, more flexible and more portable across various PCIe
> > controllers. But the most importantly is what Rob and Bjorn think
> > about that, not me.
> > 
> > -Serge(y)
> > 
> > > 
> > > > 
> > > > -Serge(y)
> > > > 
> > > > > 
> > > > > Frank
> > > > > 
> > > > > > 
> > > > > > Frank
> > > > > > 
> > > > > > > 
> > > > > > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Manivannan Sadhasivam]

On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > 
> > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > 
> > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > >            const: config
> > > > > > > > +        - description:
> > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > +          const: msg
> > > > > > > 
> > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > 
> > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > would have to be handled differently too.
> > > > > 
> > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > 
> > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > 
> > > > @rob:
> > > > 
> > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > detail direction?
> > > 
> > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > property:
> > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > 
> > > So basically Rob meant back then that
> > > either originally we should have defined a new reg-name like "atu-out"
> > > with the entire outbound iATU CPU-space specified and unpin the
> > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > or, well, stick to the chunking further. The later path was chosen
> > > after the patch with the "ecam" reg-name was accepted (see the link
> > > above).
> > > 
> > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > TLPs, etc are all application-specific features. Each of them is
> > > implemented based on a bit specific but basically the same outbound
> > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > of view it would have been enough to describe the entire outbound iATU
> > > CPU address space and then let the software do the space
> > > reconfiguration in runtime based on it' application needs.
> > 
> > There are "addr_space" in EP mode, which useful map out outbound iatu
> > region. We can reuse this name.
> > 
> > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > mem region. We can config each region size by module parameter or drvdata.
> > 
> > So we can deprecate 'config', even 'ranges'
> 
> Not sure I fully understand what you mean. In anyway the "config" reg
> name is highly utilized by the DW PCIe IP-core instances. We can't
> deprecate it that easily. At least the backwards compatibility must be
> preserved. Moreover "addr_space" is also just a single value reg which
> won't solve a problem with the disjoint DW PCIe outbound iATU memory
> regions.
> 
> The "ranges" property is a part of the DT specification.  The
> PCI-specific way of the property-based mapping is de-facto a standard
> too. So this can't be deprecated.
> 
> > 
> > > 
> > > * Rob, correct me if am wrong.
> > > 
> > > On the other hand it's possible to have more than one disjoint CPU
> > > address region handled by the outbound iATU (especially if there is no
> > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > in HW manual). Thus having a single reg-property might get to be
> > > inapplicable in some cases. Thinking about that got me to an idea.
> > > What about just extending the PCIe "ranges" property flags
> > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > use the flag to define a custom memory range for the TLP messages
> > > generation. At some point it can be also utilized for the config-space
> > > mapping. What do you think?
> > 
> 
> > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> 
> No. The lowest four bits aren't flags but the actual value. They are
> retrieved from the PCI-specific memory ranges mapping:
> https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> Currently only first four out of _sixteen_ values have been defined so
> far. So we can freely use some of the free values for custom TLPs,
> etc. Note the config-space is already defined by the ranges property
> having the 0x0 space code (see the first link above), but it isn't
> currently supported by the PCI subsystem. So at least that option can
> be considered as a ready-to-implement replacement for the "config"
> reg-name.
> 

Agree. But still, the driver has to support both options: "config" reg name and
"ranges", since ammending the binding would be an ABI break.

> > 
> > Or we can use IORESOURCE_BITS (0xff)
> > 
> > /* PCI ROM control bits (IORESOURCE_BITS) */
> > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > 
> > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > 
> > we can add
> > 
> > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > 
> > I think previous method was more extendable. How do you think?
> 
> IMO extending the PCIe "ranges" property semantics looks more
> promising, more flexible and more portable across various PCIe
> controllers. But the most importantly is what Rob and Bjorn think
> about that, not me.
> 

IMO, using the "ranges" property to allocate arbitrary memory region should be
the way forward, since it has almost all the info needed by the drivers to
allocate the memory regions.

But for the sake of DT backwards compatiblity, we have to keep supporting the
existing reg entries (addr_space, et al.), because "ranges" is not a required
property for EP controllers.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > 
> > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > >            const: config
> > > > > > > > > +        - description:
> > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > +          const: msg
> > > > > > > > 
> > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > 
> > > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > > would have to be handled differently too.
> > > > > > 
> > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > > 
> > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > 
> > > > > @rob:
> > > > > 
> > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > detail direction?
> > > > 
> > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > property:
> > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > 
> > > > So basically Rob meant back then that
> > > > either originally we should have defined a new reg-name like "atu-out"
> > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > or, well, stick to the chunking further. The later path was chosen
> > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > above).
> > > > 
> > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > TLPs, etc are all application-specific features. Each of them is
> > > > implemented based on a bit specific but basically the same outbound
> > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > of view it would have been enough to describe the entire outbound iATU
> > > > CPU address space and then let the software do the space
> > > > reconfiguration in runtime based on it' application needs.
> > > 
> > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > region. We can reuse this name.
> > > 
> > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > mem region. We can config each region size by module parameter or drvdata.
> > > 
> > > So we can deprecate 'config', even 'ranges'
> > 
> > Not sure I fully understand what you mean. In anyway the "config" reg
> > name is highly utilized by the DW PCIe IP-core instances. We can't
> > deprecate it that easily. At least the backwards compatibility must be
> > preserved. Moreover "addr_space" is also just a single value reg which
> > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > regions.
> > 
> > The "ranges" property is a part of the DT specification.  The
> > PCI-specific way of the property-based mapping is de-facto a standard
> > too. So this can't be deprecated.
> > 
> > > 
> > > > 
> > > > * Rob, correct me if am wrong.
> > > > 
> > > > On the other hand it's possible to have more than one disjoint CPU
> > > > address region handled by the outbound iATU (especially if there is no
> > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > in HW manual). Thus having a single reg-property might get to be
> > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > What about just extending the PCIe "ranges" property flags
> > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > use the flag to define a custom memory range for the TLP messages
> > > > generation. At some point it can be also utilized for the config-space
> > > > mapping. What do you think?
> > > 
> > 
> > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> > 
> > No. The lowest four bits aren't flags but the actual value. They are
> > retrieved from the PCI-specific memory ranges mapping:
> > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > Currently only first four out of _sixteen_ values have been defined so
> > far. So we can freely use some of the free values for custom TLPs,
> > etc. Note the config-space is already defined by the ranges property
> > having the 0x0 space code (see the first link above), but it isn't
> > currently supported by the PCI subsystem. So at least that option can
> > be considered as a ready-to-implement replacement for the "config"
> > reg-name.
> > 
> 
> Agree. But still, the driver has to support both options: "config" reg name and
> "ranges", since ammending the binding would be an ABI break.

of_bus_pci_get_flags()
{
	u32 w = addr[0];

	/* For PCI, we override whatever child busses may have used.  */
	flags = 0;
	switch((w >> 24) & 0x03) {
	case 0x01:
		flags |= IORESOURCE_IO;
		break;

	case 0x02: /* 32 bits */
	case 0x03: /* 64 bits */
		flags |= IORESOURCE_MEM;
		break;
	}
	if (w & 0x40000000)
		flags |= IORESOURCE_PREFETCH;
	return flags;
}

flags will be 0 for config space. It should be okay for flag: 0 as config
ranges.

but it can't resolve 'msg' space problem. Even there are more bit at
addr[0]. but there are not enough bits for flags yet.

Anyway, could you please check v4 version:
https://lore.kernel.org/imx/20240213-pme_msg-v4-0-e2acd4d7a292@nxp.com/T/#t

'msg' will reserve from IORESOURCE_MEM without change dt-bing.

Frank

> 
> > > 
> > > Or we can use IORESOURCE_BITS (0xff)
> > > 
> > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > > 
> > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > > 
> > > we can add
> > > 
> > > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > > 
> > > I think previous method was more extendable. How do you think?
> > 
> > IMO extending the PCIe "ranges" property semantics looks more
> > promising, more flexible and more portable across various PCIe
> > controllers. But the most importantly is what Rob and Bjorn think
> > about that, not me.
> > 
> 
> IMO, using the "ranges" property to allocate arbitrary memory region should be
> the way forward, since it has almost all the info needed by the drivers to
> allocate the memory regions.
> 
> But for the sake of DT backwards compatiblity, we have to keep supporting the
> existing reg entries (addr_space, et al.), because "ranges" is not a required
> property for EP controllers.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Rob Herring]

On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > 
> > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > >            const: config
> > > > > > > > > +        - description:
> > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > +          const: msg
> > > > > > > > 
> > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > 
> > > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > > would have to be handled differently too.
> > > > > > 
> > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > > 
> > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > 
> > > > > @rob:
> > > > > 
> > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > detail direction?
> > > > 
> > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > property:
> > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > 
> > > > So basically Rob meant back then that
> > > > either originally we should have defined a new reg-name like "atu-out"
> > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > or, well, stick to the chunking further. The later path was chosen
> > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > above).
> > > > 
> > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > TLPs, etc are all application-specific features. Each of them is
> > > > implemented based on a bit specific but basically the same outbound
> > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > of view it would have been enough to describe the entire outbound iATU
> > > > CPU address space and then let the software do the space
> > > > reconfiguration in runtime based on it' application needs.
> > > 
> > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > region. We can reuse this name.
> > > 
> > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > mem region. We can config each region size by module parameter or drvdata.
> > > 
> > > So we can deprecate 'config', even 'ranges'
> > 
> > Not sure I fully understand what you mean. In anyway the "config" reg
> > name is highly utilized by the DW PCIe IP-core instances. We can't
> > deprecate it that easily. At least the backwards compatibility must be
> > preserved. Moreover "addr_space" is also just a single value reg which
> > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > regions.
> > 
> > The "ranges" property is a part of the DT specification.  The
> > PCI-specific way of the property-based mapping is de-facto a standard
> > too. So this can't be deprecated.
> > 
> > > 
> > > > 
> > > > * Rob, correct me if am wrong.
> > > > 
> > > > On the other hand it's possible to have more than one disjoint CPU
> > > > address region handled by the outbound iATU (especially if there is no
> > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > in HW manual). Thus having a single reg-property might get to be
> > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > What about just extending the PCIe "ranges" property flags
> > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > use the flag to define a custom memory range for the TLP messages
> > > > generation. At some point it can be also utilized for the config-space
> > > > mapping. What do you think?
> > > 
> > 
> > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> > 
> > No. The lowest four bits aren't flags but the actual value. They are
> > retrieved from the PCI-specific memory ranges mapping:
> > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > Currently only first four out of _sixteen_ values have been defined so
> > far. So we can freely use some of the free values for custom TLPs,
> > etc. Note the config-space is already defined by the ranges property
> > having the 0x0 space code (see the first link above), but it isn't
> > currently supported by the PCI subsystem. So at least that option can
> > be considered as a ready-to-implement replacement for the "config"
> > reg-name.
> > 
> 
> Agree. But still, the driver has to support both options: "config" reg name and
> "ranges", since ammending the binding would be an ABI break.
> 
> > > 
> > > Or we can use IORESOURCE_BITS (0xff)
> > > 
> > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > > 
> > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > > 
> > > we can add
> > > 
> > > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > > 
> > > I think previous method was more extendable. How do you think?
> > 
> > IMO extending the PCIe "ranges" property semantics looks more
> > promising, more flexible and more portable across various PCIe
> > controllers. But the most importantly is what Rob and Bjorn think
> > about that, not me.
> > 
> 
> IMO, using the "ranges" property to allocate arbitrary memory region should be
> the way forward, since it has almost all the info needed by the drivers to
> allocate the memory regions.
> 
> But for the sake of DT backwards compatiblity, we have to keep supporting the
> existing reg entries (addr_space, et al.), because "ranges" is not a required
> property for EP controllers.

I don't know that its worth the effort to carry both. Maybe if it is 
useful on more than just DW host.

I believe we had config space in ranges at some point on some 
binding and moved away from that. I forget the reasoning.

Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > ---
> > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > 
> > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > >            const: config
> > > > > > > > > > +        - description:
> > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > +          const: msg
> > > > > > > > > 
> > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > 
> > > > > > > > I do think it is a bit questionable. Ideally, the driver could 
> > > > > > > > just configure this on its own. However, since we don't describe all of 
> > > > > > > > the CPU address space (that's input to the iATU) already, that's not 
> > > > > > > > going to be possible. I suppose we could fix that, but then config space 
> > > > > > > > would have to be handled differently too.
> > > > > > > 
> > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > > > > > > 
> > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > 
> > > > > > @rob:
> > > > > > 
> > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > detail direction?
> > > > > 
> > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > property:
> > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > 
> > > > > So basically Rob meant back then that
> > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > above).
> > > > > 
> > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > implemented based on a bit specific but basically the same outbound
> > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > CPU address space and then let the software do the space
> > > > > reconfiguration in runtime based on it' application needs.
> > > > 
> > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > region. We can reuse this name.
> > > > 
> > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > mem region. We can config each region size by module parameter or drvdata.
> > > > 
> > > > So we can deprecate 'config', even 'ranges'
> > > 
> > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > deprecate it that easily. At least the backwards compatibility must be
> > > preserved. Moreover "addr_space" is also just a single value reg which
> > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > regions.
> > > 
> > > The "ranges" property is a part of the DT specification.  The
> > > PCI-specific way of the property-based mapping is de-facto a standard
> > > too. So this can't be deprecated.
> > > 
> > > > 
> > > > > 
> > > > > * Rob, correct me if am wrong.
> > > > > 
> > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > address region handled by the outbound iATU (especially if there is no
> > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > What about just extending the PCIe "ranges" property flags
> > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > use the flag to define a custom memory range for the TLP messages
> > > > > generation. At some point it can be also utilized for the config-space
> > > > > mapping. What do you think?
> > > > 
> > > 
> > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS, 
> > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain. 
> > > 
> > > No. The lowest four bits aren't flags but the actual value. They are
> > > retrieved from the PCI-specific memory ranges mapping:
> > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > Currently only first four out of _sixteen_ values have been defined so
> > > far. So we can freely use some of the free values for custom TLPs,
> > > etc. Note the config-space is already defined by the ranges property
> > > having the 0x0 space code (see the first link above), but it isn't
> > > currently supported by the PCI subsystem. So at least that option can
> > > be considered as a ready-to-implement replacement for the "config"
> > > reg-name.
> > > 
> > 
> > Agree. But still, the driver has to support both options: "config" reg name and
> > "ranges", since ammending the binding would be an ABI break.
> > 
> > > > 
> > > > Or we can use IORESOURCE_BITS (0xff)
> > > > 
> > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
> > > > 
> > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
> > > > #define IORESOURCE_PCI_EA_BEI		(1<<5)	/* BAR Equivalent Indicator */
> > > > 
> > > > we can add
> > > > 
> > > > IORESOURCE_PRIV_WINDOWS			(1<<6)
> > > > 
> > > > I think previous method was more extendable. How do you think?
> > > 
> > > IMO extending the PCIe "ranges" property semantics looks more
> > > promising, more flexible and more portable across various PCIe
> > > controllers. But the most importantly is what Rob and Bjorn think
> > > about that, not me.
> > > 
> > 
> > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > the way forward, since it has almost all the info needed by the drivers to
> > allocate the memory regions.
> > 
> > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > property for EP controllers.
> 
> I don't know that its worth the effort to carry both. Maybe if it is 
> useful on more than just DW host.
> 
> I believe we had config space in ranges at some point on some 
> binding and moved away from that. I forget the reasoning.

I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
in dwc host driver in v4.

But I think it is wonthful to discuss if we can extend of_map bits, add
more type beside CONFIG/IO/MEM/MEM64.

https://elinux.org/Device_Tree_Usage#PCI_Address_Translation

phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr

There are '000' before 'ss'.  If we use it as dwc private resource.

Frank

> 
> Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Rob Herring]

On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
>
> On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > >
> > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > >            const: config
> > > > > > > > > > > +        - description:
> > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > +          const: msg
> > > > > > > > > >
> > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > >
> > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > would have to be handled differently too.
> > > > > > > >
> > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > >
> > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > >
> > > > > > > @rob:
> > > > > > >
> > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > detail direction?
> > > > > >
> > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > property:
> > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > >
> > > > > > So basically Rob meant back then that
> > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > above).
> > > > > >
> > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > CPU address space and then let the software do the space
> > > > > > reconfiguration in runtime based on it' application needs.
> > > > >
> > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > region. We can reuse this name.
> > > > >
> > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > >
> > > > > So we can deprecate 'config', even 'ranges'
> > > >
> > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > deprecate it that easily. At least the backwards compatibility must be
> > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > regions.
> > > >
> > > > The "ranges" property is a part of the DT specification.  The
> > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > too. So this can't be deprecated.
> > > >
> > > > >
> > > > > >
> > > > > > * Rob, correct me if am wrong.
> > > > > >
> > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > mapping. What do you think?
> > > > >
> > > >
> > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > >
> > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > retrieved from the PCI-specific memory ranges mapping:
> > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > Currently only first four out of _sixteen_ values have been defined so
> > > > far. So we can freely use some of the free values for custom TLPs,
> > > > etc. Note the config-space is already defined by the ranges property
> > > > having the 0x0 space code (see the first link above), but it isn't
> > > > currently supported by the PCI subsystem. So at least that option can
> > > > be considered as a ready-to-implement replacement for the "config"
> > > > reg-name.
> > > >
> > >
> > > Agree. But still, the driver has to support both options: "config" reg name and
> > > "ranges", since ammending the binding would be an ABI break.
> > >
> > > > >
> > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > >
> > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > >
> > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > >
> > > > > we can add
> > > > >
> > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > >
> > > > > I think previous method was more extendable. How do you think?
> > > >
> > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > promising, more flexible and more portable across various PCIe
> > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > about that, not me.
> > > >
> > >
> > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > the way forward, since it has almost all the info needed by the drivers to
> > > allocate the memory regions.
> > >
> > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > property for EP controllers.
> >
> > I don't know that its worth the effort to carry both. Maybe if it is
> > useful on more than just DW host.
> >
> > I believe we had config space in ranges at some point on some
> > binding and moved away from that. I forget the reasoning.
>
> I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> in dwc host driver in v4.
>
> But I think it is wonthful to discuss if we can extend of_map bits, add
> more type beside CONFIG/IO/MEM/MEM64.
>
> https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
>
> phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
>
> There are '000' before 'ss'.  If we use it as dwc private resource.

DWC (or any host controller) specific things? No!

Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > >
> > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > >            const: config
> > > > > > > > > > > > +        - description:
> > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > +          const: msg
> > > > > > > > > > >
> > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > >
> > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > would have to be handled differently too.
> > > > > > > > >
> > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > >
> > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > >
> > > > > > > > @rob:
> > > > > > > >
> > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > detail direction?
> > > > > > >
> > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > property:
> > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > >
> > > > > > > So basically Rob meant back then that
> > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > above).
> > > > > > >
> > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > CPU address space and then let the software do the space
> > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > >
> > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > region. We can reuse this name.
> > > > > >
> > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > >
> > > > > > So we can deprecate 'config', even 'ranges'
> > > > >
> > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > regions.
> > > > >
> > > > > The "ranges" property is a part of the DT specification.  The
> > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > too. So this can't be deprecated.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > * Rob, correct me if am wrong.
> > > > > > >
> > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > mapping. What do you think?
> > > > > >
> > > > >
> > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > >
> > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > etc. Note the config-space is already defined by the ranges property
> > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > reg-name.
> > > > >
> > > >
> > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > "ranges", since ammending the binding would be an ABI break.
> > > >
> > > > > >
> > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > >
> > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > >
> > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > >
> > > > > > we can add
> > > > > >
> > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > >
> > > > > > I think previous method was more extendable. How do you think?
> > > > >
> > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > promising, more flexible and more portable across various PCIe
> > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > about that, not me.
> > > > >
> > > >
> > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > the way forward, since it has almost all the info needed by the drivers to
> > > > allocate the memory regions.
> > > >
> > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > property for EP controllers.
> > >
> > > I don't know that its worth the effort to carry both. Maybe if it is
> > > useful on more than just DW host.
> > >
> > > I believe we had config space in ranges at some point on some
> > > binding and moved away from that. I forget the reasoning.
> >
> > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > in dwc host driver in v4.
> >
> > But I think it is wonthful to discuss if we can extend of_map bits, add
> > more type beside CONFIG/IO/MEM/MEM64.
> >
> > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> >
> > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> >

> > There are '000' before 'ss'.  If we use it as dwc private resource.

Frank, why do you mis-inform about the idea? The point was to use the
ranges property for:
1. PCIe Config-space mapping.
2. PCIe TLP messages region.
There is _nothing_ DWC-specific in the original suggestion. Case 1 has
already implicitly defined by the DT standard, see the link above (but
for some reason hasn't been implemented in the PCIe subsystem). Case 2
hasn't been determined, but could be seeing there are three unused
bits in the ss-code of the phys.hi cell. All of that can be used by
_any_ PCIe RC/EP device.

> 
> DWC (or any host controller) specific things? No!

Rob, could you please dive deeper in this thread? The idea is to use
the "ranges" property for the "config" (PCIe config space) and the
custom PCIe TLP messages regions.

-Serge(y)

> 
> Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Thu, Feb 29, 2024 at 02:26:34PM +0300, Serge Semin wrote:
> On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> > On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> > >
> > > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > > >
> > > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > > >            const: config
> > > > > > > > > > > > > +        - description:
> > > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > > +          const: msg
> > > > > > > > > > > >
> > > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > > >
> > > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > > would have to be handled differently too.
> > > > > > > > > >
> > > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > > >
> > > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > > >
> > > > > > > > > @rob:
> > > > > > > > >
> > > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > > detail direction?
> > > > > > > >
> > > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > > property:
> > > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > > >
> > > > > > > > So basically Rob meant back then that
> > > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > > above).
> > > > > > > >
> > > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > > CPU address space and then let the software do the space
> > > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > > >
> > > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > > region. We can reuse this name.
> > > > > > >
> > > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > > >
> > > > > > > So we can deprecate 'config', even 'ranges'
> > > > > >
> > > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > > regions.
> > > > > >
> > > > > > The "ranges" property is a part of the DT specification.  The
> > > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > > too. So this can't be deprecated.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > * Rob, correct me if am wrong.
> > > > > > > >
> > > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > > mapping. What do you think?
> > > > > > >
> > > > > >
> > > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > > >
> > > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > > etc. Note the config-space is already defined by the ranges property
> > > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > > reg-name.
> > > > > >
> > > > >
> > > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > > "ranges", since ammending the binding would be an ABI break.
> > > > >
> > > > > > >
> > > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > > >
> > > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > > >
> > > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > > >
> > > > > > > we can add
> > > > > > >
> > > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > > >
> > > > > > > I think previous method was more extendable. How do you think?
> > > > > >
> > > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > > promising, more flexible and more portable across various PCIe
> > > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > > about that, not me.
> > > > > >
> > > > >
> > > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > > the way forward, since it has almost all the info needed by the drivers to
> > > > > allocate the memory regions.
> > > > >
> > > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > > property for EP controllers.
> > > >
> > > > I don't know that its worth the effort to carry both. Maybe if it is
> > > > useful on more than just DW host.
> > > >
> > > > I believe we had config space in ranges at some point on some
> > > > binding and moved away from that. I forget the reasoning.
> > >
> > > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > > in dwc host driver in v4.
> > >
> > > But I think it is wonthful to discuss if we can extend of_map bits, add
> > > more type beside CONFIG/IO/MEM/MEM64.
> > >
> > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > >
> > > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > >
> 
> > > There are '000' before 'ss'.  If we use it as dwc private resource.
> 
> Frank, why do you mis-inform about the idea? The point was to use the

I am not sure where I mis-inform.

> ranges property for:
> 1. PCIe Config-space mapping.

This one already exist. Just dwc driver have not use it for unknown reason.
We can change driver to support it if want. That will not related
dt-binding.
 
> 2. PCIe TLP messages region.

So far, as I known, PCIe spec only define TLP message format, not define
'region'. DWC choose use mmio region to send TLP message by ATU. May other
hardware can define a register, fill TLP related information, then issue
TLP message. 

> There is _nothing_ DWC-specific in the original suggestion. Case 1 has
> already implicitly defined by the DT standard, see the link above (but
> for some reason hasn't been implemented in the PCIe subsystem). Case 2
> hasn't been determined, but could be seeing there are three unused
> bits in the ss-code of the phys.hi cell. All of that can be used by
> _any_ PCIe RC/EP device.

Please give some exmaple how you plan use these bits. 'msg' region may be
an example, although rob doesn't like.

Frank

> 
> > 
> > DWC (or any host controller) specific things? No!
> 
> Rob, could you please dive deeper in this thread? The idea is to use
> the "ranges" property for the "config" (PCIe config space) and the
> custom PCIe TLP messages regions.
> 
> -Serge(y)
> 
> > 
> > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Thu, Feb 29, 2024 at 10:44:13AM -0500, Frank Li wrote:
> On Thu, Feb 29, 2024 at 02:26:34PM +0300, Serge Semin wrote:
> > On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> > > On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> > > >
> > > > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > > > >
> > > > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > > > >            const: config
> > > > > > > > > > > > > > +        - description:
> > > > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > > > +          const: msg
> > > > > > > > > > > > >
> > > > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > > > >
> > > > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > > > would have to be handled differently too.
> > > > > > > > > > >
> > > > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > > > >
> > > > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > > > >
> > > > > > > > > > @rob:
> > > > > > > > > >
> > > > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > > > detail direction?
> > > > > > > > >
> > > > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > > > property:
> > > > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > > > >
> > > > > > > > > So basically Rob meant back then that
> > > > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > > > above).
> > > > > > > > >
> > > > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > > > CPU address space and then let the software do the space
> > > > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > > > >
> > > > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > > > region. We can reuse this name.
> > > > > > > >
> > > > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > > > >
> > > > > > > > So we can deprecate 'config', even 'ranges'
> > > > > > >
> > > > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > > > regions.
> > > > > > >
> > > > > > > The "ranges" property is a part of the DT specification.  The
> > > > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > > > too. So this can't be deprecated.
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > * Rob, correct me if am wrong.
> > > > > > > > >
> > > > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > > > mapping. What do you think?
> > > > > > > >
> > > > > > >
> > > > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > > > >
> > > > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > > > etc. Note the config-space is already defined by the ranges property
> > > > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > > > reg-name.
> > > > > > >
> > > > > >
> > > > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > > > "ranges", since ammending the binding would be an ABI break.
> > > > > >
> > > > > > > >
> > > > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > > > >
> > > > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > > > >
> > > > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > > > >
> > > > > > > > we can add
> > > > > > > >
> > > > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > > > >
> > > > > > > > I think previous method was more extendable. How do you think?
> > > > > > >
> > > > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > > > promising, more flexible and more portable across various PCIe
> > > > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > > > about that, not me.
> > > > > > >
> > > > > >
> > > > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > > > the way forward, since it has almost all the info needed by the drivers to
> > > > > > allocate the memory regions.
> > > > > >
> > > > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > > > property for EP controllers.
> > > > >
> > > > > I don't know that its worth the effort to carry both. Maybe if it is
> > > > > useful on more than just DW host.
> > > > >
> > > > > I believe we had config space in ranges at some point on some
> > > > > binding and moved away from that. I forget the reasoning.
> > > >
> > > > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > > > in dwc host driver in v4.
> > > >
> > > > But I think it is wonthful to discuss if we can extend of_map bits, add
> > > > more type beside CONFIG/IO/MEM/MEM64.
> > > >
> > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > >
> > > > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > > >
> > 
> > > > There are '000' before 'ss'.  If we use it as dwc private resource.
> > 
> > Frank, why do you mis-inform about the idea? The point was to use the
> 

> I am not sure where I mis-inform.

Right in these words: "If we use it as dwc private resource." I never
said it should have been DWC-only feature. I explicitly stated that
the solution would be portable across various devices:

On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> IMO extending the PCIe "ranges" property semantics looks more
> promising, more flexible and more portable across various PCIe
> controllers.

> 
> > ranges property for:
> > 1. PCIe Config-space mapping.
> 
> This one already exist. Just dwc driver have not use it for unknown reason.
> We can change driver to support it if want. That will not related
> dt-binding.

I bet this won't be that easy. PCIe subsystem code will need to be fixed
too, at least the devm_of_pci_get_host_bridge_resources() method and
subordinates.

>  
> > 2. PCIe TLP messages region.
> 

> So far, as I known, PCIe spec only define TLP message format, not define
> 'region'.

PCIe-spec != DT-spec.

> DWC choose use mmio region to send TLP message by ATU.

So is a lot of other controllers:
Rockchip PCIe controller (see pcie-rockchip.h AXI_WRAPPER_* macros)
Cadence PCIe controller (see pcie-cadence.h CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_* macros)
Mediatek PCIe Gen3 controller (see pcie-mediatek-gen3.c PCIE_ATR_TLP_TYPE() macro)
...

Moreover as you said yourself PCIe-spec doesn't define "regions". It's
always about TLPs. IO, MEM, CFG, INTx, PME, Vendor-defined, etc all is
kind of TLPs. Yet at least the first three are described by the
"ranges" DT-property.

Anyway let's look further in the PCIe-spec. Please note paragraph
2.1.1. of for instance the PCIe-4.0 spec:

2.1.1  Address Spaces, Transaction Types, and Usage
"Transactions form the basis for information transfer between a
Requester and Completer. Four _address spaces_ are defined, and
different Transaction types are defined, each with its own unique
intended usage, as shown in Table 2-12."

Address Space     | Transaction Types    |  Basic Usage
Memory            | Read/Write           | Transfer data to/from a
                  |                      | memory-mapped location
I/O               | Read/Write           | Transfer data to/from an
                  |                      | I/O-mapped location
Configuration     | Read/Write           | Device Function config/setup
_Message_         | Baseline (including  | From event signaling mechanism
                  | Vendor–Defined)      | to general purpose messaging

So basically the PCIe-spec defines four _address spaces_. The
_message_ space is one of them. Seeing the "ranges" DT-property is
about the space-to-space mapping there is nothing wrong with using it
for the _message_ space mapping. 

> May other
> hardware can define a register, fill TLP related information, then issue
> TLP message.

So can be, and originally was, done for the PCI config-space access.
See Mediatek PCIe controller (see pcie-mediatek.c
mtk_pcie_hw_wr_cfg()/mtk_pcie_hw_rd_cfg()) and more than half of the
drivers/pci/controllers/pci-*.c drivers. Yet we have the DT-spec
defining the PCIe-"ranges" property with the config-space mapping.

Moreover DW PCIe controllers perform the mem-to-tlp translation in the
AHB/AXI-bridge module. If not for that the address space transactions
are generated by means of the XALI/TRGT interfaces, which is a set of
the wires/signals. They can be used by the device engineers to
implement _any_ interface they wish for _any address space_. So
basically the memory, I/O and config-space transfers can be
theoretically implemented by the indirect CSRs IO in hardware. Another
story is who would wish to do that if there is a ready-to-use and
handy AXI/AHB bridges with simple memory-based interface...

> 
> > There is _nothing_ DWC-specific in the original suggestion. Case 1 has
> > already implicitly defined by the DT standard, see the link above (but
> > for some reason hasn't been implemented in the PCIe subsystem). Case 2
> > hasn't been determined, but could be seeing there are three unused
> > bits in the ss-code of the phys.hi cell. All of that can be used by
> > _any_ PCIe RC/EP device.
> 
> Please give some exmaple how you plan use these bits. 'msg' region may be
> an example, although rob doesn't like.

Rob didn't like _your_ interpretation of the suggested approach.

Anyway the implementation may look as follows:

ranges = <0x82000000 0 0x00000000 0 0xe0000000 0 0x10000000>, /* mem */
         <0x81000000 0 0x10000000 0 0xf0000000 0 0x00010000>, /* io */
         <0x80000000 0 0x10010000 0 0xf0010000 0 0x00010000>, /* cfg */
         <0x84000000 0 0x10020000 0 0xf0020000 0 0x00010000>; /* mem - note new space code 0x4 */

So the "cfg" region can be used instead of the "config" reg-name (a
more correct replacement for that), and the "mem" region with not-yet
defined space code could be utilized for various message TLPs
described in the "2.2.8 Message Request Rules" paragraph (PCIe 4.0
spec). We can go further and define various message-mapping like INTx,
PM, Vendor-specific, Locked transaction, etc, which IMO would be
better than using a single type for all kinds of the messages seeing
PCIe-spec have them described.. But it's better to get the Rob and
Bjorn opinion about the _actual_ suggestion first.

-Serge(y)

> 
> Frank
> 
> > 
> > > 
> > > DWC (or any host controller) specific things? No!
> > 
> > Rob, could you please dive deeper in this thread? The idea is to use
> > the "ranges" property for the "config" (PCIe config space) and the
> > custom PCIe TLP messages regions.

> > 
> > -Serge(y)
> > 
> > > 
> > > Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Rob Herring]

On Thu, Feb 29, 2024 at 02:26:34PM +0300, Serge Semin wrote:
> On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> > On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> > >
> > > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > > >
> > > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > > >            const: config
> > > > > > > > > > > > > +        - description:
> > > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > > +          const: msg
> > > > > > > > > > > >
> > > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > > >
> > > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > > would have to be handled differently too.
> > > > > > > > > >
> > > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > > >
> > > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > > >
> > > > > > > > > @rob:
> > > > > > > > >
> > > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > > detail direction?
> > > > > > > >
> > > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > > property:
> > > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > > >
> > > > > > > > So basically Rob meant back then that
> > > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > > above).
> > > > > > > >
> > > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > > CPU address space and then let the software do the space
> > > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > > >
> > > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > > region. We can reuse this name.
> > > > > > >
> > > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > > >
> > > > > > > So we can deprecate 'config', even 'ranges'
> > > > > >
> > > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > > regions.
> > > > > >
> > > > > > The "ranges" property is a part of the DT specification.  The
> > > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > > too. So this can't be deprecated.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > * Rob, correct me if am wrong.
> > > > > > > >
> > > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > > mapping. What do you think?
> > > > > > >
> > > > > >
> > > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > > >
> > > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > > etc. Note the config-space is already defined by the ranges property
> > > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > > reg-name.
> > > > > >
> > > > >
> > > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > > "ranges", since ammending the binding would be an ABI break.
> > > > >
> > > > > > >
> > > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > > >
> > > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > > >
> > > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > > >
> > > > > > > we can add
> > > > > > >
> > > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > > >
> > > > > > > I think previous method was more extendable. How do you think?
> > > > > >
> > > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > > promising, more flexible and more portable across various PCIe
> > > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > > about that, not me.
> > > > > >
> > > > >
> > > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > > the way forward, since it has almost all the info needed by the drivers to
> > > > > allocate the memory regions.
> > > > >
> > > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > > property for EP controllers.
> > > >
> > > > I don't know that its worth the effort to carry both. Maybe if it is
> > > > useful on more than just DW host.
> > > >
> > > > I believe we had config space in ranges at some point on some
> > > > binding and moved away from that. I forget the reasoning.
> > >
> > > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > > in dwc host driver in v4.
> > >
> > > But I think it is wonthful to discuss if we can extend of_map bits, add
> > > more type beside CONFIG/IO/MEM/MEM64.
> > >
> > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > >
> > > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > >
> 
> > > There are '000' before 'ss'.  If we use it as dwc private resource.
> 
> Frank, why do you mis-inform about the idea? The point was to use the
> ranges property for:
> 1. PCIe Config-space mapping.
> 2. PCIe TLP messages region.
> There is _nothing_ DWC-specific in the original suggestion. Case 1 has
> already implicitly defined by the DT standard, see the link above (but
> for some reason hasn't been implemented in the PCIe subsystem). Case 2
> hasn't been determined, but could be seeing there are three unused
> bits in the ss-code of the phys.hi cell. All of that can be used by
> _any_ PCIe RC/EP device.
> 
> > 
> > DWC (or any host controller) specific things? No!
> 
> Rob, could you please dive deeper in this thread? The idea is to use
> the "ranges" property for the "config" (PCIe config space) and the
> custom PCIe TLP messages regions.

I did in my prior response. Here, I was just making it clear that 
something host controller specific is a non-starter as you did.

For config, we had some bindings that did this and we moved away from 
it. I don't remember the details. Unless it's ECAM region, I don't think 
using ranges makes any sense as how to use the region will still be host 
specific.

For TLP messages, do we have other hosts that could use ranges for them? 
Is there something in the PCIe spec that defines TLP as an address 
space and what that address space looks like? IIRC, some hosts (Altera?) 
just have a message sending interface and that includes config space 
accesses.

Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Serge Semin]

On Fri, Mar 01, 2024 at 10:08:16AM -0600, Rob Herring wrote:
> On Thu, Feb 29, 2024 at 02:26:34PM +0300, Serge Semin wrote:
> > On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> > > On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> > > >
> > > > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > > > >
> > > > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > > > >            const: config
> > > > > > > > > > > > > > +        - description:
> > > > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > > > +          const: msg
> > > > > > > > > > > > >
> > > > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > > > >
> > > > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > > > would have to be handled differently too.
> > > > > > > > > > >
> > > > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > > > >
> > > > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > > > >
> > > > > > > > > > @rob:
> > > > > > > > > >
> > > > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > > > detail direction?
> > > > > > > > >
> > > > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > > > property:
> > > > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > > > >
> > > > > > > > > So basically Rob meant back then that
> > > > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > > > above).
> > > > > > > > >
> > > > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > > > CPU address space and then let the software do the space
> > > > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > > > >
> > > > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > > > region. We can reuse this name.
> > > > > > > >
> > > > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > > > >
> > > > > > > > So we can deprecate 'config', even 'ranges'
> > > > > > >
> > > > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > > > regions.
> > > > > > >
> > > > > > > The "ranges" property is a part of the DT specification.  The
> > > > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > > > too. So this can't be deprecated.
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > * Rob, correct me if am wrong.
> > > > > > > > >
> > > > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > > > mapping. What do you think?
> > > > > > > >
> > > > > > >
> > > > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > > > >
> > > > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > > > etc. Note the config-space is already defined by the ranges property
> > > > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > > > reg-name.
> > > > > > >
> > > > > >
> > > > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > > > "ranges", since ammending the binding would be an ABI break.
> > > > > >
> > > > > > > >
> > > > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > > > >
> > > > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > > > >
> > > > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > > > >
> > > > > > > > we can add
> > > > > > > >
> > > > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > > > >
> > > > > > > > I think previous method was more extendable. How do you think?
> > > > > > >
> > > > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > > > promising, more flexible and more portable across various PCIe
> > > > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > > > about that, not me.
> > > > > > >
> > > > > >
> > > > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > > > the way forward, since it has almost all the info needed by the drivers to
> > > > > > allocate the memory regions.
> > > > > >
> > > > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > > > property for EP controllers.
> > > > >
> > > > > I don't know that its worth the effort to carry both. Maybe if it is
> > > > > useful on more than just DW host.
> > > > >
> > > > > I believe we had config space in ranges at some point on some
> > > > > binding and moved away from that. I forget the reasoning.
> > > >
> > > > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > > > in dwc host driver in v4.
> > > >
> > > > But I think it is wonthful to discuss if we can extend of_map bits, add
> > > > more type beside CONFIG/IO/MEM/MEM64.
> > > >
> > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > >
> > > > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > > >
> > 
> > > > There are '000' before 'ss'.  If we use it as dwc private resource.
> > 
> > Frank, why do you mis-inform about the idea? The point was to use the
> > ranges property for:
> > 1. PCIe Config-space mapping.
> > 2. PCIe TLP messages region.
> > There is _nothing_ DWC-specific in the original suggestion. Case 1 has
> > already implicitly defined by the DT standard, see the link above (but
> > for some reason hasn't been implemented in the PCIe subsystem). Case 2
> > hasn't been determined, but could be seeing there are three unused
> > bits in the ss-code of the phys.hi cell. All of that can be used by
> > _any_ PCIe RC/EP device.
> > 
> > > 
> > > DWC (or any host controller) specific things? No!
> > 
> > Rob, could you please dive deeper in this thread? The idea is to use
> > the "ranges" property for the "config" (PCIe config space) and the
> > custom PCIe TLP messages regions.
> 

> I did in my prior response. Here, I was just making it clear that 
> something host controller specific is a non-starter as you did.

Not sure what exactly you meant by "host controller specific". Did you
mean a particular host-controller or all the host-controllers? I meant
that the "msg" range could be used by _any_ host-controller, but the
usage would be platform-specific indeed because the message-type depends on
the peripheral devices.

> 
> For config, we had some bindings that did this and we moved away from 
> it. I don't remember the details. Unless it's ECAM region, I don't think 
> using ranges makes any sense as how to use the region will still be host 
> specific.

Could you please elaborate why exactly the config-region would still
be host-specific? Strictly speaking the normal MEM or IO region is
also host-specific because what lays behind depends on the attached
device and the enumeration procedure. IMO the reason of not using the
'ranges' for the config/ECAM space would be in opposite to what you
said.  Unlike the CPU-to-MEM/IO mapping the ECAM/config-space is a
pre-determined _linear_ space with in most of the case no need in
special space remapping (unless we would wish to map particular
peripheral device config-space). So normal "reg" is enough especially
seeing the config-space is a set of registers. (Please correct me if I
was wrong.)

> 
> For TLP messages, do we have other hosts that could use ranges for them? 

AFAICS the next controllers might also be able to generate the
messages via the outbound AT-memory:
Rockchip PCIe controller (see pcie-rockchip.h AXI_WRAPPER_* macros)
Cadence PCIe controller (see pcie-cadence.h CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_* macros)
Mediatek PCIe Gen3 controller (see pcie-mediatek-gen3.c PCIE_ATR_TLP_TYPE() macro)
...
although I am not absolutely sure.

> Is there something in the PCIe spec that defines TLP as an address 
> space and what that address space looks like? IIRC, some hosts (Altera?) 
> just have a message sending interface and that includes config space 
> accesses.

I already sited it in the message to Frank here:
https://lore.kernel.org/linux-pci/pprkba3ygxwv4lzieu5spqamcn2gzdcviv4kb2kzkzam4fbhit@6uqtmevzm5uj/
Here is an excerpt from there:

< Note paragraph 2.1.1. of for instance the PCIe-4.0 spec:
< 
< 2.1.1  Address Spaces, Transaction Types, and Usage
< "Transactions form the basis for information transfer between a
< Requester and Completer. Four _address spaces_ are defined, and
< different Transaction types are defined, each with its own unique
< intended usage, as shown in Table 2-12."
< 
< Address Space     | Transaction Types    |  Basic Usage
< -------------------------------------------------------------------------
< Memory            | Read/Write           | Transfer data to/from a
<                   |                      | memory-mapped location
< I/O               | Read/Write           | Transfer data to/from an
<                   |                      | I/O-mapped location
< Configuration     | Read/Write           | Device Function config/setup
< _Message_         | Baseline (including  | From event signaling mechanism
<                   | Vendor–Defined)      | to general purpose messaging
< 
< So basically the PCIe-spec defines four _address spaces_. The
< _message_ space is one of them. Seeing the "ranges" DT-property is
< about the space-to-space mapping IMO there is nothing wrong with using
< it for the _message_ space mapping.

As you can see the MEM, IO, config and Message are defined as address
space. Looking at the message request description in the spec, there
can be various types of the messages. All of them are listed in "2.2.8
Message Request Rules". Some of them can be routed by _address_ or
_ID_ (BDF), but some of them can lack of any address/ID field. In
accordance with the "Table 2-17: Message Routing" footnote there is no
message requests defined at the moment with the Address-based routing.
In the meantime for the address-less messages there is no address
translation needs to be performed, thus having the ranges-based
mapping would be just pointless for them. But if we had a message
request defined with the address-based routing then it might have
required a mapping similar to the MEM and IO ones.

Anyway giving to all of that a second thought, I more-and-more getting
further away from my original idea of having the config and message
region mapped over the "ranges" property. There is no actual address
translation performed at least in the second cases. So using the "ranges"
property for it would be pointless indeed... ( But originally the idea
seemed very attractive seeing the PCIe-specific "ranges" property has
unused mapping type flags and permitted special address format...


Let's get back to the Frank work then. What would you suggest as a
good solution? There are two options at the moment:
1. Define DWC-specific "msg" reg-name with a peace of the outbound
iATU space which would be used to generate the messages. (thus
implementing the same approach as being utilized for the config-space
mapping).
2. Manually, in the driver, reserve a peace of the CPU-to-PCIe-MEM
"ranges" region and have it utilized for the message request TLPs
(implemented in this patch).

The later one seems less safe since the entire outbound iATU range
could be dedicated for some platform-specific means. So reserving
a peace of it will cause problems in those platforms.

-Serge(y)

> 
> Rob

Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region

[Frank Li]

On Mon, Mar 04, 2024 at 09:48:56PM +0300, Serge Semin wrote:
> On Fri, Mar 01, 2024 at 10:08:16AM -0600, Rob Herring wrote:
> > On Thu, Feb 29, 2024 at 02:26:34PM +0300, Serge Semin wrote:
> > > On Wed, Feb 28, 2024 at 06:39:36PM -0600, Rob Herring wrote:
> > > > On Wed, Feb 28, 2024 at 10:23 AM Frank Li <Frank.li@nxp.com> wrote:
> > > > >
> > > > > On Wed, Feb 28, 2024 at 10:03:46AM -0600, Rob Herring wrote:
> > > > > > On Wed, Feb 14, 2024 at 11:44:12AM +0530, Manivannan Sadhasivam wrote:
> > > > > > > On Fri, Feb 09, 2024 at 12:52:52PM +0300, Serge Semin wrote:
> > > > > > > > On Wed, Feb 07, 2024 at 11:02:02AM -0500, Frank Li wrote:
> > > > > > > > > On Wed, Feb 07, 2024 at 03:37:30PM +0300, Serge Semin wrote:
> > > > > > > > > > On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> > > > > > > > > > > On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > > > > > > > > > > > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > > > > > > > > > > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > > > > > > > > > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > > > > > > > > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > > > > > > > > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > > > > > > > > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > > > > > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > > > > > > > > > > > @@ -101,6 +101,10 @@ properties:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > > > > > > > > > > > >              the peripheral PCIe devices configuration space.
> > > > > > > > > > > > > > >            const: config
> > > > > > > > > > > > > > > +        - description:
> > > > > > > > > > > > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > > > > > > > > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > > > > > > > > > > > +          const: msg
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > > > > > > > > > > > already expressed a concern regarding having the "config" reg-name
> > > > > > > > > > > > > > describing a memory space within the outbound iATU memory which is
> > > > > > > > > > > > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > > > > > > > > > > > similar semantics I guess won't receive warm welcome.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I do think it is a bit questionable. Ideally, the driver could
> > > > > > > > > > > > > just configure this on its own. However, since we don't describe all of
> > > > > > > > > > > > > the CPU address space (that's input to the iATU) already, that's not
> > > > > > > > > > > > > going to be possible. I suppose we could fix that, but then config space
> > > > > > > > > > > > > would have to be handled differently too.
> > > > > > > > > > > >
> > > > > > > > > > > > Sorry, I have not understand what your means. Do you means, you want
> > > > > > > > > > > > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU.
> > > > > > > > > > > >
> > > > > > > > > > > > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> > > > > > > > > > >
> > > > > > > > > > > @rob:
> > > > > > > > > > >
> > > > > > > > > > >     So far, I think "msg" is feasilbe solution. Or give me some little
> > > > > > > > > > > detail direction?
> > > > > > > > > >
> > > > > > > > > > Found the Rob' note about the iATU-space chunks utilized in the reg
> > > > > > > > > > property:
> > > > > > > > > > https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/
> > > > > > > > > >
> > > > > > > > > > So basically Rob meant back then that
> > > > > > > > > > either originally we should have defined a new reg-name like "atu-out"
> > > > > > > > > > with the entire outbound iATU CPU-space specified and unpin the
> > > > > > > > > > regions like "config"/"ecam"/"msg"/etc from there in the driver
> > > > > > > > > > or, well, stick to the chunking further. The later path was chosen
> > > > > > > > > > after the patch with the "ecam" reg-name was accepted (see the link
> > > > > > > > > > above).
> > > > > > > > > >
> > > > > > > > > > Really ECAM/config space access, custom TLP messages, legacy interrupt
> > > > > > > > > > TLPs, etc are all application-specific features. Each of them is
> > > > > > > > > > implemented based on a bit specific but basically the same outbound
> > > > > > > > > > iATU engine setup. Thus from the "DT is a hardware description" point
> > > > > > > > > > of view it would have been enough to describe the entire outbound iATU
> > > > > > > > > > CPU address space and then let the software do the space
> > > > > > > > > > reconfiguration in runtime based on it' application needs.
> > > > > > > > >
> > > > > > > > > There are "addr_space" in EP mode, which useful map out outbound iatu
> > > > > > > > > region. We can reuse this name.
> > > > > > > > >
> > > > > > > > > To keep compatiblity, cut hole from 'config' and 'ranges'. If there are
> > > > > > > > > not 'config', we can alloc a 1M(default) from top for 'config', then, 4K
> > > > > > > > > (default) for msg, 64K( for IO if not IO region in 'ranges'), left is
> > > > > > > > > mem region. We can config each region size by module parameter or drvdata.
> > > > > > > > >
> > > > > > > > > So we can deprecate 'config', even 'ranges'
> > > > > > > >
> > > > > > > > Not sure I fully understand what you mean. In anyway the "config" reg
> > > > > > > > name is highly utilized by the DW PCIe IP-core instances. We can't
> > > > > > > > deprecate it that easily. At least the backwards compatibility must be
> > > > > > > > preserved. Moreover "addr_space" is also just a single value reg which
> > > > > > > > won't solve a problem with the disjoint DW PCIe outbound iATU memory
> > > > > > > > regions.
> > > > > > > >
> > > > > > > > The "ranges" property is a part of the DT specification.  The
> > > > > > > > PCI-specific way of the property-based mapping is de-facto a standard
> > > > > > > > too. So this can't be deprecated.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > * Rob, correct me if am wrong.
> > > > > > > > > >
> > > > > > > > > > On the other hand it's possible to have more than one disjoint CPU
> > > > > > > > > > address region handled by the outbound iATU (especially if there is no
> > > > > > > > > > AXI-bridge enabled, see XALI - application transmit client interfaces
> > > > > > > > > > in HW manual). Thus having a single reg-property might get to be
> > > > > > > > > > inapplicable in some cases. Thinking about that got me to an idea.
> > > > > > > > > > What about just extending the PCIe "ranges" property flags
> > > > > > > > > > (IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
> > > > > > > > > > TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
> > > > > > > > > > use the flag to define a custom memory range for the TLP messages
> > > > > > > > > > generation. At some point it can be also utilized for the config-space
> > > > > > > > > > mapping. What do you think?
> > > > > > > > >
> > > > > > > >
> > > > > > > > > IORESOURCE_TYPE_BITS is 1f, Only 5bit. If extend IORESOURCE_TYPE_BITS,
> > > > > > > > > all IORESOURCE_* bit need move. And it is actual MEMORY regain.
> > > > > > > >
> > > > > > > > No. The lowest four bits aren't flags but the actual value. They are
> > > > > > > > retrieved from the PCI-specific memory ranges mapping:
> > > > > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_64.c#L141
> > > > > > > > https://elixir.bootlin.com/linux/latest/source/arch/sparc/kernel/of_device_32.c#L78
> > > > > > > > Currently only first four out of _sixteen_ values have been defined so
> > > > > > > > far. So we can freely use some of the free values for custom TLPs,
> > > > > > > > etc. Note the config-space is already defined by the ranges property
> > > > > > > > having the 0x0 space code (see the first link above), but it isn't
> > > > > > > > currently supported by the PCI subsystem. So at least that option can
> > > > > > > > be considered as a ready-to-implement replacement for the "config"
> > > > > > > > reg-name.
> > > > > > > >
> > > > > > >
> > > > > > > Agree. But still, the driver has to support both options: "config" reg name and
> > > > > > > "ranges", since ammending the binding would be an ABI break.
> > > > > > >
> > > > > > > > >
> > > > > > > > > Or we can use IORESOURCE_BITS (0xff)
> > > > > > > > >
> > > > > > > > > /* PCI ROM control bits (IORESOURCE_BITS) */
> > > > > > > > > #define IORESOURCE_ROM_ENABLE           (1<<0)  /* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
> > > > > > > > > #define IORESOURCE_ROM_SHADOW           (1<<1)  /* Use RAM image, not ROM BAR */
> > > > > > > > >
> > > > > > > > > /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> > > > > > > > > #define IORESOURCE_PCI_FIXED            (1<<4)  /* Do not move resource */
> > > > > > > > > #define IORESOURCE_PCI_EA_BEI           (1<<5)  /* BAR Equivalent Indicator */
> > > > > > > > >
> > > > > > > > > we can add
> > > > > > > > >
> > > > > > > > > IORESOURCE_PRIV_WINDOWS                 (1<<6)
> > > > > > > > >
> > > > > > > > > I think previous method was more extendable. How do you think?
> > > > > > > >
> > > > > > > > IMO extending the PCIe "ranges" property semantics looks more
> > > > > > > > promising, more flexible and more portable across various PCIe
> > > > > > > > controllers. But the most importantly is what Rob and Bjorn think
> > > > > > > > about that, not me.
> > > > > > > >
> > > > > > >
> > > > > > > IMO, using the "ranges" property to allocate arbitrary memory region should be
> > > > > > > the way forward, since it has almost all the info needed by the drivers to
> > > > > > > allocate the memory regions.
> > > > > > >
> > > > > > > But for the sake of DT backwards compatiblity, we have to keep supporting the
> > > > > > > existing reg entries (addr_space, et al.), because "ranges" is not a required
> > > > > > > property for EP controllers.
> > > > > >
> > > > > > I don't know that its worth the effort to carry both. Maybe if it is
> > > > > > useful on more than just DW host.
> > > > > >
> > > > > > I believe we had config space in ranges at some point on some
> > > > > > binding and moved away from that. I forget the reasoning.
> > > > >
> > > > > I can alloc a 64k windows from IORESOURCE_MEM windows to do 'msg' windows
> > > > > in dwc host driver in v4.
> > > > >
> > > > > But I think it is wonthful to discuss if we can extend of_map bits, add
> > > > > more type beside CONFIG/IO/MEM/MEM64.
> > > > >
> > > > > https://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> > > > >
> > > > > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > > > >
> > > 
> > > > > There are '000' before 'ss'.  If we use it as dwc private resource.
> > > 
> > > Frank, why do you mis-inform about the idea? The point was to use the
> > > ranges property for:
> > > 1. PCIe Config-space mapping.
> > > 2. PCIe TLP messages region.
> > > There is _nothing_ DWC-specific in the original suggestion. Case 1 has
> > > already implicitly defined by the DT standard, see the link above (but
> > > for some reason hasn't been implemented in the PCIe subsystem). Case 2
> > > hasn't been determined, but could be seeing there are three unused
> > > bits in the ss-code of the phys.hi cell. All of that can be used by
> > > _any_ PCIe RC/EP device.
> > > 
> > > > 
> > > > DWC (or any host controller) specific things? No!
> > > 
> > > Rob, could you please dive deeper in this thread? The idea is to use
> > > the "ranges" property for the "config" (PCIe config space) and the
> > > custom PCIe TLP messages regions.
> > 
> 
> > I did in my prior response. Here, I was just making it clear that 
> > something host controller specific is a non-starter as you did.
> 
> Not sure what exactly you meant by "host controller specific". Did you
> mean a particular host-controller or all the host-controllers? I meant
> that the "msg" range could be used by _any_ host-controller, but the
> usage would be platform-specific indeed because the message-type depends on
> the peripheral devices.
> 
> > 
> > For config, we had some bindings that did this and we moved away from 
> > it. I don't remember the details. Unless it's ECAM region, I don't think 
> > using ranges makes any sense as how to use the region will still be host 
> > specific.
> 
> Could you please elaborate why exactly the config-region would still
> be host-specific? Strictly speaking the normal MEM or IO region is
> also host-specific because what lays behind depends on the attached
> device and the enumeration procedure. IMO the reason of not using the
> 'ranges' for the config/ECAM space would be in opposite to what you
> said.  Unlike the CPU-to-MEM/IO mapping the ECAM/config-space is a
> pre-determined _linear_ space with in most of the case no need in
> special space remapping (unless we would wish to map particular
> peripheral device config-space). So normal "reg" is enough especially
> seeing the config-space is a set of registers. (Please correct me if I
> was wrong.)
> 
> > 
> > For TLP messages, do we have other hosts that could use ranges for them? 
> 
> AFAICS the next controllers might also be able to generate the
> messages via the outbound AT-memory:
> Rockchip PCIe controller (see pcie-rockchip.h AXI_WRAPPER_* macros)
> Cadence PCIe controller (see pcie-cadence.h CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_* macros)
> Mediatek PCIe Gen3 controller (see pcie-mediatek-gen3.c PCIE_ATR_TLP_TYPE() macro)
> ...
> although I am not absolutely sure.
> 
> > Is there something in the PCIe spec that defines TLP as an address 
> > space and what that address space looks like? IIRC, some hosts (Altera?) 
> > just have a message sending interface and that includes config space 
> > accesses.
> 
> I already sited it in the message to Frank here:
> https://lore.kernel.org/linux-pci/pprkba3ygxwv4lzieu5spqamcn2gzdcviv4kb2kzkzam4fbhit@6uqtmevzm5uj/
> Here is an excerpt from there:
> 
> < Note paragraph 2.1.1. of for instance the PCIe-4.0 spec:
> < 
> < 2.1.1  Address Spaces, Transaction Types, and Usage
> < "Transactions form the basis for information transfer between a
> < Requester and Completer. Four _address spaces_ are defined, and
> < different Transaction types are defined, each with its own unique
> < intended usage, as shown in Table 2-12."
> < 
> < Address Space     | Transaction Types    |  Basic Usage
> < -------------------------------------------------------------------------
> < Memory            | Read/Write           | Transfer data to/from a
> <                   |                      | memory-mapped location
> < I/O               | Read/Write           | Transfer data to/from an
> <                   |                      | I/O-mapped location
> < Configuration     | Read/Write           | Device Function config/setup
> < _Message_         | Baseline (including  | From event signaling mechanism
> <                   | Vendor–Defined)      | to general purpose messaging
> < 
> < So basically the PCIe-spec defines four _address spaces_. The
> < _message_ space is one of them. Seeing the "ranges" DT-property is
> < about the space-to-space mapping IMO there is nothing wrong with using
> < it for the _message_ space mapping.
> 
> As you can see the MEM, IO, config and Message are defined as address
> space. Looking at the message request description in the spec, there
> can be various types of the messages. All of them are listed in "2.2.8
> Message Request Rules". Some of them can be routed by _address_ or
> _ID_ (BDF), but some of them can lack of any address/ID field. In
> accordance with the "Table 2-17: Message Routing" footnote there is no
> message requests defined at the moment with the Address-based routing.
> In the meantime for the address-less messages there is no address
> translation needs to be performed, thus having the ranges-based
> mapping would be just pointless for them. But if we had a message
> request defined with the address-based routing then it might have
> required a mapping similar to the MEM and IO ones.
> 
> Anyway giving to all of that a second thought, I more-and-more getting
> further away from my original idea of having the config and message
> region mapped over the "ranges" property. There is no actual address
> translation performed at least in the second cases. So using the "ranges"
> property for it would be pointless indeed... ( But originally the idea
> seemed very attractive seeing the PCIe-specific "ranges" property has
> unused mapping type flags and permitted special address format...
> 
> 
> Let's get back to the Frank work then. What would you suggest as a
> good solution? There are two options at the moment:
> 1. Define DWC-specific "msg" reg-name with a peace of the outbound
> iATU space which would be used to generate the messages. (thus
> implementing the same approach as being utilized for the config-space
> mapping).
> 2. Manually, in the driver, reserve a peace of the CPU-to-PCIe-MEM
> "ranges" region and have it utilized for the message request TLPs
> (implemented in this patch).

link: https://lore.kernel.org/imx/20240213-pme_msg-v4-0-e2acd4d7a292@nxp.com/

> 
> The later one seems less safe since the entire outbound iATU range
> could be dedicated for some platform-specific means. So reserving
> a peace of it will cause problems in those platforms.

@Rob, @Bjorn

Please help comments about options: 1 and 2

Frank

> 
> -Serge(y)
> 
> > 
> > Rob

[PATCH v3 6/6] PCI: dwc: Add common send PME_Turn_Off message method

[Frank Li]

Set outbound ATU map memory write to send PCI message. So one MMIO write
can trigger a PCI message, such as PME_Turn_Off.

Add common dwc_pme_turn_off() function.

Call dwc_pme_turn_off() to send out PME_Turn_Off message in general
dw_pcie_suspend_noirq() if there are not platform callback pme_turn_off()
exist.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.c      |  8 ++++
 drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 267687ab33cbc..c177f1076ecea 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -728,6 +728,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		dev_warn(pci->dev, "Ranges exceed outbound iATU size (%d)\n",
 			 pci->num_ob_windows);
 
+	pci->msg_atu_index = i;
+
 	i = 0;
 	resource_list_for_each_entry(entry, &pp->bridge->dma_ranges) {
 		if (resource_type(entry->res) != IORESOURCE_MEM)
@@ -833,11 +835,50 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_setup_rc);
 
+/* Using message outbound ATU to send out PME_Turn_Off message for dwc PCIe controller */
+static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
+{
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
+	void __iomem *m;
+	int ret = 0;
+
+	if (pci->num_ob_windows <= pci->msg_atu_index)
+		return -EINVAL;
+
+	atu.code = PCIE_MSG_CODE_PME_TURN_OFF;
+	atu.routing = PCIE_MSG_TYPE_R_BC;
+	atu.type = PCIE_ATU_TYPE_MSG;
+	atu.size = pci->msg_io_size;
+
+	if (!atu.size) {
+		dev_dbg(pci->dev,
+			"atu memory map windows is zero, please check 'msg' reg in dts\n");
+		return -ENOMEM;
+	}
+
+	atu.cpu_addr = pci->msg_io_base;
+
+	ret = dw_pcie_prog_outbound_atu(pci, &atu);
+	if (ret)
+		return ret;
+
+	m = ioremap(atu.cpu_addr, PAGE_SIZE);
+	if (!m)
+		return -ENOMEM;
+
+	/* A dummy write is converted to a Msg TLP */
+	writel(0, m);
+
+	iounmap(m);
+
+	return ret;
+}
+
 int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 {
 	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	u32 val;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * If L1SS is supported, then do not put the link into L2 as some
@@ -849,10 +890,13 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
 		return 0;
 
-	if (!pci->pp.ops->pme_turn_off)
-		return 0;
+	if (pci->pp.ops->pme_turn_off)
+		pci->pp.ops->pme_turn_off(&pci->pp);
+	else
+		ret = dw_pcie_pme_turn_off(pci);
 
-	pci->pp.ops->pme_turn_off(&pci->pp);
+	if (ret)
+		return ret;
 
 	ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
 				PCIE_PME_TO_L2_TIMEOUT_US/10,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index ba909fade9db1..eb24362009bb6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -155,6 +155,14 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
 		}
 	}
 
+	if (!pci->msg_io_base) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "msg");
+		if (res) {
+			pci->msg_io_base = res->start;
+			pci->msg_io_size = res->end - res->start + 1;
+		}
+	}
+
 	/* LLDD is supposed to manually switch the clocks and resets state */
 	if (dw_pcie_cap_is(pci, REQ_RES)) {
 		ret = dw_pcie_get_clocks(pci);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 703b50bc5e0f1..866ab44df9fd1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -424,6 +424,9 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	int			msg_atu_index;
+	phys_addr_t		msg_io_base;
+	size_t			msg_io_size;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)

-- 
2.34.1