Re: [PATCH 3/7] regulator: qcom-rpmh: Correct the pmic5_hfsmps515 buck

[skakit@codeaurora.org]

On 2021-03-02 19:51, Dmitry Baryshkov wrote:
> Hello,
> 
> On Mon, 1 Mar 2021 at 13:37, <skakit@codeaurora.org> wrote:
>> 
>> On 2021-02-26 15:57, Dmitry Baryshkov wrote:
>> > On Fri, 26 Feb 2021 at 09:59, <skakit@codeaurora.org> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On 2021-02-25 16:39, Dmitry Baryshkov wrote:
>> >> > On 24/02/2021 11:33, satya priya wrote:
>> >> >> Correct the REGULATOR_LINEAR_RANGE and n_voltges for
>> >> >> pmic5_hfsmps515 buck.
>> >> >>
>> >> >> Signed-off-by: satya priya <skakit@codeaurora.org>
>> >> >> ---
>> >> >>   drivers/regulator/qcom-rpmh-regulator.c | 4 ++--
>> >> >>   1 file changed, 2 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/regulator/qcom-rpmh-regulator.c
>> >> >> b/drivers/regulator/qcom-rpmh-regulator.c
>> >> >> index 79a554f..36542c3 100644
>> >> >> --- a/drivers/regulator/qcom-rpmh-regulator.c
>> >> >> +++ b/drivers/regulator/qcom-rpmh-regulator.c
>> >> >> @@ -726,8 +726,8 @@ static const struct rpmh_vreg_hw_data
>> >> >> pmic5_ftsmps510 = {
>> >> >>   static const struct rpmh_vreg_hw_data pmic5_hfsmps515 = {
>> >> >>      .regulator_type = VRM,
>> >> >>      .ops = &rpmh_regulator_vrm_ops,
>> >> >> -    .voltage_range = REGULATOR_LINEAR_RANGE(2800000, 0, 4, 16000),
>> >> >> -    .n_voltages = 5,
>> >> >> +    .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 235, 16000),
>> >> >> +    .n_voltages = 236,
>> >> >
>> >> > I've checked the docs for pm8009, the chip which also uses hfsmps515
>> >> > regulators. The pdf clearly states that the 'Output voltage operating
>> >> > range' is from 2.8 V to 2.85 V.
>> >> >
>> >> > So we'd probably need to define different versions of HFS515 regulator
>> >> > data (like I had to create for pm8009-1).
>> >> >
>> >> >
>> >>
>> >> The min-max voltages for S1C (PM8350c) regulator are 2190000-2210000uV
>> >> for sc7280(kodiak), so we had to modify this buck to support this
>> >> regulator.
>> >>
>> >> AFAIK, this struct defines the HW constraints of a regulator, but the
>> >> platform specific min-max values can be controlled from DT files. So,
>> >> can't we modify it like above instead of adding a new definition? the
>> >> new min_uV value (32000) is anyway not exceeding the old value
>> >> (2800000)
>> >> right? please correct me if wrong.
>> >
>> > As far as I understand for other regulators we put 'output voltage
>> > limitations' from the docs into the regulator definition and further
>> > constrain it by the platform device tree. Please correct me if I'm
>> > wrong.
>> 
>> I see that for most of the regulators, these specifications are 
>> specific
>> to regulator buck (like HFS515) but not chipset specific, we set the
>> chipset specific(like pm8009/pm8350c) requirements from DT files.
>> 
>> For example:
>> pmic5_nldo regulator spec mentions LLIMIT= 0.32V and ULIMIT =1.304V 
>> with
>> step 8mV
>> 
>> .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 123, 8000),
>> max output voltage supported by this regulator is 123*8000 + 320000 =
>> 1304000mV which is same as mentioned in the regulator spec.
>> 
>> > For pm8009 the data from the datasheet matches the regulators defined
>> > in the source file. Unfortunately I don't have kodiak specs at hand.
>> 
>>  From the HFS515 spec I got below info
>> "HFS510 and lower max output voltage is limited to 2.04V max, and
>> Yoda(pm8009) requirement was 2.4V for IOT PA and 2.85V for camera
>> application.  Hence, HFS515 added a new register and corresponding HW
>> changes to support the higher voltage.  Table 5‑24 shows the new
>> FB_RANGE bit.  When configured to 0 the buck works as earlier where 
>> Vout
>> max = 2.04V in 8mV steps, but when configured to 1 the buck range
>> doubles and can now support a Vout max = 4.08V in 16mV steps."
>> 
>> As per above, the max output voltage supported by HFS515 buck is 4.08V
>> (which is kodiak pm8350c pmic's requirement).
>> So, we have modified the buck data to support pm8350c(palani) along 
>> with
>> pm8009(yoda).
> 
> I'd still prefer to have two different regulator types (as we did for
> pm8009 P=0 and P=1 variants). However it's probably up to the
> maintainers to decide.

As Mark already picked this, I think we can leave it this way.

Thanks,
Satya Priya