[PATCH v2 0/1] Use the generic PHY framework for Ingenic USB PHY.

[PATCH v2 0/1] Use the generic PHY framework for Ingenic USB PHY.

[周琰杰 (Zhou Yanjie)]

v1->v2:
Fix bug, ".of_match_table = of_match_ptr(ingenic_usb_phy_of_matches)" is wrong
and should be replaced with ".of_match_table = ingenic_usb_phy_of_matches".

周琰杰 (Zhou Yanjie) (1):
  USB: PHY: JZ4770: Use the generic PHY framework.

 drivers/phy/Kconfig                                |   1 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/ingenic/Kconfig                        |  12 +
 drivers/phy/ingenic/Makefile                       |   2 +
 .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 ++++++++++++---------
 drivers/usb/phy/Kconfig                            |   8 -
 drivers/usb/phy/Makefile                           |   1 -
 7 files changed, 165 insertions(+), 116 deletions(-)
 create mode 100644 drivers/phy/ingenic/Kconfig
 create mode 100644 drivers/phy/ingenic/Makefile
 rename drivers/{usb/phy/phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} (63%)

-- 
2.11.0

[PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

[周琰杰 (Zhou Yanjie)]

Used the generic PHY framework API to create the PHY,
and move the driver to driver/phy/ingenic.

Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---

Notes:
    v1->v2:
    Fix bug, ".of_match_table = of_match_ptr(ingenic_usb_phy_of_matches)" is wrong
    and should be replaced with ".of_match_table = ingenic_usb_phy_of_matches".

 drivers/phy/Kconfig                                |   1 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/ingenic/Kconfig                        |  12 +
 drivers/phy/ingenic/Makefile                       |   2 +
 .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 ++++++++++++---------
 drivers/usb/phy/Kconfig                            |   8 -
 drivers/usb/phy/Makefile                           |   1 -
 7 files changed, 165 insertions(+), 116 deletions(-)
 create mode 100644 drivers/phy/ingenic/Kconfig
 create mode 100644 drivers/phy/ingenic/Makefile
 rename drivers/{usb/phy/phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} (63%)

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index de9362c25c07..0534b0fdd057 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
 source "drivers/phy/cadence/Kconfig"
 source "drivers/phy/freescale/Kconfig"
 source "drivers/phy/hisilicon/Kconfig"
+source "drivers/phy/ingenic/Kconfig"
 source "drivers/phy/lantiq/Kconfig"
 source "drivers/phy/marvell/Kconfig"
 source "drivers/phy/mediatek/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index c27408e4daae..ab24f0d20763 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -14,6 +14,7 @@ obj-y					+= allwinner/	\
 					   cadence/	\
 					   freescale/	\
 					   hisilicon/	\
+					   ingenic/	\
 					   intel/	\
 					   lantiq/	\
 					   marvell/	\
diff --git a/drivers/phy/ingenic/Kconfig b/drivers/phy/ingenic/Kconfig
new file mode 100644
index 000000000000..b9581eae89dd
--- /dev/null
+++ b/drivers/phy/ingenic/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Phy drivers for Ingenic platforms
+#
+config PHY_INGENIC_USB
+	tristate "Ingenic SoCs USB PHY Driver"
+	depends on (MACH_INGENIC && MIPS) || COMPILE_TEST
+	depends on USB_SUPPORT
+	select GENERIC_PHY
+	help
+	  This driver provides USB PHY support for the USB controller found
+	  on the JZ-series and X-series SoCs from Ingenic.
diff --git a/drivers/phy/ingenic/Makefile b/drivers/phy/ingenic/Makefile
new file mode 100644
index 000000000000..65d5ea00fc9d
--- /dev/null
+++ b/drivers/phy/ingenic/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y		+= phy-ingenic-usb.o
diff --git a/drivers/usb/phy/phy-jz4770.c b/drivers/phy/ingenic/phy-ingenic-usb.c
similarity index 63%
rename from drivers/usb/phy/phy-jz4770.c
rename to drivers/phy/ingenic/phy-ingenic-usb.c
index f6d3731581eb..86a95b498785 100644
--- a/drivers/usb/phy/phy-jz4770.c
+++ b/drivers/phy/ingenic/phy-ingenic-usb.c
@@ -7,12 +7,12 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
-#include <linux/usb/otg.h>
-#include <linux/usb/phy.h>
+#include <linux/phy/phy.h>
 
 /* OTGPHY register offsets */
 #define REG_USBPCR_OFFSET			0x00
@@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
 struct ingenic_soc_info {
 	enum ingenic_usb_phy_version version;
 
-	void (*usb_phy_init)(struct usb_phy *phy);
+	void (*usb_phy_init)(struct phy *phy);
 };
 
-struct jz4770_phy {
+struct ingenic_usb_phy {
 	const struct ingenic_soc_info *soc_info;
 
-	struct usb_phy phy;
-	struct usb_otg otg;
+	struct phy *phy;
 	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
 	struct regulator *vcc_supply;
 };
 
-static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg *otg)
+static int ingenic_usb_phy_init(struct phy *phy)
 {
-	return container_of(otg, struct jz4770_phy, otg);
-}
-
-static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy *phy)
-{
-	return container_of(phy, struct jz4770_phy, phy);
-}
-
-static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
-				     struct usb_gadget *gadget)
-{
-	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
-	u32 reg;
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
+	int err;
 
-	if (priv->soc_info->version >= ID_X1000) {
-		reg = readl(priv->base + REG_USBPCR1_OFFSET);
-		reg |= USBPCR1_BVLD_REG;
-		writel(reg, priv->base + REG_USBPCR1_OFFSET);
+	err = clk_prepare_enable(priv->clk);
+	if (err) {
+		dev_err(priv->dev, "Unable to start clock: %d\n", err);
+		return err;
 	}
 
-	reg = readl(priv->base + REG_USBPCR_OFFSET);
-	reg &= ~USBPCR_USB_MODE;
-	reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_OTG_DISABLE;
-	writel(reg, priv->base + REG_USBPCR_OFFSET);
+	priv->soc_info->usb_phy_init(phy);
 
 	return 0;
 }
 
-static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct usb_bus *host)
+static int ingenic_usb_phy_exit(struct phy *phy)
 {
-	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
-	u32 reg;
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 
-	reg = readl(priv->base + REG_USBPCR_OFFSET);
-	reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_OTG_DISABLE);
-	reg |= USBPCR_USB_MODE;
-	writel(reg, priv->base + REG_USBPCR_OFFSET);
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc_supply);
 
 	return 0;
 }
 
-static int ingenic_usb_phy_init(struct usb_phy *phy)
+static int ingenic_usb_phy_power_on(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 	int err;
-	u32 reg;
 
 	err = regulator_enable(priv->vcc_supply);
 	if (err) {
@@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct usb_phy *phy)
 		return err;
 	}
 
-	err = clk_prepare_enable(priv->clk);
-	if (err) {
-		dev_err(priv->dev, "Unable to start clock: %d\n", err);
-		return err;
-	}
-
-	priv->soc_info->usb_phy_init(phy);
-
-	/* Wait for PHY to reset */
-	usleep_range(30, 300);
-	reg = readl(priv->base + REG_USBPCR_OFFSET);
-	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
-	usleep_range(300, 1000);
-
 	return 0;
 }
 
-static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
+static int ingenic_usb_phy_power_off(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 
-	clk_disable_unprepare(priv->clk);
 	regulator_disable(priv->vcc_supply);
+
+	return 0;
 }
 
-static void ingenic_usb_phy_remove(void *phy)
+static int ingenic_usb_phy_set_mode(struct phy *phy,
+				  enum phy_mode mode, int submode)
 {
-	usb_remove_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
+	u32 reg;
+
+	switch (mode) {
+	case PHY_MODE_USB_HOST:
+		reg = readl(priv->base + REG_USBPCR_OFFSET);
+		reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_OTG_DISABLE);
+		reg |= USBPCR_USB_MODE;
+		writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+		break;
+	case PHY_MODE_USB_DEVICE:
+		if (priv->soc_info->version >= ID_X1000) {
+			reg = readl(priv->base + REG_USBPCR1_OFFSET);
+			reg |= USBPCR1_BVLD_REG;
+			writel(reg, priv->base + REG_USBPCR1_OFFSET);
+		}
+
+		reg = readl(priv->base + REG_USBPCR_OFFSET);
+		reg &= ~USBPCR_USB_MODE;
+		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_OTG_DISABLE;
+		writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+		break;
+	case PHY_MODE_USB_OTG:
+		reg = readl(priv->base + REG_USBPCR_OFFSET);
+		reg &= ~USBPCR_OTG_DISABLE;
+		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_USB_MODE;
+		writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
-static void jz4770_usb_phy_init(struct usb_phy *phy)
+static const struct phy_ops ingenic_usb_phy_ops = {
+	.init		= ingenic_usb_phy_init,
+	.exit		= ingenic_usb_phy_exit,
+	.power_on	= ingenic_usb_phy_power_on,
+	.power_off	= ingenic_usb_phy_power_off,
+	.set_mode	= ingenic_usb_phy_set_mode,
+	.owner		= THIS_MODULE,
+};
+
+static void jz4770_usb_phy_init(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 	u32 reg;
 
 	reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
@@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct usb_phy *phy)
 		USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | USBPCR_TXVREFTUNE_DFT |
 		USBPCR_POR;
 	writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+	/* Wait for PHY to reset */
+	usleep_range(30, 300);
+	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
+	usleep_range(300, 1000);
 }
 
-static void jz4780_usb_phy_init(struct usb_phy *phy)
+static void jz4780_usb_phy_init(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 	u32 reg;
 
 	reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL |
@@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct usb_phy *phy)
 
 	reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
 	writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+	/* Wait for PHY to reset */
+	usleep_range(30, 300);
+	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
+	usleep_range(300, 1000);
 }
 
-static void x1000_usb_phy_init(struct usb_phy *phy)
+static void x1000_usb_phy_init(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 	u32 reg;
 
 	reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_WORD_IF_16BIT;
@@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy *phy)
 		USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
 		USBPCR_COMMONONN | USBPCR_POR;
 	writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+	/* Wait for PHY to reset */
+	usleep_range(30, 300);
+	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
+	usleep_range(300, 1000);
 }
 
-static void x1830_usb_phy_init(struct usb_phy *phy)
+static void x1830_usb_phy_init(struct phy *phy)
 {
-	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
+	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
 	u32 reg;
 
 	/* rdt */
@@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy *phy)
 	reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |	USBPCR_TXPREEMPHTUNE |
 		USBPCR_COMMONONN | USBPCR_POR;
 	writel(reg, priv->base + REG_USBPCR_OFFSET);
+
+	/* Wait for PHY to reset */
+	usleep_range(30, 300);
+	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
+	usleep_range(300, 1000);
 }
 
 static const struct ingenic_soc_info jz4770_soc_info = {
@@ -276,87 +309,96 @@ static const struct ingenic_soc_info x1830_soc_info = {
 	.usb_phy_init = x1830_usb_phy_init,
 };
 
-static const struct of_device_id ingenic_usb_phy_of_matches[] = {
-	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
-	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
-	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
-	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
-
-static int jz4770_phy_probe(struct platform_device *pdev)
+static int ingenic_usb_phy_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct jz4770_phy *priv;
+	struct ingenic_usb_phy *priv;
+	struct phy_provider *provider;
 	int err;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	priv->dev = &pdev->dev;
+
 	priv->soc_info = device_get_match_data(&pdev->dev);
 	if (!priv->soc_info) {
 		dev_err(&pdev->dev, "Error: No device match found\n");
 		return -ENODEV;
 	}
 
-	platform_set_drvdata(pdev, priv);
-	priv->dev = dev;
-	priv->phy.dev = dev;
-	priv->phy.otg = &priv->otg;
-	priv->phy.label = "ingenic-usb-phy";
-	priv->phy.init = ingenic_usb_phy_init;
-	priv->phy.shutdown = ingenic_usb_phy_shutdown;
-
-	priv->otg.state = OTG_STATE_UNDEFINED;
-	priv->otg.usb_phy = &priv->phy;
-	priv->otg.set_host = ingenic_usb_phy_set_host;
-	priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
-
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
-		dev_err(dev, "Failed to map registers\n");
+		dev_err(priv->dev, "Failed to map registers\n");
 		return PTR_ERR(priv->base);
 	}
 
-	priv->clk = devm_clk_get(dev, NULL);
+	priv->clk = devm_clk_get(priv->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		err = PTR_ERR(priv->clk);
 		if (err != -EPROBE_DEFER)
-			dev_err(dev, "Failed to get clock\n");
+			dev_err(priv->dev, "Failed to get clock\n");
 		return err;
 	}
 
-	priv->vcc_supply = devm_regulator_get(dev, "vcc");
+	priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
 	if (IS_ERR(priv->vcc_supply)) {
 		err = PTR_ERR(priv->vcc_supply);
 		if (err != -EPROBE_DEFER)
-			dev_err(dev, "Failed to get regulator\n");
+			dev_err(priv->dev, "Failed to get regulator\n");
 		return err;
 	}
 
-	err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
-	if (err) {
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "Unable to register PHY\n");
-		return err;
+	priv->phy = devm_phy_create(priv->dev, NULL, &ingenic_usb_phy_ops);
+	if (IS_ERR(priv)) {
+		dev_err(priv->dev, "Failed to create PHY: %ld\n",	PTR_ERR(priv));
+		return PTR_ERR(priv);
+	}
+
+	provider = devm_of_phy_provider_register(priv->dev, of_phy_simple_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(priv->dev, "Failed to register PHY provider: %ld\n", PTR_ERR(provider));
+		return PTR_ERR(provider);
 	}
 
-	return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, &priv->phy);
+	platform_set_drvdata(pdev, priv);
+	phy_set_drvdata(priv->phy, priv);
+
+	return 0;
+}
+
+static int ingenic_usb_phy_remove(struct platform_device *pdev)
+{
+	struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc_supply);
+
+	return 0;
 }
 
-static struct platform_driver ingenic_phy_driver = {
-	.probe		= jz4770_phy_probe,
+static const struct of_device_id ingenic_usb_phy_of_matches[] = {
+	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
+	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
+	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
+	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
+
+static struct platform_driver ingenic_usb_phy_driver = {
+	.probe		= ingenic_usb_phy_probe,
+	.remove		= ingenic_usb_phy_remove,
 	.driver		= {
-		.name	= "jz4770-phy",
-		.of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
+		.name	= "ingenic-usb-phy",
+		.of_match_table = ingenic_usb_phy_of_matches,
 	},
 };
-module_platform_driver(ingenic_phy_driver);
+module_platform_driver(ingenic_usb_phy_driver);
 
 MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
 MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>");
 MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
 MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
+MODULE_ALIAS("jz4770_phy");
 MODULE_LICENSE("GPL");
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index ef4787cd3d37..ff24fca0a2d9 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
 	  Provides read/write operations to the ULPI phy register set for
 	  controllers with a viewport register (e.g. Chipidea/ARC controllers).
 
-config JZ4770_PHY
-	tristate "Ingenic SoCs Transceiver Driver"
-	depends on MIPS || COMPILE_TEST
-	select USB_PHY
-	help
-	  This driver provides PHY support for the USB controller found
-	  on the JZ-series and X-series SoCs from Ingenic.
-
 endmenu
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index b352bdbe8712..df1d99010079 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
 obj-$(CONFIG_USB_ULPI)			+= phy-ulpi.o
 obj-$(CONFIG_USB_ULPI_VIEWPORT)		+= phy-ulpi-viewport.o
 obj-$(CONFIG_KEYSTONE_USB_PHY)		+= phy-keystone.o
-obj-$(CONFIG_JZ4770_PHY)		+= phy-jz4770.o
-- 
2.11.0

Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

[Paul Cercueil]

Hi Zhou,

Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> Used the generic PHY framework API to create the PHY,
> and move the driver to driver/phy/ingenic.
> 
> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
> 
> Notes:
>     v1->v2:
>     Fix bug, ".of_match_table = 
> of_match_ptr(ingenic_usb_phy_of_matches)" is wrong
>     and should be replaced with ".of_match_table = 
> ingenic_usb_phy_of_matches".
> 
>  drivers/phy/Kconfig                                |   1 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/ingenic/Kconfig                        |  12 +
>  drivers/phy/ingenic/Makefile                       |   2 +
>  .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 
> ++++++++++++---------
>  drivers/usb/phy/Kconfig                            |   8 -
>  drivers/usb/phy/Makefile                           |   1 -
>  7 files changed, 165 insertions(+), 116 deletions(-)
>  create mode 100644 drivers/phy/ingenic/Kconfig
>  create mode 100644 drivers/phy/ingenic/Makefile
>  rename drivers/{usb/phy/phy-jz4770.c => 
> phy/ingenic/phy-ingenic-usb.c} (63%)
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index de9362c25c07..0534b0fdd057 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
>  source "drivers/phy/cadence/Kconfig"
>  source "drivers/phy/freescale/Kconfig"
>  source "drivers/phy/hisilicon/Kconfig"
> +source "drivers/phy/ingenic/Kconfig"
>  source "drivers/phy/lantiq/Kconfig"
>  source "drivers/phy/marvell/Kconfig"
>  source "drivers/phy/mediatek/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index c27408e4daae..ab24f0d20763 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -14,6 +14,7 @@ obj-y					+= allwinner/	\
>  					   cadence/	\
>  					   freescale/	\
>  					   hisilicon/	\
> +					   ingenic/	\
>  					   intel/	\
>  					   lantiq/	\
>  					   marvell/	\
> diff --git a/drivers/phy/ingenic/Kconfig b/drivers/phy/ingenic/Kconfig
> new file mode 100644
> index 000000000000..b9581eae89dd
> --- /dev/null
> +++ b/drivers/phy/ingenic/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Phy drivers for Ingenic platforms
> +#
> +config PHY_INGENIC_USB
> +	tristate "Ingenic SoCs USB PHY Driver"
> +	depends on (MACH_INGENIC && MIPS) || COMPILE_TEST

The original driver depends on MIPS || COMPILE_TEST, so you should do 
the same, otherwise you change more than what the patch description 
suggests.

> +	depends on USB_SUPPORT
> +	select GENERIC_PHY
> +	help
> +	  This driver provides USB PHY support for the USB controller found
> +	  on the JZ-series and X-series SoCs from Ingenic.
> diff --git a/drivers/phy/ingenic/Makefile 
> b/drivers/phy/ingenic/Makefile
> new file mode 100644
> index 000000000000..65d5ea00fc9d
> --- /dev/null
> +++ b/drivers/phy/ingenic/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y		+= phy-ingenic-usb.o
> diff --git a/drivers/usb/phy/phy-jz4770.c 
> b/drivers/phy/ingenic/phy-ingenic-usb.c
> similarity index 63%
> rename from drivers/usb/phy/phy-jz4770.c
> rename to drivers/phy/ingenic/phy-ingenic-usb.c
> index f6d3731581eb..86a95b498785 100644
> --- a/drivers/usb/phy/phy-jz4770.c
> +++ b/drivers/phy/ingenic/phy-ingenic-usb.c
> @@ -7,12 +7,12 @@
>   */
> 
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/usb/otg.h>
> -#include <linux/usb/phy.h>
> +#include <linux/phy/phy.h>
> 
>  /* OTGPHY register offsets */
>  #define REG_USBPCR_OFFSET			0x00
> @@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
>  struct ingenic_soc_info {
>  	enum ingenic_usb_phy_version version;
> 
> -	void (*usb_phy_init)(struct usb_phy *phy);
> +	void (*usb_phy_init)(struct phy *phy);
>  };
> 
> -struct jz4770_phy {
> +struct ingenic_usb_phy {
>  	const struct ingenic_soc_info *soc_info;
> 
> -	struct usb_phy phy;
> -	struct usb_otg otg;
> +	struct phy *phy;
>  	struct device *dev;
>  	void __iomem *base;
>  	struct clk *clk;
>  	struct regulator *vcc_supply;
>  };
> 
> -static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg 
> *otg)
> +static int ingenic_usb_phy_init(struct phy *phy)
>  {
> -	return container_of(otg, struct jz4770_phy, otg);
> -}
> -
> -static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy 
> *phy)
> -{
> -	return container_of(phy, struct jz4770_phy, phy);
> -}
> -
> -static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
> -				     struct usb_gadget *gadget)
> -{
> -	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
> -	u32 reg;
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> +	int err;
> 
> -	if (priv->soc_info->version >= ID_X1000) {
> -		reg = readl(priv->base + REG_USBPCR1_OFFSET);
> -		reg |= USBPCR1_BVLD_REG;
> -		writel(reg, priv->base + REG_USBPCR1_OFFSET);
> +	err = clk_prepare_enable(priv->clk);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to start clock: %d\n", err);
> +		return err;
>  	}
> 
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	reg &= ~USBPCR_USB_MODE;
> -	reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE;
> -	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +	priv->soc_info->usb_phy_init(phy);
> 
>  	return 0;
>  }
> 
> -static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct 
> usb_bus *host)
> +static int ingenic_usb_phy_exit(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
> -	u32 reg;
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> 
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE);
> -	reg |= USBPCR_USB_MODE;
> -	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc_supply);
> 
>  	return 0;
>  }
> 
> -static int ingenic_usb_phy_init(struct usb_phy *phy)
> +static int ingenic_usb_phy_power_on(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	int err;
> -	u32 reg;
> 
>  	err = regulator_enable(priv->vcc_supply);
>  	if (err) {
> @@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct usb_phy 
> *phy)
>  		return err;
>  	}
> 
> -	err = clk_prepare_enable(priv->clk);
> -	if (err) {
> -		dev_err(priv->dev, "Unable to start clock: %d\n", err);
> -		return err;
> -	}
> -
> -	priv->soc_info->usb_phy_init(phy);
> -
> -	/* Wait for PHY to reset */
> -	usleep_range(30, 300);
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> -	usleep_range(300, 1000);
> -
>  	return 0;
>  }
> 
> -static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
> +static int ingenic_usb_phy_power_off(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> 
> -	clk_disable_unprepare(priv->clk);
>  	regulator_disable(priv->vcc_supply);
> +
> +	return 0;
>  }
> 
> -static void ingenic_usb_phy_remove(void *phy)
> +static int ingenic_usb_phy_set_mode(struct phy *phy,
> +				  enum phy_mode mode, int submode)
>  {
> -	usb_remove_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> +	u32 reg;
> +
> +	switch (mode) {
> +	case PHY_MODE_USB_HOST:
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE);
> +		reg |= USBPCR_USB_MODE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	case PHY_MODE_USB_DEVICE:
> +		if (priv->soc_info->version >= ID_X1000) {
> +			reg = readl(priv->base + REG_USBPCR1_OFFSET);
> +			reg |= USBPCR1_BVLD_REG;
> +			writel(reg, priv->base + REG_USBPCR1_OFFSET);
> +		}
> +
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~USBPCR_USB_MODE;
> +		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	case PHY_MODE_USB_OTG:
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~USBPCR_OTG_DISABLE;
> +		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_USB_MODE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }

I think the diff should be a bit smaller (and easier to review) if you 
move ingenic_usb_phy_init / ingenic_usb_phy_exit here, where they used 
to be.

> 
> -static void jz4770_usb_phy_init(struct usb_phy *phy)
> +static const struct phy_ops ingenic_usb_phy_ops = {
> +	.init		= ingenic_usb_phy_init,
> +	.exit		= ingenic_usb_phy_exit,
> +	.power_on	= ingenic_usb_phy_power_on,
> +	.power_off	= ingenic_usb_phy_power_off,
> +	.set_mode	= ingenic_usb_phy_set_mode,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static void jz4770_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
> @@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct usb_phy 
> *phy)
>  		USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | 
> USBPCR_TXVREFTUNE_DFT |
>  		USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void jz4780_usb_phy_init(struct usb_phy *phy)
> +static void jz4780_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL |
> @@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct usb_phy 
> *phy)
> 
>  	reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void x1000_usb_phy_init(struct usb_phy *phy)
> +static void x1000_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
> USBPCR1_WORD_IF_16BIT;
> @@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy 
> *phy)
>  		USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>  		USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void x1830_usb_phy_init(struct usb_phy *phy)
> +static void x1830_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	/* rdt */
> @@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy 
> *phy)
>  	reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT 
> |	USBPCR_TXPREEMPHTUNE |
>  		USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);

Why is that code repeated four times now? The old driver had that in 
ingenic_usb_phy_init().

>  }
> 
>  static const struct ingenic_soc_info jz4770_soc_info = {
> @@ -276,87 +309,96 @@ static const struct ingenic_soc_info 
> x1830_soc_info = {
>  	.usb_phy_init = x1830_usb_phy_init,
>  };
> 
> -static const struct of_device_id ingenic_usb_phy_of_matches[] = {
> -	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
> -	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
> -	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
> -	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
> -
> -static int jz4770_phy_probe(struct platform_device *pdev)
> +static int ingenic_usb_phy_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &pdev->dev;
> -	struct jz4770_phy *priv;
> +	struct ingenic_usb_phy *priv;
> +	struct phy_provider *provider;
>  	int err;
> 
> -	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

I'd prefer that you keep a local 'dev' variable. Otherwise it only 
makes the diff bigger and it's harder to review.

>  	if (!priv)
>  		return -ENOMEM;
> 
> +	priv->dev = &pdev->dev;
> +
>  	priv->soc_info = device_get_match_data(&pdev->dev);
>  	if (!priv->soc_info) {
>  		dev_err(&pdev->dev, "Error: No device match found\n");
>  		return -ENODEV;
>  	}
> 
> -	platform_set_drvdata(pdev, priv);
> -	priv->dev = dev;
> -	priv->phy.dev = dev;
> -	priv->phy.otg = &priv->otg;
> -	priv->phy.label = "ingenic-usb-phy";
> -	priv->phy.init = ingenic_usb_phy_init;
> -	priv->phy.shutdown = ingenic_usb_phy_shutdown;
> -
> -	priv->otg.state = OTG_STATE_UNDEFINED;
> -	priv->otg.usb_phy = &priv->phy;
> -	priv->otg.set_host = ingenic_usb_phy_set_host;
> -	priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
> -
>  	priv->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(priv->base)) {
> -		dev_err(dev, "Failed to map registers\n");
> +		dev_err(priv->dev, "Failed to map registers\n");
>  		return PTR_ERR(priv->base);
>  	}
> 
> -	priv->clk = devm_clk_get(dev, NULL);
> +	priv->clk = devm_clk_get(priv->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		err = PTR_ERR(priv->clk);
>  		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Failed to get clock\n");
> +			dev_err(priv->dev, "Failed to get clock\n");
>  		return err;
>  	}
> 
> -	priv->vcc_supply = devm_regulator_get(dev, "vcc");
> +	priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
>  	if (IS_ERR(priv->vcc_supply)) {
>  		err = PTR_ERR(priv->vcc_supply);
>  		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Failed to get regulator\n");
> +			dev_err(priv->dev, "Failed to get regulator\n");
>  		return err;
>  	}
> 
> -	err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
> -	if (err) {
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Unable to register PHY\n");
> -		return err;
> +	priv->phy = devm_phy_create(priv->dev, NULL, &ingenic_usb_phy_ops);
> +	if (IS_ERR(priv)) {
> +		dev_err(priv->dev, "Failed to create PHY: %ld\n",	PTR_ERR(priv));
> +		return PTR_ERR(priv);
> +	}

There's a stray tabulation character here.

Also, no need to print error codes in the probe function - they will be 
printed anyway since the driver will fail to probe.

> +
> +	provider = devm_of_phy_provider_register(priv->dev, 
> of_phy_simple_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(priv->dev, "Failed to register PHY provider: %ld\n", 
> PTR_ERR(provider));
> +		return PTR_ERR(provider);
>  	}

Same here.

> 
> -	return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, 
> &priv->phy);
> +	platform_set_drvdata(pdev, priv);
> +	phy_set_drvdata(priv->phy, priv);

These two do the same thing. Also, you must do it before registering 
the PHY, otherwise you have a race.

> +
> +	return 0;
> +}
> +
> +static int ingenic_usb_phy_remove(struct platform_device *pdev)
> +{
> +	struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc_supply);

I assume that ingenic_usb_phy_power_off() and ingenic_usb_phy_exit() 
are automatically called when the module is removed, did you test 
module removal?

> +
> +	return 0;
>  }
> 
> -static struct platform_driver ingenic_phy_driver = {
> -	.probe		= jz4770_phy_probe,
> +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
> +	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
> +	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
> +	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);

You moved that code around, which only made the diff bigger and harder 
to review. Please keep it where it was.

> +
> +static struct platform_driver ingenic_usb_phy_driver = {
> +	.probe		= ingenic_usb_phy_probe,
> +	.remove		= ingenic_usb_phy_remove,
>  	.driver		= {
> -		.name	= "jz4770-phy",
> -		.of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
> +		.name	= "ingenic-usb-phy",
> +		.of_match_table = ingenic_usb_phy_of_matches,

You removed of_match_ptr(), which is a valid change (Ingenic SoCs all 
depend on Device Tree), but is unrelated to this patch.

>  	},
>  };
> -module_platform_driver(ingenic_phy_driver);
> +module_platform_driver(ingenic_usb_phy_driver);
> 
>  MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
>  MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>");
>  MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>  MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
> +MODULE_ALIAS("jz4770_phy");

Actually that would be "jz4770-phy".

Cheers,
-Paul

>  MODULE_LICENSE("GPL");
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index ef4787cd3d37..ff24fca0a2d9 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
>  	  Provides read/write operations to the ULPI phy register set for
>  	  controllers with a viewport register (e.g. Chipidea/ARC 
> controllers).
> 
> -config JZ4770_PHY
> -	tristate "Ingenic SoCs Transceiver Driver"
> -	depends on MIPS || COMPILE_TEST
> -	select USB_PHY
> -	help
> -	  This driver provides PHY support for the USB controller found
> -	  on the JZ-series and X-series SoCs from Ingenic.
> -
>  endmenu
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index b352bdbe8712..df1d99010079 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
>  obj-$(CONFIG_USB_ULPI)			+= phy-ulpi.o
>  obj-$(CONFIG_USB_ULPI_VIEWPORT)		+= phy-ulpi-viewport.o
>  obj-$(CONFIG_KEYSTONE_USB_PHY)		+= phy-keystone.o
> -obj-$(CONFIG_JZ4770_PHY)		+= phy-jz4770.o
> --
> 2.11.0
> 

Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

[Zhou Yanjie]

Hi Paul,

在 2020/9/4 下午10:10, Paul Cercueil 写道:
> Hi Zhou,
>
> Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Used the generic PHY framework API to create the PHY,
>> and move the driver to driver/phy/ingenic.
>>
>> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
>> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>     Fix bug, ".of_match_table = 
>> of_match_ptr(ingenic_usb_phy_of_matches)" is wrong
>>     and should be replaced with ".of_match_table = 
>> ingenic_usb_phy_of_matches".
>>
>>  drivers/phy/Kconfig                                |   1 +
>>  drivers/phy/Makefile                               |   1 +
>>  drivers/phy/ingenic/Kconfig                        |  12 +
>>  drivers/phy/ingenic/Makefile                       |   2 +
>>  .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 
>> ++++++++++++---------
>>  drivers/usb/phy/Kconfig                            |   8 -
>>  drivers/usb/phy/Makefile                           |   1 -
>>  7 files changed, 165 insertions(+), 116 deletions(-)
>>  create mode 100644 drivers/phy/ingenic/Kconfig
>>  create mode 100644 drivers/phy/ingenic/Makefile
>>  rename drivers/{usb/phy/phy-jz4770.c => 
>> phy/ingenic/phy-ingenic-usb.c} (63%)
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index de9362c25c07..0534b0fdd057 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
>>  source "drivers/phy/cadence/Kconfig"
>>  source "drivers/phy/freescale/Kconfig"
>>  source "drivers/phy/hisilicon/Kconfig"
>> +source "drivers/phy/ingenic/Kconfig"
>>  source "drivers/phy/lantiq/Kconfig"
>>  source "drivers/phy/marvell/Kconfig"
>>  source "drivers/phy/mediatek/Kconfig"
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index c27408e4daae..ab24f0d20763 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -14,6 +14,7 @@ obj-y                    += allwinner/    \
>>                         cadence/    \
>>                         freescale/    \
>>                         hisilicon/    \
>> +                       ingenic/    \
>>                         intel/    \
>>                         lantiq/    \
>>                         marvell/    \
>> diff --git a/drivers/phy/ingenic/Kconfig b/drivers/phy/ingenic/Kconfig
>> new file mode 100644
>> index 000000000000..b9581eae89dd
>> --- /dev/null
>> +++ b/drivers/phy/ingenic/Kconfig
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Phy drivers for Ingenic platforms
>> +#
>> +config PHY_INGENIC_USB
>> +    tristate "Ingenic SoCs USB PHY Driver"
>> +    depends on (MACH_INGENIC && MIPS) || COMPILE_TEST
>
> The original driver depends on MIPS || COMPILE_TEST, so you should do 
> the same, otherwise you change more than what the patch description 
> suggests.
>

Sure, I will change it in the next version.

