Received-SPF: pass (google.com: domain of linux-kernel+bounces-162748-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Apr 2024 19:28:48 +0200
From: Kory Maincent <kory.maincent@bootlin.com>
To: Mark Brown <broonie@kernel.org>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>, Kyle Swenson
 <kyle.swenson@est.tech>, Liam Girdwood <lgirdwood@gmail.com>,
 linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>
Subject: Re: PoE complex usage of regulator API
Message-ID: <20240429192848.13c576b7@kmaincent-XPS-13-7390>
In-Reply-To: <Zi--4ko_vAtFSxyn@finisterre.sirena.org.uk>
References: <20240426124253.56fd0933@kmaincent-XPS-13-7390>
	<Zi--4ko_vAtFSxyn@finisterre.sirena.org.uk>
Organization: bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, 30 Apr 2024 00:38:10 +0900
Mark Brown <broonie@kernel.org> wrote:

Hello all, thank for your replies!
That gives me more hint for the development.

> On Fri, Apr 26, 2024 at 12:42:53PM +0200, Kory Maincent wrote:
>=20
> > Let's begin simple, in PSE world we are more talking about power.
> > Would it be ok to add a regulator_get/set_power_limit() and
> > regulator_get_power() callback to regulator API. Would regulator API ha=
ve
> > interest to such callbacks? =20
>=20
> Why would these be different to the existing support for doing current
> limiting?  If the voltage for the supply is known then the power is a
> simple function of the current and the voltage.  I suppose you could try
> to do a convenience functions for a fixed voltage, but there'd be issues
> there if the voltage isn't configured to an exact voltage and might
> vary.

That's right I was focusing on power where I could use already implemented
voltage and current callbacks. Would you be interested to a new get_current=
()
callback to know the current and allows regulator to deduce the consumed po=
wer
or should it be specific to PSE subsystem.

> > Port priority, more complex subject:
> > Indeed a PSE controller managing several ports may be able to turn off =
ports
> > with low priority if the total power consumption exceed a certain level.
> > - There are controller like PD692x0 that can managed this on the hardwa=
re
> > side. In that case we would have a regulator_get/set_power_limit()
> > callbacks from the regulator parent (the PSE contoller) and a
> > regulator_get/set_priory() callbacks for the regulator children (PSE
> > ports). =20
>=20
> All this priority stuff feels very PSE specific but possibly doable.
> You'd have to define the domains in which priorities apply as well as
> the priorities themselves.

If you think that it is really specific to PSE no need to add it in the
regulator API, it will also remove me some brain knots.

> > - There are controller like TPS23881 or LTC4266 that can set two priori=
ties
> >   levels on their ports and a level change in one of their input pin can
> >   shutdown all the low priority ports. In that case the same callbacks
> > could be used. regulator_get/set_power_limit() from the parent will be =
only
> > at software level. regulator_get/set_priority() will set the priorities=
 of
> > the ports on hardware level. A polling function have to read frequently=
 the
> > total power used and compare it to the power budget, then it has to call
> > something like regulator_shutdown_consumer() in case of power overflow.=
   =20
>=20
> I would expect the regulators can generate notifications when they go
> out of regulation?  Having to poll feels very crude for something with
> configurable power limits.

Yep that's true. Indeed using notification would be way better!

> > https://lore.kernel.org/netdev/20240417-feature_poe-v9-10-242293fd1900@=
bootlin.com/
> > But in case the port is enabled from Linux then shutdown from the PSE
> > controller for any reason, I have to run disable and enable command to
> > enable it again. Not really efficient :/ =20
>=20
> If that is a hot path something has gone very wrong with the system,
> especially if it's such a hot path that the cost of a disable is making
> a difference. =20

That's not in the hotpath.

> Note that hardware may have multiple error handling
> strategies, some hardware will turn off outputs when there's a problem
> while other implementations will try to provide as good an output as
> they can.  Sometimes the strategy will depend on the specific error
> condition, and there may be timeouts involved.  This all makes it very
> difficult to assume any particular state after an error has occurred, or
> that the state configured in the control registers reflects the physical
> state of the hardware so you probably want some explicit handling for
> any new state you're looking for.

Alright, didn't thought of these different management of an error condition.
We might also see similar things in PSE, so I will keep it like that.

> > I am thinking of disabling the usage of counters in case of a
> > regulator_get_exclusive(). What do you think? Could it break other usag=
e? =20
>=20
> Yes, that seems likely to break other users and in general a sharp edge
> for people working with the API.

Okay,

Regards,
--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com