| 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
| ||
|
Message-ID: <20251022190402.GA1262472@bhelgaas>
Date: Wed, 22 Oct 2025 14:04:02 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Vincent Guittot <vincent.guittot@...aro.org>
Cc: chester62515@...il.com, mbrugger@...e.com,
ghennadi.procopciuc@....nxp.com, s32@....com, bhelgaas@...gle.com,
jingoohan1@...il.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, Ionut.Vicovan@....com, larisa.grigore@....com,
Ghennadi.Procopciuc@....com, ciprianmarian.costea@....com,
bogdan.hamciuc@....com, Frank.li@....com,
linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
imx@...ts.linux.dev, cassel@...nel.org
Subject: Re: [PATCH 3/4 v3] PCI: s32g: Add initial PCIe support (RC)
On Wed, Oct 22, 2025 at 07:43:08PM +0200, Vincent Guittot wrote:
> Add initial support of the PCIe controller for S32G Soc family. Only
> host mode is supported.
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -406,6 +406,16 @@ config PCIE_UNIPHIER_EP
> Say Y here if you want PCIe endpoint controller support on
> UniPhier SoCs. This driver supports Pro5 SoC.
>
> +config PCIE_NXP_S32G
> + tristate "NXP S32G PCIe controller (host mode)"
> + depends on ARCH_S32 || COMPILE_TEST
> + select PCIE_DW_HOST
> + help
> + Enable support for the PCIe controller in NXP S32G based boards to
> + work in Host mode. The controller is based on DesignWare IP and
> + can work either as RC or EP. In order to enable host-specific
> + features PCIE_S32G must be selected.
Reorder this so the menu item is sorted by vendor name.
> +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g-regs.h
> +/* Link Interrupt Control And Status */
> +#define PCIE_S32G_LINK_INT_CTRL_STS 0x40
> +#define LINK_REQ_RST_NOT_INT_EN BIT(1)
> +#define LINK_REQ_RST_NOT_CLR BIT(2)
None of these are used; remove until you need them.
> +/* PCIe controller 0 General Control 1 */
> +#define PCIE_S32G_PE0_GEN_CTRL_1 0x50
> +#define DEVICE_TYPE_MASK GENMASK(3, 0)
> +#define DEVICE_TYPE(x) FIELD_PREP(DEVICE_TYPE_MASK, x)
Not sure this adds much over just doing this:
#define DEVICE_TYPE GENMASK(3, 0)
val |= FIELD_PREP(DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT);
> +++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
> +static void s32g_pcie_reset_mstr_ace(struct dw_pcie *pci, u64 ddr_base_addr)
> +{
> + u32 ddr_base_low = lower_32_bits(ddr_base_addr);
> + u32 ddr_base_high = upper_32_bits(ddr_base_addr);
> +
> + dw_pcie_dbi_ro_wr_en(pci);
> + dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, 0x0);
> +
> + /*
> + * Ncore is a cache-coherent interconnect module that enables the
> + * integration of heterogeneous coherent and non-coherent agents in
> + * the chip. Ncore Transactions to peripheral should be non-coherent
> + * or it might drop them.
> + * One example where this is needed are PCIe MSIs, which use NoSnoop=0
> + * and might end up routed to Ncore.
> + * Define the start of DDR as seen by Linux as the boundary between
> + * "memory" and "peripherals", with peripherals being below.
Add blank lines between paragraphs.
> + */
> + dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_1_OFF,
> + (ddr_base_low & CFG_MEMTYPE_BOUNDARY_LOW_ADDR_MASK));
> + dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_2_OFF, ddr_base_high);
> + dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void s32g_init_pcie_controller(struct s32g_pcie *s32g_pp)
> +{
> + struct dw_pcie *pci = &s32g_pp->pci;
> + u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> + u32 val;
> +
> + /* Set RP mode */
> + val = s32g_pcie_readl_ctrl(s32g_pp, PCIE_S32G_PE0_GEN_CTRL_1);
> + val &= ~DEVICE_TYPE_MASK;
> + val |= DEVICE_TYPE(PCI_EXP_TYPE_ROOT_PORT);
> +
> + /* Use default CRNS */
> + val &= ~SRIS_MODE;
> +
> + s32g_pcie_writel_ctrl(s32g_pp, PCIE_S32G_PE0_GEN_CTRL_1, val);
> +
> + /* Disable phase 2,3 equalization */
> + s32g_pcie_disable_equalization(pci);
> +
> + /*
> + * Make sure we use the coherency defaults (just in case the settings
> + * have been changed from their reset values)
> + */
> + s32g_pcie_reset_mstr_ace(pci, memblock_start_of_DRAM());
This seems sketchy and no other driver uses memblock_start_of_DRAM().
Shouldn't a physical memory address like this come from devicetree
somehow?
> + dw_pcie_dbi_ro_wr_en(pci);
> +
> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_FORCE);
> + val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
> + dw_pcie_writel_dbi(pci, PCIE_PORT_FORCE, val);
> +
> + /*
> + * Set max payload supported, 256 bytes and
> + * relaxed ordering.
> + */
> + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> + val &= ~(PCI_EXP_DEVCTL_RELAX_EN |
> + PCI_EXP_DEVCTL_PAYLOAD |
> + PCI_EXP_DEVCTL_READRQ);
> + val |= PCI_EXP_DEVCTL_RELAX_EN |
> + PCI_EXP_DEVCTL_PAYLOAD_256B |
> + PCI_EXP_DEVCTL_READRQ_256B;
> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
MPS and relaxed ordering should be configured by the PCI core. Is
there some s32g-specific restriction about these?
> + /* Enable errors */
> + val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> + val |= PCI_EXP_DEVCTL_CERE |
> + PCI_EXP_DEVCTL_NFERE |
> + PCI_EXP_DEVCTL_FERE |
> + PCI_EXP_DEVCTL_URRE;
> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
Enabling these errors doesn't really seem device-specific, and
pci_enable_pcie_error_reporting() would enable all these.
But that only happens with CONFIG_PCIEAER=y, and since this is
DWC-based, you probably don't have standard interrupts for AER and
CONFIG_PCIEAER isn't useful. Someday we might have support for
non-standard AER interrupts, but we don't have it yet.
I guess you get platform-specific System Errors when any of these
errors are detected (see PCIe r7.0, sec 6.2.6)? What is the handler
for these?
> +static int s32g_pcie_host_init(struct s32g_pcie *s32g_pp)
> +{
> + struct dw_pcie *pci = &s32g_pp->pci;
> + struct dw_pcie_rp *pp = &pci->pp;
> + int ret;
> +
> + pp->ops = &s32g_pcie_host_ops;
> +
> + ret = dw_pcie_host_init(pp);
> +
> + return ret;
return dw_pcie_host_init(pp);
Not sure this is really worth a wrapper.
> +static int s32g_pcie_get_resources(struct platform_device *pdev,
> + struct s32g_pcie *s32g_pp)
> +{
> + struct device *dev = &pdev->dev;
> + struct dw_pcie *pci = &s32g_pp->pci;
> +
> + s32g_pp->phy = devm_phy_get(dev, NULL);
> + if (IS_ERR(s32g_pp->phy))
> + return dev_err_probe(dev, PTR_ERR(s32g_pp->phy),
> + "Failed to get serdes PHY\n");
Add blank line here.
> + s32g_pp->ctrl_base = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> + if (IS_ERR(s32g_pp->ctrl_base))
> + return PTR_ERR(s32g_pp->ctrl_base);
This looks like the first DWC driver that uses a "ctrl" resource. Is
this something unique to s32g, or do other drivers have something
similar but use a different name?
> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> + if (IS_ERR(pci->dbi_base))
> + return PTR_ERR(pci->dbi_base);
Isn't this already done by dw_pcie_get_resources()?
> +static int s32g_pcie_suspend_noirq(struct device *dev)
> +{
> + struct s32g_pcie *s32g_pp = dev_get_drvdata(dev);
> + struct dw_pcie *pci = &s32g_pp->pci;
> +
> + if (!dw_pcie_link_up(pci))
> + return 0;
Does something bad happen if you omit the link up check and the link
is not up when we get here? The check is racy (the link could go down
between dw_pcie_link_up() and dw_pcie_suspend_noirq()), so it's not
completely reliable.
If you have to check, please add a comment about why this driver needs
it when no other driver does.
> + return dw_pcie_suspend_noirq(pci);
> +}
> +
> +static int s32g_pcie_resume_noirq(struct device *dev)
> +{
> + struct s32g_pcie *s32g_pp = dev_get_drvdata(dev);
> + struct dw_pcie *pci = &s32g_pp->pci;
> +
> + s32g_init_pcie_controller(s32g_pp);
Odd that you need this. Other drivers really don't do anything
similar, probably because this could be done by the
pci->pp.ops->init() call in dw_pcie_resume_noirq().
> + return dw_pcie_resume_noirq(pci);
> +}
> +static const struct of_device_id s32g_pcie_of_match[] = {
> + { .compatible = "nxp,s32g2-pcie"},
Add space before "}"
> + { /* sentinel */ },
> +};
Powered by blists - more mailing lists