Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 02:59:38PM -0700, Bjorn Andersson wrote: > On Thu, May 29, 2014 at 2:18 PM, Mark Brown wrote: > > No, this is awful and there's no way in hell that stuff like this should > > be implemented in a driver since there's clearly nothing at all hardware > > specific about it. The load tracking needs to be implemented in the > > framework if it's going to be implemented, and passing it up through the > > chain is obviously going to need some conversion and accounting for > > hardware conversion losses which doesn't seem to be happening here. > > > > I'm still unclear on what the summed current is going to be used for, > > though... > You do load accumlation of all the requests from the drivers of the Linux > system, but in the Qualcomm system there might be load from the modem or the > sensor co-processor that we don't know about here. So additional accumulation > is done by the "pmic" - that is directly accessed by those other systems as > well. So the resulting load is then set directly in hardware instead of setting a mode? That would be totally fine but it doesn't free us from having the logic for accumilating the current we know about in the core; that's the bit that's just at completely the wrong abstraction layer. > I understand your strong opinions regarding this, so I will respin this to > forcefully set the regulator mode intead of merely casting a vote. I.e. > implement set_mode to actually set the mode. Or just don't implement mode setting if it's only used by this crazy stuff. > But as there are no users anymore, I could just let the constraints part go > for > now and once we've figured out the dt part there will be some way of setting > these. Okay? Yes, that's fine. signature.asc Description: Digital signature
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 2:18 PM, Mark Brown wrote:
> On Thu, May 29, 2014 at 02:03:40PM -0700, Bjorn Andersson wrote:
>
> Please fix your mailer to word wrap at less than 80 columns so quoted
> text is legible.
>
>> The hardware in this case is a "pmic" shared by all cpus in the system, so
>> when
>> we set the voltage, state or load of a regulator we merely case a vote. For
>> voltage and state this is not an issue, but for load the value that's
>> interesting for the "pmic" is the sum of the votes; i.e. the sum of the loads
>> from all systems on a single regulator.
>
>> What the code does here is to follow the hack found at codeaurora, that upon
>> get_optimum_mode we guess that the client will call get_optimum_mode followed
>> my set_mode. We keep the track of what load was last requested and use that
>> in
>> our vote.
>
> No, this is awful and there's no way in hell that stuff like this should
> be implemented in a driver since there's clearly nothing at all hardware
> specific about it. The load tracking needs to be implemented in the
> framework if it's going to be implemented, and passing it up through the
> chain is obviously going to need some conversion and accounting for
> hardware conversion losses which doesn't seem to be happening here.
>
> I'm still unclear on what the summed current is going to be used for,
> though...
>
You do load accumlation of all the requests from the drivers of the Linux
system, but in the Qualcomm system there might be load from the modem or the
sensor co-processor that we don't know about here. So additional accumulation
is done by the "pmic" - that is directly accessed by those other systems as
well.
So here we don't set the mode of the regulator, we tell the "pmic" our part and
then it can accumulate further.
I understand your strong opinions regarding this, so I will respin this to
forcefully set the regulator mode intead of merely casting a vote. I.e.
implement set_mode to actually set the mode.
>> >> + if (vreg->parts->ip.mask) {
>> >> + initdata->constraints.valid_ops_mask |=
>> >> REGULATOR_CHANGE_DRMS;
>> >> + initdata->constraints.valid_ops_mask |=
>> >> REGULATOR_CHANGE_MODE;
>> >> + initdata->constraints.valid_modes_mask |=
>> >> REGULATOR_MODE_NORMAL;
>> >> + initdata->constraints.valid_modes_mask |=
>> >> REGULATOR_MODE_IDLE;
>
>> > No, this is just plain broken. Constraints are set by the *board*, you
>> > don't know if these settings are safe on any given board.
>
>> I can see that these are coming from board files, but I didn't find any
>> example
>> of how these are supposed to be set when using DT only.
>
>> What's happening here is that given what compatible you use, different
>> "parts"
>> will be selected and based on that this code will or will not be executed;
>> hence this is defined by what compatible you're using.
>
>> But this is of course not obvious, so unless I've missed something I can
>> clean
>> this up slightly and make the connection to compatible more obvious. Okay?
>
> No, it's still just plain broken. You've no idea if the settings you're
> providing work or not on a given system (or set of drivers for that
> matter) - mode configuration really is a part of the system integration
> not an unchanging part of the PMIC. This code appears to assume that
> every client driver (plus passives) is going to accurately supply load
> information which just isn't realistic except in very controlled cases
> for a specific system.
>
> The reason it's not supported in DT at the minute is that the definition
> of modes is not at all clear in a generic fashion, plus of course the
> fact that we have no users in mainline for dynamic mode setting. Most
> PMICs these days are smart enough to do this autonomously anyway so it's
> not clear that this is something that it's worth spending time on.
>
> Please look for the prior threads on this.
At the time of implementing this the proposed sdhci driver from Qualcomm called
regulator_set_mode, but that got redesigned not to fiddle with
regulators. So it
seems you're right, no-one ever calls regulator_set_mode in mainline
and
hence doesn't have this problem.
So the only example I can find is lp872x, that for buck* does this exactly like
I do.
But as there are no users anymore, I could just let the constraints part go for
now and once we've figured out the dt part there will be some way of setting
these. Okay?
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 02:03:40PM -0700, Bjorn Andersson wrote:
Please fix your mailer to word wrap at less than 80 columns so quoted
text is legible.
> The hardware in this case is a "pmic" shared by all cpus in the system, so
> when
> we set the voltage, state or load of a regulator we merely case a vote. For
> voltage and state this is not an issue, but for load the value that's
> interesting for the "pmic" is the sum of the votes; i.e. the sum of the loads
> from all systems on a single regulator.
> What the code does here is to follow the hack found at codeaurora, that upon
> get_optimum_mode we guess that the client will call get_optimum_mode followed
> my set_mode. We keep the track of what load was last requested and use that in
> our vote.
No, this is awful and there's no way in hell that stuff like this should
be implemented in a driver since there's clearly nothing at all hardware
specific about it. The load tracking needs to be implemented in the
framework if it's going to be implemented, and passing it up through the
chain is obviously going to need some conversion and accounting for
hardware conversion losses which doesn't seem to be happening here.
I'm still unclear on what the summed current is going to be used for,
though...
> >> + if (vreg->parts->ip.mask) {
> >> + initdata->constraints.valid_ops_mask |=
> >> REGULATOR_CHANGE_DRMS;
> >> + initdata->constraints.valid_ops_mask |=
> >> REGULATOR_CHANGE_MODE;
> >> + initdata->constraints.valid_modes_mask |=
> >> REGULATOR_MODE_NORMAL;
> >> + initdata->constraints.valid_modes_mask |=
> >> REGULATOR_MODE_IDLE;
> > No, this is just plain broken. Constraints are set by the *board*, you
> > don't know if these settings are safe on any given board.
> I can see that these are coming from board files, but I didn't find any
> example
> of how these are supposed to be set when using DT only.
> What's happening here is that given what compatible you use, different "parts"
> will be selected and based on that this code will or will not be executed;
> hence this is defined by what compatible you're using.
> But this is of course not obvious, so unless I've missed something I can clean
> this up slightly and make the connection to compatible more obvious. Okay?
No, it's still just plain broken. You've no idea if the settings you're
providing work or not on a given system (or set of drivers for that
matter) - mode configuration really is a part of the system integration
not an unchanging part of the PMIC. This code appears to assume that
every client driver (plus passives) is going to accurately supply load
information which just isn't realistic except in very controlled cases
for a specific system.
The reason it's not supported in DT at the minute is that the definition
of modes is not at all clear in a generic fashion, plus of course the
fact that we have no users in mainline for dynamic mode setting. Most
PMICs these days are smart enough to do this autonomously anyway so it's
not clear that this is something that it's worth spending time on.
Please look for the prior threads on this.
signature.asc
Description: Digital signature
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Wed, May 28, 2014 at 9:55 AM, Mark Brown wrote:
> On Tue, May 27, 2014 at 10:28:41AM -0700, Bjorn Andersson wrote:
>
>> +static int rpm_reg_set_voltage(struct regulator_dev *rdev,
>> +int min_uV, int max_uV,
>> +unsigned *selector)
>> +{
>> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>> + const struct rpm_reg_parts *parts = vreg->parts;
>> + const struct request_member *req;
>> + int ret = 0;
>> + int sel;
>> + int val;
>> + int uV;
>> +
>> + dev_dbg(vreg->dev, "set_voltage(%d, %d)\n", min_uV, max_uV);
>> +
>> + if (parts->uV.mask == 0 && parts->mV.mask == 0)
>> + return -EINVAL;
>> +
>> + /*
>> + * Snap to the voltage to a supported level.
>> + */
>
> What is "snapping" a voltage?
>
I pass the requested voltage to the RPM, but it's only allowed to have valid
values, so I need to "clamp"/"snap"/"round" the voltage to a valid number.
>> + sel = regulator_map_voltage_linear_range(rdev, min_uV, max_uV);
>
> Don't open code mapping the voltage, just implement set_voltage_sel().
>
I used ot open code this part, but changed it to
regulator_map_voltage_linear_range once I had implemented list_voltages. But as
you say I should be able to replace the first half of my set_voltage with
set_voltage_sel; and then do the transition from sel to requested voltage and
send that over.
[...]
>> + mutex_lock(>lock);
>> + if (parts->uV.mask)
>> + req = >uV;
>> + else if (parts->mV.mask)
>> + req = >mV;
>> + else
>> + req = >enable_state;
>
> Just implement separate operations for the regulators with different
> register definitions. It's going to simplify the code.
>
Currently I can use e.g. ldo_ops for all the different ldos, if I split this I
need to split the ops as well. So it will for sure be more code, but I will
give it a spin and see how it looks like.
>> +static int rpm_reg_set_mode(struct regulator_dev *rdev, unsigned int mode)
>> +{
>> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>> + const struct rpm_reg_parts *parts = vreg->parts;
>> + int max_mA = parts->ip.mask >> parts->ip.shift;
>> + int load_mA;
>> + int ret;
>> +
>> + if (mode == REGULATOR_MODE_IDLE)
>> + load_mA = vreg->idle_uA / 1000;
>> + else
>> + load_mA = vreg->normal_uA / 1000;
>> +
>> + if (load_mA > max_mA)
>> + load_mA = max_mA;
>> +
>> + mutex_lock(>lock);
>> + ret = rpm_reg_write(vreg, >ip, load_mA);
>> + if (!ret)
>> + vreg->mode = mode;
>> + mutex_unlock(>lock);
>
> I'm not entirely sure what this is intended to do. It looks like it's
> doing something to do with current limits but it's a set_mode(). Some
> documentation might help. It should also be implementing only specific
> modes and rejecting unsupported modes, if we do the same operation to
> the device for two different modes that seems wrong.
>
The hardware in this case is a "pmic" shared by all cpus in the system, so when
we set the voltage, state or load of a regulator we merely case a vote. For
voltage and state this is not an issue, but for load the value that's
interesting for the "pmic" is the sum of the votes; i.e. the sum of the loads
from all systems on a single regulator.
What the code does here is to follow the hack found at codeaurora, that upon
get_optimum_mode we guess that the client will call get_optimum_mode followed
my set_mode. We keep the track of what load was last requested and use that in
our vote.
One alternative might be to use "force-mode" to actually set the mode of the
regulator, but I will need to run that by the Qualcomm guys to get an answer if
that would work (as it's not used by codeaurora).
>> +static unsigned int rpm_reg_get_optimum_mode(struct regulator_dev *rdev,
>> + int input_uV,
>> + int output_uV,
>> + int load_uA)
>> +{
>> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>> +
>> + load_uA += vreg->load_bias_uA;
>> +
>> + if (load_uA < vreg->hpm_threshold_uA) {
>> + vreg->idle_uA = load_uA;
>> + return REGULATOR_MODE_IDLE;
>> + } else {
>> + vreg->normal_uA = load_uA;
>> + return REGULATOR_MODE_NORMAL;
>> + }
>> +}
>
> This looks very broken - why are we storing anything here? What is the
> stored value supposed to do?
>
See above.
>> + if (vreg->parts->ip.mask) {
>> + initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
>> + initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
>> + initdata->constraints.valid_modes_mask |=
>> REGULATOR_MODE_NORMAL;
>> + initdata->constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;
>
> No, this is just plain broken. Constraints are set by
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Wed, May 28, 2014 at 9:55 AM, Mark Brown broo...@kernel.org wrote:
On Tue, May 27, 2014 at 10:28:41AM -0700, Bjorn Andersson wrote:
+static int rpm_reg_set_voltage(struct regulator_dev *rdev,
+int min_uV, int max_uV,
+unsigned *selector)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+ const struct rpm_reg_parts *parts = vreg-parts;
+ const struct request_member *req;
+ int ret = 0;
+ int sel;
+ int val;
+ int uV;
+
+ dev_dbg(vreg-dev, set_voltage(%d, %d)\n, min_uV, max_uV);
+
+ if (parts-uV.mask == 0 parts-mV.mask == 0)
+ return -EINVAL;
+
+ /*
+ * Snap to the voltage to a supported level.
+ */
What is snapping a voltage?
I pass the requested voltage to the RPM, but it's only allowed to have valid
values, so I need to clamp/snap/round the voltage to a valid number.
+ sel = regulator_map_voltage_linear_range(rdev, min_uV, max_uV);
Don't open code mapping the voltage, just implement set_voltage_sel().
I used ot open code this part, but changed it to
regulator_map_voltage_linear_range once I had implemented list_voltages. But as
you say I should be able to replace the first half of my set_voltage with
set_voltage_sel; and then do the transition from sel to requested voltage and
send that over.
[...]
+ mutex_lock(vreg-lock);
+ if (parts-uV.mask)
+ req = parts-uV;
+ else if (parts-mV.mask)
+ req = parts-mV;
+ else
+ req = parts-enable_state;
Just implement separate operations for the regulators with different
register definitions. It's going to simplify the code.
Currently I can use e.g. ldo_ops for all the different ldos, if I split this I
need to split the ops as well. So it will for sure be more code, but I will
give it a spin and see how it looks like.
+static int rpm_reg_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+ const struct rpm_reg_parts *parts = vreg-parts;
+ int max_mA = parts-ip.mask parts-ip.shift;
+ int load_mA;
+ int ret;
+
+ if (mode == REGULATOR_MODE_IDLE)
+ load_mA = vreg-idle_uA / 1000;
+ else
+ load_mA = vreg-normal_uA / 1000;
+
+ if (load_mA max_mA)
+ load_mA = max_mA;
+
+ mutex_lock(vreg-lock);
+ ret = rpm_reg_write(vreg, parts-ip, load_mA);
+ if (!ret)
+ vreg-mode = mode;
+ mutex_unlock(vreg-lock);
I'm not entirely sure what this is intended to do. It looks like it's
doing something to do with current limits but it's a set_mode(). Some
documentation might help. It should also be implementing only specific
modes and rejecting unsupported modes, if we do the same operation to
the device for two different modes that seems wrong.
The hardware in this case is a pmic shared by all cpus in the system, so when
we set the voltage, state or load of a regulator we merely case a vote. For
voltage and state this is not an issue, but for load the value that's
interesting for the pmic is the sum of the votes; i.e. the sum of the loads
from all systems on a single regulator.
What the code does here is to follow the hack found at codeaurora, that upon
get_optimum_mode we guess that the client will call get_optimum_mode followed
my set_mode. We keep the track of what load was last requested and use that in
our vote.
One alternative might be to use force-mode to actually set the mode of the
regulator, but I will need to run that by the Qualcomm guys to get an answer if
that would work (as it's not used by codeaurora).
+static unsigned int rpm_reg_get_optimum_mode(struct regulator_dev *rdev,
+ int input_uV,
+ int output_uV,
+ int load_uA)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+
+ load_uA += vreg-load_bias_uA;
+
+ if (load_uA vreg-hpm_threshold_uA) {
+ vreg-idle_uA = load_uA;
+ return REGULATOR_MODE_IDLE;
+ } else {
+ vreg-normal_uA = load_uA;
+ return REGULATOR_MODE_NORMAL;
+ }
+}
This looks very broken - why are we storing anything here? What is the
stored value supposed to do?
See above.
+ if (vreg-parts-ip.mask) {
+ initdata-constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
+ initdata-constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
+ initdata-constraints.valid_modes_mask |=
REGULATOR_MODE_NORMAL;
+ initdata-constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;
No, this is just plain broken. Constraints are set by the *board*, you
don't know if these settings are safe on any given board.
I can see that these are coming from board files, but I didn't find any example
of
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 02:03:40PM -0700, Bjorn Andersson wrote:
Please fix your mailer to word wrap at less than 80 columns so quoted
text is legible.
The hardware in this case is a pmic shared by all cpus in the system, so
when
we set the voltage, state or load of a regulator we merely case a vote. For
voltage and state this is not an issue, but for load the value that's
interesting for the pmic is the sum of the votes; i.e. the sum of the loads
from all systems on a single regulator.
What the code does here is to follow the hack found at codeaurora, that upon
get_optimum_mode we guess that the client will call get_optimum_mode followed
my set_mode. We keep the track of what load was last requested and use that in
our vote.
No, this is awful and there's no way in hell that stuff like this should
be implemented in a driver since there's clearly nothing at all hardware
specific about it. The load tracking needs to be implemented in the
framework if it's going to be implemented, and passing it up through the
chain is obviously going to need some conversion and accounting for
hardware conversion losses which doesn't seem to be happening here.
I'm still unclear on what the summed current is going to be used for,
though...
+ if (vreg-parts-ip.mask) {
+ initdata-constraints.valid_ops_mask |=
REGULATOR_CHANGE_DRMS;
+ initdata-constraints.valid_ops_mask |=
REGULATOR_CHANGE_MODE;
+ initdata-constraints.valid_modes_mask |=
REGULATOR_MODE_NORMAL;
+ initdata-constraints.valid_modes_mask |=
REGULATOR_MODE_IDLE;
No, this is just plain broken. Constraints are set by the *board*, you
don't know if these settings are safe on any given board.
I can see that these are coming from board files, but I didn't find any
example
of how these are supposed to be set when using DT only.
What's happening here is that given what compatible you use, different parts
will be selected and based on that this code will or will not be executed;
hence this is defined by what compatible you're using.
But this is of course not obvious, so unless I've missed something I can clean
this up slightly and make the connection to compatible more obvious. Okay?
No, it's still just plain broken. You've no idea if the settings you're
providing work or not on a given system (or set of drivers for that
matter) - mode configuration really is a part of the system integration
not an unchanging part of the PMIC. This code appears to assume that
every client driver (plus passives) is going to accurately supply load
information which just isn't realistic except in very controlled cases
for a specific system.
The reason it's not supported in DT at the minute is that the definition
of modes is not at all clear in a generic fashion, plus of course the
fact that we have no users in mainline for dynamic mode setting. Most
PMICs these days are smart enough to do this autonomously anyway so it's
not clear that this is something that it's worth spending time on.
Please look for the prior threads on this.
signature.asc
Description: Digital signature
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 2:18 PM, Mark Brown broo...@kernel.org wrote:
On Thu, May 29, 2014 at 02:03:40PM -0700, Bjorn Andersson wrote:
Please fix your mailer to word wrap at less than 80 columns so quoted
text is legible.
The hardware in this case is a pmic shared by all cpus in the system, so
when
we set the voltage, state or load of a regulator we merely case a vote. For
voltage and state this is not an issue, but for load the value that's
interesting for the pmic is the sum of the votes; i.e. the sum of the loads
from all systems on a single regulator.
What the code does here is to follow the hack found at codeaurora, that upon
get_optimum_mode we guess that the client will call get_optimum_mode followed
my set_mode. We keep the track of what load was last requested and use that
in
our vote.
No, this is awful and there's no way in hell that stuff like this should
be implemented in a driver since there's clearly nothing at all hardware
specific about it. The load tracking needs to be implemented in the
framework if it's going to be implemented, and passing it up through the
chain is obviously going to need some conversion and accounting for
hardware conversion losses which doesn't seem to be happening here.
I'm still unclear on what the summed current is going to be used for,
though...
You do load accumlation of all the requests from the drivers of the Linux
system, but in the Qualcomm system there might be load from the modem or the
sensor co-processor that we don't know about here. So additional accumulation
is done by the pmic - that is directly accessed by those other systems as
well.
So here we don't set the mode of the regulator, we tell the pmic our part and
then it can accumulate further.
I understand your strong opinions regarding this, so I will respin this to
forcefully set the regulator mode intead of merely casting a vote. I.e.
implement set_mode to actually set the mode.
+ if (vreg-parts-ip.mask) {
+ initdata-constraints.valid_ops_mask |=
REGULATOR_CHANGE_DRMS;
+ initdata-constraints.valid_ops_mask |=
REGULATOR_CHANGE_MODE;
+ initdata-constraints.valid_modes_mask |=
REGULATOR_MODE_NORMAL;
+ initdata-constraints.valid_modes_mask |=
REGULATOR_MODE_IDLE;
No, this is just plain broken. Constraints are set by the *board*, you
don't know if these settings are safe on any given board.
I can see that these are coming from board files, but I didn't find any
example
of how these are supposed to be set when using DT only.
What's happening here is that given what compatible you use, different
parts
will be selected and based on that this code will or will not be executed;
hence this is defined by what compatible you're using.
But this is of course not obvious, so unless I've missed something I can
clean
this up slightly and make the connection to compatible more obvious. Okay?
No, it's still just plain broken. You've no idea if the settings you're
providing work or not on a given system (or set of drivers for that
matter) - mode configuration really is a part of the system integration
not an unchanging part of the PMIC. This code appears to assume that
every client driver (plus passives) is going to accurately supply load
information which just isn't realistic except in very controlled cases
for a specific system.
The reason it's not supported in DT at the minute is that the definition
of modes is not at all clear in a generic fashion, plus of course the
fact that we have no users in mainline for dynamic mode setting. Most
PMICs these days are smart enough to do this autonomously anyway so it's
not clear that this is something that it's worth spending time on.
Please look for the prior threads on this.
At the time of implementing this the proposed sdhci driver from Qualcomm called
regulator_set_mode, but that got redesigned not to fiddle with
regulators. So it
seems you're right, no-one ever calls regulator_set_mode in mainline
and
hence doesn't have this problem.
So the only example I can find is lp872x, that for buck* does this exactly like
I do.
But as there are no users anymore, I could just let the constraints part go for
now and once we've figured out the dt part there will be some way of setting
these. Okay?
Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Thu, May 29, 2014 at 02:59:38PM -0700, Bjorn Andersson wrote: On Thu, May 29, 2014 at 2:18 PM, Mark Brown broo...@kernel.org wrote: No, this is awful and there's no way in hell that stuff like this should be implemented in a driver since there's clearly nothing at all hardware specific about it. The load tracking needs to be implemented in the framework if it's going to be implemented, and passing it up through the chain is obviously going to need some conversion and accounting for hardware conversion losses which doesn't seem to be happening here. I'm still unclear on what the summed current is going to be used for, though... You do load accumlation of all the requests from the drivers of the Linux system, but in the Qualcomm system there might be load from the modem or the sensor co-processor that we don't know about here. So additional accumulation is done by the pmic - that is directly accessed by those other systems as well. So the resulting load is then set directly in hardware instead of setting a mode? That would be totally fine but it doesn't free us from having the logic for accumilating the current we know about in the core; that's the bit that's just at completely the wrong abstraction layer. I understand your strong opinions regarding this, so I will respin this to forcefully set the regulator mode intead of merely casting a vote. I.e. implement set_mode to actually set the mode. Or just don't implement mode setting if it's only used by this crazy stuff. But as there are no users anymore, I could just let the constraints part go for now and once we've figured out the dt part there will be some way of setting these. Okay? Yes, that's fine. signature.asc Description: Digital signature
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Tue, May 27, 2014 at 10:28:41AM -0700, Bjorn Andersson wrote:
> +static int rpm_reg_set_voltage(struct regulator_dev *rdev,
> +int min_uV, int max_uV,
> +unsigned *selector)
> +{
> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> + const struct rpm_reg_parts *parts = vreg->parts;
> + const struct request_member *req;
> + int ret = 0;
> + int sel;
> + int val;
> + int uV;
> +
> + dev_dbg(vreg->dev, "set_voltage(%d, %d)\n", min_uV, max_uV);
> +
> + if (parts->uV.mask == 0 && parts->mV.mask == 0)
> + return -EINVAL;
> +
> + /*
> + * Snap to the voltage to a supported level.
> + */
What is "snapping" a voltage?
> + sel = regulator_map_voltage_linear_range(rdev, min_uV, max_uV);
Don't open code mapping the voltage, just implement set_voltage_sel().
> + mutex_lock(>lock);
> + if (parts->uV.mask)
> + req = >uV;
> + else if (parts->mV.mask)
> + req = >mV;
> + else
> + req = >enable_state;
Just implement separate operations for the regulators with different
register definitions. It's going to simplify the code.
> +static int rpm_reg_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> + const struct rpm_reg_parts *parts = vreg->parts;
> + int max_mA = parts->ip.mask >> parts->ip.shift;
> + int load_mA;
> + int ret;
> +
> + if (mode == REGULATOR_MODE_IDLE)
> + load_mA = vreg->idle_uA / 1000;
> + else
> + load_mA = vreg->normal_uA / 1000;
> +
> + if (load_mA > max_mA)
> + load_mA = max_mA;
> +
> + mutex_lock(>lock);
> + ret = rpm_reg_write(vreg, >ip, load_mA);
> + if (!ret)
> + vreg->mode = mode;
> + mutex_unlock(>lock);
I'm not entirely sure what this is intended to do. It looks like it's
doing something to do with current limits but it's a set_mode(). Some
documentation might help. It should also be implementing only specific
modes and rejecting unsupported modes, if we do the same operation to
the device for two different modes that seems wrong.
> +static unsigned int rpm_reg_get_optimum_mode(struct regulator_dev *rdev,
> + int input_uV,
> + int output_uV,
> + int load_uA)
> +{
> + struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> +
> + load_uA += vreg->load_bias_uA;
> +
> + if (load_uA < vreg->hpm_threshold_uA) {
> + vreg->idle_uA = load_uA;
> + return REGULATOR_MODE_IDLE;
> + } else {
> + vreg->normal_uA = load_uA;
> + return REGULATOR_MODE_NORMAL;
> + }
> +}
This looks very broken - why are we storing anything here? What is the
stored value supposed to do?
> + if (vreg->parts->ip.mask) {
> + initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
> + initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> + initdata->constraints.valid_modes_mask |= REGULATOR_MODE_NORMAL;
> + initdata->constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;
No, this is just plain broken. Constraints are set by the *board*, you
don't know if these settings are safe on any given board.
> +static int __init rpm_reg_init(void)
> +{
> + return platform_driver_register(_reg_driver);
> +}
> +arch_initcall(rpm_reg_init);
> +
> +static void __exit rpm_reg_exit(void)
> +{
> + platform_driver_unregister(_reg_driver);
> +}
> +module_exit(rpm_reg_exit)
module_platform_driver() or if you must bodge the init order at least
use subsys_initcall() like everyone else.
signature.asc
Description: Digital signature
Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
On Tue, May 27, 2014 at 10:28:41AM -0700, Bjorn Andersson wrote:
+static int rpm_reg_set_voltage(struct regulator_dev *rdev,
+int min_uV, int max_uV,
+unsigned *selector)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+ const struct rpm_reg_parts *parts = vreg-parts;
+ const struct request_member *req;
+ int ret = 0;
+ int sel;
+ int val;
+ int uV;
+
+ dev_dbg(vreg-dev, set_voltage(%d, %d)\n, min_uV, max_uV);
+
+ if (parts-uV.mask == 0 parts-mV.mask == 0)
+ return -EINVAL;
+
+ /*
+ * Snap to the voltage to a supported level.
+ */
What is snapping a voltage?
+ sel = regulator_map_voltage_linear_range(rdev, min_uV, max_uV);
Don't open code mapping the voltage, just implement set_voltage_sel().
+ mutex_lock(vreg-lock);
+ if (parts-uV.mask)
+ req = parts-uV;
+ else if (parts-mV.mask)
+ req = parts-mV;
+ else
+ req = parts-enable_state;
Just implement separate operations for the regulators with different
register definitions. It's going to simplify the code.
+static int rpm_reg_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+ const struct rpm_reg_parts *parts = vreg-parts;
+ int max_mA = parts-ip.mask parts-ip.shift;
+ int load_mA;
+ int ret;
+
+ if (mode == REGULATOR_MODE_IDLE)
+ load_mA = vreg-idle_uA / 1000;
+ else
+ load_mA = vreg-normal_uA / 1000;
+
+ if (load_mA max_mA)
+ load_mA = max_mA;
+
+ mutex_lock(vreg-lock);
+ ret = rpm_reg_write(vreg, parts-ip, load_mA);
+ if (!ret)
+ vreg-mode = mode;
+ mutex_unlock(vreg-lock);
I'm not entirely sure what this is intended to do. It looks like it's
doing something to do with current limits but it's a set_mode(). Some
documentation might help. It should also be implementing only specific
modes and rejecting unsupported modes, if we do the same operation to
the device for two different modes that seems wrong.
+static unsigned int rpm_reg_get_optimum_mode(struct regulator_dev *rdev,
+ int input_uV,
+ int output_uV,
+ int load_uA)
+{
+ struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
+
+ load_uA += vreg-load_bias_uA;
+
+ if (load_uA vreg-hpm_threshold_uA) {
+ vreg-idle_uA = load_uA;
+ return REGULATOR_MODE_IDLE;
+ } else {
+ vreg-normal_uA = load_uA;
+ return REGULATOR_MODE_NORMAL;
+ }
+}
This looks very broken - why are we storing anything here? What is the
stored value supposed to do?
+ if (vreg-parts-ip.mask) {
+ initdata-constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
+ initdata-constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
+ initdata-constraints.valid_modes_mask |= REGULATOR_MODE_NORMAL;
+ initdata-constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;
No, this is just plain broken. Constraints are set by the *board*, you
don't know if these settings are safe on any given board.
+static int __init rpm_reg_init(void)
+{
+ return platform_driver_register(rpm_reg_driver);
+}
+arch_initcall(rpm_reg_init);
+
+static void __exit rpm_reg_exit(void)
+{
+ platform_driver_unregister(rpm_reg_driver);
+}
+module_exit(rpm_reg_exit)
module_platform_driver() or if you must bodge the init order at least
use subsys_initcall() like everyone else.
signature.asc
Description: Digital signature
[PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
Driver for regulators exposed by the Resource Power Manager (RPM) found in
Qualcomm 8660, 8960 and 8064 based devices.
Signed-off-by: Bjorn Andersson
---
drivers/regulator/Kconfig | 12 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom_rpm-regulator.c | 859 +
3 files changed, 872 insertions(+)
create mode 100644 drivers/regulator/qcom_rpm-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 903eb37..fb45d40 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -423,6 +423,18 @@ config REGULATOR_PFUZE100
Say y here to support the regulators found on the Freescale
PFUZE100/PFUZE200 PMIC.
+config REGULATOR_QCOM_RPM
+ tristate "Qualcomm RPM regulator driver"
+ depends on MFD_QCOM_RPM
+ help
+ If you say yes to this option, support will be included for the
+ regulators exposed by the Resource Power Manager found in Qualcomm
+ 8660, 8960 and 8064 based devices.
+
+ Say M here if you want to include support for the regulators on the
+ Qualcomm RPM as a module. The module will be named
+ "qcom_rpm-regulator".
+
config REGULATOR_RC5T583
tristate "RICOH RC5T583 Power regulators"
depends on MFD_RC5T583
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 12ef277..53b5ce8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_REGULATOR_MAX77693) += max77693.o
obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o
+obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
diff --git a/drivers/regulator/qcom_rpm-regulator.c
b/drivers/regulator/qcom_rpm-regulator.c
new file mode 100644
index 000..4ffbb64
--- /dev/null
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -0,0 +1,859 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define MAX_REQUEST_LEN 2
+#define HPM_DEFAULT_THRESHOLD 1
+
+struct request_member {
+ int word;
+ unsigned intmask;
+ int shift;
+};
+
+struct rpm_reg_parts {
+ struct request_member mV; /* used if voltage is in mV */
+ struct request_member uV; /* used if voltage is in uV */
+ struct request_member ip; /* peak current in mA */
+ struct request_member pd; /* pull down enable */
+ struct request_member ia; /* average current in mA */
+ struct request_member fm; /* force mode */
+ struct request_member pm; /* power mode */
+ struct request_member pc; /* pin control */
+ struct request_member pf; /* pin function */
+ struct request_member enable_state; /* NCP and switch */
+ struct request_member comp_mode;/* NCP */
+ struct request_member freq; /* frequency: NCP and SMPS */
+ struct request_member freq_clk_src; /* clock source: SMPS */
+ struct request_member hpm; /* switch: control OCP and SS */
+ int request_len;
+};
+
+#define FORCE_MODE_IS_2_BITS(reg) \
+ ((vreg->parts->fm.mask >> vreg->parts->fm.shift) == 3)
+#define FORCE_MODE_IS_3_BITS(reg) \
+ ((vreg->parts->fm.mask >> vreg->parts->fm.shift) == 7)
+
+struct qcom_rpm_reg {
+ struct qcom_rpm *rpm;
+
+ struct mutex lock;
+ struct device *dev;
+ struct regulator_desc desc;
+
+ const struct rpm_reg_parts *parts;
+
+ int resource;
+
+ int is_enabled;
+
+ u32 val[MAX_REQUEST_LEN];
+
+ int uV;
+ unsigned int mode;
+
+ int hpm_threshold_uA;
+ int load_bias_uA;
+ int normal_uA;
+ int idle_uA;
+};
+
+static const struct rpm_reg_parts rpm8660_ldo_parts = {
+ .request_len= 2,
+ .mV = { 0, 0x0FFF, 0 },
+ .ip = { 0, 0x00FFF000, 12 },
+ .fm = { 0, 0x0300, 24 },
[PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
Driver for regulators exposed by the Resource Power Manager (RPM) found in
Qualcomm 8660, 8960 and 8064 based devices.
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
drivers/regulator/Kconfig | 12 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom_rpm-regulator.c | 859 +
3 files changed, 872 insertions(+)
create mode 100644 drivers/regulator/qcom_rpm-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 903eb37..fb45d40 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -423,6 +423,18 @@ config REGULATOR_PFUZE100
Say y here to support the regulators found on the Freescale
PFUZE100/PFUZE200 PMIC.
+config REGULATOR_QCOM_RPM
+ tristate Qualcomm RPM regulator driver
+ depends on MFD_QCOM_RPM
+ help
+ If you say yes to this option, support will be included for the
+ regulators exposed by the Resource Power Manager found in Qualcomm
+ 8660, 8960 and 8064 based devices.
+
+ Say M here if you want to include support for the regulators on the
+ Qualcomm RPM as a module. The module will be named
+ qcom_rpm-regulator.
+
config REGULATOR_RC5T583
tristate RICOH RC5T583 Power regulators
depends on MFD_RC5T583
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 12ef277..53b5ce8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_REGULATOR_MAX77693) += max77693.o
obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o
+obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
diff --git a/drivers/regulator/qcom_rpm-regulator.c
b/drivers/regulator/qcom_rpm-regulator.c
new file mode 100644
index 000..4ffbb64
--- /dev/null
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -0,0 +1,859 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/regulator/driver.h
+#include linux/regulator/machine.h
+#include linux/regulator/of_regulator.h
+
+#include linux/mfd/qcom_rpm.h
+
+#define MAX_REQUEST_LEN 2
+#define HPM_DEFAULT_THRESHOLD 1
+
+struct request_member {
+ int word;
+ unsigned intmask;
+ int shift;
+};
+
+struct rpm_reg_parts {
+ struct request_member mV; /* used if voltage is in mV */
+ struct request_member uV; /* used if voltage is in uV */
+ struct request_member ip; /* peak current in mA */
+ struct request_member pd; /* pull down enable */
+ struct request_member ia; /* average current in mA */
+ struct request_member fm; /* force mode */
+ struct request_member pm; /* power mode */
+ struct request_member pc; /* pin control */
+ struct request_member pf; /* pin function */
+ struct request_member enable_state; /* NCP and switch */
+ struct request_member comp_mode;/* NCP */
+ struct request_member freq; /* frequency: NCP and SMPS */
+ struct request_member freq_clk_src; /* clock source: SMPS */
+ struct request_member hpm; /* switch: control OCP and SS */
+ int request_len;
+};
+
+#define FORCE_MODE_IS_2_BITS(reg) \
+ ((vreg-parts-fm.mask vreg-parts-fm.shift) == 3)
+#define FORCE_MODE_IS_3_BITS(reg) \
+ ((vreg-parts-fm.mask vreg-parts-fm.shift) == 7)
+
+struct qcom_rpm_reg {
+ struct qcom_rpm *rpm;
+
+ struct mutex lock;
+ struct device *dev;
+ struct regulator_desc desc;
+
+ const struct rpm_reg_parts *parts;
+
+ int resource;
+
+ int is_enabled;
+
+ u32 val[MAX_REQUEST_LEN];
+
+ int uV;
+ unsigned int mode;
+
+ int hpm_threshold_uA;
+ int load_bias_uA;
+ int normal_uA;
+ int idle_uA;
+};
+
+static const struct rpm_reg_parts rpm8660_ldo_parts = {
+
