Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Fri, 04 Sep 2020 16:10:42 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework. |
| |
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 >
| ![\](/images/icornerr.gif) |