* [PATCH v2 0/1] Use the generic PHY framework for Ingenic USB PHY.
@ 2020-08-31 13:50 周琰杰 (Zhou Yanjie)
2020-08-31 13:50 ` [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework 周琰杰 (Zhou Yanjie)
0 siblings, 1 reply; 6+ messages in thread
From: 周琰杰 (Zhou Yanjie) @ 2020-08-31 13:50 UTC (permalink / raw)
To: kishon, vkoul, balbi, gregkh
Cc: linux-usb, linux-kernel, paul, christophe.jaillet, aric.pzqi,
dongsheng.qiu, rick.tyliu, yanfei.li, sernia.zhou, zhenwenjin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
2020-08-31 13:50 [PATCH v2 0/1] Use the generic PHY framework for Ingenic USB PHY 周琰杰 (Zhou Yanjie)
@ 2020-08-31 13:50 ` 周琰杰 (Zhou Yanjie)
2020-09-04 14:10 ` Paul Cercueil
0 siblings, 1 reply; 6+ messages in thread
From: 周琰杰 (Zhou Yanjie) @ 2020-08-31 13:50 UTC (permalink / raw)
To: kishon, vkoul, balbi, gregkh
Cc: linux-usb, linux-kernel, paul, christophe.jaillet, aric.pzqi,
dongsheng.qiu, rick.tyliu, yanfei.li, sernia.zhou, zhenwenjin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
2020-08-31 13:50 ` [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework 周琰杰 (Zhou Yanjie)
@ 2020-09-04 14:10 ` Paul Cercueil
2020-09-06 17:06 ` Zhou Yanjie
0 siblings, 1 reply; 6+ messages in thread
From: Paul Cercueil @ 2020-09-04 14:10 UTC (permalink / raw)
To: 周琰杰
Cc: kishon, vkoul, balbi, gregkh, linux-usb, linux-kernel,
christophe.jaillet, aric.pzqi, dongsheng.qiu, rick.tyliu,
yanfei.li, sernia.zhou, zhenwenjin
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
2020-09-04 14:10 ` Paul Cercueil
@ 2020-09-06 17:06 ` Zhou Yanjie
2020-09-06 17:15 ` Paul Cercueil
0 siblings, 1 reply; 6+ messages in thread
From: Zhou Yanjie @ 2020-09-06 17:06 UTC (permalink / raw)
To: Paul Cercueil
Cc: kishon, vkoul, balbi, gregkh, linux-usb, linux-kernel,
christophe.jaillet, aric.pzqi, dongsheng.qiu, rick.tyliu,
yanfei.li, sernia.zhou, zhenwenjin
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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
2020-09-06 17:06 ` Zhou Yanjie
@ 2020-09-06 17:15 ` Paul Cercueil
2020-09-06 17:20 ` Zhou Yanjie
0 siblings, 1 reply; 6+ messages in thread
From: Paul Cercueil @ 2020-09-06 17:15 UTC (permalink / raw)
To: Zhou Yanjie
Cc: kishon, vkoul, balbi, gregkh, linux-usb, linux-kernel,
christophe.jaillet, aric.pzqi, dongsheng.qiu, rick.tyliu,
yanfei.li, sernia.zhou, zhenwenjin
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
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
2020-09-06 17:15 ` Paul Cercueil
@ 2020-09-06 17:20 ` Zhou Yanjie
0 siblings, 0 replies; 6+ messages in thread
From: Zhou Yanjie @ 2020-09-06 17:20 UTC (permalink / raw)
To: Paul Cercueil
Cc: kishon, vkoul, balbi, gregkh, linux-usb, linux-kernel,
christophe.jaillet, aric.pzqi, dongsheng.qiu, rick.tyliu,
yanfei.li, sernia.zhou, zhenwenjin
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
>>>>
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-06 17:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31 13:50 [PATCH v2 0/1] Use the generic PHY framework for Ingenic USB PHY 周琰杰 (Zhou Yanjie)
2020-08-31 13:50 ` [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework 周琰杰 (Zhou Yanjie)
2020-09-04 14:10 ` Paul Cercueil
2020-09-06 17:06 ` Zhou Yanjie
2020-09-06 17:15 ` Paul Cercueil
2020-09-06 17:20 ` Zhou Yanjie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.