Message-ID: <3fb8b957.b81.19ac991e9f5.Coremail.zhangsenchuan@eswincomputing.com>
Date: Fri, 28 Nov 2025 16:26:14 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Manivannan Sadhasivam" <mani@...nel.org>
Cc: bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org,
	lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org,
	p.zabel@...gutronix.de, jingoohan1@...il.com,
	gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	christian.bruel@...s.st.com, mayank.rana@....qualcomm.com,
	shradha.t@...sung.com, krishna.chundru@....qualcomm.com,
	thippeswamy.havalige@....com, inochiama@...il.com,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com,
	Frank.li@....com
Subject: Re: Re: [PATCH v6 2/3] PCI: eic7700: Add Eswin PCIe host controller
 driver




> -----Original Messages-----
> From: "Manivannan Sadhasivam" <mani@...nel.org>
> Send time:Wednesday, 26/11/2025 21:14:54
> To: zhangsenchuan <zhangsenchuan@...incomputing.com>
> Cc: bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, p.zabel@...gutronix.de, jingoohan1@...il.com, gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, christian.bruel@...s.st.com, mayank.rana@....qualcomm.com, shradha.t@...sung.com, krishna.chundru@....qualcomm.com, thippeswamy.havalige@....com, inochiama@...il.com, ningyu@...incomputing.com, linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com, Frank.li@....com
> Subject: Re: [PATCH v6 2/3] PCI: eic7700: Add Eswin PCIe host controller driver
> 
> On Wed, Nov 26, 2025 at 08:15:11PM +0800, zhangsenchuan wrote:
> > 
> > 
> > 
> > > -----Original Messages-----
> > > From: "Manivannan Sadhasivam" <mani@...nel.org>
> > > Send time:Thursday, 20/11/2025 20:43:47
> > > To: zhangsenchuan@...incomputing.com
> > > Cc: bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, p.zabel@...gutronix.de, jingoohan1@...il.com, gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, christian.bruel@...s.st.com, mayank.rana@....qualcomm.com, shradha.t@...sung.com, krishna.chundru@....qualcomm.com, thippeswamy.havalige@....com, inochiama@...il.com, ningyu@...incomputing.com, linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com, Frank.li@....com
> > > Subject: Re: [PATCH v6 2/3] PCI: eic7700: Add Eswin PCIe host controller driver
> > > 
> > > On Thu, Nov 20, 2025 at 06:12:06PM +0800, zhangsenchuan@...incomputing.com wrote:
> > > > From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > > > 
> > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > the DesignWare PCIe core, IP revision 6.00a. The PCIe Gen.3 controller
> > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > interrupts.
> > > > 
> > > > Signed-off-by: Yu Ning <ningyu@...incomputing.com>
> > > > Signed-off-by: Yanghui Ou <ouyanghui@...incomputing.com>
> > > > Signed-off-by: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/Kconfig        |  11 +
> > > >  drivers/pci/controller/dwc/Makefile       |   1 +
> > > >  drivers/pci/controller/dwc/pcie-eic7700.c | 387 ++++++++++++++++++++++
> > > >  3 files changed, 399 insertions(+)
> > > >  create mode 100644 drivers/pci/controller/dwc/pcie-eic7700.c
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index 349d4657393c..66568efb324f 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -93,6 +93,17 @@ config PCIE_BT1
> > > >  	  Enables support for the PCIe controller in the Baikal-T1 SoC to work
> > > >  	  in host mode. It's based on the Synopsys DWC PCIe v4.60a IP-core.
> > > >  
> > > > +config PCIE_EIC7700
> > > > +	bool "Eswin EIC7700 PCIe controller"
> > > 
> > > You can make it tristate as you've used builtin_platform_driver() which
> > > guarantees that this driver won't be removed once loaded.
> > 
> > Okey, thanks
> > 
> > > 
> > > > +	depends on ARCH_ESWIN || COMPILE_TEST
> > > > +	depends on PCI_MSI
> > > > +	select PCIE_DW_HOST
> > > > +	help
> > > > +	  Say Y here if you want PCIe controller support for the Eswin EIC7700.
> > > > +	  The PCIe controller on EIC7700 is based on DesignWare hardware,
> > > > +	  enables support for the PCIe controller in the EIC7700 SoC to work in
> > > > +	  host mode.
> > > > +
> > > >  config PCI_IMX6
> > > >  	bool
> > > >  
> > > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > > index 7ae28f3b0fb3..04f751c49eba 100644
> > > > --- a/drivers/pci/controller/dwc/Makefile
> > > > +++ b/drivers/pci/controller/dwc/Makefile
> > > > @@ -6,6 +6,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> > > >  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> > > >  obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> > > >  obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
> > > > +obj-$(CONFIG_PCIE_EIC7700) += pcie-eic7700.o
> > > >  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
> > > >  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> > > >  obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
> > > > diff --git a/drivers/pci/controller/dwc/pcie-eic7700.c b/drivers/pci/controller/dwc/pcie-eic7700.c
> > > > new file mode 100644
> > > > index 000000000000..239fdbc501fe
> > > > --- /dev/null
> > > > +++ b/drivers/pci/controller/dwc/pcie-eic7700.c
> > > > @@ -0,0 +1,387 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * ESWIN EIC7700 PCIe root complex driver
> > > > + *
> > > > + * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
> > > > + *
> > > > + * Authors: Yu Ning <ningyu@...incomputing.com>
> > > > + *          Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > > > + *          Yanghui Ou <ouyanghui@...incomputing.com>
> > > > + */
> > > > +
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/pci.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/resource.h>
> > > > +#include <linux/reset.h>
> > > > +#include <linux/types.h>
> > > > +
> > > > +#include "pcie-designware.h"
> > > > +
> > > > +/* ELBI registers */
> > > > +#define PCIEELBI_CTRL0_OFFSET		0x0
> > > > +#define PCIEELBI_STATUS0_OFFSET		0x100
> > > > +
> > > > +/* LTSSM register fields */
> > > > +#define PCIEELBI_APP_LTSSM_ENABLE	BIT(5)
> > > > +
> > > > +/* APP_HOLD_PHY_RST register fields */
> > > > +#define PCIEELBI_APP_HOLD_PHY_RST	BIT(6)
> > > > +
> > > > +/* PM_SEL_AUX_CLK register fields */
> > > > +#define PCIEELBI_PM_SEL_AUX_CLK		BIT(16)
> > > > +
> > > > +/* DEV_TYPE register fields */
> > > > +#define PCIEELBI_CTRL0_DEV_TYPE		GENMASK(3, 0)
> > > > +
> > > > +/* Vendor and device ID value */
> > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > +
> > > > +#define EIC7700_NUM_RSTS		ARRAY_SIZE(eic7700_pcie_rsts)
> > > > +
> > > > +static const char * const eic7700_pcie_rsts[] = {
> > > > +	"pwr",
> > > > +	"dbi",
> > > > +};
> > > > +
> > > > +struct eic7700_pcie_data {
> > > > +	bool msix_cap;
> > > > +	bool no_suspport_L2;
> > > 
> > > support?
> > 
> > Okey, spelling mistake:)
> > 
> > mani, by the way, our controller cannot broadcast PME_Turn_Off. Previously,
> > i skip broadcast PME_Turn_Off in our controller code. Frank suggested that 
> > set the flag bit to skip in the public code. At present, do you think it's 
> > okay for me to add no_support_L2?
> > 
> 
> This sounds weird. As per the spec r6.0, sec 5.2, "L2/L3 Ready transition
> protocol support is required." So this means the controller has to support
> PME_Turn_Off broadcast. Are you saying that your controller is not spec
> compliant? and the link can only enter L3 (power off) abruptly?

