lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7cfc0657-e8f4-45a8-95e2-668476ffce17@quicinc.com>
Date: Thu, 7 Nov 2024 09:45:37 -0800
From: Mayank Rana <quic_mrana@...cinc.com>
To: <neil.armstrong@...aro.org>, <jingoohan1@...il.com>,
        <manivannan.sadhasivam@...aro.org>, <will@...nel.org>,
        <lpieralisi@...nel.org>, <kw@...ux.com>, <robh@...nel.org>,
        <bhelgaas@...gle.com>, <krzk@...nel.org>
CC: <linux-pci@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_krichai@...cinc.com>
Subject: Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root
 complex functionality



On 11/7/2024 12:45 AM, neil.armstrong@...aro.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@...cinc.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