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

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

[Alper Nebi Yasak]

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@gmail.com>
---

Changes in v2:
- Rename {phy_,}id_priv -> {phy_,}counts
- Split phy_get_uclass_priv -> phy_{alloc,get}_counts
- Allocate counts (or return error) in generic_phy_get_by_*()
- Remove now-unnecessary null checks for counts of valid phy handles

v1: https://patchwork.ozlabs.org/project/uboot/patch/20211210200124.19226-1-alpernebiyasak@gmail.com/

 drivers/phy/allwinner/phy-sun4i-usb.c |   9 --
 drivers/phy/phy-uclass.c              | 120 ++++++++++++++++++++++++++
 test/dm/phy.c                         |  83 ++++++++++++++++--
 3 files changed, 198 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..c45397eeef0c 100644
--- a/drivers/phy/phy-uclass.c
+++ b/drivers/phy/phy-uclass.c
@@ -11,12 +11,79 @@
 #include <dm/device_compat.h>
 #include <dm/devres.h>
 #include <generic-phy.h>
+#include <linux/list.h>
+
+struct phy_counts {
+	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_counts *phy_get_counts(struct phy *phy)
+{
+	struct list_head *uc_priv;
+	struct phy_counts *counts;
+
+	if (!generic_phy_valid(phy))
+		return NULL;
+
+	uc_priv = dev_get_uclass_priv(phy->dev);
+	list_for_each_entry(counts, uc_priv, list)
+		if (counts->id == phy->id)
+			return counts;
+
+	return NULL;
+}
+
+static int phy_alloc_counts(struct phy *phy)
+{
+	struct list_head *uc_priv;
+	struct phy_counts *counts;
+
+	if (!generic_phy_valid(phy))
+		return 0;
+	if (phy_get_counts(phy))
+		return 0;
+
+	uc_priv = dev_get_uclass_priv(phy->dev);
+	counts = kzalloc(sizeof(*counts), GFP_KERNEL);
+	if (!counts)
+		return -ENOMEM;
+
+	counts->id = phy->id;
+	counts->power_on_count = 0;
+	counts->init_count = 0;
+	list_add(&counts->list, uc_priv);
+
+	return 0;
+}
+
+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;
+}
+
+static int phy_uclass_pre_remove(struct udevice *dev)
+{
+	struct list_head *uc_priv = dev_get_uclass_priv(dev);
+	struct phy_counts *counts, *next;
+
+	list_for_each_entry_safe(counts, next, uc_priv, list)
+		kfree(counts);
+
+	return 0;
+}
+
 static int generic_phy_xlate_offs_flags(struct phy *phy,
 					struct ofnode_phandle_args *args)
 {
@@ -88,6 +155,12 @@ int generic_phy_get_by_index_nodev(ofnode node, int index, struct phy *phy)
 		goto err;
 	}
 
+	ret = phy_alloc_counts(phy);
+	if (ret) {
+		debug("phy_alloc_counts() failed: %d\n", ret);
+		goto err;
+	}
+
 	return 0;
 
 err:
