Message-ID: <gcrf4q45gpcmnvdz55qoga6sc7mxrizzhnb4h6afwgk4cmamp4@mggbezcfivff>
Date: Wed, 27 Aug 2025 22:57:57 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Saravana Kannan <saravanak@...gle.com>, linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH 5/6] PCI: qcom: Parse PERST# from all PCIe bridge nodes

On Wed, Aug 27, 2025 at 06:34:38PM GMT, Bartosz Golaszewski wrote:
> On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@...nel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> >
> > Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> > nodes, not just in Root Port node. But the current logic parses PERST# only
> > from the Root Port node. Though it is not causing any issue on the current
> > platforms, the upcoming platforms will have PERST# in PCIe switch
> > downstream ports also. So this requires parsing all the PCIe bridge nodes
> > for the PERST# GPIO.
> >
> > Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
> > from Root Port node. If the 'reset-gpios' property is found for a node, the
> > GPIO descriptor will be stored in IDR structure with node BDF as the ID.
> >
> > It should be noted that if more than one bridge node has the same GPIO for
> > PERST# (shared PERST#), the driver will error out. This is due to the
> > limitation in the GPIOLIB subsystem that allows only exclusive (non-shared)
> > access to GPIOs from consumers. But this is soon going to get fixed. Once
> > that happens, it will get incorporated in this driver.
> >
> > So for now, PERST# sharing is not supported.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 90 +++++++++++++++++++++++++++-------
> >  1 file changed, 73 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index bcd080315d70e64eafdefd852740fe07df3dbe75..5d73c46095af3219687ff77e5922f08bb41e43a9 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/limits.h>
> > +#include <linux/idr.h>
> >  #include <linux/init.h>
> >  #include <linux/of.h>
> >  #include <linux/of_pci.h>
> > @@ -286,6 +287,7 @@ struct qcom_pcie {
> >         const struct qcom_pcie_cfg *cfg;
> >         struct dentry *debugfs;
> >         struct list_head ports;
> > +       struct idr perst;
> >         bool suspended;
> >         bool use_pm_opp;
> >  };
> > @@ -294,14 +296,15 @@ struct qcom_pcie {
> >
> >  static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  {
> > -       struct qcom_pcie_port *port;
> >         int val = assert ? 1 : 0;
> > +       struct gpio_desc *perst;
> > +       int bdf;
> >
> > -       if (list_empty(&pcie->ports))
> > +       if (idr_is_empty(&pcie->perst))
> >                 gpiod_set_value_cansleep(pcie->reset, val);
> > -       else
> > -               list_for_each_entry(port, &pcie->ports, list)
> > -                       gpiod_set_value_cansleep(port->reset, val);
> > +
> > +       idr_for_each_entry(&pcie->perst, perst, bdf)
> > +               gpiod_set_value_cansleep(perst, val);
> >  }
> >
> >  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > @@ -1702,20 +1705,58 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
> >         }
> >  };
> >
> > -static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
> > +/* Parse PERST# from all nodes in depth first manner starting from @np */
> > +static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
> > +                                struct device_node *np)
> >  {
> >         struct device *dev = pcie->pci->dev;
> > -       struct qcom_pcie_port *port;
> >         struct gpio_desc *reset;
> > -       struct phy *phy;
> >         int ret;
> >
> > -       reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> > -                                     "reset", GPIOD_OUT_HIGH, "PERST#");
> > -       if (IS_ERR(reset))
> > +       if (!of_find_property(np, "reset-gpios", NULL))
> > +               goto parse_child_node;
> > +
> > +       ret = of_pci_get_bdf(np);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
> > +                                     GPIOD_OUT_HIGH, "PERST#");
> > +       if (IS_ERR(reset)) {
> > +               /*
> > +                * FIXME: GPIOLIB currently supports exclusive GPIO access only.
> > +                * Non exclusive access is broken. But shared PERST# requires
> > +                * non-exclusive access. So once GPIOLIB properly supports it,
> > +                * implement it here.
> > +                */
> > +               if (PTR_ERR(reset) == -EBUSY)
> > +                       dev_err(dev, "Shared PERST# is not supported\n");
> 
> Then maybe just use the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag for now and
> don't bail-out - it will make it easier to spot it when converting to
> the new solution?
> 

But that gives the impression that shared PERST# is supported, but in reality it
is not.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists