Message-ID: <CAMyL0qNQa1HuLE3F+rtk6YAKvthbzMKLmE+it_K7BT12vt86tA@mail.gmail.com>
Date: Tue, 9 Dec 2025 17:56:36 +0530
From: Mrinmay Sarkar <mrinmay.sarkar@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kwilczynski@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>, linux-arm-msm@...r.kernel.org,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@....qualcomm.com,
        Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>,
        Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
        quic_vbadigan@...cinc.com, quic_shazhuss@...cinc.com,
        konrad.dybcio@....qualcomm.com, Rama Krishna <quic_ramkri@...cinc.com>,
        Ayiluri Naga Rashmi <quic_nayiluri@...cinc.com>,
        Nitesh Gupta <quic_nitegupt@...cinc.com>
Subject: Re: [PATCH 2/2] PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

On Sat, Dec 6, 2025 at 2:27 AM Bjorn Andersson <andersson@...nel.org> wrote:
>
> On Wed, Dec 03, 2025 at 06:56:48PM +0530, Mrinmay Sarkar wrote:
> > Some Qualcomm platforms use firmware to manage PCIe resources such as
> > clocks, resets, and PHY through the SCMI interface. In these cases,
> > the Linux driver should not perform resource enable or disable
> > operations directly. Additionally, runtime PM support has been enabled
> > to ensure proper power state transitions.
> >
> > This commit introduces a `firmware_managed` flag in the Endpoint
> > configuration structure. When set, the driver skips resource handling
> > and uses generic runtime PM calls to let firmware do resource management.
> >
> > A new compatible string is added for SA8255P platforms where firmware
> > manages resources.
> >
> > Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@....qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 80 ++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index f1bc0ac81a928b928ab3f8cc7bf82558fc430474..38358c9fa7ab32fd36efcea0a42c52f1f86a523a 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -168,11 +168,13 @@ enum qcom_pcie_ep_link_status {
> >   * @hdma_support: HDMA support on this SoC
> >   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache snooping
> >   * @disable_mhi_ram_parity_check: Disable MHI RAM data parity error check
> > + * @firmware_managed: Set if the Endpoint controller is firmware managed
> >   */
> >  struct qcom_pcie_ep_cfg {
> >       bool hdma_support;
> >       bool override_no_snoop;
> >       bool disable_mhi_ram_parity_check;
> > +     bool firmware_managed;
> >  };
> >
> >  /**
> > @@ -377,6 +379,15 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
> >
> >  static void qcom_pcie_disable_resources(struct qcom_pcie_ep *pcie_ep)
> >  {
> > +     struct device *dev = pcie_ep->pci.dev;
> > +     int ret;
> > +
> > +     ret = pm_runtime_put_sync(dev);
>
> What's the benefit of waiting for the put to finish? (i.e. why _sync)
>
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> > +             return;
>
> For some reason the pm_runtime_put_sync() failed, so the device's state
> is going to remain active. But you prevented the resources below from
> being disabled - without returning an error, so nobody knows.
>
> So now the phy refcount etc will be wrong.
>

Thanks Bjorn for the review.
I think we can use pm_runtime_put() as we should disable resources
even if it fails.

> > +     }
> > +

And I will add a check here as we don't need below for the
firmware_managed case.

> >       icc_set_bw(pcie_ep->icc_mem, 0, 0);
> >       phy_power_off(pcie_ep->phy);
> >       phy_exit(pcie_ep->phy);
> > @@ -390,12 +401,22 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >       u32 val, offset;
> >       int ret;
> >
> > -     ret = qcom_pcie_enable_resources(pcie_ep);
> > -     if (ret) {
> > -             dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +     ret = pm_runtime_get_sync(dev);
>
> You're missing necessary error handling for pm_runtime_get_sync(), use
> pm_runtime_resume_and_get() instead.
>

Yes, we can use pm_runtime_resume_and_get() here as it will handle
errors safely.

> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
> >               return ret;
> >       }
> >
> > +     /* Enable resources if Endpoint controller is not firmware-managed */
> > +     if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> > +             ret = qcom_pcie_enable_resources(pcie_ep);
>
> Now that you're moving the driver to adequately get and put the RPM
> state, can't you move the explicit resource management to pm_ops as
> well?

Actually we are planning to enable runtime pm_ops in a separate series.
We will be taking care of this in that series.

>
> > +             if (ret) {
> > +                     dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +                     pm_runtime_put_sync(dev);
> > +                     return ret;
> > +             }
> > +     }
> > +
> >       /* Perform cleanup that requires refclk */
> >       pci_epc_deinit_notify(pci->ep.epc);
> >       dw_pcie_ep_cleanup(&pci->ep);
> > @@ -630,16 +651,6 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> >               return ret;
> >       }
> >
> > -     pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> > -     if (pcie_ep->num_clks < 0) {
> > -             dev_err(dev, "Failed to get clocks\n");
> > -             return pcie_ep->num_clks;
> > -     }
> > -
> > -     pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > -     if (IS_ERR(pcie_ep->core_reset))
> > -             return PTR_ERR(pcie_ep->core_reset);
> > -
> >       pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> >       if (IS_ERR(pcie_ep->reset))
> >               return PTR_ERR(pcie_ep->reset);
> > @@ -652,9 +663,22 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> >       if (IS_ERR(pcie_ep->phy))
> >               ret = PTR_ERR(pcie_ep->phy);
> >
> > -     pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> > -     if (IS_ERR(pcie_ep->icc_mem))
> > -             ret = PTR_ERR(pcie_ep->icc_mem);
> > +     /* Populate resources if Endpoint controller is not firmware-managed */
> > +     if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> > +             pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> > +             if (pcie_ep->num_clks < 0) {
> > +                     dev_err(dev, "Failed to get clocks\n");
> > +                     return pcie_ep->num_clks;
> > +             }
> > +
> > +             pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > +             if (IS_ERR(pcie_ep->core_reset))
> > +                     return PTR_ERR(pcie_ep->core_reset);
> > +
> > +             pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> > +             if (IS_ERR(pcie_ep->icc_mem))
> > +                     ret = PTR_ERR(pcie_ep->icc_mem);
> > +     }
> >
> >       return ret;
> >  }
> > @@ -874,6 +898,16 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >
> >       platform_set_drvdata(pdev, pcie_ep);
> >
> > +     pm_runtime_set_active(dev);
> > +     ret = devm_pm_runtime_enable(dev);
> > +     if (ret)
> > +             return ret;
> > +     ret = pm_runtime_get_sync(dev);
>
> As the device is already active, this will just bump the reference count
> and return. I think the correct way to write this is:
>
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>

