Message-ID: <9d3bf191-5dcf-4d7d-8311-0ef491cfdf65@foss.st.com>
Date: Mon, 25 Nov 2024 16:00:52 +0100
From: Christian Bruel <christian.bruel@...s.st.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <lpieralisi@...nel.org>, <kw@...ux.com>,
        <manivannan.sadhasivam@...aro.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>, <cassel@...nel.org>,
        <quic_schintav@...cinc.com>, <fabrice.gasnier@...s.st.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>
Subject: Re: [PATCH 2/5] PCI: stm32: Add PCIe host support for STM32MP25

Hello,

On 11/12/24 20:32, Bjorn Helgaas wrote:
> On Tue, Nov 12, 2024 at 05:19:22PM +0100, Christian Bruel wrote:
>> Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
>> DesignWare PCIe core.
>> Supports MSI via GICv2m, Single Virtual Channel, Single Function
> 
> Add blank lines between paragraphs.  Also applies to other patches in
> the series.
> 
>> +config PCIE_STM32
>> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (host mode)"
>> +	depends on ARCH_STM32 || COMPILE_TEST
>> +	depends on PCI_MSI
>> +	select PCIE_DW_HOST
>> +	help
>> +	  Enables 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.
> 
> Move this so the drivers stay alphabetized.  There's already a
> "STMicroelectronics SPEAr PCIe controller" entry, and this should go
> right next to it.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-stm32.c
> 
>> +static const struct of_device_id stm32_pcie_of_match[] = {
>> +	{ .compatible = "st,stm32mp25-pcie-rc" },
>> +	{},
>> +};
> 
> Most drivers put this near the platform_driver struct that references
> it.
> 
>> +static int stm32_pcie_set_max_payload(struct dw_pcie *pci, int mps)
>> +{
>> +	int ret;
>> +	struct device *dev = pci->dev;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	if (mps != 128 && mps != 256) {
>> +		dev_err(dev, "Unexpected payload size %d\n", mps);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = pcie_set_mps(pdev, mps);
>> +	if (ret)
>> +		dev_err(dev, "failed to set mps %d, error %d\n", mps, ret);
> 
> MPS config is normally not device-specific, and it's somewhat fragile
> (see pci_configure_mps() and pcie_bus_config), so I kind of hate to
> see more users.  Maybe there's some hardware issue involved here?

Indeed that fixed a debugging issue with the first designs.
Not necessary anymore, dropping.

> 
>> +static int stm32_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +	u32 ret;
>> +
>> +	if (stm32_pcie->reset_gpio) {
>> +		/* Make sure PERST# is asserted. */
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);
>> +
>> +		/* Deassert PERST# after 100us */
>> +		usleep_range(100, 200);
> 
> If this is PCIE_T_PERST_CLK_US, use that.  If not, please add a
> relevant #define with a citation to the spec.
> 
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 0);
>> +	}
>> +
>> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +				 STM32MP25_PCIECR_LTSSM_EN,
>> +				 STM32MP25_PCIECR_LTSSM_EN);
>> +
>> +	/*
>> +	 * PCIe specification states that you should not issue any config
>> +	 * requests until 100ms after asserting reset, so we enforce that here
> 
> I think it says 100ms after *deasserting* reset.  But if you use
> PCIE_T_RRS_READY_MS below, I don't think you even need this comment.

ack thanks

> 
>> +	if (stm32_pcie->reset_gpio)
>> +		msleep(100);
> 
> I think this is PCIE_T_RRS_READY_MS.
> 
>> +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);
> 
> With a half-dozen exceptions, this file fits nicely in 80 columns.
> Can you wrap this and the similar exceptions?  No need to break printf
> strings or the regmap strings that can't reasonably be wrapped.
> 
>> +	/* Assert PERST# */
>> +	if (stm32_pcie->reset_gpio)
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);
> 
> Might be nice to include "perst" in the "reset_gpio" name to identify
> it more specifically.

ok

