| 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: <20241115112123.ktv7ge3mfm6lavlj@thinkpad>
Date: Fri, 15 Nov 2024 16:51:23 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: neil.armstrong@...aro.org
Cc: Mayank Rana <quic_mrana@...cinc.com>, 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 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?
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists