[PATCH] phy: Track power-on and init counts in uclass

[Simon Glass]

Hi Alper,

On Sat, 11 Dec 2021 at 02:02, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On boards using the RK3399 SoC, the USB OHCI and EHCI controllers share
> the same PHY device instance. While these controllers are being stopped
> they both attempt to power-off and deinitialize it, but trying to
> power-off the deinitialized PHY device results in a hang. This usually
> happens just before booting an OS, and can be explicitly triggered by
> running "usb start; usb stop" in the U-Boot shell.
>
> Implement a uclass-wide counting mechanism for PHY initialization and
> power state change requests, so that we don't power-off/deinitialize a
> PHY instance until all of its users want it done. The Allwinner A10 USB
> PHY driver does this counting in-driver, remove those parts in favour of
> this in-uclass implementation.
>
> The sandbox PHY operations test needs some changes since the uclass will
> no longer call into the drivers for actions matching its tracked state
> (e.g. powering-off a powered-off PHY). Update that test, and add a new
> one which simulates multiple users of a single PHY.
>
> The major complication here is that PHY handles aren't deduplicated per
> instance, so the obvious idea of putting the counts in the PHY handles
> don't immediately work. It seems possible to bind a child udevice per
> PHY instance to the PHY provider and deduplicate the handles in each
> child's uclass-private areas, like in the CLK framework. An alternative
> approach could be to use those bound child udevices themselves as the
> PHY handles. Instead, to avoid the architectural changes those would
> require, this patch solves things by dynamically allocating a list of
> structs (one per instance) in the provider's uclass-private area.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
>
>  drivers/phy/allwinner/phy-sun4i-usb.c |   9 ---
>  drivers/phy/phy-uclass.c              | 104 ++++++++++++++++++++++++++
>  test/dm/phy.c                         |  83 ++++++++++++++++++--
>  3 files changed, 182 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index ab2a5d17fcff..86c589a65fd3 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -125,7 +125,6 @@ struct sun4i_usb_phy_info {
>
>  struct sun4i_usb_phy_plat {
>         void __iomem *pmu;
> -       int power_on_count;
>         int gpio_vbus;
>         int gpio_vbus_det;
>         int gpio_id_det;
> @@ -225,10 +224,6 @@ static int sun4i_usb_phy_power_on(struct phy *phy)
>                 initial_usb_scan_delay = 0;
>         }
>
> -       usb_phy->power_on_count++;
> -       if (usb_phy->power_on_count != 1)
> -               return 0;
> -
>         if (usb_phy->gpio_vbus >= 0)
>                 gpio_set_value(usb_phy->gpio_vbus, SUNXI_GPIO_PULL_UP);
>
> @@ -240,10 +235,6 @@ static int sun4i_usb_phy_power_off(struct phy *phy)
>         struct sun4i_usb_phy_data *data = dev_get_priv(phy->dev);
>         struct sun4i_usb_phy_plat *usb_phy = &data->usb_phy[phy->id];
>
> -       usb_phy->power_on_count--;
> -       if (usb_phy->power_on_count != 0)
> -               return 0;
> -
>         if (usb_phy->gpio_vbus >= 0)
>                 gpio_set_value(usb_phy->gpio_vbus, SUNXI_GPIO_PULL_DISABLE);
>
> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
> index 59683a080cd7..48b4b24db674 100644
> --- a/drivers/phy/phy-uclass.c
> +++ b/drivers/phy/phy-uclass.c
> @@ -11,12 +11,65 @@
>  #include <dm/device_compat.h>
>  #include <dm/devres.h>
>  #include <generic-phy.h>
> +#include <linux/list.h>
> +
> +struct phy_id_priv {
> +       unsigned long id;
> +       int power_on_count;
> +       int init_count;
> +       struct list_head list;
> +};
>
>  static inline struct phy_ops *phy_dev_ops(struct udevice *dev)
>  {
>         return (struct phy_ops *)dev->driver->ops;
>  }
>
> +static struct phy_id_priv *phy_get_uclass_priv(struct phy *phy)

This name is very confusing as it is similar to one used by DM, but
does something different.

Do you think this function should be split into something that adds
the data and something that reads it?


> +{
> +       struct list_head *uc_priv;
> +       struct phy_id_priv *id_priv;
> +
> +       if (!generic_phy_valid(phy))
> +               return NULL;
> +
> +       uc_priv = dev_get_uclass_priv(phy->dev);
> +       list_for_each_entry(id_priv, uc_priv, list)
> +               if (id_priv->id == phy->id)
> +                       return id_priv;
> +
> +       id_priv = kzalloc(sizeof(*id_priv), GFP_KERNEL);
> +       if (!id_priv)
> +               return NULL;
> +
> +       id_priv->id = phy->id;
> +       id_priv->power_on_count = 0;
> +       id_priv->init_count = 0;
> +       list_add(&id_priv->list, uc_priv);
> +
> +       return id_priv;
> +}
> +
> +static int phy_uclass_pre_probe(struct udevice *dev)
> +{
> +       struct list_head *uc_priv = dev_get_uclass_priv(dev);
> +
> +       INIT_LIST_HEAD(uc_priv);
> +
> +       return 0;
> +}
> +

Regards,
Simon