>> +    depends on USB_SUPPORT
>> +    select GENERIC_PHY
>> +    help
>> +      This driver provides USB PHY support for the USB controller found
>> +      on the JZ-series and X-series SoCs from Ingenic.
>> diff --git a/drivers/phy/ingenic/Makefile b/drivers/phy/ingenic/Makefile
>> new file mode 100644
>> index 000000000000..65d5ea00fc9d
>> --- /dev/null
>> +++ b/drivers/phy/ingenic/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-y        += phy-ingenic-usb.o
>> diff --git a/drivers/usb/phy/phy-jz4770.c 
>> b/drivers/phy/ingenic/phy-ingenic-usb.c
>> similarity index 63%
>> rename from drivers/usb/phy/phy-jz4770.c
>> rename to drivers/phy/ingenic/phy-ingenic-usb.c
>> index f6d3731581eb..86a95b498785 100644
>> --- a/drivers/usb/phy/phy-jz4770.c
>> +++ b/drivers/phy/ingenic/phy-ingenic-usb.c
>> @@ -7,12 +7,12 @@
>>   */
>>
>>  #include <linux/clk.h>
>> +#include <linux/delay.h>
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/regulator/consumer.h>
>> -#include <linux/usb/otg.h>
>> -#include <linux/usb/phy.h>
>> +#include <linux/phy/phy.h>
>>
>>  /* OTGPHY register offsets */
>>  #define REG_USBPCR_OFFSET            0x00
>> @@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
>>  struct ingenic_soc_info {
>>      enum ingenic_usb_phy_version version;
>>
>> -    void (*usb_phy_init)(struct usb_phy *phy);
>> +    void (*usb_phy_init)(struct phy *phy);
>>  };
>>
>> -struct jz4770_phy {
>> +struct ingenic_usb_phy {
>>      const struct ingenic_soc_info *soc_info;
>>
>> -    struct usb_phy phy;
>> -    struct usb_otg otg;
>> +    struct phy *phy;
>>      struct device *dev;
>>      void __iomem *base;
>>      struct clk *clk;
>>      struct regulator *vcc_supply;
>>  };
>>
>> -static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg *otg)
>> +static int ingenic_usb_phy_init(struct phy *phy)
>>  {
>> -    return container_of(otg, struct jz4770_phy, otg);
>> -}
>> -
>> -static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy *phy)
>> -{
>> -    return container_of(phy, struct jz4770_phy, phy);
>> -}
>> -
>> -static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
>> -                     struct usb_gadget *gadget)
>> -{
>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>> -    u32 reg;
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>> +    int err;
>>
>> -    if (priv->soc_info->version >= ID_X1000) {
>> -        reg = readl(priv->base + REG_USBPCR1_OFFSET);
>> -        reg |= USBPCR1_BVLD_REG;
>> -        writel(reg, priv->base + REG_USBPCR1_OFFSET);
>> +    err = clk_prepare_enable(priv->clk);
>> +    if (err) {
>> +        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>> +        return err;
>>      }
>>
>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>> -    reg &= ~USBPCR_USB_MODE;
>> -    reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>> USBPCR_OTG_DISABLE;
>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +    priv->soc_info->usb_phy_init(phy);
>>
>>      return 0;
>>  }
>>
>> -static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct 
>> usb_bus *host)
>> +static int ingenic_usb_phy_exit(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>> -    u32 reg;
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>
>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>> -    reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>> USBPCR_OTG_DISABLE);
>> -    reg |= USBPCR_USB_MODE;
>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +    clk_disable_unprepare(priv->clk);
>> +    regulator_disable(priv->vcc_supply);
>>
>>      return 0;
>>  }
>>
>> -static int ingenic_usb_phy_init(struct usb_phy *phy)
>> +static int ingenic_usb_phy_power_on(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>      int err;
>> -    u32 reg;
>>
>>      err = regulator_enable(priv->vcc_supply);
>>      if (err) {
>> @@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct usb_phy 
>> *phy)
>>          return err;
>>      }
>>
>> -    err = clk_prepare_enable(priv->clk);
>> -    if (err) {
>> -        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>> -        return err;
>> -    }
>> -
>> -    priv->soc_info->usb_phy_init(phy);
>> -
>> -    /* Wait for PHY to reset */
>> -    usleep_range(30, 300);
>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>> -    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>> -    usleep_range(300, 1000);
>> -
>>      return 0;
>>  }
>>
>> -static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
>> +static int ingenic_usb_phy_power_off(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>
>> -    clk_disable_unprepare(priv->clk);
>>      regulator_disable(priv->vcc_supply);
>> +
>> +    return 0;
>>  }
>>
>> -static void ingenic_usb_phy_remove(void *phy)
>> +static int ingenic_usb_phy_set_mode(struct phy *phy,
>> +                  enum phy_mode mode, int submode)
>>  {
>> -    usb_remove_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>> +    u32 reg;
>> +
>> +    switch (mode) {
>> +    case PHY_MODE_USB_HOST:
>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>> +        reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>> USBPCR_OTG_DISABLE);
>> +        reg |= USBPCR_USB_MODE;
>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +        break;
>> +    case PHY_MODE_USB_DEVICE:
>> +        if (priv->soc_info->version >= ID_X1000) {
>> +            reg = readl(priv->base + REG_USBPCR1_OFFSET);
>> +            reg |= USBPCR1_BVLD_REG;
>> +            writel(reg, priv->base + REG_USBPCR1_OFFSET);
>> +        }
>> +
>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>> +        reg &= ~USBPCR_USB_MODE;
>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>> USBPCR_OTG_DISABLE;
>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +        break;
>> +    case PHY_MODE_USB_OTG:
>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>> +        reg &= ~USBPCR_OTG_DISABLE;
>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>> USBPCR_USB_MODE;
>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>>  }
>
> I think the diff should be a bit smaller (and easier to review) if you 
> move ingenic_usb_phy_init / ingenic_usb_phy_exit here, where they used 
> to be.
>

Sure.
>>
>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>> +static const struct phy_ops ingenic_usb_phy_ops = {
>> +    .init        = ingenic_usb_phy_init,
>> +    .exit        = ingenic_usb_phy_exit,
>> +    .power_on    = ingenic_usb_phy_power_on,
>> +    .power_off    = ingenic_usb_phy_power_off,
>> +    .set_mode    = ingenic_usb_phy_set_mode,
>> +    .owner        = THIS_MODULE,
>> +};
>> +
>> +static void jz4770_usb_phy_init(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>      u32 reg;
>>
>>      reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
>> @@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct usb_phy 
>> *phy)
>>          USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | 
>> USBPCR_TXVREFTUNE_DFT |
>>          USBPCR_POR;
>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +    /* Wait for PHY to reset */
>> +    usleep_range(30, 300);
>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>> +    usleep_range(300, 1000);
>>  }
>>
>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>> +static void jz4780_usb_phy_init(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>      u32 reg;
>>
>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL |
>> @@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct usb_phy 
>> *phy)
>>
>>      reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +    /* Wait for PHY to reset */
>> +    usleep_range(30, 300);
>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>> +    usleep_range(300, 1000);
>>  }
>>
>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>> +static void x1000_usb_phy_init(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>      u32 reg;
>>
>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
>> USBPCR1_WORD_IF_16BIT;
>> @@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy 
>> *phy)
>>          USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>          USBPCR_COMMONONN | USBPCR_POR;
>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +    /* Wait for PHY to reset */
>> +    usleep_range(30, 300);
>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>> +    usleep_range(300, 1000);
>>  }
>>
>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>> +static void x1830_usb_phy_init(struct phy *phy)
>>  {
>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>      u32 reg;
>>
>>      /* rdt */
>> @@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy *phy)
>>      reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | 
>> USBPCR_TXPREEMPHTUNE |
>>          USBPCR_COMMONONN | USBPCR_POR;
>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>> +
>> +    /* Wait for PHY to reset */
>> +    usleep_range(30, 300);
>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>> +    usleep_range(300, 1000);
>
> Why is that code repeated four times now? The old driver had that in 
> ingenic_usb_phy_init().
>

This is my fault, I forgot to make the corresponding changes after I 
cherry-pick it from v6, I will fix this problem in the next version.

