Re: [PATCH v5 2/2] PCI: qcom: Add support for multi-root port

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jul 02, 2025 at 04:50:42PM +0530, Krishna Chaitanya Chundru wrote:
> Move phy, PERST# handling to root port and provide a way to have multi-port
> logic.
> 
> Currently, QCOM controllers only support single port, and all properties
> are present in the host bridge node itself. This is incorrect, as
> properties like phys, perst-gpios, etc.. can vary per port and should be
> present in the root port node.
> 
> To maintain DT backwards compatibility, fallback to the legacy method of
> parsing the host bridge node if the port parsing fails.
> 
> pci-bus-common.yaml uses reset-gpios property for representing PERST#, use
> same property instead of perst-gpios.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 178 ++++++++++++++++++++++++++++-----
>  1 file changed, 151 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index f7ed1e010eb6607b2e98a42f0051c47e4de2af93..56d04a15edf8f99f6d3b9bfaa037ff922b521888 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -267,6 +267,12 @@ struct qcom_pcie_cfg {
>  	bool no_l0s;
>  };
>  
> +struct qcom_pcie_port {
> +	struct list_head list;
> +	struct gpio_desc *reset;
> +	struct phy *phy;

This change is already upstream (a2fbecdbbb9d ("PCI: qcom: Add support
for parsing the new Root Port binding")), but it seems wrong to me to
have "phy" and "reset" in both struct qcom_pcie and struct
qcom_pcie_port.  

I know we need *find* those things in different places (either a
per-Root Port DT stanza or the top-level qcom host bridge), but why
can't we always put them in struct qcom_pcie_port and drop them from
struct qcom_pcie?

Having them in both places means all the users need to worry about
that DT difference and look in both places instead of always looking
at qcom_pcie_port.

> +};
> +
>  struct qcom_pcie {
>  	struct dw_pcie *pci;
>  	void __iomem *parf;			/* DT parf */
> @@ -279,24 +285,37 @@ struct qcom_pcie {
>  	struct icc_path *icc_cpu;
>  	const struct qcom_pcie_cfg *cfg;
>  	struct dentry *debugfs;
> +	struct list_head ports;
>  	bool suspended;
>  	bool use_pm_opp;
>  };

> +static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  {
> -	gpiod_set_value_cansleep(pcie->reset, 1);
> +	struct qcom_pcie_port *port;
> +	int val = assert ? 1 : 0;
> +
> +	if (list_empty(&pcie->ports))
> +		gpiod_set_value_cansleep(pcie->reset, val);
> +	else
> +		list_for_each_entry(port, &pcie->ports, list)
> +			gpiod_set_value_cansleep(port->reset, val);

This is the kind of complication I think we should avoid.

> +static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port;
> +
> +	if (list_empty(&pcie->ports))
> +		phy_exit(pcie->phy);
> +	else
> +		list_for_each_entry(port, &pcie->ports, list)
> +			phy_exit(port->phy);

And this.

> +}
> +
> +static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port;
> +
> +	if (list_empty(&pcie->ports)) {
> +		phy_power_off(pcie->phy);
> +	} else {
> +		list_for_each_entry(port, &pcie->ports, list)
> +			phy_power_off(port->phy);

And this.  And there's more.

Bjorn