On Mon, Apr 25, 2022 at 01:01:10PM -0400, Sean Anderson wrote:
> Hi Jules,
>
> On 4/21/22 12:21 PM, Jules Maselbas wrote:
> > I recently encountered some limitations when using the usb-nop-xceiv
> > driver using the usb-phy subsystem. The situation is: I have a board
> > with two external usb phys (ULPI) that only needs to be reset by the
> > USB controller. However both phys shares the same reset line, which
> > will get toggled twice by the USB driver, once by controller.
> >
> > This is what I observed with the current Linux usb-nop-xceiv driver:
> > _______ ___ _________
> > RESET: \__________________________/ \____/
> > 1^ 2^ 3^ 4^
> > 1. and 2. first controller toggle the reset
> > 3. and 4. second controller toggle the reset
> >
> > After the second toggle the first driver didn't expected the reset and
> > causing issues, but I don't recall what exactly, it is maybe a driver
> > that simply fail to get probed.
>
> Ah, a tale as old as GPIO resets... (c.f. MDIO)
I didn't get it :]
>
> > The following is what I observed when using the "usb-nop-xceiv"
> > driver in Barebox, which is a bit different than the Linux one.
> >
> > _______ _________________
> > RESET: \__________________________/
> > 1^ 2^
> >
> > In this case the reset is only toggled once, this is because the phy
> > driver count the number of time it has called init and power_on, which
> > is exactly what is done by the generic phy subsystem in Linux, but not
> > by the "legacy" usb-nop-xceiv usb-phy driver.
>
> Can you provide a snippet of your device tree? I am not quite sure how
> you are describing your phys. Do you use the same device for multiple USBs?
Yes I use the same phy "device" for multiple USBs, like so:
/ {
usb_phy0: usb-phy0 {
#phy-cells = <0>;
compatible = "usb-nop-xceiv";
reset-gpios = <&gpio1_banka 21 GPIO_ACTIVE_LOW>;
};
};
&usb0 {
dr_mode = "peripheral";
phys = <&usb_phy0>;
phy-names = "usb2-phy";
};
&usb1 {
phys = <&usb_phy0>;
phy-names = "usb2-phy";
};
>
> > This difference in behavior is what motivated to "port" the existing
> > usb-nop-xceiv driver to the generic phy subsystem. However this is far
> > from being complete... I've only tested the reset function for this nop
> > phy, and it lack a lot of features from the original driver to be a
> > valid replacement for it.
> >
> > I would like to know if there is an interest in converting the current
> > usb-nop-xceiv driver to the generic phy subsystem?
> >
> > I would like to keep the usb-nop-xceiv compatible as it used by Barebox
> > and it works OK. If a new compatible is added then device-tree nodes
> > shouldn't have both, the new and usb-nop-xceiv, compatible because of
> > the specific check in phy-core.c which will always return ENODEV.
> >
> > Signed-off-by: Jules Maselbas <jmaselbas@xxxxxxxxx>
> > ---
> > drivers/phy/Makefile | 1 +
> > drivers/phy/phy-core.c | 4 -
> > drivers/phy/phy-usb-generic.c | 241 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 242 insertions(+), 4 deletions(-)
> > create mode 100644 drivers/phy/phy-usb-generic.c
>
> Since this is a conversion, I would expect that the old driver would be
> deleted. In particular, the Kconfig will need to be adjusted so that
> users of the old driver will have CONFIG_GENERIC_PHY enabled (and some
> reimplementation of usb_phy_gen_create_phy). I think we would still need
Of course, if this is a conversion i would also remove the old driver,
but I didn't wanted to cluter the diff... this is very much an RFC,
asking for advices.
> to call usb_add_phy_dev in probe() as well.
Ok, I'll look into that.
> Though I suppose this is why this patch is RFC?
Yes, this is why.
>
> >
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 6eb2916773c5..8eeecf6d6f74 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -4,6 +4,7 @@
> > #
> >
> > obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> > +obj-$(CONFIG_GENERIC_PHY) += phy-usb-generic.o
> > obj-$(CONFIG_GENERIC_PHY_MIPI_DPHY) += phy-core-mipi-dphy.o
> > obj-$(CONFIG_PHY_LPC18XX_USB_OTG) += phy-lpc18xx-usb-otg.o
> > obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index 71cb10826326..e09fd1cb4455 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -506,10 +506,6 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
> > if (ret)
> > return ERR_PTR(-ENODEV);
> >
> > - /* This phy type handled by the usb-phy subsystem for now */
> > - if (of_device_is_compatible(args.np, "usb-nop-xceiv"))
> > - return ERR_PTR(-ENODEV);
> > -
> > mutex_lock(&phy_provider_mutex);
> > phy_provider = of_phy_provider_lookup(args.np);
> > if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner)) {
> > diff --git a/drivers/phy/phy-usb-generic.c b/drivers/phy/phy-usb-generic.c
> > new file mode 100644
> > index 000000000000..fdd05bbd98f2
> > --- /dev/null
> > +++ b/drivers/phy/phy-usb-generic.c
> > @@ -0,0 +1,241 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * derived from driver/usb/phy/phy-generic.c
> > + *
> > + * Generic USB PHY driver for all USB "nop" transceiver which are mostly
> > + * autonomous. This driver is derived from the usb-nop-xceiv driver, but
> > + * this one use the new generic phy api.
> > + *
> > + * Copyright (c) 2022 Kalray Inc.
> > + * Author(s): Jules Maselbas
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> > +#include <linux/usb/otg.h>
> > +
> > +struct phy_usb_generic {
> > + struct device *dev;
> > + struct clk *clk;
> > + struct regulator *vcc;
> > + struct gpio_desc *gpiod_reset;
> > + unsigned long mA;
> > + unsigned int vbus;
>
> These are unused
Indeed
> > + enum usb_dr_mode dr_mode;
> > +};
> > +
> > +static int phy_usb_generic_init(struct phy *phy)
> > +{
> > + struct phy_usb_generic *priv = phy_get_drvdata(phy);
> > +
> > + if (priv->clk)
> > + clk_prepare(priv->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int phy_usb_generic_exit(struct phy *phy)
> > +{
> > + struct phy_usb_generic *priv = phy_get_drvdata(phy);
> > +
> > + if (priv->clk)
> > + clk_unprepare(priv->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int phy_usb_generic_power_on(struct phy *phy)
> > +{
> > + struct phy_usb_generic *priv = phy_get_drvdata(phy);
> > + int ret;
> > +
> > + if (priv->vcc) {
> > + ret = regulator_enable(priv->vcc);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to enable power\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (priv->clk) {
> > + ret = clk_enable(priv->clk);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to enable clock\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (priv->gpiod_reset) {
> > + dev_dbg(priv->dev, "Reset toggle\n");
> > + gpiod_set_value_cansleep(priv->gpiod_reset, 1);
> > + usleep_range(10000, 20000);
> > + gpiod_set_value_cansleep(priv->gpiod_reset, 0);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int phy_usb_generic_power_off(struct phy *phy)
> > +{
> > + struct phy_usb_generic *priv = phy_get_drvdata(phy);
> > + int ret;
> > +
> > + if (priv->gpiod_reset)
> > + gpiod_set_value_cansleep(priv->gpiod_reset, 1);
> > +
> > + if (priv->clk)
> > + clk_disable_unprepare(priv->clk);
> > +
> > + if (priv->vcc) {
> > + ret = regulator_disable(priv->vcc);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to disable power\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int phy_usb_generic_set_mode(struct phy *phy,
> > + enum phy_mode mode, int submode)
> > +{
> > + struct phy_usb_generic *priv = phy_get_drvdata(phy);
> > + enum usb_dr_mode new_mode;
> > + const char *s = "";
> > +
> > + switch (mode) {
> > + case PHY_MODE_USB_HOST:
> > + case PHY_MODE_USB_HOST_LS:
> > + case PHY_MODE_USB_HOST_HS:
> > + case PHY_MODE_USB_HOST_FS:
> > + new_mode = USB_DR_MODE_HOST;
> > + s = "host";
> > + break;
> > + case PHY_MODE_USB_DEVICE:
> > + case PHY_MODE_USB_DEVICE_LS:
> > + case PHY_MODE_USB_DEVICE_HS:
> > + case PHY_MODE_USB_DEVICE_FS:
> > + new_mode = USB_DR_MODE_PERIPHERAL;
> > + s = "peripheral";
> > + break;
> > + case PHY_MODE_USB_OTG:
> > + new_mode = USB_DR_MODE_OTG;
> > + s = "otg";
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (new_mode != priv->dr_mode) {
> > + dev_info(priv->dev, "Changing dr_mode to %s\n", s);
> > + priv->dr_mode = new_mode;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops phy_usb_generic_ops = {
> > + .init = phy_usb_generic_init,
> > + .exit = phy_usb_generic_exit,
> > + .power_on = phy_usb_generic_power_on,
> > + .power_off = phy_usb_generic_power_off,
> > + .set_mode = phy_usb_generic_set_mode,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int phy_usb_generic_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > + struct phy_usb_generic *priv;
> > + struct phy_provider *provider;
> > + struct phy *phy;
> > + int err;
> > + u32 clk_rate = 0;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > + priv->dev = &pdev->dev;
> > + platform_set_drvdata(pdev, priv);
> > +
> > + priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> > + if (IS_ERR(priv->gpiod_reset)) {
> > + err = PTR_ERR(priv->gpiod_reset);
> > + dev_err(dev, "Failed to get reset gpio: %d\n", err);
>
> These should be dev_err_probe to avoid noise when we get -EPROBE_DEFER. Ditto
> for the rest of the dev_errs.
>
Thanks, TIL dev_err_probe! I'll modify this
> > + return err;
> > + }
> > +
> > + if (np && of_property_read_u32(np, "clock-frequency", &clk_rate))
> > + clk_rate = 0;
> > +
> > + priv->clk = devm_clk_get_optional(dev, "main_clk");
> > + if (IS_ERR(priv->clk)) {
> > + err = PTR_ERR(priv->clk);
> > + dev_err(dev, "Can't get phy clock: %d\n", err);
> > + return err;
> > + }
> > + if (priv->clk && clk_rate) {
> > + err = clk_set_rate(priv->clk, clk_rate);
> > + if (err) {
> > + dev_err(dev, "Error setting clock rate\n");
> > + return err;
> > + }
> > + }
> > +
> > + priv->vcc = devm_regulator_get_optional(dev, "vcc");
> > + if (IS_ERR(priv->vcc)) {
> > + err = PTR_ERR(priv->vcc);
> > + if (err == -ENODEV || err == -ENOENT) {
> > + priv->vcc = NULL;
> > + } else {
> > + dev_err(dev, "Error getting vcc regulator: %d\n", err);
> > + return err;
> > + }
> > + }
> > +
> > + phy = devm_phy_create(dev, NULL, &phy_usb_generic_ops);
> > + if (IS_ERR(phy)) {
> > + err = PTR_ERR(phy);
> > + dev_err(dev, "Failed to create PHY: %d\n", err);
> > + return err;
> > + }
> > + phy_set_drvdata(phy, priv);
> > +
> > + provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> > + if (IS_ERR(provider)) {
> > + err = PTR_ERR(provider);
> > + dev_err(dev, "Failed to register PHY provider: %d\n", err);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id phy_usb_nop_dt_ids[] = {
> > + { .compatible = "phy-usb-generic" },
> > + { .compatible = "usb-nop-xceiv" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, phy_usb_nop_dt_ids);
> > +
> > +static struct platform_driver phy_usb_generic_driver = {
> > + .probe = phy_usb_generic_probe,
> > + .driver = {
> > + .name = "phy_usb_generic",
> > + .of_match_table = phy_usb_nop_dt_ids,
> > + },
> > +};
> > +module_platform_driver(phy_usb_generic_driver);
> > +
> > +MODULE_AUTHOR("Kalray Inc");
> > +MODULE_DESCRIPTION("Generic USB PHY driver");
> > +MODULE_LICENSE("GPL");
> >
>
> The rest looks good (modulo missing features).
Yeah, I feel also there is a lot of missing features from the
usb-nop-xceiv driver. I doesn't need much more than a simple gpio
hence it is difficult for me to test porting other features.
Thanks,
-- Jules
