| Message ID | 1598892515-30950-1-git-send-email-yibin.gong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] regulator: pca9450: add enable_val for all bucks | expand |
Hi Robin, On 20-09-01 00:48, Robin Gong wrote: > BuckX enable mode > 00b = OFF > 01b = ON by PMIC_ON_REQ = H > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L > 11b = Always ON > > For such enable mode, enable_value should be clearly set in requlator desc, > 00/11 is not enough, correct it now for different bucks. For example, buck2 > is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' after kernel > enter suspend, so should be set '10b' as ON, while others is '01b' as ON. > All are the same as the default setting which means bucks no need to be > enabled again during kernel boot even if they have been enabled already after > pmic on. I wouldn't hard-code the regulator behaviour because the behaviour comes from the system design which in most cases are comming from our hw-guys. Till now I saw a few intelligent designs don't following the pmic user recommendations to save money. I would love to specify the regulator behaviour/mode within the dt or acpi. > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > --- > drivers/regulator/pca9450-regulator.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c > index eb5822b..79f2a5a 100644 > --- a/drivers/regulator/pca9450-regulator.c > +++ b/drivers/regulator/pca9450-regulator.c > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_mask = BUCK1OUT_DVS0_MASK, > .enable_reg = PCA9450_REG_BUCK1CTRL, > .enable_mask = BUCK1_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > .of_parse_cb = pca9450_set_dvs_levels, > }, > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, > .vsel_mask = BUCK2OUT_DVS0_MASK, > .enable_reg = PCA9450_REG_BUCK2CTRL, > - .enable_mask = BUCK1_ENMODE_MASK, > + .enable_mask = BUCK2_ENMODE_MASK, Unrelated change? > + .enable_val = BUCK_ENMODE_ONREQ_STBYREQ, > .owner = THIS_MODULE, > .of_parse_cb = pca9450_set_dvs_levels, > }, > @@ -299,6 +301,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_mask = BUCK3OUT_DVS0_MASK, > .enable_reg = PCA9450_REG_BUCK3CTRL, > .enable_mask = BUCK3_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > .of_parse_cb = pca9450_set_dvs_levels, > }, > @@ -324,6 +327,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_mask = BUCK4OUT_MASK, > .enable_reg = PCA9450_REG_BUCK4CTRL, > .enable_mask = BUCK4_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > @@ -342,6 +346,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_mask = BUCK5OUT_MASK, > .enable_reg = PCA9450_REG_BUCK5CTRL, > .enable_mask = BUCK5_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > @@ -360,6 +365,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { > .vsel_mask = BUCK6OUT_MASK, > .enable_reg = PCA9450_REG_BUCK6CTRL, > .enable_mask = BUCK6_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > @@ -475,6 +481,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { > .vsel_mask = BUCK1OUT_DVS0_MASK, > .enable_reg = PCA9450_REG_BUCK1CTRL, > .enable_mask = BUCK1_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > .of_parse_cb = pca9450_set_dvs_levels, > }, > @@ -499,7 +506,8 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { > .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, > .vsel_mask = BUCK2OUT_DVS0_MASK, > .enable_reg = PCA9450_REG_BUCK2CTRL, > - .enable_mask = BUCK1_ENMODE_MASK, > + .enable_mask = BUCK2_ENMODE_MASK, Unrelated change? Regards, Marco > + .enable_val = BUCK_ENMODE_ONREQ_STBYREQ, > .owner = THIS_MODULE, > .of_parse_cb = pca9450_set_dvs_levels, > }, > @@ -525,6 +533,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { > .vsel_mask = BUCK4OUT_MASK, > .enable_reg = PCA9450_REG_BUCK4CTRL, > .enable_mask = BUCK4_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > @@ -543,6 +552,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { > .vsel_mask = BUCK5OUT_MASK, > .enable_reg = PCA9450_REG_BUCK5CTRL, > .enable_mask = BUCK5_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > @@ -561,6 +571,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { > .vsel_mask = BUCK6OUT_MASK, > .enable_reg = PCA9450_REG_BUCK6CTRL, > .enable_mask = BUCK6_ENMODE_MASK, > + .enable_val = BUCK_ENMODE_ONREQ, > .owner = THIS_MODULE, > }, > }, > -- > 2.7.4 >
On 2020/08/31 18:53 Marco Felsch <m.felsch@pengutronix.de> wrote: > Hi Robin, > > On 20-09-01 00:48, Robin Gong wrote: > > BuckX enable mode > > 00b = OFF > > 01b = ON by PMIC_ON_REQ = H > > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON > > > > For such enable mode, enable_value should be clearly set in requlator > > desc, > > 00/11 is not enough, correct it now for different bucks. For example, > > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' > > after kernel enter suspend, so should be set '10b' as ON, while others is '01b' > as ON. > > All are the same as the default setting which means bucks no need to > > be enabled again during kernel boot even if they have been enabled > > already after pmic on. > > I wouldn't hard-code the regulator behaviour because the behaviour comes > from the system design which in most cases are comming from our hw-guys. > Till now I saw a few intelligent designs don't following the pmic user > recommendations to save money. I would love to specify the regulator > behaviour/mode within the dt or acpi. Well, if so the better way is moving into dts. Will implement it in v2. > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > --- > > drivers/regulator/pca9450-regulator.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/regulator/pca9450-regulator.c > > b/drivers/regulator/pca9450-regulator.c > > index eb5822b..79f2a5a 100644 > > --- a/drivers/regulator/pca9450-regulator.c > > +++ b/drivers/regulator/pca9450-regulator.c > > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc > pca9450a_regulators[] = { > > .vsel_mask = BUCK1OUT_DVS0_MASK, > > .enable_reg = PCA9450_REG_BUCK1CTRL, > > .enable_mask = BUCK1_ENMODE_MASK, > > + .enable_val = BUCK_ENMODE_ONREQ, > > .owner = THIS_MODULE, > > .of_parse_cb = pca9450_set_dvs_levels, > > }, > > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc > pca9450a_regulators[] = { > > .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, > > .vsel_mask = BUCK2OUT_DVS0_MASK, > > .enable_reg = PCA9450_REG_BUCK2CTRL, > > - .enable_mask = BUCK1_ENMODE_MASK, > > + .enable_mask = BUCK2_ENMODE_MASK, > > Unrelated change? Yes, that's just correct it minor literal since that's BUCK2 although they're the same, will split it anyway.
On 2020/09/01 9:58 Robin Gong <yibin.gong@nxp.com> wrote: > On 2020/08/31 18:53 Marco Felsch <m.felsch@pengutronix.de> wrote: > > Hi Robin, > > > > On 20-09-01 00:48, Robin Gong wrote: > > > BuckX enable mode > > > 00b = OFF > > > 01b = ON by PMIC_ON_REQ = H > > > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON > > > > > > For such enable mode, enable_value should be clearly set in > > > requlator desc, > > > 00/11 is not enough, correct it now for different bucks. For > > > example, > > > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' > > > after kernel enter suspend, so should be set '10b' as ON, while others is > '01b' > > as ON. > > > All are the same as the default setting which means bucks no need to > > > be enabled again during kernel boot even if they have been enabled > > > already after pmic on. > > > > I wouldn't hard-code the regulator behaviour because the behaviour > > comes from the system design which in most cases are comming from our > hw-guys. > > Till now I saw a few intelligent designs don't following the pmic user > > recommendations to save money. I would love to specify the regulator > > behaviour/mode within the dt or acpi. > Well, if so the better way is moving into dts. Will implement it in v2. Hi Marco, Sorry, I'm afraid that big changes have to be made for moving it to dts, because current pca9450 driver is using common regulator_enable_regmap() which means rdev->desc is const and 'enable_value' can't be changed runtime with the value in dts. Regards to pca9450 is the specific pmic designed for i.mx8m family rather than generic pmic, the power rails defining should be the same as NXP MEK reference board, especially for the BUCKs which match well with SOC's power. So may I leave it at this time, and pick it up if the 'unbelievable trouble' really come out in the future? > > > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > > --- > > > drivers/regulator/pca9450-regulator.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/regulator/pca9450-regulator.c > > > b/drivers/regulator/pca9450-regulator.c > > > index eb5822b..79f2a5a 100644 > > > --- a/drivers/regulator/pca9450-regulator.c > > > +++ b/drivers/regulator/pca9450-regulator.c > > > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc > > pca9450a_regulators[] = { > > > .vsel_mask = BUCK1OUT_DVS0_MASK, > > > .enable_reg = PCA9450_REG_BUCK1CTRL, > > > .enable_mask = BUCK1_ENMODE_MASK, > > > + .enable_val = BUCK_ENMODE_ONREQ, > > > .owner = THIS_MODULE, > > > .of_parse_cb = pca9450_set_dvs_levels, > > > }, > > > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc > > pca9450a_regulators[] = { > > > .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, > > > .vsel_mask = BUCK2OUT_DVS0_MASK, > > > .enable_reg = PCA9450_REG_BUCK2CTRL, > > > - .enable_mask = BUCK1_ENMODE_MASK, > > > + .enable_mask = BUCK2_ENMODE_MASK, > > > > Unrelated change? > Yes, that's just correct it minor literal since that's BUCK2 although they're the > same, will split it anyway.
diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c index eb5822b..79f2a5a 100644 --- a/drivers/regulator/pca9450-regulator.c +++ b/drivers/regulator/pca9450-regulator.c @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_mask = BUCK1OUT_DVS0_MASK, .enable_reg = PCA9450_REG_BUCK1CTRL, .enable_mask = BUCK1_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, .of_parse_cb = pca9450_set_dvs_levels, }, @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, .vsel_mask = BUCK2OUT_DVS0_MASK, .enable_reg = PCA9450_REG_BUCK2CTRL, - .enable_mask = BUCK1_ENMODE_MASK, + .enable_mask = BUCK2_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ_STBYREQ, .owner = THIS_MODULE, .of_parse_cb = pca9450_set_dvs_levels, }, @@ -299,6 +301,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_mask = BUCK3OUT_DVS0_MASK, .enable_reg = PCA9450_REG_BUCK3CTRL, .enable_mask = BUCK3_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, .of_parse_cb = pca9450_set_dvs_levels, }, @@ -324,6 +327,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_mask = BUCK4OUT_MASK, .enable_reg = PCA9450_REG_BUCK4CTRL, .enable_mask = BUCK4_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, }, @@ -342,6 +346,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_mask = BUCK5OUT_MASK, .enable_reg = PCA9450_REG_BUCK5CTRL, .enable_mask = BUCK5_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, }, @@ -360,6 +365,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = { .vsel_mask = BUCK6OUT_MASK, .enable_reg = PCA9450_REG_BUCK6CTRL, .enable_mask = BUCK6_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, }, @@ -475,6 +481,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { .vsel_mask = BUCK1OUT_DVS0_MASK, .enable_reg = PCA9450_REG_BUCK1CTRL, .enable_mask = BUCK1_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, .of_parse_cb = pca9450_set_dvs_levels, }, @@ -499,7 +506,8 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { .vsel_reg = PCA9450_REG_BUCK2OUT_DVS0, .vsel_mask = BUCK2OUT_DVS0_MASK, .enable_reg = PCA9450_REG_BUCK2CTRL, - .enable_mask = BUCK1_ENMODE_MASK, + .enable_mask = BUCK2_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ_STBYREQ, .owner = THIS_MODULE, .of_parse_cb = pca9450_set_dvs_levels, }, @@ -525,6 +533,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { .vsel_mask = BUCK4OUT_MASK, .enable_reg = PCA9450_REG_BUCK4CTRL, .enable_mask = BUCK4_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, }, @@ -543,6 +552,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { .vsel_mask = BUCK5OUT_MASK, .enable_reg = PCA9450_REG_BUCK5CTRL, .enable_mask = BUCK5_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, }, @@ -561,6 +571,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = { .vsel_mask = BUCK6OUT_MASK, .enable_reg = PCA9450_REG_BUCK6CTRL, .enable_mask = BUCK6_ENMODE_MASK, + .enable_val = BUCK_ENMODE_ONREQ, .owner = THIS_MODULE, }, },
BuckX enable mode 00b = OFF 01b = ON by PMIC_ON_REQ = H 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON For such enable mode, enable_value should be clearly set in requlator desc, 00/11 is not enough, correct it now for different bucks. For example, buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' after kernel enter suspend, so should be set '10b' as ON, while others is '01b' as ON. All are the same as the default setting which means bucks no need to be enabled again during kernel boot even if they have been enabled already after pmic on. Signed-off-by: Robin Gong <yibin.gong@nxp.com> --- drivers/regulator/pca9450-regulator.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)