@@ -118,6 +191,7 @@ int generic_phy_get_by_name(struct udevice *dev, const char *phy_name,
 
 int generic_phy_init(struct phy *phy)
 {
+	struct phy_counts *counts;
 	struct phy_ops const *ops;
 	int ret;
 
@@ -126,10 +200,19 @@ int generic_phy_init(struct phy *phy)
 	ops = phy_dev_ops(phy->dev);
 	if (!ops->init)
 		return 0;
+
+	counts = phy_get_counts(phy);
+	if (counts->init_count > 0) {
+		counts->init_count++;
+		return 0;
+	}
+
 	ret = ops->init(phy);
 	if (ret)
 		dev_err(phy->dev, "PHY: Failed to init %s: %d.\n",
 			phy->dev->name, ret);
+	else
+		counts->init_count = 1;
 
 	return ret;
 }
@@ -154,6 +237,7 @@ int generic_phy_reset(struct phy *phy)
 
 int generic_phy_exit(struct phy *phy)
 {
+	struct phy_counts *counts;
 	struct phy_ops const *ops;
 	int ret;
 
@@ -162,16 +246,28 @@ int generic_phy_exit(struct phy *phy)
 	ops = phy_dev_ops(phy->dev);
 	if (!ops->exit)
 		return 0;
+
+	counts = phy_get_counts(phy);
+	if (counts->init_count == 0)
+		return 0;
+	if (counts->init_count > 1) {
+		counts->init_count--;
+		return 0;
+	}
+
 	ret = ops->exit(phy);
 	if (ret)
 		dev_err(phy->dev, "PHY: Failed to exit %s: %d.\n",
 			phy->dev->name, ret);
+	else
+		counts->init_count = 0;
 
 	return ret;
 }
 
 int generic_phy_power_on(struct phy *phy)
 {
+	struct phy_counts *counts;
 	struct phy_ops const *ops;
 	int ret;
 
@@ -180,16 +276,26 @@ int generic_phy_power_on(struct phy *phy)
 	ops = phy_dev_ops(phy->dev);
 	if (!ops->power_on)
 		return 0;
+
+	counts = phy_get_counts(phy);
+	if (counts->power_on_count > 0) {
+		counts->power_on_count++;
+		return 0;
+	}
+
 	ret = ops->power_on(phy);
 	if (ret)
 		dev_err(phy->dev, "PHY: Failed to power on %s: %d.\n",
 			phy->dev->name, ret);
+	else
+		counts->power_on_count = 1;
 
 	return ret;
 }
 
 int generic_phy_power_off(struct phy *phy)
 {
+	struct phy_counts *counts;
 	struct phy_ops const *ops;
 	int ret;
 
@@ -198,10 +304,21 @@ int generic_phy_power_off(struct phy *phy)
 	ops = phy_dev_ops(phy->dev);
 	if (!ops->power_off)
 		return 0;
+
+	counts = phy_get_counts(phy);
+	if (counts->power_on_count == 0)
+		return 0;
+	if (counts->power_on_count > 1) {
+		counts->power_on_count--;
+		return 0;
+	}
+
 	ret = ops->power_off(phy);
 	if (ret)
 		dev_err(phy->dev, "PHY: Failed to power off %s: %d.\n",
 			phy->dev->name, ret);
+	else
+		counts->power_on_count = 0;
 
 	return ret;
 }
@@ -316,4 +433,7 @@ int generic_phy_power_off_bulk(struct phy_bulk *bulk)
 UCLASS_DRIVER(phy) = {
 	.id		= UCLASS_PHY,
 	.name		= "phy",
+	.pre_probe	= phy_uclass_pre_probe,
+	.pre_remove	= phy_uclass_pre_remove,
+	.per_device_auto = sizeof(struct list_head),
 };
diff --git a/test/dm/phy.c b/test/dm/phy.c
index ecbd47bf12fd..df4c73fc701f 100644
--- a/test/dm/phy.c
+++ b/test/dm/phy.c
@@ -79,12 +79,15 @@ static int dm_test_phy_ops(struct unit_test_state *uts)
 	ut_assertok(generic_phy_power_off(&phy1));
 
 	/*
-	 * test operations after exit().
-	 * The sandbox phy driver does not allow it.
+	 * Test power_on() failure after exit().
+	 * The sandbox phy driver does not allow power-on/off after
+	 * exit, but the uclass counts power-on/init calls and skips
+	 * calling the driver's ops when e.g. powering off an already
+	 * powered-off phy.
 	 */
 	ut_assertok(generic_phy_exit(&phy1));
 	ut_assert(generic_phy_power_on(&phy1) != 0);
-	ut_assert(generic_phy_power_off(&phy1) != 0);
+	ut_assertok(generic_phy_power_off(&phy1));
 
 	/*
 	 * test normal operations again (after re-init)
@@ -99,6 +102,17 @@ static int dm_test_phy_ops(struct unit_test_state *uts)
 	 */
 	ut_assertok(generic_phy_reset(&phy1));
 
+	/*
+	 * Test power_off() failure after exit().
+	 * For this we need to call exit() while the phy is powered-on,
+	 * so that the uclass actually calls the driver's power-off()
+	 * and reports the resulting failure.
+	 */
+	ut_assertok(generic_phy_power_on(&phy1));
+	ut_assertok(generic_phy_exit(&phy1));
+	ut_assert(generic_phy_power_off(&phy1) != 0);
+	ut_assertok(generic_phy_power_on(&phy1));
+
 	/* PHY2 has a known problem with power off */
 	ut_assertok(generic_phy_init(&phy2));
 	ut_assertok(generic_phy_power_on(&phy2));
@@ -106,8 +120,8 @@ static int dm_test_phy_ops(struct unit_test_state *uts)
 
 	/* PHY3 has a known problem with power off and power on */
 	ut_assertok(generic_phy_init(&phy3));
-	ut_asserteq(-EIO, generic_phy_power_off(&phy3));
-	ut_asserteq(-EIO, generic_phy_power_off(&phy3));
+	ut_asserteq(-EIO, generic_phy_power_on(&phy3));
+	ut_assertok(generic_phy_power_off(&phy3));
 
 	return 0;
 }
