On Mon, Apr 14, 2025 at 11:09:14AM +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 controller node itself. This is incorrect, as
> properties like phy, perst, wake, etc. can vary per port and should be
> present in the root port node.
>
Mention the fact that you are preserving DT backwards compatibility by
continuing to support older DTs which stuff these properties in controller node.
> 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 | 149 +++++++++++++++++++++++++++------
> 1 file changed, 123 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index dc98ae63362db0422384b1879a2b9a7dc564d091..5566c8aa7f9a9928c06aa6284ca4de21cc411874 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -262,6 +262,11 @@ struct qcom_pcie_cfg {
> bool no_l0s;
> };
>
> +struct qcom_pcie_port {
> + struct list_head list;
> + struct gpio_desc *reset;
> + struct phy *phy;
> +};
> struct qcom_pcie {
> struct dw_pcie *pci;
> void __iomem *parf; /* DT parf */
> @@ -276,21 +281,36 @@ struct qcom_pcie {
> struct dentry *debugfs;
> bool suspended;
> bool use_pm_opp;
> + struct list_head ports;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> - gpiod_set_value_cansleep(pcie->reset, 1);
> + struct qcom_pcie_port *port, *tmp;
> +
> + if (list_empty(&pcie->ports))
> + gpiod_set_value_cansleep(pcie->reset, 1);
> + else
> + list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> + gpiod_set_value_cansleep(port->reset, 1);
> +
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> {
> + struct qcom_pcie_port *port, *tmp;
> +
> /* Ensure that PERST has been asserted for at least 100 ms */
> msleep(100);
> - gpiod_set_value_cansleep(pcie->reset, 0);
> + if (list_empty(&pcie->ports))
> + gpiod_set_value_cansleep(pcie->reset, 0);
> + else
> + list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> + gpiod_set_value_cansleep(port->reset, 0);
Looks like you can use a helper here (for both assert and deassert).
> +
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> @@ -1229,10 +1249,19 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
> return !!(val & PCI_EXP_LNKSTA_DLLLA);
> }
>
> +static void qcom_pcie_port_phy_off(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_port *port, *tmp;
> +
> + list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> + phy_power_off(port->phy);
> +}
> +
> static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
> + struct qcom_pcie_port *port, *tmp;
> int ret;
>
> qcom_ep_reset_assert(pcie);
> @@ -1241,13 +1270,27 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> if (ret)
> return ret;
>
> - ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> - if (ret)
> - goto err_deinit;
> + if (list_empty(&pcie->ports)) {
> + ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> + if (ret)
> + goto err_deinit;
>
> - ret = phy_power_on(pcie->phy);
> - if (ret)
> - goto err_deinit;
> + ret = phy_power_on(pcie->phy);
> + if (ret)
> + goto err_deinit;
> + } else {
> + list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> + ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> + if (ret)
> + goto err_deinit;
> +
> + ret = phy_power_on(port->phy);
> + if (ret) {
> + qcom_pcie_port_phy_off(pcie);
> + goto err_deinit;
> + }
> + }
> + }
Again, you should consider introducing helpers wherever both multiport and
legacy methods are used. This will avoid sprinkling the list_empty() checks all
over the place.
>
> if (pcie->cfg->ops->post_init) {
> ret = pcie->cfg->ops->post_init(pcie);
> @@ -1268,7 +1311,10 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> err_assert_reset:
> qcom_ep_reset_assert(pcie);
> err_disable_phy:
> - phy_power_off(pcie->phy);
> + if (list_empty(&pcie->ports))
> + phy_power_off(pcie->phy);
> + else
> + qcom_pcie_port_phy_off(pcie);
> err_deinit:
> pcie->cfg->ops->deinit(pcie);
>
> @@ -1281,7 +1327,10 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
>
> qcom_ep_reset_assert(pcie);
> - phy_power_off(pcie->phy);
> + if (list_empty(&pcie->ports))
> + phy_power_off(pcie->phy);
> + else
> + qcom_pcie_port_phy_off(pcie);
> pcie->cfg->ops->deinit(pcie);
> }
>
> @@ -1579,11 +1628,41 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
> +{
> + struct device *dev = pcie->pci->dev;
> + struct qcom_pcie_port *port;
> + struct gpio_desc *reset;
> + struct phy *phy;
> +
> + reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> + "reset", GPIOD_OUT_HIGH, "PERST#");
> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
> +
> + phy = devm_of_phy_get(dev, node, NULL);
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + port->reset = reset;
> + port->phy = phy;
> + INIT_LIST_HEAD(&port->list);
> + list_add_tail(&port->list, &pcie->ports);
> +
> + return 0;
> +}
> +
> static int qcom_pcie_probe(struct platform_device *pdev)
> {
> const struct qcom_pcie_cfg *pcie_cfg;
> unsigned long max_freq = ULONG_MAX;
> + struct qcom_pcie_port *port, *tmp;
> struct device *dev = &pdev->dev;
> + struct device_node *of_port;
> struct dev_pm_opp *opp;
> struct qcom_pcie *pcie;
> struct dw_pcie_rp *pp;
> @@ -1611,6 +1690,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_pm_runtime_put;
>
> + INIT_LIST_HEAD(&pcie->ports);
> +
> pci->dev = dev;
> pci->ops = &dw_pcie_ops;
> pp = &pci->pp;
> @@ -1619,12 +1700,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>
> pcie->cfg = pcie_cfg;
>
> - pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> - if (IS_ERR(pcie->reset)) {
> - ret = PTR_ERR(pcie->reset);
> - goto err_pm_runtime_put;
> - }
> -
> pcie->parf = devm_platform_ioremap_resource_byname(pdev, "parf");
> if (IS_ERR(pcie->parf)) {
> ret = PTR_ERR(pcie->parf);
> @@ -1647,12 +1722,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> }
> }
>
> - pcie->phy = devm_phy_optional_get(dev, "pciephy");
> - if (IS_ERR(pcie->phy)) {
> - ret = PTR_ERR(pcie->phy);
> - goto err_pm_runtime_put;
> - }
> -
> /* OPP table is optional */
> ret = devm_pm_opp_of_add_table(dev);
> if (ret && ret != -ENODEV) {
> @@ -1699,9 +1768,31 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>
> pp->ops = &qcom_pcie_dw_ops;
>
> - ret = phy_init(pcie->phy);
> - if (ret)
> - goto err_pm_runtime_put;
> + for_each_child_of_node(dev->of_node, of_port) {
I think we should just iterate over enabled nodes instead of disabled ones also.
So use 'for_each_available_child_of_node'.
> + ret = qcom_pcie_parse_port(pcie, of_port);
> + of_node_put(of_port);
> + if (ret)
> + break;
> + }
> +
> + /* Fallback to previous method */
/*
* In the case of failure in parsing the port nodes, fallback to the
* legacy method of parsing the controller node. This is to maintain DT
* backwards compatibility.
*/
- Mani
--
மணிவண்ணன் சதாசிவம்
