| Message ID | 20210721140424.725744-53-maxime@cerno.tech |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: dts: Last round of DT schema fixes | expand |
On 7/21/21 9:04 AM, Maxime Ripard wrote: > The regulator-ramp-delay property isn't documented in the binding for > the AXP806, and it's ignored by the driver. Remove those properties. This is a generic regulator property, parsed by of_get_regulation_constraints, which is called by regulator_of_get_init_data in the regulator core. And it appears in bindings/regulator/regulator.yaml. I believe the binding needs to be fixed, not the device trees. Regards, Samuel > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 -- > arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts | 2 -- > arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts | 2 -- > 3 files changed, 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts > index 6249e9e02928..a02644eebbe4 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts > @@ -256,7 +256,6 @@ reg_dcdca: dcdca { > regulator-always-on; > regulator-min-microvolt = <810000>; > regulator-max-microvolt = <1160000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-cpu"; > }; > > @@ -264,7 +263,6 @@ reg_dcdcc: dcdcc { > regulator-enable-ramp-delay = <32000>; > regulator-min-microvolt = <810000>; > regulator-max-microvolt = <1080000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-gpu"; > }; > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts > index c45d7b7fb39a..69c0293aeb16 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts > @@ -262,7 +262,6 @@ reg_dcdca: dcdca { > regulator-always-on; > regulator-min-microvolt = <800000>; > regulator-max-microvolt = <1160000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-cpu"; > }; > > @@ -270,7 +269,6 @@ reg_dcdcc: dcdcc { > regulator-enable-ramp-delay = <32000>; > regulator-min-microvolt = <810000>; > regulator-max-microvolt = <1080000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-gpu"; > }; > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts > index 1ffd68f43f87..6a1ee4232675 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts > @@ -245,7 +245,6 @@ reg_dcdca: dcdca { > regulator-always-on; > regulator-min-microvolt = <810000>; > regulator-max-microvolt = <1160000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-cpu"; > }; > > @@ -253,7 +252,6 @@ reg_dcdcc: dcdcc { > regulator-enable-ramp-delay = <32000>; > regulator-min-microvolt = <810000>; > regulator-max-microvolt = <1080000>; > - regulator-ramp-delay = <2500>; > regulator-name = "vdd-gpu"; > }; > >
On Thu, Jul 22, 2021 at 12:55:53AM -0500, Samuel Holland wrote: > On 7/21/21 9:04 AM, Maxime Ripard wrote: > > The regulator-ramp-delay property isn't documented in the binding for > > the AXP806, and it's ignored by the driver. Remove those properties. > > This is a generic regulator property, parsed by > of_get_regulation_constraints, which is called by > regulator_of_get_init_data in the regulator core. And it appears in > bindings/regulator/regulator.yaml. I believe the binding needs to be > fixed, not the device trees. It's indeed parsed by the regulator framework, but then it calls into the driver if that property is set using set_ramp_delay if it's set. https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1378 We don't set that hook for the AXP806 DCDC-A and DCDC-E regulators (that use AXP_DESC_RANGES) at all: https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L343 And the only implementation we have (set for AXP_DESC and AXP_DESC_IO) works only for the AXP209: https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L368 So, it just looks like those properties have never been tested since they were just ignored. Maxime
在 2021-07-22星期四的 10:16 +0200，Maxime Ripard写道： > On Thu, Jul 22, 2021 at 12:55:53AM -0500, Samuel Holland wrote: > > On 7/21/21 9:04 AM, Maxime Ripard wrote: > > > The regulator-ramp-delay property isn't documented in the binding > > > for > > > the AXP806, and it's ignored by the driver. Remove those > > > properties. > > > > This is a generic regulator property, parsed by > > of_get_regulation_constraints, which is called by > > regulator_of_get_init_data in the regulator core. And it appears in > > bindings/regulator/regulator.yaml. I believe the binding needs to be > > fixed, not the device trees. > > It's indeed parsed by the regulator framework, but then it calls into > the driver if that property is set using set_ramp_delay if it's set. Not only is it used for set_ramp_delay, but it's also used to calculate a post-change delay, see the following position (it can be overrided by a custom set_voltage_time in the driver): https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L3339 > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1378 > > We don't set that hook for the AXP806 DCDC-A and DCDC-E regulators > (that > use AXP_DESC_RANGES) at all: > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L343 > > And the only implementation we have (set for AXP_DESC and AXP_DESC_IO) > works only for the AXP209: > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L368 > > So, it just looks like those properties have never been tested since > they were just ignored. > > Maxime >
On Fri, Aug 6, 2021 at 7:49 PM Icenowy Zheng <icenowy@aosc.io> wrote: > > 在 2021-07-22星期四的 10:16 +0200，Maxime Ripard写道： > > On Thu, Jul 22, 2021 at 12:55:53AM -0500, Samuel Holland wrote: > > > On 7/21/21 9:04 AM, Maxime Ripard wrote: > > > > The regulator-ramp-delay property isn't documented in the binding > > > > for > > > > the AXP806, and it's ignored by the driver. Remove those > > > > properties. > > > > > > This is a generic regulator property, parsed by > > > of_get_regulation_constraints, which is called by > > > regulator_of_get_init_data in the regulator core. And it appears in > > > bindings/regulator/regulator.yaml. I believe the binding needs to be > > > fixed, not the device trees. > > > > It's indeed parsed by the regulator framework, but then it calls into > > the driver if that property is set using set_ramp_delay if it's set. > > Not only is it used for set_ramp_delay, but it's also used to calculate > a post-change delay, see the following position (it can be overrided by > a custom set_voltage_time in the driver): > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L3339 Having just dug through the regulator core code at work, I agree. Furthermore, the commit log for this addition specifically mentions the reason for adding this property is to provide a (guessed) ramp rate for the core to do a proper delay for the regulator to ramp up, not to set the actual ramp rate in hardware. ChenYu [1] https://git.kernel.org/torvalds/c/fe79ea577be81e1e71642826ab00e676dc59c194 > > > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1378 > > > > We don't set that hook for the AXP806 DCDC-A and DCDC-E regulators > > (that > > use AXP_DESC_RANGES) at all: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L343 > > > > And the only implementation we have (set for AXP_DESC and AXP_DESC_IO) > > works only for the AXP209: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L368 > > > > So, it just looks like those properties have never been tested since > > they were just ignored. > > > > Maxime > > > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/68e4820ead3107aa4e80dcaf9243bd11de5fce98.camel%40aosc.io.
于 2021年8月6日 GMT+08:00 下午8:05:56, Chen-Yu Tsai <wens@csie.org> 写到: >On Fri, Aug 6, 2021 at 7:49 PM Icenowy Zheng <icenowy@aosc.io> wrote: >> >> 在 2021-07-22星期四的 10:16 +0200，Maxime Ripard写道： >> > On Thu, Jul 22, 2021 at 12:55:53AM -0500, Samuel Holland wrote: >> > > On 7/21/21 9:04 AM, Maxime Ripard wrote: >> > > > The regulator-ramp-delay property isn't documented in the binding >> > > > for >> > > > the AXP806, and it's ignored by the driver. Remove those >> > > > properties. >> > > >> > > This is a generic regulator property, parsed by >> > > of_get_regulation_constraints, which is called by >> > > regulator_of_get_init_data in the regulator core. And it appears in >> > > bindings/regulator/regulator.yaml. I believe the binding needs to be >> > > fixed, not the device trees. >> > >> > It's indeed parsed by the regulator framework, but then it calls into >> > the driver if that property is set using set_ramp_delay if it's set. >> >> Not only is it used for set_ramp_delay, but it's also used to calculate >> a post-change delay, see the following position (it can be overrided by >> a custom set_voltage_time in the driver): >> >> https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L3339 > >Having just dug through the regulator core code at work, I agree. > >Furthermore, the commit log for this addition specifically mentions the >reason for adding this property is to provide a (guessed) ramp rate for >the core to do a proper delay for the regulator to ramp up, not to set >the actual ramp rate in hardware. Well we must agree that we have some more suitable property that delays for a constant span, see lines below in the function I mentioned. > > >ChenYu > > >[1] https://git.kernel.org/torvalds/c/fe79ea577be81e1e71642826ab00e676dc59c194 > >> > >> > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1378 >> > >> > We don't set that hook for the AXP806 DCDC-A and DCDC-E regulators >> > (that >> > use AXP_DESC_RANGES) at all: >> > >> > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L343 >> > >> > And the only implementation we have (set for AXP_DESC and AXP_DESC_IO) >> > works only for the AXP209: >> > >> > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/axp20x-regulator.c#L368 >> > >> > So, it just looks like those properties have never been tested since >> > they were just ignored. >> > >> > Maxime >> > >> >> -- >> You received this message because you are subscribed to the Google Groups "linux-sunxi" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. >> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/68e4820ead3107aa4e80dcaf9243bd11de5fce98.camel%40aosc.io. >
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts index 6249e9e02928..a02644eebbe4 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts @@ -256,7 +256,6 @@ reg_dcdca: dcdca { regulator-always-on; regulator-min-microvolt = <810000>; regulator-max-microvolt = <1160000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-cpu"; }; @@ -264,7 +263,6 @@ reg_dcdcc: dcdcc { regulator-enable-ramp-delay = <32000>; regulator-min-microvolt = <810000>; regulator-max-microvolt = <1080000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-gpu"; }; diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts index c45d7b7fb39a..69c0293aeb16 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts @@ -262,7 +262,6 @@ reg_dcdca: dcdca { regulator-always-on; regulator-min-microvolt = <800000>; regulator-max-microvolt = <1160000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-cpu"; }; @@ -270,7 +269,6 @@ reg_dcdcc: dcdcc { regulator-enable-ramp-delay = <32000>; regulator-min-microvolt = <810000>; regulator-max-microvolt = <1080000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-gpu"; }; diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts index 1ffd68f43f87..6a1ee4232675 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts @@ -245,7 +245,6 @@ reg_dcdca: dcdca { regulator-always-on; regulator-min-microvolt = <810000>; regulator-max-microvolt = <1160000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-cpu"; }; @@ -253,7 +252,6 @@ reg_dcdcc: dcdcc { regulator-enable-ramp-delay = <32000>; regulator-min-microvolt = <810000>; regulator-max-microvolt = <1080000>; - regulator-ramp-delay = <2500>; regulator-name = "vdd-gpu"; };
The regulator-ramp-delay property isn't documented in the binding for the AXP806, and it's ignored by the driver. Remove those properties. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 -- arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts | 2 -- arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts | 2 -- 3 files changed, 6 deletions(-)