Re: PoE complex usage of regulator API

[Kory Maincent <kory.maincent@bootlin.com>]

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:
> 
> > 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 have
> > interest to such callbacks?  
> 
> 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 power
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 hardware
> > 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).  
> 
> 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 priorities
> >   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.    
> 
> 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 :/  
> 
> 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.  

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 usage?  
> 
> Yes, that seems likely to break other users and in general a sharp edge
> for people working with the API.

Okay,

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com