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