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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250908183325.GA1450728@bhelgaas>
Date: Mon, 8 Sep 2025 13:33:25 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	cros-qcom-dts-watchers@...omium.org, linux-arm-msm@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, quic_vbadigan@...cinc.com,
	quic_mrana@...cinc.com
Subject: Re: [PATCH v5 2/2] PCI: qcom: Add support for multi-root port

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@....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