Hi Andrey: Thanks for your patch-set. I have comment about the L1SS implementation below. It's better to figure out one method to fix it. BR Richard > -----Original Message----- > From: Andrey Smirnov [mailto:andrew.smirnov@xxxxxxxxx] > Sent: 2018年11月18日 2:12 > To: linux-kernel@xxxxxxxxxxxxxxx > Cc: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>; bhelgaas@xxxxxxxxxx; > Fabio Estevam <fabio.estevam@xxxxxxx>; cphealy@xxxxxxxxx; > l.stach@xxxxxxxxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>; > Aisheng DONG <aisheng.dong@xxxxxxx>; Richard Zhu > <hongxing.zhu@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ > > Cc: bhelgaas@xxxxxxxxxx > Cc: Fabio Estevam <fabio.estevam@xxxxxxx> > Cc: cphealy@xxxxxxxxx > Cc: l.stach@xxxxxxxxxxxxxx > Cc: Leonard Crestez <leonard.crestez@xxxxxxx> > Cc: "A.s. Dong" <aisheng.dong@xxxxxxx> > Cc: Richard Zhu <hongxing.zhu@xxxxxxx> > Cc: linux-imx@xxxxxxx > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > Cc: linux-pci@xxxxxxxxxxxxxxx > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx> > --- > drivers/pci/controller/dwc/Kconfig | 2 +- > drivers/pci/controller/dwc/pci-imx6.c | 117 ++++++++++++++++++++++++-- > 2 files changed, 113 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/Kconfig > b/drivers/pci/controller/dwc/Kconfig > index 91b0194240a5..2b139acccf32 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -90,7 +90,7 @@ config PCI_EXYNOS > > config PCI_IMX6 > bool "Freescale i.MX6 PCIe controller" > - depends on SOC_IMX6Q || (ARM && COMPILE_TEST) > + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST) > depends on PCI_MSI_IRQ_DOMAIN > select PCIE_DW_HOST > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > b/drivers/pci/controller/dwc/pci-imx6.c > index 3c3002861d25..8d1f310e41a6 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -8,6 +8,7 @@ > * Author: Sean Cross <xobs@xxxxxxxxxx> > */ > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/gpio.h> > @@ -30,6 +31,14 @@ > > #include "pcie-designware.h" > > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 << 15) > + > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9) > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10) > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11) > + > + > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev) > > enum imx6_pcie_variants { > @@ -37,6 +46,7 @@ enum imx6_pcie_variants { > IMX6SX, > IMX6QP, > IMX7D, > + IMX8MQ, > }; > > struct imx6_pcie { > @@ -48,8 +58,10 @@ struct imx6_pcie { > struct clk *pcie_inbound_axi; > struct clk *pcie; > struct regmap *iomuxc_gpr; > + u32 gpr1x; > struct reset_control *pciephy_reset; > struct reset_control *apps_reset; > + struct reset_control *apps_clk_req; > struct reset_control *turnoff_reset; > enum imx6_pcie_variants variant; > u32 tx_deemph_gen1; > @@ -59,6 +71,7 @@ struct imx6_pcie { > u32 tx_swing_low; > int link_gen; > struct regulator *vpcie; > + u32 device_type[2]; > }; > > /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@ > -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie > *imx6_pcie) { > u32 tmp; > > - if (imx6_pcie->variant == IMX7D) > + if (imx6_pcie->variant == IMX7D || > + imx6_pcie->variant == IMX8MQ) > return; > > pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp); @@ -261,6 > +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie) > pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp); } > > +#ifdef CONFIG_ARM > /* Added for PCI abort handling */ > static int imx6q_pcie_abort_handler(unsigned long addr, > unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@ static > int imx6q_pcie_abort_handler(unsigned long addr, > > return 1; > } > +#endif > > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct > imx6_pcie *imx6_pcie) > > switch (imx6_pcie->variant) { > case IMX7D: > + case IMX8MQ: /* FALLTHROUGH */ > reset_control_assert(imx6_pcie->pciephy_reset); > reset_control_assert(imx6_pcie->apps_reset); > break; > @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct > imx6_pcie *imx6_pcie) > break; > case IMX7D: > break; > + case IMX8MQ: > + /* > + * Set the over ride low and enabled > + * make sure that REF_CLK is turned on. > + */ > + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE, > + 0); > + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN); > + break; > } > > return ret; > @@ -397,6 +426,7 @@ static void imx6_pcie_deassert_core_reset(struct > imx6_pcie *imx6_pcie) { > struct dw_pcie *pci = imx6_pcie->pci; > struct device *dev = pci->dev; > + unsigned int val; > int ret; > > if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) { @@ > -445,6 +475,29 @@ static void imx6_pcie_deassert_core_reset(struct > imx6_pcie *imx6_pcie) > } > > switch (imx6_pcie->variant) { > + case IMX8MQ: > + reset_control_deassert(imx6_pcie->pciephy_reset); > + udelay(100); > + /* > + * Configure the CLK_REQ# high, let the L1SS > + * automatically controlled by HW. > + */ > + reset_control_assert(imx6_pcie->apps_clk_req); > + > + /* > + * Configure the L1 latency of rc to less than 64us > + * Otherwise, the L1/L1SUB wouldn't be enable by ASPM. > + */ > + val = dw_pcie_readl_dbi(imx6_pcie->pci, > + SZ_1M + > + IMX8MQ_PCIE_LINK_CAP_REG_OFFSET); > + val &= ~PCI_EXP_LNKCAP_L1EL; > + val |= IMX8MQ_PCIE_LINK_CAP_L1EL_64US; > + dw_pcie_writel_dbi(imx6_pcie->pci, > + SZ_1M + > + IMX8MQ_PCIE_LINK_CAP_REG_OFFSET, > + val); > + break; > case IMX7D: > reset_control_deassert(imx6_pcie->pciephy_reset); > imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie); > @@ -483,6 +536,15 @@ static void imx6_pcie_deassert_core_reset(struct > imx6_pcie *imx6_pcie) static void imx6_pcie_init_phy(struct imx6_pcie > *imx6_pcie) { > switch (imx6_pcie->variant) { > + case IMX8MQ: > + /* > + * TODO: Currently this code assumes external > + * oscillator is being used > + */ > + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x, > + IMX8MQ_GPR_PCIE_REF_USE_PAD, > + IMX8MQ_GPR_PCIE_REF_USE_PAD); > + break; > case IMX7D: > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0); @@ > -519,7 +581,8 @@ static void imx6_pcie_init_phy(struct imx6_pcie > *imx6_pcie) > } > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > - IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << > 12); > + imx6_pcie->device_type[0], > + imx6_pcie->device_type[1]); > } > > static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie) @@ -528,7 > +591,8 @@ static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie) > int mult, div; > u32 val; > > - if (imx6_pcie->variant == IMX7D) > + if (imx6_pcie->variant == IMX7D || > + imx6_pcie->variant == IMX8MQ) > return 0; > > switch (phy_rate) { > @@ -616,6 +680,7 @@ static void imx6_pcie_ltssm_enable(struct device > *dev) > IMX6Q_GPR12_PCIE_CTL_2); > break; > case IMX7D: > + case IMX8MQ: /* FALLTHROUGH */ > reset_control_deassert(imx6_pcie->apps_reset); > break; > } > @@ -798,10 +863,24 @@ static void imx6_pcie_clk_disable(struct imx6_pcie > *imx6_pcie) > clk_disable_unprepare(imx6_pcie->pcie_phy); > clk_disable_unprepare(imx6_pcie->pcie_bus); > > - if (imx6_pcie->variant == IMX7D) { > + switch (imx6_pcie->variant) { > + case IMX7D: > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > + break; > + /* > + * Disable the override. Configure the CLK_REQ# high, let the > + * L1SS automatically controlled by HW when link is up. > + * Otherwise, turn off the REF_CLK to save power consumption. > + */ [Richard Zhu] About the L1SS support, there is a back-compatible issue. When one none-L1SS capability EP device is used in one HW design solution. In this case, EP device wouldn't manipulated its own CLK_REQ#. The CLK_REQ# would be high when the override_en is disabled here. That means the REFCLK would be turned off abnormally. System would have problem when the REFCLK of PCIe is turned off abnormally after link is up. > + case IMX8MQ: > + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN, > + 0); > + break; > + default: > + break; > } > } > > @@ -870,6 +949,10 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > imx6_pcie->variant = > (enum imx6_pcie_variants)of_device_get_match_data(dev); > > + imx6_pcie->device_type[0] = IMX6Q_GPR12_DEVICE_TYPE; > + imx6_pcie->device_type[1] = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, > + PCI_EXP_TYPE_ROOT_PORT); > + > dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pci->dbi_base = devm_ioremap_resource(dev, dbi_base); > if (IS_ERR(pci->dbi_base)) > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > return PTR_ERR(imx6_pcie->pcie_inbound_axi); > } > break; > - case IMX7D: > + case IMX8MQ: > + if (of_property_read_u32(node, "fsl,iomux-gpr1x", > + &imx6_pcie->gpr1x)) { > + dev_err(dev, "Failed to get GPR1x address\n"); > + return -EINVAL; > + } > + > + if (of_property_read_u32_array( > + node, "fsl,gpr12-device-type", > + imx6_pcie->device_type, > + ARRAY_SIZE(imx6_pcie->device_type))) { > + dev_err(dev, "Failed to get device type mask/value\n"); > + return -EINVAL; > + } > + > + imx6_pcie->apps_clk_req = > + devm_reset_control_get_exclusive(dev, "clkreq"); > + if (IS_ERR(imx6_pcie->apps_clk_req)) { > + dev_err(dev, "Failed to get PCIE APPS CLK_REQ# reset > control\n"); > + return PTR_ERR(imx6_pcie->apps_clk_req); > + } > + case IMX7D: /* FALLTHROUGH */ > imx6_pcie->pciephy_reset = > devm_reset_control_get_exclusive(dev, > "pciephy"); > if (IS_ERR(imx6_pcie->pciephy_reset)) { @@ -1011,6 +1115,7 @@ > static const struct of_device_id imx6_pcie_of_match[] = { > { .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, }, > { .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, }, > { .compatible = "fsl,imx7d-pcie", .data = (void *)IMX7D, }, > + { .compatible = "fsl,imx8mq-pcie", .data = (void *)IMX8MQ, } , > {}, > }; > > @@ -1027,6 +1132,7 @@ static struct platform_driver imx6_pcie_driver = { > > static int __init imx6_pcie_init(void) > { > +#ifdef CONFIG_ARM > /* > * Since probe() can be deferred we need to make sure that > * hook_fault_code is not called after __init memory is freed @@ > -1036,6 +1142,7 @@ static int __init imx6_pcie_init(void) > */ > hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0, > "external abort on non-linefetch"); > +#endif > > return platform_driver_register(&imx6_pcie_driver); > } > -- > 2.19.1
