Re: [PATCH v2 2/5] PCI: stm32: Add PCIe host support for STM32MP25

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Dec 18, 2024 at 12:24:21PM +0100, Christian Bruel wrote:

[...]

> > > > > +static int stm32_pcie_suspend_noirq(struct device *dev)
> > > > > +{
> > > > > +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > +	stm32_pcie->link_is_up = dw_pcie_link_up(stm32_pcie->pci);
> > > > > +
> > > > > +	stm32_pcie_stop_link(stm32_pcie->pci);
> > > > 
> > > > I don't understand how endpoint can wakeup the host if PERST# gets asserted.
> > > 
> > > The stm32 PCIe doesn't support L2, we don't expect an in-band beacon for the
> > > wakeup. We support wakeup only from sideband WAKE#, that will restart the
> > > link from IRQ
> > > 
> > 
> > I don't understand how WAKE# is supported without L2. Only in L2 state, endpoint
> > will make use of Vaux and it will wakeup the host using either beacon or WAKE#.
> > If you don't support L2, then the endpoint will reach L3 (link off) state.
> 
> I think this is what happens, my understanding is that the device is still
> powered to get the wakeup event (eg WoL magic packet), triggers the PCIe
> wake_IRQ from the WAKE#.
> 

Honestly, I still cannot understand how this can happen.

> > 
> > > > 
> > > > > +	clk_disable_unprepare(stm32_pcie->clk);
> > > > > +
> > > > > +	if (!device_may_wakeup(dev) && !device_wakeup_path(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 *pci = stm32_pcie->pci;
> > > > > +	struct dw_pcie_rp *pp = &pci->pp;
> > > > > +	int ret;
> > > > > +
> > > > > +	/* init_state must be called first to force clk_req# gpio when no
> > > > 
> > > > CLKREQ#
> > > > 
> > > > Why RC should control CLKREQ#?
> > > 
> > > REFCLK is gated with CLKREQ#, So we cannot access the core
> > > without CLKREQ# if no device is present. So force it with a init pinmux
> > > the time to init the PHY and the core DBI registers
> > > 
> > 
> > Ok. You should add a comment to clarify it in the code as this is not a standard
> > behavior.
> > 
> 
> OK
> 
> > > > 
> > > > Also please use preferred style for multi-line comments:
> > > > 
> > > > 	/*
> > > > 	 * ...
> > > > 	 */
> > > > 
> > > > > +	 * device is plugged.
> > > > > +	 */
> > > > > +	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) && !device_wakeup_path(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 clk_err;
> > > > 
> > > > Please name the goto labels of their purpose. Like err_phy_exit.
> > > 
> > > OK
> > > 
> > > > 
> > > > > +
> > > > > +	ret = dw_pcie_setup_rc(pp);
> > > > > +	if (ret)
> > > > > +		goto pcie_err;
> > > > 
> > > > This should be, 'err_disable_clk'.
> > > > 
> > > > > +
> > > > > +	if (stm32_pcie->link_is_up) {
> > > > 
> > > > Why do you need this check? You cannot start the link in the absence of an
> > > > endpoint?
> > > > 
> > > 
> > > It is an optimization to avoid unnecessary "dw_pcie_wait_for_link" if no
> > > device is present during suspend
> > > 
> > 
> > In that case you'll not trigger LTSSM if there was no endpoint connected before
> > suspend. What if users connect an endpoint after resume?
> 
> Yes, exactly. We don't support hotplug, and plugging a device while the
> system is in stand-by is something that we don't expect. The imx6 platform
> does this also.
> 

You should reconsider this approach. You'll never know how the OEMs are going to
use the PCIe slot. And lack of standard hotplug is not preventing users from
doing hotplug (they could try to do rescan themselves etc...)

- Mani

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