Since the Qcom platforms rely on pwrctrl framework to control the power
supplies, allow it to control PERST# also. PERST# should be toggled during
the power-on and power-off scenarios.

But the controller driver still need to assert PERST# during the controller
initialization. So only skip the deassert if pwrctrl usage is detected. The
pwrctrl framework will deassert PERST# after turning on the supplies.

The usage of pwrctrl framework is detected based on the new DT binding
i.e., with the presence of PERST# and PHY properties in the Root Port node
instead of the host bridge node.

When the legacy binding is used, PERST# is only controlled by the
controller driver since it is not reliable to detect whether pwrctrl is
used or not. So the legacy platforms are untouched by this commit.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		return -ENOMEM;
 
 	pp->bridge = bridge;
+	bridge->perst = pp->perst;
 
 	ret = dw_pcie_host_get_resources(pp);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -430,6 +430,7 @@ struct dw_pcie_rp {
 	struct resource		*msg_res;
 	bool			use_linkup_irq;
 	struct pci_eq_presets	presets;
+	struct gpio_desc	**perst;
 };
 
 struct dw_pcie_ep_ops {
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
+
+	if (pp->perst)
+		return;
+
 	/* Ensure that PERST has been asserted for at least 100 ms */
 	msleep(PCIE_T_PVPERL_MS);
 	qcom_perst_assert(pcie, false);
@@ -1701,11 +1706,12 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
 
 static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
 {
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
 	struct device *dev = pcie->pci->dev;
 	struct qcom_pcie_port *port;
 	struct gpio_desc *reset;
 	struct phy *phy;
-	int ret;
+	int ret, devfn;
 
 	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
 				      "reset", GPIOD_OUT_HIGH, "PERST#");
@@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (ret)
 		return ret;
 
+	devfn = of_pci_get_devfn(node);
+	if (devfn < 0)
+		return -ENOENT;
+
+	pp->perst[PCI_SLOT(devfn)] = reset;
+
 	port->reset = reset;
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
@@ -1734,10 +1746,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 
 static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 {
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
 	struct device *dev = pcie->pci->dev;
 	struct qcom_pcie_port *port, *tmp;
+	int child_cnt;
 	int ret = -ENOENT;
 
+	child_cnt = of_get_available_child_count(dev->of_node);
+	if (!child_cnt)
+		return ret;
+
+	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
+	if (!pp->perst)
+		return -ENOMEM;
+
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = qcom_pcie_parse_port(pcie, of_port);
 		if (ret)
@@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
+	kfree(pp->perst);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		list_del(&port->list);
 
@@ -1984,6 +2007,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
 	qcom_pcie_phy_exit(pcie);
+	kfree(pp->perst);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		list_del(&port->list);
 err_pm_runtime_put:

-- 
2.45.2

Hi,

On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> Since the Qcom platforms rely on pwrctrl framework to control the power
> supplies, allow it to control PERST# also. PERST# should be toggled during
> the power-on and power-off scenarios.
> 
> But the controller driver still need to assert PERST# during the controller
> initialization. So only skip the deassert if pwrctrl usage is detected. The
> pwrctrl framework will deassert PERST# after turning on the supplies.
> 
> The usage of pwrctrl framework is detected based on the new DT binding
> i.e., with the presence of PERST# and PHY properties in the Root Port node
> instead of the host bridge node.

I just noticed what this paragraph means. IIUC, this implies that in
your new binding, one *must* describe one or more *-supply in the port
or endpoint device(s). Otherwise, no pwrctrl devices will be created,
and no one will deassert PERST# for you. My understanding is that
*-supply is optional, and so this is a poor requirement.

And even if all QCOM device trees manage to have external regulators
described in their device trees, there are certainly other systems where
the driver might (optionally) use pwrctrl for some devices, but others
will establish power on their own (e.g., PCIe tied to some other system
power rail).

I think you either need the PCI core to tell you whether pwrctrl is in
use, or else you need to disconnect this PERST# support from pwrctrl.

> When the legacy binding is used, PERST# is only controlled by the
> controller driver since it is not reliable to detect whether pwrctrl is
> used or not. So the legacy platforms are untouched by this commit.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		return -ENOMEM;
>  
>  	pp->bridge = bridge;
> +	bridge->perst = pp->perst;
>  
>  	ret = dw_pcie_host_get_resources(pp);
>  	if (ret)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -430,6 +430,7 @@ struct dw_pcie_rp {
>  	struct resource		*msg_res;
>  	bool			use_linkup_irq;
>  	struct pci_eq_presets	presets;
> +	struct gpio_desc	**perst;
>  };
>  
>  struct dw_pcie_ep_ops {
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> +
> +	if (pp->perst)

You're doing a non-NULL check here, but you forgot to reinitialize it to
NULL in some cases below (qcom_pcie_parse_ports()).

This is also apparently where you assume |perst| implies pwrctrl. Per
above, I don't think that's a good assumption.

> +		return;
> +
>  	/* Ensure that PERST has been asserted for at least 100 ms */
>  	msleep(PCIE_T_PVPERL_MS);
>  	qcom_perst_assert(pcie, false);
...
>  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
>  	struct device *dev = pcie->pci->dev;
>  	struct qcom_pcie_port *port, *tmp;
> +	int child_cnt;
>  	int ret = -ENOENT;
>  
> +	child_cnt = of_get_available_child_count(dev->of_node);
> +	if (!child_cnt)
> +		return ret;
> +
> +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> +	if (!pp->perst)
> +		return -ENOMEM;
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)
> @@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	return ret;
>  
>  err_port_del:
> +	kfree(pp->perst);

You need this too:

	pp->perst = NULL;

Otherwise, you leave a non-NULL (invalid) pointer for cases that
qcom_pcie_parse_legacy_binding() might handle.

Brian

>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>  		list_del(&port->list);
>

On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> Hi,
> 
> On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > Since the Qcom platforms rely on pwrctrl framework to control the power
> > supplies, allow it to control PERST# also. PERST# should be toggled during
> > the power-on and power-off scenarios.
> > 
> > But the controller driver still need to assert PERST# during the controller
> > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > pwrctrl framework will deassert PERST# after turning on the supplies.
> > 
> > The usage of pwrctrl framework is detected based on the new DT binding
> > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > instead of the host bridge node.
> 
> I just noticed what this paragraph means. IIUC, this implies that in
> your new binding, one *must* describe one or more *-supply in the port
> or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> and no one will deassert PERST# for you. My understanding is that
> *-supply is optional, and so this is a poor requirement.
> 

Your understanding is correct. But the problem is, you thought that pwrctrl
would work across all platforms without any modifications, which unfortunately
is not true and is the main source of confusion. And I never claim anywhere that
pwrctrl is ready for all platforms. I just want platforms to start showing
interest towards it and we will collectively solve the issues. Or I'll be happy
to solve the issues if platform maintainers show interest towards it. This is
what currently happening with brcmstb. I signed up for the transition to
pwrctrl as their out-of-tree is breaking with pwrctrl.

Right now, we indeed create pwrctrl device based on the presence of power
supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But
sure, for some other platforms we might have only 'reset-gpios'. When we have to
support those platforms, we will extend the logic.

> And even if all QCOM device trees manage to have external regulators
> described in their device trees, there are certainly other systems where
> the driver might (optionally) use pwrctrl for some devices, but others
> will establish power on their own (e.g., PCIe tied to some other system
> power rail).
> 
> I think you either need the PCI core to tell you whether pwrctrl is in
> use, or else you need to disconnect this PERST# support from pwrctrl.
> 

It is not straightforward for the PCI core to tell whether pwrctrl is in use or
not. pwrctrl has no devicetree representation as it is not a separate hardware
entity. It just reuses the PCI device DT node. So I used the -supply properties
to assume that the pwrctrl device will be used. And almost none of the upstream
DTS has -supply properties in the PCI child node (only exception is brcmstb
where they define -supply properties in the PCI child node, but only in the DT
binding). So that added up.

> > When the legacy binding is used, PERST# is only controlled by the
> > controller driver since it is not reliable to detect whether pwrctrl is
> > used or not. So the legacy platforms are untouched by this commit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  		return -ENOMEM;
> >  
> >  	pp->bridge = bridge;
> > +	bridge->perst = pp->perst;
> >  
> >  	ret = dw_pcie_host_get_resources(pp);
> >  	if (ret)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -430,6 +430,7 @@ struct dw_pcie_rp {
> >  	struct resource		*msg_res;
> >  	bool			use_linkup_irq;
> >  	struct pci_eq_presets	presets;
> > +	struct gpio_desc	**perst;
> >  };
> >  
> >  struct dw_pcie_ep_ops {
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> >  
> >  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> > +
> > +	if (pp->perst)
> 
> You're doing a non-NULL check here, but you forgot to reinitialize it to
> NULL in some cases below (qcom_pcie_parse_ports()).
> 
> This is also apparently where you assume |perst| implies pwrctrl. Per
> above, I don't think that's a good assumption.
> 

Atleast this assumption holds true for now. Otherwise, I'd have to implement
callbacks without any users.

> > +		return;
> > +
> >  	/* Ensure that PERST has been asserted for at least 100 ms */
> >  	msleep(PCIE_T_PVPERL_MS);
> >  	qcom_perst_assert(pcie, false);
> ...
> >  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> >  	struct device *dev = pcie->pci->dev;
> >  	struct qcom_pcie_port *port, *tmp;
> > +	int child_cnt;
> >  	int ret = -ENOENT;
> >  
> > +	child_cnt = of_get_available_child_count(dev->of_node);
> > +	if (!child_cnt)
> > +		return ret;
> > +
> > +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> > +	if (!pp->perst)
> > +		return -ENOMEM;
> > +
> >  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> >  		ret = qcom_pcie_parse_port(pcie, of_port);
> >  		if (ret)
> > @@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  	return ret;
> >  
> >  err_port_del:
> > +	kfree(pp->perst);
> 
> You need this too:
> 
> 	pp->perst = NULL;
> 
> Otherwise, you leave a non-NULL (invalid) pointer for cases that
> qcom_pcie_parse_legacy_binding() might handle.
> 

Right, thanks for spotting!

- Mani

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

Hi Manivannan,

Sorry for some delay. Things get busy, and I don't get the time for
proper review/reply sometimes...

On Sat, Jul 12, 2025 at 11:50:43AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> > Hi,
> > 
> > On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > > Since the Qcom platforms rely on pwrctrl framework to control the power
> > > supplies, allow it to control PERST# also. PERST# should be toggled during
> > > the power-on and power-off scenarios.
> > > 
> > > But the controller driver still need to assert PERST# during the controller
> > > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > > pwrctrl framework will deassert PERST# after turning on the supplies.
> > > 
> > > The usage of pwrctrl framework is detected based on the new DT binding
> > > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > > instead of the host bridge node.
> > 
> > I just noticed what this paragraph means. IIUC, this implies that in
> > your new binding, one *must* describe one or more *-supply in the port
> > or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> > and no one will deassert PERST# for you. My understanding is that
> > *-supply is optional, and so this is a poor requirement.
> > 
> 
> Your understanding is correct. But the problem is, you thought that pwrctrl
> would work across all platforms without any modifications, which unfortunately

I do not think this. Of course there's some modification needed on
occasion, especially when drivers assume they can poll for the link to
come up when power isn't ready, or if they want to get PERST# right
(i.e., $subject).

OTOH, I don't think you can claim that platforms *don't* support
pwrctrl. If a driver has a well-behaved start_link() behavior and
doesn't otherwise manage slot/endpoint *-supply properties (a la
pcie-brcmstb), it should mostly work without further involvement.

But crucially, that changes with PERST#. And I think you're making
very narrow assumptions when you do that.

> is not true and is the main source of confusion. And I never claim anywhere that
> pwrctrl is ready for all platforms. I just want platforms to start showing
> interest towards it and we will collectively solve the issues. Or I'll be happy
> to solve the issues if platform maintainers show interest towards it. This is
> what currently happening with brcmstb. I signed up for the transition to
> pwrctrl as their out-of-tree is breaking with pwrctrl.
> 
> Right now, we indeed create pwrctrl device based on the presence of power
> supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But

I don't see how this is really Qualcomm specific, unless you simply
require that all new Qcom DTs specify external *-supply. I don't see
that in your Documentation/devicetree/bindings/pci/qcom*.yaml though,
and I don't think that's reasonable.

> sure, for some other platforms we might have only 'reset-gpios'. When we have to
> support those platforms, we will extend the logic.

The thing is, you don't have 100% control over this. You sound like you
only want to support device trees that are shipped in the upstream
kernel, but that's not how they work -- it's totally valid to ship
non-upstream device trees, if you follow the DT bindings. And you've
already hit that pitfall with brcmstb.

Suppose you have a Qcom platform today, with pwrctrl support, and:

 1. it has GPIO PERST#
 2. some boards have external power controls for the endpoint. *-supply
    nodes are described for the endpoint, and pwrctrl is in use.
 3. some boards have hardwired power that is always-on / on at boot (no
    *-supply node, no pwrctrl).

As you've written it today, #3 will no longer work, since you're
deferring PERST# to pwrctrl, but pwrctrl never gets involved.

Crucially, you can't read the driver source to tell the difference
between #2 and #3, and it's not even in the binding schema. Now magnify
this across other drivers that might support this.

I have boards like #2 and #3, and I don't know how I'm supposed to
develop my driver.

> > And even if all QCOM device trees manage to have external regulators
> > described in their device trees, there are certainly other systems where
> > the driver might (optionally) use pwrctrl for some devices, but others
> > will establish power on their own (e.g., PCIe tied to some other system
> > power rail).
> > 
> > I think you either need the PCI core to tell you whether pwrctrl is in
> > use, or else you need to disconnect this PERST# support from pwrctrl.
> > 
> 
> It is not straightforward for the PCI core to tell whether pwrctrl is in use or
> not.

Yes, well this seems like a fundamental recurring problem at the root
here. This agnostic design just causes more problems, IMO.

> pwrctrl has no devicetree representation as it is not a separate hardware
> entity. It just reuses the PCI device DT node. So I used the -supply properties
> to assume that the pwrctrl device will be used. And almost none of the upstream
> DTS has -supply properties in the PCI child node (only exception is brcmstb
> where they define -supply properties in the PCI child node, but only in the DT
> binding). So that added up.

You gotta work off DT bindings and schema to make assertions. You can't
just guess based on in-tree device trees, and so you can't prove
non-existence, if it's not explicit in the bindings.

Brian

On Fri, Jul 25, 2025 at 01:53:02PM GMT, Brian Norris wrote:
> Hi Manivannan,
> 
> Sorry for some delay. Things get busy, and I don't get the time for
> proper review/reply sometimes...
> 
> On Sat, Jul 12, 2025 at 11:50:43AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> > > Hi,
> > > 
> > > On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > > > Since the Qcom platforms rely on pwrctrl framework to control the power
> > > > supplies, allow it to control PERST# also. PERST# should be toggled during
> > > > the power-on and power-off scenarios.
> > > > 
> > > > But the controller driver still need to assert PERST# during the controller
> > > > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > > > pwrctrl framework will deassert PERST# after turning on the supplies.
> > > > 
> > > > The usage of pwrctrl framework is detected based on the new DT binding
> > > > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > > > instead of the host bridge node.
> > > 
> > > I just noticed what this paragraph means. IIUC, this implies that in
> > > your new binding, one *must* describe one or more *-supply in the port
> > > or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> > > and no one will deassert PERST# for you. My understanding is that
> > > *-supply is optional, and so this is a poor requirement.
> > > 
> > 
> > Your understanding is correct. But the problem is, you thought that pwrctrl
> > would work across all platforms without any modifications, which unfortunately
> 
> I do not think this. Of course there's some modification needed on
> occasion, especially when drivers assume they can poll for the link to
> come up when power isn't ready, or if they want to get PERST# right
> (i.e., $subject).
> 
> OTOH, I don't think you can claim that platforms *don't* support
> pwrctrl. If a driver has a well-behaved start_link() behavior and
> doesn't otherwise manage slot/endpoint *-supply properties (a la
> pcie-brcmstb), it should mostly work without further involvement.
> 

I agree. As I dig more, there seems to be a gazellion of these combinations
exist. In the next version, I'll try to accomodate most of them.

> But crucially, that changes with PERST#. And I think you're making
> very narrow assumptions when you do that.
> 

This series does indeed. I wanted to start small, but I realized that going too
simplistic won't work for everyone.

> > is not true and is the main source of confusion. And I never claim anywhere that
> > pwrctrl is ready for all platforms. I just want platforms to start showing
> > interest towards it and we will collectively solve the issues. Or I'll be happy
> > to solve the issues if platform maintainers show interest towards it. This is
> > what currently happening with brcmstb. I signed up for the transition to
> > pwrctrl as their out-of-tree is breaking with pwrctrl.
> > 
> > Right now, we indeed create pwrctrl device based on the presence of power
> > supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But
> 
> I don't see how this is really Qualcomm specific, unless you simply
> require that all new Qcom DTs specify external *-supply. I don't see
> that in your Documentation/devicetree/bindings/pci/qcom*.yaml though,
> and I don't think that's reasonable.
> 
> > sure, for some other platforms we might have only 'reset-gpios'. When we have to
> > support those platforms, we will extend the logic.
> 
> The thing is, you don't have 100% control over this. You sound like you
> only want to support device trees that are shipped in the upstream
> kernel, but that's not how they work -- it's totally valid to ship
> non-upstream device trees, if you follow the DT bindings. And you've
> already hit that pitfall with brcmstb.
> 
> Suppose you have a Qcom platform today, with pwrctrl support, and:
> 
>  1. it has GPIO PERST#
>  2. some boards have external power controls for the endpoint. *-supply
>     nodes are described for the endpoint, and pwrctrl is in use.
>  3. some boards have hardwired power that is always-on / on at boot (no
>     *-supply node, no pwrctrl).
> 
> As you've written it today, #3 will no longer work, since you're
> deferring PERST# to pwrctrl, but pwrctrl never gets involved.
> 
> Crucially, you can't read the driver source to tell the difference
> between #2 and #3, and it's not even in the binding schema. Now magnify
> this across other drivers that might support this.
> 
> I have boards like #2 and #3, and I don't know how I'm supposed to
> develop my driver.
> 

pci_pwrctrl_create_device() should be really smart to figure out these
combinations. Let me see how smart it becomes.

> > > And even if all QCOM device trees manage to have external regulators
> > > described in their device trees, there are certainly other systems where
> > > the driver might (optionally) use pwrctrl for some devices, but others
> > > will establish power on their own (e.g., PCIe tied to some other system
> > > power rail).
> > > 
> > > I think you either need the PCI core to tell you whether pwrctrl is in
> > > use, or else you need to disconnect this PERST# support from pwrctrl.
> > > 
> > 
> > It is not straightforward for the PCI core to tell whether pwrctrl is in use or
> > not.
> 
> Yes, well this seems like a fundamental recurring problem at the root
> here. This agnostic design just causes more problems, IMO.
> 
> > pwrctrl has no devicetree representation as it is not a separate hardware
> > entity. It just reuses the PCI device DT node. So I used the -supply properties
> > to assume that the pwrctrl device will be used. And almost none of the upstream
> > DTS has -supply properties in the PCI child node (only exception is brcmstb
> > where they define -supply properties in the PCI child node, but only in the DT
> > binding). So that added up.
> 
> You gotta work off DT bindings and schema to make assertions. You can't
> just guess based on in-tree device trees, and so you can't prove
> non-existence, if it's not explicit in the bindings.
> 

This is the actual problem here. We cannot introduce any changes in the binding
for the sake of pwrctrl. Pwrctrl is just a kernel framework which is supposed to
work with the existing bindings and DTS. For sure we can ammend the binding to
make it strict. Like, mandating the -supply property even if it is always on as
the endpoint definitely needs atleast one supply from the host (well from the
system PMIC atleast). But nothing more.

- Mani

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

Hi,

On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> Since the Qcom platforms rely on pwrctrl framework to control the power
> supplies, allow it to control PERST# also. PERST# should be toggled during
> the power-on and power-off scenarios.
> 
> But the controller driver still need to assert PERST# during the controller
> initialization. So only skip the deassert if pwrctrl usage is detected. The
> pwrctrl framework will deassert PERST# after turning on the supplies.
> 
> The usage of pwrctrl framework is detected based on the new DT binding
> i.e., with the presence of PERST# and PHY properties in the Root Port node
> instead of the host bridge node.
> 
> When the legacy binding is used, PERST# is only controlled by the
> controller driver since it is not reliable to detect whether pwrctrl is
> used or not. So the legacy platforms are untouched by this commit.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

> @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  	if (ret)
>  		return ret;
>  
> +	devfn = of_pci_get_devfn(node);
> +	if (devfn < 0)
> +		return -ENOENT;
> +
> +	pp->perst[PCI_SLOT(devfn)] = reset;

It seems like you assume a well-written device tree, such that this
PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
should guard against that somehow.

Also see my comment below, where I believe even a well-written device
tree could trip this up.

> +
>  	port->reset = reset;
>  	port->phy = phy;
>  	INIT_LIST_HEAD(&port->list);
> @@ -1734,10 +1746,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  
>  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
>  	struct device *dev = pcie->pci->dev;
>  	struct qcom_pcie_port *port, *tmp;
> +	int child_cnt;
>  	int ret = -ENOENT;
>  
> +	child_cnt = of_get_available_child_count(dev->of_node);

I think you're assuming "available children" correlate precisely with a
0-indexed array of ports. But what if, e.g., port 0 is disabled in the
device tree, and only port 1 is available? Then you'll overflow.

> +	if (!child_cnt)
> +		return ret;
> +
> +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);

