Re: [PATCH v4 03/10] PCI: stm32: Add PCIe host support for STM32MP25

[Manivannan Sadhasivam]

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

[...]

> > > +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,
> 
> > 
> > > +	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
> 
> > 
> > > +
> > > +	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.

> > 
> > > +	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
> 
> > 
> > > +
> > > +	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().

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

> > 
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +

[...]

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

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