@@ -145,3 +159,62 @@ static int dm_test_phy_bulk(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_phy_bulk, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int dm_test_phy_multi_exit(struct unit_test_state *uts)
+{
+	struct phy phy1_method1;
+	struct phy phy1_method2;
+	struct phy phy1_method3;
+	struct udevice *parent;
+
+	/* Get the same phy instance in 3 different ways. */
+	ut_assertok(uclass_get_device_by_name(UCLASS_SIMPLE_BUS,
+					      "gen_phy_user", &parent));
+	ut_assertok(generic_phy_get_by_name(parent, "phy1", &phy1_method1));
+	ut_asserteq(0, phy1_method1.id);
+	ut_assertok(generic_phy_get_by_name(parent, "phy1", &phy1_method2));
+	ut_asserteq(0, phy1_method2.id);
+	ut_asserteq_ptr(phy1_method1.dev, phy1_method1.dev);
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_SIMPLE_BUS,
+					      "gen_phy_user1", &parent));
+	ut_assertok(generic_phy_get_by_name(parent, "phy1", &phy1_method3));
+	ut_asserteq(0, phy1_method3.id);
+	ut_asserteq_ptr(phy1_method1.dev, phy1_method3.dev);
+
+	/*
+	 * Test using the same PHY from different handles.
+	 * In non-test code these could be in different drivers.
+	 */
+
+	/*
+	 * These must only call the driver's ops at the first init()
+	 * and power_on().
+	 */
+	ut_assertok(generic_phy_init(&phy1_method1));
+	ut_assertok(generic_phy_init(&phy1_method2));
+	ut_assertok(generic_phy_power_on(&phy1_method1));
+	ut_assertok(generic_phy_power_on(&phy1_method2));
+	ut_assertok(generic_phy_init(&phy1_method3));
+	ut_assertok(generic_phy_power_on(&phy1_method3));
+
+	/*
+	 * These must not call the driver's ops as other handles still
+	 * want the PHY powered-on and initialized.
+	 */
+	ut_assertok(generic_phy_power_off(&phy1_method3));
+	ut_assertok(generic_phy_exit(&phy1_method3));
+
+	/*
+	 * We would get an error here if the generic_phy_exit() above
+	 * actually called the driver's exit(), as the sandbox driver
+	 * doesn't allow power-off() after exit().
+	 */
+	ut_assertok(generic_phy_power_off(&phy1_method1));
+	ut_assertok(generic_phy_power_off(&phy1_method2));
+	ut_assertok(generic_phy_exit(&phy1_method1));
+	ut_assertok(generic_phy_exit(&phy1_method2));
+
+	return 0;
+}
+DM_TEST(dm_test_phy_multi_exit, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.34.1

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

[Simon Glass]

Hi Alper,

On Fri, 24 Dec 2021 at 06:06, Alper Nebi Yasak <alpernebiyasak@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@gmail.com>
> ---
>
> Changes in v2:
> - Rename {phy_,}id_priv -> {phy_,}counts
> - Split phy_get_uclass_priv -> phy_{alloc,get}_counts
> - Allocate counts (or return error) in generic_phy_get_by_*()
> - Remove now-unnecessary null checks for counts of valid phy handles
>
> v1: https://patchwork.ozlabs.org/project/uboot/patch/20211210200124.19226-1-alpernebiyasak@gmail.com/
>
>  drivers/phy/allwinner/phy-sun4i-usb.c |   9 --
>  drivers/phy/phy-uclass.c              | 120 ++++++++++++++++++++++++++
>  test/dm/phy.c                         |  83 ++++++++++++++++--
>  3 files changed, 198 insertions(+), 14 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Should add comments for the struct

Also I wonder if a simple fixed-length array might be possible instead
of the linked list?

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]

