[PATCH v3 0/4] Add Qualcomm SA8255p based firmware managed PCIe root complex

[PATCH v3 0/4] Add Qualcomm SA8255p based firmware managed PCIe root complex

[Mayank Rana]

Based on received feedback, this patch series adds support with existing
Linux qcom-pcie.c driver to get PCIe host root complex functionality on
Qualcomm SA8255P auto platform.

1. Interface to allow requesting firmware to manage system resources and
performing PCIe Link up (devicetree binding in terms of power domain and
runtime PM APIs is used in driver)

2. SA8255P is using Synopsys Designware PCIe controller which supports MSI
controller. Using existing MSI controller based functionality by exporting
important pcie dwc core driver based MSI APIs, and using those from
pcie-qcom.c driver.

Below architecture is used on Qualcomm SA8255P auto platform to get ECAM
compliant PCIe controller based functionality. Here firmware VM based PCIe
driver takes care of resource management and performing PCIe link related
handling (D0 and D3cold). Linux pcie-qcom.c driver uses power domain to
request firmware VM to perform these operations using SCMI interface.
--------------------


                                   ┌────────────────────────┐                                               
                                   │                        │                                               
  ┌──────────────────────┐         │     SHARED MEMORY      │            ┌──────────────────────────┐       
  │     Firmware VM      │         │                        │            │         Linux VM         │       
  │ ┌─────────┐          │         │                        │            │    ┌────────────────┐    │       
  │ │ Drivers │ ┌──────┐ │         │                        │            │    │   PCIE Qcom    │    │       
  │ │ PCIE PHY◄─┤      │ │         │   ┌────────────────┐   │            │    │    driver      │    │       
  │ │         │ │ SCMI │ │         │   │                │   │            │    │                │    │       
  │ │PCIE CTL │ │      │ ├─────────┼───►    PCIE        ◄───┼─────┐      │    └──┬──────────▲──┘    │       
  │ │         ├─►Server│ │         │   │    SHMEM       │   │     │      │       │          │       │       
  │ │Clk, Vreg│ │      │ │         │   │                │   │     │      │    ┌──▼──────────┴──┐    │       
  │ │GPIO,GDSC│ └─▲──┬─┘ │         │   └────────────────┘   │     └──────┼────┤PCIE SCMI Inst  │    │       
  │ └─────────┘   │  │   │         │                        │            │    └──▲──────────┬──┘    │       
  │               │  │   │         │                        │            │       │          │       │       
  └───────────────┼──┼───┘         │                        │            └───────┼──────────┼───────┘       
                  │  │             │                        │                    │          │               
                  │  │             └────────────────────────┘                    │          │               
                  │  │                                                           │          │               
                  │  │                                                           │          │               
                  │  │                                                           │          │               
                  │  │                                                           │IRQ       │HVC            
              IRQ │  │HVC                                                        │          │               
                  │  │                                                           │          │               
                  │  │                                                           │          │               
                  │  │                                                           │          │               
┌─────────────────┴──▼───────────────────────────────────────────────────────────┴──────────▼──────────────┐
│                                                                                                          │
│                                                                                                          │
│                                      HYPERVISOR                                                          │
│                                                                                                          │
│                                                                                                          │
│                                                                                                          │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
                                                                                                            
  ┌─────────────┐    ┌─────────────┐  ┌──────────┐   ┌───────────┐   ┌─────────────┐  ┌────────────┐        
  │             │    │             │  │          │   │           │   │  PCIE       │  │   PCIE     │        
  │   CLOCK     │    │   REGULATOR │  │   GPIO   │   │   GDSC    │   │  PHY        │  │ controller │        
  └─────────────┘    └─────────────┘  └──────────┘   └───────────┘   └─────────────┘  └────────────┘        
                                                                                                            
----------
Changes in V3:
- Drop usage of PCIE host generic driver usage, and splitting of MSI functionality
- Modified existing pcie-qcom.c driver to add support for getting ECAM compliant and firmware managed
PCIe root complex functionality
Link to v2: https://lore.kernel.org/linux-arm-kernel/925d1eca-975f-4eec-bdf8-ca07a892361a@quicinc.com/T/

Changes in V2:
- Drop new PCIe Qcom ECAM driver, and use existing PCIe designware based MSI functionality
- Add power domain based functionality within existing ECAM driver
Link to v1: https://lore.kernel.org/all/d10199df-5fb3-407b-b404-a0a4d067341f@quicinc.com/T/                                                                                                      

Tested:
- Validated NVME functionality with PCIe0 on SA8255P-RIDE platform

Mayank Rana (3):
  PCI: dwc: Export dwc MSI controller related APIs
  PCI: qcom: Add firmware managed ECAM compliant PCIe root complex
    functionality
  dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root
    complex

 .../devicetree/bindings/pci/qcom,pcie-sa8255p.yaml | 100 +++++++++++++++++++++
 drivers/pci/controller/dwc/Kconfig                 |   1 +
 drivers/pci/controller/dwc/pcie-designware-host.c  |  38 ++++----
 drivers/pci/controller/dwc/pcie-designware.h       |  14 +++
 drivers/pci/controller/dwc/pcie-qcom.c             |  69 ++++++++++++--
 5 files changed, 199 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml

-- 
2.7.4

[PATCH v3 1/4] PCI: dwc: Export dwc MSI controller related APIs

[Mayank Rana]

To allow dwc PCIe controller based MSI functionality from ECAM pcie
driver, export dw_pcie_msi_host_init(), dw_pcie_msi_init() and
dw_pcie_msi_free() APIs. Also move MSI IRQ related initialization code
into dw_pcie_msi_init() as this code executes before dw_pcie_msi_init()
API to use with ECAM driver.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  | 14 +++++++
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c7290..25020a090db8 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -250,7 +250,7 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp)
 	return 0;
 }
 
-static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
+void dw_pcie_free_msi(struct dw_pcie_rp *pp)
 {
 	u32 ctrl;
 
@@ -263,19 +263,34 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
 	irq_domain_remove(pp->msi_domain);
 	irq_domain_remove(pp->irq_domain);
 }
+EXPORT_SYMBOL_GPL(dw_pcie_free_msi);
 
-static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
+void dw_pcie_msi_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	u64 msi_target = (u64)pp->msi_data;
+	u32 ctrl, num_ctrls;
 
 	if (!pci_msi_enabled() || !pp->has_msi_ctrl)
 		return;
 
+	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
+
+	/* Initialize IRQ Status array */
+	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
+		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
+				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+				    pp->irq_mask[ctrl]);
+		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
+				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+				    ~0);
+	}
+
 	/* Program the msi_data */
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
 }
+EXPORT_SYMBOL_GPL(dw_pcie_msi_init);
 
 static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
 {
@@ -317,7 +332,7 @@ static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
 	return 0;
 }
 
-static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
+int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct device *dev = pci->dev;
@@ -391,6 +406,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dw_pcie_msi_host_init);
 
 static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 {
@@ -802,7 +818,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	u32 val, ctrl, num_ctrls;
+	u32 val;
 	int ret;
 
 	/*
@@ -813,20 +829,6 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 
 	dw_pcie_setup(pci);
 
-	if (pp->has_msi_ctrl) {
-		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
-
-		/* Initialize IRQ Status array */
-		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
-			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
-					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
-					    pp->irq_mask[ctrl]);
-			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
-					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
-					    ~0);
-		}
-	}
-
 	dw_pcie_msi_init(pp);
 
 	/* Setup RC BARs */
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..ef748d82c663 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -679,6 +679,9 @@ static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
 
 #ifdef CONFIG_PCIE_DW_HOST
 irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
+void dw_pcie_msi_init(struct dw_pcie_rp *pp);
+int dw_pcie_msi_host_init(struct dw_pcie_rp *pp);
+void dw_pcie_free_msi(struct dw_pcie_rp *pp);
 int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
 int dw_pcie_host_init(struct dw_pcie_rp *pp);
 void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
@@ -691,6 +694,17 @@ static inline irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp)
 	return IRQ_NONE;
 }
 
+static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
+{ }
+
+static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	return -ENODEV;
+}
+
+static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
+{ }
+
 static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 {
 	return 0;
-- 
2.25.1

Re: [PATCH v3 1/4] PCI: dwc: Export dwc MSI controller related APIs

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 02:13:38PM -0800, Mayank Rana wrote:
> To allow dwc PCIe controller based MSI functionality from ECAM pcie
> driver, export dw_pcie_msi_host_init(), dw_pcie_msi_init() and
> dw_pcie_msi_free() APIs. Also move MSI IRQ related initialization code
> into dw_pcie_msi_init() as this code executes before dw_pcie_msi_init()
> API to use with ECAM driver.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.h  | 14 +++++++
>  2 files changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 3e41865c7290..25020a090db8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -250,7 +250,7 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> -static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> +void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>  {
>  	u32 ctrl;
>  
> @@ -263,19 +263,34 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>  	irq_domain_remove(pp->msi_domain);
>  	irq_domain_remove(pp->irq_domain);
>  }
> +EXPORT_SYMBOL_GPL(dw_pcie_free_msi);
>  
> -static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> +void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	u64 msi_target = (u64)pp->msi_data;
> +	u32 ctrl, num_ctrls;
>  
>  	if (!pci_msi_enabled() || !pp->has_msi_ctrl)
>  		return;
>  
> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> +
> +	/* Initialize IRQ Status array */
> +	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> +		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
> +				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> +				    pp->irq_mask[ctrl]);
> +		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
> +				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> +				    ~0);
> +	}
> +
>  	/* Program the msi_data */
>  	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>  	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>  }
> +EXPORT_SYMBOL_GPL(dw_pcie_msi_init);
>  
>  static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
>  {
> @@ -317,7 +332,7 @@ static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> -static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> +int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct device *dev = pci->dev;
> @@ -391,6 +406,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(dw_pcie_msi_host_init);
>  
>  static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>  {
> @@ -802,7 +818,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -	u32 val, ctrl, num_ctrls;
> +	u32 val;
>  	int ret;
>  
>  	/*
> @@ -813,20 +829,6 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  
>  	dw_pcie_setup(pci);
>  
> -	if (pp->has_msi_ctrl) {
> -		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> -
> -		/* Initialize IRQ Status array */
> -		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> -			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
> -					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> -					    pp->irq_mask[ctrl]);
> -			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
> -					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> -					    ~0);
> -		}
> -	}
> -
>  	dw_pcie_msi_init(pp);
>  
>  	/* Setup RC BARs */
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35a..ef748d82c663 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -679,6 +679,9 @@ static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
>  
>  #ifdef CONFIG_PCIE_DW_HOST
>  irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
> +void dw_pcie_msi_init(struct dw_pcie_rp *pp);
> +int dw_pcie_msi_host_init(struct dw_pcie_rp *pp);
> +void dw_pcie_free_msi(struct dw_pcie_rp *pp);
>  int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
>  int dw_pcie_host_init(struct dw_pcie_rp *pp);
>  void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
> @@ -691,6 +694,17 @@ static inline irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp)
>  	return IRQ_NONE;
>  }
>  
> +static void dw_pcie_msi_init(struct dw_pcie_rp *pp)

Missing 'inline' here and below?

- Mani

> +{ }
> +
> +static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> +{
> +	return -ENODEV;
> +}
> +
> +static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> +{ }
> +
>  static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  {
>  	return 0;
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 1/4] PCI: dwc: Export dwc MSI controller related APIs

[Mayank Rana]

