Re: [PATCH v12 2/9] PCI: stm32: Add PCIe host support for STM32MP25

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Linus]

On Mon, Aug 18, 2025 at 12:50:19PM +0200, Christian Bruel wrote:
> On 8/13/25 21:29, Bjorn Helgaas wrote:
> > On Tue, Jun 10, 2025 at 11:07:07AM +0200, Christian Bruel wrote:
> > > Add driver for the STM32MP25 SoC PCIe Gen1 2.5 GT/s and Gen2 5GT/s
> > > controller based on the DesignWare PCIe core.
> > > ...

> > > +static int stm32_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> > > +	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.
> > 
> > What happens on initial probe if no device is present?  I assume we
> > access DBI registers in the dw_pcie_host_init() path, and it seems
> > like we'd have the same issue with DBI not being accessible when no
> > device is present.
> 
> Correct, same issue. The magic happens with the PINCTRL init state that is
> automatically called before probe. As I tried to explain in the
> Documentation in the pinctrl patch:
> 
> - if ``init`` and ``default`` are defined in the device tree, the "init"
>   state is selected before the driver probe and the "default" state is
>   selected after the driver probe.

OK, thanks for that reminder!

The fact that the pinctrl init state is set implicitly before .probe(),
but we have to do it explicitly in .stm32_pcie_resume_noirq() seems a
*little* bit weird and makes the driver harder to review.  But ... I
guess that's out of scope for this series.

I see that Linus has given his approval to merge the new
pinctrl_pm_select_init_state() along with this series.  Would you mind
pulling those changes [1] and the use [2] into a v13 of this series?
That way I won't have to collect up all the pieces and risk missing
something.

BTW, I noticed a s/platformm/platform/ typo in the "[PATCH v1 2/2]
pinctrl: Add pinctrl_pm_select_init_state helper function" patch.

> > > +	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;
> > > +	}

> > > +static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie)
> > > +{
> > > +	struct device *dev = stm32_pcie->pci.dev;
> > > +	unsigned int wake_irq;
> > > +	int ret;
> > > +
> > > +	/* Start to enable resources with PERST# asserted */
> > 
> > I guess if device tree doesn't describe PERST#, we assume PERST# is
> > actually *deasserted* already at this point (because
> > stm32_pcie_deassert_perst() does nothing other than the delay)?
> 
> yes, this also implies that PV is stable

Maybe we could update the comment somehow?  Or maybe even just remove
it, since we actually don't know the state of PERST# at this point?

It sounds like we don't know the PERST# state until after we call
stm32_pcie_deassert_perst() below.

> > > +	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;
> > > +
> > > +	stm32_pcie_deassert_perst(stm32_pcie);

Bjorn

[1] https://lore.kernel.org/r/20250813081139.93201-1-christian.bruel@foss.st.com
[2] https://lore.kernel.org/r/20250813115319.212721-1-christian.bruel@foss.st.com