On Tue, Dec 28, 2021 at 01:34:18AM -0700, Simon Glass wrote:
> Hi Alper,
> 
> On Fri, 24 Dec 2021 at 06:06, Alper Nebi Yasak <alpernebiyasak@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@gmail.com>
> > ---
> >
> > Changes in v2:
> > - Rename {phy_,}id_priv -> {phy_,}counts
> > - Split phy_get_uclass_priv -> phy_{alloc,get}_counts
> > - Allocate counts (or return error) in generic_phy_get_by_*()
> > - Remove now-unnecessary null checks for counts of valid phy handles
> >
> > v1: https://patchwork.ozlabs.org/project/uboot/patch/20211210200124.19226-1-alpernebiyasak@gmail.com/
> >
> >  drivers/phy/allwinner/phy-sun4i-usb.c |   9 --
> >  drivers/phy/phy-uclass.c              | 120 ++++++++++++++++++++++++++
> >  test/dm/phy.c                         |  83 ++++++++++++++++--
> >  3 files changed, 198 insertions(+), 14 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Should add comments for the struct
> 
> Also I wonder if a simple fixed-length array might be possible instead
> of the linked list?

Thanks for the review Simon.  Since I think this should unblock some
common hardware, does everyone think this should be safe enough to pull
in now, or no, I shouldn't bend the rules, and take this for v2021.04
instead?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Simon Glass]

Hi Tom,

On Tue, 28 Dec 2021 at 05:55, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 28, 2021 at 01:34:18AM -0700, Simon Glass wrote:
> > Hi Alper,
> >
> > On Fri, 24 Dec 2021 at 06:06, Alper Nebi Yasak <alpernebiyasak@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@gmail.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Rename {phy_,}id_priv -> {phy_,}counts
> > > - Split phy_get_uclass_priv -> phy_{alloc,get}_counts
> > > - Allocate counts (or return error) in generic_phy_get_by_*()
> > > - Remove now-unnecessary null checks for counts of valid phy handles
> > >
> > > v1: https://patchwork.ozlabs.org/project/uboot/patch/20211210200124.19226-1-alpernebiyasak@gmail.com/
> > >
> > >  drivers/phy/allwinner/phy-sun4i-usb.c |   9 --
> > >  drivers/phy/phy-uclass.c              | 120 ++++++++++++++++++++++++++
> > >  test/dm/phy.c                         |  83 ++++++++++++++++--
> > >  3 files changed, 198 insertions(+), 14 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Should add comments for the struct
> >
> > Also I wonder if a simple fixed-length array might be possible instead
> > of the linked list?
>
> Thanks for the review Simon.  Since I think this should unblock some
> common hardware, does everyone think this should be safe enough to pull
> in now, or no, I shouldn't bend the rules, and take this for v2021.04
> instead?

It looks safe to me and it has a test. If there are boards destined
for this release that need it, then I have no objection.

Regards,
Simon

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

[Alper Nebi Yasak]

On 28/12/2021 11:34, Simon Glass wrote:
> Should add comments for the struct

I can do that and send as a v3.

> Also I wonder if a simple fixed-length array might be possible instead
> of the linked list?

I think it's possible, my first prototype was something like:

  #define MAX_PHYS 16

  struct phy_uc_priv {
      int power_on_count[MAX_PHYS];
      int init_count[MAX_PHYS];
  };

  uc_priv = dev_get_uclass_priv(phy->dev);
  uc_priv->power_on_count[phy->id];
  uc_priv->init_count[phy->id];

  UCLASS_DRIVER(phy) = {
      ...
      .per_device_auto = sizeof(struct phy_uc_priv),
  }

But I rewrote as a linked list because I couldn't decide on a reasonable
value for that MAX_PHYS. Best guess I have is from
drivers/phy/cadence/phy-cadence-sierra.c which defines SIERRA_MAX_LANES
as 16.


As array-of-structs, I guess things would be roughly:

  #define MAX_PHYS 16

  struct phy_counts {
      int power_on_count;
      int init_count;
  };

  counts = dev_get_uclass_priv(phy->dev)[phy->id];
  counts->power_on_count;
  counts->init_count;

  UCLASS_DRIVER(phy) = {
      ...
      .per_device_auto = sizeof(struct phy_counts[MAX_PHYS]),
  }

Should I change to that?

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

[Alper Nebi Yasak]

On 28/12/2021 16:08, Simon Glass wrote:
> On Tue, 28 Dec 2021 at 05:55, Tom Rini <trini@konsulko.com> wrote:
>> Thanks for the review Simon.  Since I think this should unblock some
>> common hardware, does everyone think this should be safe enough to pull
>> in now, or no, I shouldn't bend the rules, and take this for v2021.04
>> instead?
> 
> It looks safe to me and it has a test. If there are boards destined
> for this release that need it, then I have no objection.

I'm fine with either option. I guess most people already know and use
the rockchip-inno-usb2 patch to get around this, so I don't exactly
consider this urgent, especially since I have bigger problems with my board.

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

[Simon Glass]

Hi Alper,

On Wed, 29 Dec 2021 at 15:01, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 28/12/2021 11:34, Simon Glass wrote:
> > Should add comments for the struct
>
> I can do that and send as a v3.
>
> > Also I wonder if a simple fixed-length array might be possible instead
> > of the linked list?
>
> I think it's possible, my first prototype was something like:
>
>   #define MAX_PHYS 16
>
>   struct phy_uc_priv {
>       int power_on_count[MAX_PHYS];
>       int init_count[MAX_PHYS];
>   };
>
>   uc_priv = dev_get_uclass_priv(phy->dev);
>   uc_priv->power_on_count[phy->id];
>   uc_priv->init_count[phy->id];
>
>   UCLASS_DRIVER(phy) = {
>       ...
>       .per_device_auto = sizeof(struct phy_uc_priv),
>   }
>
> But I rewrote as a linked list because I couldn't decide on a reasonable
> value for that MAX_PHYS. Best guess I have is from
> drivers/phy/cadence/phy-cadence-sierra.c which defines SIERRA_MAX_LANES
> as 16.

You could make it a CONFIG.

I was more thinking of calculating the size and then allocating it in
one hit, for simplicity. But given that you have something more
flexible and it seems to be needed, let's leave it as it is.

>
>
> As array-of-structs, I guess things would be roughly:
>
>   #define MAX_PHYS 16
>
>   struct phy_counts {
>       int power_on_count;
>       int init_count;
>   };
>
>   counts = dev_get_uclass_priv(phy->dev)[phy->id];
>   counts->power_on_count;
>   counts->init_count;
>
>   UCLASS_DRIVER(phy) = {
>       ...
>       .per_device_auto = sizeof(struct phy_counts[MAX_PHYS]),
>   }
>
> Should I change to that?

I really like driver model doing the allocation. But I think if you
did that, it would be better to have a struct that included the array,
so we can have a simple sizeof(struct xxx) there.

Regards,
Simon

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

[Peter Robinson]

On Tue, Dec 28, 2021 at 12:55 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 28, 2021 at 01:34:18AM -0700, Simon Glass wrote:
> > Hi Alper,
> >
> > On Fri, 24 Dec 2021 at 06:06, Alper Nebi Yasak <alpernebiyasak@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@gmail.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Rename {phy_,}id_priv -> {phy_,}counts
> > > - Split phy_get_uclass_priv -> phy_{alloc,get}_counts
> > > - Allocate counts (or return error) in generic_phy_get_by_*()
> > > - Remove now-unnecessary null checks for counts of valid phy handles
> > >
> > > v1: https://patchwork.ozlabs.org/project/uboot/patch/20211210200124.19226-1-alpernebiyasak@gmail.com/
> > >
> > >  drivers/phy/allwinner/phy-sun4i-usb.c |   9 --
> > >  drivers/phy/phy-uclass.c              | 120 ++++++++++++++++++++++++++
> > >  test/dm/phy.c                         |  83 ++++++++++++++++--
> > >  3 files changed, 198 insertions(+), 14 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Should add comments for the struct
> >
> > Also I wonder if a simple fixed-length array might be possible instead
> > of the linked list?
>
> Thanks for the review Simon.  Since I think this should unblock some
> common hardware, does everyone think this should be safe enough to pull
> in now, or no, I shouldn't bend the rules, and take this for v2021.04
> instead?

We've had a number of queries each release around this problem so it
maybe worthwhile to include, I think that most distros that pre-build
things for users are aware of it enough that they've had local patches
for some time so personally for me I'm OK either way.

Peter