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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8ac3ad85-9ab8-4044-9118-69dd78333c45@foss.st.com>
Date: Tue, 19 Aug 2025 15:01:01 +0200
From: Christian Bruel <christian.bruel@...s.st.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <lpieralisi@...nel.org>, <kwilczynski@...nel.org>, <mani@...nel.org>,
        <robh@...nel.org>, <bhelgaas@...gle.com>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <mcoquelin.stm32@...il.com>,
        <alexandre.torgue@...s.st.com>, <p.zabel@...gutronix.de>,
        <johan+linaro@...nel.org>, <cassel@...nel.org>,
        <shradha.t@...sung.com>, <thippeswamy.havalige@....com>,
        <quic_schintav@...cinc.com>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        "Linus Walleij" <linus.walleij@...aro.org>
Subject: Re: [PATCH v12 2/9] PCI: stm32: Add PCIe host support for STM32MP25



On 8/19/25 01:06, Bjorn Helgaas wrote:
> [+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.

sure, I will rebase and re-spin with squashed last bits.

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

thank you,

> 
>>>> +	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() 

If perst_gpio is defined in the DT, PERST# is asserted (GPIOD_OUT_HIGH) 
in stm32_pcie_parse_port().
If it is not, we must assume PERST# was already started de-asserted from 
the environment along with the REFCLK

So locally I agree, we don't know the PERST# state.
I will remove this useless ambiguous comment

Christian

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