> 
>> +}
>> +
>> +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);
>> +	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 was set in pinctrl_bind_pins() before probe */
>> +	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;
>> +
>> +	ret = stm32_pcie_host_init(pp);
>> +	if (ret)
>> +		goto host_err;
>> +
>> +	ret = dw_pcie_setup_rc(pp);
>> +	if (ret)
>> +		goto pcie_err;
>> +
>> +	if (stm32_pcie->link_is_up) {
>> +		ret = stm32_pcie_start_link(stm32_pcie->pci);
>> +		if (ret)
>> +			goto pcie_err;
>> +
>> +		/* Ignore errors, the link may come up later */
>> +		dw_pcie_wait_for_link(stm32_pcie->pci);
>> +	}
>> +
>> +	pinctrl_pm_select_default_state(dev);
> 
> Interesting that pcie-stm32.c, pci-tegra.c, and pcie-tegra194.c are
> the only PCI controller drivers that use this.  I have no idea what
> this is; it just makes me wonder if these three are just special, or
> if others should be using it?

Here default_state balances with pinctrl_pm_select_sleep_state in 
suspend_noirq. So we should have the same probing sequence:

suspend_noirq()
   pinctrl_pm_select_sleep_state()

resume_noirq()
   pinctrl_select_state(dev->pins->p, dev->pins->init_state);
   ... restart resources and link
   pinctrl_pm_select_default_state()

for the other targets, I have no idea

