| 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
| ||
|
Message-ID: <8128ddba-3db9-4908-95b3-a624081f3725@quicinc.com>
Date: Fri, 15 Nov 2024 10:17:02 -0800
From: Mayank Rana <quic_mrana@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
<neil.armstrong@...aro.org>
CC: <jingoohan1@...il.com>, <will@...nel.org>, <lpieralisi@...nel.org>,
<kw@...ux.com>, <robh@...nel.org>, <bhelgaas@...gle.com>,
<krzk@...nel.org>, <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/15/2024 3:21 AM, Manivannan Sadhasivam wrote:
> On Fri, Nov 08, 2024 at 11:22:52AM +0100, neil.armstrong@...aro.org wrote:
>> On 07/11/2024 18:45, Mayank Rana wrote:
>>>
>>>
>>> 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.
>>
>> 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
>
Powered by blists - more mailing lists