diff mbox series

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

Message ID 20241106221341.2218416-5-quic_mrana@quicinc.com
State New
Headers show
Series Add Qualcomm SA8255p based firmware managed PCIe root complex | expand

Commit Message

Mayank Rana Nov. 6, 2024, 10:13 p.m. UTC
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(-)

Comments

Neil Armstrong Nov. 7, 2024, 8:45 a.m. UTC | #1
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
Mayank Rana Nov. 7, 2024, 5:45 p.m. UTC | #2
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
Neil Armstrong Nov. 8, 2024, 10:22 a.m. UTC | #3
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
kernel test robot Nov. 9, 2024, 11:45 p.m. UTC | #4
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
Manivannan Sadhasivam Nov. 15, 2024, 11:21 a.m. UTC | #5
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
Manivannan Sadhasivam Nov. 15, 2024, 11:25 a.m. UTC | #6
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
Mayank Rana Nov. 15, 2024, 6:17 p.m. UTC | #7
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
>
Mayank Rana Nov. 15, 2024, 6:28 p.m. UTC | #8
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
>
Manivannan Sadhasivam Nov. 19, 2024, 5:14 p.m. UTC | #9
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
diff mbox series

Patch

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 },