> 
>> +static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
>> +			       struct platform_device *pdev)
>> +{
>> +	struct dw_pcie *pci = stm32_pcie->pci;
>> +	struct device *dev = pci->dev;
>> +	struct dw_pcie_rp *pp = &pci->pp;
>> +	int ret;
>> +
>> +	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 phy_disable;
>> +
>> +	reset_control_assert(stm32_pcie->rst);
>> +	reset_control_deassert(stm32_pcie->rst);
> 
> Is there any reset pulse width requirement here?

looking at the timing diagram, I don't think so, the transition looks 
quite fast

At the end we will use the reset_control_deassert API to hide this 
mechanism, when the stm32-reset driver is ready

> 
>> +	ret = clk_prepare_enable(stm32_pcie->clk);
>> +	if (ret) {
>> +		dev_err(dev, "Core clock enable failed %d\n", ret);
>> +		goto phy_disable;
>> +	}
>> +
>> +	pp->ops = &stm32_pcie_host_ops;
>> +	ret = dw_pcie_host_init(pp);
>> +	if (ret) {
>> +		dev_err(dev, "failed to initialize host: %d\n", ret);
> 
> Consider making all the messages consistent in terms of sentence
> structure and capitalization.
> 
>> +static int stm32_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct stm32_pcie *stm32_pcie;
>> +	struct dw_pcie *dw;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int ret;
>> +
>> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
>> +	if (!stm32_pcie)
>> +		return -ENOMEM;
>> +
>> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
>> +	if (!dw)
>> +		return -ENOMEM;
> 
> Add blank line.
> 
>> +static struct platform_driver stm32_pcie_driver = {
>> +	.probe = stm32_pcie_probe,
>> +	.remove_new = stm32_pcie_remove,
> 
> Use .remove instead of .remove_new; see 0edb555a65d1 ("platform: Make
> platform_driver::remove() return void").
> 
>> +	.driver = {
>> +		.name = "stm32-pcie",
>> +		.of_match_table = stm32_pcie_of_match,
>> +		.pm		= &stm32_pcie_pm_ops,
>> +	},
>> +};
>> +
>> +static bool is_stm32_pcie_driver(struct device *dev)
>> +{
>> +	/* PCI bridge */
>> +	dev = get_device(dev);
>> +
>> +	/* Platform driver */
>> +	dev = get_device(dev->parent);
>> +
>> +	return (dev->driver == &stm32_pcie_driver.driver);
> 
> Ugh.  Some MPS/MRRS magic going on here, evidently related to the STM
> integration of DWC IP? >
>> +static bool apply_mrrs_quirk(struct pci_dev *root_port)
>> +{
>> +	struct dw_pcie_rp *pp;
>> +	struct dw_pcie *dw_pci;
>> +	struct stm32_pcie *stm32_pcie;
>> +
>> +	if (WARN_ON(!root_port) || !is_stm32_pcie_driver(root_port->dev.parent))
>> +		return false;
>> +
>> +	pp = root_port->bus->sysdata;
>> +	dw_pci = to_dw_pcie_from_pp(pp);
>> +	stm32_pcie = to_stm32_pcie(dw_pci);
>> +
>> +	if (!stm32_pcie->limit_downstream_mrrs)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static void quirk_stm32_pcie_limit_mrrs(struct pci_dev *pci)
>> +{
>> +	struct pci_dev *root_port;
>> +	struct pci_bus *bus = pci->bus;
>> +	int readrq;
>> +	int mps;
>> +
>> +	if (pci_is_root_bus(bus))
>> +		return;
>> +
>> +	root_port = pcie_find_root_port(pci);
>> +
>> +	if (!apply_mrrs_quirk(root_port))
>> +		return;
>> +
>> +	mps = pcie_get_mps(root_port);
>> +
>> +	/*
>> +	 * STM32 PCI controller has a h/w performance limitation on the AXI DDR requests.
>> +	 * Limit the maximum read request size to 256B on all downstream devices.
> 
> I guess this is some kind of platform erratum, since there's no way
> for us to discover a limit on supported MRRS values?

Yes. Those quirk are not necessary anymore. will drop

> 
>> +	readrq = pcie_get_readrq(pci);
>> +	if (readrq > 256) {
>> +		int mrrs = min(mps, 256);
>> +
>> +		pcie_set_readrq(pci, mrrs);
>> +
>> +		pci_info(pci, "Max Read Rq set to %4d (was %4d)\n", mrrs, readrq);
>> +	}
>> +}
>> +
>> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID,
>> +			 quirk_stm32_pcie_limit_mrrs);
>> 
>> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
>> +{
>> +	dev_dbg(&pdev->dev, "set bus_dma_limit");
>> +
>> +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);
> 
> This is quite unusual and deserves some comment about why we need
> it.
> 

32 bus master DMA cannot access the last 2GB in addressing space (out of 
a 6GB addressing space).

Proposing the following comment
"DMA masters can only access the first 4GB of memory space, so setup the 
bus DMA limit accordingly."

Saw a similar quirk in mips/pci/fixup-sb1250.c



>> +	return 0;
>> +}
>> +
>> +static void quirk_stm32_dma_mask(struct pci_dev *pci)
>> +{
>> +	struct pci_dev *root_port;
>> +
>> +	root_port = pcie_find_root_port(pci);
>> +
>> +	if (root_port && (root_port->dev.parent))
>> +		pci_walk_bus(pci->bus, stm32_dma_limit, NULL);
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SYNOPSYS, 0x0550, quirk_stm32_dma_mask);
> 
> I guess this applies to [16c3:0550] devices and everything below them?
> It looks like these must be Root Ports?  And they identify as
> PCI_VENDOR_ID_SYNOPSYS instead of PCI_VENDOR_ID_STMICRO (104a)?

Yes we are based on the designware v5.50a PCIe version. The idea here is 
to set bus_dma_limit for all devices on the root_port.
is_stm32_pcie_driver is a sanity double check in case the quirk is 
applied to another target linked at compile time

> 
> Could be added at https://admin.pci-ids.ucw.cz/read/PC/16c3/ if you
> want lspci to name them correctly.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-stm32.h
> 
>> +#define STM32MP25_PCIECR_EP		0
> 
> Ideally would go in the patch that uses it.

This is a bit num. Similar to STM32MP25_PCIECR_RC BIT(10), I preper to 
see the definition close to the GENMASK that uses it, making the bit
checking a little bit easier to follow...

> 
>> +#define SYSCFG_PCIEPMEMSICR		0x6004
>> +#define SYSCFG_PCIEAERRCMSICR		0x6008
>> +#define SYSCFG_PCIESR1			0x6100
>> +#define SYSCFG_PCIESR2			0x6104
>> +
>> +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
>> +#define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
> 
> These are all unused, drop them until you need them.

OK, thanks !!

Christian

Powered by blists - more mailing lists