Hi, Mani

Clarification
After confirmation with the hardware team, L2/L3 low-power link state is not 
supported, and cannot be controlled to enter L2/L3 through trigger 
PME_turn_off/PME_To-Ack handshake, because this function is not implemented in 
hardware, nor is the Vmain/Vaux power domain implemented to implement L2/L3 
low-power wake-up mechanism.

Our PCIe only implements L0s/L1/L1.1 low-power mode. if want to put PCIe into 
a more energy-efficient state, only power down PCIe. Although the PCIe Spec 
requires the implementation of L2/L3 ready state, it is regrettable that we
have not been able to match the protocol requirements at this point. We will
gradually improve more features required by the Spec in future products.

Regarding the handling of skipping PME_Turn_Off broadcast, should I add a 
flag bit to skip it in Synopsys public code or register a callback function 
to skip it in our platform driver? Which approach is better? I'd like to hear
your suggestions. Thanks!

Kind regards,
Senchuan Zhang

> 
> > > > +static void eic7700_pcie_hide_broken_msix_cap(struct dw_pcie *pci)
> > > > +{
> > > > +	u16 offset, val;
> > > > +
> > > > +	/*
> > > > +	 * Hardware doesn't support MSI-X but it advertises MSI-X capability,
> > > > +	 * to avoid this problem, the MSI-X capability in the PCIe capabilities
> > > > +	 * linked-list needs to be disabled. Since the PCI Express capability
> > > > +	 * structure's next pointer points to the MSI-X capability, and the
> > > > +	 * MSI-X capability's next pointer is null (00H), so only the PCI
> > > > +	 * Express capability structure's next pointer needs to be set 00H.
> > > > +	 */
> > > > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > > +	val = dw_pcie_readl_dbi(pci, offset);
> > > > +	val &= ~PCI_CAP_LIST_NEXT_MASK;
> > > > +	dw_pcie_writel_dbi(pci, offset, val);
> > > 
> > > I hate to enforce dependency for your series, but this is properly handled here:
> > > https://lore.kernel.org/linux-pci/20251109-remove_cap-v1-2-2208f46f4dc2@oss.qualcomm.com
> > 
> > Okey, then I can rely on this patch to solve the problem of the missing MSI-X. 
> > Thank you for the reminder.
> > 
> > > 
> > > > +}
> > > > +
> > > > +static int eic7700_pcie_host_init(struct dw_pcie_rp *pp)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct eic7700_pcie *pcie = to_eic7700_pcie(pci);
> > > > +	struct eic7700_pcie_port *port;
> > > > +	u8 msi_cap;
> > > > +	u32 val;
> > > > +	int ret;
> > > > +
> > > > +	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
> > > > +	if (pcie->num_clks < 0)
> > > > +		return dev_err_probe(pci->dev, pcie->num_clks,
> > > > +				     "Failed to get pcie clocks\n");
> > > > +
> > > > +	ret = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
> > > 
> > > I think this is being called too early.
> > 
> > Before accessing the register. I need to deassert, otherwise, there will
> > be problems with register access.
> > 
> > > 
> > > > +	if (ret) {
> > > > +		dev_err(pcie->pci.dev, "Failed to deassert resets\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* Configure Root Port type */
> > > > +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > > > +	val &= ~PCIEELBI_CTRL0_DEV_TYPE;
> > > > +	val |= FIELD_PREP(PCIEELBI_CTRL0_DEV_TYPE, PCI_EXP_TYPE_ROOT_PORT);
> > > > +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > > > +
> > > > +	list_for_each_entry(port, &pcie->ports, list) {
> > > > +		ret = eic7700_pcie_perst_deassert(port, pcie);
> > > > +		if (ret)
> > > > +			goto err_perst;
> > > > +	}
> > > > +
> > > > +	/* Configure app_hold_phy_rst */
> > > > +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > > > +	val &= ~PCIEELBI_APP_HOLD_PHY_RST;
> > > > +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> > > > +
> > > > +	/* The maximum waiting time for the clock switch lock is 20ms */
> > > > +	ret = readl_poll_timeout(pci->elbi_base + PCIEELBI_STATUS0_OFFSET,
> > > > +				 val, !(val & PCIEELBI_PM_SEL_AUX_CLK), 1000,
> > > > +				 20000);
> > > > +	if (ret) {
> > > > +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> > > > +		goto err_phy_init;
> > > > +	}
> > > 
> > > You seem to be configuring the PHY reset and Aux clock, which should come before
> > > deasserting PERST# IMO. PERST# deassertion indicates that the power and clock
> > > are stable and the endpoint can start its operation. I don't know the impact of
> > > these configurations, but it is safe to do them earlier.
> > 
> > I think your understanding is correct. Unfortunately, in our hardware design, we 
> > need to deassert perst first before we can operate the configuration of the phy.
> > 
> 
> Sorry, I don't understand how it is possible, unless this reset is not PERST#.
> PERST# is just an indication to the component and has no relation with the
> controller.

After confirmation with the hardware team, Clarification as follows:

The power and reference clk of phy for our product PCIe are provided by the power
module and reference clock crystal oscillator on the board, and the power and 
reference clk of phy for the remote EP are also provided by the power and reference
clock crystal oscillator on the board through slots, using PCIe common reference 
clock mode. 
   
PERST# is sent to the local PCIe through the configuration register inside our 
chip, and this PERST# is also sent to the remote EP through the slot. Therefore,
in order to deassert PERST#, we first need our system to boot and configure it
through software, hen app_hold_phy_rst can be configured through software, the 
entire chip is already in a stable state, including the stability of power and 
reference clock.

Both our PCIe controller and phy require PERST# as the primary reset source, 
the configuration of app_hold_phy_rst is controlled by the controller to control
the state of phy through the pipe_raneX_powerdown signal in the pipe interface. 
Therefore, it is necessary to first deassert PERST# release controller and phy's 
Perst_n before configuring app_hold_phy_rst to allow the controller to control 
the pipe_raneX_powerdown signal.

PERST# is sent to both local PCIe and remote EP simultaneously for RC and EP 
reset synchronization, as well as to meet PCIe Spec requirements, which require 
entering the training process after stable power on and 20ms after deassert PERST#.
The signal control diagram is as follows:
                                           |
                                           | PERST#
                                           |
                    |----------------------|------------------------|                       
                    |                      |                        |
                    V                      V                        V
             |--------------|       |--------------|        |--------------|
             |  Controler   |       |      PHY     |        |      EP      |               
             |--------------|       |--------------|        |--------------| 
                    Λ                      Λ    Λ                      Λ
                    |        pci—>elbi_base|    |                      |
                    |                      |    |----------------------|
                    |pcie->resets          |                Λ
                    |(pwr/dbi resets)      |                |External ref clk
                                                            |

Kind regards,
Senchuan Zhang

Powered by blists - more mailing lists