On 12/18/24 10:46, Manivannan Sadhasivam wrote:
On Mon, Dec 16, 2024 at 10:00:27AM +0100, Christian Bruel wrote:
[...]
+ msleep(PCIE_T_RRS_READY_MS);
+
+ return ret;
+}
+
+static void stm32_pcie_stop_link(struct dw_pcie *pci)
+{
+ struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+
+ regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+ STM32MP25_PCIECR_LTSSM_EN, 0);
+
+ /* Assert PERST# */
+ if (stm32_pcie->perst_gpio)
+ gpiod_set_value(stm32_pcie->perst_gpio, 1);
I don't like tying PERST# handling with start/stop link. PERST# should be
handled based on the power/clock state.
I don't understand your point: We turn off the PHY in suspend_noirq(), so
that seems a logical place to de-assert in resume_noirq after the refclk is
ready. PERST# should be kept active until the PHY stablilizes the clock in
resume. From the PCIe electromechanical specs, PERST# goes active while the
refclk is not stable/
While your understanding about PERST# is correct, your implementation is not.
You are toggling PERST# from start/stop link callbacks which are supposed to
control the LTSSM state only. I don't have issues with toggling PERST# in
stm32_pcie_{suspend/resume}_noirq().
Ah OK. this function is split now into 2 functional blocks in the
upcoming version
+}
+
+static int stm32_pcie_suspend(struct device *dev)
+{
+ struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev) || device_wakeup_path(dev))
+ enable_irq_wake(stm32_pcie->wake_irq);
+
+ return 0;
+}
+
+static int stm32_pcie_resume(struct device *dev)
+{
+ struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev) || device_wakeup_path(dev))
+ disable_irq_wake(stm32_pcie->wake_irq);
+
+ return 0;
+}
+
+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#.
+ 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.
thanks,
Christian
- Mani
