Re: [RFC PATCH] phy: Convert usb-nop-xceiv to the generic phy subsystem

[Sean Anderson]

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)

> 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?

> 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
to call usb_add_phy_dev in probe() as well.

Though I suppose this is why this patch is RFC?

> 
> 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

> +	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.

> +		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).

--Sean