Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ

From: Andrey Smirnov
Date: Mon Nov 26 2018 - 13:10:06 EST

Next message: David Miller: "Re: [PATCH v2 16/20] sparc: perf/core: advertise PMU exclusion capability"
Previous message: Dan Williams: "Re: [PATCH 0/7] ACPI HMAT memory sysfs representation"
Next in thread: Richard Zhu: "RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu <hongxing.zhu@xxxxxxx> wrote:
>
> 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.
>

I don't have a lot of expertise in this area, so please take my
comment with a grain of salt. The way I understand it, is in legacy
systems that do not support L1SS with CLKREQ that feature shouldn't be
enabled/used. Unless I've missed something, I think it should be
possible to do so and disable the feature by configuring corresponding
CLKREQ_B pad as GPIO and adding a GPIO hog node to pull CLKREQ low. Do
you see any problems with this approach?

Regardless though, I wasn't really planning on including PM support in
this series. The code in question shouldn't even be in the patch since
it'll never be executed because imx6_pcie_clk_disable() is only called
by imx6_pcie_suspend_noirq() which is a no-op on all variants except
i.MX7D.

I'll drop all of the unused PM-related bits I missed from the series
and we can add them later in a separate submission.

Thanks,
Andrey Smirnov

Next message: David Miller: "Re: [PATCH v2 16/20] sparc: perf/core: advertise PMU exclusion capability"
Previous message: Dan Williams: "Re: [PATCH 0/7] ACPI HMAT memory sysfs representation"
Next in thread: Richard Zhu: "RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]