| Message ID | 20200831135046.54460-2-zhouyanjie@wanyeetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use the generic PHY framework for Ingenic USB PHY. | expand |
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 >
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 >> >
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) >> <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? 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 >>> 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 >>> >>
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) >>> <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? > > 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 >>>> 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 >>>> >>> >
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