Message-ID: <CAKfTPtCtHquxtK=Zx2WSNm15MmqeUXO8XXi8FkS4EpuP80PP7g@mail.gmail.com>
Date: Fri, 24 Oct 2025 08:50:46 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Bjorn Helgaas <helgaas@...nel.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, 22 Oct 2025 at 21:04, Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> 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.

Okay

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

That's a mistake and should not be in there.

>
> > +/* 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);

fair enough

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

okay

>
> > +      */
> > +     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?

I was using DT but has been asked to not use it and was proposed to
use memblock_start_of_DRAM() instead

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

I will check with the team why they did that

>
> > +     /* 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?

Same has above but I suspect we can remove it

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

fair enough

>
> Not sure this is really worth a wrapper.

More feature will come but with init ops, i will put everything in probe for now

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

yes

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

AFAICT this seems to be s32g specific in the RM

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

We needed dbi to be mapped before dw host init but this will not be
the case anymore with ops->init()

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

dw_pcie_suspend_noirq returns an error and the suspend fails

I will add a comment
/*
 * If the link is not up, there is nothing to suspend and resume
 */

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

no reason, i misread the impact of init in dw_pcie_host_init()



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