Hi Tom, On Tue, 28 Dec 2021 at 05:55, Tom Rini <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > > > --- > > > > > > 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/[email protected]/ > > > > > > 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 <[email protected]> > > > > 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
