On Tue, 20 Feb 2024 at 21:02, Marek Vasut <ma...@denx.de> wrote: > > On 2/20/24 14:10, Sumit Garg wrote: > > pcie_imx doesn't seem to share any useful code for iMX8 SoC and it is > > tied to quite old port of pcie_designware driver from Linux which > > suffices only iMX6 specific needs. > > > > But currently we have the common DWC specific bits which alligns pretty > > well with DW PCIe controller on iMX8MP SoC. So lets reuse those common > > bits instead as a new driver for iMX8 SoCs. It should be fairly easy to > > add support for other iMX8 variants to this driver. > > > > iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we > > can reuse the generic PHY infrastructure to power on PCIe PHY. > > > > Signed-off-by: Sumit Garg <sumit.g...@linaro.org> > > --- > > drivers/pci/Kconfig | 8 + > > drivers/pci/Makefile | 1 + > > drivers/pci/pcie_dw_imx8.c | 348 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 357 insertions(+) > > create mode 100644 drivers/pci/pcie_dw_imx8.c > > > > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig > > index 463ec47eb92..b7c7922b091 100644 > > --- a/drivers/pci/Kconfig > > +++ b/drivers/pci/Kconfig > > @@ -413,4 +413,12 @@ config PCIE_STARFIVE_JH7110 > > Say Y here if you want to enable PLDA XpressRich PCIe controller > > support on StarFive JH7110 SoC. > > > > +config PCIE_DW_IMX8 > > + bool "i.MX8 PCIe support" > > + depends on ARCH_IMX8M > > + select PCIE_DW_COMMON > > + help > > + Say Y here if you want to enable DW PCIe controller support on > > + iMX8 SoCs. > > + > > endif > > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile > > index 72ef8b4bc77..cddbb902095 100644 > > --- a/drivers/pci/Makefile > > +++ b/drivers/pci/Makefile > > @@ -53,3 +53,4 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o > > obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o > > obj-$(CONFIG_PCIE_PLDA_COMMON) += pcie_plda_common.o > > obj-$(CONFIG_PCIE_STARFIVE_JH7110) += pcie_starfive_jh7110.o > > +obj-$(CONFIG_PCIE_DW_IMX8) += pcie_dw_imx8.o > > diff --git a/drivers/pci/pcie_dw_imx8.c b/drivers/pci/pcie_dw_imx8.c > > new file mode 100644 > > index 00000000000..b9921644765 > > --- /dev/null > > +++ b/drivers/pci/pcie_dw_imx8.c > > @@ -0,0 +1,348 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2024 Linaro Ltd. > > + * > > + * Author: Sumit Garg <sumit.g...@linaro.org> > > + */ > > + > > +#include <asm/io.h> > > +#include <asm-generic/gpio.h> > > +#include <clk.h> > > +#include <dm.h> > > +#include <dm/device_compat.h> > > +#include <generic-phy.h> > > +#include <linux/bitops.h> > > +#include <linux/delay.h> > > +#include <linux/err.h> > > +#include <log.h> > > +#include <pci.h> > > +#include <regmap.h> > > +#include <reset.h> > > +#include <syscon.h> > > +#include <time.h> > > + > > +#include "pcie_dw_common.h" > > + > > +#define PCIE_LINK_CAPABILITY 0x7c > > +#define TARGET_LINK_SPEED_MASK 0xf > > +#define LINK_SPEED_GEN_1 0x1 > > +#define LINK_SPEED_GEN_2 0x2 > > +#define LINK_SPEED_GEN_3 0x3 > > + > > +#define PCIE_MISC_CONTROL_1_OFF 0x8bc > > +#define PCIE_DBI_RO_WR_EN BIT(0) > > + > > +#define PCIE_PORT_DEBUG0 0x728 > > +#define PCIE_PORT_DEBUG1 0x72c > > +#define PCIE_PORT_DEBUG1_LINK_UP BIT(4) > > +#define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) > > + > > +#define PCIE_LINK_UP_TIMEOUT_MS 100 > > + > > +#define IOMUXC_GPR14_OFFSET 0x38 > > +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10) > > +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11) > > + > > +struct pcie_dw_imx8 { > > + /* Must be first member of the struct */ > > + struct pcie_dw dw; > > + struct regmap *iomuxc_gpr; > > + struct clk pcie; > > + struct clk pcie_bus; > > + struct clk pcie_aux; > > + struct gpio_desc reset_gpio; > > + struct reset_ctl apps_reset; > > + struct phy phy; > > +}; > > + > > +static void pcie_dw_configure(struct pcie_dw_imx8 *priv, u32 cap_speed) > > +{ > > + u32 val; > > + > > + dw_pcie_dbi_write_enable(&priv->dw, true); > > + > > + val = readl(priv->dw.dbi_base + PCIE_LINK_CAPABILITY); > > + val &= ~TARGET_LINK_SPEED_MASK; > > + val |= cap_speed; > > + writel(val, priv->dw.dbi_base + PCIE_LINK_CAPABILITY); > > clrsetbits_le32()
Ack. > > > + > > + dw_pcie_dbi_write_enable(&priv->dw, false); > > +} > > + > > +static void imx8_pcie_ltssm_enable(struct pcie_dw_imx8 *priv) > > +{ > > + reset_deassert(&priv->apps_reset); > > +} > > + > > +static void imx8_pcie_ltssm_disable(struct pcie_dw_imx8 *priv) > > +{ > > + reset_assert(&priv->apps_reset); > > +} > > + > > +static int is_link_up(struct pcie_dw_imx8 *priv) > > +{ > > + u32 val; > > + > > + val = readl(priv->dw.dbi_base + PCIE_PORT_DEBUG1); > > + > > + return ((val & PCIE_PORT_DEBUG1_LINK_UP) && > > + (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); > > +} > > + > > +static int wait_link_up(struct pcie_dw_imx8 *priv) > > +{ > > + unsigned long timeout; > > + > > + timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS; > > wait_for_bit() or read_poll_timeout() They won't appropriately fit here as I would like to add delay in between consecutive polls too... > > > + while (!is_link_up(priv)) { > > + if (get_timer(0) > timeout) > > + return 0; > > + mdelay(10); ...here. > > + }; > > + > > + return 1; > > return -ETIMEDOUT ? Here 0 represents timeout and 1 represents success condition. > > > +} > > + > > +static int pcie_link_up(struct pcie_dw_imx8 *priv, u32 cap_speed) > > +{ > > + if (is_link_up(priv)) { > > + printf("PCI Link already up before configuration!\n"); > > + return 1; > > + } > > + > > + /* DW pre link configurations */ > > + pcie_dw_configure(priv, cap_speed); > > + > > + /* Initiate link training */ > > + imx8_pcie_ltssm_enable(priv); > > + > > + /* Check that link was established */ > > + if (!wait_link_up(priv)) { > > + imx8_pcie_ltssm_disable(priv); > > + return 0; > > + } > > + > > + return 1; > > +} > > + > > +static int imx8_pcie_assert_core_reset(struct pcie_dw_imx8 *priv) > > +{ > > + if (dm_gpio_is_valid(&priv->reset_gpio)) { > > + dm_gpio_set_value(&priv->reset_gpio, 1); > > + mdelay(20); > > + } > > + > > + return reset_assert(&priv->apps_reset); > > +} > > + > > +static int imx8_pcie_clk_enable(struct pcie_dw_imx8 *priv) > > +{ > > + int ret; > > + > > + ret = clk_enable(&priv->pcie); > > Can you use clk_bulk functions here ? > Sure, I can try to use them. > [...] > > > +static int imx8_pcie_deassert_core_reset(struct pcie_dw_imx8 *priv) > > +{ > > + if (dm_gpio_is_valid(&priv->reset_gpio)) { > > Invert the match here and return early to reduce indent. Ack. > > > + mdelay(100); > > + dm_gpio_set_value(&priv->reset_gpio, 0); > > + /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) > > */ > > + mdelay(100); > > + } > > + > > + return 0; > > +} > > + > > +static int pcie_dw_imx8_probe(struct udevice *dev) > > +{ > > + struct pcie_dw_imx8 *priv = dev_get_priv(dev); > > + struct udevice *ctlr = pci_get_controller(dev); > > + struct pci_controller *hose = dev_get_uclass_priv(ctlr); > > + int ret; > > + > > + ret = imx8_pcie_assert_core_reset(priv); > > + if (ret) { > > + dev_err(dev, "failed to assert core reset\n"); > > + return ret; > > + } > > + > > + ret = imx8_pcie_clk_enable(priv); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks\n"); > > This needs a fail path, else reset remains released on failure here . > Ack, I will add that. -Sumit > > + return ret; > > + } > > + > > + generic_phy_init(&priv->phy); > > + generic_phy_power_on(&priv->phy); > > + > > + ret = imx8_pcie_deassert_core_reset(priv); > > + if (ret) { > > + dev_err(dev, "failed to assert core reset\n"); > > + return ret; > > + } > > + > > + priv->dw.first_busno = dev_seq(dev); > > + priv->dw.dev = dev; > > + > > + pcie_dw_setup_host(&priv->dw); > > + > > + if (!pcie_link_up(priv, LINK_SPEED_GEN_1)) { > > + printf("PCIE-%d: Link down\n", dev_seq(dev)); > > + return -ENODEV; > > + } > > + > > + printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev), > > + pcie_dw_get_link_speed(&priv->dw), > > + pcie_dw_get_link_width(&priv->dw), > > + hose->first_busno); > > + > > + pcie_dw_prog_outbound_atu_unroll(&priv->dw, PCIE_ATU_REGION_INDEX0, > > + PCIE_ATU_TYPE_MEM, > > + priv->dw.mem.phys_start, > > + priv->dw.mem.bus_start, > > priv->dw.mem.size); > > + > > + return 0; > > +} > > + > > +static int pcie_dw_imx8_remove(struct udevice *dev) > > +{ > > + struct pcie_dw_imx8 *priv = dev_get_priv(dev); > > + > > + imx8_pcie_assert_core_reset(priv); > > + > > + return 0; > > +} > > [...]