PERST# is an (optional) auxiliary signal provided by the PCIe host to
components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
sec 6.6.1.

If PERST# is available, it's state will be toggled during the component
power-up and power-down scenarios as per the PCI Express Card
Electromechanical Spec v4.0, sec 2.2.

Historically, the PCIe controller drivers were directly controlling the
PERST# signal together with the power supplies. But with the advent of the
pwrctrl framework, the power supply control is now moved to the pwrctrl,
but controller drivers still ended up toggling the PERST# signal. This only
happens on Qcom platforms where pwrctrl framework is being used. But
nevertheseless, it is wrong to toggle PERST# (especially deassert) without
controlling the power supplies.

So allow the pwrctrl core to control the PERST# GPIO is available. The
controller drivers still need to parse them and populate the
'host_bridge->perst' GPIO descriptor array based on the available slots.
Unfortunately, we cannot just move the PERST# handling from controller
drivers as most of the controller drivers need to assert PERST# during the
controller initialization.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrctrl.h |  2 ++
 include/linux/pci.h         |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -3,14 +3,19 @@
  * Copyright (C) 2024 Linaro Ltd.
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci-pwrctrl.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 
+#include "../pci.h"
+
 static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
 			      void *data)
 {
@@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
  */
 void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
 {
+	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
+	int devfn;
+
 	pwrctrl->dev = dev;
 	INIT_WORK(&pwrctrl->work, rescan_work_func);
+
+	if (!host_bridge->perst)
+		return;
+
+	devfn = of_pci_get_devfn(dev_of_node(dev));
+	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
+		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
 
+static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
+{
+	/* Bail out early to avoid the delay if PERST# is not available */
+	if (!pwrctrl->perst)
+		return;
+
+	msleep(PCIE_T_PVPERL_MS);
+	gpiod_set_value_cansleep(pwrctrl->perst, 0);
+	/*
+	 * FIXME: The following delay is only required for downstream ports not
+	 * supporting link speed greater than 5.0 GT/s.
+	 */
+	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
+}
+
+static void pci_pwrctrl_perst_assert(struct pci_pwrctrl *pwrctrl)
+{
+	/* No need to validate desc here as gpiod APIs handle it for us */
+	gpiod_set_value_cansleep(pwrctrl->perst, 1);
+}
+
 /**
  * pci_pwrctrl_device_set_ready() - Notify the pwrctrl subsystem that the PCI
  * device is powered-up and ready to be detected.
@@ -82,6 +118,8 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
 	if (!pwrctrl->dev)
 		return -ENODEV;
 
+	pci_pwrctrl_perst_deassert(pwrctrl);
+
 	pwrctrl->nb.notifier_call = pci_pwrctrl_notify;
 	ret = bus_register_notifier(&pci_bus_type, &pwrctrl->nb);
 	if (ret)
@@ -103,6 +141,7 @@ void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
 {
 	cancel_work_sync(&pwrctrl->work);
 
+	pci_pwrctrl_perst_assert(pwrctrl);
 	/*
 	 * We don't have to delete the link here. Typically, this function
 	 * is only called when the power control device is being detached. If
diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
index 7d439b0675e91e920737287eaf1937f38e47f2ce..1ce6aec343fea1b77a146682f499ece791be70dc 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -31,6 +31,7 @@ struct device_link;
 /**
  * struct pci_pwrctrl - PCI device power control context.
  * @dev: Address of the power controlling device.
+ * @perst: PERST# GPIO connected to the PCI device.
  *
  * An object of this type must be allocated by the PCI power control device and
  * passed to the pwrctrl subsystem to trigger a bus rescan and setup a device
@@ -38,6 +39,7 @@ struct device_link;
  */
 struct pci_pwrctrl {
 	struct device *dev;
+	struct gpio_desc *perst;
 
 	/* Private: don't use. */
 	struct notifier_block nb;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..fcce106c7e2985ee1dd79bfcd241f133e5599fe1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -39,6 +39,7 @@
 #include <linux/io.h>
 #include <linux/resource_ext.h>
 #include <linux/msi_api.h>
+#include <linux/gpio/consumer.h>
 #include <uapi/linux/pci.h>
 
 #include <linux/pci_ids.h>
@@ -600,6 +601,7 @@ struct pci_host_bridge {
 	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
 	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
 	void		*release_data;
+	struct gpio_desc **perst;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	no_inc_mrrs:1;		/* No Increase MRRS */

-- 
2.45.2

Sorry for so many individual reviews, but I've passed over this a few
times and had new questions/comments several times:

On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> PERST# is an (optional) auxiliary signal provided by the PCIe host to
> components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> sec 6.6.1.

>  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  {
> +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> +	int devfn;
> +
>  	pwrctrl->dev = dev;
>  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> +
> +	if (!host_bridge->perst)
> +		return;
> +
> +	devfn = of_pci_get_devfn(dev_of_node(dev));
> +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])

This seems to imply a 1:1 correlation between slots and pwrctrl devices,
almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
But there is also endpoint-specific pwrctrl support, and there's quite
a bit of flexibility around what these hierarchies can look like.

How do you account for that?

For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
they both grab the same PERST# GPIO here? And might that incur excessive
resets, possibly even clobbering each other?

Or what if multiple slots are governed by a single GPIO? Do you expect
the bridge perst[] array to contain redundant GPIOs?

Brian

> +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);

On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> Sorry for so many individual reviews, but I've passed over this a few
> times and had new questions/comments several times:
> 

That's fine. I'm happy to answer as someone other than me is interested in
pwrctrl :)

> On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > sec 6.6.1.
> 
> >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > +	int devfn;
> > +
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +
> > +	if (!host_bridge->perst)
> > +		return;
> > +
> > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> 
> This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> But there is also endpoint-specific pwrctrl support, and there's quite
> a bit of flexibility around what these hierarchies can look like.
> 
> How do you account for that?
> 
> For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> they both grab the same PERST# GPIO here? And might that incur excessive
> resets, possibly even clobbering each other?
> 

If both port and endpoint nodes are present, then only one will contain
'reset-gpios'. Right now, the DT binding only supports PERST#, WAKE#, CLKREQ#
properties in RP node, but that won't work if we have multiple lines per slot/
controller. Ideally, we would want the properties to be present in endpoint node
if available. But if we have only standard expansion slots, then it makes sense
to define them in the port node. But doing so, we can only expect the slot to
have only one instance of these properties as we cannot reliably map which
property corresponds to the endpoint.

I've opened a dtschema issue for this:
https://github.com/devicetree-org/dt-schema/issues/168

This is what I have in my mind:

1) For expansion slots:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	reset-gpios = <>;
	wake-gpios = <>;
};

2) For endpoint nodes:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	pcie@0 {
		reg = <0x10000 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	pcie@1 {
		reg = <0x10800 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	...
};

I don't think there is any usecase to have both slot and endpoint nodes defining
these properties.

But for your initial question on what if the platform doesn't define any supply
and uses non-GPIO based PERST#/WAKE#/CLKREQ#, I don't know how to let PCI core
create the pwrctrl device. This is something we need to brainstorm.

> Or what if multiple slots are governed by a single GPIO? Do you expect
> the bridge perst[] array to contain redundant GPIOs?
> 

It is common for devices within a slot/hierarchy to share these GPIOs (with some
drawbacks), but I'm not sure how all slots can share a single GPIO. That would
mean, if the host wants to reset one endpoint in a slot, it has to reset all the
endpoints connected to all slots. Sounds like a bizarre design to me.

- Mani

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

On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > Sorry for so many individual reviews, but I've passed over this a few
> > times and had new questions/comments several times:
> > 
> 
> That's fine. I'm happy to answer as someone other than me is interested in
> pwrctrl :)
> 
> > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > sec 6.6.1.
> > 
> > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > >  {
> > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > +	int devfn;
> > > +
> > >  	pwrctrl->dev = dev;
> > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > +
> > > +	if (!host_bridge->perst)
> > > +		return;
> > > +
> > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > 
> > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > But there is also endpoint-specific pwrctrl support, and there's quite
> > a bit of flexibility around what these hierarchies can look like.
> > 
> > How do you account for that?
> > 
> > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > they both grab the same PERST# GPIO here? And might that incur excessive
> > resets, possibly even clobbering each other?
> > 
> 
> If both port and endpoint nodes are present, then only one will contain
> 'reset-gpios'. Right now, the DT binding only supports PERST#, WAKE#, CLKREQ#
> properties in RP node, but that won't work if we have multiple lines per slot/
> controller. Ideally, we would want the properties to be present in endpoint node
> if available. But if we have only standard expansion slots, then it makes sense
> to define them in the port node. But doing so, we can only expect the slot to
> have only one instance of these properties as we cannot reliably map which
> property corresponds to the endpoint.
> 
> I've opened a dtschema issue for this:
> https://github.com/devicetree-org/dt-schema/issues/168
> 

I realized that there is no need to define these properties (PERST#, WAKE#,
CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
These properties should just exist in the Root Port node as there can be only
one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
per Root Port and the endpoint need to share them.

So I closed the dtschema issue.

- Mani

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

Thanks for clearing up some confusion. I was misled on some aspects. But
I think there's still a problem in here:

On Thu, Jul 24, 2025 at 07:43:38PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> > On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > > sec 6.6.1.
> > > 
> > > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > > >  {
> > > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > > +	int devfn;
> > > > +
> > > >  	pwrctrl->dev = dev;
> > > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > > +
> > > > +	if (!host_bridge->perst)
> > > > +		return;
> > > > +
> > > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > > 
> > > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > > But there is also endpoint-specific pwrctrl support, and there's quite
> > > a bit of flexibility around what these hierarchies can look like.
> > > 
> > > How do you account for that?
> > > 
> > > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > > they both grab the same PERST# GPIO here? And might that incur excessive
> > > resets, possibly even clobbering each other?
...
> I realized that there is no need to define these properties (PERST#, WAKE#,
> CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
> These properties should just exist in the Root Port node as there can be only
> one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
> per Root Port and the endpoint need to share them.

That implies it's not a 1:1 correlation between PERST# GPIO and pwrctrl
device. Multiple endpoints might need powered up, but they may share a
PERST#. I don't think this patch solves this properly, as it allows the
first one to deassert PERST# before the other(s) are powered.

Or maybe I'm missing something yet again :)

Brian

On Fri, Jul 25, 2025 at 02:04:28PM GMT, Brian Norris wrote:
> Thanks for clearing up some confusion. I was misled on some aspects. But
> I think there's still a problem in here:
> 
> On Thu, Jul 24, 2025 at 07:43:38PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > > > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > > > sec 6.6.1.
> > > > 
> > > > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > > > >  {
> > > > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > > > +	int devfn;
> > > > > +
> > > > >  	pwrctrl->dev = dev;
> > > > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > > > +
> > > > > +	if (!host_bridge->perst)
> > > > > +		return;
> > > > > +
> > > > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > > > 
> > > > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > > > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > > > But there is also endpoint-specific pwrctrl support, and there's quite
> > > > a bit of flexibility around what these hierarchies can look like.
> > > > 
> > > > How do you account for that?
> > > > 
> > > > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > > > they both grab the same PERST# GPIO here? And might that incur excessive
> > > > resets, possibly even clobbering each other?
> ...
> > I realized that there is no need to define these properties (PERST#, WAKE#,
> > CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
> > These properties should just exist in the Root Port node as there can be only
> > one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
> > per Root Port and the endpoint need to share them.
> 
> That implies it's not a 1:1 correlation between PERST# GPIO and pwrctrl
> device. Multiple endpoints might need powered up, but they may share a
> PERST#. I don't think this patch solves this properly, as it allows the
> first one to deassert PERST# before the other(s) are powered.
> 

You are right. This series doesn't take account of this configuration and I plan
to incorporate it in the next one. It might take some time as supporting such
configuration (and others that we discussed in this thread) is not
straightforward.

- Mani

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

Hi Manivannan,

On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> PERST# is an (optional) auxiliary signal provided by the PCIe host to
> components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> sec 6.6.1.
> 
> If PERST# is available, it's state will be toggled during the component
> power-up and power-down scenarios as per the PCI Express Card
> Electromechanical Spec v4.0, sec 2.2.
> 
> Historically, the PCIe controller drivers were directly controlling the
> PERST# signal together with the power supplies. But with the advent of the
> pwrctrl framework, the power supply control is now moved to the pwrctrl,
> but controller drivers still ended up toggling the PERST# signal.

[reflowed:]
> This only happens on Qcom platforms where pwrctrl framework is being
> used.

What do you mean by this sentence? That this problem only occurs on Qcom
platforms? (I believe that's false.) Or that the problem doesn't occur
if the platform is not using pwrctrl? (i.e., it maintained power in some
other way, before the controller driver gets involved. I believe this
variation is correct.)

> But
> nevertheseless, it is wrong to toggle PERST# (especially deassert) without
> controlling the power supplies.
> 
> So allow the pwrctrl core to control the PERST# GPIO is available. The

s/is/if/

?

> controller drivers still need to parse them and populate the
> 'host_bridge->perst' GPIO descriptor array based on the available slots.
> Unfortunately, we cannot just move the PERST# handling from controller
> drivers as most of the controller drivers need to assert PERST# during the
> controller initialization.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-pwrctrl.h |  2 ++
>  include/linux/pci.h         |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c

>  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
> @@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
>   */
>  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  {
> +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> +	int devfn;
> +
>  	pwrctrl->dev = dev;
>  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> +
> +	if (!host_bridge->perst)
> +		return;
> +
> +	devfn = of_pci_get_devfn(dev_of_node(dev));
> +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];

It seems a little suspect that we trust the device tree slot
specification to not overflow the perst[] array. I think we can
reasonably mitigate that in the controller driver (so, patch 3 in this
series), but I want to call that out, in case there's something we can
do here too.

>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
>  
> +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> +{
> +	/* Bail out early to avoid the delay if PERST# is not available */
> +	if (!pwrctrl->perst)
> +		return;
> +
> +	msleep(PCIE_T_PVPERL_MS);
> +	gpiod_set_value_cansleep(pwrctrl->perst, 0);

What if PERST# was already deasserted? On one hand, we're wasting time
here if so. On the other, you're not accomplishing your spec-compliance
goal if it was.

> +	/*
> +	 * FIXME: The following delay is only required for downstream ports not
> +	 * supporting link speed greater than 5.0 GT/s.
> +	 */
> +	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);

Should this be PCIE_RESET_CONFIG_DEVICE_WAIT_MS or PCIE_T_RRS_READY_MS?
Or are those describing the same thing? It seems like they were added
within a month or two of each other, so maybe they're just duplicates.

BTW, I see you have a FIXME here, but anyway, I wonder if both of the
msleep() delays in this function will need some kind of override (e.g.,
via Device Tree), since there's room for implementation or form factor
differences, if I'm reading the spec correctly. Maybe that's a question
for another time, with actual proof / use case.

> +}
> +

Brian

On Tue, Jul 08, 2025 at 08:15:39PM GMT, Brian Norris wrote:
> Hi Manivannan,
> 
> On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > sec 6.6.1.
> > 
> > If PERST# is available, it's state will be toggled during the component
> > power-up and power-down scenarios as per the PCI Express Card
> > Electromechanical Spec v4.0, sec 2.2.
> > 
> > Historically, the PCIe controller drivers were directly controlling the
> > PERST# signal together with the power supplies. But with the advent of the
> > pwrctrl framework, the power supply control is now moved to the pwrctrl,
> > but controller drivers still ended up toggling the PERST# signal.
> 
> [reflowed:]
> > This only happens on Qcom platforms where pwrctrl framework is being
> > used.
> 
> What do you mean by this sentence? That this problem only occurs on Qcom
> platforms? (I believe that's false.) Or that the problem doesn't occur
> if the platform is not using pwrctrl? (i.e., it maintained power in some
> other way, before the controller driver gets involved. I believe this
> variation is correct.)
> 

The latter one. I will rephrase this sentence in next version.

> > But
> > nevertheseless, it is wrong to toggle PERST# (especially deassert) without
> > controlling the power supplies.
> > 
> > So allow the pwrctrl core to control the PERST# GPIO is available. The
> 
> s/is/if/
> 
> ?
> 
> > controller drivers still need to parse them and populate the
> > 'host_bridge->perst' GPIO descriptor array based on the available slots.
> > Unfortunately, we cannot just move the PERST# handling from controller
> > drivers as most of the controller drivers need to assert PERST# during the
> > controller initialization.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-pwrctrl.h |  2 ++
> >  include/linux/pci.h         |  2 ++
> >  3 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> 
> >  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
> >  			      void *data)
> >  {
> > @@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
> >   */
> >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > +	int devfn;
> > +
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +
> > +	if (!host_bridge->perst)
> > +		return;
> > +
> > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
> 
> It seems a little suspect that we trust the device tree slot
> specification to not overflow the perst[] array. I think we can
> reasonably mitigate that in the controller driver (so, patch 3 in this
> series), but I want to call that out, in case there's something we can
> do here too.
> 
> >  }
> >  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
> >  
> > +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> > +{
> > +	/* Bail out early to avoid the delay if PERST# is not available */
> > +	if (!pwrctrl->perst)
> > +		return;
> > +
> > +	msleep(PCIE_T_PVPERL_MS);
> > +	gpiod_set_value_cansleep(pwrctrl->perst, 0);
> 
> What if PERST# was already deasserted? On one hand, we're wasting time
> here if so. On the other, you're not accomplishing your spec-compliance
> goal if it was.
> 

If controller drivers populate 'pci_host_bridge::perst', then they should not
deassert PERST# as they don't control the supplies. I've mentioned it in the
cover letter, but I will mention it in commit message also.

> > +	/*
> > +	 * FIXME: The following delay is only required for downstream ports not
> > +	 * supporting link speed greater than 5.0 GT/s.
> > +	 */
> > +	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
> 
> Should this be PCIE_RESET_CONFIG_DEVICE_WAIT_MS or PCIE_T_RRS_READY_MS?
> Or are those describing the same thing? It seems like they were added
> within a month or two of each other, so maybe they're just duplicates.
> 

You are right. This is already taken care in:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/linkup-fix&id=bbc6a829ad3f054181d24a56944f944002e68898

I will rebase the next version on top of pci/next to make use of it.

> BTW, I see you have a FIXME here, but anyway, I wonder if both of the
> msleep() delays in this function will need some kind of override (e.g.,
> via Device Tree), since there's room for implementation or form factor
> differences, if I'm reading the spec correctly. Maybe that's a question
> for another time, with actual proof / use case.
> 

First delay cannot be skipped as both PCIe CEM and M.2 FF, mandates this delay.
Though, M.2 mandates only a min delay of 50ms, and leaves the value to be
defined by the vendor. So a common 100ms would be on the safe side.

For the second delay, it comes from the PCIe spec itself. Refer r6.0, sec
6.6.1. So we cannot skip that, though we should only need it if the link is
operating at <= 5.0 GT/s. Right now, I haven't implemented the logic to detect
the link speed, hence the TODO.

- Mani

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

On Wed, Jul 09, 2025 at 01:35:08PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 08, 2025 at 08:15:39PM GMT, Brian Norris wrote:
> > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > --- a/drivers/pci/pwrctrl/core.c
> > > +++ b/drivers/pci/pwrctrl/core.c
> > > +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> > > +{
> > > +	/* Bail out early to avoid the delay if PERST# is not available */
> > > +	if (!pwrctrl->perst)
> > > +		return;
> > > +
> > > +	msleep(PCIE_T_PVPERL_MS);
> > > +	gpiod_set_value_cansleep(pwrctrl->perst, 0);
> > 
> > What if PERST# was already deasserted? On one hand, we're wasting time
> > here if so. On the other, you're not accomplishing your spec-compliance
> > goal if it was.
> > 
> 
> If controller drivers populate 'pci_host_bridge::perst', then they should not
> deassert PERST# as they don't control the supplies. I've mentioned it in the
> cover letter, but I will mention it in commit message also.

Sorry, I think I partially read that, but didn't really grasp how it fit
in here.

I commented on patch 3 where you try to implement this. IIUC, you're
making excessive assumptions about the use of pwrctrl.

Brian