diff mbox series

[v4,5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

Message ID 1635406037-20900-6-git-send-email-hongxing.zhu@nxp.com
State Superseded
Headers show
Series add the imx8m pcie phy driver and imx8mm pcie support | expand

Commit Message

Richard Zhu Oct. 28, 2021, 7:27 a.m. UTC
Add the standalone i.MX8 PCIe PHY driver.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
 drivers/phy/freescale/Kconfig              |   9 +
 drivers/phy/freescale/Makefile             |   1 +
 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234 +++++++++++++++++++++
 3 files changed, 244 insertions(+)
 create mode 100644 drivers/phy/freescale/phy-fsl-imx8m-pcie.c

Comments

Marcel Ziswiler Oct. 29, 2021, 8:12 a.m. UTC | #1
On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> Add the standalone i.MX8 PCIe PHY driver.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Unfortunately, this version no longer works for our IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in-
lined below.

> ---
>  drivers/phy/freescale/Kconfig              |   9 +
>  drivers/phy/freescale/Makefile             |   1 +
>  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234 +++++++++++++++++++++
>  3 files changed, 244 insertions(+)
>  create mode 100644 drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> 
> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
> index 320630ffe3cd..de9ee7020f76 100644
> --- a/drivers/phy/freescale/Kconfig
> +++ b/drivers/phy/freescale/Kconfig
> @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
>         help
>           Enable this to add support for the Mixel DSI PHY as found
>           on NXP's i.MX8 family of SOCs.
> +
> +config PHY_FSL_IMX8M_PCIE
> +       tristate "Freescale i.MX8 PCIE PHY"

Above description is missing the M as in i.MX 8M.

> +       depends on OF && HAS_IOMEM
> +       select GENERIC_PHY
> +       default ARCH_MXC && ARM64
> +       help
> +         Enable this to add support for the PCIE PHY as found on
> +         i.MX8M family of SOCs.
> diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
> index 1d02e3869b45..55d07c742ab0 100644
> --- a/drivers/phy/freescale/Makefile
> +++ b/drivers/phy/freescale/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       += phy-fsl-imx8mq-usb.o
>  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      += phy-fsl-imx8-mipi-dphy.o
> +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       += phy-fsl-imx8m-pcie.o
> diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> new file mode 100644
> index 000000000000..4b4402eaddcc
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021 NXP
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <dt-bindings/phy/phy-imx8-pcie.h>
> +
> +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184
> +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0)
> +#define IMX8MM_PCIE_PHY_CMN_REG062     0x188
> +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3)
> +#define IMX8MM_PCIE_PHY_CMN_REG063     0x18C
> +#define  AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6)
> +#define IMX8MM_PCIE_PHY_CMN_REG064     0x190
> +#define  ANA_AUX_RX_TX_SEL_TX          BIT(7)
> +#define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> +#define  ANA_AUX_TX_TERM               BIT(2)
> +#define IMX8MM_PCIE_PHY_CMN_REG065     0x194
> +#define  ANA_AUX_RX_TERM               (BIT(7) | BIT(4))
> +#define  ANA_AUX_TX_LVL                        GENMASK(3, 0)
> +#define IMX8MM_PCIE_PHY_CMN_REG75      0x1D4
> +#define  PCIE_PHY_CMN_REG75_PLL_DONE   0x3
> +#define PCIE_PHY_TRSV_REG5             0x414
> +#define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D
> +#define PCIE_PHY_TRSV_REG6             0x418
> +#define  PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> +
> +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> +#define IMX8MM_GPR_PCIE_REF_CLK_PLL    FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3)
> +#define IMX8MM_GPR_PCIE_REF_CLK_EXT    FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2)
> +#define IMX8MM_GPR_PCIE_AUX_EN         BIT(19)
> +#define IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> +#define IMX8MM_GPR_PCIE_POWER_OFF      BIT(17)
> +#define IMX8MM_GPR_PCIE_SSC_EN         BIT(16)
> +#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> +
> +struct imx8_pcie_phy {
> +       void __iomem            *base;
> +       struct clk              *clk;
> +       struct phy              *phy;
> +       struct regmap           *iomuxc_gpr;
> +       struct reset_control    *reset;
> +       u32                     refclk_pad_mode;
> +       u32                     tx_deemph_gen1;
> +       u32                     tx_deemph_gen2;
> +       bool                    clkreq_unused;
> +};
> +
> +static int imx8_pcie_phy_init(struct phy *phy)
> +{
> +       int ret;
> +       u32 val, pad_mode;
> +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> +
> +       reset_control_assert(imx8_phy->reset);
> +
> +       pad_mode = imx8_phy->refclk_pad_mode;
> +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't hooked */
> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,
> +                          imx8_phy->clkreq_unused ?
> +                          0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_AUX_EN,
> +                          pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
> +                          IMX8MM_GPR_PCIE_AUX_EN : 0);

V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4 stopped working for our output use-case as it only
enables it for the input use-case. If I enable this one always it starts working again.

> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_POWER_OFF, 0);
> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_SSC_EN, 0);
> +
> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_REF_CLK_SEL,
> +                          pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
> +                          IMX8MM_GPR_PCIE_REF_CLK_EXT :
> +                          IMX8MM_GPR_PCIE_REF_CLK_PLL);
> +       usleep_range(100, 200);
> +
> +       /* Do the PHY common block reset */
> +       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> +                          IMX8MM_GPR_PCIE_CMN_RST,
> +                          IMX8MM_GPR_PCIE_CMN_RST);
> +       usleep_range(200, 500);
> +
> +
> +       if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> +               /* Configure the pad as input */
> +               val = readl(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +               writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +       } else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> +               /* Configure the PHY to output the refclock via pad */
> +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
> +               writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
> +               val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
> +               writel(val | ANA_AUX_RX_TERM_GND_EN,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);
> +               writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
> +                      imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG065);
> +       }
> +
> +       /* Tune PHY de-emphasis setting to pass PCIe compliance. */
> +       writel(imx8_phy->tx_deemph_gen1, imx8_phy->base + PCIE_PHY_TRSV_REG5);
> +       writel(imx8_phy->tx_deemph_gen2, imx8_phy->base + PCIE_PHY_TRSV_REG6);
> +
> +       reset_control_deassert(imx8_phy->reset);
> +
> +       /* Polling to check the phy is ready or not. */
> +       ret = readl_poll_timeout(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG75,
> +                                val, val == PCIE_PHY_CMN_REG75_PLL_DONE,
> +                                10, 20000);
> +       return ret;
> +}
> +
> +static int imx8_pcie_phy_power_on(struct phy *phy)
> +{
> +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> +
> +       return clk_prepare_enable(imx8_phy->clk);
> +}
> +
> +static int imx8_pcie_phy_power_off(struct phy *phy)
> +{
> +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> +
> +       clk_disable_unprepare(imx8_phy->clk);
> +
> +       return 0;
> +}
> +
> +static const struct phy_ops imx8_pcie_phy_ops = {
> +       .init           = imx8_pcie_phy_init,
> +       .power_on       = imx8_pcie_phy_power_on,
> +       .power_off      = imx8_pcie_phy_power_off,
> +       .owner          = THIS_MODULE,
> +};
> +
> +static int imx8_pcie_phy_probe(struct platform_device *pdev)
> +{
> +       struct phy_provider *phy_provider;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct imx8_pcie_phy *imx8_phy;
> +       struct resource *res;
> +
> +       imx8_phy = devm_kzalloc(dev, sizeof(*imx8_phy), GFP_KERNEL);
> +       if (!imx8_phy)
> +               return -ENOMEM;
> +
> +       /* get PHY refclk pad mode */
> +       of_property_read_u32(np, "fsl,refclk-pad-mode",
> +                            &imx8_phy->refclk_pad_mode);
> +
> +       if (of_property_read_u32(np, "fsl,tx-deemph-gen1",
> +                                &imx8_phy->tx_deemph_gen1))
> +               imx8_phy->tx_deemph_gen1 = 0;

Is zero here really a sane default or should we rather use the previous hard-coded value?

> +
> +       if (of_property_read_u32(np, "fsl,tx-deemph-gen2",
> +                                &imx8_phy->tx_deemph_gen2))
> +               imx8_phy->tx_deemph_gen2 = 0;

Ditto.

> +
> +       if (of_property_read_bool(np, "fsl,clkreq-unsupported"))
> +               imx8_phy->clkreq_unused = true;
> +       else
> +               imx8_phy->clkreq_unused = false;
> +
> +       imx8_phy->clk = devm_clk_get(dev, "ref");
> +       if (IS_ERR(imx8_phy->clk)) {
> +               dev_err(dev, "failed to get imx pcie phy clock\n");
> +               return PTR_ERR(imx8_phy->clk);
> +       }
> +
> +       /* Grab GPR config register range */
> +       imx8_phy->iomuxc_gpr =
> +                syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +       if (IS_ERR(imx8_phy->iomuxc_gpr)) {
> +               dev_err(dev, "unable to find iomuxc registers\n");
> +               return PTR_ERR(imx8_phy->iomuxc_gpr);
> +       }
> +
> +       imx8_phy->reset = devm_reset_control_get_exclusive(dev, "pciephy");
> +       if (IS_ERR(imx8_phy->reset)) {
> +               dev_err(dev, "Failed to get PCIEPHY reset control\n");
> +               return PTR_ERR(imx8_phy->reset);
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       imx8_phy->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(imx8_phy->base))
> +               return PTR_ERR(imx8_phy->base);
> +
> +       imx8_phy->phy = devm_phy_create(dev, NULL, &imx8_pcie_phy_ops);
> +       if (IS_ERR(imx8_phy->phy))
> +               return PTR_ERR(imx8_phy->phy);
> +
> +       phy_set_drvdata(imx8_phy->phy, imx8_phy);
> +
> +       phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +       return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id imx8_pcie_phy_of_match[] = {
> +       {.compatible = "fsl,imx8mm-pcie-phy",},
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
> +
> +static struct platform_driver imx8_pcie_phy_driver = {
> +       .probe  = imx8_pcie_phy_probe,
> +       .driver = {
> +               .name   = "imx8-pcie-phy",
> +               .of_match_table = imx8_pcie_phy_of_match,
> +       }
> +};
> +module_platform_driver(imx8_pcie_phy_driver);
> +
> +MODULE_DESCRIPTION("FSL IMX8 PCIE PHY driver");
> +MODULE_LICENSE("GPL");
Richard Zhu Oct. 29, 2021, 8:45 a.m. UTC | #2
> -----Original Message-----
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Sent: Friday, October 29, 2021 4:13 PM
> To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> l.stach@pengutronix.de; shawnguo@kernel.org;
> tharvey@gateworks.com; galak@kernel.crashing.org; Richard Zhu
> <hongxing.zhu@nxp.com>
> Cc: linux-phy@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> standalone phy driver
> 
> On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > Add the standalone i.MX8 PCIe PHY driver.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Unfortunately, this version no longer works for our
> IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in- lined
> below.
> 
[Richard Zhu] Sorry to hear about that. Then, it seems that this bit should be
 set anyway.
It's hard to understand this bit refer to the RM document. Sigh ☹.
Would set the AUX_EN bit later.

> > ---
> >  drivers/phy/freescale/Kconfig              |   9 +
> >  drivers/phy/freescale/Makefile             |   1 +
> >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > +++++++++++++++++++++
> >  3 files changed, 244 insertions(+)
> >  create mode 100644 drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> >
> > diff --git a/drivers/phy/freescale/Kconfig
> > b/drivers/phy/freescale/Kconfig index 320630ffe3cd..de9ee7020f76
> > 100644
> > --- a/drivers/phy/freescale/Kconfig
> > +++ b/drivers/phy/freescale/Kconfig
> > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> >         help
> >           Enable this to add support for the Mixel DSI PHY as
> found
> >           on NXP's i.MX8 family of SOCs.
> > +
> > +config PHY_FSL_IMX8M_PCIE
> > +       tristate "Freescale i.MX8 PCIE PHY"
> 
> Above description is missing the M as in i.MX 8M.
[Richard Zhu] Okay, would be added later.
Thanks.

> 
> > +       depends on OF && HAS_IOMEM
> > +       select GENERIC_PHY
> > +       default ARCH_MXC && ARM64
> > +       help
> > +         Enable this to add support for the PCIE PHY as found on
> > +         i.MX8M family of SOCs.
> > diff --git a/drivers/phy/freescale/Makefile
> > b/drivers/phy/freescale/Makefile index 1d02e3869b45..55d07c742ab0
> > 100644
> > --- a/drivers/phy/freescale/Makefile
> > +++ b/drivers/phy/freescale/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> phy-fsl-imx8mq-usb.o
> >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> phy-fsl-imx8-mipi-dphy.o
> > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> phy-fsl-imx8m-pcie.o
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > new file mode 100644
> > index 000000000000..4b4402eaddcc
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > @@ -0,0 +1,234 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2021 NXP
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/delay.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > +#include <linux/module.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <dt-bindings/phy/phy-imx8-pcie.h>
> > +
> > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184 #define
> > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> #define  ANA_AUX_RX_TX_SEL_TX
> > +BIT(7)
> #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> #define
> > +ANA_AUX_TX_TERM               BIT(2) #define
> > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> #define  ANA_AUX_RX_TERM
> > +(BIT(7) | BIT(4))
> #define  ANA_AUX_TX_LVL
> > +GENMASK(3, 0) #define
> IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> PCIE_PHY_TRSV_REG5
> > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D #define
> > +PCIE_PHY_TRSV_REG6             0x418 #define
> > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > +
> > +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> #define
> > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> #define
> > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> IMX8MM_GPR_PCIE_SSC_EN
> > +BIT(16) #define
> IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > +
> > +struct imx8_pcie_phy {
> > +       void __iomem            *base;
> > +       struct clk              *clk;
> > +       struct phy              *phy;
> > +       struct regmap           *iomuxc_gpr;
> > +       struct reset_control    *reset;
> > +       u32                     refclk_pad_mode;
> > +       u32                     tx_deemph_gen1;
> > +       u32                     tx_deemph_gen2;
> > +       bool                    clkreq_unused; };
> > +
> > +static int imx8_pcie_phy_init(struct phy *phy) {
> > +       int ret;
> > +       u32 val, pad_mode;
> > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > +
> > +       reset_control_assert(imx8_phy->reset);
> > +
> > +       pad_mode = imx8_phy->refclk_pad_mode;
> > +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't
> hooked */
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >
> +                          IMX8MM_GPR_PCIE_AUX_EN_
> OVERRIDE,
> > +                          imx8_phy->clkreq_unused ?
> > +                          0 :
> IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> > +                          IMX8MM_GPR_PCIE_AUX_EN,
> > +                          pad_mode ==
> IMX8_PCIE_REFCLK_PAD_INPUT ?
> > +                          IMX8MM_GPR_PCIE_AUX_EN :
> 0);
> 
> V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4
> stopped working for our output use-case as it only enables it for the input
> use-case. If I enable this one always it starts working again.
[Richard Zhu] See my comment above.
Sorry to bring the regression on your board.

> 
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >
> +                          IMX8MM_GPR_PCIE_POWER_O
> FF, 0);
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> > +                          IMX8MM_GPR_PCIE_SSC_EN,
> 0);
> > +
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >
> +                          IMX8MM_GPR_PCIE_REF_CLK_S
> EL,
> > +                          pad_mode ==
> IMX8_PCIE_REFCLK_PAD_INPUT ?
> >
> +                          IMX8MM_GPR_PCIE_REF_CLK_
> EXT :
> >
> +                          IMX8MM_GPR_PCIE_REF_CLK_
> PLL);
> > +       usleep_range(100, 200);
> > +
> > +       /* Do the PHY common block reset */
> > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >
> +                          IMX8MM_GPR_PCIE_CMN_RST,
> >
> +                          IMX8MM_GPR_PCIE_CMN_RST)
> ;
> > +       usleep_range(200, 500);
> > +
> > +
> > +       if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> > +               /* Configure the pad as input */
> > +               val = readl(imx8_phy->base +
> > +IMX8MM_PCIE_PHY_CMN_REG061);
> > +               writel(val &
> ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG061);
> > +       } else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> > +               /* Configure the PHY to output the refclock via
> pad */
> > +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG061);
> > +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG062);
> > +               writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG063);
> > +               val = ANA_AUX_RX_TX_SEL_TX |
> ANA_AUX_TX_TERM;
> > +               writel(val | ANA_AUX_RX_TERM_GND_EN,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG064);
> > +               writel(ANA_AUX_RX_TERM |
> ANA_AUX_TX_LVL,
> > +                      imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG065);
> > +       }
> > +
> > +       /* Tune PHY de-emphasis setting to pass PCIe compliance.
> */
> > +       writel(imx8_phy->tx_deemph_gen1, imx8_phy->base +
> > +PCIE_PHY_TRSV_REG5);
> > +       writel(imx8_phy->tx_deemph_gen2, imx8_phy->base +
> > +PCIE_PHY_TRSV_REG6);
> > +
> > +       reset_control_deassert(imx8_phy->reset);
> > +
> > +       /* Polling to check the phy is ready or not. */
> > +       ret = readl_poll_timeout(imx8_phy->base +
> > +IMX8MM_PCIE_PHY_CMN_REG75,
> > +                                val, val ==
> > +PCIE_PHY_CMN_REG75_PLL_DONE,
> > +                                10, 20000);
> > +       return ret;
> > +}
> > +
> > +static int imx8_pcie_phy_power_on(struct phy *phy) {
> > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > +
> > +       return clk_prepare_enable(imx8_phy->clk);
> > +}
> > +
> > +static int imx8_pcie_phy_power_off(struct phy *phy) {
> > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > +
> > +       clk_disable_unprepare(imx8_phy->clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct phy_ops imx8_pcie_phy_ops = {
> > +       .init           = imx8_pcie_phy_init,
> > +       .power_on       = imx8_pcie_phy_power_on,
> > +       .power_off      = imx8_pcie_phy_power_off,
> > +       .owner          = THIS_MODULE, };
> > +
> > +static int imx8_pcie_phy_probe(struct platform_device *pdev) {
> > +       struct phy_provider *phy_provider;
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       struct imx8_pcie_phy *imx8_phy;
> > +       struct resource *res;
> > +
> > +       imx8_phy = devm_kzalloc(dev, sizeof(*imx8_phy),
> GFP_KERNEL);
> > +       if (!imx8_phy)
> > +               return -ENOMEM;
> > +
> > +       /* get PHY refclk pad mode */
> > +       of_property_read_u32(np, "fsl,refclk-pad-mode",
> >
> +                            &imx8_phy->refclk_pad_mod
> e);
> > +
> > +       if (of_property_read_u32(np, "fsl,tx-deemph-gen1",
> >
> +                                &imx8_phy->tx_deemp
> h_gen1))
> > +               imx8_phy->tx_deemph_gen1 = 0;
> 
> Is zero here really a sane default or should we rather use the previous
> hard-coded value?
> 
> > +
> > +       if (of_property_read_u32(np, "fsl,tx-deemph-gen2",
> >
> +                                &imx8_phy->tx_deemp
> h_gen2))
> > +               imx8_phy->tx_deemph_gen2 = 0;
> 
> Ditto.
[Richard Zhu] Refer to the RM document, the default values is zero.
How about to not touch the registers if the values of the tx_deemph_gen#
 are zero?

In the end, thanks a lot for your tests on this series patches.

BR
Richard
> 
> > +
> > +       if (of_property_read_bool(np, "fsl,clkreq-unsupported"))
> > +               imx8_phy->clkreq_unused = true;
> > +       else
> > +               imx8_phy->clkreq_unused = false;
> > +
> > +       imx8_phy->clk = devm_clk_get(dev, "ref");
> > +       if (IS_ERR(imx8_phy->clk)) {
> > +               dev_err(dev, "failed to get imx pcie phy
> clock\n");
> > +               return PTR_ERR(imx8_phy->clk);
> > +       }
> > +
> > +       /* Grab GPR config register range */
> > +       imx8_phy->iomuxc_gpr =
> > +
> > +syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> > +       if (IS_ERR(imx8_phy->iomuxc_gpr)) {
> > +               dev_err(dev, "unable to find iomuxc
> registers\n");
> > +               return PTR_ERR(imx8_phy->iomuxc_gpr);
> > +       }
> > +
> > +       imx8_phy->reset = devm_reset_control_get_exclusive(dev,
> > +"pciephy");
> > +       if (IS_ERR(imx8_phy->reset)) {
> > +               dev_err(dev, "Failed to get PCIEPHY reset
> control\n");
> > +               return PTR_ERR(imx8_phy->reset);
> > +       }
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       imx8_phy->base = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(imx8_phy->base))
> > +               return PTR_ERR(imx8_phy->base);
> > +
> > +       imx8_phy->phy = devm_phy_create(dev, NULL,
> > +&imx8_pcie_phy_ops);
> > +       if (IS_ERR(imx8_phy->phy))
> > +               return PTR_ERR(imx8_phy->phy);
> > +
> > +       phy_set_drvdata(imx8_phy->phy, imx8_phy);
> > +
> > +       phy_provider = devm_of_phy_provider_register(dev,
> > +of_phy_simple_xlate);
> > +
> > +       return PTR_ERR_OR_ZERO(phy_provider); }
> > +
> > +static const struct of_device_id imx8_pcie_phy_of_match[] = {
> > +       {.compatible = "fsl,imx8mm-pcie-phy",},
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
> > +
> > +static struct platform_driver imx8_pcie_phy_driver = {
> > +       .probe  = imx8_pcie_phy_probe,
> > +       .driver = {
> > +               .name   = "imx8-pcie-phy",
> > +               .of_match_table = imx8_pcie_phy_of_match,
> > +       }
> > +};
> > +module_platform_driver(imx8_pcie_phy_driver);
> > +
> > +MODULE_DESCRIPTION("FSL IMX8 PCIE PHY driver");
> > +MODULE_LICENSE("GPL");
Tim Harvey Oct. 29, 2021, 5:44 p.m. UTC | #3
On Fri, Oct 29, 2021 at 1:45 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
>
> > -----Original Message-----
> > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > Sent: Friday, October 29, 2021 4:13 PM
> > To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > l.stach@pengutronix.de; shawnguo@kernel.org;
> > tharvey@gateworks.com; galak@kernel.crashing.org; Richard Zhu
> > <hongxing.zhu@nxp.com>
> > Cc: linux-phy@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> > kernel@pengutronix.de; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> > standalone phy driver
> >
> > On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > > Add the standalone i.MX8 PCIe PHY driver.
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> >
> > Unfortunately, this version no longer works for our
> > IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in- lined
> > below.
> >
> [Richard Zhu] Sorry to hear about that. Then, it seems that this bit should be
>  set anyway.
> It's hard to understand this bit refer to the RM document. Sigh ☹.
> Would set the AUX_EN bit later.
>
> > > ---
> > >  drivers/phy/freescale/Kconfig              |   9 +
> > >  drivers/phy/freescale/Makefile             |   1 +
> > >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > > +++++++++++++++++++++
> > >  3 files changed, 244 insertions(+)
> > >  create mode 100644 drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > >
> > > diff --git a/drivers/phy/freescale/Kconfig
> > > b/drivers/phy/freescale/Kconfig index 320630ffe3cd..de9ee7020f76
> > > 100644
> > > --- a/drivers/phy/freescale/Kconfig
> > > +++ b/drivers/phy/freescale/Kconfig
> > > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> > >         help
> > >           Enable this to add support for the Mixel DSI PHY as
> > found
> > >           on NXP's i.MX8 family of SOCs.
> > > +
> > > +config PHY_FSL_IMX8M_PCIE
> > > +       tristate "Freescale i.MX8 PCIE PHY"
> >
> > Above description is missing the M as in i.MX 8M.
> [Richard Zhu] Okay, would be added later.
> Thanks.
>
> >
> > > +       depends on OF && HAS_IOMEM
> > > +       select GENERIC_PHY
> > > +       default ARCH_MXC && ARM64
> > > +       help
> > > +         Enable this to add support for the PCIE PHY as found on
> > > +         i.MX8M family of SOCs.
> > > diff --git a/drivers/phy/freescale/Makefile
> > > b/drivers/phy/freescale/Makefile index 1d02e3869b45..55d07c742ab0
> > > 100644
> > > --- a/drivers/phy/freescale/Makefile
> > > +++ b/drivers/phy/freescale/Makefile
> > > @@ -1,3 +1,4 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> > phy-fsl-imx8mq-usb.o
> > >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> > phy-fsl-imx8-mipi-dphy.o
> > > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> > phy-fsl-imx8m-pcie.o
> > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > new file mode 100644
> > > index 000000000000..4b4402eaddcc
> > > --- /dev/null
> > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > @@ -0,0 +1,234 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2021 NXP
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > +#include <linux/module.h>
> > > +#include <linux/phy/phy.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/reset.h>
> > > +#include <dt-bindings/phy/phy-imx8-pcie.h>
> > > +
> > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184 #define
> > > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > #define  ANA_AUX_RX_TX_SEL_TX
> > > +BIT(7)
> > #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> > #define
> > > +ANA_AUX_TX_TERM               BIT(2) #define
> > > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > #define  ANA_AUX_RX_TERM
> > > +(BIT(7) | BIT(4))
> > #define  ANA_AUX_TX_LVL
> > > +GENMASK(3, 0) #define
> > IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> > PCIE_PHY_TRSV_REG5
> > > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D #define
> > > +PCIE_PHY_TRSV_REG6             0x418 #define
> > > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > > +
> > > +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> > #define
> > > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > > +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> > #define
> > > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> > IMX8MM_GPR_PCIE_SSC_EN
> > > +BIT(16) #define
> > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > > +
> > > +struct imx8_pcie_phy {
> > > +       void __iomem            *base;
> > > +       struct clk              *clk;
> > > +       struct phy              *phy;
> > > +       struct regmap           *iomuxc_gpr;
> > > +       struct reset_control    *reset;
> > > +       u32                     refclk_pad_mode;
> > > +       u32                     tx_deemph_gen1;
> > > +       u32                     tx_deemph_gen2;
> > > +       bool                    clkreq_unused; };
> > > +
> > > +static int imx8_pcie_phy_init(struct phy *phy) {
> > > +       int ret;
> > > +       u32 val, pad_mode;
> > > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > > +
> > > +       reset_control_assert(imx8_phy->reset);
> > > +
> > > +       pad_mode = imx8_phy->refclk_pad_mode;
> > > +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't
> > hooked */
> > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > IOMUXC_GPR14,
> > >
> > +                          IMX8MM_GPR_PCIE_AUX_EN_
> > OVERRIDE,
> > > +                          imx8_phy->clkreq_unused ?
> > > +                          0 :
> > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > IOMUXC_GPR14,
> > > +                          IMX8MM_GPR_PCIE_AUX_EN,
> > > +                          pad_mode ==
> > IMX8_PCIE_REFCLK_PAD_INPUT ?
> > > +                          IMX8MM_GPR_PCIE_AUX_EN :
> > 0);
> >
> > V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4
> > stopped working for our output use-case as it only enables it for the input
> > use-case. If I enable this one always it starts working again.
> [Richard Zhu] See my comment above.
> Sorry to bring the regression on your board.
>

Marcel,

Your board does not use an external clk, but does it hook up CLKREQ#
from the socket to either I2C4_SCL or UART4_RXD and pin muxed as such?

For my board that uses an external clk and does not connect CLKREQ# to
the IMX8MM I need to disable that bit. As Richard says we have invalid
documentation for these bits unfortunately which is not helping.

Richard, when we do figure out proper documentation for these bits I
suggest you also add a comment block right above their #defines in the
phy driver with the correct documentation to avoid future confusion.
NXP has had so many mistakes in the various IMX8M RM's and I fear they
will never get fixed.

Best regards,

Tim
Richard Zhu Nov. 1, 2021, 8:19 a.m. UTC | #4
> -----Original Message-----
> From: Tim Harvey <tharvey@gateworks.com>
> Sent: Saturday, October 30, 2021 1:45 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>
> Cc: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> l.stach@pengutronix.de; shawnguo@kernel.org;
> galak@kernel.crashing.org; linux-phy@lists.infradead.org;
> linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> standalone phy driver
> 
> On Fri, Oct 29, 2021 at 1:45 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > Sent: Friday, October 29, 2021 4:13 PM
> > > To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > l.stach@pengutronix.de; shawnguo@kernel.org;
> tharvey@gateworks.com;
> > > galak@kernel.crashing.org; Richard Zhu <hongxing.zhu@nxp.com>
> > > Cc: linux-phy@lists.infradead.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > kernel@pengutronix.de; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the
> > > imx8 pcie standalone phy driver
> > >
> > > On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > > > Add the standalone i.MX8 PCIe PHY driver.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > >
> > > Unfortunately, this version no longer works for our
> > > IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in-
> lined
> > > below.
> > >
> > [Richard Zhu] Sorry to hear about that. Then, it seems that this bit
> > should be  set anyway.
> > It's hard to understand this bit refer to the RM document. Sigh ☹.
> > Would set the AUX_EN bit later.
> >
> > > > ---
> > > >  drivers/phy/freescale/Kconfig              |   9 +
> > > >  drivers/phy/freescale/Makefile             |   1 +
> > > >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > > > +++++++++++++++++++++
> > > >  3 files changed, 244 insertions(+)  create mode 100644
> > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > >
> > > > diff --git a/drivers/phy/freescale/Kconfig
> > > > b/drivers/phy/freescale/Kconfig index 320630ffe3cd..de9ee7020f76
> > > > 100644
> > > > --- a/drivers/phy/freescale/Kconfig
> > > > +++ b/drivers/phy/freescale/Kconfig
> > > > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> > > >         help
> > > >           Enable this to add support for the Mixel DSI PHY as
> > > found
> > > >           on NXP's i.MX8 family of SOCs.
> > > > +
> > > > +config PHY_FSL_IMX8M_PCIE
> > > > +       tristate "Freescale i.MX8 PCIE PHY"
> > >
> > > Above description is missing the M as in i.MX 8M.
> > [Richard Zhu] Okay, would be added later.
> > Thanks.
> >
> > >
> > > > +       depends on OF && HAS_IOMEM
> > > > +       select GENERIC_PHY
> > > > +       default ARCH_MXC && ARM64
> > > > +       help
> > > > +         Enable this to add support for the PCIE PHY as found on
> > > > +         i.MX8M family of SOCs.
> > > > diff --git a/drivers/phy/freescale/Makefile
> > > > b/drivers/phy/freescale/Makefile index
> 1d02e3869b45..55d07c742ab0
> > > > 100644
> > > > --- a/drivers/phy/freescale/Makefile
> > > > +++ b/drivers/phy/freescale/Makefile
> > > > @@ -1,3 +1,4 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> > > phy-fsl-imx8mq-usb.o
> > > >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> > > phy-fsl-imx8-mipi-dphy.o
> > > > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> > > phy-fsl-imx8m-pcie.o
> > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > new file mode 100644
> > > > index 000000000000..4b4402eaddcc
> > > > --- /dev/null
> > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > @@ -0,0 +1,234 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright 2021 NXP
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/phy/phy.h>
> > > > +#include <linux/platform_device.h> #include <linux/regmap.h>
> > > > +#include <linux/reset.h> #include
> > > > +<dt-bindings/phy/phy-imx8-pcie.h>
> > > > +
> > > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184 #define
> > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > > > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > > > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > > > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > > > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > > #define  ANA_AUX_RX_TX_SEL_TX
> > > > +BIT(7)
> > > #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> > > #define
> > > > +ANA_AUX_TX_TERM               BIT(2) #define
> > > > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > > #define  ANA_AUX_RX_TERM
> > > > +(BIT(7) | BIT(4))
> > > #define  ANA_AUX_TX_LVL
> > > > +GENMASK(3, 0) #define
> > > IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > > > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> > > PCIE_PHY_TRSV_REG5
> > > > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D
> #define
> > > > +PCIE_PHY_TRSV_REG6             0x418 #define
> > > > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > > > +
> > > > +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> > > #define
> > > > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > > > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > > > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > > > +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> > > #define
> > > > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> > > IMX8MM_GPR_PCIE_SSC_EN
> > > > +BIT(16) #define
> > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > > > +
> > > > +struct imx8_pcie_phy {
> > > > +       void __iomem            *base;
> > > > +       struct clk              *clk;
> > > > +       struct phy              *phy;
> > > > +       struct regmap           *iomuxc_gpr;
> > > > +       struct reset_control    *reset;
> > > > +       u32                     refclk_pad_mode;
> > > > +       u32                     tx_deemph_gen1;
> > > > +       u32                     tx_deemph_gen2;
> > > > +       bool                    clkreq_unused; };
> > > > +
> > > > +static int imx8_pcie_phy_init(struct phy *phy) {
> > > > +       int ret;
> > > > +       u32 val, pad_mode;
> > > > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > > > +
> > > > +       reset_control_assert(imx8_phy->reset);
> > > > +
> > > > +       pad_mode = imx8_phy->refclk_pad_mode;
> > > > +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't
> > > hooked */
> > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > IOMUXC_GPR14,
> > > >
> > > +                          IMX8MM_GPR_PCIE_AUX_EN_
> > > OVERRIDE,
> > > > +                          imx8_phy->clkreq_unused ?
> > > > +                          0 :
> > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > IOMUXC_GPR14,
> > > > +                          IMX8MM_GPR_PCIE_AUX_EN,
> > > > +                          pad_mode ==
> > > IMX8_PCIE_REFCLK_PAD_INPUT ?
> > > > +                          IMX8MM_GPR_PCIE_AUX_EN :
> > > 0);
> > >
> > > V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4
> stopped
> > > working for our output use-case as it only enables it for the input
> > > use-case. If I enable this one always it starts working again.
> > [Richard Zhu] See my comment above.
> > Sorry to bring the regression on your board.
> >
> 
> Marcel,
> 
> Your board does not use an external clk, but does it hook up CLKREQ#
> from the socket to either I2C4_SCL or UART4_RXD and pin muxed as
> such?
> 
> For my board that uses an external clk and does not connect CLKREQ# to
> the IMX8MM I need to disable that bit. As Richard says we have invalid
> documentation for these bits unfortunately which is not helping.
> 
> Richard, when we do figure out proper documentation for these bits I
> suggest you also add a comment block right above their #defines in the
> phy driver with the correct documentation to avoid future confusion.
> NXP has had so many mistakes in the various IMX8M RM's and I fear they
> will never get fixed.
[Richard Zhu] Hi Tim: 
I took look at the validation codes, and found that the AUX_EN is always
 set to be 1b'1. Whatever the reference clock mode is selected.
I'm sending a query email to design team, but I'm not sure I can get response
in time.
Can you help to take a double tests at your board when AUX_EN(bit19 of GPR14)
is set to be 1b'1 firstly?
Thanks in advanced.

BR
Richard
> 
> Best regards,
> 
> Tim
Tim Harvey Nov. 1, 2021, 5:13 p.m. UTC | #5
On Mon, Nov 1, 2021 at 1:19 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Tim Harvey <tharvey@gateworks.com>
> > Sent: Saturday, October 30, 2021 1:45 AM
> > To: Richard Zhu <hongxing.zhu@nxp.com>; Marcel Ziswiler
> > <marcel.ziswiler@toradex.com>
> > Cc: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > l.stach@pengutronix.de; shawnguo@kernel.org;
> > galak@kernel.crashing.org; linux-phy@lists.infradead.org;
> > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> > standalone phy driver
> >
> > On Fri, Oct 29, 2021 at 1:45 AM Richard Zhu <hongxing.zhu@nxp.com>
> > wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > Sent: Friday, October 29, 2021 4:13 PM
> > > > To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > > l.stach@pengutronix.de; shawnguo@kernel.org;
> > tharvey@gateworks.com;
> > > > galak@kernel.crashing.org; Richard Zhu <hongxing.zhu@nxp.com>
> > > > Cc: linux-phy@lists.infradead.org;
> > > > linux-arm-kernel@lists.infradead.org;
> > > > kernel@pengutronix.de; devicetree@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the
> > > > imx8 pcie standalone phy driver
> > > >
> > > > On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > > > > Add the standalone i.MX8 PCIe PHY driver.
> > > > >
> > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > >
> > > > Unfortunately, this version no longer works for our
> > > > IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in-
> > lined
> > > > below.
> > > >
> > > [Richard Zhu] Sorry to hear about that. Then, it seems that this bit
> > > should be  set anyway.
> > > It's hard to understand this bit refer to the RM document. Sigh ☹.
> > > Would set the AUX_EN bit later.
> > >
> > > > > ---
> > > > >  drivers/phy/freescale/Kconfig              |   9 +
> > > > >  drivers/phy/freescale/Makefile             |   1 +
> > > > >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > > > > +++++++++++++++++++++
> > > > >  3 files changed, 244 insertions(+)  create mode 100644
> > > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > >
> > > > > diff --git a/drivers/phy/freescale/Kconfig
> > > > > b/drivers/phy/freescale/Kconfig index 320630ffe3cd..de9ee7020f76
> > > > > 100644
> > > > > --- a/drivers/phy/freescale/Kconfig
> > > > > +++ b/drivers/phy/freescale/Kconfig
> > > > > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> > > > >         help
> > > > >           Enable this to add support for the Mixel DSI PHY as
> > > > found
> > > > >           on NXP's i.MX8 family of SOCs.
> > > > > +
> > > > > +config PHY_FSL_IMX8M_PCIE
> > > > > +       tristate "Freescale i.MX8 PCIE PHY"
> > > >
> > > > Above description is missing the M as in i.MX 8M.
> > > [Richard Zhu] Okay, would be added later.
> > > Thanks.
> > >
> > > >
> > > > > +       depends on OF && HAS_IOMEM
> > > > > +       select GENERIC_PHY
> > > > > +       default ARCH_MXC && ARM64
> > > > > +       help
> > > > > +         Enable this to add support for the PCIE PHY as found on
> > > > > +         i.MX8M family of SOCs.
> > > > > diff --git a/drivers/phy/freescale/Makefile
> > > > > b/drivers/phy/freescale/Makefile index
> > 1d02e3869b45..55d07c742ab0
> > > > > 100644
> > > > > --- a/drivers/phy/freescale/Makefile
> > > > > +++ b/drivers/phy/freescale/Makefile
> > > > > @@ -1,3 +1,4 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> > > > phy-fsl-imx8mq-usb.o
> > > > >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> > > > phy-fsl-imx8-mipi-dphy.o
> > > > > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> > > > phy-fsl-imx8m-pcie.o
> > > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > new file mode 100644
> > > > > index 000000000000..4b4402eaddcc
> > > > > --- /dev/null
> > > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > @@ -0,0 +1,234 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Copyright 2021 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/iopoll.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/phy/phy.h>
> > > > > +#include <linux/platform_device.h> #include <linux/regmap.h>
> > > > > +#include <linux/reset.h> #include
> > > > > +<dt-bindings/phy/phy-imx8-pcie.h>
> > > > > +
> > > > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184 #define
> > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > > > > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > > > > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > > > > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > > > > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > > > #define  ANA_AUX_RX_TX_SEL_TX
> > > > > +BIT(7)
> > > > #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> > > > #define
> > > > > +ANA_AUX_TX_TERM               BIT(2) #define
> > > > > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > > > #define  ANA_AUX_RX_TERM
> > > > > +(BIT(7) | BIT(4))
> > > > #define  ANA_AUX_TX_LVL
> > > > > +GENMASK(3, 0) #define
> > > > IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > > > > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> > > > PCIE_PHY_TRSV_REG5
> > > > > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D
> > #define
> > > > > +PCIE_PHY_TRSV_REG6             0x418 #define
> > > > > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > > > > +
> > > > > +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> > > > #define
> > > > > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > > > > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > > > > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > > > > +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> > > > #define
> > > > > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> > > > IMX8MM_GPR_PCIE_SSC_EN
> > > > > +BIT(16) #define
> > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > > > > +
> > > > > +struct imx8_pcie_phy {
> > > > > +       void __iomem            *base;
> > > > > +       struct clk              *clk;
> > > > > +       struct phy              *phy;
> > > > > +       struct regmap           *iomuxc_gpr;
> > > > > +       struct reset_control    *reset;
> > > > > +       u32                     refclk_pad_mode;
> > > > > +       u32                     tx_deemph_gen1;
> > > > > +       u32                     tx_deemph_gen2;
> > > > > +       bool                    clkreq_unused; };
> > > > > +
> > > > > +static int imx8_pcie_phy_init(struct phy *phy) {
> > > > > +       int ret;
> > > > > +       u32 val, pad_mode;
> > > > > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > > > > +
> > > > > +       reset_control_assert(imx8_phy->reset);
> > > > > +
> > > > > +       pad_mode = imx8_phy->refclk_pad_mode;
> > > > > +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't
> > > > hooked */
> > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > IOMUXC_GPR14,
> > > > >
> > > > +                          IMX8MM_GPR_PCIE_AUX_EN_
> > > > OVERRIDE,
> > > > > +                          imx8_phy->clkreq_unused ?
> > > > > +                          0 :
> > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > IOMUXC_GPR14,
> > > > > +                          IMX8MM_GPR_PCIE_AUX_EN,
> > > > > +                          pad_mode ==
> > > > IMX8_PCIE_REFCLK_PAD_INPUT ?
> > > > > +                          IMX8MM_GPR_PCIE_AUX_EN :
> > > > 0);
> > > >
> > > > V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4
> > stopped
> > > > working for our output use-case as it only enables it for the input
> > > > use-case. If I enable this one always it starts working again.
> > > [Richard Zhu] See my comment above.
> > > Sorry to bring the regression on your board.
> > >
> >
> > Marcel,
> >
> > Your board does not use an external clk, but does it hook up CLKREQ#
> > from the socket to either I2C4_SCL or UART4_RXD and pin muxed as
> > such?
> >
> > For my board that uses an external clk and does not connect CLKREQ# to
> > the IMX8MM I need to disable that bit. As Richard says we have invalid
> > documentation for these bits unfortunately which is not helping.
> >
> > Richard, when we do figure out proper documentation for these bits I
> > suggest you also add a comment block right above their #defines in the
> > phy driver with the correct documentation to avoid future confusion.
> > NXP has had so many mistakes in the various IMX8M RM's and I fear they
> > will never get fixed.
> [Richard Zhu] Hi Tim:
> I took look at the validation codes, and found that the AUX_EN is always
>  set to be 1b'1. Whatever the reference clock mode is selected.
> I'm sending a query email to design team, but I'm not sure I can get response
> in time.
> Can you help to take a double tests at your board when AUX_EN(bit19 of GPR14)
> is set to be 1b'1 firstly?
> Thanks in advanced.
>

Richard and Marcel,

I apologize, my mistake I was referring to GPR14 bit 9 and not bit19
in my testing above.

I tested leaving bit 9 set and this still works on my boards with ext
clk and no CLKREQ# as well as imx8mm-evk with ext clk and CLKREQ#.

So the change on top of your v4 would be:

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index 4b4402eaddcc..003f575b36f0 100644
--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -73,8 +73,7 @@ static int imx8_pcie_phy_init(struct phy *phy)
                           0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
        regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
                           IMX8MM_GPR_PCIE_AUX_EN,
-                          pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
-                          IMX8MM_GPR_PCIE_AUX_EN : 0);
+                          IMX8MM_GPR_PCIE_AUX_EN);
        regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
                           IMX8MM_GPR_PCIE_POWER_OFF, 0);
        regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,

Marcel, does this look right for your board?

Best regards,

Tim
Marcel Ziswiler Nov. 1, 2021, 11:52 p.m. UTC | #6
Hi Tim and Richard

On Mon, 2021-11-01 at 10:13 -0700, Tim Harvey wrote:
> On Mon, Nov 1, 2021 at 1:19 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
> > 
> > > -----Original Message-----
> > > From: Tim Harvey <tharvey@gateworks.com>
> > > Sent: Saturday, October 30, 2021 1:45 AM
> > > To: Richard Zhu <hongxing.zhu@nxp.com>; Marcel Ziswiler
> > > <marcel.ziswiler@toradex.com>
> > > Cc: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > l.stach@pengutronix.de; shawnguo@kernel.org;
> > > galak@kernel.crashing.org; linux-phy@lists.infradead.org;
> > > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> > > standalone phy driver
> > > 
> > > On Fri, Oct 29, 2021 at 1:45 AM Richard Zhu <hongxing.zhu@nxp.com>
> > > wrote:
> > > > 
> > > > 
> > > > > -----Original Message-----
> > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > Sent: Friday, October 29, 2021 4:13 PM
> > > > > To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > > > l.stach@pengutronix.de; shawnguo@kernel.org;
> > > tharvey@gateworks.com;
> > > > > galak@kernel.crashing.org; Richard Zhu <hongxing.zhu@nxp.com>
> > > > > Cc: linux-phy@lists.infradead.org;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > kernel@pengutronix.de; devicetree@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the
> > > > > imx8 pcie standalone phy driver
> > > > > 
> > > > > On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > > > > > Add the standalone i.MX8 PCIe PHY driver.
> > > > > > 
> > > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > 
> > > > > Unfortunately, this version no longer works for our
> > > > > IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments in-
> > > lined
> > > > > below.
> > > > > 
> > > > [Richard Zhu] Sorry to hear about that. Then, it seems that this bit
> > > > should be  set anyway.
> > > > It's hard to understand this bit refer to the RM document. Sigh ☹.
> > > > Would set the AUX_EN bit later.
> > > > 
> > > > > > ---
> > > > > >  drivers/phy/freescale/Kconfig              |   9 +
> > > > > >  drivers/phy/freescale/Makefile             |   1 +
> > > > > >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > > > > > +++++++++++++++++++++
> > > > > >  3 files changed, 244 insertions(+)  create mode 100644
> > > > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > 
> > > > > > diff --git a/drivers/phy/freescale/Kconfig
> > > > > > b/drivers/phy/freescale/Kconfig index 320630ffe3cd..de9ee7020f76
> > > > > > 100644
> > > > > > --- a/drivers/phy/freescale/Kconfig
> > > > > > +++ b/drivers/phy/freescale/Kconfig
> > > > > > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> > > > > >         help
> > > > > >           Enable this to add support for the Mixel DSI PHY as
> > > > > found
> > > > > >           on NXP's i.MX8 family of SOCs.
> > > > > > +
> > > > > > +config PHY_FSL_IMX8M_PCIE
> > > > > > +       tristate "Freescale i.MX8 PCIE PHY"
> > > > > 
> > > > > Above description is missing the M as in i.MX 8M.
> > > > [Richard Zhu] Okay, would be added later.
> > > > Thanks.
> > > > 
> > > > > 
> > > > > > +       depends on OF && HAS_IOMEM
> > > > > > +       select GENERIC_PHY
> > > > > > +       default ARCH_MXC && ARM64
> > > > > > +       help
> > > > > > +         Enable this to add support for the PCIE PHY as found on
> > > > > > +         i.MX8M family of SOCs.
> > > > > > diff --git a/drivers/phy/freescale/Makefile
> > > > > > b/drivers/phy/freescale/Makefile index
> > > 1d02e3869b45..55d07c742ab0
> > > > > > 100644
> > > > > > --- a/drivers/phy/freescale/Makefile
> > > > > > +++ b/drivers/phy/freescale/Makefile
> > > > > > @@ -1,3 +1,4 @@
> > > > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> > > > > phy-fsl-imx8mq-usb.o
> > > > > >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> > > > > phy-fsl-imx8-mipi-dphy.o
> > > > > > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> > > > > phy-fsl-imx8m-pcie.o
> > > > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..4b4402eaddcc
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > @@ -0,0 +1,234 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * Copyright 2021 NXP
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/clk.h>
> > > > > > +#include <linux/io.h>
> > > > > > +#include <linux/iopoll.h>
> > > > > > +#include <linux/delay.h>
> > > > > > +#include <linux/mfd/syscon.h>
> > > > > > +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/phy/phy.h>
> > > > > > +#include <linux/platform_device.h> #include <linux/regmap.h>
> > > > > > +#include <linux/reset.h> #include
> > > > > > +<dt-bindings/phy/phy-imx8-pcie.h>
> > > > > > +
> > > > > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184 #define
> > > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > > > > > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > > > > > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > > > > > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > > > > > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > > > > #define  ANA_AUX_RX_TX_SEL_TX
> > > > > > +BIT(7)
> > > > > #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> > > > > #define
> > > > > > +ANA_AUX_TX_TERM               BIT(2) #define
> > > > > > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > > > > #define  ANA_AUX_RX_TERM
> > > > > > +(BIT(7) | BIT(4))
> > > > > #define  ANA_AUX_TX_LVL
> > > > > > +GENMASK(3, 0) #define
> > > > > IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > > > > > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> > > > > PCIE_PHY_TRSV_REG5
> > > > > > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D
> > > #define
> > > > > > +PCIE_PHY_TRSV_REG6             0x418 #define
> > > > > > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > > > > > +
> > > > > > +#define IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> > > > > #define
> > > > > > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > > > > > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > > > > > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > > > > > +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> > > > > #define
> > > > > > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> > > > > IMX8MM_GPR_PCIE_SSC_EN
> > > > > > +BIT(16) #define
> > > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > > > > > +
> > > > > > +struct imx8_pcie_phy {
> > > > > > +       void __iomem            *base;
> > > > > > +       struct clk              *clk;
> > > > > > +       struct phy              *phy;
> > > > > > +       struct regmap           *iomuxc_gpr;
> > > > > > +       struct reset_control    *reset;
> > > > > > +       u32                     refclk_pad_mode;
> > > > > > +       u32                     tx_deemph_gen1;
> > > > > > +       u32                     tx_deemph_gen2;
> > > > > > +       bool                    clkreq_unused; };
> > > > > > +
> > > > > > +static int imx8_pcie_phy_init(struct phy *phy) {
> > > > > > +       int ret;
> > > > > > +       u32 val, pad_mode;
> > > > > > +       struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
> > > > > > +
> > > > > > +       reset_control_assert(imx8_phy->reset);
> > > > > > +
> > > > > > +       pad_mode = imx8_phy->refclk_pad_mode;
> > > > > > +       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't
> > > > > hooked */
> > > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > > IOMUXC_GPR14,
> > > > > > 
> > > > > +                          IMX8MM_GPR_PCIE_AUX_EN_
> > > > > OVERRIDE,
> > > > > > +                          imx8_phy->clkreq_unused ?
> > > > > > +                          0 :
> > > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > > IOMUXC_GPR14,
> > > > > > +                          IMX8MM_GPR_PCIE_AUX_EN,
> > > > > > +                          pad_mode ==
> > > > > IMX8_PCIE_REFCLK_PAD_INPUT ?
> > > > > > +                          IMX8MM_GPR_PCIE_AUX_EN :
> > > > > 0);
> > > > > 
> > > > > V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out V4
> > > stopped
> > > > > working for our output use-case as it only enables it for the input
> > > > > use-case. If I enable this one always it starts working again.
> > > > [Richard Zhu] See my comment above.
> > > > Sorry to bring the regression on your board.
> > > > 
> > > 
> > > Marcel,
> > > 
> > > Your board does not use an external clk, but does it hook up CLKREQ#
> > > from the socket to either I2C4_SCL or UART4_RXD and pin muxed as
> > > such?
> > > 
> > > For my board that uses an external clk and does not connect CLKREQ# to
> > > the IMX8MM I need to disable that bit. As Richard says we have invalid
> > > documentation for these bits unfortunately which is not helping.
> > > 
> > > Richard, when we do figure out proper documentation for these bits I
> > > suggest you also add a comment block right above their #defines in the
> > > phy driver with the correct documentation to avoid future confusion.
> > > NXP has had so many mistakes in the various IMX8M RM's and I fear they
> > > will never get fixed.
> > [Richard Zhu] Hi Tim:
> > I took look at the validation codes, and found that the AUX_EN is always
> >  set to be 1b'1. Whatever the reference clock mode is selected.
> > I'm sending a query email to design team, but I'm not sure I can get response
> > in time.
> > Can you help to take a double tests at your board when AUX_EN(bit19 of GPR14)
> > is set to be 1b'1 firstly?
> > Thanks in advanced.
> > 
> 
> Richard and Marcel,
> 
> I apologize, my mistake I was referring to GPR14 bit 9 and not bit19
> in my testing above.
> 
> I tested leaving bit 9 set and this still works on my boards with ext
> clk and no CLKREQ# as well as imx8mm-evk with ext clk and CLKREQ#.
> 
> So the change on top of your v4 would be:
> 
> diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> index 4b4402eaddcc..003f575b36f0 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> @@ -73,8 +73,7 @@ static int imx8_pcie_phy_init(struct phy *phy)
>                            0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
>         regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
>                            IMX8MM_GPR_PCIE_AUX_EN,
> -                          pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
> -                          IMX8MM_GPR_PCIE_AUX_EN : 0);
> +                          IMX8MM_GPR_PCIE_AUX_EN);
>         regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
>                            IMX8MM_GPR_PCIE_POWER_OFF, 0);
>         regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
> 
> Marcel, does this look right for your board?

Yes, that is exactly how I run it now. Thanks!

> Best regards,
> 
> Tim

Cheers

Marcel
Richard Zhu Nov. 2, 2021, 2:41 a.m. UTC | #7
> -----Original Message-----
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Sent: Tuesday, November 2, 2021 7:53 AM
> To: tharvey@gateworks.com; Richard Zhu <hongxing.zhu@nxp.com>
> Cc: kishon@ti.com; vkoul@kernel.org; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> robh@kernel.org; l.stach@pengutronix.de; shawnguo@kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-phy@lists.infradead.org;
> galak@kernel.crashing.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> standalone phy driver
> 
> Hi Tim and Richard
> 
> On Mon, 2021-11-01 at 10:13 -0700, Tim Harvey wrote:
> > On Mon, Nov 1, 2021 at 1:19 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > Sent: Saturday, October 30, 2021 1:45 AM
> > > > To: Richard Zhu <hongxing.zhu@nxp.com>; Marcel Ziswiler
> > > > <marcel.ziswiler@toradex.com>
> > > > Cc: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > > l.stach@pengutronix.de; shawnguo@kernel.org;
> > > > galak@kernel.crashing.org; linux-phy@lists.infradead.org;
> > > > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize the
> > > > imx8 pcie standalone phy driver
> > > >
> > > > On Fri, Oct 29, 2021 at 1:45 AM Richard Zhu
> <hongxing.zhu@nxp.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > > Sent: Friday, October 29, 2021 4:13 PM
> > > > > > To: kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > > > > > l.stach@pengutronix.de; shawnguo@kernel.org;
> > > > tharvey@gateworks.com;
> > > > > > galak@kernel.crashing.org; Richard Zhu
> <hongxing.zhu@nxp.com>
> > > > > > Cc: linux-phy@lists.infradead.org;
> > > > > > linux-arm-kernel@lists.infradead.org;
> > > > > > kernel@pengutronix.de; devicetree@vger.kernel.org;
> > > > > > linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> > > > > > Subject: Re: [PATCH v4 5/8] phy: freescale: pcie: Initialize
> > > > > > the
> > > > > > imx8 pcie standalone phy driver
> > > > > >
> > > > > > On Thu, 2021-10-28 at 15:27 +0800, Richard Zhu wrote:
> > > > > > > Add the standalone i.MX8 PCIe PHY driver.
> > > > > > >
> > > > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > >
> > > > > > Unfortunately, this version no longer works for our
> > > > > > IMX8_PCIE_REFCLK_PAD_OUTPUT use-case. Further comments
> in-
> > > > lined
> > > > > > below.
> > > > > >
> > > > > [Richard Zhu] Sorry to hear about that. Then, it seems that this
> > > > > bit should be  set anyway.
> > > > > It's hard to understand this bit refer to the RM document. Sigh
> ☹.
> > > > > Would set the AUX_EN bit later.
> > > > >
> > > > > > > ---
> > > > > > >  drivers/phy/freescale/Kconfig              |   9 +
> > > > > > >  drivers/phy/freescale/Makefile             |   1 +
> > > > > > >  drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 234
> > > > > > > +++++++++++++++++++++
> > > > > > >  3 files changed, 244 insertions(+)  create mode 100644
> > > > > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > >
> > > > > > > diff --git a/drivers/phy/freescale/Kconfig
> > > > > > > b/drivers/phy/freescale/Kconfig index
> > > > > > > 320630ffe3cd..de9ee7020f76
> > > > > > > 100644
> > > > > > > --- a/drivers/phy/freescale/Kconfig
> > > > > > > +++ b/drivers/phy/freescale/Kconfig
> > > > > > > @@ -14,3 +14,12 @@ config PHY_MIXEL_MIPI_DPHY
> > > > > > >         help
> > > > > > >           Enable this to add support for the Mixel DSI
> PHY
> > > > > > > as
> > > > > > found
> > > > > > >           on NXP's i.MX8 family of SOCs.
> > > > > > > +
> > > > > > > +config PHY_FSL_IMX8M_PCIE
> > > > > > > +       tristate "Freescale i.MX8 PCIE PHY"
> > > > > >
> > > > > > Above description is missing the M as in i.MX 8M.
> > > > > [Richard Zhu] Okay, would be added later.
> > > > > Thanks.
> > > > >
> > > > > >
> > > > > > > +       depends on OF && HAS_IOMEM
> > > > > > > +       select GENERIC_PHY
> > > > > > > +       default ARCH_MXC && ARM64
> > > > > > > +       help
> > > > > > > +         Enable this to add support for the PCIE PHY as
> > > > > > > +found on
> > > > > > > +         i.MX8M family of SOCs.
> > > > > > > diff --git a/drivers/phy/freescale/Makefile
> > > > > > > b/drivers/phy/freescale/Makefile index
> > > > 1d02e3869b45..55d07c742ab0
> > > > > > > 100644
> > > > > > > --- a/drivers/phy/freescale/Makefile
> > > > > > > +++ b/drivers/phy/freescale/Makefile
> > > > > > > @@ -1,3 +1,4 @@
> > > > > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > > > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       +=
> > > > > > phy-fsl-imx8mq-usb.o
> > > > > > >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      +=
> > > > > > phy-fsl-imx8-mipi-dphy.o
> > > > > > > +obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)       +=
> > > > > > phy-fsl-imx8m-pcie.o
> > > > > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..4b4402eaddcc
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > > > > @@ -0,0 +1,234 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > +/*
> > > > > > > + * Copyright 2021 NXP
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/clk.h>
> > > > > > > +#include <linux/io.h>
> > > > > > > +#include <linux/iopoll.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/mfd/syscon.h> #include
> > > > > > > +<linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/phy/phy.h>
> > > > > > > +#include <linux/platform_device.h> #include
> > > > > > > +<linux/regmap.h> #include <linux/reset.h> #include
> > > > > > > +<dt-bindings/phy/phy-imx8-pcie.h>
> > > > > > > +
> > > > > > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184
> #define
> > > > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0) #define
> > > > > > > +IMX8MM_PCIE_PHY_CMN_REG062     0x188 #define
> > > > > > > +ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3) #define
> > > > > > > +IMX8MM_PCIE_PHY_CMN_REG063     0x18C #define
> > > > > > > +AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6) #define
> > > > > > > +IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > > > > > #define  ANA_AUX_RX_TX_SEL_TX
> > > > > > > +BIT(7)
> > > > > >
> #define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> #define
> > > > > > > +ANA_AUX_TX_TERM               BIT(2) #define
> > > > > > > +IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > > > > > #define  ANA_AUX_RX_TERM
> > > > > > > +(BIT(7) | BIT(4))
> > > > > > #define  ANA_AUX_TX_LVL
> > > > > > > +GENMASK(3, 0) #define
> > > > > > IMX8MM_PCIE_PHY_CMN_REG75      0x1D4 #define
> > > > > > > +PCIE_PHY_CMN_REG75_PLL_DONE   0x3 #define
> > > > > > PCIE_PHY_TRSV_REG5
> > > > > > > +0x414 #define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP 0x2D
> > > > #define
> > > > > > > +PCIE_PHY_TRSV_REG6             0x418 #define
> > > > > > > +PCIE_PHY_TRSV_REG6_GEN2_DEEMP 0xF
> > > > > > > +
> > > > > > > +#define
> IMX8MM_GPR_PCIE_REF_CLK_SEL    GENMASK(25, 24)
> > > > > > #define
> > > > > > > +IMX8MM_GPR_PCIE_REF_CLK_PLL
> > > > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3) #define
> > > > > > > +IMX8MM_GPR_PCIE_REF_CLK_EXT
> > > > > > > +FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2) #define
> > > > > > > +IMX8MM_GPR_PCIE_AUX_EN         BIT(19) #define
> > > > > > >
> +IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
> > > > > > #define
> > > > > > > +IMX8MM_GPR_PCIE_POWER_OFF      BIT(17) #define
> > > > > > IMX8MM_GPR_PCIE_SSC_EN
> > > > > > > +BIT(16) #define
> > > > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)
> > > > > > > +
> > > > > > > +struct imx8_pcie_phy {
> > > > > > > +       void __iomem            *base;
> > > > > > > +       struct clk              *clk;
> > > > > > > +       struct phy              *phy;
> > > > > > > +       struct regmap           *iomuxc_gpr;
> > > > > > > +       struct reset_control    *reset;
> > > > > > >
> +       u32                     refclk_pad_mode;
> > > > > > >
> +       u32                     tx_deemph_gen1;
> > > > > > >
> +       u32                     tx_deemph_gen2;
> > > > > > >
> +       bool                    clkreq_unused; };
> > > > > > > +
> > > > > > > +static int imx8_pcie_phy_init(struct phy *phy) {
> > > > > > > +       int ret;
> > > > > > > +       u32 val, pad_mode;
> > > > > > > +       struct imx8_pcie_phy *imx8_phy =
> > > > > > > +phy_get_drvdata(phy);
> > > > > > > +
> > > > > > > +       reset_control_assert(imx8_phy->reset);
> > > > > > > +
> > > > > > > +       pad_mode = imx8_phy->refclk_pad_mode;
> > > > > > > +       /* Set AUX_EN_OVERRIDE 1'b0, when the
> CLKREQ# isn't
> > > > > > hooked */
> > > > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > > > IOMUXC_GPR14,
> > > > > > >
> > > > > >
> +                          IMX8MM_GPR_PCIE_AUX_EN_
> > > > > > OVERRIDE,
> > > > > > >
> +                          imx8_phy->clkreq_unused ?
> > > > > > > +                          0 :
> > > > > > IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> > > > > > > +       regmap_update_bits(imx8_phy->iomuxc_gpr,
> > > > > > IOMUXC_GPR14,
> > > > > > >
> +                          IMX8MM_GPR_PCIE_AUX_EN,
> > > > > > > +                          pad_mode ==
> > > > > > IMX8_PCIE_REFCLK_PAD_INPUT ?
> > > > > > >
> +                          IMX8MM_GPR_PCIE_AUX_EN :
> > > > > > 0);
> > > > > >
> > > > > > V3 had IMX8MM_GPR_PCIE_AUX_EN always enabled. Turns out
> V4
> > > > stopped
> > > > > > working for our output use-case as it only enables it for the
> > > > > > input use-case. If I enable this one always it starts working
> again.
> > > > > [Richard Zhu] See my comment above.
> > > > > Sorry to bring the regression on your board.
> > > > >
> > > >
> > > > Marcel,
> > > >
> > > > Your board does not use an external clk, but does it hook up
> > > > CLKREQ# from the socket to either I2C4_SCL or UART4_RXD and pin
> > > > muxed as such?
> > > >
> > > > For my board that uses an external clk and does not connect
> > > > CLKREQ# to the IMX8MM I need to disable that bit. As Richard says
> > > > we have invalid documentation for these bits unfortunately which is
> not helping.
> > > >
> > > > Richard, when we do figure out proper documentation for these bits
> > > > I suggest you also add a comment block right above their #defines
> > > > in the phy driver with the correct documentation to avoid future
> confusion.
> > > > NXP has had so many mistakes in the various IMX8M RM's and I
> fear
> > > > they will never get fixed.
> > > [Richard Zhu] Hi Tim:
> > > I took look at the validation codes, and found that the AUX_EN is
> > > always
> > >  set to be 1b'1. Whatever the reference clock mode is selected.
> > > I'm sending a query email to design team, but I'm not sure I can get
> > > response in time.
> > > Can you help to take a double tests at your board when AUX_EN(bit19
> > > of GPR14) is set to be 1b'1 firstly?
> > > Thanks in advanced.
> > >
> >
> > Richard and Marcel,
> >
> > I apologize, my mistake I was referring to GPR14 bit 9 and not bit19
> > in my testing above.
> >
> > I tested leaving bit 9 set and this still works on my boards with ext
> > clk and no CLKREQ# as well as imx8mm-evk with ext clk and CLKREQ#.
> >
> > So the change on top of your v4 would be:
> >
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > index 4b4402eaddcc..003f575b36f0 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > @@ -73,8 +73,7 @@ static int imx8_pcie_phy_init(struct phy *phy)
> >                            0 :
> IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
> >         regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >                            IMX8MM_GPR_PCIE_AUX_E
> N,
> > -                          pad_mode ==
> IMX8_PCIE_REFCLK_PAD_INPUT ?
> > -                          IMX8MM_GPR_PCIE_AUX_EN :
> 0);
> >
> +                          IMX8MM_GPR_PCIE_AUX_EN);
> >         regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >                            IMX8MM_GPR_PCIE_POWER
> _OFF, 0);
> >         regmap_update_bits(imx8_phy->iomuxc_gpr,
> IOMUXC_GPR14,
> >
> > Marcel, does this look right for your board?
> 
> Yes, that is exactly how I run it now. Thanks!
[Richard Zhu] That's great.
Thanks a lot. I will send v5 set a moment later.
BR
Richard

> 
> > Best regards,
> >
> > Tim
> 
> Cheers
> 
> Marcel
diff mbox series

Patch

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 320630ffe3cd..de9ee7020f76 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -14,3 +14,12 @@  config PHY_MIXEL_MIPI_DPHY
 	help
 	  Enable this to add support for the Mixel DSI PHY as found
 	  on NXP's i.MX8 family of SOCs.
+
+config PHY_FSL_IMX8M_PCIE
+	tristate "Freescale i.MX8 PCIE PHY"
+	depends on OF && HAS_IOMEM
+	select GENERIC_PHY
+	default ARCH_MXC && ARM64
+	help
+	  Enable this to add support for the PCIE PHY as found on
+	  i.MX8M family of SOCs.
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index 1d02e3869b45..55d07c742ab0 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -1,3 +1,4 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
 obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
+obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)	+= phy-fsl-imx8m-pcie.o
diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
new file mode 100644
index 000000000000..4b4402eaddcc
--- /dev/null
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -0,0 +1,234 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <dt-bindings/phy/phy-imx8-pcie.h>
+
+#define IMX8MM_PCIE_PHY_CMN_REG061	0x184
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN	BIT(0)
+#define IMX8MM_PCIE_PHY_CMN_REG062	0x188
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL	BIT(3)
+#define IMX8MM_PCIE_PHY_CMN_REG063	0x18C
+#define  AUX_PLL_REFCLK_SEL_SYS_PLL	GENMASK(7, 6)
+#define IMX8MM_PCIE_PHY_CMN_REG064	0x190
+#define  ANA_AUX_RX_TX_SEL_TX		BIT(7)
+#define  ANA_AUX_RX_TERM_GND_EN		BIT(3)
+#define  ANA_AUX_TX_TERM		BIT(2)
+#define IMX8MM_PCIE_PHY_CMN_REG065	0x194
+#define  ANA_AUX_RX_TERM		(BIT(7) | BIT(4))
+#define  ANA_AUX_TX_LVL			GENMASK(3, 0)
+#define IMX8MM_PCIE_PHY_CMN_REG75	0x1D4
+#define  PCIE_PHY_CMN_REG75_PLL_DONE	0x3
+#define PCIE_PHY_TRSV_REG5		0x414
+#define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP	0x2D
+#define PCIE_PHY_TRSV_REG6		0x418
+#define  PCIE_PHY_TRSV_REG6_GEN2_DEEMP	0xF
+
+#define IMX8MM_GPR_PCIE_REF_CLK_SEL	GENMASK(25, 24)
+#define IMX8MM_GPR_PCIE_REF_CLK_PLL	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3)
+#define IMX8MM_GPR_PCIE_REF_CLK_EXT	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2)
+#define IMX8MM_GPR_PCIE_AUX_EN		BIT(19)
+#define IMX8MM_GPR_PCIE_CMN_RST		BIT(18)
+#define IMX8MM_GPR_PCIE_POWER_OFF	BIT(17)
+#define IMX8MM_GPR_PCIE_SSC_EN		BIT(16)
+#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE	BIT(9)
+
+struct imx8_pcie_phy {
+	void __iomem		*base;
+	struct clk		*clk;
+	struct phy		*phy;
+	struct regmap		*iomuxc_gpr;
+	struct reset_control	*reset;
+	u32			refclk_pad_mode;
+	u32			tx_deemph_gen1;
+	u32			tx_deemph_gen2;
+	bool			clkreq_unused;
+};
+
+static int imx8_pcie_phy_init(struct phy *phy)
+{
+	int ret;
+	u32 val, pad_mode;
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	reset_control_assert(imx8_phy->reset);
+
+	pad_mode = imx8_phy->refclk_pad_mode;
+	/* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't hooked */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,
+			   imx8_phy->clkreq_unused ?
+			   0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_AUX_EN,
+			   pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
+			   IMX8MM_GPR_PCIE_AUX_EN : 0);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_POWER_OFF, 0);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_SSC_EN, 0);
+
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_REF_CLK_SEL,
+			   pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
+			   IMX8MM_GPR_PCIE_REF_CLK_EXT :
+			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
+	usleep_range(100, 200);
+
+	/* Do the PHY common block reset */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_CMN_RST,
+			   IMX8MM_GPR_PCIE_CMN_RST);
+	usleep_range(200, 500);
+
+
+	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
+		/* Configure the pad as input */
+		val = readl(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+		writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+	} else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
+		/* Configure the PHY to output the refclock via pad */
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
+		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
+		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
+		writel(val | ANA_AUX_RX_TERM_GND_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);
+		writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG065);
+	}
+
+	/* Tune PHY de-emphasis setting to pass PCIe compliance. */
+	writel(imx8_phy->tx_deemph_gen1, imx8_phy->base + PCIE_PHY_TRSV_REG5);
+	writel(imx8_phy->tx_deemph_gen2, imx8_phy->base + PCIE_PHY_TRSV_REG6);
+
+	reset_control_deassert(imx8_phy->reset);
+
+	/* Polling to check the phy is ready or not. */
+	ret = readl_poll_timeout(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG75,
+				 val, val == PCIE_PHY_CMN_REG75_PLL_DONE,
+				 10, 20000);
+	return ret;
+}
+
+static int imx8_pcie_phy_power_on(struct phy *phy)
+{
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	return clk_prepare_enable(imx8_phy->clk);
+}
+
+static int imx8_pcie_phy_power_off(struct phy *phy)
+{
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(imx8_phy->clk);
+
+	return 0;
+}
+
+static const struct phy_ops imx8_pcie_phy_ops = {
+	.init		= imx8_pcie_phy_init,
+	.power_on	= imx8_pcie_phy_power_on,
+	.power_off	= imx8_pcie_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int imx8_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx8_pcie_phy *imx8_phy;
+	struct resource *res;
+
+	imx8_phy = devm_kzalloc(dev, sizeof(*imx8_phy), GFP_KERNEL);
+	if (!imx8_phy)
+		return -ENOMEM;
+
+	/* get PHY refclk pad mode */
+	of_property_read_u32(np, "fsl,refclk-pad-mode",
+			     &imx8_phy->refclk_pad_mode);
+
+	if (of_property_read_u32(np, "fsl,tx-deemph-gen1",
+				 &imx8_phy->tx_deemph_gen1))
+		imx8_phy->tx_deemph_gen1 = 0;
+
+	if (of_property_read_u32(np, "fsl,tx-deemph-gen2",
+				 &imx8_phy->tx_deemph_gen2))
+		imx8_phy->tx_deemph_gen2 = 0;
+
+	if (of_property_read_bool(np, "fsl,clkreq-unsupported"))
+		imx8_phy->clkreq_unused = true;
+	else
+		imx8_phy->clkreq_unused = false;
+
+	imx8_phy->clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(imx8_phy->clk)) {
+		dev_err(dev, "failed to get imx pcie phy clock\n");
+		return PTR_ERR(imx8_phy->clk);
+	}
+
+	/* Grab GPR config register range */
+	imx8_phy->iomuxc_gpr =
+		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (IS_ERR(imx8_phy->iomuxc_gpr)) {
+		dev_err(dev, "unable to find iomuxc registers\n");
+		return PTR_ERR(imx8_phy->iomuxc_gpr);
+	}
+
+	imx8_phy->reset = devm_reset_control_get_exclusive(dev, "pciephy");
+	if (IS_ERR(imx8_phy->reset)) {
+		dev_err(dev, "Failed to get PCIEPHY reset control\n");
+		return PTR_ERR(imx8_phy->reset);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	imx8_phy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(imx8_phy->base))
+		return PTR_ERR(imx8_phy->base);
+
+	imx8_phy->phy = devm_phy_create(dev, NULL, &imx8_pcie_phy_ops);
+	if (IS_ERR(imx8_phy->phy))
+		return PTR_ERR(imx8_phy->phy);
+
+	phy_set_drvdata(imx8_phy->phy, imx8_phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id imx8_pcie_phy_of_match[] = {
+	{.compatible = "fsl,imx8mm-pcie-phy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
+
+static struct platform_driver imx8_pcie_phy_driver = {
+	.probe	= imx8_pcie_phy_probe,
+	.driver = {
+		.name	= "imx8-pcie-phy",
+		.of_match_table	= imx8_pcie_phy_of_match,
+	}
+};
+module_platform_driver(imx8_pcie_phy_driver);
+
+MODULE_DESCRIPTION("FSL IMX8 PCIE PHY driver");
+MODULE_LICENSE("GPL");