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

[Christian Bruel <christian.bruel@foss.st.com>]

On 5/15/25 13:29, Manivannan Sadhasivam wrote:
> On Mon, May 12, 2025 at 05:08:13PM +0200, Christian Bruel wrote:
>> Hi Manivannan,
>>
>> On 4/30/25 09:30, Manivannan Sadhasivam wrote:
>>> On Wed, Apr 23, 2025 at 11:01:12AM +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.
>>>>
>>>> Supports MSI via GICv2m, Single Virtual Channel, Single Function
>>>>
>>>> Supports WAKE# GPIO.
>>>>
>>>
>>> Mostly looks good. Just a couple of comments below.
>>>
>>>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>>>> ---
>>>>    drivers/pci/controller/dwc/Kconfig      |  12 +
>>>>    drivers/pci/controller/dwc/Makefile     |   1 +
>>>>    drivers/pci/controller/dwc/pcie-stm32.c | 368 ++++++++++++++++++++++++
>>>>    drivers/pci/controller/dwc/pcie-stm32.h |  15 +
>>>>    4 files changed, 396 insertions(+)
>>>>    create mode 100644 drivers/pci/controller/dwc/pcie-stm32.c
>>>>    create mode 100644 drivers/pci/controller/dwc/pcie-stm32.h
>>>>
>>>
>>> [...]
>>>
>>>> +static int stm32_pcie_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct stm32_pcie *stm32_pcie;
>>>> +	struct device *dev = &pdev->dev;
>>>> +	int ret;
>>>> +
>>>> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
>>>> +	if (!stm32_pcie)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	stm32_pcie->pci.dev = dev;
>>>> +	stm32_pcie->pci.ops = &dw_pcie_ops;
>>>> +	stm32_pcie->pci.pp.ops = &stm32_pcie_host_ops;
>>>> +
>>>> +	stm32_pcie->regmap = syscon_regmap_lookup_by_compatible("st,stm32mp25-syscfg");
>>>> +	if (IS_ERR(stm32_pcie->regmap))
>>>> +		return dev_err_probe(dev, PTR_ERR(stm32_pcie->regmap),
>>>> +				     "No syscfg specified\n");
>>>> +
>>>> +	stm32_pcie->clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(stm32_pcie->clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(stm32_pcie->clk),
>>>> +				     "Failed to get PCIe clock source\n");
>>>> +
>>>> +	stm32_pcie->rst = devm_reset_control_get_exclusive(dev, NULL);
>>>> +	if (IS_ERR(stm32_pcie->rst))
>>>> +		return dev_err_probe(dev, PTR_ERR(stm32_pcie->rst),
>>>> +				     "Failed to get PCIe reset\n");
>>>> +
>>>> +	ret = stm32_pcie_parse_port(stm32_pcie);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	platform_set_drvdata(pdev, stm32_pcie);
>>>> +
>>>> +	ret = pm_runtime_set_active(dev);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "Failed to activate runtime PM %d\n", ret);
>>>
>>> Please use dev_err_probe() here and below.
>>
>> OK, will report this in the EP driver also.
>>
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = devm_pm_runtime_enable(dev);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "Failed to enable runtime PM %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	pm_runtime_get_noresume(dev);
>>>> +
>>>
>>> I know that a lot of the controller drivers do this for no obvious reason. But
>>> in this case, I believe you want to enable power domain or genpd before
>>> registering the host bridge. Is that right?
>>
>> We call pm_runtime_enable() before stm32_add_pcie_port() and
>> dw_pcie_host_init(). This ensures that PCIe is active during the PERST#
>> sequence and before accessing the DWC registers.
>>
> 
> What do you mean by 'PCIe is active'? Who is activating it other than this
> driver?

"PCIe is active" in the sense of pm_runtime_active() and PM runtime_enabled.

A better call point would be just before dw_host_init(), after the PCIe 
controller is reset :

stm32_add_pcie_port()
clk_prepare_enable()
devm_pm_runtime_enable()
dw_pcie_host_init()

with this sequence, the stm32_pcie_suspend_noirq() is well balanced. 
does that sound better ?

> 
>>> And the fact that you are not
>>> decrementing the runtime usage count means, you want to keep it ON all the time?
>>> Beware that your system suspend/resume calls would never get executed.
>>
>> We do not support PM runtime autosuspend, so we must notify PM runtime that
>> PCIe is always active. Without invoking pm_runtime_get_noresume(), PCIe
>> would mistakenly be marked as suspended.
> 
> This cannot happen unless the child devices are also suspended? Or if there are
> no child devices connected.

If no device is connected or if one is active, without 
pm_runtime_get_noresume(), pm_genpd_summary says "PCIe suspended" 
despite being clocked and having accessible configuration space

thank you,

Christian

> 
> - Mani
>