From: Hongxing Zhu <hongxing.zhu@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
"robh@kernel.org" <robh@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"vkoul@kernel.org" <vkoul@kernel.org>,
"alexander.stein@ew.tq-group.com"
<alexander.stein@ew.tq-group.com>,
"marex@denx.de" <marex@denx.de>,
"richard.leitner@linux.dev" <richard.leitner@linux.dev>
Cc: "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY support
Date: Thu, 1 Sep 2022 01:28:11 +0000 [thread overview]
Message-ID: <AS8PR04MB86760623F932DE8EF23AA5718C7B9@AS8PR04MB8676.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <5e357e6b4bb4957a74913d561f0da491e8121ed6.camel@pengutronix.de>
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年8月31日 16:34
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com;
> marex@denx.de; richard.leitner@linux.dev
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY
> support
>
> Am Mittwoch, dem 31.08.2022 um 06:16 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年8月30日 21:07
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> > > bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> > > shawnguo@kernel.org; vkoul@kernel.org;
> > > alexander.stein@ew.tq-group.com; marex@denx.de;
> > > richard.leitner@linux.dev
> > > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP
> > > PCIe PHY support
> > >
> > > Am Dienstag, dem 30.08.2022 um 15:46 +0800 schrieb Richard Zhu:
> > > > Add i.MX8MP PCIe PHY support.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > Tested-by: Marek Vasut <marex@denx.de>
> > > > Tested-by: Richard Leitner <richard.leitner@skidata.com>
> > > > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 137
> > > > +++++++++++++--------
> > > > 1 file changed, 89 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > index ad7d2edfc414..c76e3a1a5f51 100644
> > > > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > @@ -11,6 +11,9 @@
> > > > #include <linux/mfd/syscon.h>
> > > > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > #include <linux/module.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/phy/phy.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/regmap.h>
> > > > @@ -31,12 +34,10 @@
> > > > #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 IMX8MM_PCIE_PHY_CMN_REG075 0x1D4
> > > > +#define ANA_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)
> > > > @@ -47,16 +48,23 @@
> > > > #define IMX8MM_GPR_PCIE_SSC_EN BIT(16)
> > > > #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9)
> > > >
> > > > +enum imx8_pcie_phy_type {
> > > > + IMX8MM,
> > > > + IMX8MP,
> > > > +};
> > > > +
> > > > struct imx8_pcie_phy {
> > > > void __iomem *base;
> > > > struct clk *clk;
> > > > struct phy *phy;
> > > > struct regmap *iomuxc_gpr;
> > > > struct reset_control *reset;
> > > > + struct reset_control *perst;
> > > > u32 refclk_pad_mode;
> > > > u32 tx_deemph_gen1;
> > > > u32 tx_deemph_gen2;
> > > > bool clkreq_unused;
> > > > + enum imx8_pcie_phy_type variant;
> > > > };
> > > >
> > > > static int imx8_pcie_phy_init(struct phy *phy) @@ -68,31 +76,20
> > > > @@ static int imx8_pcie_phy_init(struct phy *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,
> > > > - 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,
> > > > - 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);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + /* Tune PHY de-emphasis setting to pass PCIe
> > > > compliance. */
> > > > + if (imx8_phy->tx_deemph_gen1)
> > > > + writel(imx8_phy->tx_deemph_gen1,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG5);
> > > > + if (imx8_phy->tx_deemph_gen2)
> > > > + writel(imx8_phy->tx_deemph_gen2,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG6);
> > > > + break;
> > > > + case IMX8MP:
> > > > + reset_control_assert(imx8_phy->perst);
> > > > + break;
> > > > + }
> > > >
> > > > if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ||
> > > > pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) { @@ -
> > > > 120,20
> > > +117,44 @@
> > > > static int imx8_pcie_phy_init(struct phy *phy)
> > > > imx8_phy->base +
> > > > IMX8MM_PCIE_PHY_CMN_REG065);
> > > > }
> > > >
> > > > - /* Tune PHY de-emphasis setting to pass PCIe compliance.
> > > > */
> > > > - if (imx8_phy->tx_deemph_gen1)
> > > > - writel(imx8_phy->tx_deemph_gen1,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG5);
> > > > - if (imx8_phy->tx_deemph_gen2)
> > > > - writel(imx8_phy->tx_deemph_gen2,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG6);
> > > > + /* 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,
> > > > + 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,
> > > > + 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);
> > > >
> > > > - reset_control_deassert(imx8_phy->reset);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MP:
> > > > + reset_control_deassert(imx8_phy->perst);
> > > > + fallthrough;
> > > > + case IMX8MM:
> > > > + reset_control_deassert(imx8_phy->reset);
> > > > + usleep_range(200, 500);
> > > > + break;
> > > > + }
> > > >
> > > > /* 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);
> > > > + ret = readl_poll_timeout(imx8_phy->base +
> > > IMX8MM_PCIE_PHY_CMN_REG075,
> > > > + val, val == ANA_PLL_DONE, 10,
> > > > 20000);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -160,6 +181,13 @@ static const struct phy_ops imx8_pcie_phy_ops
> > > > = {
> > > > .owner = THIS_MODULE,
> > > > };
> > > >
> > > > +static const struct of_device_id imx8_pcie_phy_of_match[] = {
> > > > + {.compatible = "fsl,imx8mm-pcie-phy", .data = (void
> > > > *)IMX8MM},
> > > > + {.compatible = "fsl,imx8mp-pcie-phy", .data = (void
> > > > *)IMX8MP},
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
> > > > +
> > > > static int imx8_pcie_phy_probe(struct platform_device *pdev) {
> > > > struct phy_provider *phy_provider; @@ -172,6 +200,9 @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > if (!imx8_phy)
> > > > return -ENOMEM;
> > > >
> > > > + imx8_phy->variant =
> > > > + (enum
> > > > imx8_pcie_phy_type)of_device_get_match_data(dev);
> > > > +
> > > > /* get PHY refclk pad mode */
> > > > of_property_read_u32(np, "fsl,refclk-pad-mode",
> > > > &imx8_phy->refclk_pad_mode); @@ -196,8 +227,16
> @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > }
> > > >
> > > > /* Grab GPR config register range */
> > > > - imx8_phy->iomuxc_gpr =
> > > > - syscon_regmap_lookup_by_compatible("fsl,imx6q-
> > > > iomuxc-gpr");
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mm-iomuxc-gpr");
> > > > + break;
> > > > + case IMX8MP:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mp-iomuxc-gpr");
> > > > + break;
> > > > + }
> > >
> > > Oh, I had a real phandle in DT in mind for this, but I see how this
> > > would be hard to introduce in a backward compatible manner for the
> > > 8MM.
> > > At least this way it is fully contained in the driver and doesn't
> > > leak into DT compatibles.
> > >
> > > Maybe we could make this a little nicer by just having an const
> > > array of iomux syscon compatibles indexed by imx8_phy->variant, to
> > > avoid the switch and the resulting code (almost-)duplication.
> > >
> > Hi Lucas:
> > Thanks for your comments.
> > Do you mean a drvdata struct indexed by variant, and contains the
> > const array of iomux syscon compatible like below?
> >
> > +static const struct imx8_pcie_phy_drvdata drvdata[] = {
> > + [IMX8MM] = {
> > + .variant = IMX8MM,
> > + .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + },
> > +
> > + [IMX8MP] = {
> > + .variant = IMX8MP,
> > + .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + },
> > +};
> > +
> >
> > Then, we can get the drvdata, and find the according gpr syscon regmap
> > below.
> >
> > + imx8_phy->drvdata = of_device_get_match_data(dev);
> > ...
> > + imx8_phy->iomuxc_gpr =
> > + syscon_regmap_lookup_by_compatible(imx8_phy-
> > >drvdata->gpr);
> >
> I had a simple array of GPR compatible strings in mind, so we could lookup the
> compatible from the variant. But I think putting it directly in drvdata, like done
> in your suggestion above, looks even better.
>
> > If yes, I would do the changes, and issue the v6 later.
>
> Yep, please change as suggested above. Same comment applies to the next
> patch "PCI: imx6: Add iMX8MP PCIe support".
>
> Also in both patches the dot between i and MX in i.MX8MP is missing in the
> patch description subject line.
Okay, thanks.
Best Regards
Richard Zhu
>
> Regards,
> Lucas
WARNING: multiple messages have this Message-ID (diff)
From: Hongxing Zhu <hongxing.zhu@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
"robh@kernel.org" <robh@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"vkoul@kernel.org" <vkoul@kernel.org>,
"alexander.stein@ew.tq-group.com"
<alexander.stein@ew.tq-group.com>,
"marex@denx.de" <marex@denx.de>,
"richard.leitner@linux.dev" <richard.leitner@linux.dev>
Cc: "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY support
Date: Thu, 1 Sep 2022 01:28:11 +0000 [thread overview]
Message-ID: <AS8PR04MB86760623F932DE8EF23AA5718C7B9@AS8PR04MB8676.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <5e357e6b4bb4957a74913d561f0da491e8121ed6.camel@pengutronix.de>
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年8月31日 16:34
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com;
> marex@denx.de; richard.leitner@linux.dev
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY
> support
>
> Am Mittwoch, dem 31.08.2022 um 06:16 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年8月30日 21:07
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> > > bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> > > shawnguo@kernel.org; vkoul@kernel.org;
> > > alexander.stein@ew.tq-group.com; marex@denx.de;
> > > richard.leitner@linux.dev
> > > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP
> > > PCIe PHY support
> > >
> > > Am Dienstag, dem 30.08.2022 um 15:46 +0800 schrieb Richard Zhu:
> > > > Add i.MX8MP PCIe PHY support.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > Tested-by: Marek Vasut <marex@denx.de>
> > > > Tested-by: Richard Leitner <richard.leitner@skidata.com>
> > > > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 137
> > > > +++++++++++++--------
> > > > 1 file changed, 89 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > index ad7d2edfc414..c76e3a1a5f51 100644
> > > > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > @@ -11,6 +11,9 @@
> > > > #include <linux/mfd/syscon.h>
> > > > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > #include <linux/module.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/phy/phy.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/regmap.h>
> > > > @@ -31,12 +34,10 @@
> > > > #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 IMX8MM_PCIE_PHY_CMN_REG075 0x1D4
> > > > +#define ANA_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)
> > > > @@ -47,16 +48,23 @@
> > > > #define IMX8MM_GPR_PCIE_SSC_EN BIT(16)
> > > > #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9)
> > > >
> > > > +enum imx8_pcie_phy_type {
> > > > + IMX8MM,
> > > > + IMX8MP,
> > > > +};
> > > > +
> > > > struct imx8_pcie_phy {
> > > > void __iomem *base;
> > > > struct clk *clk;
> > > > struct phy *phy;
> > > > struct regmap *iomuxc_gpr;
> > > > struct reset_control *reset;
> > > > + struct reset_control *perst;
> > > > u32 refclk_pad_mode;
> > > > u32 tx_deemph_gen1;
> > > > u32 tx_deemph_gen2;
> > > > bool clkreq_unused;
> > > > + enum imx8_pcie_phy_type variant;
> > > > };
> > > >
> > > > static int imx8_pcie_phy_init(struct phy *phy) @@ -68,31 +76,20
> > > > @@ static int imx8_pcie_phy_init(struct phy *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,
> > > > - 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,
> > > > - 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);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + /* Tune PHY de-emphasis setting to pass PCIe
> > > > compliance. */
> > > > + if (imx8_phy->tx_deemph_gen1)
> > > > + writel(imx8_phy->tx_deemph_gen1,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG5);
> > > > + if (imx8_phy->tx_deemph_gen2)
> > > > + writel(imx8_phy->tx_deemph_gen2,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG6);
> > > > + break;
> > > > + case IMX8MP:
> > > > + reset_control_assert(imx8_phy->perst);
> > > > + break;
> > > > + }
> > > >
> > > > if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ||
> > > > pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) { @@ -
> > > > 120,20
> > > +117,44 @@
> > > > static int imx8_pcie_phy_init(struct phy *phy)
> > > > imx8_phy->base +
> > > > IMX8MM_PCIE_PHY_CMN_REG065);
> > > > }
> > > >
> > > > - /* Tune PHY de-emphasis setting to pass PCIe compliance.
> > > > */
> > > > - if (imx8_phy->tx_deemph_gen1)
> > > > - writel(imx8_phy->tx_deemph_gen1,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG5);
> > > > - if (imx8_phy->tx_deemph_gen2)
> > > > - writel(imx8_phy->tx_deemph_gen2,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG6);
> > > > + /* 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,
> > > > + 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,
> > > > + 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);
> > > >
> > > > - reset_control_deassert(imx8_phy->reset);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MP:
> > > > + reset_control_deassert(imx8_phy->perst);
> > > > + fallthrough;
> > > > + case IMX8MM:
> > > > + reset_control_deassert(imx8_phy->reset);
> > > > + usleep_range(200, 500);
> > > > + break;
> > > > + }
> > > >
> > > > /* 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);
> > > > + ret = readl_poll_timeout(imx8_phy->base +
> > > IMX8MM_PCIE_PHY_CMN_REG075,
> > > > + val, val == ANA_PLL_DONE, 10,
> > > > 20000);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -160,6 +181,13 @@ static const struct phy_ops imx8_pcie_phy_ops
> > > > = {
> > > > .owner = THIS_MODULE,
> > > > };
> > > >
> > > > +static const struct of_device_id imx8_pcie_phy_of_match[] = {
> > > > + {.compatible = "fsl,imx8mm-pcie-phy", .data = (void
> > > > *)IMX8MM},
> > > > + {.compatible = "fsl,imx8mp-pcie-phy", .data = (void
> > > > *)IMX8MP},
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
> > > > +
> > > > static int imx8_pcie_phy_probe(struct platform_device *pdev) {
> > > > struct phy_provider *phy_provider; @@ -172,6 +200,9 @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > if (!imx8_phy)
> > > > return -ENOMEM;
> > > >
> > > > + imx8_phy->variant =
> > > > + (enum
> > > > imx8_pcie_phy_type)of_device_get_match_data(dev);
> > > > +
> > > > /* get PHY refclk pad mode */
> > > > of_property_read_u32(np, "fsl,refclk-pad-mode",
> > > > &imx8_phy->refclk_pad_mode); @@ -196,8 +227,16
> @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > }
> > > >
> > > > /* Grab GPR config register range */
> > > > - imx8_phy->iomuxc_gpr =
> > > > - syscon_regmap_lookup_by_compatible("fsl,imx6q-
> > > > iomuxc-gpr");
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mm-iomuxc-gpr");
> > > > + break;
> > > > + case IMX8MP:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mp-iomuxc-gpr");
> > > > + break;
> > > > + }
> > >
> > > Oh, I had a real phandle in DT in mind for this, but I see how this
> > > would be hard to introduce in a backward compatible manner for the
> > > 8MM.
> > > At least this way it is fully contained in the driver and doesn't
> > > leak into DT compatibles.
> > >
> > > Maybe we could make this a little nicer by just having an const
> > > array of iomux syscon compatibles indexed by imx8_phy->variant, to
> > > avoid the switch and the resulting code (almost-)duplication.
> > >
> > Hi Lucas:
> > Thanks for your comments.
> > Do you mean a drvdata struct indexed by variant, and contains the
> > const array of iomux syscon compatible like below?
> >
> > +static const struct imx8_pcie_phy_drvdata drvdata[] = {
> > + [IMX8MM] = {
> > + .variant = IMX8MM,
> > + .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + },
> > +
> > + [IMX8MP] = {
> > + .variant = IMX8MP,
> > + .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + },
> > +};
> > +
> >
> > Then, we can get the drvdata, and find the according gpr syscon regmap
> > below.
> >
> > + imx8_phy->drvdata = of_device_get_match_data(dev);
> > ...
> > + imx8_phy->iomuxc_gpr =
> > + syscon_regmap_lookup_by_compatible(imx8_phy-
> > >drvdata->gpr);
> >
> I had a simple array of GPR compatible strings in mind, so we could lookup the
> compatible from the variant. But I think putting it directly in drvdata, like done
> in your suggestion above, looks even better.
>
> > If yes, I would do the changes, and issue the v6 later.
>
> Yep, please change as suggested above. Same comment applies to the next
> patch "PCI: imx6: Add iMX8MP PCIe support".
>
> Also in both patches the dot between i and MX in i.MX8MP is missing in the
> patch description subject line.
Okay, thanks.
Best Regards
Richard Zhu
>
> Regards,
> Lucas
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
WARNING: multiple messages have this Message-ID (diff)
From: Hongxing Zhu <hongxing.zhu@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
"robh@kernel.org" <robh@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"vkoul@kernel.org" <vkoul@kernel.org>,
"alexander.stein@ew.tq-group.com"
<alexander.stein@ew.tq-group.com>,
"marex@denx.de" <marex@denx.de>,
"richard.leitner@linux.dev" <richard.leitner@linux.dev>
Cc: "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY support
Date: Thu, 1 Sep 2022 01:28:11 +0000 [thread overview]
Message-ID: <AS8PR04MB86760623F932DE8EF23AA5718C7B9@AS8PR04MB8676.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <5e357e6b4bb4957a74913d561f0da491e8121ed6.camel@pengutronix.de>
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年8月31日 16:34
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com;
> marex@denx.de; richard.leitner@linux.dev
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY
> support
>
> Am Mittwoch, dem 31.08.2022 um 06:16 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年8月30日 21:07
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> > > bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> > > shawnguo@kernel.org; vkoul@kernel.org;
> > > alexander.stein@ew.tq-group.com; marex@denx.de;
> > > richard.leitner@linux.dev
> > > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP
> > > PCIe PHY support
> > >
> > > Am Dienstag, dem 30.08.2022 um 15:46 +0800 schrieb Richard Zhu:
> > > > Add i.MX8MP PCIe PHY support.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > Tested-by: Marek Vasut <marex@denx.de>
> > > > Tested-by: Richard Leitner <richard.leitner@skidata.com>
> > > > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 137
> > > > +++++++++++++--------
> > > > 1 file changed, 89 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > index ad7d2edfc414..c76e3a1a5f51 100644
> > > > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
> > > > @@ -11,6 +11,9 @@
> > > > #include <linux/mfd/syscon.h>
> > > > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
> > > > #include <linux/module.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/phy/phy.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/regmap.h>
> > > > @@ -31,12 +34,10 @@
> > > > #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 IMX8MM_PCIE_PHY_CMN_REG075 0x1D4
> > > > +#define ANA_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)
> > > > @@ -47,16 +48,23 @@
> > > > #define IMX8MM_GPR_PCIE_SSC_EN BIT(16)
> > > > #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9)
> > > >
> > > > +enum imx8_pcie_phy_type {
> > > > + IMX8MM,
> > > > + IMX8MP,
> > > > +};
> > > > +
> > > > struct imx8_pcie_phy {
> > > > void __iomem *base;
> > > > struct clk *clk;
> > > > struct phy *phy;
> > > > struct regmap *iomuxc_gpr;
> > > > struct reset_control *reset;
> > > > + struct reset_control *perst;
> > > > u32 refclk_pad_mode;
> > > > u32 tx_deemph_gen1;
> > > > u32 tx_deemph_gen2;
> > > > bool clkreq_unused;
> > > > + enum imx8_pcie_phy_type variant;
> > > > };
> > > >
> > > > static int imx8_pcie_phy_init(struct phy *phy) @@ -68,31 +76,20
> > > > @@ static int imx8_pcie_phy_init(struct phy *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,
> > > > - 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,
> > > > - 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);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + /* Tune PHY de-emphasis setting to pass PCIe
> > > > compliance. */
> > > > + if (imx8_phy->tx_deemph_gen1)
> > > > + writel(imx8_phy->tx_deemph_gen1,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG5);
> > > > + if (imx8_phy->tx_deemph_gen2)
> > > > + writel(imx8_phy->tx_deemph_gen2,
> > > > + imx8_phy->base +
> > > > PCIE_PHY_TRSV_REG6);
> > > > + break;
> > > > + case IMX8MP:
> > > > + reset_control_assert(imx8_phy->perst);
> > > > + break;
> > > > + }
> > > >
> > > > if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ||
> > > > pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) { @@ -
> > > > 120,20
> > > +117,44 @@
> > > > static int imx8_pcie_phy_init(struct phy *phy)
> > > > imx8_phy->base +
> > > > IMX8MM_PCIE_PHY_CMN_REG065);
> > > > }
> > > >
> > > > - /* Tune PHY de-emphasis setting to pass PCIe compliance.
> > > > */
> > > > - if (imx8_phy->tx_deemph_gen1)
> > > > - writel(imx8_phy->tx_deemph_gen1,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG5);
> > > > - if (imx8_phy->tx_deemph_gen2)
> > > > - writel(imx8_phy->tx_deemph_gen2,
> > > > - imx8_phy->base + PCIE_PHY_TRSV_REG6);
> > > > + /* 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,
> > > > + 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,
> > > > + 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);
> > > >
> > > > - reset_control_deassert(imx8_phy->reset);
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MP:
> > > > + reset_control_deassert(imx8_phy->perst);
> > > > + fallthrough;
> > > > + case IMX8MM:
> > > > + reset_control_deassert(imx8_phy->reset);
> > > > + usleep_range(200, 500);
> > > > + break;
> > > > + }
> > > >
> > > > /* 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);
> > > > + ret = readl_poll_timeout(imx8_phy->base +
> > > IMX8MM_PCIE_PHY_CMN_REG075,
> > > > + val, val == ANA_PLL_DONE, 10,
> > > > 20000);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -160,6 +181,13 @@ static const struct phy_ops imx8_pcie_phy_ops
> > > > = {
> > > > .owner = THIS_MODULE,
> > > > };
> > > >
> > > > +static const struct of_device_id imx8_pcie_phy_of_match[] = {
> > > > + {.compatible = "fsl,imx8mm-pcie-phy", .data = (void
> > > > *)IMX8MM},
> > > > + {.compatible = "fsl,imx8mp-pcie-phy", .data = (void
> > > > *)IMX8MP},
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
> > > > +
> > > > static int imx8_pcie_phy_probe(struct platform_device *pdev) {
> > > > struct phy_provider *phy_provider; @@ -172,6 +200,9 @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > if (!imx8_phy)
> > > > return -ENOMEM;
> > > >
> > > > + imx8_phy->variant =
> > > > + (enum
> > > > imx8_pcie_phy_type)of_device_get_match_data(dev);
> > > > +
> > > > /* get PHY refclk pad mode */
> > > > of_property_read_u32(np, "fsl,refclk-pad-mode",
> > > > &imx8_phy->refclk_pad_mode); @@ -196,8 +227,16
> @@ static
> > > > int imx8_pcie_phy_probe(struct
> > > platform_device *pdev)
> > > > }
> > > >
> > > > /* Grab GPR config register range */
> > > > - imx8_phy->iomuxc_gpr =
> > > > - syscon_regmap_lookup_by_compatible("fsl,imx6q-
> > > > iomuxc-gpr");
> > > > + switch (imx8_phy->variant) {
> > > > + case IMX8MM:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mm-iomuxc-gpr");
> > > > + break;
> > > > + case IMX8MP:
> > > > + imx8_phy->iomuxc_gpr =
> > > > +
> > > syscon_regmap_lookup_by_compatible("fsl,imx8mp-iomuxc-gpr");
> > > > + break;
> > > > + }
> > >
> > > Oh, I had a real phandle in DT in mind for this, but I see how this
> > > would be hard to introduce in a backward compatible manner for the
> > > 8MM.
> > > At least this way it is fully contained in the driver and doesn't
> > > leak into DT compatibles.
> > >
> > > Maybe we could make this a little nicer by just having an const
> > > array of iomux syscon compatibles indexed by imx8_phy->variant, to
> > > avoid the switch and the resulting code (almost-)duplication.
> > >
> > Hi Lucas:
> > Thanks for your comments.
> > Do you mean a drvdata struct indexed by variant, and contains the
> > const array of iomux syscon compatible like below?
> >
> > +static const struct imx8_pcie_phy_drvdata drvdata[] = {
> > + [IMX8MM] = {
> > + .variant = IMX8MM,
> > + .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + },
> > +
> > + [IMX8MP] = {
> > + .variant = IMX8MP,
> > + .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + },
> > +};
> > +
> >
> > Then, we can get the drvdata, and find the according gpr syscon regmap
> > below.
> >
> > + imx8_phy->drvdata = of_device_get_match_data(dev);
> > ...
> > + imx8_phy->iomuxc_gpr =
> > + syscon_regmap_lookup_by_compatible(imx8_phy-
> > >drvdata->gpr);
> >
> I had a simple array of GPR compatible strings in mind, so we could lookup the
> compatible from the variant. But I think putting it directly in drvdata, like done
> in your suggestion above, looks even better.
>
> > If yes, I would do the changes, and issue the v6 later.
>
> Yep, please change as suggested above. Same comment applies to the next
> patch "PCI: imx6: Add iMX8MP PCIe support".
>
> Also in both patches the dot between i and MX in i.MX8MP is missing in the
> patch description subject line.
Okay, thanks.
Best Regards
Richard Zhu
>
> Regards,
> Lucas
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-09-01 1:28 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-30 7:45 [PATCH v5 0/7] Add the iMX8MP PCIe support Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:45 ` [PATCH v5 1/7] dt-binding: phy: Add iMX8MP PCIe PHY binding Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:45 ` [PATCH v5 2/7] arm64: dts: imx8mp: Add iMX8MP PCIe support Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:45 ` Richard Zhu
2022-08-30 7:46 ` [PATCH v5 3/7] arm64: dts: imx8mp-evk: Add " Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-31 10:18 ` Marcel Ziswiler
2022-08-31 10:18 ` Marcel Ziswiler
2022-08-31 10:18 ` Marcel Ziswiler
2022-09-01 1:28 ` Hongxing Zhu
2022-09-01 1:28 ` Hongxing Zhu
2022-09-01 1:28 ` Hongxing Zhu
2022-08-30 7:46 ` [PATCH v5 4/7] reset: imx7: Fix the iMX8MP PCIe PHY PERST support Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 16:46 ` Philipp Zabel
2022-08-30 16:46 ` Philipp Zabel
2022-08-30 16:46 ` Philipp Zabel
2022-08-31 0:38 ` Hongxing Zhu
2022-08-31 0:38 ` Hongxing Zhu
2022-08-31 0:38 ` Hongxing Zhu
2022-08-30 7:46 ` [PATCH v5 5/7] soc: imx: imx8mp-blk-ctrl: handle PCIe PHY resets Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-31 8:36 ` Lucas Stach
2022-08-31 8:36 ` Lucas Stach
2022-08-31 8:36 ` Lucas Stach
2022-09-01 1:28 ` Hongxing Zhu
2022-09-01 1:28 ` Hongxing Zhu
2022-09-01 1:28 ` Hongxing Zhu
2022-08-30 7:46 ` [PATCH v5 6/7] phy: freescale: imx8m-pcie: Add iMX8MP PCIe PHY support Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 13:07 ` Lucas Stach
2022-08-30 13:07 ` Lucas Stach
2022-08-30 13:07 ` Lucas Stach
2022-08-31 6:16 ` Hongxing Zhu
2022-08-31 6:16 ` Hongxing Zhu
2022-08-31 6:16 ` Hongxing Zhu
2022-08-31 8:34 ` Lucas Stach
2022-08-31 8:34 ` Lucas Stach
2022-08-31 8:34 ` Lucas Stach
2022-09-01 1:28 ` Hongxing Zhu [this message]
2022-09-01 1:28 ` Hongxing Zhu
2022-09-01 1:28 ` Hongxing Zhu
2022-08-30 7:46 ` [PATCH v5 7/7] PCI: imx6: Add iMX8MP PCIe support Richard Zhu
2022-08-30 7:46 ` Richard Zhu
2022-08-30 7:46 ` Richard Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8PR04MB86760623F932DE8EF23AA5718C7B9@AS8PR04MB8676.eurprd04.prod.outlook.com \
--to=hongxing.zhu@nxp.com \
--cc=alexander.stein@ew.tq-group.com \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=l.stach@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=marex@denx.de \
--cc=p.zabel@pengutronix.de \
--cc=richard.leitner@linux.dev \
--cc=robh@kernel.org \
--cc=shawnguo@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.