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

Christian Bruel
2024-12-18T11:28:28Z

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
>