[PATCH 0/1] regulators: bd718x7: Add enable times

[PATCH 0/1] regulators: bd718x7: Add enable times

[Guido Günther]

Use the typical startup times from the data sheet so boards get a
reasonable default. Not setting any enable time can lead to board hangs
when e.g. clocks are enabled too soon afterwards.

This fixes gpu power domain resume on the Librem 5.

Guido Günther (1):
  regulators: bd718x7: Add enable times

 drivers/regulator/bd718x7-regulator.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

-- 
2.29.2

[PATCH 1/1] regulators: bd718x7: Add enable times

[Guido Günther]

Use the typical startup times from the data sheet so boards get a
reasonable default. Not setting any enable time can lead to board hangs
when e.g. clocks are enabled too soon afterwards.

This fixes gpu power domain resume on the Librem 5.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/regulator/bd718x7-regulator.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index e6d5d98c3cea..d6d34aa4ee2e 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -613,6 +613,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 144,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -646,6 +647,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 162,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -680,6 +682,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.linear_range_selectors = bd71847_buck3_volt_range_sel,
 			.enable_reg = BD718XX_REG_1ST_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 162,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -706,6 +709,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_range_mask = BD71847_BUCK4_RANGE_MASK,
 			.linear_range_selectors = bd71847_buck4_volt_range_sel,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 240,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -727,6 +731,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = BD718XX_3RD_NODVS_BUCK_MASK,
 			.enable_reg = BD718XX_REG_3RD_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 220,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -750,6 +755,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = BD718XX_4TH_NODVS_BUCK_MASK,
 			.enable_reg = BD718XX_REG_4TH_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 200,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -775,6 +781,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.linear_range_selectors = bd718xx_ldo1_volt_range_sel,
 			.enable_reg = BD718XX_REG_LDO1_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 440,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -796,6 +803,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.n_voltages = ARRAY_SIZE(ldo_2_volts),
 			.enable_reg = BD718XX_REG_LDO2_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 370,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -818,6 +826,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = BD718XX_LDO3_MASK,
 			.enable_reg = BD718XX_REG_LDO3_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 310,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -840,6 +849,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = BD718XX_LDO4_MASK,
 			.enable_reg = BD718XX_REG_LDO4_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 400,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -865,6 +875,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.linear_range_selectors = bd71847_ldo5_volt_range_sel,
 			.enable_reg = BD718XX_REG_LDO5_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 530,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -889,6 +900,7 @@ static struct bd718xx_regulator_data bd71847_regulators[] = {
 			.vsel_mask = BD718XX_LDO6_MASK,
 			.enable_reg = BD718XX_REG_LDO6_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 400,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -942,6 +954,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 160,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -975,6 +988,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 180,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -1005,6 +1019,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD71837_REG_BUCK3_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 180,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -1033,6 +1048,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = DVS_BUCK_RUN_MASK,
 			.enable_reg = BD71837_REG_BUCK4_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 180,
 			.owner = THIS_MODULE,
 			.of_parse_cb = buck_set_hw_dvs_levels,
 		},
@@ -1065,6 +1081,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.linear_range_selectors = bd71837_buck5_volt_range_sel,
 			.enable_reg = BD718XX_REG_1ST_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 160,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1088,6 +1105,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD71837_BUCK6_MASK,
 			.enable_reg = BD718XX_REG_2ND_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 240,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1109,6 +1127,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD718XX_3RD_NODVS_BUCK_MASK,
 			.enable_reg = BD718XX_REG_3RD_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 220,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1132,6 +1151,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD718XX_4TH_NODVS_BUCK_MASK,
 			.enable_reg = BD718XX_REG_4TH_NODVS_BUCK_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
