Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.
From: Paul Cercueil
Date: Sun Sep 06 2020 - 13:15:42 EST
Le lun. 7 sept. 2020 à 1:06, Zhou Yanjie <zhouyanjie@xxxxxxxxxxxxxx> a
écrit :
Hi Paul,
在 2020/9/4 下午10:10, Paul Cercueil 写道:
Hi Zhou,
Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie)
<zhouyanjie@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
---
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?
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@xxxxxxxxxxxxxx>");
MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>");
MODULE_AUTHOR("Paul Cercueil <paul@xxxxxxxxxxxxxxx>");
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