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