Message ID | cover.1571638827.git.eswara.kota@linux.intel.com |
---|---|
Headers | show |
Series | PCI: Add Intel PCIe Driver and respective dt-binding yaml file | expand |
Hi, On Mon, Oct 21, 2019 at 7:39:17, Dilip Kota <eswara.kota@linux.intel.com> wrote: > Intel PCIe is synopsys based controller utilizes the Designware Please do this general replacement in all your patches. s/synopsys/Synopsys and s/Designware/DesignWare > framework for host initialization and intel application > specific register configurations. > > Changes on v4: > Add lane resizing API in PCIe DesignWare driver. > Intel PCIe driver uses it for lane resizing which > is being exposed through sysfs attributes. > Add Intel PCIe sysfs attributes is in separate patch. > Address review comments given on v3. > > Changes on v3: > Compared to v2, map_irq() patch is removed as it is no longer > required for Intel PCIe driver. Intel PCIe driver does platform > specific interrupt configuration during core initialization. So > changed the subject line too. > Address v2 review comments for DT binding and PCIe driver > > Dilip Kota (3): > dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller > dwc: PCI: intel: PCIe RC controller driver > pci: intel: Add sysfs attributes to configure pcie link > > .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 ++++ > drivers/pci/controller/dwc/Kconfig | 10 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-designware.c | 43 ++ > drivers/pci/controller/dwc/pcie-designware.h | 15 + > drivers/pci/controller/dwc/pcie-intel-gw.c | 700 +++++++++++++++ > include/uapi/linux/pci_regs.h | 1 + > 7 files changed, 905 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml > create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c > > -- > 2.11.0
Hi On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota <eswara.kota@linux.intel.com> wrote: > Add support to PCIe RC controller on Intel Gateway SoCs. > PCIe controller is based of Synopsys DesignWare pci core. > > Intel PCIe driver requires Upconfig support, fast training > sequence configuration and link speed change. So adding the > respective helper functions in the pcie DesignWare framework. > It also programs hardware autonomous speed during speed > configuration so defining it in pci_regs.h. Please do the replacement in all of your patches s/pcie/PCIe s/pci/PCI Also I think the correct term is Upconfigure and not Upconfig > > Changes on v4: > Rename the driver naming and description to > "PCIe RC controller on Intel Gateway SoCs". > Use PCIe core register macros defined in pci_regs.h > and remove respective local definitions. > Remove pcie core interrupt handling. > Move pcie link control speed change, upconfig and FTS. > configuration functions to DesignWare framework. > Use of_pci_get_max_link_speed(). > Mark dependency on X86 and COMPILE_TEST in Kconfig. > Remove lanes and add n_fts variables in intel_pcie_port structure. > Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. > Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() > Move sysfs attributes specific code to separate patch. > Remove redundant error handling. > Reduce LoCs by doing variable initializations while declaration itself. > Add extra line after closing parenthesis. > Move intel_pcie_ep_rst_init() out of get_resources() > > changes on v3: > Rename PCIe app logic registers with PCIE_APP prefix. > PCIE_IOP_CTRL configuration is not required. Remove respective code. > Remove wrapper functions for clk enable/disable APIs. > Use platform_get_resource_byname() instead of > devm_platform_ioremap_resource() to be similar with DWC framework. > Rename phy name to "pciephy". > Modify debug message in msi_init() callback to be more specific. > Remove map_irq() callback. > Enable the INTx interrupts at the time of PCIe initialization. > Reduce memory fragmentation by using variable "struct dw_pcie pci" > instead of allocating memory. > Reduce the delay to 100us during enpoint initialization > intel_pcie_ep_rst_init(). > Call dw_pcie_host_deinit() during remove() instead of directly > calling PCIe core APIs. > Rename "intel,rst-interval" to "reset-assert-ms". > Remove unused APP logic Interrupt bit macro definitions. > Use dwc framework's dw_pcie_setup_rc() for PCIe host specific > configuration instead of redefining the same functionality in > the driver. > Move the whole DT parsing specific code to intel_pcie_get_resources() > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/pci/controller/dwc/Kconfig | 10 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-designware.c | 34 ++ > drivers/pci/controller/dwc/pcie-designware.h | 12 + > drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++++++++++++++++++++++++++ > include/uapi/linux/pci_regs.h | 1 + > 6 files changed, 648 insertions(+) > create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 0ba988b5b5bc..b33ed1cc873d 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP > order to enable device-specific features PCI_DW_PLAT_EP must be > selected. > > +config PCIE_INTEL_GW > + bool "Intel Gateway PCIe host controller support" > + depends on OF && (X86 || COMPILE_TEST) > + select PCIE_DW_HOST > + help > + Say 'Y' here to enable support for PCIe Host controller driver. > + The PCIe controller on Intel Gateway SoCs is based on the Synopsys > + DesignWare pcie core and therefore uses the DesignWare core > + functions for the driver implementation. > + > config PCI_EXYNOS > bool "Samsung Exynos PCIe controller" > depends on SOC_EXYNOS5440 || COMPILE_TEST > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index b30336181d46..99db83cd2f35 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o > obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o > +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o > obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o > obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o > obj-$(CONFIG_PCI_IMX6) += pci-imx6.o > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 820488dfeaed..4c391bfd681a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -474,6 +474,40 @@ int dw_pcie_link_up(struct dw_pcie *pci) > (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); > } > > + > +void dw_pcie_upconfig_setup(struct dw_pcie *pci) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); > + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, > + val | PORT_MLTI_UPCFG_SUPPORT); > +} > + > +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > + > + if (enable) > + val |= PORT_LOGIC_SPEED_CHANGE; > + else > + val &= ~PORT_LOGIC_SPEED_CHANGE; > + > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > +} > + > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > + val &= ~PORT_LOGIC_N_FTS; > + val |= n_fts; > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > +} > + > static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci) > { > u32 val; > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 5a18e94e52c8..3beac10e4a4c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -30,7 +30,12 @@ > #define LINK_WAIT_IATU 9 > > /* Synopsys-specific PCIe configuration registers */ > +#define PCIE_PORT_AFR 0x70C > +#define PORT_AFR_N_FTS_MASK GENMASK(15, 8) > +#define PORT_AFR_CC_N_FTS_MASK GENMASK(23, 16) > + > #define PCIE_PORT_LINK_CONTROL 0x710 > +#define PORT_LINK_DLL_LINK_EN BIT(5) > #define PORT_LINK_MODE_MASK GENMASK(21, 16) > #define PORT_LINK_MODE(n) FIELD_PREP(PORT_LINK_MODE_MASK, n) > #define PORT_LINK_MODE_1_LANES PORT_LINK_MODE(0x1) > @@ -46,6 +51,7 @@ > #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) > > #define PCIE_LINK_WIDTH_SPEED_CONTROL 0x80C > +#define PORT_LOGIC_N_FTS GENMASK(7, 0) > #define PORT_LOGIC_SPEED_CHANGE BIT(17) > #define PORT_LOGIC_LINK_WIDTH_MASK GENMASK(12, 8) > #define PORT_LOGIC_LINK_WIDTH(n) FIELD_PREP(PORT_LOGIC_LINK_WIDTH_MASK, n) > @@ -60,6 +66,9 @@ > #define PCIE_MSI_INTR0_MASK 0x82C > #define PCIE_MSI_INTR0_STATUS 0x830 > > +#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > +#define PORT_MLTI_UPCFG_SUPPORT BIT(7) > + > #define PCIE_ATU_VIEWPORT 0x900 > #define PCIE_ATU_REGION_INBOUND BIT(31) > #define PCIE_ATU_REGION_OUTBOUND 0 > @@ -273,6 +282,9 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); > void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > int dw_pcie_link_up(struct dw_pcie *pci); > +void dw_pcie_upconfig_setup(struct dw_pcie *pci); > +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); > int dw_pcie_wait_for_link(struct dw_pcie *pci); > void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, > int type, u64 cpu_addr, u64 pci_addr, > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c > new file mode 100644 > index 000000000000..9142c70db808 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c > @@ -0,0 +1,590 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for Intel Gateway SoCs > + * > + * Copyright (c) 2019 Intel Corporation. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/iopoll.h> > +#include <linux/of_irq.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/pci_regs.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > + > +#include "../../pci.h" > +#include "pcie-designware.h" > + > +#define PCIE_CAP_OFST 0x70 > + > +#define PORT_AFR_N_FTS_GEN12_DFT (SZ_128 - 1) > +#define PORT_AFR_N_FTS_GEN3 180 > +#define PORT_AFR_N_FTS_GEN4 196 > + > +/* PCIe Application logic Registers */ > +#define PCIE_APP_CCR 0x10 > +#define PCIE_APP_CCR_LTSSM_ENABLE BIT(0) > + > +#define PCIE_APP_MSG_CR 0x30 > +#define PCIE_APP_MSG_XMT_PM_TURNOFF BIT(0) > + > +#define PCIE_APP_PMC 0x44 > +#define PCIE_APP_PMC_IN_L2 BIT(20) > + > +#define PCIE_APP_IRNEN 0xF4 > +#define PCIE_APP_IRNCR 0xF8 > +#define PCIE_APP_IRN_AER_REPORT BIT(0) > +#define PCIE_APP_IRN_PME BIT(2) > +#define PCIE_APP_IRN_RX_VDM_MSG BIT(4) > +#define PCIE_APP_IRN_PM_TO_ACK BIT(9) > +#define PCIE_APP_IRN_LINK_AUTO_BW_STAT BIT(11) > +#define PCIE_APP_IRN_BW_MGT BIT(12) > +#define PCIE_APP_IRN_MSG_LTR BIT(18) > +#define PCIE_APP_IRN_SYS_ERR_RC BIT(29) > +#define PCIE_APP_INTX_OFST 12 > + > +#define PCIE_APP_IRN_INT \ > + (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ > + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ > + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ > + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) > + > +#define BUS_IATU_OFFS SZ_256M > +#define RST_INTRVL_DFT_MS 100 > + > +enum { > + PCIE_LINK_SPEED_AUTO = 0, > + PCIE_LINK_SPEED_GEN1, > + PCIE_LINK_SPEED_GEN2, > + PCIE_LINK_SPEED_GEN3, > + PCIE_LINK_SPEED_GEN4, > +}; > + > +struct intel_pcie_soc { > + unsigned int pcie_ver; > + unsigned int pcie_atu_offset; > + u32 num_viewport; > +}; > + > +struct intel_pcie_port { > + struct dw_pcie pci; > + unsigned int id; /* Physical RC Index */ > + void __iomem *app_base; > + struct gpio_desc *reset_gpio; > + u32 rst_intrvl; > + u32 max_speed; > + u32 link_gen; > + u32 max_width; > + u32 n_fts; > + struct clk *core_clk; > + struct reset_control *core_rst; > + struct phy *phy; > +}; > + > +static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, u32 ofs) > +{ > + u32 orig, tmp; > + > + orig = readl(base + ofs); > + > + tmp = (orig & ~mask) | (val & mask); > + > + if (tmp != orig) > + writel(tmp, base + ofs); > +} I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more intuitive names such as new and old, instead of orig and tmp. > +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs) > +{ > + return readl(lpp->app_base + ofs); > +} > + > +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) > +{ > + writel(val, lpp->app_base + ofs); > +} > + > +static void pcie_app_wr_mask(struct intel_pcie_port *lpp, > + u32 mask, u32 val, u32 ofs) > +{ > + pcie_update_bits(lpp->app_base, mask, val, ofs); > +} > + > +static inline u32 pcie_rc_cfg_rd(struct intel_pcie_port *lpp, u32 ofs) > +{ > + return dw_pcie_readl_dbi(&lpp->pci, ofs); > +} > + > +static inline void pcie_rc_cfg_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) > +{ > + dw_pcie_writel_dbi(&lpp->pci, ofs, val); > +} > + > +static void pcie_rc_cfg_wr_mask(struct intel_pcie_port *lpp, > + u32 mask, u32 val, u32 ofs) > +{ > + pcie_update_bits(lpp->pci.dbi_base, mask, val, ofs); > +} > + > +static void intel_pcie_ltssm_enable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, > + PCIE_APP_CCR_LTSSM_ENABLE, PCIE_APP_CCR); > +} > + > +static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); > +} > + > +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) > +{ > + u32 val; > + > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); > + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); > + > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + > + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); > + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | > + PCI_EXP_LNKCTL_RCB; > + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > +} > + > +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) > +{ > + u32 reg, val; > + > + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); > + switch (lpp->link_gen) { > + case PCIE_LINK_SPEED_GEN1: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_2_5GT; > + break; > + case PCIE_LINK_SPEED_GEN2: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_5_0GT; > + break; > + case PCIE_LINK_SPEED_GEN3: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_8_0GT; > + break; > + case PCIE_LINK_SPEED_GEN4: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_16_0GT; > + break; > + default: > + /* Use hardware capability */ > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); > + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + reg &= ~PCI_EXP_LNKCTL2_HASD; > + reg |= val; > + break; > + } > + > + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); > + dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts); > +} > + > + Reduce the number of empty lines here. > + > +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) > +{ > + u32 val, mask; > + > + switch (lpp->max_speed) { > + case PCIE_LINK_SPEED_GEN3: > + lpp->n_fts = PORT_AFR_N_FTS_GEN3; > + break; > + case PCIE_LINK_SPEED_GEN4: > + lpp->n_fts = PORT_AFR_N_FTS_GEN4; > + break; > + default: > + lpp->n_fts = PORT_AFR_N_FTS_GEN12_DFT; > + break; > + } > + > + mask = PORT_AFR_N_FTS_MASK | PORT_AFR_CC_N_FTS_MASK; > + val = FIELD_PREP(PORT_AFR_N_FTS_MASK, lpp->n_fts) | > + FIELD_PREP(PORT_AFR_CC_N_FTS_MASK, lpp->n_fts); > + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_PORT_AFR); > + > + /* Port Link Control Register */ > + pcie_rc_cfg_wr_mask(lpp, PORT_LINK_DLL_LINK_EN, > + PORT_LINK_DLL_LINK_EN, PCIE_PORT_LINK_CONTROL); > +} > + > +static void intel_pcie_rc_setup(struct intel_pcie_port *lpp) > +{ > + intel_pcie_ltssm_disable(lpp); > + intel_pcie_link_setup(lpp); > + dw_pcie_setup_rc(&lpp->pci.pp); > + dw_pcie_upconfig_setup(&lpp->pci); > + intel_pcie_port_logic_setup(lpp); > + intel_pcie_max_speed_setup(lpp); > +} > + > +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp) > +{ > + struct device *dev = lpp->pci.dev; > + int ret; > + > + lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(lpp->reset_gpio)) { > + ret = PTR_ERR(lpp->reset_gpio); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to request PCIe GPIO: %d\n", ret); > + return ret; > + } > + > + /* Make initial reset last for 100us */ > + usleep_range(100, 200); > + > + return 0; > +} > + > +static void intel_pcie_core_rst_assert(struct intel_pcie_port *lpp) > +{ > + reset_control_assert(lpp->core_rst); > +} > + > +static void intel_pcie_core_rst_deassert(struct intel_pcie_port *lpp) > +{ > + /* > + * One micro-second delay to make sure the reset pulse > + * wide enough so that core reset is clean. > + */ > + udelay(1); > + reset_control_deassert(lpp->core_rst); > + > + /* > + * Some SoC core reset also reset PHY, more delay needed > + * to make sure the reset process is done. > + */ > + usleep_range(1000, 2000); > +} > + > +static void intel_pcie_device_rst_assert(struct intel_pcie_port *lpp) > +{ > + gpiod_set_value_cansleep(lpp->reset_gpio, 1); > +} > + > +static void intel_pcie_device_rst_deassert(struct intel_pcie_port *lpp) > +{ > + msleep(lpp->rst_intrvl); > + gpiod_set_value_cansleep(lpp->reset_gpio, 0); > +} > + > +static int intel_pcie_app_logic_setup(struct intel_pcie_port *lpp) > +{ > + intel_pcie_device_rst_deassert(lpp); > + intel_pcie_ltssm_enable(lpp); > + > + return dw_pcie_wait_for_link(&lpp->pci); > +} > + > +static void intel_pcie_core_irq_disable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr(lpp, 0, PCIE_APP_IRNEN); > + pcie_app_wr(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRNCR); > +} > + > +static int intel_pcie_get_resources(struct platform_device *pdev) > +{ > + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); > + struct dw_pcie *pci = &lpp->pci; > + struct device *dev = pci->dev; > + struct resource *res; > + int ret; > + > + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); > + if (ret) { > + dev_err(dev, "failed to get domain id, errno %d\n", ret); > + return ret; > + } > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); > + > + pci->dbi_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > + > + lpp->core_clk = devm_clk_get(dev, NULL); > + if (IS_ERR(lpp->core_clk)) { > + ret = PTR_ERR(lpp->core_clk); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to get clks: %d\n", ret); > + return ret; > + } > + > + lpp->core_rst = devm_reset_control_get(dev, NULL); > + if (IS_ERR(lpp->core_rst)) { > + ret = PTR_ERR(lpp->core_rst); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to get resets: %d\n", ret); > + return ret; > + } > + > + ret = device_property_match_string(dev, "device_type", "pci"); > + if (ret) { > + dev_err(dev, "failed to find pci device type: %d\n", ret); > + return ret; > + } > + > + if (device_property_read_u32(dev, "reset-assert-ms", &lpp->rst_intrvl)) > + lpp->rst_intrvl = RST_INTRVL_DFT_MS; > + > + ret = of_pci_get_max_link_speed(dev->of_node); > + lpp->link_gen = ret < 0 ? 0 : ret; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > + > + lpp->app_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(lpp->app_base)) > + return PTR_ERR(lpp->app_base); > + > + lpp->phy = devm_phy_get(dev, "pcie"); > + if (IS_ERR(lpp->phy)) { > + ret = PTR_ERR(lpp->phy); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) > +{ > + phy_exit(lpp->phy); > +} > + > +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) > +{ > + u32 value; > + int ret; > + > + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) > + return 0; > + > + /* Send PME_TURN_OFF message */ > + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, > + PCIE_APP_MSG_XMT_PM_TURNOFF, PCIE_APP_MSG_CR); > + > + /* Read PMC status and wait for falling into L2 link state */ > + ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value, > + (value & PCIE_APP_PMC_IN_L2), 20, > + jiffies_to_usecs(5 * HZ)); > + if (ret) > + dev_err(lpp->pci.dev, "PCIe link enter L2 timeout!\n"); > + > + return ret; > +} > + > +static void intel_pcie_turn_off(struct intel_pcie_port *lpp) > +{ > + if (dw_pcie_link_up(&lpp->pci)) > + intel_pcie_wait_l2(lpp); > + > + /* Put endpoint device in reset state */ > + intel_pcie_device_rst_assert(lpp); > + pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY, 0, PCI_COMMAND); > +} > + > +static int intel_pcie_host_setup(struct intel_pcie_port *lpp) > +{ > + int ret; > + > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_device_rst_assert(lpp); > + > + ret = phy_init(lpp->phy); > + if (ret) > + return ret; > + > + intel_pcie_core_rst_deassert(lpp); > + > + ret = clk_prepare_enable(lpp->core_clk); > + if (ret) { > + dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret); > + goto clk_err; > + } > + > + intel_pcie_rc_setup(lpp); > + ret = intel_pcie_app_logic_setup(lpp); > + if (ret) > + goto app_init_err; > + > + /* Enable integrated interrupts */ > + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRN_INT, > + PCIE_APP_IRNEN); > + return 0; > + > +app_init_err: > + clk_disable_unprepare(lpp->core_clk); > +clk_err: > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_deinit_phy(lpp); > + > + return ret; > +} > + > +static void __intel_pcie_remove(struct intel_pcie_port *lpp) > +{ > + intel_pcie_core_irq_disable(lpp); > + intel_pcie_turn_off(lpp); > + clk_disable_unprepare(lpp->core_clk); > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_deinit_phy(lpp); > +} > + > +static int intel_pcie_remove(struct platform_device *pdev) > +{ > + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); > + struct pcie_port *pp = &lpp->pci.pp; > + > + dw_pcie_host_deinit(pp); > + __intel_pcie_remove(lpp); > + > + return 0; > +} > + > +static int __maybe_unused intel_pcie_suspend_noirq(struct device *dev) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + int ret; > + > + intel_pcie_core_irq_disable(lpp); > + ret = intel_pcie_wait_l2(lpp); > + if (ret) > + return ret; > + > + intel_pcie_deinit_phy(lpp); > + clk_disable_unprepare(lpp->core_clk); > + return ret; > +} > + > +static int __maybe_unused intel_pcie_resume_noirq(struct device *dev) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + > + return intel_pcie_host_setup(lpp); > +} > + > +static int intel_pcie_rc_init(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); > + > + return intel_pcie_host_setup(lpp); > +} > + > +int intel_pcie_msi_init(struct pcie_port *pp) > +{ > + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ > + return 0; > +} > + > +u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr) > +{ > + return cpu_addr + BUS_IATU_OFFS; > +} > + > +static const struct dw_pcie_ops intel_pcie_ops = { > + .cpu_addr_fixup = intel_pcie_cpu_addr, > +}; > + > +static const struct dw_pcie_host_ops intel_pcie_dw_ops = { > + .host_init = intel_pcie_rc_init, > + .msi_host_init = intel_pcie_msi_init, > +}; > + > +static const struct intel_pcie_soc pcie_data = { > + .pcie_ver = 0x520A, > + .pcie_atu_offset = 0xC0000, > + .num_viewport = 3, > +}; > + > +static int intel_pcie_probe(struct platform_device *pdev) > +{ > + const struct intel_pcie_soc *data; > + struct device *dev = &pdev->dev; > + struct intel_pcie_port *lpp; > + struct pcie_port *pp; > + struct dw_pcie *pci; > + int ret; > + > + lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL); > + if (!lpp) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, lpp); > + pci = &lpp->pci; > + pci->dev = dev; > + pp = &pci->pp; > + > + ret = intel_pcie_get_resources(pdev); > + if (ret) > + return ret; > + > + ret = intel_pcie_ep_rst_init(lpp); > + if (ret) > + return ret; > + > + data = device_get_match_data(dev); > + pci->ops = &intel_pcie_ops; > + pci->version = data->pcie_ver; > + pci->atu_base = pci->dbi_base + data->pcie_atu_offset; > + pp->ops = &intel_pcie_dw_ops; > + > + ret = dw_pcie_host_init(pp); > + if (ret) { > + dev_err(dev, "cannot initialize host\n"); > + return ret; > + } > + > + /* Intel PCIe doesn't configure IO region, so configure > + * viewport to not to access IO region during register > + * read write operations. > + */ > + pci->num_viewport = data->num_viewport; > + dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); > + > + return ret; > +} > + > +static const struct dev_pm_ops intel_pcie_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pcie_suspend_noirq, > + intel_pcie_resume_noirq) > +}; > + > +static const struct of_device_id of_intel_pcie_match[] = { > + { .compatible = "intel,lgm-pcie", .data = &pcie_data }, > + {} > +}; > + > +static struct platform_driver intel_pcie_driver = { > + .probe = intel_pcie_probe, > + .remove = intel_pcie_remove, > + .driver = { > + .name = "intel-gw-pcie", > + .of_match_table = of_intel_pcie_match, > + .pm = &intel_pcie_pm_ops, > + }, > +}; > +builtin_platform_driver(intel_pcie_driver); > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > index 29d6e93fd15e..f6e7e402f879 100644 > --- a/include/uapi/linux/pci_regs.h > +++ b/include/uapi/linux/pci_regs.h > @@ -673,6 +673,7 @@ > #define PCI_EXP_LNKCTL2_TLS_8_0GT 0x0003 /* Supported Speed 8GT/s */ > #define PCI_EXP_LNKCTL2_TLS_16_0GT 0x0004 /* Supported Speed 16GT/s */ > #define PCI_EXP_LNKCTL2_TLS_32_0GT 0x0005 /* Supported Speed 32GT/s */ > +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed Disable */ s/Hw/HW > #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ > #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */ > #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ > -- > 2.11.0
Hi Gustavo Pimentel, On 10/21/2019 4:08 PM, Gustavo Pimentel wrote: > Hi, > > > On Mon, Oct 21, 2019 at 7:39:17, Dilip Kota <eswara.kota@linux.intel.com> > wrote: > >> Intel PCIe is synopsys based controller utilizes the Designware > Please do this general replacement in all your patches. > > s/synopsys/Synopsys > > and > > s/Designware/DesignWare Sure, i will update it in the next patch version. (In the all other patches naming is proper, i got missed it here.) I will ensure and take care of it. Regards, Dilip > >> framework for host initialization and intel application >> specific register configurations. >> >> Changes on v4: >> Add lane resizing API in PCIe DesignWare driver. >> Intel PCIe driver uses it for lane resizing which >> is being exposed through sysfs attributes. >> Add Intel PCIe sysfs attributes is in separate patch. >> Address review comments given on v3. >> >> Changes on v3: >> Compared to v2, map_irq() patch is removed as it is no longer >> required for Intel PCIe driver. Intel PCIe driver does platform >> specific interrupt configuration during core initialization. So >> changed the subject line too. >> Address v2 review comments for DT binding and PCIe driver >> >> Dilip Kota (3): >> dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller >> dwc: PCI: intel: PCIe RC controller driver >> pci: intel: Add sysfs attributes to configure pcie link >> >> .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 ++++ >> drivers/pci/controller/dwc/Kconfig | 10 + >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-designware.c | 43 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 15 + >> drivers/pci/controller/dwc/pcie-intel-gw.c | 700 +++++++++++++++ >> include/uapi/linux/pci_regs.h | 1 + >> 7 files changed, 905 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml >> create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c >> >> -- >> 2.11.0 >
On Mon, Oct 21, 2019 at 7:39:20, Dilip Kota <eswara.kota@linux.intel.com> wrote: > PCIe RC driver on Intel Gateway SoCs have a requirement > of changing link width and speed on the fly. > So add the sysfs attributes to show and store the link > properties. > Add the respective link resize function in pcie DesignWare > framework so that Intel PCIe driver can use during link > width configuration on the fly. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 9 +++ > drivers/pci/controller/dwc/pcie-designware.h | 3 + > drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++++++++++++++++++++++++++- > 3 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 4c391bfd681a..662fdcb4f2d6 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) > (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); > } > > +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); > + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); > + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; > + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); > +} > > void dw_pcie_upconfig_setup(struct dw_pcie *pci) > { > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 3beac10e4a4c..fcf0442341fd 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -67,6 +67,8 @@ > #define PCIE_MSI_INTR0_STATUS 0x830 > > #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) > +#define PORT_MLTI_LNK_WDTH_CHNG BIT(6) > #define PORT_MLTI_UPCFG_SUPPORT BIT(7) > > #define PCIE_ATU_VIEWPORT 0x900 > @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); > void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > int dw_pcie_link_up(struct dw_pcie *pci); > +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); > void dw_pcie_upconfig_setup(struct dw_pcie *pci); > void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); > void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c > index 9142c70db808..b9be0921671d 100644 > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c > @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) > pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); > } > > +static const char *pcie_link_gen_to_str(int gen) > +{ > + switch (gen) { > + case PCIE_LINK_SPEED_GEN1: > + return "2.5"; > + case PCIE_LINK_SPEED_GEN2: > + return "5.0"; > + case PCIE_LINK_SPEED_GEN3: > + return "8.0"; > + case PCIE_LINK_SPEED_GEN4: > + return "16.0"; > + default: > + return "???"; > + } > +} > + > static void intel_pcie_link_setup(struct intel_pcie_port *lpp) > { > u32 val; > @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) > return ret; > } > > +static ssize_t pcie_link_status_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + u32 reg, width, gen; > + > + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); > + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); > + > + if (gen > lpp->max_speed) > + return -EINVAL; > + > + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, > + width, pcie_link_gen_to_str(gen)); > +} > +static DEVICE_ATTR_RO(pcie_link_status); Dilip please check pci.h there are there already enums and strings relatively to PCIe speed and width, that you can use. > + > +static ssize_t pcie_speed_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + unsigned long val; > + int ret; > + > + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val > lpp->max_speed) > + return -EINVAL; > + > + lpp->link_gen = val; > + intel_pcie_max_speed_setup(lpp); > + dw_pcie_link_speed_change(&lpp->pci, false); > + dw_pcie_link_speed_change(&lpp->pci, true); > + > + return len; > +} > +static DEVICE_ATTR_WO(pcie_speed); > + > +/* > + * Link width change on the fly is not always successful. > + * It also depends on the partner. > + */ > +static ssize_t pcie_width_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + unsigned long val; > + int ret; > + > + lpp = dev_get_drvdata(dev); > + > + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val > lpp->max_width) > + return -EINVAL; > + > + /* HW auto bandwidth negotiation must be enabled */ > + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, > + PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + dw_pcie_link_width_resize(&lpp->pci, val); > + > + return len; > +} > +static DEVICE_ATTR_WO(pcie_width); > + > +static struct attribute *pcie_cfg_attrs[] = { > + &dev_attr_pcie_link_status.attr, > + &dev_attr_pcie_speed.attr, > + &dev_attr_pcie_width.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(pcie_cfg); > + > +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp) > +{ > + return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups); > +} > + > static void __intel_pcie_remove(struct intel_pcie_port *lpp) > { > intel_pcie_core_irq_disable(lpp); > @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); > + int ret; > > - return intel_pcie_host_setup(lpp); > + ret = intel_pcie_host_setup(lpp); > + if (ret) > + return ret; > + > + ret = intel_pcie_sysfs_init(lpp); > + if (ret) > + __intel_pcie_remove(lpp); > + > + return ret; > } > > int intel_pcie_msi_init(struct pcie_port *pp) > -- > 2.11.0
Hi Gustavo Pimentel, On 10/21/2019 4:40 PM, Gustavo Pimentel wrote: > On Mon, Oct 21, 2019 at 7:39:20, Dilip Kota <eswara.kota@linux.intel.com> > wrote: > >> PCIe RC driver on Intel Gateway SoCs have a requirement >> of changing link width and speed on the fly. >> So add the sysfs attributes to show and store the link >> properties. >> Add the respective link resize function in pcie DesignWare >> framework so that Intel PCIe driver can use during link >> width configuration on the fly. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.c | 9 +++ >> drivers/pci/controller/dwc/pcie-designware.h | 3 + >> drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++++++++++++++++++++++++++- >> 3 files changed, 123 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 4c391bfd681a..662fdcb4f2d6 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) >> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); >> } >> >> +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); >> + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); >> + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; >> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); >> +} >> >> void dw_pcie_upconfig_setup(struct dw_pcie *pci) >> { >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 3beac10e4a4c..fcf0442341fd 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -67,6 +67,8 @@ >> #define PCIE_MSI_INTR0_STATUS 0x830 >> >> #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >> +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) >> +#define PORT_MLTI_LNK_WDTH_CHNG BIT(6) >> #define PORT_MLTI_UPCFG_SUPPORT BIT(7) >> >> #define PCIE_ATU_VIEWPORT 0x900 >> @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); >> void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> int dw_pcie_link_up(struct dw_pcie *pci); >> +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); >> void dw_pcie_upconfig_setup(struct dw_pcie *pci); >> void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); >> void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); >> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c >> index 9142c70db808..b9be0921671d 100644 >> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c >> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c >> @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) >> pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); >> } >> >> +static const char *pcie_link_gen_to_str(int gen) >> +{ >> + switch (gen) { >> + case PCIE_LINK_SPEED_GEN1: >> + return "2.5"; >> + case PCIE_LINK_SPEED_GEN2: >> + return "5.0"; >> + case PCIE_LINK_SPEED_GEN3: >> + return "8.0"; >> + case PCIE_LINK_SPEED_GEN4: >> + return "16.0"; >> + default: >> + return "???"; >> + } >> +} >> + >> static void intel_pcie_link_setup(struct intel_pcie_port *lpp) >> { >> u32 val; >> @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) >> return ret; >> } >> >> +static ssize_t pcie_link_status_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + u32 reg, width, gen; >> + >> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); >> + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); >> + >> + if (gen > lpp->max_speed) >> + return -EINVAL; >> + >> + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, >> + width, pcie_link_gen_to_str(gen)); >> +} >> +static DEVICE_ATTR_RO(pcie_link_status); > Dilip please check pci.h there are there already enums and strings > relatively to PCIe speed and width, that you can use. Yes i can see a global array "pcie_link_speed[]" and a macro PCIE_SPEED2STR[]. I will update the driver. Whereas width enum, it is not required here as directly storing the register value. Thanks for pointing it. Regards, Dilip > >> + >> +static ssize_t pcie_speed_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + unsigned long val; >> + int ret; >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret) >> + return ret; >> + >> + if (val > lpp->max_speed) >> + return -EINVAL; >> + >> + lpp->link_gen = val; >> + intel_pcie_max_speed_setup(lpp); >> + dw_pcie_link_speed_change(&lpp->pci, false); >> + dw_pcie_link_speed_change(&lpp->pci, true); >> + >> + return len; >> +} >> +static DEVICE_ATTR_WO(pcie_speed); >> + >> +/* >> + * Link width change on the fly is not always successful. >> + * It also depends on the partner. >> + */ >> +static ssize_t pcie_width_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + unsigned long val; >> + int ret; >> + >> + lpp = dev_get_drvdata(dev); >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret) >> + return ret; >> + >> + if (val > lpp->max_width) >> + return -EINVAL; >> + >> + /* HW auto bandwidth negotiation must be enabled */ >> + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, >> + PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + dw_pcie_link_width_resize(&lpp->pci, val); >> + >> + return len; >> +} >> +static DEVICE_ATTR_WO(pcie_width); >> + >> +static struct attribute *pcie_cfg_attrs[] = { >> + &dev_attr_pcie_link_status.attr, >> + &dev_attr_pcie_speed.attr, >> + &dev_attr_pcie_width.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(pcie_cfg); >> + >> +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp) >> +{ >> + return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups); >> +} >> + >> static void __intel_pcie_remove(struct intel_pcie_port *lpp) >> { >> intel_pcie_core_irq_disable(lpp); >> @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp) >> { >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); >> + int ret; >> >> - return intel_pcie_host_setup(lpp); >> + ret = intel_pcie_host_setup(lpp); >> + if (ret) >> + return ret; >> + >> + ret = intel_pcie_sysfs_init(lpp); >> + if (ret) >> + __intel_pcie_remove(lpp); >> + >> + return ret; >> } >> >> int intel_pcie_msi_init(struct pcie_port *pp) >> -- >> 2.11.0 >
Hi Gustavo Pimentel, On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: > Hi > > On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota <eswara.kota@linux.intel.com> > wrote: > >> Add support to PCIe RC controller on Intel Gateway SoCs. >> PCIe controller is based of Synopsys DesignWare pci core. >> >> Intel PCIe driver requires Upconfig support, fast training >> sequence configuration and link speed change. So adding the >> respective helper functions in the pcie DesignWare framework. >> It also programs hardware autonomous speed during speed >> configuration so defining it in pci_regs.h. > Please do the replacement in all of your patches > > s/pcie/PCIe > s/pci/PCI > > Also I think the correct term is Upconfigure and not Upconfig Yes, i will update it. > >> Changes on v4: >> Rename the driver naming and description to >> "PCIe RC controller on Intel Gateway SoCs". >> Use PCIe core register macros defined in pci_regs.h >> and remove respective local definitions. >> Remove pcie core interrupt handling. >> Move pcie link control speed change, upconfig and FTS. >> configuration functions to DesignWare framework. >> Use of_pci_get_max_link_speed(). >> Mark dependency on X86 and COMPILE_TEST in Kconfig. >> Remove lanes and add n_fts variables in intel_pcie_port structure. >> Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. >> Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() >> Move sysfs attributes specific code to separate patch. >> Remove redundant error handling. >> Reduce LoCs by doing variable initializations while declaration itself. >> Add extra line after closing parenthesis. >> Move intel_pcie_ep_rst_init() out of get_resources() >> >> changes on v3: >> Rename PCIe app logic registers with PCIE_APP prefix. >> PCIE_IOP_CTRL configuration is not required. Remove respective code. >> Remove wrapper functions for clk enable/disable APIs. >> Use platform_get_resource_byname() instead of >> devm_platform_ioremap_resource() to be similar with DWC framework. >> Rename phy name to "pciephy". >> Modify debug message in msi_init() callback to be more specific. >> Remove map_irq() callback. >> Enable the INTx interrupts at the time of PCIe initialization. >> Reduce memory fragmentation by using variable "struct dw_pcie pci" >> instead of allocating memory. >> Reduce the delay to 100us during enpoint initialization >> intel_pcie_ep_rst_init(). >> Call dw_pcie_host_deinit() during remove() instead of directly >> calling PCIe core APIs. >> Rename "intel,rst-interval" to "reset-assert-ms". >> Remove unused APP logic Interrupt bit macro definitions. >> Use dwc framework's dw_pcie_setup_rc() for PCIe host specific >> configuration instead of redefining the same functionality in >> the driver. >> Move the whole DT parsing specific code to intel_pcie_get_resources() >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/pci/controller/dwc/Kconfig | 10 + >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-designware.c | 34 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 12 + >> drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++++++++++++++++++++++++++ >> include/uapi/linux/pci_regs.h | 1 + >> 6 files changed, 648 insertions(+) >> create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c >> >> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig >> index 0ba988b5b5bc..b33ed1cc873d 100644 >> --- a/drivers/pci/controller/dwc/Kconfig >> +++ b/drivers/pci/controller/dwc/Kconfig >> @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP >> order to enable device-specific features PCI_DW_PLAT_EP must be >> selected. >> >> +config PCIE_INTEL_GW >> + bool "Intel Gateway PCIe host controller support" >> + depends on OF && (X86 || COMPILE_TEST) >> + select PCIE_DW_HOST >> + help >> + Say 'Y' here to enable support for PCIe Host controller driver. >> + The PCIe controller on Intel Gateway SoCs is based on the Synopsys >> + DesignWare pcie core and therefore uses the DesignWare core >> + functions for the driver implementation. >> + >> config PCI_EXYNOS >> bool "Samsung Exynos PCIe controller" >> depends on SOC_EXYNOS5440 || COMPILE_TEST >> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile >> index b30336181d46..99db83cd2f35 100644 >> --- a/drivers/pci/controller/dwc/Makefile >> +++ b/drivers/pci/controller/dwc/Makefile >> @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o >> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o >> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o >> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o >> +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o >> obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o >> obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o >> obj-$(CONFIG_PCI_IMX6) += pci-imx6.o >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 820488dfeaed..4c391bfd681a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -474,6 +474,40 @@ int dw_pcie_link_up(struct dw_pcie *pci) >> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); >> } >> >> + >> +void dw_pcie_upconfig_setup(struct dw_pcie *pci) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); >> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, >> + val | PORT_MLTI_UPCFG_SUPPORT); >> +} >> + >> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >> + >> + if (enable) >> + val |= PORT_LOGIC_SPEED_CHANGE; >> + else >> + val &= ~PORT_LOGIC_SPEED_CHANGE; >> + >> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >> +} >> + >> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >> + val &= ~PORT_LOGIC_N_FTS; >> + val |= n_fts; >> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >> +} >> + >> static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci) >> { >> u32 val; >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 5a18e94e52c8..3beac10e4a4c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -30,7 +30,12 @@ >> #define LINK_WAIT_IATU 9 >> >> /* Synopsys-specific PCIe configuration registers */ >> +#define PCIE_PORT_AFR 0x70C >> +#define PORT_AFR_N_FTS_MASK GENMASK(15, 8) >> +#define PORT_AFR_CC_N_FTS_MASK GENMASK(23, 16) >> + >> #define PCIE_PORT_LINK_CONTROL 0x710 >> +#define PORT_LINK_DLL_LINK_EN BIT(5) >> #define PORT_LINK_MODE_MASK GENMASK(21, 16) >> #define PORT_LINK_MODE(n) FIELD_PREP(PORT_LINK_MODE_MASK, n) >> #define PORT_LINK_MODE_1_LANES PORT_LINK_MODE(0x1) >> @@ -46,6 +51,7 @@ >> #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) >> >> #define PCIE_LINK_WIDTH_SPEED_CONTROL 0x80C >> +#define PORT_LOGIC_N_FTS GENMASK(7, 0) >> #define PORT_LOGIC_SPEED_CHANGE BIT(17) >> #define PORT_LOGIC_LINK_WIDTH_MASK GENMASK(12, 8) >> #define PORT_LOGIC_LINK_WIDTH(n) FIELD_PREP(PORT_LOGIC_LINK_WIDTH_MASK, n) >> @@ -60,6 +66,9 @@ >> #define PCIE_MSI_INTR0_MASK 0x82C >> #define PCIE_MSI_INTR0_STATUS 0x830 >> >> +#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >> +#define PORT_MLTI_UPCFG_SUPPORT BIT(7) >> + >> #define PCIE_ATU_VIEWPORT 0x900 >> #define PCIE_ATU_REGION_INBOUND BIT(31) >> #define PCIE_ATU_REGION_OUTBOUND 0 >> @@ -273,6 +282,9 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); >> void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> int dw_pcie_link_up(struct dw_pcie *pci); >> +void dw_pcie_upconfig_setup(struct dw_pcie *pci); >> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); >> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); >> int dw_pcie_wait_for_link(struct dw_pcie *pci); >> void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, >> int type, u64 cpu_addr, u64 pci_addr, >> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c >> new file mode 100644 >> index 000000000000..9142c70db808 >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c >> @@ -0,0 +1,590 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PCIe host controller driver for Intel Gateway SoCs >> + * >> + * Copyright (c) 2019 Intel Corporation. >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/clk.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/interrupt.h> >> +#include <linux/iopoll.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_pci.h> >> +#include <linux/of_platform.h> >> +#include <linux/pci_regs.h> >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset.h> >> + >> +#include "../../pci.h" >> +#include "pcie-designware.h" >> + >> +#define PCIE_CAP_OFST 0x70 >> + >> +#define PORT_AFR_N_FTS_GEN12_DFT (SZ_128 - 1) >> +#define PORT_AFR_N_FTS_GEN3 180 >> +#define PORT_AFR_N_FTS_GEN4 196 >> + >> +/* PCIe Application logic Registers */ >> +#define PCIE_APP_CCR 0x10 >> +#define PCIE_APP_CCR_LTSSM_ENABLE BIT(0) >> + >> +#define PCIE_APP_MSG_CR 0x30 >> +#define PCIE_APP_MSG_XMT_PM_TURNOFF BIT(0) >> + >> +#define PCIE_APP_PMC 0x44 >> +#define PCIE_APP_PMC_IN_L2 BIT(20) >> + >> +#define PCIE_APP_IRNEN 0xF4 >> +#define PCIE_APP_IRNCR 0xF8 >> +#define PCIE_APP_IRN_AER_REPORT BIT(0) >> +#define PCIE_APP_IRN_PME BIT(2) >> +#define PCIE_APP_IRN_RX_VDM_MSG BIT(4) >> +#define PCIE_APP_IRN_PM_TO_ACK BIT(9) >> +#define PCIE_APP_IRN_LINK_AUTO_BW_STAT BIT(11) >> +#define PCIE_APP_IRN_BW_MGT BIT(12) >> +#define PCIE_APP_IRN_MSG_LTR BIT(18) >> +#define PCIE_APP_IRN_SYS_ERR_RC BIT(29) >> +#define PCIE_APP_INTX_OFST 12 >> + >> +#define PCIE_APP_IRN_INT \ >> + (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ >> + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ >> + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ >> + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) >> + >> +#define BUS_IATU_OFFS SZ_256M >> +#define RST_INTRVL_DFT_MS 100 >> + >> +enum { >> + PCIE_LINK_SPEED_AUTO = 0, >> + PCIE_LINK_SPEED_GEN1, >> + PCIE_LINK_SPEED_GEN2, >> + PCIE_LINK_SPEED_GEN3, >> + PCIE_LINK_SPEED_GEN4, >> +}; >> + >> +struct intel_pcie_soc { >> + unsigned int pcie_ver; >> + unsigned int pcie_atu_offset; >> + u32 num_viewport; >> +}; >> + >> +struct intel_pcie_port { >> + struct dw_pcie pci; >> + unsigned int id; /* Physical RC Index */ >> + void __iomem *app_base; >> + struct gpio_desc *reset_gpio; >> + u32 rst_intrvl; >> + u32 max_speed; >> + u32 link_gen; >> + u32 max_width; >> + u32 n_fts; >> + struct clk *core_clk; >> + struct reset_control *core_rst; >> + struct phy *phy; >> +}; >> + >> +static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, u32 ofs) >> +{ >> + u32 orig, tmp; >> + >> + orig = readl(base + ofs); >> + >> + tmp = (orig & ~mask) | (val & mask); >> + >> + if (tmp != orig) >> + writel(tmp, base + ofs); >> +} > I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more > intuitive names such as new and old, instead of orig and tmp. Sure, i will update it. > >> +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs) >> +{ >> + return readl(lpp->app_base + ofs); >> +} >> + >> +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) >> +{ >> + writel(val, lpp->app_base + ofs); >> +} >> + >> +static void pcie_app_wr_mask(struct intel_pcie_port *lpp, >> + u32 mask, u32 val, u32 ofs) >> +{ >> + pcie_update_bits(lpp->app_base, mask, val, ofs); >> +} >> + >> +static inline u32 pcie_rc_cfg_rd(struct intel_pcie_port *lpp, u32 ofs) >> +{ >> + return dw_pcie_readl_dbi(&lpp->pci, ofs); >> +} >> + >> +static inline void pcie_rc_cfg_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) >> +{ >> + dw_pcie_writel_dbi(&lpp->pci, ofs, val); >> +} >> + >> +static void pcie_rc_cfg_wr_mask(struct intel_pcie_port *lpp, >> + u32 mask, u32 val, u32 ofs) >> +{ >> + pcie_update_bits(lpp->pci.dbi_base, mask, val, ofs); >> +} >> + >> +static void intel_pcie_ltssm_enable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, >> + PCIE_APP_CCR_LTSSM_ENABLE, PCIE_APP_CCR); >> +} >> + >> +static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); >> +} >> + >> +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 val; >> + >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >> + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >> + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); >> + >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + >> + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); >> + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | >> + PCI_EXP_LNKCTL_RCB; >> + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> +} >> + >> +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 reg, val; >> + >> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >> + switch (lpp->link_gen) { >> + case PCIE_LINK_SPEED_GEN1: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_2_5GT; >> + break; >> + case PCIE_LINK_SPEED_GEN2: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_5_0GT; >> + break; >> + case PCIE_LINK_SPEED_GEN3: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_8_0GT; >> + break; >> + case PCIE_LINK_SPEED_GEN4: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_16_0GT; >> + break; >> + default: >> + /* Use hardware capability */ >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >> + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >> + reg &= ~PCI_EXP_LNKCTL2_HASD; >> + reg |= val; >> + break; >> + } >> + >> + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >> + dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts); >> +} >> + >> + > Reduce the number of empty lines here. Ok, will remove it. > >> + >> +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 val, mask; >> + >> + switch (lpp->max_speed) { >> + case PCIE_LINK_SPEED_GEN3: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN3; >> + break; >> + case PCIE_LINK_SPEED_GEN4: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN4; >> + break; >> + default: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN12_DFT; >> + break; >> + } >> + >> + mask = PORT_AFR_N_FTS_MASK | PORT_AFR_CC_N_FTS_MASK; >> + val = FIELD_PREP(PORT_AFR_N_FTS_MASK, lpp->n_fts) | >> + FIELD_PREP(PORT_AFR_CC_N_FTS_MASK, lpp->n_fts); >> + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_PORT_AFR); >> + >> + /* Port Link Control Register */ >> + pcie_rc_cfg_wr_mask(lpp, PORT_LINK_DLL_LINK_EN, >> + PORT_LINK_DLL_LINK_EN, PCIE_PORT_LINK_CONTROL); >> +} >> + >> +static void intel_pcie_rc_setup(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_ltssm_disable(lpp); >> + intel_pcie_link_setup(lpp); >> + dw_pcie_setup_rc(&lpp->pci.pp); >> + dw_pcie_upconfig_setup(&lpp->pci); >> + intel_pcie_port_logic_setup(lpp); >> + intel_pcie_max_speed_setup(lpp); >> +} >> + >> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp) >> +{ >> + struct device *dev = lpp->pci.dev; >> + int ret; >> + >> + lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(lpp->reset_gpio)) { >> + ret = PTR_ERR(lpp->reset_gpio); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to request PCIe GPIO: %d\n", ret); >> + return ret; >> + } >> + >> + /* Make initial reset last for 100us */ >> + usleep_range(100, 200); >> + >> + return 0; >> +} >> + >> +static void intel_pcie_core_rst_assert(struct intel_pcie_port *lpp) >> +{ >> + reset_control_assert(lpp->core_rst); >> +} >> + >> +static void intel_pcie_core_rst_deassert(struct intel_pcie_port *lpp) >> +{ >> + /* >> + * One micro-second delay to make sure the reset pulse >> + * wide enough so that core reset is clean. >> + */ >> + udelay(1); >> + reset_control_deassert(lpp->core_rst); >> + >> + /* >> + * Some SoC core reset also reset PHY, more delay needed >> + * to make sure the reset process is done. >> + */ >> + usleep_range(1000, 2000); >> +} >> + >> +static void intel_pcie_device_rst_assert(struct intel_pcie_port *lpp) >> +{ >> + gpiod_set_value_cansleep(lpp->reset_gpio, 1); >> +} >> + >> +static void intel_pcie_device_rst_deassert(struct intel_pcie_port *lpp) >> +{ >> + msleep(lpp->rst_intrvl); >> + gpiod_set_value_cansleep(lpp->reset_gpio, 0); >> +} >> + >> +static int intel_pcie_app_logic_setup(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_device_rst_deassert(lpp); >> + intel_pcie_ltssm_enable(lpp); >> + >> + return dw_pcie_wait_for_link(&lpp->pci); >> +} >> + >> +static void intel_pcie_core_irq_disable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr(lpp, 0, PCIE_APP_IRNEN); >> + pcie_app_wr(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRNCR); >> +} >> + >> +static int intel_pcie_get_resources(struct platform_device *pdev) >> +{ >> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >> + struct dw_pcie *pci = &lpp->pci; >> + struct device *dev = pci->dev; >> + struct resource *res; >> + int ret; >> + >> + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); >> + if (ret) { >> + dev_err(dev, "failed to get domain id, errno %d\n", ret); >> + return ret; >> + } >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); >> + >> + pci->dbi_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(pci->dbi_base)) >> + return PTR_ERR(pci->dbi_base); >> + >> + lpp->core_clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(lpp->core_clk)) { >> + ret = PTR_ERR(lpp->core_clk); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to get clks: %d\n", ret); >> + return ret; >> + } >> + >> + lpp->core_rst = devm_reset_control_get(dev, NULL); >> + if (IS_ERR(lpp->core_rst)) { >> + ret = PTR_ERR(lpp->core_rst); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to get resets: %d\n", ret); >> + return ret; >> + } >> + >> + ret = device_property_match_string(dev, "device_type", "pci"); >> + if (ret) { >> + dev_err(dev, "failed to find pci device type: %d\n", ret); >> + return ret; >> + } >> + >> + if (device_property_read_u32(dev, "reset-assert-ms", &lpp->rst_intrvl)) >> + lpp->rst_intrvl = RST_INTRVL_DFT_MS; >> + >> + ret = of_pci_get_max_link_speed(dev->of_node); >> + lpp->link_gen = ret < 0 ? 0 : ret; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); >> + >> + lpp->app_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(lpp->app_base)) >> + return PTR_ERR(lpp->app_base); >> + >> + lpp->phy = devm_phy_get(dev, "pcie"); >> + if (IS_ERR(lpp->phy)) { >> + ret = PTR_ERR(lpp->phy); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) >> +{ >> + phy_exit(lpp->phy); >> +} >> + >> +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) >> +{ >> + u32 value; >> + int ret; >> + >> + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) >> + return 0; >> + >> + /* Send PME_TURN_OFF message */ >> + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, >> + PCIE_APP_MSG_XMT_PM_TURNOFF, PCIE_APP_MSG_CR); >> + >> + /* Read PMC status and wait for falling into L2 link state */ >> + ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value, >> + (value & PCIE_APP_PMC_IN_L2), 20, >> + jiffies_to_usecs(5 * HZ)); >> + if (ret) >> + dev_err(lpp->pci.dev, "PCIe link enter L2 timeout!\n"); >> + >> + return ret; >> +} >> + >> +static void intel_pcie_turn_off(struct intel_pcie_port *lpp) >> +{ >> + if (dw_pcie_link_up(&lpp->pci)) >> + intel_pcie_wait_l2(lpp); >> + >> + /* Put endpoint device in reset state */ >> + intel_pcie_device_rst_assert(lpp); >> + pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY, 0, PCI_COMMAND); >> +} >> + >> +static int intel_pcie_host_setup(struct intel_pcie_port *lpp) >> +{ >> + int ret; >> + >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_device_rst_assert(lpp); >> + >> + ret = phy_init(lpp->phy); >> + if (ret) >> + return ret; >> + >> + intel_pcie_core_rst_deassert(lpp); >> + >> + ret = clk_prepare_enable(lpp->core_clk); >> + if (ret) { >> + dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret); >> + goto clk_err; >> + } >> + >> + intel_pcie_rc_setup(lpp); >> + ret = intel_pcie_app_logic_setup(lpp); >> + if (ret) >> + goto app_init_err; >> + >> + /* Enable integrated interrupts */ >> + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRN_INT, >> + PCIE_APP_IRNEN); >> + return 0; >> + >> +app_init_err: >> + clk_disable_unprepare(lpp->core_clk); >> +clk_err: >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_deinit_phy(lpp); >> + >> + return ret; >> +} >> + >> +static void __intel_pcie_remove(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_core_irq_disable(lpp); >> + intel_pcie_turn_off(lpp); >> + clk_disable_unprepare(lpp->core_clk); >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_deinit_phy(lpp); >> +} >> + >> +static int intel_pcie_remove(struct platform_device *pdev) >> +{ >> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >> + struct pcie_port *pp = &lpp->pci.pp; >> + >> + dw_pcie_host_deinit(pp); >> + __intel_pcie_remove(lpp); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused intel_pcie_suspend_noirq(struct device *dev) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + int ret; >> + >> + intel_pcie_core_irq_disable(lpp); >> + ret = intel_pcie_wait_l2(lpp); >> + if (ret) >> + return ret; >> + >> + intel_pcie_deinit_phy(lpp); >> + clk_disable_unprepare(lpp->core_clk); >> + return ret; >> +} >> + >> +static int __maybe_unused intel_pcie_resume_noirq(struct device *dev) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + >> + return intel_pcie_host_setup(lpp); >> +} >> + >> +static int intel_pcie_rc_init(struct pcie_port *pp) >> +{ >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> + struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); >> + >> + return intel_pcie_host_setup(lpp); >> +} >> + >> +int intel_pcie_msi_init(struct pcie_port *pp) >> +{ >> + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ >> + return 0; >> +} >> + >> +u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr) >> +{ >> + return cpu_addr + BUS_IATU_OFFS; >> +} >> + >> +static const struct dw_pcie_ops intel_pcie_ops = { >> + .cpu_addr_fixup = intel_pcie_cpu_addr, >> +}; >> + >> +static const struct dw_pcie_host_ops intel_pcie_dw_ops = { >> + .host_init = intel_pcie_rc_init, >> + .msi_host_init = intel_pcie_msi_init, >> +}; >> + >> +static const struct intel_pcie_soc pcie_data = { >> + .pcie_ver = 0x520A, >> + .pcie_atu_offset = 0xC0000, >> + .num_viewport = 3, >> +}; >> + >> +static int intel_pcie_probe(struct platform_device *pdev) >> +{ >> + const struct intel_pcie_soc *data; >> + struct device *dev = &pdev->dev; >> + struct intel_pcie_port *lpp; >> + struct pcie_port *pp; >> + struct dw_pcie *pci; >> + int ret; >> + >> + lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL); >> + if (!lpp) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, lpp); >> + pci = &lpp->pci; >> + pci->dev = dev; >> + pp = &pci->pp; >> + >> + ret = intel_pcie_get_resources(pdev); >> + if (ret) >> + return ret; >> + >> + ret = intel_pcie_ep_rst_init(lpp); >> + if (ret) >> + return ret; >> + >> + data = device_get_match_data(dev); >> + pci->ops = &intel_pcie_ops; >> + pci->version = data->pcie_ver; >> + pci->atu_base = pci->dbi_base + data->pcie_atu_offset; >> + pp->ops = &intel_pcie_dw_ops; >> + >> + ret = dw_pcie_host_init(pp); >> + if (ret) { >> + dev_err(dev, "cannot initialize host\n"); >> + return ret; >> + } >> + >> + /* Intel PCIe doesn't configure IO region, so configure >> + * viewport to not to access IO region during register >> + * read write operations. >> + */ >> + pci->num_viewport = data->num_viewport; >> + dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); >> + >> + return ret; >> +} >> + >> +static const struct dev_pm_ops intel_pcie_pm_ops = { >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pcie_suspend_noirq, >> + intel_pcie_resume_noirq) >> +}; >> + >> +static const struct of_device_id of_intel_pcie_match[] = { >> + { .compatible = "intel,lgm-pcie", .data = &pcie_data }, >> + {} >> +}; >> + >> +static struct platform_driver intel_pcie_driver = { >> + .probe = intel_pcie_probe, >> + .remove = intel_pcie_remove, >> + .driver = { >> + .name = "intel-gw-pcie", >> + .of_match_table = of_intel_pcie_match, >> + .pm = &intel_pcie_pm_ops, >> + }, >> +}; >> +builtin_platform_driver(intel_pcie_driver); >> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h >> index 29d6e93fd15e..f6e7e402f879 100644 >> --- a/include/uapi/linux/pci_regs.h >> +++ b/include/uapi/linux/pci_regs.h >> @@ -673,6 +673,7 @@ >> #define PCI_EXP_LNKCTL2_TLS_8_0GT 0x0003 /* Supported Speed 8GT/s */ >> #define PCI_EXP_LNKCTL2_TLS_16_0GT 0x0004 /* Supported Speed 16GT/s */ >> #define PCI_EXP_LNKCTL2_TLS_32_0GT 0x0005 /* Supported Speed 32GT/s */ >> +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed Disable */ > s/Hw/HW Sure, i will correct it. Thanks for the valuable inputs. Regards, Dilip > >> #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ >> #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */ >> #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ >> -- >> 2.11.0 >
On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: > Add support to PCIe RC controller on Intel Gateway SoCs. > PCIe controller is based of Synopsys DesignWare pci core. > > Intel PCIe driver requires Upconfig support, fast training > sequence configuration and link speed change. So adding the > respective helper functions in the pcie DesignWare framework. > It also programs hardware autonomous speed during speed > configuration so defining it in pci_regs.h. > > Changes on v4: > Rename the driver naming and description to > "PCIe RC controller on Intel Gateway SoCs". > Use PCIe core register macros defined in pci_regs.h > and remove respective local definitions. > Remove pcie core interrupt handling. > Move pcie link control speed change, upconfig and FTS. > configuration functions to DesignWare framework. > Use of_pci_get_max_link_speed(). > Mark dependency on X86 and COMPILE_TEST in Kconfig. > Remove lanes and add n_fts variables in intel_pcie_port structure. > Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. > Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() > Move sysfs attributes specific code to separate patch. > Remove redundant error handling. > Reduce LoCs by doing variable initializations while declaration itself. > Add extra line after closing parenthesis. > Move intel_pcie_ep_rst_init() out of get_resources() > > changes on v3: > Rename PCIe app logic registers with PCIE_APP prefix. > PCIE_IOP_CTRL configuration is not required. Remove respective code. > Remove wrapper functions for clk enable/disable APIs. > Use platform_get_resource_byname() instead of > devm_platform_ioremap_resource() to be similar with DWC framework. > Rename phy name to "pciephy". > Modify debug message in msi_init() callback to be more specific. > Remove map_irq() callback. > Enable the INTx interrupts at the time of PCIe initialization. > Reduce memory fragmentation by using variable "struct dw_pcie pci" > instead of allocating memory. > Reduce the delay to 100us during enpoint initialization > intel_pcie_ep_rst_init(). > Call dw_pcie_host_deinit() during remove() instead of directly > calling PCIe core APIs. > Rename "intel,rst-interval" to "reset-assert-ms". > Remove unused APP logic Interrupt bit macro definitions. > Use dwc framework's dw_pcie_setup_rc() for PCIe host specific > configuration instead of redefining the same functionality in > the driver. > Move the whole DT parsing specific code to intel_pcie_get_resources() > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/pci/controller/dwc/Kconfig | 10 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-designware.c | 34 ++ > drivers/pci/controller/dwc/pcie-designware.h | 12 + > drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++++++++++++++++++++++++++ > include/uapi/linux/pci_regs.h | 1 + > 6 files changed, 648 insertions(+) > create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 0ba988b5b5bc..b33ed1cc873d 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP > order to enable device-specific features PCI_DW_PLAT_EP must be > selected. > > +config PCIE_INTEL_GW > + bool "Intel Gateway PCIe host controller support" > + depends on OF && (X86 || COMPILE_TEST) > + select PCIE_DW_HOST > + help > + Say 'Y' here to enable support for PCIe Host controller driver. This sentence is very generic, we don't even say what controller driver. > + The PCIe controller on Intel Gateway SoCs is based on the Synopsys > + DesignWare pcie core and therefore uses the DesignWare core > + functions for the driver implementation. Thanks for changing the driver name, though the description hasn't really changed since the last revision. In particular, Christoph's feedback mentioned that it would be useful to describe where this IP block may be used - is there any reason why we can't make this more useful for users? > + > config PCI_EXYNOS > bool "Samsung Exynos PCIe controller" > depends on SOC_EXYNOS5440 || COMPILE_TEST > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index b30336181d46..99db83cd2f35 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o > obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o > +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o > obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o > obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o > obj-$(CONFIG_PCI_IMX6) += pci-imx6.o > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 820488dfeaed..4c391bfd681a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -474,6 +474,40 @@ int dw_pcie_link_up(struct dw_pcie *pci) > (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); > } > > + > +void dw_pcie_upconfig_setup(struct dw_pcie *pci) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); > + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, > + val | PORT_MLTI_UPCFG_SUPPORT); > +} > + > +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > + > + if (enable) > + val |= PORT_LOGIC_SPEED_CHANGE; > + else > + val &= ~PORT_LOGIC_SPEED_CHANGE; > + > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > +} It's always great to see code move from specific drivers to parent drivers as it helps reduce duplication of code. Though I notice that imx6_pcie_establish_link (pci-imx6.c) and dw_pcie_setup_rc (pcie-designware-host.c) also does this. Rather than have duplicated code, I think it makes sense to adopt those users to this new code. In any case, the above function isn't used here. Can you add it in any patch that does use it please? > + > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > + val &= ~PORT_LOGIC_N_FTS; > + val |= n_fts; > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > +} I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS and defines a bunch of macros to support this. It doesn't make sense to duplicate this there. Therefore I think we need to update pcie-artpec6.c to use this new function. > + > static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci) > { > u32 val; > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 5a18e94e52c8..3beac10e4a4c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -30,7 +30,12 @@ > #define LINK_WAIT_IATU 9 > > /* Synopsys-specific PCIe configuration registers */ > +#define PCIE_PORT_AFR 0x70C > +#define PORT_AFR_N_FTS_MASK GENMASK(15, 8) > +#define PORT_AFR_CC_N_FTS_MASK GENMASK(23, 16) > + > #define PCIE_PORT_LINK_CONTROL 0x710 > +#define PORT_LINK_DLL_LINK_EN BIT(5) > #define PORT_LINK_MODE_MASK GENMASK(21, 16) > #define PORT_LINK_MODE(n) FIELD_PREP(PORT_LINK_MODE_MASK, n) > #define PORT_LINK_MODE_1_LANES PORT_LINK_MODE(0x1) > @@ -46,6 +51,7 @@ > #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) > > #define PCIE_LINK_WIDTH_SPEED_CONTROL 0x80C > +#define PORT_LOGIC_N_FTS GENMASK(7, 0) > #define PORT_LOGIC_SPEED_CHANGE BIT(17) > #define PORT_LOGIC_LINK_WIDTH_MASK GENMASK(12, 8) > #define PORT_LOGIC_LINK_WIDTH(n) FIELD_PREP(PORT_LOGIC_LINK_WIDTH_MASK, n) > @@ -60,6 +66,9 @@ > #define PCIE_MSI_INTR0_MASK 0x82C > #define PCIE_MSI_INTR0_STATUS 0x830 > > +#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > +#define PORT_MLTI_UPCFG_SUPPORT BIT(7) > + > #define PCIE_ATU_VIEWPORT 0x900 > #define PCIE_ATU_REGION_INBOUND BIT(31) > #define PCIE_ATU_REGION_OUTBOUND 0 > @@ -273,6 +282,9 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); > void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > int dw_pcie_link_up(struct dw_pcie *pci); > +void dw_pcie_upconfig_setup(struct dw_pcie *pci); > +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); > int dw_pcie_wait_for_link(struct dw_pcie *pci); > void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, > int type, u64 cpu_addr, u64 pci_addr, > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c > new file mode 100644 > index 000000000000..9142c70db808 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c > @@ -0,0 +1,590 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for Intel Gateway SoCs > + * > + * Copyright (c) 2019 Intel Corporation. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/iopoll.h> > +#include <linux/of_irq.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/pci_regs.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > + > +#include "../../pci.h" I expected this to be removed, is it needed now? > +#include "pcie-designware.h" > + > +#define PCIE_CAP_OFST 0x70 > + > +#define PORT_AFR_N_FTS_GEN12_DFT (SZ_128 - 1) > +#define PORT_AFR_N_FTS_GEN3 180 > +#define PORT_AFR_N_FTS_GEN4 196 > + > +/* PCIe Application logic Registers */ > +#define PCIE_APP_CCR 0x10 > +#define PCIE_APP_CCR_LTSSM_ENABLE BIT(0) > + > +#define PCIE_APP_MSG_CR 0x30 > +#define PCIE_APP_MSG_XMT_PM_TURNOFF BIT(0) > + > +#define PCIE_APP_PMC 0x44 > +#define PCIE_APP_PMC_IN_L2 BIT(20) > + > +#define PCIE_APP_IRNEN 0xF4 > +#define PCIE_APP_IRNCR 0xF8 > +#define PCIE_APP_IRN_AER_REPORT BIT(0) > +#define PCIE_APP_IRN_PME BIT(2) > +#define PCIE_APP_IRN_RX_VDM_MSG BIT(4) > +#define PCIE_APP_IRN_PM_TO_ACK BIT(9) > +#define PCIE_APP_IRN_LINK_AUTO_BW_STAT BIT(11) > +#define PCIE_APP_IRN_BW_MGT BIT(12) > +#define PCIE_APP_IRN_MSG_LTR BIT(18) > +#define PCIE_APP_IRN_SYS_ERR_RC BIT(29) > +#define PCIE_APP_INTX_OFST 12 > + > +#define PCIE_APP_IRN_INT \ > + (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ > + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ > + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ > + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ > + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) > + > +#define BUS_IATU_OFFS SZ_256M > +#define RST_INTRVL_DFT_MS 100 > + > +enum { > + PCIE_LINK_SPEED_AUTO = 0, > + PCIE_LINK_SPEED_GEN1, > + PCIE_LINK_SPEED_GEN2, > + PCIE_LINK_SPEED_GEN3, > + PCIE_LINK_SPEED_GEN4, > +}; > + > +struct intel_pcie_soc { > + unsigned int pcie_ver; > + unsigned int pcie_atu_offset; > + u32 num_viewport; > +}; > + > +struct intel_pcie_port { > + struct dw_pcie pci; > + unsigned int id; /* Physical RC Index */ > + void __iomem *app_base; > + struct gpio_desc *reset_gpio; > + u32 rst_intrvl; > + u32 max_speed; > + u32 link_gen; > + u32 max_width; > + u32 n_fts; > + struct clk *core_clk; > + struct reset_control *core_rst; > + struct phy *phy; > +}; > + > +static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, u32 ofs) > +{ > + u32 orig, tmp; > + > + orig = readl(base + ofs); > + > + tmp = (orig & ~mask) | (val & mask); > + > + if (tmp != orig) > + writel(tmp, base + ofs); > +} > + > +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs) > +{ > + return readl(lpp->app_base + ofs); > +} > + > +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) > +{ > + writel(val, lpp->app_base + ofs); > +} > + > +static void pcie_app_wr_mask(struct intel_pcie_port *lpp, > + u32 mask, u32 val, u32 ofs) > +{ > + pcie_update_bits(lpp->app_base, mask, val, ofs); > +} > + > +static inline u32 pcie_rc_cfg_rd(struct intel_pcie_port *lpp, u32 ofs) > +{ > + return dw_pcie_readl_dbi(&lpp->pci, ofs); > +} > + > +static inline void pcie_rc_cfg_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) > +{ > + dw_pcie_writel_dbi(&lpp->pci, ofs, val); > +} > + > +static void pcie_rc_cfg_wr_mask(struct intel_pcie_port *lpp, > + u32 mask, u32 val, u32 ofs) > +{ > + pcie_update_bits(lpp->pci.dbi_base, mask, val, ofs); > +} > + > +static void intel_pcie_ltssm_enable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, > + PCIE_APP_CCR_LTSSM_ENABLE, PCIE_APP_CCR); > +} > + > +static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); > +} > + > +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) > +{ > + u32 val; > + > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); > + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); > + > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + > + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); > + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | > + PCI_EXP_LNKCTL_RCB; > + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > +} > + > +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) > +{ > + u32 reg, val; > + > + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); > + switch (lpp->link_gen) { > + case PCIE_LINK_SPEED_GEN1: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_2_5GT; > + break; > + case PCIE_LINK_SPEED_GEN2: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_5_0GT; > + break; > + case PCIE_LINK_SPEED_GEN3: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_8_0GT; > + break; > + case PCIE_LINK_SPEED_GEN4: > + reg &= ~PCI_EXP_LNKCTL2_TLS; > + reg |= PCI_EXP_LNKCTL2_HASD| > + PCI_EXP_LNKCTL2_TLS_16_0GT; > + break; > + default: > + /* Use hardware capability */ > + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); > + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); > + reg &= ~PCI_EXP_LNKCTL2_HASD; > + reg |= val; > + break; > + } > + > + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); > + dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts); > +} > + > + > + Lots of space here isn't needed. > +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) > +{ > + u32 val, mask; > + > + switch (lpp->max_speed) { > + case PCIE_LINK_SPEED_GEN3: > + lpp->n_fts = PORT_AFR_N_FTS_GEN3; > + break; > + case PCIE_LINK_SPEED_GEN4: > + lpp->n_fts = PORT_AFR_N_FTS_GEN4; > + break; > + default: > + lpp->n_fts = PORT_AFR_N_FTS_GEN12_DFT; > + break; > + } > + > + mask = PORT_AFR_N_FTS_MASK | PORT_AFR_CC_N_FTS_MASK; > + val = FIELD_PREP(PORT_AFR_N_FTS_MASK, lpp->n_fts) | > + FIELD_PREP(PORT_AFR_CC_N_FTS_MASK, lpp->n_fts); > + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_PORT_AFR); > + > + /* Port Link Control Register */ > + pcie_rc_cfg_wr_mask(lpp, PORT_LINK_DLL_LINK_EN, > + PORT_LINK_DLL_LINK_EN, PCIE_PORT_LINK_CONTROL); > +} > + > +static void intel_pcie_rc_setup(struct intel_pcie_port *lpp) > +{ > + intel_pcie_ltssm_disable(lpp); > + intel_pcie_link_setup(lpp); > + dw_pcie_setup_rc(&lpp->pci.pp); > + dw_pcie_upconfig_setup(&lpp->pci); > + intel_pcie_port_logic_setup(lpp); > + intel_pcie_max_speed_setup(lpp); > +} > + > +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp) > +{ > + struct device *dev = lpp->pci.dev; > + int ret; > + > + lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(lpp->reset_gpio)) { > + ret = PTR_ERR(lpp->reset_gpio); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to request PCIe GPIO: %d\n", ret); > + return ret; > + } > + > + /* Make initial reset last for 100us */ > + usleep_range(100, 200); > + > + return 0; > +} > + > +static void intel_pcie_core_rst_assert(struct intel_pcie_port *lpp) > +{ > + reset_control_assert(lpp->core_rst); > +} > + > +static void intel_pcie_core_rst_deassert(struct intel_pcie_port *lpp) > +{ > + /* > + * One micro-second delay to make sure the reset pulse > + * wide enough so that core reset is clean. > + */ > + udelay(1); > + reset_control_deassert(lpp->core_rst); > + > + /* > + * Some SoC core reset also reset PHY, more delay needed > + * to make sure the reset process is done. > + */ > + usleep_range(1000, 2000); > +} > + > +static void intel_pcie_device_rst_assert(struct intel_pcie_port *lpp) > +{ > + gpiod_set_value_cansleep(lpp->reset_gpio, 1); > +} > + > +static void intel_pcie_device_rst_deassert(struct intel_pcie_port *lpp) > +{ > + msleep(lpp->rst_intrvl); > + gpiod_set_value_cansleep(lpp->reset_gpio, 0); > +} > + > +static int intel_pcie_app_logic_setup(struct intel_pcie_port *lpp) > +{ > + intel_pcie_device_rst_deassert(lpp); > + intel_pcie_ltssm_enable(lpp); > + > + return dw_pcie_wait_for_link(&lpp->pci); > +} > + > +static void intel_pcie_core_irq_disable(struct intel_pcie_port *lpp) > +{ > + pcie_app_wr(lpp, 0, PCIE_APP_IRNEN); > + pcie_app_wr(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRNCR); > +} > + > +static int intel_pcie_get_resources(struct platform_device *pdev) > +{ > + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); > + struct dw_pcie *pci = &lpp->pci; > + struct device *dev = pci->dev; > + struct resource *res; > + int ret; > + > + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); > + if (ret) { > + dev_err(dev, "failed to get domain id, errno %d\n", ret); > + return ret; > + } Why is this a required property? > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); > + > + pci->dbi_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > + > + lpp->core_clk = devm_clk_get(dev, NULL); > + if (IS_ERR(lpp->core_clk)) { > + ret = PTR_ERR(lpp->core_clk); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to get clks: %d\n", ret); > + return ret; > + } > + > + lpp->core_rst = devm_reset_control_get(dev, NULL); > + if (IS_ERR(lpp->core_rst)) { > + ret = PTR_ERR(lpp->core_rst); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to get resets: %d\n", ret); > + return ret; > + } > + > + ret = device_property_match_string(dev, "device_type", "pci"); > + if (ret) { > + dev_err(dev, "failed to find pci device type: %d\n", ret); > + return ret; > + } > + > + if (device_property_read_u32(dev, "reset-assert-ms", &lpp->rst_intrvl)) > + lpp->rst_intrvl = RST_INTRVL_DFT_MS; > + > + ret = of_pci_get_max_link_speed(dev->of_node); > + lpp->link_gen = ret < 0 ? 0 : ret; I think the spacing may be a little off above. > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > + > + lpp->app_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(lpp->app_base)) > + return PTR_ERR(lpp->app_base); > + > + lpp->phy = devm_phy_get(dev, "pcie"); > + if (IS_ERR(lpp->phy)) { > + ret = PTR_ERR(lpp->phy); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) > +{ > + phy_exit(lpp->phy); > +} > + > +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) > +{ > + u32 value; > + int ret; > + > + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) > + return 0; > + > + /* Send PME_TURN_OFF message */ > + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, > + PCIE_APP_MSG_XMT_PM_TURNOFF, PCIE_APP_MSG_CR); > + > + /* Read PMC status and wait for falling into L2 link state */ > + ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value, > + (value & PCIE_APP_PMC_IN_L2), 20, > + jiffies_to_usecs(5 * HZ)); > + if (ret) > + dev_err(lpp->pci.dev, "PCIe link enter L2 timeout!\n"); > + > + return ret; > +} > + > +static void intel_pcie_turn_off(struct intel_pcie_port *lpp) > +{ > + if (dw_pcie_link_up(&lpp->pci)) > + intel_pcie_wait_l2(lpp); > + > + /* Put endpoint device in reset state */ > + intel_pcie_device_rst_assert(lpp); > + pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY, 0, PCI_COMMAND); > +} > + > +static int intel_pcie_host_setup(struct intel_pcie_port *lpp) > +{ > + int ret; > + > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_device_rst_assert(lpp); > + > + ret = phy_init(lpp->phy); > + if (ret) > + return ret; > + > + intel_pcie_core_rst_deassert(lpp); > + > + ret = clk_prepare_enable(lpp->core_clk); > + if (ret) { > + dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret); > + goto clk_err; > + } > + > + intel_pcie_rc_setup(lpp); > + ret = intel_pcie_app_logic_setup(lpp); > + if (ret) > + goto app_init_err; > + > + /* Enable integrated interrupts */ > + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRN_INT, > + PCIE_APP_IRNEN); > + return 0; > + > +app_init_err: > + clk_disable_unprepare(lpp->core_clk); > +clk_err: > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_deinit_phy(lpp); > + > + return ret; > +} > + > +static void __intel_pcie_remove(struct intel_pcie_port *lpp) > +{ > + intel_pcie_core_irq_disable(lpp); > + intel_pcie_turn_off(lpp); > + clk_disable_unprepare(lpp->core_clk); > + intel_pcie_core_rst_assert(lpp); > + intel_pcie_deinit_phy(lpp); > +} > + > +static int intel_pcie_remove(struct platform_device *pdev) > +{ > + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); > + struct pcie_port *pp = &lpp->pci.pp; > + > + dw_pcie_host_deinit(pp); > + __intel_pcie_remove(lpp); > + > + return 0; > +} > + > +static int __maybe_unused intel_pcie_suspend_noirq(struct device *dev) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + int ret; > + > + intel_pcie_core_irq_disable(lpp); > + ret = intel_pcie_wait_l2(lpp); > + if (ret) > + return ret; > + > + intel_pcie_deinit_phy(lpp); > + clk_disable_unprepare(lpp->core_clk); > + return ret; > +} > + > +static int __maybe_unused intel_pcie_resume_noirq(struct device *dev) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + > + return intel_pcie_host_setup(lpp); > +} > + > +static int intel_pcie_rc_init(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); > + > + return intel_pcie_host_setup(lpp); > +} > + > +int intel_pcie_msi_init(struct pcie_port *pp) > +{ > + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ > + return 0; > +} > + > +u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr) > +{ > + return cpu_addr + BUS_IATU_OFFS; > +} > + > +static const struct dw_pcie_ops intel_pcie_ops = { > + .cpu_addr_fixup = intel_pcie_cpu_addr, > +}; > + > +static const struct dw_pcie_host_ops intel_pcie_dw_ops = { > + .host_init = intel_pcie_rc_init, > + .msi_host_init = intel_pcie_msi_init, > +}; > + > +static const struct intel_pcie_soc pcie_data = { > + .pcie_ver = 0x520A, > + .pcie_atu_offset = 0xC0000, > + .num_viewport = 3, > +}; > + > +static int intel_pcie_probe(struct platform_device *pdev) > +{ > + const struct intel_pcie_soc *data; > + struct device *dev = &pdev->dev; > + struct intel_pcie_port *lpp; > + struct pcie_port *pp; > + struct dw_pcie *pci; > + int ret; > + > + lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL); > + if (!lpp) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, lpp); > + pci = &lpp->pci; > + pci->dev = dev; > + pp = &pci->pp; > + > + ret = intel_pcie_get_resources(pdev); > + if (ret) > + return ret; > + > + ret = intel_pcie_ep_rst_init(lpp); > + if (ret) > + return ret; > + > + data = device_get_match_data(dev); > + pci->ops = &intel_pcie_ops; > + pci->version = data->pcie_ver; > + pci->atu_base = pci->dbi_base + data->pcie_atu_offset; > + pp->ops = &intel_pcie_dw_ops; > + > + ret = dw_pcie_host_init(pp); > + if (ret) { > + dev_err(dev, "cannot initialize host\n"); > + return ret; > + } > + > + /* Intel PCIe doesn't configure IO region, so configure > + * viewport to not to access IO region during register > + * read write operations. > + */ > + pci->num_viewport = data->num_viewport; > + dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); > + > + return ret; > +} > + > +static const struct dev_pm_ops intel_pcie_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pcie_suspend_noirq, > + intel_pcie_resume_noirq) > +}; > + > +static const struct of_device_id of_intel_pcie_match[] = { > + { .compatible = "intel,lgm-pcie", .data = &pcie_data }, > + {} > +}; > + > +static struct platform_driver intel_pcie_driver = { > + .probe = intel_pcie_probe, > + .remove = intel_pcie_remove, > + .driver = { > + .name = "intel-gw-pcie", > + .of_match_table = of_intel_pcie_match, > + .pm = &intel_pcie_pm_ops, > + }, > +}; > +builtin_platform_driver(intel_pcie_driver); > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > index 29d6e93fd15e..f6e7e402f879 100644 > --- a/include/uapi/linux/pci_regs.h > +++ b/include/uapi/linux/pci_regs.h > @@ -673,6 +673,7 @@ > #define PCI_EXP_LNKCTL2_TLS_8_0GT 0x0003 /* Supported Speed 8GT/s */ > #define PCI_EXP_LNKCTL2_TLS_16_0GT 0x0004 /* Supported Speed 16GT/s */ > #define PCI_EXP_LNKCTL2_TLS_32_0GT 0x0005 /* Supported Speed 32GT/s */ > +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed Disable */ I think this should be 0x0020. Thanks, Andrew Murray > #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ > #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */ > #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ > -- > 2.11.0 >
On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > PCIe RC driver on Intel Gateway SoCs have a requirement > of changing link width and speed on the fly. > So add the sysfs attributes to show and store the link > properties. > Add the respective link resize function in pcie DesignWare > framework so that Intel PCIe driver can use during link > width configuration on the fly. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 9 +++ > drivers/pci/controller/dwc/pcie-designware.h | 3 + > drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++++++++++++++++++++++++++- > 3 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 4c391bfd681a..662fdcb4f2d6 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) > (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); > } > > +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) > +{ > + u32 val; > + > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); > + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); > + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; > + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); > +} > > void dw_pcie_upconfig_setup(struct dw_pcie *pci) > { > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 3beac10e4a4c..fcf0442341fd 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -67,6 +67,8 @@ > #define PCIE_MSI_INTR0_STATUS 0x830 > > #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) > +#define PORT_MLTI_LNK_WDTH_CHNG BIT(6) > #define PORT_MLTI_UPCFG_SUPPORT BIT(7) > > #define PCIE_ATU_VIEWPORT 0x900 > @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); > void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); > int dw_pcie_link_up(struct dw_pcie *pci); > +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); > void dw_pcie_upconfig_setup(struct dw_pcie *pci); > void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); > void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c > index 9142c70db808..b9be0921671d 100644 > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c > @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) > pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); > } > > +static const char *pcie_link_gen_to_str(int gen) > +{ > + switch (gen) { > + case PCIE_LINK_SPEED_GEN1: > + return "2.5"; > + case PCIE_LINK_SPEED_GEN2: > + return "5.0"; > + case PCIE_LINK_SPEED_GEN3: > + return "8.0"; > + case PCIE_LINK_SPEED_GEN4: > + return "16.0"; > + default: > + return "???"; > + } > +} > + > static void intel_pcie_link_setup(struct intel_pcie_port *lpp) > { > u32 val; > @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) > return ret; > } > > +static ssize_t pcie_link_status_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + u32 reg, width, gen; > + > + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); > + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); > + > + if (gen > lpp->max_speed) > + return -EINVAL; > + > + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, > + width, pcie_link_gen_to_str(gen)); > +} > +static DEVICE_ATTR_RO(pcie_link_status); > + > +static ssize_t pcie_speed_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + unsigned long val; > + int ret; > + > + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val > lpp->max_speed) > + return -EINVAL; > + > + lpp->link_gen = val; > + intel_pcie_max_speed_setup(lpp); > + dw_pcie_link_speed_change(&lpp->pci, false); > + dw_pcie_link_speed_change(&lpp->pci, true); > + > + return len; > +} > +static DEVICE_ATTR_WO(pcie_speed); > + > +/* > + * Link width change on the fly is not always successful. > + * It also depends on the partner. > + */ > +static ssize_t pcie_width_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > + unsigned long val; > + int ret; > + > + lpp = dev_get_drvdata(dev); > + > + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val > lpp->max_width) > + return -EINVAL; > + > + /* HW auto bandwidth negotiation must be enabled */ > + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, > + PCIE_CAP_OFST + PCI_EXP_LNKCTL); > + dw_pcie_link_width_resize(&lpp->pci, val); > + > + return len; > +} > +static DEVICE_ATTR_WO(pcie_width); > + > +static struct attribute *pcie_cfg_attrs[] = { > + &dev_attr_pcie_link_status.attr, > + &dev_attr_pcie_speed.attr, > + &dev_attr_pcie_width.attr, > + NULL, > +}; Is there a reason that these are limited only to the Intel driver and not the wider set of DWC drivers? Is there anything specific here about the Intel GW driver? Thanks, Andrew Murray > +ATTRIBUTE_GROUPS(pcie_cfg); > + > +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp) > +{ > + return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups); > +} > + > static void __intel_pcie_remove(struct intel_pcie_port *lpp) > { > intel_pcie_core_irq_disable(lpp); > @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); > + int ret; > > - return intel_pcie_host_setup(lpp); > + ret = intel_pcie_host_setup(lpp); > + if (ret) > + return ret; > + > + ret = intel_pcie_sysfs_init(lpp); > + if (ret) > + __intel_pcie_remove(lpp); > + > + return ret; > } > > int intel_pcie_msi_init(struct pcie_port *pp) > -- > 2.11.0 >
On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > PCIe RC driver on Intel Gateway SoCs have a requirement > > of changing link width and speed on the fly. Please add more details about why this is needed. Since you're adding sysfs files, it sounds like it's not actually the *driver* that needs this; it's something in userspace? The normal scenario is that the hardware negotiates link widths and speeds without any software involvement (PCIe r5.0, sec 1.2). If this is to work around hardware defects, we should try to do that inside the kernel because we can't expect userspace to do it reliably. As Andrew points out below, this all sounds like it should be generic rather than Intel-specific. > > So add the sysfs attributes to show and store the link > > properties. > > Add the respective link resize function in pcie DesignWare > > framework so that Intel PCIe driver can use during link > > width configuration on the fly. > > ... > > +static ssize_t pcie_link_status_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > > + u32 reg, width, gen; > > + > > + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); > > + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); > > + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); > > + > > + if (gen > lpp->max_speed) > > + return -EINVAL; > > + > > + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, > > + width, pcie_link_gen_to_str(gen)); > > +} > > +static DEVICE_ATTR_RO(pcie_link_status); We already have generic current_link_speed and current_link_width files. > > +static ssize_t pcie_speed_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > > + unsigned long val; > > + int ret; > > + > > + ret = kstrtoul(buf, 10, &val); > > + if (ret) > > + return ret; > > + > > + if (val > lpp->max_speed) > > + return -EINVAL; > > + > > + lpp->link_gen = val; > > + intel_pcie_max_speed_setup(lpp); > > + dw_pcie_link_speed_change(&lpp->pci, false); > > + dw_pcie_link_speed_change(&lpp->pci, true); > > + > > + return len; > > +} > > +static DEVICE_ATTR_WO(pcie_speed); > > + > > +/* > > + * Link width change on the fly is not always successful. > > + * It also depends on the partner. > > + */ > > +static ssize_t pcie_width_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > > + unsigned long val; > > + int ret; > > + > > + lpp = dev_get_drvdata(dev); > > + > > + ret = kstrtoul(buf, 10, &val); > > + if (ret) > > + return ret; > > + > > + if (val > lpp->max_width) > > + return -EINVAL; > > + > > + /* HW auto bandwidth negotiation must be enabled */ > > + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, > > + PCIE_CAP_OFST + PCI_EXP_LNKCTL); > > + dw_pcie_link_width_resize(&lpp->pci, val); > > + > > + return len; > > +} > > +static DEVICE_ATTR_WO(pcie_width); > > + > > +static struct attribute *pcie_cfg_attrs[] = { > > + &dev_attr_pcie_link_status.attr, > > + &dev_attr_pcie_speed.attr, > > + &dev_attr_pcie_width.attr, > > + NULL, > > +}; > > Is there a reason that these are limited only to the Intel driver and > not the wider set of DWC drivers? > > Is there anything specific here about the Intel GW driver?
Hi Andrew Murray, On 10/21/2019 9:03 PM, Andrew Murray wrote: > On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: >> Add support to PCIe RC controller on Intel Gateway SoCs. >> PCIe controller is based of Synopsys DesignWare pci core. >> >> Intel PCIe driver requires Upconfig support, fast training >> sequence configuration and link speed change. So adding the >> respective helper functions in the pcie DesignWare framework. >> It also programs hardware autonomous speed during speed >> configuration so defining it in pci_regs.h. >> >> Changes on v4: >> Rename the driver naming and description to >> "PCIe RC controller on Intel Gateway SoCs". >> Use PCIe core register macros defined in pci_regs.h >> and remove respective local definitions. >> Remove pcie core interrupt handling. >> Move pcie link control speed change, upconfig and FTS. >> configuration functions to DesignWare framework. >> Use of_pci_get_max_link_speed(). >> Mark dependency on X86 and COMPILE_TEST in Kconfig. >> Remove lanes and add n_fts variables in intel_pcie_port structure. >> Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. >> Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() >> Move sysfs attributes specific code to separate patch. >> Remove redundant error handling. >> Reduce LoCs by doing variable initializations while declaration itself. >> Add extra line after closing parenthesis. >> Move intel_pcie_ep_rst_init() out of get_resources() >> >> changes on v3: >> Rename PCIe app logic registers with PCIE_APP prefix. >> PCIE_IOP_CTRL configuration is not required. Remove respective code. >> Remove wrapper functions for clk enable/disable APIs. >> Use platform_get_resource_byname() instead of >> devm_platform_ioremap_resource() to be similar with DWC framework. >> Rename phy name to "pciephy". >> Modify debug message in msi_init() callback to be more specific. >> Remove map_irq() callback. >> Enable the INTx interrupts at the time of PCIe initialization. >> Reduce memory fragmentation by using variable "struct dw_pcie pci" >> instead of allocating memory. >> Reduce the delay to 100us during enpoint initialization >> intel_pcie_ep_rst_init(). >> Call dw_pcie_host_deinit() during remove() instead of directly >> calling PCIe core APIs. >> Rename "intel,rst-interval" to "reset-assert-ms". >> Remove unused APP logic Interrupt bit macro definitions. >> Use dwc framework's dw_pcie_setup_rc() for PCIe host specific >> configuration instead of redefining the same functionality in >> the driver. >> Move the whole DT parsing specific code to intel_pcie_get_resources() >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/pci/controller/dwc/Kconfig | 10 + >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-designware.c | 34 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 12 + >> drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++++++++++++++++++++++++++ >> include/uapi/linux/pci_regs.h | 1 + >> 6 files changed, 648 insertions(+) >> create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c >> >> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig >> index 0ba988b5b5bc..b33ed1cc873d 100644 >> --- a/drivers/pci/controller/dwc/Kconfig >> +++ b/drivers/pci/controller/dwc/Kconfig >> @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP >> order to enable device-specific features PCI_DW_PLAT_EP must be >> selected. >> >> +config PCIE_INTEL_GW >> + bool "Intel Gateway PCIe host controller support" >> + depends on OF && (X86 || COMPILE_TEST) >> + select PCIE_DW_HOST >> + help >> + Say 'Y' here to enable support for PCIe Host controller driver. > This sentence is very generic, we don't even say what controller driver. > >> + The PCIe controller on Intel Gateway SoCs is based on the Synopsys >> + DesignWare pcie core and therefore uses the DesignWare core >> + functions for the driver implementation. > Thanks for changing the driver name, though the description hasn't really > changed since the last revision. In particular, Christoph's feedback mentioned > that it would be useful to describe where this IP block may be used - is there > any reason why we can't make this more useful for users? I will add more information. > >> + >> config PCI_EXYNOS >> bool "Samsung Exynos PCIe controller" >> depends on SOC_EXYNOS5440 || COMPILE_TEST >> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile >> index b30336181d46..99db83cd2f35 100644 >> --- a/drivers/pci/controller/dwc/Makefile >> +++ b/drivers/pci/controller/dwc/Makefile >> @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o >> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o >> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o >> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o >> +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o >> obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o >> obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o >> obj-$(CONFIG_PCI_IMX6) += pci-imx6.o >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 820488dfeaed..4c391bfd681a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -474,6 +474,40 @@ int dw_pcie_link_up(struct dw_pcie *pci) >> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); >> } >> >> + >> +void dw_pcie_upconfig_setup(struct dw_pcie *pci) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); >> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, >> + val | PORT_MLTI_UPCFG_SUPPORT); >> +} >> + >> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >> + >> + if (enable) >> + val |= PORT_LOGIC_SPEED_CHANGE; >> + else >> + val &= ~PORT_LOGIC_SPEED_CHANGE; >> + >> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >> +} > It's always great to see code move from specific drivers to parent drivers > as it helps reduce duplication of code. > > Though I notice that imx6_pcie_establish_link (pci-imx6.c) and > dw_pcie_setup_rc (pcie-designware-host.c) also does this. Rather than have > duplicated code, I think it makes sense to adopt those users to this new > code. > > In any case, the above function isn't used here. Can you add it in any > patch that does use it please? In the earlier patch version this function is being called in intel_pcie_rc_setup(). After noticing this operation is being performed in dw_pcie_setup_rc(), i removed the dw_pcie_link_speed_change() function call, but missed to move the function definition to sysfs attribute patch as it is not being called here. I will move it to the sysfs attribute patch. Thanks for pointing it. > >> + >> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >> + val &= ~PORT_LOGIC_N_FTS; >> + val |= n_fts; >> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >> +} > I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS > and defines a bunch of macros to support this. It doesn't make sense to > duplicate this there. Therefore I think we need to update pcie-artpec6.c > to use this new function. I think we can do in a separate patch after these changes get merged and keep this patch series for intel PCIe driver and required changes in PCIe DesignWare framework. > >> + >> static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci) >> { >> u32 val; >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 5a18e94e52c8..3beac10e4a4c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -30,7 +30,12 @@ >> #define LINK_WAIT_IATU 9 >> >> /* Synopsys-specific PCIe configuration registers */ >> +#define PCIE_PORT_AFR 0x70C >> +#define PORT_AFR_N_FTS_MASK GENMASK(15, 8) >> +#define PORT_AFR_CC_N_FTS_MASK GENMASK(23, 16) >> + >> #define PCIE_PORT_LINK_CONTROL 0x710 >> +#define PORT_LINK_DLL_LINK_EN BIT(5) >> #define PORT_LINK_MODE_MASK GENMASK(21, 16) >> #define PORT_LINK_MODE(n) FIELD_PREP(PORT_LINK_MODE_MASK, n) >> #define PORT_LINK_MODE_1_LANES PORT_LINK_MODE(0x1) >> @@ -46,6 +51,7 @@ >> #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) >> >> #define PCIE_LINK_WIDTH_SPEED_CONTROL 0x80C >> +#define PORT_LOGIC_N_FTS GENMASK(7, 0) >> #define PORT_LOGIC_SPEED_CHANGE BIT(17) >> #define PORT_LOGIC_LINK_WIDTH_MASK GENMASK(12, 8) >> #define PORT_LOGIC_LINK_WIDTH(n) FIELD_PREP(PORT_LOGIC_LINK_WIDTH_MASK, n) >> @@ -60,6 +66,9 @@ >> #define PCIE_MSI_INTR0_MASK 0x82C >> #define PCIE_MSI_INTR0_STATUS 0x830 >> >> +#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >> +#define PORT_MLTI_UPCFG_SUPPORT BIT(7) >> + >> #define PCIE_ATU_VIEWPORT 0x900 >> #define PCIE_ATU_REGION_INBOUND BIT(31) >> #define PCIE_ATU_REGION_OUTBOUND 0 >> @@ -273,6 +282,9 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); >> void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> int dw_pcie_link_up(struct dw_pcie *pci); >> +void dw_pcie_upconfig_setup(struct dw_pcie *pci); >> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); >> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); >> int dw_pcie_wait_for_link(struct dw_pcie *pci); >> void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, >> int type, u64 cpu_addr, u64 pci_addr, >> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c >> new file mode 100644 >> index 000000000000..9142c70db808 >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c >> @@ -0,0 +1,590 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PCIe host controller driver for Intel Gateway SoCs >> + * >> + * Copyright (c) 2019 Intel Corporation. >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/clk.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/interrupt.h> >> +#include <linux/iopoll.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_pci.h> >> +#include <linux/of_platform.h> >> +#include <linux/pci_regs.h> >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset.h> >> + >> +#include "../../pci.h" > I expected this to be removed, is it needed now? In the earlier patch you suggested to use "of_pci_get_max_link_speed()" instead of device_get_*. And, pci.h is required for it. > >> +#include "pcie-designware.h" >> + >> +#define PCIE_CAP_OFST 0x70 >> + >> +#define PORT_AFR_N_FTS_GEN12_DFT (SZ_128 - 1) >> +#define PORT_AFR_N_FTS_GEN3 180 >> +#define PORT_AFR_N_FTS_GEN4 196 >> + >> +/* PCIe Application logic Registers */ >> +#define PCIE_APP_CCR 0x10 >> +#define PCIE_APP_CCR_LTSSM_ENABLE BIT(0) >> + >> +#define PCIE_APP_MSG_CR 0x30 >> +#define PCIE_APP_MSG_XMT_PM_TURNOFF BIT(0) >> + >> +#define PCIE_APP_PMC 0x44 >> +#define PCIE_APP_PMC_IN_L2 BIT(20) >> + >> +#define PCIE_APP_IRNEN 0xF4 >> +#define PCIE_APP_IRNCR 0xF8 >> +#define PCIE_APP_IRN_AER_REPORT BIT(0) >> +#define PCIE_APP_IRN_PME BIT(2) >> +#define PCIE_APP_IRN_RX_VDM_MSG BIT(4) >> +#define PCIE_APP_IRN_PM_TO_ACK BIT(9) >> +#define PCIE_APP_IRN_LINK_AUTO_BW_STAT BIT(11) >> +#define PCIE_APP_IRN_BW_MGT BIT(12) >> +#define PCIE_APP_IRN_MSG_LTR BIT(18) >> +#define PCIE_APP_IRN_SYS_ERR_RC BIT(29) >> +#define PCIE_APP_INTX_OFST 12 >> + >> +#define PCIE_APP_IRN_INT \ >> + (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ >> + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ >> + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ >> + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ >> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) >> + >> +#define BUS_IATU_OFFS SZ_256M >> +#define RST_INTRVL_DFT_MS 100 >> + >> +enum { >> + PCIE_LINK_SPEED_AUTO = 0, >> + PCIE_LINK_SPEED_GEN1, >> + PCIE_LINK_SPEED_GEN2, >> + PCIE_LINK_SPEED_GEN3, >> + PCIE_LINK_SPEED_GEN4, >> +}; >> + >> +struct intel_pcie_soc { >> + unsigned int pcie_ver; >> + unsigned int pcie_atu_offset; >> + u32 num_viewport; >> +}; >> + >> +struct intel_pcie_port { >> + struct dw_pcie pci; >> + unsigned int id; /* Physical RC Index */ >> + void __iomem *app_base; >> + struct gpio_desc *reset_gpio; >> + u32 rst_intrvl; >> + u32 max_speed; >> + u32 link_gen; >> + u32 max_width; >> + u32 n_fts; >> + struct clk *core_clk; >> + struct reset_control *core_rst; >> + struct phy *phy; >> +}; >> + >> +static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, u32 ofs) >> +{ >> + u32 orig, tmp; >> + >> + orig = readl(base + ofs); >> + >> + tmp = (orig & ~mask) | (val & mask); >> + >> + if (tmp != orig) >> + writel(tmp, base + ofs); >> +} >> + >> +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs) >> +{ >> + return readl(lpp->app_base + ofs); >> +} >> + >> +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) >> +{ >> + writel(val, lpp->app_base + ofs); >> +} >> + >> +static void pcie_app_wr_mask(struct intel_pcie_port *lpp, >> + u32 mask, u32 val, u32 ofs) >> +{ >> + pcie_update_bits(lpp->app_base, mask, val, ofs); >> +} >> + >> +static inline u32 pcie_rc_cfg_rd(struct intel_pcie_port *lpp, u32 ofs) >> +{ >> + return dw_pcie_readl_dbi(&lpp->pci, ofs); >> +} >> + >> +static inline void pcie_rc_cfg_wr(struct intel_pcie_port *lpp, u32 val, u32 ofs) >> +{ >> + dw_pcie_writel_dbi(&lpp->pci, ofs, val); >> +} >> + >> +static void pcie_rc_cfg_wr_mask(struct intel_pcie_port *lpp, >> + u32 mask, u32 val, u32 ofs) >> +{ >> + pcie_update_bits(lpp->pci.dbi_base, mask, val, ofs); >> +} >> + >> +static void intel_pcie_ltssm_enable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, >> + PCIE_APP_CCR_LTSSM_ENABLE, PCIE_APP_CCR); >> +} >> + >> +static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); >> +} >> + >> +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 val; >> + >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >> + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >> + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); >> + >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + >> + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); >> + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | >> + PCI_EXP_LNKCTL_RCB; >> + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> +} >> + >> +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 reg, val; >> + >> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >> + switch (lpp->link_gen) { >> + case PCIE_LINK_SPEED_GEN1: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_2_5GT; >> + break; >> + case PCIE_LINK_SPEED_GEN2: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_5_0GT; >> + break; >> + case PCIE_LINK_SPEED_GEN3: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_8_0GT; >> + break; >> + case PCIE_LINK_SPEED_GEN4: >> + reg &= ~PCI_EXP_LNKCTL2_TLS; >> + reg |= PCI_EXP_LNKCTL2_HASD| >> + PCI_EXP_LNKCTL2_TLS_16_0GT; >> + break; >> + default: >> + /* Use hardware capability */ >> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >> + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >> + reg &= ~PCI_EXP_LNKCTL2_HASD; >> + reg |= val; >> + break; >> + } >> + >> + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >> + dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts); >> +} >> + >> + >> + > Lots of space here isn't needed. i will remove it. > >> +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) >> +{ >> + u32 val, mask; >> + >> + switch (lpp->max_speed) { >> + case PCIE_LINK_SPEED_GEN3: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN3; >> + break; >> + case PCIE_LINK_SPEED_GEN4: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN4; >> + break; >> + default: >> + lpp->n_fts = PORT_AFR_N_FTS_GEN12_DFT; >> + break; >> + } >> + >> + mask = PORT_AFR_N_FTS_MASK | PORT_AFR_CC_N_FTS_MASK; >> + val = FIELD_PREP(PORT_AFR_N_FTS_MASK, lpp->n_fts) | >> + FIELD_PREP(PORT_AFR_CC_N_FTS_MASK, lpp->n_fts); >> + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_PORT_AFR); >> + >> + /* Port Link Control Register */ >> + pcie_rc_cfg_wr_mask(lpp, PORT_LINK_DLL_LINK_EN, >> + PORT_LINK_DLL_LINK_EN, PCIE_PORT_LINK_CONTROL); >> +} >> + >> +static void intel_pcie_rc_setup(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_ltssm_disable(lpp); >> + intel_pcie_link_setup(lpp); >> + dw_pcie_setup_rc(&lpp->pci.pp); >> + dw_pcie_upconfig_setup(&lpp->pci); >> + intel_pcie_port_logic_setup(lpp); >> + intel_pcie_max_speed_setup(lpp); >> +} >> + >> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp) >> +{ >> + struct device *dev = lpp->pci.dev; >> + int ret; >> + >> + lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(lpp->reset_gpio)) { >> + ret = PTR_ERR(lpp->reset_gpio); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to request PCIe GPIO: %d\n", ret); >> + return ret; >> + } >> + >> + /* Make initial reset last for 100us */ >> + usleep_range(100, 200); >> + >> + return 0; >> +} >> + >> +static void intel_pcie_core_rst_assert(struct intel_pcie_port *lpp) >> +{ >> + reset_control_assert(lpp->core_rst); >> +} >> + >> +static void intel_pcie_core_rst_deassert(struct intel_pcie_port *lpp) >> +{ >> + /* >> + * One micro-second delay to make sure the reset pulse >> + * wide enough so that core reset is clean. >> + */ >> + udelay(1); >> + reset_control_deassert(lpp->core_rst); >> + >> + /* >> + * Some SoC core reset also reset PHY, more delay needed >> + * to make sure the reset process is done. >> + */ >> + usleep_range(1000, 2000); >> +} >> + >> +static void intel_pcie_device_rst_assert(struct intel_pcie_port *lpp) >> +{ >> + gpiod_set_value_cansleep(lpp->reset_gpio, 1); >> +} >> + >> +static void intel_pcie_device_rst_deassert(struct intel_pcie_port *lpp) >> +{ >> + msleep(lpp->rst_intrvl); >> + gpiod_set_value_cansleep(lpp->reset_gpio, 0); >> +} >> + >> +static int intel_pcie_app_logic_setup(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_device_rst_deassert(lpp); >> + intel_pcie_ltssm_enable(lpp); >> + >> + return dw_pcie_wait_for_link(&lpp->pci); >> +} >> + >> +static void intel_pcie_core_irq_disable(struct intel_pcie_port *lpp) >> +{ >> + pcie_app_wr(lpp, 0, PCIE_APP_IRNEN); >> + pcie_app_wr(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRNCR); >> +} >> + >> +static int intel_pcie_get_resources(struct platform_device *pdev) >> +{ >> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >> + struct dw_pcie *pci = &lpp->pci; >> + struct device *dev = pci->dev; >> + struct resource *res; >> + int ret; >> + >> + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); >> + if (ret) { >> + dev_err(dev, "failed to get domain id, errno %d\n", ret); >> + return ret; >> + } > Why is this a required property? I marked it required property because lpp->id is being used during debug prints. -> sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, -> dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); Hmmm.. I found, domain id can be traversed from pci_host_bridge structure after dw_pcie_host_init(). I will remove the code for getting the pci-domain here. > >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); >> + >> + pci->dbi_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(pci->dbi_base)) >> + return PTR_ERR(pci->dbi_base); >> + >> + lpp->core_clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(lpp->core_clk)) { >> + ret = PTR_ERR(lpp->core_clk); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to get clks: %d\n", ret); >> + return ret; >> + } >> + >> + lpp->core_rst = devm_reset_control_get(dev, NULL); >> + if (IS_ERR(lpp->core_rst)) { >> + ret = PTR_ERR(lpp->core_rst); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to get resets: %d\n", ret); >> + return ret; >> + } >> + >> + ret = device_property_match_string(dev, "device_type", "pci"); >> + if (ret) { >> + dev_err(dev, "failed to find pci device type: %d\n", ret); >> + return ret; >> + } >> + >> + if (device_property_read_u32(dev, "reset-assert-ms", &lpp->rst_intrvl)) >> + lpp->rst_intrvl = RST_INTRVL_DFT_MS; >> + >> + ret = of_pci_get_max_link_speed(dev->of_node); >> + lpp->link_gen = ret < 0 ? 0 : ret; > I think the spacing may be a little off above. Yes, typo error. I will correct it. > >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); >> + >> + lpp->app_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(lpp->app_base)) >> + return PTR_ERR(lpp->app_base); >> + >> + lpp->phy = devm_phy_get(dev, "pcie"); >> + if (IS_ERR(lpp->phy)) { >> + ret = PTR_ERR(lpp->phy); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) >> +{ >> + phy_exit(lpp->phy); >> +} >> + >> +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) >> +{ >> + u32 value; >> + int ret; >> + >> + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) >> + return 0; >> + >> + /* Send PME_TURN_OFF message */ >> + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, >> + PCIE_APP_MSG_XMT_PM_TURNOFF, PCIE_APP_MSG_CR); >> + >> + /* Read PMC status and wait for falling into L2 link state */ >> + ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value, >> + (value & PCIE_APP_PMC_IN_L2), 20, >> + jiffies_to_usecs(5 * HZ)); >> + if (ret) >> + dev_err(lpp->pci.dev, "PCIe link enter L2 timeout!\n"); >> + >> + return ret; >> +} >> + >> +static void intel_pcie_turn_off(struct intel_pcie_port *lpp) >> +{ >> + if (dw_pcie_link_up(&lpp->pci)) >> + intel_pcie_wait_l2(lpp); >> + >> + /* Put endpoint device in reset state */ >> + intel_pcie_device_rst_assert(lpp); >> + pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY, 0, PCI_COMMAND); >> +} >> + >> +static int intel_pcie_host_setup(struct intel_pcie_port *lpp) >> +{ >> + int ret; >> + >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_device_rst_assert(lpp); >> + >> + ret = phy_init(lpp->phy); >> + if (ret) >> + return ret; >> + >> + intel_pcie_core_rst_deassert(lpp); >> + >> + ret = clk_prepare_enable(lpp->core_clk); >> + if (ret) { >> + dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret); >> + goto clk_err; >> + } >> + >> + intel_pcie_rc_setup(lpp); >> + ret = intel_pcie_app_logic_setup(lpp); >> + if (ret) >> + goto app_init_err; >> + >> + /* Enable integrated interrupts */ >> + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRN_INT, >> + PCIE_APP_IRNEN); >> + return 0; >> + >> +app_init_err: >> + clk_disable_unprepare(lpp->core_clk); >> +clk_err: >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_deinit_phy(lpp); >> + >> + return ret; >> +} >> + >> +static void __intel_pcie_remove(struct intel_pcie_port *lpp) >> +{ >> + intel_pcie_core_irq_disable(lpp); >> + intel_pcie_turn_off(lpp); >> + clk_disable_unprepare(lpp->core_clk); >> + intel_pcie_core_rst_assert(lpp); >> + intel_pcie_deinit_phy(lpp); >> +} >> + >> +static int intel_pcie_remove(struct platform_device *pdev) >> +{ >> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >> + struct pcie_port *pp = &lpp->pci.pp; >> + >> + dw_pcie_host_deinit(pp); >> + __intel_pcie_remove(lpp); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused intel_pcie_suspend_noirq(struct device *dev) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + int ret; >> + >> + intel_pcie_core_irq_disable(lpp); >> + ret = intel_pcie_wait_l2(lpp); >> + if (ret) >> + return ret; >> + >> + intel_pcie_deinit_phy(lpp); >> + clk_disable_unprepare(lpp->core_clk); >> + return ret; >> +} >> + >> +static int __maybe_unused intel_pcie_resume_noirq(struct device *dev) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + >> + return intel_pcie_host_setup(lpp); >> +} >> + >> +static int intel_pcie_rc_init(struct pcie_port *pp) >> +{ >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> + struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); >> + >> + return intel_pcie_host_setup(lpp); >> +} >> + >> +int intel_pcie_msi_init(struct pcie_port *pp) >> +{ >> + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ >> + return 0; >> +} >> + >> +u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr) >> +{ >> + return cpu_addr + BUS_IATU_OFFS; >> +} >> + >> +static const struct dw_pcie_ops intel_pcie_ops = { >> + .cpu_addr_fixup = intel_pcie_cpu_addr, >> +}; >> + >> +static const struct dw_pcie_host_ops intel_pcie_dw_ops = { >> + .host_init = intel_pcie_rc_init, >> + .msi_host_init = intel_pcie_msi_init, >> +}; >> + >> +static const struct intel_pcie_soc pcie_data = { >> + .pcie_ver = 0x520A, >> + .pcie_atu_offset = 0xC0000, >> + .num_viewport = 3, >> +}; >> + >> +static int intel_pcie_probe(struct platform_device *pdev) >> +{ >> + const struct intel_pcie_soc *data; >> + struct device *dev = &pdev->dev; >> + struct intel_pcie_port *lpp; >> + struct pcie_port *pp; >> + struct dw_pcie *pci; >> + int ret; >> + >> + lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL); >> + if (!lpp) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, lpp); >> + pci = &lpp->pci; >> + pci->dev = dev; >> + pp = &pci->pp; >> + >> + ret = intel_pcie_get_resources(pdev); >> + if (ret) >> + return ret; >> + >> + ret = intel_pcie_ep_rst_init(lpp); >> + if (ret) >> + return ret; >> + >> + data = device_get_match_data(dev); >> + pci->ops = &intel_pcie_ops; >> + pci->version = data->pcie_ver; >> + pci->atu_base = pci->dbi_base + data->pcie_atu_offset; >> + pp->ops = &intel_pcie_dw_ops; >> + >> + ret = dw_pcie_host_init(pp); >> + if (ret) { >> + dev_err(dev, "cannot initialize host\n"); >> + return ret; >> + } >> + >> + /* Intel PCIe doesn't configure IO region, so configure >> + * viewport to not to access IO region during register >> + * read write operations. >> + */ >> + pci->num_viewport = data->num_viewport; >> + dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); >> + >> + return ret; >> +} >> + >> +static const struct dev_pm_ops intel_pcie_pm_ops = { >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pcie_suspend_noirq, >> + intel_pcie_resume_noirq) >> +}; >> + >> +static const struct of_device_id of_intel_pcie_match[] = { >> + { .compatible = "intel,lgm-pcie", .data = &pcie_data }, >> + {} >> +}; >> + >> +static struct platform_driver intel_pcie_driver = { >> + .probe = intel_pcie_probe, >> + .remove = intel_pcie_remove, >> + .driver = { >> + .name = "intel-gw-pcie", >> + .of_match_table = of_intel_pcie_match, >> + .pm = &intel_pcie_pm_ops, >> + }, >> +}; >> +builtin_platform_driver(intel_pcie_driver); >> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h >> index 29d6e93fd15e..f6e7e402f879 100644 >> --- a/include/uapi/linux/pci_regs.h >> +++ b/include/uapi/linux/pci_regs.h >> @@ -673,6 +673,7 @@ >> #define PCI_EXP_LNKCTL2_TLS_8_0GT 0x0003 /* Supported Speed 8GT/s */ >> #define PCI_EXP_LNKCTL2_TLS_16_0GT 0x0004 /* Supported Speed 16GT/s */ >> #define PCI_EXP_LNKCTL2_TLS_32_0GT 0x0005 /* Supported Speed 32GT/s */ >> +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed Disable */ > I think this should be 0x0020. Yes, my miss, i will update. Thanks for pointing it. Thanks for reviewing and providing the inputs. Regards, Dilip > > Thanks, > > Andrew Murray > >> #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ >> #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */ >> #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ >> -- >> 2.11.0 >>
Hi Andrew Murray, On 10/21/2019 9:38 PM, Andrew Murray wrote: > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: >> PCIe RC driver on Intel Gateway SoCs have a requirement >> of changing link width and speed on the fly. >> So add the sysfs attributes to show and store the link >> properties. >> Add the respective link resize function in pcie DesignWare >> framework so that Intel PCIe driver can use during link >> width configuration on the fly. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.c | 9 +++ >> drivers/pci/controller/dwc/pcie-designware.h | 3 + >> drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++++++++++++++++++++++++++- >> 3 files changed, 123 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 4c391bfd681a..662fdcb4f2d6 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) >> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); >> } >> >> +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) >> +{ >> + u32 val; >> + >> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); >> + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); >> + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; >> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); >> +} >> >> void dw_pcie_upconfig_setup(struct dw_pcie *pci) >> { >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 3beac10e4a4c..fcf0442341fd 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -67,6 +67,8 @@ >> #define PCIE_MSI_INTR0_STATUS 0x830 >> >> #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >> +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) >> +#define PORT_MLTI_LNK_WDTH_CHNG BIT(6) >> #define PORT_MLTI_UPCFG_SUPPORT BIT(7) >> >> #define PCIE_ATU_VIEWPORT 0x900 >> @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); >> void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); >> int dw_pcie_link_up(struct dw_pcie *pci); >> +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); >> void dw_pcie_upconfig_setup(struct dw_pcie *pci); >> void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); >> void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); >> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c >> index 9142c70db808..b9be0921671d 100644 >> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c >> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c >> @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) >> pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); >> } >> >> +static const char *pcie_link_gen_to_str(int gen) >> +{ >> + switch (gen) { >> + case PCIE_LINK_SPEED_GEN1: >> + return "2.5"; >> + case PCIE_LINK_SPEED_GEN2: >> + return "5.0"; >> + case PCIE_LINK_SPEED_GEN3: >> + return "8.0"; >> + case PCIE_LINK_SPEED_GEN4: >> + return "16.0"; >> + default: >> + return "???"; >> + } >> +} >> + >> static void intel_pcie_link_setup(struct intel_pcie_port *lpp) >> { >> u32 val; >> @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) >> return ret; >> } >> >> +static ssize_t pcie_link_status_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + u32 reg, width, gen; >> + >> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); >> + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); >> + >> + if (gen > lpp->max_speed) >> + return -EINVAL; >> + >> + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, >> + width, pcie_link_gen_to_str(gen)); >> +} >> +static DEVICE_ATTR_RO(pcie_link_status); >> + >> +static ssize_t pcie_speed_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + unsigned long val; >> + int ret; >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret) >> + return ret; >> + >> + if (val > lpp->max_speed) >> + return -EINVAL; >> + >> + lpp->link_gen = val; >> + intel_pcie_max_speed_setup(lpp); >> + dw_pcie_link_speed_change(&lpp->pci, false); >> + dw_pcie_link_speed_change(&lpp->pci, true); >> + >> + return len; >> +} >> +static DEVICE_ATTR_WO(pcie_speed); >> + >> +/* >> + * Link width change on the fly is not always successful. >> + * It also depends on the partner. >> + */ >> +static ssize_t pcie_width_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >> + unsigned long val; >> + int ret; >> + >> + lpp = dev_get_drvdata(dev); >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret) >> + return ret; >> + >> + if (val > lpp->max_width) >> + return -EINVAL; >> + >> + /* HW auto bandwidth negotiation must be enabled */ >> + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, >> + PCIE_CAP_OFST + PCI_EXP_LNKCTL); >> + dw_pcie_link_width_resize(&lpp->pci, val); >> + >> + return len; >> +} >> +static DEVICE_ATTR_WO(pcie_width); >> + >> +static struct attribute *pcie_cfg_attrs[] = { >> + &dev_attr_pcie_link_status.attr, >> + &dev_attr_pcie_speed.attr, >> + &dev_attr_pcie_width.attr, >> + NULL, >> +}; > Is there a reason that these are limited only to the Intel driver and > not the wider set of DWC drivers? > > Is there anything specific here about the Intel GW driver? Yes, they need intel_pcie_max_speed_setup() and pcie_link_gen_to_str(). Once intel_pcie_max_speed_setup() moved to DesignWare framework (as per Bjorn Helgaas inputs) and use pcie_link_speed[] array instead of pcie_link_gen_to_str() (as per gustavo pimentel inputs) we can move this to PCIe DesignWare framework or to pci sysfs file. Regards, Dilip > > Thanks, > > Andrew Murray > >> +ATTRIBUTE_GROUPS(pcie_cfg); >> + >> +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp) >> +{ >> + return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups); >> +} >> + >> static void __intel_pcie_remove(struct intel_pcie_port *lpp) >> { >> intel_pcie_core_irq_disable(lpp); >> @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp) >> { >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); >> + int ret; >> >> - return intel_pcie_host_setup(lpp); >> + ret = intel_pcie_host_setup(lpp); >> + if (ret) >> + return ret; >> + >> + ret = intel_pcie_sysfs_init(lpp); >> + if (ret) >> + __intel_pcie_remove(lpp); >> + >> + return ret; >> } >> >> int intel_pcie_msi_init(struct pcie_port *pp) >> -- >> 2.11.0 >>
Hi Bjorn Helgaas, On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: >> On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: >>> PCIe RC driver on Intel Gateway SoCs have a requirement >>> of changing link width and speed on the fly. > Please add more details about why this is needed. Since you're adding > sysfs files, it sounds like it's not actually the *driver* that needs > this; it's something in userspace? We have use cases to change the link speed and width on the fly. One is EMI check and other is power saving. Some battery backed applications have to switch PCIe link from higher GEN to GEN1 and width to x1. During the cases like external power supply got disconnected or broken. Once external power supply is connected then switch PCIe link to higher GEN and width. > > The normal scenario is that the hardware negotiates link widths and > speeds without any software involvement (PCIe r5.0, sec 1.2). > > If this is to work around hardware defects, we should try to do that > inside the kernel because we can't expect userspace to do it reliably. > > As Andrew points out below, this all sounds like it should be generic > rather than Intel-specific. > >>> So add the sysfs attributes to show and store the link >>> properties. >>> Add the respective link resize function in pcie DesignWare >>> framework so that Intel PCIe driver can use during link >>> width configuration on the fly. >>> ... >>> +static ssize_t pcie_link_status_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>> + u32 reg, width, gen; >>> + >>> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); >>> + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); >>> + >>> + if (gen > lpp->max_speed) >>> + return -EINVAL; >>> + >>> + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, >>> + width, pcie_link_gen_to_str(gen)); >>> +} >>> +static DEVICE_ATTR_RO(pcie_link_status); > We already have generic current_link_speed and current_link_width > files. Thanks for pointing it. I will remove the pcie_link_status. Regards, Dilip > >>> +static ssize_t pcie_speed_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t len) >>> +{ >>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>> + unsigned long val; >>> + int ret; >>> + >>> + ret = kstrtoul(buf, 10, &val); >>> + if (ret) >>> + return ret; >>> + >>> + if (val > lpp->max_speed) >>> + return -EINVAL; >>> + >>> + lpp->link_gen = val; >>> + intel_pcie_max_speed_setup(lpp); >>> + dw_pcie_link_speed_change(&lpp->pci, false); >>> + dw_pcie_link_speed_change(&lpp->pci, true); >>> + >>> + return len; >>> +} >>> +static DEVICE_ATTR_WO(pcie_speed); >>> + >>> +/* >>> + * Link width change on the fly is not always successful. >>> + * It also depends on the partner. >>> + */ >>> +static ssize_t pcie_width_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t len) >>> +{ >>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>> + unsigned long val; >>> + int ret; >>> + >>> + lpp = dev_get_drvdata(dev); >>> + >>> + ret = kstrtoul(buf, 10, &val); >>> + if (ret) >>> + return ret; >>> + >>> + if (val > lpp->max_width) >>> + return -EINVAL; >>> + >>> + /* HW auto bandwidth negotiation must be enabled */ >>> + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, >>> + PCIE_CAP_OFST + PCI_EXP_LNKCTL); >>> + dw_pcie_link_width_resize(&lpp->pci, val); >>> + >>> + return len; >>> +} >>> +static DEVICE_ATTR_WO(pcie_width); >>> + >>> +static struct attribute *pcie_cfg_attrs[] = { >>> + &dev_attr_pcie_link_status.attr, >>> + &dev_attr_pcie_speed.attr, >>> + &dev_attr_pcie_width.attr, >>> + NULL, >>> +}; >> Is there a reason that these are limited only to the Intel driver and >> not the wider set of DWC drivers? >> >> Is there anything specific here about the Intel GW driver?
Hi Gustavo Pimentel, On 10/21/2019 6:44 PM, Dilip Kota wrote: > Hi Gustavo Pimentel, > > On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: >> Hi >> >> On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota >> <eswara.kota@linux.intel.com> >> wrote: >> >>> Add support to PCIe RC controller on Intel Gateway SoCs. >>> PCIe controller is based of Synopsys DesignWare pci core. >>> >>> Intel PCIe driver requires Upconfig support, fast training >>> sequence configuration and link speed change. So adding the >>> respective helper functions in the pcie DesignWare framework. >>> It also programs hardware autonomous speed during speed >>> configuration so defining it in pci_regs.h. >> Please do the replacement in all of your patches >> >> s/pcie/PCIe >> s/pci/PCI >> >> Also I think the correct term is Upconfigure and not Upconfig > Yes, i will update it. >> >>> Changes on v4: >>> Rename the driver naming and description to >>> "PCIe RC controller on Intel Gateway SoCs". >>> Use PCIe core register macros defined in pci_regs.h >>> and remove respective local definitions. >>> Remove pcie core interrupt handling. >>> Move pcie link control speed change, upconfig and FTS. >>> configuration functions to DesignWare framework. >>> Use of_pci_get_max_link_speed(). >>> Mark dependency on X86 and COMPILE_TEST in Kconfig. >>> Remove lanes and add n_fts variables in intel_pcie_port structure. >>> Rename rst_interval variable to rst_intrvl in intel_pcie_port >>> structure. >>> Remove intel_pcie_mem_iatu() as it is already perfomed in >>> dw_setup_rc() >>> Move sysfs attributes specific code to separate patch. >>> Remove redundant error handling. >>> Reduce LoCs by doing variable initializations while declaration >>> itself. >>> Add extra line after closing parenthesis. >>> Move intel_pcie_ep_rst_init() out of get_resources() >>> >>> changes on v3: >>> Rename PCIe app logic registers with PCIE_APP prefix. >>> PCIE_IOP_CTRL configuration is not required. Remove respective >>> code. >>> Remove wrapper functions for clk enable/disable APIs. >>> Use platform_get_resource_byname() instead of >>> devm_platform_ioremap_resource() to be similar with DWC >>> framework. >>> Rename phy name to "pciephy". >>> Modify debug message in msi_init() callback to be more specific. >>> Remove map_irq() callback. >>> Enable the INTx interrupts at the time of PCIe initialization. >>> Reduce memory fragmentation by using variable "struct dw_pcie pci" >>> instead of allocating memory. >>> Reduce the delay to 100us during enpoint initialization >>> intel_pcie_ep_rst_init(). >>> Call dw_pcie_host_deinit() during remove() instead of directly >>> calling PCIe core APIs. >>> Rename "intel,rst-interval" to "reset-assert-ms". >>> Remove unused APP logic Interrupt bit macro definitions. >>> Use dwc framework's dw_pcie_setup_rc() for PCIe host specific >>> configuration instead of redefining the same functionality in >>> the driver. >>> Move the whole DT parsing specific code to >>> intel_pcie_get_resources() >>> >>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >>> --- >>> drivers/pci/controller/dwc/Kconfig | 10 + >>> drivers/pci/controller/dwc/Makefile | 1 + >>> drivers/pci/controller/dwc/pcie-designware.c | 34 ++ >>> drivers/pci/controller/dwc/pcie-designware.h | 12 + >>> drivers/pci/controller/dwc/pcie-intel-gw.c | 590 >>> +++++++++++++++++++++++++++ >>> include/uapi/linux/pci_regs.h | 1 + >>> 6 files changed, 648 insertions(+) >>> create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c >>> >>> diff --git a/drivers/pci/controller/dwc/Kconfig >>> b/drivers/pci/controller/dwc/Kconfig >>> index 0ba988b5b5bc..b33ed1cc873d 100644 >>> --- a/drivers/pci/controller/dwc/Kconfig >>> +++ b/drivers/pci/controller/dwc/Kconfig >>> @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP >>> order to enable device-specific features PCI_DW_PLAT_EP must be >>> selected. >>> +config PCIE_INTEL_GW >>> + bool "Intel Gateway PCIe host controller support" >>> + depends on OF && (X86 || COMPILE_TEST) >>> + select PCIE_DW_HOST >>> + help >>> + Say 'Y' here to enable support for PCIe Host controller >>> driver. >>> + The PCIe controller on Intel Gateway SoCs is based on the >>> Synopsys >>> + DesignWare pcie core and therefore uses the DesignWare core >>> + functions for the driver implementation. >>> + >>> config PCI_EXYNOS >>> bool "Samsung Exynos PCIe controller" >>> depends on SOC_EXYNOS5440 || COMPILE_TEST >>> diff --git a/drivers/pci/controller/dwc/Makefile >>> b/drivers/pci/controller/dwc/Makefile >>> index b30336181d46..99db83cd2f35 100644 >>> --- a/drivers/pci/controller/dwc/Makefile >>> +++ b/drivers/pci/controller/dwc/Makefile >>> @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o >>> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o >>> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o >>> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o >>> +obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o >>> obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o >>> obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o >>> obj-$(CONFIG_PCI_IMX6) += pci-imx6.o >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c >>> b/drivers/pci/controller/dwc/pcie-designware.c >>> index 820488dfeaed..4c391bfd681a 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware.c >>> @@ -474,6 +474,40 @@ int dw_pcie_link_up(struct dw_pcie *pci) >>> (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); >>> } >>> + >>> +void dw_pcie_upconfig_setup(struct dw_pcie *pci) >>> +{ >>> + u32 val; >>> + >>> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); >>> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, >>> + val | PORT_MLTI_UPCFG_SUPPORT); >>> +} >>> + >>> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable) >>> +{ >>> + u32 val; >>> + >>> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >>> + >>> + if (enable) >>> + val |= PORT_LOGIC_SPEED_CHANGE; >>> + else >>> + val &= ~PORT_LOGIC_SPEED_CHANGE; >>> + >>> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >>> +} >>> + >>> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) >>> +{ >>> + u32 val; >>> + >>> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >>> + val &= ~PORT_LOGIC_N_FTS; >>> + val |= n_fts; >>> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >>> +} >>> + >>> static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci) >>> { >>> u32 val; >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h >>> b/drivers/pci/controller/dwc/pcie-designware.h >>> index 5a18e94e52c8..3beac10e4a4c 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>> @@ -30,7 +30,12 @@ >>> #define LINK_WAIT_IATU 9 >>> /* Synopsys-specific PCIe configuration registers */ >>> +#define PCIE_PORT_AFR 0x70C >>> +#define PORT_AFR_N_FTS_MASK GENMASK(15, 8) >>> +#define PORT_AFR_CC_N_FTS_MASK GENMASK(23, 16) >>> + >>> #define PCIE_PORT_LINK_CONTROL 0x710 >>> +#define PORT_LINK_DLL_LINK_EN BIT(5) >>> #define PORT_LINK_MODE_MASK GENMASK(21, 16) >>> #define PORT_LINK_MODE(n) FIELD_PREP(PORT_LINK_MODE_MASK, n) >>> #define PORT_LINK_MODE_1_LANES PORT_LINK_MODE(0x1) >>> @@ -46,6 +51,7 @@ >>> #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING BIT(29) >>> #define PCIE_LINK_WIDTH_SPEED_CONTROL 0x80C >>> +#define PORT_LOGIC_N_FTS GENMASK(7, 0) >>> #define PORT_LOGIC_SPEED_CHANGE BIT(17) >>> #define PORT_LOGIC_LINK_WIDTH_MASK GENMASK(12, 8) >>> #define PORT_LOGIC_LINK_WIDTH(n) >>> FIELD_PREP(PORT_LOGIC_LINK_WIDTH_MASK, n) >>> @@ -60,6 +66,9 @@ >>> #define PCIE_MSI_INTR0_MASK 0x82C >>> #define PCIE_MSI_INTR0_STATUS 0x830 >>> +#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >>> +#define PORT_MLTI_UPCFG_SUPPORT BIT(7) >>> + >>> #define PCIE_ATU_VIEWPORT 0x900 >>> #define PCIE_ATU_REGION_INBOUND BIT(31) >>> #define PCIE_ATU_REGION_OUTBOUND 0 >>> @@ -273,6 +282,9 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 >>> reg, size_t size, u32 val); >>> u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); >>> void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, >>> u32 val); >>> int dw_pcie_link_up(struct dw_pcie *pci); >>> +void dw_pcie_upconfig_setup(struct dw_pcie *pci); >>> +void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); >>> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); >>> int dw_pcie_wait_for_link(struct dw_pcie *pci); >>> void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, >>> int type, u64 cpu_addr, u64 pci_addr, >>> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c >>> b/drivers/pci/controller/dwc/pcie-intel-gw.c >>> new file mode 100644 >>> index 000000000000..9142c70db808 >>> --- /dev/null >>> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c >>> @@ -0,0 +1,590 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * PCIe host controller driver for Intel Gateway SoCs >>> + * >>> + * Copyright (c) 2019 Intel Corporation. >>> + */ >>> + >>> +#include <linux/bitfield.h> >>> +#include <linux/clk.h> >>> +#include <linux/gpio/consumer.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/iopoll.h> >>> +#include <linux/of_irq.h> >>> +#include <linux/of_pci.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/pci_regs.h> >>> +#include <linux/phy/phy.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/reset.h> >>> + >>> +#include "../../pci.h" >>> +#include "pcie-designware.h" >>> + >>> +#define PCIE_CAP_OFST 0x70 >>> + >>> +#define PORT_AFR_N_FTS_GEN12_DFT (SZ_128 - 1) >>> +#define PORT_AFR_N_FTS_GEN3 180 >>> +#define PORT_AFR_N_FTS_GEN4 196 >>> + >>> +/* PCIe Application logic Registers */ >>> +#define PCIE_APP_CCR 0x10 >>> +#define PCIE_APP_CCR_LTSSM_ENABLE BIT(0) >>> + >>> +#define PCIE_APP_MSG_CR 0x30 >>> +#define PCIE_APP_MSG_XMT_PM_TURNOFF BIT(0) >>> + >>> +#define PCIE_APP_PMC 0x44 >>> +#define PCIE_APP_PMC_IN_L2 BIT(20) >>> + >>> +#define PCIE_APP_IRNEN 0xF4 >>> +#define PCIE_APP_IRNCR 0xF8 >>> +#define PCIE_APP_IRN_AER_REPORT BIT(0) >>> +#define PCIE_APP_IRN_PME BIT(2) >>> +#define PCIE_APP_IRN_RX_VDM_MSG BIT(4) >>> +#define PCIE_APP_IRN_PM_TO_ACK BIT(9) >>> +#define PCIE_APP_IRN_LINK_AUTO_BW_STAT BIT(11) >>> +#define PCIE_APP_IRN_BW_MGT BIT(12) >>> +#define PCIE_APP_IRN_MSG_LTR BIT(18) >>> +#define PCIE_APP_IRN_SYS_ERR_RC BIT(29) >>> +#define PCIE_APP_INTX_OFST 12 >>> + >>> +#define PCIE_APP_IRN_INT \ >>> + (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ >>> + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ >>> + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ >>> + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ >>> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ >>> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ >>> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ >>> + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) >>> + >>> +#define BUS_IATU_OFFS SZ_256M >>> +#define RST_INTRVL_DFT_MS 100 >>> + >>> +enum { >>> + PCIE_LINK_SPEED_AUTO = 0, >>> + PCIE_LINK_SPEED_GEN1, >>> + PCIE_LINK_SPEED_GEN2, >>> + PCIE_LINK_SPEED_GEN3, >>> + PCIE_LINK_SPEED_GEN4, >>> +}; >>> + >>> +struct intel_pcie_soc { >>> + unsigned int pcie_ver; >>> + unsigned int pcie_atu_offset; >>> + u32 num_viewport; >>> +}; >>> + >>> +struct intel_pcie_port { >>> + struct dw_pcie pci; >>> + unsigned int id; /* Physical RC Index */ >>> + void __iomem *app_base; >>> + struct gpio_desc *reset_gpio; >>> + u32 rst_intrvl; >>> + u32 max_speed; >>> + u32 link_gen; >>> + u32 max_width; >>> + u32 n_fts; >>> + struct clk *core_clk; >>> + struct reset_control *core_rst; >>> + struct phy *phy; >>> +}; >>> + >>> +static void pcie_update_bits(void __iomem *base, u32 mask, u32 val, >>> u32 ofs) >>> +{ >>> + u32 orig, tmp; >>> + >>> + orig = readl(base + ofs); >>> + >>> + tmp = (orig & ~mask) | (val & mask); >>> + >>> + if (tmp != orig) >>> + writel(tmp, base + ofs); >>> +} >> I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more >> intuitive names such as new and old, instead of orig and tmp. > Sure, i will update it. I tried using FIELD_PREP and FIELD_GET but it is failing because FIELD_PREP and FIELD_GET are expecting mask should be constant macro. u32 or u32 const are not accepted. Regards, Dilip >> >>> +static inline u32 pcie_app_rd(struct intel_pcie_port *lpp, u32 ofs) >>> +{ >>> + return readl(lpp->app_base + ofs); >>> +} >>> + >>> +static inline void pcie_app_wr(struct intel_pcie_port *lpp, u32 >>> val, u32 ofs) >>> +{ >>> + writel(val, lpp->app_base + ofs); >>> +} >>> + >>> +static void pcie_app_wr_mask(struct intel_pcie_port *lpp, >>> + u32 mask, u32 val, u32 ofs) >>> +{ >>> + pcie_update_bits(lpp->app_base, mask, val, ofs); >>> +} >>> + >>> +static inline u32 pcie_rc_cfg_rd(struct intel_pcie_port *lpp, u32 ofs) >>> +{ >>> + return dw_pcie_readl_dbi(&lpp->pci, ofs); >>> +} >>> + >>> +static inline void pcie_rc_cfg_wr(struct intel_pcie_port *lpp, u32 >>> val, u32 ofs) >>> +{ >>> + dw_pcie_writel_dbi(&lpp->pci, ofs, val); >>> +} >>> + >>> +static void pcie_rc_cfg_wr_mask(struct intel_pcie_port *lpp, >>> + u32 mask, u32 val, u32 ofs) >>> +{ >>> + pcie_update_bits(lpp->pci.dbi_base, mask, val, ofs); >>> +} >>> + >>> +static void intel_pcie_ltssm_enable(struct intel_pcie_port *lpp) >>> +{ >>> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, >>> + PCIE_APP_CCR_LTSSM_ENABLE, PCIE_APP_CCR); >>> +} >>> + >>> +static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) >>> +{ >>> + pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); >>> +} >>> + >>> +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) >>> +{ >>> + u32 val; >>> + >>> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >>> + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >>> + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); >>> + >>> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >>> + >>> + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); >>> + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | >>> + PCI_EXP_LNKCTL_RCB; >>> + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); >>> +} >>> + >>> +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) >>> +{ >>> + u32 reg, val; >>> + >>> + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >>> + switch (lpp->link_gen) { >>> + case PCIE_LINK_SPEED_GEN1: >>> + reg &= ~PCI_EXP_LNKCTL2_TLS; >>> + reg |= PCI_EXP_LNKCTL2_HASD| >>> + PCI_EXP_LNKCTL2_TLS_2_5GT; >>> + break; >>> + case PCIE_LINK_SPEED_GEN2: >>> + reg &= ~PCI_EXP_LNKCTL2_TLS; >>> + reg |= PCI_EXP_LNKCTL2_HASD| >>> + PCI_EXP_LNKCTL2_TLS_5_0GT; >>> + break; >>> + case PCIE_LINK_SPEED_GEN3: >>> + reg &= ~PCI_EXP_LNKCTL2_TLS; >>> + reg |= PCI_EXP_LNKCTL2_HASD| >>> + PCI_EXP_LNKCTL2_TLS_8_0GT; >>> + break; >>> + case PCIE_LINK_SPEED_GEN4: >>> + reg &= ~PCI_EXP_LNKCTL2_TLS; >>> + reg |= PCI_EXP_LNKCTL2_HASD| >>> + PCI_EXP_LNKCTL2_TLS_16_0GT; >>> + break; >>> + default: >>> + /* Use hardware capability */ >>> + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); >>> + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); >>> + reg &= ~PCI_EXP_LNKCTL2_HASD; >>> + reg |= val; >>> + break; >>> + } >>> + >>> + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); >>> + dw_pcie_link_set_n_fts(&lpp->pci, lpp->n_fts); >>> +} >>> + >>> + >> Reduce the number of empty lines here. > Ok, will remove it. >> >>> + >>> +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) >>> +{ >>> + u32 val, mask; >>> + >>> + switch (lpp->max_speed) { >>> + case PCIE_LINK_SPEED_GEN3: >>> + lpp->n_fts = PORT_AFR_N_FTS_GEN3; >>> + break; >>> + case PCIE_LINK_SPEED_GEN4: >>> + lpp->n_fts = PORT_AFR_N_FTS_GEN4; >>> + break; >>> + default: >>> + lpp->n_fts = PORT_AFR_N_FTS_GEN12_DFT; >>> + break; >>> + } >>> + >>> + mask = PORT_AFR_N_FTS_MASK | PORT_AFR_CC_N_FTS_MASK; >>> + val = FIELD_PREP(PORT_AFR_N_FTS_MASK, lpp->n_fts) | >>> + FIELD_PREP(PORT_AFR_CC_N_FTS_MASK, lpp->n_fts); >>> + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_PORT_AFR); >>> + >>> + /* Port Link Control Register */ >>> + pcie_rc_cfg_wr_mask(lpp, PORT_LINK_DLL_LINK_EN, >>> + PORT_LINK_DLL_LINK_EN, PCIE_PORT_LINK_CONTROL); >>> +} >>> + >>> +static void intel_pcie_rc_setup(struct intel_pcie_port *lpp) >>> +{ >>> + intel_pcie_ltssm_disable(lpp); >>> + intel_pcie_link_setup(lpp); >>> + dw_pcie_setup_rc(&lpp->pci.pp); >>> + dw_pcie_upconfig_setup(&lpp->pci); >>> + intel_pcie_port_logic_setup(lpp); >>> + intel_pcie_max_speed_setup(lpp); >>> +} >>> + >>> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp) >>> +{ >>> + struct device *dev = lpp->pci.dev; >>> + int ret; >>> + >>> + lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >>> + if (IS_ERR(lpp->reset_gpio)) { >>> + ret = PTR_ERR(lpp->reset_gpio); >>> + if (ret != -EPROBE_DEFER) >>> + dev_err(dev, "failed to request PCIe GPIO: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + /* Make initial reset last for 100us */ >>> + usleep_range(100, 200); >>> + >>> + return 0; >>> +} >>> + >>> +static void intel_pcie_core_rst_assert(struct intel_pcie_port *lpp) >>> +{ >>> + reset_control_assert(lpp->core_rst); >>> +} >>> + >>> +static void intel_pcie_core_rst_deassert(struct intel_pcie_port *lpp) >>> +{ >>> + /* >>> + * One micro-second delay to make sure the reset pulse >>> + * wide enough so that core reset is clean. >>> + */ >>> + udelay(1); >>> + reset_control_deassert(lpp->core_rst); >>> + >>> + /* >>> + * Some SoC core reset also reset PHY, more delay needed >>> + * to make sure the reset process is done. >>> + */ >>> + usleep_range(1000, 2000); >>> +} >>> + >>> +static void intel_pcie_device_rst_assert(struct intel_pcie_port *lpp) >>> +{ >>> + gpiod_set_value_cansleep(lpp->reset_gpio, 1); >>> +} >>> + >>> +static void intel_pcie_device_rst_deassert(struct intel_pcie_port >>> *lpp) >>> +{ >>> + msleep(lpp->rst_intrvl); >>> + gpiod_set_value_cansleep(lpp->reset_gpio, 0); >>> +} >>> + >>> +static int intel_pcie_app_logic_setup(struct intel_pcie_port *lpp) >>> +{ >>> + intel_pcie_device_rst_deassert(lpp); >>> + intel_pcie_ltssm_enable(lpp); >>> + >>> + return dw_pcie_wait_for_link(&lpp->pci); >>> +} >>> + >>> +static void intel_pcie_core_irq_disable(struct intel_pcie_port *lpp) >>> +{ >>> + pcie_app_wr(lpp, 0, PCIE_APP_IRNEN); >>> + pcie_app_wr(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRNCR); >>> +} >>> + >>> +static int intel_pcie_get_resources(struct platform_device *pdev) >>> +{ >>> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >>> + struct dw_pcie *pci = &lpp->pci; >>> + struct device *dev = pci->dev; >>> + struct resource *res; >>> + int ret; >>> + >>> + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); >>> + if (ret) { >>> + dev_err(dev, "failed to get domain id, errno %d\n", ret); >>> + return ret; >>> + } >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); >>> + >>> + pci->dbi_base = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(pci->dbi_base)) >>> + return PTR_ERR(pci->dbi_base); >>> + >>> + lpp->core_clk = devm_clk_get(dev, NULL); >>> + if (IS_ERR(lpp->core_clk)) { >>> + ret = PTR_ERR(lpp->core_clk); >>> + if (ret != -EPROBE_DEFER) >>> + dev_err(dev, "failed to get clks: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + lpp->core_rst = devm_reset_control_get(dev, NULL); >>> + if (IS_ERR(lpp->core_rst)) { >>> + ret = PTR_ERR(lpp->core_rst); >>> + if (ret != -EPROBE_DEFER) >>> + dev_err(dev, "failed to get resets: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = device_property_match_string(dev, "device_type", "pci"); >>> + if (ret) { >>> + dev_err(dev, "failed to find pci device type: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + if (device_property_read_u32(dev, "reset-assert-ms", >>> &lpp->rst_intrvl)) >>> + lpp->rst_intrvl = RST_INTRVL_DFT_MS; >>> + >>> + ret = of_pci_get_max_link_speed(dev->of_node); >>> + lpp->link_gen = ret < 0 ? 0 : ret; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); >>> + >>> + lpp->app_base = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(lpp->app_base)) >>> + return PTR_ERR(lpp->app_base); >>> + >>> + lpp->phy = devm_phy_get(dev, "pcie"); >>> + if (IS_ERR(lpp->phy)) { >>> + ret = PTR_ERR(lpp->phy); >>> + if (ret != -EPROBE_DEFER) >>> + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) >>> +{ >>> + phy_exit(lpp->phy); >>> +} >>> + >>> +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) >>> +{ >>> + u32 value; >>> + int ret; >>> + >>> + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) >>> + return 0; >>> + >>> + /* Send PME_TURN_OFF message */ >>> + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, >>> + PCIE_APP_MSG_XMT_PM_TURNOFF, PCIE_APP_MSG_CR); >>> + >>> + /* Read PMC status and wait for falling into L2 link state */ >>> + ret = readl_poll_timeout(lpp->app_base + PCIE_APP_PMC, value, >>> + (value & PCIE_APP_PMC_IN_L2), 20, >>> + jiffies_to_usecs(5 * HZ)); >>> + if (ret) >>> + dev_err(lpp->pci.dev, "PCIe link enter L2 timeout!\n"); >>> + >>> + return ret; >>> +} >>> + >>> +static void intel_pcie_turn_off(struct intel_pcie_port *lpp) >>> +{ >>> + if (dw_pcie_link_up(&lpp->pci)) >>> + intel_pcie_wait_l2(lpp); >>> + >>> + /* Put endpoint device in reset state */ >>> + intel_pcie_device_rst_assert(lpp); >>> + pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY, 0, PCI_COMMAND); >>> +} >>> + >>> +static int intel_pcie_host_setup(struct intel_pcie_port *lpp) >>> +{ >>> + int ret; >>> + >>> + intel_pcie_core_rst_assert(lpp); >>> + intel_pcie_device_rst_assert(lpp); >>> + >>> + ret = phy_init(lpp->phy); >>> + if (ret) >>> + return ret; >>> + >>> + intel_pcie_core_rst_deassert(lpp); >>> + >>> + ret = clk_prepare_enable(lpp->core_clk); >>> + if (ret) { >>> + dev_err(lpp->pci.dev, "Core clock enable failed: %d\n", ret); >>> + goto clk_err; >>> + } >>> + >>> + intel_pcie_rc_setup(lpp); >>> + ret = intel_pcie_app_logic_setup(lpp); >>> + if (ret) >>> + goto app_init_err; >>> + >>> + /* Enable integrated interrupts */ >>> + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, PCIE_APP_IRN_INT, >>> + PCIE_APP_IRNEN); >>> + return 0; >>> + >>> +app_init_err: >>> + clk_disable_unprepare(lpp->core_clk); >>> +clk_err: >>> + intel_pcie_core_rst_assert(lpp); >>> + intel_pcie_deinit_phy(lpp); >>> + >>> + return ret; >>> +} >>> + >>> +static void __intel_pcie_remove(struct intel_pcie_port *lpp) >>> +{ >>> + intel_pcie_core_irq_disable(lpp); >>> + intel_pcie_turn_off(lpp); >>> + clk_disable_unprepare(lpp->core_clk); >>> + intel_pcie_core_rst_assert(lpp); >>> + intel_pcie_deinit_phy(lpp); >>> +} >>> + >>> +static int intel_pcie_remove(struct platform_device *pdev) >>> +{ >>> + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); >>> + struct pcie_port *pp = &lpp->pci.pp; >>> + >>> + dw_pcie_host_deinit(pp); >>> + __intel_pcie_remove(lpp); >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused intel_pcie_suspend_noirq(struct device *dev) >>> +{ >>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + intel_pcie_core_irq_disable(lpp); >>> + ret = intel_pcie_wait_l2(lpp); >>> + if (ret) >>> + return ret; >>> + >>> + intel_pcie_deinit_phy(lpp); >>> + clk_disable_unprepare(lpp->core_clk); >>> + return ret; >>> +} >>> + >>> +static int __maybe_unused intel_pcie_resume_noirq(struct device *dev) >>> +{ >>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>> + >>> + return intel_pcie_host_setup(lpp); >>> +} >>> + >>> +static int intel_pcie_rc_init(struct pcie_port *pp) >>> +{ >>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>> + struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); >>> + >>> + return intel_pcie_host_setup(lpp); >>> +} >>> + >>> +int intel_pcie_msi_init(struct pcie_port *pp) >>> +{ >>> + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ >>> + return 0; >>> +} >>> + >>> +u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr) >>> +{ >>> + return cpu_addr + BUS_IATU_OFFS; >>> +} >>> + >>> +static const struct dw_pcie_ops intel_pcie_ops = { >>> + .cpu_addr_fixup = intel_pcie_cpu_addr, >>> +}; >>> + >>> +static const struct dw_pcie_host_ops intel_pcie_dw_ops = { >>> + .host_init = intel_pcie_rc_init, >>> + .msi_host_init = intel_pcie_msi_init, >>> +}; >>> + >>> +static const struct intel_pcie_soc pcie_data = { >>> + .pcie_ver = 0x520A, >>> + .pcie_atu_offset = 0xC0000, >>> + .num_viewport = 3, >>> +}; >>> + >>> +static int intel_pcie_probe(struct platform_device *pdev) >>> +{ >>> + const struct intel_pcie_soc *data; >>> + struct device *dev = &pdev->dev; >>> + struct intel_pcie_port *lpp; >>> + struct pcie_port *pp; >>> + struct dw_pcie *pci; >>> + int ret; >>> + >>> + lpp = devm_kzalloc(dev, sizeof(*lpp), GFP_KERNEL); >>> + if (!lpp) >>> + return -ENOMEM; >>> + >>> + platform_set_drvdata(pdev, lpp); >>> + pci = &lpp->pci; >>> + pci->dev = dev; >>> + pp = &pci->pp; >>> + >>> + ret = intel_pcie_get_resources(pdev); >>> + if (ret) >>> + return ret; >>> + >>> + ret = intel_pcie_ep_rst_init(lpp); >>> + if (ret) >>> + return ret; >>> + >>> + data = device_get_match_data(dev); >>> + pci->ops = &intel_pcie_ops; >>> + pci->version = data->pcie_ver; >>> + pci->atu_base = pci->dbi_base + data->pcie_atu_offset; >>> + pp->ops = &intel_pcie_dw_ops; >>> + >>> + ret = dw_pcie_host_init(pp); >>> + if (ret) { >>> + dev_err(dev, "cannot initialize host\n"); >>> + return ret; >>> + } >>> + >>> + /* Intel PCIe doesn't configure IO region, so configure >>> + * viewport to not to access IO region during register >>> + * read write operations. >>> + */ >>> + pci->num_viewport = data->num_viewport; >>> + dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", >>> lpp->id); >>> + >>> + return ret; >>> +} >>> + >>> +static const struct dev_pm_ops intel_pcie_pm_ops = { >>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pcie_suspend_noirq, >>> + intel_pcie_resume_noirq) >>> +}; >>> + >>> +static const struct of_device_id of_intel_pcie_match[] = { >>> + { .compatible = "intel,lgm-pcie", .data = &pcie_data }, >>> + {} >>> +}; >>> + >>> +static struct platform_driver intel_pcie_driver = { >>> + .probe = intel_pcie_probe, >>> + .remove = intel_pcie_remove, >>> + .driver = { >>> + .name = "intel-gw-pcie", >>> + .of_match_table = of_intel_pcie_match, >>> + .pm = &intel_pcie_pm_ops, >>> + }, >>> +}; >>> +builtin_platform_driver(intel_pcie_driver); >>> diff --git a/include/uapi/linux/pci_regs.h >>> b/include/uapi/linux/pci_regs.h >>> index 29d6e93fd15e..f6e7e402f879 100644 >>> --- a/include/uapi/linux/pci_regs.h >>> +++ b/include/uapi/linux/pci_regs.h >>> @@ -673,6 +673,7 @@ >>> #define PCI_EXP_LNKCTL2_TLS_8_0GT 0x0003 /* Supported Speed >>> 8GT/s */ >>> #define PCI_EXP_LNKCTL2_TLS_16_0GT 0x0004 /* Supported Speed >>> 16GT/s */ >>> #define PCI_EXP_LNKCTL2_TLS_32_0GT 0x0005 /* Supported Speed >>> 32GT/s */ >>> +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed >>> Disable */ >> s/Hw/HW > > Sure, i will correct it. > > Thanks for the valuable inputs. > > Regards, > Dilip > >> >>> #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ >>> #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints >>> with link end here */ >>> #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ >>> -- >>> 2.11.0 >>
On Tue, Oct 22, 2019 at 06:18:57PM +0800, Dilip Kota wrote: > On 10/21/2019 6:44 PM, Dilip Kota wrote: > > On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: > > > On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota > > > <eswara.kota@linux.intel.com> > > > wrote: First of all, it's a good behaviour to avoid way long quoting. > > > > +static void pcie_update_bits(void __iomem *base, u32 mask, u32 > > > > val, u32 ofs) > > > > +{ > > > > + u32 orig, tmp; > > > > + > > > > + orig = readl(base + ofs); > > > > + > > > > + tmp = (orig & ~mask) | (val & mask); > > > > + > > > > + if (tmp != orig) > > > > + writel(tmp, base + ofs); > > > > +} > > > I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more > > > intuitive names such as new and old, instead of orig and tmp. > > Sure, i will update it. > I tried using FIELD_PREP and FIELD_GET but it is failing because FIELD_PREP > and FIELD_GET > are expecting mask should be constant macro. u32 or u32 const are not > accepted. If you look at bitfield.h carefully you may find in particular u32_replace_bits().
[+cc Rafael, linux-pm, beginning of discussion at https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > of changing link width and speed on the fly. > > Please add more details about why this is needed. Since you're adding > > sysfs files, it sounds like it's not actually the *driver* that needs > > this; it's something in userspace? > We have use cases to change the link speed and width on the fly. > One is EMI check and other is power saving. Some battery backed > applications have to switch PCIe link from higher GEN to GEN1 and > width to x1. During the cases like external power supply got > disconnected or broken. Once external power supply is connected then > switch PCIe link to higher GEN and width. That sounds plausible, but of course nothing there is specific to the Intel Gateway, so we should implement this generically so it would work on all hardware. I'm not sure what the interface should look like -- should it be a low-level interface as you propose where userspace would have to identify each link of interest, or is there some system-wide power/performance knob that could tune all links? Cc'd Rafael and linux-pm in case they have ideas. Bjorn
On Tue, Oct 22, 2019 at 02:44:13PM +0300, andriy.shevchenko@intel.com wrote: > On Tue, Oct 22, 2019 at 06:18:57PM +0800, Dilip Kota wrote: > > On 10/21/2019 6:44 PM, Dilip Kota wrote: > > > On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: > > > > On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota > > > > <eswara.kota@linux.intel.com> > > > > wrote: > > First of all, it's a good behaviour to avoid way long quoting. > > > > > > +static void pcie_update_bits(void __iomem *base, u32 mask, u32 > > > > > val, u32 ofs) > > > > > +{ > > > > > + u32 orig, tmp; > > > > > + > > > > > + orig = readl(base + ofs); > > > > > + > > > > > + tmp = (orig & ~mask) | (val & mask); > > > > > + > > > > > + if (tmp != orig) > > > > > + writel(tmp, base + ofs); > > > > > +} > > > > I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more > > > > intuitive names such as new and old, instead of orig and tmp. > > > Sure, i will update it. > > I tried using FIELD_PREP and FIELD_GET but it is failing because FIELD_PREP > > and FIELD_GET > > are expecting mask should be constant macro. u32 or u32 const are not > > accepted. > > If you look at bitfield.h carefully you may find in particular > u32_replace_bits(). Thanks Andy - I was looking for something like this when I was reviewing the patch but wasn't able to find it. Andrew Murray > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Oct 22, 2019 at 05:04:21PM +0800, Dilip Kota wrote: > Hi Andrew Murray, > > On 10/21/2019 9:03 PM, Andrew Murray wrote: > > On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: > > > + > > > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) > > > +{ > > > + u32 val; > > > + > > > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > > > + val &= ~PORT_LOGIC_N_FTS; > > > + val |= n_fts; > > > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > > > +} > > I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS > > and defines a bunch of macros to support this. It doesn't make sense to > > duplicate this there. Therefore I think we need to update pcie-artpec6.c > > to use this new function. > I think we can do in a separate patch after these changes get merged and > keep this patch series for intel PCIe driver and required changes in PCIe > DesignWare framework. The pcie-artpec6.c is a DWC driver as well. So I think we can do all this together. This helps reduce the technical debt that will otherwise build up in duplicated code. > > > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c > > > new file mode 100644 > > > index 000000000000..9142c70db808 > > > --- /dev/null > > > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c > > > @@ -0,0 +1,590 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * PCIe host controller driver for Intel Gateway SoCs > > > + * > > > + * Copyright (c) 2019 Intel Corporation. > > > + */ > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/clk.h> > > > +#include <linux/gpio/consumer.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/iopoll.h> > > > +#include <linux/of_irq.h> > > > +#include <linux/of_pci.h> > > > +#include <linux/of_platform.h> > > > +#include <linux/pci_regs.h> > > > +#include <linux/phy/phy.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/reset.h> > > > + > > > +#include "../../pci.h" > > I expected this to be removed, is it needed now? > > In the earlier patch you suggested to use "of_pci_get_max_link_speed()" > instead of device_get_*. > And, pci.h is required for it. OK that makes sense. > > > + > > > +static int intel_pcie_get_resources(struct platform_device *pdev) > > > +{ > > > + struct intel_pcie_port *lpp = platform_get_drvdata(pdev); > > > + struct dw_pcie *pci = &lpp->pci; > > > + struct device *dev = pci->dev; > > > + struct resource *res; > > > + int ret; > > > + > > > + ret = device_property_read_u32(dev, "linux,pci-domain", &lpp->id); > > > + if (ret) { > > > + dev_err(dev, "failed to get domain id, errno %d\n", ret); > > > + return ret; > > > + } > > Why is this a required property? > I marked it required property because lpp->id is being used during debug > prints. > -> sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, > -> dev_info(dev, "Intel PCIe Root Complex Port %d init done\n", lpp->id); > > Hmmm.. I found, domain id can be traversed from pci_host_bridge structure > after dw_pcie_host_init(). > I will remove the code for getting the pci-domain here. Excellent. > > > > > > +#define PCI_EXP_LNKCTL2_HASD 0x0200 /* Hw Autonomous Speed Disable */ > > I think this should be 0x0020. > Yes, my miss, i will update. > Thanks for pointing it. > > Thanks for reviewing and providing the inputs. > Regards, > Dilip > > > > Thanks, > > > > Andrew Murray Thanks, Andrew Murray > > > > > #define PCI_EXP_LNKSTA2 50 /* Link Status 2 */ > > > #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */ > > > #define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */ > > > -- > > > 2.11.0 > > >
On Tue, Oct 22, 2019 at 05:20:00PM +0800, Dilip Kota wrote: > Hi Andrew Murray, > > On 10/21/2019 9:38 PM, Andrew Murray wrote: > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > of changing link width and speed on the fly. > > > So add the sysfs attributes to show and store the link > > > properties. > > > Add the respective link resize function in pcie DesignWare > > > framework so that Intel PCIe driver can use during link > > > width configuration on the fly. > > > > > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > > > +/* > > > + * Link width change on the fly is not always successful. > > > + * It also depends on the partner. > > > + */ > > > +static ssize_t pcie_width_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t len) > > > +{ > > > + struct intel_pcie_port *lpp = dev_get_drvdata(dev); > > > + unsigned long val; > > > + int ret; > > > + > > > + lpp = dev_get_drvdata(dev); > > > + > > > + ret = kstrtoul(buf, 10, &val); > > > + if (ret) > > > + return ret; > > > + > > > + if (val > lpp->max_width) > > > + return -EINVAL; > > > + > > > + /* HW auto bandwidth negotiation must be enabled */ > > > + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, > > > + PCIE_CAP_OFST + PCI_EXP_LNKCTL); > > > + dw_pcie_link_width_resize(&lpp->pci, val); > > > + > > > + return len; > > > +} > > > +static DEVICE_ATTR_WO(pcie_width); > > > + > > > +static struct attribute *pcie_cfg_attrs[] = { > > > + &dev_attr_pcie_link_status.attr, > > > + &dev_attr_pcie_speed.attr, > > > + &dev_attr_pcie_width.attr, > > > + NULL, > > > +}; > > Is there a reason that these are limited only to the Intel driver and > > not the wider set of DWC drivers? > > > > Is there anything specific here about the Intel GW driver? > > Yes, they need intel_pcie_max_speed_setup() and pcie_link_gen_to_str(). > Once intel_pcie_max_speed_setup() moved to DesignWare framework (as per > Bjorn Helgaas inputs) and use pcie_link_speed[] array instead of > pcie_link_gen_to_str() (as per gustavo pimentel inputs) we can move this to > PCIe DesignWare framework or to pci sysfs file. I think the key concern here is this: If you introduce sysfs controls that represent generic PCI concepts (such as changing the link speed) - the concept isn't limited to a particular host controller, it's limited to PCI. Therefore the sysfs control should also apply more widely to all PCI controllers. This is important as otherwise you may end up getting a slightly different user interface to achieve the same consequence depending on the host-controller in use. If each controller driver has a different way of doing things, then it lends itself to having some set of ops that they can all implement. Or perhaps there is a middle-ground solution where this applies just to DWC devices and not all devices. Thanks, Andrew Murray > > Regards, > Dilip > > > > > Thanks, > > > > Andrew Murray > > > > > +ATTRIBUTE_GROUPS(pcie_cfg); > > > + > > > +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp) > > > +{ > > > + return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups); > > > +} > > > + > > > static void __intel_pcie_remove(struct intel_pcie_port *lpp) > > > { > > > intel_pcie_core_irq_disable(lpp); > > > @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp) > > > { > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > > struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev); > > > + int ret; > > > - return intel_pcie_host_setup(lpp); > > > + ret = intel_pcie_host_setup(lpp); > > > + if (ret) > > > + return ret; > > > + > > > + ret = intel_pcie_sysfs_init(lpp); > > > + if (ret) > > > + __intel_pcie_remove(lpp); > > > + > > > + return ret; > > > } > > > int intel_pcie_msi_init(struct pcie_port *pp) > > > -- > > > 2.11.0 > > >
On 10/22/2019 7:44 PM, andriy.shevchenko@intel.com wrote: > On Tue, Oct 22, 2019 at 06:18:57PM +0800, Dilip Kota wrote: >> On 10/21/2019 6:44 PM, Dilip Kota wrote: >>> On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: >>>> On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota >>>> <eswara.kota@linux.intel.com> >>>> wrote: > First of all, it's a good behaviour to avoid way long quoting. (Sorry for the late reply, I am back today from sick leave.) Noted. Will take care of it. > >>>>> +static void pcie_update_bits(void __iomem *base, u32 mask, u32 >>>>> val, u32 ofs) >>>>> +{ >>>>> + u32 orig, tmp; >>>>> + >>>>> + orig = readl(base + ofs); >>>>> + >>>>> + tmp = (orig & ~mask) | (val & mask); >>>>> + >>>>> + if (tmp != orig) >>>>> + writel(tmp, base + ofs); >>>>> +} >>>> I'd suggest to the a take on FIELD_PREP() and FIELD_GET() and use more >>>> intuitive names such as new and old, instead of orig and tmp. >>> Sure, i will update it. >> I tried using FIELD_PREP and FIELD_GET but it is failing because FIELD_PREP >> and FIELD_GET >> are expecting mask should be constant macro. u32 or u32 const are not >> accepted. > If you look at bitfield.h carefully you may find in particular > u32_replace_bits(). Thanks for pointing it. But, I found bitfields are not contiguous during few function calls, so cannot use them here. Regards, Dilip >
On 10/25/2019 5:09 PM, Andrew Murray wrote: > On Tue, Oct 22, 2019 at 05:04:21PM +0800, Dilip Kota wrote: >> Hi Andrew Murray, >> >> On 10/21/2019 9:03 PM, Andrew Murray wrote: >>> On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: >>>> + >>>> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) >>>> +{ >>>> + u32 val; >>>> + >>>> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >>>> + val &= ~PORT_LOGIC_N_FTS; >>>> + val |= n_fts; >>>> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >>>> +} >>> I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS >>> and defines a bunch of macros to support this. It doesn't make sense to >>> duplicate this there. Therefore I think we need to update pcie-artpec6.c >>> to use this new function. >> I think we can do in a separate patch after these changes get merged and >> keep this patch series for intel PCIe driver and required changes in PCIe >> DesignWare framework. > The pcie-artpec6.c is a DWC driver as well. So I think we can do all this > together. This helps reduce the technical debt that will otherwise build up > in duplicated code. I agree with you to remove duplicated code, but at this point not sure what all drivers has defined FTS configuration. Reviewing all other DWC drivers and removing them can be done in one single separate patch. Regards, Dilip
On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > [+cc Rafael, linux-pm, beginning of discussion at > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: >> On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: >>> On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: >>>> On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: >>>>> PCIe RC driver on Intel Gateway SoCs have a requirement >>>>> of changing link width and speed on the fly. >>> Please add more details about why this is needed. Since you're adding >>> sysfs files, it sounds like it's not actually the *driver* that needs >>> this; it's something in userspace? >> We have use cases to change the link speed and width on the fly. >> One is EMI check and other is power saving. Some battery backed >> applications have to switch PCIe link from higher GEN to GEN1 and >> width to x1. During the cases like external power supply got >> disconnected or broken. Once external power supply is connected then >> switch PCIe link to higher GEN and width. > That sounds plausible, but of course nothing there is specific to the > Intel Gateway, so we should implement this generically so it would > work on all hardware. Agree. > > I'm not sure what the interface should look like -- should it be a > low-level interface as you propose where userspace would have to > identify each link of interest, or is there some system-wide > power/performance knob that could tune all links? Cc'd Rafael and > linux-pm in case they have ideas. To my knowledge sysfs is the appropriate way to go. If there are any other best possible knobs, will be helpful. Regards, Dilip > > Bjorn
On 10/25/2019 5:34 PM, Andrew Murray wrote: >>>> +/* >>>> + * Link width change on the fly is not always successful. >>>> + * It also depends on the partner. >>>> + */ >>>> +static ssize_t pcie_width_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t len) >>>> +{ >>>> + struct intel_pcie_port *lpp = dev_get_drvdata(dev); >>>> + unsigned long val; >>>> + int ret; >>>> + >>>> + lpp = dev_get_drvdata(dev); >>>> + >>>> + ret = kstrtoul(buf, 10, &val); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (val > lpp->max_width) >>>> + return -EINVAL; >>>> + >>>> + /* HW auto bandwidth negotiation must be enabled */ >>>> + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, >>>> + PCIE_CAP_OFST + PCI_EXP_LNKCTL); >>>> + dw_pcie_link_width_resize(&lpp->pci, val); >>>> + >>>> + return len; >>>> +} >>>> +static DEVICE_ATTR_WO(pcie_width); >>>> + >>>> +static struct attribute *pcie_cfg_attrs[] = { >>>> + &dev_attr_pcie_link_status.attr, >>>> + &dev_attr_pcie_speed.attr, >>>> + &dev_attr_pcie_width.attr, >>>> + NULL, >>>> +}; >>> Is there a reason that these are limited only to the Intel driver and >>> not the wider set of DWC drivers? >>> >>> Is there anything specific here about the Intel GW driver? >> Yes, they need intel_pcie_max_speed_setup() and pcie_link_gen_to_str(). >> Once intel_pcie_max_speed_setup() moved to DesignWare framework (as per >> Bjorn Helgaas inputs) and use pcie_link_speed[] array instead of >> pcie_link_gen_to_str() (as per gustavo pimentel inputs) we can move this to >> PCIe DesignWare framework or to pci sysfs file. > I think the key concern here is this: If you introduce sysfs controls that > represent generic PCI concepts (such as changing the link speed) - the concept > isn't limited to a particular host controller, it's limited to PCI. Therefore > the sysfs control should also apply more widely to all PCI controllers. This > is important as otherwise you may end up getting a slightly different user > interface to achieve the same consequence depending on the host-controller in > use. > > If each controller driver has a different way of doing things, then it lends > itself to having some set of ops that they can all implement. Or perhaps there > is a middle-ground solution where this applies just to DWC devices and not all > devices. I see the better way is to move the implementation to PCIe DesignWare framework because of the registers programming. Regards, Dilip > Thanks, > > Andrew Murray > >>
On Tue, Oct 22, 2019 at 2:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Rafael, linux-pm, beginning of discussion at > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > of changing link width and speed on the fly. > > > Please add more details about why this is needed. Since you're adding > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > this; it's something in userspace? > > > We have use cases to change the link speed and width on the fly. > > One is EMI check and other is power saving. Some battery backed > > applications have to switch PCIe link from higher GEN to GEN1 and > > width to x1. During the cases like external power supply got > > disconnected or broken. Once external power supply is connected then > > switch PCIe link to higher GEN and width. > > That sounds plausible, but of course nothing there is specific to the > Intel Gateway, so we should implement this generically so it would > work on all hardware. > > I'm not sure what the interface should look like -- should it be a > low-level interface as you propose where userspace would have to > identify each link of interest, or is there some system-wide > power/performance knob that could tune all links? Cc'd Rafael and > linux-pm in case they have ideas. Frankly, I need some time to think about this and, in case you are wondering about whether or not it has been discussed with me already, it hasn't. At this point I can only say that since we have an ASPM interface, which IMO is not fantastic, it may be good to come up with a common link management interface. Cheers!
[+cc Heiner for ASPM conversation] On Tue, Oct 29, 2019 at 11:42:53AM +0100, Rafael J. Wysocki wrote: > On Tue, Oct 22, 2019 at 2:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > [+cc Rafael, linux-pm, beginning of discussion at > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > of changing link width and speed on the fly. > > > > Please add more details about why this is needed. Since you're adding > > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > > this; it's something in userspace? > > > > > We have use cases to change the link speed and width on the fly. > > > One is EMI check and other is power saving. Some battery backed > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > width to x1. During the cases like external power supply got > > > disconnected or broken. Once external power supply is connected then > > > switch PCIe link to higher GEN and width. > > > > That sounds plausible, but of course nothing there is specific to the > > Intel Gateway, so we should implement this generically so it would > > work on all hardware. > > > > I'm not sure what the interface should look like -- should it be a > > low-level interface as you propose where userspace would have to > > identify each link of interest, or is there some system-wide > > power/performance knob that could tune all links? Cc'd Rafael and > > linux-pm in case they have ideas. > > Frankly, I need some time to think about this and, in case you are > wondering about whether or not it has been discussed with me already, > it hasn't. > > At this point I can only say that since we have an ASPM interface, > which IMO is not fantastic, it may be good to come up with a common > link management interface. The ASPM interface hasn't been merged yet, so if you have better ideas, now is the time. That one is definitely very low-level, partly because the first use case is working around defects in a specific device. Some sort of unification of link management does sound like a good idea. Bjorn
[+cc Heiner, Rajat] On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > [+cc Rafael, linux-pm, beginning of discussion at > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > of changing link width and speed on the fly. > > > > Please add more details about why this is needed. Since you're adding > > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > > this; it's something in userspace? > > > We have use cases to change the link speed and width on the fly. > > > One is EMI check and other is power saving. Some battery backed > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > width to x1. During the cases like external power supply got > > > disconnected or broken. Once external power supply is connected then > > > switch PCIe link to higher GEN and width. > > That sounds plausible, but of course nothing there is specific to the > > Intel Gateway, so we should implement this generically so it would > > work on all hardware. > Agree. > > > > I'm not sure what the interface should look like -- should it be a > > low-level interface as you propose where userspace would have to > > identify each link of interest, or is there some system-wide > > power/performance knob that could tune all links? Cc'd Rafael and > > linux-pm in case they have ideas. > > To my knowledge sysfs is the appropriate way to go. > If there are any other best possible knobs, will be helpful. I agree sysfs is the right place for it; my question was whether we should have files like: /sys/.../0000:00:1f.3/pcie_speed /sys/.../0000:00:1f.3/pcie_width as I think this patch would add (BTW, please include sample paths like the above in the commit log), or whether there should be a more global thing that would affect all the links in the system. I think the low-level files like you propose would be better because one might want to tune link performance differently for different types of devices and workloads. We also have to decide if these files should be associated with the device at the upstream or downstream end of the link. For ASPM, the current proposal [1] has the files at the downstream end on the theory that the GPU, NIC, NVMe device, etc is the user-recognizable one. Also, neither ASPM nor link speed/width make any sense unless there *is* a device at the downstream end, so putting them there automatically makes them visible only when they're useful. Rafael had some concerns about the proposed ASPM interface [2], but I don't know what they are yet. For ASPM we added a "link_pm" directory, and maybe that's too specific. Maybe it should be a generic "link_mgt" or even "pcie" directory that could contain both the ASPM and width/speed files. There's also a change coming to put AER stats in something like this: /sys/.../0000:00:1f.3/aer_stats/correctable_rx_err /sys/.../0000:00:1f.3/aer_stats/correctable_timeout /sys/.../0000:00:1f.3/aer_stats/fatal_TLP ... It would certainly be good to have some organizational scheme or we'll end up with a real hodge-podge. [1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/aspm&id=ad46fe1c733656611788e2cd59793e891ed7ded7 [2] https://lore.kernel.org/r/CAJZ5v0jdxR4roEUC_Hs3puCzGY4ThdLsi_XcxfBUUxqruP4z7A@mail.gmail.com
On Wed, Oct 30, 2019 at 11:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Heiner, Rajat] > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > > [+cc Rafael, linux-pm, beginning of discussion at > > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > > of changing link width and speed on the fly. > > > > > Please add more details about why this is needed. Since you're adding > > > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > > > this; it's something in userspace? > > > > We have use cases to change the link speed and width on the fly. > > > > One is EMI check and other is power saving. Some battery backed > > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > > width to x1. During the cases like external power supply got > > > > disconnected or broken. Once external power supply is connected then > > > > switch PCIe link to higher GEN and width. > > > That sounds plausible, but of course nothing there is specific to the > > > Intel Gateway, so we should implement this generically so it would > > > work on all hardware. > > Agree. > > > > > > I'm not sure what the interface should look like -- should it be a > > > low-level interface as you propose where userspace would have to > > > identify each link of interest, or is there some system-wide > > > power/performance knob that could tune all links? Cc'd Rafael and > > > linux-pm in case they have ideas. > > > > To my knowledge sysfs is the appropriate way to go. > > If there are any other best possible knobs, will be helpful. > > I agree sysfs is the right place for it; my question was whether we > should have files like: > > /sys/.../0000:00:1f.3/pcie_speed > /sys/.../0000:00:1f.3/pcie_width > > as I think this patch would add (BTW, please include sample paths like > the above in the commit log), or whether there should be a more global > thing that would affect all the links in the system. > > I think the low-level files like you propose would be better because > one might want to tune link performance differently for different > types of devices and workloads. > > We also have to decide if these files should be associated with the > device at the upstream or downstream end of the link. For ASPM, the > current proposal [1] has the files at the downstream end on the theory > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > Also, neither ASPM nor link speed/width make any sense unless there > *is* a device at the downstream end, so putting them there > automatically makes them visible only when they're useful. > > Rafael had some concerns about the proposed ASPM interface [2], but I > don't know what they are yet. I was talking about the existing ASPM interface in sysfs. The new one I still have to review, but I'm kind of wondering what about people who used the old one? Would it be supported going forward? > For ASPM we added a "link_pm" directory, and maybe that's too > specific. Maybe it should be a generic "link_mgt" or even "pcie" > directory that could contain both the ASPM and width/speed files. > > There's also a change coming to put AER stats in something like this: > > /sys/.../0000:00:1f.3/aer_stats/correctable_rx_err > /sys/.../0000:00:1f.3/aer_stats/correctable_timeout > /sys/.../0000:00:1f.3/aer_stats/fatal_TLP > ... > > It would certainly be good to have some organizational scheme or we'll > end up with a real hodge-podge. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/aspm&id=ad46fe1c733656611788e2cd59793e891ed7ded7 > [2] https://lore.kernel.org/r/CAJZ5v0jdxR4roEUC_Hs3puCzGY4ThdLsi_XcxfBUUxqruP4z7A@mail.gmail.com
On Thu, Oct 31, 2019 at 12:31:44AM +0100, Rafael J. Wysocki wrote: > On Wed, Oct 30, 2019 at 11:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > [+cc Heiner, Rajat] > > > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > > > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > > > [+cc Rafael, linux-pm, beginning of discussion at > > > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > > > of changing link width and speed on the fly. > > > > > > Please add more details about why this is needed. Since you're adding > > > > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > > > > this; it's something in userspace? > > > > > We have use cases to change the link speed and width on the fly. > > > > > One is EMI check and other is power saving. Some battery backed > > > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > > > width to x1. During the cases like external power supply got > > > > > disconnected or broken. Once external power supply is connected then > > > > > switch PCIe link to higher GEN and width. > > > > That sounds plausible, but of course nothing there is specific to the > > > > Intel Gateway, so we should implement this generically so it would > > > > work on all hardware. > > > Agree. > > > > > > > > I'm not sure what the interface should look like -- should it be a > > > > low-level interface as you propose where userspace would have to > > > > identify each link of interest, or is there some system-wide > > > > power/performance knob that could tune all links? Cc'd Rafael and > > > > linux-pm in case they have ideas. > > > > > > To my knowledge sysfs is the appropriate way to go. > > > If there are any other best possible knobs, will be helpful. > > > > I agree sysfs is the right place for it; my question was whether we > > should have files like: > > > > /sys/.../0000:00:1f.3/pcie_speed > > /sys/.../0000:00:1f.3/pcie_width > > > > as I think this patch would add (BTW, please include sample paths like > > the above in the commit log), or whether there should be a more global > > thing that would affect all the links in the system. > > > > I think the low-level files like you propose would be better because > > one might want to tune link performance differently for different > > types of devices and workloads. > > > > We also have to decide if these files should be associated with the > > device at the upstream or downstream end of the link. For ASPM, the > > current proposal [1] has the files at the downstream end on the theory > > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > > Also, neither ASPM nor link speed/width make any sense unless there > > *is* a device at the downstream end, so putting them there > > automatically makes them visible only when they're useful. > > > > Rafael had some concerns about the proposed ASPM interface [2], but I > > don't know what they are yet. > > I was talking about the existing ASPM interface in sysfs. The new one > I still have to review, but I'm kind of wondering what about people > who used the old one? Would it be supported going forward? The old one interface was enabled by CONFIG_PCIEASPM_DEBUG. Red Hat doesn't enable that. Ubuntu does. I *thought* we heard from a Canonical person who said they didn't have any tools that used it, but I can't find that now. I don't know about SUSE. So the idea was to drop it on the theory that nobody is using it. Possibly that's too aggressive. Bjorn
On Thursday, October 31, 2019 3:56:37 AM CET Bjorn Helgaas wrote: > On Thu, Oct 31, 2019 at 12:31:44AM +0100, Rafael J. Wysocki wrote: > > On Wed, Oct 30, 2019 at 11:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > [+cc Heiner, Rajat] > > > > > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > > > > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > > > > [+cc Rafael, linux-pm, beginning of discussion at > > > > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > > > > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > > > > of changing link width and speed on the fly. > > > > > > > Please add more details about why this is needed. Since you're adding > > > > > > > sysfs files, it sounds like it's not actually the *driver* that needs > > > > > > > this; it's something in userspace? > > > > > > We have use cases to change the link speed and width on the fly. > > > > > > One is EMI check and other is power saving. Some battery backed > > > > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > > > > width to x1. During the cases like external power supply got > > > > > > disconnected or broken. Once external power supply is connected then > > > > > > switch PCIe link to higher GEN and width. > > > > > That sounds plausible, but of course nothing there is specific to the > > > > > Intel Gateway, so we should implement this generically so it would > > > > > work on all hardware. > > > > Agree. > > > > > > > > > > I'm not sure what the interface should look like -- should it be a > > > > > low-level interface as you propose where userspace would have to > > > > > identify each link of interest, or is there some system-wide > > > > > power/performance knob that could tune all links? Cc'd Rafael and > > > > > linux-pm in case they have ideas. > > > > > > > > To my knowledge sysfs is the appropriate way to go. > > > > If there are any other best possible knobs, will be helpful. > > > > > > I agree sysfs is the right place for it; my question was whether we > > > should have files like: > > > > > > /sys/.../0000:00:1f.3/pcie_speed > > > /sys/.../0000:00:1f.3/pcie_width > > > > > > as I think this patch would add (BTW, please include sample paths like > > > the above in the commit log), or whether there should be a more global > > > thing that would affect all the links in the system. > > > > > > I think the low-level files like you propose would be better because > > > one might want to tune link performance differently for different > > > types of devices and workloads. > > > > > > We also have to decide if these files should be associated with the > > > device at the upstream or downstream end of the link. For ASPM, the > > > current proposal [1] has the files at the downstream end on the theory > > > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > > > Also, neither ASPM nor link speed/width make any sense unless there > > > *is* a device at the downstream end, so putting them there > > > automatically makes them visible only when they're useful. > > > > > > Rafael had some concerns about the proposed ASPM interface [2], but I > > > don't know what they are yet. > > > > I was talking about the existing ASPM interface in sysfs. The new one > > I still have to review, but I'm kind of wondering what about people > > who used the old one? Would it be supported going forward? > > The old one interface was enabled by CONFIG_PCIEASPM_DEBUG. Red Hat > doesn't enable that. Ubuntu does. I *thought* we heard from a > Canonical person who said they didn't have any tools that used it, but > I can't find that now. I don't know about SUSE. > > So the idea was to drop it on the theory that nobody is using it. > Possibly that's too aggressive. Well, one problem is that the "old" (actually existing) I/F has made it to one of my OSS EU presentation slides (I did not talk to this particular slide, but it is there in the deck that's available for downloading), so who knows who is going to use it. :-) So I guess that there's a risk that needs to be taken into consideration. What could be done, in principle, would be to make the new I/F depend on CONFIG_PCIEASPM_DEBUG being unset and provide the "old" one when it is set. In any case, the pcie_aspm.policy module parameter cannot be dropped, because AFAICS there is quite a bit of user space using it (e.g. TLP). Cheers!
On 10/31/2019 6:14 AM, Bjorn Helgaas wrote: > [+cc Heiner, Rajat] > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: >> On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: >>> [+cc Rafael, linux-pm, beginning of discussion at >>> https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] >>> >>> On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: >>>> On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: >>>>> On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: >>>>>> On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: >>>>>>> PCIe RC driver on Intel Gateway SoCs have a requirement >>>>>>> of changing link width and speed on the fly. >>>>> Please add more details about why this is needed. Since you're adding >>>>> sysfs files, it sounds like it's not actually the *driver* that needs >>>>> this; it's something in userspace? >>>> We have use cases to change the link speed and width on the fly. >>>> One is EMI check and other is power saving. Some battery backed >>>> applications have to switch PCIe link from higher GEN to GEN1 and >>>> width to x1. During the cases like external power supply got >>>> disconnected or broken. Once external power supply is connected then >>>> switch PCIe link to higher GEN and width. >>> That sounds plausible, but of course nothing there is specific to the >>> Intel Gateway, so we should implement this generically so it would >>> work on all hardware. >> Agree. >>> I'm not sure what the interface should look like -- should it be a >>> low-level interface as you propose where userspace would have to >>> identify each link of interest, or is there some system-wide >>> power/performance knob that could tune all links? Cc'd Rafael and >>> linux-pm in case they have ideas. >> To my knowledge sysfs is the appropriate way to go. >> If there are any other best possible knobs, will be helpful. > I agree sysfs is the right place for it; my question was whether we > should have files like: > > /sys/.../0000:00:1f.3/pcie_speed > /sys/.../0000:00:1f.3/pcie_width > > as I think this patch would add (BTW, please include sample paths like > the above in the commit log), or whether there should be a more global > thing that would affect all the links in the system. Sure, i will add them. > > I think the low-level files like you propose would be better because > one might want to tune link performance differently for different > types of devices and workloads. > > We also have to decide if these files should be associated with the > device at the upstream or downstream end of the link. For ASPM, the > current proposal [1] has the files at the downstream end on the theory > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > Also, neither ASPM nor link speed/width make any sense unless there > *is* a device at the downstream end, so putting them there > automatically makes them visible only when they're useful. This patch places the speed and width in the host controller directory. /sys/.../xxx.pcie/pcie_speed /sys/.../xxx.pcie/pcie_width I agree with you partially, because i am having couple of points making me to keep speed and width change entries in controller directory: -- For changing the speed/width with device node, software ends up traversing to the controller from the device and do the operations. -- Change speed and width are performed at controller level, -- Keeping speed and width in controller gives a perspective (to the user) of changing them only once irrespective of no. of devices. -- For speed and link change in Synopsys PCIe controller, specific registers need to be configured. This prevents or complicates adding the speed and width change functionality in pci-sysfs or pci framework. Regards, Dilip
On Thu, Oct 31, 2019 at 10:13:11AM +0100, Rafael J. Wysocki wrote: > On Thursday, October 31, 2019 3:56:37 AM CET Bjorn Helgaas wrote: > > On Thu, Oct 31, 2019 at 12:31:44AM +0100, Rafael J. Wysocki wrote: > > > On Wed, Oct 30, 2019 at 11:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > Rafael had some concerns about the proposed ASPM interface [2], but I > > > > don't know what they are yet. > > > > > > I was talking about the existing ASPM interface in sysfs. The new one > > > I still have to review, but I'm kind of wondering what about people > > > who used the old one? Would it be supported going forward? > > > > The old one interface was enabled by CONFIG_PCIEASPM_DEBUG. Red Hat > > doesn't enable that. Ubuntu does. I *thought* we heard from a > > Canonical person who said they didn't have any tools that used it, but > > I can't find that now. I don't know about SUSE. > > > > So the idea was to drop it on the theory that nobody is using it. > > Possibly that's too aggressive. > > Well, one problem is that the "old" (actually existing) I/F has made it > to one of my OSS EU presentation slides (I did not talk to this particular > slide, but it is there in the deck that's available for downloading), so who > knows who is going to use it. :-) > > So I guess that there's a risk that needs to be taken into consideration. > > What could be done, in principle, would be to make the new I/F depend on > CONFIG_PCIEASPM_DEBUG being unset and provide the "old" one when it is set. I would prefer to enable the new interface unconditionally to make it easier for userspace tools like powertop to use it. I think the existing and new interfaces could coexist, with the existing interface being enabled by CONFIG_PCIEASPM_DEBUG as it is today. The patch that removes the existing interface is the last in the series and could easily be dropped. > In any case, the pcie_aspm.policy module parameter cannot be dropped, because > AFAICS there is quite a bit of user space using it (e.g. TLP). What is TLP? Since CONFIG_PCIEASPM is a bool, aspm.o is built in statically if enabled, so pcie_aspm.policy is effectively a boot-time kernel parameter, right? We don't have a plan to remove it. Bjorn
On Thu, Oct 31, 2019 at 06:47:10PM +0800, Dilip Kota wrote: > On 10/31/2019 6:14 AM, Bjorn Helgaas wrote: > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > > > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > > > [+cc Rafael, linux-pm, beginning of discussion at > > > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > > > of changing link width and speed on the fly. > > > > > > Please add more details about why this is needed. Since > > > > > > you're adding sysfs files, it sounds like it's not > > > > > > actually the *driver* that needs this; it's something in > > > > > > userspace? > > > > > We have use cases to change the link speed and width on the fly. > > > > > One is EMI check and other is power saving. Some battery backed > > > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > > > width to x1. During the cases like external power supply got > > > > > disconnected or broken. Once external power supply is connected then > > > > > switch PCIe link to higher GEN and width. > > > > That sounds plausible, but of course nothing there is specific to the > > > > Intel Gateway, so we should implement this generically so it would > > > > work on all hardware. > > > Agree. > > > > I'm not sure what the interface should look like -- should it be a > > > > low-level interface as you propose where userspace would have to > > > > identify each link of interest, or is there some system-wide > > > > power/performance knob that could tune all links? Cc'd Rafael and > > > > linux-pm in case they have ideas. > > > To my knowledge sysfs is the appropriate way to go. > > > If there are any other best possible knobs, will be helpful. > > I agree sysfs is the right place for it; my question was whether we > > should have files like: > > > > /sys/.../0000:00:1f.3/pcie_speed > > /sys/.../0000:00:1f.3/pcie_width > > > > as I think this patch would add (BTW, please include sample paths like > > the above in the commit log), or whether there should be a more global > > thing that would affect all the links in the system. > Sure, i will add them. > > > > I think the low-level files like you propose would be better because > > one might want to tune link performance differently for different > > types of devices and workloads. > > > > We also have to decide if these files should be associated with the > > device at the upstream or downstream end of the link. For ASPM, the > > current proposal [1] has the files at the downstream end on the theory > > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > > Also, neither ASPM nor link speed/width make any sense unless there > > *is* a device at the downstream end, so putting them there > > automatically makes them visible only when they're useful. > > This patch places the speed and width in the host controller directory. > /sys/.../xxx.pcie/pcie_speed > /sys/.../xxx.pcie/pcie_width > > I agree with you partially, because i am having couple of points > making me to keep speed and width change entries in controller > directory: > > -- For changing the speed/width with device node, software ends up > traversing to the controller from the device and do the > operations. > -- Change speed and width are performed at controller level, The controller is effectively a Root Complex, which may contain several Root Ports. I have the impression that the Synopsys controller only supports a single Root Port, but that's just a detail of the Synopsys implementation. I think it should be possible to configure the width/speed of each Root Port individually. > -- Keeping speed and width in controller gives a perspective (to the > user) of changing them only once irrespective of no. of devices. What if there's a switch? If we change the width/speed of the link between the Root Port and the Switch Upstream Port, that doesn't do anything about the links from the Switch Downstream Ports. > -- For speed and link change in Synopsys PCIe controller, specific > registers need to be configured. This prevents or complicates > adding the speed and width change functionality in pci-sysfs or > pci framework. Don't the Link Control and related registers in PCIe spec give us enough control to manage the link width/speed of *all* links, including those from Root Ports and Switch Downstream Ports? If the Synopsys controller requires controller-specific registers, that sounds to me like it doesn't quite conform to the spec. Maybe that means we would need some sort of quirk or controller callback? Bjorn
On 10/31/2019 9:22 PM, Bjorn Helgaas wrote: > On Thu, Oct 31, 2019 at 06:47:10PM +0800, Dilip Kota wrote: >> On 10/31/2019 6:14 AM, Bjorn Helgaas wrote: >>> On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: >>>> On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: >>>>> [+cc Rafael, linux-pm, beginning of discussion at >>>>> https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] >>>>> >>>>> On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: >>>>>> On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: >>>>>>> On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: >>>>>>>> On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: >>>>>>>>> PCIe RC driver on Intel Gateway SoCs have a requirement >>>>>>>>> of changing link width and speed on the fly. >>>>>>> Please add more details about why this is needed. Since >>>>>>> you're adding sysfs files, it sounds like it's not >>>>>>> actually the *driver* that needs this; it's something in >>>>>>> userspace? >>>>>> We have use cases to change the link speed and width on the fly. >>>>>> One is EMI check and other is power saving. Some battery backed >>>>>> applications have to switch PCIe link from higher GEN to GEN1 and >>>>>> width to x1. During the cases like external power supply got >>>>>> disconnected or broken. Once external power supply is connected then >>>>>> switch PCIe link to higher GEN and width. >>>>> That sounds plausible, but of course nothing there is specific to the >>>>> Intel Gateway, so we should implement this generically so it would >>>>> work on all hardware. >>>> Agree. >>>>> I'm not sure what the interface should look like -- should it be a >>>>> low-level interface as you propose where userspace would have to >>>>> identify each link of interest, or is there some system-wide >>>>> power/performance knob that could tune all links? Cc'd Rafael and >>>>> linux-pm in case they have ideas. >>>> To my knowledge sysfs is the appropriate way to go. >>>> If there are any other best possible knobs, will be helpful. >>> I agree sysfs is the right place for it; my question was whether we >>> should have files like: >>> >>> /sys/.../0000:00:1f.3/pcie_speed >>> /sys/.../0000:00:1f.3/pcie_width >>> >>> as I think this patch would add (BTW, please include sample paths like >>> the above in the commit log), or whether there should be a more global >>> thing that would affect all the links in the system. >> Sure, i will add them. >>> I think the low-level files like you propose would be better because >>> one might want to tune link performance differently for different >>> types of devices and workloads. >>> >>> We also have to decide if these files should be associated with the >>> device at the upstream or downstream end of the link. For ASPM, the >>> current proposal [1] has the files at the downstream end on the theory >>> that the GPU, NIC, NVMe device, etc is the user-recognizable one. >>> Also, neither ASPM nor link speed/width make any sense unless there >>> *is* a device at the downstream end, so putting them there >>> automatically makes them visible only when they're useful. >> This patch places the speed and width in the host controller directory. >> /sys/.../xxx.pcie/pcie_speed >> /sys/.../xxx.pcie/pcie_width >> >> I agree with you partially, because i am having couple of points >> making me to keep speed and width change entries in controller >> directory: >> >> -- For changing the speed/width with device node, software ends up >> traversing to the controller from the device and do the >> operations. >> -- Change speed and width are performed at controller level, > The controller is effectively a Root Complex, which may contain > several Root Ports. I have the impression that the Synopsys > controller only supports a single Root Port, but that's just a detail > of the Synopsys implementation. I think it should be possible to > configure the width/speed of each Root Port individually. > >> -- Keeping speed and width in controller gives a perspective (to the >> user) of changing them only once irrespective of no. of devices. > What if there's a switch? If we change the width/speed of the link > between the Root Port and the Switch Upstream Port, that doesn't do > anything about the links from the Switch Downstream Ports. I missed to evaluate the multiple root port and switch scenarios, thanks for pointing it. Then, placing the link speed and width change entries in the device node will be appropriate. Software will traverse to the respective port or bus through the device node and does the changes. > >> -- For speed and link change in Synopsys PCIe controller, specific >> registers need to be configured. This prevents or complicates >> adding the speed and width change functionality in pci-sysfs or >> pci framework. > Don't the Link Control and related registers in PCIe spec give us > enough control to manage the link width/speed of *all* links, > including those from Root Ports and Switch Downstream Ports? > > If the Synopsys controller requires controller-specific registers, > that sounds to me like it doesn't quite conform to the spec. Maybe > that means we would need some sort of quirk or controller callback? Yes, Synopsys has specific registers configuration for link width resizing and speed change. I will evaluate the possible mechanism for plugging in the controller specific changes to the framework. Regards, Dilip > > Bjorn
On Tue, Oct 29, 2019 at 04:59:17PM +0800, Dilip Kota wrote: > > On 10/25/2019 5:09 PM, Andrew Murray wrote: > > On Tue, Oct 22, 2019 at 05:04:21PM +0800, Dilip Kota wrote: > > > Hi Andrew Murray, > > > > > > On 10/21/2019 9:03 PM, Andrew Murray wrote: > > > > On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: > > > > > + > > > > > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) > > > > > +{ > > > > > + u32 val; > > > > > + > > > > > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > > > > > + val &= ~PORT_LOGIC_N_FTS; > > > > > + val |= n_fts; > > > > > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > > > > > +} > > > > I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS > > > > and defines a bunch of macros to support this. It doesn't make sense to > > > > duplicate this there. Therefore I think we need to update pcie-artpec6.c > > > > to use this new function. > > > I think we can do in a separate patch after these changes get merged and > > > keep this patch series for intel PCIe driver and required changes in PCIe > > > DesignWare framework. > > The pcie-artpec6.c is a DWC driver as well. So I think we can do all this > > together. This helps reduce the technical debt that will otherwise build up > > in duplicated code. > I agree with you to remove duplicated code, but at this point not sure what > all drivers has defined FTS configuration. > Reviewing all other DWC drivers and removing them can be done in one single > separate patch. I'm not asking to set up an FTS configuration for all DWC drivers, but instead to move this helper function you've created to somewhere like pcie-designware.c and call it from this driver and pcie-artpec6.c. Thanks, Andrew Murray > > Regards, > Dilip
On Fri, Nov 01, 2019 at 01:47:39PM +0800, Dilip Kota wrote: > > On 10/31/2019 9:22 PM, Bjorn Helgaas wrote: > > On Thu, Oct 31, 2019 at 06:47:10PM +0800, Dilip Kota wrote: > > > On 10/31/2019 6:14 AM, Bjorn Helgaas wrote: > > > > On Tue, Oct 29, 2019 at 05:31:18PM +0800, Dilip Kota wrote: > > > > > On 10/22/2019 8:59 PM, Bjorn Helgaas wrote: > > > > > > [+cc Rafael, linux-pm, beginning of discussion at > > > > > > https://lore.kernel.org/r/d8574605f8e70f41ce1e88ccfb56b63c8f85e4df.1571638827.git.eswara.kota@linux.intel.com] > > > > > > > > > > > > On Tue, Oct 22, 2019 at 05:27:38PM +0800, Dilip Kota wrote: > > > > > > > On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: > > > > > > > > On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: > > > > > > > > > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: > > > > > > > > > > PCIe RC driver on Intel Gateway SoCs have a requirement > > > > > > > > > > of changing link width and speed on the fly. > > > > > > > > Please add more details about why this is needed. Since > > > > > > > > you're adding sysfs files, it sounds like it's not > > > > > > > > actually the *driver* that needs this; it's something in > > > > > > > > userspace? > > > > > > > We have use cases to change the link speed and width on the fly. > > > > > > > One is EMI check and other is power saving. Some battery backed > > > > > > > applications have to switch PCIe link from higher GEN to GEN1 and > > > > > > > width to x1. During the cases like external power supply got > > > > > > > disconnected or broken. Once external power supply is connected then > > > > > > > switch PCIe link to higher GEN and width. > > > > > > That sounds plausible, but of course nothing there is specific to the > > > > > > Intel Gateway, so we should implement this generically so it would > > > > > > work on all hardware. > > > > > Agree. > > > > > > I'm not sure what the interface should look like -- should it be a > > > > > > low-level interface as you propose where userspace would have to > > > > > > identify each link of interest, or is there some system-wide > > > > > > power/performance knob that could tune all links? Cc'd Rafael and > > > > > > linux-pm in case they have ideas. > > > > > To my knowledge sysfs is the appropriate way to go. > > > > > If there are any other best possible knobs, will be helpful. > > > > I agree sysfs is the right place for it; my question was whether we > > > > should have files like: > > > > > > > > /sys/.../0000:00:1f.3/pcie_speed > > > > /sys/.../0000:00:1f.3/pcie_width > > > > > > > > as I think this patch would add (BTW, please include sample paths like > > > > the above in the commit log), or whether there should be a more global > > > > thing that would affect all the links in the system. > > > Sure, i will add them. > > > > I think the low-level files like you propose would be better because > > > > one might want to tune link performance differently for different > > > > types of devices and workloads. > > > > > > > > We also have to decide if these files should be associated with the > > > > device at the upstream or downstream end of the link. For ASPM, the > > > > current proposal [1] has the files at the downstream end on the theory > > > > that the GPU, NIC, NVMe device, etc is the user-recognizable one. > > > > Also, neither ASPM nor link speed/width make any sense unless there > > > > *is* a device at the downstream end, so putting them there > > > > automatically makes them visible only when they're useful. > > > This patch places the speed and width in the host controller directory. > > > /sys/.../xxx.pcie/pcie_speed > > > /sys/.../xxx.pcie/pcie_width > > > > > > I agree with you partially, because i am having couple of points > > > making me to keep speed and width change entries in controller > > > directory: > > > > > > -- For changing the speed/width with device node, software ends up > > > traversing to the controller from the device and do the > > > operations. > > > -- Change speed and width are performed at controller level, > > The controller is effectively a Root Complex, which may contain > > several Root Ports. I have the impression that the Synopsys > > controller only supports a single Root Port, but that's just a detail > > of the Synopsys implementation. I think it should be possible to > > configure the width/speed of each Root Port individually. > > > > > -- Keeping speed and width in controller gives a perspective (to the > > > user) of changing them only once irrespective of no. of devices. > > What if there's a switch? If we change the width/speed of the link > > between the Root Port and the Switch Upstream Port, that doesn't do > > anything about the links from the Switch Downstream Ports. > I missed to evaluate the multiple root port and switch scenarios, thanks for > pointing it. > Then, placing the link speed and width change entries in the device node > will be appropriate. > Software will traverse to the respective port or bus through the device node > and does the changes. > > > > > -- For speed and link change in Synopsys PCIe controller, specific > > > registers need to be configured. This prevents or complicates > > > adding the speed and width change functionality in pci-sysfs or > > > pci framework. > > Don't the Link Control and related registers in PCIe spec give us > > enough control to manage the link width/speed of *all* links, > > including those from Root Ports and Switch Downstream Ports? > > > > If the Synopsys controller requires controller-specific registers, > > that sounds to me like it doesn't quite conform to the spec. Maybe > > that means we would need some sort of quirk or controller callback? > Yes, Synopsys has specific registers configuration for link width resizing > and speed change. > I will evaluate the possible mechanism for plugging in the controller > specific changes to the framework. According to the spec, "Software is permitted to restrict the maximum speed of Link operation and set the perferred Link speed by setting the value in the Target Link Speed field in the Upstream component." - This is the Link Control 2 Register, and a link retrain should then be triggered. With regards to this proposed sysfs API - I wonder if this implies we should also disable 'Hardware Autonomous Speed Disable' to prevent a link speed change for device specific reasons? In my view, this means we *can* have a sysfs control for limiting the link speed using standard PCI means - though callbacks and quirks may be needed for host bridge controllers and similar. With regards to link width, I can't see any obvious software initiated means to change the link width (they are all RO) - though a device can change its own link width so long as it's 'Hardware Autonomous Width Disable' bit is clear. So whilst there may be some benefit for the initial links of a few host bridge controllers that may opt-in to some framework for this - such an API wouldn't benefit the majority of links in a PCI fabric. Perhaps this (width) should be DWC specific. Thanks, Andrew Murray > > Regards, > Dilip > > > > Bjorn
On 11/1/2019 6:59 PM, Andrew Murray wrote: > On Tue, Oct 29, 2019 at 04:59:17PM +0800, Dilip Kota wrote: >> On 10/25/2019 5:09 PM, Andrew Murray wrote: >>> On Tue, Oct 22, 2019 at 05:04:21PM +0800, Dilip Kota wrote: >>>> Hi Andrew Murray, >>>> >>>> On 10/21/2019 9:03 PM, Andrew Murray wrote: >>>>> On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: >>>>>> + >>>>>> +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) >>>>>> +{ >>>>>> + u32 val; >>>>>> + >>>>>> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); >>>>>> + val &= ~PORT_LOGIC_N_FTS; >>>>>> + val |= n_fts; >>>>>> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); >>>>>> +} >>>>> I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS >>>>> and defines a bunch of macros to support this. It doesn't make sense to >>>>> duplicate this there. Therefore I think we need to update pcie-artpec6.c >>>>> to use this new function. >>>> I think we can do in a separate patch after these changes get merged and >>>> keep this patch series for intel PCIe driver and required changes in PCIe >>>> DesignWare framework. >>> The pcie-artpec6.c is a DWC driver as well. So I think we can do all this >>> together. This helps reduce the technical debt that will otherwise build up >>> in duplicated code. >> I agree with you to remove duplicated code, but at this point not sure what >> all drivers has defined FTS configuration. >> Reviewing all other DWC drivers and removing them can be done in one single >> separate patch. > I'm not asking to set up an FTS configuration for all DWC drivers, but instead > to move this helper function you've created to somewhere like pcie-designware.c > and call it from this driver and pcie-artpec6.c. What i mean is, we need to check how many of the current DWC drivers are configuring the FTS and call the helper function. Today i have grep all the DWC based drivers and i see pcie-artpec6.c is the only driver doing FTS configuration. I will add the helper function call in pcie-artpec6.c in the next patch version. Regards, Dilip > > Thanks, > > Andrew Murray > >> Regards, >> Dilip
On Mon, Nov 04, 2019 at 05:34:54PM +0800, Dilip Kota wrote: > > On 11/1/2019 6:59 PM, Andrew Murray wrote: > > On Tue, Oct 29, 2019 at 04:59:17PM +0800, Dilip Kota wrote: > > > On 10/25/2019 5:09 PM, Andrew Murray wrote: > > > > On Tue, Oct 22, 2019 at 05:04:21PM +0800, Dilip Kota wrote: > > > > > Hi Andrew Murray, > > > > > > > > > > On 10/21/2019 9:03 PM, Andrew Murray wrote: > > > > > > On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: > > > > > > > + > > > > > > > +void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts) > > > > > > > +{ > > > > > > > + u32 val; > > > > > > > + > > > > > > > + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL); > > > > > > > + val &= ~PORT_LOGIC_N_FTS; > > > > > > > + val |= n_fts; > > > > > > > + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val); > > > > > > > +} > > > > > > I notice that pcie-artpec6.c (artpec6_pcie_set_nfts) also writes the FTS > > > > > > and defines a bunch of macros to support this. It doesn't make sense to > > > > > > duplicate this there. Therefore I think we need to update pcie-artpec6.c > > > > > > to use this new function. > > > > > I think we can do in a separate patch after these changes get merged and > > > > > keep this patch series for intel PCIe driver and required changes in PCIe > > > > > DesignWare framework. > > > > The pcie-artpec6.c is a DWC driver as well. So I think we can do all this > > > > together. This helps reduce the technical debt that will otherwise build up > > > > in duplicated code. > > > I agree with you to remove duplicated code, but at this point not sure what > > > all drivers has defined FTS configuration. > > > Reviewing all other DWC drivers and removing them can be done in one single > > > separate patch. > > I'm not asking to set up an FTS configuration for all DWC drivers, but instead > > to move this helper function you've created to somewhere like pcie-designware.c > > and call it from this driver and pcie-artpec6.c. > What i mean is, we need to check how many of the current DWC drivers are > configuring the FTS > and call the helper function. > Today i have grep all the DWC based drivers and i see pcie-artpec6.c is the > only driver doing FTS configuration. > > I will add the helper function call in pcie-artpec6.c in the next patch > version. Thanks that's very much appreciated. Thanks, Andrew Murray > > > Regards, > Dilip > > > > > > Thanks, > > > > Andrew Murray > > > > > Regards, > > > Dilip