>>  }
>>
>>  static const struct ingenic_soc_info jz4770_soc_info = {
>> @@ -276,87 +309,96 @@ static const struct ingenic_soc_info 
>> x1830_soc_info = {
>>      .usb_phy_init = x1830_usb_phy_init,
>>  };
>>
>> -static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>> -    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
>> -    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
>> -    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>> -    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>> -    { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>> -
>> -static int jz4770_phy_probe(struct platform_device *pdev)
>> +static int ingenic_usb_phy_probe(struct platform_device *pdev)
>>  {
>> -    struct device *dev = &pdev->dev;
>> -    struct jz4770_phy *priv;
>> +    struct ingenic_usb_phy *priv;
>> +    struct phy_provider *provider;
>>      int err;
>>
>> -    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>
> I'd prefer that you keep a local 'dev' variable. Otherwise it only 
> makes the diff bigger and it's harder to review.
>

Sure.

>>      if (!priv)
>>          return -ENOMEM;
>>
>> +    priv->dev = &pdev->dev;
>> +
>>      priv->soc_info = device_get_match_data(&pdev->dev);
>>      if (!priv->soc_info) {
>>          dev_err(&pdev->dev, "Error: No device match found\n");
>>          return -ENODEV;
>>      }
>>
>> -    platform_set_drvdata(pdev, priv);
>> -    priv->dev = dev;
>> -    priv->phy.dev = dev;
>> -    priv->phy.otg = &priv->otg;
>> -    priv->phy.label = "ingenic-usb-phy";
>> -    priv->phy.init = ingenic_usb_phy_init;
>> -    priv->phy.shutdown = ingenic_usb_phy_shutdown;
>> -
>> -    priv->otg.state = OTG_STATE_UNDEFINED;
>> -    priv->otg.usb_phy = &priv->phy;
>> -    priv->otg.set_host = ingenic_usb_phy_set_host;
>> -    priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
>> -
>>      priv->base = devm_platform_ioremap_resource(pdev, 0);
>>      if (IS_ERR(priv->base)) {
>> -        dev_err(dev, "Failed to map registers\n");
>> +        dev_err(priv->dev, "Failed to map registers\n");
>>          return PTR_ERR(priv->base);
>>      }
>>
>> -    priv->clk = devm_clk_get(dev, NULL);
>> +    priv->clk = devm_clk_get(priv->dev, NULL);
>>      if (IS_ERR(priv->clk)) {
>>          err = PTR_ERR(priv->clk);
>>          if (err != -EPROBE_DEFER)
>> -            dev_err(dev, "Failed to get clock\n");
>> +            dev_err(priv->dev, "Failed to get clock\n");
>>          return err;
>>      }
>>
>> -    priv->vcc_supply = devm_regulator_get(dev, "vcc");
>> +    priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
>>      if (IS_ERR(priv->vcc_supply)) {
>>          err = PTR_ERR(priv->vcc_supply);
>>          if (err != -EPROBE_DEFER)
>> -            dev_err(dev, "Failed to get regulator\n");
>> +            dev_err(priv->dev, "Failed to get regulator\n");
>>          return err;
>>      }
>>
>> -    err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
>> -    if (err) {
>> -        if (err != -EPROBE_DEFER)
>> -            dev_err(dev, "Unable to register PHY\n");
>> -        return err;
>> +    priv->phy = devm_phy_create(priv->dev, NULL, &ingenic_usb_phy_ops);
>> +    if (IS_ERR(priv)) {
>> +        dev_err(priv->dev, "Failed to create PHY: %ld\n", 
>> PTR_ERR(priv));
>> +        return PTR_ERR(priv);
>> +    }
>
> There's a stray tabulation character here.
>
> Also, no need to print error codes in the probe function - they will 
> be printed anyway since the driver will fail to probe.
>
Sure.
>> +
>> +    provider = devm_of_phy_provider_register(priv->dev, 
>> of_phy_simple_xlate);
>> +    if (IS_ERR(provider)) {
>> +        dev_err(priv->dev, "Failed to register PHY provider: %ld\n", 
>> PTR_ERR(provider));
>> +        return PTR_ERR(provider);
>>      }
>
> Same here.
>
>>
>> -    return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, 
>> &priv->phy);
>> +    platform_set_drvdata(pdev, priv);
>> +    phy_set_drvdata(priv->phy, priv);
>
> These two do the same thing. Also, you must do it before registering 
> the PHY, otherwise you have a race.
>
OK, I will fix it in the next version.
>> +
>> +    return 0;
>> +}
>> +
>> +static int ingenic_usb_phy_remove(struct platform_device *pdev)
>> +{
>> +    struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
>> +
>> +    clk_disable_unprepare(priv->clk);
>> +    regulator_disable(priv->vcc_supply);
>
> I assume that ingenic_usb_phy_power_off() and ingenic_usb_phy_exit() 
> are automatically called when the module is removed, did you test 
> module removal?
>

I think I have an oversignt, only the module install was tested, but the 
module removal was not tested.

>> +
>> +    return 0;
>>  }
>>
>> -static struct platform_driver ingenic_phy_driver = {
>> -    .probe        = jz4770_phy_probe,
>> +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>> +    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
>> +    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
>> +    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>> +    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>
> You moved that code around, which only made the diff bigger and harder 
> to review. Please keep it where it was.
>

Sure.

>> +
>> +static struct platform_driver ingenic_usb_phy_driver = {
>> +    .probe        = ingenic_usb_phy_probe,
>> +    .remove        = ingenic_usb_phy_remove,
>>      .driver        = {
>> -        .name    = "jz4770-phy",
>> -        .of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
>> +        .name    = "ingenic-usb-phy",
>> +        .of_match_table = ingenic_usb_phy_of_matches,
>
> You removed of_match_ptr(), which is a valid change (Ingenic SoCs all 
> depend on Device Tree), but is unrelated to this patch.
>

It was not removed in the previous version, so the test robot sent me an 
email. In this case, should I remove it directly herer or remove it in a 
separate patch?

>>      },
>>  };
>> -module_platform_driver(ingenic_phy_driver);
>> +module_platform_driver(ingenic_usb_phy_driver);
>>
>>  MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
>>  MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>");
>>  MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
>> +MODULE_ALIAS("jz4770_phy");
>
> Actually that would be "jz4770-phy".
>

Sure, I'll change it in the next version.

Thanks and best regards!

> Cheers,
> -Paul
>
>>  MODULE_LICENSE("GPL");
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index ef4787cd3d37..ff24fca0a2d9 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
>>        Provides read/write operations to the ULPI phy register set for
>>        controllers with a viewport register (e.g. Chipidea/ARC 
>> controllers).
>>
>> -config JZ4770_PHY
>> -    tristate "Ingenic SoCs Transceiver Driver"
>> -    depends on MIPS || COMPILE_TEST
>> -    select USB_PHY
>> -    help
>> -      This driver provides PHY support for the USB controller found
>> -      on the JZ-series and X-series SoCs from Ingenic.
>> -
>>  endmenu
>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>> index b352bdbe8712..df1d99010079 100644
>> --- a/drivers/usb/phy/Makefile
>> +++ b/drivers/usb/phy/Makefile
>> @@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)        += phy-mxs-usb.o
>>  obj-$(CONFIG_USB_ULPI)            += phy-ulpi.o
>>  obj-$(CONFIG_USB_ULPI_VIEWPORT)        += phy-ulpi-viewport.o
>>  obj-$(CONFIG_KEYSTONE_USB_PHY)        += phy-keystone.o
>> -obj-$(CONFIG_JZ4770_PHY)        += phy-jz4770.o
>> -- 
>> 2.11.0
>>
>

Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

[Paul Cercueil]

Le lun. 7 sept. 2020 à 1:06, Zhou Yanjie <zhouyanjie@wanyeetech.com> a 
écrit :
> Hi Paul,
> 
> 在 2020/9/4 下午10:10, Paul Cercueil 写道:
>> Hi Zhou,
>> 
>> Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie) 
>> \x7f<zhouyanjie@wanyeetech.com> a écrit :
>>> Used the generic PHY framework API to create the PHY,
>>> and move the driver to driver/phy/ingenic.
>>> 
>>> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
>>> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>>> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>> ---
>>> 
>>> Notes:
>>>     v1->v2:
>>>     Fix bug, ".of_match_table = 
>>> \x7f\x7fof_match_ptr(ingenic_usb_phy_of_matches)" is wrong
>>>     and should be replaced with ".of_match_table = 
>>> \x7f\x7fingenic_usb_phy_of_matches".
>>> 
>>>  drivers/phy/Kconfig                                |   1 +
>>>  drivers/phy/Makefile                               |   1 +
>>>  drivers/phy/ingenic/Kconfig                        |  12 +
>>>  drivers/phy/ingenic/Makefile                       |   2 +
>>>  .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 
>>> \x7f\x7f++++++++++++---------
>>>  drivers/usb/phy/Kconfig                            |   8 -
>>>  drivers/usb/phy/Makefile                           |   1 -
>>>  7 files changed, 165 insertions(+), 116 deletions(-)
>>>  create mode 100644 drivers/phy/ingenic/Kconfig
>>>  create mode 100644 drivers/phy/ingenic/Makefile
>>>  rename drivers/{usb/phy/phy-jz4770.c => 
>>> \x7f\x7fphy/ingenic/phy-ingenic-usb.c} (63%)
>>> 
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index de9362c25c07..0534b0fdd057 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
>>>  source "drivers/phy/cadence/Kconfig"
>>>  source "drivers/phy/freescale/Kconfig"
>>>  source "drivers/phy/hisilicon/Kconfig"
>>> +source "drivers/phy/ingenic/Kconfig"
>>>  source "drivers/phy/lantiq/Kconfig"
>>>  source "drivers/phy/marvell/Kconfig"
>>>  source "drivers/phy/mediatek/Kconfig"
>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>> index c27408e4daae..ab24f0d20763 100644
>>> --- a/drivers/phy/Makefile
>>> +++ b/drivers/phy/Makefile
>>> @@ -14,6 +14,7 @@ obj-y                    += allwinner/    \
>>>                         cadence/    \
>>>                         freescale/    \
>>>                         hisilicon/    \
>>> +                       ingenic/    \
>>>                         intel/    \
>>>                         lantiq/    \
>>>                         marvell/    \
>>> diff --git a/drivers/phy/ingenic/Kconfig 
>>> b/drivers/phy/ingenic/Kconfig
>>> new file mode 100644
>>> index 000000000000..b9581eae89dd
>>> --- /dev/null
>>> +++ b/drivers/phy/ingenic/Kconfig
>>> @@ -0,0 +1,12 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Phy drivers for Ingenic platforms
>>> +#
>>> +config PHY_INGENIC_USB
>>> +    tristate "Ingenic SoCs USB PHY Driver"
>>> +    depends on (MACH_INGENIC && MIPS) || COMPILE_TEST
>> 
>> The original driver depends on MIPS || COMPILE_TEST, so you should 
>> do \x7fthe same, otherwise you change more than what the patch 
>> description \x7fsuggests.
>> 
> 
> Sure, I will change it in the next version.
> 
>>> +    depends on USB_SUPPORT
>>> +    select GENERIC_PHY
>>> +    help
>>> +      This driver provides USB PHY support for the USB controller 
>>> found
>>> +      on the JZ-series and X-series SoCs from Ingenic.
>>> diff --git a/drivers/phy/ingenic/Makefile 
>>> b/drivers/phy/ingenic/Makefile
>>> new file mode 100644
>>> index 000000000000..65d5ea00fc9d
>>> --- /dev/null
>>> +++ b/drivers/phy/ingenic/Makefile
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +obj-y        += phy-ingenic-usb.o
>>> diff --git a/drivers/usb/phy/phy-jz4770.c 
>>> \x7f\x7fb/drivers/phy/ingenic/phy-ingenic-usb.c
>>> similarity index 63%
>>> rename from drivers/usb/phy/phy-jz4770.c
>>> rename to drivers/phy/ingenic/phy-ingenic-usb.c
>>> index f6d3731581eb..86a95b498785 100644
>>> --- a/drivers/usb/phy/phy-jz4770.c
>>> +++ b/drivers/phy/ingenic/phy-ingenic-usb.c
>>> @@ -7,12 +7,12 @@
>>>   */
>>> 
>>>  #include <linux/clk.h>
>>> +#include <linux/delay.h>
>>>  #include <linux/io.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regulator/consumer.h>
>>> -#include <linux/usb/otg.h>
>>> -#include <linux/usb/phy.h>
>>> +#include <linux/phy/phy.h>
>>> 
>>>  /* OTGPHY register offsets */
>>>  #define REG_USBPCR_OFFSET            0x00
>>> @@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
>>>  struct ingenic_soc_info {
>>>      enum ingenic_usb_phy_version version;
>>> 
>>> -    void (*usb_phy_init)(struct usb_phy *phy);
>>> +    void (*usb_phy_init)(struct phy *phy);
>>>  };
>>> 
>>> -struct jz4770_phy {
>>> +struct ingenic_usb_phy {
>>>      const struct ingenic_soc_info *soc_info;
>>> 
>>> -    struct usb_phy phy;
>>> -    struct usb_otg otg;
>>> +    struct phy *phy;
>>>      struct device *dev;
>>>      void __iomem *base;
>>>      struct clk *clk;
>>>      struct regulator *vcc_supply;
>>>  };
>>> 
>>> -static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg 
>>> *otg)
>>> +static int ingenic_usb_phy_init(struct phy *phy)
>>>  {
>>> -    return container_of(otg, struct jz4770_phy, otg);
>>> -}
>>> -
>>> -static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy 
>>> *phy)
>>> -{
>>> -    return container_of(phy, struct jz4770_phy, phy);
>>> -}
>>> -
>>> -static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
>>> -                     struct usb_gadget *gadget)
>>> -{
>>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>>> -    u32 reg;
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>> +    int err;
>>> 
>>> -    if (priv->soc_info->version >= ID_X1000) {
>>> -        reg = readl(priv->base + REG_USBPCR1_OFFSET);
>>> -        reg |= USBPCR1_BVLD_REG;
>>> -        writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>> +    err = clk_prepare_enable(priv->clk);
>>> +    if (err) {
>>> +        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>>> +        return err;
>>>      }
>>> 
>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> -    reg &= ~USBPCR_USB_MODE;
>>> -    reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>> \x7f\x7fUSBPCR_OTG_DISABLE;
>>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +    priv->soc_info->usb_phy_init(phy);
>>> 
>>>      return 0;
>>>  }
>>> 
>>> -static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct 
>>> \x7f\x7fusb_bus *host)
>>> +static int ingenic_usb_phy_exit(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>>> -    u32 reg;
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>> 
>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> -    reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>> \x7f\x7fUSBPCR_OTG_DISABLE);
>>> -    reg |= USBPCR_USB_MODE;
>>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +    clk_disable_unprepare(priv->clk);
>>> +    regulator_disable(priv->vcc_supply);
>>> 
>>>      return 0;
>>>  }
>>> 
>>> -static int ingenic_usb_phy_init(struct usb_phy *phy)
>>> +static int ingenic_usb_phy_power_on(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>      int err;
>>> -    u32 reg;
>>> 
>>>      err = regulator_enable(priv->vcc_supply);
>>>      if (err) {
>>> @@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct 
>>> usb_phy \x7f\x7f*phy)
>>>          return err;
>>>      }
>>> 
>>> -    err = clk_prepare_enable(priv->clk);
>>> -    if (err) {
>>> -        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>>> -        return err;
>>> -    }
>>> -
>>> -    priv->soc_info->usb_phy_init(phy);
>>> -
>>> -    /* Wait for PHY to reset */
>>> -    usleep_range(30, 300);
>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> -    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>> -    usleep_range(300, 1000);
>>> -
>>>      return 0;
>>>  }
>>> 
>>> -static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
>>> +static int ingenic_usb_phy_power_off(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>> 
>>> -    clk_disable_unprepare(priv->clk);
>>>      regulator_disable(priv->vcc_supply);
>>> +
>>> +    return 0;
>>>  }
>>> 
>>> -static void ingenic_usb_phy_remove(void *phy)
>>> +static int ingenic_usb_phy_set_mode(struct phy *phy,
>>> +                  enum phy_mode mode, int submode)
>>>  {
>>> -    usb_remove_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>> +    u32 reg;
>>> +
>>> +    switch (mode) {
>>> +    case PHY_MODE_USB_HOST:
>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> +        reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>> \x7f\x7fUSBPCR_OTG_DISABLE);
>>> +        reg |= USBPCR_USB_MODE;
>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +        break;
>>> +    case PHY_MODE_USB_DEVICE:
>>> +        if (priv->soc_info->version >= ID_X1000) {
>>> +            reg = readl(priv->base + REG_USBPCR1_OFFSET);
>>> +            reg |= USBPCR1_BVLD_REG;
>>> +            writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>> +        }
>>> +
>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> +        reg &= ~USBPCR_USB_MODE;
>>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>> \x7f\x7fUSBPCR_OTG_DISABLE;
>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +        break;
>>> +    case PHY_MODE_USB_OTG:
>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>> +        reg &= ~USBPCR_OTG_DISABLE;
>>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>> \x7f\x7fUSBPCR_USB_MODE;
>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>>  }
>> 
>> I think the diff should be a bit smaller (and easier to review) if 
>> you \x7fmove ingenic_usb_phy_init / ingenic_usb_phy_exit here, where 
>> they used \x7fto be.
>> 
> 
> Sure.
>>> 
>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>> +static const struct phy_ops ingenic_usb_phy_ops = {
>>> +    .init        = ingenic_usb_phy_init,
>>> +    .exit        = ingenic_usb_phy_exit,
>>> +    .power_on    = ingenic_usb_phy_power_on,
>>> +    .power_off    = ingenic_usb_phy_power_off,
>>> +    .set_mode    = ingenic_usb_phy_set_mode,
>>> +    .owner        = THIS_MODULE,
>>> +};
>>> +
>>> +static void jz4770_usb_phy_init(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>      u32 reg;
>>> 
>>>      reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | 
>>> USBPCR_IDPULLUP_ALWAYS |
>>> @@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct 
>>> usb_phy \x7f\x7f*phy)
>>>          USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | 
>>> \x7f\x7fUSBPCR_TXVREFTUNE_DFT |
>>>          USBPCR_POR;
>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +    /* Wait for PHY to reset */
>>> +    usleep_range(30, 300);
>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>> +    usleep_range(300, 1000);
>>>  }
>>> 
>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>> +static void jz4780_usb_phy_init(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>      u32 reg;
>>> 
>>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL 
>>> |
>>> @@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct 
>>> usb_phy \x7f\x7f*phy)
>>> 
>>>      reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +    /* Wait for PHY to reset */
>>> +    usleep_range(30, 300);
>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>> +    usleep_range(300, 1000);
>>>  }
>>> 
>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>> +static void x1000_usb_phy_init(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>      u32 reg;
>>> 
>>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
>>> \x7f\x7fUSBPCR1_WORD_IF_16BIT;
>>> @@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy 
>>> \x7f\x7f*phy)
>>>          USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>>          USBPCR_COMMONONN | USBPCR_POR;
>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +    /* Wait for PHY to reset */
>>> +    usleep_range(30, 300);
>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>> +    usleep_range(300, 1000);
>>>  }
>>> 
>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>> +static void x1830_usb_phy_init(struct phy *phy)
>>>  {
>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>      u32 reg;
>>> 
>>>      /* rdt */
>>> @@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy 
>>> *phy)
>>>      reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | 
>>> \x7f\x7fUSBPCR_TXPREEMPHTUNE |
>>>          USBPCR_COMMONONN | USBPCR_POR;
>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> +
>>> +    /* Wait for PHY to reset */
>>> +    usleep_range(30, 300);
>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>> +    usleep_range(300, 1000);
>> 
>> Why is that code repeated four times now? The old driver had that in 
>> \x7fingenic_usb_phy_init().
>> 
> 
> This is my fault, I forgot to make the corresponding changes after I 
> cherry-pick it from v6, I will fix this problem in the next version.
> 
>>>  }
>>> 
>>>  static const struct ingenic_soc_info jz4770_soc_info = {
>>> @@ -276,87 +309,96 @@ static const struct ingenic_soc_info 
>>> \x7f\x7fx1830_soc_info = {
>>>      .usb_phy_init = x1830_usb_phy_init,
>>>  };
>>> 
>>> -static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>>> -    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info 
>>> },
>>> -    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info 
>>> },
>>> -    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>>> -    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>>> -    { /* sentinel */ }
>>> -};
>>> -MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>>> -
>>> -static int jz4770_phy_probe(struct platform_device *pdev)
>>> +static int ingenic_usb_phy_probe(struct platform_device *pdev)
>>>  {
>>> -    struct device *dev = &pdev->dev;
>>> -    struct jz4770_phy *priv;
>>> +    struct ingenic_usb_phy *priv;
>>> +    struct phy_provider *provider;
>>>      int err;
>>> 
>>> -    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +    priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> 
>> I'd prefer that you keep a local 'dev' variable. Otherwise it only 
>> \x7fmakes the diff bigger and it's harder to review.
>> 
> 
> Sure.
> 
>>>      if (!priv)
>>>          return -ENOMEM;
>>> 
>>> +    priv->dev = &pdev->dev;
>>> +
>>>      priv->soc_info = device_get_match_data(&pdev->dev);
>>>      if (!priv->soc_info) {
>>>          dev_err(&pdev->dev, "Error: No device match found\n");
>>>          return -ENODEV;
>>>      }
>>> 
>>> -    platform_set_drvdata(pdev, priv);
>>> -    priv->dev = dev;
>>> -    priv->phy.dev = dev;
>>> -    priv->phy.otg = &priv->otg;
>>> -    priv->phy.label = "ingenic-usb-phy";
>>> -    priv->phy.init = ingenic_usb_phy_init;
>>> -    priv->phy.shutdown = ingenic_usb_phy_shutdown;
>>> -
>>> -    priv->otg.state = OTG_STATE_UNDEFINED;
>>> -    priv->otg.usb_phy = &priv->phy;
>>> -    priv->otg.set_host = ingenic_usb_phy_set_host;
>>> -    priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
>>> -
>>>      priv->base = devm_platform_ioremap_resource(pdev, 0);
>>>      if (IS_ERR(priv->base)) {
>>> -        dev_err(dev, "Failed to map registers\n");
>>> +        dev_err(priv->dev, "Failed to map registers\n");
>>>          return PTR_ERR(priv->base);
>>>      }
>>> 
>>> -    priv->clk = devm_clk_get(dev, NULL);
>>> +    priv->clk = devm_clk_get(priv->dev, NULL);
>>>      if (IS_ERR(priv->clk)) {
>>>          err = PTR_ERR(priv->clk);
>>>          if (err != -EPROBE_DEFER)
>>> -            dev_err(dev, "Failed to get clock\n");
>>> +            dev_err(priv->dev, "Failed to get clock\n");
>>>          return err;
>>>      }
>>> 
>>> -    priv->vcc_supply = devm_regulator_get(dev, "vcc");
>>> +    priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
>>>      if (IS_ERR(priv->vcc_supply)) {
>>>          err = PTR_ERR(priv->vcc_supply);
>>>          if (err != -EPROBE_DEFER)
>>> -            dev_err(dev, "Failed to get regulator\n");
>>> +            dev_err(priv->dev, "Failed to get regulator\n");
>>>          return err;
>>>      }
>>> 
>>> -    err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
>>> -    if (err) {
>>> -        if (err != -EPROBE_DEFER)
>>> -            dev_err(dev, "Unable to register PHY\n");
>>> -        return err;
>>> +    priv->phy = devm_phy_create(priv->dev, NULL, 
>>> &ingenic_usb_phy_ops);
>>> +    if (IS_ERR(priv)) {
>>> +        dev_err(priv->dev, "Failed to create PHY: %ld\n", 
>>> \x7f\x7fPTR_ERR(priv));
>>> +        return PTR_ERR(priv);
>>> +    }
>> 
>> There's a stray tabulation character here.
>> 
>> Also, no need to print error codes in the probe function - they will 
>> \x7fbe printed anyway since the driver will fail to probe.
>> 
> Sure.
>>> +
>>> +    provider = devm_of_phy_provider_register(priv->dev, 
>>> \x7f\x7fof_phy_simple_xlate);
>>> +    if (IS_ERR(provider)) {
>>> +        dev_err(priv->dev, "Failed to register PHY provider: 
>>> %ld\n", \x7f\x7fPTR_ERR(provider));
>>> +        return PTR_ERR(provider);
>>>      }
>> 
>> Same here.
>> 
>>> 
>>> -    return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, 
>>> \x7f\x7f&priv->phy);
>>> +    platform_set_drvdata(pdev, priv);
>>> +    phy_set_drvdata(priv->phy, priv);
>> 
>> These two do the same thing. Also, you must do it before registering 
>> \x7fthe PHY, otherwise you have a race.
>> 
> OK, I will fix it in the next version.
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ingenic_usb_phy_remove(struct platform_device *pdev)
>>> +{
>>> +    struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
>>> +
>>> +    clk_disable_unprepare(priv->clk);
>>> +    regulator_disable(priv->vcc_supply);
>> 
>> I assume that ingenic_usb_phy_power_off() and ingenic_usb_phy_exit() 
>> \x7fare automatically called when the module is removed, did you test 
>> \x7fmodule removal?
>> 
> 
> I think I have an oversignt, only the module install was tested, but 
> the module removal was not tested.
> 
>>> +
>>> +    return 0;
>>>  }
>>> 
>>> -static struct platform_driver ingenic_phy_driver = {
>>> -    .probe        = jz4770_phy_probe,
>>> +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>>> +    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info 
>>> },
>>> +    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info 
>>> },
>>> +    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>>> +    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>>> +    { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>> 
>> You moved that code around, which only made the diff bigger and 
>> harder \x7fto review. Please keep it where it was.
>> 
> 
> Sure.
> 
>>> +
>>> +static struct platform_driver ingenic_usb_phy_driver = {
>>> +    .probe        = ingenic_usb_phy_probe,
>>> +    .remove        = ingenic_usb_phy_remove,
>>>      .driver        = {
>>> -        .name    = "jz4770-phy",
>>> -        .of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
>>> +        .name    = "ingenic-usb-phy",
>>> +        .of_match_table = ingenic_usb_phy_of_matches,
>> 
>> You removed of_match_ptr(), which is a valid change (Ingenic SoCs 
>> all \x7fdepend on Device Tree), but is unrelated to this patch.
>> 
> 
> It was not removed in the previous version, so the test robot sent me 
> an email. In this case, should I remove it directly herer or remove 
> it in a separate patch?

Separate patch please, before the move to the generic PHY.

Cheers,
-Paul

>>>      },
>>>  };
>>> -module_platform_driver(ingenic_phy_driver);
>>> +module_platform_driver(ingenic_usb_phy_driver);
>>> 
>>>  MODULE_AUTHOR("周琰杰 (Zhou Yanjie) 
>>> <zhouyanjie@wanyeetech.com>");
>>>  MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>");
>>>  MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>>  MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
>>> +MODULE_ALIAS("jz4770_phy");
>> 
>> Actually that would be "jz4770-phy".
>> 
> 
> Sure, I'll change it in the next version.
> 
> Thanks and best regards!
> 
>> Cheers,
>> -Paul
>> 
>>>  MODULE_LICENSE("GPL");
>>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>>> index ef4787cd3d37..ff24fca0a2d9 100644
>>> --- a/drivers/usb/phy/Kconfig
>>> +++ b/drivers/usb/phy/Kconfig
>>> @@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
>>>        Provides read/write operations to the ULPI phy register set 
>>> for
>>>        controllers with a viewport register (e.g. Chipidea/ARC 
>>> \x7f\x7fcontrollers).
>>> 
>>> -config JZ4770_PHY
>>> -    tristate "Ingenic SoCs Transceiver Driver"
>>> -    depends on MIPS || COMPILE_TEST
>>> -    select USB_PHY
>>> -    help
>>> -      This driver provides PHY support for the USB controller found
>>> -      on the JZ-series and X-series SoCs from Ingenic.
>>> -
>>>  endmenu
>>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>>> index b352bdbe8712..df1d99010079 100644
>>> --- a/drivers/usb/phy/Makefile
>>> +++ b/drivers/usb/phy/Makefile
>>> @@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)        += phy-mxs-usb.o
>>>  obj-$(CONFIG_USB_ULPI)            += phy-ulpi.o
>>>  obj-$(CONFIG_USB_ULPI_VIEWPORT)        += phy-ulpi-viewport.o
>>>  obj-$(CONFIG_KEYSTONE_USB_PHY)        += phy-keystone.o
>>> -obj-$(CONFIG_JZ4770_PHY)        += phy-jz4770.o
>>> --
>>> 2.11.0
>>> 
>> 

Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

[Zhou Yanjie]

Hi Paul,

在 2020/9/7 上午1:15, Paul Cercueil 写道:
>
>
> Le lun. 7 sept. 2020 à 1:06, Zhou Yanjie <zhouyanjie@wanyeetech.com> a 
> écrit :
>> Hi Paul,
>>
>> 在 2020/9/4 下午10:10, Paul Cercueil 写道:
>>> Hi Zhou,
>>>
>>> Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie) 
>>> \x7f<zhouyanjie@wanyeetech.com> a écrit :
>>>> Used the generic PHY framework API to create the PHY,
>>>> and move the driver to driver/phy/ingenic.
>>>>
>>>> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
>>>> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>>>> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     v1->v2:
>>>>     Fix bug, ".of_match_table = 
>>>> \x7f\x7fof_match_ptr(ingenic_usb_phy_of_matches)" is wrong
>>>>     and should be replaced with ".of_match_table = 
>>>> \x7f\x7fingenic_usb_phy_of_matches".
>>>>
>>>>  drivers/phy/Kconfig                                |   1 +
>>>>  drivers/phy/Makefile                               |   1 +
>>>>  drivers/phy/ingenic/Kconfig                        |  12 +
>>>>  drivers/phy/ingenic/Makefile                       |   2 +
>>>>  .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 
>>>> \x7f\x7f++++++++++++---------
>>>>  drivers/usb/phy/Kconfig                            |   8 -
>>>>  drivers/usb/phy/Makefile                           |   1 -
>>>>  7 files changed, 165 insertions(+), 116 deletions(-)
>>>>  create mode 100644 drivers/phy/ingenic/Kconfig
>>>>  create mode 100644 drivers/phy/ingenic/Makefile
>>>>  rename drivers/{usb/phy/phy-jz4770.c => 
>>>> \x7f\x7fphy/ingenic/phy-ingenic-usb.c} (63%)
>>>>
>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>>> index de9362c25c07..0534b0fdd057 100644
>>>> --- a/drivers/phy/Kconfig
>>>> +++ b/drivers/phy/Kconfig
>>>> @@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
>>>>  source "drivers/phy/cadence/Kconfig"
>>>>  source "drivers/phy/freescale/Kconfig"
>>>>  source "drivers/phy/hisilicon/Kconfig"
>>>> +source "drivers/phy/ingenic/Kconfig"
>>>>  source "drivers/phy/lantiq/Kconfig"
>>>>  source "drivers/phy/marvell/Kconfig"
>>>>  source "drivers/phy/mediatek/Kconfig"
>>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>>> index c27408e4daae..ab24f0d20763 100644
>>>> --- a/drivers/phy/Makefile
>>>> +++ b/drivers/phy/Makefile
>>>> @@ -14,6 +14,7 @@ obj-y                    += allwinner/ \
>>>>                         cadence/    \
>>>>                         freescale/    \
>>>>                         hisilicon/    \
>>>> +                       ingenic/    \
>>>>                         intel/    \
>>>>                         lantiq/    \
>>>>                         marvell/    \
>>>> diff --git a/drivers/phy/ingenic/Kconfig b/drivers/phy/ingenic/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..b9581eae89dd
>>>> --- /dev/null
>>>> +++ b/drivers/phy/ingenic/Kconfig
>>>> @@ -0,0 +1,12 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +#
>>>> +# Phy drivers for Ingenic platforms
>>>> +#
>>>> +config PHY_INGENIC_USB
>>>> +    tristate "Ingenic SoCs USB PHY Driver"
>>>> +    depends on (MACH_INGENIC && MIPS) || COMPILE_TEST
>>>
>>> The original driver depends on MIPS || COMPILE_TEST, so you should 
>>> do \x7fthe same, otherwise you change more than what the patch 
>>> description \x7fsuggests.
>>>
>>
>> Sure, I will change it in the next version.
>>
>>>> +    depends on USB_SUPPORT
>>>> +    select GENERIC_PHY
>>>> +    help
>>>> +      This driver provides USB PHY support for the USB controller 
>>>> found
>>>> +      on the JZ-series and X-series SoCs from Ingenic.
>>>> diff --git a/drivers/phy/ingenic/Makefile 
>>>> b/drivers/phy/ingenic/Makefile
>>>> new file mode 100644
>>>> index 000000000000..65d5ea00fc9d
>>>> --- /dev/null
>>>> +++ b/drivers/phy/ingenic/Makefile
>>>> @@ -0,0 +1,2 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +obj-y        += phy-ingenic-usb.o
>>>> diff --git a/drivers/usb/phy/phy-jz4770.c 
>>>> \x7f\x7fb/drivers/phy/ingenic/phy-ingenic-usb.c
>>>> similarity index 63%
>>>> rename from drivers/usb/phy/phy-jz4770.c
>>>> rename to drivers/phy/ingenic/phy-ingenic-usb.c
>>>> index f6d3731581eb..86a95b498785 100644
>>>> --- a/drivers/usb/phy/phy-jz4770.c
>>>> +++ b/drivers/phy/ingenic/phy-ingenic-usb.c
>>>> @@ -7,12 +7,12 @@
>>>>   */
>>>>
>>>>  #include <linux/clk.h>
>>>> +#include <linux/delay.h>
>>>>  #include <linux/io.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/regulator/consumer.h>
>>>> -#include <linux/usb/otg.h>
>>>> -#include <linux/usb/phy.h>
>>>> +#include <linux/phy/phy.h>
>>>>
>>>>  /* OTGPHY register offsets */
>>>>  #define REG_USBPCR_OFFSET            0x00
>>>> @@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
>>>>  struct ingenic_soc_info {
>>>>      enum ingenic_usb_phy_version version;
>>>>
>>>> -    void (*usb_phy_init)(struct usb_phy *phy);
>>>> +    void (*usb_phy_init)(struct phy *phy);
>>>>  };
>>>>
>>>> -struct jz4770_phy {
>>>> +struct ingenic_usb_phy {
>>>>      const struct ingenic_soc_info *soc_info;
>>>>
>>>> -    struct usb_phy phy;
>>>> -    struct usb_otg otg;
>>>> +    struct phy *phy;
>>>>      struct device *dev;
>>>>      void __iomem *base;
>>>>      struct clk *clk;
>>>>      struct regulator *vcc_supply;
>>>>  };
>>>>
>>>> -static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg 
>>>> *otg)
>>>> +static int ingenic_usb_phy_init(struct phy *phy)
>>>>  {
>>>> -    return container_of(otg, struct jz4770_phy, otg);
>>>> -}
>>>> -
>>>> -static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy 
>>>> *phy)
>>>> -{
>>>> -    return container_of(phy, struct jz4770_phy, phy);
>>>> -}
>>>> -
>>>> -static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
>>>> -                     struct usb_gadget *gadget)
>>>> -{
>>>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>>>> -    u32 reg;
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>> +    int err;
>>>>
>>>> -    if (priv->soc_info->version >= ID_X1000) {
>>>> -        reg = readl(priv->base + REG_USBPCR1_OFFSET);
>>>> -        reg |= USBPCR1_BVLD_REG;
>>>> -        writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>> +    err = clk_prepare_enable(priv->clk);
>>>> +    if (err) {
>>>> +        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>>>> +        return err;
>>>>      }
>>>>
>>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> -    reg &= ~USBPCR_USB_MODE;
>>>> -    reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>>> \x7f\x7fUSBPCR_OTG_DISABLE;
>>>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +    priv->soc_info->usb_phy_init(phy);
>>>>
>>>>      return 0;
>>>>  }
>>>>
>>>> -static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct 
>>>> \x7f\x7fusb_bus *host)
>>>> +static int ingenic_usb_phy_exit(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
>>>> -    u32 reg;
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>
>>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> -    reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>>> \x7f\x7fUSBPCR_OTG_DISABLE);
>>>> -    reg |= USBPCR_USB_MODE;
>>>> -    writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +    clk_disable_unprepare(priv->clk);
>>>> +    regulator_disable(priv->vcc_supply);
>>>>
>>>>      return 0;
>>>>  }
>>>>
>>>> -static int ingenic_usb_phy_init(struct usb_phy *phy)
>>>> +static int ingenic_usb_phy_power_on(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>      int err;
>>>> -    u32 reg;
>>>>
>>>>      err = regulator_enable(priv->vcc_supply);
>>>>      if (err) {
>>>> @@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct 
>>>> usb_phy \x7f\x7f*phy)
>>>>          return err;
>>>>      }
>>>>
>>>> -    err = clk_prepare_enable(priv->clk);
>>>> -    if (err) {
>>>> -        dev_err(priv->dev, "Unable to start clock: %d\n", err);
>>>> -        return err;
>>>> -    }
>>>> -
>>>> -    priv->soc_info->usb_phy_init(phy);
>>>> -
>>>> -    /* Wait for PHY to reset */
>>>> -    usleep_range(30, 300);
>>>> -    reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> -    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>>> -    usleep_range(300, 1000);
>>>> -
>>>>      return 0;
>>>>  }
>>>>
>>>> -static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
>>>> +static int ingenic_usb_phy_power_off(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>
>>>> -    clk_disable_unprepare(priv->clk);
>>>>      regulator_disable(priv->vcc_supply);
>>>> +
>>>> +    return 0;
>>>>  }
>>>>
>>>> -static void ingenic_usb_phy_remove(void *phy)
>>>> +static int ingenic_usb_phy_set_mode(struct phy *phy,
>>>> +                  enum phy_mode mode, int submode)
>>>>  {
>>>> -    usb_remove_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>> +    u32 reg;
>>>> +
>>>> +    switch (mode) {
>>>> +    case PHY_MODE_USB_HOST:
>>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> +        reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>>> \x7f\x7fUSBPCR_OTG_DISABLE);
>>>> +        reg |= USBPCR_USB_MODE;
>>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +        break;
>>>> +    case PHY_MODE_USB_DEVICE:
>>>> +        if (priv->soc_info->version >= ID_X1000) {
>>>> +            reg = readl(priv->base + REG_USBPCR1_OFFSET);
>>>> +            reg |= USBPCR1_BVLD_REG;
>>>> +            writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>> +        }
>>>> +
>>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> +        reg &= ~USBPCR_USB_MODE;
>>>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>>> \x7f\x7fUSBPCR_OTG_DISABLE;
>>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +        break;
>>>> +    case PHY_MODE_USB_OTG:
>>>> +        reg = readl(priv->base + REG_USBPCR_OFFSET);
>>>> +        reg &= ~USBPCR_OTG_DISABLE;
>>>> +        reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
>>>> \x7f\x7fUSBPCR_USB_MODE;
>>>> +        writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +        break;
>>>> +    default:
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>>  }
>>>
>>> I think the diff should be a bit smaller (and easier to review) if 
>>> you \x7fmove ingenic_usb_phy_init / ingenic_usb_phy_exit here, where 
>>> they used \x7fto be.
>>>
>>
>> Sure.
>>>>
>>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>>> +static const struct phy_ops ingenic_usb_phy_ops = {
>>>> +    .init        = ingenic_usb_phy_init,
>>>> +    .exit        = ingenic_usb_phy_exit,
>>>> +    .power_on    = ingenic_usb_phy_power_on,
>>>> +    .power_off    = ingenic_usb_phy_power_off,
>>>> +    .set_mode    = ingenic_usb_phy_set_mode,
>>>> +    .owner        = THIS_MODULE,
>>>> +};
>>>> +
>>>> +static void jz4770_usb_phy_init(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>      u32 reg;
>>>>
>>>>      reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | 
>>>> USBPCR_IDPULLUP_ALWAYS |
>>>> @@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct 
>>>> usb_phy \x7f\x7f*phy)
>>>>          USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | 
>>>> \x7f\x7fUSBPCR_TXVREFTUNE_DFT |
>>>>          USBPCR_POR;
>>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +    /* Wait for PHY to reset */
>>>> +    usleep_range(30, 300);
>>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>>> +    usleep_range(300, 1000);
>>>>  }
>>>>
>>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>>> +static void jz4780_usb_phy_init(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>      u32 reg;
>>>>
>>>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL |
>>>> @@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct 
>>>> usb_phy \x7f\x7f*phy)
>>>>
>>>>      reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +    /* Wait for PHY to reset */
>>>> +    usleep_range(30, 300);
>>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>>> +    usleep_range(300, 1000);
>>>>  }
>>>>
>>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>>> +static void x1000_usb_phy_init(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>      u32 reg;
>>>>
>>>>      reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
>>>> \x7f\x7fUSBPCR1_WORD_IF_16BIT;
>>>> @@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy 
>>>> \x7f\x7f*phy)
>>>>          USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>>>          USBPCR_COMMONONN | USBPCR_POR;
>>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +    /* Wait for PHY to reset */
>>>> +    usleep_range(30, 300);
>>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>>> +    usleep_range(300, 1000);
>>>>  }
>>>>
>>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>>> +static void x1830_usb_phy_init(struct phy *phy)
>>>>  {
>>>> -    struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> +    struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>>>>      u32 reg;
>>>>
>>>>      /* rdt */
>>>> @@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy 
>>>> *phy)
>>>>      reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | 
>>>> \x7f\x7fUSBPCR_TXPREEMPHTUNE |
>>>>          USBPCR_COMMONONN | USBPCR_POR;
>>>>      writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> +
>>>> +    /* Wait for PHY to reset */
>>>> +    usleep_range(30, 300);
>>>> +    writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
>>>> +    usleep_range(300, 1000);
>>>
>>> Why is that code repeated four times now? The old driver had that in 
>>> \x7fingenic_usb_phy_init().
>>>
>>
>> This is my fault, I forgot to make the corresponding changes after I 
>> cherry-pick it from v6, I will fix this problem in the next version.
>>
>>>>  }
>>>>
>>>>  static const struct ingenic_soc_info jz4770_soc_info = {
>>>> @@ -276,87 +309,96 @@ static const struct ingenic_soc_info 
>>>> \x7f\x7fx1830_soc_info = {
>>>>      .usb_phy_init = x1830_usb_phy_init,
>>>>  };
>>>>
>>>> -static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>>>> -    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
>>>> -    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
>>>> -    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>>>> -    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>>>> -    { /* sentinel */ }
>>>> -};
>>>> -MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>>>> -
>>>> -static int jz4770_phy_probe(struct platform_device *pdev)
>>>> +static int ingenic_usb_phy_probe(struct platform_device *pdev)
>>>>  {
>>>> -    struct device *dev = &pdev->dev;
>>>> -    struct jz4770_phy *priv;
>>>> +    struct ingenic_usb_phy *priv;
>>>> +    struct phy_provider *provider;
>>>>      int err;
>>>>
>>>> -    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +    priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>
>>> I'd prefer that you keep a local 'dev' variable. Otherwise it only 
>>> \x7fmakes the diff bigger and it's harder to review.
>>>
>>
>> Sure.
>>
>>>>      if (!priv)
>>>>          return -ENOMEM;
>>>>
>>>> +    priv->dev = &pdev->dev;
>>>> +
>>>>      priv->soc_info = device_get_match_data(&pdev->dev);
>>>>      if (!priv->soc_info) {
>>>>          dev_err(&pdev->dev, "Error: No device match found\n");
>>>>          return -ENODEV;
>>>>      }
>>>>
>>>> -    platform_set_drvdata(pdev, priv);
>>>> -    priv->dev = dev;
>>>> -    priv->phy.dev = dev;
>>>> -    priv->phy.otg = &priv->otg;
>>>> -    priv->phy.label = "ingenic-usb-phy";
>>>> -    priv->phy.init = ingenic_usb_phy_init;
>>>> -    priv->phy.shutdown = ingenic_usb_phy_shutdown;
>>>> -
>>>> -    priv->otg.state = OTG_STATE_UNDEFINED;
>>>> -    priv->otg.usb_phy = &priv->phy;
>>>> -    priv->otg.set_host = ingenic_usb_phy_set_host;
>>>> -    priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
>>>> -
>>>>      priv->base = devm_platform_ioremap_resource(pdev, 0);
>>>>      if (IS_ERR(priv->base)) {
>>>> -        dev_err(dev, "Failed to map registers\n");
>>>> +        dev_err(priv->dev, "Failed to map registers\n");
>>>>          return PTR_ERR(priv->base);
>>>>      }
>>>>
>>>> -    priv->clk = devm_clk_get(dev, NULL);
>>>> +    priv->clk = devm_clk_get(priv->dev, NULL);
>>>>      if (IS_ERR(priv->clk)) {
>>>>          err = PTR_ERR(priv->clk);
>>>>          if (err != -EPROBE_DEFER)
>>>> -            dev_err(dev, "Failed to get clock\n");
>>>> +            dev_err(priv->dev, "Failed to get clock\n");
>>>>          return err;
>>>>      }
>>>>
>>>> -    priv->vcc_supply = devm_regulator_get(dev, "vcc");
>>>> +    priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
>>>>      if (IS_ERR(priv->vcc_supply)) {
>>>>          err = PTR_ERR(priv->vcc_supply);
>>>>          if (err != -EPROBE_DEFER)
>>>> -            dev_err(dev, "Failed to get regulator\n");
>>>> +            dev_err(priv->dev, "Failed to get regulator\n");
>>>>          return err;
>>>>      }
>>>>
>>>> -    err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
>>>> -    if (err) {
>>>> -        if (err != -EPROBE_DEFER)
>>>> -            dev_err(dev, "Unable to register PHY\n");
>>>> -        return err;
>>>> +    priv->phy = devm_phy_create(priv->dev, NULL, 
>>>> &ingenic_usb_phy_ops);
>>>> +    if (IS_ERR(priv)) {
>>>> +        dev_err(priv->dev, "Failed to create PHY: %ld\n", 
>>>> \x7f\x7fPTR_ERR(priv));
>>>> +        return PTR_ERR(priv);
>>>> +    }
>>>
>>> There's a stray tabulation character here.
>>>
>>> Also, no need to print error codes in the probe function - they will 
>>> \x7fbe printed anyway since the driver will fail to probe.
>>>
>> Sure.
>>>> +
>>>> +    provider = devm_of_phy_provider_register(priv->dev, 
>>>> \x7f\x7fof_phy_simple_xlate);
>>>> +    if (IS_ERR(provider)) {
>>>> +        dev_err(priv->dev, "Failed to register PHY provider: 
>>>> %ld\n", \x7f\x7fPTR_ERR(provider));
>>>> +        return PTR_ERR(provider);
>>>>      }
>>>
>>> Same here.
>>>
>>>>
>>>> -    return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, 
>>>> \x7f\x7f&priv->phy);
>>>> +    platform_set_drvdata(pdev, priv);
>>>> +    phy_set_drvdata(priv->phy, priv);
>>>
>>> These two do the same thing. Also, you must do it before registering 
>>> \x7fthe PHY, otherwise you have a race.
>>>
>> OK, I will fix it in the next version.
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ingenic_usb_phy_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
>>>> +
>>>> +    clk_disable_unprepare(priv->clk);
>>>> +    regulator_disable(priv->vcc_supply);
>>>
>>> I assume that ingenic_usb_phy_power_off() and ingenic_usb_phy_exit() 
>>> \x7fare automatically called when the module is removed, did you test 
>>> \x7fmodule removal?
>>>
>>
>> I think I have an oversignt, only the module install was tested, but 
>> the module removal was not tested.
>>
>>>> +
>>>> +    return 0;
>>>>  }
>>>>
>>>> -static struct platform_driver ingenic_phy_driver = {
>>>> -    .probe        = jz4770_phy_probe,
>>>> +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
>>>> +    { .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
>>>> +    { .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
>>>> +    { .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
>>>> +    { .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
>>>> +    { /* sentinel */ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
>>>
>>> You moved that code around, which only made the diff bigger and 
>>> harder \x7fto review. Please keep it where it was.
>>>
>>
>> Sure.
>>
>>>> +
>>>> +static struct platform_driver ingenic_usb_phy_driver = {
>>>> +    .probe        = ingenic_usb_phy_probe,
>>>> +    .remove        = ingenic_usb_phy_remove,
>>>>      .driver        = {
>>>> -        .name    = "jz4770-phy",
>>>> -        .of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
>>>> +        .name    = "ingenic-usb-phy",
>>>> +        .of_match_table = ingenic_usb_phy_of_matches,
>>>
>>> You removed of_match_ptr(), which is a valid change (Ingenic SoCs 
>>> all \x7fdepend on Device Tree), but is unrelated to this patch.
>>>
>>
>> It was not removed in the previous version, so the test robot sent me 
>> an email. In this case, should I remove it directly herer or remove 
>> it in a separate patch?
>
> Separate patch please, before the move to the generic PHY.
>
Sure.
> Cheers,
> -Paul
>
>>>>      },
>>>>  };
>>>> -module_platform_driver(ingenic_phy_driver);
>>>> +module_platform_driver(ingenic_usb_phy_driver);
>>>>
>>>>  MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
>>>>  MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>");
>>>>  MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>>>  MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
>>>> +MODULE_ALIAS("jz4770_phy");
>>>
>>> Actually that would be "jz4770-phy".
>>>
>>
>> Sure, I'll change it in the next version.
>>
>> Thanks and best regards!
>>
>>> Cheers,
>>> -Paul
>>>
>>>>  MODULE_LICENSE("GPL");
>>>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>>>> index ef4787cd3d37..ff24fca0a2d9 100644
>>>> --- a/drivers/usb/phy/Kconfig
>>>> +++ b/drivers/usb/phy/Kconfig
>>>> @@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
>>>>        Provides read/write operations to the ULPI phy register set for
>>>>        controllers with a viewport register (e.g. Chipidea/ARC 
>>>> \x7f\x7fcontrollers).
>>>>
>>>> -config JZ4770_PHY
>>>> -    tristate "Ingenic SoCs Transceiver Driver"
>>>> -    depends on MIPS || COMPILE_TEST
>>>> -    select USB_PHY
>>>> -    help
>>>> -      This driver provides PHY support for the USB controller found
>>>> -      on the JZ-series and X-series SoCs from Ingenic.
>>>> -
>>>>  endmenu
>>>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>>>> index b352bdbe8712..df1d99010079 100644
>>>> --- a/drivers/usb/phy/Makefile
>>>> +++ b/drivers/usb/phy/Makefile
>>>> @@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)        += phy-mxs-usb.o
>>>>  obj-$(CONFIG_USB_ULPI)            += phy-ulpi.o
>>>>  obj-$(CONFIG_USB_ULPI_VIEWPORT)        += phy-ulpi-viewport.o
>>>>  obj-$(CONFIG_KEYSTONE_USB_PHY)        += phy-keystone.o
>>>> -obj-$(CONFIG_JZ4770_PHY)        += phy-jz4770.o
>>>> -- 
>>>> 2.11.0
>>>>
>>>
>