[PATCH 1/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Wed, Apr 18, 2018 at 10:55:49PM -0700, Stephen Boyd wrote:

> I lost the code context, but my general comment was that the modes that
> the hardware supports should come from the driver. If there's some sort
> of constraint on what those modes end up being because some board has an
> issue with some mode then we would want a binding that indicates such a

No, that's *never* how regulator constraints work.  We only change the
hardware configuration if we have explicit permission from constraints
to do so, otherwise everything is left as we found it.  That way if your
board catches fire or whatever it wasn't something that Linux initiated
just from default behaviour.

> > > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
> > > registers. This probably needs to be pushed into the framework and come
> > > down through a 'set_headroom' op in the regulator ops via a
> > > regulator-headroom-microvolt property that's parsed in of_regulator.c.

> > The qcom,headroom-voltage property is equivalent to struct
> > regulator_desc.min_dropout_uV, but handled in hardware.  I don't see the
> > need to make a new regulator op to configure this value dynamically.

> Ok? We have other regulator ops just to configure boot time things. The
> goal is to come up with generic regulator properties that can be applied
> from the framework perspective and be used by other regulator drivers in
> the future.

This doesn't sound like what the min_dropout_uV constraint is intended
to handle - that's there for the regulator driver (not constraints) to
indicate how much headroom the regulator needs in the supply voltage in
order to provide regulation.  It's not something the regulator uses,
it's something that gets fed into voltage requests made on the supply of
the regulator which I can't see that the hardware is going to be able to
handle unaided.

> Cool. This should be a generic regulator DT property that all regulators
> can use. We have the same problem on other RPM based regulator drivers
> where boot sends a bunch of voltages because we don't know what it is by
> default out of boot and we can't read it.

Ideally future versions of the RPM will have improved interfaces,
there's a bunch of problems like this :(

> > 
> > >> +       if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
> > >> +               vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> > >> +               init_data->constraints.apply_uV = 0;
> > >> +               vreg->rdesc.n_voltages = 1;
> > >> +       }

> > > What is this doing? Usually constraints aren't touched by the driver.

> > For XOB managed regulators, we need to set fixed_uV to match the DT
> > constraint voltage and n_voltages = 1.  This allows consumers
> > regulator_set_voltage() calls to succeed for such regulators.  It works
> > the same as a fixed regulator.  I think that apply_uV = 0 could be left out.

> Wouldn't XOB regulators only have one voltage specified for min/max in
> DT already though? I seem to recall that's how we make set_voltage() in
> those cases work. Or it could be that drivers aren't supposed to call
> set_voltage() on single or fixed voltage regulators anyway, because
> constraints take care of it for them.

Yes, constraints that specify a single voltage are done by setting min
and max to the same value.  fixed_uV is *only* for regulators that have
a physically fixed voltage.

> > >> +       if (vreg->hw_data->mode_map) {
> > >> +               init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;

A driver must *NEVER* modify any constraints.

> > > Huh, I thought this was assigned by the framework.

> > No, this is not set anywhere in the regulator framework.  There isn't a DT
> > method to configure it.  It seems that it could only be handled before
> > with board files.  Other regulator drivers also configure it.

> Hmm ok. Would something be bad if we did support it through DT? I can't
> seem to recall the history here.

Yes, if someone wants to configure this through DT they should add
support for configuring it using DT.  The mode support in most
regulators is not practically useful so nobody did that yet.  Mostly the
hardware does a much better job of adjusting modes on the fly for
anything that's going on at runtime than software is ever likely to do
so it's not worth it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180419/eaa7da5d/attachment.sig>