Message-ID: <20200527203453.GJ5308@sirena.org.uk>
Date:   Wed, 27 May 2020 21:34:53 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Android Kernel Team <kernel-team@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] regulator: Add support for sync_state() callbacks

On Wed, May 27, 2020 at 10:17:21AM -0700, Saravana Kannan wrote:

> If fw_devlink is off or not supported by the firmware (Eg: ACPI), the
> behavior doesn't change. We act as if there are no consumers and turn
> stuff off 30s after late_initcall_sync(). If fw_devlink is on, then it
> makes sure all the consumers (in DT) are linked to the regulator
> device as soon as it is added. So that solves your "we don't know if
> there'll ever be consumers".

No, it doesn't help at all with figuring out if consumers will ever
instantiate...

> The next concern is, "will the drivers ever be loaded for these
> consumers". To handle these cases, I can update the "30s timeout code"
> to just release all the "hold state". And I can make the time out a
> kernel command line param that if set to 0, then it will actually wait
> for all the consumers.

...due to this issue.  The DT describes the hardware, not the software
that will run on it.  Making the timeout configurable is fine.

> Does that seem like something that'd work for you? That way, if no one
> sets this command line param, it won't affect them. And if they set
> it, then things work as intended when the system is configured so that
> everything does eventually come up.

This still leaves you with the problem that we're going to just ignore
operations that happen while implementation of operations is blocked.
If we queue up actually implementing changes to the hardware until after
we claimed we'd done them then that's asking for trouble, especally with
voltage changes - they need to be complete when regulator_set_voltage()
returns, we can't defer syncing that to the hardware to some later
point. 

Actually now I try to make sense of the code I *think* that rather than
holding off on writing changes out like sync_state() and your changelog
suggests this is only trying to defer disables.  That's a bit safer
but then again it won't help as soon as we run into a system with a
device that glitches due to some other change like a voltage being
changed unexpectedly and it's adding complexity.  The entire patch is
super unclear though, I can't understand what sync_supply is supposed to
be.  It appears to functionally just be a flag but it's done using this
weirdly allocated and manipulated struct regulator.

> > > This commit adds a regulator_sync_state() helper function that
> > > takes
> > > care of all the "boot on" regulator clean up for any regulator driver.
> > > All one needs to do is add the following line to the driver struct.

> > None of the issues around this have *anything* to do with individual
> > drivers,

> Fair enough. I was just trying to give a way for systems that don't
> have the "consumers might never come up" issue (Eg: Android phones) a
> way to start using this while we try to figure out what to do for the
> systems where it might be a problem.

Drivers can be run on multiple systems, this is not a decision that can
be made based on the driver.

> > all this is doing is forcing us to go through and add this to
> > every single driver which doesn't accomplish anything.

> I don't see what's wrong with that. The kernel has made plenty of
> changes where all the drivers using an API had to be updated in one
> shot. This patch doesn't even require a one shot change. Anyway, if

We do gradual API updates when there are actual changes required in
drivers.  This change requires *zero* changes to drivers, there is
absolutely nothing driver specific or relevant about it.  Nothing about
the driver tells you if the flag should be set or not (it's a callback
in the code but since there's only one possible implementation it's
really a flag) so there's no purpose in having a flag in the driver in
the first place.

> > Please go and look at the previous discussions of this topic, this needs
> > to work for other users as well.

> I'd be happy to, if you can point me to some of them. Sorry, I didn't
> know what to even search for to get a meaningful list of search
> results.

The first hits I found were:

   https://lore.kernel.org/linux-arm-kernel/1368076726-11492-1-git-send-email-skannan@codeaurora.org/#t
   https://lore.kernel.org/linux-arm-kernel/CAKON4OzO427-MU4FqmF8AP5V=CXHuGdV1GTptiaJiRY7DLfDRA@mail.gmail.com/

Search around for deferred initcall, regulator_late_init() and so on.
Please also talk to your colleagues, IIRC a good proportion of the
variations on this have come from them.

You haven't mentioned an actual use case here (your changelog just
declares the solution) but in general these things are init ordering
issues, if some device (typically the display stuff) is going to cause
user visible glitches during init then things will work best if the
system tries to ensure that that device gets started as early as
possible.  In general these things are best addressed at the system
level rather than by bodging some low level thing.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists