RE: [PATCH v6 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

-----Original Message-----
From: Vinod Koul <vkoul@kernel.org>
Sent: Thursday, December 2, 2021 12:30 PM
To: Hongxing Zhu <hongxing.zhu@nxp.com>
Cc: l.stach@pengutronix.de; bhelgaas@google.com;
lorenzo.pieralisi@arm.com; Marcel Ziswiler
[off-list ref]; tharvey@gateworks.com;
kishon@ti.com; robh@kernel.org; galak@kernel.crashing.org;
shawnguo@kernel.org; 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 [off-list ref]
Subject: Re: [PATCH v6 5/8] phy: freescale: pcie: Initialize the imx8 pcie
standalone phy driver

On 18-11-21, 09:54, Richard Zhu wrote:

quoted

Add the standalone i.MX8 PCIe PHY driver.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <redacted>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/phy/freescale/Kconfig              |   9 +
 drivers/phy/freescale/Makefile             |   1 +
 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 237
+++++++++++++++++++++
 3 files changed, 247 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..e821498b1f7f
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.MX8M PCIE PHY"
+	depends on OF && HAS_IOMEM
+	select GENERIC_PHY
+	default ARCH_MXC && ARM64

Why should this be default ? We dont do that for new drivers.. You may
add this to respective config file though...

[Richard Zhu] First of all, thanks a lot for your review comments.
I see, and would remove the default in the kconfig.

quoted

+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,
+			   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);
+
+

No multi blank line please

[Richard Zhu] Ok, got that.

quoted

+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");

This does not match the SPDX tag you have given

[Richard Zhu] Should the "GPL v2" correct one?
If yes, I would change it this way later.
Thanks.

Best Regards
Richard

--
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel