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
