On Tue, Feb 04, 2025 at 05:23:05PM +0100, Christian Bruel wrote:

[...]

quoted

quoted

+static int stm32_pcie_suspend_noirq(struct device *dev)
+{

Can you consider making use of dw_pcie_{suspend/resume}_noirq()?

I considered this, but dw_pcie_suspend_noirq needs to be tweaked as it
checks both the pme_turn_off hook and whether we are entering into L2, which
we don't support.

For the former, I can check the PCI_EXP_DEVSTAT_AUXPD capability before
polling for L2 LTSSM. It looks like only the Layerscape platform uses this.
I will need a Tested-by for this new dw_pcie_suspend_noirq.

Do you advise keeping stm32_pcie_suspend_noirq or modifying
dw_pcie_suspend_noirq to this effect?

Please modify dw_pcie_suspend_noirq() to fit your needs. We will review the
change.

For testing the change, you can CC the Layerscape maintainer and request for
testing.

Thanks,

quoted

quoted

+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+	stm32_pcie_stop_link(&stm32_pcie->pci);
+
+	stm32_pcie_assert_perst(stm32_pcie);
+
+	clk_disable_unprepare(stm32_pcie->clk);
+
+	if (!device_may_wakeup(dev))
+		phy_exit(stm32_pcie->phy);
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int stm32_pcie_resume_noirq(struct device *dev)
+{
+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+	struct dw_pcie_rp *pp = &stm32_pcie->pci.pp;
+	int ret;
+
+	/*
+	 * The core clock is gated with CLKREQ# from the COMBOPHY REFCLK,
+	 * thus if no device is present, must force it low with an init pinmux
+	 * to be able to access the DBI registers.
+	 */
+	if (!IS_ERR(dev->pins->init_state))
+		ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
+	else
+		ret = pinctrl_pm_select_default_state(dev);
+
+	if (ret) {
+		dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
+		return ret;
+	}
+
+	if (!device_may_wakeup(dev)) {
+		ret = phy_init(stm32_pcie->phy);
+		if (ret) {
+			pinctrl_pm_select_default_state(dev);
+			return ret;
+		}
+	}
+
+	ret = clk_prepare_enable(stm32_pcie->clk);
+	if (ret)
+		goto err_phy_exit;
+
+	stm32_pcie_deassert_perst(stm32_pcie);
+
+	ret = dw_pcie_setup_rc(pp);
+	if (ret)
+		goto err_disable_clk;
+
+	ret = stm32_pcie_start_link(&stm32_pcie->pci);
+	if (ret)
+		goto err_disable_clk;
+
+	/* Ignore errors, the link may come up later */
+	dw_pcie_wait_for_link(&stm32_pcie->pci);

These can be dropped when using dw_pcie_resume_noirq().

OK for dw_pcie_resume_noirq if we can keep it balanced with
dw_pcie_suspend_noirq

quoted

quoted

+
+	pinctrl_pm_select_default_state(dev);
+
+	return 0;
+
+err_disable_clk:
+	stm32_pcie_assert_perst(stm32_pcie);
+	clk_disable_unprepare(stm32_pcie->clk);
+
+err_phy_exit:
+	phy_exit(stm32_pcie->phy);
+	pinctrl_pm_select_default_state(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops stm32_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend_noirq,
+				  stm32_pcie_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend, stm32_pcie_resume)
+};
+
+static const struct dw_pcie_host_ops stm32_pcie_host_ops = {
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = stm32_pcie_start_link,
+	.stop_link = stm32_pcie_stop_link
+};
+
+static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
+			       struct platform_device *pdev)
+{
+	struct device *dev = stm32_pcie->pci.dev;
+	struct dw_pcie_rp *pp = &stm32_pcie->pci.pp;
+	int ret;
+

You need to assert PERST# before configuring the resources.

It is already initialized to GPIOD_OUT_HIGH in gpiod_get, I can have an
explicit stm32_pcie_assert_perst but is it necessary ?

Ok, not necessary. Although, I would recommend to keep a comment here so that if
someone refactors the code, they would notice it.

quoted

quoted

+	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
+	if (ret)
+		return ret;
+
+	ret = phy_init(stm32_pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+				 STM32MP25_PCIECR_TYPE_MASK,
+				 STM32MP25_PCIECR_RC);
+	if (ret)
+		goto err_phy_exit;
+
+	reset_control_assert(stm32_pcie->rst);
+	reset_control_deassert(stm32_pcie->rst);
+
+	ret = clk_prepare_enable(stm32_pcie->clk);
+	if (ret) {
+		dev_err(dev, "Core clock enable failed %d\n", ret);
+		goto err_phy_exit;
+	}
+
+	stm32_pcie_deassert_perst(stm32_pcie);
+
+	pp->ops = &stm32_pcie_host_ops;
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "Failed to initialize host: %d\n", ret);
+		goto err_disable_clk;
+	}