IIUC, you kfree() this on error, but otherwise, you never free it. I
also see that this driver can't actually be unbound (commit f9a666008338
("PCI: qcom: Make explicitly non-modular")), so technically there's no
way to "leak" this other than by probe errors...
...but it still seems like devm_*() would fit better.

(NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
doesn't have a sensible use case anyway". That just sounds like
laziness. And it *can* have a useful purpose for testing.)

Brian

> +	if (!pp->perst)
> +		return -ENOMEM;
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)

On Tue, Jul 08, 2025 at 08:18:06PM GMT, Brian Norris wrote:
> Hi,
> 
> On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > Since the Qcom platforms rely on pwrctrl framework to control the power
> > supplies, allow it to control PERST# also. PERST# should be toggled during
> > the power-on and power-off scenarios.
> > 
> > But the controller driver still need to assert PERST# during the controller
> > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > pwrctrl framework will deassert PERST# after turning on the supplies.
> > 
> > The usage of pwrctrl framework is detected based on the new DT binding
> > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > instead of the host bridge node.
> > 
> > When the legacy binding is used, PERST# is only controlled by the
> > controller driver since it is not reliable to detect whether pwrctrl is
> > used or not. So the legacy platforms are untouched by this commit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> 
> > @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  	if (ret)
> >  		return ret;
> >  
> > +	devfn = of_pci_get_devfn(node);
> > +	if (devfn < 0)
> > +		return -ENOENT;
> > +
> > +	pp->perst[PCI_SLOT(devfn)] = reset;
> 
> It seems like you assume a well-written device tree, such that this
> PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
> should guard against that somehow.
> 