+			.enable_time = 200,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1157,6 +1177,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.linear_range_selectors = bd718xx_ldo1_volt_range_sel,
 			.enable_reg = BD718XX_REG_LDO1_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 440,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1178,6 +1199,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.n_voltages = ARRAY_SIZE(ldo_2_volts),
 			.enable_reg = BD718XX_REG_LDO2_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 370,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1200,6 +1222,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD718XX_LDO3_MASK,
 			.enable_reg = BD718XX_REG_LDO3_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 310,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1222,6 +1245,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD718XX_LDO4_MASK,
 			.enable_reg = BD718XX_REG_LDO4_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 400,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1246,6 +1270,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD71837_LDO5_MASK,
 			.enable_reg = BD718XX_REG_LDO5_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 310,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1272,6 +1297,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD718XX_LDO6_MASK,
 			.enable_reg = BD718XX_REG_LDO6_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 400,
 			.owner = THIS_MODULE,
 		},
 		.init = {
@@ -1296,6 +1322,7 @@ static struct bd718xx_regulator_data bd71837_regulators[] = {
 			.vsel_mask = BD71837_LDO7_MASK,
 			.enable_reg = BD71837_REG_LDO7_VOLT,
 			.enable_mask = BD718XX_LDO_EN,
+			.enable_time = 530,
 			.owner = THIS_MODULE,
 		},
 		.init = {
-- 
2.29.2

Re: [PATCH 1/1] regulators: bd718x7: Add enable times

[Vaittinen, Matti]

Hello Guido,

Thanks for looking at this!

On Wed, 2020-12-16 at 12:05 +0100, Guido Günther wrote:
> Use the typical startup times from the data sheet so boards get a
> reasonable default. Not setting any enable time can lead to board
> hangs
> when e.g. clocks are enabled too soon afterwards.
> 
> This fixes gpu power domain resume on the Librem 5.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  drivers/regulator/bd718x7-regulator.c | 27
> +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/regulator/bd718x7-regulator.c
> b/drivers/regulator/bd718x7-regulator.c
> index e6d5d98c3cea..d6d34aa4ee2e 100644
> --- a/drivers/regulator/bd718x7-regulator.c
> +++ b/drivers/regulator/bd718x7-regulator.c
> @@ -613,6 +613,7 @@ static struct bd718xx_regulator_data
> bd71847_regulators[] = {
>  			.vsel_mask = DVS_BUCK_RUN_MASK,
>  			.enable_reg = BD718XX_REG_BUCK1_CTRL,
>  			.enable_mask = BD718XX_BUCK_EN,
> +			.enable_time = 144,

Where are these values obtained from? I have a feeling they might be
board / load specific. If this is the case - can the "regulator-enable-
ramp-delay" from device-tree be used instead to avoid hard-coding board
specific values in the driver? Although, sane defaults would probably
not be a bad idea - if I read code correctly then the constrains from
DT can be used to override these values.

I'd prefer well named defines over raw numeric values though.

Best Regards
    Matti Vaittinen

Re: [PATCH 1/1] regulators: bd718x7: Add enable times

[Guido Günther]

Hi Matti,
On Wed, Dec 16, 2020 at 12:29:20PM +0000, Vaittinen, Matti wrote:
> Hello Guido,
> 
> Thanks for looking at this!
> 
> On Wed, 2020-12-16 at 12:05 +0100, Guido Günther wrote:
> > Use the typical startup times from the data sheet so boards get a
> > reasonable default. Not setting any enable time can lead to board
> > hangs
> > when e.g. clocks are enabled too soon afterwards.
> > 
> > This fixes gpu power domain resume on the Librem 5.
> > 
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > ---
> >  drivers/regulator/bd718x7-regulator.c | 27
> > +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/regulator/bd718x7-regulator.c
> > b/drivers/regulator/bd718x7-regulator.c
> > index e6d5d98c3cea..d6d34aa4ee2e 100644
> > --- a/drivers/regulator/bd718x7-regulator.c
> > +++ b/drivers/regulator/bd718x7-regulator.c
> > @@ -613,6 +613,7 @@ static struct bd718xx_regulator_data
> > bd71847_regulators[] = {
> >  			.vsel_mask = DVS_BUCK_RUN_MASK,
> >  			.enable_reg = BD718XX_REG_BUCK1_CTRL,
> >  			.enable_mask = BD718XX_BUCK_EN,
> > +			.enable_time = 144,
> 
> Where are these values obtained from? I have a feeling they might be
> board / load specific. If this is the case - can the "regulator-enable-
> ramp-delay" from device-tree be used instead to avoid hard-coding board
> specific values in the driver? Although, sane defaults would probably
> not be a bad idea - if I read code correctly then the constrains from
> DT can be used to override these values.

They're the 'typical values' from the data sheet and it's basically all
about setting a default for "regulator-enable-ramp-delay" to avoid
having every board do the same. If that's not the right thing todo let
me know and i add these to each of our boards (which is where i
basically started from but then figured that this would be busywork
and every board would hit that problem).

> I'd prefer well named defines over raw numeric values though.

So s.th. like

BD71837_BUCK1_STARTUP_TIME 144

(using the terminology from the datasheet)? If that works I'll send a
v2.

Cheers,
 --Guido

> 
> Best Regards
>     Matti Vaittinen
> 
> 

Re: [PATCH 1/1] regulators: bd718x7: Add enable times

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Wed, Dec 16, 2020 at 01:41:33PM +0100, Guido Günther wrote:
> On Wed, Dec 16, 2020 at 12:29:20PM +0000, Vaittinen, Matti wrote:

> > > +			.enable_time = 144,

> > Where are these values obtained from? I have a feeling they might be
> > board / load specific. If this is the case - can the "regulator-enable-
> > ramp-delay" from device-tree be used instead to avoid hard-coding board
> > specific values in the driver? Although, sane defaults would probably
> > not be a bad idea - if I read code correctly then the constrains from
> > DT can be used to override these values.

> They're the 'typical values' from the data sheet and it's basically all
> about setting a default for "regulator-enable-ramp-delay" to avoid
> having every board do the same. If that's not the right thing todo let
> me know and i add these to each of our boards (which is where i
> basically started from but then figured that this would be busywork
> and every board would hit that problem).

If it's a DCDC they're *probably* generic values rather than board
specific, they tend to be more predictable as they're regulating much
more actively than LDOs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/1] regulators: bd718x7: Add enable times

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Wed, Dec 16, 2020 at 12:05:53PM +0100, Guido Günther wrote:
> Use the typical startup times from the data sheet so boards get a
> reasonable default. Not setting any enable time can lead to board hangs
> when e.g. clocks are enabled too soon afterwards.

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] regulators: bd718x7: Add enable times

[Vaittinen, Matti]

On Wed, 2020-12-16 at 12:48 +0000, Mark Brown wrote:
> On Wed, Dec 16, 2020 at 01:41:33PM +0100, Guido Günther wrote:
> > On Wed, Dec 16, 2020 at 12:29:20PM +0000, Vaittinen, Matti wrote:
> > > > +			.enable_time = 144,
> > > Where are these values obtained from? I have a feeling they might
> > > be
> > > board / load specific. If this is the case - can the "regulator-
> > > enable-
> > > ramp-delay" from device-tree be used instead to avoid hard-coding 
> > > board
> > > specific values in the driver? Although, sane defaults would
> > > probably
> > > not be a bad idea - if I read code correctly then the constrains
> > > from
> > > DT can be used to override these values.
> > They're the 'typical values' from the data sheet and it's basically
> > all
> > about setting a default for "regulator-enable-ramp-delay" to avoid
> > having every board do the same. If that's not the right thing todo
> > let
> > me know and i add these to each of our boards (which is where i
> > basically started from but then figured that this would be busywork
> > and every board would hit that problem).
> 
> If it's a DCDC they're *probably* generic values rather than board
> specific, they tend to be more predictable as they're regulating much
> more actively than LDOs.

Thank you for explanation Mark & Guido.
Then I'd say the 'default' enable times do make sense. Thanks for
improving the driver!

Best Regards
	Matti Vaittinen

Re: [PATCH 1/1] regulators: bd718x7: Add enable times

[Vaittinen, Matti]

On Wed, 2020-12-16 at 13:41 +0100, Guido Günther wrote:
> Hi Matti,
> On Wed, Dec 16, 2020 at 12:29:20PM +0000, Vaittinen, Matti wrote:
> > Hello Guido,
> > 
> > Thanks for looking at this!
> > 
> > On Wed, 2020-12-16 at 12:05 +0100, Guido Günther wrote:
> > > Use the typical startup times from the data sheet so boards get a
> > > reasonable default. Not setting any enable time can lead to board
> > > hangs
> > > when e.g. clocks are enabled too soon afterwards.
> > > 
> > > This fixes gpu power domain resume on the Librem 5.
> > > 
> > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > ---
> > >  drivers/regulator/bd718x7-regulator.c | 27
> > > +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/drivers/regulator/bd718x7-regulator.c
> > > b/drivers/regulator/bd718x7-regulator.c
> > > index e6d5d98c3cea..d6d34aa4ee2e 100644
> > > --- a/drivers/regulator/bd718x7-regulator.c
> > > +++ b/drivers/regulator/bd718x7-regulator.c
> > > @@ -613,6 +613,7 @@ static struct bd718xx_regulator_data
> > > bd71847_regulators[] = {
> > >  			.vsel_mask = DVS_BUCK_RUN_MASK,
> > >  			.enable_reg = BD718XX_REG_BUCK1_CTRL,
> > >  			.enable_mask = BD718XX_BUCK_EN,
> > > +			.enable_time = 144,
> > 
> > Where are these values obtained from? I have a feeling they might
> > be
> > board / load specific. If this is the case - can the "regulator-
> > enable-
> > ramp-delay" from device-tree be used instead to avoid hard-coding
> > board
> > specific values in the driver? Although, sane defaults would
> > probably
> > not be a bad idea - if I read code correctly then the constrains
> > from
> > DT can be used to override these values.
> 
> They're the 'typical values' from the data sheet and it's basically
> all
> about setting a default for "regulator-enable-ramp-delay" to avoid
> having every board do the same. If that's not the right thing todo
> let
> me know and i add these to each of our boards (which is where i
> basically started from but then figured that this would be busywork
> and every board would hit that problem).
> 
> > I'd prefer well named defines over raw numeric values though.
> 
> So s.th. like
> 
> BD71837_BUCK1_STARTUP_TIME 144
> 
> (using the terminology from the datasheet)? If that works I'll send a
> v2.

Just noticed I never replied. Sorry. This looks good to me!

Best Regards
  --Matti