Technically, dw_pcie_host_init() is not related to root port. So please move it
to probe() instead.

OK will move, thanks

quoted

quoted

+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(stm32_pcie->clk);
+	stm32_pcie_assert_perst(stm32_pcie);
+
+err_phy_exit:
+	phy_exit(stm32_pcie->phy);
+
+	return ret;
+}
+
+static int stm32_pcie_parse_port(struct stm32_pcie *stm32_pcie)
+{
+	struct device *dev = stm32_pcie->pci.dev;
+	struct device_node *root_port;
+
+	root_port = of_get_next_available_child(dev->of_node, NULL);
+
+	stm32_pcie->phy = devm_of_phy_get(dev, root_port, NULL);
+	if (IS_ERR(stm32_pcie->phy))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->phy),
+				     "Failed to get pcie-phy\n");

OF refcount not decremented in both the error and success case.

I don't understand your point, isn't devm_of_phy_get managed to decrement
the phy resources ?

You should drop the refcount of the root_port using of_node_put().

quoted

quoted

+
+	stm32_pcie->perst_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(root_port),
+						       "reset", GPIOD_OUT_HIGH, NULL);
+	if (IS_ERR(stm32_pcie->perst_gpio)) {
+		if (PTR_ERR(stm32_pcie->perst_gpio) != -ENOENT)
+			return dev_err_probe(dev, PTR_ERR(stm32_pcie->perst_gpio),
+					     "Failed to get reset GPIO\n");
+		stm32_pcie->perst_gpio = NULL;
+	}
+
+	if (device_property_read_bool(dev, "wakeup-source")) {

As per the current logic, 'wakeup-source' is applicable even without WAKE# GPIO,
which doesn't make sense.

Agree, wakeup-source is not needed

quoted

quoted

+		stm32_pcie->wake_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(root_port),
+							      "wake", GPIOD_IN, NULL);
+
+		if (IS_ERR(stm32_pcie->wake_gpio)) {
+			if (PTR_ERR(stm32_pcie->wake_gpio) != -ENOENT)
+				return dev_err_probe(dev, PTR_ERR(stm32_pcie->wake_gpio),
+						     "Failed to get wake GPIO\n");
+			stm32_pcie->wake_gpio = NULL;
+		}

Hmm. I think we need to move WAKE# handling inside drivers/pci/pcie/portdrv.c
since that is responsible for the root port. While other root port properties
have some dependency with the RC (like PERST#, PHY etc...), WAKE# handling could
be moved safel >
And once done, it can benefit all platforms.

OK I'll check if there is a convenient way to do this through a port_service

You can drop the WAKE# support altogether and add it in the subsequent patches
once this initial driver gets merged. It is up to you.

quoted

quoted

+	}
+
+	return 0;
+}
+

[...]

quoted

You can just move these definitions inside the driver itself.

Some definitions will be duplicated with the ep driver, but on the other
side this file is very small... is it OK to duplicate definitions instead of
having the bitfields together ?

I didn't notice that these are reused by the ep driver. Fine with me.

- Mani

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