Message-ID: <tdgyva6qyn6qwzvft4f7r3tgp5qswuv4q5swoaeomnnbxtmz5j@zo3gvevx2skp>
Date: Wed, 30 Apr 2025 13:20:06 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: lpieralisi@...nel.org, kw@...ux.com, 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, 
	thippeswamy.havalige@....com, shradha.t@...sung.com, quic_schintav@...cinc.com, 
	cassel@...nel.org, johan+linaro@...nel.org, 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
Subject: Re: [PATCH v8 4/9] PCI: stm32: Add PCIe Endpoint support for
 STM32MP25

On Wed, Apr 23, 2025 at 11:01:14AM +0200, Christian Bruel wrote:
> Add driver to configure the STM32MP25 SoC PCIe Gen1 2.5GT/s or Gen2 5GT/s
> controller based on the DesignWare PCIe core in endpoint mode.
> 
> Uses the common reference clock provided by the host.
> 
> The PCIe core_clk receives the pipe0_clk from the ComboPHY as input,
> and the ComboPHY PLL must be locked for pipe0_clk to be ready.
> Consequently, PCIe core registers cannot be accessed until the ComboPHY is
> fully initialised and refclk is enabled and ready.
> 
> Signed-off-by: Christian Bruel <christian.bruel@...s.st.com>
> ---
>  drivers/pci/controller/dwc/Kconfig         |  12 +
>  drivers/pci/controller/dwc/Makefile        |   1 +
>  drivers/pci/controller/dwc/pcie-stm32-ep.c | 414 +++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-stm32.h    |   1 +
>  4 files changed, 428 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-stm32-ep.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 2aec5d2f9a46..aceff7d1ef33 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -422,6 +422,18 @@ config PCIE_STM32_HOST
>  	  This driver can also be built as a module. If so, the module
>  	  will be called pcie-stm32.
>  
> +config PCIE_STM32_EP
> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (endpoint mode)"
> +	depends on ARCH_STM32 || COMPILE_TEST
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Enables endpoint support for DesignWare core based PCIe controller
> +	  found in STM32MP25 SoC.

Can you please use similar description for the RC driver also?

"Enables Root Complex (RC) support for the DesignWare core based PCIe host
controller found in STM32MP25 SoC."

> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called pcie-stm32-ep.
> +
>  config PCI_DRA7XX
>  	tristate
>  

[...]

> +static int stm32_add_pcie_ep(struct stm32_pcie *stm32_pcie,
> +			     struct platform_device *pdev)
> +{
> +	struct dw_pcie_ep *ep = &stm32_pcie->pci.ep;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);

This needs to be called before devm_pm_runtime_enable().

> +	if (ret < 0) {
> +		dev_err(dev, "pm runtime resume failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +				 STM32MP25_PCIECR_TYPE_MASK,
> +				 STM32MP25_PCIECR_EP);
> +	if (ret) {
> +		goto err_pm_put_sync;
> +		return ret;
> +	}
> +
> +	reset_control_assert(stm32_pcie->rst);
> +	reset_control_deassert(stm32_pcie->rst);
> +
> +	ep->ops = &stm32_pcie_ep_ops;
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize ep: %d\n", ret);
> +		goto err_pm_put_sync;
> +	}
> +
> +	ret = stm32_pcie_enable_resources(stm32_pcie);
> +	if (ret) {
> +		dev_err(dev, "failed to enable resources: %d\n", ret);
> +		goto err_ep_deinit;
> +	}
> +
> +	ret = dw_pcie_ep_init_registers(ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
> +		goto err_disable_resources;
> +	}
> +
> +	pci_epc_init_notify(ep->epc);
> +

Hmm, looks like you need to duplicate dw_pcie_ep_init_registers() and
pci_epc_init_notify() in stm32_pcie_perst_deassert() for hw specific reasons.
So can you drop these from there?

> +	return 0;
> +
> +err_disable_resources:
> +	stm32_pcie_disable_resources(stm32_pcie);
> +
> +err_ep_deinit:
> +	dw_pcie_ep_deinit(ep);
> +
> +err_pm_put_sync:
> +	pm_runtime_put_sync(dev);
> +	return ret;
> +}
> +
> +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->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->phy = devm_phy_get(dev, NULL);
> +	if (IS_ERR(stm32_pcie->phy))
> +		return dev_err_probe(dev, PTR_ERR(stm32_pcie->phy),
> +				     "failed to get pcie-phy\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");
> +
> +	stm32_pcie->perst_gpio = devm_gpiod_get(dev, "reset", GPIOD_IN);
> +	if (IS_ERR(stm32_pcie->perst_gpio))
> +		return dev_err_probe(dev, PTR_ERR(stm32_pcie->perst_gpio),
> +				     "Failed to get reset GPIO\n");
> +
> +	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, stm32_pcie);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable pm runtime %d\n", ret);

Use dev_err_probe() please for consistency.

- Mani

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

Powered by blists - more mailing lists