On 11/15/2024 1:14 AM, Manivannan Sadhasivam wrote:
> On Wed, Nov 06, 2024 at 02:13:38PM -0800, Mayank Rana wrote:
>> To allow dwc PCIe controller based MSI functionality from ECAM pcie
>> driver, export dw_pcie_msi_host_init(), dw_pcie_msi_init() and
>> dw_pcie_msi_free() APIs. Also move MSI IRQ related initialization code
>> into dw_pcie_msi_init() as this code executes before dw_pcie_msi_init()
>> API to use with ECAM driver.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>   .../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++---------
>>   drivers/pci/controller/dwc/pcie-designware.h  | 14 +++++++
>>   2 files changed, 34 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 3e41865c7290..25020a090db8 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -250,7 +250,7 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp)
>>   	return 0;
>>   }
>>   
>> -static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>> +void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>>   {
>>   	u32 ctrl;
>>   
>> @@ -263,19 +263,34 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>>   	irq_domain_remove(pp->msi_domain);
>>   	irq_domain_remove(pp->irq_domain);
>>   }
>> +EXPORT_SYMBOL_GPL(dw_pcie_free_msi);
>>   
>> -static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>> +void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>   	u64 msi_target = (u64)pp->msi_data;
>> +	u32 ctrl, num_ctrls;
>>   
>>   	if (!pci_msi_enabled() || !pp->has_msi_ctrl)
>>   		return;
>>   
>> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>> +
>> +	/* Initialize IRQ Status array */
>> +	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
>> +		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
>> +				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>> +				    pp->irq_mask[ctrl]);
>> +		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
>> +				    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>> +				    ~0);
>> +	}
>> +
>>   	/* Program the msi_data */
>>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>>   }
>> +EXPORT_SYMBOL_GPL(dw_pcie_msi_init);
>>   
>>   static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
>>   {
>> @@ -317,7 +332,7 @@ static int dw_pcie_parse_split_msi_irq(struct dw_pcie_rp *pp)
>>   	return 0;
>>   }
>>   
>> -static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>> +int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>   	struct device *dev = pci->dev;
>> @@ -391,6 +406,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(dw_pcie_msi_host_init);
>>   
>>   static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>>   {
>> @@ -802,7 +818,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>   int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> -	u32 val, ctrl, num_ctrls;
>> +	u32 val;
>>   	int ret;
>>   
>>   	/*
>> @@ -813,20 +829,6 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>>   
>>   	dw_pcie_setup(pci);
>>   
>> -	if (pp->has_msi_ctrl) {
>> -		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>> -
>> -		/* Initialize IRQ Status array */
>> -		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
>> -			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
>> -					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>> -					    pp->irq_mask[ctrl]);
>> -			dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
>> -					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>> -					    ~0);
>> -		}
>> -	}
>> -
>>   	dw_pcie_msi_init(pp);
>>   
>>   	/* Setup RC BARs */
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 347ab74ac35a..ef748d82c663 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -679,6 +679,9 @@ static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
>>   
>>   #ifdef CONFIG_PCIE_DW_HOST
>>   irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
>> +void dw_pcie_msi_init(struct dw_pcie_rp *pp);
>> +int dw_pcie_msi_host_init(struct dw_pcie_rp *pp);
>> +void dw_pcie_free_msi(struct dw_pcie_rp *pp);
>>   int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
>>   int dw_pcie_host_init(struct dw_pcie_rp *pp);
>>   void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
>> @@ -691,6 +694,17 @@ static inline irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp)
>>   	return IRQ_NONE;
>>   }
>>   
>> +static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> 
> Missing 'inline' here and below?
ACK
> 
> - Mani
> 
>> +{ }
>> +
>> +static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>> +static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>> +{ }
>> +
>>   static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>>   {
>>   	return 0;
>> -- 
>> 2.25.1
>>
> 

[PATCH v3 2/4] PCI: host-generic: Export gen_pci_init() API to allow ECAM creation

[Mayank Rana]

Export gen_pci_init() API to create ECAM and initialized ECAM OPs
from PCIe driver which don't have way to populate driver_data as
just ECAM ops.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/pci/controller/pci-host-common.c | 3 ++-
 include/linux/pci-ecam.h                 | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index cf5f59a745b3..b9460a4c5b7e 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -20,7 +20,7 @@ static void gen_pci_unmap_cfg(void *ptr)
 	pci_ecam_free((struct pci_config_window *)ptr);
 }
 
