| Message ID | 1664440622-18556-4-git-send-email-hongxing.zhu@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add the iMX8MP PCIe support | expand |
On 29.09.22 09:37, Richard Zhu wrote: > To make it more flexible and easy to expand. Refine i.MX8MM PCIe PHY > driver. > - Use gpr compatible string to avoid the codes duplications when add > another platform PCIe PHY support. > - Re-orange the codes to let it more flexible and easy to expand. Re-arrange > No functions changes basicly. No functional change. > > 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> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 106 +++++++++++++-------- > 1 file changed, 66 insertions(+), 40 deletions(-) > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > index 2377ed307b53..59b46a4ae069 100644 > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > @@ -11,6 +11,7 @@ > #include <linux/mfd/syscon.h> > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > @@ -45,6 +46,15 @@ > #define IMX8MM_GPR_PCIE_SSC_EN BIT(16) > #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9) > > +enum imx8_pcie_phy_type { > + IMX8MM, > +}; > + > +struct imx8_pcie_phy_drvdata { > + enum imx8_pcie_phy_type variant; Better do indentation on the member name. > + const char *gpr; > +}; > + > struct imx8_pcie_phy { > void __iomem *base; > struct clk *clk; > @@ -55,6 +65,7 @@ struct imx8_pcie_phy { > u32 tx_deemph_gen1; > u32 tx_deemph_gen2; > bool clkreq_unused; > + const struct imx8_pcie_phy_drvdata *drvdata; > }; > > static int imx8_pcie_phy_init(struct phy *phy) > @@ -66,31 +77,17 @@ 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->drvdata->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; > + } If you say no functional change intended, I'd expect that register writes happen in the same sequence. It might be ok, that you do this tuning here, but I think it warrants a mention in the commit message why it's ok. Looks good otherwise. With nitpicks addressed: Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: 2022年9月30日 16:29 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; vkoul@kernel.org; > p.zabel@pengutronix.de; l.stach@pengutronix.de; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; robh@kernel.org; shawnguo@kernel.org; > alexander.stein@ew.tq-group.com; marex@denx.de; richard.leitner@linux.dev > Cc: devicetree@vger.kernel.org; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; linux-phy@lists.infradead.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v10 3/4] phy: freescale: imx8m-pcie: Refine i.MX8MM > PCIe PHY driver > > On 29.09.22 09:37, Richard Zhu wrote: > > To make it more flexible and easy to expand. Refine i.MX8MM PCIe PHY > > driver. > > - Use gpr compatible string to avoid the codes duplications when add > > another platform PCIe PHY support. > > - Re-orange the codes to let it more flexible and easy to expand. > > Re-arrange Sorry for the spell mistake. Thanks for your review comments. > > > No functions changes basicly. > > No functional change. Got that, thanks. > > > > > 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> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 106 > > +++++++++++++-------- > > 1 file changed, 66 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > > b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > > index 2377ed307b53..59b46a4ae069 100644 > > --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > > +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c > > @@ -11,6 +11,7 @@ > > #include <linux/mfd/syscon.h> > > #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> > > #include <linux/module.h> > > +#include <linux/of_device.h> > > #include <linux/phy/phy.h> > > #include <linux/platform_device.h> > > #include <linux/regmap.h> > > @@ -45,6 +46,15 @@ > > #define IMX8MM_GPR_PCIE_SSC_EN BIT(16) > > #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9) > > > > +enum imx8_pcie_phy_type { > > + IMX8MM, > > +}; > > + > > +struct imx8_pcie_phy_drvdata { > > + enum imx8_pcie_phy_type variant; > > Better do indentation on the member name. Got that, would make them indent later thanks. > > > + const char *gpr; > > +}; > > + > > struct imx8_pcie_phy { > > void __iomem *base; > > struct clk *clk; > > @@ -55,6 +65,7 @@ struct imx8_pcie_phy { > > u32 tx_deemph_gen1; > > u32 tx_deemph_gen2; > > bool clkreq_unused; > > + const struct imx8_pcie_phy_drvdata *drvdata; > > }; > > > > static int imx8_pcie_phy_init(struct phy *phy) @@ -66,31 +77,17 @@ > > 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->drvdata->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; > > + } > > If you say no functional change intended, I'd expect that register writes happen > in the same sequence. It might be ok, that you do this tuning here, but I think > it warrants a mention in the commit message why it's ok. > > > Looks good otherwise. With nitpicks addressed: > > Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > Got that, thanks a lot. Best Regards Richard Zhu > > -- > Pengutronix e.K. | > | > Steuerwalder Str. 21 | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe > ngutronix.de%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C3f75 > 643ad03b406ddfbf08daa2bdd804%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C638001233500315244%7CUnknown%7CTWFpbGZsb3d8eyJW > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > 000%7C%7C%7C&sdata=6NPsGMBN8culT%2BjIWtay0qT0o4L66lnmOtXI > 4fo7z0M%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: > +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c index 2377ed307b53..59b46a4ae069 100644 --- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c +++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c @@ -11,6 +11,7 @@ #include <linux/mfd/syscon.h> #include <linux/mfd/syscon/imx7-iomuxc-gpr.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> #include <linux/regmap.h> @@ -45,6 +46,15 @@ #define IMX8MM_GPR_PCIE_SSC_EN BIT(16) #define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE BIT(9) +enum imx8_pcie_phy_type { + IMX8MM, +}; + +struct imx8_pcie_phy_drvdata { + enum imx8_pcie_phy_type variant; + const char *gpr; +}; + struct imx8_pcie_phy { void __iomem *base; struct clk *clk; @@ -55,6 +65,7 @@ struct imx8_pcie_phy { u32 tx_deemph_gen1; u32 tx_deemph_gen2; bool clkreq_unused; + const struct imx8_pcie_phy_drvdata *drvdata; }; static int imx8_pcie_phy_init(struct phy *phy) @@ -66,31 +77,17 @@ 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->drvdata->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; + } if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT || pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) { @@ -118,15 +115,37 @@ 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); - reset_control_deassert(imx8_phy->reset); + 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); + + switch (imx8_phy->drvdata->variant) { + 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_REG075, @@ -157,6 +176,17 @@ static const struct phy_ops imx8_pcie_phy_ops = { .owner = THIS_MODULE, }; +static const struct imx8_pcie_phy_drvdata imx8mm_drvdata = { + .variant = IMX8MM, + .gpr = "fsl,imx8mm-iomuxc-gpr", +}; + +static const struct of_device_id imx8_pcie_phy_of_match[] = { + {.compatible = "fsl,imx8mm-pcie-phy", .data = &imx8mm_drvdata, }, + { }, +}; +MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match); + static int imx8_pcie_phy_probe(struct platform_device *pdev) { struct phy_provider *phy_provider; @@ -169,6 +199,8 @@ static int imx8_pcie_phy_probe(struct platform_device *pdev) if (!imx8_phy) return -ENOMEM; + imx8_phy->drvdata = 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); @@ -194,7 +226,7 @@ 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"); + syscon_regmap_lookup_by_compatible(imx8_phy->drvdata->gpr); if (IS_ERR(imx8_phy->iomuxc_gpr)) { dev_err(dev, "unable to find iomuxc registers\n"); return PTR_ERR(imx8_phy->iomuxc_gpr); @@ -222,12 +254,6 @@ static int imx8_pcie_phy_probe(struct platform_device *pdev) 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 = {