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@xxxxxxxxxx> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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
--
மணிவண்ணன் சதாசிவம்