-static struct pci_config_window *gen_pci_init(struct device *dev,
+struct pci_config_window *gen_pci_init(struct device *dev,
 		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops)
 {
 	int err;
@@ -48,6 +48,7 @@ static struct pci_config_window *gen_pci_init(struct device *dev,
 
 	return cfg;
 }
+EXPORT_SYMBOL_GPL(gen_pci_init);
 
 int pci_host_common_probe(struct platform_device *pdev)
 {
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 3a4860bd2758..386c08349169 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -94,5 +94,7 @@ extern const struct pci_ecam_ops loongson_pci_ecam_ops; /* Loongson PCIe */
 /* for DT-based PCI controllers that support ECAM */
 int pci_host_common_probe(struct platform_device *pdev);
 void pci_host_common_remove(struct platform_device *pdev);
+struct pci_config_window *gen_pci_init(struct device *dev,
+		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
 #endif
 #endif
-- 
2.25.1

Re: [PATCH v3 2/4] PCI: host-generic: Export gen_pci_init() API to allow ECAM creation

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 02:13:39PM -0800, Mayank Rana wrote:
> Export gen_pci_init() API to create ECAM and initialized ECAM OPs
> from PCIe driver which don't have way to populate driver_data as
> just ECAM ops.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/pci-host-common.c | 3 ++-
>  include/linux/pci-ecam.h                 | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index cf5f59a745b3..b9460a4c5b7e 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -20,7 +20,7 @@ static void gen_pci_unmap_cfg(void *ptr)
>  	pci_ecam_free((struct pci_config_window *)ptr);
>  }
>  
> -static struct pci_config_window *gen_pci_init(struct device *dev,
> +struct pci_config_window *gen_pci_init(struct device *dev,

Please rename the API to something like 'pci_host_common_init()'.
'gen_pci_init()' is fine within the driver, but doesn't look good when exported
outside.

- Mani

>  		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops)
>  {
>  	int err;
> @@ -48,6 +48,7 @@ static struct pci_config_window *gen_pci_init(struct device *dev,
>  
>  	return cfg;
>  }
> +EXPORT_SYMBOL_GPL(gen_pci_init);
>  
>  int pci_host_common_probe(struct platform_device *pdev)
>  {
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 3a4860bd2758..386c08349169 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -94,5 +94,7 @@ extern const struct pci_ecam_ops loongson_pci_ecam_ops; /* Loongson PCIe */
>  /* for DT-based PCI controllers that support ECAM */
>  int pci_host_common_probe(struct platform_device *pdev);
>  void pci_host_common_remove(struct platform_device *pdev);
> +struct pci_config_window *gen_pci_init(struct device *dev,
> +		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
>  #endif
>  #endif
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 2/4] PCI: host-generic: Export gen_pci_init() API to allow ECAM creation

[Mayank Rana]

On 11/15/2024 1:17 AM, Manivannan Sadhasivam wrote:
> On Wed, Nov 06, 2024 at 02:13:39PM -0800, Mayank Rana wrote:
>> Export gen_pci_init() API to create ECAM and initialized ECAM OPs
>> from PCIe driver which don't have way to populate driver_data as
>> just ECAM ops.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>   drivers/pci/controller/pci-host-common.c | 3 ++-
>>   include/linux/pci-ecam.h                 | 2 ++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
>> index cf5f59a745b3..b9460a4c5b7e 100644
>> --- a/drivers/pci/controller/pci-host-common.c
>> +++ b/drivers/pci/controller/pci-host-common.c
>> @@ -20,7 +20,7 @@ static void gen_pci_unmap_cfg(void *ptr)
>>   	pci_ecam_free((struct pci_config_window *)ptr);
>>   }
>>   
>> -static struct pci_config_window *gen_pci_init(struct device *dev,
>> +struct pci_config_window *gen_pci_init(struct device *dev,
> 
> Please rename the API to something like 'pci_host_common_init()'.
> 'gen_pci_init()' is fine within the driver, but doesn't look good when exported
> outside.
ACK

> - Mani
> 
>>   		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops)
>>   {
>>   	int err;
>> @@ -48,6 +48,7 @@ static struct pci_config_window *gen_pci_init(struct device *dev,
>>   
>>   	return cfg;
>>   }
>> +EXPORT_SYMBOL_GPL(gen_pci_init);
>>   
>>   int pci_host_common_probe(struct platform_device *pdev)
>>   {
>> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
>> index 3a4860bd2758..386c08349169 100644
>> --- a/include/linux/pci-ecam.h
>> +++ b/include/linux/pci-ecam.h
>> @@ -94,5 +94,7 @@ extern const struct pci_ecam_ops loongson_pci_ecam_ops; /* Loongson PCIe */
>>   /* for DT-based PCI controllers that support ECAM */
>>   int pci_host_common_probe(struct platform_device *pdev);
>>   void pci_host_common_remove(struct platform_device *pdev);
>> +struct pci_config_window *gen_pci_init(struct device *dev,
>> +		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
>>   #endif
>>   #endif
>> -- 
>> 2.25.1
>>
> 

[PATCH v3 3/4] dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root complex

[Mayank Rana]

On SA8255p, PCIe root complex is managed by firmware using power-domain
based handling. This root complex is configured as ECAM compliant.
Document required configuration to enable PCIe root complex.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 .../bindings/pci/qcom,pcie-sa8255p.yaml       | 103 ++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
new file mode 100644
index 000000000000..9b09c3923ba0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/qcom,pcie-sa8255p.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SA8255p based firmware managed and ECAM compliant PCIe Root Complex
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+description:
+  Qualcomm SA8255p SoC PCIe root complex controller is based on the Synopsys
+  DesignWare PCIe IP which is managed by firmware, and configured in ECAM mode.
+
+properties:
+  compatible:
+    const: qcom,pcie-sa8255p
+
+  reg:
+    description:
+      The Configuration Space base address and size, as accessed from the parent
+      bus. The base address corresponds to the first bus in the "bus-range"
+      property. If no "bus-range" is specified, this will be bus 0 (the
+      default).
+    maxItems: 1
+
+  ranges:
+    description:
+      As described in IEEE Std 1275-1994, but must provide at least a
+      definition of non-prefetchable memory. One or both of prefetchable Memory
+      may also be provided.
+    minItems: 1
+    maxItems: 2
+
+  interrupts:
+    minItems: 8
+    maxItems: 8
+
+  interrupt-names:
+    items:
+      - const: msi0
+      - const: msi1
+      - const: msi2
+      - const: msi3
+      - const: msi4
+      - const: msi5
+      - const: msi6
+      - const: msi7
+
+  power-domains:
+    maxItems: 1
+
+  dma-coherent: true
+  iommu-map: true
+
+required:
+  - compatible
+  - reg
+  - ranges
+  - power-domains
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pci@1c00000 {
+           compatible = "qcom,pcie-sa8255p";
+           reg = <0x4 0x00000000 0 0x10000000>;
+           device_type = "pci";
+           #address-cells = <3>;
+           #size-cells = <2>;
+           ranges = <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>,
+                    <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x40000000>;
+           bus-range = <0x00 0xff>;
+           dma-coherent;
+           linux,pci-domain = <0>;
+           power-domains = <&scmi5_pd 0>;
+           iommu-map = <0x0 &pcie_smmu 0x0000 0x1>,
+                       <0x100 &pcie_smmu 0x0001 0x1>;
+           interrupt-parent = <&intc>;
+           interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>,
+                        <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>;
+           interrupt-names = "msi0", "msi1", "msi2", "msi3",
+                                  "msi4", "msi5", "msi6", "msi7";
+        };
+    };
-- 
2.25.1

Re: [PATCH v3 3/4] dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root complex

[Krzysztof Kozlowski]

On 06/11/2024 23:13, Mayank Rana wrote:
> On SA8255p, PCIe root complex is managed by firmware using power-domain
> based handling. This root complex is configured as ECAM compliant.
> Document required configuration to enable PCIe root complex.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  .../bindings/pci/qcom,pcie-sa8255p.yaml       | 103 ++++++++++++++++++

NAK

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

BTW, you also Cc-ed incorrect addresses :/

Best regards,
Krzysztof

Re: [PATCH v3 3/4] dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root complex

[Mayank Rana]

On 11/7/2024 1:35 AM, Krzysztof Kozlowski wrote:
> On 06/11/2024 23:13, Mayank Rana wrote:
>> On SA8255p, PCIe root complex is managed by firmware using power-domain
>> based handling. This root complex is configured as ECAM compliant.
>> Document required configuration to enable PCIe root complex.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>   .../bindings/pci/qcom,pcie-sa8255p.yaml       | 103 ++++++++++++++++++
> 
> NAK
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
> 
> BTW, you also Cc-ed incorrect addresses :/
sorry, I missed adding right set of folks and mailing list here.
I shall resend after updating correct reviewers.

> Best regards,
> Krzysztof
> 

Re: [PATCH v3 3/4] dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root complex

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 02:13:40PM -0800, Mayank Rana wrote:
> On SA8255p, PCIe root complex is managed by firmware using power-domain
> based handling. This root complex is configured as ECAM compliant.
> Document required configuration to enable PCIe root complex.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  .../bindings/pci/qcom,pcie-sa8255p.yaml       | 103 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
> new file mode 100644
> index 000000000000..9b09c3923ba0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-sa8255p.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SA8255p based firmware managed and ECAM compliant PCIe Root Complex
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> +
> +description:
> +  Qualcomm SA8255p SoC PCIe root complex controller is based on the Synopsys
> +  DesignWare PCIe IP which is managed by firmware, and configured in ECAM mode.
> +
> +properties:
> +  compatible:
> +    const: qcom,pcie-sa8255p
> +
> +  reg:
> +    description:
> +      The Configuration Space base address and size, as accessed from the parent
> +      bus. The base address corresponds to the first bus in the "bus-range"
> +      property. If no "bus-range" is specified, this will be bus 0 (the
> +      default).

I don't think the 'no bus-range' configuration is supported. You can get rid if
this statement.

> +    maxItems: 1
> +
> +  ranges:
> +    description:
> +      As described in IEEE Std 1275-1994, but must provide at least a
> +      definition of non-prefetchable memory. One or both of prefetchable Memory
> +      may also be provided.
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupts:
> +    minItems: 8
> +    maxItems: 8
> +
> +  interrupt-names:
> +    items:
> +      - const: msi0
> +      - const: msi1
> +      - const: msi2
> +      - const: msi3
> +      - const: msi4
> +      - const: msi5
> +      - const: msi6
> +      - const: msi7
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  dma-coherent: true
> +  iommu-map: true

Can't you add 'msi-map' also since the hardware supports it?

- Mani

> +
> +required:
> +  - compatible
> +  - reg
> +  - ranges
> +  - power-domains
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pci@1c00000 {
> +           compatible = "qcom,pcie-sa8255p";
> +           reg = <0x4 0x00000000 0 0x10000000>;
> +           device_type = "pci";
> +           #address-cells = <3>;
> +           #size-cells = <2>;
> +           ranges = <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>,
> +                    <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x40000000>;
> +           bus-range = <0x00 0xff>;
> +           dma-coherent;
> +           linux,pci-domain = <0>;
> +           power-domains = <&scmi5_pd 0>;
> +           iommu-map = <0x0 &pcie_smmu 0x0000 0x1>,
> +                       <0x100 &pcie_smmu 0x0001 0x1>;
> +           interrupt-parent = <&intc>;
> +           interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>,
> +                        <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>;
> +           interrupt-names = "msi0", "msi1", "msi2", "msi3",
> +                                  "msi4", "msi5", "msi6", "msi7";
> +        };
> +    };
> -- 
> 2.25.1
> 

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

[PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Mayank Rana]

On SA8255p ride platform, PCIe root complex is firmware managed as well
configured into ECAM compliant mode. This change adds functionality to
enable resource management (system resource as well PCIe controller and
PHY configuration) through firmware, and enumerating ECAM compliant root
complex.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/pci/controller/dwc/Kconfig     |   1 +
 drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
 2 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..0fe76bd39d69 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -275,6 +275,7 @@ config PCIE_QCOM
 	select PCIE_DW_HOST
 	select CRC8
 	select PCIE_QCOM_COMMON
+	select PCI_HOST_COMMON
 	help
 	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
 	  PCIe controller uses the DesignWare core plus Qualcomm-specific
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ef44a82be058..2cb74f902baf 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -21,7 +21,9 @@
 #include <linux/limits.h>
 #include <linux/init.h>
 #include <linux/of.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
+#include <linux/pci-ecam.h>
 #include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
@@ -254,10 +256,12 @@ struct qcom_pcie_ops {
   * @ops: qcom PCIe ops structure
   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
   * snooping
+  * @firmware_managed: Set if PCIe root complex is firmware managed
   */
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
 	bool override_no_snoop;
+	bool firmware_managed;
 	bool no_l0s;
 };
 
@@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
 	.no_l0s = true,
 };
 
+static const struct qcom_pcie_cfg cfg_fw_managed = {
+	.firmware_managed = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
@@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void qcom_pci_free_msi(void *ptr)
+{
+	struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
+
+	if (pp && pp->has_msi_ctrl)
+		dw_pcie_free_msi(pp);
+}
+
+static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
+{
+	struct device *dev = cfg->parent;
+	struct dw_pcie_rp *pp;
+	struct dw_pcie *pci;
+	int ret;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pp = &pci->pp;
+	pci->dbi_base = cfg->win;
+	pp->num_vectors = MSI_DEF_NUM_VECTORS;
+
+	ret = dw_pcie_msi_host_init(pp);
+	if (ret)
+		return ret;
+
+	pp->has_msi_ctrl = true;
+	dw_pcie_msi_init(pp);
+
+	ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
+	return ret;
+}
+
+/* ECAM ops */
+const struct pci_ecam_ops pci_qcom_ecam_ops = {
+	.init		= qcom_pcie_ecam_host_init,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
+
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	const struct qcom_pcie_cfg *pcie_cfg;
@@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	char *name;
 
 	pcie_cfg = of_device_get_match_data(dev);
-	if (!pcie_cfg || !pcie_cfg->ops) {
-		dev_err(dev, "Invalid platform data\n");
+	if (!pcie_cfg) {
+		dev_err(dev, "No platform data\n");
+		return -EINVAL;
+	}
+
+	if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
+		dev_err(dev, "No platform ops\n");
 		return -EINVAL;
 	}
 
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto err_pm_runtime_put;
+
+	if (pcie_cfg->firmware_managed) {
+		struct pci_host_bridge *bridge;
+		struct pci_config_window *cfg;
+
+		bridge = devm_pci_alloc_host_bridge(dev, 0);
+		if (!bridge) {
+			ret = -ENOMEM;
+			goto err_pm_runtime_put;
+		}
+
+		of_pci_check_probe_only();
+		/* Parse and map our Configuration Space windows */
+		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
+		if (IS_ERR(cfg)) {
+			ret = PTR_ERR(cfg);
+			goto err_pm_runtime_put;
+		}
+
+		bridge->sysdata = cfg;
+		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
+		bridge->msi_domain = true;
+
+		ret = pci_host_probe(bridge);
+		if (ret) {
+			dev_err(dev, "pci_host_probe() failed:%d\n", ret);
+			goto err_pm_runtime_put;
+		}
+
+		return ret;
+	}
+
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
@@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (!pci)
 		return -ENOMEM;
 
-	pm_runtime_enable(dev);
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto err_pm_runtime_put;
-
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
 	pp = &pci->pp;
@@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 static int qcom_pcie_suspend_noirq(struct device *dev)
 {
-	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	struct qcom_pcie *pcie;
 	int ret = 0;
 
+	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
+		return 0;
+
+	pcie = dev_get_drvdata(dev);
 	/*
 	 * Set minimum bandwidth required to keep data path functional during
 	 * suspend.
@@ -1795,9 +1888,13 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 
 static int qcom_pcie_resume_noirq(struct device *dev)
 {
-	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	struct qcom_pcie *pcie;
 	int ret;
 
+	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
+		return 0;
+
+	pcie = dev_get_drvdata(dev);
 	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
 		ret = icc_enable(pcie->icc_cpu);
 		if (ret) {
@@ -1830,6 +1927,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
 	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
 	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
+	{ .compatible = "qcom,pcie-sa8255p", .data = &cfg_fw_managed },
 	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp },
 	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
 	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
-- 
2.25.1

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[neil.armstrong]

Hi,

On 06/11/2024 23:13, Mayank Rana wrote:
> On SA8255p ride platform, PCIe root complex is firmware managed as well
> configured into ECAM compliant mode. This change adds functionality to
> enable resource management (system resource as well PCIe controller and
> PHY configuration) through firmware, and enumerating ECAM compliant root
> complex.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>   drivers/pci/controller/dwc/Kconfig     |   1 +
>   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>   2 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b6d6778b0698..0fe76bd39d69 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -275,6 +275,7 @@ config PCIE_QCOM
>   	select PCIE_DW_HOST
>   	select CRC8
>   	select PCIE_QCOM_COMMON
> +	select PCI_HOST_COMMON
>   	help
>   	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>   	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ef44a82be058..2cb74f902baf 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -21,7 +21,9 @@
>   #include <linux/limits.h>
>   #include <linux/init.h>
>   #include <linux/of.h>
> +#include <linux/of_pci.h>
>   #include <linux/pci.h>
> +#include <linux/pci-ecam.h>
>   #include <linux/pm_opp.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/platform_device.h>
> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>     * @ops: qcom PCIe ops structure
>     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
>     * snooping
> +  * @firmware_managed: Set if PCIe root complex is firmware managed
>     */
>   struct qcom_pcie_cfg {
>   	const struct qcom_pcie_ops *ops;
>   	bool override_no_snoop;
> +	bool firmware_managed;
>   	bool no_l0s;
>   };
>   
> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>   	.no_l0s = true,
>   };
>   
> +static const struct qcom_pcie_cfg cfg_fw_managed = {
> +	.firmware_managed = true,
> +};
> +
>   static const struct dw_pcie_ops dw_pcie_ops = {
>   	.link_up = qcom_pcie_link_up,
>   	.start_link = qcom_pcie_start_link,
> @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>   	return IRQ_HANDLED;
>   }
>   
> +static void qcom_pci_free_msi(void *ptr)
> +{
> +	struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
> +
> +	if (pp && pp->has_msi_ctrl)
> +		dw_pcie_free_msi(pp);
> +}
> +
> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
> +{
> +	struct device *dev = cfg->parent;
> +	struct dw_pcie_rp *pp;
> +	struct dw_pcie *pci;
> +	int ret;
> +
> +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +	if (!pci)
> +		return -ENOMEM;
> +
> +	pci->dev = dev;
> +	pp = &pci->pp;
> +	pci->dbi_base = cfg->win;
> +	pp->num_vectors = MSI_DEF_NUM_VECTORS;
> +
> +	ret = dw_pcie_msi_host_init(pp);
> +	if (ret)
> +		return ret;
> +
> +	pp->has_msi_ctrl = true;
> +	dw_pcie_msi_init(pp);
> +
> +	ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
> +	return ret;
> +}
> +
> +/* ECAM ops */
> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
> +	.init		= qcom_pcie_ecam_host_init,
> +	.pci_ops	= {
> +		.map_bus	= pci_ecam_map_bus,
> +		.read		= pci_generic_config_read,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +
>   static int qcom_pcie_probe(struct platform_device *pdev)
>   {
>   	const struct qcom_pcie_cfg *pcie_cfg;
> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   	char *name;
>   
>   	pcie_cfg = of_device_get_match_data(dev);
> -	if (!pcie_cfg || !pcie_cfg->ops) {
> -		dev_err(dev, "Invalid platform data\n");
> +	if (!pcie_cfg) {
> +		dev_err(dev, "No platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
> +		dev_err(dev, "No platform ops\n");
>   		return -EINVAL;
>   	}
>   
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0)
> +		goto err_pm_runtime_put;
> +
> +	if (pcie_cfg->firmware_managed) {
> +		struct pci_host_bridge *bridge;
> +		struct pci_config_window *cfg;
> +
> +		bridge = devm_pci_alloc_host_bridge(dev, 0);
> +		if (!bridge) {
> +			ret = -ENOMEM;
> +			goto err_pm_runtime_put;
> +		}
> +
> +		of_pci_check_probe_only();
> +		/* Parse and map our Configuration Space windows */
> +		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
> +		if (IS_ERR(cfg)) {
> +			ret = PTR_ERR(cfg);
> +			goto err_pm_runtime_put;
> +		}
> +
> +		bridge->sysdata = cfg;
> +		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
> +		bridge->msi_domain = true;
> +
> +		ret = pci_host_probe(bridge);
> +		if (ret) {
> +			dev_err(dev, "pci_host_probe() failed:%d\n", ret);
> +			goto err_pm_runtime_put;
> +		}
> +
> +		return ret;
> +	}
> +
>   	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>   	if (!pcie)
>   		return -ENOMEM;
> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   	if (!pci)
>   		return -ENOMEM;
>   
> -	pm_runtime_enable(dev);
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> -		goto err_pm_runtime_put;
> -
>   	pci->dev = dev;
>   	pci->ops = &dw_pcie_ops;
>   	pp = &pci->pp;
> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   
>   static int qcom_pcie_suspend_noirq(struct device *dev)
>   {
> -	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	struct qcom_pcie *pcie;
>   	int ret = 0;
>   
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))

Can't you use if (pcie_cfg->firmware_managed) here instead ?

> +		return 0;
> +
> +	pcie = dev_get_drvdata(dev);
>   	/*
>   	 * Set minimum bandwidth required to keep data path functional during
>   	 * suspend.
> @@ -1795,9 +1888,13 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>   
>   static int qcom_pcie_resume_noirq(struct device *dev)
>   {
> -	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	struct qcom_pcie *pcie;
>   	int ret;
>   
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))

Ditto

> +		return 0;
> +
> +	pcie = dev_get_drvdata(dev);
>   	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>   		ret = icc_enable(pcie->icc_cpu);
>   		if (ret) {
> @@ -1830,6 +1927,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>   	{ .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
>   	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>   	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> +	{ .compatible = "qcom,pcie-sa8255p", .data = &cfg_fw_managed },
>   	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp },
>   	{ .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
>   	{ .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },

Thanks,
Neil

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Mayank Rana]

On 11/7/2024 12:45 AM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 06/11/2024 23:13, Mayank Rana wrote:
>> On SA8255p ride platform, PCIe root complex is firmware managed as well
>> configured into ECAM compliant mode. This change adds functionality to
>> enable resource management (system resource as well PCIe controller and
>> PHY configuration) through firmware, and enumerating ECAM compliant root
>> complex.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/Kconfig     |   1 +
>>   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>>   2 files changed, 108 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig 
>> b/drivers/pci/controller/dwc/Kconfig
>> index b6d6778b0698..0fe76bd39d69 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -275,6 +275,7 @@ config PCIE_QCOM
>>       select PCIE_DW_HOST
>>       select CRC8
>>       select PCIE_QCOM_COMMON
>> +    select PCI_HOST_COMMON
>>       help
>>         Say Y here to enable PCIe controller support on Qualcomm SoCs. 
>> The
>>         PCIe controller uses the DesignWare core plus Qualcomm-specific
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>> index ef44a82be058..2cb74f902baf 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -21,7 +21,9 @@
>>   #include <linux/limits.h>
>>   #include <linux/init.h>
>>   #include <linux/of.h>
>> +#include <linux/of_pci.h>
>>   #include <linux/pci.h>
>> +#include <linux/pci-ecam.h>
>>   #include <linux/pm_opp.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/platform_device.h>
>> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>>     * @ops: qcom PCIe ops structure
>>     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable 
>> cache
>>     * snooping
>> +  * @firmware_managed: Set if PCIe root complex is firmware managed
>>     */
>>   struct qcom_pcie_cfg {
>>       const struct qcom_pcie_ops *ops;
>>       bool override_no_snoop;
>> +    bool firmware_managed;
>>       bool no_l0s;
>>   };
>> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>>       .no_l0s = true,
>>   };
>> +static const struct qcom_pcie_cfg cfg_fw_managed = {
>> +    .firmware_managed = true,
>> +};
>> +
>>   static const struct dw_pcie_ops dw_pcie_ops = {
>>       .link_up = qcom_pcie_link_up,
>>       .start_link = qcom_pcie_start_link,
>> @@ -1566,6 +1574,51 @@ static irqreturn_t 
>> qcom_pcie_global_irq_thread(int irq, void *data)
>>       return IRQ_HANDLED;
>>   }
>> +static void qcom_pci_free_msi(void *ptr)
>> +{
>> +    struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
>> +
>> +    if (pp && pp->has_msi_ctrl)
>> +        dw_pcie_free_msi(pp);
>> +}
>> +
>> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>> +{
>> +    struct device *dev = cfg->parent;
>> +    struct dw_pcie_rp *pp;
>> +    struct dw_pcie *pci;
>> +    int ret;
>> +
>> +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>> +    if (!pci)
>> +        return -ENOMEM;
>> +
>> +    pci->dev = dev;
>> +    pp = &pci->pp;
>> +    pci->dbi_base = cfg->win;
>> +    pp->num_vectors = MSI_DEF_NUM_VECTORS;
>> +
>> +    ret = dw_pcie_msi_host_init(pp);
>> +    if (ret)
>> +        return ret;
>> +
>> +    pp->has_msi_ctrl = true;
>> +    dw_pcie_msi_init(pp);
>> +
>> +    ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>> +    return ret;
>> +}
>> +
>> +/* ECAM ops */
>> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
>> +    .init        = qcom_pcie_ecam_host_init,
>> +    .pci_ops    = {
>> +        .map_bus    = pci_ecam_map_bus,
>> +        .read        = pci_generic_config_read,
>> +        .write        = pci_generic_config_write,
>> +    }
>> +};
>> +
>>   static int qcom_pcie_probe(struct platform_device *pdev)
>>   {
>>       const struct qcom_pcie_cfg *pcie_cfg;
>> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct 
>> platform_device *pdev)
>>       char *name;
>>       pcie_cfg = of_device_get_match_data(dev);
>> -    if (!pcie_cfg || !pcie_cfg->ops) {
>> -        dev_err(dev, "Invalid platform data\n");
>> +    if (!pcie_cfg) {
>> +        dev_err(dev, "No platform data\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
>> +        dev_err(dev, "No platform ops\n");
>>           return -EINVAL;
>>       }
>> +    pm_runtime_enable(dev);
>> +    ret = pm_runtime_get_sync(dev);
>> +    if (ret < 0)
>> +        goto err_pm_runtime_put;
>> +
>> +    if (pcie_cfg->firmware_managed) {
>> +        struct pci_host_bridge *bridge;
>> +        struct pci_config_window *cfg;
>> +
>> +        bridge = devm_pci_alloc_host_bridge(dev, 0);
>> +        if (!bridge) {
>> +            ret = -ENOMEM;
>> +            goto err_pm_runtime_put;
>> +        }
>> +
>> +        of_pci_check_probe_only();
>> +        /* Parse and map our Configuration Space windows */
>> +        cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
>> +        if (IS_ERR(cfg)) {
>> +            ret = PTR_ERR(cfg);
>> +            goto err_pm_runtime_put;
>> +        }
>> +
>> +        bridge->sysdata = cfg;
>> +        bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
>> +        bridge->msi_domain = true;
>> +
>> +        ret = pci_host_probe(bridge);
>> +        if (ret) {
>> +            dev_err(dev, "pci_host_probe() failed:%d\n", ret);
>> +            goto err_pm_runtime_put;
>> +        }
>> +
>> +        return ret;
>> +    }
>> +
>>       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>>       if (!pcie)
>>           return -ENOMEM;
>> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct 
>> platform_device *pdev)
>>       if (!pci)
>>           return -ENOMEM;
>> -    pm_runtime_enable(dev);
>> -    ret = pm_runtime_get_sync(dev);
>> -    if (ret < 0)
>> -        goto err_pm_runtime_put;
>> -
>>       pci->dev = dev;
>>       pci->ops = &dw_pcie_ops;
>>       pp = &pci->pp;
>> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct 
>> platform_device *pdev)
>>   static int qcom_pcie_suspend_noirq(struct device *dev)
>>   {
>> -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +    struct qcom_pcie *pcie;
>>       int ret = 0;
>> +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
> 
> Can't you use if (pcie_cfg->firmware_managed) here instead ?
yes, although with firmware managed mode, struct qcom_pcie *pcie is not 
allocated, and just
to get access to pcie_cfg for this check, I took this approach. I am 
thiking to do allocating struct qcom_pcie *pcie and using it in future 
if we need more other related functionality which needs usage of this 
structure for functionality like global interrupt etc.

Although if you still prefer to allocate struct qcom_pcie based memory 
to access pcie_cfg, then I can consider to update in next patchset. 
Please suggest.
>> +        return 0;
>> +
>> +    pcie = dev_get_drvdata(dev);
>>       /*
>>        * Set minimum bandwidth required to keep data path functional 
>> during
>>        * suspend.
>> @@ -1795,9 +1888,13 @@ static int qcom_pcie_suspend_noirq(struct 
>> device *dev)
>>   static int qcom_pcie_resume_noirq(struct device *dev)
>>   {
>> -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +    struct qcom_pcie *pcie;
>>       int ret;
>> +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
> 
> Ditto
> 
>> +        return 0;
>> +
>> +    pcie = dev_get_drvdata(dev);
>>       if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>>           ret = icc_enable(pcie->icc_cpu);
>>           if (ret) {
>> @@ -1830,6 +1927,7 @@ static const struct of_device_id 
>> qcom_pcie_match[] = {
>>       { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
>>       { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>>       { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>> +    { .compatible = "qcom,pcie-sa8255p", .data = &cfg_fw_managed },
>>       { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp },
>>       { .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
>>       { .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> 
> Thanks,
> Neil

Regards,
Mayank

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[neil.armstrong]

On 07/11/2024 18:45, Mayank Rana wrote:
> 
> 
> On 11/7/2024 12:45 AM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 06/11/2024 23:13, Mayank Rana wrote:
>>> On SA8255p ride platform, PCIe root complex is firmware managed as well
>>> configured into ECAM compliant mode. This change adds functionality to
>>> enable resource management (system resource as well PCIe controller and
>>> PHY configuration) through firmware, and enumerating ECAM compliant root
>>> complex.
>>>
>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>>> ---
>>>   drivers/pci/controller/dwc/Kconfig     |   1 +
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>>>   2 files changed, 108 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>>> index b6d6778b0698..0fe76bd39d69 100644
>>> --- a/drivers/pci/controller/dwc/Kconfig
>>> +++ b/drivers/pci/controller/dwc/Kconfig
>>> @@ -275,6 +275,7 @@ config PCIE_QCOM
>>>       select PCIE_DW_HOST
>>>       select CRC8
>>>       select PCIE_QCOM_COMMON
>>> +    select PCI_HOST_COMMON
>>>       help
>>>         Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>>>         PCIe controller uses the DesignWare core plus Qualcomm-specific
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index ef44a82be058..2cb74f902baf 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -21,7 +21,9 @@
>>>   #include <linux/limits.h>
>>>   #include <linux/init.h>
>>>   #include <linux/of.h>
>>> +#include <linux/of_pci.h>
>>>   #include <linux/pci.h>
>>> +#include <linux/pci-ecam.h>
>>>   #include <linux/pm_opp.h>
>>>   #include <linux/pm_runtime.h>
>>>   #include <linux/platform_device.h>
>>> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>>>     * @ops: qcom PCIe ops structure
>>>     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
>>>     * snooping
>>> +  * @firmware_managed: Set if PCIe root complex is firmware managed
>>>     */
>>>   struct qcom_pcie_cfg {
>>>       const struct qcom_pcie_ops *ops;
>>>       bool override_no_snoop;
>>> +    bool firmware_managed;
>>>       bool no_l0s;
>>>   };
>>> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>>>       .no_l0s = true,
>>>   };
>>> +static const struct qcom_pcie_cfg cfg_fw_managed = {
>>> +    .firmware_managed = true,
>>> +};
>>> +
>>>   static const struct dw_pcie_ops dw_pcie_ops = {
>>>       .link_up = qcom_pcie_link_up,
>>>       .start_link = qcom_pcie_start_link,
>>> @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>>       return IRQ_HANDLED;
>>>   }
>>> +static void qcom_pci_free_msi(void *ptr)
>>> +{
>>> +    struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
>>> +
>>> +    if (pp && pp->has_msi_ctrl)
>>> +        dw_pcie_free_msi(pp);
>>> +}
>>> +
>>> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>>> +{
>>> +    struct device *dev = cfg->parent;
>>> +    struct dw_pcie_rp *pp;
>>> +    struct dw_pcie *pci;
>>> +    int ret;
>>> +
>>> +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>>> +    if (!pci)
>>> +        return -ENOMEM;
>>> +
>>> +    pci->dev = dev;
>>> +    pp = &pci->pp;
>>> +    pci->dbi_base = cfg->win;
>>> +    pp->num_vectors = MSI_DEF_NUM_VECTORS;
>>> +
>>> +    ret = dw_pcie_msi_host_init(pp);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    pp->has_msi_ctrl = true;
>>> +    dw_pcie_msi_init(pp);
>>> +
>>> +    ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>>> +    return ret;
>>> +}
>>> +
>>> +/* ECAM ops */
>>> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
>>> +    .init        = qcom_pcie_ecam_host_init,
>>> +    .pci_ops    = {
>>> +        .map_bus    = pci_ecam_map_bus,
>>> +        .read        = pci_generic_config_read,
>>> +        .write        = pci_generic_config_write,
>>> +    }
>>> +};
>>> +
>>>   static int qcom_pcie_probe(struct platform_device *pdev)
>>>   {
>>>       const struct qcom_pcie_cfg *pcie_cfg;
>>> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>       char *name;
>>>       pcie_cfg = of_device_get_match_data(dev);
>>> -    if (!pcie_cfg || !pcie_cfg->ops) {
>>> -        dev_err(dev, "Invalid platform data\n");
>>> +    if (!pcie_cfg) {
>>> +        dev_err(dev, "No platform data\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
>>> +        dev_err(dev, "No platform ops\n");
>>>           return -EINVAL;
>>>       }
>>> +    pm_runtime_enable(dev);
>>> +    ret = pm_runtime_get_sync(dev);
>>> +    if (ret < 0)
>>> +        goto err_pm_runtime_put;
>>> +
>>> +    if (pcie_cfg->firmware_managed) {
>>> +        struct pci_host_bridge *bridge;
>>> +        struct pci_config_window *cfg;
>>> +
>>> +        bridge = devm_pci_alloc_host_bridge(dev, 0);
>>> +        if (!bridge) {
>>> +            ret = -ENOMEM;
>>> +            goto err_pm_runtime_put;
>>> +        }
>>> +
>>> +        of_pci_check_probe_only();
>>> +        /* Parse and map our Configuration Space windows */
>>> +        cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
>>> +        if (IS_ERR(cfg)) {
>>> +            ret = PTR_ERR(cfg);
>>> +            goto err_pm_runtime_put;
>>> +        }
>>> +
>>> +        bridge->sysdata = cfg;
>>> +        bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
>>> +        bridge->msi_domain = true;
>>> +
>>> +        ret = pci_host_probe(bridge);
>>> +        if (ret) {
>>> +            dev_err(dev, "pci_host_probe() failed:%d\n", ret);
>>> +            goto err_pm_runtime_put;
>>> +        }
>>> +
>>> +        return ret;
>>> +    }
>>> +
>>>       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>>>       if (!pcie)
>>>           return -ENOMEM;
>>> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>       if (!pci)
>>>           return -ENOMEM;
>>> -    pm_runtime_enable(dev);
>>> -    ret = pm_runtime_get_sync(dev);
>>> -    if (ret < 0)
>>> -        goto err_pm_runtime_put;
>>> -
>>>       pci->dev = dev;
>>>       pci->ops = &dw_pcie_ops;
>>>       pp = &pci->pp;
>>> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>   static int qcom_pcie_suspend_noirq(struct device *dev)
>>>   {
>>> -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>> +    struct qcom_pcie *pcie;
>>>       int ret = 0;
>>> +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
>>
>> Can't you use if (pcie_cfg->firmware_managed) here instead ?
> yes, although with firmware managed mode, struct qcom_pcie *pcie is not allocated, and just
> to get access to pcie_cfg for this check, I took this approach. I am thiking to do allocating struct qcom_pcie *pcie and using it in future if we need more other related functionality which needs usage of this structure for functionality like global interrupt etc.
> 
> Although if you still prefer to allocate struct qcom_pcie based memory to access pcie_cfg, then I can consider to update in next patchset. Please suggest.

I understand, but running of_device_is_compatible() in runtime PM is not something we should do,
so either allocate pcie_cfg, or add a firmware_managed bool to qcom_pcie copied from pcie_cfg,
or move runtime pm callbacks in qcom_pcie_ops and don't declare any in cfg_fw_managed->ops.

I think the latter would be more scalable so we could add runtime pm variant handling
for each IP versions. But it may be quite quite useless for now.

I'll leave Mani comment on that.

Neil


>>> +        return 0;
>>> +
>>> +    pcie = dev_get_drvdata(dev);
>>>       /*
>>>        * Set minimum bandwidth required to keep data path functional during
>>>        * suspend.
>>> @@ -1795,9 +1888,13 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>>>   static int qcom_pcie_resume_noirq(struct device *dev)
>>>   {
>>> -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>> +    struct qcom_pcie *pcie;
>>>       int ret;
>>> +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
>>
>> Ditto
>>
>>> +        return  0;
>>> +
>>> +    pcie = dev_get_drvdata(dev);
>>>       if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>>>           ret = icc_enable(pcie->icc_cpu);
>>>           if (ret) {
>>> @@ -1830,6 +1927,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>>       { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
>>>       { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>>>       { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>>> +    { .compatible = "qcom,pcie-sa8255p", .data = &cfg_fw_managed },
>>>       { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp },
>>>       { .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_34_0},
>>>       { .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
>>
>> Thanks,
>> Neil
> 
> Regards,
> Mayank

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Manivannan Sadhasivam]

On Fri, Nov 08, 2024 at 11:22:52AM +0100, neil.armstrong@linaro.org wrote:
> On 07/11/2024 18:45, Mayank Rana wrote:
> > 
> > 
> > On 11/7/2024 12:45 AM, neil.armstrong@linaro.org wrote:
> > > Hi,
> > > 
> > > On 06/11/2024 23:13, Mayank Rana wrote:
> > > > On SA8255p ride platform, PCIe root complex is firmware managed as well
> > > > configured into ECAM compliant mode. This change adds functionality to
> > > > enable resource management (system resource as well PCIe controller and
> > > > PHY configuration) through firmware, and enumerating ECAM compliant root
> > > > complex.
> > > > 
> > > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/Kconfig     |   1 +
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
> > > >   2 files changed, 108 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index b6d6778b0698..0fe76bd39d69 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -275,6 +275,7 @@ config PCIE_QCOM
> > > >       select PCIE_DW_HOST
> > > >       select CRC8
> > > >       select PCIE_QCOM_COMMON
> > > > +    select PCI_HOST_COMMON
> > > >       help
> > > >         Say Y here to enable PCIe controller support on Qualcomm SoCs. The
> > > >         PCIe controller uses the DesignWare core plus Qualcomm-specific
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index ef44a82be058..2cb74f902baf 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -21,7 +21,9 @@
> > > >   #include <linux/limits.h>
> > > >   #include <linux/init.h>
> > > >   #include <linux/of.h>
> > > > +#include <linux/of_pci.h>
> > > >   #include <linux/pci.h>
> > > > +#include <linux/pci-ecam.h>
> > > >   #include <linux/pm_opp.h>
> > > >   #include <linux/pm_runtime.h>
> > > >   #include <linux/platform_device.h>
> > > > @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
> > > >     * @ops: qcom PCIe ops structure
> > > >     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
> > > >     * snooping
> > > > +  * @firmware_managed: Set if PCIe root complex is firmware managed
> > > >     */
> > > >   struct qcom_pcie_cfg {
> > > >       const struct qcom_pcie_ops *ops;
> > > >       bool override_no_snoop;
> > > > +    bool firmware_managed;
> > > >       bool no_l0s;
> > > >   };
> > > > @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> > > >       .no_l0s = true,
> > > >   };
> > > > +static const struct qcom_pcie_cfg cfg_fw_managed = {
> > > > +    .firmware_managed = true,
> > > > +};
> > > > +
> > > >   static const struct dw_pcie_ops dw_pcie_ops = {
> > > >       .link_up = qcom_pcie_link_up,
> > > >       .start_link = qcom_pcie_start_link,
> > > > @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> > > >       return IRQ_HANDLED;
> > > >   }
> > > > +static void qcom_pci_free_msi(void *ptr)
> > > > +{
> > > > +    struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
> > > > +
> > > > +    if (pp && pp->has_msi_ctrl)
> > > > +        dw_pcie_free_msi(pp);
> > > > +}
> > > > +
> > > > +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
> > > > +{
> > > > +    struct device *dev = cfg->parent;
> > > > +    struct dw_pcie_rp *pp;
> > > > +    struct dw_pcie *pci;
> > > > +    int ret;
> > > > +
> > > > +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > +    if (!pci)
> > > > +        return -ENOMEM;
> > > > +
> > > > +    pci->dev = dev;
> > > > +    pp = &pci->pp;
> > > > +    pci->dbi_base = cfg->win;
> > > > +    pp->num_vectors = MSI_DEF_NUM_VECTORS;
> > > > +
> > > > +    ret = dw_pcie_msi_host_init(pp);
> > > > +    if (ret)
> > > > +        return ret;
> > > > +
> > > > +    pp->has_msi_ctrl = true;
> > > > +    dw_pcie_msi_init(pp);
> > > > +
> > > > +    ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +/* ECAM ops */
> > > > +const struct pci_ecam_ops pci_qcom_ecam_ops = {
> > > > +    .init        = qcom_pcie_ecam_host_init,
> > > > +    .pci_ops    = {
> > > > +        .map_bus    = pci_ecam_map_bus,
> > > > +        .read        = pci_generic_config_read,
> > > > +        .write        = pci_generic_config_write,
> > > > +    }
> > > > +};
> > > > +
> > > >   static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   {
> > > >       const struct qcom_pcie_cfg *pcie_cfg;
> > > > @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >       char *name;
> > > >       pcie_cfg = of_device_get_match_data(dev);
> > > > -    if (!pcie_cfg || !pcie_cfg->ops) {
> > > > -        dev_err(dev, "Invalid platform data\n");
> > > > +    if (!pcie_cfg) {
> > > > +        dev_err(dev, "No platform data\n");
> > > > +        return -EINVAL;
> > > > +    }
> > > > +
> > > > +    if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
> > > > +        dev_err(dev, "No platform ops\n");
> > > >           return -EINVAL;
> > > >       }
> > > > +    pm_runtime_enable(dev);
> > > > +    ret = pm_runtime_get_sync(dev);
> > > > +    if (ret < 0)
> > > > +        goto err_pm_runtime_put;
> > > > +
> > > > +    if (pcie_cfg->firmware_managed) {
> > > > +        struct pci_host_bridge *bridge;
> > > > +        struct pci_config_window *cfg;
> > > > +
> > > > +        bridge = devm_pci_alloc_host_bridge(dev, 0);
> > > > +        if (!bridge) {
> > > > +            ret = -ENOMEM;
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        of_pci_check_probe_only();
> > > > +        /* Parse and map our Configuration Space windows */
> > > > +        cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
> > > > +        if (IS_ERR(cfg)) {
> > > > +            ret = PTR_ERR(cfg);
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        bridge->sysdata = cfg;
> > > > +        bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
> > > > +        bridge->msi_domain = true;
> > > > +
> > > > +        ret = pci_host_probe(bridge);
> > > > +        if (ret) {
> > > > +            dev_err(dev, "pci_host_probe() failed:%d\n", ret);
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        return ret;
> > > > +    }
> > > > +
> > > >       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > >       if (!pcie)
> > > >           return -ENOMEM;
> > > > @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >       if (!pci)
> > > >           return -ENOMEM;
> > > > -    pm_runtime_enable(dev);
> > > > -    ret = pm_runtime_get_sync(dev);
> > > > -    if (ret < 0)
> > > > -        goto err_pm_runtime_put;
> > > > -
> > > >       pci->dev = dev;
> > > >       pci->ops = &dw_pcie_ops;
> > > >       pp = &pci->pp;
> > > > @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   static int qcom_pcie_suspend_noirq(struct device *dev)
> > > >   {
> > > > -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > +    struct qcom_pcie *pcie;
> > > >       int ret = 0;
> > > > +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
> > > 
> > > Can't you use if (pcie_cfg->firmware_managed) here instead ?
> > yes, although with firmware managed mode, struct qcom_pcie *pcie is not allocated, and just
> > to get access to pcie_cfg for this check, I took this approach. I am thiking to do allocating struct qcom_pcie *pcie and using it in future if we need more other related functionality which needs usage of this structure for functionality like global interrupt etc.
> > 
> > Although if you still prefer to allocate struct qcom_pcie based memory to access pcie_cfg, then I can consider to update in next patchset. Please suggest.
> 
> I understand, but running of_device_is_compatible() in runtime PM is not something we should do,
> so either allocate pcie_cfg, or add a firmware_managed bool to qcom_pcie copied from pcie_cfg,
> or move runtime pm callbacks in qcom_pcie_ops and don't declare any in cfg_fw_managed->ops.
> 
> I think the latter would be more scalable so we could add runtime pm variant handling
> for each IP versions. But it may be quite quite useless for now.
> 

Or just bail out if dev_get_drvdata() return NULL?

- Mani

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

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Mayank Rana]

On 11/15/2024 3:21 AM, Manivannan Sadhasivam wrote:
> On Fri, Nov 08, 2024 at 11:22:52AM +0100, neil.armstrong@linaro.org wrote:
>> On 07/11/2024 18:45, Mayank Rana wrote:
>>>
>>>
>>> On 11/7/2024 12:45 AM, neil.armstrong@linaro.org wrote:
>>>> Hi,
>>>>
>>>> On 06/11/2024 23:13, Mayank Rana wrote:
>>>>> On SA8255p ride platform, PCIe root complex is firmware managed as well
>>>>> configured into ECAM compliant mode. This change adds functionality to
>>>>> enable resource management (system resource as well PCIe controller and
>>>>> PHY configuration) through firmware, and enumerating ECAM compliant root
>>>>> complex.
>>>>>
>>>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>>>>> ---
>>>>>    drivers/pci/controller/dwc/Kconfig     |   1 +
>>>>>    drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>>>>>    2 files changed, 108 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>>>>> index b6d6778b0698..0fe76bd39d69 100644
>>>>> --- a/drivers/pci/controller/dwc/Kconfig
>>>>> +++ b/drivers/pci/controller/dwc/Kconfig
>>>>> @@ -275,6 +275,7 @@ config PCIE_QCOM
>>>>>        select PCIE_DW_HOST
>>>>>        select CRC8
>>>>>        select PCIE_QCOM_COMMON
>>>>> +    select PCI_HOST_COMMON
>>>>>        help
>>>>>          Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>>>>>          PCIe controller uses the DesignWare core plus Qualcomm-specific
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> index ef44a82be058..2cb74f902baf 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> @@ -21,7 +21,9 @@
>>>>>    #include <linux/limits.h>
>>>>>    #include <linux/init.h>
>>>>>    #include <linux/of.h>
>>>>> +#include <linux/of_pci.h>
>>>>>    #include <linux/pci.h>
>>>>> +#include <linux/pci-ecam.h>
>>>>>    #include <linux/pm_opp.h>
>>>>>    #include <linux/pm_runtime.h>
>>>>>    #include <linux/platform_device.h>
>>>>> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>>>>>      * @ops: qcom PCIe ops structure
>>>>>      * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
>>>>>      * snooping
>>>>> +  * @firmware_managed: Set if PCIe root complex is firmware managed
>>>>>      */
>>>>>    struct qcom_pcie_cfg {
>>>>>        const struct qcom_pcie_ops *ops;
>>>>>        bool override_no_snoop;
>>>>> +    bool firmware_managed;
>>>>>        bool no_l0s;
>>>>>    };
>>>>> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>>>>>        .no_l0s = true,
>>>>>    };
>>>>> +static const struct qcom_pcie_cfg cfg_fw_managed = {
>>>>> +    .firmware_managed = true,
>>>>> +};
>>>>> +
>>>>>    static const struct dw_pcie_ops dw_pcie_ops = {
>>>>>        .link_up = qcom_pcie_link_up,
>>>>>        .start_link = qcom_pcie_start_link,
>>>>> @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>>>>        return IRQ_HANDLED;
>>>>>    }
>>>>> +static void qcom_pci_free_msi(void *ptr)
>>>>> +{
>>>>> +    struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
>>>>> +
>>>>> +    if (pp && pp->has_msi_ctrl)
>>>>> +        dw_pcie_free_msi(pp);
>>>>> +}
>>>>> +
>>>>> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>>>>> +{
>>>>> +    struct device *dev = cfg->parent;
>>>>> +    struct dw_pcie_rp *pp;
>>>>> +    struct dw_pcie *pci;
>>>>> +    int ret;
>>>>> +
>>>>> +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>>>>> +    if (!pci)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    pci->dev = dev;
>>>>> +    pp = &pci->pp;
>>>>> +    pci->dbi_base = cfg->win;
>>>>> +    pp->num_vectors = MSI_DEF_NUM_VECTORS;
>>>>> +
>>>>> +    ret = dw_pcie_msi_host_init(pp);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    pp->has_msi_ctrl = true;
>>>>> +    dw_pcie_msi_init(pp);
>>>>> +
>>>>> +    ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +/* ECAM ops */
>>>>> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
>>>>> +    .init        = qcom_pcie_ecam_host_init,
>>>>> +    .pci_ops    = {
>>>>> +        .map_bus    = pci_ecam_map_bus,
>>>>> +        .read        = pci_generic_config_read,
>>>>> +        .write        = pci_generic_config_write,
>>>>> +    }
>>>>> +};
>>>>> +
>>>>>    static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>    {
>>>>>        const struct qcom_pcie_cfg *pcie_cfg;
>>>>> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>        char *name;
>>>>>        pcie_cfg = of_device_get_match_data(dev);
>>>>> -    if (!pcie_cfg || !pcie_cfg->ops) {
>>>>> -        dev_err(dev, "Invalid platform data\n");
>>>>> +    if (!pcie_cfg) {
>>>>> +        dev_err(dev, "No platform data\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
>>>>> +        dev_err(dev, "No platform ops\n");
>>>>>            return -EINVAL;
>>>>>        }
>>>>> +    pm_runtime_enable(dev);
>>>>> +    ret = pm_runtime_get_sync(dev);
>>>>> +    if (ret < 0)
>>>>> +        goto err_pm_runtime_put;
>>>>> +
>>>>> +    if (pcie_cfg->firmware_managed) {
>>>>> +        struct pci_host_bridge *bridge;
>>>>> +        struct pci_config_window *cfg;
>>>>> +
>>>>> +        bridge = devm_pci_alloc_host_bridge(dev, 0);
>>>>> +        if (!bridge) {
>>>>> +            ret = -ENOMEM;
>>>>> +            goto err_pm_runtime_put;
>>>>> +        }
>>>>> +
>>>>> +        of_pci_check_probe_only();
>>>>> +        /* Parse and map our Configuration Space windows */
>>>>> +        cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
>>>>> +        if (IS_ERR(cfg)) {
>>>>> +            ret = PTR_ERR(cfg);
>>>>> +            goto err_pm_runtime_put;
>>>>> +        }
>>>>> +
>>>>> +        bridge->sysdata = cfg;
>>>>> +        bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
>>>>> +        bridge->msi_domain = true;
>>>>> +
>>>>> +        ret = pci_host_probe(bridge);
>>>>> +        if (ret) {
>>>>> +            dev_err(dev, "pci_host_probe() failed:%d\n", ret);
>>>>> +            goto err_pm_runtime_put;
>>>>> +        }
>>>>> +
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>>        pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>>>>>        if (!pcie)
>>>>>            return -ENOMEM;
>>>>> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>        if (!pci)
>>>>>            return -ENOMEM;
>>>>> -    pm_runtime_enable(dev);
>>>>> -    ret = pm_runtime_get_sync(dev);
>>>>> -    if (ret < 0)
>>>>> -        goto err_pm_runtime_put;
>>>>> -
>>>>>        pci->dev = dev;
>>>>>        pci->ops = &dw_pcie_ops;
>>>>>        pp = &pci->pp;
>>>>> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>>>>    static int qcom_pcie_suspend_noirq(struct device *dev)
>>>>>    {
>>>>> -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>>>> +    struct qcom_pcie *pcie;
>>>>>        int ret = 0;
>>>>> +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
>>>>
>>>> Can't you use if (pcie_cfg->firmware_managed) here instead ?
>>> yes, although with firmware managed mode, struct qcom_pcie *pcie is not allocated, and just
>>> to get access to pcie_cfg for this check, I took this approach. I am thiking to do allocating struct qcom_pcie *pcie and using it in future if we need more other related functionality which needs usage of this structure for functionality like global interrupt etc.
>>>
>>> Although if you still prefer to allocate struct qcom_pcie based memory to access pcie_cfg, then I can consider to update in next patchset. Please suggest.
>>
>> I understand, but running of_device_is_compatible() in runtime PM is not something we should do,
>> so either allocate pcie_cfg, or add a firmware_managed bool to qcom_pcie copied from pcie_cfg,
>> or move runtime pm callbacks in qcom_pcie_ops and don't declare any in cfg_fw_managed->ops.
>>
>> I think the latter would be more scalable so we could add runtime pm variant handling
>> for each IP versions. But it may be quite quite useless for now.
>>
> 
> Or just bail out if dev_get_drvdata() return NULL?
Agree. This would also work for current requirement.

Regards,
Mayank
> 
> - Mani
> 

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[kernel test robot]

Hi Mayank,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus mani-mhi/mhi-next robh/for-next linus/master v6.12-rc6 next-20241108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mayank-Rana/PCI-dwc-Export-dwc-MSI-controller-related-APIs/20241107-061634
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241106221341.2218416-5-quic_mrana%40quicinc.com
patch subject: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality
config: powerpc-randconfig-r131-20241109 (https://download.01.org/0day-ci/archive/20241110/202411100738.kCgjRkIv-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce: (https://download.01.org/0day-ci/archive/20241110/202411100738.kCgjRkIv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411100738.kCgjRkIv-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pci/controller/dwc/pcie-qcom.c:1613:27: sparse: sparse: symbol 'pci_qcom_ecam_ops' was not declared. Should it be static?

vim +/pci_qcom_ecam_ops +1613 drivers/pci/controller/dwc/pcie-qcom.c

  1611	
  1612	/* ECAM ops */
> 1613	const struct pci_ecam_ops pci_qcom_ecam_ops = {
  1614		.init		= qcom_pcie_ecam_host_init,
  1615		.pci_ops	= {
  1616			.map_bus	= pci_ecam_map_bus,
  1617			.read		= pci_generic_config_read,
  1618			.write		= pci_generic_config_write,
  1619		}
  1620	};
  1621	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 02:13:41PM -0800, Mayank Rana wrote:
> On SA8255p ride platform, PCIe root complex is firmware managed as well
> configured into ECAM compliant mode. This change adds functionality to
> enable resource management (system resource as well PCIe controller and
> PHY configuration) through firmware, and enumerating ECAM compliant root
> complex.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/Kconfig     |   1 +
>  drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>  2 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b6d6778b0698..0fe76bd39d69 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -275,6 +275,7 @@ config PCIE_QCOM
>  	select PCIE_DW_HOST
>  	select CRC8
>  	select PCIE_QCOM_COMMON
> +	select PCI_HOST_COMMON
>  	help
>  	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ef44a82be058..2cb74f902baf 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -21,7 +21,9 @@
>  #include <linux/limits.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
> +#include <linux/pci-ecam.h>
>  #include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/platform_device.h>
> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>    * @ops: qcom PCIe ops structure
>    * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
>    * snooping
> +  * @firmware_managed: Set if PCIe root complex is firmware managed

ecam_compliant?

>    */
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
>  	bool override_no_snoop;
> +	bool firmware_managed;
>  	bool no_l0s;
>  };
>  
> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>  	.no_l0s = true,
>  };
>  
> +static const struct qcom_pcie_cfg cfg_fw_managed = {

cfg_ecam?

> +	.firmware_managed = true,
> +};
> +
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void qcom_pci_free_msi(void *ptr)
> +{
> +	struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
> +
> +	if (pp && pp->has_msi_ctrl)
> +		dw_pcie_free_msi(pp);
> +}
> +
> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
> +{
> +	struct device *dev = cfg->parent;
> +	struct dw_pcie_rp *pp;
> +	struct dw_pcie *pci;
> +	int ret;
> +
> +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +	if (!pci)
> +		return -ENOMEM;
> +
> +	pci->dev = dev;
> +	pp = &pci->pp;
> +	pci->dbi_base = cfg->win;
> +	pp->num_vectors = MSI_DEF_NUM_VECTORS;
> +
> +	ret = dw_pcie_msi_host_init(pp);
> +	if (ret)
> +		return ret;
> +
> +	pp->has_msi_ctrl = true;
> +	dw_pcie_msi_init(pp);
> +
> +	ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);

return devm_add_action_or_reset()...

> +	return ret;
> +}
> +
> +/* ECAM ops */
> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
> +	.init		= qcom_pcie_ecam_host_init,
> +	.pci_ops	= {
> +		.map_bus	= pci_ecam_map_bus,
> +		.read		= pci_generic_config_read,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +
>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	const struct qcom_pcie_cfg *pcie_cfg;
> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	char *name;
>  
>  	pcie_cfg = of_device_get_match_data(dev);
> -	if (!pcie_cfg || !pcie_cfg->ops) {
> -		dev_err(dev, "Invalid platform data\n");
> +	if (!pcie_cfg) {
> +		dev_err(dev, "No platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
> +		dev_err(dev, "No platform ops\n");
>  		return -EINVAL;
>  	}
>  
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0)
> +		goto err_pm_runtime_put;
> +
> +	if (pcie_cfg->firmware_managed) {
> +		struct pci_host_bridge *bridge;
> +		struct pci_config_window *cfg;
> +
> +		bridge = devm_pci_alloc_host_bridge(dev, 0);
> +		if (!bridge) {
> +			ret = -ENOMEM;
> +			goto err_pm_runtime_put;
> +		}
> +
> +		of_pci_check_probe_only();

You haven't defined the "linux,pci-probe-only" property in DT. So if everything
works fine, then you can leave this call.

> +		/* Parse and map our Configuration Space windows */
> +		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
> +		if (IS_ERR(cfg)) {
> +			ret = PTR_ERR(cfg);
> +			goto err_pm_runtime_put;
> +		}
> +
> +		bridge->sysdata = cfg;
> +		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
> +		bridge->msi_domain = true;
> +
> +		ret = pci_host_probe(bridge);

return pci_host_probe()...

> +		if (ret) {
> +			dev_err(dev, "pci_host_probe() failed:%d\n", ret);
> +			goto err_pm_runtime_put;
> +		}
> +
> +		return ret;
> +	}
> +
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
>  		return -ENOMEM;
> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (!pci)
>  		return -ENOMEM;
>  
> -	pm_runtime_enable(dev);
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> -		goto err_pm_runtime_put;
> -
>  	pci->dev = dev;
>  	pci->ops = &dw_pcie_ops;
>  	pp = &pci->pp;
> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  static int qcom_pcie_suspend_noirq(struct device *dev)
>  {
> -	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	struct qcom_pcie *pcie;
>  	int ret = 0;
>  
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
> +		return 0;

How about bailing out if dev_get_drvdata() returns NULL?

- Mani

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

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Mayank Rana]

On 11/15/2024 3:25 AM, Manivannan Sadhasivam wrote:
> On Wed, Nov 06, 2024 at 02:13:41PM -0800, Mayank Rana wrote:
>> On SA8255p ride platform, PCIe root complex is firmware managed as well
>> configured into ECAM compliant mode. This change adds functionality to
>> enable resource management (system resource as well PCIe controller and
>> PHY configuration) through firmware, and enumerating ECAM compliant root
>> complex.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/Kconfig     |   1 +
>>   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
>>   2 files changed, 108 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index b6d6778b0698..0fe76bd39d69 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -275,6 +275,7 @@ config PCIE_QCOM
>>   	select PCIE_DW_HOST
>>   	select CRC8
>>   	select PCIE_QCOM_COMMON
>> +	select PCI_HOST_COMMON
>>   	help
>>   	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>>   	  PCIe controller uses the DesignWare core plus Qualcomm-specific
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index ef44a82be058..2cb74f902baf 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -21,7 +21,9 @@
>>   #include <linux/limits.h>
>>   #include <linux/init.h>
>>   #include <linux/of.h>
>> +#include <linux/of_pci.h>
>>   #include <linux/pci.h>
>> +#include <linux/pci-ecam.h>
>>   #include <linux/pm_opp.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/platform_device.h>
>> @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
>>     * @ops: qcom PCIe ops structure
>>     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
>>     * snooping
>> +  * @firmware_managed: Set if PCIe root complex is firmware managed
> 
> ecam_compliant?
I assume you mean to update as Set if ECAM compliant PCIe root complex 
is firmware manage
>>     */
>>   struct qcom_pcie_cfg {
>>   	const struct qcom_pcie_ops *ops;
>>   	bool override_no_snoop;
>> +	bool firmware_managed;
>>   	bool no_l0s;
>>   };
>>   
>> @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>>   	.no_l0s = true,
>>   };
>>   
>> +static const struct qcom_pcie_cfg cfg_fw_managed = {
> 
> cfg_ecam?
Putting more emphasize on fw_managed as don't want to conflict with new 
work in progress (krishna is working on it)
to make Qualcomm PCIe root complex as ECAM compliant (non firmware 
managed one). are you OK with using cfg_ecam_fw_managed ?

>> +	.firmware_managed = true,
>> +};
>> +
>>   static const struct dw_pcie_ops dw_pcie_ops = {
>>   	.link_up = qcom_pcie_link_up,
>>   	.start_link = qcom_pcie_start_link,
>> @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static void qcom_pci_free_msi(void *ptr)
>> +{
>> +	struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
>> +
>> +	if (pp && pp->has_msi_ctrl)
>> +		dw_pcie_free_msi(pp);
>> +}
>> +
>> +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
>> +{
>> +	struct device *dev = cfg->parent;
>> +	struct dw_pcie_rp *pp;
>> +	struct dw_pcie *pci;
>> +	int ret;
>> +
>> +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>> +	if (!pci)
>> +		return -ENOMEM;
>> +
>> +	pci->dev = dev;
>> +	pp = &pci->pp;
>> +	pci->dbi_base = cfg->win;
>> +	pp->num_vectors = MSI_DEF_NUM_VECTORS;
>> +
>> +	ret = dw_pcie_msi_host_init(pp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pp->has_msi_ctrl = true;
>> +	dw_pcie_msi_init(pp);
>> +
>> +	ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
> 
> return devm_add_action_or_reset()...
Done.

>> +	return ret;
>> +}
>> +
>> +/* ECAM ops */
>> +const struct pci_ecam_ops pci_qcom_ecam_ops = {
>> +	.init		= qcom_pcie_ecam_host_init,
>> +	.pci_ops	= {
>> +		.map_bus	= pci_ecam_map_bus,
>> +		.read		= pci_generic_config_read,
>> +		.write		= pci_generic_config_write,
>> +	}
>> +};
>> +
>>   static int qcom_pcie_probe(struct platform_device *pdev)
>>   {
>>   	const struct qcom_pcie_cfg *pcie_cfg;
>> @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	char *name;
>>   
>>   	pcie_cfg = of_device_get_match_data(dev);
>> -	if (!pcie_cfg || !pcie_cfg->ops) {
>> -		dev_err(dev, "Invalid platform data\n");
>> +	if (!pcie_cfg) {
>> +		dev_err(dev, "No platform data\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
>> +		dev_err(dev, "No platform ops\n");
>>   		return -EINVAL;
>>   	}
>>   
>> +	pm_runtime_enable(dev);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0)
>> +		goto err_pm_runtime_put;
>> +
>> +	if (pcie_cfg->firmware_managed) {
>> +		struct pci_host_bridge *bridge;
>> +		struct pci_config_window *cfg;
>> +
>> +		bridge = devm_pci_alloc_host_bridge(dev, 0);
>> +		if (!bridge) {
>> +			ret = -ENOMEM;
>> +			goto err_pm_runtime_put;
>> +		}
>> +
>> +		of_pci_check_probe_only();
> 
> You haven't defined the "linux,pci-probe-only" property in DT. So if everything
> works fine, then you can leave this call.
ok will review more and update accordingly.

>> +		/* Parse and map our Configuration Space windows */
>> +		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
>> +		if (IS_ERR(cfg)) {
>> +			ret = PTR_ERR(cfg);
>> +			goto err_pm_runtime_put;
>> +		}
>> +
>> +		bridge->sysdata = cfg;
>> +		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
>> +		bridge->msi_domain = true;
>> +
>> +		ret = pci_host_probe(bridge);
> 
> return pci_host_probe()...
need to perform pm_runtime_put_sync() and pm_runtime_disable() in 
failure case.
Hence checking error here and doing goto err_pm_runtime_put
>> +		if (ret) {
>> +			dev_err(dev, "pci_host_probe() failed:%d\n", ret);
>> +			goto err_pm_runtime_put;
>> +		}
>> +
>> +		return ret;
>> +	}
>> +
>>   	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>>   	if (!pcie)
>>   		return -ENOMEM;
>> @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	if (!pci)
>>   		return -ENOMEM;
>>   
>> -	pm_runtime_enable(dev);
>> -	ret = pm_runtime_get_sync(dev);
>> -	if (ret < 0)
>> -		goto err_pm_runtime_put;
>> -
>>   	pci->dev = dev;
>>   	pci->ops = &dw_pcie_ops;
>>   	pp = &pci->pp;
>> @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   
>>   static int qcom_pcie_suspend_noirq(struct device *dev)
>>   {
>> -	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +	struct qcom_pcie *pcie;
>>   	int ret = 0;
>>   
>> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
>> +		return 0;
> 
> How about bailing out if dev_get_drvdata() returns NULL?
Done

Regards,
Mayank
> - Mani
> 

Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Manivannan Sadhasivam]

On Fri, Nov 15, 2024 at 10:28:23AM -0800, Mayank Rana wrote:
> 
> 
> On 11/15/2024 3:25 AM, Manivannan Sadhasivam wrote:
> > On Wed, Nov 06, 2024 at 02:13:41PM -0800, Mayank Rana wrote:
> > > On SA8255p ride platform, PCIe root complex is firmware managed as well
> > > configured into ECAM compliant mode. This change adds functionality to
> > > enable resource management (system resource as well PCIe controller and
> > > PHY configuration) through firmware, and enumerating ECAM compliant root
> > > complex.
> > > 
> > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > ---
> > >   drivers/pci/controller/dwc/Kconfig     |   1 +
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
> > >   2 files changed, 108 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index b6d6778b0698..0fe76bd39d69 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -275,6 +275,7 @@ config PCIE_QCOM
> > >   	select PCIE_DW_HOST
> > >   	select CRC8
> > >   	select PCIE_QCOM_COMMON
> > > +	select PCI_HOST_COMMON
> > >   	help
> > >   	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
> > >   	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index ef44a82be058..2cb74f902baf 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -21,7 +21,9 @@
> > >   #include <linux/limits.h>
> > >   #include <linux/init.h>
> > >   #include <linux/of.h>
> > > +#include <linux/of_pci.h>
> > >   #include <linux/pci.h>
> > > +#include <linux/pci-ecam.h>
> > >   #include <linux/pm_opp.h>
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/platform_device.h>
> > > @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
> > >     * @ops: qcom PCIe ops structure
> > >     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
> > >     * snooping
> > > +  * @firmware_managed: Set if PCIe root complex is firmware managed
> > 
> > ecam_compliant?
> I assume you mean to update as Set if ECAM compliant PCIe root complex is
> firmware manage
> > >     */
> > >   struct qcom_pcie_cfg {
> > >   	const struct qcom_pcie_ops *ops;
> > >   	bool override_no_snoop;
> > > +	bool firmware_managed;
> > >   	bool no_l0s;
> > >   };
> > > @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> > >   	.no_l0s = true,
> > >   };
> > > +static const struct qcom_pcie_cfg cfg_fw_managed = {
> > 
> > cfg_ecam?
> Putting more emphasize on fw_managed as don't want to conflict with new work
> in progress (krishna is working on it)
> to make Qualcomm PCIe root complex as ECAM compliant (non firmware managed
> one). are you OK with using cfg_ecam_fw_managed ?
> 

Ah, I completely missed that. Ignore my comments about renaming to ecam.

> > > +	.firmware_managed = true,
> > > +};
> > > +

[...]

> > > +		/* Parse and map our Configuration Space windows */
> > > +		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
> > > +		if (IS_ERR(cfg)) {
> > > +			ret = PTR_ERR(cfg);
> > > +			goto err_pm_runtime_put;
> > > +		}
> > > +
> > > +		bridge->sysdata = cfg;
> > > +		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
> > > +		bridge->msi_domain = true;
> > > +
> > > +		ret = pci_host_probe(bridge);
> > 
> > return pci_host_probe()...
> need to perform pm_runtime_put_sync() and pm_runtime_disable() in failure
> case.
> Hence checking error here and doing goto err_pm_runtime_put

Right. This one I overlooked.

- Mani

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

Re: [PATCH v3 0/4] Add Qualcomm SA8255p based firmware managed PCIe root complex

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 02:13:37PM -0800, Mayank Rana wrote:
> Based on received feedback, this patch series adds support with existing
> Linux qcom-pcie.c driver to get PCIe host root complex functionality on
> Qualcomm SA8255P auto platform.
> 
> 1. Interface to allow requesting firmware to manage system resources and
> performing PCIe Link up (devicetree binding in terms of power domain and
> runtime PM APIs is used in driver)
> 
> 2. SA8255P is using Synopsys Designware PCIe controller which supports MSI
> controller. Using existing MSI controller based functionality by exporting
> important pcie dwc core driver based MSI APIs, and using those from
> pcie-qcom.c driver.
> 
> Below architecture is used on Qualcomm SA8255P auto platform to get ECAM
> compliant PCIe controller based functionality. Here firmware VM based PCIe
> driver takes care of resource management and performing PCIe link related
> handling (D0 and D3cold). Linux pcie-qcom.c driver uses power domain to
> request firmware VM to perform these operations using SCMI interface.
> --------------------
> 
> 
>                                    ┌────────────────────────┐                                               
>                                    │                        │                                               
>   ┌──────────────────────┐         │     SHARED MEMORY      │            ┌──────────────────────────┐       
>   │     Firmware VM      │         │                        │            │         Linux VM         │       
>   │ ┌─────────┐          │         │                        │            │    ┌────────────────┐    │       
>   │ │ Drivers │ ┌──────┐ │         │                        │            │    │   PCIE Qcom    │    │       
>   │ │ PCIE PHY◄─┤      │ │         │   ┌────────────────┐   │            │    │    driver      │    │       
>   │ │         │ │ SCMI │ │         │   │                │   │            │    │                │    │       
>   │ │PCIE CTL │ │      │ ├─────────┼───►    PCIE        ◄───┼─────┐      │    └──┬──────────▲──┘    │       
>   │ │         ├─►Server│ │         │   │    SHMEM       │   │     │      │       │          │       │       
>   │ │Clk, Vreg│ │      │ │         │   │                │   │     │      │    ┌──▼──────────┴──┐    │       
>   │ │GPIO,GDSC│ └─▲──┬─┘ │         │   └────────────────┘   │     └──────┼────┤PCIE SCMI Inst  │    │       
>   │ └─────────┘   │  │   │         │                        │            │    └──▲──────────┬──┘    │       
>   │               │  │   │         │                        │            │       │          │       │       
>   └───────────────┼──┼───┘         │                        │            └───────┼──────────┼───────┘       
>                   │  │             │                        │                    │          │               
>                   │  │             └────────────────────────┘                    │          │               
>                   │  │                                                           │          │               
>                   │  │                                                           │          │               
>                   │  │                                                           │          │               
>                   │  │                                                           │IRQ       │HVC            
>               IRQ │  │HVC                                                        │          │               
>                   │  │                                                           │          │               
>                   │  │                                                           │          │               
>                   │  │                                                           │          │               
> ┌─────────────────┴──▼───────────────────────────────────────────────────────────┴──────────▼──────────────┐
> │                                                                                                          │
> │                                                                                                          │
> │                                      HYPERVISOR                                                          │
> │                                                                                                          │
> │                                                                                                          │
> │                                                                                                          │
> └──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
>                                                                                                             
>   ┌─────────────┐    ┌─────────────┐  ┌──────────┐   ┌───────────┐   ┌─────────────┐  ┌────────────┐        
>   │             │    │             │  │          │   │           │   │  PCIE       │  │   PCIE     │        
>   │   CLOCK     │    │   REGULATOR │  │   GPIO   │   │   GDSC    │   │  PHY        │  │ controller │        
>   └─────────────┘    └─────────────┘  └──────────┘   └───────────┘   └─────────────┘  └────────────┘        
>                                                                                                             

Thanks a lot for working on this Mayank! This version looks good to me. I've
left some comments, nothing alarming though.

But I do want to hold up this series until we finalize the SCMI based design.

- Mani

> ----------
> Changes in V3:
> - Drop usage of PCIE host generic driver usage, and splitting of MSI functionality
> - Modified existing pcie-qcom.c driver to add support for getting ECAM compliant and firmware managed
> PCIe root complex functionality
> Link to v2: https://lore.kernel.org/linux-arm-kernel/925d1eca-975f-4eec-bdf8-ca07a892361a@quicinc.com/T/
> 
> Changes in V2:
> - Drop new PCIe Qcom ECAM driver, and use existing PCIe designware based MSI functionality
> - Add power domain based functionality within existing ECAM driver
> Link to v1: https://lore.kernel.org/all/d10199df-5fb3-407b-b404-a0a4d067341f@quicinc.com/T/                                                                                                      
> 
> Tested:
> - Validated NVME functionality with PCIe0 on SA8255P-RIDE platform
> 
> Mayank Rana (3):
>   PCI: dwc: Export dwc MSI controller related APIs
>   PCI: qcom: Add firmware managed ECAM compliant PCIe root complex
>     functionality
>   dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root
>     complex
> 
>  .../devicetree/bindings/pci/qcom,pcie-sa8255p.yaml | 100 +++++++++++++++++++++
>  drivers/pci/controller/dwc/Kconfig                 |   1 +
>  drivers/pci/controller/dwc/pcie-designware-host.c  |  38 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h       |  14 +++
>  drivers/pci/controller/dwc/pcie-qcom.c             |  69 ++++++++++++--
>  5 files changed, 199 insertions(+), 23 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
> 
> -- 
> 2.7.4
> 

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

Re: [PATCH v3 0/4] Add Qualcomm SA8255p based firmware managed PCIe root complex

[Mayank Rana]

On 11/15/2024 3:28 AM, Manivannan Sadhasivam wrote:
> On Wed, Nov 06, 2024 at 02:13:37PM -0800, Mayank Rana wrote:
>> Based on received feedback, this patch series adds support with existing
>> Linux qcom-pcie.c driver to get PCIe host root complex functionality on
>> Qualcomm SA8255P auto platform.
>>
>> 1. Interface to allow requesting firmware to manage system resources and
>> performing PCIe Link up (devicetree binding in terms of power domain and
>> runtime PM APIs is used in driver)
>>
>> 2. SA8255P is using Synopsys Designware PCIe controller which supports MSI
>> controller. Using existing MSI controller based functionality by exporting
>> important pcie dwc core driver based MSI APIs, and using those from
>> pcie-qcom.c driver.
>>
>> Below architecture is used on Qualcomm SA8255P auto platform to get ECAM
>> compliant PCIe controller based functionality. Here firmware VM based PCIe
>> driver takes care of resource management and performing PCIe link related
>> handling (D0 and D3cold). Linux pcie-qcom.c driver uses power domain to
>> request firmware VM to perform these operations using SCMI interface.
>> --------------------
>>
>>
>>                                     ┌────────────────────────┐
>>                                     │                        │
>>    ┌──────────────────────┐         │     SHARED MEMORY      │            ┌──────────────────────────┐
>>    │     Firmware VM      │         │                        │            │         Linux VM         │
>>    │ ┌─────────┐          │         │                        │            │    ┌────────────────┐    │
>>    │ │ Drivers │ ┌──────┐ │         │                        │            │    │   PCIE Qcom    │    │
>>    │ │ PCIE PHY◄─┤      │ │         │   ┌────────────────┐   │            │    │    driver      │    │
>>    │ │         │ │ SCMI │ │         │   │                │   │            │    │                │    │
>>    │ │PCIE CTL │ │      │ ├─────────┼───►    PCIE        ◄───┼─────┐      │    └──┬──────────▲──┘    │
>>    │ │         ├─►Server│ │         │   │    SHMEM       │   │     │      │       │          │       │
>>    │ │Clk, Vreg│ │      │ │         │   │                │   │     │      │    ┌──▼──────────┴──┐    │
>>    │ │GPIO,GDSC│ └─▲──┬─┘ │         │   └────────────────┘   │     └──────┼────┤PCIE SCMI Inst  │    │
>>    │ └─────────┘   │  │   │         │                        │            │    └──▲──────────┬──┘    │
>>    │               │  │   │         │                        │            │       │          │       │
>>    └───────────────┼──┼───┘         │                        │            └───────┼──────────┼───────┘
>>                    │  │             │                        │                    │          │
>>                    │  │             └────────────────────────┘                    │          │
>>                    │  │                                                           │          │
>>                    │  │                                                           │          │
>>                    │  │                                                           │          │
>>                    │  │                                                           │IRQ       │HVC
>>                IRQ │  │HVC                                                        │          │
>>                    │  │                                                           │          │
>>                    │  │                                                           │          │
>>                    │  │                                                           │          │
>> ┌─────────────────┴──▼───────────────────────────────────────────────────────────┴──────────▼──────────────┐
>> │                                                                                                          │
>> │                                                                                                          │
>> │                                      HYPERVISOR                                                          │
>> │                                                                                                          │
>> │                                                                                                          │
>> │                                                                                                          │
>> └──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
>>                                                                                                              
>>    ┌─────────────┐    ┌─────────────┐  ┌──────────┐   ┌───────────┐   ┌─────────────┐  ┌────────────┐
>>    │             │    │             │  │          │   │           │   │  PCIE       │  │   PCIE     │
>>    │   CLOCK     │    │   REGULATOR │  │   GPIO   │   │   GDSC    │   │  PHY        │  │ controller │
>>    └─────────────┘    └─────────────┘  └──────────┘   └───────────┘   └─────────────┘  └────────────┘
>>                                                                                                              
> 
> Thanks a lot for working on this Mayank! This version looks good to me. I've
> left some comments, nothing alarming though.
Thanks for reviewing change. I would address those in next patchset.

> But I do want to hold up this series until we finalize the SCMI based design.
ok. I want to send these changes which are prepared based on previously 
provided feedback, to see if we have any major concern here in terms of 
getting functionality.

Regards,
Mayank
> - Mani
> 
>> ----------
>> Changes in V3:
>> - Drop usage of PCIE host generic driver usage, and splitting of MSI functionality
>> - Modified existing pcie-qcom.c driver to add support for getting ECAM compliant and firmware managed
>> PCIe root complex functionality
>> Link to v2: https://lore.kernel.org/linux-arm-kernel/925d1eca-975f-4eec-bdf8-ca07a892361a@quicinc.com/T/
>>
>> Changes in V2:
>> - Drop new PCIe Qcom ECAM driver, and use existing PCIe designware based MSI functionality
>> - Add power domain based functionality within existing ECAM driver
>> Link to v1: https://lore.kernel.org/all/d10199df-5fb3-407b-b404-a0a4d067341f@quicinc.com/T/
>>
>> Tested:
>> - Validated NVME functionality with PCIe0 on SA8255P-RIDE platform
>>
>> Mayank Rana (3):
>>    PCI: dwc: Export dwc MSI controller related APIs
>>    PCI: qcom: Add firmware managed ECAM compliant PCIe root complex
>>      functionality
>>    dt-bindings: PCI: qcom,pcie-sa8255p: Document ECAM compliant PCIe root
>>      complex
>>
>>   .../devicetree/bindings/pci/qcom,pcie-sa8255p.yaml | 100 +++++++++++++++++++++
>>   drivers/pci/controller/dwc/Kconfig                 |   1 +
>>   drivers/pci/controller/dwc/pcie-designware-host.c  |  38 ++++----
>>   drivers/pci/controller/dwc/pcie-designware.h       |  14 +++
>>   drivers/pci/controller/dwc/pcie-qcom.c             |  69 ++++++++++++--
>>   5 files changed, 199 insertions(+), 23 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-sa8255p.yaml
>>
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH v3 0/4] Add Qualcomm SA8255p based firmware managed PCIe root complex

[Manivannan Sadhasivam]

On Fri, Nov 15, 2024 at 10:31:17AM -0800, Mayank Rana wrote:
> 
> 
> On 11/15/2024 3:28 AM, Manivannan Sadhasivam wrote:
> > On Wed, Nov 06, 2024 at 02:13:37PM -0800, Mayank Rana wrote:
> > > Based on received feedback, this patch series adds support with existing
> > > Linux qcom-pcie.c driver to get PCIe host root complex functionality on
> > > Qualcomm SA8255P auto platform.
> > > 
> > > 1. Interface to allow requesting firmware to manage system resources and
> > > performing PCIe Link up (devicetree binding in terms of power domain and
> > > runtime PM APIs is used in driver)
> > > 
> > > 2. SA8255P is using Synopsys Designware PCIe controller which supports MSI
> > > controller. Using existing MSI controller based functionality by exporting
> > > important pcie dwc core driver based MSI APIs, and using those from
> > > pcie-qcom.c driver.
> > > 
> > > Below architecture is used on Qualcomm SA8255P auto platform to get ECAM
> > > compliant PCIe controller based functionality. Here firmware VM based PCIe
> > > driver takes care of resource management and performing PCIe link related
> > > handling (D0 and D3cold). Linux pcie-qcom.c driver uses power domain to
> > > request firmware VM to perform these operations using SCMI interface.
> > > --------------------
> > > 
> > > 
> > >                                     ┌────────────────────────┐
> > >                                     │                        │
> > >    ┌──────────────────────┐         │     SHARED MEMORY      │            ┌──────────────────────────┐
> > >    │     Firmware VM      │         │                        │            │         Linux VM         │
> > >    │ ┌─────────┐          │         │                        │            │    ┌────────────────┐    │
> > >    │ │ Drivers │ ┌──────┐ │         │                        │            │    │   PCIE Qcom    │    │
> > >    │ │ PCIE PHY◄─┤      │ │         │   ┌────────────────┐   │            │    │    driver      │    │
> > >    │ │         │ │ SCMI │ │         │   │                │   │            │    │                │    │
> > >    │ │PCIE CTL │ │      │ ├─────────┼───►    PCIE        ◄───┼─────┐      │    └──┬──────────▲──┘    │
> > >    │ │         ├─►Server│ │         │   │    SHMEM       │   │     │      │       │          │       │
> > >    │ │Clk, Vreg│ │      │ │         │   │                │   │     │      │    ┌──▼──────────┴──┐    │
> > >    │ │GPIO,GDSC│ └─▲──┬─┘ │         │   └────────────────┘   │     └──────┼────┤PCIE SCMI Inst  │    │
> > >    │ └─────────┘   │  │   │         │                        │            │    └──▲──────────┬──┘    │
> > >    │               │  │   │         │                        │            │       │          │       │
> > >    └───────────────┼──┼───┘         │                        │            └───────┼──────────┼───────┘
> > >                    │  │             │                        │                    │          │
> > >                    │  │             └────────────────────────┘                    │          │
> > >                    │  │                                                           │          │
> > >                    │  │                                                           │          │
> > >                    │  │                                                           │          │
> > >                    │  │                                                           │IRQ       │HVC
> > >                IRQ │  │HVC                                                        │          │
> > >                    │  │                                                           │          │
> > >                    │  │                                                           │          │
> > >                    │  │                                                           │          │
> > > ┌─────────────────┴──▼───────────────────────────────────────────────────────────┴──────────▼──────────────┐
> > > │                                                                                                          │
> > > │                                                                                                          │
> > > │                                      HYPERVISOR                                                          │
> > > │                                                                                                          │
> > > │                                                                                                          │
> > > │                                                                                                          │
> > > └──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
> > >    ┌─────────────┐    ┌─────────────┐  ┌──────────┐   ┌───────────┐   ┌─────────────┐  ┌────────────┐
> > >    │             │    │             │  │          │   │           │   │  PCIE       │  │   PCIE     │
> > >    │   CLOCK     │    │   REGULATOR │  │   GPIO   │   │   GDSC    │   │  PHY        │  │ controller │
> > >    └─────────────┘    └─────────────┘  └──────────┘   └───────────┘   └─────────────┘  └────────────┘
> > 
> > Thanks a lot for working on this Mayank! This version looks good to me. I've
> > left some comments, nothing alarming though.
> Thanks for reviewing change. I would address those in next patchset.
> 
> > But I do want to hold up this series until we finalize the SCMI based design.
> ok. I want to send these changes which are prepared based on previously
> provided feedback, to see if we have any major concern here in terms of
> getting functionality.
> 

That's fine. My comment was a note to the maintainers.

- Mani

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