Yes, here pm_runtime_get_sync() is just incrementing the usage_count
as the device is already active.
we can use pm_runtime_get_noresume() instead.

The reason I was using devm_pm_runtime_enable() is because it
automatically disables runtime PM
in case of probe failure. Please let  me know your thoughts on this.

>
> But to handle the non-fw-managed case, you probably want to just remove
> the pm_runtime_set_active() and keep the get_sync(), to allow the
> resources to be turned on, thus would though have to happen after you
> acquire the resources below.
>
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> >       ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> >       if (ret)
> >               return ret;
> > @@ -897,6 +931,12 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >       pcie_ep->debugfs = debugfs_create_dir(name, NULL);
> >       qcom_pcie_ep_init_debugfs(pcie_ep);
>
> This was last, because we don't care about failures. But now that you're
> adding a source of errors below, you need to remove these entries again
> if below fails (or keep the debugfs creation last).

I will move debugfs creation last.

Thanks,
Mrinmay

>
> >
> > +     ret = pm_runtime_put_sync(dev);
> > +     if (ret < 0) {
>
> I don't think this is adequately error handled.
>
> Regards,
> Bjorn
>
> > +             dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> > +             goto err_disable_irqs;
> > +     }
> > +
> >       return 0;
> >
> >  err_disable_irqs:
> > @@ -930,7 +970,15 @@ static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
> >       .disable_mhi_ram_parity_check = true,
> >  };
> >
> > +static const struct qcom_pcie_ep_cfg cfg_1_34_0_fw_managed = {
> > +     .hdma_support = true,
> > +     .override_no_snoop = true,
> > +     .disable_mhi_ram_parity_check = true,
> > +     .firmware_managed = true,
> > +};
> > +
> >  static const struct of_device_id qcom_pcie_ep_match[] = {
> > +     { .compatible = "qcom,sa8255p-pcie-ep", .data = &cfg_1_34_0_fw_managed},
> >       { .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
> >       { .compatible = "qcom,sdx55-pcie-ep", },
> >       { .compatible = "qcom,sm8450-pcie-ep", },
> >
> > --
> > 2.25.1
> >
> >

Powered by blists - more mailing lists