Sure. I will add a check.

> Also see my comment below, where I believe even a well-written device
> tree could trip this up.
> 
> > +
> >  	port->reset = reset;
> >  	port->phy = phy;
> >  	INIT_LIST_HEAD(&port->list);
> > @@ -1734,10 +1746,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  
> >  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> >  	struct device *dev = pcie->pci->dev;
> >  	struct qcom_pcie_port *port, *tmp;
> > +	int child_cnt;
> >  	int ret = -ENOENT;
> >  
> > +	child_cnt = of_get_available_child_count(dev->of_node);
> 
> I think you're assuming "available children" correlate precisely with a
> 0-indexed array of ports. But what if, e.g., port 0 is disabled in the
> device tree, and only port 1 is available? Then you'll overflow.
> 

Right. I will take care it in next version.

> > +	if (!child_cnt)
> > +		return ret;
> > +
> > +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> 
> IIUC, you kfree() this on error, but otherwise, you never free it. I
> also see that this driver can't actually be unbound (commit f9a666008338
> ("PCI: qcom: Make explicitly non-modular")), so technically there's no
> way to "leak" this other than by probe errors...
> ...but it still seems like devm_*() would fit better.
> 

Even if we use devm_(), we need to free the array when qcom_pcie_parse_port()
fails. And as you spotted, we don't remove the driver currently. So I decided to
use kfree(). Someone would argue that if we manually free the memory, then it
defeats the purpose of devm_() variants.

> (NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
> doesn't have a sensible use case anyway". That just sounds like
> laziness. And it *can* have a useful purpose for testing.)
> 

There was a whole debate on it. It is mostly due to the fact that this driver
implements the MSI controller and the IRQCHIP drivers are not expected to go
away during runtime. But atleast, I would like to build this driver as a module
and not remove it. The patch is pending for some time.

- Mani

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