| 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: <xkfoddg6n3fr26maq4a647y36tbigjpivsz2n3tqfmxi3wmwo3@fhajg6zgl2wv>
Date: Fri, 28 Nov 2025 20:37:07 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
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 Fri, Nov 28, 2025 at 04:26:14PM +0800, zhangsenchuan wrote:
>
>
>
> > -----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.
>
Ok, thanks for checking.
> 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!
>
I'd prefer a flag, like, 'struct dw_pcie::no_pme_handshake'.
> 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.
>
It looks crazy (using PERST# for both controller and the endpoint). Anyway, you
should include this information in the comment above
reset_control_bulk_deassert().
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists